| Message ID | 20230120114524.408368-8-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | leds: lookup-table support + int3472/media privacy LED support | expand |
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@linux.intel.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > 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
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@linux.intel.com> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> 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 >
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) { +#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