[PATCH 0/2] media: i2c: ov5647: Add test pattern support

[PATCH 0/2] media: i2c: ov5647: Add test pattern support

[Jacopo Mondi]

A small series that upports commit 7da051cfc67 ("media: i2c: ov5647: Add test
pattern control") from the Renesas 6.1 BSP.

While at there, add small patch to make the i2c read transactions safer by
ensuring the bus is locked.

Jacopo Mondi (1):
  media: i2c: ov5647: Use bus-locked i2c_transfer()

Valentine Barshak (1):
  media: i2c: ov5647: Add test pattern control

 drivers/media/i2c/ov5647.c | 52 ++++++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 10 deletions(-)

--
2.39.0

[PATCH 1/2] media: i2c: ov5647: Add test pattern control

[Jacopo Mondi]

From: Valentine Barshak <valentine.barshak@cogentembedded.com>

This adds V4L2_CID_TEST_PATTERN control support.

Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/ov5647.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 847a7bbb69c5..0b88ac6dee41 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -58,6 +58,7 @@
 #define OV5647_REG_MIPI_CTRL00		0x4800
 #define OV5647_REG_MIPI_CTRL14		0x4814
 #define OV5647_REG_AWB			0x5001
+#define OV5647_REG_ISPCTRL3D		0x503d

 #define REG_TERM 0xfffe
 #define VAL_TERM 0xfe
@@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov5647, sd);
 }

+static const char * const ov5647_test_pattern_menu[] = {
+	"Disabled",
+	"Color Bars",
+	"Color Squares",
+	"Random Data",
+	"Input Data"
+};
+
+static u8 ov5647_test_pattern_val[] = {
+	0x00,	/* Disabled */
+	0x80,	/* Color Bars */
+	0x82,	/* Color Squares */
+	0x81,	/* Random Data */
+	0x83,	/* Input Data */
+};
+
 static const struct regval_list sensor_oe_disable_regs[] = {
 	{0x3000, 0x00},
 	{0x3001, 0x00},
@@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
 				     sensor->mode->format.height + ctrl->val);
 		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
+				   ov5647_test_pattern_val[ctrl->val]);
+		break;

 	/* Read-only, but we adjust it based on mode. */
 	case V4L2_CID_PIXEL_RATE:
@@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
 	int hblank, exposure_max, exposure_def;

-	v4l2_ctrl_handler_init(&sensor->ctrls, 8);
+	v4l2_ctrl_handler_init(&sensor->ctrls, 9);

 	v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
 			  V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
@@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor)
 					   sensor->mode->vts -
 					   sensor->mode->format.height);

+	v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
+				     0, 0, ov5647_test_pattern_menu);
+
 	if (sensor->ctrls.error)
 		goto handler_free;

--
2.39.0

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

[Jacopo Mondi]

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

Re: [PATCH 1/2] media: i2c: ov5647: Add test pattern control

[Tommaso Merciai]

Hi Jacopo,

On Mon, Feb 20, 2023 at 01:40:41PM +0100, Jacopo Mondi wrote:
> From: Valentine Barshak <valentine.barshak@cogentembedded.com>
> 
> This adds V4L2_CID_TEST_PATTERN control support.
> 
> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 847a7bbb69c5..0b88ac6dee41 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -58,6 +58,7 @@
>  #define OV5647_REG_MIPI_CTRL00		0x4800
>  #define OV5647_REG_MIPI_CTRL14		0x4814
>  #define OV5647_REG_AWB			0x5001
> +#define OV5647_REG_ISPCTRL3D		0x503d
> 
>  #define REG_TERM 0xfffe
>  #define VAL_TERM 0xfe
> @@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
>  	return container_of(sd, struct ov5647, sd);
>  }
> 
> +static const char * const ov5647_test_pattern_menu[] = {
> +	"Disabled",
> +	"Color Bars",
> +	"Color Squares",
> +	"Random Data",
> +	"Input Data"
> +};
> +
> +static u8 ov5647_test_pattern_val[] = {
> +	0x00,	/* Disabled */
> +	0x80,	/* Color Bars */
> +	0x82,	/* Color Squares */
> +	0x81,	/* Random Data */
> +	0x83,	/* Input Data */
> +};
> +
>  static const struct regval_list sensor_oe_disable_regs[] = {
>  	{0x3000, 0x00},
>  	{0x3001, 0x00},
> @@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
>  				     sensor->mode->format.height + ctrl->val);
>  		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
> +				   ov5647_test_pattern_val[ctrl->val]);
> +		break;
> 
>  	/* Read-only, but we adjust it based on mode. */
>  	case V4L2_CID_PIXEL_RATE:
> @@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)
>  	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>  	int hblank, exposure_max, exposure_def;
> 
> -	v4l2_ctrl_handler_init(&sensor->ctrls, 8);
> +	v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> 
>  	v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
>  			  V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> @@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor)
>  					   sensor->mode->vts -
>  					   sensor->mode->format.height);
> 
> +	v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> +				     0, 0, ov5647_test_pattern_menu);
> +
>  	if (sensor->ctrls.error)
>  		goto handler_free;
> 
> --
> 2.39.0
> 

