[PATCH 00/16] Updates to ov9282 sensor driver

[PATCH 00/16] Updates to ov9282 sensor driver

[Dave Stevenson]

This series adds to the functionality of the Ominvision
OV9282 driver to make it usable with libcamera.

Tested on a Raspberry Pi with OV9281 sensor (same as
OV9282 but with alternate CRA on the optics)

Dave Stevenson (16):
  media: i2c: ov9282: Remove duplication of registers
  media: i2c: ov9282: Split registers into common and mode specific
  media: i2c: ov9282: Remove format code from the mode
  media: i2c: ov9282: Remove pixel rate from mode definition
  media: i2c: ov9281: Support more than 1 mode.
  media: i2c: ov9282: Correct HTS register for configured pixel rate
  media: i2c: ov9282: Reduce vblank_min values based on testing
  media: i2c: ov9282: Add selection for CSI2 clock mode
  media: i2c: ov9282: Add the properties from fwnode
  media: i2c: ov9282: Action CID_VBLANK when set.
  media: i2c: ov9282: Add HFLIP and VFLIP support
  media: i2c: ov9282: Make V4L2_CID_HBLANK r/w
  media: i2c: ov9282: Add selection API calls for cropping info
  media: i2c: ov9282: Add support for 1280x800 and 640x400 modes
  media: i2c: ov9282: Add support for 8bit readout
  media: i2c: ov9282: Support event handlers

 drivers/media/i2c/ov9282.c | 552 ++++++++++++++++++++++++++++++-------
 1 file changed, 446 insertions(+), 106 deletions(-)

-- 
2.34.1

[PATCH 01/16] media: i2c: ov9282: Remove duplication of registers

[Dave Stevenson]

TIMING_VTS (registers 0x380e/f), EXPOSURE (registers
0x3500/1/2), and GAIN (0x3509) are all set from
ov9282_update_exp_gain as part of the control handler,
therefore they do not need to be in the main list of
registers.

Remove them.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 699fc5b753b4..2c13bcd59c2a 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -173,14 +173,10 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3030, 0x10},
 	{0x3039, 0x32},
 	{0x303a, 0x00},
-	{0x3500, 0x00},
-	{0x3501, 0x5f},
-	{0x3502, 0x1e},
 	{0x3503, 0x08},
 	{0x3505, 0x8c},
 	{0x3507, 0x03},
 	{0x3508, 0x00},
-	{0x3509, 0x10},
 	{0x3610, 0x80},
 	{0x3611, 0xa0},
 	{0x3620, 0x6e},
@@ -214,8 +210,6 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x380b, 0xd0},
 	{0x380c, 0x05},
 	{0x380d, 0xfa},
-	{0x380e, 0x06},
-	{0x380f, 0xce},
 	{0x3810, 0x00},
 	{0x3811, 0x08},
 	{0x3812, 0x00},
-- 
2.34.1

Re: [PATCH 01/16] media: i2c: ov9282: Remove duplication of registers

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:27:54PM +0100, Dave Stevenson wrote:
> TIMING_VTS (registers 0x380e/f), EXPOSURE (registers
> 0x3500/1/2), and GAIN (0x3509) are all set from
> ov9282_update_exp_gain as part of the control handler,
> therefore they do not need to be in the main list of
> registers.
>
> Remove them.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

I don't have the datashee but I can confirm the removed registers are
programmed when setting up controls

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  drivers/media/i2c/ov9282.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 699fc5b753b4..2c13bcd59c2a 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -173,14 +173,10 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x3030, 0x10},
>  	{0x3039, 0x32},
>  	{0x303a, 0x00},
> -	{0x3500, 0x00},
> -	{0x3501, 0x5f},
> -	{0x3502, 0x1e},
>  	{0x3503, 0x08},
>  	{0x3505, 0x8c},
>  	{0x3507, 0x03},
>  	{0x3508, 0x00},
> -	{0x3509, 0x10},
>  	{0x3610, 0x80},
>  	{0x3611, 0xa0},
>  	{0x3620, 0x6e},
> @@ -214,8 +210,6 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x380b, 0xd0},
>  	{0x380c, 0x05},
>  	{0x380d, 0xfa},
> -	{0x380e, 0x06},
> -	{0x380f, 0xce},
>  	{0x3810, 0x00},
>  	{0x3811, 0x08},
>  	{0x3812, 0x00},
> --
> 2.34.1
>

[PATCH 02/16] media: i2c: ov9282: Split registers into common and mode specific

[Dave Stevenson]

Currently only one mode is supported, so all registers were
dropped in one list.
In preparation for adding more modes, split out the common registers
from those which configure the mode.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 77 +++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 2c13bcd59c2a..9842080cf66f 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -157,8 +157,8 @@ static const s64 link_freq[] = {
 	OV9282_LINK_FREQ,
 };
 
-/* Sensor mode registers */
-static const struct ov9282_reg mode_1280x720_regs[] = {
+/* Common registers */
+static const struct ov9282_reg common_regs[] = {
 	{0x0302, 0x32},
 	{0x030d, 0x50},
 	{0x030e, 0x02},
@@ -189,13 +189,49 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x372d, 0x22},
 	{0x3731, 0x80},
 	{0x3732, 0x30},
-	{0x3778, 0x00},
 	{0x377d, 0x22},
 	{0x3788, 0x02},
 	{0x3789, 0xa4},
 	{0x378a, 0x00},
 	{0x378b, 0x4a},
 	{0x3799, 0x20},
+	{0x3881, 0x42},
+	{0x38a8, 0x02},
+	{0x38a9, 0x80},
+	{0x38b1, 0x00},
+	{0x38c4, 0x00},
+	{0x38c5, 0xc0},
+	{0x38c6, 0x04},
+	{0x38c7, 0x80},
+	{0x3920, 0xff},
+	{0x4010, 0x40},
+	{0x4043, 0x40},
+	{0x4307, 0x30},
+	{0x4317, 0x00},
+	{0x4501, 0x00},
+	{0x450a, 0x08},
+	{0x4601, 0x04},
+	{0x470f, 0x00},
+	{0x4f07, 0x00},
+	{0x4800, 0x20},
+	{0x5000, 0x9f},
+	{0x5001, 0x00},
+	{0x5e00, 0x00},
+	{0x5d00, 0x07},
+	{0x5d01, 0x00},
+	{0x0101, 0x01},
+	{0x1000, 0x03},
+	{0x5a08, 0x84},
+};
+
+struct ov9282_reg_list common_regs_list = {
+	.num_of_regs = ARRAY_SIZE(common_regs),
+	.regs = common_regs,
+};
+
+/* Sensor mode registers */
+static const struct ov9282_reg mode_1280x720_regs[] = {
+	{0x3778, 0x00},
 	{0x3800, 0x00},
 	{0x3801, 0x00},
 	{0x3802, 0x00},
@@ -218,40 +254,13 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3815, 0x11},
 	{0x3820, 0x3c},
 	{0x3821, 0x84},
-	{0x3881, 0x42},
-	{0x38a8, 0x02},
-	{0x38a9, 0x80},
-	{0x38b1, 0x00},
-	{0x38c4, 0x00},
-	{0x38c5, 0xc0},
-	{0x38c6, 0x04},
-	{0x38c7, 0x80},
-	{0x3920, 0xff},
 	{0x4003, 0x40},
 	{0x4008, 0x02},
 	{0x4009, 0x05},
 	{0x400c, 0x00},
 	{0x400d, 0x03},
-	{0x4010, 0x40},
-	{0x4043, 0x40},
-	{0x4307, 0x30},
-	{0x4317, 0x00},
-	{0x4501, 0x00},
 	{0x4507, 0x00},
 	{0x4509, 0x80},
-	{0x450a, 0x08},
-	{0x4601, 0x04},
-	{0x470f, 0x00},
-	{0x4f07, 0x00},
-	{0x4800, 0x20},
-	{0x5000, 0x9f},
-	{0x5001, 0x00},
-	{0x5e00, 0x00},
-	{0x5d00, 0x07},
-	{0x5d01, 0x00},
-	{0x0101, 0x01},
-	{0x1000, 0x03},
-	{0x5a08, 0x84},
 };
 
 /* Supported sensor mode configurations */
@@ -663,6 +672,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
 	const struct ov9282_reg_list *reg_list;
 	int ret;
 
+	/* Write common registers */
+	ret = ov9282_write_regs(ov9282, common_regs_list.regs,
+				common_regs_list.num_of_regs);
+	if (ret) {
+		dev_err(ov9282->dev, "fail to write common registers");
+		return ret;
+	}
+
 	/* Write sensor mode registers */
 	reg_list = &ov9282->cur_mode->reg_list;
 	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
-- 
2.34.1

Re: [PATCH 02/16] media: i2c: ov9282: Split registers into common and mode specific

[Jacopo Mondi]

Hi Dave,

On Wed, Oct 05, 2022 at 04:27:55PM +0100, Dave Stevenson wrote:
> Currently only one mode is supported, so all registers were
> dropped in one list.
> In preparation for adding more modes, split out the common registers
> from those which configure the mode.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

I don't have the datasheet but I can confirm all registeres removed
from the common part are now part of the mode

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> ---
>  drivers/media/i2c/ov9282.c | 77 +++++++++++++++++++++++---------------
>  1 file changed, 47 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 2c13bcd59c2a..9842080cf66f 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -157,8 +157,8 @@ static const s64 link_freq[] = {
>  	OV9282_LINK_FREQ,
>  };
>
> -/* Sensor mode registers */
> -static const struct ov9282_reg mode_1280x720_regs[] = {
> +/* Common registers */
> +static const struct ov9282_reg common_regs[] = {
>  	{0x0302, 0x32},
>  	{0x030d, 0x50},
>  	{0x030e, 0x02},
> @@ -189,13 +189,49 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x372d, 0x22},
>  	{0x3731, 0x80},
>  	{0x3732, 0x30},
> -	{0x3778, 0x00},
>  	{0x377d, 0x22},
>  	{0x3788, 0x02},
>  	{0x3789, 0xa4},
>  	{0x378a, 0x00},
>  	{0x378b, 0x4a},
>  	{0x3799, 0x20},
> +	{0x3881, 0x42},
> +	{0x38a8, 0x02},
> +	{0x38a9, 0x80},
> +	{0x38b1, 0x00},
> +	{0x38c4, 0x00},
> +	{0x38c5, 0xc0},
> +	{0x38c6, 0x04},
> +	{0x38c7, 0x80},
> +	{0x3920, 0xff},
> +	{0x4010, 0x40},
> +	{0x4043, 0x40},
> +	{0x4307, 0x30},
> +	{0x4317, 0x00},
> +	{0x4501, 0x00},
> +	{0x450a, 0x08},
> +	{0x4601, 0x04},
> +	{0x470f, 0x00},
> +	{0x4f07, 0x00},
> +	{0x4800, 0x20},
> +	{0x5000, 0x9f},
> +	{0x5001, 0x00},
> +	{0x5e00, 0x00},
> +	{0x5d00, 0x07},
> +	{0x5d01, 0x00},
> +	{0x0101, 0x01},
> +	{0x1000, 0x03},
> +	{0x5a08, 0x84},
> +};
> +
> +struct ov9282_reg_list common_regs_list = {
> +	.num_of_regs = ARRAY_SIZE(common_regs),
> +	.regs = common_regs,
> +};
> +
> +/* Sensor mode registers */
> +static const struct ov9282_reg mode_1280x720_regs[] = {
> +	{0x3778, 0x00},
>  	{0x3800, 0x00},
>  	{0x3801, 0x00},
>  	{0x3802, 0x00},
> @@ -218,40 +254,13 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x3815, 0x11},
>  	{0x3820, 0x3c},
>  	{0x3821, 0x84},
> -	{0x3881, 0x42},
> -	{0x38a8, 0x02},
> -	{0x38a9, 0x80},
> -	{0x38b1, 0x00},
> -	{0x38c4, 0x00},
> -	{0x38c5, 0xc0},
> -	{0x38c6, 0x04},
> -	{0x38c7, 0x80},
> -	{0x3920, 0xff},
>  	{0x4003, 0x40},
>  	{0x4008, 0x02},
>  	{0x4009, 0x05},
>  	{0x400c, 0x00},
>  	{0x400d, 0x03},
> -	{0x4010, 0x40},
> -	{0x4043, 0x40},
> -	{0x4307, 0x30},
> -	{0x4317, 0x00},
> -	{0x4501, 0x00},
>  	{0x4507, 0x00},
>  	{0x4509, 0x80},
> -	{0x450a, 0x08},
> -	{0x4601, 0x04},
> -	{0x470f, 0x00},
> -	{0x4f07, 0x00},
> -	{0x4800, 0x20},
> -	{0x5000, 0x9f},
> -	{0x5001, 0x00},
> -	{0x5e00, 0x00},
> -	{0x5d00, 0x07},
> -	{0x5d01, 0x00},
> -	{0x0101, 0x01},
> -	{0x1000, 0x03},
> -	{0x5a08, 0x84},
>  };
>
>  /* Supported sensor mode configurations */
> @@ -663,6 +672,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
>  	const struct ov9282_reg_list *reg_list;
>  	int ret;
>
> +	/* Write common registers */
> +	ret = ov9282_write_regs(ov9282, common_regs_list.regs,
> +				common_regs_list.num_of_regs);
> +	if (ret) {
> +		dev_err(ov9282->dev, "fail to write common registers");
> +		return ret;
> +	}
> +
>  	/* Write sensor mode registers */
>  	reg_list = &ov9282->cur_mode->reg_list;
>  	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
> --
> 2.34.1
>

[PATCH 03/16] media: i2c: ov9282: Remove format code from the mode

[Dave Stevenson]

