| Message ID | 20221216113013.126881-7-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: lookup-table support + int3472/media privacy LED support | expand |
On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote: > Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs > for sensors with a privacy LED, rather then having to duplicate this code > in all the sensor drivers. ... > +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) > +{ > + if (!sd->dev) > + return; > + > + /* Try to get privacy-led once, at first s_stream() */ > + if (!sd->privacy_led) > + sd->privacy_led = led_get(sd->dev, "privacy-led"); > + Redundant blank line? > + if (IS_ERR(sd->privacy_led)) > + return; I'm not sure I have got the logic right. Let's assume we call it with _led == NULL. Then in case of error, we feel it with the error pointer. If we call again, we check for NULL, and return error pointer. So, we won't try the second time. Is it by design? Or should it be struct ... *led; if (!privacy_led) { led = ... if (IS_ERR()) return; privacy_led = led; } ? > +}
Hi, On 12/16/22 15:02, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote: >> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs >> for sensors with a privacy LED, rather then having to duplicate this code >> in all the sensor drivers. > > ... > >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) >> +{ >> + if (!sd->dev) >> + return; >> + >> + /* Try to get privacy-led once, at first s_stream() */ >> + if (!sd->privacy_led) >> + sd->privacy_led = led_get(sd->dev, "privacy-led"); > >> + > > Redundant blank line? I find this more readable with the blank line between the 2 ifs. > >> + if (IS_ERR(sd->privacy_led)) >> + return; > > I'm not sure I have got the logic right. Let's assume we call it with > _led == NULL. Then in case of error, we feel it with the error pointer. > If we call again, we check for NULL, and return error pointer. > > So, we won't try the second time. Is it by design? Or should it be It is by design, there even is a comment which says so: /* Try to get privacy-led once, at first s_stream() */ Regards, Hans > > struct ... *led; > > if (!privacy_led) { > led = ... > if (IS_ERR()) > return; > privacy_led = led; > } > > ? > >> +} >
Hi Hans, On Fri, Dec 16, 2022 at 04:45:29PM +0100, Hans de Goede wrote: > On 12/16/22 14:56, Laurent Pinchart wrote: > > On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote: > >> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs > >> for sensors with a privacy LED, rather then having to duplicate this code > >> in all the sensor drivers. > >> > >> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++ > >> include/media/v4l2-subdev.h | 3 ++ > >> 2 files changed, 43 insertions(+) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >> index 4988a25bd8f4..7344f6cd58b7 100644 > >> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >> @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, > >> sd->ops->pad->get_mbus_config(sd, pad, config); > >> } > >> > >> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > >> +#include <linux/leds.h> > > > > Can this be moved to the top of the file ? It doesn't have to be > > conditionally compiled there. > > You mean just the #include right? Ack to that. Yes, that's what I meant. > >> + > >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) > >> +{ > >> + if (!sd->dev) > >> + return; > >> + > >> + /* Try to get privacy-led once, at first s_stream() */ > >> + if (!sd->privacy_led) > >> + sd->privacy_led = led_get(sd->dev, "privacy-led"); > > > > I'm not sure I like this much. If the LED provider isn't available > > (yet), the LED will stay off. That's a privacy concern. > > At first I tried to put the led_get() inside v4l2_async_register_subdev_sensor(), > which could then return an error like -EPROBE_DEFER if the led_get() > fails (and nicely limits the led_get() to sensors). > > The problem which I hit is that v4l2-fwnode.c is build according to > CONFIG_V4L2_FWNODE where as v4l2-subdev.c is build according to > CONFIG_VIDEO_DEV > > And e.g. CONFIG_VIDEO_DEV could be builtin while CONFIG_V4L2_FWNODE > could be a module and then having the #if IS_REACHABLE(CONFIG_LEDS_CLASS) > spread over the 2 could result in different answers in the different > files ... > > One solution here could be to change CONFIG_V4L2_FWNODE and V4L2_ASYNC > to bools and link the (quite small) objects for these 2 into videodev.ko: > > videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o > videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o There's a long overdue simplification of Kconfig symbols in the subsystem. Another option would be to compile both in a single module, as they're often used together. I'll let Sakari chime in, I don't have a strong preference. > >> + > >> + if (IS_ERR(sd->privacy_led)) > >> + return; > >> + > >> + mutex_lock(&sd->privacy_led->led_access); > >> + > >> + if (enable) { > >> + led_sysfs_disable(sd->privacy_led); > >> + led_trigger_remove(sd->privacy_led); > >> + led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness); > >> + } else { > >> + led_set_brightness(sd->privacy_led, 0); > >> + led_sysfs_enable(sd->privacy_led); > > > > I don't think you should reenable control through sysfs here. I don't > > really see a use case, and you've removed the trigger anyway, so the > > behaviour would be quite inconsistent. > > Hmm, I thought this was a good compromise, this way the LED > can be used for other purposes when the sensor is off if users > want to. > > Right if users want to use a trigger then they would need > to re-attach the trigger after using the camera. > > But this way at least they can use the LED for other purposes > which since many users don't use their webcam that often > might be interesting for some users ... If the privacy LED starts being used for other purposes, users may get used to seeing it on at random times, which defeats the point of the privacy LED in the first place. > And this is consistent with how flash LEDs are handled. > > > Also, I think it would be better if the LED device was marked as "no > > sysfs" when it is registered. > > If we decide to permanently disallow userspace access then > yes this is an option. Or maybe better (to keep the LED > drivers generic), do the disable directly after the led_get() ? I suppose the small race condition wouldn't be a big issue, but if we decide that the privacy LED should really not be used for user purposes, then I'd still rather disable userspace access when registering the LED. > >> + } > >> + > >> + mutex_unlock(&sd->privacy_led->led_access); > >> +} > >> +#else > >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {} > >> +#endif > >> + > >> static int call_s_stream(struct v4l2_subdev *sd, int enable) > >> { > >> int ret; > >> > >> + call_s_stream_update_pled(sd, enable); > >> + > >> ret = sd->ops->video->s_stream(sd, enable); > >> > >> if (!enable && ret < 0) { > > > > You need to turn the LED off when enabling if .s_stream() fails. > > Ack.
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 4988a25bd8f4..7344f6cd58b7 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, sd->ops->pad->get_mbus_config(sd, pad, config); } +#if IS_REACHABLE(CONFIG_LEDS_CLASS) +#include <linux/leds.h> + +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) +{ + if (!sd->dev) + return; + + /* Try to get privacy-led once, at first s_stream() */ + if (!sd->privacy_led) + sd->privacy_led = led_get(sd->dev, "privacy-led"); + + if (IS_ERR(sd->privacy_led)) + return; + + mutex_lock(&sd->privacy_led->led_access); + + if (enable) { + led_sysfs_disable(sd->privacy_led); + led_trigger_remove(sd->privacy_led); + led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness); + } else { + led_set_brightness(sd->privacy_led, 0); + led_sysfs_enable(sd->privacy_led); + } + + mutex_unlock(&sd->privacy_led->led_access); +} +#else +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {} +#endif + static int call_s_stream(struct v4l2_subdev *sd, int enable) { int ret; + call_s_stream_update_pled(sd, enable); + ret = sd->ops->video->s_stream(sd, enable); if (!enable && ret < 0) { @@ -1050,6 +1084,11 @@ EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize); void v4l2_subdev_cleanup(struct v4l2_subdev *sd) { +#if IS_REACHABLE(CONFIG_LEDS_CLASS) + if (!IS_ERR_OR_NULL(sd->privacy_led)) + led_put(sd->privacy_led); +#endif + __v4l2_subdev_state_free(sd->active_state); sd->active_state = NULL; } @@ -1090,6 +1129,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) sd->grp_id = 0; sd->dev_priv = NULL; sd->host_priv = NULL; + sd->privacy_led = NULL; #if defined(CONFIG_MEDIA_CONTROLLER) sd->entity.name = sd->name; sd->entity.obj_type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV; diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index b15fa9930f30..0547313f98cc 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -38,6 +38,7 @@ struct v4l2_subdev; struct v4l2_subdev_fh; struct tuner_setup; struct v4l2_mbus_frame_desc; +struct led_classdev; /** * struct v4l2_decode_vbi_line - used to decode_vbi_line @@ -982,6 +983,8 @@ struct v4l2_subdev { * appropriate functions. */ + struct led_classdev *privacy_led; + /* * TODO: active_state should most likely be changed from a pointer to an * embedded field. For the time being it's kept as a pointer to more
Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs for sensors with a privacy LED, rather then having to duplicate this code in all the sensor drivers. Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++ include/media/v4l2-subdev.h | 3 ++ 2 files changed, 43 insertions(+)