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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx>
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
> >