The format code is independent of mode, and each mode could
support both Y10 and Y8, so disassociate the code from the
mode.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 9842080cf66f..1c77b77427f0 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -88,7 +88,6 @@ struct ov9282_reg_list {
  * struct ov9282_mode - ov9282 sensor mode structure
  * @width: Frame width
  * @height: Frame height
- * @code: Format code
  * @hblank: Horizontal blanking in lines
  * @vblank: Vertical blanking in lines
  * @vblank_min: Minimum vertical blanking in lines
@@ -100,7 +99,6 @@ struct ov9282_reg_list {
 struct ov9282_mode {
 	u32 width;
 	u32 height;
-	u32 code;
 	u32 hblank;
 	u32 vblank;
 	u32 vblank_min;
@@ -273,7 +271,6 @@ static const struct ov9282_mode supported_mode = {
 	.vblank_max = 51540,
 	.pclk = 160000000,
 	.link_freq_idx = 0,
-	.code = MEDIA_BUS_FMT_Y10_1X10,
 	.reg_list = {
 		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
 		.regs = mode_1280x720_regs,
@@ -523,7 +520,7 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index > 0)
 		return -EINVAL;
 
-	code->code = supported_mode.code;
+	code->code = MEDIA_BUS_FMT_Y10_1X10;
 
 	return 0;
 }
@@ -543,7 +540,7 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
 	if (fsize->index > 0)
 		return -EINVAL;
 
-	if (fsize->code != supported_mode.code)
+	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
 		return -EINVAL;
 
 	fsize->min_width = supported_mode.width;
@@ -567,7 +564,7 @@ static void ov9282_fill_pad_format(struct ov9282 *ov9282,
 {
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
-	fmt->format.code = mode->code;
+	fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
 	fmt->format.field = V4L2_FIELD_NONE;
 	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
 	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
-- 
2.34.1

Re: [PATCH 03/16] media: i2c: ov9282: Remove format code from the mode

[Jacopo Mondi]

Hi Dave,

On Wed, Oct 05, 2022 at 04:27:56PM +0100, Dave Stevenson wrote:
> The format code is independent of mode, and each mode could
> support both Y10 and Y8, so disassociate the code from the
> mode.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Makes sense!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> ---
>  drivers/media/i2c/ov9282.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 9842080cf66f..1c77b77427f0 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -88,7 +88,6 @@ struct ov9282_reg_list {
>   * struct ov9282_mode - ov9282 sensor mode structure
>   * @width: Frame width
>   * @height: Frame height
> - * @code: Format code
>   * @hblank: Horizontal blanking in lines
>   * @vblank: Vertical blanking in lines
>   * @vblank_min: Minimum vertical blanking in lines
> @@ -100,7 +99,6 @@ struct ov9282_reg_list {
>  struct ov9282_mode {
>  	u32 width;
>  	u32 height;
> -	u32 code;
>  	u32 hblank;
>  	u32 vblank;
>  	u32 vblank_min;
> @@ -273,7 +271,6 @@ static const struct ov9282_mode supported_mode = {
>  	.vblank_max = 51540,
>  	.pclk = 160000000,
>  	.link_freq_idx = 0,
> -	.code = MEDIA_BUS_FMT_Y10_1X10,
>  	.reg_list = {
>  		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
>  		.regs = mode_1280x720_regs,
> @@ -523,7 +520,7 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev *sd,
>  	if (code->index > 0)
>  		return -EINVAL;
>
> -	code->code = supported_mode.code;
> +	code->code = MEDIA_BUS_FMT_Y10_1X10;
>
>  	return 0;
>  }
> @@ -543,7 +540,7 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
>  	if (fsize->index > 0)
>  		return -EINVAL;
>
> -	if (fsize->code != supported_mode.code)
> +	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
>  		return -EINVAL;
>
>  	fsize->min_width = supported_mode.width;
> @@ -567,7 +564,7 @@ static void ov9282_fill_pad_format(struct ov9282 *ov9282,
>  {
>  	fmt->format.width = mode->width;
>  	fmt->format.height = mode->height;
> -	fmt->format.code = mode->code;
> +	fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
>  	fmt->format.field = V4L2_FIELD_NONE;
>  	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
>  	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> --
> 2.34.1
>

[PATCH 04/16] media: i2c: ov9282: Remove pixel rate from mode definition

[Dave Stevenson]

The pixel rate is determined by the PLL setup in the common
registers, not by the mode specific registers, therefore
remove it from the mode definition and define it for all modes.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 1c77b77427f0..798ff8ba50bd 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -53,6 +53,10 @@
 #define OV9282_LINK_FREQ	400000000
 #define OV9282_NUM_DATA_LANES	2
 
+/* Pixel rate */
+#define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
+				 OV9282_NUM_DATA_LANES / 10)
+
 #define OV9282_REG_MIN		0x00
 #define OV9282_REG_MAX		0xfffff
 
@@ -92,7 +96,6 @@ struct ov9282_reg_list {
  * @vblank: Vertical blanking in lines
  * @vblank_min: Minimum vertical blanking in lines
  * @vblank_max: Maximum vertical blanking in lines
- * @pclk: Sensor pixel clock
  * @link_freq_idx: Link frequency index
  * @reg_list: Register list for sensor mode
  */
@@ -103,7 +106,6 @@ struct ov9282_mode {
 	u32 vblank;
 	u32 vblank_min;
 	u32 vblank_max;
-	u64 pclk;
 	u32 link_freq_idx;
 	struct ov9282_reg_list reg_list;
 };
@@ -118,7 +120,6 @@ struct ov9282_mode {
  * @inclk: Sensor input clock
  * @ctrl_handler: V4L2 control handler
  * @link_freq_ctrl: Pointer to link frequency control
- * @pclk_ctrl: Pointer to pixel clock control
  * @hblank_ctrl: Pointer to horizontal blanking control
  * @vblank_ctrl: Pointer to vertical blanking control
  * @exp_ctrl: Pointer to exposure control
@@ -138,7 +139,6 @@ struct ov9282 {
 	struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_ctrl *link_freq_ctrl;
-	struct v4l2_ctrl *pclk_ctrl;
 	struct v4l2_ctrl *hblank_ctrl;
 	struct v4l2_ctrl *vblank_ctrl;
 	struct {
@@ -269,7 +269,6 @@ static const struct ov9282_mode supported_mode = {
 	.vblank = 1022,
 	.vblank_min = 151,
 	.vblank_max = 51540,
-	.pclk = 160000000,
 	.link_freq_idx = 0,
 	.reg_list = {
 		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
@@ -1006,11 +1005,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 						1, mode->vblank);
 
 	/* Read only controls */
-	ov9282->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
-					      &ov9282_ctrl_ops,
-					      V4L2_CID_PIXEL_RATE,
-					      mode->pclk, mode->pclk,
-					      1, mode->pclk);
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
+			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
+			  OV9282_PIXEL_RATE);
 
 	ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 							&ov9282_ctrl_ops,
-- 
2.34.1

Re: [PATCH 04/16] media: i2c: ov9282: Remove pixel rate from mode definition

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:27:57PM +0100, Dave Stevenson wrote:
> The pixel rate is determined by the PLL setup in the common
> registers, not by the mode specific registers, therefore
> remove it from the mode definition and define it for all modes.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 1c77b77427f0..798ff8ba50bd 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -53,6 +53,10 @@
>  #define OV9282_LINK_FREQ	400000000
>  #define OV9282_NUM_DATA_LANES	2
>
> +/* Pixel rate */
> +#define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
> +				 OV9282_NUM_DATA_LANES / 10)

This will work as long as there's a single Y10 supported mode.
I haven't looked at the rest of the series yet but if Y8 gets
supported it might be worth dynamically re-computing the PIXEL_RATE
when a new mode is applied.

For now this is good, and if Y10 remains the default format is fine to
initialize PIXEL_RATE assuming 10 bpp

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +
>  #define OV9282_REG_MIN		0x00
>  #define OV9282_REG_MAX		0xfffff
>
> @@ -92,7 +96,6 @@ struct ov9282_reg_list {
>   * @vblank: Vertical blanking in lines
>   * @vblank_min: Minimum vertical blanking in lines
>   * @vblank_max: Maximum vertical blanking in lines
> - * @pclk: Sensor pixel clock
>   * @link_freq_idx: Link frequency index
>   * @reg_list: Register list for sensor mode
>   */
> @@ -103,7 +106,6 @@ struct ov9282_mode {
>  	u32 vblank;
>  	u32 vblank_min;
>  	u32 vblank_max;
> -	u64 pclk;
>  	u32 link_freq_idx;
>  	struct ov9282_reg_list reg_list;
>  };
> @@ -118,7 +120,6 @@ struct ov9282_mode {
>   * @inclk: Sensor input clock
>   * @ctrl_handler: V4L2 control handler
>   * @link_freq_ctrl: Pointer to link frequency control
> - * @pclk_ctrl: Pointer to pixel clock control
>   * @hblank_ctrl: Pointer to horizontal blanking control
>   * @vblank_ctrl: Pointer to vertical blanking control
>   * @exp_ctrl: Pointer to exposure control
> @@ -138,7 +139,6 @@ struct ov9282 {
>  	struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_ctrl *link_freq_ctrl;
> -	struct v4l2_ctrl *pclk_ctrl;
>  	struct v4l2_ctrl *hblank_ctrl;
>  	struct v4l2_ctrl *vblank_ctrl;
>  	struct {
> @@ -269,7 +269,6 @@ static const struct ov9282_mode supported_mode = {
>  	.vblank = 1022,
>  	.vblank_min = 151,
>  	.vblank_max = 51540,
> -	.pclk = 160000000,
>  	.link_freq_idx = 0,
>  	.reg_list = {
>  		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> @@ -1006,11 +1005,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  						1, mode->vblank);
>
>  	/* Read only controls */
> -	ov9282->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> -					      &ov9282_ctrl_ops,
> -					      V4L2_CID_PIXEL_RATE,
> -					      mode->pclk, mode->pclk,
> -					      1, mode->pclk);
> +	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
> +			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> +			  OV9282_PIXEL_RATE);
>
>  	ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>  							&ov9282_ctrl_ops,
> --
> 2.34.1
>

Re: [PATCH 04/16] media: i2c: ov9282: Remove pixel rate from mode definition

[Dave Stevenson]

Hi Jacopo

Thanks for the review

On Thu, 6 Oct 2022 at 10:17, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:27:57PM +0100, Dave Stevenson wrote:
> > The pixel rate is determined by the PLL setup in the common
> > registers, not by the mode specific registers, therefore
> > remove it from the mode definition and define it for all modes.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 1c77b77427f0..798ff8ba50bd 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -53,6 +53,10 @@
> >  #define OV9282_LINK_FREQ     400000000
> >  #define OV9282_NUM_DATA_LANES        2
> >
> > +/* Pixel rate */
> > +#define OV9282_PIXEL_RATE    (OV9282_LINK_FREQ * 2 * \
> > +                              OV9282_NUM_DATA_LANES / 10)
>
> This will work as long as there's a single Y10 supported mode.
> I haven't looked at the rest of the series yet but if Y8 gets
> supported it might be worth dynamically re-computing the PIXEL_RATE
> when a new mode is applied.
>
> For now this is good, and if Y10 remains the default format is fine to
> initialize PIXEL_RATE assuming 10 bpp

Y8 support comes later in the series. At this point there are many
checks that the format is Y10.

  Dave

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>   j
>
> > +
> >  #define OV9282_REG_MIN               0x00
> >  #define OV9282_REG_MAX               0xfffff
> >
> > @@ -92,7 +96,6 @@ struct ov9282_reg_list {
> >   * @vblank: Vertical blanking in lines
> >   * @vblank_min: Minimum vertical blanking in lines
> >   * @vblank_max: Maximum vertical blanking in lines
> > - * @pclk: Sensor pixel clock
> >   * @link_freq_idx: Link frequency index
> >   * @reg_list: Register list for sensor mode
> >   */
> > @@ -103,7 +106,6 @@ struct ov9282_mode {
> >       u32 vblank;
> >       u32 vblank_min;
> >       u32 vblank_max;
> > -     u64 pclk;
> >       u32 link_freq_idx;
> >       struct ov9282_reg_list reg_list;
> >  };
> > @@ -118,7 +120,6 @@ struct ov9282_mode {
> >   * @inclk: Sensor input clock
> >   * @ctrl_handler: V4L2 control handler
> >   * @link_freq_ctrl: Pointer to link frequency control
> > - * @pclk_ctrl: Pointer to pixel clock control
> >   * @hblank_ctrl: Pointer to horizontal blanking control
> >   * @vblank_ctrl: Pointer to vertical blanking control
> >   * @exp_ctrl: Pointer to exposure control
> > @@ -138,7 +139,6 @@ struct ov9282 {
> >       struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
> >       struct v4l2_ctrl_handler ctrl_handler;
> >       struct v4l2_ctrl *link_freq_ctrl;
> > -     struct v4l2_ctrl *pclk_ctrl;
> >       struct v4l2_ctrl *hblank_ctrl;
> >       struct v4l2_ctrl *vblank_ctrl;
> >       struct {
> > @@ -269,7 +269,6 @@ static const struct ov9282_mode supported_mode = {
> >       .vblank = 1022,
> >       .vblank_min = 151,
> >       .vblank_max = 51540,
> > -     .pclk = 160000000,
> >       .link_freq_idx = 0,
> >       .reg_list = {
> >               .num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> > @@ -1006,11 +1005,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >                                               1, mode->vblank);
> >
> >       /* Read only controls */
> > -     ov9282->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> > -                                           &ov9282_ctrl_ops,
> > -                                           V4L2_CID_PIXEL_RATE,
> > -                                           mode->pclk, mode->pclk,
> > -                                           1, mode->pclk);
> > +     v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > +                       OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> > +                       OV9282_PIXEL_RATE);
> >
> >       ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> >                                                       &ov9282_ctrl_ops,
> > --
> > 2.34.1
> >

[PATCH 05/16] media: i2c: ov9281: Support more than 1 mode.

[Dave Stevenson]

The driver currently has multiple assumptions that there is
only one supported mode.

Convert supported_mode to an array, and fix up all references
to correctly look at that array.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 44 ++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 798ff8ba50bd..f7823d584522 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -262,20 +262,24 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 };
 
 /* Supported sensor mode configurations */
-static const struct ov9282_mode supported_mode = {
-	.width = 1280,
-	.height = 720,
-	.hblank = 250,
-	.vblank = 1022,
-	.vblank_min = 151,
-	.vblank_max = 51540,
-	.link_freq_idx = 0,
-	.reg_list = {
-		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
-		.regs = mode_1280x720_regs,
+static const struct ov9282_mode supported_modes[] = {
+	{
+		.width = 1280,
+		.height = 720,
+		.hblank = 250,
+		.vblank = 1022,
+		.vblank_min = 151,
+		.vblank_max = 51540,
+		.link_freq_idx = 0,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
+			.regs = mode_1280x720_regs,
+		},
 	},
 };
 
+#define OV9282_NUM_MODES ARRAY_SIZE(supported_modes)
+
 /**
  * to_ov9282() - ov9282 V4L2 sub-device to ov9282 device.
  * @subdev: pointer to ov9282 V4L2 sub-device
@@ -536,15 +540,15 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_frame_size_enum *fsize)
 {
-	if (fsize->index > 0)
+	if (fsize->index >= OV9282_NUM_MODES)
 		return -EINVAL;
 
 	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
 		return -EINVAL;
 
-	fsize->min_width = supported_mode.width;
+	fsize->min_width = supported_modes[fsize->index].width;
 	fsize->max_width = fsize->min_width;
-	fsize->min_height = supported_mode.height;
+	fsize->min_height = supported_modes[fsize->index].height;
 	fsize->max_height = fsize->min_height;
 
 	return 0;
@@ -619,7 +623,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
 
 	mutex_lock(&ov9282->mutex);
 
-	mode = &supported_mode;
+	mode = v4l2_find_nearest_size(supported_modes,
+				      OV9282_NUM_MODES,
+				      width, height,
+				      fmt->format.width,
+				      fmt->format.height);
 	ov9282_fill_pad_format(ov9282, mode, fmt);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
@@ -652,7 +660,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
 	struct v4l2_subdev_format fmt = { 0 };
 
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	ov9282_fill_pad_format(ov9282, &supported_mode, &fmt);
+	ov9282_fill_pad_format(ov9282, &supported_modes[0], &fmt);
 
 	return ov9282_set_pad_format(sd, sd_state, &fmt);
 }
@@ -1081,8 +1089,8 @@ static int ov9282_probe(struct i2c_client *client)
 		goto error_power_off;
 	}
 
-	/* Set default mode to max resolution */
-	ov9282->cur_mode = &supported_mode;
+	/* Set default mode to first mode */
+	ov9282->cur_mode = &supported_modes[0];
 	ov9282->vblank = ov9282->cur_mode->vblank;
 
 	ret = ov9282_init_controls(ov9282);
-- 
2.34.1

Re: [PATCH 05/16] media: i2c: ov9281: Support more than 1 mode.

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:27:58PM +0100, Dave Stevenson wrote:
> The driver currently has multiple assumptions that there is
> only one supported mode.
>
> Convert supported_mode to an array, and fix up all references
> to correctly look at that array.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 44 ++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 798ff8ba50bd..f7823d584522 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -262,20 +262,24 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  };
>
>  /* Supported sensor mode configurations */
> -static const struct ov9282_mode supported_mode = {
> -	.width = 1280,
> -	.height = 720,
> -	.hblank = 250,
> -	.vblank = 1022,
> -	.vblank_min = 151,
> -	.vblank_max = 51540,
> -	.link_freq_idx = 0,
> -	.reg_list = {
> -		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> -		.regs = mode_1280x720_regs,
> +static const struct ov9282_mode supported_modes[] = {
> +	{
> +		.width = 1280,
> +		.height = 720,
> +		.hblank = 250,
> +		.vblank = 1022,
> +		.vblank_min = 151,
> +		.vblank_max = 51540,
> +		.link_freq_idx = 0,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> +			.regs = mode_1280x720_regs,
> +		},
>  	},
>  };
>
> +#define OV9282_NUM_MODES ARRAY_SIZE(supported_modes)
> +
>  /**
>   * to_ov9282() - ov9282 V4L2 sub-device to ov9282 device.
>   * @subdev: pointer to ov9282 V4L2 sub-device
> @@ -536,15 +540,15 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_frame_size_enum *fsize)
>  {
> -	if (fsize->index > 0)
> +	if (fsize->index >= OV9282_NUM_MODES)
>  		return -EINVAL;
>
>  	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
>  		return -EINVAL;
>
> -	fsize->min_width = supported_mode.width;
> +	fsize->min_width = supported_modes[fsize->index].width;
>  	fsize->max_width = fsize->min_width;
> -	fsize->min_height = supported_mode.height;
> +	fsize->min_height = supported_modes[fsize->index].height;
>  	fsize->max_height = fsize->min_height;
>
>  	return 0;
> @@ -619,7 +623,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>
>  	mutex_lock(&ov9282->mutex);
>
> -	mode = &supported_mode;
> +	mode = v4l2_find_nearest_size(supported_modes,
> +				      OV9282_NUM_MODES,
> +				      width, height,
> +				      fmt->format.width,
> +				      fmt->format.height);
>  	ov9282_fill_pad_format(ov9282, mode, fmt);
>
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> @@ -652,7 +660,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_subdev_format fmt = { 0 };
>
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ov9282_fill_pad_format(ov9282, &supported_mode, &fmt);
> +	ov9282_fill_pad_format(ov9282, &supported_modes[0], &fmt);

When new modes gets added, it might be worth defining
#define OV9286_DEFAULT_MODE     0

To make sure the active and the try formats are initialized with the
same value

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
>  	return ov9282_set_pad_format(sd, sd_state, &fmt);
>  }
> @@ -1081,8 +1089,8 @@ static int ov9282_probe(struct i2c_client *client)
>  		goto error_power_off;
>  	}
>
> -	/* Set default mode to max resolution */
> -	ov9282->cur_mode = &supported_mode;
> +	/* Set default mode to first mode */
> +	ov9282->cur_mode = &supported_modes[0];
>  	ov9282->vblank = ov9282->cur_mode->vblank;
>
>  	ret = ov9282_init_controls(ov9282);
> --
> 2.34.1
>

Re: [PATCH 05/16] media: i2c: ov9281: Support more than 1 mode.

[Sakari Ailus]

Hi Dave,

On Wed, Oct 05, 2022 at 04:27:58PM +0100, Dave Stevenson wrote:
> The driver currently has multiple assumptions that there is
> only one supported mode.
> 
> Convert supported_mode to an array, and fix up all references
> to correctly look at that array.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 44 ++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 798ff8ba50bd..f7823d584522 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -262,20 +262,24 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  };
>  
>  /* Supported sensor mode configurations */
> -static const struct ov9282_mode supported_mode = {
> -	.width = 1280,
> -	.height = 720,
> -	.hblank = 250,
> -	.vblank = 1022,
> -	.vblank_min = 151,
> -	.vblank_max = 51540,
> -	.link_freq_idx = 0,
> -	.reg_list = {
> -		.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> -		.regs = mode_1280x720_regs,
> +static const struct ov9282_mode supported_modes[] = {
> +	{
> +		.width = 1280,
> +		.height = 720,
> +		.hblank = 250,
> +		.vblank = 1022,
> +		.vblank_min = 151,
> +		.vblank_max = 51540,
> +		.link_freq_idx = 0,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> +			.regs = mode_1280x720_regs,
> +		},
>  	},
>  };
>  
> +#define OV9282_NUM_MODES ARRAY_SIZE(supported_modes)

I think the code would be cleaner without the additional macro, i.e. always
using ARRAY_SIZE(supported_modes).

> +
>  /**
>   * to_ov9282() - ov9282 V4L2 sub-device to ov9282 device.
>   * @subdev: pointer to ov9282 V4L2 sub-device
> @@ -536,15 +540,15 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_frame_size_enum *fsize)
>  {
> -	if (fsize->index > 0)
> +	if (fsize->index >= OV9282_NUM_MODES)
>  		return -EINVAL;
>  
>  	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
>  		return -EINVAL;
>  
> -	fsize->min_width = supported_mode.width;
> +	fsize->min_width = supported_modes[fsize->index].width;
>  	fsize->max_width = fsize->min_width;
> -	fsize->min_height = supported_mode.height;
> +	fsize->min_height = supported_modes[fsize->index].height;
>  	fsize->max_height = fsize->min_height;
>  
>  	return 0;
> @@ -619,7 +623,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>  
>  	mutex_lock(&ov9282->mutex);
>  
> -	mode = &supported_mode;
> +	mode = v4l2_find_nearest_size(supported_modes,
> +				      OV9282_NUM_MODES,
> +				      width, height,
> +				      fmt->format.width,
> +				      fmt->format.height);
>  	ov9282_fill_pad_format(ov9282, mode, fmt);
>  
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> @@ -652,7 +660,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_subdev_format fmt = { 0 };
>  
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ov9282_fill_pad_format(ov9282, &supported_mode, &fmt);
> +	ov9282_fill_pad_format(ov9282, &supported_modes[0], &fmt);
>  
>  	return ov9282_set_pad_format(sd, sd_state, &fmt);
>  }
> @@ -1081,8 +1089,8 @@ static int ov9282_probe(struct i2c_client *client)
>  		goto error_power_off;
>  	}
>  
> -	/* Set default mode to max resolution */
> -	ov9282->cur_mode = &supported_mode;
> +	/* Set default mode to first mode */
> +	ov9282->cur_mode = &supported_modes[0];
>  	ov9282->vblank = ov9282->cur_mode->vblank;
>  
>  	ret = ov9282_init_controls(ov9282);

-- 
Kind regards,

Sakari Ailus

[PATCH 06/16] media: i2c: ov9282: Correct HTS register for configured pixel rate

[Dave Stevenson]

The calculations from pixel rate, width+hblank, and height+vblank
do not give the correct framerate - it's half the speed it should
be.

Whilst not documented as such, the TIMING_HTS register (0x380c/d)
appears to be in units of 2 pixels.
The default is 0x2d8 (728) which can not be valid as-is when the
frame is 1280 active pixels wide. Doubling to 0x5b0 (1456) results
in the correct timings.

This driver isn't using the default frame width + hblank, so
use 0x02fd which is half of the width of 1280 and hblank of 250
which is reported to userspace. With this the frame rate calculations
work correctly.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index f7823d584522..1cd6cb4addfb 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -242,8 +242,8 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3809, 0x00},
 	{0x380a, 0x02},
 	{0x380b, 0xd0},
-	{0x380c, 0x05},
-	{0x380d, 0xfa},
+	{0x380c, 0x02},
+	{0x380d, 0xfd},
 	{0x3810, 0x00},
 	{0x3811, 0x08},
 	{0x3812, 0x00},
-- 
2.34.1

Re: [PATCH 06/16] media: i2c: ov9282: Correct HTS register for configured pixel rate

[Jacopo Mondi]

Hi Dave,

On Wed, Oct 05, 2022 at 04:27:59PM +0100, Dave Stevenson wrote:
> The calculations from pixel rate, width+hblank, and height+vblank
> do not give the correct framerate - it's half the speed it should
> be.
>
> Whilst not documented as such, the TIMING_HTS register (0x380c/d)
> appears to be in units of 2 pixels.
> The default is 0x2d8 (728) which can not be valid as-is when the
> frame is 1280 active pixels wide. Doubling to 0x5b0 (1456) results
> in the correct timings.

Not sure I get this one. If TIMING_HTS has a 2-pixels units, won't
0x2d8 be correct (as it equals to your desired 1456 pixels) ?

>
> This driver isn't using the default frame width + hblank, so
> use 0x02fd which is half of the width of 1280 and hblank of 250
> which is reported to userspace. With this the frame rate calculations
> work correctly.

So this patch is basically changing the default vblank from 728 to 760 ?

Should we rather move the per-mode blankings to the mode definition
and program them when applying a new mode ?

>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index f7823d584522..1cd6cb4addfb 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -242,8 +242,8 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x3809, 0x00},
>  	{0x380a, 0x02},
>  	{0x380b, 0xd0},
> -	{0x380c, 0x05},
> -	{0x380d, 0xfa},
> +	{0x380c, 0x02},
> +	{0x380d, 0xfd},
>  	{0x3810, 0x00},
>  	{0x3811, 0x08},
>  	{0x3812, 0x00},
> --
> 2.34.1
>

Re: [PATCH 06/16] media: i2c: ov9282: Correct HTS register for configured pixel rate

[Dave Stevenson]

Hi Jacopo

Thanks for the review (missed responding on my first pass)

On Thu, 6 Oct 2022 at 10:23, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave,
>
> On Wed, Oct 05, 2022 at 04:27:59PM +0100, Dave Stevenson wrote:
> > The calculations from pixel rate, width+hblank, and height+vblank
> > do not give the correct framerate - it's half the speed it should
> > be.
> >
> > Whilst not documented as such, the TIMING_HTS register (0x380c/d)
> > appears to be in units of 2 pixels.
> > The default is 0x2d8 (728) which can not be valid as-is when the
> > frame is 1280 active pixels wide. Doubling to 0x5b0 (1456) results
> > in the correct timings.
>
> Not sure I get this one. If TIMING_HTS has a 2-pixels units, won't
> 0x2d8 be correct (as it equals to your desired 1456 pixels) ?

Bad wording by me.
From the datasheet the default register value is 0x2d8, which is less
than the width.
Treating it as 0x2d8 * 2 = 1456 gives the right numbers.

> >
> > This driver isn't using the default frame width + hblank, so
> > use 0x02fd which is half of the width of 1280 and hblank of 250
> > which is reported to userspace. With this the frame rate calculations
> > work correctly.
>
> So this patch is basically changing the default vblank from 728 to 760 ?

Again bad wording.
The driver was using a value of 0x5fa intending it to be 1530 pixels.
(As discussed later, this is due to the driver using non-continuous
clock mode, which needs an increased hblanking).
However as the units are 2-pixels, the actual value required is 0x5fa
/ 2 = 0x2fd.

I'll reword this.

> Should we rather move the per-mode blankings to the mode definition
> and program them when applying a new mode ?
>
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index f7823d584522..1cd6cb4addfb 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -242,8 +242,8 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
> >       {0x3809, 0x00},
> >       {0x380a, 0x02},
> >       {0x380b, 0xd0},
> > -     {0x380c, 0x05},
> > -     {0x380d, 0xfa},
> > +     {0x380c, 0x02},
> > +     {0x380d, 0xfd},
> >       {0x3810, 0x00},
> >       {0x3811, 0x08},
> >       {0x3812, 0x00},
> > --
> > 2.34.1
> >

[PATCH 07/16] media: i2c: ov9282: Reduce vblank_min values based on testing

[Dave Stevenson]

The configured vblank_min setting of 250 (meaning VTS of
720 + 250 = 970) is far higher than the setting that works on
the sensor, and there are no obvious restrictions stated in the
datasheet.

Reduce the vblank_min to allow for faster frame rates.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 1cd6cb4addfb..abb1223c0260 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -268,7 +268,7 @@ static const struct ov9282_mode supported_modes[] = {
 		.height = 720,
 		.hblank = 250,
 		.vblank = 1022,
-		.vblank_min = 151,
+		.vblank_min = 41,
 		.vblank_max = 51540,
 		.link_freq_idx = 0,
 		.reg_list = {
-- 
2.34.1

Re: [PATCH 07/16] media: i2c: ov9282: Reduce vblank_min values based on testing

[Jacopo Mondi]

Hi Dave,

On Wed, Oct 05, 2022 at 04:28:00PM +0100, Dave Stevenson wrote:
> The configured vblank_min setting of 250 (meaning VTS of

Do you mean 151 ?

> 720 + 250 = 970) is far higher than the setting that works on
> the sensor, and there are no obvious restrictions stated in the
> datasheet.
>
> Reduce the vblank_min to allow for faster frame rates.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 1cd6cb4addfb..abb1223c0260 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -268,7 +268,7 @@ static const struct ov9282_mode supported_modes[] = {
>  		.height = 720,
>  		.hblank = 250,
>  		.vblank = 1022,
> -		.vblank_min = 151,
> +		.vblank_min = 41,
>  		.vblank_max = 51540,
>  		.link_freq_idx = 0,
>  		.reg_list = {
> --
> 2.34.1
>

Re: [PATCH 07/16] media: i2c: ov9282: Reduce vblank_min values based on testing

[Dave Stevenson]

Hi Jacopo

On Thu, 6 Oct 2022 at 12:56, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave,
>
> On Wed, Oct 05, 2022 at 04:28:00PM +0100, Dave Stevenson wrote:
> > The configured vblank_min setting of 250 (meaning VTS of
>
> Do you mean 151 ?

I did. Misread hblank for vblank.

  Dave

> > 720 + 250 = 970) is far higher than the setting that works on
> > the sensor, and there are no obvious restrictions stated in the
> > datasheet.
> >
> > Reduce the vblank_min to allow for faster frame rates.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 1cd6cb4addfb..abb1223c0260 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -268,7 +268,7 @@ static const struct ov9282_mode supported_modes[] = {
> >               .height = 720,
> >               .hblank = 250,
> >               .vblank = 1022,
> > -             .vblank_min = 151,
> > +             .vblank_min = 41,
> >               .vblank_max = 51540,
> >               .link_freq_idx = 0,
> >               .reg_list = {
> > --
> > 2.34.1
> >

[PATCH 08/16] media: i2c: ov9282: Add selection for CSI2 clock mode

[Dave Stevenson]

The sensor supports either having the CSI2 clock lane free
running, or gated when there is no packet to transmit.
The driver only selected gated (non-continuous) clock mode.

Add code to allow fwnode to configure whether the clock is
gated or free running.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index abb1223c0260..334b31af34a4 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -46,6 +46,9 @@
 /* Group hold register */
 #define OV9282_REG_HOLD		0x3308
 
+#define OV9282_REG_MIPI_CTRL00	0x4800
+#define OV9282_GATED_CLOCK	BIT(5)
+
 /* Input clock rate */
 #define OV9282_INCLK_RATE	24000000
 
@@ -138,6 +141,7 @@ struct ov9282 {
 	struct clk *inclk;
 	struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
 	struct v4l2_ctrl_handler ctrl_handler;
+	bool noncontinuous_clock;
 	struct v4l2_ctrl *link_freq_ctrl;
 	struct v4l2_ctrl *hblank_ctrl;
 	struct v4l2_ctrl *vblank_ctrl;
@@ -211,7 +215,6 @@ static const struct ov9282_reg common_regs[] = {
 	{0x4601, 0x04},
 	{0x470f, 0x00},
 	{0x4f07, 0x00},
-	{0x4800, 0x20},
 	{0x5000, 0x9f},
 	{0x5001, 0x00},
 	{0x5e00, 0x00},
@@ -684,6 +687,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
 		return ret;
 	}
 
+	ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1,
+			       ov9282->noncontinuous_clock ?
+					OV9282_GATED_CLOCK : 0);
+	if (ret) {
+		dev_err(ov9282->dev, "fail to write MIPI_CTRL00");
+		return ret;
+	}
+
 	/* Write sensor mode registers */
 	reg_list = &ov9282->cur_mode->reg_list;
 	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
@@ -861,6 +872,9 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
 	if (ret)
 		return ret;
 
+	ov9282->noncontinuous_clock =
+		bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
 	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
 		dev_err(ov9282->dev,
 			"number of CSI2 data lanes %d is not supported",
-- 
2.34.1

Re: [PATCH 08/16] media: i2c: ov9282: Add selection for CSI2 clock mode

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:28:01PM +0100, Dave Stevenson wrote:
> The sensor supports either having the CSI2 clock lane free
> running, or gated when there is no packet to transmit.
> The driver only selected gated (non-continuous) clock mode.
>
> Add code to allow fwnode to configure whether the clock is
> gated or free running.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index abb1223c0260..334b31af34a4 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -46,6 +46,9 @@
>  /* Group hold register */
>  #define OV9282_REG_HOLD		0x3308
>
> +#define OV9282_REG_MIPI_CTRL00	0x4800
> +#define OV9282_GATED_CLOCK	BIT(5)
> +
>  /* Input clock rate */
>  #define OV9282_INCLK_RATE	24000000
>
> @@ -138,6 +141,7 @@ struct ov9282 {
>  	struct clk *inclk;
>  	struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
>  	struct v4l2_ctrl_handler ctrl_handler;
> +	bool noncontinuous_clock;

Nit: I would move this down after the controls, where vblank is

>  	struct v4l2_ctrl *link_freq_ctrl;
>  	struct v4l2_ctrl *hblank_ctrl;
>  	struct v4l2_ctrl *vblank_ctrl;
> @@ -211,7 +215,6 @@ static const struct ov9282_reg common_regs[] = {
>  	{0x4601, 0x04},
>  	{0x470f, 0x00},
>  	{0x4f07, 0x00},
> -	{0x4800, 0x20},
>  	{0x5000, 0x9f},
>  	{0x5001, 0x00},
>  	{0x5e00, 0x00},
> @@ -684,6 +687,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
>  		return ret;
>  	}
>
> +	ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1,
> +			       ov9282->noncontinuous_clock ?
> +					OV9282_GATED_CLOCK : 0);
> +	if (ret) {
> +		dev_err(ov9282->dev, "fail to write MIPI_CTRL00");
> +		return ret;
> +	}
> +
>  	/* Write sensor mode registers */
>  	reg_list = &ov9282->cur_mode->reg_list;
>  	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
> @@ -861,6 +872,9 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
>  	if (ret)
>  		return ret;
>
> +	ov9282->noncontinuous_clock =
> +		bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> +

The patch looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
>  		dev_err(ov9282->dev,
>  			"number of CSI2 data lanes %d is not supported",
> --
> 2.34.1
>

Re: [PATCH 08/16] media: i2c: ov9282: Add selection for CSI2 clock mode

[Sakari Ailus]

Hi Dave,

On Wed, Oct 05, 2022 at 04:28:01PM +0100, Dave Stevenson wrote:
> The sensor supports either having the CSI2 clock lane free
> running, or gated when there is no packet to transmit.
> The driver only selected gated (non-continuous) clock mode.
> 
> Add code to allow fwnode to configure whether the clock is
> gated or free running.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index abb1223c0260..334b31af34a4 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -46,6 +46,9 @@
>  /* Group hold register */
>  #define OV9282_REG_HOLD		0x3308
>  
> +#define OV9282_REG_MIPI_CTRL00	0x4800
> +#define OV9282_GATED_CLOCK	BIT(5)
> +
>  /* Input clock rate */
>  #define OV9282_INCLK_RATE	24000000
>  
> @@ -138,6 +141,7 @@ struct ov9282 {
>  	struct clk *inclk;
>  	struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
>  	struct v4l2_ctrl_handler ctrl_handler;
> +	bool noncontinuous_clock;
>  	struct v4l2_ctrl *link_freq_ctrl;
>  	struct v4l2_ctrl *hblank_ctrl;
>  	struct v4l2_ctrl *vblank_ctrl;
> @@ -211,7 +215,6 @@ static const struct ov9282_reg common_regs[] = {
>  	{0x4601, 0x04},
>  	{0x470f, 0x00},
>  	{0x4f07, 0x00},
> -	{0x4800, 0x20},
>  	{0x5000, 0x9f},
>  	{0x5001, 0x00},
>  	{0x5e00, 0x00},
> @@ -684,6 +687,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
>  		return ret;
>  	}
>  
> +	ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1,
> +			       ov9282->noncontinuous_clock ?
> +					OV9282_GATED_CLOCK : 0);

Wouldn't this better fit for power on?

> +	if (ret) {
> +		dev_err(ov9282->dev, "fail to write MIPI_CTRL00");
> +		return ret;
> +	}
> +
>  	/* Write sensor mode registers */
>  	reg_list = &ov9282->cur_mode->reg_list;
>  	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
> @@ -861,6 +872,9 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
>  	if (ret)
>  		return ret;
>  
> +	ov9282->noncontinuous_clock =
> +		bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> +
>  	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
>  		dev_err(ov9282->dev,
>  			"number of CSI2 data lanes %d is not supported",

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 08/16] media: i2c: ov9282: Add selection for CSI2 clock mode

[Dave Stevenson]

Hi Sakari

On Wed, 26 Oct 2022 at 08:21, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Dave,
>
> On Wed, Oct 05, 2022 at 04:28:01PM +0100, Dave Stevenson wrote:
> > The sensor supports either having the CSI2 clock lane free
> > running, or gated when there is no packet to transmit.
> > The driver only selected gated (non-continuous) clock mode.
> >
> > Add code to allow fwnode to configure whether the clock is
> > gated or free running.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index abb1223c0260..334b31af34a4 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -46,6 +46,9 @@
> >  /* Group hold register */
> >  #define OV9282_REG_HOLD              0x3308
> >
> > +#define OV9282_REG_MIPI_CTRL00       0x4800
> > +#define OV9282_GATED_CLOCK   BIT(5)
> > +
> >  /* Input clock rate */
> >  #define OV9282_INCLK_RATE    24000000
> >
> > @@ -138,6 +141,7 @@ struct ov9282 {
> >       struct clk *inclk;
> >       struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
> >       struct v4l2_ctrl_handler ctrl_handler;
> > +     bool noncontinuous_clock;
> >       struct v4l2_ctrl *link_freq_ctrl;
> >       struct v4l2_ctrl *hblank_ctrl;
> >       struct v4l2_ctrl *vblank_ctrl;
> > @@ -211,7 +215,6 @@ static const struct ov9282_reg common_regs[] = {
> >       {0x4601, 0x04},
> >       {0x470f, 0x00},
> >       {0x4f07, 0x00},
> > -     {0x4800, 0x20},
> >       {0x5000, 0x9f},
> >       {0x5001, 0x00},
> >       {0x5e00, 0x00},
> > @@ -684,6 +687,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
> >               return ret;
> >       }
> >
> > +     ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1,
> > +                            ov9282->noncontinuous_clock ?
> > +                                     OV9282_GATED_CLOCK : 0);
>
> Wouldn't this better fit for power on?

It can be done in ov9282_power_on, but is then totally redundantly set
when powering the sensor up to read the ID during initial probe.
Doing so also means there needs to be a great big warning never to
change the driver and hit the software reset via writing 0x01 to
register 0x0103 as part of any register array (very common in many
other sensor drivers).

I'll move it and add a comment before the register tables.

  Dave

> > +     if (ret) {
> > +             dev_err(ov9282->dev, "fail to write MIPI_CTRL00");
> > +             return ret;
> > +     }
> > +
> >       /* Write sensor mode registers */
> >       reg_list = &ov9282->cur_mode->reg_list;
> >       ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
> > @@ -861,6 +872,9 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
> >       if (ret)
> >               return ret;
> >
> > +     ov9282->noncontinuous_clock =
> > +             bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> > +
> >       if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
> >               dev_err(ov9282->dev,
> >                       "number of CSI2 data lanes %d is not supported",
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH 08/16] media: i2c: ov9282: Add selection for CSI2 clock mode

[Sakari Ailus]

Hi Dave,

On Fri, Oct 28, 2022 at 01:57:48PM +0100, Dave Stevenson wrote:
> Hi Sakari
> 
> On Wed, 26 Oct 2022 at 08:21, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Dave,
> >
> > On Wed, Oct 05, 2022 at 04:28:01PM +0100, Dave Stevenson wrote:
> > > The sensor supports either having the CSI2 clock lane free
> > > running, or gated when there is no packet to transmit.
> > > The driver only selected gated (non-continuous) clock mode.
> > >
> > > Add code to allow fwnode to configure whether the clock is
> > > gated or free running.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index abb1223c0260..334b31af34a4 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -46,6 +46,9 @@
> > >  /* Group hold register */
> > >  #define OV9282_REG_HOLD              0x3308
> > >
> > > +#define OV9282_REG_MIPI_CTRL00       0x4800
> > > +#define OV9282_GATED_CLOCK   BIT(5)
> > > +
> > >  /* Input clock rate */
> > >  #define OV9282_INCLK_RATE    24000000
> > >
> > > @@ -138,6 +141,7 @@ struct ov9282 {
> > >       struct clk *inclk;
> > >       struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
> > >       struct v4l2_ctrl_handler ctrl_handler;
> > > +     bool noncontinuous_clock;
> > >       struct v4l2_ctrl *link_freq_ctrl;
> > >       struct v4l2_ctrl *hblank_ctrl;
> > >       struct v4l2_ctrl *vblank_ctrl;
> > > @@ -211,7 +215,6 @@ static const struct ov9282_reg common_regs[] = {
> > >       {0x4601, 0x04},
> > >       {0x470f, 0x00},
> > >       {0x4f07, 0x00},
> > > -     {0x4800, 0x20},
> > >       {0x5000, 0x9f},
> > >       {0x5001, 0x00},
> > >       {0x5e00, 0x00},
> > > @@ -684,6 +687,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
> > >               return ret;
> > >       }
> > >
> > > +     ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1,
> > > +                            ov9282->noncontinuous_clock ?
> > > +                                     OV9282_GATED_CLOCK : 0);
> >
> > Wouldn't this better fit for power on?
> 
> It can be done in ov9282_power_on, but is then totally redundantly set
> when powering the sensor up to read the ID during initial probe.

This is the same also when streaming is enabled and disabled multiple times
while the sensor is powered on. Although without autosuspend this may be
unlikely.

> Doing so also means there needs to be a great big warning never to
> change the driver and hit the software reset via writing 0x01 to
> register 0x0103 as part of any register array (very common in many
> other sensor drivers).

If there's a desire to reset the sensor after powering it up, that should
be done as the first thing after power-up. Setting non-continuous clock
isn't anything special here.

But that's up to you. I guess lane configuration etc. is part of the big
register lists.

> 
> I'll move it and add a comment before the register tables.

I think it's unnecessary.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 08/16] media: i2c: ov9282: Add selection for CSI2 clock mode

[Dave Stevenson]

Hi Sakari

On Fri, 28 Oct 2022 at 15:30, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Dave,
>
> On Fri, Oct 28, 2022 at 01:57:48PM +0100, Dave Stevenson wrote:
> > Hi Sakari
> >
> > On Wed, 26 Oct 2022 at 08:21, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > >
> > > Hi Dave,
> > >
> > > On Wed, Oct 05, 2022 at 04:28:01PM +0100, Dave Stevenson wrote:
> > > > The sensor supports either having the CSI2 clock lane free
> > > > running, or gated when there is no packet to transmit.
> > > > The driver only selected gated (non-continuous) clock mode.
> > > >
> > > > Add code to allow fwnode to configure whether the clock is
> > > > gated or free running.
> > > >
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > ---
> > > >  drivers/media/i2c/ov9282.c | 16 +++++++++++++++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > index abb1223c0260..334b31af34a4 100644
> > > > --- a/drivers/media/i2c/ov9282.c
> > > > +++ b/drivers/media/i2c/ov9282.c
> > > > @@ -46,6 +46,9 @@
> > > >  /* Group hold register */
> > > >  #define OV9282_REG_HOLD              0x3308
> > > >
> > > > +#define OV9282_REG_MIPI_CTRL00       0x4800
> > > > +#define OV9282_GATED_CLOCK   BIT(5)
> > > > +
> > > >  /* Input clock rate */
> > > >  #define OV9282_INCLK_RATE    24000000
> > > >
> > > > @@ -138,6 +141,7 @@ struct ov9282 {
> > > >       struct clk *inclk;
> > > >       struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
> > > >       struct v4l2_ctrl_handler ctrl_handler;
> > > > +     bool noncontinuous_clock;
> > > >       struct v4l2_ctrl *link_freq_ctrl;
> > > >       struct v4l2_ctrl *hblank_ctrl;
> > > >       struct v4l2_ctrl *vblank_ctrl;
> > > > @@ -211,7 +215,6 @@ static const struct ov9282_reg common_regs[] = {
> > > >       {0x4601, 0x04},
> > > >       {0x470f, 0x00},
> > > >       {0x4f07, 0x00},
> > > > -     {0x4800, 0x20},
> > > >       {0x5000, 0x9f},
> > > >       {0x5001, 0x00},
> > > >       {0x5e00, 0x00},
> > > > @@ -684,6 +687,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
> > > >               return ret;
> > > >       }
> > > >
> > > > +     ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1,
> > > > +                            ov9282->noncontinuous_clock ?
> > > > +                                     OV9282_GATED_CLOCK : 0);
> > >
> > > Wouldn't this better fit for power on?
> >
> > It can be done in ov9282_power_on, but is then totally redundantly set
> > when powering the sensor up to read the ID during initial probe.
>
> This is the same also when streaming is enabled and disabled multiple times
> while the sensor is powered on. Although without autosuspend this may be
> unlikely.
>
> > Doing so also means there needs to be a great big warning never to
> > change the driver and hit the software reset via writing 0x01 to
> > register 0x0103 as part of any register array (very common in many
> > other sensor drivers).
>
> If there's a desire to reset the sensor after powering it up, that should
> be done as the first thing after power-up. Setting non-continuous clock
> isn't anything special here.

I'm only looking at existing drivers in mainline as there is no clear
documentation on do's and don't's within sensor drivers (I know
writing good documentation is hard).
ov7251 [1] reset in ov7251_global_init_setting
ov8856 [2] reset in the lane config tables
ov5695 [3] reset in ov5695_global_regs
ov2740 [4] reset in mipi_data_rate_720mbps
ov13858 [5] explicit reset in ov13858_start_streaming
ov13b10 [6] explicit reset in ov13b10_start_streaming

In my book that's a common enough pattern in mainline drivers for it
to be worth warning against introducing it when it will cause quirky
behaviour.

[1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov7251.c#L238
[2] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov8856.c#L167
[3] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov5695.c#L128
[4] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov2740.c#L127
[5] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov13858.c#L1421
[6] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov13b10.c#L1033

> But that's up to you. I guess lane configuration etc. is part of the big
> register lists.

Only 2 data lanes are currently supported by the driver.
Bit 5 of 0x3039 in theory allows you to drop to 1 lane, but I've not
got it to work. I suspect further clock tree changes are required.

A 400MHz link freq (800Mbit/s/lane) is already required for the max
1280x800@120fps.
Dropping to 1 lane therefore either requires reducing the max frame
rate, or potentially running at 800MHz link freq, which is well in
excess of the earlier versions of CSI-2 spec (500MHz or 1Gbit/s). IMHO
It's not worth pursuing.

  Dave

> >
> > I'll move it and add a comment before the register tables.
>
> I think it's unnecessary.
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH 08/16] media: i2c: ov9282: Add selection for CSI2 clock mode

[Sakari Ailus]

Hi Dave,

On Fri, Oct 28, 2022 at 04:03:45PM +0100, Dave Stevenson wrote:
> Hi Sakari
> 
> On Fri, 28 Oct 2022 at 15:30, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Dave,
> >
> > On Fri, Oct 28, 2022 at 01:57:48PM +0100, Dave Stevenson wrote:
> > > Hi Sakari
> > >
> > > On Wed, 26 Oct 2022 at 08:21, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > >
> > > > Hi Dave,
> > > >
> > > > On Wed, Oct 05, 2022 at 04:28:01PM +0100, Dave Stevenson wrote:
> > > > > The sensor supports either having the CSI2 clock lane free
> > > > > running, or gated when there is no packet to transmit.
> > > > > The driver only selected gated (non-continuous) clock mode.
> > > > >
> > > > > Add code to allow fwnode to configure whether the clock is
> > > > > gated or free running.
> > > > >
> > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > ---
> > > > >  drivers/media/i2c/ov9282.c | 16 +++++++++++++++-
> > > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > > index abb1223c0260..334b31af34a4 100644
> > > > > --- a/drivers/media/i2c/ov9282.c
> > > > > +++ b/drivers/media/i2c/ov9282.c
> > > > > @@ -46,6 +46,9 @@
> > > > >  /* Group hold register */
> > > > >  #define OV9282_REG_HOLD              0x3308
> > > > >
> > > > > +#define OV9282_REG_MIPI_CTRL00       0x4800
> > > > > +#define OV9282_GATED_CLOCK   BIT(5)
> > > > > +
> > > > >  /* Input clock rate */
> > > > >  #define OV9282_INCLK_RATE    24000000
> > > > >
> > > > > @@ -138,6 +141,7 @@ struct ov9282 {
> > > > >       struct clk *inclk;
> > > > >       struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
> > > > >       struct v4l2_ctrl_handler ctrl_handler;
> > > > > +     bool noncontinuous_clock;
> > > > >       struct v4l2_ctrl *link_freq_ctrl;
> > > > >       struct v4l2_ctrl *hblank_ctrl;
> > > > >       struct v4l2_ctrl *vblank_ctrl;
> > > > > @@ -211,7 +215,6 @@ static const struct ov9282_reg common_regs[] = {
> > > > >       {0x4601, 0x04},
> > > > >       {0x470f, 0x00},
> > > > >       {0x4f07, 0x00},
> > > > > -     {0x4800, 0x20},
> > > > >       {0x5000, 0x9f},
> > > > >       {0x5001, 0x00},
> > > > >       {0x5e00, 0x00},
> > > > > @@ -684,6 +687,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
> > > > >               return ret;
> > > > >       }
> > > > >
> > > > > +     ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1,
> > > > > +                            ov9282->noncontinuous_clock ?
> > > > > +                                     OV9282_GATED_CLOCK : 0);
> > > >
> > > > Wouldn't this better fit for power on?
> > >
> > > It can be done in ov9282_power_on, but is then totally redundantly set
> > > when powering the sensor up to read the ID during initial probe.
> >
> > This is the same also when streaming is enabled and disabled multiple times
> > while the sensor is powered on. Although without autosuspend this may be
> > unlikely.
> >
> > > Doing so also means there needs to be a great big warning never to
> > > change the driver and hit the software reset via writing 0x01 to
> > > register 0x0103 as part of any register array (very common in many
> > > other sensor drivers).
> >
> > If there's a desire to reset the sensor after powering it up, that should
> > be done as the first thing after power-up. Setting non-continuous clock
> > isn't anything special here.
> 
> I'm only looking at existing drivers in mainline as there is no clear
> documentation on do's and don't's within sensor drivers (I know
> writing good documentation is hard).
> ov7251 [1] reset in ov7251_global_init_setting
> ov8856 [2] reset in the lane config tables
> ov5695 [3] reset in ov5695_global_regs
> ov2740 [4] reset in mipi_data_rate_720mbps
> ov13858 [5] explicit reset in ov13858_start_streaming
> ov13b10 [6] explicit reset in ov13b10_start_streaming
> 
> In my book that's a common enough pattern in mainline drivers for it
> to be worth warning against introducing it when it will cause quirky
> behaviour.
> 
> [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov7251.c#L238
> [2] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov8856.c#L167
> [3] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov5695.c#L128
> [4] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov2740.c#L127
> [5] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov13858.c#L1421
> [6] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov13b10.c#L1033

Well, register list based sensor drivers are hardly exemplary in this
respect. Some drivers reset the device after resuming it, some do not.

As noted, it's up to you.

> 
> > But that's up to you. I guess lane configuration etc. is part of the big
> > register lists.
> 
> Only 2 data lanes are currently supported by the driver.
> Bit 5 of 0x3039 in theory allows you to drop to 1 lane, but I've not
> got it to work. I suspect further clock tree changes are required.
> 
> A 400MHz link freq (800Mbit/s/lane) is already required for the max
> 1280x800@120fps.
> Dropping to 1 lane therefore either requires reducing the max frame
> rate, or potentially running at 800MHz link freq, which is well in
> excess of the earlier versions of CSI-2 spec (500MHz or 1Gbit/s). IMHO
> It's not worth pursuing.

Works for me.

-- 
Kind regards,

Sakari Ailus

[PATCH 09/16] media: i2c: ov9282: Add the properties from fwnode

[Dave Stevenson]

Use v4l2_ctrl_new_fwnode_properties to add V4L2_CID_CAMERA_ORIENTATION
and V4L2_CID_CAMERA_SENSOR_ROTATION.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 334b31af34a4..183283d191b1 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -989,10 +989,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 {
 	struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
 	const struct ov9282_mode *mode = ov9282->cur_mode;
+	struct v4l2_fwnode_device_properties props;
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 6);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
 	if (ret)
 		return ret;
 
@@ -1050,7 +1051,14 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	if (ov9282->hblank_ctrl)
 		ov9282->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-	if (ctrl_hdlr->error) {
+	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
+	if (!ret) {
+		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
+		v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov9282_ctrl_ops,
+						&props);
+	}
+
+	if (ctrl_hdlr->error || ret) {
 		dev_err(ov9282->dev, "control init failed: %d",
 			ctrl_hdlr->error);
 		v4l2_ctrl_handler_free(ctrl_hdlr);
-- 
2.34.1

Re: [PATCH 09/16] media: i2c: ov9282: Add the properties from fwnode

[Jacopo Mondi]

Hi Dave,

On Wed, Oct 05, 2022 at 04:28:02PM +0100, Dave Stevenson wrote:
> Use v4l2_ctrl_new_fwnode_properties to add V4L2_CID_CAMERA_ORIENTATION
> and V4L2_CID_CAMERA_SENSOR_ROTATION.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Seems like I forgot this patch
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  drivers/media/i2c/ov9282.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 334b31af34a4..183283d191b1 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -989,10 +989,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  {
>  	struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
>  	const struct ov9282_mode *mode = ov9282->cur_mode;
> +	struct v4l2_fwnode_device_properties props;
>  	u32 lpfr;
>  	int ret;
>
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 6);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
>  	if (ret)
>  		return ret;
>
> @@ -1050,7 +1051,14 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	if (ov9282->hblank_ctrl)
>  		ov9282->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> -	if (ctrl_hdlr->error) {
> +	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> +	if (!ret) {
> +		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> +		v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov9282_ctrl_ops,
> +						&props);
> +	}
> +
> +	if (ctrl_hdlr->error || ret) {
>  		dev_err(ov9282->dev, "control init failed: %d",
>  			ctrl_hdlr->error);
>  		v4l2_ctrl_handler_free(ctrl_hdlr);
> --
> 2.34.1
>

[PATCH 10/16] media: i2c: ov9282: Action CID_VBLANK when set.

[Dave Stevenson]

Programming the sensor with TIMING_VTS (aka LPFR) was done
when triggered by a change in exposure or gain, but not
when V4L2_CID_VBLANK was changed. Dynamic frame rate
changes could therefore not be achieved.

Separate out programming TIMING_VTS so that it is triggered
by set_ctrl(V4L2_CID_VBLANK)

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 183283d191b1..5ddef6e2b3ac 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -419,22 +419,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
  */
 static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 {
-	u32 lpfr;
 	int ret;
 
-	lpfr = ov9282->vblank + ov9282->cur_mode->height;
-
-	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u, lpfr %u",
-		exposure, gain, lpfr);
+	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
+		exposure, gain);
 
 	ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
 	if (ret)
 		return ret;
 
-	ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
-	if (ret)
-		goto error_release_group_hold;
-
 	ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4);
 	if (ret)
 		goto error_release_group_hold;
@@ -465,6 +458,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		container_of(ctrl->handler, struct ov9282, ctrl_handler);
 	u32 analog_gain;
 	u32 exposure;
+	u32 lpfr;
 	int ret;
 
 	switch (ctrl->id) {
@@ -482,10 +476,14 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 					       OV9282_EXPOSURE_OFFSET,
 					       1, OV9282_EXPOSURE_DEFAULT);
 		break;
+	}
+
+	/* Set controls only if sensor is in power on state */
+	if (!pm_runtime_get_if_in_use(ov9282->dev))
+		return 0;
+
+	switch (ctrl->id) {
 	case V4L2_CID_EXPOSURE:
-		/* Set controls only if sensor is in power on state */
-		if (!pm_runtime_get_if_in_use(ov9282->dev))
-			return 0;
 
 		exposure = ctrl->val;
 		analog_gain = ov9282->again_ctrl->val;
@@ -495,14 +493,19 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
 
-		pm_runtime_put(ov9282->dev);
 
+		break;
+	case V4L2_CID_VBLANK:
+		lpfr = ov9282->vblank + ov9282->cur_mode->height;
+		ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
 		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
 	}
 
+	pm_runtime_put(ov9282->dev);
+
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH 10/16] media: i2c: ov9282: Action CID_VBLANK when set.

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:28:03PM +0100, Dave Stevenson wrote:
> Programming the sensor with TIMING_VTS (aka LPFR) was done
> when triggered by a change in exposure or gain, but not
> when V4L2_CID_VBLANK was changed. Dynamic frame rate
> changes could therefore not be achieved.
>
> Separate out programming TIMING_VTS so that it is triggered
> by set_ctrl(V4L2_CID_VBLANK)
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 183283d191b1..5ddef6e2b3ac 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -419,22 +419,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
>   */
>  static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
>  {
> -	u32 lpfr;
>  	int ret;
>
> -	lpfr = ov9282->vblank + ov9282->cur_mode->height;
> -
> -	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u, lpfr %u",
> -		exposure, gain, lpfr);
> +	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
> +		exposure, gain);
>
>  	ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
>  	if (ret)
>  		return ret;
>
> -	ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
> -	if (ret)
> -		goto error_release_group_hold;
> -
>  	ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4);
>  	if (ret)
>  		goto error_release_group_hold;
> @@ -465,6 +458,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>  		container_of(ctrl->handler, struct ov9282, ctrl_handler);
>  	u32 analog_gain;
>  	u32 exposure;
> +	u32 lpfr;

Only a nit about the fact lpfr is a u32 while you're writing 2 bytes.
I guess it's safe as we likely don't risk any overflow

>  	int ret;
>
>  	switch (ctrl->id) {
> @@ -482,10 +476,14 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>  					       OV9282_EXPOSURE_OFFSET,
>  					       1, OV9282_EXPOSURE_DEFAULT);
>  		break;
> +	}
> +
> +	/* Set controls only if sensor is in power on state */
> +	if (!pm_runtime_get_if_in_use(ov9282->dev))
> +		return 0;
> +
> +	switch (ctrl->id) {
>  	case V4L2_CID_EXPOSURE:
> -		/* Set controls only if sensor is in power on state */
> -		if (!pm_runtime_get_if_in_use(ov9282->dev))
> -			return 0;
>
>  		exposure = ctrl->val;
>  		analog_gain = ov9282->again_ctrl->val;
> @@ -495,14 +493,19 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>
>  		ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
>
> -		pm_runtime_put(ov9282->dev);
>
Double empty line
With this fixed:

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j
> +		break;
> +	case V4L2_CID_VBLANK:
> +		lpfr = ov9282->vblank + ov9282->cur_mode->height;
> +		ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
>  		break;
>  	default:
>  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
>  		ret = -EINVAL;
>  	}
>
> +	pm_runtime_put(ov9282->dev);
> +
>  	return ret;
>  }
>
> --
> 2.34.1
>

Re: [PATCH 10/16] media: i2c: ov9282: Action CID_VBLANK when set.

[Dave Stevenson]

Hi Jacopo

On Thu, 6 Oct 2022 at 10:29, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:28:03PM +0100, Dave Stevenson wrote:
> > Programming the sensor with TIMING_VTS (aka LPFR) was done
> > when triggered by a change in exposure or gain, but not
> > when V4L2_CID_VBLANK was changed. Dynamic frame rate
> > changes could therefore not be achieved.
> >
> > Separate out programming TIMING_VTS so that it is triggered
> > by set_ctrl(V4L2_CID_VBLANK)
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 29 ++++++++++++++++-------------
> >  1 file changed, 16 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 183283d191b1..5ddef6e2b3ac 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -419,22 +419,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
> >   */
> >  static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> >  {
> > -     u32 lpfr;
> >       int ret;
> >
> > -     lpfr = ov9282->vblank + ov9282->cur_mode->height;
> > -
> > -     dev_dbg(ov9282->dev, "Set exp %u, analog gain %u, lpfr %u",
> > -             exposure, gain, lpfr);
> > +     dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
> > +             exposure, gain);
> >
> >       ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
> >       if (ret)
> >               return ret;
> >
> > -     ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
> > -     if (ret)
> > -             goto error_release_group_hold;
> > -
> >       ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4);
> >       if (ret)
> >               goto error_release_group_hold;
> > @@ -465,6 +458,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >               container_of(ctrl->handler, struct ov9282, ctrl_handler);
> >       u32 analog_gain;
> >       u32 exposure;
> > +     u32 lpfr;
>
> Only a nit about the fact lpfr is a u32 while you're writing 2 bytes.
> I guess it's safe as we likely don't risk any overflow

I was moving u32 lpfr from ov9282_update_exp_gain to here. All the
handling of lpfr (aka TIMING_VTS) is done as u32 in this driver.

The max range of V4L2_CID_VBLANK is set from the ov9282_mode
definition (not strictly necessary as it should be a fixed max value
handled by the register), so as long as the mode is defined correctly
then there should be no overflow.

  Dave

> >       int ret;
> >
> >       switch (ctrl->id) {
> > @@ -482,10 +476,14 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >                                              OV9282_EXPOSURE_OFFSET,
> >                                              1, OV9282_EXPOSURE_DEFAULT);
> >               break;
> > +     }
> > +
> > +     /* Set controls only if sensor is in power on state */
> > +     if (!pm_runtime_get_if_in_use(ov9282->dev))
> > +             return 0;
> > +
> > +     switch (ctrl->id) {
> >       case V4L2_CID_EXPOSURE:
> > -             /* Set controls only if sensor is in power on state */
> > -             if (!pm_runtime_get_if_in_use(ov9282->dev))
> > -                     return 0;
> >
> >               exposure = ctrl->val;
> >               analog_gain = ov9282->again_ctrl->val;
> > @@ -495,14 +493,19 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >
> >               ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
> >
> > -             pm_runtime_put(ov9282->dev);
> >
> Double empty line
> With this fixed:
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>    j
> > +             break;
> > +     case V4L2_CID_VBLANK:
> > +             lpfr = ov9282->vblank + ov9282->cur_mode->height;
> > +             ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
> >               break;
> >       default:
> >               dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >               ret = -EINVAL;
> >       }
> >
> > +     pm_runtime_put(ov9282->dev);
> > +
> >       return ret;
> >  }
> >
> > --
> > 2.34.1
> >

[PATCH 11/16] media: i2c: ov9282: Add HFLIP and VFLIP support

[Dave Stevenson]

Adds support for V4L2_CID_HFLIP and V4L2_CID_VFLIP to allow
flipping the image.

The driver previously enabled H & V flips in the register table,
therefore the controls default to the same settings to avoid
changing the behaviour.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 52 +++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 5ddef6e2b3ac..12cbe401fd78 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -46,6 +46,10 @@
 /* Group hold register */
 #define OV9282_REG_HOLD		0x3308
 
+#define OV9282_REG_TIMING_FORMAT_1	0x3820
+#define OV9282_REG_TIMING_FORMAT_2	0x3821
+#define OV9282_FLIP_BIT			BIT(2)
+
 #define OV9282_REG_MIPI_CTRL00	0x4800
 #define OV9282_GATED_CLOCK	BIT(5)
 
@@ -440,6 +444,38 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 	return ret;
 }
 
+static int ov9282_set_ctrl_hflip(struct ov9282 *ov9282, int value)
+{
+	u32 current_val;
+	int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
+				  &current_val);
+	if (!ret) {
+		if (value)
+			current_val |= OV9282_FLIP_BIT;
+		else
+			current_val &= ~OV9282_FLIP_BIT;
+		return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
+					current_val);
+	}
+	return ret;
+}
+
+static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
+{
+	u32 current_val;
+	int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
+				  &current_val);
+	if (!ret) {
+		if (value)
+			current_val |= OV9282_FLIP_BIT;
+		else
+			current_val &= ~OV9282_FLIP_BIT;
+		return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
+					current_val);
+	}
+	return ret;
+}
+
 /**
  * ov9282_set_ctrl() - Set subdevice control
  * @ctrl: pointer to v4l2_ctrl structure
@@ -484,7 +520,6 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_EXPOSURE:
-
 		exposure = ctrl->val;
 		analog_gain = ov9282->again_ctrl->val;
 
@@ -493,12 +528,17 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
 
-
 		break;
 	case V4L2_CID_VBLANK:
 		lpfr = ov9282->vblank + ov9282->cur_mode->height;
 		ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
 		break;
+	case V4L2_CID_HFLIP:
+		ret = ov9282_set_ctrl_hflip(ov9282, ctrl->val);
+		break;
+	case V4L2_CID_VFLIP:
+		ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -996,7 +1036,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
 	if (ret)
 		return ret;
 
@@ -1030,6 +1070,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 						mode->vblank_max,
 						1, mode->vblank);
 
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_VFLIP,
+			  0, 1, 1, 1);
+
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_HFLIP,
+			  0, 1, 1, 1);
+
 	/* Read only controls */
 	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
 			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
-- 
2.34.1

Re: [PATCH 11/16] media: i2c: ov9282: Add HFLIP and VFLIP support

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:28:04PM +0100, Dave Stevenson wrote:
> Adds support for V4L2_CID_HFLIP and V4L2_CID_VFLIP to allow
> flipping the image.
>
> The driver previously enabled H & V flips in the register table,
> therefore the controls default to the same settings to avoid
> changing the behaviour.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 52 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 5ddef6e2b3ac..12cbe401fd78 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -46,6 +46,10 @@
>  /* Group hold register */
>  #define OV9282_REG_HOLD		0x3308
>
> +#define OV9282_REG_TIMING_FORMAT_1	0x3820
> +#define OV9282_REG_TIMING_FORMAT_2	0x3821
> +#define OV9282_FLIP_BIT			BIT(2)
> +

Can we remove them from the list of common registers or do those
registers contains other settings which might get lost ?

>  #define OV9282_REG_MIPI_CTRL00	0x4800
>  #define OV9282_GATED_CLOCK	BIT(5)
>
> @@ -440,6 +444,38 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
>  	return ret;
>  }
>
> +static int ov9282_set_ctrl_hflip(struct ov9282 *ov9282, int value)
> +{
> +	u32 current_val;
> +	int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
> +				  &current_val);
> +	if (!ret) {
> +		if (value)
> +			current_val |= OV9282_FLIP_BIT;
> +		else
> +			current_val &= ~OV9282_FLIP_BIT;
> +		return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
> +					current_val);
> +	}
> +	return ret;

Or simply

        int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
                        &current_val);
        if (ret)
                return ret;

        if (value)
                current_val |= OV9282_FLIP_BIT;
        else
                current_val &= ~OV9282_FLIP_BIT;

        return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
                                current_val);


> +}
> +
> +static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
> +{
> +	u32 current_val;
> +	int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
> +				  &current_val);
> +	if (!ret) {
> +		if (value)
> +			current_val |= OV9282_FLIP_BIT;
> +		else
> +			current_val &= ~OV9282_FLIP_BIT;
> +		return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
> +					current_val);
> +	}
> +	return ret;
> +}
> +
>  /**
>   * ov9282_set_ctrl() - Set subdevice control
>   * @ctrl: pointer to v4l2_ctrl structure
> @@ -484,7 +520,6 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>
>  	switch (ctrl->id) {
>  	case V4L2_CID_EXPOSURE:
> -

Unrelated and possibly part of the previous patch ?

>  		exposure = ctrl->val;
>  		analog_gain = ov9282->again_ctrl->val;
>
> @@ -493,12 +528,17 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>
>  		ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
>
> -

same here

>  		break;
>  	case V4L2_CID_VBLANK:
>  		lpfr = ov9282->vblank + ov9282->cur_mode->height;
>  		ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
>  		break;
> +	case V4L2_CID_HFLIP:
> +		ret = ov9282_set_ctrl_hflip(ov9282, ctrl->val);
> +		break;
> +	case V4L2_CID_VFLIP:
> +		ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
> +		break;
>  	default:
>  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
>  		ret = -EINVAL;
> @@ -996,7 +1036,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	u32 lpfr;
>  	int ret;
>
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
>  	if (ret)
>  		return ret;
>
> @@ -1030,6 +1070,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  						mode->vblank_max,
>  						1, mode->vblank);
>
> +	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_VFLIP,
> +			  0, 1, 1, 1);
> +
> +	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_HFLIP,
> +			  0, 1, 1, 1);

By default 0x3820 and 0x3821 are initialized to 0x3c and 0x84 which
have BIT(2) set, so the image is "flipped" by default to compensate
the sensor mounting orientation. For users though the image appears
"not flipped", and if they have to set the control to 0 to flip it it
would feel a bit weird. Should the logic be inverted ? ie setting the
FLIP controls value to 1 results in BIT(2) being cleared ?

> +
>  	/* Read only controls */
>  	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
>  			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> --
> 2.34.1
>

Re: [PATCH 11/16] media: i2c: ov9282: Add HFLIP and VFLIP support

[Dave Stevenson]

Hi Jacopo

cc Sakari for policy view.

On Thu, 6 Oct 2022 at 10:38, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:28:04PM +0100, Dave Stevenson wrote:
> > Adds support for V4L2_CID_HFLIP and V4L2_CID_VFLIP to allow
> > flipping the image.
> >
> > The driver previously enabled H & V flips in the register table,
> > therefore the controls default to the same settings to avoid
> > changing the behaviour.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 52 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 49 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 5ddef6e2b3ac..12cbe401fd78 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -46,6 +46,10 @@
> >  /* Group hold register */
> >  #define OV9282_REG_HOLD              0x3308
> >
> > +#define OV9282_REG_TIMING_FORMAT_1   0x3820
> > +#define OV9282_REG_TIMING_FORMAT_2   0x3821
> > +#define OV9282_FLIP_BIT                      BIT(2)
> > +
>
> Can we remove them from the list of common registers or do those
> registers contains other settings which might get lost ?

They also contain the binning information, so generally best to keep
them in the mode list. (640x400 mode added later makes use of
binning).

> >  #define OV9282_REG_MIPI_CTRL00       0x4800
> >  #define OV9282_GATED_CLOCK   BIT(5)
> >
> > @@ -440,6 +444,38 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> >       return ret;
> >  }
> >
> > +static int ov9282_set_ctrl_hflip(struct ov9282 *ov9282, int value)
> > +{
> > +     u32 current_val;
> > +     int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
> > +                               &current_val);
> > +     if (!ret) {
> > +             if (value)
> > +                     current_val |= OV9282_FLIP_BIT;
> > +             else
> > +                     current_val &= ~OV9282_FLIP_BIT;
> > +             return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
> > +                                     current_val);
> > +     }
> > +     return ret;
>
> Or simply
>
>         int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
>                         &current_val);
>         if (ret)
>                 return ret;
>
>         if (value)
>                 current_val |= OV9282_FLIP_BIT;
>         else
>                 current_val &= ~OV9282_FLIP_BIT;
>
>         return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1,
>                                 current_val);
>

