1. Patchew
  2. linux
  3. [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282
  4. View patch

[PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration

Richard Leitner posted 10 patches 1 month ago
[PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration
Posted by Richard Leitner 1 month ago 
Add support for the new V4L2_CID_FLASH_DURATION control to the v4l2
flash led class.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/v4l2-core/v4l2-flash-led-class.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
index 355595a0fefac72c2f6941a30fa430d37dbdccfe..875d56d7190592c1e5ab7acd617b76dcec8792da 100644
--- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
+++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
@@ -29,6 +29,7 @@ enum ctrl_init_data_id {
 	INDICATOR_INTENSITY,
 	FLASH_TIMEOUT,
 	STROBE_SOURCE,
+	FLASH_DURATION,
 	/*
 	 * Only above values are applicable to
 	 * the 'ctrls' array in the struct v4l2_flash.
@@ -298,6 +299,12 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
 		 * microamperes for flash intensity units.
 		 */
 		return led_set_flash_brightness(fled_cdev, c->val);
+	case V4L2_CID_FLASH_DURATION:
+		/*
+		 * No conversion is needed as LED Flash class also uses
+		 * microseconds for flash duration units.
+		 */
+		return led_set_flash_duration(fled_cdev, c->val);
 	}
 
 	return -EINVAL;
@@ -424,6 +431,14 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
 		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
 				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
 	}
+
+	/* Init FLASH_DURATION ctrl data */
+	if (has_flash_op(fled_cdev, duration_set)) {
+		ctrl_init_data[FLASH_DURATION].cid = V4L2_CID_FLASH_DURATION;
+		ctrl_cfg = &ctrl_init_data[FLASH_DURATION].config;
+		__lfs_to_v4l2_ctrl_config(&fled_cdev->duration, ctrl_cfg);
+		ctrl_cfg->id = V4L2_CID_FLASH_DURATION;
+	}
 }
 
 static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash,
@@ -543,6 +558,16 @@ static int __sync_device_with_v4l2_controls(struct v4l2_flash *v4l2_flash)
 			return ret;
 	}
 