Looks good to me.
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>

Thanks,
Tommaso

Tommaso

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

[Tommaso Merciai]

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

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

[Dave Stevenson]

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
>

Re: [PATCH 1/2] media: i2c: ov5647: Add test pattern control

[Dave Stevenson]

Hi Jacopo

On Mon, 20 Feb 2023 at 12:40, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> From: Valentine Barshak <valentine.barshak@cogentembedded.com>
>
> This adds V4L2_CID_TEST_PATTERN control support.
>
> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/ov5647.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 847a7bbb69c5..0b88ac6dee41 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -58,6 +58,7 @@
>  #define OV5647_REG_MIPI_CTRL00         0x4800
>  #define OV5647_REG_MIPI_CTRL14         0x4814
>  #define OV5647_REG_AWB                 0x5001
> +#define OV5647_REG_ISPCTRL3D           0x503d
>
>  #define REG_TERM 0xfffe
>  #define VAL_TERM 0xfe
> @@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
>         return container_of(sd, struct ov5647, sd);
>  }
>
> +static const char * const ov5647_test_pattern_menu[] = {
> +       "Disabled",
> +       "Color Bars",
> +       "Color Squares",
> +       "Random Data",
> +       "Input Data"

"Input Data" appears to give me just a black image. Have I missed
something? What's it meant to be the input to?
Is it worth adding 0x92 for a black and white checkboard as well?

Whichever way:

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Just as a note, the test patterns appear to be valid only if 0x3820
bit 1 = 0 and 0x3821 bit 1 = 1 (V & H flips respectively).
The sensor appears to be assuming one particular colour pattern (BGGR)
when producing a test pattern, so flips altering the format give some
very weird effects. I do have patches that add the V4L2 flip controls,
so those expose some interesting effects :-/

  Dave

> +};
> +
> +static u8 ov5647_test_pattern_val[] = {
> +       0x00,   /* Disabled */
> +       0x80,   /* Color Bars */
> +       0x82,   /* Color Squares */
> +       0x81,   /* Random Data */
> +       0x83,   /* Input Data */
> +};
> +
>  static const struct regval_list sensor_oe_disable_regs[] = {
>         {0x3000, 0x00},
>         {0x3001, 0x00},
> @@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>                 ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
>                                      sensor->mode->format.height + ctrl->val);
>                 break;
> +       case V4L2_CID_TEST_PATTERN:
> +               ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
> +                                  ov5647_test_pattern_val[ctrl->val]);
> +               break;
>
>         /* Read-only, but we adjust it based on mode. */
>         case V4L2_CID_PIXEL_RATE:
> @@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)
>         struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>         int hblank, exposure_max, exposure_def;
>
> -       v4l2_ctrl_handler_init(&sensor->ctrls, 8);
> +       v4l2_ctrl_handler_init(&sensor->ctrls, 9);
>
>         v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
>                           V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> @@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor)
>                                            sensor->mode->vts -
>                                            sensor->mode->format.height);
>
> +       v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops,
> +                                    V4L2_CID_TEST_PATTERN,
> +                                    ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> +                                    0, 0, ov5647_test_pattern_menu);
> +
>         if (sensor->ctrls.error)
>                 goto handler_free;
>
> --
> 2.39.0
>

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

[Jacopo Mondi]

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

Re: [PATCH 1/2] media: i2c: ov5647: Add test pattern control

[Jacopo Mondi]

Hi Dave

