Re: [PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>]

On Fri, Dec 16, 2022 at 03:56:33PM +0200, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> >  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.
> 
> > +
> > +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.
> 
> > +
> > +	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.

Also, I think it would be better if the LED device was marked as "no
sysfs" when it is registered.

> > +	}
> > +
> > +	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.
> 
> > @@ -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

-- 
Regards,

Laurent Pinchart