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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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