Hi Sakari,
On 1/20/23 13:51, Sakari Ailus wrote:
> 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?
Good point.
Looking into this also made me realize that I forgot to cleanup
the LED reference (and re-enable sysfs control) in case of
errors in v4l2_async_register_subdev_sensor() after getting it.
Fixing this requires adding a v4l2_subdev_put_privacy_led()
helper. At which point it makes sense to also put the code
to get the led in a v4l2_subdev_get_privacy_led() helper
and then all privacy-led code lives inside a v4l2-subdev.c
removing the need for:
[PATCH v5 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko
all together :) So I'll drop that from v6 of this series to
make the series simpler.
Regards,
Hans
>
>> {
>> +#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
>
