Message ID | 20230220124101.1010317-1-jacopo.mondi@ideasonboard.com (mailing list archive) |
---|---|
State | Accepted |
Delegated to: | Sakari Ailus |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1pU5Ti-005kay-7H; Mon, 20 Feb 2023 12:41:10 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231817AbjBTMlJ (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 20 Feb 2023 07:41:09 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59582 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231857AbjBTMlI (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 20 Feb 2023 07:41:08 -0500 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7EEDC1B561 for <linux-media@vger.kernel.org>; Mon, 20 Feb 2023 04:41:07 -0800 (PST) Received: from uno.homenet.telecomitalia.it (host-95-252-227-22.retail.telecomitalia.it [95.252.227.22]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 8653AA49; Mon, 20 Feb 2023 13:41:05 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1676896866; bh=rujmqGnJ/oR37BjpbMq5yAQrfqhHliE+cxkopZk4rrk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=OIvDQhJVXSdT8rp1OF6Zf8A0IKyYrUwvKQ/Y4rCYbcx2+tEA22Oy14zuAvoe/k2hz 1j5SBf7NRoldrDRC87tbVRmPC+FK6gmSH8OCDQXRkJ994jWgFwL5Hhk3+UdG6CmExo FzCxeVEZWsBwUjQCre3bmbymDLX4jUzuI2OyZzMk= From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> To: Mikhail Rudenko <mike.rudenko@gmail.com>, Dave Stevenson <dave.stevenson@raspberrypi.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com>, linux-media@vger.kernel.org Subject: [PATCH 2/2] media: i2c: ov5647: Use bus-locked i2c_transfer() Date: Mon, 20 Feb 2023 13:41:01 +0100 Message-Id: <20230220124101.1010317-1-jacopo.mondi@ideasonboard.com> X-Mailer: git-send-email 2.39.0 In-Reply-To: <20230219180334.980950-1-jacopo.mondi@ideasonboard.com> References: <20230219180334.980950-1-jacopo.mondi@ideasonboard.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_PASS,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
media: i2c: ov5647: Add test pattern support
|
|
Commit Message
Jacopo Mondi
Feb. 20, 2023, 12:41 p.m. UTC
The ov5647_read() functions calls i2c_master_send() and
i2c_master_read() in sequence. However this leaves space for other
clients to contend the bus and insert a unrelated transaction in between
the two calls.
Replace the two calls with a single i2c_transfer() one, that locks the
bus in between the transactions.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
drivers/media/i2c/ov5647.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
--
2.39.0
Comments
Hi Jacopo, On Mon, Feb 20, 2023 at 01:41:01PM +0100, Jacopo Mondi wrote: > The ov5647_read() functions calls i2c_master_send() and > i2c_master_read() in sequence. However this leaves space for other > clients to contend the bus and insert a unrelated transaction in between > the two calls. > > Replace the two calls with a single i2c_transfer() one, that locks the > bus in between the transactions. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/i2c/ov5647.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > index 0b88ac6dee41..a423ee8fe20c 100644 > --- a/drivers/media/i2c/ov5647.c > +++ b/drivers/media/i2c/ov5647.c > @@ -631,23 +631,29 @@ static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val) > > static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val) > { > - unsigned char data_w[2] = { reg >> 8, reg & 0xff }; > struct i2c_client *client = v4l2_get_subdevdata(sd); > + u8 buf[2] = { reg >> 8, reg & 0xff }; > + struct i2c_msg msg[2]; > int ret; > > - ret = i2c_master_send(client, data_w, 2); > + msg[0].addr = client->addr; > + msg[0].flags = client->flags; > + msg[0].buf = buf; > + msg[0].len = sizeof(buf); > + > + msg[1].addr = client->addr; > + msg[1].flags = client->flags | I2C_M_RD; > + msg[1].buf = buf; > + msg[1].len = 1; > + > + ret = i2c_transfer(client->adapter, msg, 2); > if (ret < 0) { > - dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n", > + dev_err(&client->dev, "%s: i2c read error, reg: %x\n", > __func__, reg); > return ret; > } > > - ret = i2c_master_recv(client, val, 1); > - if (ret < 0) { > - dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n", > - __func__, reg); > - return ret; > - } > + *val = buf[0]; > > return 0; > } > -- > 2.39.0 > Fully agree. Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com> Thanks, Tommaso
Hi Jacopo On Mon, 20 Feb 2023 at 12:41, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > The ov5647_read() functions calls i2c_master_send() and > i2c_master_read() in sequence. However this leaves space for other > clients to contend the bus and insert a unrelated transaction in between > the two calls. > > Replace the two calls with a single i2c_transfer() one, that locks the > bus in between the transactions. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/i2c/ov5647.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > index 0b88ac6dee41..a423ee8fe20c 100644 > --- a/drivers/media/i2c/ov5647.c > +++ b/drivers/media/i2c/ov5647.c > @@ -631,23 +631,29 @@ static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val) > > static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val) > { > - unsigned char data_w[2] = { reg >> 8, reg & 0xff }; > struct i2c_client *client = v4l2_get_subdevdata(sd); > + u8 buf[2] = { reg >> 8, reg & 0xff }; > + struct i2c_msg msg[2]; > int ret; > > - ret = i2c_master_send(client, data_w, 2); > + msg[0].addr = client->addr; > + msg[0].flags = client->flags; > + msg[0].buf = buf; > + msg[0].len = sizeof(buf); > + > + msg[1].addr = client->addr; > + msg[1].flags = client->flags | I2C_M_RD; > + msg[1].buf = buf; > + msg[1].len = 1; > + > + ret = i2c_transfer(client->adapter, msg, 2); > if (ret < 0) { i2c_transfer * Returns negative errno, else the number of messages executed. Is there a valid failure case where it returns 1 having done the write but failed the read? It's deferred to the individual I2C driver, so could quite easily be iffy. Personally I'd be tempted to check if (ret != 2), and remap any other positive value to -EINVAL. Otherwise: Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > - dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n", > + dev_err(&client->dev, "%s: i2c read error, reg: %x\n", > __func__, reg); > return ret; > } > > - ret = i2c_master_recv(client, val, 1); > - if (ret < 0) { > - dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n", > - __func__, reg); > - return ret; > - } > + *val = buf[0]; > > return 0; > } > -- > 2.39.0 >
Hi Dave On Mon, Feb 20, 2023 at 05:46:02PM +0000, Dave Stevenson wrote: > Hi Jacopo > > On Mon, 20 Feb 2023 at 12:41, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > The ov5647_read() functions calls i2c_master_send() and > > i2c_master_read() in sequence. However this leaves space for other > > clients to contend the bus and insert a unrelated transaction in between > > the two calls. > > > > Replace the two calls with a single i2c_transfer() one, that locks the > > bus in between the transactions. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > drivers/media/i2c/ov5647.c | 24 +++++++++++++++--------- > > 1 file changed, 15 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > > index 0b88ac6dee41..a423ee8fe20c 100644 > > --- a/drivers/media/i2c/ov5647.c > > +++ b/drivers/media/i2c/ov5647.c > > @@ -631,23 +631,29 @@ static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val) > > > > static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val) > > { > > - unsigned char data_w[2] = { reg >> 8, reg & 0xff }; > > struct i2c_client *client = v4l2_get_subdevdata(sd); > > + u8 buf[2] = { reg >> 8, reg & 0xff }; > > + struct i2c_msg msg[2]; > > int ret; > > > > - ret = i2c_master_send(client, data_w, 2); > > + msg[0].addr = client->addr; > > + msg[0].flags = client->flags; > > + msg[0].buf = buf; > > + msg[0].len = sizeof(buf); > > + > > + msg[1].addr = client->addr; > > + msg[1].flags = client->flags | I2C_M_RD; > > + msg[1].buf = buf; > > + msg[1].len = 1; > > + > > + ret = i2c_transfer(client->adapter, msg, 2); > > if (ret < 0) { > > i2c_transfer > * Returns negative errno, else the number of messages executed. > > Is there a valid failure case where it returns 1 having done the write > but failed the read? It's deferred to the individual I2C driver, so > could quite easily be iffy. > Personally I'd be tempted to check if (ret != 2), and remap any other > positive value to -EINVAL. Seems indeed up to the individual drivers implementation of master_xfer, whose return value is documented as: include/linux/i2c.h: * master_xfer should return the number of messages successfully include/linux/i2c.h- * processed, or a negative value on error I can indeed: if (ret != 2) { dev_err(.., "i2c write error, reg: %x\n"); return ret > 0 ? -EINVAL : ret; } *val = buf[0]; return 0; > > Otherwise: > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Thanks for checking > > > - dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n", > > + dev_err(&client->dev, "%s: i2c read error, reg: %x\n", > > __func__, reg); > > return ret; > > } > > > > - ret = i2c_master_recv(client, val, 1); > > - if (ret < 0) { > > - dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n", > > - __func__, reg); > > - return ret; > > - } > > + *val = buf[0]; > > > > return 0; > > } > > -- > > 2.39.0 > >
Hi Jacopo On Tue, Feb 21, 2023 at 09:18:51AM +0100, Jacopo Mondi wrote: > On Mon, Feb 20, 2023 at 05:46:02PM +0000, Dave Stevenson wrote: > > On Mon, 20 Feb 2023 at 12:41, Jacopo Mondi wrote: > > > > > > The ov5647_read() functions calls i2c_master_send() and > > > i2c_master_read() in sequence. However this leaves space for other > > > clients to contend the bus and insert a unrelated transaction in between s/a unrelated/an unrelated/ > > > the two calls. > > > > > > Replace the two calls with a single i2c_transfer() one, that locks the > > > bus in between the transactions. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > drivers/media/i2c/ov5647.c | 24 +++++++++++++++--------- > > > 1 file changed, 15 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > > > index 0b88ac6dee41..a423ee8fe20c 100644 > > > --- a/drivers/media/i2c/ov5647.c > > > +++ b/drivers/media/i2c/ov5647.c > > > @@ -631,23 +631,29 @@ static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val) > > > > > > static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val) > > > { > > > - unsigned char data_w[2] = { reg >> 8, reg & 0xff }; > > > struct i2c_client *client = v4l2_get_subdevdata(sd); > > > + u8 buf[2] = { reg >> 8, reg & 0xff }; > > > + struct i2c_msg msg[2]; > > > int ret; > > > > > > - ret = i2c_master_send(client, data_w, 2); > > > + msg[0].addr = client->addr; > > > + msg[0].flags = client->flags; > > > + msg[0].buf = buf; > > > + msg[0].len = sizeof(buf); > > > + > > > + msg[1].addr = client->addr; > > > + msg[1].flags = client->flags | I2C_M_RD; > > > + msg[1].buf = buf; > > > + msg[1].len = 1; > > > + > > > + ret = i2c_transfer(client->adapter, msg, 2); > > > if (ret < 0) { > > > > i2c_transfer > > * Returns negative errno, else the number of messages executed. > > > > Is there a valid failure case where it returns 1 having done the write > > but failed the read? It's deferred to the individual I2C driver, so > > could quite easily be iffy. > > Personally I'd be tempted to check if (ret != 2), and remap any other > > positive value to -EINVAL. > > Seems indeed up to the individual drivers implementation of > master_xfer, whose return value is documented as: > > include/linux/i2c.h: * master_xfer should return the number of messages successfully > include/linux/i2c.h- * processed, or a negative value on error > > I can indeed: > > if (ret != 2) { > dev_err(.., "i2c write error, reg: %x\n"); I would also print the ret value, that's useful. > return ret > 0 ? -EINVAL : ret; Make it >= just to be safe. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > > *val = buf[0]; > > return 0; > > > Otherwise: > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Thanks for checking > > > > - dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n", > > > + dev_err(&client->dev, "%s: i2c read error, reg: %x\n", > > > __func__, reg); > > > return ret; > > > } > > > > > > - ret = i2c_master_recv(client, val, 1); > > > - if (ret < 0) { > > > - dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n", > > > - __func__, reg); > > > - return ret; > > > - } > > > + *val = buf[0]; > > > > > > return 0; > > > }
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c index 0b88ac6dee41..a423ee8fe20c 100644 --- a/drivers/media/i2c/ov5647.c +++ b/drivers/media/i2c/ov5647.c @@ -631,23 +631,29 @@ static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val) static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val) { - unsigned char data_w[2] = { reg >> 8, reg & 0xff }; struct i2c_client *client = v4l2_get_subdevdata(sd); + u8 buf[2] = { reg >> 8, reg & 0xff }; + struct i2c_msg msg[2]; int ret; - ret = i2c_master_send(client, data_w, 2); + msg[0].addr = client->addr; + msg[0].flags = client->flags; + msg[0].buf = buf; + msg[0].len = sizeof(buf); + + msg[1].addr = client->addr; + msg[1].flags = client->flags | I2C_M_RD; + msg[1].buf = buf; + msg[1].len = 1; + + ret = i2c_transfer(client->adapter, msg, 2); if (ret < 0) { - dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n", + dev_err(&client->dev, "%s: i2c read error, reg: %x\n", __func__, reg); return ret; } - ret = i2c_master_recv(client, val, 1); - if (ret < 0) { - dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n", - __func__, reg); - return ret; - } + *val = buf[0]; return 0; }