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

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

[Hans de Goede]

Hi All,

Here is version 5 of my series to adjust the INT3472 code's handling of
the privacy LED on x86 laptops with MIPI camera(s) so that it will also
work on devices which have a privacy-LED GPIO but not a clk-enable GPIO
(so that we cannot just tie the LED state to the clk-enable state).

Changes in v5:
- Rename lookup-table names to match those from the gpio and reset lookups:
  s/led_name/provider/
  s/consumer_dev_name/dev_id/
  s/consumer_function/con_id/
- Add static inline wrappers for the v4l2_async debugfs init/exit funcs,
  to fix build errors when CONFIG_V4L2_ASYNC is not enabled

Changes in v4:
- Rename new __led_get() helper to led_module_get()
- Drop of/devicetree support from "led-class: Add generic [devm_]led_get()"
- Add RFC patch to re-add of/devicetree support to show that the new
  led_get() can easily be extended with dt support when the need for this
  arises (proof-of-concept dt code, not intended for merging)
- New patch to built async and fwnode code into videodev.ko,
  to avoid issues with some of the new LED code getting builtin vs
  other parts possibly being in a module
- Move the led_get() call to v4l2_async_register_subdev_sensor()
- Move the led_disable_sysfs() call to be done at led_get() time
- Address some other minor review comments

Changes in v3:
- Due to popular request by multiple people this new version now models
  the privacy LED as a LED class device. This requires being able to
  "tie" the LED class device to a specific camera sensor (some devices
  have multiple sensors + privacy-LEDs).

Patches 1-5 are LED subsystem patches for this. 1 is a bug fix, 2-4 add
the new [devm_]led_get() functions. Patch 5 is the RFC patch adding dt
support to led_get() and is not intended for merging.

Patch 6 + 7 add generic privacy-LED support to the v4l2-core/v4l2-subdev.c
code automatically enabling the privacy-LED when s_stream(subdev, 1)
is called. So that we don't need to add privacy-LED code to all the
camera sensor drivers separately (as requested by Sakari).

Patches 8-11 are patches to the platform specific INT3472 code to register
privacy-LED class devices + lookup table entries for privacy-LEDs described
in the special INT3472 ACPI nodes found on x86 devices with MIPI cameras.

Assuming at least the LED maintainers are happy with the approach suggested
here, the first step to merging this would be to merge patches 1-4 and then
provide an immutable branch with those to merge for the other subsystems
since the other changes depend on these.

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          |  17 +-
 drivers/media/v4l2-core/v4l2-dev-priv.h       |  19 ++
 drivers/media/v4l2-core/v4l2-dev.c            |   8 +
 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     |  34 +++-
 drivers/platform/x86/intel/int3472/common.h   |  18 +-
 drivers/platform/x86/intel/int3472/discrete.c | 100 +++++-----
 drivers/platform/x86/intel/int3472/led.c      |  74 ++++++++
 include/linux/leds.h                          |  21 +++
 include/media/v4l2-subdev.h                   |   3 +
 15 files changed, 399 insertions(+), 117 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-dev-priv.h
 create mode 100644 drivers/platform/x86/intel/int3472/led.c

-- 
2.39.0

[PATCH v5 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.

Fixes: 699a8c7c4bd3 ("leds: Add of_led_get() and led_put()")
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
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

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

[Lee Jones]

On Fri, 20 Jan 2023, Hans de Goede 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.
> 
> Fixes: 699a8c7c4bd3 ("leds: Add of_led_get() and led_put()")
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/leds/led-class.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH v5 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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
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

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

[Lee Jones]

On Fri, 20 Jan 2023, Hans de Goede 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>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 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(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH v5 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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
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

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

[Lee Jones]

On Fri, 20 Jan 2023, Hans de Goede 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.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/leds/led-class.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH v5 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.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
- Rename lookup-table names to match those from the gpio and reset lookups:
  s/led_name/provider/
  s/consumer_dev_name/dev_id/
  s/consumer_function/con_id/

Changes in v4:
- Split out support for led_get() devicetree name-based lookup support
  into a separate RFC patch as there currently are no users for this
- Use kstrdup_const() / kfree_const() for the led_name
---
 drivers/leds/led-class.c | 84 ++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     | 21 ++++++++++
 2 files changed, 105 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 4904d140a560..0c4b8d8d2b4f 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
+ * @con_id: name of the LED from the device's point of view
+ *
+ * @return a pointer to a LED device or ERR_PTR(errno) on failure.
+ */
+struct led_classdev *led_get(struct device *dev, char *con_id)
+{
+	struct led_lookup_data *lookup;
+	const char *provider = NULL;
+	struct device *led_dev;
+
+	mutex_lock(&leds_lookup_lock);
+	list_for_each_entry(lookup, &leds_lookup_list, list) {
+		if (!strcmp(lookup->dev_id, dev_name(dev)) &&
+		    !strcmp(lookup->con_id, con_id)) {
+			provider = kstrdup_const(lookup->provider, GFP_KERNEL);
+			break;
+		}
+	}
+	mutex_unlock(&leds_lookup_lock);
+
+	if (!provider)
+		return ERR_PTR(-ENOENT);
+
+	led_dev = class_find_device_by_name(leds_class, provider);
+	kfree_const(provider);
+
+	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
+ * @con_id: name of the LED from the device's point of view
+ *
+ * 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 *con_id)
+{
+	struct led_classdev *led;
+
+	led = led_get(dev, con_id);
+	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..8d6767072ebf 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -39,6 +39,21 @@ enum led_default_state {
 	LEDS_DEFSTATE_KEEP	= 2,
 };
 
+/**
+ * struct led_lookup_data - represents a single LED lookup entry
+ *
+ * @list: internal list of all LED lookup entries
+ * @provider: name of led_classdev providing the LED
+ * @dev_id: name of the device associated with this LED
+ * @con_id: name of the LED from the device's point of view
+ */
+struct led_lookup_data {
+	struct list_head list;
+	const char *provider;
+	const char *dev_id;
+	const char *con_id;
+};
+
 struct led_init_data {
 	/* device fwnode handle */
 	struct fwnode_handle *fwnode;
@@ -211,6 +226,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 *con_id);
+struct led_classdev *__must_check devm_led_get(struct device *dev, char *con_id);
+
 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

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

[Lee Jones]

On Fri, 20 Jan 2023, Hans de Goede 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.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v5:
> - Rename lookup-table names to match those from the gpio and reset lookups:
>   s/led_name/provider/
>   s/consumer_dev_name/dev_id/
>   s/consumer_function/con_id/
> 
> Changes in v4:
> - Split out support for led_get() devicetree name-based lookup support
>   into a separate RFC patch as there currently are no users for this
> - Use kstrdup_const() / kfree_const() for the led_name
> ---
>  drivers/leds/led-class.c | 84 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/leds.h     | 21 ++++++++++
>  2 files changed, 105 insertions(+)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH v5 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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
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 0c4b8d8d2b4f..2f3af6e30208 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 *con_id)
 {
 	struct led_lookup_data *lookup;
+	struct led_classdev *led_cdev;
 	const char *provider = NULL;
 	struct device *led_dev;
 
+	if (dev->of_node) {
+		led_cdev = __of_led_get(dev->of_node, -1, con_id);
+		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->dev_id, dev_name(dev)) &&
-- 
2.39.0

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

[Lee Jones]

On Fri, 20 Jan 2023, Hans de Goede wrote:

> Turn of_led_get() into a more generic __of_led_get() helper function,
> which can lookup LEDs in devicetree by either name or index.
> 
> 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.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/leds/led-class.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)

Reviewed-by: Lee Jones <lee@kernel.org>

> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 0c4b8d8d2b4f..2f3af6e30208 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 *con_id)
>  {
>  	struct led_lookup_data *lookup;
> +	struct led_classdev *led_cdev;
>  	const char *provider = NULL;
>  	struct device *led_dev;
>  
> +	if (dev->of_node) {
> +		led_cdev = __of_led_get(dev->of_node, -1, con_id);
> +		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->dev_id, dev_name(dev)) &&
> -- 
> 2.39.0
> 

-- 
Lee Jones [李琼斯]

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

[Aleksandrs Vinarskis]

On 1/20/23 12:45, Hans de Goede wrote:
> Turn of_led_get() into a more generic __of_led_get() helper function,
> which can lookup LEDs in devicetree by either name or index.
>
> 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.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>


Hi,

It appears this patch was marked as RFC due some missing dt-bindings and 
not having direct DT consumers at the time, and was eventually left out. 
With recent inflow of arm64-power laptops (Snapdragon X1E/X1P) which 
mostly use MIPI cameras, this feature becomes more desired. I have 
rebased this patch onto 6.17-rc2, and can confirm its (still) working as 
expected (with respective DT changes) on Dell XPS 9345.

What would be the best approach to revive this patch? For Hans to respin 
this? Alternatively I could respin it myself keeping the original 
authorship.
Regarding missing dt-binding documentation, would 
`Documentation/devicetree/bindings/leds/common.yaml` be the good place 
for it? Afaiu it was mentioned that no  appropriate LED bindings exists 
in this series (3yo), but this binding is ~6yo, so perhaps its not a 
right place after all.

Thank you in advance,
Alex

> ---
>   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 0c4b8d8d2b4f..2f3af6e30208 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 *con_id)
>   {
>   	struct led_lookup_data *lookup;
> +	struct led_classdev *led_cdev;
>   	const char *provider = NULL;
>   	struct device *led_dev;
>   
> +	if (dev->of_node) {
> +		led_cdev = __of_led_get(dev->of_node, -1, con_id);
> +		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->dev_id, dev_name(dev)) &&

[PATCH v5 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.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
- Add a new v4l2-dev-priv.h for the async debugfs prototypes and add
  static inline wrappers there when CONFIG_V4L2_ASYNC is not enabled

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    | 17 ++++-------------
 drivers/media/v4l2-core/v4l2-dev-priv.h | 19 +++++++++++++++++++
 drivers/media/v4l2-core/v4l2-dev.c      |  8 ++++++++
 drivers/media/v4l2-core/v4l2-fwnode.c   |  6 ------
 6 files changed, 35 insertions(+), 23 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-dev-priv.h

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..024d6b82b50a 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>
@@ -24,6 +23,8 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
+#include "v4l2-dev-priv.h"
+
 static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
 				    struct v4l2_subdev *subdev,
 				    struct v4l2_async_subdev *asd)
@@ -900,25 +901,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-priv.h b/drivers/media/v4l2-core/v4l2-dev-priv.h
new file mode 100644
index 000000000000..b5b1ee78be20
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-dev-priv.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Video capture interface for Linux version 2 private header.
+ *
+ * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
+ */
+
+#ifndef _V4L2_DEV_PRIV_H_
+#define _V4L2_DEV_PRIV_H_
+
+#if IS_ENABLED(CONFIG_V4L2_ASYNC)
+void v4l2_async_debugfs_init(void);
+void v4l2_async_debugfs_exit(void);
+#else
+static inline void v4l2_async_debugfs_init(void) {}
+static inline void v4l2_async_debugfs_exit(void) {}
+#endif
+
+#endif
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 397d553177fa..10ba2e4196a6 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -31,6 +31,8 @@
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-event.h>
 
+#include "v4l2-dev-priv.h"
+
 #define VIDEO_NUM_DEVICES	256
 #define VIDEO_NAME              "video4linux"
 
@@ -1190,6 +1192,7 @@ static int __init videodev_init(void)
 		return -EIO;
 	}
 
+	v4l2_async_debugfs_init();
 	return 0;
 }
 
