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@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");
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@xxxxxxxxxxxxxxxx>
> }
>
> *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;
> > > }
--
Regards,
Laurent Pinchart