On Mon, Feb 20, 2023 at 06:18:14PM +0000, Dave Stevenson wrote:
> Hi Jacopo
>
> On Mon, 20 Feb 2023 at 12:40, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > From: Valentine Barshak <valentine.barshak@cogentembedded.com>
> >
> > This adds V4L2_CID_TEST_PATTERN control support.
> >
> > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov5647.c | 28 +++++++++++++++++++++++++++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index 847a7bbb69c5..0b88ac6dee41 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -58,6 +58,7 @@
> >  #define OV5647_REG_MIPI_CTRL00         0x4800
> >  #define OV5647_REG_MIPI_CTRL14         0x4814
> >  #define OV5647_REG_AWB                 0x5001
> > +#define OV5647_REG_ISPCTRL3D           0x503d
> >
> >  #define REG_TERM 0xfffe
> >  #define VAL_TERM 0xfe
> > @@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> >         return container_of(sd, struct ov5647, sd);
> >  }
> >
> > +static const char * const ov5647_test_pattern_menu[] = {
> > +       "Disabled",
> > +       "Color Bars",
> > +       "Color Squares",
> > +       "Random Data",
> > +       "Input Data"
>
> "Input Data" appears to give me just a black image. Have I missed
> something? What's it meant to be the input to?

I noticed as well, but not knowing what "input data" meant I just
"meh" and moved on

Should it be dropped in your opinion ?

> Is it worth adding 0x92 for a black and white checkboard as well?

I could

>
> Whichever way:
>
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> Just as a note, the test patterns appear to be valid only if 0x3820
> bit 1 = 0 and 0x3821 bit 1 = 1 (V & H flips respectively).

Unrelated: why do I see the 2592x1944 mode with {0x3821, 0x06} ?
I only tested whatever qcam gave me, I should better re-check

> The sensor appears to be assuming one particular colour pattern (BGGR)
> when producing a test pattern, so flips altering the format give some
> very weird effects. I do have patches that add the V4L2 flip controls,

Ah, that surprises me, I recall we discussed in the past the fact that
flip shouldn't alter test patterns...

> so those expose some interesting effects :-/
>
>   Dave
>
> > +};
> > +
> > +static u8 ov5647_test_pattern_val[] = {
> > +       0x00,   /* Disabled */
> > +       0x80,   /* Color Bars */
> > +       0x82,   /* Color Squares */
> > +       0x81,   /* Random Data */
> > +       0x83,   /* Input Data */
> > +};
> > +
> >  static const struct regval_list sensor_oe_disable_regs[] = {
> >         {0x3000, 0x00},
> >         {0x3001, 0x00},
> > @@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> >                 ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
> >                                      sensor->mode->format.height + ctrl->val);
> >                 break;
> > +       case V4L2_CID_TEST_PATTERN:
> > +               ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
> > +                                  ov5647_test_pattern_val[ctrl->val]);
> > +               break;
> >
> >         /* Read-only, but we adjust it based on mode. */
> >         case V4L2_CID_PIXEL_RATE:
> > @@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)
> >         struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
> >         int hblank, exposure_max, exposure_def;
> >
> > -       v4l2_ctrl_handler_init(&sensor->ctrls, 8);
> > +       v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> >
> >         v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> >                           V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> > @@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor)
> >                                            sensor->mode->vts -
> >                                            sensor->mode->format.height);
> >
> > +       v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops,
> > +                                    V4L2_CID_TEST_PATTERN,
> > +                                    ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> > +                                    0, 0, ov5647_test_pattern_menu);
> > +
> >         if (sensor->ctrls.error)
> >                 goto handler_free;
> >
> > --
> > 2.39.0
> >

Re: [PATCH 1/2] media: i2c: ov5647: Add test pattern control

[Dave Stevenson]

Hi Jacopo