@@ -1197,6 +1200,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 +1209,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>");
-- 
2.39.0

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

[Sakari Ailus]

Hi Hans,

Many thanks for working on this.

On Fri, Jan 20, 2023 at 12:45:19PM +0100, Hans de Goede wrote:
> 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.
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v5:
> - Add a new v4l2-dev-priv.h for the async debugfs prototypes and add
>   static inline wrappers there when CONFIG_V4L2_ASYNC is not enabled
> 
> 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    | 17 ++++-------------
>  drivers/media/v4l2-core/v4l2-dev-priv.h | 19 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-dev.c      |  8 ++++++++
>  drivers/media/v4l2-core/v4l2-fwnode.c   |  6 ------
>  6 files changed, 35 insertions(+), 23 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-dev-priv.h
> 
> 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..024d6b82b50a 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>
> @@ -24,6 +23,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "v4l2-dev-priv.h"
> +
>  static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
>  				    struct v4l2_subdev *subdev,
>  				    struct v4l2_async_subdev *asd)
> @@ -900,25 +901,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-priv.h b/drivers/media/v4l2-core/v4l2-dev-priv.h
> new file mode 100644
> index 000000000000..b5b1ee78be20
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-dev-priv.h

Could we call this v4l2-async-debugfs.h? I don't necessarily expect more
material here.

> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Video capture interface for Linux version 2 private header.
> + *
> + * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
> + */
> +
> +#ifndef _V4L2_DEV_PRIV_H_
> +#define _V4L2_DEV_PRIV_H_
> +
> +#if IS_ENABLED(CONFIG_V4L2_ASYNC)
> +void v4l2_async_debugfs_init(void);
> +void v4l2_async_debugfs_exit(void);
> +#else
> +static inline void v4l2_async_debugfs_init(void) {}
> +static inline void v4l2_async_debugfs_exit(void) {}
> +#endif
> +
> +#endif
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 397d553177fa..10ba2e4196a6 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -31,6 +31,8 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-event.h>
>  
> +#include "v4l2-dev-priv.h"
> +
>  #define VIDEO_NUM_DEVICES	256
>  #define VIDEO_NAME              "video4linux"
>  
> @@ -1190,6 +1192,7 @@ static int __init videodev_init(void)
>  		return -EIO;
>  	}
>  
> +	v4l2_async_debugfs_init();
>  	return 0;
>  }
>  
> @@ -1197,6 +1200,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 +1209,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>");
> -- 
> 2.39.0
> 

