mbox series

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

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

Message

Jacopo Mondi Feb. 19, 2023, 6:03 p.m. UTC 
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

Comments

Tommaso Merciai Feb. 20, 2023, 2:42 p.m. UTC | #1 
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
Dave Stevenson Feb. 20, 2023, 6:18 p.m. UTC | #2 
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
>