Done

> > +}
> > +
> > +static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
> > +{
> > +     u32 current_val;
> > +     int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
> > +                               &current_val);
> > +     if (!ret) {
> > +             if (value)
> > +                     current_val |= OV9282_FLIP_BIT;
> > +             else
> > +                     current_val &= ~OV9282_FLIP_BIT;
> > +             return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1,
> > +                                     current_val);
> > +     }
> > +     return ret;
> > +}
> > +
> >  /**
> >   * ov9282_set_ctrl() - Set subdevice control
> >   * @ctrl: pointer to v4l2_ctrl structure
> > @@ -484,7 +520,6 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >
> >       switch (ctrl->id) {
> >       case V4L2_CID_EXPOSURE:
> > -
>
> Unrelated and possibly part of the previous patch ?
>
> >               exposure = ctrl->val;
> >               analog_gain = ov9282->again_ctrl->val;
> >
> > @@ -493,12 +528,17 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >
> >               ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
> >
> > -
>
> same here
>
> >               break;
> >       case V4L2_CID_VBLANK:
> >               lpfr = ov9282->vblank + ov9282->cur_mode->height;
> >               ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr);
> >               break;
> > +     case V4L2_CID_HFLIP:
> > +             ret = ov9282_set_ctrl_hflip(ov9282, ctrl->val);
> > +             break;
> > +     case V4L2_CID_VFLIP:
> > +             ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
> > +             break;
> >       default:
> >               dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >               ret = -EINVAL;
> > @@ -996,7 +1036,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >       u32 lpfr;
> >       int ret;
> >
> > -     ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> > +     ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> >       if (ret)
> >               return ret;
> >
> > @@ -1030,6 +1070,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >                                               mode->vblank_max,
> >                                               1, mode->vblank);
> >
> > +     v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_VFLIP,
> > +                       0, 1, 1, 1);
> > +
> > +     v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_HFLIP,
> > +                       0, 1, 1, 1);
>
> By default 0x3820 and 0x3821 are initialized to 0x3c and 0x84 which
> have BIT(2) set, so the image is "flipped" by default to compensate
> the sensor mounting orientation. For users though the image appears
> "not flipped", and if they have to set the control to 0 to flip it it
> would feel a bit weird. Should the logic be inverted ? ie setting the
> FLIP controls value to 1 results in BIT(2) being cleared ?

This comes back to my comment at the Dublin Media Summit of wanting
fully featured drivers instead of product specific ones.

Drivers have been accepted with no flip controls and some form of
inversion buried in the registers. I'll acknowledge that restrictions
on datasheets makes it almost impossible for reviewers to pick up on
this, but we could push for flip controls to be mandatory in future if
the sensor supports it.
Looking at imx258 we again have a hardcoded rotation of 180degrees[1],
and it won't even probe if a "rotation" property isn't set to 180 [2].
It seems to be almost normal for the company who submitted both imx258
and ov9282 drivers that their sensors are inverted.

I guess the question really comes down to how HFLIP and VFLIP should
be defined. Are they relative to the platform, or relative to the
sensor vendor's definition? If relative to the platform, then do we
merge in V4L2_CID_CAMERA_SENSOR_ROTATION somehow?
With Bayer sensors it gets even more confusing as the flips typically
change the Bayer order, so someone referencing the datasheet will
start scratching their head over why they're getting BGGR when they
haven't asked for flips despite the datasheet saying the sensor is
RGGB.

Perhaps a more general discussion on how to handle these cases of
adding HFLIP and VFLIP is warranted.

If necessary then I'll drop this patch for now, but some path for
adding these features needs to be formulated. OV7251 is next on my
list to send patches, and then IMX258, so the subject will come up
again very soon.

  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L968
[2] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L1283

> > +
> >       /* Read only controls */
> >       v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
> >                         OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> > --
> > 2.34.1
> >

[PATCH 12/16] media: i2c: ov9282: Make V4L2_CID_HBLANK r/w

[Dave Stevenson]

There's no reason why HBLANK has to be read-only as it
only changes the TIMING_HTS register in the sensor.

Remove the READ_ONLY flag, and add the relevant handling
for it.

The minimum value also varies based on whether continuous clock
mode is being used or not, so allow hblank_min to depend on
that.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 12cbe401fd78..8e86aa7e4b2a 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -22,6 +22,9 @@
 #define OV9282_MODE_STANDBY	0x00
 #define OV9282_MODE_STREAMING	0x01
 
+#define OV9282_REG_TIMING_HTS	0x380c
+#define OV9282_TIMING_HTS_MAX	0x7fff
+
 /* Lines per frame */
 #define OV9282_REG_LPFR		0x380e
 
@@ -99,7 +102,8 @@ struct ov9282_reg_list {
  * struct ov9282_mode - ov9282 sensor mode structure
  * @width: Frame width
  * @height: Frame height
- * @hblank: Horizontal blanking in lines
+ * @hblank_min: Minimum horizontal blanking in lines for non-continuous[0] and
+ *		continuous[1] clock modes
  * @vblank: Vertical blanking in lines
  * @vblank_min: Minimum vertical blanking in lines
  * @vblank_max: Maximum vertical blanking in lines
@@ -109,7 +113,7 @@ struct ov9282_reg_list {
 struct ov9282_mode {
 	u32 width;
 	u32 height;
-	u32 hblank;
+	u32 hblank_min[2];
 	u32 vblank;
 	u32 vblank_min;
 	u32 vblank_max;
@@ -249,8 +253,6 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3809, 0x00},
 	{0x380a, 0x02},
 	{0x380b, 0xd0},
-	{0x380c, 0x02},
-	{0x380d, 0xfd},
 	{0x3810, 0x00},
 	{0x3811, 0x08},
 	{0x3812, 0x00},
@@ -273,7 +275,7 @@ static const struct ov9282_mode supported_modes[] = {
 	{
 		.width = 1280,
 		.height = 720,
-		.hblank = 250,
+		.hblank_min = { 250, 176 },
 		.vblank = 1022,
 		.vblank_min = 41,
 		.vblank_max = 51540,
@@ -399,15 +401,17 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
 static int ov9282_update_controls(struct ov9282 *ov9282,
 				  const struct ov9282_mode *mode)
 {
+	u32 hblank_min;
 	int ret;
 
 	ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
 	if (ret)
 		return ret;
 
-	ret = __v4l2_ctrl_s_ctrl(ov9282->hblank_ctrl, mode->hblank);
-	if (ret)
-		return ret;
+	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
+	ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
+					OV9282_TIMING_HTS_MAX - mode->width, 1,
+					hblank_min);
 
 	return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
 					mode->vblank_max, 1, mode->vblank);
@@ -539,6 +543,10 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_VFLIP:
 		ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
 		break;
+	case V4L2_CID_HBLANK:
+		ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
+				       (ctrl->val + ov9282->cur_mode->width) >> 1);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -1033,6 +1041,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
 	const struct ov9282_mode *mode = ov9282->cur_mode;
 	struct v4l2_fwnode_device_properties props;
+	u32 hblank_min;
 	u32 lpfr;
 	int ret;
 
@@ -1091,14 +1100,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	if (ov9282->link_freq_ctrl)
 		ov9282->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
+	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
 	ov9282->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
 						&ov9282_ctrl_ops,
 						V4L2_CID_HBLANK,
-						OV9282_REG_MIN,
-						OV9282_REG_MAX,
-						1, mode->hblank);
-	if (ov9282->hblank_ctrl)
-		ov9282->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+						hblank_min,
+						OV9282_TIMING_HTS_MAX - mode->width,
+						1, hblank_min);
 
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
-- 
2.34.1

Re: [PATCH 12/16] media: i2c: ov9282: Make V4L2_CID_HBLANK r/w

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:28:05PM +0100, Dave Stevenson wrote:
> There's no reason why HBLANK has to be read-only as it
> only changes the TIMING_HTS register in the sensor.
>
> Remove the READ_ONLY flag, and add the relevant handling
> for it.
>
> The minimum value also varies based on whether continuous clock
> mode is being used or not, so allow hblank_min to depend on
> that.

Interesting, do you know why they're different and why the continous
version is smaller ?

>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 12cbe401fd78..8e86aa7e4b2a 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -22,6 +22,9 @@
>  #define OV9282_MODE_STANDBY	0x00
>  #define OV9282_MODE_STREAMING	0x01
>
> +#define OV9282_REG_TIMING_HTS	0x380c
> +#define OV9282_TIMING_HTS_MAX	0x7fff
> +
>  /* Lines per frame */
>  #define OV9282_REG_LPFR		0x380e
>
> @@ -99,7 +102,8 @@ struct ov9282_reg_list {
>   * struct ov9282_mode - ov9282 sensor mode structure
>   * @width: Frame width
>   * @height: Frame height
> - * @hblank: Horizontal blanking in lines
> + * @hblank_min: Minimum horizontal blanking in lines for non-continuous[0] and
> + *		continuous[1] clock modes
>   * @vblank: Vertical blanking in lines
>   * @vblank_min: Minimum vertical blanking in lines
>   * @vblank_max: Maximum vertical blanking in lines
> @@ -109,7 +113,7 @@ struct ov9282_reg_list {
>  struct ov9282_mode {
>  	u32 width;
>  	u32 height;
> -	u32 hblank;
> +	u32 hblank_min[2];
>  	u32 vblank;
>  	u32 vblank_min;
>  	u32 vblank_max;
> @@ -249,8 +253,6 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x3809, 0x00},
>  	{0x380a, 0x02},
>  	{0x380b, 0xd0},
> -	{0x380c, 0x02},
> -	{0x380d, 0xfd},
>  	{0x3810, 0x00},
>  	{0x3811, 0x08},
>  	{0x3812, 0x00},
> @@ -273,7 +275,7 @@ static const struct ov9282_mode supported_modes[] = {
>  	{
>  		.width = 1280,
>  		.height = 720,
> -		.hblank = 250,
> +		.hblank_min = { 250, 176 },
>  		.vblank = 1022,
>  		.vblank_min = 41,
>  		.vblank_max = 51540,
> @@ -399,15 +401,17 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
>  static int ov9282_update_controls(struct ov9282 *ov9282,
>  				  const struct ov9282_mode *mode)
>  {
> +	u32 hblank_min;
>  	int ret;
>
>  	ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
>  	if (ret)
>  		return ret;
>
> -	ret = __v4l2_ctrl_s_ctrl(ov9282->hblank_ctrl, mode->hblank);
> -	if (ret)
> -		return ret;
> +	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> +	ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> +					OV9282_TIMING_HTS_MAX - mode->width, 1,
> +					hblank_min);
>
>  	return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
>  					mode->vblank_max, 1, mode->vblank);
> @@ -539,6 +543,10 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_VFLIP:
>  		ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
>  		break;
> +	case V4L2_CID_HBLANK:
> +		ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
> +				       (ctrl->val + ov9282->cur_mode->width) >> 1);
> +		break;
>  	default:
>  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
>  		ret = -EINVAL;
> @@ -1033,6 +1041,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
>  	const struct ov9282_mode *mode = ov9282->cur_mode;
>  	struct v4l2_fwnode_device_properties props;
> +	u32 hblank_min;
>  	u32 lpfr;
>  	int ret;
>
> @@ -1091,14 +1100,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	if (ov9282->link_freq_ctrl)
>  		ov9282->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> +	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
>  	ov9282->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
>  						&ov9282_ctrl_ops,
>  						V4L2_CID_HBLANK,
> -						OV9282_REG_MIN,
> -						OV9282_REG_MAX,
> -						1, mode->hblank);
> -	if (ov9282->hblank_ctrl)
> -		ov9282->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +						hblank_min,
> +						OV9282_TIMING_HTS_MAX - mode->width,
> +						1, hblank_min);
>
>  	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
>  	if (!ret) {
> --
> 2.34.1
>

Re: [PATCH 12/16] media: i2c: ov9282: Make V4L2_CID_HBLANK r/w

[Dave Stevenson]

Hi Jacopo.

On Thu, 6 Oct 2022 at 10:41, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:28:05PM +0100, Dave Stevenson wrote:
> > There's no reason why HBLANK has to be read-only as it
> > only changes the TIMING_HTS register in the sensor.
> >
> > Remove the READ_ONLY flag, and add the relevant handling
> > for it.
> >
> > The minimum value also varies based on whether continuous clock
> > mode is being used or not, so allow hblank_min to depend on
> > that.
>
> Interesting, do you know why they're different and why the continous
> version is smaller ?

Dropping from HS to LP-11 and back to HS for non-continuous mode takes
a minimum amount of time, so the time per line on the MIPI bus is
increased. Whilst there is a FIFO between the pixel array and MIPI
block, having to expend that time on the MIPI side means that the FIFO
can overflow.

I haven't found this behaviour documented, but I was trying to work
out the difference between this driver and our downstream driver [1]
in max frame rate. The default value for TIMING_HTS of 0x2d8 (doubled
to 1456 pixels) didn't work with this mainline driver, and it used
0x2fd (1530 pixels). The CSI clock mode turned out to be the
difference.
The minimum HTS has therefore been left alone for non-continuous clock
mode, and the lower setting added for continuous clock.

  Dave

[1] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/ov9281.c
based on https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/media/i2c/ov9281.c

> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 34 +++++++++++++++++++++-------------
> >  1 file changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 12cbe401fd78..8e86aa7e4b2a 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -22,6 +22,9 @@
> >  #define OV9282_MODE_STANDBY  0x00
> >  #define OV9282_MODE_STREAMING        0x01
> >
> > +#define OV9282_REG_TIMING_HTS        0x380c
> > +#define OV9282_TIMING_HTS_MAX        0x7fff
> > +
> >  /* Lines per frame */
> >  #define OV9282_REG_LPFR              0x380e
> >
> > @@ -99,7 +102,8 @@ struct ov9282_reg_list {
> >   * struct ov9282_mode - ov9282 sensor mode structure
> >   * @width: Frame width
> >   * @height: Frame height
> > - * @hblank: Horizontal blanking in lines
> > + * @hblank_min: Minimum horizontal blanking in lines for non-continuous[0] and
> > + *           continuous[1] clock modes
> >   * @vblank: Vertical blanking in lines
> >   * @vblank_min: Minimum vertical blanking in lines
> >   * @vblank_max: Maximum vertical blanking in lines
> > @@ -109,7 +113,7 @@ struct ov9282_reg_list {
> >  struct ov9282_mode {
> >       u32 width;
> >       u32 height;
> > -     u32 hblank;
> > +     u32 hblank_min[2];
> >       u32 vblank;
> >       u32 vblank_min;
> >       u32 vblank_max;
> > @@ -249,8 +253,6 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
> >       {0x3809, 0x00},
> >       {0x380a, 0x02},
> >       {0x380b, 0xd0},
> > -     {0x380c, 0x02},
> > -     {0x380d, 0xfd},
> >       {0x3810, 0x00},
> >       {0x3811, 0x08},
> >       {0x3812, 0x00},
> > @@ -273,7 +275,7 @@ static const struct ov9282_mode supported_modes[] = {
> >       {
> >               .width = 1280,
> >               .height = 720,
> > -             .hblank = 250,
> > +             .hblank_min = { 250, 176 },
> >               .vblank = 1022,
> >               .vblank_min = 41,
> >               .vblank_max = 51540,
> > @@ -399,15 +401,17 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
> >  static int ov9282_update_controls(struct ov9282 *ov9282,
> >                                 const struct ov9282_mode *mode)
> >  {
> > +     u32 hblank_min;
> >       int ret;
> >
> >       ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
> >       if (ret)
> >               return ret;
> >
> > -     ret = __v4l2_ctrl_s_ctrl(ov9282->hblank_ctrl, mode->hblank);
> > -     if (ret)
> > -             return ret;
> > +     hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> > +     ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> > +                                     OV9282_TIMING_HTS_MAX - mode->width, 1,
> > +                                     hblank_min);
> >
> >       return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
> >                                       mode->vblank_max, 1, mode->vblank);
> > @@ -539,6 +543,10 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >       case V4L2_CID_VFLIP:
> >               ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
> >               break;
> > +     case V4L2_CID_HBLANK:
> > +             ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
> > +                                    (ctrl->val + ov9282->cur_mode->width) >> 1);
> > +             break;
> >       default:
> >               dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >               ret = -EINVAL;
> > @@ -1033,6 +1041,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >       struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
> >       const struct ov9282_mode *mode = ov9282->cur_mode;
> >       struct v4l2_fwnode_device_properties props;
> > +     u32 hblank_min;
> >       u32 lpfr;
> >       int ret;
> >
> > @@ -1091,14 +1100,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >       if (ov9282->link_freq_ctrl)
> >               ov9282->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > +     hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> >       ov9282->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> >                                               &ov9282_ctrl_ops,
> >                                               V4L2_CID_HBLANK,
> > -                                             OV9282_REG_MIN,
> > -                                             OV9282_REG_MAX,
> > -                                             1, mode->hblank);
> > -     if (ov9282->hblank_ctrl)
> > -             ov9282->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +                                             hblank_min,
> > +                                             OV9282_TIMING_HTS_MAX - mode->width,
> > +                                             1, hblank_min);
> >
> >       ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> >       if (!ret) {
> > --
> > 2.34.1
> >

Re: [PATCH 12/16] media: i2c: ov9282: Make V4L2_CID_HBLANK r/w

[Jacopo Mondi]

Hi Dave,

On Thu, Oct 06, 2022 at 12:33:01PM +0100, Dave Stevenson wrote:
> Hi Jacopo.
>
> On Thu, 6 Oct 2022 at 10:41, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Dave
> >
> > On Wed, Oct 05, 2022 at 04:28:05PM +0100, Dave Stevenson wrote:
> > > There's no reason why HBLANK has to be read-only as it
> > > only changes the TIMING_HTS register in the sensor.
> > >
> > > Remove the READ_ONLY flag, and add the relevant handling
> > > for it.
> > >
> > > The minimum value also varies based on whether continuous clock
> > > mode is being used or not, so allow hblank_min to depend on
> > > that.
> >
> > Interesting, do you know why they're different and why the continous
> > version is smaller ?
>
> Dropping from HS to LP-11 and back to HS for non-continuous mode takes
> a minimum amount of time, so the time per line on the MIPI bus is
> increased. Whilst there is a FIFO between the pixel array and MIPI
> block, having to expend that time on the MIPI side means that the FIFO
> can overflow.
>
> I haven't found this behaviour documented, but I was trying to work
> out the difference between this driver and our downstream driver [1]
> in max frame rate. The default value for TIMING_HTS of 0x2d8 (doubled
> to 1456 pixels) didn't work with this mainline driver, and it used
> 0x2fd (1530 pixels). The CSI clock mode turned out to be the
> difference.
> The minimum HTS has therefore been left alone for non-continuous clock
> mode, and the lower setting added for continuous clock.
>

Makes sense! Thanks for the explanation

Please add my tag to v2
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>   Dave
>
> [1] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/ov9281.c
> based on https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/media/i2c/ov9281.c
>
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 34 +++++++++++++++++++++-------------
> > >  1 file changed, 21 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index 12cbe401fd78..8e86aa7e4b2a 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -22,6 +22,9 @@
> > >  #define OV9282_MODE_STANDBY  0x00
> > >  #define OV9282_MODE_STREAMING        0x01
> > >
> > > +#define OV9282_REG_TIMING_HTS        0x380c
> > > +#define OV9282_TIMING_HTS_MAX        0x7fff
> > > +
> > >  /* Lines per frame */
> > >  #define OV9282_REG_LPFR              0x380e
> > >
> > > @@ -99,7 +102,8 @@ struct ov9282_reg_list {
> > >   * struct ov9282_mode - ov9282 sensor mode structure
> > >   * @width: Frame width
> > >   * @height: Frame height
> > > - * @hblank: Horizontal blanking in lines
> > > + * @hblank_min: Minimum horizontal blanking in lines for non-continuous[0] and
> > > + *           continuous[1] clock modes
> > >   * @vblank: Vertical blanking in lines
> > >   * @vblank_min: Minimum vertical blanking in lines
> > >   * @vblank_max: Maximum vertical blanking in lines
> > > @@ -109,7 +113,7 @@ struct ov9282_reg_list {
> > >  struct ov9282_mode {
> > >       u32 width;
> > >       u32 height;
> > > -     u32 hblank;
> > > +     u32 hblank_min[2];
> > >       u32 vblank;
> > >       u32 vblank_min;
> > >       u32 vblank_max;
> > > @@ -249,8 +253,6 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
> > >       {0x3809, 0x00},
> > >       {0x380a, 0x02},
> > >       {0x380b, 0xd0},
> > > -     {0x380c, 0x02},
> > > -     {0x380d, 0xfd},
> > >       {0x3810, 0x00},
> > >       {0x3811, 0x08},
> > >       {0x3812, 0x00},
> > > @@ -273,7 +275,7 @@ static const struct ov9282_mode supported_modes[] = {
> > >       {
> > >               .width = 1280,
> > >               .height = 720,
> > > -             .hblank = 250,
> > > +             .hblank_min = { 250, 176 },
> > >               .vblank = 1022,
> > >               .vblank_min = 41,
> > >               .vblank_max = 51540,
> > > @@ -399,15 +401,17 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
> > >  static int ov9282_update_controls(struct ov9282 *ov9282,
> > >                                 const struct ov9282_mode *mode)
> > >  {
> > > +     u32 hblank_min;
> > >       int ret;
> > >
> > >       ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
> > >       if (ret)
> > >               return ret;
> > >
> > > -     ret = __v4l2_ctrl_s_ctrl(ov9282->hblank_ctrl, mode->hblank);
> > > -     if (ret)
> > > -             return ret;
> > > +     hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> > > +     ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> > > +                                     OV9282_TIMING_HTS_MAX - mode->width, 1,
> > > +                                     hblank_min);
> > >
> > >       return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
> > >                                       mode->vblank_max, 1, mode->vblank);
> > > @@ -539,6 +543,10 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> > >       case V4L2_CID_VFLIP:
> > >               ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
> > >               break;
> > > +     case V4L2_CID_HBLANK:
> > > +             ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
> > > +                                    (ctrl->val + ov9282->cur_mode->width) >> 1);
> > > +             break;
> > >       default:
> > >               dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> > >               ret = -EINVAL;
> > > @@ -1033,6 +1041,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > >       struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
> > >       const struct ov9282_mode *mode = ov9282->cur_mode;
> > >       struct v4l2_fwnode_device_properties props;
> > > +     u32 hblank_min;
> > >       u32 lpfr;
> > >       int ret;
> > >
> > > @@ -1091,14 +1100,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > >       if (ov9282->link_freq_ctrl)
> > >               ov9282->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >
> > > +     hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> > >       ov9282->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> > >                                               &ov9282_ctrl_ops,
> > >                                               V4L2_CID_HBLANK,
> > > -                                             OV9282_REG_MIN,
> > > -                                             OV9282_REG_MAX,
> > > -                                             1, mode->hblank);
> > > -     if (ov9282->hblank_ctrl)
> > > -             ov9282->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +                                             hblank_min,
> > > +                                             OV9282_TIMING_HTS_MAX - mode->width,
> > > +                                             1, hblank_min);
> > >
> > >       ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > >       if (!ret) {
> > > --
> > > 2.34.1
> > >

[PATCH 13/16] media: i2c: ov9282: Add selection API calls for cropping info

[Dave Stevenson]

As required by libcamera, add the relevant cropping targets
to report which portion of the sensor is being read out in
any mode.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 75 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 8e86aa7e4b2a..d892f53fb1ea 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -67,6 +67,17 @@
 #define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
 				 OV9282_NUM_DATA_LANES / 10)
 
+/*
+ * OV9282 native and active pixel array size.
+ * 8 dummy rows/columns on each edge of a 1280x800 active array
+ */
+#define OV9282_NATIVE_WIDTH		1296U
+#define OV9282_NATIVE_HEIGHT		816U
+#define OV9282_PIXEL_ARRAY_LEFT		8U
+#define OV9282_PIXEL_ARRAY_TOP		8U
+#define OV9282_PIXEL_ARRAY_WIDTH	1280U
+#define OV9282_PIXEL_ARRAY_HEIGHT	800U
+
 #define OV9282_REG_MIN		0x00
 #define OV9282_REG_MAX		0xfffff
 
@@ -118,6 +129,7 @@ struct ov9282_mode {
 	u32 vblank_min;
 	u32 vblank_max;
 	u32 link_freq_idx;
+	struct v4l2_rect crop;
 	struct ov9282_reg_list reg_list;
 };
 
@@ -280,6 +292,16 @@ static const struct ov9282_mode supported_modes[] = {
 		.vblank_min = 41,
 		.vblank_max = 51540,
 		.link_freq_idx = 0,
+		.crop = {
+			/*
+			 * Note that this mode takes the top 720 lines from the
+			 * 800 of the sensor. It does not take a middle crop.
+			 */
+			.left = OV9282_PIXEL_ARRAY_LEFT,
+			.top = OV9282_PIXEL_ARRAY_TOP,
+			.width = 1280,
+			.height = 720
+		},
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
 			.regs = mode_1280x720_regs,
@@ -719,6 +741,58 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
 	return ov9282_set_pad_format(sd, sd_state, &fmt);
 }
 
+static const struct v4l2_rect *
+__ov9282_get_pad_crop(struct ov9282 *ov9282,
+		      struct v4l2_subdev_state *sd_state,
+		      unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(&ov9282->sd, sd_state, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &ov9282->cur_mode->crop;
+	}
+
+	return NULL;
+}
+
+static int ov9282_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP: {
+		struct ov9282 *ov9282 = to_ov9282(sd);
+
+		mutex_lock(&ov9282->mutex);
+		sel->r = *__ov9282_get_pad_crop(ov9282, sd_state, sel->pad,
+						sel->which);
+		mutex_unlock(&ov9282->mutex);
+
+		return 0;
+	}
+
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV9282_NATIVE_WIDTH;
+		sel->r.height = OV9282_NATIVE_HEIGHT;
+
+		return 0;
+
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = OV9282_PIXEL_ARRAY_TOP;
+		sel->r.left = OV9282_PIXEL_ARRAY_LEFT;
+		sel->r.width = OV9282_PIXEL_ARRAY_WIDTH;
+		sel->r.height = OV9282_PIXEL_ARRAY_HEIGHT;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 /**
  * ov9282_start_streaming() - Start sensor stream
  * @ov9282: pointer to ov9282 device
@@ -963,6 +1037,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
 	.enum_frame_size = ov9282_enum_frame_size,
 	.get_fmt = ov9282_get_pad_format,
 	.set_fmt = ov9282_set_pad_format,
+	.get_selection = ov9282_get_selection,
 };
 
 static const struct v4l2_subdev_ops ov9282_subdev_ops = {
-- 
2.34.1

Re: [PATCH 13/16] media: i2c: ov9282: Add selection API calls for cropping info

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:28:06PM +0100, Dave Stevenson wrote:
> As required by libcamera, add the relevant cropping targets
> to report which portion of the sensor is being read out in
> any mode.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 75 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 8e86aa7e4b2a..d892f53fb1ea 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -67,6 +67,17 @@
>  #define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
>  				 OV9282_NUM_DATA_LANES / 10)
>
> +/*
> + * OV9282 native and active pixel array size.
> + * 8 dummy rows/columns on each edge of a 1280x800 active array
> + */
> +#define OV9282_NATIVE_WIDTH		1296U
> +#define OV9282_NATIVE_HEIGHT		816U
> +#define OV9282_PIXEL_ARRAY_LEFT		8U
> +#define OV9282_PIXEL_ARRAY_TOP		8U
> +#define OV9282_PIXEL_ARRAY_WIDTH	1280U
> +#define OV9282_PIXEL_ARRAY_HEIGHT	800U
> +
>  #define OV9282_REG_MIN		0x00
>  #define OV9282_REG_MAX		0xfffff
>
> @@ -118,6 +129,7 @@ struct ov9282_mode {
>  	u32 vblank_min;
>  	u32 vblank_max;
>  	u32 link_freq_idx;
> +	struct v4l2_rect crop;
>  	struct ov9282_reg_list reg_list;
>  };
>
> @@ -280,6 +292,16 @@ static const struct ov9282_mode supported_modes[] = {
>  		.vblank_min = 41,
>  		.vblank_max = 51540,
>  		.link_freq_idx = 0,
> +		.crop = {
> +			/*
> +			 * Note that this mode takes the top 720 lines from the
> +			 * 800 of the sensor. It does not take a middle crop.
> +			 */
> +			.left = OV9282_PIXEL_ARRAY_LEFT,
> +			.top = OV9282_PIXEL_ARRAY_TOP,
> +			.width = 1280,
> +			.height = 720
> +		},
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
>  			.regs = mode_1280x720_regs,
> @@ -719,6 +741,58 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
>  	return ov9282_set_pad_format(sd, sd_state, &fmt);
>  }
>
> +static const struct v4l2_rect *
> +__ov9282_get_pad_crop(struct ov9282 *ov9282,
> +		      struct v4l2_subdev_state *sd_state,
> +		      unsigned int pad, enum v4l2_subdev_format_whence which)
> +{
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		return v4l2_subdev_get_try_crop(&ov9282->sd, sd_state, pad);
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return &ov9282->cur_mode->crop;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int ov9282_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP: {
> +		struct ov9282 *ov9282 = to_ov9282(sd);
> +
> +		mutex_lock(&ov9282->mutex);

As there's no set_selection, do we need the mutex here ?

> +		sel->r = *__ov9282_get_pad_crop(ov9282, sd_state, sel->pad,
> +						sel->which);
> +		mutex_unlock(&ov9282->mutex);
> +
> +		return 0;
> +	}
> +
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = OV9282_NATIVE_WIDTH;
> +		sel->r.height = OV9282_NATIVE_HEIGHT;
> +
> +		return 0;
> +
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = OV9282_PIXEL_ARRAY_TOP;
> +		sel->r.left = OV9282_PIXEL_ARRAY_LEFT;
> +		sel->r.width = OV9282_PIXEL_ARRAY_WIDTH;
> +		sel->r.height = OV9282_PIXEL_ARRAY_HEIGHT;
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  /**
>   * ov9282_start_streaming() - Start sensor stream
>   * @ov9282: pointer to ov9282 device
> @@ -963,6 +1037,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
>  	.enum_frame_size = ov9282_enum_frame_size,
>  	.get_fmt = ov9282_get_pad_format,
>  	.set_fmt = ov9282_set_pad_format,
> +	.get_selection = ov9282_get_selection,
>  };
>
>  static const struct v4l2_subdev_ops ov9282_subdev_ops = {
> --
> 2.34.1
>

Re: [PATCH 13/16] media: i2c: ov9282: Add selection API calls for cropping info

[Dave Stevenson]

Hi Jacopo

Thanks for the review.

On Thu, 6 Oct 2022 at 10:43, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:28:06PM +0100, Dave Stevenson wrote:
> > As required by libcamera, add the relevant cropping targets
> > to report which portion of the sensor is being read out in
> > any mode.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 75 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 8e86aa7e4b2a..d892f53fb1ea 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -67,6 +67,17 @@
> >  #define OV9282_PIXEL_RATE    (OV9282_LINK_FREQ * 2 * \
> >                                OV9282_NUM_DATA_LANES / 10)
> >
> > +/*
> > + * OV9282 native and active pixel array size.
> > + * 8 dummy rows/columns on each edge of a 1280x800 active array
> > + */
> > +#define OV9282_NATIVE_WIDTH          1296U
> > +#define OV9282_NATIVE_HEIGHT         816U
> > +#define OV9282_PIXEL_ARRAY_LEFT              8U
> > +#define OV9282_PIXEL_ARRAY_TOP               8U
> > +#define OV9282_PIXEL_ARRAY_WIDTH     1280U
> > +#define OV9282_PIXEL_ARRAY_HEIGHT    800U
> > +
> >  #define OV9282_REG_MIN               0x00
> >  #define OV9282_REG_MAX               0xfffff
> >
> > @@ -118,6 +129,7 @@ struct ov9282_mode {
> >       u32 vblank_min;
> >       u32 vblank_max;
> >       u32 link_freq_idx;
> > +     struct v4l2_rect crop;
> >       struct ov9282_reg_list reg_list;
> >  };
> >
> > @@ -280,6 +292,16 @@ static const struct ov9282_mode supported_modes[] = {
> >               .vblank_min = 41,
> >               .vblank_max = 51540,
> >               .link_freq_idx = 0,
> > +             .crop = {
> > +                     /*
> > +                      * Note that this mode takes the top 720 lines from the
> > +                      * 800 of the sensor. It does not take a middle crop.
> > +                      */
> > +                     .left = OV9282_PIXEL_ARRAY_LEFT,
> > +                     .top = OV9282_PIXEL_ARRAY_TOP,
> > +                     .width = 1280,
> > +                     .height = 720
> > +             },
> >               .reg_list = {
> >                       .num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> >                       .regs = mode_1280x720_regs,
> > @@ -719,6 +741,58 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
> >       return ov9282_set_pad_format(sd, sd_state, &fmt);
> >  }
> >
> > +static const struct v4l2_rect *
> > +__ov9282_get_pad_crop(struct ov9282 *ov9282,
> > +                   struct v4l2_subdev_state *sd_state,
> > +                   unsigned int pad, enum v4l2_subdev_format_whence which)
> > +{
> > +     switch (which) {
> > +     case V4L2_SUBDEV_FORMAT_TRY:
> > +             return v4l2_subdev_get_try_crop(&ov9282->sd, sd_state, pad);
> > +     case V4L2_SUBDEV_FORMAT_ACTIVE:
> > +             return &ov9282->cur_mode->crop;
> > +     }
> > +
> > +     return NULL;
> > +}
> > +
> > +static int ov9282_get_selection(struct v4l2_subdev *sd,
> > +                             struct v4l2_subdev_state *sd_state,
> > +                             struct v4l2_subdev_selection *sel)
> > +{
> > +     switch (sel->target) {
> > +     case V4L2_SEL_TGT_CROP: {
> > +             struct ov9282 *ov9282 = to_ov9282(sd);
> > +
> > +             mutex_lock(&ov9282->mutex);
>
> As there's no set_selection, do we need the mutex here ?

__ov9282_get_pad_crop is looking at the current mode, so the mutex is
against ov9282_set_pad_format changing the mode.

You'll find the same pattern in imx214, imx219, ov5640, ov5647,
ov8865, and hopefully all other sensor drivers implementing
get_selection for V4L2_SEL_TGT_CROP.

  Dave

> > +             sel->r = *__ov9282_get_pad_crop(ov9282, sd_state, sel->pad,
> > +                                             sel->which);
> > +             mutex_unlock(&ov9282->mutex);
> > +
> > +             return 0;
> > +     }
> > +
> > +     case V4L2_SEL_TGT_NATIVE_SIZE:
> > +             sel->r.top = 0;
> > +             sel->r.left = 0;
> > +             sel->r.width = OV9282_NATIVE_WIDTH;
> > +             sel->r.height = OV9282_NATIVE_HEIGHT;
> > +
> > +             return 0;
> > +
> > +     case V4L2_SEL_TGT_CROP_DEFAULT:
> > +     case V4L2_SEL_TGT_CROP_BOUNDS:
> > +             sel->r.top = OV9282_PIXEL_ARRAY_TOP;
> > +             sel->r.left = OV9282_PIXEL_ARRAY_LEFT;
> > +             sel->r.width = OV9282_PIXEL_ARRAY_WIDTH;
> > +             sel->r.height = OV9282_PIXEL_ARRAY_HEIGHT;
> > +
> > +             return 0;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> >  /**
> >   * ov9282_start_streaming() - Start sensor stream
> >   * @ov9282: pointer to ov9282 device
> > @@ -963,6 +1037,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
> >       .enum_frame_size = ov9282_enum_frame_size,
> >       .get_fmt = ov9282_get_pad_format,
> >       .set_fmt = ov9282_set_pad_format,
> > +     .get_selection = ov9282_get_selection,
> >  };
> >
> >  static const struct v4l2_subdev_ops ov9282_subdev_ops = {
> > --
> > 2.34.1
> >

Re: [PATCH 13/16] media: i2c: ov9282: Add selection API calls for cropping info

[Jacopo Mondi]

Hi David

On Thu, Oct 06, 2022 at 12:39:35PM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> Thanks for the review.
>
> On Thu, 6 Oct 2022 at 10:43, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Dave
> >
> > On Wed, Oct 05, 2022 at 04:28:06PM +0100, Dave Stevenson wrote:
> > > As required by libcamera, add the relevant cropping targets
> > > to report which portion of the sensor is being read out in
> > > any mode.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 75 ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 75 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index 8e86aa7e4b2a..d892f53fb1ea 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -67,6 +67,17 @@
> > >  #define OV9282_PIXEL_RATE    (OV9282_LINK_FREQ * 2 * \
> > >                                OV9282_NUM_DATA_LANES / 10)
> > >
> > > +/*
> > > + * OV9282 native and active pixel array size.
> > > + * 8 dummy rows/columns on each edge of a 1280x800 active array
> > > + */
> > > +#define OV9282_NATIVE_WIDTH          1296U
> > > +#define OV9282_NATIVE_HEIGHT         816U
> > > +#define OV9282_PIXEL_ARRAY_LEFT              8U
> > > +#define OV9282_PIXEL_ARRAY_TOP               8U
> > > +#define OV9282_PIXEL_ARRAY_WIDTH     1280U
> > > +#define OV9282_PIXEL_ARRAY_HEIGHT    800U
> > > +
> > >  #define OV9282_REG_MIN               0x00
> > >  #define OV9282_REG_MAX               0xfffff
> > >
> > > @@ -118,6 +129,7 @@ struct ov9282_mode {
> > >       u32 vblank_min;
> > >       u32 vblank_max;
> > >       u32 link_freq_idx;
> > > +     struct v4l2_rect crop;
> > >       struct ov9282_reg_list reg_list;
> > >  };
> > >
> > > @@ -280,6 +292,16 @@ static const struct ov9282_mode supported_modes[] = {
> > >               .vblank_min = 41,
> > >               .vblank_max = 51540,
> > >               .link_freq_idx = 0,
> > > +             .crop = {
> > > +                     /*
> > > +                      * Note that this mode takes the top 720 lines from the
> > > +                      * 800 of the sensor. It does not take a middle crop.
> > > +                      */
> > > +                     .left = OV9282_PIXEL_ARRAY_LEFT,
> > > +                     .top = OV9282_PIXEL_ARRAY_TOP,
> > > +                     .width = 1280,
> > > +                     .height = 720
> > > +             },
> > >               .reg_list = {
> > >                       .num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> > >                       .regs = mode_1280x720_regs,
> > > @@ -719,6 +741,58 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
> > >       return ov9282_set_pad_format(sd, sd_state, &fmt);
> > >  }
> > >
> > > +static const struct v4l2_rect *
> > > +__ov9282_get_pad_crop(struct ov9282 *ov9282,
> > > +                   struct v4l2_subdev_state *sd_state,
> > > +                   unsigned int pad, enum v4l2_subdev_format_whence which)
> > > +{
> > > +     switch (which) {
> > > +     case V4L2_SUBDEV_FORMAT_TRY:
> > > +             return v4l2_subdev_get_try_crop(&ov9282->sd, sd_state, pad);
> > > +     case V4L2_SUBDEV_FORMAT_ACTIVE:
> > > +             return &ov9282->cur_mode->crop;
> > > +     }
> > > +
> > > +     return NULL;
> > > +}
> > > +
> > > +static int ov9282_get_selection(struct v4l2_subdev *sd,
> > > +                             struct v4l2_subdev_state *sd_state,
> > > +                             struct v4l2_subdev_selection *sel)
> > > +{
> > > +     switch (sel->target) {
> > > +     case V4L2_SEL_TGT_CROP: {
> > > +             struct ov9282 *ov9282 = to_ov9282(sd);
> > > +
> > > +             mutex_lock(&ov9282->mutex);
> >
> > As there's no set_selection, do we need the mutex here ?
>
> __ov9282_get_pad_crop is looking at the current mode, so the mutex is
> against ov9282_set_pad_format changing the mode.

Ah right

Please add my tag to v2
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
> You'll find the same pattern in imx214, imx219, ov5640, ov5647,
> ov8865, and hopefully all other sensor drivers implementing
> get_selection for V4L2_SEL_TGT_CROP.
>
>   Dave
>
> > > +             sel->r = *__ov9282_get_pad_crop(ov9282, sd_state, sel->pad,
> > > +                                             sel->which);
> > > +             mutex_unlock(&ov9282->mutex);
> > > +
> > > +             return 0;
> > > +     }
> > > +
> > > +     case V4L2_SEL_TGT_NATIVE_SIZE:
> > > +             sel->r.top = 0;
> > > +             sel->r.left = 0;
> > > +             sel->r.width = OV9282_NATIVE_WIDTH;
> > > +             sel->r.height = OV9282_NATIVE_HEIGHT;
> > > +
> > > +             return 0;
> > > +
> > > +     case V4L2_SEL_TGT_CROP_DEFAULT:
> > > +     case V4L2_SEL_TGT_CROP_BOUNDS:
> > > +             sel->r.top = OV9282_PIXEL_ARRAY_TOP;
> > > +             sel->r.left = OV9282_PIXEL_ARRAY_LEFT;
> > > +             sel->r.width = OV9282_PIXEL_ARRAY_WIDTH;
> > > +             sel->r.height = OV9282_PIXEL_ARRAY_HEIGHT;
> > > +
> > > +             return 0;
> > > +     }
> > > +
> > > +     return -EINVAL;
> > > +}
> > > +
> > >  /**
> > >   * ov9282_start_streaming() - Start sensor stream
> > >   * @ov9282: pointer to ov9282 device
> > > @@ -963,6 +1037,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
> > >       .enum_frame_size = ov9282_enum_frame_size,
> > >       .get_fmt = ov9282_get_pad_format,
> > >       .set_fmt = ov9282_set_pad_format,
> > > +     .get_selection = ov9282_get_selection,
> > >  };
> > >
> > >  static const struct v4l2_subdev_ops ov9282_subdev_ops = {
> > > --
> > > 2.34.1
> > >

[PATCH 14/16] media: i2c: ov9282: Add support for 1280x800 and 640x400 modes

[Dave Stevenson]

Adds register settings for additional modes.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 97 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index d892f53fb1ea..ec1599488f21 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -251,6 +251,37 @@ struct ov9282_reg_list common_regs_list = {
 };
 
 /* Sensor mode registers */
+static const struct ov9282_reg mode_1280x800_regs[] = {
+	{0x3778, 0x00},
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x05},
+	{0x3805, 0x0f},
+	{0x3806, 0x03},
+	{0x3807, 0x2f},
+	{0x3808, 0x05},
+	{0x3809, 0x00},
+	{0x380a, 0x03},
+	{0x380b, 0x20},
+	{0x3810, 0x00},
+	{0x3811, 0x08},
+	{0x3812, 0x00},
+	{0x3813, 0x08},
+	{0x3814, 0x11},
+	{0x3815, 0x11},
+	{0x3820, 0x40},
+	{0x3821, 0x00},
+	{0x4003, 0x40},
+	{0x4008, 0x04},
+	{0x4009, 0x0b},
+	{0x400c, 0x00},
+	{0x400d, 0x07},
+	{0x4507, 0x00},
+	{0x4509, 0x00},
+};
+
 static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x3778, 0x00},
 	{0x3800, 0x00},
@@ -282,6 +313,36 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
 	{0x4509, 0x80},
 };
 
+static const struct ov9282_reg mode_640x400_regs[] = {
+	{0x3778, 0x10},
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x05},
+	{0x3805, 0x0f},
+	{0x3806, 0x03},
+	{0x3807, 0x2f},
+	{0x3808, 0x02},
+	{0x3809, 0x80},
+	{0x380a, 0x01},
+	{0x380b, 0x90},
+	{0x3810, 0x00},
+	{0x3811, 0x04},
+	{0x3812, 0x00},
+	{0x3813, 0x04},
+	{0x3814, 0x31},
+	{0x3815, 0x22},
+	{0x3820, 0x60},
+	{0x3821, 0x01},
+	{0x4008, 0x02},
+	{0x4009, 0x05},
+	{0x400c, 0x00},
+	{0x400d, 0x03},
+	{0x4507, 0x03},
+	{0x4509, 0x80},
+};
+
 /* Supported sensor mode configurations */
 static const struct ov9282_mode supported_modes[] = {
 	{
@@ -306,6 +367,42 @@ static const struct ov9282_mode supported_modes[] = {
 			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
 			.regs = mode_1280x720_regs,
 		},
+	}, {
+		.width = 1280,
+		.height = 800,
+		.hblank_min = { 250, 176 },
+		.vblank = 1022,
+		.vblank_min = 110,
+		.vblank_max = 51540,
+		.link_freq_idx = 0,
+		.crop = {
+			.left = OV9282_PIXEL_ARRAY_LEFT,
+			.top = OV9282_PIXEL_ARRAY_TOP,
+			.width = 1280,
+			.height = 800
+		},
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1280x800_regs),
+			.regs = mode_1280x800_regs,
+		},
+	}, {
+		.width = 640,
+		.height = 400,
+		.hblank_min = { 890, 816 },
+		.vblank = 1022,
+		.vblank_min = 22,
+		.vblank_max = 51540,
+		.link_freq_idx = 0,
+		.crop = {
+			.left = OV9282_PIXEL_ARRAY_LEFT,
+			.top = OV9282_PIXEL_ARRAY_TOP,
+			.width = 1280,
+			.height = 800
+		},
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_640x400_regs),
+			.regs = mode_640x400_regs,
+		},
 	},
 };
 
-- 
2.34.1

Re: [PATCH 14/16] media: i2c: ov9282: Add support for 1280x800 and 640x400 modes

[Jacopo Mondi]

On Wed, Oct 05, 2022 at 04:28:07PM +0100, Dave Stevenson wrote:
> Adds register settings for additional modes.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 97 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index d892f53fb1ea..ec1599488f21 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -251,6 +251,37 @@ struct ov9282_reg_list common_regs_list = {
>  };
>
>  /* Sensor mode registers */
> +static const struct ov9282_reg mode_1280x800_regs[] = {
> +	{0x3778, 0x00},
> +	{0x3800, 0x00},
> +	{0x3801, 0x00},
> +	{0x3802, 0x00},
> +	{0x3803, 0x00},
> +	{0x3804, 0x05},
> +	{0x3805, 0x0f},
> +	{0x3806, 0x03},
> +	{0x3807, 0x2f},
> +	{0x3808, 0x05},
> +	{0x3809, 0x00},
> +	{0x380a, 0x03},
> +	{0x380b, 0x20},
> +	{0x3810, 0x00},
> +	{0x3811, 0x08},
> +	{0x3812, 0x00},
> +	{0x3813, 0x08},
> +	{0x3814, 0x11},
> +	{0x3815, 0x11},
> +	{0x3820, 0x40},
> +	{0x3821, 0x00},
> +	{0x4003, 0x40},
> +	{0x4008, 0x04},
> +	{0x4009, 0x0b},
> +	{0x400c, 0x00},
> +	{0x400d, 0x07},
> +	{0x4507, 0x00},
> +	{0x4509, 0x00},
> +};
> +
>  static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x3778, 0x00},
>  	{0x3800, 0x00},
> @@ -282,6 +313,36 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
>  	{0x4509, 0x80},
>  };
>
> +static const struct ov9282_reg mode_640x400_regs[] = {
> +	{0x3778, 0x10},
> +	{0x3800, 0x00},
> +	{0x3801, 0x00},
> +	{0x3802, 0x00},
> +	{0x3803, 0x00},
> +	{0x3804, 0x05},
> +	{0x3805, 0x0f},

I assume this mode is then binned as the analog crop rectangle is the
same as the other modes, right ?

> +	{0x3806, 0x03},
> +	{0x3807, 0x2f},
> +	{0x3808, 0x02},
> +	{0x3809, 0x80},
> +	{0x380a, 0x01},
> +	{0x380b, 0x90},
> +	{0x3810, 0x00},
> +	{0x3811, 0x04},
> +	{0x3812, 0x00},
> +	{0x3813, 0x04},
> +	{0x3814, 0x31},
> +	{0x3815, 0x22},
> +	{0x3820, 0x60},
> +	{0x3821, 0x01},
> +	{0x4008, 0x02},
> +	{0x4009, 0x05},
> +	{0x400c, 0x00},
> +	{0x400d, 0x03},
> +	{0x4507, 0x03},
> +	{0x4509, 0x80},
> +};
> +
>  /* Supported sensor mode configurations */
>  static const struct ov9282_mode supported_modes[] = {
>  	{
> @@ -306,6 +367,42 @@ static const struct ov9282_mode supported_modes[] = {
>  			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
>  			.regs = mode_1280x720_regs,
>  		},
> +	}, {
> +		.width = 1280,
> +		.height = 800,
> +		.hblank_min = { 250, 176 },
> +		.vblank = 1022,
> +		.vblank_min = 110,
> +		.vblank_max = 51540,
> +		.link_freq_idx = 0,
> +		.crop = {
> +			.left = OV9282_PIXEL_ARRAY_LEFT,
> +			.top = OV9282_PIXEL_ARRAY_TOP,
> +			.width = 1280,
> +			.height = 800
> +		},
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1280x800_regs),
> +			.regs = mode_1280x800_regs,
> +		},
> +	}, {
> +		.width = 640,
> +		.height = 400,
> +		.hblank_min = { 890, 816 },
> +		.vblank = 1022,
> +		.vblank_min = 22,
> +		.vblank_max = 51540,
> +		.link_freq_idx = 0,
> +		.crop = {
> +			.left = OV9282_PIXEL_ARRAY_LEFT,
> +			.top = OV9282_PIXEL_ARRAY_TOP,
> +			.width = 1280,
> +			.height = 800
> +		},

This seems to confirm it.

I would have ordered them, but this will change the default mode
unless we define its index and use it at initialization time

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_640x400_regs),
> +			.regs = mode_640x400_regs,
> +		},
>  	},
>  };
>
> --
> 2.34.1
>

Re: [PATCH 14/16] media: i2c: ov9282: Add support for 1280x800 and 640x400 modes

[Dave Stevenson]

Hi Jacopo

Thanks for the review

On Thu, 6 Oct 2022 at 10:48, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> On Wed, Oct 05, 2022 at 04:28:07PM +0100, Dave Stevenson wrote:
> > Adds register settings for additional modes.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 97 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 97 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index d892f53fb1ea..ec1599488f21 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -251,6 +251,37 @@ struct ov9282_reg_list common_regs_list = {
> >  };
> >
> >  /* Sensor mode registers */
> > +static const struct ov9282_reg mode_1280x800_regs[] = {
> > +     {0x3778, 0x00},
> > +     {0x3800, 0x00},
> > +     {0x3801, 0x00},
> > +     {0x3802, 0x00},
> > +     {0x3803, 0x00},
> > +     {0x3804, 0x05},
> > +     {0x3805, 0x0f},
> > +     {0x3806, 0x03},
> > +     {0x3807, 0x2f},
> > +     {0x3808, 0x05},
> > +     {0x3809, 0x00},
> > +     {0x380a, 0x03},
> > +     {0x380b, 0x20},
> > +     {0x3810, 0x00},
> > +     {0x3811, 0x08},
> > +     {0x3812, 0x00},
> > +     {0x3813, 0x08},
> > +     {0x3814, 0x11},
> > +     {0x3815, 0x11},
> > +     {0x3820, 0x40},
> > +     {0x3821, 0x00},
> > +     {0x4003, 0x40},
> > +     {0x4008, 0x04},
> > +     {0x4009, 0x0b},
> > +     {0x400c, 0x00},
> > +     {0x400d, 0x07},
> > +     {0x4507, 0x00},
> > +     {0x4509, 0x00},
> > +};
> > +
> >  static const struct ov9282_reg mode_1280x720_regs[] = {
> >       {0x3778, 0x00},
> >       {0x3800, 0x00},
> > @@ -282,6 +313,36 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
> >       {0x4509, 0x80},
> >  };
> >
> > +static const struct ov9282_reg mode_640x400_regs[] = {
> > +     {0x3778, 0x10},
> > +     {0x3800, 0x00},
> > +     {0x3801, 0x00},
> > +     {0x3802, 0x00},
> > +     {0x3803, 0x00},
> > +     {0x3804, 0x05},
> > +     {0x3805, 0x0f},
>
> I assume this mode is then binned as the analog crop rectangle is the
> same as the other modes, right ?

Yes, it is binned.
0x3820 bit 1 controls vertical binning. 0x3821 bit 0 is horizontal
binning. These are the same registers that control flips, hence the
flip handler having to read-modify-write.

> > +     {0x3806, 0x03},
> > +     {0x3807, 0x2f},
> > +     {0x3808, 0x02},
> > +     {0x3809, 0x80},
> > +     {0x380a, 0x01},
> > +     {0x380b, 0x90},
> > +     {0x3810, 0x00},
> > +     {0x3811, 0x04},
> > +     {0x3812, 0x00},
> > +     {0x3813, 0x04},
> > +     {0x3814, 0x31},
> > +     {0x3815, 0x22},
> > +     {0x3820, 0x60},
> > +     {0x3821, 0x01},
> > +     {0x4008, 0x02},
> > +     {0x4009, 0x05},
> > +     {0x400c, 0x00},
> > +     {0x400d, 0x03},
> > +     {0x4507, 0x03},
> > +     {0x4509, 0x80},
> > +};
> > +
> >  /* Supported sensor mode configurations */
> >  static const struct ov9282_mode supported_modes[] = {
> >       {
> > @@ -306,6 +367,42 @@ static const struct ov9282_mode supported_modes[] = {
> >                       .num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> >                       .regs = mode_1280x720_regs,
> >               },
> > +     }, {
> > +             .width = 1280,
> > +             .height = 800,
> > +             .hblank_min = { 250, 176 },
> > +             .vblank = 1022,
> > +             .vblank_min = 110,
> > +             .vblank_max = 51540,
> > +             .link_freq_idx = 0,
> > +             .crop = {
> > +                     .left = OV9282_PIXEL_ARRAY_LEFT,
> > +                     .top = OV9282_PIXEL_ARRAY_TOP,
> > +                     .width = 1280,
> > +                     .height = 800
> > +             },
> > +             .reg_list = {
> > +                     .num_of_regs = ARRAY_SIZE(mode_1280x800_regs),
> > +                     .regs = mode_1280x800_regs,
> > +             },
> > +     }, {
> > +             .width = 640,
> > +             .height = 400,
> > +             .hblank_min = { 890, 816 },
> > +             .vblank = 1022,
> > +             .vblank_min = 22,
> > +             .vblank_max = 51540,
> > +             .link_freq_idx = 0,
> > +             .crop = {
> > +                     .left = OV9282_PIXEL_ARRAY_LEFT,
> > +                     .top = OV9282_PIXEL_ARRAY_TOP,
> > +                     .width = 1280,
> > +                     .height = 800
> > +             },
>
> This seems to confirm it.
>
> I would have ordered them, but this will change the default mode
> unless we define its index and use it at initialization time

Yes, I didn't want to change the default mode, although why the
existing mode was a non-centred crop I have no idea!

I'm happy to add a define or enum for each mode, initialise this table
using them, and then have a define for which mode is the default. That
removes any ambiguity, and means the table can be ordered.

  Dave
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>   j
>
> > +             .reg_list = {
> > +                     .num_of_regs = ARRAY_SIZE(mode_640x400_regs),
> > +                     .regs = mode_640x400_regs,
> > +             },
> >       },
> >  };
> >
> > --
> > 2.34.1
> >

[PATCH 15/16] media: i2c: ov9282: Add support for 8bit readout

[Dave Stevenson]

The sensor supports 8 or 10 bit readout modes, but the
driver only supported 10 bit.

Add 8 bit readout.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 110 ++++++++++++++++++++++++++++++-------
 1 file changed, 89 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index ec1599488f21..bc429455421e 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -22,6 +22,10 @@
 #define OV9282_MODE_STANDBY	0x00
 #define OV9282_MODE_STREAMING	0x01
 
+#define OV9282_REG_PLL_CTRL_0D	0x030d
+#define OV9282_PLL_CTRL_0D_RAW8		0x60
+#define OV9282_PLL_CTRL_0D_RAW10	0x50
+
 #define OV9282_REG_TIMING_HTS	0x380c
 #define OV9282_TIMING_HTS_MAX	0x7fff
 
@@ -49,6 +53,10 @@
 /* Group hold register */
 #define OV9282_REG_HOLD		0x3308
 
+#define OV9282_REG_ANA_CORE_2	0x3662
+#define OV9282_ANA_CORE2_RAW8	0x07
+#define OV9282_ANA_CORE2_RAW10	0x05
+
 #define OV9282_REG_TIMING_FORMAT_1	0x3820
 #define OV9282_REG_TIMING_FORMAT_2	0x3821
 #define OV9282_FLIP_BIT			BIT(2)
@@ -64,8 +72,10 @@
 #define OV9282_NUM_DATA_LANES	2
 
 /* Pixel rate */
-#define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
-				 OV9282_NUM_DATA_LANES / 10)
+#define OV9282_PIXEL_RATE_10BIT		(OV9282_LINK_FREQ * 2 * \
+					 OV9282_NUM_DATA_LANES / 10)
+#define OV9282_PIXEL_RATE_8BIT		(OV9282_LINK_FREQ * 2 * \
+					 OV9282_NUM_DATA_LANES / 8)
 
 /*
  * OV9282 native and active pixel array size.
@@ -149,6 +159,7 @@ struct ov9282_mode {
  * @again_ctrl: Pointer to analog gain control
  * @vblank: Vertical blanking in lines
  * @cur_mode: Pointer to current selected sensor mode
+ * @code: Mbus code currently selected
  * @mutex: Mutex for serializing sensor controls
  * @streaming: Flag indicating streaming state
  */
@@ -169,8 +180,10 @@ struct ov9282 {
 		struct v4l2_ctrl *exp_ctrl;
 		struct v4l2_ctrl *again_ctrl;
 	};
+	struct v4l2_ctrl *pixel_rate;
 	u32 vblank;
 	const struct ov9282_mode *cur_mode;
+	u32 code;
 	struct mutex mutex;
 	bool streaming;
 };
@@ -182,7 +195,6 @@ static const s64 link_freq[] = {
 /* Common registers */
 static const struct ov9282_reg common_regs[] = {
 	{0x0302, 0x32},
-	{0x030d, 0x50},
 	{0x030e, 0x02},
 	{0x3001, 0x00},
 	{0x3004, 0x00},
@@ -514,23 +526,41 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
  * ov9282_update_controls() - Update control ranges based on streaming mode
  * @ov9282: pointer to ov9282 device
  * @mode: pointer to ov9282_mode sensor mode
+ * @fmt: pointer to the requested mode
  *
  * Return: 0 if successful, error code otherwise.
  */
 static int ov9282_update_controls(struct ov9282 *ov9282,
-				  const struct ov9282_mode *mode)
+				  const struct ov9282_mode *mode,
+				  const struct v4l2_subdev_format *fmt)
 {
 	u32 hblank_min;
+	s64 pixel_rate;
 	int ret;
 
 	ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
 	if (ret)
 		return ret;
 
-	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
-	ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
-					OV9282_TIMING_HTS_MAX - mode->width, 1,
-					hblank_min);
+	pixel_rate = (fmt->format.code == MEDIA_BUS_FMT_Y10_1X10) ?
+		OV9282_PIXEL_RATE_10BIT : OV9282_PIXEL_RATE_8BIT;
+	ret = __v4l2_ctrl_modify_range(ov9282->pixel_rate, pixel_rate,
+				       pixel_rate, 1, pixel_rate);
+	if (ret)
+		return ret;
+
+	if (ov9282->cur_mode != mode) {
+		hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
+		ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
+						OV9282_TIMING_HTS_MAX - mode->width, 1,
+						hblank_min);
+		if (ret)
+			return ret;
+
+		ret =  __v4l2_ctrl_s_ctrl(ov9282->hblank_ctrl, hblank_min);
+		if (ret)
+			return ret;
+	}
 
 	return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
 					mode->vblank_max, 1, mode->vblank);
@@ -693,10 +723,16 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (code->index > 0)
+	switch (code->index) {
+	default:
 		return -EINVAL;
-
-	code->code = MEDIA_BUS_FMT_Y10_1X10;
+	case 0:
+		code->code = MEDIA_BUS_FMT_Y10_1X10;
+		break;
+	case 1:
+		code->code = MEDIA_BUS_FMT_Y8_1X8;
+		break;
+	}
 
 	return 0;
 }
@@ -716,7 +752,8 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
 	if (fsize->index >= OV9282_NUM_MODES)
 		return -EINVAL;
 
-	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
+	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10 &&
+	    fsize->code != MEDIA_BUS_FMT_Y8_1X8)
 		return -EINVAL;
 
 	fsize->min_width = supported_modes[fsize->index].width;
@@ -732,15 +769,17 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
  *                            from selected sensor mode
  * @ov9282: pointer to ov9282 device
  * @mode: pointer to ov9282_mode sensor mode
+ * @code: mbus code to be stored
  * @fmt: V4L2 sub-device format need to be filled
  */
 static void ov9282_fill_pad_format(struct ov9282 *ov9282,
 				   const struct ov9282_mode *mode,
+				   u32 code,
 				   struct v4l2_subdev_format *fmt)
 {
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
-	fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
+	fmt->format.code = code;
 	fmt->format.field = V4L2_FIELD_NONE;
 	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
 	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
@@ -770,7 +809,8 @@ static int ov9282_get_pad_format(struct v4l2_subdev *sd,
 		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
 		fmt->format = *framefmt;
 	} else {
-		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, fmt);
+		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, ov9282->code,
+				       fmt);
 	}
 
 	mutex_unlock(&ov9282->mutex);
@@ -792,6 +832,7 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
 {
 	struct ov9282 *ov9282 = to_ov9282(sd);
 	const struct ov9282_mode *mode;
+	u32 code;
 	int ret = 0;
 
 	mutex_lock(&ov9282->mutex);
@@ -801,7 +842,12 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
 				      width, height,
 				      fmt->format.width,
 				      fmt->format.height);
-	ov9282_fill_pad_format(ov9282, mode, fmt);
+	if (fmt->format.code == MEDIA_BUS_FMT_Y8_1X8)
+		code = MEDIA_BUS_FMT_Y8_1X8;
+	else
+		code = MEDIA_BUS_FMT_Y10_1X10;
+
+	ov9282_fill_pad_format(ov9282, mode, code, fmt);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
 		struct v4l2_mbus_framefmt *framefmt;
@@ -809,9 +855,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
 		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
 		*framefmt = fmt->format;
 	} else {
-		ret = ov9282_update_controls(ov9282, mode);
-		if (!ret)
+		ret = ov9282_update_controls(ov9282, mode, fmt);
+		if (!ret) {
 			ov9282->cur_mode = mode;
+			ov9282->code = code;
+		}
 	}
 
 	mutex_unlock(&ov9282->mutex);
@@ -833,7 +881,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
 	struct v4l2_subdev_format fmt = { 0 };
 
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	ov9282_fill_pad_format(ov9282, &supported_modes[0], &fmt);
+	ov9282_fill_pad_format(ov9282, &supported_modes[0], ov9282->code, &fmt);
 
 	return ov9282_set_pad_format(sd, sd_state, &fmt);
 }
@@ -898,7 +946,17 @@ static int ov9282_get_selection(struct v4l2_subdev *sd,
  */
 static int ov9282_start_streaming(struct ov9282 *ov9282)
 {
+	const struct ov9282_reg bitdepth_regs[2][2] = {
+		{
+			{OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW10},
+			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10},
+		}, {
+			{OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW8},
+			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
+		}
+	};
 	const struct ov9282_reg_list *reg_list;
+	int bitdepth_index;
 	int ret;
 
 	/* Write common registers */
@@ -917,6 +975,13 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
 		return ret;
 	}
 
+	bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
+	ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2);
+	if (ret) {
+		dev_err(ov9282->dev, "fail to write bitdepth regs");
+		return ret;
+	}
+
 	/* Write sensor mode registers */
 	reg_list = &ov9282->cur_mode->reg_list;
 	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
@@ -1258,9 +1323,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 			  0, 1, 1, 1);
 
 	/* Read only controls */
-	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
-			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
-			  OV9282_PIXEL_RATE);
+	ov9282->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
+					       V4L2_CID_PIXEL_RATE,
+					       OV9282_PIXEL_RATE_10BIT,
+					       OV9282_PIXEL_RATE_10BIT, 1,
+					       OV9282_PIXEL_RATE_10BIT);
 
 	ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 							&ov9282_ctrl_ops,
@@ -1342,6 +1409,7 @@ static int ov9282_probe(struct i2c_client *client)
 
 	/* Set default mode to first mode */
 	ov9282->cur_mode = &supported_modes[0];
+	ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
 	ov9282->vblank = ov9282->cur_mode->vblank;
 
 	ret = ov9282_init_controls(ov9282);
-- 
2.34.1

Re: [PATCH 15/16] media: i2c: ov9282: Add support for 8bit readout

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:28:08PM +0100, Dave Stevenson wrote:
> The sensor supports 8 or 10 bit readout modes, but the
> driver only supported 10 bit.
>
> Add 8 bit readout.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 110 ++++++++++++++++++++++++++++++-------
>  1 file changed, 89 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index ec1599488f21..bc429455421e 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -22,6 +22,10 @@
>  #define OV9282_MODE_STANDBY	0x00
>  #define OV9282_MODE_STREAMING	0x01
>
> +#define OV9282_REG_PLL_CTRL_0D	0x030d
> +#define OV9282_PLL_CTRL_0D_RAW8		0x60
> +#define OV9282_PLL_CTRL_0D_RAW10	0x50
> +
>  #define OV9282_REG_TIMING_HTS	0x380c
>  #define OV9282_TIMING_HTS_MAX	0x7fff
>
> @@ -49,6 +53,10 @@
>  /* Group hold register */
>  #define OV9282_REG_HOLD		0x3308
>
> +#define OV9282_REG_ANA_CORE_2	0x3662
> +#define OV9282_ANA_CORE2_RAW8	0x07
> +#define OV9282_ANA_CORE2_RAW10	0x05
> +
>  #define OV9282_REG_TIMING_FORMAT_1	0x3820
>  #define OV9282_REG_TIMING_FORMAT_2	0x3821
>  #define OV9282_FLIP_BIT			BIT(2)
> @@ -64,8 +72,10 @@
>  #define OV9282_NUM_DATA_LANES	2
>
>  /* Pixel rate */
> -#define OV9282_PIXEL_RATE	(OV9282_LINK_FREQ * 2 * \
> -				 OV9282_NUM_DATA_LANES / 10)
> +#define OV9282_PIXEL_RATE_10BIT		(OV9282_LINK_FREQ * 2 * \
> +					 OV9282_NUM_DATA_LANES / 10)
> +#define OV9282_PIXEL_RATE_8BIT		(OV9282_LINK_FREQ * 2 * \
> +					 OV9282_NUM_DATA_LANES / 8)

Here you go... This can also be dyanmically calculated, but with only
2 modes and a single link_freq I guess this is fine for now

>
>  /*
>   * OV9282 native and active pixel array size.
> @@ -149,6 +159,7 @@ struct ov9282_mode {
>   * @again_ctrl: Pointer to analog gain control
>   * @vblank: Vertical blanking in lines
>   * @cur_mode: Pointer to current selected sensor mode
> + * @code: Mbus code currently selected
>   * @mutex: Mutex for serializing sensor controls
>   * @streaming: Flag indicating streaming state
>   */
> @@ -169,8 +180,10 @@ struct ov9282 {
>  		struct v4l2_ctrl *exp_ctrl;
>  		struct v4l2_ctrl *again_ctrl;
>  	};
> +	struct v4l2_ctrl *pixel_rate;
>  	u32 vblank;
>  	const struct ov9282_mode *cur_mode;
> +	u32 code;
>  	struct mutex mutex;
>  	bool streaming;
>  };
> @@ -182,7 +195,6 @@ static const s64 link_freq[] = {
>  /* Common registers */
>  static const struct ov9282_reg common_regs[] = {
>  	{0x0302, 0x32},
> -	{0x030d, 0x50},
>  	{0x030e, 0x02},
>  	{0x3001, 0x00},
>  	{0x3004, 0x00},
> @@ -514,23 +526,41 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
>   * ov9282_update_controls() - Update control ranges based on streaming mode
>   * @ov9282: pointer to ov9282 device
>   * @mode: pointer to ov9282_mode sensor mode
> + * @fmt: pointer to the requested mode
>   *
>   * Return: 0 if successful, error code otherwise.
>   */
>  static int ov9282_update_controls(struct ov9282 *ov9282,
> -				  const struct ov9282_mode *mode)
> +				  const struct ov9282_mode *mode,
> +				  const struct v4l2_subdev_format *fmt)
>  {
>  	u32 hblank_min;
> +	s64 pixel_rate;
>  	int ret;
>
>  	ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
>  	if (ret)
>  		return ret;
>
> -	hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> -	ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> -					OV9282_TIMING_HTS_MAX - mode->width, 1,
> -					hblank_min);
> +	pixel_rate = (fmt->format.code == MEDIA_BUS_FMT_Y10_1X10) ?
> +		OV9282_PIXEL_RATE_10BIT : OV9282_PIXEL_RATE_8BIT;
> +	ret = __v4l2_ctrl_modify_range(ov9282->pixel_rate, pixel_rate,
> +				       pixel_rate, 1, pixel_rate);
> +	if (ret)
> +		return ret;
> +
> +	if (ov9282->cur_mode != mode) {

Why has this become conditional in this patch, and why only for hblank ?

> +		hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> +		ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> +						OV9282_TIMING_HTS_MAX - mode->width, 1,
> +						hblank_min);
> +		if (ret)
> +			return ret;
> +
> +		ret =  __v4l2_ctrl_s_ctrl(ov9282->hblank_ctrl, hblank_min);
> +		if (ret)
> +			return ret;
> +	}
>
>  	return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
>  					mode->vblank_max, 1, mode->vblank);
> @@ -693,10 +723,16 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
>  {
> -	if (code->index > 0)
> +	switch (code->index) {
> +	default:
>  		return -EINVAL;

Can default: come after the valid cases ?

> -
> -	code->code = MEDIA_BUS_FMT_Y10_1X10;
> +	case 0:
> +		code->code = MEDIA_BUS_FMT_Y10_1X10;
> +		break;
> +	case 1:
> +		code->code = MEDIA_BUS_FMT_Y8_1X8;
> +		break;
> +	}
>
>  	return 0;
>  }
> @@ -716,7 +752,8 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
>  	if (fsize->index >= OV9282_NUM_MODES)
>  		return -EINVAL;
>
> -	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
> +	if (fsize->code != MEDIA_BUS_FMT_Y10_1X10 &&
> +	    fsize->code != MEDIA_BUS_FMT_Y8_1X8)
>  		return -EINVAL;
>
>  	fsize->min_width = supported_modes[fsize->index].width;
> @@ -732,15 +769,17 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
>   *                            from selected sensor mode
>   * @ov9282: pointer to ov9282 device
>   * @mode: pointer to ov9282_mode sensor mode
> + * @code: mbus code to be stored
>   * @fmt: V4L2 sub-device format need to be filled
>   */
>  static void ov9282_fill_pad_format(struct ov9282 *ov9282,
>  				   const struct ov9282_mode *mode,
> +				   u32 code,
>  				   struct v4l2_subdev_format *fmt)
>  {
>  	fmt->format.width = mode->width;
>  	fmt->format.height = mode->height;
> -	fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
> +	fmt->format.code = code;
>  	fmt->format.field = V4L2_FIELD_NONE;
>  	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
>  	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> @@ -770,7 +809,8 @@ static int ov9282_get_pad_format(struct v4l2_subdev *sd,
>  		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
>  		fmt->format = *framefmt;
>  	} else {
> -		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, fmt);
> +		ov9282_fill_pad_format(ov9282, ov9282->cur_mode, ov9282->code,
> +				       fmt);
>  	}
>
>  	mutex_unlock(&ov9282->mutex);
> @@ -792,6 +832,7 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>  {
>  	struct ov9282 *ov9282 = to_ov9282(sd);
>  	const struct ov9282_mode *mode;
> +	u32 code;
>  	int ret = 0;
>
>  	mutex_lock(&ov9282->mutex);
> @@ -801,7 +842,12 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>  				      width, height,
>  				      fmt->format.width,
>  				      fmt->format.height);
> -	ov9282_fill_pad_format(ov9282, mode, fmt);
> +	if (fmt->format.code == MEDIA_BUS_FMT_Y8_1X8)
> +		code = MEDIA_BUS_FMT_Y8_1X8;
> +	else
> +		code = MEDIA_BUS_FMT_Y10_1X10;
> +
> +	ov9282_fill_pad_format(ov9282, mode, code, fmt);
>
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>  		struct v4l2_mbus_framefmt *framefmt;
> @@ -809,9 +855,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>  		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
>  		*framefmt = fmt->format;
>  	} else {
> -		ret = ov9282_update_controls(ov9282, mode);
> -		if (!ret)
> +		ret = ov9282_update_controls(ov9282, mode, fmt);
> +		if (!ret) {
>  			ov9282->cur_mode = mode;
> +			ov9282->code = code;
> +		}
>  	}
>
>  	mutex_unlock(&ov9282->mutex);
> @@ -833,7 +881,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_subdev_format fmt = { 0 };
>
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	ov9282_fill_pad_format(ov9282, &supported_modes[0], &fmt);
> +	ov9282_fill_pad_format(ov9282, &supported_modes[0], ov9282->code, &fmt);
>
>  	return ov9282_set_pad_format(sd, sd_state, &fmt);
>  }
> @@ -898,7 +946,17 @@ static int ov9282_get_selection(struct v4l2_subdev *sd,
>   */
>  static int ov9282_start_streaming(struct ov9282 *ov9282)
>  {
> +	const struct ov9282_reg bitdepth_regs[2][2] = {
> +		{
> +			{OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW10},
> +			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10},
> +		}, {
> +			{OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW8},
> +			{OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
> +		}
> +	};
>  	const struct ov9282_reg_list *reg_list;
> +	int bitdepth_index;
>  	int ret;
>
>  	/* Write common registers */
> @@ -917,6 +975,13 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
>  		return ret;
>  	}
>
> +	bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
> +	ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2);
> +	if (ret) {
> +		dev_err(ov9282->dev, "fail to write bitdepth regs");
> +		return ret;
> +	}
> +
>  	/* Write sensor mode registers */
>  	reg_list = &ov9282->cur_mode->reg_list;
>  	ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
> @@ -1258,9 +1323,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  			  0, 1, 1, 1);
>
>  	/* Read only controls */
> -	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
> -			  OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> -			  OV9282_PIXEL_RATE);
> +	ov9282->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> +					       V4L2_CID_PIXEL_RATE,
> +					       OV9282_PIXEL_RATE_10BIT,
> +					       OV9282_PIXEL_RATE_10BIT, 1,
> +					       OV9282_PIXEL_RATE_10BIT);
>
>  	ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>  							&ov9282_ctrl_ops,
> @@ -1342,6 +1409,7 @@ static int ov9282_probe(struct i2c_client *client)
>
>  	/* Set default mode to first mode */
>  	ov9282->cur_mode = &supported_modes[0];
> +	ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
>  	ov9282->vblank = ov9282->cur_mode->vblank;
>
>  	ret = ov9282_init_controls(ov9282);
> --
> 2.34.1
>

Re: [PATCH 15/16] media: i2c: ov9282: Add support for 8bit readout

[Dave Stevenson]

Hi Jacopo

Thanks for the review

On Thu, 6 Oct 2022 at 10:58, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:28:08PM +0100, Dave Stevenson wrote:
> > The sensor supports 8 or 10 bit readout modes, but the
> > driver only supported 10 bit.
> >
> > Add 8 bit readout.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 110 ++++++++++++++++++++++++++++++-------
> >  1 file changed, 89 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index ec1599488f21..bc429455421e 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -22,6 +22,10 @@
> >  #define OV9282_MODE_STANDBY  0x00
> >  #define OV9282_MODE_STREAMING        0x01
> >
> > +#define OV9282_REG_PLL_CTRL_0D       0x030d
> > +#define OV9282_PLL_CTRL_0D_RAW8              0x60
> > +#define OV9282_PLL_CTRL_0D_RAW10     0x50
> > +
> >  #define OV9282_REG_TIMING_HTS        0x380c
> >  #define OV9282_TIMING_HTS_MAX        0x7fff
> >
> > @@ -49,6 +53,10 @@
> >  /* Group hold register */
> >  #define OV9282_REG_HOLD              0x3308
> >
> > +#define OV9282_REG_ANA_CORE_2        0x3662
> > +#define OV9282_ANA_CORE2_RAW8        0x07
> > +#define OV9282_ANA_CORE2_RAW10       0x05
> > +
> >  #define OV9282_REG_TIMING_FORMAT_1   0x3820
> >  #define OV9282_REG_TIMING_FORMAT_2   0x3821
> >  #define OV9282_FLIP_BIT                      BIT(2)
> > @@ -64,8 +72,10 @@
> >  #define OV9282_NUM_DATA_LANES        2
> >
> >  /* Pixel rate */
> > -#define OV9282_PIXEL_RATE    (OV9282_LINK_FREQ * 2 * \
> > -                              OV9282_NUM_DATA_LANES / 10)
> > +#define OV9282_PIXEL_RATE_10BIT              (OV9282_LINK_FREQ * 2 * \
> > +                                      OV9282_NUM_DATA_LANES / 10)
> > +#define OV9282_PIXEL_RATE_8BIT               (OV9282_LINK_FREQ * 2 * \
> > +                                      OV9282_NUM_DATA_LANES / 8)
>
> Here you go... This can also be dyanmically calculated, but with only
> 2 modes and a single link_freq I guess this is fine for now

It's 2 pixel formats and a single link freq. Readout modes don't affect it.

Technically it's if anyone adds any additional PLL configs for PLL2
(drives the pixel array) then the pixel rate will change. The FIFO
between image array and MIPI block will allow link frequency to be
changed independently of pixel rate, just as long as link freq is fast
enough to get rid of the data generated.

  Dave

> >
> >  /*
> >   * OV9282 native and active pixel array size.
> > @@ -149,6 +159,7 @@ struct ov9282_mode {
> >   * @again_ctrl: Pointer to analog gain control
> >   * @vblank: Vertical blanking in lines
> >   * @cur_mode: Pointer to current selected sensor mode
> > + * @code: Mbus code currently selected
> >   * @mutex: Mutex for serializing sensor controls
> >   * @streaming: Flag indicating streaming state
> >   */
> > @@ -169,8 +180,10 @@ struct ov9282 {
> >               struct v4l2_ctrl *exp_ctrl;
> >               struct v4l2_ctrl *again_ctrl;
> >       };
> > +     struct v4l2_ctrl *pixel_rate;
> >       u32 vblank;
> >       const struct ov9282_mode *cur_mode;
> > +     u32 code;
> >       struct mutex mutex;
> >       bool streaming;
> >  };
> > @@ -182,7 +195,6 @@ static const s64 link_freq[] = {
> >  /* Common registers */
> >  static const struct ov9282_reg common_regs[] = {
> >       {0x0302, 0x32},
> > -     {0x030d, 0x50},
> >       {0x030e, 0x02},
> >       {0x3001, 0x00},
> >       {0x3004, 0x00},
> > @@ -514,23 +526,41 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
> >   * ov9282_update_controls() - Update control ranges based on streaming mode
> >   * @ov9282: pointer to ov9282 device
> >   * @mode: pointer to ov9282_mode sensor mode
> > + * @fmt: pointer to the requested mode
> >   *
> >   * Return: 0 if successful, error code otherwise.
> >   */
> >  static int ov9282_update_controls(struct ov9282 *ov9282,
> > -                               const struct ov9282_mode *mode)
> > +                               const struct ov9282_mode *mode,
> > +                               const struct v4l2_subdev_format *fmt)
> >  {
> >       u32 hblank_min;
> > +     s64 pixel_rate;
> >       int ret;
> >
> >       ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
> >       if (ret)
> >               return ret;
> >
> > -     hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> > -     ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> > -                                     OV9282_TIMING_HTS_MAX - mode->width, 1,
> > -                                     hblank_min);
> > +     pixel_rate = (fmt->format.code == MEDIA_BUS_FMT_Y10_1X10) ?
> > +             OV9282_PIXEL_RATE_10BIT : OV9282_PIXEL_RATE_8BIT;
> > +     ret = __v4l2_ctrl_modify_range(ov9282->pixel_rate, pixel_rate,
> > +                                    pixel_rate, 1, pixel_rate);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (ov9282->cur_mode != mode) {
>
> Why has this become conditional in this patch, and why only for hblank ?

Testing before Naush's patches to libcamera supported setting hblank.
All mode sets were resetting controls, so libcamera starting up
triggered a mode change and reset hblank.

AIUI changing mode should not reset controls unless they are now
invalid - I'll do that as a new patch.

> > +             hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> > +             ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> > +                                             OV9282_TIMING_HTS_MAX - mode->width, 1,
> > +                                             hblank_min);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret =  __v4l2_ctrl_s_ctrl(ov9282->hblank_ctrl, hblank_min);
> > +             if (ret)
> > +                     return ret;
> > +     }
> >
> >       return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
> >                                       mode->vblank_max, 1, mode->vblank);
> > @@ -693,10 +723,16 @@ static int ov9282_enum_mbus_code(struct v4l2_subdev *sd,
> >                                struct v4l2_subdev_state *sd_state,
> >                                struct v4l2_subdev_mbus_code_enum *code)
> >  {
> > -     if (code->index > 0)
> > +     switch (code->index) {
> > +     default:
> >               return -EINVAL;
>
> Can default: come after the valid cases ?

Sure, I'll do that.

  Dave

> > -
> > -     code->code = MEDIA_BUS_FMT_Y10_1X10;
> > +     case 0:
> > +             code->code = MEDIA_BUS_FMT_Y10_1X10;
> > +             break;
> > +     case 1:
> > +             code->code = MEDIA_BUS_FMT_Y8_1X8;
> > +             break;
> > +     }
> >
> >       return 0;
> >  }
> > @@ -716,7 +752,8 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
> >       if (fsize->index >= OV9282_NUM_MODES)
> >               return -EINVAL;
> >
> > -     if (fsize->code != MEDIA_BUS_FMT_Y10_1X10)
> > +     if (fsize->code != MEDIA_BUS_FMT_Y10_1X10 &&
> > +         fsize->code != MEDIA_BUS_FMT_Y8_1X8)
> >               return -EINVAL;
> >
> >       fsize->min_width = supported_modes[fsize->index].width;
> > @@ -732,15 +769,17 @@ static int ov9282_enum_frame_size(struct v4l2_subdev *sd,
> >   *                            from selected sensor mode
> >   * @ov9282: pointer to ov9282 device
> >   * @mode: pointer to ov9282_mode sensor mode
> > + * @code: mbus code to be stored
> >   * @fmt: V4L2 sub-device format need to be filled
> >   */
> >  static void ov9282_fill_pad_format(struct ov9282 *ov9282,
> >                                  const struct ov9282_mode *mode,
> > +                                u32 code,
> >                                  struct v4l2_subdev_format *fmt)
> >  {
> >       fmt->format.width = mode->width;
> >       fmt->format.height = mode->height;
> > -     fmt->format.code = MEDIA_BUS_FMT_Y10_1X10;
> > +     fmt->format.code = code;
> >       fmt->format.field = V4L2_FIELD_NONE;
> >       fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> >       fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > @@ -770,7 +809,8 @@ static int ov9282_get_pad_format(struct v4l2_subdev *sd,
> >               framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> >               fmt->format = *framefmt;
> >       } else {
> > -             ov9282_fill_pad_format(ov9282, ov9282->cur_mode, fmt);
> > +             ov9282_fill_pad_format(ov9282, ov9282->cur_mode, ov9282->code,
> > +                                    fmt);
> >       }
> >
> >       mutex_unlock(&ov9282->mutex);
> > @@ -792,6 +832,7 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
> >  {
> >       struct ov9282 *ov9282 = to_ov9282(sd);
> >       const struct ov9282_mode *mode;
> > +     u32 code;
> >       int ret = 0;
> >
> >       mutex_lock(&ov9282->mutex);
> > @@ -801,7 +842,12 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
> >                                     width, height,
> >                                     fmt->format.width,
> >                                     fmt->format.height);
> > -     ov9282_fill_pad_format(ov9282, mode, fmt);
> > +     if (fmt->format.code == MEDIA_BUS_FMT_Y8_1X8)
> > +             code = MEDIA_BUS_FMT_Y8_1X8;
> > +     else
> > +             code = MEDIA_BUS_FMT_Y10_1X10;
> > +
> > +     ov9282_fill_pad_format(ov9282, mode, code, fmt);
> >
> >       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> >               struct v4l2_mbus_framefmt *framefmt;
> > @@ -809,9 +855,11 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
> >               framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> >               *framefmt = fmt->format;
> >       } else {
> > -             ret = ov9282_update_controls(ov9282, mode);
> > -             if (!ret)
> > +             ret = ov9282_update_controls(ov9282, mode, fmt);
> > +             if (!ret) {
> >                       ov9282->cur_mode = mode;
> > +                     ov9282->code = code;
> > +             }
> >       }
> >
> >       mutex_unlock(&ov9282->mutex);
> > @@ -833,7 +881,7 @@ static int ov9282_init_pad_cfg(struct v4l2_subdev *sd,
> >       struct v4l2_subdev_format fmt = { 0 };
> >
> >       fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> > -     ov9282_fill_pad_format(ov9282, &supported_modes[0], &fmt);
> > +     ov9282_fill_pad_format(ov9282, &supported_modes[0], ov9282->code, &fmt);
> >
> >       return ov9282_set_pad_format(sd, sd_state, &fmt);
> >  }
> > @@ -898,7 +946,17 @@ static int ov9282_get_selection(struct v4l2_subdev *sd,
> >   */
> >  static int ov9282_start_streaming(struct ov9282 *ov9282)
> >  {
> > +     const struct ov9282_reg bitdepth_regs[2][2] = {
> > +             {
> > +                     {OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW10},
> > +                     {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10},
> > +             }, {
> > +                     {OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW8},
> > +                     {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
> > +             }
> > +     };
> >       const struct ov9282_reg_list *reg_list;
> > +     int bitdepth_index;
> >       int ret;
> >
> >       /* Write common registers */
> > @@ -917,6 +975,13 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
> >               return ret;
> >       }
> >
> > +     bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
> > +     ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2);
> > +     if (ret) {
> > +             dev_err(ov9282->dev, "fail to write bitdepth regs");
> > +             return ret;
> > +     }
> > +
> >       /* Write sensor mode registers */
> >       reg_list = &ov9282->cur_mode->reg_list;
> >       ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
> > @@ -1258,9 +1323,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >                         0, 1, 1, 1);
> >
> >       /* Read only controls */
> > -     v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > -                       OV9282_PIXEL_RATE, OV9282_PIXEL_RATE, 1,
> > -                       OV9282_PIXEL_RATE);
> > +     ov9282->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> > +                                            V4L2_CID_PIXEL_RATE,
> > +                                            OV9282_PIXEL_RATE_10BIT,
> > +                                            OV9282_PIXEL_RATE_10BIT, 1,
> > +                                            OV9282_PIXEL_RATE_10BIT);
> >
> >       ov9282->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> >                                                       &ov9282_ctrl_ops,
> > @@ -1342,6 +1409,7 @@ static int ov9282_probe(struct i2c_client *client)
> >
> >       /* Set default mode to first mode */
> >       ov9282->cur_mode = &supported_modes[0];
> > +     ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
> >       ov9282->vblank = ov9282->cur_mode->vblank;
> >
> >       ret = ov9282_init_controls(ov9282);
> > --
> > 2.34.1
> >

[PATCH 16/16] media: i2c: ov9282: Support event handlers

[Dave Stevenson]

As noted in the headers for V4L2_SUBDEV_FL_HAS_EVENTS,
"controls can send events, thus drivers exposing controls
should set this flag".

This driver exposes controls, but didn't reflect that it
could generate events. Correct this, and add the default
event handler functions.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index bc429455421e..416c9656e3ac 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -14,6 +14,7 @@
 #include <linux/regulator/consumer.h>
 
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
@@ -1189,6 +1190,11 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
 }
 
 /* V4l2 subdevice ops */
+static const struct v4l2_subdev_core_ops ov9282_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_video_ops ov9282_video_ops = {
 	.s_stream = ov9282_set_stream,
 };
@@ -1203,6 +1209,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
 };
 
 static const struct v4l2_subdev_ops ov9282_subdev_ops = {
+	.core = &ov9282_core_ops,
 	.video = &ov9282_video_ops,
 	.pad = &ov9282_pad_ops,
 };
@@ -1419,7 +1426,8 @@ static int ov9282_probe(struct i2c_client *client)
 	}
 
 	/* Initialize subdev */
-	ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			    V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
 	/* Initialize source pad */
-- 
2.34.1

Re: [PATCH 16/16] media: i2c: ov9282: Support event handlers

[Jacopo Mondi]

Hi Dave

On Wed, Oct 05, 2022 at 04:28:09PM +0100, Dave Stevenson wrote:
> As noted in the headers for V4L2_SUBDEV_FL_HAS_EVENTS,
> "controls can send events, thus drivers exposing controls
> should set this flag".
>

I was expecting this to only apply to drivers that actually generate
events...

Not sure I can give a tag here as my understanding of this part is
limited, let's wait for other opinions :)


> This driver exposes controls, but didn't reflect that it
> could generate events. Correct this, and add the default
> event handler functions.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index bc429455421e..416c9656e3ac 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -14,6 +14,7 @@
>  #include <linux/regulator/consumer.h>
>
>  #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>
> @@ -1189,6 +1190,11 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
>  }
>
>  /* V4l2 subdevice ops */
> +static const struct v4l2_subdev_core_ops ov9282_core_ops = {
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
>  static const struct v4l2_subdev_video_ops ov9282_video_ops = {
>  	.s_stream = ov9282_set_stream,
>  };
> @@ -1203,6 +1209,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
>  };
>
>  static const struct v4l2_subdev_ops ov9282_subdev_ops = {
> +	.core = &ov9282_core_ops,
>  	.video = &ov9282_video_ops,
>  	.pad = &ov9282_pad_ops,
>  };
> @@ -1419,7 +1426,8 @@ static int ov9282_probe(struct i2c_client *client)
>  	}
>
>  	/* Initialize subdev */
> -	ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			    V4L2_SUBDEV_FL_HAS_EVENTS;
>  	ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>
>  	/* Initialize source pad */
> --
> 2.34.1
>

Re: [PATCH 16/16] media: i2c: ov9282: Support event handlers

[Dave Stevenson]

Hi Jacopo

+ Hans for comment on compliance / controls framework

On Thu, 6 Oct 2022 at 10:59, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:28:09PM +0100, Dave Stevenson wrote:
> > As noted in the headers for V4L2_SUBDEV_FL_HAS_EVENTS,
> > "controls can send events, thus drivers exposing controls
> > should set this flag".
> >
>
> I was expecting this to only apply to drivers that actually generate
> events...
>
> Not sure I can give a tag here as my understanding of this part is
> limited, let's wait for other opinions :)

I had to rack my memory on this one.

It fixes a v4l2-compliance failure:
fail: v4l2-test-controls.cpp(835): subscribe event for control 'User
Controls' failed
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
(v4l-utils built at ToT from today, so fd544473800 "support INTEGER
and INTEGER64 control arrays")

So either it is required by all drivers that expose controls, or it's
an issue in v4l2-compliance.
I believe it's the former as all controls can create a V4L2_EVENT_CTRL
event should the value or range change (very common for things like
HBLANK and VBLANK in image sensor drivers).

Thanks
  Dave

> > This driver exposes controls, but didn't reflect that it
> > could generate events. Correct this, and add the default
> > event handler functions.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index bc429455421e..416c9656e3ac 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/regulator/consumer.h>
> >
> >  #include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-event.h>
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >
> > @@ -1189,6 +1190,11 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
> >  }
> >
> >  /* V4l2 subdevice ops */
> > +static const struct v4l2_subdev_core_ops ov9282_core_ops = {
> > +     .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > +     .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > +};
> > +
> >  static const struct v4l2_subdev_video_ops ov9282_video_ops = {
> >       .s_stream = ov9282_set_stream,
> >  };
> > @@ -1203,6 +1209,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
> >  };
> >
> >  static const struct v4l2_subdev_ops ov9282_subdev_ops = {
> > +     .core = &ov9282_core_ops,
> >       .video = &ov9282_video_ops,
> >       .pad = &ov9282_pad_ops,
> >  };
> > @@ -1419,7 +1426,8 @@ static int ov9282_probe(struct i2c_client *client)
> >       }
> >
> >       /* Initialize subdev */
> > -     ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +     ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > +                         V4L2_SUBDEV_FL_HAS_EVENTS;
> >       ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> >
> >       /* Initialize source pad */
> > --
> > 2.34.1
> >

Re: [PATCH 16/16] media: i2c: ov9282: Support event handlers

[Jacopo Mondi]

On Fri, Oct 07, 2022 at 11:22:23AM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> + Hans for comment on compliance / controls framework
>
> On Thu, 6 Oct 2022 at 10:59, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Dave
> >
> > On Wed, Oct 05, 2022 at 04:28:09PM +0100, Dave Stevenson wrote:
> > > As noted in the headers for V4L2_SUBDEV_FL_HAS_EVENTS,
> > > "controls can send events, thus drivers exposing controls
> > > should set this flag".
> > >
> >
> > I was expecting this to only apply to drivers that actually generate
> > events...
> >
> > Not sure I can give a tag here as my understanding of this part is
> > limited, let's wait for other opinions :)
>
> I had to rack my memory on this one.
>
> It fixes a v4l2-compliance failure:
> fail: v4l2-test-controls.cpp(835): subscribe event for control 'User
> Controls' failed
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
> (v4l-utils built at ToT from today, so fd544473800 "support INTEGER
> and INTEGER64 control arrays")
>
> So either it is required by all drivers that expose controls, or it's
> an issue in v4l2-compliance.
> I believe it's the former as all controls can create a V4L2_EVENT_CTRL
> event should the value or range change (very common for things like
> HBLANK and VBLANK in image sensor drivers).
>

It seems only 19 sensor drivers in total implement a .subscribe_event
function... let's say there's room for improvements :)

> Thanks
>   Dave
>
> > > This driver exposes controls, but didn't reflect that it
> > > could generate events. Correct this, and add the default
> > > event handler functions.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index bc429455421e..416c9656e3ac 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/regulator/consumer.h>
> > >
> > >  #include <media/v4l2-ctrls.h>
> > > +#include <media/v4l2-event.h>
> > >  #include <media/v4l2-fwnode.h>
> > >  #include <media/v4l2-subdev.h>
> > >
> > > @@ -1189,6 +1190,11 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
> > >  }
> > >
> > >  /* V4l2 subdevice ops */
> > > +static const struct v4l2_subdev_core_ops ov9282_core_ops = {
> > > +     .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > > +     .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > > +};
> > > +
> > >  static const struct v4l2_subdev_video_ops ov9282_video_ops = {
> > >       .s_stream = ov9282_set_stream,
> > >  };
> > > @@ -1203,6 +1209,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
> > >  };
> > >
> > >  static const struct v4l2_subdev_ops ov9282_subdev_ops = {
> > > +     .core = &ov9282_core_ops,
> > >       .video = &ov9282_video_ops,
> > >       .pad = &ov9282_pad_ops,
> > >  };
> > > @@ -1419,7 +1426,8 @@ static int ov9282_probe(struct i2c_client *client)
> > >       }
> > >
> > >       /* Initialize subdev */
> > > -     ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +     ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > > +                         V4L2_SUBDEV_FL_HAS_EVENTS;
> > >       ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > >
> > >       /* Initialize source pad */
> > > --
> > > 2.34.1
> > >