Hi, On 1/30/23 11:17, Sakari Ailus wrote: > 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@xxxxxxxxxxxxxxx> >> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx> > > On my behalf it can be merged via another tree, I don't expect conflicts. > Also cc Hans Verkuil. Thanks. I've merged the entire series into my pdx86/review-hans branch now: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans (with the lines in this patch shortened to 80 chars). Once the builders have had some time to play with this branch (and once I've run some tests to make sure the patches still work as expected) I will push to platform-drivers-x86/for-next . Regards, Hans > > And the rest: > > Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxx> >> + */ >> + >> +#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 >
