Hi Hans,
On Fri, Jan 20, 2023 at 12:45:20PM +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@xxxxxxxxxxxxxxx>
> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> 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-fwnode.c | 15 +++++++++++++++
> drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++++++++++++++
> include/media/v4l2-subdev.h | 3 +++
> 3 files changed, 36 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index c8a2264262bc..cfac1e2ae501 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -16,6 +16,7 @@
> */
> #include <linux/acpi.h>
> #include <linux/kernel.h>
> +#include <linux/leds.h>
> #include <linux/mm.h>
> #include <linux/of.h>
> #include <linux/property.h>
> @@ -1295,6 +1296,20 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
> if (WARN_ON(!sd->dev))
> return -ENODEV;
>
> +#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
> +
> notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
> if (!notifier)
> return -ENOMEM;
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4988a25bd8f4..f33e943aab3f 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>
> @@ -322,6 +323,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) {
> @@ -1050,6 +1059,14 @@ EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
>
> void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
v4l2_subdev_cleanup() is currently called by drivers using V4L2 subdev
state at the moment, making it unsuitable for the purpose of releasing the
privacy led.
Could you move this to v4l2_async_unregister_subdev() instead?
> {
> +#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
> __v4l2_subdev_state_free(sd->active_state);
> sd->active_state = NULL;
> }
> @@ -1090,6 +1107,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
--
Kind regards,
Sakari Ailus
