* Re: [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails
2024-04-10 11:47 [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails Sakari Ailus
@ 2024-04-10 11:48 ` Hans de Goede
2024-04-11 5:44 ` Umang Jain
2024-04-10 11:53 ` Tomi Valkeinen
2024-04-12 17:46 ` Laurent Pinchart
2 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2024-04-10 11:48 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: tomi.valkeinen, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Umang Jain, linux-kernel
Hi,
On 4/10/24 1:47 PM, Sakari Ailus wrote:
> Turn on the privacy LED only if streamon succeeds. This can be done after
> enabling streaming on the sensor.
>
> Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..012b757eac9f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> if (WARN_ON(!!sd->enabled_streams == !!enable))
> return 0;
>
> -#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> - if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> - if (enable)
> - led_set_brightness(sd->privacy_led,
> - sd->privacy_led->max_brightness);
> - else
> - led_set_brightness(sd->privacy_led, 0);
> - }
> -#endif
> ret = sd->ops->video->s_stream(sd, enable);
>
> if (!enable && ret < 0) {
> @@ -428,9 +419,20 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> ret = 0;
> }
>
> - if (!ret)
> + if (!ret) {
> sd->enabled_streams = enable ? BIT(0) : 0;
>
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> + if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> + if (enable)
> + led_set_brightness(sd->privacy_led,
> + sd->privacy_led->max_brightness);
> + else
> + led_set_brightness(sd->privacy_led, 0);
> + }
> +#endif
> + }
> +
> return ret;
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails
2024-04-10 11:48 ` Hans de Goede
@ 2024-04-11 5:44 ` Umang Jain
0 siblings, 0 replies; 9+ messages in thread
From: Umang Jain @ 2024-04-11 5:44 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: tomi.valkeinen, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, linux-kernel, Hans de Goede
Hi Sakari,
Thank you for the patch
On 10/04/24 5:18 pm, Hans de Goede wrote:
> Hi,
>
> On 4/10/24 1:47 PM, Sakari Ailus wrote:
>> Turn on the privacy LED only if streamon succeeds. This can be done after
>> enabling streaming on the sensor.
>>
>> Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present")
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Thanks, patch looks good to me:
Looks good to me too
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>
> Regards,
>
> Hans
>
>
>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++----------
>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 4c6198c48dd6..012b757eac9f 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>> if (WARN_ON(!!sd->enabled_streams == !!enable))
>> return 0;
>>
>> -#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> - if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> - if (enable)
>> - led_set_brightness(sd->privacy_led,
>> - sd->privacy_led->max_brightness);
>> - else
>> - led_set_brightness(sd->privacy_led, 0);
>> - }
>> -#endif
>> ret = sd->ops->video->s_stream(sd, enable);
>>
>> if (!enable && ret < 0) {
>> @@ -428,9 +419,20 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>> ret = 0;
>> }
>>
>> - if (!ret)
>> + if (!ret) {
>> sd->enabled_streams = enable ? BIT(0) : 0;
>>
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> + if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> + if (enable)
>> + led_set_brightness(sd->privacy_led,
>> + sd->privacy_led->max_brightness);
>> + else
>> + led_set_brightness(sd->privacy_led, 0);
>> + }
>> +#endif
>> + }
>> +
>> return ret;
>> }
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails
2024-04-10 11:47 [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails Sakari Ailus
2024-04-10 11:48 ` Hans de Goede
@ 2024-04-10 11:53 ` Tomi Valkeinen
2024-04-12 17:46 ` Laurent Pinchart
2 siblings, 0 replies; 9+ messages in thread
From: Tomi Valkeinen @ 2024-04-10 11:53 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: Hans de Goede, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Umang Jain, linux-kernel
On 10/04/2024 14:47, Sakari Ailus wrote:
> Turn on the privacy LED only if streamon succeeds. This can be done after
> enabling streaming on the sensor.
>
> Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..012b757eac9f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> if (WARN_ON(!!sd->enabled_streams == !!enable))
> return 0;
>
> -#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> - if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> - if (enable)
> - led_set_brightness(sd->privacy_led,
> - sd->privacy_led->max_brightness);
> - else
> - led_set_brightness(sd->privacy_led, 0);
> - }
> -#endif
> ret = sd->ops->video->s_stream(sd, enable);
>
> if (!enable && ret < 0) {
> @@ -428,9 +419,20 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> ret = 0;
> }
>
> - if (!ret)
> + if (!ret) {
> sd->enabled_streams = enable ? BIT(0) : 0;
>
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> + if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> + if (enable)
> + led_set_brightness(sd->privacy_led,
> + sd->privacy_led->max_brightness);
> + else
> + led_set_brightness(sd->privacy_led, 0);
> + }
> +#endif
> + }
> +
> return ret;
> }
>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tomi
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails
2024-04-10 11:47 [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails Sakari Ailus
2024-04-10 11:48 ` Hans de Goede
2024-04-10 11:53 ` Tomi Valkeinen
@ 2024-04-12 17:46 ` Laurent Pinchart
2024-04-15 7:15 ` Sakari Ailus
2 siblings, 1 reply; 9+ messages in thread
From: Laurent Pinchart @ 2024-04-12 17:46 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, tomi.valkeinen, Hans de Goede,
Mauro Carvalho Chehab, Hans Verkuil, Umang Jain, linux-kernel
Hi Sakari,
Thank you for the patch.
On Wed, Apr 10, 2024 at 02:47:12PM +0300, Sakari Ailus wrote:
> Turn on the privacy LED only if streamon succeeds. This can be done after
> enabling streaming on the sensor.
>
> Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..012b757eac9f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> if (WARN_ON(!!sd->enabled_streams == !!enable))
> return 0;
>
> -#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> - if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> - if (enable)
> - led_set_brightness(sd->privacy_led,
> - sd->privacy_led->max_brightness);
> - else
> - led_set_brightness(sd->privacy_led, 0);
> - }
> -#endif
> ret = sd->ops->video->s_stream(sd, enable);
>
> if (!enable && ret < 0) {
> @@ -428,9 +419,20 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> ret = 0;
> }
>
> - if (!ret)
> + if (!ret) {
> sd->enabled_streams = enable ? BIT(0) : 0;
>
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> + if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> + if (enable)
> + led_set_brightness(sd->privacy_led,
> + sd->privacy_led->max_brightness);
> + else
> + led_set_brightness(sd->privacy_led, 0);
> + }
> +#endif
This means that the LED will be turned slightly after the camera is
enabled. I don't think it's an issue in practice. Another possibly more
important concern is that we should maybe check the return value of
led_set_brightness(), and fail .s_stream() when we can't enable the
privacy LED at stream on time. In that case, it would be best to keep
turning the privacy LED on before calling .s_stream(). It should still
be turned off only after calling .s_stream() though.
> + }
> +
> return ret;
> }
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails
2024-04-12 17:46 ` Laurent Pinchart
@ 2024-04-15 7:15 ` Sakari Ailus
2024-04-15 7:18 ` Laurent Pinchart
0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2024-04-15 7:15 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, tomi.valkeinen, Hans de Goede,
Mauro Carvalho Chehab, Hans Verkuil, Umang Jain, linux-kernel
Hi Laurent,
On Fri, Apr 12, 2024 at 08:46:21PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Wed, Apr 10, 2024 at 02:47:12PM +0300, Sakari Ailus wrote:
> > Turn on the privacy LED only if streamon succeeds. This can be done after
> > enabling streaming on the sensor.
> >
> > Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present")
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++----------
> > 1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 4c6198c48dd6..012b757eac9f 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> > if (WARN_ON(!!sd->enabled_streams == !!enable))
> > return 0;
> >
> > -#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> > - if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> > - if (enable)
> > - led_set_brightness(sd->privacy_led,
> > - sd->privacy_led->max_brightness);
> > - else
> > - led_set_brightness(sd->privacy_led, 0);
> > - }
> > -#endif
> > ret = sd->ops->video->s_stream(sd, enable);
> >
> > if (!enable && ret < 0) {
> > @@ -428,9 +419,20 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> > ret = 0;
> > }
> >
> > - if (!ret)
> > + if (!ret) {
> > sd->enabled_streams = enable ? BIT(0) : 0;
> >
> > +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> > + if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> > + if (enable)
> > + led_set_brightness(sd->privacy_led,
> > + sd->privacy_led->max_brightness);
> > + else
> > + led_set_brightness(sd->privacy_led, 0);
> > + }
> > +#endif
>
> This means that the LED will be turned slightly after the camera is
> enabled. I don't think it's an issue in practice. Another possibly more
That's what I'd think as well. Typically even the exposure time is much,
much longer than what it takes to get here.
> important concern is that we should maybe check the return value of
> led_set_brightness(), and fail .s_stream() when we can't enable the
> privacy LED at stream on time. In that case, it would be best to keep
> turning the privacy LED on before calling .s_stream(). It should still
> be turned off only after calling .s_stream() though.
The return type of led_set_brightness() is void. Maybe because a large
majority is GPIO-controlled?
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails
2024-04-15 7:15 ` Sakari Ailus
@ 2024-04-15 7:18 ` Laurent Pinchart
2024-04-15 7:29 ` Sakari Ailus
0 siblings, 1 reply; 9+ messages in thread
From: Laurent Pinchart @ 2024-04-15 7:18 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, tomi.valkeinen, Hans de Goede,
Mauro Carvalho Chehab, Hans Verkuil, Umang Jain, linux-kernel
On Mon, Apr 15, 2024 at 07:15:42AM +0000, Sakari Ailus wrote:
> Hi Laurent,
>
> On Fri, Apr 12, 2024 at 08:46:21PM +0300, Laurent Pinchart wrote:
> > Hi Sakari,
> >
> > Thank you for the patch.
> >
> > On Wed, Apr 10, 2024 at 02:47:12PM +0300, Sakari Ailus wrote:
> > > Turn on the privacy LED only if streamon succeeds. This can be done after
> > > enabling streaming on the sensor.
> > >
> > > Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present")
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++----------
> > > 1 file changed, 12 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 4c6198c48dd6..012b757eac9f 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> > > if (WARN_ON(!!sd->enabled_streams == !!enable))
> > > return 0;
> > >
> > > -#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> > > - if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> > > - if (enable)
> > > - led_set_brightness(sd->privacy_led,
> > > - sd->privacy_led->max_brightness);
> > > - else
> > > - led_set_brightness(sd->privacy_led, 0);
> > > - }
> > > -#endif
> > > ret = sd->ops->video->s_stream(sd, enable);
> > >
> > > if (!enable && ret < 0) {
> > > @@ -428,9 +419,20 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> > > ret = 0;
> > > }
> > >
> > > - if (!ret)
> > > + if (!ret) {
> > > sd->enabled_streams = enable ? BIT(0) : 0;
> > >
> > > +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> > > + if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> > > + if (enable)
> > > + led_set_brightness(sd->privacy_led,
> > > + sd->privacy_led->max_brightness);
> > > + else
> > > + led_set_brightness(sd->privacy_led, 0);
> > > + }
> > > +#endif
> >
> > This means that the LED will be turned slightly after the camera is
> > enabled. I don't think it's an issue in practice. Another possibly more
>
> That's what I'd think as well. Typically even the exposure time is much,
> much longer than what it takes to get here.
>
> > important concern is that we should maybe check the return value of
> > led_set_brightness(), and fail .s_stream() when we can't enable the
> > privacy LED at stream on time. In that case, it would be best to keep
> > turning the privacy LED on before calling .s_stream(). It should still
> > be turned off only after calling .s_stream() though.
>
> The return type of led_set_brightness() is void.
Oops :-S
> Maybe because a large majority is GPIO-controlled?
GPIOs can fail, in particular when they're on I2C GPIO expanders.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails
2024-04-15 7:18 ` Laurent Pinchart
@ 2024-04-15 7:29 ` Sakari Ailus
2024-04-15 7:40 ` Laurent Pinchart
0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2024-04-15 7:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, tomi.valkeinen, Hans de Goede,
Mauro Carvalho Chehab, Hans Verkuil, Umang Jain, linux-kernel
On Mon, Apr 15, 2024 at 10:18:12AM +0300, Laurent Pinchart wrote:
> > Maybe because a large majority is GPIO-controlled?
>
> GPIOs can fail, in particular when they're on I2C GPIO expanders.
Sure, but gpiod_set_value() return type is also void.
It just works... right?
--
Sakari Ailus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/1] media: v4l: Don't turn on privacy LED if streamon fails
2024-04-15 7:29 ` Sakari Ailus
@ 2024-04-15 7:40 ` Laurent Pinchart
0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2024-04-15 7:40 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, tomi.valkeinen, Hans de Goede,
Mauro Carvalho Chehab, Hans Verkuil, Umang Jain, linux-kernel
On Mon, Apr 15, 2024 at 07:29:41AM +0000, Sakari Ailus wrote:
> On Mon, Apr 15, 2024 at 10:18:12AM +0300, Laurent Pinchart wrote:
> > > Maybe because a large majority is GPIO-controlled?
> >
> > GPIOs can fail, in particular when they're on I2C GPIO expanders.
>
> Sure, but gpiod_set_value() return type is also void.
I know, so there's nothing we can do short term.
> It just works... right?
Until it doesn't :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread