mbox series

[v4,00/11] leds: lookup-table support + int3472/media privacy LED support

Message ID 20230119130053.111344-1-hdegoede@redhat.com
Headers show
Series leds: lookup-table support + int3472/media privacy LED support | expand

Message

Hans de Goede Jan. 19, 2023, 1 p.m. UTC
Hi All,

Here is my version 4 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 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.

If you are one of the folks who requested the new LED lookup table +
led_get() approach I would appreciate a Reviewed-by or Acked-by for
patches 1-4.

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_module_get() helper
  leds: led-class: Add __devm_led_get() helper
  leds: led-class: Add generic [devm_]led_get()
  [RFC] leds: led-class: Add devicetree support to led_get()
  media: v4l2-core: Built async and fwnode code into videodev.ko
  media: v4l2-core: Make the v4l2-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: Get the polarity from the _DSM entry

 drivers/leds/led-class.c                      | 173 +++++++++++++++---
 drivers/media/v4l2-core/Kconfig               |   4 +-
 drivers/media/v4l2-core/Makefile              |   4 +-
 drivers/media/v4l2-core/v4l2-async.c          |  15 +-
 drivers/media/v4l2-core/v4l2-dev.c            |   7 +
 drivers/media/v4l2-core/v4l2-fwnode.c         |  21 ++-
 drivers/media/v4l2-core/v4l2-subdev.c         |  18 ++
 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 | 100 +++++-----
 drivers/platform/x86/intel/int3472/led.c      |  75 ++++++++
 include/linux/leds.h                          |  18 ++
 include/media/v4l2-async.h                    |   4 +
 include/media/v4l2-subdev.h                   |   3 +
 15 files changed, 380 insertions(+), 117 deletions(-)
 create mode 100644 drivers/platform/x86/intel/int3472/led.c

Comments

Andy Shevchenko Jan. 19, 2023, 1:29 p.m. UTC | #1
On Thu, Jan 19, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> 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.
>
> And use this new helper to add devicetree support to the generic
> (non devicetree specific) [devm_]led_get() function.
>
> This uses the standard devicetree pattern of adding a -names string array
> to map names to the indexes for an array of resources.
>
> Note the new led-names property for LED consumers is not added
> to the devicetree documentation because there seems to be no
> documentation for the leds property itself to extend it with this.
> It seems that how LED consumers should be described is not documented
> at all ATM.
>
> This patch is marked as RFC because of both the missing devicetree
> documentation and because there are no devicetree users of
> the generic [devm_]led_get() function for now.

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Yeah, it's a pity we have no documentation for the LED properties...

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/leds/led-class.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 6dff57c41e96..22f658c750d1 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -234,19 +234,18 @@ static struct led_classdev *led_module_get(struct device *led_dev)
>         return led_cdev;
>  }
>
> -/**
> - * of_led_get() - request a LED device via the LED framework
> - * @np: device node to get the LED device from
> - * @index: the index of the LED
> - *
> - * Returns the LED device parsed from the phandle specified in the "leds"
> - * property of a device tree node or a negative error-code on failure.
> - */
> -struct led_classdev *of_led_get(struct device_node *np, int index)
> +static struct led_classdev *__of_led_get(struct device_node *np, int index,
> +                                        const char *name)
>  {
>         struct device *led_dev;
>         struct device_node *led_node;
>
> +       /*
> +        * For named LEDs, first look up the name in the "led-names" property.
> +        * If it cannot be found, then of_parse_phandle() will propagate the error.
> +        */
> +       if (name)
> +               index = of_property_match_string(np, "led-names", name);
>         led_node = of_parse_phandle(np, "leds", index);
>         if (!led_node)
>                 return ERR_PTR(-ENOENT);
> @@ -256,6 +255,19 @@ struct led_classdev *of_led_get(struct device_node *np, int index)
>
>         return led_module_get(led_dev);
>  }
> +
> +/**
> + * of_led_get() - request a LED device via the LED framework
> + * @np: device node to get the LED device from
> + * @index: the index of the LED
> + *
> + * Returns the LED device parsed from the phandle specified in the "leds"
> + * property of a device tree node or a negative error-code on failure.
> + */
> +struct led_classdev *of_led_get(struct device_node *np, int index)
> +{
> +       return __of_led_get(np, index, NULL);
> +}
>  EXPORT_SYMBOL_GPL(of_led_get);
>
>  /**
> @@ -329,9 +341,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
>  struct led_classdev *led_get(struct device *dev, char *function)
>  {
>         struct led_lookup_data *lookup;
> +       struct led_classdev *led_cdev;
>         const char *led_name = NULL;
>         struct device *led_dev;
>
> +       if (dev->of_node) {
> +               led_cdev = __of_led_get(dev->of_node, -1, function);
> +               if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
> +                       return led_cdev;
> +       }
> +
>         mutex_lock(&leds_lookup_lock);
>         list_for_each_entry(lookup, &leds_lookup_list, list) {
>                 if (!strcmp(lookup->consumer_dev_name, dev_name(dev)) &&
> --
> 2.39.0
>
Linus Walleij Jan. 20, 2023, 7:23 a.m. UTC | #2
On Thu, Jan 19, 2023 at 2:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

> led_put() is used to "undo" a successful of_led_get() call,
> of_led_get() uses class_find_device_by_of_node() which returns
> a reference to the device which must be free-ed with put_device()
> when the caller is done with it.
>
> Add a put_device() call to led_put() to free the reference returned
> by class_find_device_by_of_node().
>
> And also add a put_device() in the error-exit case of try_module_get()
> failing.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Linus Walleij Jan. 20, 2023, 7:25 a.m. UTC | #3
On Thu, Jan 19, 2023 at 2:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Add a __devm_led_get() helper which registers a passed in led_classdev
> with devm for unregistration.
>
> This is a preparation patch for adding a generic (non devicetree specific)
> devm_led_get() function.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I'm not a fan of __inner_functions because they are easy to
confuse with the namespace for __compiler_directives
but no big deal so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Linus Walleij Jan. 20, 2023, 7:27 a.m. UTC | #4
On Thu, Jan 19, 2023 at 2:01 PM Hans de Goede <hdegoede@redhat.com> 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.
>
> And use this new helper to add devicetree support to the generic
> (non devicetree specific) [devm_]led_get() function.
>
> This uses the standard devicetree pattern of adding a -names string array
> to map names to the indexes for an array of resources.
>
> Note the new led-names property for LED consumers is not added
> to the devicetree documentation because there seems to be no
> documentation for the leds property itself to extend it with this.
> It seems that how LED consumers should be described is not documented
> at all ATM.
>
> This patch is marked as RFC because of both the missing devicetree
> documentation and because there are no devicetree users of
> the generic [devm_]led_get() function for now.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Same grumpiness about __functions but this is overall nice so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Linus Walleij Jan. 20, 2023, 7:28 a.m. UTC | #5
On Thu, Jan 19, 2023 at 2:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Here is my version 4 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 v4:

I think this is good for merge, I reviewed the LED stuff that I understand,
but for the rest in drivers/media FWIW:
Acked-by: Linus Walleij <linus.walleij@linaro.org>
as well.

I really like how this developed to solve a real old outstanding hole
in the implementation.

Yours,
Linus Walleij