Re: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Fri, Jan 27, 2023 at 09:37:25PM +0100, Hans de Goede wrote:
> Make v4l2_async_register_subdev_sensor() try to get a privacy LED
> associated with the sensor and extend the call_s_stream() wrapper to
> enable/disable the privacy LED if found.
> 
> This makes the core handle privacy LED control, rather then having to
> duplicate this code in all the sensor drivers.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Please wrap the lines over 80, unless there are tangible reasons to keep
them as-is.

For this patch:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

On my behalf it can be merged via another tree, I don't expect conflicts.
Also cc Hans Verkuil.

And the rest:

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Please also see my comment on the 3rd patch.

> ---
> Changes in v6:
> - Add v4l2_subdev_privacy_led_get()/_put() helpers
> - At least the _put helper is necessary for cleanup on errors later on in
>   v4l2_async_register_subdev_sensor()
> - This puts all the LED related coded into a single file (v4l2-subdev.c)
>   removing the need to build the async + fwnode code into videodev.ko,
>   so that patch is dropped
> - Move the (non-error-exit) cleanup from v4l2_subdev_cleanup() to
>    v4l2_async_unregister_subdev()
> 
> Changes in v4 (requested by Laurent Pinchart):
> - Move the led_get() call to v4l2_async_register_subdev_sensor() and
>   make errors other then -ENOENT fail the register() call.
> - Move the led_disable_sysfs() call to be done at led_get() time, instead
>   of only disabling the sysfs interface when the sensor is streaming.
> ---
>  drivers/media/v4l2-core/v4l2-async.c       |  4 ++
>  drivers/media/v4l2-core/v4l2-fwnode.c      |  7 ++++
>  drivers/media/v4l2-core/v4l2-subdev-priv.h | 14 +++++++
>  drivers/media/v4l2-core/v4l2-subdev.c      | 44 ++++++++++++++++++++++
>  include/media/v4l2-subdev.h                |  3 ++
>  5 files changed, 72 insertions(+)
>  create mode 100644 drivers/media/v4l2-core/v4l2-subdev-priv.h
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 2f1b718a9189..d7e9ffc7aa23 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -24,6 +24,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "v4l2-subdev-priv.h"
> +
>  static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
>  				    struct v4l2_subdev *subdev,
>  				    struct v4l2_async_subdev *asd)
> @@ -822,6 +824,8 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  	if (!sd->async_list.next)
>  		return;
>  
> +	v4l2_subdev_put_privacy_led(sd);
> +
>  	mutex_lock(&list_lock);
>  
>  	__v4l2_async_nf_unregister(sd->subdev_notifier);
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3d9533c1b202..049c2f2001ea 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -28,6 +28,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "v4l2-subdev-priv.h"
> +
>  static const struct v4l2_fwnode_bus_conv {
>  	enum v4l2_fwnode_bus_type fwnode_bus_type;
>  	enum v4l2_mbus_type mbus_type;
> @@ -1302,6 +1304,10 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  
>  	v4l2_async_nf_init(notifier);
>  
> +	ret = v4l2_subdev_get_privacy_led(sd);
> +	if (ret < 0)
> +		goto out_cleanup;
> +
>  	ret = v4l2_async_nf_parse_fwnode_sensor(sd->dev, notifier);
>  	if (ret < 0)
>  		goto out_cleanup;
> @@ -1322,6 +1328,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  	v4l2_async_nf_unregister(notifier);
>  
>  out_cleanup:
> +	v4l2_subdev_put_privacy_led(sd);
>  	v4l2_async_nf_cleanup(notifier);
>  	kfree(notifier);
>  
> diff --git a/drivers/media/v4l2-core/v4l2-subdev-priv.h b/drivers/media/v4l2-core/v4l2-subdev-priv.h
> new file mode 100644
> index 000000000000..52391d6d8ab7
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-subdev-priv.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * V4L2 sub-device pivate header.
> + *
> + * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
> + */
> +
> +#ifndef _V4L2_SUBDEV_PRIV_H_
> +#define _V4L2_SUBDEV_PRIV_H_
> +
> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd);
> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd);
> +
> +#endif
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4988a25bd8f4..9fd183628285 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/ioctl.h>
> +#include <linux/leds.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> @@ -23,6 +24,8 @@
>  #include <media/v4l2-fh.h>
>  #include <media/v4l2-event.h>
>  
> +#include "v4l2-subdev-priv.h"
> +
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>  static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>  {
> @@ -322,6 +325,14 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	int ret;
>  
> +#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) {
> @@ -1090,6 +1101,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;
> @@ -1105,3 +1117,35 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
> +
> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
> +{
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	sd->privacy_led = led_get(sd->dev, "privacy-led");
> +	if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
> +		return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
> +
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		mutex_lock(&sd->privacy_led->led_access);
> +		led_sysfs_disable(sd->privacy_led);
> +		led_trigger_remove(sd->privacy_led);
> +		led_set_brightness(sd->privacy_led, 0);
> +		mutex_unlock(&sd->privacy_led->led_access);
> +	}
> +#endif
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
> +
> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd)
> +{
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		mutex_lock(&sd->privacy_led->led_access);
> +		led_sysfs_enable(sd->privacy_led);
> +		mutex_unlock(&sd->privacy_led->led_access);
> +		led_put(sd->privacy_led);
> +	}
> +#endif
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_put_privacy_led);
> 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

-- 
Kind regards,

Sakari Ailus