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

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

[Hans de Goede]

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

-- 
2.39.0

[PATCH v4 01/11] leds: led-class: Add missing put_device() to led_put()

[Hans de Goede]

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>
---
 drivers/leds/led-class.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 6a8ea94834fa..7391d2cf1370 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -241,8 +241,10 @@ struct led_classdev *of_led_get(struct device_node *np, int index)
 
 	led_cdev = dev_get_drvdata(led_dev);
 
-	if (!try_module_get(led_cdev->dev->parent->driver->owner))
+	if (!try_module_get(led_cdev->dev->parent->driver->owner)) {
+		put_device(led_cdev->dev);
 		return ERR_PTR(-ENODEV);
+	}
 
 	return led_cdev;
 }
@@ -255,6 +257,7 @@ EXPORT_SYMBOL_GPL(of_led_get);
 void led_put(struct led_classdev *led_cdev)
 {
 	module_put(led_cdev->dev->parent->driver->owner);
+	put_device(led_cdev->dev);
 }
 EXPORT_SYMBOL_GPL(led_put);
 
-- 
2.39.0

[PATCH v4 02/11] leds: led-class: Add led_module_get() helper

[Hans de Goede]

Split out part of of_led_get() into a generic led_module_get() helper
function.

This is a preparation patch for adding a generic (non devicetree specific)
led_get() function.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Rename helper from __led_get() to led_module_get()
---
 drivers/leds/led-class.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 7391d2cf1370..743d97b082dc 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -215,6 +215,23 @@ static int led_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
 