+	if (ctrls[FLASH_DURATION]) {
+		if (WARN_ON_ONCE(!fled_cdev))
+			return -EINVAL;
+
+		ret = led_set_flash_duration(fled_cdev,
+					     ctrls[FLASH_DURATION]->val);
+		if (ret < 0)
+			return ret;
+	}
+
 	/*
 	 * For some hardware arrangements setting strobe source may affect
 	 * torch mode. Synchronize strobe source setting only if not in torch

-- 
2.47.2
Re: [PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration
Posted by Laurent Pinchart 3 weeks, 4 days ago 
Hi Richard,

Thank you for the patch.

On Mon, Sep 01, 2025 at 05:05:07PM +0200, Richard Leitner wrote:
> Add support for the new V4L2_CID_FLASH_DURATION control to the v4l2
> flash led class.

I don't think this is a good idea, based on the reasoning explained in
the review of 01/10. If V4L2_CID_FLASH_DURATION is meant to indicate the
duration of the external flash/strobe pulse signal, it should be
implemented by the source of the signal, and for external strobe mode
only. The flash controller, which receives the flash/strobe pulse,
should implement the timeout control.

> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index 355595a0fefac72c2f6941a30fa430d37dbdccfe..875d56d7190592c1e5ab7acd617b76dcec8792da 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -29,6 +29,7 @@ enum ctrl_init_data_id {
>  	INDICATOR_INTENSITY,
>  	FLASH_TIMEOUT,
>  	STROBE_SOURCE,
> +	FLASH_DURATION,
>  	/*
>  	 * Only above values are applicable to
>  	 * the 'ctrls' array in the struct v4l2_flash.
> @@ -298,6 +299,12 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
>  		 * microamperes for flash intensity units.
>  		 */
>  		return led_set_flash_brightness(fled_cdev, c->val);
> +	case V4L2_CID_FLASH_DURATION:
> +		/*
> +		 * No conversion is needed as LED Flash class also uses
> +		 * microseconds for flash duration units.
> +		 */
> +		return led_set_flash_duration(fled_cdev, c->val);
>  	}
>  
>  	return -EINVAL;
> @@ -424,6 +431,14 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
>  		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
>  				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>  	}
> +
> +	/* Init FLASH_DURATION ctrl data */
> +	if (has_flash_op(fled_cdev, duration_set)) {
> +		ctrl_init_data[FLASH_DURATION].cid = V4L2_CID_FLASH_DURATION;
> +		ctrl_cfg = &ctrl_init_data[FLASH_DURATION].config;
> +		__lfs_to_v4l2_ctrl_config(&fled_cdev->duration, ctrl_cfg);
> +		ctrl_cfg->id = V4L2_CID_FLASH_DURATION;
> +	}
>  }
>  
>  static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash,
> @@ -543,6 +558,16 @@ static int __sync_device_with_v4l2_controls(struct v4l2_flash *v4l2_flash)
>  			return ret;
>  	}
>  
> +	if (ctrls[FLASH_DURATION]) {
> +		if (WARN_ON_ONCE(!fled_cdev))
> +			return -EINVAL;
> +
> +		ret = led_set_flash_duration(fled_cdev,
> +					     ctrls[FLASH_DURATION]->val);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	/*
>  	 * For some hardware arrangements setting strobe source may affect
>  	 * torch mode. Synchronize strobe source setting only if not in torch

-- 
Regards,

Laurent Pinchart
Re: [PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration
Posted by Richard Leitner 3 weeks, 3 days ago 
Hi Laurent,

On Sun, Sep 07, 2025 at 09:05:20PM +0200, Laurent Pinchart wrote:
> Hi Richard,
> 
> Thank you for the patch.
> 
> On Mon, Sep 01, 2025 at 05:05:07PM +0200, Richard Leitner wrote:
> > Add support for the new V4L2_CID_FLASH_DURATION control to the v4l2
> > flash led class.
> 
> I don't think this is a good idea, based on the reasoning explained in
> the review of 01/10. If V4L2_CID_FLASH_DURATION is meant to indicate the
> duration of the external flash/strobe pulse signal, it should be
> implemented by the source of the signal, and for external strobe mode
> only. The flash controller, which receives the flash/strobe pulse,
> should implement the timeout control.

You're right. From that point of view it makes no sense to have this
functions in v4l2-flash-led-class.c. I'll move the implementation to
ov9282 for v9.

If I do so I guess the patch already merged by Lee needs reverting?
6a09ae828198 (leds: flash: Add support for flash/strobe duration, 2025-05-07)
Should the revert be included in this series then?

> 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/v4l2-core/v4l2-flash-led-class.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > index 355595a0fefac72c2f6941a30fa430d37dbdccfe..875d56d7190592c1e5ab7acd617b76dcec8792da 100644
> > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > @@ -29,6 +29,7 @@ enum ctrl_init_data_id {
> >  	INDICATOR_INTENSITY,
> >  	FLASH_TIMEOUT,
> >  	STROBE_SOURCE,
> > +	FLASH_DURATION,
> >  	/*
> >  	 * Only above values are applicable to
> >  	 * the 'ctrls' array in the struct v4l2_flash.
> > @@ -298,6 +299,12 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> >  		 * microamperes for flash intensity units.
> >  		 */
> >  		return led_set_flash_brightness(fled_cdev, c->val);
> > +	case V4L2_CID_FLASH_DURATION:
> > +		/*
> > +		 * No conversion is needed as LED Flash class also uses
> > +		 * microseconds for flash duration units.
> > +		 */
> > +		return led_set_flash_duration(fled_cdev, c->val);
> >  	}
> >  
> >  	return -EINVAL;
> > @@ -424,6 +431,14 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
> >  		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
> >  				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> >  	}
> > +
> > +	/* Init FLASH_DURATION ctrl data */
> > +	if (has_flash_op(fled_cdev, duration_set)) {
> > +		ctrl_init_data[FLASH_DURATION].cid = V4L2_CID_FLASH_DURATION;
> > +		ctrl_cfg = &ctrl_init_data[FLASH_DURATION].config;
> > +		__lfs_to_v4l2_ctrl_config(&fled_cdev->duration, ctrl_cfg);
> > +		ctrl_cfg->id = V4L2_CID_FLASH_DURATION;
> > +	}
> >  }
> >  
> >  static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash,
> > @@ -543,6 +558,16 @@ static int __sync_device_with_v4l2_controls(struct v4l2_flash *v4l2_flash)
> >  			return ret;
> >  	}
> >  
> > +	if (ctrls[FLASH_DURATION]) {
> > +		if (WARN_ON_ONCE(!fled_cdev))
> > +			return -EINVAL;
> > +
> > +		ret = led_set_flash_duration(fled_cdev,
> > +					     ctrls[FLASH_DURATION]->val);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> >  	/*
> >  	 * For some hardware arrangements setting strobe source may affect
> >  	 * torch mode. Synchronize strobe source setting only if not in torch
> 
> -- 
> Regards,
> 
> Laurent Pinchart

thanks again for your review!

regards;rl
Re: [PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration
Posted by Laurent Pinchart 3 weeks, 3 days ago 
On Mon, Sep 08, 2025 at 04:15:48PM +0200, Richard Leitner wrote:
> On Sun, Sep 07, 2025 at 09:05:20PM +0200, Laurent Pinchart wrote:
> > On Mon, Sep 01, 2025 at 05:05:07PM +0200, Richard Leitner wrote:
> > > Add support for the new V4L2_CID_FLASH_DURATION control to the v4l2
> > > flash led class.
> > 
> > I don't think this is a good idea, based on the reasoning explained in
> > the review of 01/10. If V4L2_CID_FLASH_DURATION is meant to indicate the
> > duration of the external flash/strobe pulse signal, it should be
> > implemented by the source of the signal, and for external strobe mode
> > only. The flash controller, which receives the flash/strobe pulse,
> > should implement the timeout control.
> 
> You're right. From that point of view it makes no sense to have this
> functions in v4l2-flash-led-class.c. I'll move the implementation to
> ov9282 for v9.
> 
> If I do so I guess the patch already merged by Lee needs reverting?
> 6a09ae828198 (leds: flash: Add support for flash/strobe duration, 2025-05-07)
> Should the revert be included in this series then?

The revert can be merged separately. Let's first finalize this series,
and then send the revert, just in case over changes to the LED API may
be needed.

> > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-flash-led-class.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > index 355595a0fefac72c2f6941a30fa430d37dbdccfe..875d56d7190592c1e5ab7acd617b76dcec8792da 100644
> > > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > @@ -29,6 +29,7 @@ enum ctrl_init_data_id {
> > >  	INDICATOR_INTENSITY,
> > >  	FLASH_TIMEOUT,
> > >  	STROBE_SOURCE,
> > > +	FLASH_DURATION,
> > >  	/*
> > >  	 * Only above values are applicable to
> > >  	 * the 'ctrls' array in the struct v4l2_flash.
> > > @@ -298,6 +299,12 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > >  		 * microamperes for flash intensity units.
> > >  		 */
> > >  		return led_set_flash_brightness(fled_cdev, c->val);
> > > +	case V4L2_CID_FLASH_DURATION:
> > > +		/*
> > > +		 * No conversion is needed as LED Flash class also uses
> > > +		 * microseconds for flash duration units.
> > > +		 */
> > > +		return led_set_flash_duration(fled_cdev, c->val);
> > >  	}
> > >  
> > >  	return -EINVAL;
> > > @@ -424,6 +431,14 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
> > >  		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
> > >  				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> > >  	}
> > > +
> > > +	/* Init FLASH_DURATION ctrl data */
> > > +	if (has_flash_op(fled_cdev, duration_set)) {
> > > +		ctrl_init_data[FLASH_DURATION].cid = V4L2_CID_FLASH_DURATION;
> > > +		ctrl_cfg = &ctrl_init_data[FLASH_DURATION].config;
> > > +		__lfs_to_v4l2_ctrl_config(&fled_cdev->duration, ctrl_cfg);
> > > +		ctrl_cfg->id = V4L2_CID_FLASH_DURATION;
> > > +	}
> > >  }
> > >  
> > >  static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash,
> > > @@ -543,6 +558,16 @@ static int __sync_device_with_v4l2_controls(struct v4l2_flash *v4l2_flash)
> > >  			return ret;
> > >  	}
> > >  
> > > +	if (ctrls[FLASH_DURATION]) {
> > > +		if (WARN_ON_ONCE(!fled_cdev))
> > > +			return -EINVAL;
> > > +
> > > +		ret = led_set_flash_duration(fled_cdev,
> > > +					     ctrls[FLASH_DURATION]->val);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > > +
> > >  	/*
> > >  	 * For some hardware arrangements setting strobe source may affect
> > >  	 * torch mode. Synchronize strobe source setting only if not in torch

-- 
Regards,

Laurent Pinchart

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.