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

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

Richard Leitner posted 10 patches 3 months, 3 weeks ago
[PATCH v5 06/10] media: i2c: ov9282: add led_mode v4l2 control
Posted by Richard Leitner 3 months, 3 weeks ago 
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, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index f42e0d439753e74d14e3a3592029e48f49234927..b6de96997426f7225a061bfdc841aa062e8d0891 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -670,6 +670,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 +753,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;
@@ -1326,7 +1346,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
 	if (ret)
 		return ret;
 
@@ -1391,6 +1411,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 						OV9282_TIMING_HTS_MAX - mode->width,
 						1, hblank_min);
 
+	/* Flash/Strobe controls */
+	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 v5 06/10] media: i2c: ov9282: add led_mode v4l2 control
Posted by Sakari Ailus 3 months ago 
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?

-- 
Kind regards,

Sakari Ailus
Re: [PATCH v5 06/10] media: i2c: ov9282: add led_mode v4l2 control
Posted by Richard Leitner 3 months ago 
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?
Or should it be V4L2_FLASH_LED_MODE_STROBE_PULSE?

Regarding disabling the strobe output: Is sticking with V4L2_FLASH_LED_MODE_NONE
fine? Or do you prefer an additional V4L2_FLASH_(MODE_)STROBE_DISABLE
or something similar?

regards;rl

> 
> -- 
> Kind regards,
> 
> Sakari Ailus
Re: [PATCH v5 06/10] media: i2c: ov9282: add led_mode v4l2 control
Posted by Sakari Ailus 3 months ago 
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?

> Or should it be V4L2_FLASH_LED_MODE_STROBE_PULSE?
> 
> Regarding disabling the strobe output: Is sticking with V4L2_FLASH_LED_MODE_NONE
> fine? Or do you prefer an additional V4L2_FLASH_(MODE_)STROBE_DISABLE
> or something similar?

This isn't about the LED flash mode and we shouldn't suggest it is.

-- 
Regards,

Sakari Ailus
Re: [PATCH v5 06/10] media: i2c: ov9282: add led_mode v4l2 control
Posted by Richard Leitner 3 months ago 
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?

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?

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.

> 
> > Or should it be V4L2_FLASH_LED_MODE_STROBE_PULSE?
> > 
> > Regarding disabling the strobe output: Is sticking with V4L2_FLASH_LED_MODE_NONE
> > fine? Or do you prefer an additional V4L2_FLASH_(MODE_)STROBE_DISABLE
> > or something similar?
> 
> This isn't about the LED flash mode and we shouldn't suggest it is.

Thanks for the clarification. I guess I got your point now :)

regards;rl

> 
> -- 
> Regards,
> 
> Sakari Ailus
Re: [PATCH v5 06/10] media: i2c: ov9282: add led_mode v4l2 control
Posted by Sakari Ailus 2 months, 1 week ago 
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
Re: [PATCH v5 06/10] media: i2c: ov9282: add led_mode v4l2 control
Posted by Richard Leitner 2 months ago 
On Wed, Jul 30, 2025 at 08:39:46PM +0000, Sakari Ailus wrote:
> 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.

Thanks again for your response. I already sent out a v6 with that
control name. I hope that's fine.

> 
> 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.

I'm not sure if pixels is a good unit for that. The strobe duration and
strobe timeout are both defined as µs. Therefore I would prefer to also
use µs for the strobe shift/offset.

The sensor features a control named "strobe shift" which allows to move the
point of time when the strobe starts before/after the start of the exposure.

I've named it V4L2_FLASH_HW_STROBE_SIGNAL_SHIFT in my downstream tree.
What do you think about it?

I have had planned to submit support for the strobe shift/offset control
as soon as this series got accepted. Mainly to keept the series smaller
and easier to handle (at least for me). Tbh I still want to stick to that
approach. Is that fine with you? Or should I include those patches
in this series?

> 
> > 
> > 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

Thanks again for reviewing the series!

regards;rl

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.