* [PATCH v7 01/10] media: v4l: ctrls: add a control for flash/strobe duration
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-07 18:55 ` Laurent Pinchart
2025-09-01 15:05 ` [PATCH v7 02/10] media: v4l2-flash: add support " Richard Leitner
` (8 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
Add a control V4L2_CID_FLASH_DURATION to set the duration of a
flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
control, as the timeout defines a limit after which the flash is
"forcefully" turned off again.
On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
of the flash/strobe pulse.
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
include/uapi/linux/v4l2-controls.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_FLASH_FAULT: return "Faults";
case V4L2_CID_FLASH_CHARGE: return "Charge";
case V4L2_CID_FLASH_READY: return "Ready to Strobe";
+ case V4L2_CID_FLASH_DURATION: return "Strobe Duration";
/* JPEG encoder controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index f836512e9debbc65d62a9fe04069b056be42f7b2..a5b7c382d77118eb7966385c5b22d5a89bc2b272 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1186,6 +1186,7 @@ enum v4l2_flash_strobe_source {
#define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11)
#define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12)
+#define V4L2_CID_FLASH_DURATION (V4L2_CID_FLASH_CLASS_BASE + 13)
/* JPEG-class control IDs */
--
2.47.2
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 01/10] media: v4l: ctrls: add a control for flash/strobe duration
2025-09-01 15:05 ` [PATCH v7 01/10] media: v4l: ctrls: add a control for flash/strobe duration Richard Leitner
@ 2025-09-07 18:55 ` Laurent Pinchart
2025-09-08 14:41 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-07 18:55 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Richard,
Thank you for the patch.
On Mon, Sep 01, 2025 at 05:05:06PM +0200, Richard Leitner wrote:
> Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> control, as the timeout defines a limit after which the flash is
> "forcefully" turned off again.
>
> On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> of the flash/strobe pulse.
It took me a while to understand the difference between the
V4L2_CID_FLASH_TIMEOUT and V4L2_CID_FLASH_DURATION controls, as I
wondered how a device could implement different duration and timeout
values. Then I realized that the timeout control is meant for flash
controllers, while the duration control is meant for the source of the
flash controller's external hardware strobe signal, typically the camera
sensor. I'd like this to be more explicit, here and in the
documentation. Here's a proposal for an updated commit message:
----
Add a V4L2_CID_FLASH_DURATION control to set the duration of a
flash/strobe pulse. This controls the length of the flash/strobe pulse
output by device (typically a camera sensor) and connected to the flash
controller. This is different to the V4L2_CID_FLASH_TIMEOUT control,
which is implemented by the flash controller and defines a limit after
which the flash is "forcefully" turned off again.
----
This could probably be improved, but it's good enough for me for the
commit message.
On a side note, I think we could have reused the V4L2_CID_FLASH_TIMEOUT
control for this purpose, even if the name isn't the best match, as the
two usages are implemented on different devices (flash controller vs.
camera sensor). We have no shortage of control ID space, so a separate
control ID is fine too, and probably clearer (as long as we document it
clearly).
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> include/uapi/linux/v4l2-controls.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_FLASH_FAULT: return "Faults";
> case V4L2_CID_FLASH_CHARGE: return "Charge";
> case V4L2_CID_FLASH_READY: return "Ready to Strobe";
> + case V4L2_CID_FLASH_DURATION: return "Strobe Duration";
>
> /* JPEG encoder controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index f836512e9debbc65d62a9fe04069b056be42f7b2..a5b7c382d77118eb7966385c5b22d5a89bc2b272 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1186,6 +1186,7 @@ enum v4l2_flash_strobe_source {
>
> #define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11)
> #define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12)
> +#define V4L2_CID_FLASH_DURATION (V4L2_CID_FLASH_CLASS_BASE + 13)
>
>
> /* JPEG-class control IDs */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 01/10] media: v4l: ctrls: add a control for flash/strobe duration
2025-09-07 18:55 ` Laurent Pinchart
@ 2025-09-08 14:41 ` Richard Leitner
2025-09-08 15:40 ` Laurent Pinchart
0 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-08 14:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
On Sun, Sep 07, 2025 at 08:55:12PM +0200, Laurent Pinchart wrote:
> Hi Richard,
>
> Thank you for the patch.
>
> On Mon, Sep 01, 2025 at 05:05:06PM +0200, Richard Leitner wrote:
> > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > control, as the timeout defines a limit after which the flash is
> > "forcefully" turned off again.
> >
> > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > of the flash/strobe pulse.
>
> It took me a while to understand the difference between the
> V4L2_CID_FLASH_TIMEOUT and V4L2_CID_FLASH_DURATION controls, as I
> wondered how a device could implement different duration and timeout
> values. Then I realized that the timeout control is meant for flash
> controllers, while the duration control is meant for the source of the
> flash controller's external hardware strobe signal, typically the camera
> sensor. I'd like this to be more explicit, here and in the
> documentation. Here's a proposal for an updated commit message:
Thanks for that proposal! Sorry for not writing clear documentation on
this. I think I was too deep in the topic for too long and couldn't
step back enough to write something that would make sense on a first read
>
> ----
> Add a V4L2_CID_FLASH_DURATION control to set the duration of a
> flash/strobe pulse. This controls the length of the flash/strobe pulse
> output by device (typically a camera sensor) and connected to the flash
> controller. This is different to the V4L2_CID_FLASH_TIMEOUT control,
> which is implemented by the flash controller and defines a limit after
> which the flash is "forcefully" turned off again.
> ----
>
> This could probably be improved, but it's good enough for me for the
> commit message.
Thanks. I will adopt it for the next version of the series.
>
> On a side note, I think we could have reused the V4L2_CID_FLASH_TIMEOUT
> control for this purpose, even if the name isn't the best match, as the
> two usages are implemented on different devices (flash controller vs.
> camera sensor). We have no shortage of control ID space, so a separate
> control ID is fine too, and probably clearer (as long as we document it
> clearly).
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > include/uapi/linux/v4l2-controls.h | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > @@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > case V4L2_CID_FLASH_FAULT: return "Faults";
> > case V4L2_CID_FLASH_CHARGE: return "Charge";
> > case V4L2_CID_FLASH_READY: return "Ready to Strobe";
> > + case V4L2_CID_FLASH_DURATION: return "Strobe Duration";
> >
> > /* JPEG encoder controls */
> > /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index f836512e9debbc65d62a9fe04069b056be42f7b2..a5b7c382d77118eb7966385c5b22d5a89bc2b272 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -1186,6 +1186,7 @@ enum v4l2_flash_strobe_source {
> >
> > #define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11)
> > #define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12)
> > +#define V4L2_CID_FLASH_DURATION (V4L2_CID_FLASH_CLASS_BASE + 13)
> >
> >
> > /* JPEG-class control IDs */
>
> --
> Regards,
>
> Laurent Pinchart
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 01/10] media: v4l: ctrls: add a control for flash/strobe duration
2025-09-08 14:41 ` Richard Leitner
@ 2025-09-08 15:40 ` Laurent Pinchart
0 siblings, 0 replies; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-08 15:40 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
On Mon, Sep 08, 2025 at 04:41:30PM +0200, Richard Leitner wrote:
> On Sun, Sep 07, 2025 at 08:55:12PM +0200, Laurent Pinchart wrote:
> > On Mon, Sep 01, 2025 at 05:05:06PM +0200, Richard Leitner wrote:
> > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > control, as the timeout defines a limit after which the flash is
> > > "forcefully" turned off again.
> > >
> > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > of the flash/strobe pulse.
> >
> > It took me a while to understand the difference between the
> > V4L2_CID_FLASH_TIMEOUT and V4L2_CID_FLASH_DURATION controls, as I
> > wondered how a device could implement different duration and timeout
> > values. Then I realized that the timeout control is meant for flash
> > controllers, while the duration control is meant for the source of the
> > flash controller's external hardware strobe signal, typically the camera
> > sensor. I'd like this to be more explicit, here and in the
> > documentation. Here's a proposal for an updated commit message:
>
> Thanks for that proposal! Sorry for not writing clear documentation on
> this. I think I was too deep in the topic for too long and couldn't
> step back enough to write something that would make sense on a first read
No need to apologize, it happens all the time, everywhere, and to
everybody (myself included). Writing documentation is hard. That makes
reviews from people not familiar with the topic important.
> > ----
> > Add a V4L2_CID_FLASH_DURATION control to set the duration of a
> > flash/strobe pulse. This controls the length of the flash/strobe pulse
> > output by device (typically a camera sensor) and connected to the flash
> > controller. This is different to the V4L2_CID_FLASH_TIMEOUT control,
> > which is implemented by the flash controller and defines a limit after
> > which the flash is "forcefully" turned off again.
> > ----
> >
> > This could probably be improved, but it's good enough for me for the
> > commit message.
>
> Thanks. I will adopt it for the next version of the series.
>
> > On a side note, I think we could have reused the V4L2_CID_FLASH_TIMEOUT
> > control for this purpose, even if the name isn't the best match, as the
> > two usages are implemented on different devices (flash controller vs.
> > camera sensor). We have no shortage of control ID space, so a separate
> > control ID is fine too, and probably clearer (as long as we document it
> > clearly).
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > ---
> > > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > > include/uapi/linux/v4l2-controls.h | 1 +
> > > 2 files changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > @@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > > case V4L2_CID_FLASH_FAULT: return "Faults";
> > > case V4L2_CID_FLASH_CHARGE: return "Charge";
> > > case V4L2_CID_FLASH_READY: return "Ready to Strobe";
> > > + case V4L2_CID_FLASH_DURATION: return "Strobe Duration";
> > >
> > > /* JPEG encoder controls */
> > > /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index f836512e9debbc65d62a9fe04069b056be42f7b2..a5b7c382d77118eb7966385c5b22d5a89bc2b272 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -1186,6 +1186,7 @@ enum v4l2_flash_strobe_source {
> > >
> > > #define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11)
> > > #define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12)
> > > +#define V4L2_CID_FLASH_DURATION (V4L2_CID_FLASH_CLASS_BASE + 13)
> > >
> > >
> > > /* JPEG-class control IDs */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
2025-09-01 15:05 ` [PATCH v7 01/10] media: v4l: ctrls: add a control for flash/strobe duration Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-07 19:05 ` Laurent Pinchart
2025-09-01 15:05 ` [PATCH v7 03/10] media: v4l: ctrls: add a control for enabling hw strobe signal Richard Leitner
` (7 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration
2025-09-01 15:05 ` [PATCH v7 02/10] media: v4l2-flash: add support " Richard Leitner
@ 2025-09-07 19:05 ` Laurent Pinchart
2025-09-08 14:15 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-07 19:05 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration
2025-09-07 19:05 ` Laurent Pinchart
@ 2025-09-08 14:15 ` Richard Leitner
2025-09-08 15:39 ` Laurent Pinchart
0 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-08 14:15 UTC (permalink / raw)
To: Laurent Pinchart, Lee Jones
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Pavel Machek,
linux-media, linux-kernel, linux-leds, Hans Verkuil
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration
2025-09-08 14:15 ` Richard Leitner
@ 2025-09-08 15:39 ` Laurent Pinchart
0 siblings, 0 replies; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-08 15:39 UTC (permalink / raw)
To: Richard Leitner
Cc: Lee Jones, Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 03/10] media: v4l: ctrls: add a control for enabling hw strobe signal
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
2025-09-01 15:05 ` [PATCH v7 01/10] media: v4l: ctrls: add a control for flash/strobe duration Richard Leitner
2025-09-01 15:05 ` [PATCH v7 02/10] media: v4l2-flash: add support " Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-01 15:05 ` [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL} Richard Leitner
` (6 subsequent siblings)
9 siblings, 0 replies; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
Add a control V4L2_CID_FLASH_HW_STROBE_SIGNAL to en- or disable the
strobe output of v4l2 devices (most likely sensors).
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 2 ++
include/uapi/linux/v4l2-controls.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af..e13214ca362b9bdd2302118963008b04fcad8d4c 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1136,6 +1136,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_FLASH_CHARGE: return "Charge";
case V4L2_CID_FLASH_READY: return "Ready to Strobe";
case V4L2_CID_FLASH_DURATION: return "Strobe Duration";
+ case V4L2_CID_FLASH_HW_STROBE_SIGNAL: return "Hardware Strobe Signal";
/* JPEG encoder controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1282,6 +1283,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
case V4L2_CID_FLASH_STROBE_STATUS:
case V4L2_CID_FLASH_CHARGE:
case V4L2_CID_FLASH_READY:
+ case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:
case V4L2_CID_MPEG_VIDEO_DECODER_SLICE_INTERFACE:
case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index a5b7c382d77118eb7966385c5b22d5a89bc2b272..2a09815939a6d9abbe2babb3429e2227970275e1 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1187,6 +1187,7 @@ enum v4l2_flash_strobe_source {
#define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11)
#define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12)
#define V4L2_CID_FLASH_DURATION (V4L2_CID_FLASH_CLASS_BASE + 13)
+#define V4L2_CID_FLASH_HW_STROBE_SIGNAL (V4L2_CID_FLASH_CLASS_BASE + 14)
/* JPEG-class control IDs */
--
2.47.2
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
` (2 preceding siblings ...)
2025-09-01 15:05 ` [PATCH v7 03/10] media: v4l: ctrls: add a control for enabling hw strobe signal Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-07 19:49 ` Laurent Pinchart
2025-09-01 15:05 ` [PATCH v7 05/10] media: i2c: ov9282: add output enable register definitions Richard Leitner
` (5 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
Add the new strobe duration and hardware strobe signal control to v4l
uAPI documentation. Additionally add labels for cross-referencing v4l
controls.
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
.../userspace-api/media/v4l/ext-ctrls-flash.rst | 29 ++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
index d22c5efb806a183a3ad67ec3e6550b002a51659a..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
@@ -57,6 +57,8 @@ Flash Control IDs
``V4L2_CID_FLASH_CLASS (class)``
The FLASH class descriptor.
+.. _v4l2-cid-flash-led-mode:
+
``V4L2_CID_FLASH_LED_MODE (menu)``
Defines the mode of the flash LED, the high-power white LED attached
to the flash controller. Setting this control may not be possible in
@@ -80,6 +82,8 @@ Flash Control IDs
+.. _v4l2-cid-flash-strobe-source:
+
``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
Defines the source of the flash LED strobe.
@@ -186,3 +190,28 @@ Flash Control IDs
charged before strobing. LED flashes often require a cooldown period
after strobe during which another strobe will not be possible. This
is a read-only control.
+
+.. _v4l2-cid-flash-duration:
+
+``V4L2_CID_FLASH_DURATION (integer)``
+ Duration of the flash strobe pulse generated by the strobe source,
+ typically a camera sensor. This method of controlling flash LED strobe
+ duration has three prerequisites: the strobe source's
+ :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
+ enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
+ must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
+ :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
+ ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
+ if possible.
+
+.. _v4l2-cid-flash-hw-strobe-signal:
+
+``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
+ Enables the output of a hardware strobe signal from the strobe source,
+ typically a camera sensor. To control a flash LED driver connected to this
+ hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
+ must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
+ :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
+ ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
+ supports it, the length of the strobe signal can be configured by
+ adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
--
2.47.2
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
2025-09-01 15:05 ` [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL} Richard Leitner
@ 2025-09-07 19:49 ` Laurent Pinchart
2025-09-08 12:37 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-07 19:49 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Richard,
Thank you for the patch.
On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> Add the new strobe duration and hardware strobe signal control to v4l
> uAPI documentation. Additionally add labels for cross-referencing v4l
> controls.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
> .../userspace-api/media/v4l/ext-ctrls-flash.rst | 29 ++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> index d22c5efb806a183a3ad67ec3e6550b002a51659a..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> @@ -57,6 +57,8 @@ Flash Control IDs
> ``V4L2_CID_FLASH_CLASS (class)``
> The FLASH class descriptor.
>
> +.. _v4l2-cid-flash-led-mode:
> +
> ``V4L2_CID_FLASH_LED_MODE (menu)``
> Defines the mode of the flash LED, the high-power white LED attached
> to the flash controller. Setting this control may not be possible in
> @@ -80,6 +82,8 @@ Flash Control IDs
>
>
>
> +.. _v4l2-cid-flash-strobe-source:
> +
> ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> Defines the source of the flash LED strobe.
>
> @@ -186,3 +190,28 @@ Flash Control IDs
> charged before strobing. LED flashes often require a cooldown period
> after strobe during which another strobe will not be possible. This
> is a read-only control.
> +
> +.. _v4l2-cid-flash-duration:
> +
> +``V4L2_CID_FLASH_DURATION (integer)``
> + Duration of the flash strobe pulse generated by the strobe source,
> + typically a camera sensor. This method of controlling flash LED strobe
> + duration has three prerequisites: the strobe source's
> + :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> + enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> + must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> + if possible.
As mentioned in the review of 01/10, I think this needs to be clarified.
Ideally we should add a new document in
Documentation/userspace-api/media/v4l/ to explain the flash API, but in
the meantime let's at lets improve the description of the duration
control. Here's a proposal.
``V4L2_CID_FLASH_DURATION (integer)``
Duration of the flash strobe pulse generated by the strobe source, when
using external strobe. This control shall be implemented by the device
generating the hardware flash strobe signal, typically a camera sensor,
connected to a flash controller. It must not be implemented by the flash
controller.
This method of controlling flash LED strobe duration has three
prerequisites: the strobe source's :ref:`hardware strobe signal
<v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
:ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
<v4l2-cid-flash-strobe-source>` must be configured to
``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
The unit should be microseconds (µs) if possible.
The second paragraph may be better replaced by expanding the
documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
place to document how external strobe works.
As for the unit, is microseconds really the best option ? I would expect
most sensors to express the strobe pulse width in unit of lines.
I think we also need to decide how to handle camera sensors whose flash
strobe pulse width can't be controlled. For instance, the AR0144 can
output a flash signal, and its width is always equal to the exposure
time. The most straightforward solution seems to implement
V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
sensor driver. Could this cause issues in any use case ? Is there a
better solution ? I would like this to be documented.
Finally, I think we also need to standardize the flash strobe offset.
> +
> +.. _v4l2-cid-flash-hw-strobe-signal:
> +
> +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
Nitpicking a bit on the name, I would have called this
V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> + Enables the output of a hardware strobe signal from the strobe source,
> + typically a camera sensor. To control a flash LED driver connected to this
> + hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> + must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> + supports it, the length of the strobe signal can be configured by
> + adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
in a similar way as V4L2_CID_FLASH_DURATION.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
2025-09-07 19:49 ` Laurent Pinchart
@ 2025-09-08 12:37 ` Richard Leitner
2025-09-08 15:59 ` Laurent Pinchart
0 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-08 12:37 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Laurent,
thanks for reviewing this!
On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> Hi Richard,
>
> Thank you for the patch.
>
> On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > Add the new strobe duration and hardware strobe signal control to v4l
> > uAPI documentation. Additionally add labels for cross-referencing v4l
> > controls.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> > .../userspace-api/media/v4l/ext-ctrls-flash.rst | 29 ++++++++++++++++++++++
> > 1 file changed, 29 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > index d22c5efb806a183a3ad67ec3e6550b002a51659a..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > @@ -57,6 +57,8 @@ Flash Control IDs
> > ``V4L2_CID_FLASH_CLASS (class)``
> > The FLASH class descriptor.
> >
> > +.. _v4l2-cid-flash-led-mode:
> > +
> > ``V4L2_CID_FLASH_LED_MODE (menu)``
> > Defines the mode of the flash LED, the high-power white LED attached
> > to the flash controller. Setting this control may not be possible in
> > @@ -80,6 +82,8 @@ Flash Control IDs
> >
> >
> >
> > +.. _v4l2-cid-flash-strobe-source:
> > +
> > ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > Defines the source of the flash LED strobe.
> >
> > @@ -186,3 +190,28 @@ Flash Control IDs
> > charged before strobing. LED flashes often require a cooldown period
> > after strobe during which another strobe will not be possible. This
> > is a read-only control.
> > +
> > +.. _v4l2-cid-flash-duration:
> > +
> > +``V4L2_CID_FLASH_DURATION (integer)``
> > + Duration of the flash strobe pulse generated by the strobe source,
> > + typically a camera sensor. This method of controlling flash LED strobe
> > + duration has three prerequisites: the strobe source's
> > + :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > + enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > + must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > + if possible.
>
> As mentioned in the review of 01/10, I think this needs to be clarified.
> Ideally we should add a new document in
> Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> the meantime let's at lets improve the description of the duration
> control. Here's a proposal.
Understood. Thank you for your proposal!
>
> ``V4L2_CID_FLASH_DURATION (integer)``
> Duration of the flash strobe pulse generated by the strobe source, when
> using external strobe. This control shall be implemented by the device
> generating the hardware flash strobe signal, typically a camera sensor,
> connected to a flash controller. It must not be implemented by the flash
> controller.
>
> This method of controlling flash LED strobe duration has three
> prerequisites: the strobe source's :ref:`hardware strobe signal
> <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> <v4l2-cid-flash-strobe-source>` must be configured to
> ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
>
> The unit should be microseconds (µs) if possible.
>
>
> The second paragraph may be better replaced by expanding the
> documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> place to document how external strobe works.
That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
v9.
>
> As for the unit, is microseconds really the best option ? I would expect
> most sensors to express the strobe pulse width in unit of lines.
We had that discussion already somewhere during this series. Tbh for me
microseconds seems fine. Most (professional) flashes are configured with
s^-1, so that would also be an option, but as flash_timeout is
configured in microseconds, i chose it for flash_duration too.
Nonetheless technically it shouldn't be a problem to express it as
number of lines... Is there a reason to prefer this?
>
> I think we also need to decide how to handle camera sensors whose flash
> strobe pulse width can't be controlled. For instance, the AR0144 can
> output a flash signal, and its width is always equal to the exposure
> time. The most straightforward solution seems to implement
> V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> sensor driver. Could this cause issues in any use case ? Is there a
> better solution ? I would like this to be documented.
Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
provided as a read-only property too. So userspace is explicitely aware
of the acutal value and doesn't have to make assumptions.
Should I add documentation on this topic to this patch?
>
> Finally, I think we also need to standardize the flash strobe offset.
I guess I somewhere mentioned this already: I have some patches for
configuring the strobe offset of ov9282 and adding the corresponding
v4l2 control. But to keep this series simple I'm planning to send them
as soon as this one is "done".
IMHO the offset should then have the same unit as the flash_duration.
>
> > +
> > +.. _v4l2-cid-flash-hw-strobe-signal:
> > +
> > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
>
> Nitpicking a bit on the name, I would have called this
> V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
I'm always open to name-nitpicking ;-)
V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
>
> > + Enables the output of a hardware strobe signal from the strobe source,
> > + typically a camera sensor. To control a flash LED driver connected to this
> > + hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > + must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > + supports it, the length of the strobe signal can be configured by
> > + adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
>
> The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> in a similar way as V4L2_CID_FLASH_DURATION.
Sure. I will adapt this for v9.
>
> --
> Regards,
>
> Laurent Pinchart
thanks again for your great review!
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
2025-09-08 12:37 ` Richard Leitner
@ 2025-09-08 15:59 ` Laurent Pinchart
2025-09-09 10:29 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-08 15:59 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > Add the new strobe duration and hardware strobe signal control to v4l
> > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > controls.
> > >
> > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > ---
> > > .../userspace-api/media/v4l/ext-ctrls-flash.rst | 29 ++++++++++++++++++++++
> > > 1 file changed, 29 insertions(+)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > index d22c5efb806a183a3ad67ec3e6550b002a51659a..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > @@ -57,6 +57,8 @@ Flash Control IDs
> > > ``V4L2_CID_FLASH_CLASS (class)``
> > > The FLASH class descriptor.
> > >
> > > +.. _v4l2-cid-flash-led-mode:
> > > +
> > > ``V4L2_CID_FLASH_LED_MODE (menu)``
> > > Defines the mode of the flash LED, the high-power white LED attached
> > > to the flash controller. Setting this control may not be possible in
> > > @@ -80,6 +82,8 @@ Flash Control IDs
> > >
> > >
> > >
> > > +.. _v4l2-cid-flash-strobe-source:
> > > +
> > > ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > > Defines the source of the flash LED strobe.
> > >
> > > @@ -186,3 +190,28 @@ Flash Control IDs
> > > charged before strobing. LED flashes often require a cooldown period
> > > after strobe during which another strobe will not be possible. This
> > > is a read-only control.
> > > +
> > > +.. _v4l2-cid-flash-duration:
> > > +
> > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > + Duration of the flash strobe pulse generated by the strobe source,
> > > + typically a camera sensor. This method of controlling flash LED strobe
> > > + duration has three prerequisites: the strobe source's
> > > + :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > + enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > + if possible.
> >
> > As mentioned in the review of 01/10, I think this needs to be clarified.
> > Ideally we should add a new document in
> > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > the meantime let's at lets improve the description of the duration
> > control. Here's a proposal.
>
> Understood. Thank you for your proposal!
>
> > ``V4L2_CID_FLASH_DURATION (integer)``
> > Duration of the flash strobe pulse generated by the strobe source, when
> > using external strobe. This control shall be implemented by the device
> > generating the hardware flash strobe signal, typically a camera sensor,
> > connected to a flash controller. It must not be implemented by the flash
> > controller.
> >
> > This method of controlling flash LED strobe duration has three
> > prerequisites: the strobe source's :ref:`hardware strobe signal
> > <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> > :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> > ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> > <v4l2-cid-flash-strobe-source>` must be configured to
> > ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> >
> > The unit should be microseconds (µs) if possible.
> >
> >
> > The second paragraph may be better replaced by expanding the
> > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > place to document how external strobe works.
>
> That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> v9.
Sakari, could you please check if you agree with the above ? Let's avoid
going back and forth with reviews (and I'll try my best to review the
next version quickly).
> > As for the unit, is microseconds really the best option ? I would expect
> > most sensors to express the strobe pulse width in unit of lines.
>
> We had that discussion already somewhere during this series. Tbh for me
> microseconds seems fine. Most (professional) flashes are configured with
> s^-1, so that would also be an option, but as flash_timeout is
> configured in microseconds, i chose it for flash_duration too.
>
> Nonetheless technically it shouldn't be a problem to express it as
> number of lines... Is there a reason to prefer this?
A few observations have confirmed my gut feeling that this is how
sensors typically express the pulse width. Expressing the value in its
hardware unit means we won't have rounding issues, and drivers will also
be simpler. We're missing data though, it would be nice to check a wider
variety of camera sensors.
> > I think we also need to decide how to handle camera sensors whose flash
> > strobe pulse width can't be controlled. For instance, the AR0144 can
> > output a flash signal, and its width is always equal to the exposure
> > time. The most straightforward solution seems to implement
> > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > sensor driver. Could this cause issues in any use case ? Is there a
> > better solution ? I would like this to be documented.
>
> Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> provided as a read-only property too. So userspace is explicitely aware
> of the acutal value and doesn't have to make assumptions.
The value would change depending on the exposure time. Given how control
change events are implemented that would be difficult to use from
userspace at best. I think not exposing the control would be as useful
as exposing a read-only value, and it would be simpler to implement in
kernel drivers.
> Should I add documentation on this topic to this patch?
That would be nice, thank you.
> > Finally, I think we also need to standardize the flash strobe offset.
>
> I guess I somewhere mentioned this already: I have some patches for
> configuring the strobe offset of ov9282 and adding the corresponding
> v4l2 control. But to keep this series simple I'm planning to send them
> as soon as this one is "done".
>
> IMHO the offset should then have the same unit as the flash_duration.
What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
expressed in units of half a line.
> > > +
> > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > +
> > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> >
> > Nitpicking a bit on the name, I would have called this
> > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
>
> I'm always open to name-nitpicking ;-)
>
> V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
Sakari, what's your opinion ?
> > > + Enables the output of a hardware strobe signal from the strobe source,
> > > + typically a camera sensor. To control a flash LED driver connected to this
> > > + hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > + supports it, the length of the strobe signal can be configured by
> > > + adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> >
> > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > in a similar way as V4L2_CID_FLASH_DURATION.
>
> Sure. I will adapt this for v9.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
2025-09-08 15:59 ` Laurent Pinchart
@ 2025-09-09 10:29 ` Richard Leitner
2025-09-17 10:14 ` Richard Leitner
2025-09-17 15:43 ` Sakari Ailus
0 siblings, 2 replies; 45+ messages in thread
From: Richard Leitner @ 2025-09-09 10:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Laurent,
thanks for your great (and quick) feedback!
On Mon, Sep 08, 2025 at 05:59:17PM +0200, Laurent Pinchart wrote:
> On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> > On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > > Add the new strobe duration and hardware strobe signal control to v4l
> > > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > > controls.
> > > >
> > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > ---
> > > > .../userspace-api/media/v4l/ext-ctrls-flash.rst | 29 ++++++++++++++++++++++
> > > > 1 file changed, 29 insertions(+)
> > > >
> > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > index d22c5efb806a183a3ad67ec3e6550b002a51659a..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > @@ -57,6 +57,8 @@ Flash Control IDs
> > > > ``V4L2_CID_FLASH_CLASS (class)``
> > > > The FLASH class descriptor.
> > > >
> > > > +.. _v4l2-cid-flash-led-mode:
> > > > +
> > > > ``V4L2_CID_FLASH_LED_MODE (menu)``
> > > > Defines the mode of the flash LED, the high-power white LED attached
> > > > to the flash controller. Setting this control may not be possible in
> > > > @@ -80,6 +82,8 @@ Flash Control IDs
> > > >
> > > >
> > > >
> > > > +.. _v4l2-cid-flash-strobe-source:
> > > > +
> > > > ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > > > Defines the source of the flash LED strobe.
> > > >
> > > > @@ -186,3 +190,28 @@ Flash Control IDs
> > > > charged before strobing. LED flashes often require a cooldown period
> > > > after strobe during which another strobe will not be possible. This
> > > > is a read-only control.
> > > > +
> > > > +.. _v4l2-cid-flash-duration:
> > > > +
> > > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > > + Duration of the flash strobe pulse generated by the strobe source,
> > > > + typically a camera sensor. This method of controlling flash LED strobe
> > > > + duration has three prerequisites: the strobe source's
> > > > + :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > > + enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > > + if possible.
> > >
> > > As mentioned in the review of 01/10, I think this needs to be clarified.
> > > Ideally we should add a new document in
> > > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > > the meantime let's at lets improve the description of the duration
> > > control. Here's a proposal.
> >
> > Understood. Thank you for your proposal!
> >
> > > ``V4L2_CID_FLASH_DURATION (integer)``
> > > Duration of the flash strobe pulse generated by the strobe source, when
> > > using external strobe. This control shall be implemented by the device
> > > generating the hardware flash strobe signal, typically a camera sensor,
> > > connected to a flash controller. It must not be implemented by the flash
> > > controller.
> > >
> > > This method of controlling flash LED strobe duration has three
> > > prerequisites: the strobe source's :ref:`hardware strobe signal
> > > <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> > > :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> > > ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> > > <v4l2-cid-flash-strobe-source>` must be configured to
> > > ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> > >
> > > The unit should be microseconds (µs) if possible.
> > >
> > >
> > > The second paragraph may be better replaced by expanding the
> > > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > > place to document how external strobe works.
> >
> > That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> > V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> > v9.
>
> Sakari, could you please check if you agree with the above ? Let's avoid
> going back and forth with reviews (and I'll try my best to review the
> next version quickly).
My current proposal:
* - ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``
- The flash strobe is triggered by an external source. Typically
this is a sensor, which makes it possible to synchronise the
flash strobe start to exposure start.
This method of controlling flash LED strobe has two additional
prerequisites: the strobe source's :ref:`flash strobe output
<v4l2-cid-flash-strobe-oe>` must be enabled (if available)
and the flash controller's :ref:`flash LED mode
<v4l2-cid-flash-led-mode>` must be set to
``V4L2_FLASH_LED_MODE_FLASH``. Additionally the :ref:`flash duration
<v4l2-cid-flash-duration>` may be adjusted by the strobe source.
``V4L2_CID_FLASH_DURATION (integer)``
Duration of the flash strobe pulse generated by the strobe source, when
using external strobe. This control shall be implemented by the device
generating the hardware flash strobe signal, typically a camera sensor,
connected to a flash controller. It must not be implemented by the flash
controller. Typically the flash strobe pulse needs to be activated by
enabling the strobe source's :ref:`flash strobe output
<v4l2-cid-flash-strobe-oe>`.
The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
mode of operation.
The unit should be number of lines if possible.
``V4L2_CID_FLASH_STROBE_OE (boolean)``
Enables the output of a hardware strobe signal from the strobe source,
when using external strobe. This control shall be implemented by the device
generating the hardware flash strobe signal, typically a camera sensor,
connected to a flash controller.
Provided the signal generating device driver supports it, the length of the
strobe signal can be configured by adjusting its
:ref:`flash duration <v4l2-cid-flash-duration>`. In case the device has a
fixed strobe length, the flash duration control must not be implemented.
The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
mode of operation.
>
> > > As for the unit, is microseconds really the best option ? I would expect
> > > most sensors to express the strobe pulse width in unit of lines.
> >
> > We had that discussion already somewhere during this series. Tbh for me
> > microseconds seems fine. Most (professional) flashes are configured with
> > s^-1, so that would also be an option, but as flash_timeout is
> > configured in microseconds, i chose it for flash_duration too.
> >
> > Nonetheless technically it shouldn't be a problem to express it as
> > number of lines... Is there a reason to prefer this?
>
> A few observations have confirmed my gut feeling that this is how
> sensors typically express the pulse width. Expressing the value in its
> hardware unit means we won't have rounding issues, and drivers will also
> be simpler. We're missing data though, it would be nice to check a wider
> variety of camera sensors.
I have done some more measurements and calculation on this for ov9281.
It seems you are (somehow?) right. The strobe_frame_span (aka strobe
duration) register value seems to represent the duration of the strobe in
number of lines plus a constant and variable offset based on the hblank
value. Other settings (e.g. vblank, exposure, ...) have no influence on
the duration.
After about 50 measurements using different strobe_frame_span and hblank
values and 1280x800 as resolution I came up with the following formulas:
line_factor = active_width + hblank * 1,04 + 56
t_strobe = strobe_frame_span * line_factor / pixel_rate
Which matches all tested cased nicely...
Nonetheless I'm still unsure on what unit to use for flash duration...
The exposure time for ov9282 is set as "number of row periods, where the
low 4 bits are fraction bits" in the registers. The v4l2 control should
on the other hand accept 100 µs units as value.
From a user perspective it would make sense to me to configure exposure
time, flash duration and flash/strobe offset using the same base units.
On the other hand we may have rounding issues and formulas based on
assumptions or reverse-engineering when implementing this for a
sensor...
What's your opinion on this, Sakari, Laurent, Dave?
>
> > > I think we also need to decide how to handle camera sensors whose flash
> > > strobe pulse width can't be controlled. For instance, the AR0144 can
> > > output a flash signal, and its width is always equal to the exposure
> > > time. The most straightforward solution seems to implement
> > > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > > sensor driver. Could this cause issues in any use case ? Is there a
> > > better solution ? I would like this to be documented.
> >
> > Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> > provided as a read-only property too. So userspace is explicitely aware
> > of the acutal value and doesn't have to make assumptions.
>
> The value would change depending on the exposure time. Given how control
> change events are implemented that would be difficult to use from
> userspace at best. I think not exposing the control would be as useful
> as exposing a read-only value, and it would be simpler to implement in
> kernel drivers.
That's true. I guess keeping the drivers simple and moving this "logic"
to a possible client/userspace application (if needed) is fine with me.
As you may have seen above, I've tried to integrate this in the
documentation proposal already.
>
> > Should I add documentation on this topic to this patch?
>
> That would be nice, thank you.
>
> > > Finally, I think we also need to standardize the flash strobe offset.
> >
> > I guess I somewhere mentioned this already: I have some patches for
> > configuring the strobe offset of ov9282 and adding the corresponding
> > v4l2 control. But to keep this series simple I'm planning to send them
> > as soon as this one is "done".
> >
> > IMHO the offset should then have the same unit as the flash_duration.
>
> What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
> expressed in units of half a line.
>
> > > > +
> > > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > > +
> > > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> > >
> > > Nitpicking a bit on the name, I would have called this
> > > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> >
> > I'm always open to name-nitpicking ;-)
> >
> > V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> > shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
>
> Sakari, what's your opinion ?
>
> > > > + Enables the output of a hardware strobe signal from the strobe source,
> > > > + typically a camera sensor. To control a flash LED driver connected to this
> > > > + hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > > + supports it, the length of the strobe signal can be configured by
> > > > + adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> > >
> > > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > > in a similar way as V4L2_CID_FLASH_DURATION.
> >
> > Sure. I will adapt this for v9.
>
> --
> Regards,
>
> Laurent Pinchart
thanks & regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
2025-09-09 10:29 ` Richard Leitner
@ 2025-09-17 10:14 ` Richard Leitner
2025-09-17 15:43 ` Sakari Ailus
1 sibling, 0 replies; 45+ messages in thread
From: Richard Leitner @ 2025-09-17 10:14 UTC (permalink / raw)
To: Sakari Ailus, Laurent Pinchart
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Laurent and Sakari,
just a friendly ping for the pending feedback on this series :-)
regards;rl
On Tue, Sep 09, 2025 at 12:29:59PM +0200, Richard Leitner wrote:
> Hi Laurent,
>
> thanks for your great (and quick) feedback!
>
> On Mon, Sep 08, 2025 at 05:59:17PM +0200, Laurent Pinchart wrote:
> > On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> > > On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > > > Add the new strobe duration and hardware strobe signal control to v4l
> > > > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > > > controls.
> > > > >
> > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > ---
> > > > > .../userspace-api/media/v4l/ext-ctrls-flash.rst | 29 ++++++++++++++++++++++
> > > > > 1 file changed, 29 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > index d22c5efb806a183a3ad67ec3e6550b002a51659a..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > @@ -57,6 +57,8 @@ Flash Control IDs
> > > > > ``V4L2_CID_FLASH_CLASS (class)``
> > > > > The FLASH class descriptor.
> > > > >
> > > > > +.. _v4l2-cid-flash-led-mode:
> > > > > +
> > > > > ``V4L2_CID_FLASH_LED_MODE (menu)``
> > > > > Defines the mode of the flash LED, the high-power white LED attached
> > > > > to the flash controller. Setting this control may not be possible in
> > > > > @@ -80,6 +82,8 @@ Flash Control IDs
> > > > >
> > > > >
> > > > >
> > > > > +.. _v4l2-cid-flash-strobe-source:
> > > > > +
> > > > > ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > > > > Defines the source of the flash LED strobe.
> > > > >
> > > > > @@ -186,3 +190,28 @@ Flash Control IDs
> > > > > charged before strobing. LED flashes often require a cooldown period
> > > > > after strobe during which another strobe will not be possible. This
> > > > > is a read-only control.
> > > > > +
> > > > > +.. _v4l2-cid-flash-duration:
> > > > > +
> > > > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > > > + Duration of the flash strobe pulse generated by the strobe source,
> > > > > + typically a camera sensor. This method of controlling flash LED strobe
> > > > > + duration has three prerequisites: the strobe source's
> > > > > + :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > > > + enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > > > + if possible.
> > > >
> > > > As mentioned in the review of 01/10, I think this needs to be clarified.
> > > > Ideally we should add a new document in
> > > > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > > > the meantime let's at lets improve the description of the duration
> > > > control. Here's a proposal.
> > >
> > > Understood. Thank you for your proposal!
> > >
> > > > ``V4L2_CID_FLASH_DURATION (integer)``
> > > > Duration of the flash strobe pulse generated by the strobe source, when
> > > > using external strobe. This control shall be implemented by the device
> > > > generating the hardware flash strobe signal, typically a camera sensor,
> > > > connected to a flash controller. It must not be implemented by the flash
> > > > controller.
> > > >
> > > > This method of controlling flash LED strobe duration has three
> > > > prerequisites: the strobe source's :ref:`hardware strobe signal
> > > > <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> > > > :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> > > > ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> > > > <v4l2-cid-flash-strobe-source>` must be configured to
> > > > ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> > > >
> > > > The unit should be microseconds (µs) if possible.
> > > >
> > > >
> > > > The second paragraph may be better replaced by expanding the
> > > > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > > > place to document how external strobe works.
> > >
> > > That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> > > V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> > > v9.
> >
> > Sakari, could you please check if you agree with the above ? Let's avoid
> > going back and forth with reviews (and I'll try my best to review the
> > next version quickly).
>
> My current proposal:
>
> * - ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``
> - The flash strobe is triggered by an external source. Typically
> this is a sensor, which makes it possible to synchronise the
> flash strobe start to exposure start.
> This method of controlling flash LED strobe has two additional
> prerequisites: the strobe source's :ref:`flash strobe output
> <v4l2-cid-flash-strobe-oe>` must be enabled (if available)
> and the flash controller's :ref:`flash LED mode
> <v4l2-cid-flash-led-mode>` must be set to
> ``V4L2_FLASH_LED_MODE_FLASH``. Additionally the :ref:`flash duration
> <v4l2-cid-flash-duration>` may be adjusted by the strobe source.
>
>
> ``V4L2_CID_FLASH_DURATION (integer)``
> Duration of the flash strobe pulse generated by the strobe source, when
> using external strobe. This control shall be implemented by the device
> generating the hardware flash strobe signal, typically a camera sensor,
> connected to a flash controller. It must not be implemented by the flash
> controller. Typically the flash strobe pulse needs to be activated by
> enabling the strobe source's :ref:`flash strobe output
> <v4l2-cid-flash-strobe-oe>`.
>
> The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
> must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
> mode of operation.
>
> The unit should be number of lines if possible.
>
>
> ``V4L2_CID_FLASH_STROBE_OE (boolean)``
> Enables the output of a hardware strobe signal from the strobe source,
> when using external strobe. This control shall be implemented by the device
> generating the hardware flash strobe signal, typically a camera sensor,
> connected to a flash controller.
>
> Provided the signal generating device driver supports it, the length of the
> strobe signal can be configured by adjusting its
> :ref:`flash duration <v4l2-cid-flash-duration>`. In case the device has a
> fixed strobe length, the flash duration control must not be implemented.
>
> The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
> must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
> mode of operation.
>
> >
> > > > As for the unit, is microseconds really the best option ? I would expect
> > > > most sensors to express the strobe pulse width in unit of lines.
> > >
> > > We had that discussion already somewhere during this series. Tbh for me
> > > microseconds seems fine. Most (professional) flashes are configured with
> > > s^-1, so that would also be an option, but as flash_timeout is
> > > configured in microseconds, i chose it for flash_duration too.
> > >
> > > Nonetheless technically it shouldn't be a problem to express it as
> > > number of lines... Is there a reason to prefer this?
> >
> > A few observations have confirmed my gut feeling that this is how
> > sensors typically express the pulse width. Expressing the value in its
> > hardware unit means we won't have rounding issues, and drivers will also
> > be simpler. We're missing data though, it would be nice to check a wider
> > variety of camera sensors.
>
> I have done some more measurements and calculation on this for ov9281.
> It seems you are (somehow?) right. The strobe_frame_span (aka strobe
> duration) register value seems to represent the duration of the strobe in
> number of lines plus a constant and variable offset based on the hblank
> value. Other settings (e.g. vblank, exposure, ...) have no influence on
> the duration.
>
> After about 50 measurements using different strobe_frame_span and hblank
> values and 1280x800 as resolution I came up with the following formulas:
>
> line_factor = active_width + hblank * 1,04 + 56
>
> t_strobe = strobe_frame_span * line_factor / pixel_rate
>
> Which matches all tested cased nicely...
>
> Nonetheless I'm still unsure on what unit to use for flash duration...
>
> The exposure time for ov9282 is set as "number of row periods, where the
> low 4 bits are fraction bits" in the registers. The v4l2 control should
> on the other hand accept 100 µs units as value.
>
> From a user perspective it would make sense to me to configure exposure
> time, flash duration and flash/strobe offset using the same base units.
> On the other hand we may have rounding issues and formulas based on
> assumptions or reverse-engineering when implementing this for a
> sensor...
>
> What's your opinion on this, Sakari, Laurent, Dave?
>
> >
> > > > I think we also need to decide how to handle camera sensors whose flash
> > > > strobe pulse width can't be controlled. For instance, the AR0144 can
> > > > output a flash signal, and its width is always equal to the exposure
> > > > time. The most straightforward solution seems to implement
> > > > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > > > sensor driver. Could this cause issues in any use case ? Is there a
> > > > better solution ? I would like this to be documented.
> > >
> > > Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> > > provided as a read-only property too. So userspace is explicitely aware
> > > of the acutal value and doesn't have to make assumptions.
> >
> > The value would change depending on the exposure time. Given how control
> > change events are implemented that would be difficult to use from
> > userspace at best. I think not exposing the control would be as useful
> > as exposing a read-only value, and it would be simpler to implement in
> > kernel drivers.
>
> That's true. I guess keeping the drivers simple and moving this "logic"
> to a possible client/userspace application (if needed) is fine with me.
>
> As you may have seen above, I've tried to integrate this in the
> documentation proposal already.
>
> >
> > > Should I add documentation on this topic to this patch?
> >
> > That would be nice, thank you.
> >
> > > > Finally, I think we also need to standardize the flash strobe offset.
> > >
> > > I guess I somewhere mentioned this already: I have some patches for
> > > configuring the strobe offset of ov9282 and adding the corresponding
> > > v4l2 control. But to keep this series simple I'm planning to send them
> > > as soon as this one is "done".
> > >
> > > IMHO the offset should then have the same unit as the flash_duration.
> >
> > What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
> > expressed in units of half a line.
> >
> > > > > +
> > > > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > > > +
> > > > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> > > >
> > > > Nitpicking a bit on the name, I would have called this
> > > > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> > >
> > > I'm always open to name-nitpicking ;-)
> > >
> > > V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> > > shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
> >
> > Sakari, what's your opinion ?
> >
> > > > > + Enables the output of a hardware strobe signal from the strobe source,
> > > > > + typically a camera sensor. To control a flash LED driver connected to this
> > > > > + hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > > > + supports it, the length of the strobe signal can be configured by
> > > > > + adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> > > >
> > > > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > > > in a similar way as V4L2_CID_FLASH_DURATION.
> > >
> > > Sure. I will adapt this for v9.
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> thanks & regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
2025-09-09 10:29 ` Richard Leitner
2025-09-17 10:14 ` Richard Leitner
@ 2025-09-17 15:43 ` Sakari Ailus
2025-09-18 8:07 ` Richard Leitner
1 sibling, 1 reply; 45+ messages in thread
From: Sakari Ailus @ 2025-09-17 15:43 UTC (permalink / raw)
To: Richard Leitner
Cc: Laurent Pinchart, Dave Stevenson, Mauro Carvalho Chehab,
Lee Jones, Pavel Machek, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Richard,
On Tue, Sep 09, 2025 at 12:29:55PM +0200, Richard Leitner wrote:
> Hi Laurent,
>
> thanks for your great (and quick) feedback!
>
> On Mon, Sep 08, 2025 at 05:59:17PM +0200, Laurent Pinchart wrote:
> > On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> > > On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > > > Add the new strobe duration and hardware strobe signal control to v4l
> > > > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > > > controls.
> > > > >
> > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > ---
> > > > > .../userspace-api/media/v4l/ext-ctrls-flash.rst | 29 ++++++++++++++++++++++
> > > > > 1 file changed, 29 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > index d22c5efb806a183a3ad67ec3e6550b002a51659a..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > @@ -57,6 +57,8 @@ Flash Control IDs
> > > > > ``V4L2_CID_FLASH_CLASS (class)``
> > > > > The FLASH class descriptor.
> > > > >
> > > > > +.. _v4l2-cid-flash-led-mode:
> > > > > +
> > > > > ``V4L2_CID_FLASH_LED_MODE (menu)``
> > > > > Defines the mode of the flash LED, the high-power white LED attached
> > > > > to the flash controller. Setting this control may not be possible in
> > > > > @@ -80,6 +82,8 @@ Flash Control IDs
> > > > >
> > > > >
> > > > >
> > > > > +.. _v4l2-cid-flash-strobe-source:
> > > > > +
> > > > > ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > > > > Defines the source of the flash LED strobe.
> > > > >
> > > > > @@ -186,3 +190,28 @@ Flash Control IDs
> > > > > charged before strobing. LED flashes often require a cooldown period
> > > > > after strobe during which another strobe will not be possible. This
> > > > > is a read-only control.
> > > > > +
> > > > > +.. _v4l2-cid-flash-duration:
> > > > > +
> > > > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > > > + Duration of the flash strobe pulse generated by the strobe source,
> > > > > + typically a camera sensor. This method of controlling flash LED strobe
> > > > > + duration has three prerequisites: the strobe source's
> > > > > + :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > > > + enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > > > + if possible.
> > > >
> > > > As mentioned in the review of 01/10, I think this needs to be clarified.
> > > > Ideally we should add a new document in
> > > > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > > > the meantime let's at lets improve the description of the duration
> > > > control. Here's a proposal.
> > >
> > > Understood. Thank you for your proposal!
> > >
> > > > ``V4L2_CID_FLASH_DURATION (integer)``
> > > > Duration of the flash strobe pulse generated by the strobe source, when
> > > > using external strobe. This control shall be implemented by the device
> > > > generating the hardware flash strobe signal, typically a camera sensor,
> > > > connected to a flash controller. It must not be implemented by the flash
> > > > controller.
> > > >
> > > > This method of controlling flash LED strobe duration has three
> > > > prerequisites: the strobe source's :ref:`hardware strobe signal
> > > > <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> > > > :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> > > > ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> > > > <v4l2-cid-flash-strobe-source>` must be configured to
> > > > ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> > > >
> > > > The unit should be microseconds (µs) if possible.
> > > >
> > > >
> > > > The second paragraph may be better replaced by expanding the
> > > > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > > > place to document how external strobe works.
> > >
> > > That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> > > V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> > > v9.
> >
> > Sakari, could you please check if you agree with the above ? Let's avoid
> > going back and forth with reviews (and I'll try my best to review the
> > next version quickly).
>
> My current proposal:
>
> * - ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``
> - The flash strobe is triggered by an external source. Typically
> this is a sensor, which makes it possible to synchronise the
> flash strobe start to exposure start.
> This method of controlling flash LED strobe has two additional
> prerequisites: the strobe source's :ref:`flash strobe output
> <v4l2-cid-flash-strobe-oe>` must be enabled (if available)
> and the flash controller's :ref:`flash LED mode
> <v4l2-cid-flash-led-mode>` must be set to
> ``V4L2_FLASH_LED_MODE_FLASH``. Additionally the :ref:`flash duration
> <v4l2-cid-flash-duration>` may be adjusted by the strobe source.
>
>
> ``V4L2_CID_FLASH_DURATION (integer)``
> Duration of the flash strobe pulse generated by the strobe source, when
> using external strobe. This control shall be implemented by the device
> generating the hardware flash strobe signal, typically a camera sensor,
> connected to a flash controller. It must not be implemented by the flash
> controller. Typically the flash strobe pulse needs to be activated by
I'd drop the sentence on flash controller as this is UAPI documentation.
> enabling the strobe source's :ref:`flash strobe output
> <v4l2-cid-flash-strobe-oe>`.
>
> The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
> must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
> mode of operation.
>
> The unit should be number of lines if possible.
>
>
> ``V4L2_CID_FLASH_STROBE_OE (boolean)``
> Enables the output of a hardware strobe signal from the strobe source,
> when using external strobe. This control shall be implemented by the device
I'd remove the comma.
> generating the hardware flash strobe signal, typically a camera sensor,
> connected to a flash controller.
>
> Provided the signal generating device driver supports it, the length of the
> strobe signal can be configured by adjusting its
> :ref:`flash duration <v4l2-cid-flash-duration>`. In case the device has a
> fixed strobe length, the flash duration control must not be implemented.
I don't see why the duration control wouldn't be implemented in this case:
it's still relevant for the user space to know how long the duration is.
I'd simply drop this sentence.
>
> The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
> must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
> mode of operation.
>
> >
> > > > As for the unit, is microseconds really the best option ? I would expect
> > > > most sensors to express the strobe pulse width in unit of lines.
> > >
> > > We had that discussion already somewhere during this series. Tbh for me
> > > microseconds seems fine. Most (professional) flashes are configured with
> > > s^-1, so that would also be an option, but as flash_timeout is
> > > configured in microseconds, i chose it for flash_duration too.
> > >
> > > Nonetheless technically it shouldn't be a problem to express it as
> > > number of lines... Is there a reason to prefer this?
> >
> > A few observations have confirmed my gut feeling that this is how
> > sensors typically express the pulse width. Expressing the value in its
> > hardware unit means we won't have rounding issues, and drivers will also
> > be simpler. We're missing data though, it would be nice to check a wider
> > variety of camera sensors.
>
> I have done some more measurements and calculation on this for ov9281.
> It seems you are (somehow?) right. The strobe_frame_span (aka strobe
> duration) register value seems to represent the duration of the strobe in
> number of lines plus a constant and variable offset based on the hblank
> value. Other settings (e.g. vblank, exposure, ...) have no influence on
> the duration.
>
> After about 50 measurements using different strobe_frame_span and hblank
> values and 1280x800 as resolution I came up with the following formulas:
>
> line_factor = active_width + hblank * 1,04 + 56
>
> t_strobe = strobe_frame_span * line_factor / pixel_rate
>
> Which matches all tested cased nicely...
>
> Nonetheless I'm still unsure on what unit to use for flash duration...
>
> The exposure time for ov9282 is set as "number of row periods, where the
> low 4 bits are fraction bits" in the registers. The v4l2 control should
> on the other hand accept 100 µs units as value.
>
> From a user perspective it would make sense to me to configure exposure
> time, flash duration and flash/strobe offset using the same base units.
> On the other hand we may have rounding issues and formulas based on
> assumptions or reverse-engineering when implementing this for a
> sensor...
>
> What's your opinion on this, Sakari, Laurent, Dave?
I checked what CCS defines exposure time as a function of the external
clock frequency:
exposure time = tFlash_strobe_width_ctrl / ext_clk_freq *
flash_strobe_adjustment
The added accuracy is relevant for xenon (admittedly rare these days, but
depends on the device) flash but probably not so for LEDs.
So I'm fine with keeping this as-is and perhaps adding CCS specific private
controls separately.
>
> >
> > > > I think we also need to decide how to handle camera sensors whose flash
> > > > strobe pulse width can't be controlled. For instance, the AR0144 can
> > > > output a flash signal, and its width is always equal to the exposure
> > > > time. The most straightforward solution seems to implement
> > > > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > > > sensor driver. Could this cause issues in any use case ? Is there a
> > > > better solution ? I would like this to be documented.
> > >
> > > Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> > > provided as a read-only property too. So userspace is explicitely aware
> > > of the acutal value and doesn't have to make assumptions.
> >
> > The value would change depending on the exposure time. Given how control
> > change events are implemented that would be difficult to use from
> > userspace at best. I think not exposing the control would be as useful
> > as exposing a read-only value, and it would be simpler to implement in
> > kernel drivers.
>
> That's true. I guess keeping the drivers simple and moving this "logic"
> to a possible client/userspace application (if needed) is fine with me.
>
> As you may have seen above, I've tried to integrate this in the
> documentation proposal already.
>
> >
> > > Should I add documentation on this topic to this patch?
> >
> > That would be nice, thank you.
> >
> > > > Finally, I think we also need to standardize the flash strobe offset.
> > >
> > > I guess I somewhere mentioned this already: I have some patches for
> > > configuring the strobe offset of ov9282 and adding the corresponding
> > > v4l2 control. But to keep this series simple I'm planning to send them
> > > as soon as this one is "done".
> > >
> > > IMHO the offset should then have the same unit as the flash_duration.
> >
> > What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
> > expressed in units of half a line.
> >
> > > > > +
> > > > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > > > +
> > > > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> > > >
> > > > Nitpicking a bit on the name, I would have called this
> > > > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> > >
> > > I'm always open to name-nitpicking ;-)
> > >
> > > V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> > > shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
> >
> > Sakari, what's your opinion ?
I slightly prefer the former, too.
> >
> > > > > + Enables the output of a hardware strobe signal from the strobe source,
> > > > > + typically a camera sensor. To control a flash LED driver connected to this
> > > > > + hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > > > + supports it, the length of the strobe signal can be configured by
> > > > > + adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> > > >
> > > > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > > > in a similar way as V4L2_CID_FLASH_DURATION.
> > >
> > > Sure. I will adapt this for v9.
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> thanks & regards;rl
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
2025-09-17 15:43 ` Sakari Ailus
@ 2025-09-18 8:07 ` Richard Leitner
0 siblings, 0 replies; 45+ messages in thread
From: Richard Leitner @ 2025-09-18 8:07 UTC (permalink / raw)
To: Laurent Pinchart, Sakari Ailus
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Sakari,
thanks for your feedback!
On Wed, Sep 17, 2025 at 06:43:49PM +0300, Sakari Ailus wrote:
> Hi Richard,
>
> On Tue, Sep 09, 2025 at 12:29:55PM +0200, Richard Leitner wrote:
> > Hi Laurent,
> >
> > thanks for your great (and quick) feedback!
> >
> > On Mon, Sep 08, 2025 at 05:59:17PM +0200, Laurent Pinchart wrote:
> > > On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> > > > On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > > > > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > > > > Add the new strobe duration and hardware strobe signal control to v4l
> > > > > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > > > > controls.
> > > > > >
> > > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > > ---
> > > > > > .../userspace-api/media/v4l/ext-ctrls-flash.rst | 29 ++++++++++++++++++++++
> > > > > > 1 file changed, 29 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > > index d22c5efb806a183a3ad67ec3e6550b002a51659a..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > > @@ -57,6 +57,8 @@ Flash Control IDs
> > > > > > ``V4L2_CID_FLASH_CLASS (class)``
> > > > > > The FLASH class descriptor.
> > > > > >
> > > > > > +.. _v4l2-cid-flash-led-mode:
> > > > > > +
> > > > > > ``V4L2_CID_FLASH_LED_MODE (menu)``
> > > > > > Defines the mode of the flash LED, the high-power white LED attached
> > > > > > to the flash controller. Setting this control may not be possible in
> > > > > > @@ -80,6 +82,8 @@ Flash Control IDs
> > > > > >
> > > > > >
> > > > > >
> > > > > > +.. _v4l2-cid-flash-strobe-source:
> > > > > > +
> > > > > > ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > > > > > Defines the source of the flash LED strobe.
> > > > > >
> > > > > > @@ -186,3 +190,28 @@ Flash Control IDs
> > > > > > charged before strobing. LED flashes often require a cooldown period
> > > > > > after strobe during which another strobe will not be possible. This
> > > > > > is a read-only control.
> > > > > > +
> > > > > > +.. _v4l2-cid-flash-duration:
> > > > > > +
> > > > > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > > > > + Duration of the flash strobe pulse generated by the strobe source,
> > > > > > + typically a camera sensor. This method of controlling flash LED strobe
> > > > > > + duration has three prerequisites: the strobe source's
> > > > > > + :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > > > > + enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > > > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > > > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > > > > + if possible.
> > > > >
> > > > > As mentioned in the review of 01/10, I think this needs to be clarified.
> > > > > Ideally we should add a new document in
> > > > > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > > > > the meantime let's at lets improve the description of the duration
> > > > > control. Here's a proposal.
> > > >
> > > > Understood. Thank you for your proposal!
> > > >
> > > > > ``V4L2_CID_FLASH_DURATION (integer)``
> > > > > Duration of the flash strobe pulse generated by the strobe source, when
> > > > > using external strobe. This control shall be implemented by the device
> > > > > generating the hardware flash strobe signal, typically a camera sensor,
> > > > > connected to a flash controller. It must not be implemented by the flash
> > > > > controller.
> > > > >
> > > > > This method of controlling flash LED strobe duration has three
> > > > > prerequisites: the strobe source's :ref:`hardware strobe signal
> > > > > <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> > > > > :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> > > > > ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> > > > > <v4l2-cid-flash-strobe-source>` must be configured to
> > > > > ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> > > > >
> > > > > The unit should be microseconds (µs) if possible.
> > > > >
> > > > >
> > > > > The second paragraph may be better replaced by expanding the
> > > > > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > > > > place to document how external strobe works.
> > > >
> > > > That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> > > > V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> > > > v9.
> > >
> > > Sakari, could you please check if you agree with the above ? Let's avoid
> > > going back and forth with reviews (and I'll try my best to review the
> > > next version quickly).
> >
> > My current proposal:
> >
> > * - ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``
> > - The flash strobe is triggered by an external source. Typically
> > this is a sensor, which makes it possible to synchronise the
> > flash strobe start to exposure start.
> > This method of controlling flash LED strobe has two additional
> > prerequisites: the strobe source's :ref:`flash strobe output
> > <v4l2-cid-flash-strobe-oe>` must be enabled (if available)
> > and the flash controller's :ref:`flash LED mode
> > <v4l2-cid-flash-led-mode>` must be set to
> > ``V4L2_FLASH_LED_MODE_FLASH``. Additionally the :ref:`flash duration
> > <v4l2-cid-flash-duration>` may be adjusted by the strobe source.
> >
> >
> > ``V4L2_CID_FLASH_DURATION (integer)``
> > Duration of the flash strobe pulse generated by the strobe source, when
> > using external strobe. This control shall be implemented by the device
> > generating the hardware flash strobe signal, typically a camera sensor,
> > connected to a flash controller. It must not be implemented by the flash
> > controller. Typically the flash strobe pulse needs to be activated by
>
> I'd drop the sentence on flash controller as this is UAPI documentation.
Is there any other place where this can/should be documented. As this
was suggested by Laurent: What's your opinion on this?
>
> > enabling the strobe source's :ref:`flash strobe output
> > <v4l2-cid-flash-strobe-oe>`.
> >
> > The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
> > must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
> > mode of operation.
> >
> > The unit should be number of lines if possible.
> >
> >
> > ``V4L2_CID_FLASH_STROBE_OE (boolean)``
> > Enables the output of a hardware strobe signal from the strobe source,
> > when using external strobe. This control shall be implemented by the device
>
> I'd remove the comma.
>
> > generating the hardware flash strobe signal, typically a camera sensor,
> > connected to a flash controller.
> >
> > Provided the signal generating device driver supports it, the length of the
> > strobe signal can be configured by adjusting its
> > :ref:`flash duration <v4l2-cid-flash-duration>`. In case the device has a
> > fixed strobe length, the flash duration control must not be implemented.
>
> I don't see why the duration control wouldn't be implemented in this case:
> it's still relevant for the user space to know how long the duration is.
> I'd simply drop this sentence.
Laurent and I were discussing this a few mails ago in this thread.
He suggested to do not expose the control on fixed strobe length to keep
the drivers simpler as this information is/should be available somewhere
else. I.e. defined by the hardware.
I suggested to add a read-only flash duration control for fixed strobe
length devices. This makes drivers more complicated and may add some
rounding issues when the control is given in (micro) seconds.
As mentioned previously: I have no strong opinion on this. So how to
proceed here?
>
> >
> > The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
> > must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
> > mode of operation.
> >
> > >
> > > > > As for the unit, is microseconds really the best option ? I would expect
> > > > > most sensors to express the strobe pulse width in unit of lines.
> > > >
> > > > We had that discussion already somewhere during this series. Tbh for me
> > > > microseconds seems fine. Most (professional) flashes are configured with
> > > > s^-1, so that would also be an option, but as flash_timeout is
> > > > configured in microseconds, i chose it for flash_duration too.
> > > >
> > > > Nonetheless technically it shouldn't be a problem to express it as
> > > > number of lines... Is there a reason to prefer this?
> > >
> > > A few observations have confirmed my gut feeling that this is how
> > > sensors typically express the pulse width. Expressing the value in its
> > > hardware unit means we won't have rounding issues, and drivers will also
> > > be simpler. We're missing data though, it would be nice to check a wider
> > > variety of camera sensors.
> >
> > I have done some more measurements and calculation on this for ov9281.
> > It seems you are (somehow?) right. The strobe_frame_span (aka strobe
> > duration) register value seems to represent the duration of the strobe in
> > number of lines plus a constant and variable offset based on the hblank
> > value. Other settings (e.g. vblank, exposure, ...) have no influence on
> > the duration.
> >
> > After about 50 measurements using different strobe_frame_span and hblank
> > values and 1280x800 as resolution I came up with the following formulas:
> >
> > line_factor = active_width + hblank * 1,04 + 56
> >
> > t_strobe = strobe_frame_span * line_factor / pixel_rate
> >
> > Which matches all tested cased nicely...
> >
> > Nonetheless I'm still unsure on what unit to use for flash duration...
> >
> > The exposure time for ov9282 is set as "number of row periods, where the
> > low 4 bits are fraction bits" in the registers. The v4l2 control should
> > on the other hand accept 100 µs units as value.
> >
> > From a user perspective it would make sense to me to configure exposure
> > time, flash duration and flash/strobe offset using the same base units.
> > On the other hand we may have rounding issues and formulas based on
> > assumptions or reverse-engineering when implementing this for a
> > sensor...
> >
> > What's your opinion on this, Sakari, Laurent, Dave?
>
> I checked what CCS defines exposure time as a function of the external
> clock frequency:
>
> exposure time = tFlash_strobe_width_ctrl / ext_clk_freq *
> flash_strobe_adjustment
>
> The added accuracy is relevant for xenon (admittedly rare these days, but
> depends on the device) flash but probably not so for LEDs.
>
> So I'm fine with keeping this as-is and perhaps adding CCS specific private
> controls separately.
As mentioned previously I prefer (micro)seconds too for this control.
Yes, it results in more complicated drivers, but makes live for
userspace much easier IMHO.
>
> >
> > >
> > > > > I think we also need to decide how to handle camera sensors whose flash
> > > > > strobe pulse width can't be controlled. For instance, the AR0144 can
> > > > > output a flash signal, and its width is always equal to the exposure
> > > > > time. The most straightforward solution seems to implement
> > > > > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > > > > sensor driver. Could this cause issues in any use case ? Is there a
> > > > > better solution ? I would like this to be documented.
> > > >
> > > > Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> > > > provided as a read-only property too. So userspace is explicitely aware
> > > > of the acutal value and doesn't have to make assumptions.
> > >
> > > The value would change depending on the exposure time. Given how control
> > > change events are implemented that would be difficult to use from
> > > userspace at best. I think not exposing the control would be as useful
> > > as exposing a read-only value, and it would be simpler to implement in
> > > kernel drivers.
> >
> > That's true. I guess keeping the drivers simple and moving this "logic"
> > to a possible client/userspace application (if needed) is fine with me.
> >
> > As you may have seen above, I've tried to integrate this in the
> > documentation proposal already.
> >
> > >
> > > > Should I add documentation on this topic to this patch?
> > >
> > > That would be nice, thank you.
> > >
> > > > > Finally, I think we also need to standardize the flash strobe offset.
> > > >
> > > > I guess I somewhere mentioned this already: I have some patches for
> > > > configuring the strobe offset of ov9282 and adding the corresponding
> > > > v4l2 control. But to keep this series simple I'm planning to send them
> > > > as soon as this one is "done".
> > > >
> > > > IMHO the offset should then have the same unit as the flash_duration.
> > >
> > > What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
> > > expressed in units of half a line.
> > >
> > > > > > +
> > > > > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > > > > +
> > > > > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> > > > >
> > > > > Nitpicking a bit on the name, I would have called this
> > > > > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> > > >
> > > > I'm always open to name-nitpicking ;-)
> > > >
> > > > V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> > > > shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
> > >
> > > Sakari, what's your opinion ?
>
> I slightly prefer the former, too.
Thanks. Fine with me!
So I'll change the name to V4L2_CID_FLASH_STROBE_OE in the next version.
>
> > >
> > > > > > + Enables the output of a hardware strobe signal from the strobe source,
> > > > > > + typically a camera sensor. To control a flash LED driver connected to this
> > > > > > + hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > > + must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > > > > + :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > > > > + ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > > > > + supports it, the length of the strobe signal can be configured by
> > > > > > + adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> > > > >
> > > > > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > > > > in a similar way as V4L2_CID_FLASH_DURATION.
> > > >
> > > > Sure. I will adapt this for v9.
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> >
> > thanks & regards;rl
>
> --
> Kind regards,
>
> Sakari Ailus
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 05/10] media: i2c: ov9282: add output enable register definitions
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
` (3 preceding siblings ...)
2025-09-01 15:05 ` [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL} Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-01 15:05 ` [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control Richard Leitner
` (4 subsequent siblings)
9 siblings, 0 replies; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
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).
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
` (4 preceding siblings ...)
2025-09-01 15:05 ` [PATCH v7 05/10] media: i2c: ov9282: add output enable register definitions Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-01 20:57 ` Sakari Ailus
2025-09-01 15:05 ` [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration " Richard Leitner
` (3 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
"strobe output enable" feature of the sensor.
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 | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 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_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
+{
+ u32 current_val;
+ int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
+ ¤t_val);
+ if (ret)
+ return ret;
+
+ if (enable)
+ 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_HW_STROBE_SIGNAL:
+ ret = ov9282_set_ctrl_flash_hw_strobe_signal(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,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
OV9282_TIMING_HTS_MAX - mode->width,
1, hblank_min);
+ /* Flash/Strobe controls */
+ v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
+
ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
if (!ret) {
/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
--
2.47.2
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control
2025-09-01 15:05 ` [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control Richard Leitner
@ 2025-09-01 20:57 ` Sakari Ailus
2025-09-03 6:58 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Sakari Ailus @ 2025-09-01 20:57 UTC (permalink / raw)
To: Richard Leitner
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
Laurent Pinchart, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Richard,
On Mon, Sep 01, 2025 at 05:05:11PM +0200, Richard Leitner wrote:
> Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
> "strobe output enable" feature of the sensor.
>
> 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 | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 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_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
> +{
> + u32 current_val;
> + int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> + ¤t_val);
> + if (ret)
> + return ret;
Please don't do assignments in variable declaration if that involves error
handling.
> +
> + if (enable)
> + 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_HW_STROBE_SIGNAL:
> + ret = ov9282_set_ctrl_flash_hw_strobe_signal(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,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> OV9282_TIMING_HTS_MAX - mode->width,
> 1, hblank_min);
>
> + /* Flash/Strobe controls */
> + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
This seems rather long.
> +
> ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> if (!ret) {
> /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control
2025-09-01 20:57 ` Sakari Ailus
@ 2025-09-03 6:58 ` Richard Leitner
2025-09-07 20:08 ` Laurent Pinchart
0 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-03 6:58 UTC (permalink / raw)
To: Sakari Ailus
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
Laurent Pinchart, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Sakari,
On Mon, Sep 01, 2025 at 11:57:15PM +0300, Sakari Ailus wrote:
> Hi Richard,
>
> On Mon, Sep 01, 2025 at 05:05:11PM +0200, Richard Leitner wrote:
> > Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
> > "strobe output enable" feature of the sensor.
> >
> > 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 | 25 ++++++++++++++++++++++++-
> > 1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 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_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
> > +{
> > + u32 current_val;
> > + int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > + ¤t_val);
> > + if (ret)
> > + return ret;
>
> Please don't do assignments in variable declaration if that involves error
> handling.
Sure. Will fix that!
>
> > +
> > + if (enable)
> > + 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_HW_STROBE_SIGNAL:
> > + ret = ov9282_set_ctrl_flash_hw_strobe_signal(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,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > OV9282_TIMING_HTS_MAX - mode->width,
> > 1, hblank_min);
> >
> > + /* Flash/Strobe controls */
> > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
>
> This seems rather long.
It's exactly 100 chars wide, so from a policy point of view it should be
fine ;-). But I'm also fine with breaking it to 80 if you prefer?
>
> > +
> > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > if (!ret) {
> > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> >
>
> --
> Regards,
>
> Sakari Ailus
thanks!
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control
2025-09-03 6:58 ` Richard Leitner
@ 2025-09-07 20:08 ` Laurent Pinchart
2025-09-08 12:09 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-07 20:08 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
On Wed, Sep 03, 2025 at 08:58:04AM +0200, Richard Leitner wrote:
> On Mon, Sep 01, 2025 at 11:57:15PM +0300, Sakari Ailus wrote:
> > On Mon, Sep 01, 2025 at 05:05:11PM +0200, Richard Leitner wrote:
> > > Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
> > > "strobe output enable" feature of the sensor.
> > >
> > > 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 | 25 ++++++++++++++++++++++++-
> > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 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_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
> > > +{
> > > + u32 current_val;
> > > + int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > > + ¤t_val);
> > > + if (ret)
> > > + return ret;
> >
> > Please don't do assignments in variable declaration if that involves error
> > handling.
>
> Sure. Will fix that!
>
> > > +
> > > + if (enable)
> > > + 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);
It would be nice to cache the register value instead of reading it back.
Regmap may help (and then the driver should use the CCI helpers). This
can be done separately.
> > > +}
> > > +
> > > /**
> > > * 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_HW_STROBE_SIGNAL:
> > > + ret = ov9282_set_ctrl_flash_hw_strobe_signal(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,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > OV9282_TIMING_HTS_MAX - mode->width,
> > > 1, hblank_min);
> > >
> > > + /* Flash/Strobe controls */
> > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> >
> > This seems rather long.
>
> It's exactly 100 chars wide, so from a policy point of view it should be
> fine ;-). But I'm also fine with breaking it to 80 if you prefer?
That's the usual policy in V4L2, yes. 80 columns is the preferred soft
limit.
> > > +
> > > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > > if (!ret) {
> > > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> > >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control
2025-09-07 20:08 ` Laurent Pinchart
@ 2025-09-08 12:09 ` Richard Leitner
2025-09-08 13:47 ` Laurent Pinchart
0 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-08 12:09 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Laurent,
thanks for your review!
On Sun, Sep 07, 2025 at 10:08:11PM +0200, Laurent Pinchart wrote:
> On Wed, Sep 03, 2025 at 08:58:04AM +0200, Richard Leitner wrote:
> > On Mon, Sep 01, 2025 at 11:57:15PM +0300, Sakari Ailus wrote:
> > > On Mon, Sep 01, 2025 at 05:05:11PM +0200, Richard Leitner wrote:
> > > > Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
> > > > "strobe output enable" feature of the sensor.
> > > >
> > > > 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 | 25 ++++++++++++++++++++++++-
> > > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 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_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
> > > > +{
> > > > + u32 current_val;
> > > > + int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > > > + ¤t_val);
> > > > + if (ret)
> > > > + return ret;
> > >
> > > Please don't do assignments in variable declaration if that involves error
> > > handling.
> >
> > Sure. Will fix that!
> >
> > > > +
> > > > + if (enable)
> > > > + 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);
>
> It would be nice to cache the register value instead of reading it back.
> Regmap may help (and then the driver should use the CCI helpers). This
> can be done separately.
>
Currently all set_ctrl calls in the ov9282 driver have this
read/modify/write pattern. As mentioned in the cover letter I'm planning
to migrate to cci helpers in a future series to keep the set smaller.
But if you prefer the migration in this series I can try to rebase on
it?
> > > > +}
> > > > +
> > > > /**
> > > > * 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_HW_STROBE_SIGNAL:
> > > > + ret = ov9282_set_ctrl_flash_hw_strobe_signal(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,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > OV9282_TIMING_HTS_MAX - mode->width,
> > > > 1, hblank_min);
> > > >
> > > > + /* Flash/Strobe controls */
> > > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > >
> > > This seems rather long.
> >
> > It's exactly 100 chars wide, so from a policy point of view it should be
> > fine ;-). But I'm also fine with breaking it to 80 if you prefer?
>
> That's the usual policy in V4L2, yes. 80 columns is the preferred soft
> limit.
So I should break this line in this case? Tbh I'm often unsure on
breaking on 80 or 100... Personally 100 is fine for me, but that's
"your" subsystem/driver, so I guess it's your descision ;-)
>
> > > > +
> > > > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > > > if (!ret) {
> > > > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control
2025-09-08 12:09 ` Richard Leitner
@ 2025-09-08 13:47 ` Laurent Pinchart
2025-09-08 14:47 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-08 13:47 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
On Mon, Sep 08, 2025 at 02:09:06PM +0200, Richard Leitner wrote:
> On Sun, Sep 07, 2025 at 10:08:11PM +0200, Laurent Pinchart wrote:
> > On Wed, Sep 03, 2025 at 08:58:04AM +0200, Richard Leitner wrote:
> > > On Mon, Sep 01, 2025 at 11:57:15PM +0300, Sakari Ailus wrote:
> > > > On Mon, Sep 01, 2025 at 05:05:11PM +0200, Richard Leitner wrote:
> > > > > Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
> > > > > "strobe output enable" feature of the sensor.
> > > > >
> > > > > 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 | 25 ++++++++++++++++++++++++-
> > > > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > > index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 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_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
> > > > > +{
> > > > > + u32 current_val;
> > > > > + int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > > > > + ¤t_val);
> > > > > + if (ret)
> > > > > + return ret;
> > > >
> > > > Please don't do assignments in variable declaration if that involves error
> > > > handling.
> > >
> > > Sure. Will fix that!
> > >
> > > > > +
> > > > > + if (enable)
> > > > > + 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);
> >
> > It would be nice to cache the register value instead of reading it back.
> > Regmap may help (and then the driver should use the CCI helpers). This
> > can be done separately.
>
> Currently all set_ctrl calls in the ov9282 driver have this
> read/modify/write pattern. As mentioned in the cover letter I'm planning
> to migrate to cci helpers in a future series to keep the set smaller.
> But if you prefer the migration in this series I can try to rebase on
> it?
No, it's fine on top. I ask for enough yak-shaving already :-)
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * 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_HW_STROBE_SIGNAL:
> > > > > + ret = ov9282_set_ctrl_flash_hw_strobe_signal(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,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > > OV9282_TIMING_HTS_MAX - mode->width,
> > > > > 1, hblank_min);
> > > > >
> > > > > + /* Flash/Strobe controls */
> > > > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > > >
> > > > This seems rather long.
> > >
> > > It's exactly 100 chars wide, so from a policy point of view it should be
> > > fine ;-). But I'm also fine with breaking it to 80 if you prefer?
> >
> > That's the usual policy in V4L2, yes. 80 columns is the preferred soft
> > limit.
>
> So I should break this line in this case? Tbh I'm often unsure on
> breaking on 80 or 100... Personally 100 is fine for me, but that's
> "your" subsystem/driver, so I guess it's your descision ;-)
Sakari is even more strict than me about line lengths :-)
It a line is just a couple of charaters about 80 columns and doesn't
have a nice split point I would avoid breaking it as I feel the result
would be less readable.
In this particular case, breaking the line would lead to
v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
which I think is OK. It's very subjective of course.
> > > > > +
> > > > > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > > > > if (!ret) {
> > > > > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> > > > >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control
2025-09-08 13:47 ` Laurent Pinchart
@ 2025-09-08 14:47 ` Richard Leitner
0 siblings, 0 replies; 45+ messages in thread
From: Richard Leitner @ 2025-09-08 14:47 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
On Mon, Sep 08, 2025 at 03:47:35PM +0200, Laurent Pinchart wrote:
> On Mon, Sep 08, 2025 at 02:09:06PM +0200, Richard Leitner wrote:
> > On Sun, Sep 07, 2025 at 10:08:11PM +0200, Laurent Pinchart wrote:
> > > On Wed, Sep 03, 2025 at 08:58:04AM +0200, Richard Leitner wrote:
> > > > On Mon, Sep 01, 2025 at 11:57:15PM +0300, Sakari Ailus wrote:
> > > > > On Mon, Sep 01, 2025 at 05:05:11PM +0200, Richard Leitner wrote:
> > > > > > Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
> > > > > > "strobe output enable" feature of the sensor.
> > > > > >
> > > > > > 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 | 25 ++++++++++++++++++++++++-
> > > > > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > > > index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 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_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
> > > > > > +{
> > > > > > + u32 current_val;
> > > > > > + int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > > > > > + ¤t_val);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > >
> > > > > Please don't do assignments in variable declaration if that involves error
> > > > > handling.
> > > >
> > > > Sure. Will fix that!
> > > >
> > > > > > +
> > > > > > + if (enable)
> > > > > > + 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);
> > >
> > > It would be nice to cache the register value instead of reading it back.
> > > Regmap may help (and then the driver should use the CCI helpers). This
> > > can be done separately.
> >
> > Currently all set_ctrl calls in the ov9282 driver have this
> > read/modify/write pattern. As mentioned in the cover letter I'm planning
> > to migrate to cci helpers in a future series to keep the set smaller.
> > But if you prefer the migration in this series I can try to rebase on
> > it?
>
> No, it's fine on top. I ask for enough yak-shaving already :-)
:-) great. thanks :-)
>
> > > > > > +}
> > > > > > +
> > > > > > /**
> > > > > > * 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_HW_STROBE_SIGNAL:
> > > > > > + ret = ov9282_set_ctrl_flash_hw_strobe_signal(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,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > > > OV9282_TIMING_HTS_MAX - mode->width,
> > > > > > 1, hblank_min);
> > > > > >
> > > > > > + /* Flash/Strobe controls */
> > > > > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > > > >
> > > > > This seems rather long.
> > > >
> > > > It's exactly 100 chars wide, so from a policy point of view it should be
> > > > fine ;-). But I'm also fine with breaking it to 80 if you prefer?
> > >
> > > That's the usual policy in V4L2, yes. 80 columns is the preferred soft
> > > limit.
> >
> > So I should break this line in this case? Tbh I'm often unsure on
> > breaking on 80 or 100... Personally 100 is fine for me, but that's
> > "your" subsystem/driver, so I guess it's your descision ;-)
>
> Sakari is even more strict than me about line lengths :-)
>
> It a line is just a couple of charaters about 80 columns and doesn't
> have a nice split point I would avoid breaking it as I feel the result
> would be less readable.
>
> In this particular case, breaking the line would lead to
>
> v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
>
>
> which I think is OK. It's very subjective of course.
Thanks for the clarification. Sure. That's fine for me.
I will adapt the patch accordingly.
>
> > > > > > +
> > > > > > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > > > > > if (!ret) {
> > > > > > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> > > > > >
>
> --
> Regards,
>
> Laurent Pinchart
thanks & regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
` (5 preceding siblings ...)
2025-09-01 15:05 ` [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-01 20:55 ` Sakari Ailus
2025-09-01 15:05 ` [PATCH v7 08/10] media: i2c: ov9282: add strobe_source " Richard Leitner
` (2 subsequent siblings)
9 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
Add V4L2_CID_FLASH_DURATION 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 default
framerate.
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, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 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
@@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
current_val);
}
+static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 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
@@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
break;
+ case V4L2_CID_FLASH_DURATION:
+ ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
+ break;
default:
dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
ret = -EINVAL;
@@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
u32 lpfr;
int ret;
- ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
+ ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
if (ret)
return ret;
@@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
/* Flash/Strobe controls */
v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
+ 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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control
2025-09-01 15:05 ` [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration " Richard Leitner
@ 2025-09-01 20:55 ` Sakari Ailus
2025-09-03 6:54 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Sakari Ailus @ 2025-09-01 20:55 UTC (permalink / raw)
To: Richard Leitner
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
Laurent Pinchart, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Richard,
On Mon, Sep 01, 2025 at 05:05:12PM +0200, Richard Leitner wrote:
> Add V4L2_CID_FLASH_DURATION 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 default
> framerate.
>
> 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, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 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
>
> @@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
> current_val);
> }
>
> +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 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);
The bitwise and operation is redundant.
Could you do this in a single write?
Also error handling is (largely) missing.
> +}
> +
> /**
> * ov9282_set_ctrl() - Set subdevice control
> * @ctrl: pointer to v4l2_ctrl structure
> @@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
> ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
> break;
> + case V4L2_CID_FLASH_DURATION:
> + ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
> + break;
> default:
> dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> ret = -EINVAL;
> @@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> u32 lpfr;
> int ret;
>
> - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> if (ret)
> return ret;
>
> @@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> /* Flash/Strobe controls */
> v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
>
> + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> + 0, 13900, 1, 8);
> +
> ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> if (!ret) {
> /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control
2025-09-01 20:55 ` Sakari Ailus
@ 2025-09-03 6:54 ` Richard Leitner
2025-09-07 20:18 ` Laurent Pinchart
0 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-03 6:54 UTC (permalink / raw)
To: Laurent Pinchart, Sakari Ailus
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Sakari,
thanks again for the review :)
On Mon, Sep 01, 2025 at 11:55:37PM +0300, Sakari Ailus wrote:
> Hi Richard,
>
> On Mon, Sep 01, 2025 at 05:05:12PM +0200, Richard Leitner wrote:
> > Add V4L2_CID_FLASH_DURATION 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 default
> > framerate.
> >
> > 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, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 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
> >
> > @@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
> > current_val);
> > }
> >
> > +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 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);
>
> The bitwise and operation is redundant.
True. Thanks for the catch!
>
> Could you do this in a single write?
I've implemented this in single byte writes due to some "special
behaviour" of the vision components ov9281 modules. On those modules
single byte interactions seem broken in some cases. Maybe Laurent knows
more about this and the current state, as he was/is in contact with VC.
See also: https://lore.kernel.org/all/918ce2ca-55ff-aff8-ea6c-0c17f566d59d@online.de/
Nonetheless, thanks for the pointer. I haven't documented this
accordingly. I will try to reproduce the issue again and either change
this to a single write or add a describing comment.
>
> Also error handling is (largely) missing.
Good catch. Thanks.
>
> > +}
> > +
> > /**
> > * ov9282_set_ctrl() - Set subdevice control
> > * @ctrl: pointer to v4l2_ctrl structure
> > @@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> > case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
> > ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
> > break;
> > + case V4L2_CID_FLASH_DURATION:
> > + ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
> > + break;
> > default:
> > dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> > ret = -EINVAL;
> > @@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > u32 lpfr;
> > int ret;
> >
> > - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> > if (ret)
> > return ret;
> >
> > @@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > /* Flash/Strobe controls */
> > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> >
> > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > + 0, 13900, 1, 8);
> > +
> > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > if (!ret) {
> > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> >
>
> --
> Regards,
>
> Sakari Ailus
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control
2025-09-03 6:54 ` Richard Leitner
@ 2025-09-07 20:18 ` Laurent Pinchart
2025-09-07 20:21 ` Laurent Pinchart
0 siblings, 1 reply; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-07 20:18 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
On Wed, Sep 03, 2025 at 08:54:42AM +0200, Richard Leitner wrote:
> On Mon, Sep 01, 2025 at 11:55:37PM +0300, Sakari Ailus wrote:
> > On Mon, Sep 01, 2025 at 05:05:12PM +0200, Richard Leitner wrote:
> > > Add V4L2_CID_FLASH_DURATION 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).
You name the register OV9282_REG_FLASH_DURATION below. Is
"FLASH_DURATION" a term found in the datasheet ?
> > >
> > > The maximum control value is set to the period of the current default
> > > framerate.
Should it be adjusted based on the sensor 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, 30 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 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
> > >
> > > @@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
> > > current_val);
> > > }
> > >
> > > +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 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);
I wonder if the register value ends up being expressed as a number of
lines.
> > > +
> > > + 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);
The CCI helpers would make this much simpler.
> > The bitwise and operation is redundant.
>
> True. Thanks for the catch!
>
> > Could you do this in a single write?
>
> I've implemented this in single byte writes due to some "special
> behaviour" of the vision components ov9281 modules. On those modules
> single byte interactions seem broken in some cases. Maybe Laurent knows
> more about this and the current state, as he was/is in contact with VC.
>
> See also: https://lore.kernel.org/all/918ce2ca-55ff-aff8-ea6c-0c17f566d59d@online.de/
>
> Nonetheless, thanks for the pointer. I haven't documented this
> accordingly. I will try to reproduce the issue again and either change
> this to a single write or add a describing comment.
>
> > Also error handling is (largely) missing.
>
> Good catch. Thanks.
>
> > > +}
> > > +
> > > /**
> > > * ov9282_set_ctrl() - Set subdevice control
> > > * @ctrl: pointer to v4l2_ctrl structure
> > > @@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> > > case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
> > > ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
> > > break;
> > > + case V4L2_CID_FLASH_DURATION:
> > > + ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
> > > + break;
> > > default:
> > > dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> > > ret = -EINVAL;
> > > @@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > u32 lpfr;
> > > int ret;
> > >
> > > - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> > > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > /* Flash/Strobe controls */
> > > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > >
> > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > + 0, 13900, 1, 8);
> > > +
> > > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > > if (!ret) {
> > > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control
2025-09-07 20:18 ` Laurent Pinchart
@ 2025-09-07 20:21 ` Laurent Pinchart
2025-09-08 11:57 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-07 20:21 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
On Sun, Sep 07, 2025 at 10:18:40PM +0200, Laurent Pinchart wrote:
> On Wed, Sep 03, 2025 at 08:54:42AM +0200, Richard Leitner wrote:
> > On Mon, Sep 01, 2025 at 11:55:37PM +0300, Sakari Ailus wrote:
> > > On Mon, Sep 01, 2025 at 05:05:12PM +0200, Richard Leitner wrote:
> > > > Add V4L2_CID_FLASH_DURATION 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).
>
> You name the register OV9282_REG_FLASH_DURATION below. Is
> "FLASH_DURATION" a term found in the datasheet ?
>
> > > >
> > > > The maximum control value is set to the period of the current default
> > > > framerate.
>
> Should it be adjusted based on the sensor configuration ?
I've now noticed patch 10/10.
> > > >
> > > > 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, 30 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 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
> > > >
> > > > @@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
> > > > current_val);
> > > > }
> > > >
> > > > +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 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);
>
> I wonder if the register value ends up being expressed as a number of
> lines.
>
> > > > +
> > > > + 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);
>
> The CCI helpers would make this much simpler.
>
> > > The bitwise and operation is redundant.
> >
> > True. Thanks for the catch!
> >
> > > Could you do this in a single write?
> >
> > I've implemented this in single byte writes due to some "special
> > behaviour" of the vision components ov9281 modules. On those modules
> > single byte interactions seem broken in some cases. Maybe Laurent knows
> > more about this and the current state, as he was/is in contact with VC.
> >
> > See also: https://lore.kernel.org/all/918ce2ca-55ff-aff8-ea6c-0c17f566d59d@online.de/
> >
> > Nonetheless, thanks for the pointer. I haven't documented this
> > accordingly. I will try to reproduce the issue again and either change
> > this to a single write or add a describing comment.
> >
> > > Also error handling is (largely) missing.
> >
> > Good catch. Thanks.
> >
> > > > +}
> > > > +
> > > > /**
> > > > * ov9282_set_ctrl() - Set subdevice control
> > > > * @ctrl: pointer to v4l2_ctrl structure
> > > > @@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
> > > > ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
> > > > break;
> > > > + case V4L2_CID_FLASH_DURATION:
> > > > + ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
> > > > + break;
> > > > default:
> > > > dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> > > > ret = -EINVAL;
> > > > @@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > u32 lpfr;
> > > > int ret;
> > > >
> > > > - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> > > > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > @@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > /* Flash/Strobe controls */
> > > > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > > >
> > > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > + 0, 13900, 1, 8);
> > > > +
> > > > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > > > if (!ret) {
> > > > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control
2025-09-07 20:21 ` Laurent Pinchart
@ 2025-09-08 11:57 ` Richard Leitner
2025-09-08 13:54 ` Laurent Pinchart
0 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-08 11:57 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Laurent,
thanks for the review!
On Sun, Sep 07, 2025 at 10:21:54PM +0200, Laurent Pinchart wrote:
> On Sun, Sep 07, 2025 at 10:18:40PM +0200, Laurent Pinchart wrote:
> > On Wed, Sep 03, 2025 at 08:54:42AM +0200, Richard Leitner wrote:
> > > On Mon, Sep 01, 2025 at 11:55:37PM +0300, Sakari Ailus wrote:
> > > > On Mon, Sep 01, 2025 at 05:05:12PM +0200, Richard Leitner wrote:
> > > > > Add V4L2_CID_FLASH_DURATION 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).
> >
> > You name the register OV9282_REG_FLASH_DURATION below. Is
> > "FLASH_DURATION" a term found in the datasheet ?
No, the datasheet named the registers strobe_frame_span. I guess it may
be clearer to name it OV9282_REG_STROBE_FRAME_SPAN then... I will update
the series accordingly. Thanks for the catch!
> >
> > > > >
> > > > > The maximum control value is set to the period of the current default
> > > > > framerate.
> >
> > Should it be adjusted based on the sensor configuration ?
>
> I've now noticed patch 10/10.
Is this a problem? Should those patches be merged? The reason for this
being a separate patch is it based on review by Sakari and therefore
joined the series in v4. I left it in a separate patch as IMHO it's
easier to review this way. But I'm open for merging it into this one if
that's preferred.
>
> > > > >
> > > > > 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, 30 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > > index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 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
> > > > >
> > > > > @@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
> > > > > current_val);
> > > > > }
> > > > >
> > > > > +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 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);
> >
> > I wonder if the register value ends up being expressed as a number of
> > lines.
I'm not sure... As mentioned in the comment this is not clearly
documented in the datasheet. Tbh, I don't think it's "number of lines",
but I can do some more measurements just to be sure...
> >
> > > > > +
> > > > > + 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);
> >
> > The CCI helpers would make this much simpler.
As mentioned in the cover letter I'm planning to do the migration to
v4l2-cci helpers in a seprate series. If that's fine with you I prefer
to stick to this plan :)
> >
> > > > The bitwise and operation is redundant.
> > >
> > > True. Thanks for the catch!
> > >
> > > > Could you do this in a single write?
> > >
> > > I've implemented this in single byte writes due to some "special
> > > behaviour" of the vision components ov9281 modules. On those modules
> > > single byte interactions seem broken in some cases. Maybe Laurent knows
> > > more about this and the current state, as he was/is in contact with VC.
> > >
> > > See also: https://lore.kernel.org/all/918ce2ca-55ff-aff8-ea6c-0c17f566d59d@online.de/
> > >
> > > Nonetheless, thanks for the pointer. I haven't documented this
> > > accordingly. I will try to reproduce the issue again and either change
> > > this to a single write or add a describing comment.
> > >
> > > > Also error handling is (largely) missing.
> > >
> > > Good catch. Thanks.
> > >
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * ov9282_set_ctrl() - Set subdevice control
> > > > > * @ctrl: pointer to v4l2_ctrl structure
> > > > > @@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
> > > > > ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
> > > > > break;
> > > > > + case V4L2_CID_FLASH_DURATION:
> > > > > + ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
> > > > > + break;
> > > > > default:
> > > > > dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> > > > > ret = -EINVAL;
> > > > > @@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > > u32 lpfr;
> > > > > int ret;
> > > > >
> > > > > - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> > > > > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > @@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > > /* Flash/Strobe controls */
> > > > > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > > > >
> > > > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > > + 0, 13900, 1, 8);
> > > > > +
> > > > > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > > > > if (!ret) {
> > > > > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
>
> --
> Regards,
>
> Laurent Pinchart
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control
2025-09-08 11:57 ` Richard Leitner
@ 2025-09-08 13:54 ` Laurent Pinchart
0 siblings, 0 replies; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-08 13:54 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
On Mon, Sep 08, 2025 at 01:57:48PM +0200, Richard Leitner wrote:
> On Sun, Sep 07, 2025 at 10:21:54PM +0200, Laurent Pinchart wrote:
> > On Sun, Sep 07, 2025 at 10:18:40PM +0200, Laurent Pinchart wrote:
> > > On Wed, Sep 03, 2025 at 08:54:42AM +0200, Richard Leitner wrote:
> > > > On Mon, Sep 01, 2025 at 11:55:37PM +0300, Sakari Ailus wrote:
> > > > > On Mon, Sep 01, 2025 at 05:05:12PM +0200, Richard Leitner wrote:
> > > > > > Add V4L2_CID_FLASH_DURATION 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).
> > >
> > > You name the register OV9282_REG_FLASH_DURATION below. Is
> > > "FLASH_DURATION" a term found in the datasheet ?
>
> No, the datasheet named the registers strobe_frame_span. I guess it may
> be clearer to name it OV9282_REG_STROBE_FRAME_SPAN then... I will update
> the series accordingly. Thanks for the catch!
>
> > > > > >
> > > > > > The maximum control value is set to the period of the current default
> > > > > > framerate.
> > >
> > > Should it be adjusted based on the sensor configuration ?
> >
> > I've now noticed patch 10/10.
>
> Is this a problem? Should those patches be merged? The reason for this
> being a separate patch is it based on review by Sakari and therefore
> joined the series in v4. I left it in a separate patch as IMHO it's
> easier to review this way. But I'm open for merging it into this one if
> that's preferred.
It's fine as a separate patch.
> > > > > >
> > > > > > 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, 30 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > > > index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 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
> > > > > >
> > > > > > @@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
> > > > > > current_val);
> > > > > > }
> > > > > >
> > > > > > +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 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);
> > >
> > > I wonder if the register value ends up being expressed as a number of
> > > lines.
>
> I'm not sure... As mentioned in the comment this is not clearly
> documented in the datasheet. Tbh, I don't think it's "number of lines",
> but I can do some more measurements just to be sure...
Based on the above formula, the register value is
val = time (s) * 192e6 / line length
I would guess that the pixel rate is 192MPixel/s, which would lead to
the the register value expressed as a number of lines.
> > > > > > +
> > > > > > + 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);
> > >
> > > The CCI helpers would make this much simpler.
>
> As mentioned in the cover letter I'm planning to do the migration to
> v4l2-cci helpers in a seprate series. If that's fine with you I prefer
> to stick to this plan :)
That's all fine. Thank you for the continuous improvements to the driver
:-)
> > > > > The bitwise and operation is redundant.
> > > >
> > > > True. Thanks for the catch!
> > > >
> > > > > Could you do this in a single write?
> > > >
> > > > I've implemented this in single byte writes due to some "special
> > > > behaviour" of the vision components ov9281 modules. On those modules
> > > > single byte interactions seem broken in some cases. Maybe Laurent knows
> > > > more about this and the current state, as he was/is in contact with VC.
> > > >
> > > > See also: https://lore.kernel.org/all/918ce2ca-55ff-aff8-ea6c-0c17f566d59d@online.de/
> > > >
> > > > Nonetheless, thanks for the pointer. I haven't documented this
> > > > accordingly. I will try to reproduce the issue again and either change
> > > > this to a single write or add a describing comment.
> > > >
> > > > > Also error handling is (largely) missing.
> > > >
> > > > Good catch. Thanks.
> > > >
> > > > > > +}
> > > > > > +
> > > > > > /**
> > > > > > * ov9282_set_ctrl() - Set subdevice control
> > > > > > * @ctrl: pointer to v4l2_ctrl structure
> > > > > > @@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
> > > > > > ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
> > > > > > break;
> > > > > > + case V4L2_CID_FLASH_DURATION:
> > > > > > + ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
> > > > > > + break;
> > > > > > default:
> > > > > > dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> > > > > > ret = -EINVAL;
> > > > > > @@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > > > u32 lpfr;
> > > > > > int ret;
> > > > > >
> > > > > > - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> > > > > > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> > > > > > if (ret)
> > > > > > return ret;
> > > > > >
> > > > > > @@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > > > /* Flash/Strobe controls */
> > > > > > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > > > > >
> > > > > > + v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > > > + 0, 13900, 1, 8);
> > > > > > +
> > > > > > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > > > > > if (!ret) {
> > > > > > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 08/10] media: i2c: ov9282: add strobe_source v4l2 control
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
` (6 preceding siblings ...)
2025-09-01 15:05 ` [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration " Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-07 20:20 ` Laurent Pinchart
2025-09-01 15:05 ` [PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration Richard Leitner
2025-09-01 15:05 ` [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum Richard Leitner
9 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
Add read-only V4L2_CID_FLASH_STROBE_SOURCE control. Its value is fixed
to V4L2_FLASH_STROBE_SOURCE_EXTERNAL as the camera sensor triggers the
strobe based on its register settings.
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
drivers/media/i2c/ov9282.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c405e3411daf37cf98d5af3535702f8321394af5..9efc82a1929a76c6fb245e7dbfb5276a133d1c5d 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -1368,11 +1368,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
const struct ov9282_mode *mode = ov9282->cur_mode;
struct v4l2_fwnode_device_properties props;
+ struct v4l2_ctrl *ctrl;
u32 hblank_min;
u32 lpfr;
int ret;
- ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
+ ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
if (ret)
return ret;
@@ -1443,6 +1444,14 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
0, 13900, 1, 8);
+ ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
+ V4L2_CID_FLASH_STROBE_SOURCE,
+ V4L2_FLASH_STROBE_SOURCE_EXTERNAL,
+ ~(1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL),
+ V4L2_FLASH_STROBE_SOURCE_EXTERNAL);
+ if (ctrl)
+ ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
if (!ret) {
/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
--
2.47.2
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 08/10] media: i2c: ov9282: add strobe_source v4l2 control
2025-09-01 15:05 ` [PATCH v7 08/10] media: i2c: ov9282: add strobe_source " Richard Leitner
@ 2025-09-07 20:20 ` Laurent Pinchart
2025-09-08 11:38 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Laurent Pinchart @ 2025-09-07 20:20 UTC (permalink / raw)
To: Richard Leitner
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Richard,
Thank you for the patch.
On Mon, Sep 01, 2025 at 05:05:13PM +0200, Richard Leitner wrote:
> Add read-only V4L2_CID_FLASH_STROBE_SOURCE control. Its value is fixed
> to V4L2_FLASH_STROBE_SOURCE_EXTERNAL as the camera sensor triggers the
> strobe based on its register settings.
I don't think you should implement this control in the sensor driver. It
should only be implemented in the flash controller driver to select the
source.
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
> drivers/media/i2c/ov9282.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index c405e3411daf37cf98d5af3535702f8321394af5..9efc82a1929a76c6fb245e7dbfb5276a133d1c5d 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -1368,11 +1368,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
> const struct ov9282_mode *mode = ov9282->cur_mode;
> struct v4l2_fwnode_device_properties props;
> + struct v4l2_ctrl *ctrl;
> u32 hblank_min;
> u32 lpfr;
> int ret;
>
> - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
> if (ret)
> return ret;
>
> @@ -1443,6 +1444,14 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> 0, 13900, 1, 8);
>
> + ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> + V4L2_CID_FLASH_STROBE_SOURCE,
> + V4L2_FLASH_STROBE_SOURCE_EXTERNAL,
> + ~(1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL),
> + V4L2_FLASH_STROBE_SOURCE_EXTERNAL);
> + if (ctrl)
> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> if (!ret) {
> /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 08/10] media: i2c: ov9282: add strobe_source v4l2 control
2025-09-07 20:20 ` Laurent Pinchart
@ 2025-09-08 11:38 ` Richard Leitner
0 siblings, 0 replies; 45+ messages in thread
From: Richard Leitner @ 2025-09-08 11:38 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, linux-media, linux-kernel, linux-leds, Hans Verkuil
Hi Laurent,
thanks for your feedback!
On Sun, Sep 07, 2025 at 10:20:49PM +0200, Laurent Pinchart wrote:
> Hi Richard,
>
> Thank you for the patch.
>
> On Mon, Sep 01, 2025 at 05:05:13PM +0200, Richard Leitner wrote:
> > Add read-only V4L2_CID_FLASH_STROBE_SOURCE control. Its value is fixed
> > to V4L2_FLASH_STROBE_SOURCE_EXTERNAL as the camera sensor triggers the
> > strobe based on its register settings.
>
> I don't think you should implement this control in the sensor driver. It
> should only be implemented in the flash controller driver to select the
> source.
Totally true. This was introduced in the first version of this series to
enable the flash output. Now as we have V4L2_CID_FLASH_STROBE_SOURCE (or
however it will be named finally) this, for no reason, became a read-only
property. This shouldn't be there at all.
Thanks for the catch. I will remove it from the series!
>
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> > drivers/media/i2c/ov9282.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index c405e3411daf37cf98d5af3535702f8321394af5..9efc82a1929a76c6fb245e7dbfb5276a133d1c5d 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -1368,11 +1368,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
> > const struct ov9282_mode *mode = ov9282->cur_mode;
> > struct v4l2_fwnode_device_properties props;
> > + struct v4l2_ctrl *ctrl;
> > u32 hblank_min;
> > u32 lpfr;
> > int ret;
> >
> > - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
> > if (ret)
> > return ret;
> >
> > @@ -1443,6 +1444,14 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > 0, 13900, 1, 8);
> >
> > + ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> > + V4L2_CID_FLASH_STROBE_SOURCE,
> > + V4L2_FLASH_STROBE_SOURCE_EXTERNAL,
> > + ~(1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL),
> > + V4L2_FLASH_STROBE_SOURCE_EXTERNAL);
> > + if (ctrl)
> > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> > if (!ret) {
> > /* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
>
> --
> Regards,
>
> Laurent Pinchart
thanks again!
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
` (7 preceding siblings ...)
2025-09-01 15:05 ` [PATCH v7 08/10] media: i2c: ov9282: add strobe_source " Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-01 21:06 ` Sakari Ailus
2025-09-05 17:31 ` kernel test robot
2025-09-01 15:05 ` [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum Richard Leitner
9 siblings, 2 replies; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
As the granularity of the hardware supported values is lower than the
control value, implement a try_ctrl() function for
V4L2_CID_FLASH_DURATION. This function calculates the nearest possible
µs strobe duration for the given value and returns it back to the
caller.
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
drivers/media/i2c/ov9282.c | 54 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 9efc82a1929a76c6fb245e7dbfb5276a133d1c5d..b104ae77f00e9e7777342e48b7bf3caa6d297f69 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -128,6 +128,8 @@
#define OV9282_REG_MIN 0x00
#define OV9282_REG_MAX 0xfffff
+#define OV9282_STROBE_SPAN_FACTOR 192
+
static const char * const ov9282_supply_names[] = {
"avdd", /* Analog power */
"dovdd", /* Digital I/O power */
@@ -691,7 +693,7 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
current_val);
}
-static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
+static u32 ov9282_us_to_flash_duration(struct ov9282 *ov9282, u32 value)
{
/*
* Calculate "strobe_frame_span" increments from a given value (µs).
@@ -702,7 +704,27 @@ static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
* 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);
+ u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
+
+ return value * OV9282_STROBE_SPAN_FACTOR / frame_width;
+}
+
+static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value)
+{
+ /*
+ * As the calculation in ov9282_us_to_flash_duration uses an integer
+ * divison calculate in ns here to get more precision. Then check if
+ * we need to compensate that divison by incrementing the µs result.
+ */
+ u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
+ u64 ns = value * 1000 * frame_width / OV9282_STROBE_SPAN_FACTOR;
+
+ return DIV_ROUND_UP(ns, 1000);
+}
+
+static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
+{
+ u32 val = ov9282_us_to_flash_duration(ov9282, value);
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);
@@ -792,9 +814,37 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
return ret;
}
+static int ov9282_try_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct ov9282 *ov9282 =
+ container_of(ctrl->handler, struct ov9282, ctrl_handler);
+
+ if (ctrl->id == V4L2_CID_FLASH_DURATION) {
+ u32 fd = ov9282_us_to_flash_duration(ov9282, ctrl->val);
+ u32 us = ctrl->val;
+
+ /* get nearest strobe_duration value */
+ u32 us0 = ov9282_flash_duration_to_us(ov9282, fd);
+ u32 us1 = ov9282_flash_duration_to_us(ov9282, (fd + 1));
+
+ if (abs(us1 - us) < abs(us - us0))
+ ctrl->val = us1;
+ else
+ ctrl->val = us0;
+
+ if (us != ctrl->val) {
+ dev_dbg(ov9282->dev, "using next valid strobe_duration %u instead of %u\n",
+ ctrl->val, us);
+ }
+ }
+
+ return 0;
+}
+
/* V4l2 subdevice control ops*/
static const struct v4l2_ctrl_ops ov9282_ctrl_ops = {
.s_ctrl = ov9282_set_ctrl,
+ .try_ctrl = ov9282_try_ctrl,
};
/**
--
2.47.2
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration
2025-09-01 15:05 ` [PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration Richard Leitner
@ 2025-09-01 21:06 ` Sakari Ailus
2025-09-05 17:31 ` kernel test robot
1 sibling, 0 replies; 45+ messages in thread
From: Sakari Ailus @ 2025-09-01 21:06 UTC (permalink / raw)
To: Richard Leitner
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
Laurent Pinchart, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Richard,
On Mon, Sep 01, 2025 at 05:05:14PM +0200, Richard Leitner wrote:
> As the granularity of the hardware supported values is lower than the
> control value, implement a try_ctrl() function for
> V4L2_CID_FLASH_DURATION. This function calculates the nearest possible
> µs strobe duration for the given value and returns it back to the
> caller.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
> drivers/media/i2c/ov9282.c | 54 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 9efc82a1929a76c6fb245e7dbfb5276a133d1c5d..b104ae77f00e9e7777342e48b7bf3caa6d297f69 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -128,6 +128,8 @@
> #define OV9282_REG_MIN 0x00
> #define OV9282_REG_MAX 0xfffff
>
> +#define OV9282_STROBE_SPAN_FACTOR 192
> +
> static const char * const ov9282_supply_names[] = {
> "avdd", /* Analog power */
> "dovdd", /* Digital I/O power */
> @@ -691,7 +693,7 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
> current_val);
> }
>
> -static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
> +static u32 ov9282_us_to_flash_duration(struct ov9282 *ov9282, u32 value)
> {
> /*
> * Calculate "strobe_frame_span" increments from a given value (µs).
> @@ -702,7 +704,27 @@ static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
> * 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);
> + u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
> +
> + return value * OV9282_STROBE_SPAN_FACTOR / frame_width;
> +}
> +
> +static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value)
> +{
> + /*
> + * As the calculation in ov9282_us_to_flash_duration uses an integer
> + * divison calculate in ns here to get more precision. Then check if
> + * we need to compensate that divison by incrementing the µs result.
> + */
> + u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
> + u64 ns = value * 1000 * frame_width / OV9282_STROBE_SPAN_FACTOR;
> +
> + return DIV_ROUND_UP(ns, 1000);
> +}
> +
> +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
> +{
> + u32 val = ov9282_us_to_flash_duration(ov9282, value);
>
> 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);
> @@ -792,9 +814,37 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> return ret;
> }
>
> +static int ov9282_try_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct ov9282 *ov9282 =
> + container_of(ctrl->handler, struct ov9282, ctrl_handler);
container_of_const(), please.
> +
> + if (ctrl->id == V4L2_CID_FLASH_DURATION) {
> + u32 fd = ov9282_us_to_flash_duration(ov9282, ctrl->val);
> + u32 us = ctrl->val;
You could use us as argument to ov9282_us_to_flash_duration() if you switch
the order.
> +
> + /* get nearest strobe_duration value */
> + u32 us0 = ov9282_flash_duration_to_us(ov9282, fd);
> + u32 us1 = ov9282_flash_duration_to_us(ov9282, (fd + 1));
Redundant parentheses.
> +
> + if (abs(us1 - us) < abs(us - us0))
> + ctrl->val = us1;
> + else
> + ctrl->val = us0;
> +
> + if (us != ctrl->val) {
> + dev_dbg(ov9282->dev, "using next valid strobe_duration %u instead of %u\n",
> + ctrl->val, us);
> + }
Redundant braces.
> + }
> +
> + return 0;
> +}
> +
> /* V4l2 subdevice control ops*/
> static const struct v4l2_ctrl_ops ov9282_ctrl_ops = {
> .s_ctrl = ov9282_set_ctrl,
> + .try_ctrl = ov9282_try_ctrl,
> };
>
> /**
>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration
2025-09-01 15:05 ` [PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration Richard Leitner
2025-09-01 21:06 ` Sakari Ailus
@ 2025-09-05 17:31 ` kernel test robot
1 sibling, 0 replies; 45+ messages in thread
From: kernel test robot @ 2025-09-05 17:31 UTC (permalink / raw)
To: Richard Leitner, Sakari Ailus, Dave Stevenson,
Mauro Carvalho Chehab, Lee Jones, Pavel Machek, Laurent Pinchart
Cc: oe-kbuild-all, linux-media, linux-kernel, linux-leds,
Richard Leitner, Hans Verkuil
Hi Richard,
kernel test robot noticed the following build errors:
[auto build test ERROR on d9946fe286439c2aeaa7953b8c316efe5b83d515]
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Leitner/media-v4l-ctrls-add-a-control-for-flash-strobe-duration/20250901-231141
base: d9946fe286439c2aeaa7953b8c316efe5b83d515
patch link: https://lore.kernel.org/r/20250901-ov9282-flash-strobe-v7-9-d58d5a694afc%40linux.dev
patch subject: [PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration
config: sh-randconfig-002-20250905 (https://download.01.org/0day-ci/archive/20250906/202509060108.kK81M3GB-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250906/202509060108.kK81M3GB-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509060108.kK81M3GB-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__udivdi3" [drivers/media/i2c/ov9282.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum
2025-09-01 15:05 [PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282 Richard Leitner
` (8 preceding siblings ...)
2025-09-01 15:05 ` [PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration Richard Leitner
@ 2025-09-01 15:05 ` Richard Leitner
2025-09-01 21:16 ` Sakari Ailus
9 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-01 15:05 UTC (permalink / raw)
To: Sakari Ailus, Dave Stevenson, Mauro Carvalho Chehab, Lee Jones,
Pavel Machek, Laurent Pinchart
Cc: linux-media, linux-kernel, linux-leds, Richard Leitner,
Hans Verkuil
This patch sets the current exposure time as maximum for the
flash_duration control. As Flash/Strobes which are longer than the
exposure time have no effect.
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
drivers/media/i2c/ov9282.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index b104ae77f00e9e7777342e48b7bf3caa6d297f69..3253d9f271cb3caef6d85837ebec4f5beb466a4d 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -198,6 +198,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_duration: Pointer to flash duration control
* @vblank: Vertical blanking in lines
* @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
* @cur_mode: Pointer to current selected sensor mode
@@ -220,6 +221,7 @@ struct ov9282 {
struct v4l2_ctrl *again_ctrl;
};
struct v4l2_ctrl *pixel_rate;
+ struct v4l2_ctrl *flash_duration;
u32 vblank;
bool noncontinuous_clock;
const struct ov9282_mode *cur_mode;
@@ -611,6 +613,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
mode->vblank_max, 1, mode->vblank);
}
+static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure)
+{
+ /* calculate exposure time in µs */
+ u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
+ u32 trow_us = (frame_width * 1000000UL) / ov9282->pixel_rate->val;
+
+ return exposure * trow_us;
+}
+
/**
* ov9282_update_exp_gain() - Set updated exposure and gain
* @ov9282: pointer to ov9282 device
@@ -622,9 +633,10 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
{
int ret;
+ u32 exposure_us = ov9282_exposure_to_us(ov9282, exposure);
- dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
- exposure, gain);
+ dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u",
+ exposure, exposure_us, gain);
ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
if (ret)
@@ -635,6 +647,12 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
goto error_release_group_hold;
ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
+ if (ret)
+ goto error_release_group_hold;
+
+ ret = __v4l2_ctrl_modify_range(ov9282->flash_duration,
+ 0, exposure_us, 1,
+ OV9282_FLASH_DURATION_DEFAULT);
error_release_group_hold:
ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
@@ -1420,6 +1438,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
struct v4l2_fwnode_device_properties props;
struct v4l2_ctrl *ctrl;
u32 hblank_min;
+ u32 exposure_us;
u32 lpfr;
int ret;
@@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
/* Flash/Strobe controls */
v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
- v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
- 0, 13900, 1, 8);
+ exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
+ ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
+ &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
+ 0, exposure_us,
+ 1, OV9282_FLASH_DURATION_DEFAULT);
ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
V4L2_CID_FLASH_STROBE_SOURCE,
--
2.47.2
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum
2025-09-01 15:05 ` [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum Richard Leitner
@ 2025-09-01 21:16 ` Sakari Ailus
2025-09-03 7:13 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Sakari Ailus @ 2025-09-01 21:16 UTC (permalink / raw)
To: Richard Leitner
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
Laurent Pinchart, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Richard,
On Mon, Sep 01, 2025 at 05:05:15PM +0200, Richard Leitner wrote:
> This patch sets the current exposure time as maximum for the
> flash_duration control. As Flash/Strobes which are longer than the
> exposure time have no effect.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
> drivers/media/i2c/ov9282.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index b104ae77f00e9e7777342e48b7bf3caa6d297f69..3253d9f271cb3caef6d85837ebec4f5beb466a4d 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -198,6 +198,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_duration: Pointer to flash duration control
> * @vblank: Vertical blanking in lines
> * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
> * @cur_mode: Pointer to current selected sensor mode
> @@ -220,6 +221,7 @@ struct ov9282 {
> struct v4l2_ctrl *again_ctrl;
> };
> struct v4l2_ctrl *pixel_rate;
> + struct v4l2_ctrl *flash_duration;
> u32 vblank;
> bool noncontinuous_clock;
> const struct ov9282_mode *cur_mode;
> @@ -611,6 +613,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
> mode->vblank_max, 1, mode->vblank);
> }
>
> +static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure)
> +{
> + /* calculate exposure time in µs */
> + u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
> + u32 trow_us = (frame_width * 1000000UL) / ov9282->pixel_rate->val;
Redundant parentheses.
> +
> + return exposure * trow_us;
> +}
> +
> /**
> * ov9282_update_exp_gain() - Set updated exposure and gain
> * @ov9282: pointer to ov9282 device
> @@ -622,9 +633,10 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
> static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> {
> int ret;
> + u32 exposure_us = ov9282_exposure_to_us(ov9282, exposure);
>
> - dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
> - exposure, gain);
> + dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u",
> + exposure, exposure_us, gain);
>
> ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
> if (ret)
> @@ -635,6 +647,12 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> goto error_release_group_hold;
>
> ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> + if (ret)
> + goto error_release_group_hold;
> +
> + ret = __v4l2_ctrl_modify_range(ov9282->flash_duration,
> + 0, exposure_us, 1,
> + OV9282_FLASH_DURATION_DEFAULT);
>
> error_release_group_hold:
> ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> @@ -1420,6 +1438,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> struct v4l2_fwnode_device_properties props;
> struct v4l2_ctrl *ctrl;
> u32 hblank_min;
> + u32 exposure_us;
> u32 lpfr;
> int ret;
>
> @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> /* Flash/Strobe controls */
> v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
>
> - v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> - 0, 13900, 1, 8);
> + exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> + ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> + &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> + 0, exposure_us,
> + 1, OV9282_FLASH_DURATION_DEFAULT);
Wrap this differently, please, e.g. after '='.
>
> ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> V4L2_CID_FLASH_STROBE_SOURCE,
>
To me the set looks good but I wouldn't mind about having a bit more
review.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum
2025-09-01 21:16 ` Sakari Ailus
@ 2025-09-03 7:13 ` Richard Leitner
2025-09-03 7:48 ` Sakari Ailus
0 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-03 7:13 UTC (permalink / raw)
To: Sakari Ailus
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
Laurent Pinchart, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Sakari,
On Tue, Sep 02, 2025 at 12:16:51AM +0300, Sakari Ailus wrote:
> Hi Richard,
>
> On Mon, Sep 01, 2025 at 05:05:15PM +0200, Richard Leitner wrote:
> > This patch sets the current exposure time as maximum for the
> > flash_duration control. As Flash/Strobes which are longer than the
> > exposure time have no effect.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> > drivers/media/i2c/ov9282.c | 30 ++++++++++++++++++++++++++----
> > 1 file changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index b104ae77f00e9e7777342e48b7bf3caa6d297f69..3253d9f271cb3caef6d85837ebec4f5beb466a4d 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -198,6 +198,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_duration: Pointer to flash duration control
> > * @vblank: Vertical blanking in lines
> > * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
> > * @cur_mode: Pointer to current selected sensor mode
> > @@ -220,6 +221,7 @@ struct ov9282 {
> > struct v4l2_ctrl *again_ctrl;
> > };
> > struct v4l2_ctrl *pixel_rate;
> > + struct v4l2_ctrl *flash_duration;
> > u32 vblank;
> > bool noncontinuous_clock;
> > const struct ov9282_mode *cur_mode;
> > @@ -611,6 +613,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
> > mode->vblank_max, 1, mode->vblank);
> > }
> >
> > +static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure)
> > +{
> > + /* calculate exposure time in µs */
> > + u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
> > + u32 trow_us = (frame_width * 1000000UL) / ov9282->pixel_rate->val;
>
> Redundant parentheses.
True. Will fix this. Thanks for the catch.
>
> > +
> > + return exposure * trow_us;
> > +}
> > +
> > /**
> > * ov9282_update_exp_gain() - Set updated exposure and gain
> > * @ov9282: pointer to ov9282 device
> > @@ -622,9 +633,10 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
> > static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> > {
> > int ret;
> > + u32 exposure_us = ov9282_exposure_to_us(ov9282, exposure);
> >
> > - dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
> > - exposure, gain);
> > + dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u",
> > + exposure, exposure_us, gain);
> >
> > ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
> > if (ret)
> > @@ -635,6 +647,12 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> > goto error_release_group_hold;
> >
> > ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> > + if (ret)
> > + goto error_release_group_hold;
> > +
> > + ret = __v4l2_ctrl_modify_range(ov9282->flash_duration,
> > + 0, exposure_us, 1,
> > + OV9282_FLASH_DURATION_DEFAULT);
> >
> > error_release_group_hold:
> > ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> > @@ -1420,6 +1438,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > struct v4l2_fwnode_device_properties props;
> > struct v4l2_ctrl *ctrl;
> > u32 hblank_min;
> > + u32 exposure_us;
> > u32 lpfr;
> > int ret;
> >
> > @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > /* Flash/Strobe controls */
> > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> >
> > - v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > - 0, 13900, 1, 8);
> > + exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> > + ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> > + &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > + 0, exposure_us,
> > + 1, OV9282_FLASH_DURATION_DEFAULT);
>
> Wrap this differently, please, e.g. after '='.
This is wrapped the same way as all other v4l2_ctrl_new_X() calls in
ov9282_init_controls(). Therefore I've chosen to do it this way here
too.
So if I'm going to change this one, IMHO all others should be changed
too (exp_ctrl, again_ctrl, vblank_ctrl, pixel_rate, link_freq_ctrl,
hblank_ctrl). Is this intended?
If so I'm wondering if this would be a suiteable approach?
ov9282->flash_duration =
v4l2_ctrl_new_std(ctrl_hdlr,
&ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
0, exposure_us,
1, OV9282_FLASH_DURATION_DEFAULT);
It is fine for checkpatch, but introduces a newline for every ctrl and
tbh I'm not sure if it improves readability?
>
> >
> > ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> > V4L2_CID_FLASH_STROBE_SOURCE,
> >
>
> To me the set looks good but I wouldn't mind about having a bit more
> review.
Thanks for your continuous feedback! It improved the series a lot!
Is there anyhthing I can assists/help?
>
> --
> Kind regards,
>
> Sakari Ailus
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum
2025-09-03 7:13 ` Richard Leitner
@ 2025-09-03 7:48 ` Sakari Ailus
2025-09-03 8:24 ` Richard Leitner
0 siblings, 1 reply; 45+ messages in thread
From: Sakari Ailus @ 2025-09-03 7:48 UTC (permalink / raw)
To: Richard Leitner
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
Laurent Pinchart, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Richard,
On Wed, Sep 03, 2025 at 09:13:35AM +0200, Richard Leitner wrote:
> > > @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > /* Flash/Strobe controls */
> > > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > >
> > > - v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > - 0, 13900, 1, 8);
> > > + exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> > > + ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> > > + &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > + 0, exposure_us,
> > > + 1, OV9282_FLASH_DURATION_DEFAULT);
> >
> > Wrap this differently, please, e.g. after '='.
>
> This is wrapped the same way as all other v4l2_ctrl_new_X() calls in
> ov9282_init_controls(). Therefore I've chosen to do it this way here
> too.
>
> So if I'm going to change this one, IMHO all others should be changed
> too (exp_ctrl, again_ctrl, vblank_ctrl, pixel_rate, link_freq_ctrl,
> hblank_ctrl). Is this intended?
>
> If so I'm wondering if this would be a suiteable approach?
>
> ov9282->flash_duration =
> v4l2_ctrl_new_std(ctrl_hdlr,
> &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> 0, exposure_us,
> 1, OV9282_FLASH_DURATION_DEFAULT);
>
> It is fine for checkpatch, but introduces a newline for every ctrl and
> tbh I'm not sure if it improves readability?
I don't think it's worse at least. You can also rewrap the rest of the
lines:
ov9282->flash_duration =
v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
V4L2_CID_FLASH_DURATION, 0, exposure_us, 1,
OV9282_FLASH_DURATION_DEFAULT);
> > > ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> > > V4L2_CID_FLASH_STROBE_SOURCE,
> > >
> >
> > To me the set looks good but I wouldn't mind about having a bit more
> > review.
>
> Thanks for your continuous feedback! It improved the series a lot!
>
> Is there anyhthing I can assists/help?
I asked Laurent if he could check this out, it'd be nice to get these to
6.18.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum
2025-09-03 7:48 ` Sakari Ailus
@ 2025-09-03 8:24 ` Richard Leitner
2025-09-03 8:55 ` Sakari Ailus
0 siblings, 1 reply; 45+ messages in thread
From: Richard Leitner @ 2025-09-03 8:24 UTC (permalink / raw)
To: Sakari Ailus
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
Laurent Pinchart, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Sakari,
On Wed, Sep 03, 2025 at 10:48:48AM +0300, Sakari Ailus wrote:
> Hi Richard,
>
> On Wed, Sep 03, 2025 at 09:13:35AM +0200, Richard Leitner wrote:
> > > > @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > /* Flash/Strobe controls */
> > > > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > > >
> > > > - v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > - 0, 13900, 1, 8);
> > > > + exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> > > > + ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > + &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > + 0, exposure_us,
> > > > + 1, OV9282_FLASH_DURATION_DEFAULT);
> > >
> > > Wrap this differently, please, e.g. after '='.
> >
> > This is wrapped the same way as all other v4l2_ctrl_new_X() calls in
> > ov9282_init_controls(). Therefore I've chosen to do it this way here
> > too.
> >
> > So if I'm going to change this one, IMHO all others should be changed
> > too (exp_ctrl, again_ctrl, vblank_ctrl, pixel_rate, link_freq_ctrl,
> > hblank_ctrl). Is this intended?
> >
> > If so I'm wondering if this would be a suiteable approach?
> >
> > ov9282->flash_duration =
> > v4l2_ctrl_new_std(ctrl_hdlr,
> > &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > 0, exposure_us,
> > 1, OV9282_FLASH_DURATION_DEFAULT);
> >
> > It is fine for checkpatch, but introduces a newline for every ctrl and
> > tbh I'm not sure if it improves readability?
>
> I don't think it's worse at least. You can also rewrap the rest of the
> lines:
>
> ov9282->flash_duration =
> v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> V4L2_CID_FLASH_DURATION, 0, exposure_us, 1,
> OV9282_FLASH_DURATION_DEFAULT);
>
Ok. Fine with me ;)
So I will adapt this patch and add a new patch which changes the wrapping
for all remaining v4l2_ctrl_new_X() calls in ov9282_init_controls() to the
series and send a v8? Or should I wait for feedback from Laurent for v8?
> > > > ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> > > > V4L2_CID_FLASH_STROBE_SOURCE,
> > > >
> > >
> > > To me the set looks good but I wouldn't mind about having a bit more
> > > review.
> >
> > Thanks for your continuous feedback! It improved the series a lot!
> >
> > Is there anyhthing I can assists/help?
>
> I asked Laurent if he could check this out, it'd be nice to get these to
> 6.18.
Nice. Thanks! Yeah, that would be nice, indeed :)
>
> --
> Kind regards,
>
> Sakari Ailus
regards;rl
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum
2025-09-03 8:24 ` Richard Leitner
@ 2025-09-03 8:55 ` Sakari Ailus
0 siblings, 0 replies; 45+ messages in thread
From: Sakari Ailus @ 2025-09-03 8:55 UTC (permalink / raw)
To: Richard Leitner
Cc: Dave Stevenson, Mauro Carvalho Chehab, Lee Jones, Pavel Machek,
Laurent Pinchart, linux-media, linux-kernel, linux-leds,
Hans Verkuil
Hi Richard,
On Wed, Sep 03, 2025 at 10:24:46AM +0200, Richard Leitner wrote:
> Hi Sakari,
>
> On Wed, Sep 03, 2025 at 10:48:48AM +0300, Sakari Ailus wrote:
> > Hi Richard,
> >
> > On Wed, Sep 03, 2025 at 09:13:35AM +0200, Richard Leitner wrote:
> > > > > @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > > /* Flash/Strobe controls */
> > > > > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > > > >
> > > > > - v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > > - 0, 13900, 1, 8);
> > > > > + exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> > > > > + ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > + &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > > + 0, exposure_us,
> > > > > + 1, OV9282_FLASH_DURATION_DEFAULT);
> > > >
> > > > Wrap this differently, please, e.g. after '='.
> > >
> > > This is wrapped the same way as all other v4l2_ctrl_new_X() calls in
> > > ov9282_init_controls(). Therefore I've chosen to do it this way here
> > > too.
> > >
> > > So if I'm going to change this one, IMHO all others should be changed
> > > too (exp_ctrl, again_ctrl, vblank_ctrl, pixel_rate, link_freq_ctrl,
> > > hblank_ctrl). Is this intended?
> > >
> > > If so I'm wondering if this would be a suiteable approach?
> > >
> > > ov9282->flash_duration =
> > > v4l2_ctrl_new_std(ctrl_hdlr,
> > > &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > 0, exposure_us,
> > > 1, OV9282_FLASH_DURATION_DEFAULT);
> > >
> > > It is fine for checkpatch, but introduces a newline for every ctrl and
> > > tbh I'm not sure if it improves readability?
> >
> > I don't think it's worse at least. You can also rewrap the rest of the
> > lines:
> >
> > ov9282->flash_duration =
> > v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> > V4L2_CID_FLASH_DURATION, 0, exposure_us, 1,
> > OV9282_FLASH_DURATION_DEFAULT);
> >
>
> Ok. Fine with me ;)
>
> So I will adapt this patch and add a new patch which changes the wrapping
> for all remaining v4l2_ctrl_new_X() calls in ov9282_init_controls() to the
> series and send a v8? Or should I wait for feedback from Laurent for v8?
Let's wait for Laurent to review this first. The changes I asked for are
minor.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 45+ messages in thread