| Message ID | 20240405-enable-streams-impro-v2-1-22bca967665d@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/9] media: subdev: Add privacy led helpers | expand |
Moi, On Fri, Apr 05, 2024 at 12:14:19PM +0300, Tomi Valkeinen wrote: > Add helper functions to enable and disable the privacy led. This moves > the #if from the call site to the privacy led functions, and makes > adding privacy led support to v4l2_subdev_enable/disable_streams() > cleaner. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 31 ++++++++++++++++++++++--------- > 1 file changed, 22 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 4c6198c48dd6..942c7a644033 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -148,6 +148,23 @@ static int subdev_close(struct file *file) > } > #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */ > > +static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd) > +{ > +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > + if (!IS_ERR_OR_NULL(sd->privacy_led)) > + led_set_brightness(sd->privacy_led, > + sd->privacy_led->max_brightness); > +#endif > +} > + > +static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd) > +{ > +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > + if (!IS_ERR_OR_NULL(sd->privacy_led)) > + led_set_brightness(sd->privacy_led, 0); > +#endif > +} > + > static inline int check_which(u32 which) > { > if (which != V4L2_SUBDEV_FORMAT_TRY && > @@ -412,15 +429,11 @@ 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 > + if (enable) > + v4l2_subdev_enable_privacy_led(sd); > + else > + v4l2_subdev_disable_privacy_led(sd); > + > ret = sd->ops->video->s_stream(sd, enable); I see that you're only making changes before the s_stream call which also reveals a bug here: if streaming on fails, the LED will remain lit. It should be fixed before this set. I cc'd Hans de Goede. > > if (!enable && ret < 0) { >
Hi, On 4/10/24 11:07 AM, Sakari Ailus wrote: > Moi, > > On Fri, Apr 05, 2024 at 12:14:19PM +0300, Tomi Valkeinen wrote: >> Add helper functions to enable and disable the privacy led. This moves >> the #if from the call site to the privacy led functions, and makes >> adding privacy led support to v4l2_subdev_enable/disable_streams() >> cleaner. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 31 ++++++++++++++++++++++--------- >> 1 file changed, 22 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 4c6198c48dd6..942c7a644033 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -148,6 +148,23 @@ static int subdev_close(struct file *file) >> } >> #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */ >> >> +static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd) >> +{ >> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) >> + if (!IS_ERR_OR_NULL(sd->privacy_led)) >> + led_set_brightness(sd->privacy_led, >> + sd->privacy_led->max_brightness); >> +#endif >> +} >> + >> +static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd) >> +{ >> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) >> + if (!IS_ERR_OR_NULL(sd->privacy_led)) >> + led_set_brightness(sd->privacy_led, 0); >> +#endif >> +} >> + >> static inline int check_which(u32 which) >> { >> if (which != V4L2_SUBDEV_FORMAT_TRY && >> @@ -412,15 +429,11 @@ 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 >> + if (enable) >> + v4l2_subdev_enable_privacy_led(sd); >> + else >> + v4l2_subdev_disable_privacy_led(sd); >> + >> ret = sd->ops->video->s_stream(sd, enable); > > I see that you're only making changes before the s_stream call which also > reveals a bug here: if streaming on fails, the LED will remain lit. It > should be fixed before this set. > > I cc'd Hans de Goede. Right that seems like a bug which I introduced. I think it would be best to move the setting of the LED on/off to after the s_stream() call in the: if (!ret) sd->enabled_streams = enable ? BIT(0) : 0; block, so that the LED is not touched when s_stream(sd, 1) fails. This delay enabling the LED slightly on stream start but IMHO that is not a bit issue. Regards, Hans
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 4c6198c48dd6..942c7a644033 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -148,6 +148,23 @@ static int subdev_close(struct file *file) } #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */ +static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd) +{ +#if IS_REACHABLE(CONFIG_LEDS_CLASS) + if (!IS_ERR_OR_NULL(sd->privacy_led)) + led_set_brightness(sd->privacy_led, + sd->privacy_led->max_brightness); +#endif +} + +static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd) +{ +#if IS_REACHABLE(CONFIG_LEDS_CLASS) + if (!IS_ERR_OR_NULL(sd->privacy_led)) + led_set_brightness(sd->privacy_led, 0); +#endif +} + static inline int check_which(u32 which) { if (which != V4L2_SUBDEV_FORMAT_TRY && @@ -412,15 +429,11 @@ 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 + if (enable) + v4l2_subdev_enable_privacy_led(sd); + else + v4l2_subdev_disable_privacy_led(sd); + ret = sd->ops->video->s_stream(sd, enable); if (!enable && ret < 0) {
Add helper functions to enable and disable the privacy led. This moves the #if from the call site to the privacy led functions, and makes adding privacy led support to v4l2_subdev_enable/disable_streams() cleaner. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)