+static struct led_classdev *led_module_get(struct device *led_dev)
+{
+	struct led_classdev *led_cdev;
+
+	if (!led_dev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	led_cdev = dev_get_drvdata(led_dev);
+
+	if (!try_module_get(led_cdev->dev->parent->driver->owner)) {
+		put_device(led_cdev->dev);
+		return ERR_PTR(-ENODEV);
+	}
+
+	return led_cdev;
+}
+
 /**
  * of_led_get() - request a LED device via the LED framework
  * @np: device node to get the LED device from
@@ -226,7 +243,6 @@ static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
 struct led_classdev *of_led_get(struct device_node *np, int index)
 {
 	struct device *led_dev;
-	struct led_classdev *led_cdev;
 	struct device_node *led_node;
 
 	led_node = of_parse_phandle(np, "leds", index);
@@ -236,17 +252,7 @@ struct led_classdev *of_led_get(struct device_node *np, int index)
 	led_dev = class_find_device_by_of_node(leds_class, led_node);
 	of_node_put(led_node);
 
-	if (!led_dev)
-		return ERR_PTR(-EPROBE_DEFER);
-
-	led_cdev = dev_get_drvdata(led_dev);
-
-	if (!try_module_get(led_cdev->dev->parent->driver->owner)) {
-		put_device(led_cdev->dev);
-		return ERR_PTR(-ENODEV);
-	}
-
-	return led_cdev;
+	return led_module_get(led_dev);
 }
 EXPORT_SYMBOL_GPL(of_led_get);
 
-- 
2.39.0

[PATCH v4 03/11] leds: led-class: Add __devm_led_get() helper

[Hans de Goede]

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>
---
 drivers/leds/led-class.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 743d97b082dc..4904d140a560 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -274,6 +274,22 @@ static void devm_led_release(struct device *dev, void *res)
 	led_put(*p);
 }
 
+static struct led_classdev *__devm_led_get(struct device *dev, struct led_classdev *led)
+{
+	struct led_classdev **dr;
+
+	dr = devres_alloc(devm_led_release, sizeof(struct led_classdev *), GFP_KERNEL);
+	if (!dr) {
+		led_put(led);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	*dr = led;
+	devres_add(dev, dr);
+
+	return led;
+}
+
 /**
  * devm_of_led_get - Resource-managed request of a LED device
  * @dev:	LED consumer
@@ -289,7 +305,6 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 						  int index)
 {
 	struct led_classdev *led;
-	struct led_classdev **dr;
 
 	if (!dev)
 		return ERR_PTR(-EINVAL);
@@ -298,17 +313,7 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 	if (IS_ERR(led))
 		return led;
 
-	dr = devres_alloc(devm_led_release, sizeof(struct led_classdev *),
-			  GFP_KERNEL);
-	if (!dr) {
-		led_put(led);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	*dr = led;
-	devres_add(dev, dr);
-
-	return led;
+	return __devm_led_get(dev, led);
 }
 EXPORT_SYMBOL_GPL(devm_of_led_get);
 
-- 
2.39.0

[PATCH v4 04/11] leds: led-class: Add generic [devm_]led_get()

[Hans de Goede]

Add a generic [devm_]led_get() method which can be used on both devicetree
and non devicetree platforms to get a LED classdev associated with
a specific function on a specific device, e.g. the privacy LED associated
with a specific camera sensor.

Note unlike of_led_get() this takes a string describing the function
rather then an index. This is done because e.g. camera sensors might
have a privacy LED, or a flash LED, or both and using an index
approach leaves it unclear what the function of index 0 is if there is
only 1 LED.

This uses a lookup-table mechanism for non devicetree platforms.
This allows the platform code to map specific LED class_dev-s to a specific
device,function combinations this way.

For devicetree platforms getting the LED by function-name could be made
to work using the standard devicetree pattern of adding a -names string
array to map names to the indexes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Split out support for led_get() devicetree name-based lookup support
  into a separate RFC patch as there currently are no user for this
- Use kstrdup_const() / kfree_const() for the led_name
---
 drivers/leds/led-class.c | 84 ++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     | 18 +++++++++
 2 files changed, 102 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 4904d140a560..6dff57c41e96 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -23,6 +23,8 @@
 #include "leds.h"
 
 static struct class *leds_class;
+static DEFINE_MUTEX(leds_lookup_lock);
+static LIST_HEAD(leds_lookup_list);
 
 static ssize_t brightness_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
@@ -317,6 +319,88 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_of_led_get);
 
+/**
+ * led_get() - request a LED device via the LED framework
+ * @dev: device for which to get the LED device
+ * @function: string describing the function of the LED device
+ *
+ * @return a pointer to a LED device or ERR_PTR(errno) on failure.
+ */
+struct led_classdev *led_get(struct device *dev, char *function)
+{
+	struct led_lookup_data *lookup;
+	const char *led_name = NULL;
+	struct device *led_dev;
+
+	mutex_lock(&leds_lookup_lock);
+	list_for_each_entry(lookup, &leds_lookup_list, list) {
+		if (!strcmp(lookup->consumer_dev_name, dev_name(dev)) &&
+		    !strcmp(lookup->consumer_function, function)) {
+			led_name = kstrdup_const(lookup->led_name, GFP_KERNEL);
+			break;
+		}
+	}
+	mutex_unlock(&leds_lookup_lock);
+
+	if (!led_name)
+		return ERR_PTR(-ENOENT);
+
+	led_dev = class_find_device_by_name(leds_class, led_name);
+	kfree_const(led_name);
+
+	return led_module_get(led_dev);
+}
+EXPORT_SYMBOL_GPL(led_get);
+
+/**
+ * devm_led_get() - request a LED device via the LED framework
+ * @dev: device for which to get the LED device
+ * @function: string describing the function of the LED device
+ *
+ * The LED device returned from this function is automatically released
+ * on driver detach.
+ *
+ * @return a pointer to a LED device or ERR_PTR(errno) on failure.
+ */
+struct led_classdev *devm_led_get(struct device *dev, char *function)
+{
+	struct led_classdev *led;
+
+	led = led_get(dev, function);
+	if (IS_ERR(led))
+		return led;
+
+	return __devm_led_get(dev, led);
+}
+EXPORT_SYMBOL_GPL(devm_led_get);
+
+/**
+ * led_add_lookup() - Add a LED lookup table entry
+ * @led_lookup: the lookup table entry to add
+ *
+ * Add a LED lookup table entry. On systems without devicetree the lookup table
+ * is used by led_get() to find LEDs.
+ */
+void led_add_lookup(struct led_lookup_data *led_lookup)
+{
+	mutex_lock(&leds_lookup_lock);
+	list_add_tail(&led_lookup->list, &leds_lookup_list);
+	mutex_unlock(&leds_lookup_lock);
+}
+EXPORT_SYMBOL_GPL(led_add_lookup);
+
+/**
+ * led_remove_lookup() - Remove a LED lookup table entry
+ * @led_lookup: the lookup table entry to remove
+ */
+void led_remove_lookup(struct led_lookup_data *led_lookup)
+{
+	mutex_lock(&leds_lookup_lock);
+	list_del(&led_lookup->list);
+	mutex_unlock(&leds_lookup_lock);
+}
+EXPORT_SYMBOL_GPL(led_remove_lookup);
+
 static int led_classdev_next_name(const char *init_name, char *name,
 				  size_t len)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..e44fc5ec7c9a 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -39,6 +39,18 @@ enum led_default_state {
 	LEDS_DEFSTATE_KEEP	= 2,
 };
 
+/*
+ * This is used to tell led_get() device which led_classdev to return for
+ * a specific consumer device-name, function pair on non devicetree platforms.
+ * Note all strings must be set.
+ */
+struct led_lookup_data {
+	struct list_head list;
+	const char *led_name;
+	const char *consumer_dev_name;
+	const char *consumer_function;
+};
+
 struct led_init_data {
 	/* device fwnode handle */
 	struct fwnode_handle *fwnode;
@@ -211,6 +223,12 @@ void devm_led_classdev_unregister(struct device *parent,
 void led_classdev_suspend(struct led_classdev *led_cdev);
 void led_classdev_resume(struct led_classdev *led_cdev);
 
+void led_add_lookup(struct led_lookup_data *led_lookup);
+void led_remove_lookup(struct led_lookup_data *led_lookup);
+
+struct led_classdev *__must_check led_get(struct device *dev, char *function);
+struct led_classdev *__must_check devm_led_get(struct device *dev, char *function);
+
 extern struct led_classdev *of_led_get(struct device_node *np, int index);
 extern void led_put(struct led_classdev *led_cdev);
 struct led_classdev *__must_check devm_of_led_get(struct device *dev,
-- 
2.39.0

[PATCH v4 05/11] [RFC] leds: led-class: Add devicetree support to led_get()

[Hans de Goede]

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>
---
 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

[PATCH v4 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko

[Hans de Goede]

Currently the videodev.ko code may be builtin while e.g. v4l2-fwnode.ko
is build as a module.

This makes it hard to add code depending on other subsystems spanning
both videodev.ko and v4l2-fwnode.ko. Specifically this block adding code
depending on the LED subsystem.

This is made even harder because CONFIG_V4L2_FWNODE is selected,
not depended on so it itself cannot depend on another subsystem without
editing all the Kconfig symbols selecting it to also list the dependency
and there are many of such symbols.

Adding a "select LED_CLASS if NEW_LEDS" to CONFIG_V4L2_FWNODE leads
to Kconfig erroring out with "error: recursive dependency detected!".

To fix this dependency mess, change the V4L2_FWNODE and V4L2_ASYNC
(which V4L2_FWNODE selects) Kconfig symbols from tristate to bools and
link their code into videodev.ko instead of making them separate modules.

This will allow using IS_REACHABLE(LED_CLASS) for the new LED integration
code without needing to worry that it expands to 0 in some places and
1 in other places because some of the code being builtin vs modular.

On x86_64 this leads to the following size changes for videodev.ko

[hans@shalem linux]$ size drivers/media/v4l2-core/videodev.ko

Before:
   text	   data	    bss	    dec	    hex	filename
 218206	  14395	   2448	 235049	  39629 drivers/media/v4l2-core/videodev.ko
After:
   text	   data	    bss	    dec	    hex	filename
 243213	  17615	   2456	 263284	  40474	drivers/media/v4l2-core/videodev.ko

So (as expected) there is some increase in size here, but it
really is not that much.

And the uncompressed no-debuginfo .ko file disk-usage actually shrinks
by 17 KiB (comparing the slightly larger videodev.ko against the
3 original modules) and loading time will also be better.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- New patch in v4 of this patch-set
---
 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 |  6 ------
 include/media/v4l2-async.h            |  4 ++++
 6 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index 348559bc2468..73574d946010 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -68,11 +68,11 @@ config V4L2_FLASH_LED_CLASS
 	  When in doubt, say N.
 
 config V4L2_FWNODE
-	tristate
+	bool
 	select V4L2_ASYNC
 
 config V4L2_ASYNC
-	tristate
+	bool
 
 # Used by drivers that need Videobuf modules
 config VIDEOBUF_GEN
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 41d91bd10cf2..8c5a1ab8d939 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -15,7 +15,9 @@ videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
 
 # Please keep it alphabetically sorted by Kconfig name
 # (e. g. LC_ALL=C sort Makefile)
+videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
 videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
+videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
 videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
 videodev-$(CONFIG_SPI) += v4l2-spi.o
 videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
@@ -24,9 +26,7 @@ videodev-$(CONFIG_VIDEO_V4L2_I2C) += v4l2-i2c.o
 # Please keep it alphabetically sorted by Kconfig name
 # (e. g. LC_ALL=C sort Makefile)
 
-obj-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
 obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash-led-class.o
-obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
 obj-$(CONFIG_V4L2_H264) += v4l2-h264.o
 obj-$(CONFIG_V4L2_JPEG_HELPER) += v4l2-jpeg.o
 obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 2f1b718a9189..b53012cbda34 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -11,7 +11,6 @@
 #include <linux/i2c.h>
 #include <linux/list.h>
 #include <linux/mm.h>
-#include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -900,25 +899,15 @@ DEFINE_SHOW_ATTRIBUTE(pending_subdevs);
 
 static struct dentry *v4l2_async_debugfs_dir;
 
-static int __init v4l2_async_init(void)
+void __init v4l2_async_debugfs_init(void)
 {
 	v4l2_async_debugfs_dir = debugfs_create_dir("v4l2-async", NULL);
 	debugfs_create_file("pending_async_subdevices", 0444,
 			    v4l2_async_debugfs_dir, NULL,
 			    &pending_subdevs_fops);
-
-	return 0;
 }
 
-static void __exit v4l2_async_exit(void)
+void __exit v4l2_async_debugfs_exit(void)
 {
 	debugfs_remove_recursive(v4l2_async_debugfs_dir);
 }
-
-subsys_initcall(v4l2_async_init);
-module_exit(v4l2_async_exit);
-
-MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
-MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
-MODULE_AUTHOR("Ezequiel Garcia <ezequiel@collabora.com>");
-MODULE_LICENSE("GPL");
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 397d553177fa..02364985817c 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -26,6 +26,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 
+#include <media/v4l2-async.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
@@ -1190,6 +1191,7 @@ static int __init videodev_init(void)
 		return -EIO;
 	}
 
+	v4l2_async_debugfs_init();
 	return 0;
 }
 
@@ -1197,6 +1199,7 @@ static void __exit videodev_exit(void)
 {
 	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
 
+	v4l2_async_debugfs_exit();
 	class_unregister(&video_class);
 	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
 }
@@ -1205,6 +1208,10 @@ subsys_initcall(videodev_init);
 module_exit(videodev_exit)
 
 MODULE_AUTHOR("Alan Cox, Mauro Carvalho Chehab <mchehab@kernel.org>, Bill Dirks, Justin Schoeman, Gerd Knorr");
+MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
+MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
+MODULE_AUTHOR("Ezequiel Garcia <ezequiel@collabora.com>");
+MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
 MODULE_DESCRIPTION("Video4Linux2 core driver");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_CHARDEV_MAJOR(VIDEO_MAJOR);
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 3d9533c1b202..c8a2264262bc 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -17,7 +17,6 @@
 #include <linux/acpi.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
-#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/property.h>
 #include <linux/slab.h>
@@ -1328,8 +1327,3 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor);
-
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
-MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
-MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@gmx.de>");
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 25eb1d138c06..c6a59ccc0c28 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -313,4 +313,8 @@ v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd);
  * @sd: pointer to &struct v4l2_subdev
  */
 void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);