-- 
Sakari Ailus

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

[Hans de Goede]

Hi,

On 1/20/23 13:47, Sakari Ailus wrote:
> Hi Hans,
> 
> Many thanks for working on this.

You are welcome.

> On Fri, Jan 20, 2023 at 12:45:19PM +0100, Hans de Goede wrote:
>> 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.
>>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v5:
>> - Add a new v4l2-dev-priv.h for the async debugfs prototypes and add
>>   static inline wrappers there when CONFIG_V4L2_ASYNC is not enabled
>>
>> 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    | 17 ++++-------------
>>  drivers/media/v4l2-core/v4l2-dev-priv.h | 19 +++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-dev.c      |  8 ++++++++
>>  drivers/media/v4l2-core/v4l2-fwnode.c   |  6 ------
>>  6 files changed, 35 insertions(+), 23 deletions(-)
>>  create mode 100644 drivers/media/v4l2-core/v4l2-dev-priv.h
>>
>> 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..024d6b82b50a 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>
>> @@ -24,6 +23,8 @@
>>  #include <media/v4l2-fwnode.h>
>>  #include <media/v4l2-subdev.h>
>>  
>> +#include "v4l2-dev-priv.h"
>> +
>>  static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
>>  				    struct v4l2_subdev *subdev,
>>  				    struct v4l2_async_subdev *asd)
>> @@ -900,25 +901,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-priv.h b/drivers/media/v4l2-core/v4l2-dev-priv.h
>> new file mode 100644
>> index 000000000000..b5b1ee78be20
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-dev-priv.h
> 
> Could we call this v4l2-async-debugfs.h? I don't necessarily expect more
> material here.

Renaming it is fine by my. I have renamed it to
v4l2-async-debugfs.h for the upcoming v6 of this series.

Regards,

Hans



> 
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Video capture interface for Linux version 2 private header.
>> + *
>> + * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
>> + */
>> +
>> +#ifndef _V4L2_DEV_PRIV_H_
>> +#define _V4L2_DEV_PRIV_H_
>> +
>> +#if IS_ENABLED(CONFIG_V4L2_ASYNC)
>> +void v4l2_async_debugfs_init(void);
>> +void v4l2_async_debugfs_exit(void);
>> +#else
>> +static inline void v4l2_async_debugfs_init(void) {}
>> +static inline void v4l2_async_debugfs_exit(void) {}
>> +#endif
>> +
>> +#endif
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 397d553177fa..10ba2e4196a6 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -31,6 +31,8 @@
>>  #include <media/v4l2-ioctl.h>
>>  #include <media/v4l2-event.h>
>>  
>> +#include "v4l2-dev-priv.h"
>> +
>>  #define VIDEO_NUM_DEVICES	256
>>  #define VIDEO_NAME              "video4linux"
>>  
>> @@ -1190,6 +1192,7 @@ static int __init videodev_init(void)
>>  		return -EIO;
>>  	}
>>  
>> +	v4l2_async_debugfs_init();
>>  	return 0;
>>  }
>>  
>> @@ -1197,6 +1200,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 +1209,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>");
>> -- 
>> 2.39.0
>>
> 

[PATCH v5 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>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4 (requested by Laurent Pinchart):
- Move the led_get() call to v4l2_async_register_subdev_sensor() and
  make errors other then -ENOENT fail the register() call.
- Move the led_disable_sysfs() call to be done at led_get() time, instead
  of only disabling the sysfs interface when the sensor is streaming.
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 15 +++++++++++++++
 drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++++++++++++++
 include/media/v4l2-subdev.h           |  3 +++
 3 files changed, 36 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index c8a2264262bc..cfac1e2ae501 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -16,6 +16,7 @@
  */
 #include <linux/acpi.h>
 #include <linux/kernel.h>
+#include <linux/leds.h>
 #include <linux/mm.h>
 #include <linux/of.h>
 #include <linux/property.h>
@@ -1295,6 +1296,20 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
 	if (WARN_ON(!sd->dev))
 		return -ENODEV;
 
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
+	sd->privacy_led = led_get(sd->dev, "privacy-led");
+	if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
+		return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
+
+	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
+		mutex_lock(&sd->privacy_led->led_access);
+		led_sysfs_disable(sd->privacy_led);
+		led_trigger_remove(sd->privacy_led);
+		led_set_brightness(sd->privacy_led, 0);
+		mutex_unlock(&sd->privacy_led->led_access);
+	}
+#endif
+
 	notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
 	if (!notifier)
 		return -ENOMEM;
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 4988a25bd8f4..f33e943aab3f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/ioctl.h>
+#include <linux/leds.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -322,6 +323,14 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	int ret;
 
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
+	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
+		if (enable)
+			led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
+		else
+			led_set_brightness(sd->privacy_led, 0);
+	}
+#endif
 	ret = sd->ops->video->s_stream(sd, enable);
 
 	if (!enable && ret < 0) {
@@ -1050,6 +1059,14 @@ EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
 
 void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
 {
+#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

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

[Sakari Ailus]

Hi Hans,

On Fri, Jan 20, 2023 at 12:45:20PM +0100, Hans de Goede wrote:
> Make v4l2_async_register_subdev_sensor() try to get a privacy LED
> associated with the sensor and extend the call_s_stream() wrapper to
> enable/disable the privacy LED if found.
> 
> This makes the core handle privacy LED control, rather then having to
> duplicate this code in all the sensor drivers.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4 (requested by Laurent Pinchart):
> - Move the led_get() call to v4l2_async_register_subdev_sensor() and
>   make errors other then -ENOENT fail the register() call.
> - Move the led_disable_sysfs() call to be done at led_get() time, instead
>   of only disabling the sysfs interface when the sensor is streaming.
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 15 +++++++++++++++
>  drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++++++++++++++
>  include/media/v4l2-subdev.h           |  3 +++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index c8a2264262bc..cfac1e2ae501 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -16,6 +16,7 @@
>   */
>  #include <linux/acpi.h>
>  #include <linux/kernel.h>
> +#include <linux/leds.h>
>  #include <linux/mm.h>
>  #include <linux/of.h>
>  #include <linux/property.h>
> @@ -1295,6 +1296,20 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  	if (WARN_ON(!sd->dev))
>  		return -ENODEV;
>  
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	sd->privacy_led = led_get(sd->dev, "privacy-led");
> +	if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
> +		return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
> +
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		mutex_lock(&sd->privacy_led->led_access);
> +		led_sysfs_disable(sd->privacy_led);
> +		led_trigger_remove(sd->privacy_led);
> +		led_set_brightness(sd->privacy_led, 0);
> +		mutex_unlock(&sd->privacy_led->led_access);
> +	}
> +#endif
> +
>  	notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
>  	if (!notifier)
>  		return -ENOMEM;
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4988a25bd8f4..f33e943aab3f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/ioctl.h>
> +#include <linux/leds.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> @@ -322,6 +323,14 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	int ret;
>  
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		if (enable)
> +			led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
> +		else
> +			led_set_brightness(sd->privacy_led, 0);
> +	}
> +#endif
>  	ret = sd->ops->video->s_stream(sd, enable);
>  
>  	if (!enable && ret < 0) {
> @@ -1050,6 +1059,14 @@ EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
>  
>  void v4l2_subdev_cleanup(struct v4l2_subdev *sd)

v4l2_subdev_cleanup() is currently called by drivers using V4L2 subdev
state at the moment, making it unsuitable for the purpose of releasing the
privacy led.

Could you move this to v4l2_async_unregister_subdev() instead?

>  {
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		mutex_lock(&sd->privacy_led->led_access);
> +		led_sysfs_enable(sd->privacy_led);
> +		mutex_unlock(&sd->privacy_led->led_access);
> +		led_put(sd->privacy_led);
> +	}
> +#endif
>  	__v4l2_subdev_state_free(sd->active_state);
>  	sd->active_state = NULL;
>  }
> @@ -1090,6 +1107,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>  	sd->grp_id = 0;
>  	sd->dev_priv = NULL;
>  	sd->host_priv = NULL;
> +	sd->privacy_led = NULL;
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	sd->entity.name = sd->name;
>  	sd->entity.obj_type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index b15fa9930f30..0547313f98cc 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -38,6 +38,7 @@ struct v4l2_subdev;
>  struct v4l2_subdev_fh;
>  struct tuner_setup;
>  struct v4l2_mbus_frame_desc;
> +struct led_classdev;
>  
>  /**
>   * struct v4l2_decode_vbi_line - used to decode_vbi_line
> @@ -982,6 +983,8 @@ struct v4l2_subdev {
>  	 * appropriate functions.
>  	 */
>  
> +	struct led_classdev *privacy_led;
> +
>  	/*
>  	 * TODO: active_state should most likely be changed from a pointer to an
>  	 * embedded field. For the time being it's kept as a pointer to more

-- 
Kind regards,

Sakari Ailus

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

[Hans de Goede]

Hi Sakari,

On 1/20/23 13:51, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Jan 20, 2023 at 12:45:20PM +0100, Hans de Goede wrote:
>> Make v4l2_async_register_subdev_sensor() try to get a privacy LED
>> associated with the sensor and extend the call_s_stream() wrapper to
>> enable/disable the privacy LED if found.
>>
>> This makes the core handle privacy LED control, rather then having to
>> duplicate this code in all the sensor drivers.
>>
>> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v4 (requested by Laurent Pinchart):
>> - Move the led_get() call to v4l2_async_register_subdev_sensor() and
>>   make errors other then -ENOENT fail the register() call.
>> - Move the led_disable_sysfs() call to be done at led_get() time, instead
>>   of only disabling the sysfs interface when the sensor is streaming.
>> ---
>>  drivers/media/v4l2-core/v4l2-fwnode.c | 15 +++++++++++++++
>>  drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++++++++++++++
>>  include/media/v4l2-subdev.h           |  3 +++
>>  3 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>> index c8a2264262bc..cfac1e2ae501 100644
>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>> @@ -16,6 +16,7 @@
>>   */
>>  #include <linux/acpi.h>
>>  #include <linux/kernel.h>
>> +#include <linux/leds.h>
>>  #include <linux/mm.h>
>>  #include <linux/of.h>
>>  #include <linux/property.h>
>> @@ -1295,6 +1296,20 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>>  	if (WARN_ON(!sd->dev))
>>  		return -ENODEV;
>>  
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	sd->privacy_led = led_get(sd->dev, "privacy-led");
>> +	if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
>> +		return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
>> +
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> +		mutex_lock(&sd->privacy_led->led_access);
>> +		led_sysfs_disable(sd->privacy_led);
>> +		led_trigger_remove(sd->privacy_led);
>> +		led_set_brightness(sd->privacy_led, 0);
>> +		mutex_unlock(&sd->privacy_led->led_access);
>> +	}
>> +#endif
>> +
>>  	notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
>>  	if (!notifier)
>>  		return -ENOMEM;
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 4988a25bd8f4..f33e943aab3f 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -9,6 +9,7 @@
>>   */
>>  
>>  #include <linux/ioctl.h>
>> +#include <linux/leds.h>
>>  #include <linux/mm.h>
>>  #include <linux/module.h>
>>  #include <linux/slab.h>
>> @@ -322,6 +323,14 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>  {
>>  	int ret;
>>  
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> +		if (enable)
>> +			led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
>> +		else
>> +			led_set_brightness(sd->privacy_led, 0);
>> +	}
>> +#endif
>>  	ret = sd->ops->video->s_stream(sd, enable);
>>  
>>  	if (!enable && ret < 0) {
>> @@ -1050,6 +1059,14 @@ EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
>>  
>>  void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
> 
> v4l2_subdev_cleanup() is currently called by drivers using V4L2 subdev
> state at the moment, making it unsuitable for the purpose of releasing the
> privacy led.
> 
> Could you move this to v4l2_async_unregister_subdev() instead?

Good point.

Looking into this also made me realize that I forgot to cleanup
the LED reference (and re-enable sysfs control) in case of
errors in v4l2_async_register_subdev_sensor() after getting it.

Fixing this requires adding a v4l2_subdev_put_privacy_led()
helper. At which point it makes sense to also put the code
to get the led in a v4l2_subdev_get_privacy_led() helper
and then all privacy-led code lives inside a v4l2-subdev.c
removing the need for:

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

all together :)   So I'll drop that from v6 of this series to
make the series simpler.

