Message ID | 20230120114524.408368-1-hdegoede@redhat.com |
---|---|
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 >
On Fri, 20 Jan 2023, Hans de Goede wrote: > Hi All, > > Here is version 5 of my series to adjust the INT3472 code's handling of > the privacy LED on x86 laptops with MIPI camera(s) so that it will also > work on devices which have a privacy-LED GPIO but not a clk-enable GPIO > (so that we cannot just tie the LED state to the clk-enable state). > > Changes in v5: > - Rename lookup-table names to match those from the gpio and reset lookups: > s/led_name/provider/ > s/consumer_dev_name/dev_id/ > s/consumer_function/con_id/ > - Add static inline wrappers for the v4l2_async debugfs init/exit funcs, > to fix build errors when CONFIG_V4L2_ASYNC is not enabled > > Changes in v4: > - Rename new __led_get() helper to led_module_get() > - Drop of/devicetree support from "led-class: Add generic [devm_]led_get()" > - Add RFC patch to re-add of/devicetree support to show that the new > led_get() can easily be extended with dt support when the need for this > arises (proof-of-concept dt code, not intended for merging) > - New patch to built async and fwnode code into videodev.ko, > to avoid issues with some of the new LED code getting builtin vs > other parts possibly being in a module > - Move the led_get() call to v4l2_async_register_subdev_sensor() > - Move the led_disable_sysfs() call to be done at led_get() time > - Address some other minor review comments > > Changes in v3: > - Due to popular request by multiple people this new version now models > the privacy LED as a LED class device. This requires being able to > "tie" the LED class device to a specific camera sensor (some devices > have multiple sensors + privacy-LEDs). > > Patches 1-5 are LED subsystem patches for this. 1 is a bug fix, 2-4 add > the new [devm_]led_get() functions. Patch 5 is the RFC patch adding dt > support to led_get() and is not intended for merging. > > Patch 6 + 7 add generic privacy-LED support to the v4l2-core/v4l2-subdev.c > code automatically enabling the privacy-LED when s_stream(subdev, 1) > is called. So that we don't need to add privacy-LED code to all the > camera sensor drivers separately (as requested by Sakari). > > Patches 8-11 are patches to the platform specific INT3472 code to register > privacy-LED class devices + lookup table entries for privacy-LEDs described > in the special INT3472 ACPI nodes found on x86 devices with MIPI cameras. > > Assuming at least the LED maintainers are happy with the approach suggested > here, the first step to merging this would be to merge patches 1-4 and then > provide an immutable branch with those to merge for the other subsystems > since the other changes depend on these. LEDs pull request to follow.
Hi, On 1/27/23 12:08, Lee Jones wrote: > On Fri, 20 Jan 2023, Hans de Goede wrote: > >> Hi All, >> >> Here is version 5 of my series to adjust the INT3472 code's handling of >> the privacy LED on x86 laptops with MIPI camera(s) so that it will also >> work on devices which have a privacy-LED GPIO but not a clk-enable GPIO >> (so that we cannot just tie the LED state to the clk-enable state). >> >> Changes in v5: >> - Rename lookup-table names to match those from the gpio and reset lookups: >> s/led_name/provider/ >> s/consumer_dev_name/dev_id/ >> s/consumer_function/con_id/ >> - Add static inline wrappers for the v4l2_async debugfs init/exit funcs, >> to fix build errors when CONFIG_V4L2_ASYNC is not enabled >> >> Changes in v4: >> - Rename new __led_get() helper to led_module_get() >> - Drop of/devicetree support from "led-class: Add generic [devm_]led_get()" >> - Add RFC patch to re-add of/devicetree support to show that the new >> led_get() can easily be extended with dt support when the need for this >> arises (proof-of-concept dt code, not intended for merging) >> - New patch to built async and fwnode code into videodev.ko, >> to avoid issues with some of the new LED code getting builtin vs >> other parts possibly being in a module >> - Move the led_get() call to v4l2_async_register_subdev_sensor() >> - Move the led_disable_sysfs() call to be done at led_get() time >> - Address some other minor review comments >> >> Changes in v3: >> - Due to popular request by multiple people this new version now models >> the privacy LED as a LED class device. This requires being able to >> "tie" the LED class device to a specific camera sensor (some devices >> have multiple sensors + privacy-LEDs). >> >> Patches 1-5 are LED subsystem patches for this. 1 is a bug fix, 2-4 add >> the new [devm_]led_get() functions. Patch 5 is the RFC patch adding dt >> support to led_get() and is not intended for merging. >> >> Patch 6 + 7 add generic privacy-LED support to the v4l2-core/v4l2-subdev.c >> code automatically enabling the privacy-LED when s_stream(subdev, 1) >> is called. So that we don't need to add privacy-LED code to all the >> camera sensor drivers separately (as requested by Sakari). >> >> Patches 8-11 are patches to the platform specific INT3472 code to register >> privacy-LED class devices + lookup table entries for privacy-LEDs described >> in the special INT3472 ACPI nodes found on x86 devices with MIPI cameras. >> >> Assuming at least the LED maintainers are happy with the approach suggested >> here, the first step to merging this would be to merge patches 1-4 and then >> provide an immutable branch with those to merge for the other subsystems >> since the other changes depend on these. > > LEDs pull request to follow. Great, thank you! Regards, Hans
Enjoy! The following changes since commit 1b929c02afd37871d5afb9d498426f83432e71c2: Linux 6.2-rc1 (2022-12-25 13:41:39 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git ib-leds-led_get-6.3 for you to fetch changes up to abc3100fcba6827444ef4bdb17065ac3b6619dff: leds: led-class: Add generic [devm_]led_get() (2023-01-27 11:07:11 +0000) ---------------------------------------------------------------- Hans de Goede (4): leds: led-class: Add missing put_device() to led_put() leds: led-class: Add led_module_get() helper leds: led-class: Add __devm_led_get() helper leds: led-class: Add generic [devm_]led_get() drivers/leds/led-class.c | 138 ++++++++++++++++++++++++++++++++++++++++------- include/linux/leds.h | 21 ++++++++ 2 files changed, 139 insertions(+), 20 deletions(-)