+
+void v4l2_async_debugfs_init(void);
+void v4l2_async_debugfs_exit(void);
+
 #endif
-- 
2.39.0

[PATCH v4 07/11] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[Hans de Goede]

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>
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)
 {
+#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
-- 
2.39.0

[PATCH v4 08/11] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Hans de Goede]

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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Add break to default case

Changes in v2:
- Make the helper function doing the type -> function mapping,
  also return a default polarity for the function.
---
 drivers/platform/x86/intel/int3472/discrete.c | 45 +++++++++++++++----
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index c42c3faa2c32..708d51f9b41d 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -188,6 +188,36 @@ static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
 	return 0;
 }
 
+static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
+{
+	switch (type) {
+	case INT3472_GPIO_TYPE_RESET:
+		*func = "reset";
+		*polarity = GPIO_ACTIVE_LOW;
+		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;
+	}
+}
+
 /**
  * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
  * @ares: A pointer to a &struct acpi_resource
@@ -227,6 +257,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	struct acpi_resource_gpio *agpio;
 	union acpi_object *obj;
 	const char *err_msg;
+	const char *func;
+	u32 polarity;
 	int ret;
 	u8 type;
 
@@ -250,19 +282,14 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	type = obj->integer.value & 0xff;
 
+	int3472_get_func_and_polarity(type, &func, &polarity);
+
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
-		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "reset",
-						     GPIO_ACTIVE_LOW);
-		if (ret)
-			err_msg = "Failed to map reset pin to sensor\n";
-
-		break;
 	case INT3472_GPIO_TYPE_POWERDOWN:
-		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "powerdown",
-						     GPIO_ACTIVE_LOW);
+		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
 		if (ret)
-			err_msg = "Failed to map powerdown pin to sensor\n";
+			err_msg = "Failed to map GPIO pin to sensor\n";
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-- 
2.39.0

[PATCH v4 09/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Hans de Goede]

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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Make struct led_classdev the first member of the pled struct
- Use strchr to replace the : with _ in the acpi_dev_name()
---
 drivers/platform/x86/intel/int3472/Makefile   |  2 +-
 .../x86/intel/int3472/clk_and_regulator.c     |  3 -
 drivers/platform/x86/intel/int3472/common.h   | 15 +++-
 drivers/platform/x86/intel/int3472/discrete.c | 58 ++++-----------
 drivers/platform/x86/intel/int3472/led.c      | 74 +++++++++++++++++++
 5 files changed, 105 insertions(+), 47 deletions(-)
 create mode 100644 drivers/platform/x86/intel/int3472/led.c

diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile
index cfec7784c5c9..9f16cb514397 100644
--- a/drivers/platform/x86/intel/int3472/Makefile
+++ b/drivers/platform/x86/intel/int3472/Makefile
@@ -1,4 +1,4 @@
 obj-$(CONFIG_INTEL_SKL_INT3472)		+= intel_skl_int3472_discrete.o \
 					   intel_skl_int3472_tps68470.o
-intel_skl_int3472_discrete-y		:= discrete.o clk_and_regulator.o common.o
+intel_skl_int3472_discrete-y		:= discrete.o clk_and_regulator.o led.o common.o
 intel_skl_int3472_tps68470-y		:= tps68470.o tps68470_board_data.o common.o
diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 74dc2cff799e..e3b597d93388 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -23,8 +23,6 @@ static int skl_int3472_clk_prepare(struct clk_hw *hw)
 	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
 
 	gpiod_set_value_cansleep(clk->ena_gpio, 1);
-	gpiod_set_value_cansleep(clk->led_gpio, 1);
-
 	return 0;
 }
 
@@ -33,7 +31,6 @@ static void skl_int3472_clk_unprepare(struct clk_hw *hw)
 	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
 
 	gpiod_set_value_cansleep(clk->ena_gpio, 0);
-	gpiod_set_value_cansleep(clk->led_gpio, 0);
 }
 
 static int skl_int3472_clk_enable(struct clk_hw *hw)
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 53270d19c73a..82dc37e08882 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -6,6 +6,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/gpio/machine.h>
+#include <linux/leds.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/types.h>
@@ -28,6 +29,8 @@
 #define GPIO_REGULATOR_NAME_LENGTH				21
 #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH			9
 
+#define INT3472_LED_MAX_NAME_LEN				32
+
 #define CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET			86
 
 #define INT3472_REGULATOR(_name, _supply, _ops)			\
@@ -96,10 +99,16 @@ struct int3472_discrete_device {
 		struct clk_hw clk_hw;
 		struct clk_lookup *cl;
 		struct gpio_desc *ena_gpio;
-		struct gpio_desc *led_gpio;
 		u32 frequency;
 	} clock;
 
+	struct int3472_pled {
+		struct led_classdev classdev;
+		struct led_lookup_data lookup;
+		char name[INT3472_LED_MAX_NAME_LEN];
+		struct gpio_desc *gpio;
+	} pled;
+
 	unsigned int ngpios; /* how many GPIOs have we seen */
 	unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
 	struct gpiod_lookup_table gpios;
@@ -119,4 +128,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 				   struct acpi_resource_gpio *agpio);
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472);
 
+int skl_int3472_register_pled(struct int3472_discrete_device *int3472,
+			      struct acpi_resource_gpio *agpio, u32 polarity);
+void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472);
+
 #endif
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 708d51f9b41d..38b1372e0745 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -155,37 +155,21 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 }
 
 static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
