| Message ID | 20221216113013.126881-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | leds: lookup-table support + int3472/media privacy LED support | expand |
Hi, On 12/16/22 12:30, Hans de Goede wrote: > Hi All, > > Here is my 3th attempt at adjusting 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). > > 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 > is a bit of refactoring in preparation for patch 5 which adds > generic (non devicetree specific) led_get() and devm_led_get() function > (which will also work with devicetree) and lookup table support to > allow platform code to add LED class-device <-> consumer-dev,function > lookups for non devicetree platforms. > > Patch 6 adds 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 privacy-LED code to all the > camera sensor drivers separately (as requested by Sakari). > > These are all new patches in version 3. Patches 7-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 (+ prep work + some other INT3472 fixes). > > Assuming the LED and media maintainers are happy with the approach > suggested here (if you are please give your Ack / Reviewed-by) we > need to talk about how to merge this since patches 6 and 7-11 > depend on the LED subsystem changes. I think it would be best if > the LED subsystem can provide an immutable branch with patches 1-5 > (on top of 6.2-rc1 once it is out) and then the media folks and I > can merge that branch and then apply the other patches on top. > > This series has been tested on: > > - Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED > - Dell Latitude 9420, IPU 6, front: ov01a1s with privacy LED > - Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED > back: ov8865 with privacy LED (pled not yet supported) > > Regards, > > Hans p.s. I have matching out of tree IPU6 driver changes here: https://github.com/jwrdegoede/ipu6-drivers/commits/master once this series has landed these changes will allow using the out of tree IPU6 driver with an unmodified upstream kernel. Regards, Hans
Hi Hans, Thank you for the patch. On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote: > Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs > for sensors with a privacy LED, rather then having to duplicate this code > in all the sensor drivers. > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++ > include/media/v4l2-subdev.h | 3 ++ > 2 files changed, 43 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 4988a25bd8f4..7344f6cd58b7 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, > sd->ops->pad->get_mbus_config(sd, pad, config); > } > > +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > +#include <linux/leds.h> Can this be moved to the top of the file ? It doesn't have to be conditionally compiled there. > + > +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) > +{ > + if (!sd->dev) > + return; > + > + /* Try to get privacy-led once, at first s_stream() */ > + if (!sd->privacy_led) > + sd->privacy_led = led_get(sd->dev, "privacy-led"); I'm not sure I like this much. If the LED provider isn't available (yet), the LED will stay off. That's a privacy concern. > + > + if (IS_ERR(sd->privacy_led)) > + return; > + > + mutex_lock(&sd->privacy_led->led_access); > + > + if (enable) { > + led_sysfs_disable(sd->privacy_led); > + led_trigger_remove(sd->privacy_led); > + led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness); > + } else { > + led_set_brightness(sd->privacy_led, 0); > + led_sysfs_enable(sd->privacy_led); I don't think you should reenable control through sysfs here. I don't really see a use case, and you've removed the trigger anyway, so the behaviour would be quite inconsistent. > + } > + > + mutex_unlock(&sd->privacy_led->led_access); > +} > +#else > +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {} > +#endif > + > static int call_s_stream(struct v4l2_subdev *sd, int enable) > { > int ret; > > + call_s_stream_update_pled(sd, enable); > + > ret = sd->ops->video->s_stream(sd, enable); > > if (!enable && ret < 0) { You need to turn the LED off when enabling if .s_stream() fails. > @@ -1050,6 +1084,11 @@ 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)) > + led_put(sd->privacy_led); > +#endif > + > __v4l2_subdev_state_free(sd->active_state); > sd->active_state = NULL; > } > @@ -1090,6 +1129,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, Dec 16, 2022 at 03:56:33PM +0200, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote: > > Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs > > for sensors with a privacy LED, rather then having to duplicate this code > > in all the sensor drivers. > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++ > > include/media/v4l2-subdev.h | 3 ++ > > 2 files changed, 43 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 4988a25bd8f4..7344f6cd58b7 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, > > sd->ops->pad->get_mbus_config(sd, pad, config); > > } > > > > +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > > +#include <linux/leds.h> > > Can this be moved to the top of the file ? It doesn't have to be > conditionally compiled there. > > > + > > +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) > > +{ > > + if (!sd->dev) > > + return; > > + > > + /* Try to get privacy-led once, at first s_stream() */ > > + if (!sd->privacy_led) > > + sd->privacy_led = led_get(sd->dev, "privacy-led"); > > I'm not sure I like this much. If the LED provider isn't available > (yet), the LED will stay off. That's a privacy concern. > > > + > > + if (IS_ERR(sd->privacy_led)) > > + return; > > + > > + mutex_lock(&sd->privacy_led->led_access); > > + > > + if (enable) { > > + led_sysfs_disable(sd->privacy_led); > > + led_trigger_remove(sd->privacy_led); > > + led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness); > > + } else { > > + led_set_brightness(sd->privacy_led, 0); > > + led_sysfs_enable(sd->privacy_led); > > I don't think you should reenable control through sysfs here. I don't > really see a use case, and you've removed the trigger anyway, so the > behaviour would be quite inconsistent. Also, I think it would be better if the LED device was marked as "no sysfs" when it is registered. > > + } > > + > > + mutex_unlock(&sd->privacy_led->led_access); > > +} > > +#else > > +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {} > > +#endif > > + > > static int call_s_stream(struct v4l2_subdev *sd, int enable) > > { > > int ret; > > > > + call_s_stream_update_pled(sd, enable); > > + > > ret = sd->ops->video->s_stream(sd, enable); > > > > if (!enable && ret < 0) { > > You need to turn the LED off when enabling if .s_stream() fails. > > > @@ -1050,6 +1084,11 @@ 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)) > > + led_put(sd->privacy_led); > > +#endif > > + > > __v4l2_subdev_state_free(sd->active_state); > > sd->active_state = NULL; > > } > > @@ -1090,6 +1129,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, On 12/16/22 14:56, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote: >> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs >> for sensors with a privacy LED, rather then having to duplicate this code >> in all the sensor drivers. >> >> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++ >> include/media/v4l2-subdev.h | 3 ++ >> 2 files changed, 43 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 4988a25bd8f4..7344f6cd58b7 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, >> sd->ops->pad->get_mbus_config(sd, pad, config); >> } >> >> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) >> +#include <linux/leds.h> > > Can this be moved to the top of the file ? It doesn't have to be > conditionally compiled there. You mean just the #include right? Ack to that. > >> + >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) >> +{ >> + if (!sd->dev) >> + return; >> + >> + /* Try to get privacy-led once, at first s_stream() */ >> + if (!sd->privacy_led) >> + sd->privacy_led = led_get(sd->dev, "privacy-led"); > > I'm not sure I like this much. If the LED provider isn't available > (yet), the LED will stay off. That's a privacy concern. At first I tried to put the led_get() inside v4l2_async_register_subdev_sensor(), which could then return an error like -EPROBE_DEFER if the led_get() fails (and nicely limits the led_get() to sensors). The problem which I hit is that v4l2-fwnode.c is build according to CONFIG_V4L2_FWNODE where as v4l2-subdev.c is build according to CONFIG_VIDEO_DEV And e.g. CONFIG_VIDEO_DEV could be builtin while CONFIG_V4L2_FWNODE could be a module and then having the #if IS_REACHABLE(CONFIG_LEDS_CLASS) spread over the 2 could result in different answers in the different files ... One solution here could be to change CONFIG_V4L2_FWNODE and V4L2_ASYNC to bools and link the (quite small) objects for these 2 into videodev.ko: videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o > >> + >> + if (IS_ERR(sd->privacy_led)) >> + return; >> + >> + mutex_lock(&sd->privacy_led->led_access); >> + >> + if (enable) { >> + led_sysfs_disable(sd->privacy_led); >> + led_trigger_remove(sd->privacy_led); >> + led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness); >> + } else { >> + led_set_brightness(sd->privacy_led, 0); >> + led_sysfs_enable(sd->privacy_led); > > I don't think you should reenable control through sysfs here. I don't > really see a use case, and you've removed the trigger anyway, so the > behaviour would be quite inconsistent. Hmm, I thought this was a good compromise, this way the LED can be used for other purposes when the sensor is off if users want to. Right if users want to use a trigger then they would need to re-attach the trigger after using the camera. But this way at least they can use the LED for other purposes which since many users don't use their webcam that often might be interesting for some users ... And this is consistent with how flash LEDs are handled. > Also, I think it would be better if the LED device was marked as "no > sysfs" when it is registered. If we decide to permanently disallow userspace access then yes this is an option. Or maybe better (to keep the LED drivers generic), do the disable directly after the led_get() ? > >> + } >> + >> + mutex_unlock(&sd->privacy_led->led_access); >> +} >> +#else >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {} >> +#endif >> + >> static int call_s_stream(struct v4l2_subdev *sd, int enable) >> { >> int ret; >> >> + call_s_stream_update_pled(sd, enable); >> + >> ret = sd->ops->video->s_stream(sd, enable); >> >> if (!enable && ret < 0) { > > You need to turn the LED off when enabling if .s_stream() fails. Ack. Regards, Hans
Hi, On 12/16/22 14:50, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 12:30:05PM +0100, Hans de Goede wrote: >> Turn of_led_get() into a more generic __of_led_get() helper function, >> which can lookup LEDs in devicetree by either name or index. > > ... > >> + /* >> + * For named LEDs, first look up the name in the "leds-names" property. >> + * If it cannot be found, then of_parse_phandle() will propagate the error. >> + */ >> + if (name) >> + index = of_property_match_string(np, "leds-names", name); > > I can't find this property anywhere in the kernel. Is it new? Yes and no, adding a foo-names property for foo[] arrays to be able to get members by name is a standard template for devicetree bindings. See e.g. the clock bindings: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml > If so, where is the bindings? As for why not document this, there are currently no devicetree users and the devicetree maintainers have repeatedly let me know not to submit new bindings for fwnode x86 bindings ... > And why entire code can't be converted > to use fwnode for this case? This is a trivial change to allow the new functions to work with devicetree. Note this series does not introduce any devicetree users, hence no bindings. But it is good to have compatibility backed in from day 1. Converting to fwnode APIs would be more involved and I cannot test those changes. Regards, Hans
On Fri, Dec 16, 2022 at 04:52:33PM +0100, Hans de Goede wrote: > On 12/16/22 14:50, Andy Shevchenko wrote: > > On Fri, Dec 16, 2022 at 12:30:05PM +0100, Hans de Goede wrote: ... > >> + /* > >> + * For named LEDs, first look up the name in the "leds-names" property. > >> + * If it cannot be found, then of_parse_phandle() will propagate the error. > >> + */ > >> + if (name) > >> + index = of_property_match_string(np, "leds-names", name); > > > > I can't find this property anywhere in the kernel. Is it new? > > Yes and no, adding a foo-names property for foo[] arrays to be > able to get members by name is a standard template for devicetree > bindings. See e.g. the clock bindings: > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml > > > If so, where is the bindings? > > As for why not document this, there are currently no devicetree users > and the devicetree maintainers have repeatedly let me know not to > submit new bindings for fwnode x86 bindings ... How above is related to fwnode as you put that? (I do see OF specific code which is required to have a binding, right?) > > And why entire code can't be converted > > to use fwnode for this case? > > This is a trivial change to allow the new functions to work > with devicetree. Note this series does not introduce any devicetree > users, hence no bindings. But it is good to have compatibility backed in > from day 1. AFAIU the OF properties must be documented from day 1. > Converting to fwnode APIs would be more involved and I cannot test > those changes.
Hi, On 12/16/22 14:57, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 12:30:09PM +0100, Hans de Goede wrote: >> Add a helper function to map the type returned by the _DSM >> method to a function name + the default polarity for that function. >> >> And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN >> cases into a single generic case. >> >> This is a preparation patch for further GPIO mapping changes. > > ... > >> +static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity) > > I find a bit strange not making this a function that returns func: > > static const char *int3472_get_func_and_polarity(u8 type, u32 *polarity) Why make it return func and not polarity ? Since there are 2 values to return it makes sense to be consistent and return both by reference. Also this sort of comments really get into the territory of bikeshedding which is not helpful IMHO. Regards, Hans > >> +{ >> + switch (type) { >> + case INT3472_GPIO_TYPE_RESET: >> + *func = "reset"; >> + *polarity = GPIO_ACTIVE_LOW; > > return "reset"; > > etc. > >> + break; >> + case INT3472_GPIO_TYPE_POWERDOWN: >> + *func = "powerdown"; >> + *polarity = GPIO_ACTIVE_LOW; >> + break; >> + case INT3472_GPIO_TYPE_CLK_ENABLE: >> + *func = "clk-enable"; >> + *polarity = GPIO_ACTIVE_HIGH; >> + break; >> + case INT3472_GPIO_TYPE_PRIVACY_LED: >> + *func = "privacy-led"; >> + *polarity = GPIO_ACTIVE_HIGH; >> + break; >> + case INT3472_GPIO_TYPE_POWER_ENABLE: >> + *func = "power-enable"; >> + *polarity = GPIO_ACTIVE_HIGH; >> + break; >> + default: >> + *func = "unknown"; >> + *polarity = GPIO_ACTIVE_HIGH; >> + break; >> + } >> +} >
Hi, On 12/16/22 15:16, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote: >> On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad >> X1 Nano gen 2 there is no clock-enable pin, triggering the: >> "No clk GPIO. The privacy LED won't work" warning and causing the privacy >> LED to not work. >> >> Fix this by modeling the privacy LED as a LED class device rather then >> integrating it with the registered clock. >> >> Note this relies on media subsys changes to actually turn the LED on/off >> when the sensor's v4l2_subdev's s_stream() operand gets called. > > ... > >> + struct int3472_pled { >> + char name[INT3472_LED_MAX_NAME_LEN]; >> + struct led_lookup_data lookup; > >> + struct led_classdev classdev; > > Why not putting this as a first member in the struct, so any container_of() > against it become no-op at compile time? Ack will fix for v4. > >> + struct gpio_desc *gpio; >> + } pled; > > ... > >> + if (IS_ERR(int3472->pled.gpio)) { >> + ret = PTR_ERR(int3472->pled.gpio); >> + return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n"); > > return dev_err_probe(...); That goes over 100 chars. > >> + } > > ... > >> + /* Generate the name, replacing the ':' in the ACPI devname with '_' */ >> + snprintf(int3472->pled.name, sizeof(int3472->pled.name), >> + "%s::privacy_led", acpi_dev_name(int3472->sensor)); > >> + for (i = 0; int3472->pled.name[i]; i++) { >> + if (int3472->pled.name[i] == ':') { >> + int3472->pled.name[i] = '_'; >> + break; >> + } >> + } > > NIH strreplace(). Please look more careful, quoting from the strreplace() docs: * strreplace - Replace all occurrences of character in string. Notice the *all* and we only want to replace the first ':' here, because the ':' char has a special meaning in LED class-device-names. > > ... > >> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472) >> +{ >> + if (IS_ERR_OR_NULL(int3472->pled.classdev.dev)) >> + return; > > This dups the check inside the _unregister() below, right? Right. > >> + led_remove_lookup(&int3472->pled.lookup); > > With list_del_init() I believe the above check can be droped. No it cannot, list_del_init() inside led_remove_lookup() would protect against double led_remove_lookup() calls. But here we may have a completely uninitialized list_head on devices without an INT3472 privacy-led, which will trigger either __list_del_entry_valid() errors or lead to NULL pointer derefs. > >> + led_classdev_unregister(&int3472->pled.classdev); >> + gpiod_put(int3472->pled.gpio); >> +} > Regards. Hans
Hi Hans, On Fri, Dec 16, 2022 at 04:45:29PM +0100, Hans de Goede wrote: > On 12/16/22 14:56, Laurent Pinchart wrote: > > On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote: > >> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs > >> for sensors with a privacy LED, rather then having to duplicate this code > >> in all the sensor drivers. > >> > >> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++ > >> include/media/v4l2-subdev.h | 3 ++ > >> 2 files changed, 43 insertions(+) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >> index 4988a25bd8f4..7344f6cd58b7 100644 > >> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >> @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, > >> sd->ops->pad->get_mbus_config(sd, pad, config); > >> } > >> > >> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > >> +#include <linux/leds.h> > > > > Can this be moved to the top of the file ? It doesn't have to be > > conditionally compiled there. > > You mean just the #include right? Ack to that. Yes, that's what I meant. > >> + > >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) > >> +{ > >> + if (!sd->dev) > >> + return; > >> + > >> + /* Try to get privacy-led once, at first s_stream() */ > >> + if (!sd->privacy_led) > >> + sd->privacy_led = led_get(sd->dev, "privacy-led"); > > > > I'm not sure I like this much. If the LED provider isn't available > > (yet), the LED will stay off. That's a privacy concern. > > At first I tried to put the led_get() inside v4l2_async_register_subdev_sensor(), > which could then return an error like -EPROBE_DEFER if the led_get() > fails (and nicely limits the led_get() to sensors). > > The problem which I hit is that v4l2-fwnode.c is build according to > CONFIG_V4L2_FWNODE where as v4l2-subdev.c is build according to > CONFIG_VIDEO_DEV > > And e.g. CONFIG_VIDEO_DEV could be builtin while CONFIG_V4L2_FWNODE > could be a module and then having the #if IS_REACHABLE(CONFIG_LEDS_CLASS) > spread over the 2 could result in different answers in the different > files ... > > One solution here could be to change CONFIG_V4L2_FWNODE and V4L2_ASYNC > to bools and link the (quite small) objects for these 2 into videodev.ko: > > videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o > videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o There's a long overdue simplification of Kconfig symbols in the subsystem. Another option would be to compile both in a single module, as they're often used together. I'll let Sakari chime in, I don't have a strong preference. > >> + > >> + if (IS_ERR(sd->privacy_led)) > >> + return; > >> + > >> + mutex_lock(&sd->privacy_led->led_access); > >> + > >> + if (enable) { > >> + led_sysfs_disable(sd->privacy_led); > >> + led_trigger_remove(sd->privacy_led); > >> + led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness); > >> + } else { > >> + led_set_brightness(sd->privacy_led, 0); > >> + led_sysfs_enable(sd->privacy_led); > > > > I don't think you should reenable control through sysfs here. I don't > > really see a use case, and you've removed the trigger anyway, so the > > behaviour would be quite inconsistent. > > Hmm, I thought this was a good compromise, this way the LED > can be used for other purposes when the sensor is off if users > want to. > > Right if users want to use a trigger then they would need > to re-attach the trigger after using the camera. > > But this way at least they can use the LED for other purposes > which since many users don't use their webcam that often > might be interesting for some users ... If the privacy LED starts being used for other purposes, users may get used to seeing it on at random times, which defeats the point of the privacy LED in the first place. > And this is consistent with how flash LEDs are handled. > > > Also, I think it would be better if the LED device was marked as "no > > sysfs" when it is registered. > > If we decide to permanently disallow userspace access then > yes this is an option. Or maybe better (to keep the LED > drivers generic), do the disable directly after the led_get() ? I suppose the small race condition wouldn't be a big issue, but if we decide that the privacy LED should really not be used for user purposes, then I'd still rather disable userspace access when registering the LED. > >> + } > >> + > >> + mutex_unlock(&sd->privacy_led->led_access); > >> +} > >> +#else > >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {} > >> +#endif > >> + > >> static int call_s_stream(struct v4l2_subdev *sd, int enable) > >> { > >> int ret; > >> > >> + call_s_stream_update_pled(sd, enable); > >> + > >> ret = sd->ops->video->s_stream(sd, enable); > >> > >> if (!enable && ret < 0) { > > > > You need to turn the LED off when enabling if .s_stream() fails. > > Ack.
Hi, On 12/16/22 17:44, Laurent Pinchart wrote: > Hi Hans, > > On Fri, Dec 16, 2022 at 04:45:29PM +0100, Hans de Goede wrote: >> On 12/16/22 14:56, Laurent Pinchart wrote: >>> On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote: >>>> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs >>>> for sensors with a privacy LED, rather then having to duplicate this code >>>> in all the sensor drivers. >>>> >>>> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++ >>>> include/media/v4l2-subdev.h | 3 ++ >>>> 2 files changed, 43 insertions(+) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >>>> index 4988a25bd8f4..7344f6cd58b7 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>>> @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad, >>>> sd->ops->pad->get_mbus_config(sd, pad, config); >>>> } >>>> >>>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) >>>> +#include <linux/leds.h> >>> >>> Can this be moved to the top of the file ? It doesn't have to be >>> conditionally compiled there. >> >> You mean just the #include right? Ack to that. > > Yes, that's what I meant. > >>>> + >>>> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) >>>> +{ >>>> + if (!sd->dev) >>>> + return; >>>> + >>>> + /* Try to get privacy-led once, at first s_stream() */ >>>> + if (!sd->privacy_led) >>>> + sd->privacy_led = led_get(sd->dev, "privacy-led"); >>> >>> I'm not sure I like this much. If the LED provider isn't available >>> (yet), the LED will stay off. That's a privacy concern. >> >> At first I tried to put the led_get() inside v4l2_async_register_subdev_sensor(), >> which could then return an error like -EPROBE_DEFER if the led_get() >> fails (and nicely limits the led_get() to sensors). >> >> The problem which I hit is that v4l2-fwnode.c is build according to >> CONFIG_V4L2_FWNODE where as v4l2-subdev.c is build according to >> CONFIG_VIDEO_DEV >> >> And e.g. CONFIG_VIDEO_DEV could be builtin while CONFIG_V4L2_FWNODE >> could be a module and then having the #if IS_REACHABLE(CONFIG_LEDS_CLASS) >> spread over the 2 could result in different answers in the different >> files ... >> >> One solution here could be to change CONFIG_V4L2_FWNODE and V4L2_ASYNC >> to bools and link the (quite small) objects for these 2 into videodev.ko: >> >> videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o >> videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o > > There's a long overdue simplification of Kconfig symbols in the > subsystem. Another option would be to compile both in a single module, > as they're often used together. I'll let Sakari chime in, I don't have a > strong preference. > >>>> + >>>> + if (IS_ERR(sd->privacy_led)) >>>> + return; >>>> + >>>> + mutex_lock(&sd->privacy_led->led_access); >>>> + >>>> + if (enable) { >>>> + led_sysfs_disable(sd->privacy_led); >>>> + led_trigger_remove(sd->privacy_led); >>>> + led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness); >>>> + } else { >>>> + led_set_brightness(sd->privacy_led, 0); >>>> + led_sysfs_enable(sd->privacy_led); >>> >>> I don't think you should reenable control through sysfs here. I don't >>> really see a use case, and you've removed the trigger anyway, so the >>> behaviour would be quite inconsistent. >> >> Hmm, I thought this was a good compromise, this way the LED >> can be used for other purposes when the sensor is off if users >> want to. >> >> Right if users want to use a trigger then they would need >> to re-attach the trigger after using the camera. >> >> But this way at least they can use the LED for other purposes >> which since many users don't use their webcam that often >> might be interesting for some users ... > > If the privacy LED starts being used for other purposes, users may get > used to seeing it on at random times, which defeats the point of the > privacy LED in the first place. Using it for other purposes it not something which I expect e.g. distros to do OOTB, so normal users won't see the LED used in another way. But it may be useful for tinkerers who do this as a local modification, in which case they know the LED behavior. With that said I'm fine with just disabling the sysfs interface once at probe / register time. Regards, Hans > >> And this is consistent with how flash LEDs are handled. >> >>> Also, I think it would be better if the LED device was marked as "no >>> sysfs" when it is registered. >> >> If we decide to permanently disallow userspace access then >> yes this is an option. Or maybe better (to keep the LED >> drivers generic), do the disable directly after the led_get() ? > > I suppose the small race condition wouldn't be a big issue, but if we > decide that the privacy LED should really not be used for user purposes, > then I'd still rather disable userspace access when registering the LED. > >>>> + } >>>> + >>>> + mutex_unlock(&sd->privacy_led->led_access); >>>> +} >>>> +#else >>>> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {} >>>> +#endif >>>> + >>>> static int call_s_stream(struct v4l2_subdev *sd, int enable) >>>> { >>>> int ret; >>>> >>>> + call_s_stream_update_pled(sd, enable); >>>> + >>>> ret = sd->ops->video->s_stream(sd, enable); >>>> >>>> if (!enable && ret < 0) { >>> >>> You need to turn the LED off when enabling if .s_stream() fails. >> >> Ack. >
Hi, On 12/16/22 18:14, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 05:29:13PM +0100, Hans de Goede wrote: >> On 12/16/22 15:16, Andy Shevchenko wrote: >>> On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote: > > ... > >>>> + if (IS_ERR(int3472->pled.gpio)) { >>>> + ret = PTR_ERR(int3472->pled.gpio); >>>> + return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n"); >>> >>> return dev_err_probe(...); >> >> That goes over 100 chars. > > The point is you don't need ret to be initialized. Moreover, no-one prevents > you to split the line to two. The compiler is perfectly capable of optimizing away the store in ret if that is not necessary; and splitting the line instead of doing it above will just make the code harder to read. Also this really is bikeshedding... > >>>> + } > > ... > >>>> + /* Generate the name, replacing the ':' in the ACPI devname with '_' */ >>>> + snprintf(int3472->pled.name, sizeof(int3472->pled.name), >>>> + "%s::privacy_led", acpi_dev_name(int3472->sensor)); >>> >>>> + for (i = 0; int3472->pled.name[i]; i++) { >>>> + if (int3472->pled.name[i] == ':') { >>>> + int3472->pled.name[i] = '_'; >>>> + break; >>>> + } >>>> + } >>> >>> NIH strreplace(). >> >> Please look more careful, quoting from the strreplace() docs: >> >> * strreplace - Replace all occurrences of character in string. >> >> Notice the *all* and we only want to replace the first ':' here, >> because the ':' char has a special meaning in LED class-device-names. > > It's still possible to use that, but anyway, the above is still > something NIH. > > char *p; > > p = strchr(name, ':'); > *p = '_'; Ok, In will switch to this for the next version. > But either code has an issue if by some reason you need to check if : is ever > present in acpi_dev_name(). acpi device names are set by this code: result = ida_alloc(instance_ida, GFP_KERNEL); if (result < 0) return result; device->pnp.instance_no = result; dev_set_name(&device->dev, "%s:%02x", acpi_device_bus_id->bus_id, result); And the bus_id cannot have a : in it, so there always is a single :. > > The more robust is either to copy acpi_dev_name(), call strreplace(), so you > will be sure that _all_ : from ACPI device name will be covered and then attach > the rest. > > ... > >>>> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472) >>>> +{ >>>> + if (IS_ERR_OR_NULL(int3472->pled.classdev.dev)) >>>> + return; >>> >>> This dups the check inside the _unregister() below, right? >> >> Right. >> >>>> + led_remove_lookup(&int3472->pled.lookup); >>> >>> With list_del_init() I believe the above check can be droped. >> >> No it cannot, list_del_init() inside led_remove_lookup() would >> protect against double led_remove_lookup() calls. >> >> But here we may have a completely uninitialized list_head on >> devices without an INT3472 privacy-led, which will trigger >> either __list_del_entry_valid() errors or lead to NULL >> pointer derefs. > > But we can initialize that as well... The standard pattern in the kernel is that INIT_LIST_HEAD() is only used for list_head-s which are actually used as the head of the list. list_head-s used to track members of the list are usually not initialized until they are added to the list. Doing multiple list-init-s in multiple cases, including one in *subsystem core code* just to drop an if here seems counter productive. Also checking that we can move forward with the unregister is a good idea regardless of all the called functions being able to run safely if the register never happened, because future changes to the unregister function might end up doing something which is unsafe when the LED was never registered in the first place. Regards, Hans > >>>> + led_classdev_unregister(&int3472->pled.classdev); >>>> + gpiod_put(int3472->pled.gpio); >>>> +} >
Hi All, Here is my 3th attempt at adjusting 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). 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 is a bit of refactoring in preparation for patch 5 which adds generic (non devicetree specific) led_get() and devm_led_get() function (which will also work with devicetree) and lookup table support to allow platform code to add LED class-device <-> consumer-dev,function lookups for non devicetree platforms. Patch 6 adds 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 privacy-LED code to all the camera sensor drivers separately (as requested by Sakari). These are all new patches in version 3. Patches 7-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 (+ prep work + some other INT3472 fixes). Assuming the LED and media maintainers are happy with the approach suggested here (if you are please give your Ack / Reviewed-by) we need to talk about how to merge this since patches 6 and 7-11 depend on the LED subsystem changes. I think it would be best if the LED subsystem can provide an immutable branch with patches 1-5 (on top of 6.2-rc1 once it is out) and then the media folks and I can merge that branch and then apply the other patches on top. This series has been tested on: - Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED - Dell Latitude 9420, IPU 6, front: ov01a1s with privacy LED - Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED back: ov8865 with privacy LED (pled not yet supported) Regards, Hans Hans de Goede (11): leds: led-class: Add missing put_device() to led_put() leds: led-class: Add __led_get() helper function leds: led-class: Add __of_led_get() helper leds: led-class: Add __devm_led_get() helper leds: led-class: Add generic [devm_]led_get() v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present platform/x86: int3472/discrete: Refactor GPIO to sensor mapping platform/x86: int3472/discrete: Create a LED class device for the privacy LED platform/x86: int3472/discrete: Move GPIO request to skl_int3472_register_clock() platform/x86: int3472/discrete: Ensure the clk/power enable pins are in output mode platform/x86: int3472/discrete: Get the polarity from the _DSM entry drivers/leds/led-class.c | 174 +++++++++++++++--- drivers/media/v4l2-core/v4l2-subdev.c | 40 ++++ drivers/platform/x86/intel/int3472/Makefile | 2 +- .../x86/intel/int3472/clk_and_regulator.c | 35 +++- drivers/platform/x86/intel/int3472/common.h | 18 +- drivers/platform/x86/intel/int3472/discrete.c | 96 +++++----- drivers/platform/x86/intel/int3472/led.c | 75 ++++++++ include/linux/leds.h | 18 ++ include/media/v4l2-subdev.h | 3 + 9 files changed, 371 insertions(+), 90 deletions(-) create mode 100644 drivers/platform/x86/intel/int3472/led.c