Re: [PATCH v5 06/10] media: i2c: ov9282: add led_mode v4l2 control

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Richard,

On Fri, Jul 11, 2025 at 09:41:52AM +0200, Richard Leitner wrote:
> Hi Sakari,
> 
> On Thu, Jul 10, 2025 at 08:14:07AM +0000, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > On Thu, Jul 10, 2025 at 08:50:24AM +0200, Richard Leitner wrote:
> > > Hi Sakari,
> > > 
> > > thanks for the feedback :)
> > > 
> > > On Wed, Jul 09, 2025 at 09:12:57PM +0000, Sakari Ailus wrote:
> > > > Hi Richard,
> > > > 
> > > > Thanks for the update.
> > > > 
> > > > On Tue, Jun 17, 2025 at 09:31:40AM +0200, Richard Leitner 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
> > > > 
> > > > I really think you should use a different control for this. The sensor can
> > > > strobe the flash but it won't control its mode.
> > > > 
> > > > How about calling it V4L2_FLASH_STROBE_ENABLE?
> > > 
> > > I agree on that. But tbh V4L2_FLASH_STROBE_ENABLE somehow sounds wrong
> > > to me. As the strobe output in the ov9282 case goes high for the strobe
> > > duration, what do you think about calling it V4L2_FLASH_STROBE_PULSE?
> > 
> > That's how the hardware strobe is supposed to work, there's nothing unusual
> > in that. How about V4L2_FLASH_HW_STROBE_ENABLE?
> 
> Ah. Sorry. I believe I completely misunderstood your previous point.
> You're not referring to a new menu entry in V4L2_CID_FLASH_LED_MODE,
> but rather proposing a completely new boolean control, correct?

Correct.

> 
> As this would be separating the V4L2_CID's of "strobe signal source"
> (aka sensor) and "strobe signal consumer" (aka flash device/LEDs), that
> makes perfect sense to me now! :)
> 
> What are your thoughts on naming it V4L2_FLASH_HW_STROBE_SIGNAL?

Seems reasonable.

In order to use the control, the user space would need to know when to use
it, i.e. when the latching point would be in order to hit a particular
frame. If the strobe can start before the exposure (and presumably it needs
to), the latching point is before that point of time. I wonder if pixels
(as in the pixel array) would be reasonable unit for this as well.

Does the sensor datasheet shed any light on this? It might be good to add a
control for that as well.

> 
> The main reason behind removing the "ENABLE" suffix is that none of
> the V4L2_CID_FLASH_* controls currently include "ENABLE" in their
> names. Altough, for example, V4L2_CID_FLASH_CHARGE performs a
> similar function (en-/disabling the charging of the capacitor).
> 
> On the other hand, adding "SIGNAL" to the control name, in my opinion,
> makes it clearer that it only enables a signal and not some kind of
> flash device or LED.

-- 
Kind regards,

Sakari Ailus