-				       struct acpi_resource_gpio *agpio, u8 type)
+				       struct acpi_resource_gpio *agpio)
 {
 	char *path = agpio->resource_source.string_ptr;
 	u16 pin = agpio->pin_table[0];
 	struct gpio_desc *gpio;
 
-	switch (type) {
-	case INT3472_GPIO_TYPE_CLK_ENABLE:
-		gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
-		if (IS_ERR(gpio))
-			return (PTR_ERR(gpio));
-
-		int3472->clock.ena_gpio = gpio;
-		/* Ensure the pin is in output mode and non-active state */
-		gpiod_direction_output(int3472->clock.ena_gpio, 0);
-		break;
-	case INT3472_GPIO_TYPE_PRIVACY_LED:
-		gpio = acpi_get_and_request_gpiod(path, pin, "int3472,privacy-led");
-		if (IS_ERR(gpio))
-			return (PTR_ERR(gpio));
+	gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
+	if (IS_ERR(gpio))
+		return (PTR_ERR(gpio));
 
-		int3472->clock.led_gpio = gpio;
-		/* Ensure the pin is in output mode and non-active state */
-		gpiod_direction_output(int3472->clock.led_gpio, 0);
-		break;
-	default:
-		dev_err(int3472->dev, "Invalid GPIO type 0x%02x for clock\n", type);
-		break;
-	}
+	int3472->clock.ena_gpio = gpio;
+	/* Ensure the pin is in output mode and non-active state */
+	gpiod_direction_output(int3472->clock.ena_gpio, 0);
 
-	return 0;
+	return skl_int3472_register_clock(int3472);
 }
 
 static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
@@ -293,11 +277,16 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-	case INT3472_GPIO_TYPE_PRIVACY_LED:
-		ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type);
+		ret = skl_int3472_map_gpio_to_clk(int3472, agpio);
 		if (ret)
 			err_msg = "Failed to map GPIO to clock\n";
 
+		break;
+	case INT3472_GPIO_TYPE_PRIVACY_LED:
+		ret = skl_int3472_register_pled(int3472, agpio, polarity);
+		if (ret)
+			err_msg = "Failed to register LED\n";
+
 		break;
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
 		ret = skl_int3472_register_regulator(int3472, agpio);
@@ -341,21 +330,6 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
 
 	acpi_dev_free_resource_list(&resource_list);
 
-	/*
-	 * If we find no clock enable GPIO pin then the privacy LED won't work.
-	 * We've never seen that situation, but it's possible. Warn the user so
-	 * it's clear what's happened.
-	 */
-	if (int3472->clock.ena_gpio) {
-		ret = skl_int3472_register_clock(int3472);
-		if (ret)
-			return ret;
-	} else {
-		if (int3472->clock.led_gpio)
-			dev_warn(int3472->dev,
-				 "No clk GPIO. The privacy LED won't work\n");
-	}
-
 	int3472->gpios.dev_id = int3472->sensor_name;
 	gpiod_add_lookup_table(&int3472->gpios);
 
@@ -372,8 +346,8 @@ static int skl_int3472_discrete_remove(struct platform_device *pdev)
 		skl_int3472_unregister_clock(int3472);
 
 	gpiod_put(int3472->clock.ena_gpio);
-	gpiod_put(int3472->clock.led_gpio);
 
+	skl_int3472_unregister_pled(int3472);
 	skl_int3472_unregister_regulator(int3472);
 
 	return 0;
diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
new file mode 100644
index 000000000000..00269e9af9c5
--- /dev/null
+++ b/drivers/platform/x86/intel/int3472/led.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Hans de Goede <hdegoede@redhat.com> */
+
+#include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/leds.h>
+#include "common.h"
+
+static int int3472_pled_set(struct led_classdev *led_cdev,
+				     enum led_brightness brightness)
+{
+	struct int3472_discrete_device *int3472 =
+		container_of(led_cdev, struct int3472_discrete_device, pled.classdev);
+
+	gpiod_set_value_cansleep(int3472->pled.gpio, brightness);
+	return 0;
+}
+
+int skl_int3472_register_pled(struct int3472_discrete_device *int3472,
+			      struct acpi_resource_gpio *agpio, u32 polarity)
+{
+	char *p, *path = agpio->resource_source.string_ptr;
+	int ret;
+
+	if (int3472->pled.classdev.dev)
+		return -EBUSY;
+
+	int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
+							     "int3472,privacy-led");
+	if (IS_ERR(int3472->pled.gpio))
+		return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio),
+				     "getting privacy LED GPIO\n");
+
+	if (polarity == GPIO_ACTIVE_LOW)
+		gpiod_toggle_active_low(int3472->pled.gpio);
+
+	/* Ensure the pin is in output mode and non-active state */
+	gpiod_direction_output(int3472->pled.gpio, 0);
+
+	/* 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));
+	p = strchr(int3472->pled.name, ':');
+	*p = '_';
+
+	int3472->pled.classdev.name = int3472->pled.name;
+	int3472->pled.classdev.max_brightness = 1;
+	int3472->pled.classdev.brightness_set_blocking = int3472_pled_set;
+
+	ret = led_classdev_register(int3472->dev, &int3472->pled.classdev);
+	if (ret)
+		goto err_free_gpio;
+
+	int3472->pled.lookup.led_name = int3472->pled.name;
+	int3472->pled.lookup.consumer_dev_name = int3472->sensor_name;
+	int3472->pled.lookup.consumer_function = "privacy-led";
+	led_add_lookup(&int3472->pled.lookup);
+
+	return 0;
+
+err_free_gpio:
+	gpiod_put(int3472->pled.gpio);
+	return ret;
+}
+
+void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
+{
+	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
+		return;
+
+	led_remove_lookup(&int3472->pled.lookup);
+	led_classdev_unregister(&int3472->pled.classdev);
+	gpiod_put(int3472->pled.gpio);
+}
-- 
2.39.0

[PATCH v4 10/11] platform/x86: int3472/discrete: Move GPIO request to skl_int3472_register_clock()

[Hans de Goede]

Move the requesting of the clk-enable GPIO to skl_int3472_register_clock()
(and move the gpiod_put to unregister).

This mirrors the GPIO handling in skl_int3472_register_regulator() and
allows removing skl_int3472_map_gpio_to_clk() from discrete.c.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../x86/intel/int3472/clk_and_regulator.c     | 28 +++++++++++++++--
 drivers/platform/x86/intel/int3472/common.h   |  3 +-
 drivers/platform/x86/intel/int3472/discrete.c | 30 ++-----------------
 3 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index e3b597d93388..626e5e86f4e0 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -86,18 +86,34 @@ static const struct clk_ops skl_int3472_clock_ops = {
 	.recalc_rate = skl_int3472_clk_recalc_rate,
 };
 
-int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
+int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
+			       struct acpi_resource_gpio *agpio)
 {
+	char *path = agpio->resource_source.string_ptr;
 	struct clk_init_data init = {
 		.ops = &skl_int3472_clock_ops,
 		.flags = CLK_GET_RATE_NOCACHE,
 	};
 	int ret;
 
+	if (int3472->clock.cl)
+		return -EBUSY;
+
+	int3472->clock.ena_gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
+							     "int3472,clk-enable");
+	if (IS_ERR(int3472->clock.ena_gpio))
+		return dev_err_probe(int3472->dev, PTR_ERR(int3472->clock.ena_gpio),
+				     "getting clk-enable GPIO\n");
+
+	/* Ensure the pin is in output mode and non-active state */
+	gpiod_direction_output(int3472->clock.ena_gpio, 0);
+
 	init.name = kasprintf(GFP_KERNEL, "%s-clk",
 			      acpi_dev_name(int3472->adev));