Regards,

Hans





> 
>>  {
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> +		mutex_lock(&sd->privacy_led->led_access);
>> +		led_sysfs_enable(sd->privacy_led);
>> +		mutex_unlock(&sd->privacy_led->led_access);
>> +		led_put(sd->privacy_led);
>> +	}
>> +#endif
>>  	__v4l2_subdev_state_free(sd->active_state);
>>  	sd->active_state = NULL;
>>  }
>> @@ -1090,6 +1107,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>>  	sd->grp_id = 0;
>>  	sd->dev_priv = NULL;
>>  	sd->host_priv = NULL;
>> +	sd->privacy_led = NULL;
>>  #if defined(CONFIG_MEDIA_CONTROLLER)
>>  	sd->entity.name = sd->name;
>>  	sd->entity.obj_type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index b15fa9930f30..0547313f98cc 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -38,6 +38,7 @@ struct v4l2_subdev;
>>  struct v4l2_subdev_fh;
>>  struct tuner_setup;
>>  struct v4l2_mbus_frame_desc;
>> +struct led_classdev;
>>  
>>  /**
>>   * struct v4l2_decode_vbi_line - used to decode_vbi_line
>> @@ -982,6 +983,8 @@ struct v4l2_subdev {
>>  	 * appropriate functions.
>>  	 */
>>  
>> +	struct led_classdev *privacy_led;
>> +
>>  	/*
>>  	 * TODO: active_state should most likely be changed from a pointer to an
>>  	 * embedded field. For the time being it's kept as a pointer to more
> 

[PATCH v5 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 v5 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..251c6524458e
--- /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.provider = int3472->pled.name;
+	int3472->pled.lookup.dev_id = int3472->sensor_name;
+	int3472->pled.lookup.con_id = "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 v5 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 v5 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 v5 00/11] leds: lookup-table support + int3472/media privacy LED support

[Lee Jones]

On Fri, 20 Jan 2023, Hans de Goede wrote:

> Hi All,
> 
> Here is version 5 of my series to adjust the INT3472 code's handling of
> the privacy LED on x86 laptops with MIPI camera(s) so that it will also
> work on devices which have a privacy-LED GPIO but not a clk-enable GPIO
> (so that we cannot just tie the LED state to the clk-enable state).
> 
> Changes in v5:
> - Rename lookup-table names to match those from the gpio and reset lookups:
>   s/led_name/provider/
>   s/consumer_dev_name/dev_id/
>   s/consumer_function/con_id/
> - Add static inline wrappers for the v4l2_async debugfs init/exit funcs,
>   to fix build errors when CONFIG_V4L2_ASYNC is not enabled
> 
> Changes in v4:
> - Rename new __led_get() helper to led_module_get()
> - Drop of/devicetree support from "led-class: Add generic [devm_]led_get()"
> - Add RFC patch to re-add of/devicetree support to show that the new
>   led_get() can easily be extended with dt support when the need for this
>   arises (proof-of-concept dt code, not intended for merging)
> - New patch to built async and fwnode code into videodev.ko,
>   to avoid issues with some of the new LED code getting builtin vs
>   other parts possibly being in a module
> - Move the led_get() call to v4l2_async_register_subdev_sensor()
> - Move the led_disable_sysfs() call to be done at led_get() time
> - Address some other minor review comments
> 
> Changes in v3:
> - Due to popular request by multiple people this new version now models
>   the privacy LED as a LED class device. This requires being able to
>   "tie" the LED class device to a specific camera sensor (some devices
>   have multiple sensors + privacy-LEDs).
> 
> Patches 1-5 are LED subsystem patches for this. 1 is a bug fix, 2-4 add
> the new [devm_]led_get() functions. Patch 5 is the RFC patch adding dt
> support to led_get() and is not intended for merging.
> 
> Patch 6 + 7 add generic privacy-LED support to the v4l2-core/v4l2-subdev.c
> code automatically enabling the privacy-LED when s_stream(subdev, 1)
> is called. So that we don't need to add privacy-LED code to all the
> camera sensor drivers separately (as requested by Sakari).
> 
> Patches 8-11 are patches to the platform specific INT3472 code to register
> privacy-LED class devices + lookup table entries for privacy-LEDs described
> in the special INT3472 ACPI nodes found on x86 devices with MIPI cameras.
> 
> Assuming at least the LED maintainers are happy with the approach suggested
> here, the first step to merging this would be to merge patches 1-4 and then
> provide an immutable branch with those to merge for the other subsystems
> since the other changes depend on these.

LEDs pull request to follow.

-- 
Lee Jones [李琼斯]

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

[Hans de Goede]

Hi,

On 1/27/23 12:08, Lee Jones wrote:
> On Fri, 20 Jan 2023, Hans de Goede wrote:
> 
>> Hi All,
>>
>> Here is version 5 of my series to adjust the INT3472 code's handling of
>> the privacy LED on x86 laptops with MIPI camera(s) so that it will also
>> work on devices which have a privacy-LED GPIO but not a clk-enable GPIO
>> (so that we cannot just tie the LED state to the clk-enable state).
>>
>> Changes in v5:
>> - Rename lookup-table names to match those from the gpio and reset lookups:
>>   s/led_name/provider/
>>   s/consumer_dev_name/dev_id/
>>   s/consumer_function/con_id/
>> - Add static inline wrappers for the v4l2_async debugfs init/exit funcs,
>>   to fix build errors when CONFIG_V4L2_ASYNC is not enabled
>>
>> Changes in v4:
>> - Rename new __led_get() helper to led_module_get()
>> - Drop of/devicetree support from "led-class: Add generic [devm_]led_get()"
>> - Add RFC patch to re-add of/devicetree support to show that the new
>>   led_get() can easily be extended with dt support when the need for this
>>   arises (proof-of-concept dt code, not intended for merging)
>> - New patch to built async and fwnode code into videodev.ko,
>>   to avoid issues with some of the new LED code getting builtin vs
>>   other parts possibly being in a module
>> - Move the led_get() call to v4l2_async_register_subdev_sensor()
>> - Move the led_disable_sysfs() call to be done at led_get() time
>> - Address some other minor review comments
>>
>> Changes in v3:
>> - Due to popular request by multiple people this new version now models
>>   the privacy LED as a LED class device. This requires being able to
>>   "tie" the LED class device to a specific camera sensor (some devices
>>   have multiple sensors + privacy-LEDs).
>>
>> Patches 1-5 are LED subsystem patches for this. 1 is a bug fix, 2-4 add
>> the new [devm_]led_get() functions. Patch 5 is the RFC patch adding dt
>> support to led_get() and is not intended for merging.
>>
>> Patch 6 + 7 add generic privacy-LED support to the v4l2-core/v4l2-subdev.c
>> code automatically enabling the privacy-LED when s_stream(subdev, 1)
>> is called. So that we don't need to add privacy-LED code to all the
>> camera sensor drivers separately (as requested by Sakari).
>>
>> Patches 8-11 are patches to the platform specific INT3472 code to register
>> privacy-LED class devices + lookup table entries for privacy-LEDs described
>> in the special INT3472 ACPI nodes found on x86 devices with MIPI cameras.
>>
>> Assuming at least the LED maintainers are happy with the approach suggested
>> here, the first step to merging this would be to merge patches 1-4 and then
>> provide an immutable branch with those to merge for the other subsystems
>> since the other changes depend on these.
> 
> LEDs pull request to follow.

Great, thank you!

Regards,

Hans

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

[Lee Jones]

Enjoy!

The following changes since commit 1b929c02afd37871d5afb9d498426f83432e71c2:

  Linux 6.2-rc1 (2022-12-25 13:41:39 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git ib-leds-led_get-6.3

for you to fetch changes up to abc3100fcba6827444ef4bdb17065ac3b6619dff:

  leds: led-class: Add generic [devm_]led_get() (2023-01-27 11:07:11 +0000)

----------------------------------------------------------------
Hans de Goede (4):
      leds: led-class: Add missing put_device() to led_put()
      leds: led-class: Add led_module_get() helper
      leds: led-class: Add __devm_led_get() helper
      leds: led-class: Add generic [devm_]led_get()

 drivers/leds/led-class.c | 138 ++++++++++++++++++++++++++++++++++++++++-------
 include/linux/leds.h     |  21 ++++++++
 2 files changed, 139 insertions(+), 20 deletions(-)

-- 
Lee Jones [李琼斯]

[GIT PULL] Immutable branch from LEDs due for the v6.3 merge window

[Lee Jones]

Enjoy!

The following changes since commit 1b929c02afd37871d5afb9d498426f83432e71c2:

  Linux 6.2-rc1 (2022-12-25 13:41:39 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git tags/ib-leds-led_get-v6.3

for you to fetch changes up to abc3100fcba6827444ef4bdb17065ac3b6619dff:

  leds: led-class: Add generic [devm_]led_get() (2023-01-27 11:07:11 +0000)

----------------------------------------------------------------
Immutable branch from LEDs due for the v6.3 merge window

----------------------------------------------------------------
Hans de Goede (4):
      leds: led-class: Add missing put_device() to led_put()
      leds: led-class: Add led_module_get() helper
      leds: led-class: Add __devm_led_get() helper
      leds: led-class: Add generic [devm_]led_get()

 drivers/leds/led-class.c | 138 ++++++++++++++++++++++++++++++++++++++++-------
 include/linux/leds.h     |  21 ++++++++
 2 files changed, 139 insertions(+), 20 deletions(-)

-- 
Lee Jones [李琼斯]