diff mbox series

[2/2] media: i2c: ov5647: Use bus-locked i2c_transfer()

Message ID 20230220124101.1010317-1-jacopo.mondi@ideasonboard.com
State New
Headers show
Series media: i2c: ov5647: Add test pattern support | expand

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

Tommaso Merciai Feb. 20, 2023, 2:43 p.m. UTC | #1 
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
Dave Stevenson Feb. 20, 2023, 5:46 p.m. UTC | #2 
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
>
Jacopo Mondi Feb. 21, 2023, 8:18 a.m. UTC | #3 
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
> >
diff mbox series

Patch

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;
 }