-	if (!init.name)
-		return -ENOMEM;
+	if (!init.name) {
+		ret = -ENOMEM;
+		goto out_put_gpio;
+	}
 
 	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
 
@@ -123,14 +139,20 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
 	clk_unregister(int3472->clock.clk);
 out_free_init_name:
 	kfree(init.name);
+out_put_gpio:
+	gpiod_put(int3472->clock.ena_gpio);
 
 	return ret;
 }
 
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
 {
+	if (!int3472->clock.cl)
+		return;
+
 	clkdev_drop(int3472->clock.cl);
 	clk_unregister(int3472->clock.clk);
+	gpiod_put(int3472->clock.ena_gpio);
 }
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 82dc37e08882..0d4fa7d00b5f 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -121,7 +121,8 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 					 struct acpi_device **sensor_adev_ret,
 					 const char **name_ret);
 
-int skl_int3472_register_clock(struct int3472_discrete_device *int3472);
+int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
+			       struct acpi_resource_gpio *agpio);
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 38b1372e0745..b7752c2b798d 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -2,8 +2,6 @@
 /* Author: Dan Scally <djrscally@gmail.com> */
 
 #include <linux/acpi.h>
-#include <linux/clkdev.h>
-#include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/machine.h>
@@ -154,24 +152,6 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 	return 0;
 }
 
-static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
-				       struct acpi_resource_gpio *agpio)
-{
-	char *path = agpio->resource_source.string_ptr;
-	u16 pin = agpio->pin_table[0];
-	struct gpio_desc *gpio;
-
-	gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
-	if (IS_ERR(gpio))
-		return (PTR_ERR(gpio));
-
-	int3472->clock.ena_gpio = gpio;
-	/* Ensure the pin is in output mode and non-active state */
-	gpiod_direction_output(int3472->clock.ena_gpio, 0);
-
-	return skl_int3472_register_clock(int3472);
-}
-
 static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
 {
 	switch (type) {
@@ -277,9 +257,9 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-		ret = skl_int3472_map_gpio_to_clk(int3472, agpio);
+		ret = skl_int3472_register_clock(int3472, agpio);
 		if (ret)
-			err_msg = "Failed to map GPIO to clock\n";
+			err_msg = "Failed to register clock\n";
 
 		break;
 	case INT3472_GPIO_TYPE_PRIVACY_LED:
@@ -342,11 +322,7 @@ static int skl_int3472_discrete_remove(struct platform_device *pdev)
 
 	gpiod_remove_lookup_table(&int3472->gpios);
 
-	if (int3472->clock.cl)
-		skl_int3472_unregister_clock(int3472);
-
-	gpiod_put(int3472->clock.ena_gpio);
-
+	skl_int3472_unregister_clock(int3472);
 	skl_int3472_unregister_pled(int3472);
 	skl_int3472_unregister_regulator(int3472);
 
-- 
2.39.0

[PATCH v4 11/11] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

[Hans de Goede]

According to:
https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch

Bits 31-24 of the _DSM pin entry integer value codes the active-value,
that is the actual physical signal (0 or 1) which needs to be output on
the pin to turn the sensor chip on (to make it active).

So if bits 31-24 are 0 for a reset pin, then the actual value of the reset
pin needs to be 0 to take the chip out of reset. IOW in this case the reset
signal is active-high rather then the default active-low.

And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk
pin needs to be 0 to enable the clk. So in this case the clk-en signal
is active-low rather then the default active-high.

IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin
is inverted.

Add a check for this and also propagate this new polarity to the clock
registration.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../platform/x86/intel/int3472/clk_and_regulator.c  |  5 ++++-
 drivers/platform/x86/intel/int3472/common.h         |  2 +-
 drivers/platform/x86/intel/int3472/discrete.c       | 13 +++++++++++--
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 626e5e86f4e0..1086c3d83494 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -87,7 +87,7 @@ static const struct clk_ops skl_int3472_clock_ops = {
 };
 
 int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
-			       struct acpi_resource_gpio *agpio)
+			       struct acpi_resource_gpio *agpio, u32 polarity)
 {
 	char *path = agpio->resource_source.string_ptr;
 	struct clk_init_data init = {
@@ -105,6 +105,9 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
 		return dev_err_probe(int3472->dev, PTR_ERR(int3472->clock.ena_gpio),
 				     "getting clk-enable GPIO\n");
 
+	if (polarity == GPIO_ACTIVE_LOW)
+		gpiod_toggle_active_low(int3472->clock.ena_gpio);
+
 	/* Ensure the pin is in output mode and non-active state */
 	gpiod_direction_output(int3472->clock.ena_gpio, 0);
 
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 0d4fa7d00b5f..61688e450ce5 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -122,7 +122,7 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 					 const char **name_ret);
 
 int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
-			       struct acpi_resource_gpio *agpio);
+			       struct acpi_resource_gpio *agpio, u32 polarity);
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index b7752c2b798d..96963e30ab6c 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -220,11 +220,11 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	struct int3472_discrete_device *int3472 = data;
 	struct acpi_resource_gpio *agpio;
 	union acpi_object *obj;
+	u8 active_value, type;
 	const char *err_msg;
 	const char *func;
 	u32 polarity;
 	int ret;
-	u8 type;
 
 	if (!acpi_gpio_get_io_resource(ares, &agpio))
 		return 1;
@@ -248,6 +248,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	int3472_get_func_and_polarity(type, &func, &polarity);
 
+	/* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */
+	active_value = obj->integer.value >> 24;
+	if (!active_value)
+		polarity ^= GPIO_ACTIVE_LOW;
+
+	dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func,
+		agpio->resource_source.string_ptr, agpio->pin_table[0],
+		(polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
+
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
 	case INT3472_GPIO_TYPE_POWERDOWN:
@@ -257,7 +266,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-		ret = skl_int3472_register_clock(int3472, agpio);
+		ret = skl_int3472_register_clock(int3472, agpio, polarity);
 		if (ret)
 			err_msg = "Failed to register clock\n";
 
-- 
2.39.0

Re: [PATCH v4 01/11] leds: led-class: Add missing put_device() to led_put()

[Andy Shevchenko]

On Thu, Jan 19, 2023 at 3: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.

This sounds to me like a bugfix. Why not the Fixes tag?
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/leds/led-class.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 6a8ea94834fa..7391d2cf1370 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -241,8 +241,10 @@ struct led_classdev *of_led_get(struct device_node *np, int index)
>
>         led_cdev = dev_get_drvdata(led_dev);
>
> -       if (!try_module_get(led_cdev->dev->parent->driver->owner))
> +       if (!try_module_get(led_cdev->dev->parent->driver->owner)) {
> +               put_device(led_cdev->dev);
>                 return ERR_PTR(-ENODEV);
> +       }
>
>         return led_cdev;
>  }
> @@ -255,6 +257,7 @@ EXPORT_SYMBOL_GPL(of_led_get);
>  void led_put(struct led_classdev *led_cdev)
>  {
>         module_put(led_cdev->dev->parent->driver->owner);
> +       put_device(led_cdev->dev);
>  }
>  EXPORT_SYMBOL_GPL(led_put);
>
> --
> 2.39.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 02/11] leds: led-class: Add led_module_get() helper