On Tue, 21 Feb 2023 at 08:25, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Dave
>
> On Mon, Feb 20, 2023 at 06:18:14PM +0000, Dave Stevenson wrote:
> > Hi Jacopo
> >
> > On Mon, 20 Feb 2023 at 12:40, Jacopo Mondi
> > <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > From: Valentine Barshak <valentine.barshak@cogentembedded.com>
> > >
> > > This adds V4L2_CID_TEST_PATTERN control support.
> > >
> > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/ov5647.c | 28 +++++++++++++++++++++++++++-
> > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > index 847a7bbb69c5..0b88ac6dee41 100644
> > > --- a/drivers/media/i2c/ov5647.c
> > > +++ b/drivers/media/i2c/ov5647.c
> > > @@ -58,6 +58,7 @@
> > >  #define OV5647_REG_MIPI_CTRL00         0x4800
> > >  #define OV5647_REG_MIPI_CTRL14         0x4814
> > >  #define OV5647_REG_AWB                 0x5001
> > > +#define OV5647_REG_ISPCTRL3D           0x503d
> > >
> > >  #define REG_TERM 0xfffe
> > >  #define VAL_TERM 0xfe
> > > @@ -116,6 +117,22 @@ static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > >         return container_of(sd, struct ov5647, sd);
> > >  }
> > >
> > > +static const char * const ov5647_test_pattern_menu[] = {
> > > +       "Disabled",
> > > +       "Color Bars",
> > > +       "Color Squares",
> > > +       "Random Data",
> > > +       "Input Data"
> >
> > "Input Data" appears to give me just a black image. Have I missed
> > something? What's it meant to be the input to?
>
> I noticed as well, but not knowing what "input data" meant I just
> "meh" and moved on
>
> Should it be dropped in your opinion ?

Personally I would drop it as it is undefined what that really means.

> > Is it worth adding 0x92 for a black and white checkboard as well?
>
> I could

I'll leave that one up to you. I have no major interest in test patterns.

> >
> > Whichever way:
> >
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > Just as a note, the test patterns appear to be valid only if 0x3820
> > bit 1 = 0 and 0x3821 bit 1 = 1 (V & H flips respectively).
>
> Unrelated: why do I see the 2592x1944 mode with {0x3821, 0x06} ?
> I only tested whatever qcam gave me, I should better re-check

The sensor appears to have an inherent H-flip, so for "normal" readout
you need to set 0x3821 bit 1 as you see in the driver.
I don't remember if I've ever really experimented with the difference
between r_mirror_isp (bit 2) and r_mirror_snr (bit 1)

> > The sensor appears to be assuming one particular colour pattern (BGGR)
> > when producing a test pattern, so flips altering the format give some
> > very weird effects. I do have patches that add the V4L2 flip controls,
>
> Ah, that surprises me, I recall we discussed in the past the fact that
> flip shouldn't alter test patterns...

I don't recall the exact discussion. Was it around a sensor which
deliberately changed the flip register as it enabled the pattern?
In my experience it's not uncommon that they are affected by flips.

Flips are implemented by changing the readout order on the sensor
array. The sensor itself doesn't care that doing so changes the Bayer
order as it knows nothing about the colour filter or even if it is
present.
Test patterns are generally implemented further down the pipeline by
forcing the pixel state of each pixel in turn, ignoring flips. If the
receiver pipeline is working on the basis that it knows the flips and
how that would normally affect the Bayer order, then you get wrong
colours.

V4L2_CID_TEST_PATTERN_[RED|GREENR|GREENB|BLUE] exist, and I see they
have been implemented for CCS and imx219 (IIRC done by others rather
than myself, but in the original upstreamed version of the driver).
Testing with imx219 confirms that it also has the same issue - colours
are only correct if hflip = 0 and vflip = 0. V4L2_CID_TEST_PATTERN
ought to set MODIFY_LAYOUT, override the Bayer order, and be grabbed
at start/stop streaming.
Of course this gets even more confusing when device tree can advertise
a mounting rotation though V4L2_CID_CAMERA_SENSOR_ROTATION, so
libcamera may enable flips automatically when told to stream in the
native orientation, and then the test patterns are unexpectedly wrong
as the user didn't ask for flips.
It's one of the reasons I generally don't care about test patterns -
they're generally of little use other than to prove the sensor
streams, so how much effort is it worth expending in making them work
perfectly under all conditions?

  Dave

> > so those expose some interesting effects :-/
> >
> >   Dave
> >
> > > +};
> > > +
> > > +static u8 ov5647_test_pattern_val[] = {
> > > +       0x00,   /* Disabled */
> > > +       0x80,   /* Color Bars */
> > > +       0x82,   /* Color Squares */
> > > +       0x81,   /* Random Data */
> > > +       0x83,   /* Input Data */
> > > +};
> > > +
> > >  static const struct regval_list sensor_oe_disable_regs[] = {
> > >         {0x3000, 0x00},
> > >         {0x3001, 0x00},
> > > @@ -1242,6 +1259,10 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> > >                 ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
> > >                                      sensor->mode->format.height + ctrl->val);
> > >                 break;
> > > +       case V4L2_CID_TEST_PATTERN:
> > > +               ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
> > > +                                  ov5647_test_pattern_val[ctrl->val]);
> > > +               break;
> > >
> > >         /* Read-only, but we adjust it based on mode. */
> > >         case V4L2_CID_PIXEL_RATE:
> > > @@ -1270,7 +1291,7 @@ static int ov5647_init_controls(struct ov5647 *sensor)
> > >         struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
> > >         int hblank, exposure_max, exposure_def;
> > >
> > > -       v4l2_ctrl_handler_init(&sensor->ctrls, 8);
> > > +       v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> > >
> > >         v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > >                           V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> > > @@ -1314,6 +1335,11 @@ static int ov5647_init_controls(struct ov5647 *sensor)
> > >                                            sensor->mode->vts -
> > >                                            sensor->mode->format.height);
> > >
> > > +       v4l2_ctrl_new_std_menu_items(&sensor->ctrls, &ov5647_ctrl_ops,
> > > +                                    V4L2_CID_TEST_PATTERN,
> > > +                                    ARRAY_SIZE(ov5647_test_pattern_menu) - 1,
> > > +                                    0, 0, ov5647_test_pattern_menu);
> > > +
> > >         if (sensor->ctrls.error)
> > >                 goto handler_free;
> > >
> > > --
> > > 2.39.0
> > >

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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart