[PATCH 0/3] Add flash/strobe support for ov9282

[PATCH 0/3] Add flash/strobe support for ov9282

[Richard Leitner]

This series adds basic flash/strobe support for ov9282 sensors using
their "hardware strobe output".

Apart from en-/disabling the flash/strobe output, setting a timeout
(duration of activated strobe per frame) is implemented. The calculation
of this timeout is only interpolated from various measurements, as no
documentation was found.

Further flash/strobe-related controls as well as a migration to v4l2-cci
helpers will likely be implemented in future series.

All register addresses/values are based on the OV9281 datasheet v1.53
(january 2019). This series was tested using an ov9281 VisionComponents
camera module.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
Richard Leitner (3):
      media: i2c: ov9282: add output enable register definitions
      media: i2c: ov9282: add led_mode v4l2 control
      media: i2c: ov9282: add strobe_timeout v4l2 control

 drivers/media/i2c/ov9282.c | 89 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 86 insertions(+), 3 deletions(-)
---
base-commit: f41427b3bdee7d9845b13a80c0d03882212f4b20
change-id: 20250303-ov9282-flash-strobe-ac6bd00c9de6
prerequisite-change-id: 20250225-b4-ov9282-gain-ef1cdaba5bfd:v1
prerequisite-patch-id: 86f2582378ff7095ab65ce4bb25a143eb639e840
prerequisite-patch-id: b06eb6ec697aaf0b3155b4b2370f171d0d304ae2
prerequisite-patch-id: b123047d71bfb9b93f743bbdd6893d5a98495801

Best regards,
-- 
Richard Leitner <richard.leitner@linux.dev>

[PATCH 1/3] media: i2c: ov9282: add output enable register definitions

[Richard Leitner]

Add #define's for the output enable registers (0x3004, 0x3005, 0x3006),
also known as SC_CTRL_04, SC_CTRL_05, SC_CTRL_04. Use those register
definitions instead of the raw values in the `common_regs` struct.

All values are based on the OV9281 datasheet v1.53 (january 2019).

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c882a021cf18852237bf9b9524d3de0c5b48cbcb..f42e0d439753e74d14e3a3592029e48f49234927 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -37,6 +37,29 @@
 #define OV9282_REG_ID		0x300a
 #define OV9282_ID		0x9281
 
+/* Output enable registers */
+#define OV9282_REG_OUTPUT_ENABLE4	0x3004
+#define OV9282_OUTPUT_ENABLE4_GPIO2	BIT(1)
+#define OV9282_OUTPUT_ENABLE4_D9	BIT(0)
+
+#define OV9282_REG_OUTPUT_ENABLE5	0x3005
+#define OV9282_OUTPUT_ENABLE5_D8	BIT(7)
+#define OV9282_OUTPUT_ENABLE5_D7	BIT(6)
+#define OV9282_OUTPUT_ENABLE5_D6	BIT(5)
+#define OV9282_OUTPUT_ENABLE5_D5	BIT(4)
+#define OV9282_OUTPUT_ENABLE5_D4	BIT(3)
+#define OV9282_OUTPUT_ENABLE5_D3	BIT(2)
+#define OV9282_OUTPUT_ENABLE5_D2	BIT(1)
+#define OV9282_OUTPUT_ENABLE5_D1	BIT(0)
+
+#define OV9282_REG_OUTPUT_ENABLE6	0x3006
+#define OV9282_OUTPUT_ENABLE6_D0	BIT(7)
+#define OV9282_OUTPUT_ENABLE6_PCLK	BIT(6)
+#define OV9282_OUTPUT_ENABLE6_HREF	BIT(5)
+#define OV9282_OUTPUT_ENABLE6_STROBE	BIT(3)
+#define OV9282_OUTPUT_ENABLE6_ILPWM	BIT(2)
+#define OV9282_OUTPUT_ENABLE6_VSYNC	BIT(1)
+
 /* Exposure control */
 #define OV9282_REG_EXPOSURE	0x3500
 #define OV9282_EXPOSURE_MIN	1
@@ -213,9 +236,9 @@ static const struct ov9282_reg common_regs[] = {
 	{0x0302, 0x32},
 	{0x030e, 0x02},
 	{0x3001, 0x00},
-	{0x3004, 0x00},
-	{0x3005, 0x00},
-	{0x3006, 0x04},
+	{OV9282_REG_OUTPUT_ENABLE4, 0x00},
+	{OV9282_REG_OUTPUT_ENABLE5, 0x00},
+	{OV9282_REG_OUTPUT_ENABLE6, OV9282_OUTPUT_ENABLE6_ILPWM},
 	{0x3011, 0x0a},
 	{0x3013, 0x18},
 	{0x301c, 0xf0},

-- 
2.47.2

Re: [PATCH 1/3] media: i2c: ov9282: add output enable register definitions

[Dave Stevenson]

Hi Richard

On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@linux.dev> wrote:
>
> Add #define's for the output enable registers (0x3004, 0x3005, 0x3006),
> also known as SC_CTRL_04, SC_CTRL_05, SC_CTRL_04. Use those register
> definitions instead of the raw values in the `common_regs` struct.
>
> All values are based on the OV9281 datasheet v1.53 (january 2019).
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>

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

> ---
>  drivers/media/i2c/ov9282.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index c882a021cf18852237bf9b9524d3de0c5b48cbcb..f42e0d439753e74d14e3a3592029e48f49234927 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -37,6 +37,29 @@
>  #define OV9282_REG_ID          0x300a
>  #define OV9282_ID              0x9281
>
> +/* Output enable registers */
> +#define OV9282_REG_OUTPUT_ENABLE4      0x3004
> +#define OV9282_OUTPUT_ENABLE4_GPIO2    BIT(1)
> +#define OV9282_OUTPUT_ENABLE4_D9       BIT(0)
> +
> +#define OV9282_REG_OUTPUT_ENABLE5      0x3005
> +#define OV9282_OUTPUT_ENABLE5_D8       BIT(7)
> +#define OV9282_OUTPUT_ENABLE5_D7       BIT(6)
> +#define OV9282_OUTPUT_ENABLE5_D6       BIT(5)
> +#define OV9282_OUTPUT_ENABLE5_D5       BIT(4)
> +#define OV9282_OUTPUT_ENABLE5_D4       BIT(3)
> +#define OV9282_OUTPUT_ENABLE5_D3       BIT(2)
> +#define OV9282_OUTPUT_ENABLE5_D2       BIT(1)
> +#define OV9282_OUTPUT_ENABLE5_D1       BIT(0)
> +
> +#define OV9282_REG_OUTPUT_ENABLE6      0x3006
> +#define OV9282_OUTPUT_ENABLE6_D0       BIT(7)
> +#define OV9282_OUTPUT_ENABLE6_PCLK     BIT(6)
> +#define OV9282_OUTPUT_ENABLE6_HREF     BIT(5)
> +#define OV9282_OUTPUT_ENABLE6_STROBE   BIT(3)
> +#define OV9282_OUTPUT_ENABLE6_ILPWM    BIT(2)
> +#define OV9282_OUTPUT_ENABLE6_VSYNC    BIT(1)
> +
>  /* Exposure control */
>  #define OV9282_REG_EXPOSURE    0x3500
>  #define OV9282_EXPOSURE_MIN    1
> @@ -213,9 +236,9 @@ static const struct ov9282_reg common_regs[] = {
>         {0x0302, 0x32},
>         {0x030e, 0x02},
>         {0x3001, 0x00},
> -       {0x3004, 0x00},
> -       {0x3005, 0x00},
> -       {0x3006, 0x04},
> +       {OV9282_REG_OUTPUT_ENABLE4, 0x00},
> +       {OV9282_REG_OUTPUT_ENABLE5, 0x00},
> +       {OV9282_REG_OUTPUT_ENABLE6, OV9282_OUTPUT_ENABLE6_ILPWM},
>         {0x3011, 0x0a},
>         {0x3013, 0x18},
>         {0x301c, 0xf0},
>
> --
> 2.47.2
>
>

[PATCH 2/3] media: i2c: ov9282: add led_mode v4l2 control

[Richard Leitner]

Add V4L2_CID_FLASH_LED_MODE support using the "strobe output enable"
feature of the sensor. This implements following modes:

 - V4L2_FLASH_LED_MODE_NONE, which disables the strobe output
 - V4L2_FLASH_LED_MODE_FLASH, which enables the strobe output

All values are based on the OV9281 datasheet v1.53 (january 2019) and
tested using an ov9281 VisionComponents module.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index f42e0d439753e74d14e3a3592029e48f49234927..c98ba466e9aea29baff0b13578d760bf69c958c5 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -192,6 +192,7 @@ struct ov9282_mode {
  * @exp_ctrl: Pointer to exposure control
  * @again_ctrl: Pointer to analog gain control
  * @pixel_rate: Pointer to pixel rate control
+ * @flash_led_mode: Pointer to flash led mode control
  * @vblank: Vertical blanking in lines
  * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
  * @cur_mode: Pointer to current selected sensor mode
@@ -214,6 +215,7 @@ struct ov9282 {
 		struct v4l2_ctrl *again_ctrl;
 	};
 	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *flash_led_mode;
 	u32 vblank;
 	bool noncontinuous_clock;
 	const struct ov9282_mode *cur_mode;
@@ -670,6 +672,23 @@ static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
 				current_val);
 }
 
+static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
+{
+	u32 current_val;
+	int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
+				  &current_val);
+	if (ret)
+		return ret;
+
+	if (mode == V4L2_FLASH_LED_MODE_FLASH)
+		current_val |= OV9282_OUTPUT_ENABLE6_STROBE;
+	else
+		current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE;
+
+	return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
+				current_val);
+}
+
 /**
  * ov9282_set_ctrl() - Set subdevice control
  * @ctrl: pointer to v4l2_ctrl structure
@@ -736,6 +755,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
 				       (ctrl->val + ov9282->cur_mode->width) >> 1);
 		break;
+	case V4L2_CID_FLASH_LED_MODE:
+		ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -1391,6 +1413,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 						OV9282_TIMING_HTS_MAX - mode->width,
 						1, hblank_min);
 
+	/* Flash/Strobe controls */
+	ov9282->flash_led_mode = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
+							V4L2_CID_FLASH_LED_MODE,
+							V4L2_FLASH_LED_MODE_TORCH,
+							(1 << V4L2_FLASH_LED_MODE_TORCH),
+							V4L2_FLASH_LED_MODE_NONE);
+
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
 		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */

-- 
2.47.2

Re: [PATCH 2/3] media: i2c: ov9282: add led_mode v4l2 control

[Dave Stevenson]

Hi Richard

On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@linux.dev> wrote:
>
> Add V4L2_CID_FLASH_LED_MODE support using the "strobe output enable"
> feature of the sensor. This implements following modes:
>
>  - V4L2_FLASH_LED_MODE_NONE, which disables the strobe output
>  - V4L2_FLASH_LED_MODE_FLASH, which enables the strobe output
>
> All values are based on the OV9281 datasheet v1.53 (january 2019) and
> tested using an ov9281 VisionComponents module.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/i2c/ov9282.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index f42e0d439753e74d14e3a3592029e48f49234927..c98ba466e9aea29baff0b13578d760bf69c958c5 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -192,6 +192,7 @@ struct ov9282_mode {
>   * @exp_ctrl: Pointer to exposure control
>   * @again_ctrl: Pointer to analog gain control
>   * @pixel_rate: Pointer to pixel rate control
> + * @flash_led_mode: Pointer to flash led mode control
>   * @vblank: Vertical blanking in lines
>   * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
>   * @cur_mode: Pointer to current selected sensor mode
> @@ -214,6 +215,7 @@ struct ov9282 {
>                 struct v4l2_ctrl *again_ctrl;
>         };
>         struct v4l2_ctrl *pixel_rate;
> +       struct v4l2_ctrl *flash_led_mode;

As with 3/3, you only use this control from within ov9282_set_ctrl
where you are given the struct v4l2_ctrl, so there is no need to store
it in the device state structure.

>         u32 vblank;
>         bool noncontinuous_clock;
>         const struct ov9282_mode *cur_mode;
> @@ -670,6 +672,23 @@ static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
>                                 current_val);
>  }
>
> +static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
> +{
> +       u32 current_val;
> +       int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> +                                 &current_val);
> +       if (ret)
> +               return ret;
> +
> +       if (mode == V4L2_FLASH_LED_MODE_FLASH)
> +               current_val |= OV9282_OUTPUT_ENABLE6_STROBE;
> +       else
> +               current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE;
> +
> +       return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> +                               current_val);
> +}
> +
>  /**
>   * ov9282_set_ctrl() - Set subdevice control
>   * @ctrl: pointer to v4l2_ctrl structure
> @@ -736,6 +755,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>                 ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
>                                        (ctrl->val + ov9282->cur_mode->width) >> 1);
>                 break;
> +       case V4L2_CID_FLASH_LED_MODE:
> +               ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
> +               break;
>         default:
>                 dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
>                 ret = -EINVAL;
> @@ -1391,6 +1413,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>                                                 OV9282_TIMING_HTS_MAX - mode->width,
>                                                 1, hblank_min);
>
> +       /* Flash/Strobe controls */
> +       ov9282->flash_led_mode = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> +                                                       V4L2_CID_FLASH_LED_MODE,
> +                                                       V4L2_FLASH_LED_MODE_TORCH,
> +                                                       (1 << V4L2_FLASH_LED_MODE_TORCH),
> +                                                       V4L2_FLASH_LED_MODE_NONE);
> +
>         ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
>         if (!ret) {
>                 /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
>
> --
> 2.47.2
>
>

Re: [PATCH 2/3] media: i2c: ov9282: add led_mode v4l2 control

[Richard Leitner]

Hi Dave,
thanks for the review.
I really appreciate your quick responses.

On Tue, Mar 04, 2025 at 05:27:34PM +0000, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@linux.dev> wrote:
> >
> > Add V4L2_CID_FLASH_LED_MODE support using the "strobe output enable"
> > feature of the sensor. This implements following modes:
> >
> >  - V4L2_FLASH_LED_MODE_NONE, which disables the strobe output
> >  - V4L2_FLASH_LED_MODE_FLASH, which enables the strobe output
> >
> > All values are based on the OV9281 datasheet v1.53 (january 2019) and
> > tested using an ov9281 VisionComponents module.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index f42e0d439753e74d14e3a3592029e48f49234927..c98ba466e9aea29baff0b13578d760bf69c958c5 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -192,6 +192,7 @@ struct ov9282_mode {
> >   * @exp_ctrl: Pointer to exposure control
> >   * @again_ctrl: Pointer to analog gain control
> >   * @pixel_rate: Pointer to pixel rate control
> > + * @flash_led_mode: Pointer to flash led mode control
> >   * @vblank: Vertical blanking in lines
> >   * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
> >   * @cur_mode: Pointer to current selected sensor mode
> > @@ -214,6 +215,7 @@ struct ov9282 {
> >                 struct v4l2_ctrl *again_ctrl;
> >         };
> >         struct v4l2_ctrl *pixel_rate;
> > +       struct v4l2_ctrl *flash_led_mode;
> 
> As with 3/3, you only use this control from within ov9282_set_ctrl
> where you are given the struct v4l2_ctrl, so there is no need to store
> it in the device state structure.

Sure. Thanks for bringing this up. Will fix that in v2.

> 
> >         u32 vblank;
> >         bool noncontinuous_clock;
> >         const struct ov9282_mode *cur_mode;
> > @@ -670,6 +672,23 @@ static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
> >                                 current_val);
> >  }
> >
> > +static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
> > +{
> > +       u32 current_val;
> > +       int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > +                                 &current_val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (mode == V4L2_FLASH_LED_MODE_FLASH)
> > +               current_val |= OV9282_OUTPUT_ENABLE6_STROBE;
> > +       else
> > +               current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE;
> > +
> > +       return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > +                               current_val);
> > +}
> > +
> >  /**
> >   * ov9282_set_ctrl() - Set subdevice control
> >   * @ctrl: pointer to v4l2_ctrl structure
> > @@ -736,6 +755,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >                 ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
> >                                        (ctrl->val + ov9282->cur_mode->width) >> 1);
> >                 break;
> > +       case V4L2_CID_FLASH_LED_MODE:
> > +               ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
> > +               break;
> >         default:
> >                 dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >                 ret = -EINVAL;
> > @@ -1391,6 +1413,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >                                                 OV9282_TIMING_HTS_MAX - mode->width,
> >                                                 1, hblank_min);
> >
> > +       /* Flash/Strobe controls */
> > +       ov9282->flash_led_mode = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> > +                                                       V4L2_CID_FLASH_LED_MODE,
> > +                                                       V4L2_FLASH_LED_MODE_TORCH,
> > +                                                       (1 << V4L2_FLASH_LED_MODE_TORCH),
> > +                                                       V4L2_FLASH_LED_MODE_NONE);
> > +
> >         ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> >         if (!ret) {
> >                 /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> >
> > --
> > 2.47.2
> >
> >

regards;rl

[PATCH 3/3] media: i2c: ov9282: add strobe_timeout v4l2 control

[Richard Leitner]

Add V4L2_CID_FLASH_TIMEOUT support using the "strobe_frame_span"
feature of the sensor. This is implemented by transforming the given µs
value by an interpolated formula to a "span step width" value and
writing it to register PWM_CTRL_25, PWM_CTRL_26, PWM_CTRL_27,
PWM_CTRL_28 (0x3925, 0x3926, 0x3927, 0x3928).

The maximum control value is set to the period of the current framerate.
This must be changed to a dynamic range as soon as this driver
implements the set_frame_interval() pad operation.

All register values are based on the OV9281 datasheet v1.53 (jan 2019)
and tested using an ov9281 VisionComponents module.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c98ba466e9aea29baff0b13578d760bf69c958c5..f7dfe8987e524b73af7e16e12567e96627b4f89a 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -97,6 +97,10 @@
 #define OV9282_REG_MIPI_CTRL00	0x4800
 #define OV9282_GATED_CLOCK	BIT(5)
 
+/* Flash/Strobe control registers */
+#define OV9282_REG_FLASH_DURATION	0x3925
+#define OV9282_FLASH_DURATION_DEFAULT	0x0000001A
+
 /* Input clock rate */
 #define OV9282_INCLK_RATE	24000000
 
@@ -193,6 +197,7 @@ struct ov9282_mode {
  * @again_ctrl: Pointer to analog gain control
  * @pixel_rate: Pointer to pixel rate control
  * @flash_led_mode: Pointer to flash led mode control
+ * @flash_timeout: Pointer to flash timeout control
  * @vblank: Vertical blanking in lines
  * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
  * @cur_mode: Pointer to current selected sensor mode
@@ -216,6 +221,7 @@ struct ov9282 {
 	};
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *flash_led_mode;
+	struct v4l2_ctrl *flash_timeout;
 	u32 vblank;
 	bool noncontinuous_clock;
 	const struct ov9282_mode *cur_mode;
@@ -689,6 +695,24 @@ static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
 				current_val);
 }
 
+static int ov9282_set_ctrl_flash_timeout(struct ov9282 *ov9282, int value)
+{
+	/* Calculate "strobe_frame_span" increments from a given value (µs).
+	 * This is quite tricky as "The step width of shift and span is
+	 * programmable under system clock domain.", but it's not documented
+	 * how to program this step width (at least in the datasheet available
+	 * to the author at time of writing).
+	 * The formula below is interpolated from different modes/framerates
+	 * and should work quite well for most settings.
+	 */
+	u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
+
+	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
+	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
+	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 2, 1, (val >> 8) & 0xff);
+	return ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 3, 1, val & 0xff);
+}
+
 /**
  * ov9282_set_ctrl() - Set subdevice control
  * @ctrl: pointer to v4l2_ctrl structure
@@ -758,6 +782,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_FLASH_LED_MODE:
 		ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
 		break;
+	case V4L2_CID_FLASH_TIMEOUT:
+		ret = ov9282_set_ctrl_flash_timeout(ov9282, ctrl->val);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -1420,6 +1447,10 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 							(1 << V4L2_FLASH_LED_MODE_TORCH),
 							V4L2_FLASH_LED_MODE_NONE);
 
+	ov9282->flash_timeout = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
+						  V4L2_CID_FLASH_TIMEOUT,
+						  0, 13900, 1, 8);
+
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
 		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */

-- 
2.47.2

Re: [PATCH 3/3] media: i2c: ov9282: add strobe_timeout v4l2 control

[Dave Stevenson]

Hi Richard

On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@linux.dev> wrote:
>
> Add V4L2_CID_FLASH_TIMEOUT support using the "strobe_frame_span"
> feature of the sensor. This is implemented by transforming the given µs
> value by an interpolated formula to a "span step width" value and
> writing it to register PWM_CTRL_25, PWM_CTRL_26, PWM_CTRL_27,
> PWM_CTRL_28 (0x3925, 0x3926, 0x3927, 0x3928).
>
> The maximum control value is set to the period of the current framerate.
> This must be changed to a dynamic range as soon as this driver
> implements the set_frame_interval() pad operation.
>
> All register values are based on the OV9281 datasheet v1.53 (jan 2019)
> and tested using an ov9281 VisionComponents module.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/i2c/ov9282.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index c98ba466e9aea29baff0b13578d760bf69c958c5..f7dfe8987e524b73af7e16e12567e96627b4f89a 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -97,6 +97,10 @@
>  #define OV9282_REG_MIPI_CTRL00 0x4800
>  #define OV9282_GATED_CLOCK     BIT(5)
>
> +/* Flash/Strobe control registers */
> +#define OV9282_REG_FLASH_DURATION      0x3925
> +#define OV9282_FLASH_DURATION_DEFAULT  0x0000001A
> +
>  /* Input clock rate */
>  #define OV9282_INCLK_RATE      24000000
>
> @@ -193,6 +197,7 @@ struct ov9282_mode {
>   * @again_ctrl: Pointer to analog gain control
>   * @pixel_rate: Pointer to pixel rate control
>   * @flash_led_mode: Pointer to flash led mode control
> + * @flash_timeout: Pointer to flash timeout control
>   * @vblank: Vertical blanking in lines
>   * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
>   * @cur_mode: Pointer to current selected sensor mode
> @@ -216,6 +221,7 @@ struct ov9282 {
>         };
>         struct v4l2_ctrl *pixel_rate;
>         struct v4l2_ctrl *flash_led_mode;
> +       struct v4l2_ctrl *flash_timeout;

You only access this in ov9282_set_ctrl where you already have the
struct v4l2_ctrl, so there is no need to store this in the main device
state.

  Dave

>         u32 vblank;
>         bool noncontinuous_clock;
>         const struct ov9282_mode *cur_mode;
> @@ -689,6 +695,24 @@ static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
>                                 current_val);
>  }
>
> +static int ov9282_set_ctrl_flash_timeout(struct ov9282 *ov9282, int value)
> +{
> +       /* Calculate "strobe_frame_span" increments from a given value (µs).
> +        * This is quite tricky as "The step width of shift and span is
> +        * programmable under system clock domain.", but it's not documented
> +        * how to program this step width (at least in the datasheet available
> +        * to the author at time of writing).
> +        * The formula below is interpolated from different modes/framerates
> +        * and should work quite well for most settings.
> +        */
> +       u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
> +
> +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
> +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
> +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 2, 1, (val >> 8) & 0xff);
> +       return ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 3, 1, val & 0xff);
> +}
> +
>  /**
>   * ov9282_set_ctrl() - Set subdevice control
>   * @ctrl: pointer to v4l2_ctrl structure
> @@ -758,6 +782,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>         case V4L2_CID_FLASH_LED_MODE:
>                 ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
>                 break;
> +       case V4L2_CID_FLASH_TIMEOUT:
> +               ret = ov9282_set_ctrl_flash_timeout(ov9282, ctrl->val);
> +               break;
>         default:
>                 dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
>                 ret = -EINVAL;
> @@ -1420,6 +1447,10 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>                                                         (1 << V4L2_FLASH_LED_MODE_TORCH),
>                                                         V4L2_FLASH_LED_MODE_NONE);
>
> +       ov9282->flash_timeout = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> +                                                 V4L2_CID_FLASH_TIMEOUT,
> +                                                 0, 13900, 1, 8);
> +
>         ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
>         if (!ret) {
>                 /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
>
> --
> 2.47.2
>
>

Re: [PATCH 3/3] media: i2c: ov9282: add strobe_timeout v4l2 control

[Dave Stevenson]

On Tue, 4 Mar 2025 at 10:28, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Richard
>
> On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@linux.dev> wrote:
> >
> > Add V4L2_CID_FLASH_TIMEOUT support using the "strobe_frame_span"
> > feature of the sensor. This is implemented by transforming the given µs
> > value by an interpolated formula to a "span step width" value and
> > writing it to register PWM_CTRL_25, PWM_CTRL_26, PWM_CTRL_27,
> > PWM_CTRL_28 (0x3925, 0x3926, 0x3927, 0x3928).
> >
> > The maximum control value is set to the period of the current framerate.
> > This must be changed to a dynamic range as soon as this driver
> > implements the set_frame_interval() pad operation.

Rereading the patch as I'm looking at the whole series.

set_frame_interval() will never be implemented as it is the wrong API
for raw sensors.
See https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#raw-camera-sensors
This driver already implements PIXEL_RATE, HBLANK, and VBLANK for
frame interval configuration.

> >
> > All register values are based on the OV9281 datasheet v1.53 (jan 2019)
> > and tested using an ov9281 VisionComponents module.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index c98ba466e9aea29baff0b13578d760bf69c958c5..f7dfe8987e524b73af7e16e12567e96627b4f89a 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -97,6 +97,10 @@
> >  #define OV9282_REG_MIPI_CTRL00 0x4800
> >  #define OV9282_GATED_CLOCK     BIT(5)
> >
> > +/* Flash/Strobe control registers */
> > +#define OV9282_REG_FLASH_DURATION      0x3925
> > +#define OV9282_FLASH_DURATION_DEFAULT  0x0000001A
> > +
> >  /* Input clock rate */
> >  #define OV9282_INCLK_RATE      24000000
> >
> > @@ -193,6 +197,7 @@ struct ov9282_mode {
> >   * @again_ctrl: Pointer to analog gain control
> >   * @pixel_rate: Pointer to pixel rate control
> >   * @flash_led_mode: Pointer to flash led mode control
> > + * @flash_timeout: Pointer to flash timeout control
> >   * @vblank: Vertical blanking in lines
> >   * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
> >   * @cur_mode: Pointer to current selected sensor mode
> > @@ -216,6 +221,7 @@ struct ov9282 {
> >         };
> >         struct v4l2_ctrl *pixel_rate;
> >         struct v4l2_ctrl *flash_led_mode;
> > +       struct v4l2_ctrl *flash_timeout;
>
> You only access this in ov9282_set_ctrl where you already have the
> struct v4l2_ctrl, so there is no need to store this in the main device
> state.
>
>   Dave
>
> >         u32 vblank;
> >         bool noncontinuous_clock;
> >         const struct ov9282_mode *cur_mode;
> > @@ -689,6 +695,24 @@ static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
> >                                 current_val);
> >  }
> >
> > +static int ov9282_set_ctrl_flash_timeout(struct ov9282 *ov9282, int value)
> > +{
> > +       /* Calculate "strobe_frame_span" increments from a given value (µs).
> > +        * This is quite tricky as "The step width of shift and span is
> > +        * programmable under system clock domain.", but it's not documented
> > +        * how to program this step width (at least in the datasheet available
> > +        * to the author at time of writing).
> > +        * The formula below is interpolated from different modes/framerates
> > +        * and should work quite well for most settings.
> > +        */
> > +       u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
> > +
> > +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
> > +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
> > +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 2, 1, (val >> 8) & 0xff);
> > +       return ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 3, 1, val & 0xff);
> > +}
> > +
> >  /**
> >   * ov9282_set_ctrl() - Set subdevice control
> >   * @ctrl: pointer to v4l2_ctrl structure
> > @@ -758,6 +782,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >         case V4L2_CID_FLASH_LED_MODE:
> >                 ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
> >                 break;
> > +       case V4L2_CID_FLASH_TIMEOUT:
> > +               ret = ov9282_set_ctrl_flash_timeout(ov9282, ctrl->val);
> > +               break;
> >         default:
> >                 dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >                 ret = -EINVAL;
> > @@ -1420,6 +1447,10 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >                                                         (1 << V4L2_FLASH_LED_MODE_TORCH),
> >                                                         V4L2_FLASH_LED_MODE_NONE);
> >
> > +       ov9282->flash_timeout = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> > +                                                 V4L2_CID_FLASH_TIMEOUT,
> > +                                                 0, 13900, 1, 8);
> > +
> >         ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> >         if (!ret) {
> >                 /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> >
> > --
> > 2.47.2
> >
> >

Re: [PATCH 3/3] media: i2c: ov9282: add strobe_timeout v4l2 control

[Richard Leitner]

Hi Dave,

On Tue, Mar 04, 2025 at 05:30:13PM +0000, Dave Stevenson wrote:
> On Tue, 4 Mar 2025 at 10:28, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Richard
> >
> > On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@linux.dev> wrote:
> > >
> > > Add V4L2_CID_FLASH_TIMEOUT support using the "strobe_frame_span"
> > > feature of the sensor. This is implemented by transforming the given µs
> > > value by an interpolated formula to a "span step width" value and
> > > writing it to register PWM_CTRL_25, PWM_CTRL_26, PWM_CTRL_27,
> > > PWM_CTRL_28 (0x3925, 0x3926, 0x3927, 0x3928).
> > >
> > > The maximum control value is set to the period of the current framerate.
> > > This must be changed to a dynamic range as soon as this driver
> > > implements the set_frame_interval() pad operation.
> 
> Rereading the patch as I'm looking at the whole series.
> 
> set_frame_interval() will never be implemented as it is the wrong API
> for raw sensors.
> See https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#raw-camera-sensors
> This driver already implements PIXEL_RATE, HBLANK, and VBLANK for
> frame interval configuration.

Thanks Dave for bringing this up and sorry for not having known that
before posting this series. It's my first work on camera sensors/v4l2
and altough I have read a ton of docs I somehow missed this one.

So regarding this patch: Should the maximum flash duration be calculated
from the frame interval (as described in the docs)?
Or is a "pretty high" constant maximum value preferred?

regards;rl

> 
> > >
> > > All register values are based on the OV9281 datasheet v1.53 (jan 2019)
> > > and tested using an ov9281 VisionComponents module.
> > >
> > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index c98ba466e9aea29baff0b13578d760bf69c958c5..f7dfe8987e524b73af7e16e12567e96627b4f89a 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -97,6 +97,10 @@
> > >  #define OV9282_REG_MIPI_CTRL00 0x4800
> > >  #define OV9282_GATED_CLOCK     BIT(5)
> > >
> > > +/* Flash/Strobe control registers */
> > > +#define OV9282_REG_FLASH_DURATION      0x3925
> > > +#define OV9282_FLASH_DURATION_DEFAULT  0x0000001A
> > > +
> > >  /* Input clock rate */
> > >  #define OV9282_INCLK_RATE      24000000
> > >
> > > @@ -193,6 +197,7 @@ struct ov9282_mode {
> > >   * @again_ctrl: Pointer to analog gain control
> > >   * @pixel_rate: Pointer to pixel rate control
> > >   * @flash_led_mode: Pointer to flash led mode control
> > > + * @flash_timeout: Pointer to flash timeout control
> > >   * @vblank: Vertical blanking in lines
> > >   * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
> > >   * @cur_mode: Pointer to current selected sensor mode
> > > @@ -216,6 +221,7 @@ struct ov9282 {
> > >         };
> > >         struct v4l2_ctrl *pixel_rate;
> > >         struct v4l2_ctrl *flash_led_mode;
> > > +       struct v4l2_ctrl *flash_timeout;
> >
> > You only access this in ov9282_set_ctrl where you already have the
> > struct v4l2_ctrl, so there is no need to store this in the main device
> > state.
> >
> >   Dave
> >
> > >         u32 vblank;
> > >         bool noncontinuous_clock;
> > >         const struct ov9282_mode *cur_mode;
> > > @@ -689,6 +695,24 @@ static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
> > >                                 current_val);
> > >  }
> > >
> > > +static int ov9282_set_ctrl_flash_timeout(struct ov9282 *ov9282, int value)
> > > +{
> > > +       /* Calculate "strobe_frame_span" increments from a given value (µs).
> > > +        * This is quite tricky as "The step width of shift and span is
> > > +        * programmable under system clock domain.", but it's not documented
> > > +        * how to program this step width (at least in the datasheet available
> > > +        * to the author at time of writing).
> > > +        * The formula below is interpolated from different modes/framerates
> > > +        * and should work quite well for most settings.
> > > +        */
> > > +       u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
> > > +
> > > +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
> > > +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
> > > +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 2, 1, (val >> 8) & 0xff);
> > > +       return ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 3, 1, val & 0xff);
> > > +}
> > > +
> > >  /**
> > >   * ov9282_set_ctrl() - Set subdevice control
> > >   * @ctrl: pointer to v4l2_ctrl structure
> > > @@ -758,6 +782,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> > >         case V4L2_CID_FLASH_LED_MODE:
> > >                 ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
> > >                 break;
> > > +       case V4L2_CID_FLASH_TIMEOUT:
> > > +               ret = ov9282_set_ctrl_flash_timeout(ov9282, ctrl->val);
> > > +               break;
> > >         default:
> > >                 dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> > >                 ret = -EINVAL;
> > > @@ -1420,6 +1447,10 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > >                                                         (1 << V4L2_FLASH_LED_MODE_TORCH),
> > >                                                         V4L2_FLASH_LED_MODE_NONE);
> > >
> > > +       ov9282->flash_timeout = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> > > +                                                 V4L2_CID_FLASH_TIMEOUT,
> > > +                                                 0, 13900, 1, 8);
> > > +
> > >         ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > >         if (!ret) {
> > >                 /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> > >
> > > --
> > > 2.47.2
> > >
> > >

Re: [PATCH 3/3] media: i2c: ov9282: add strobe_timeout v4l2 control

[Richard Leitner]

On Tue, Mar 04, 2025 at 10:28:45AM +0000, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@linux.dev> wrote:
> >
> > Add V4L2_CID_FLASH_TIMEOUT support using the "strobe_frame_span"
> > feature of the sensor. This is implemented by transforming the given µs
> > value by an interpolated formula to a "span step width" value and
> > writing it to register PWM_CTRL_25, PWM_CTRL_26, PWM_CTRL_27,
> > PWM_CTRL_28 (0x3925, 0x3926, 0x3927, 0x3928).
> >
> > The maximum control value is set to the period of the current framerate.
> > This must be changed to a dynamic range as soon as this driver
> > implements the set_frame_interval() pad operation.
> >
> > All register values are based on the OV9281 datasheet v1.53 (jan 2019)
> > and tested using an ov9281 VisionComponents module.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index c98ba466e9aea29baff0b13578d760bf69c958c5..f7dfe8987e524b73af7e16e12567e96627b4f89a 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -97,6 +97,10 @@
> >  #define OV9282_REG_MIPI_CTRL00 0x4800
> >  #define OV9282_GATED_CLOCK     BIT(5)
> >
> > +/* Flash/Strobe control registers */
> > +#define OV9282_REG_FLASH_DURATION      0x3925
> > +#define OV9282_FLASH_DURATION_DEFAULT  0x0000001A
> > +
> >  /* Input clock rate */
> >  #define OV9282_INCLK_RATE      24000000
> >
> > @@ -193,6 +197,7 @@ struct ov9282_mode {
> >   * @again_ctrl: Pointer to analog gain control
> >   * @pixel_rate: Pointer to pixel rate control
> >   * @flash_led_mode: Pointer to flash led mode control
> > + * @flash_timeout: Pointer to flash timeout control
> >   * @vblank: Vertical blanking in lines
> >   * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
> >   * @cur_mode: Pointer to current selected sensor mode
> > @@ -216,6 +221,7 @@ struct ov9282 {
> >         };
> >         struct v4l2_ctrl *pixel_rate;
> >         struct v4l2_ctrl *flash_led_mode;
> > +       struct v4l2_ctrl *flash_timeout;
> 
> You only access this in ov9282_set_ctrl where you already have the
> struct v4l2_ctrl, so there is no need to store this in the main device
> state.

Sure. Thanks for reviewing and pointing this out. Will fix that in v2.

(At least I was consistent and made the same mistakes for both controls ;-))

regards;rl

> 
>   Dave
> 
> >         u32 vblank;
> >         bool noncontinuous_clock;
> >         const struct ov9282_mode *cur_mode;
> > @@ -689,6 +695,24 @@ static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
> >                                 current_val);
> >  }
> >
> > +static int ov9282_set_ctrl_flash_timeout(struct ov9282 *ov9282, int value)
> > +{
> > +       /* Calculate "strobe_frame_span" increments from a given value (µs).
> > +        * This is quite tricky as "The step width of shift and span is
> > +        * programmable under system clock domain.", but it's not documented
> > +        * how to program this step width (at least in the datasheet available
> > +        * to the author at time of writing).
> > +        * The formula below is interpolated from different modes/framerates
> > +        * and should work quite well for most settings.
> > +        */
> > +       u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
> > +
> > +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
> > +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
> > +       ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 2, 1, (val >> 8) & 0xff);
> > +       return ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 3, 1, val & 0xff);
> > +}
> > +
> >  /**
> >   * ov9282_set_ctrl() - Set subdevice control
> >   * @ctrl: pointer to v4l2_ctrl structure
> > @@ -758,6 +782,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >         case V4L2_CID_FLASH_LED_MODE:
> >                 ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
> >                 break;
> > +       case V4L2_CID_FLASH_TIMEOUT:
> > +               ret = ov9282_set_ctrl_flash_timeout(ov9282, ctrl->val);
> > +               break;
> >         default:
> >                 dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >                 ret = -EINVAL;
> > @@ -1420,6 +1447,10 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >                                                         (1 << V4L2_FLASH_LED_MODE_TORCH),
> >                                                         V4L2_FLASH_LED_MODE_NONE);
> >
> > +       ov9282->flash_timeout = v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> > +                                                 V4L2_CID_FLASH_TIMEOUT,
> > +                                                 0, 13900, 1, 8);
> > +
> >         ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> >         if (!ret) {
> >                 /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> >
> > --
> > 2.47.2
> >
> >

Re: [PATCH 0/3] Add flash/strobe support for ov9282

[Dave Stevenson]

Hi Richard

Thanks for the series.

On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@linux.dev> wrote:
>
> This series adds basic flash/strobe support for ov9282 sensors using
> their "hardware strobe output".
>
> Apart from en-/disabling the flash/strobe output, setting a timeout
> (duration of activated strobe per frame) is implemented. The calculation
> of this timeout is only interpolated from various measurements, as no
> documentation was found.

The bigger picture question is whether using these flash controls is
appropriate for controlling the strobe output on a sensor. That's a
question for others (mainly Sakari and Laurent).

V4L2_CID_FLASH_TIMEOUT feels wrong for setting the duration of the strobe pulse.
Whilst the description in the docs [1] is a little brief, you then
have the description of V4L2_FLASH_FAULT_TIMEOUT for
V4L2_CID_FLASH_FAULT
"The flash strobe was still on when the timeout set by the user ---
V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
controllers may set this in all such conditions."
which implies it is the hardware watchdog timeout to ensure the flash
LEDs don't burn out, not configuring the duration of the flash pulse.
Then again adp1653 adopts it as the flash duration.

Is there an expectation that V4L2_CID_FLASH_STROBE_SOURCE should also
be implemented, even if it is fixed to
V4L2_FLASH_STROBE_SOURCE_EXTERNAL?

The one saving grace with this sensor is that it has a global shutter,
so the strobe does correspond to the exposure period. With rolling
shutter sensors, the flash duration is typically two frames to cover
the exposure duration of all lines as the shutter rolls down.

  Dave

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html

> Further flash/strobe-related controls as well as a migration to v4l2-cci
> helpers will likely be implemented in future series.
>
> All register addresses/values are based on the OV9281 datasheet v1.53
> (january 2019). This series was tested using an ov9281 VisionComponents
> camera module.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
> Richard Leitner (3):
>       media: i2c: ov9282: add output enable register definitions
>       media: i2c: ov9282: add led_mode v4l2 control
>       media: i2c: ov9282: add strobe_timeout v4l2 control
>
>  drivers/media/i2c/ov9282.c | 89 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 86 insertions(+), 3 deletions(-)
> ---
> base-commit: f41427b3bdee7d9845b13a80c0d03882212f4b20
> change-id: 20250303-ov9282-flash-strobe-ac6bd00c9de6
> prerequisite-change-id: 20250225-b4-ov9282-gain-ef1cdaba5bfd:v1
> prerequisite-patch-id: 86f2582378ff7095ab65ce4bb25a143eb639e840
> prerequisite-patch-id: b06eb6ec697aaf0b3155b4b2370f171d0d304ae2
> prerequisite-patch-id: b123047d71bfb9b93f743bbdd6893d5a98495801
>
> Best regards,
> --
> Richard Leitner <richard.leitner@linux.dev>
>
>

Re: [PATCH 0/3] Add flash/strobe support for ov9282

[Richard Leitner]

On Tue, Mar 04, 2025 at 05:46:34PM +0000, Dave Stevenson wrote:
> Hi Richard
> 
> Thanks for the series.

Hi Dave,

thanks for your quick and detailled review!

> 
> On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@linux.dev> wrote:
> >
> > This series adds basic flash/strobe support for ov9282 sensors using
> > their "hardware strobe output".
> >
> > Apart from en-/disabling the flash/strobe output, setting a timeout
> > (duration of activated strobe per frame) is implemented. The calculation
> > of this timeout is only interpolated from various measurements, as no
> > documentation was found.
> 
> The bigger picture question is whether using these flash controls is
> appropriate for controlling the strobe output on a sensor. That's a
> question for others (mainly Sakari and Laurent).

Thanks. So I'm looking forward to their response :-)

> V4L2_CID_FLASH_TIMEOUT feels wrong for setting the duration of the strobe pulse.
> Whilst the description in the docs [1] is a little brief, you then
> have the description of V4L2_FLASH_FAULT_TIMEOUT for
> V4L2_CID_FLASH_FAULT
> "The flash strobe was still on when the timeout set by the user ---
> V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> controllers may set this in all such conditions."
> which implies it is the hardware watchdog timeout to ensure the flash
> LEDs don't burn out, not configuring the duration of the flash pulse.
> Then again adp1653 adopts it as the flash duration.

I also thought of (and in fact did) implementing this using sensor
specific used CIDs, but then decided to go for FLASH_TIMEOUT.

If you think it's a better way to introduce either a completely new
"common control" or use another one I'm perfectly fine with that and
will try that for a v2.

> 
> Is there an expectation that V4L2_CID_FLASH_STROBE_SOURCE should also
> be implemented, even if it is fixed to
> V4L2_FLASH_STROBE_SOURCE_EXTERNAL?

I've already done this in my local tree, but was not sure if it "fits"
in this series...

So I guess I should include it in v2?

> 
> The one saving grace with this sensor is that it has a global shutter,
> so the strobe does correspond to the exposure period. With rolling
> shutter sensors, the flash duration is typically two frames to cover
> the exposure duration of all lines as the shutter rolls down.

Totally agree. Without global shutter configuring the flash duration
would not make that much sense.

Just to have mentioned it: I tested this quite heavily using an ov9281,
ran analysis on the resulting images and did lots of scope measurements
to make sure it really works as described.

regards;rl

> 
>   Dave
> 
> [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> 
> > Further flash/strobe-related controls as well as a migration to v4l2-cci
> > helpers will likely be implemented in future series.
> >
> > All register addresses/values are based on the OV9281 datasheet v1.53
> > (january 2019). This series was tested using an ov9281 VisionComponents
> > camera module.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> > Richard Leitner (3):
> >       media: i2c: ov9282: add output enable register definitions
> >       media: i2c: ov9282: add led_mode v4l2 control
> >       media: i2c: ov9282: add strobe_timeout v4l2 control
> >
> >  drivers/media/i2c/ov9282.c | 89 ++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 86 insertions(+), 3 deletions(-)
> > ---
> > base-commit: f41427b3bdee7d9845b13a80c0d03882212f4b20
> > change-id: 20250303-ov9282-flash-strobe-ac6bd00c9de6
> > prerequisite-change-id: 20250225-b4-ov9282-gain-ef1cdaba5bfd:v1
> > prerequisite-patch-id: 86f2582378ff7095ab65ce4bb25a143eb639e840
> > prerequisite-patch-id: b06eb6ec697aaf0b3155b4b2370f171d0d304ae2
> > prerequisite-patch-id: b123047d71bfb9b93f743bbdd6893d5a98495801
> >
> > Best regards,
> > --
> > Richard Leitner <richard.leitner@linux.dev>
> >
> >