[Andy Shevchenko]

On Thu, Jan 19, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Split out part of of_led_get() into a generic led_module_get() helper
> function.
>
> This is a preparation patch for adding a generic (non devicetree specific)
> led_get() function.

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

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> - Rename helper from __led_get() to led_module_get()
> ---
>  drivers/leds/led-class.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 7391d2cf1370..743d97b082dc 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -215,6 +215,23 @@ static int led_resume(struct device *dev)
>
>  static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
>
> +static struct led_classdev *led_module_get(struct device *led_dev)
> +{
> +       struct led_classdev *led_cdev;
> +
> +       if (!led_dev)
> +               return ERR_PTR(-EPROBE_DEFER);
> +
> +       led_cdev = dev_get_drvdata(led_dev);
> +
> +       if (!try_module_get(led_cdev->dev->parent->driver->owner)) {
> +               put_device(led_cdev->dev);
> +               return ERR_PTR(-ENODEV);
> +       }
> +
> +       return led_cdev;
> +}
> +
>  /**
>   * of_led_get() - request a LED device via the LED framework
>   * @np: device node to get the LED device from
> @@ -226,7 +243,6 @@ static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
>  struct led_classdev *of_led_get(struct device_node *np, int index)
>  {
>         struct device *led_dev;
> -       struct led_classdev *led_cdev;
>         struct device_node *led_node;
>
>         led_node = of_parse_phandle(np, "leds", index);
> @@ -236,17 +252,7 @@ struct led_classdev *of_led_get(struct device_node *np, int index)
>         led_dev = class_find_device_by_of_node(leds_class, led_node);
>         of_node_put(led_node);
>
> -       if (!led_dev)
> -               return ERR_PTR(-EPROBE_DEFER);
> -
> -       led_cdev = dev_get_drvdata(led_dev);
> -
> -       if (!try_module_get(led_cdev->dev->parent->driver->owner)) {
> -               put_device(led_cdev->dev);
> -               return ERR_PTR(-ENODEV);
> -       }
> -
> -       return led_cdev;
> +       return led_module_get(led_dev);
>  }
>  EXPORT_SYMBOL_GPL(of_led_get);
>
> --
> 2.39.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 03/11] leds: led-class: Add __devm_led_get() helper

[Andy Shevchenko]

On Thu, Jan 19, 2023 at 3: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.

It's just a move of the existing code...
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
...but I would expect that someone converts this to use
devm_add_action_or_reset().

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/leds/led-class.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 743d97b082dc..4904d140a560 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -274,6 +274,22 @@ static void devm_led_release(struct device *dev, void *res)
>         led_put(*p);
>  }
>
> +static struct led_classdev *__devm_led_get(struct device *dev, struct led_classdev *led)
> +{
> +       struct led_classdev **dr;
> +
> +       dr = devres_alloc(devm_led_release, sizeof(struct led_classdev *), GFP_KERNEL);
> +       if (!dr) {
> +               led_put(led);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       *dr = led;
> +       devres_add(dev, dr);
> +
> +       return led;
> +}
> +
>  /**
>   * devm_of_led_get - Resource-managed request of a LED device
>   * @dev:       LED consumer
> @@ -289,7 +305,6 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
>                                                   int index)
>  {
>         struct led_classdev *led;
> -       struct led_classdev **dr;
>
>         if (!dev)
>                 return ERR_PTR(-EINVAL);
> @@ -298,17 +313,7 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
>         if (IS_ERR(led))
>                 return led;
>
> -       dr = devres_alloc(devm_led_release, sizeof(struct led_classdev *),
> -                         GFP_KERNEL);
> -       if (!dr) {
> -               led_put(led);
> -               return ERR_PTR(-ENOMEM);
> -       }
> -
> -       *dr = led;
> -       devres_add(dev, dr);
> -
> -       return led;
> +       return __devm_led_get(dev, led);
>  }
>  EXPORT_SYMBOL_GPL(devm_of_led_get);
>
> --
> 2.39.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 05/11] [RFC] leds: led-class: Add devicetree support to led_get()

[Andy Shevchenko]

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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/11] leds: led-class: Add generic [devm_]led_get()

[Andy Shevchenko]

On Thu, Jan 19, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add a generic [devm_]led_get() method which can be used on both devicetree
> and non devicetree platforms to get a LED classdev associated with
> a specific function on a specific device, e.g. the privacy LED associated
> with a specific camera sensor.
>
> Note unlike of_led_get() this takes a string describing the function
> rather then an index. This is done because e.g. camera sensors might

than

> have a privacy LED, or a flash LED, or both and using an index
> approach leaves it unclear what the function of index 0 is if there is
> only 1 LED.
>
> This uses a lookup-table mechanism for non devicetree platforms.
> This allows the platform code to map specific LED class_dev-s to a specific
> device,function combinations this way.
>
> For devicetree platforms getting the LED by function-name could be made
> to work using the standard devicetree pattern of adding a -names string
> array to map names to the indexes.

...

> +/*
> + * This is used to tell led_get() device which led_classdev to return for
> + * a specific consumer device-name, function pair on non devicetree platforms.
> + * Note all strings must be set.
> + */
> +struct led_lookup_data {
> +       struct list_head list;
> +       const char *led_name;
> +       const char *consumer_dev_name;
> +       const char *consumer_function;
> +};

I'm wondering if it would be better to have something like

struct led_function_map {
       const char *name;
       const char *function;
};

struct led_lookup_data {
       struct list_head list;
       const char *dev_name;
       const struct led_function_map map[];
};

as you may have more than one LED per the device and it would be a
more compressed list in this case. I'm basically referring to the GPIO
lookup table.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/11] leds: led-class: Add generic [devm_]led_get()

[Hans de Goede]

Hi,

On 1/19/23 15:04, Andy Shevchenko wrote:
> On Thu, Jan 19, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Add a generic [devm_]led_get() method which can be used on both devicetree
>> and non devicetree platforms to get a LED classdev associated with
>> a specific function on a specific device, e.g. the privacy LED associated
>> with a specific camera sensor.
>>
>> Note unlike of_led_get() this takes a string describing the function
>> rather then an index. This is done because e.g. camera sensors might
> 
> than
> 
>> have a privacy LED, or a flash LED, or both and using an index
>> approach leaves it unclear what the function of index 0 is if there is
>> only 1 LED.
>>
>> This uses a lookup-table mechanism for non devicetree platforms.
>> This allows the platform code to map specific LED class_dev-s to a specific
>> device,function combinations this way.
>>
>> For devicetree platforms getting the LED by function-name could be made
>> to work using the standard devicetree pattern of adding a -names string
>> array to map names to the indexes.
> 
> ...
> 
>> +/*
>> + * This is used to tell led_get() device which led_classdev to return for
>> + * a specific consumer device-name, function pair on non devicetree platforms.
>> + * Note all strings must be set.
>> + */
>> +struct led_lookup_data {
>> +       struct list_head list;
>> +       const char *led_name;
>> +       const char *consumer_dev_name;
>> +       const char *consumer_function;
>> +};
> 
> I'm wondering if it would be better to have something like
> 
> struct led_function_map {
>        const char *name;
>        const char *function;
> };
> 
> struct led_lookup_data {
>        struct list_head list;
>        const char *dev_name;
>        const struct led_function_map map[];
> };

Thank you for the review.

Since led_lookup_data now is variable sized, AFAIK this means that
the led_lookup_data now can no longer be embedded in another struct and
instead it must always be dynamically allocated, including adding error
checking + rollback for said allocation.

If you look at the only current consumer of this:

[PATCH v4 09/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

then the code there would become more complicated.

> as you may have more than one LED per the device and it would be a
> more compressed list in this case. I'm basically referring to the GPIO
> lookup table.

Right, but having more then 1 GPIO per device is quite common while
I expect having more then 1 (or maybe 2) LEDs per device to be rare while
at the same time the suggested change makes things slightly more
complicated for consumers of the API which know before hand how much
lookup entries they will need (typically 1).

So all in all I believe staying with the current implementation is better
but if there is a strong preference to switch to the structure you suggest
then I have no objection against that.

Regards,

Hans

Re: [PATCH v4 04/11] leds: led-class: Add generic [devm_]led_get()

[Andy Shevchenko]

On Thu, Jan 19, 2023 at 4:16 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 1/19/23 15:04, Andy Shevchenko wrote:
> > On Thu, Jan 19, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >> +/*
> >> + * This is used to tell led_get() device which led_classdev to return for
> >> + * a specific consumer device-name, function pair on non devicetree platforms.
> >> + * Note all strings must be set.
> >> + */
> >> +struct led_lookup_data {
> >> +       struct list_head list;
> >> +       const char *led_name;
> >> +       const char *consumer_dev_name;
> >> +       const char *consumer_function;
> >> +};
> >
> > I'm wondering if it would be better to have something like
> >
> > struct led_function_map {
> >        const char *name;
> >        const char *function;
> > };
> >
> > struct led_lookup_data {
> >        struct list_head list;
> >        const char *dev_name;
> >        const struct led_function_map map[];
> > };
>
> Thank you for the review.
>
> Since led_lookup_data now is variable sized, AFAIK this means that
> the led_lookup_data now can no longer be embedded in another struct and
> instead it must always be dynamically allocated, including adding error
> checking + rollback for said allocation.

I'm not sure what you are talking about here. GPIO lookup table
defined in the same way and doesn't strictly require heap allocation.
For the embedding into another structure, it can be as the last entry AFAIU.

> If you look at the only current consumer of this:
>
> [PATCH v4 09/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED
>
> then the code there would become more complicated.

> > as you may have more than one LED per the device and it would be a
> > more compressed list in this case. I'm basically referring to the GPIO
> > lookup table.
>
> Right, but having more then 1 GPIO per device is quite common while
> I expect having more then 1 (or maybe 2) LEDs per device to be rare while
> at the same time the suggested change makes things slightly more
> complicated for consumers of the API which know before hand how much
> lookup entries they will need (typically 1).
>
> So all in all I believe staying with the current implementation is better
> but if there is a strong preference to switch to the structure you suggest
> then I have no objection against that.

I have no strong opinion, I just want to have fewer variants of the
lookup tables.
Anyway, reset framework has something similar to yours. Question: can you
rename fields to be something like dev_id, con_id, etc as it's done in the most
of the lookup data types?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/11] leds: led-class: Add generic [devm_]led_get()

[Hans de Goede]

Hi,

On 1/19/23 15:54, Andy Shevchenko wrote:
> On Thu, Jan 19, 2023 at 4:16 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 1/19/23 15:04, Andy Shevchenko wrote:
>>> On Thu, Jan 19, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>> +/*
>>>> + * This is used to tell led_get() device which led_classdev to return for
>>>> + * a specific consumer device-name, function pair on non devicetree platforms.
>>>> + * Note all strings must be set.
>>>> + */
>>>> +struct led_lookup_data {
>>>> +       struct list_head list;
>>>> +       const char *led_name;
>>>> +       const char *consumer_dev_name;
>>>> +       const char *consumer_function;
>>>> +};
>>>
>>> I'm wondering if it would be better to have something like
>>>
>>> struct led_function_map {
>>>        const char *name;
>>>        const char *function;
>>> };
>>>
>>> struct led_lookup_data {
>>>        struct list_head list;
>>>        const char *dev_name;
>>>        const struct led_function_map map[];
>>> };
>>
>> Thank you for the review.
>>
>> Since led_lookup_data now is variable sized, AFAIK this means that
>> the led_lookup_data now can no longer be embedded in another struct and
>> instead it must always be dynamically allocated, including adding error
>> checking + rollback for said allocation.
> 
> I'm not sure what you are talking about here. GPIO lookup table
> defined in the same way and doesn't strictly require heap allocation.
> For the embedding into another structure, it can be as the last entry AFAIU.

That will probably work, but only if there is only 1 variable sized
struct which you want to embed.

Also note that in the current use-case the struct is embedded in
a sub-struct of the main driver_data struct, so then not only
would this need to be the last member of the sub-struct, but
the sub-struct itself would need to be the last member of
the main driver_data struct.

Variable sized structs can be nice sometimes, but in cases where
we may want to embed them they are not always ideal.

>> If you look at the only current consumer of this:
>>
>> [PATCH v4 09/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED
>>
>> then the code there would become more complicated.
> 
>>> as you may have more than one LED per the device and it would be a
>>> more compressed list in this case. I'm basically referring to the GPIO
>>> lookup table.
>>
>> Right, but having more then 1 GPIO per device is quite common while
>> I expect having more then 1 (or maybe 2) LEDs per device to be rare while
>> at the same time the suggested change makes things slightly more
>> complicated for consumers of the API which know before hand how much
>> lookup entries they will need (typically 1).
>>
>> So all in all I believe staying with the current implementation is better
>> but if there is a strong preference to switch to the structure you suggest
>> then I have no objection against that.
> 
> I have no strong opinion, I just want to have fewer variants of the
> lookup tables.
> Anyway, reset framework has something similar to yours.

Right, so there is precedent for this variant too.

> Question: can you
> rename fields to be something like dev_id, con_id, etc as it's done in the most
> of the lookup data types?

I see that the gpio and reset lookups indeed both use dev_id and con_id
I will change the LED lookups to use this to for version 5 of this
patch-set.

Regards,

Hans

Re: [PATCH v4 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko

[kernel test robot]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc4]
[cannot apply to media-tree/master pavel-leds/for-next next-20230119]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/leds-led-class-Add-missing-put_device-to-led_put/20230119-210441
patch link:    https://lore.kernel.org/r/20230119130053.111344-7-hdegoede%40redhat.com
patch subject: [PATCH v4 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230120/202301200320.AbLvd1xh-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/adfeffb48aad34dd2148e22caaf13d67cd92c285
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-de-Goede/leds-led-class-Add-missing-put_device-to-led_put/20230119-210441
        git checkout adfeffb48aad34dd2148e22caaf13d67cd92c285
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: vmlinux.o: in function `videodev_init':
>> v4l2-dev.c:(.init.text+0x4c7c5): undefined reference to `v4l2_async_debugfs_init'
   ld: vmlinux.o: in function `videodev_exit':
>> v4l2-dev.c:(.exit.text+0x1f95): undefined reference to `v4l2_async_debugfs_exit'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v4 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko

[kernel test robot]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc4]
[cannot apply to media-tree/master pavel-leds/for-next next-20230119]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/leds-led-class-Add-missing-put_device-to-led_put/20230119-210441
patch link:    https://lore.kernel.org/r/20230119130053.111344-7-hdegoede%40redhat.com
patch subject: [PATCH v4 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko
config: i386-randconfig-a004 (https://download.01.org/0day-ci/archive/20230120/202301200441.zHWxHnG4-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/adfeffb48aad34dd2148e22caaf13d67cd92c285
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-de-Goede/leds-led-class-Add-missing-put_device-to-led_put/20230119-210441
        git checkout adfeffb48aad34dd2148e22caaf13d67cd92c285
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: v4l2_async_debugfs_exit
   >>> referenced by v4l2-dev.c:1202 (drivers/media/v4l2-core/v4l2-dev.c:1202)
   >>>               drivers/media/v4l2-core/v4l2-dev.o:(videodev_exit) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: v4l2_async_debugfs_init
   >>> referenced by v4l2-dev.c:1194 (drivers/media/v4l2-core/v4l2-dev.c:1194)
   >>>               drivers/media/v4l2-core/v4l2-dev.o:(videodev_init) in archive vmlinux.a

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v4 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko

[kernel test robot]

Hi Hans,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc4]
[cannot apply to media-tree/master pavel-leds/for-next next-20230119]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/leds-led-class-Add-missing-put_device-to-led_put/20230119-210441
patch link:    https://lore.kernel.org/r/20230119130053.111344-7-hdegoede%40redhat.com
patch subject: [PATCH v4 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko
config: x86_64-rhel-8.3-syz (https://download.01.org/0day-ci/archive/20230120/202301200413.64Gyz6yI-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/adfeffb48aad34dd2148e22caaf13d67cd92c285
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-de-Goede/leds-led-class-Add-missing-put_device-to-led_put/20230119-210441
        git checkout adfeffb48aad34dd2148e22caaf13d67cd92c285
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "v4l2_async_debugfs_exit" [drivers/media/v4l2-core/videodev.ko] undefined!
>> ERROR: modpost: "v4l2_async_debugfs_init" [drivers/media/v4l2-core/videodev.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v4 01/11] leds: led-class: Add missing put_device() to led_put()

[Linus Walleij]

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

Re: [PATCH v4 02/11] leds: led-class: Add led_module_get() helper

[Linus Walleij]

On Thu, Jan 19, 2023 at 2:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Split out part of of_led_get() into a generic led_module_get() helper
> function.
>
> This is a preparation patch for adding a generic (non devicetree specific)
> led_get() function.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> - Rename helper from __led_get() to led_module_get()

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

Yours,
Linus Walleij

Re: [PATCH v4 03/11] leds: led-class: Add __devm_led_get() helper

[Linus Walleij]

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

Re: [PATCH v4 04/11] leds: led-class: Add generic [devm_]led_get()

[Linus Walleij]

On Thu, Jan 19, 2023 at 2:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Add a generic [devm_]led_get() method which can be used on both devicetree
> and non devicetree platforms to get a LED classdev associated with
> a specific function on a specific device, e.g. the privacy LED associated
> with a specific camera sensor.
>
> Note unlike of_led_get() this takes a string describing the function
> rather then an index. This is done because e.g. camera sensors might
> have a privacy LED, or a flash LED, or both and using an index
> approach leaves it unclear what the function of index 0 is if there is
> only 1 LED.
>
> This uses a lookup-table mechanism for non devicetree platforms.
> This allows the platform code to map specific LED class_dev-s to a specific
> device,function combinations this way.
>
> For devicetree platforms getting the LED by function-name could be made
> to work using the standard devicetree pattern of adding a -names string
> array to map names to the indexes.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> - Split out support for led_get() devicetree name-based lookup support
>   into a separate RFC patch as there currently are no user for this
> - Use kstrdup_const() / kfree_const() for the led_name

This is how I would implement it so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4 05/11] [RFC] leds: led-class: Add devicetree support to led_get()

[Linus Walleij]

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

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

[Linus Walleij]

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

Re: [PATCH v4 04/11] leds: led-class: Add generic [devm_]led_get()

[Lee Jones]

On Fri, 20 Jan 2023, Linus Walleij wrote:

> On Thu, Jan 19, 2023 at 2:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> > Add a generic [devm_]led_get() method which can be used on both devicetree
> > and non devicetree platforms to get a LED classdev associated with
> > a specific function on a specific device, e.g. the privacy LED associated
> > with a specific camera sensor.
> >
> > Note unlike of_led_get() this takes a string describing the function
> > rather then an index. This is done because e.g. camera sensors might
> > have a privacy LED, or a flash LED, or both and using an index
> > approach leaves it unclear what the function of index 0 is if there is
> > only 1 LED.
> >
> > This uses a lookup-table mechanism for non devicetree platforms.
> > This allows the platform code to map specific LED class_dev-s to a specific
> > device,function combinations this way.
> >
> > For devicetree platforms getting the LED by function-name could be made
> > to work using the standard devicetree pattern of adding a -names string
> > array to map names to the indexes.
> >
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > Changes in v4:
> > - Split out support for led_get() devicetree name-based lookup support
> >   into a separate RFC patch as there currently are no user for this
> > - Use kstrdup_const() / kfree_const() for the led_name
> 
> This is how I would implement it so:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks Linus, this is all really helpful.

-- 
Lee Jones [李琼斯]

Re: [PATCH v4 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko

[Hans de Goede]

Hi,

On 1/19/23 20:47, kernel test robot wrote:
> Hi Hans,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v6.2-rc4]
> [cannot apply to media-tree/master pavel-leds/for-next next-20230119]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/leds-led-class-Add-missing-put_device-to-led_put/20230119-210441
> patch link:    https://lore.kernel.org/r/20230119130053.111344-7-hdegoede%40redhat.com
> patch subject: [PATCH v4 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko
> config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230120/202301200320.AbLvd1xh-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/intel-lab-lkp/linux/commit/adfeffb48aad34dd2148e22caaf13d67cd92c285
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Hans-de-Goede/leds-led-class-Add-missing-put_device-to-led_put/20230119-210441
>         git checkout adfeffb48aad34dd2148e22caaf13d67cd92c285
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         make W=1 O=build_dir ARCH=x86_64 olddefconfig
>         make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    ld: vmlinux.o: in function `videodev_init':
>>> v4l2-dev.c:(.init.text+0x4c7c5): undefined reference to `v4l2_async_debugfs_init'
>    ld: vmlinux.o: in function `videodev_exit':
>>> v4l2-dev.c:(.exit.text+0x1f95): undefined reference to `v4l2_async_debugfs_exit'


Right, v4l2_async_debugfs_init + v4l2_async_debugfs_exit need
static inline stubs for when CONFIG_V4L2_ASYNC is not set,
I will fix this in the next version.

Regards,

Hans