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

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

[Hans de Goede]

Hi All,

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

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

Patches 1-5 are LED subsystem patches for this. 1 is a bug fix, 2-4
is a bit of refactoring in preparation for patch 5 which adds
generic (non devicetree specific) led_get() and devm_led_get() function
(which will also work with devicetree) and lookup table support to
allow platform code to add LED class-device <-> consumer-dev,function
lookups for non devicetree platforms.

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

These are all new patches in version 3. Patches 7-11 are patches
to the platform specific INT3472 code to register privacy-LED class
devices + lookup table entries for privacy-LEDs described in
the special INT3472 ACPI nodes found on x86 devices with MIPI
cameras (+ prep work + some other INT3472 fixes).

Assuming the LED and media maintainers are happy with the approach
suggested here (if you are please give your Ack / Reviewed-by) we
need to talk about how to merge this since patches 6 and 7-11
depend on the LED subsystem changes. I think it would be best if
the LED subsystem can provide an immutable branch with patches 1-5
(on top of 6.2-rc1 once it is out) and then the media folks and I
can merge that branch and then apply the other patches on top.

This series has been tested on:

- Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED
- Dell Latitude 9420, IPU 6, front: ov01a1s with privacy LED
- Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED
                              back: ov8865 with privacy LED (pled not yet supported)

Regards,

Hans


Hans de Goede (11):
  leds: led-class: Add missing put_device() to led_put()
  leds: led-class: Add __led_get() helper function
  leds: led-class: Add __of_led_get() helper
  leds: led-class: Add __devm_led_get() helper
  leds: led-class: Add generic [devm_]led_get()
  v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy
    LED if present
  platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
  platform/x86: int3472/discrete: Create a LED class device for the
    privacy LED
  platform/x86: int3472/discrete: Move GPIO request to
    skl_int3472_register_clock()
  platform/x86: int3472/discrete: Ensure the clk/power enable pins are
    in output mode
  platform/x86: int3472/discrete: Get the polarity from the _DSM entry

 drivers/leds/led-class.c                      | 174 +++++++++++++++---
 drivers/media/v4l2-core/v4l2-subdev.c         |  40 ++++
 drivers/platform/x86/intel/int3472/Makefile   |   2 +-
 .../x86/intel/int3472/clk_and_regulator.c     |  35 +++-
 drivers/platform/x86/intel/int3472/common.h   |  18 +-
 drivers/platform/x86/intel/int3472/discrete.c |  96 +++++-----
 drivers/platform/x86/intel/int3472/led.c      |  75 ++++++++
 include/linux/leds.h                          |  18 ++
 include/media/v4l2-subdev.h                   |   3 +
 9 files changed, 371 insertions(+), 90 deletions(-)
 create mode 100644 drivers/platform/x86/intel/int3472/led.c

-- 
2.38.1

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

[Hans de Goede]

led_put() is used to "undo" a successful of_led_get() call,
of_led_get() uses class_find_device_by_of_node() which returns
a reference to the device which must be free-ed with put_device()
when the caller is done with it.

Add a put_device() call to led_put() to free the reference returned
by class_find_device_by_of_node().

And also add a put_device() in the error-exit case of try_module_get()
failing.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/led-class.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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

[PATCH v3 02/11] leds: led-class: Add __led_get() helper function

[Hans de Goede]

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

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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..5f40110bd260 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_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_get(led_dev);
 }
 EXPORT_SYMBOL_GPL(of_led_get);
 
-- 
2.38.1

[PATCH v3 03/11] leds: led-class: Add __of_led_get() helper

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

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/led-class.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 5f40110bd260..1dd421ca56c1 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -232,19 +232,18 @@ static struct led_classdev *__led_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 "leds-names" property.
+	 * If it cannot be found, then of_parse_phandle() will propagate the error.
+	 */
+	if (name)
+		index = of_property_match_string(np, "leds-names", name);
 	led_node = of_parse_phandle(np, "leds", index);
 	if (!led_node)
 		return ERR_PTR(-ENOENT);
@@ -254,6 +253,19 @@ struct led_classdev *of_led_get(struct device_node *np, int index)
 
 	return __led_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);
 
 /**
-- 
2.38.1

[PATCH v3 04/11] leds: led-class: Add __devm_led_get() helper

[Hans de Goede]

Add a __devm_led_get() helper which registers a passed in led_classdev
with devm for unregistration.

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/led-class.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 1dd421ca56c1..6e25c411ba35 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -286,6 +286,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
@@ -301,7 +317,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);
@@ -310,17 +325,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.38.1

[PATCH v3 05/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. The existing of support is extended to also support
getting a LED by function-name using the standard devicetree pattern
of adding a -names string array to map names to the indexes.

For non devicetree platforms a lookup-table mechanism is added to
allow the platform code to map specific LED class_dev-s to specific
device,function combinations this way.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/led-class.c | 91 ++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     | 18 ++++++++
 2 files changed, 109 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 6e25c411ba35..76b3ab66eb88 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)
@@ -329,6 +331,95 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_of_led_get);
 
+/**
+ * led_get() - request a LED device via the LED framework
+ * @dev: device for which to get the LED device
+ * @function: string describing the function of the LED device
+ *
+ * @return a pointer to a LED device or ERR_PTR(errno) on failure.
+ */
+struct led_classdev *led_get(struct device *dev, char *function)
+{
+	struct led_lookup_data *lookup;
+	struct led_classdev *led_cdev;
+	const char *led_name = NULL;
+	struct device *led_dev;
+
+	if (dev->of_node) {
+		led_cdev = __of_led_get(dev->of_node, -1, function);
+		if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
+			return led_cdev;
+	}
+
+	mutex_lock(&leds_lookup_lock);
+	list_for_each_entry(lookup, &leds_lookup_list, list) {
+		if (!strcmp(lookup->consumer_dev_name, dev_name(dev)) &&
+		    !strcmp(lookup->consumer_function, function)) {
+			led_name = kstrdup(lookup->led_name, GFP_KERNEL);
+			break;
+		}
+	}
+	mutex_unlock(&leds_lookup_lock);
+
+	if (!led_name)
+		return ERR_PTR(-ENOENT);
+
+	led_dev = class_find_device_by_name(leds_class, led_name);
+	kfree(led_name);
+
+	return __led_get(led_dev);
+}
+EXPORT_SYMBOL_GPL(led_get);
+
+/**
+ * devm_led_get() - request a LED device via the LED framework
+ * @dev: device for which to get the LED device
+ * @function: string describing the function of the LED device
+ *
+ * The LED device returned from this function is automatically released
+ * on driver detach.
+ *
+ * @return a pointer to a LED device or ERR_PTR(errno) on failure.
+ */
+struct led_classdev *devm_led_get(struct device *dev, char *function)
+{
+	struct led_classdev *led;
+
+	led = led_get(dev, function);
+	if (IS_ERR(led))
+		return led;
+
+	return __devm_led_get(dev, led);
+}
+EXPORT_SYMBOL_GPL(devm_led_get);
+
+/**
+ * led_add_lookup() - Add a LED lookup table entry
+ * @led_lookup: the lookup table entry to add
+ *
+ * Add a LED lookup table entry. On systems without devicetree the lookup table
+ * is used by led_get() to find LEDs.
+ */
+void led_add_lookup(struct led_lookup_data *led_lookup)
+{
+	mutex_lock(&leds_lookup_lock);
+	list_add_tail(&led_lookup->list, &leds_lookup_list);
+	mutex_unlock(&leds_lookup_lock);
+}
+EXPORT_SYMBOL_GPL(led_add_lookup);
+
+/**
+ * led_remove_lookup() - Remove a LED lookup table entry
+ * @led_lookup: the lookup table entry to remove
+ */
+void led_remove_lookup(struct led_lookup_data *led_lookup)
+{
+	mutex_lock(&leds_lookup_lock);
+	list_del(&led_lookup->list);
+	mutex_unlock(&leds_lookup_lock);
+}
+EXPORT_SYMBOL_GPL(led_remove_lookup);
+
 static int led_classdev_next_name(const char *init_name, char *name,
 				  size_t len)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index ba4861ec73d3..e44fc5ec7c9a 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -39,6 +39,18 @@ enum led_default_state {
 	LEDS_DEFSTATE_KEEP	= 2,
 };
 
+/*
+ * This is used to tell led_get() device which led_classdev to return for
+ * a specific consumer device-name, function pair on non devicetree platforms.
+ * Note all strings must be set.
+ */
+struct led_lookup_data {
+	struct list_head list;
+	const char *led_name;
+	const char *consumer_dev_name;
+	const char *consumer_function;
+};
+
 struct led_init_data {
 	/* device fwnode handle */
 	struct fwnode_handle *fwnode;
@@ -211,6 +223,12 @@ void devm_led_classdev_unregister(struct device *parent,
 void led_classdev_suspend(struct led_classdev *led_cdev);
 void led_classdev_resume(struct led_classdev *led_cdev);
 
+void led_add_lookup(struct led_lookup_data *led_lookup);
+void led_remove_lookup(struct led_lookup_data *led_lookup);
+
+struct led_classdev *__must_check led_get(struct device *dev, char *function);
+struct led_classdev *__must_check devm_led_get(struct device *dev, char *function);
+
 extern struct led_classdev *of_led_get(struct device_node *np, int index);
 extern void led_put(struct led_classdev *led_cdev);
 struct led_classdev *__must_check devm_of_led_get(struct device *dev,
-- 
2.38.1

[PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Hans de Goede]

Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
for sensors with a privacy LED, rather then having to duplicate this code
in all the sensor drivers.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++
 include/media/v4l2-subdev.h           |  3 ++
 2 files changed, 43 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 4988a25bd8f4..7344f6cd58b7 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
 	       sd->ops->pad->get_mbus_config(sd, pad, config);
 }
 
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
+#include <linux/leds.h>
+
+static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
+{
+	if (!sd->dev)
+		return;
+
+	/* Try to get privacy-led once, at first s_stream() */
+	if (!sd->privacy_led)
+		sd->privacy_led = led_get(sd->dev, "privacy-led");
+
+	if (IS_ERR(sd->privacy_led))
+		return;
+
+	mutex_lock(&sd->privacy_led->led_access);
+
+	if (enable) {
+		led_sysfs_disable(sd->privacy_led);
+		led_trigger_remove(sd->privacy_led);
+		led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
+	} else {
+		led_set_brightness(sd->privacy_led, 0);
+		led_sysfs_enable(sd->privacy_led);
+	}
+
+	mutex_unlock(&sd->privacy_led->led_access);
+}
+#else
+static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {}
+#endif
+
 static int call_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	int ret;
 
+	call_s_stream_update_pled(sd, enable);
+
 	ret = sd->ops->video->s_stream(sd, enable);
 
 	if (!enable && ret < 0) {
@@ -1050,6 +1084,11 @@ EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
 
 void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
 {
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
+	if (!IS_ERR_OR_NULL(sd->privacy_led))
+		led_put(sd->privacy_led);
+#endif
+
 	__v4l2_subdev_state_free(sd->active_state);
 	sd->active_state = NULL;
 }
@@ -1090,6 +1129,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
 	sd->grp_id = 0;
 	sd->dev_priv = NULL;
 	sd->host_priv = NULL;
+	sd->privacy_led = NULL;
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	sd->entity.name = sd->name;
 	sd->entity.obj_type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b15fa9930f30..0547313f98cc 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -38,6 +38,7 @@ struct v4l2_subdev;
 struct v4l2_subdev_fh;
 struct tuner_setup;
 struct v4l2_mbus_frame_desc;
+struct led_classdev;
 
 /**
  * struct v4l2_decode_vbi_line - used to decode_vbi_line
@@ -982,6 +983,8 @@ struct v4l2_subdev {
 	 * appropriate functions.
 	 */
 
+	struct led_classdev *privacy_led;
+
 	/*
 	 * TODO: active_state should most likely be changed from a pointer to an
 	 * embedded field. For the time being it's kept as a pointer to more
-- 
2.38.1

[PATCH v3 07/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 974a132db651..bd3797ce64bf 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -184,6 +184,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
@@ -223,6 +253,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;
 
@@ -246,19 +278,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.38.1

[PATCH v3 08/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>
---
 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 | 52 ++++--------
 drivers/platform/x86/intel/int3472/led.c      | 79 +++++++++++++++++++
 5 files changed, 108 insertions(+), 43 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 1cf958983e86..e61119b17677 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..e106bbfe8ffa 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 {
+		char name[INT3472_LED_MAX_NAME_LEN];
+		struct led_lookup_data lookup;
+		struct led_classdev classdev;
+		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 bd3797ce64bf..1a1e0b196bfa 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -155,33 +155,19 @@ 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;
-		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;
-		break;
-	default:
-		dev_err(int3472->dev, "Invalid GPIO type 0x%02x for clock\n", type);
-		break;
-	}
+	int3472->clock.ena_gpio = gpio;
 
-	return 0;
+	return skl_int3472_register_clock(int3472);
 }
 
 static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
@@ -289,11 +275,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);
@@ -337,21 +328,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);
 
@@ -368,8 +344,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..05c58ba23570
--- /dev/null
+++ b/drivers/platform/x86/intel/int3472/led.c
@@ -0,0 +1,79 @@
+// 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_register_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 *path = agpio->resource_source.string_ptr;
+	int i, 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)) {
+		ret = PTR_ERR(int3472->pled.gpio);
+		return dev_err_probe(int3472->dev, ret, "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));
+	for (i = 0; int3472->pled.name[i]; i++) {
+		if (int3472->pled.name[i] == ':') {
+			int3472->pled.name[i] = '_';
+			break;
+		}
+	}
+
+	int3472->pled.classdev.name = int3472->pled.name;
+	int3472->pled.classdev.max_brightness = 1;
+	int3472->pled.classdev.brightness_set_blocking = int3472_register_pled_set;
+
+	ret = led_classdev_register(int3472->dev, &int3472->pled.classdev);
+	if (ret)
+		goto err_free_gpio;
+
+	int3472->pled.lookup.led_name = int3472->pled.name;
+	int3472->pled.lookup.consumer_dev_name = int3472->sensor_name;
+	int3472->pled.lookup.consumer_function = "privacy-led";
+	led_add_lookup(&int3472->pled.lookup);
+
+	return 0;
+
+err_free_gpio:
+	gpiod_put(int3472->pled.gpio);
+	return ret;
+}
+
+void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
+{
+	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
+		return;
+
+	led_remove_lookup(&int3472->pled.lookup);
+	led_classdev_unregister(&int3472->pled.classdev);
+	gpiod_put(int3472->pled.gpio);
+}
-- 
2.38.1

[PATCH v3 09/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     | 26 +++++++++++++++--
 drivers/platform/x86/intel/int3472/common.h   |  3 +-
 drivers/platform/x86/intel/int3472/discrete.c | 28 ++-----------------
 3 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index e61119b17677..4b4d77db44ce 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -86,18 +86,32 @@ 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)) {
+		ret = PTR_ERR(int3472->clock.ena_gpio);
+		return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n");
+	}
+
 	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 +137,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 e106bbfe8ffa..5ec89038ae07 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 1a1e0b196bfa..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,22 +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;
-
-	return skl_int3472_register_clock(int3472);
-}
-
 static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
 {
 	switch (type) {
@@ -275,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:
@@ -340,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.38.1

[PATCH v3 10/11] platform/x86: int3472/discrete: Ensure the clk/power enable pins are in output mode

[Hans de Goede]

acpi_get_and_request_gpiod() does not take a gpio_lookup_flags argument
specifying that the pins direction should be initialized to a specific
value.

This means that in some cases the pins might be left in input mode, causing
the gpiod_set() calls made to enable the clk / regulator to not work.

One example of this problem is the clk-enable GPIO for the ov01a1s sensor
on a Dell Latitude 9420 being left in input mode causing the clk to
never get enabled.

Explicitly set the direction of the pins to output to fix this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/clk_and_regulator.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 4b4d77db44ce..15a8bff645f7 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -106,6 +106,9 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
 		return dev_err_probe(int3472->dev, ret, "getting regulator 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) {
@@ -198,6 +201,9 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 		return PTR_ERR(int3472->regulator.gpio);
 	}
 
+	/* Ensure the pin is in output mode and non-active state */
+	gpiod_direction_output(int3472->regulator.gpio, 0);
+
 	cfg.dev = &int3472->adev->dev;
 	cfg.init_data = &init_data;
 	cfg.ena_gpiod = int3472->regulator.gpio;
-- 
2.38.1

[PATCH v3 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 15a8bff645f7..2b81066b024e 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 = {
@@ -106,6 +106,9 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
 		return dev_err_probe(int3472->dev, ret, "getting regulator 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 5ec89038ae07..4317c77e53d1 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.38.1

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

[Hans de Goede]

Hi,

On 12/16/22 12:30, Hans de Goede wrote:
> Hi All,
> 
> Here is my 3th attempt at adjusting the INT3472 code's handling of
> the privacy LED on x86 laptops with MIPI camera(s) so that it will also
> work on devices which have a privacy-LED GPIO but not a clk-enable GPIO
> (so that we cannot just tie the LED state to the clk-enable state).
> 
> Due to popular request by multiple people this new version now models
> the privacy LED as a LED class device. This requires being able to
> "tie" the LED class device to a specific camera sensor (some devices
> have multiple sensors + privacy-LEDs).
> 
> Patches 1-5 are LED subsystem patches for this. 1 is a bug fix, 2-4
> is a bit of refactoring in preparation for patch 5 which adds
> generic (non devicetree specific) led_get() and devm_led_get() function
> (which will also work with devicetree) and lookup table support to
> allow platform code to add LED class-device <-> consumer-dev,function
> lookups for non devicetree platforms.
> 
> Patch 6 adds generic privacy-LED support to the v4l2-core/v4l2-subdev.c
> code automatically enabling the privacy-LED when s_stream(subdev, 1)
> is called. So that we don't need to privacy-LED code to all the
> camera sensor drivers separately (as requested by Sakari).
> 
> These are all new patches in version 3. Patches 7-11 are patches
> to the platform specific INT3472 code to register privacy-LED class
> devices + lookup table entries for privacy-LEDs described in
> the special INT3472 ACPI nodes found on x86 devices with MIPI
> cameras (+ prep work + some other INT3472 fixes).
> 
> Assuming the LED and media maintainers are happy with the approach
> suggested here (if you are please give your Ack / Reviewed-by) we
> need to talk about how to merge this since patches 6 and 7-11
> depend on the LED subsystem changes. I think it would be best if
> the LED subsystem can provide an immutable branch with patches 1-5
> (on top of 6.2-rc1 once it is out) and then the media folks and I
> can merge that branch and then apply the other patches on top.
> 
> This series has been tested on:
> 
> - Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED
> - Dell Latitude 9420, IPU 6, front: ov01a1s with privacy LED
> - Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED
>                               back: ov8865 with privacy LED (pled not yet supported)
> 
> Regards,
> 
> Hans

p.s.

I have matching out of tree IPU6 driver changes here:

https://github.com/jwrdegoede/ipu6-drivers/commits/master

once this series has landed these changes will allow using
the out of tree IPU6 driver with an unmodified upstream kernel.

Regards,

Hans

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:03PM +0100, 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.

...

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

...

>  void led_put(struct led_classdev *led_cdev)
>  {
>  	module_put(led_cdev->dev->parent->driver->owner);
> +	put_device(led_cdev->dev);

Hmm... It was in the original submission.

https://lore.kernel.org/linux-leds/1443605522-1118-2-git-send-email-tomi.valkeinen@ti.com/

Nevertheless, shouldn't you put device before putting module? (It may need to
save the owner of the driver, I think.)

>  }

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:07PM +0100, 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. The existing of support is extended to also support
> getting a LED by function-name using the standard devicetree pattern
> of adding a -names string array to map names to the indexes.
> 
> For non devicetree platforms a lookup-table mechanism is added to
> allow the platform code to map specific LED class_dev-s to specific
> device,function combinations this way.

...

> +	list_for_each_entry(lookup, &leds_lookup_list, list) {
> +		if (!strcmp(lookup->consumer_dev_name, dev_name(dev)) &&
> +		    !strcmp(lookup->consumer_function, function)) {
> +			led_name = kstrdup(lookup->led_name, GFP_KERNEL);

kstrdup_const() ?

> +			break;
> +		}
> +	}

> +	if (!led_name)
> +		return ERR_PTR(-ENOENT);
> +
> +	led_dev = class_find_device_by_name(leds_class, led_name);
> +	kfree(led_name);

kfree_const() ?

> +	return __led_get(led_dev);
> +}

...

> +EXPORT_SYMBOL_GPL(led_add_lookup);

> +EXPORT_SYMBOL_GPL(led_remove_lookup);

Wondering why we can't make at least those two to be namespaced from day 1,

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:05PM +0100, Hans de Goede wrote:
> Turn of_led_get() into a more generic __of_led_get() helper function,
> which can lookup LEDs in devicetree by either name or index.

...

> +	/*
> +	 * For named LEDs, first look up the name in the "leds-names" property.
> +	 * If it cannot be found, then of_parse_phandle() will propagate the error.
> +	 */
> +	if (name)
> +		index = of_property_match_string(np, "leds-names", name);

I can't find this property anywhere in the kernel. Is it new?
If so, where is the bindings? And why entire code can't be converted
to use fwnode for this case?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/11] leds: led-class: Add __led_get() helper function

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:04PM +0100, Hans de Goede wrote:
> Split out part of of_led_get() into a generic __led_get() helper function.
> 
> This is a preparation patch for adding a generic (non devicetree specific)
> led_get() function.

...

> +static struct led_classdev *__led_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;
> +}

If I'm not mistaken, the entire series leaves this function as is.
At the end I found three _get functions:
 __led_get
 led_get
 __devm_led_get


As far as I can see the above can be named more precisely, i.e.
led_module_get(). Or alike if this sounds not good either.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 03:35:29PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:03PM +0100, Hans de Goede wrote:

...

> >  	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;
> 
> ...
> 
> >  void led_put(struct led_classdev *led_cdev)
> >  {
> >  	module_put(led_cdev->dev->parent->driver->owner);
> > +	put_device(led_cdev->dev);
> 
> Hmm... It was in the original submission.
> 
> https://lore.kernel.org/linux-leds/1443605522-1118-2-git-send-email-tomi.valkeinen@ti.com/

...

> Nevertheless, shouldn't you put device before putting module? (It may need to
> save the owner of the driver, I think.)

I think this is wrong, the symmetry is kept correct in your patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote:
> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
> for sensors with a privacy LED, rather then having to duplicate this code
> in all the sensor drivers.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++
>  include/media/v4l2-subdev.h           |  3 ++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4988a25bd8f4..7344f6cd58b7 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
>  	       sd->ops->pad->get_mbus_config(sd, pad, config);
>  }
>  
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +#include <linux/leds.h>

Can this be moved to the top of the file ? It doesn't have to be
conditionally compiled there.

> +
> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
> +{
> +	if (!sd->dev)
> +		return;
> +
> +	/* Try to get privacy-led once, at first s_stream() */
> +	if (!sd->privacy_led)
> +		sd->privacy_led = led_get(sd->dev, "privacy-led");

I'm not sure I like this much. If the LED provider isn't available
(yet), the LED will stay off. That's a privacy concern.

> +
> +	if (IS_ERR(sd->privacy_led))
> +		return;
> +
> +	mutex_lock(&sd->privacy_led->led_access);
> +
> +	if (enable) {
> +		led_sysfs_disable(sd->privacy_led);
> +		led_trigger_remove(sd->privacy_led);
> +		led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
> +	} else {
> +		led_set_brightness(sd->privacy_led, 0);
> +		led_sysfs_enable(sd->privacy_led);

I don't think you should reenable control through sysfs here. I don't
really see a use case, and you've removed the trigger anyway, so the
behaviour would be quite inconsistent.

> +	}
> +
> +	mutex_unlock(&sd->privacy_led->led_access);
> +}
> +#else
> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {}
> +#endif
> +
>  static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	int ret;
>  
> +	call_s_stream_update_pled(sd, enable);
> +
>  	ret = sd->ops->video->s_stream(sd, enable);
>  
>  	if (!enable && ret < 0) {

You need to turn the LED off when enabling if .s_stream() fails.

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 07/11] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:09PM +0100, Hans de Goede wrote:
> Add a helper function to map the type returned by the _DSM
> method to a function name + the default polarity for that function.
> 
> And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN
> cases into a single generic case.
> 
> This is a preparation patch for further GPIO mapping changes.

...

> +static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)

I find a bit strange not making this a function that returns func:

static const char *int3472_get_func_and_polarity(u8 type, u32 *polarity)

> +{
> +	switch (type) {
> +	case INT3472_GPIO_TYPE_RESET:
> +		*func = "reset";
> +		*polarity = GPIO_ACTIVE_LOW;

		return "reset";

etc.

> +		break;
> +	case INT3472_GPIO_TYPE_POWERDOWN:
> +		*func = "powerdown";
> +		*polarity = GPIO_ACTIVE_LOW;
> +		break;
> +	case INT3472_GPIO_TYPE_CLK_ENABLE:
> +		*func = "clk-enable";
> +		*polarity = GPIO_ACTIVE_HIGH;
> +		break;
> +	case INT3472_GPIO_TYPE_PRIVACY_LED:
> +		*func = "privacy-led";
> +		*polarity = GPIO_ACTIVE_HIGH;
> +		break;
> +	case INT3472_GPIO_TYPE_POWER_ENABLE:
> +		*func = "power-enable";
> +		*polarity = GPIO_ACTIVE_HIGH;
> +		break;
> +	default:
> +		*func = "unknown";
> +		*polarity = GPIO_ACTIVE_HIGH;
> +		break;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Laurent Pinchart]

On Fri, Dec 16, 2022 at 03:56:33PM +0200, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote:
> > Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
> > for sensors with a privacy LED, rather then having to duplicate this code
> > in all the sensor drivers.
> > 
> > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++
> >  include/media/v4l2-subdev.h           |  3 ++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 4988a25bd8f4..7344f6cd58b7 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
> >  	       sd->ops->pad->get_mbus_config(sd, pad, config);
> >  }
> >  
> > +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> > +#include <linux/leds.h>
> 
> Can this be moved to the top of the file ? It doesn't have to be
> conditionally compiled there.
> 
> > +
> > +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
> > +{
> > +	if (!sd->dev)
> > +		return;
> > +
> > +	/* Try to get privacy-led once, at first s_stream() */
> > +	if (!sd->privacy_led)
> > +		sd->privacy_led = led_get(sd->dev, "privacy-led");
> 
> I'm not sure I like this much. If the LED provider isn't available
> (yet), the LED will stay off. That's a privacy concern.
> 
> > +
> > +	if (IS_ERR(sd->privacy_led))
> > +		return;
> > +
> > +	mutex_lock(&sd->privacy_led->led_access);
> > +
> > +	if (enable) {
> > +		led_sysfs_disable(sd->privacy_led);
> > +		led_trigger_remove(sd->privacy_led);
> > +		led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
> > +	} else {
> > +		led_set_brightness(sd->privacy_led, 0);
> > +		led_sysfs_enable(sd->privacy_led);
> 
> I don't think you should reenable control through sysfs here. I don't
> really see a use case, and you've removed the trigger anyway, so the
> behaviour would be quite inconsistent.

Also, I think it would be better if the LED device was marked as "no
sysfs" when it is registered.

> > +	}
> > +
> > +	mutex_unlock(&sd->privacy_led->led_access);
> > +}
> > +#else
> > +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {}
> > +#endif
> > +
> >  static int call_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	int ret;
> >  
> > +	call_s_stream_update_pled(sd, enable);
> > +
> >  	ret = sd->ops->video->s_stream(sd, enable);
> >  
> >  	if (!enable && ret < 0) {
> 
> You need to turn the LED off when enabling if .s_stream() fails.
> 
> > @@ -1050,6 +1084,11 @@ EXPORT_SYMBOL_GPL(__v4l2_subdev_init_finalize);
> >  
> >  void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
> >  {
> > +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> > +	if (!IS_ERR_OR_NULL(sd->privacy_led))
> > +		led_put(sd->privacy_led);
> > +#endif
> > +
> >  	__v4l2_subdev_state_free(sd->active_state);
> >  	sd->active_state = NULL;
> >  }
> > @@ -1090,6 +1129,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
> >  	sd->grp_id = 0;
> >  	sd->dev_priv = NULL;
> >  	sd->host_priv = NULL;
> > +	sd->privacy_led = NULL;
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> >  	sd->entity.name = sd->name;
> >  	sd->entity.obj_type = MEDIA_ENTITY_TYPE_V4L2_SUBDEV;
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index b15fa9930f30..0547313f98cc 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -38,6 +38,7 @@ struct v4l2_subdev;
> >  struct v4l2_subdev_fh;
> >  struct tuner_setup;
> >  struct v4l2_mbus_frame_desc;
> > +struct led_classdev;
> >  
> >  /**
> >   * struct v4l2_decode_vbi_line - used to decode_vbi_line
> > @@ -982,6 +983,8 @@ struct v4l2_subdev {
> >  	 * appropriate functions.
> >  	 */
> >  
> > +	struct led_classdev *privacy_led;
> > +
> >  	/*
> >  	 * TODO: active_state should most likely be changed from a pointer to an
> >  	 * embedded field. For the time being it's kept as a pointer to more

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote:
> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
> for sensors with a privacy LED, rather then having to duplicate this code
> in all the sensor drivers.

...

> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
> +{
> +	if (!sd->dev)
> +		return;
> +
> +	/* Try to get privacy-led once, at first s_stream() */
> +	if (!sd->privacy_led)
> +		sd->privacy_led = led_get(sd->dev, "privacy-led");

> +

Redundant blank line?

> +	if (IS_ERR(sd->privacy_led))
> +		return;

I'm not sure I have got the logic right. Let's assume we call it with
_led == NULL. Then in case of error, we feel it with the error pointer.
If we call again, we check for NULL, and return error pointer.

So, we won't try the second time. Is it by design? Or should it be

	struct ... *led;

	if (!privacy_led) {
		led = ...
		if (IS_ERR())
			return;
		privacy_led = led;
	}

?

> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:07PM +0100, 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. The existing of support is extended to also support
> getting a LED by function-name using the standard devicetree pattern
> of adding a -names string array to map names to the indexes.
> 
> For non devicetree platforms a lookup-table mechanism is added to
> allow the platform code to map specific LED class_dev-s to specific
> device,function combinations this way.

...

> +void led_remove_lookup(struct led_lookup_data *led_lookup)
> +{
> +	mutex_lock(&leds_lookup_lock);
> +	list_del(&led_lookup->list);

Perhaps list_del_init(). Some thoughts about this in the one of the following
comments.

> +	mutex_unlock(&leds_lookup_lock);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote:
> On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad
> X1 Nano gen 2 there is no clock-enable pin, triggering the:
> "No clk GPIO. The privacy LED won't work" warning and causing the privacy
> LED to not work.
> 
> Fix this by modeling the privacy LED as a LED class device rather then
> integrating it with the registered clock.
> 
> Note this relies on media subsys changes to actually turn the LED on/off
> when the sensor's v4l2_subdev's s_stream() operand gets called.

...

> +	struct int3472_pled {
> +		char name[INT3472_LED_MAX_NAME_LEN];
> +		struct led_lookup_data lookup;

> +		struct led_classdev classdev;

Why not putting this as a first member in the struct, so any container_of()
against it become no-op at compile time?

> +		struct gpio_desc *gpio;
> +	} pled;

...

> +	if (IS_ERR(int3472->pled.gpio)) {
> +		ret = PTR_ERR(int3472->pled.gpio);
> +		return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n");

	return dev_err_probe(...);

> +	}

...

> +	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
> +	snprintf(int3472->pled.name, sizeof(int3472->pled.name),
> +		 "%s::privacy_led", acpi_dev_name(int3472->sensor));

> +	for (i = 0; int3472->pled.name[i]; i++) {
> +		if (int3472->pled.name[i] == ':') {
> +			int3472->pled.name[i] = '_';
> +			break;
> +		}
> +	}

NIH strreplace().

...

> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
> +{
> +	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
> +		return;

This dups the check inside the _unregister() below, right?

> +	led_remove_lookup(&int3472->pled.lookup);

With list_del_init() I believe the above check can be droped.

> +	led_classdev_unregister(&int3472->pled.classdev);
> +	gpiod_put(int3472->pled.gpio);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] platform/x86: int3472/discrete: Move GPIO request to skl_int3472_register_clock()

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:11PM +0100, Hans de Goede wrote:
> 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 .

Extra space.

...

> +	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(...); ?

> +		ret = PTR_ERR(int3472->clock.ena_gpio);
> +		return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n");
> +	}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede wrote:
> 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,

Here and in the code you actually can refer to it as 3rd byte.

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

...

> +	/* 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)

Not sure why you need a temporary variable for this. Just use
GENMASK()/GENMASK_ULL()?

	if (obj->integer.value & GENMASK(31, 24));

In this case you even don't need to repeat bit numbers in the comment.

> +		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");

Yet another high/low :-)

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 04:57:02PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede wrote:

> > +	/* 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)
> 
> Not sure why you need a temporary variable for this. Just use
> GENMASK()/GENMASK_ULL()?
> 
> 	if (obj->integer.value & GENMASK(31, 24));

Of course should be

	if (!(obj->integer.value & GENMASK(31, 24)))

> In this case you even don't need to repeat bit numbers in the comment.
> 
> > +		polarity ^= GPIO_ACTIVE_LOW;

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 12/16/22 14:55, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 03:35:29PM +0200, Andy Shevchenko wrote:
>> On Fri, Dec 16, 2022 at 12:30:03PM +0100, Hans de Goede wrote:
> 
> ...
> 
>>>  	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;
>>
>> ...
>>
>>>  void led_put(struct led_classdev *led_cdev)
>>>  {
>>>  	module_put(led_cdev->dev->parent->driver->owner);
>>> +	put_device(led_cdev->dev);
>>
>> Hmm... It was in the original submission.
>>
>> https://lore.kernel.org/linux-leds/1443605522-1118-2-git-send-email-tomi.valkeinen@ti.com/
> 
> ...
> 
>> Nevertheless, shouldn't you put device before putting module? (It may need to
>> save the owner of the driver, I think.)
> 
> I think this is wrong, the symmetry is kept correct in your patch.

Right, the line above dereferences led_cdev->dev, so the put()
must be done after that line.

Regards,

Hans

Re: [PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Hans de Goede]

Hi,

On 12/16/22 14:56, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote:
>> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
>> for sensors with a privacy LED, rather then having to duplicate this code
>> in all the sensor drivers.
>>
>> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++
>>  include/media/v4l2-subdev.h           |  3 ++
>>  2 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 4988a25bd8f4..7344f6cd58b7 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
>>  	       sd->ops->pad->get_mbus_config(sd, pad, config);
>>  }
>>  
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +#include <linux/leds.h>
> 
> Can this be moved to the top of the file ? It doesn't have to be
> conditionally compiled there.

You mean just the #include right? Ack to that.

> 
>> +
>> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
>> +{
>> +	if (!sd->dev)
>> +		return;
>> +
>> +	/* Try to get privacy-led once, at first s_stream() */
>> +	if (!sd->privacy_led)
>> +		sd->privacy_led = led_get(sd->dev, "privacy-led");
> 
> I'm not sure I like this much. If the LED provider isn't available
> (yet), the LED will stay off. That's a privacy concern.

At first I tried to put the led_get() inside v4l2_async_register_subdev_sensor(),
which could then return an error like -EPROBE_DEFER if the led_get()
fails (and nicely limits the led_get() to sensors).

The problem which I hit is that v4l2-fwnode.c is build according to
CONFIG_V4L2_FWNODE where as v4l2-subdev.c is build according to
CONFIG_VIDEO_DEV 

And e.g. CONFIG_VIDEO_DEV could be builtin while CONFIG_V4L2_FWNODE
could be a module and then having the #if IS_REACHABLE(CONFIG_LEDS_CLASS)
spread over the 2 could result in different answers in the different
files ...

One solution here could be to change CONFIG_V4L2_FWNODE and V4L2_ASYNC
to bools and link the (quite small) objects for these 2 into videodev.ko:

videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o






> 
>> +
>> +	if (IS_ERR(sd->privacy_led))
>> +		return;
>> +
>> +	mutex_lock(&sd->privacy_led->led_access);
>> +
>> +	if (enable) {
>> +		led_sysfs_disable(sd->privacy_led);
>> +		led_trigger_remove(sd->privacy_led);
>> +		led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
>> +	} else {
>> +		led_set_brightness(sd->privacy_led, 0);
>> +		led_sysfs_enable(sd->privacy_led);
> 
> I don't think you should reenable control through sysfs here. I don't
> really see a use case, and you've removed the trigger anyway, so the
> behaviour would be quite inconsistent.

Hmm, I thought this was a good compromise, this way the LED
can be used for other purposes when the sensor is off if users
want to.

Right if users want to use a trigger then they would need
to re-attach the trigger after using the camera.

But this way at least they can use the LED for other purposes
which since many users don't use their webcam that often
might be interesting for some users ...

And this is consistent with how flash LEDs are handled.

> Also, I think it would be better if the LED device was marked as "no
> sysfs" when it is registered.

If we decide to permanently disallow userspace access then
yes this is an option. Or maybe better (to keep the LED
drivers generic), do the disable directly after the led_get() ?



> 
>> +	}
>> +
>> +	mutex_unlock(&sd->privacy_led->led_access);
>> +}
>> +#else
>> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {}
>> +#endif
>> +
>>  static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>  {
>>  	int ret;
>>  
>> +	call_s_stream_update_pled(sd, enable);
>> +
>>  	ret = sd->ops->video->s_stream(sd, enable);
>>  
>>  	if (!enable && ret < 0) {
> 
> You need to turn the LED off when enabling if .s_stream() fails.

Ack.

Regards,

Hans

Re: [PATCH v3 02/11] leds: led-class: Add __led_get() helper function

[Hans de Goede]

Hi,

On 12/16/22 14:54, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:04PM +0100, Hans de Goede wrote:
>> Split out part of of_led_get() into a generic __led_get() helper function.
>>
>> This is a preparation patch for adding a generic (non devicetree specific)
>> led_get() function.
> 
> ...
> 
>> +static struct led_classdev *__led_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;
>> +}
> 
> If I'm not mistaken, the entire series leaves this function as is.
> At the end I found three _get functions:
>  __led_get
>  led_get
>  __devm_led_get
> 
> 
> As far as I can see the above can be named more precisely, i.e.
> led_module_get(). Or alike if this sounds not good either.

led_module_get() sounds good, I will rename this for v4.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 12/16/22 14:50, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:05PM +0100, Hans de Goede wrote:
>> Turn of_led_get() into a more generic __of_led_get() helper function,
>> which can lookup LEDs in devicetree by either name or index.
> 
> ...
> 
>> +	/*
>> +	 * For named LEDs, first look up the name in the "leds-names" property.
>> +	 * If it cannot be found, then of_parse_phandle() will propagate the error.
>> +	 */
>> +	if (name)
>> +		index = of_property_match_string(np, "leds-names", name);
> 
> I can't find this property anywhere in the kernel. Is it new?

Yes and no, adding a foo-names property for foo[] arrays to be
able to get members by name is a standard template for devicetree
bindings. See e.g. the clock bindings:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml

> If so, where is the bindings?

As for why not document this, there are currently no devicetree users
and the devicetree maintainers have repeatedly let me know not to
submit new bindings for fwnode x86 bindings ...

> And why entire code can't be converted
> to use fwnode for this case?

This is a trivial change to allow the new functions to work
with devicetree. Note this series does not introduce any devicetree
users, hence no bindings. But it is good to have compatibility backed in
from day 1.

Converting to fwnode APIs would be more involved and I cannot test
those changes.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 12/16/22 14:43, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:07PM +0100, 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. The existing of support is extended to also support
>> getting a LED by function-name using the standard devicetree pattern
>> of adding a -names string array to map names to the indexes.
>>
>> For non devicetree platforms a lookup-table mechanism is added to
>> allow the platform code to map specific LED class_dev-s to specific
>> device,function combinations this way.
> 
> ...
> 
>> +	list_for_each_entry(lookup, &leds_lookup_list, list) {
>> +		if (!strcmp(lookup->consumer_dev_name, dev_name(dev)) &&
>> +		    !strcmp(lookup->consumer_function, function)) {
>> +			led_name = kstrdup(lookup->led_name, GFP_KERNEL);
> 
> kstrdup_const() ?

Ack.

> 
>> +			break;
>> +		}
>> +	}
> 
>> +	if (!led_name)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	led_dev = class_find_device_by_name(leds_class, led_name);
>> +	kfree(led_name);
> 
> kfree_const() ?

Ack, will change for v4.

> 
>> +	return __led_get(led_dev);
>> +}
> 
> ...
> 
>> +EXPORT_SYMBOL_GPL(led_add_lookup);
> 
>> +EXPORT_SYMBOL_GPL(led_remove_lookup);
> 
> Wondering why we can't make at least those two to be namespaced from day 1,

switching the LED subsystem code to use module namespaces is
a RFE which is independent of / orthogonal to this patchset.

Regards,

Hans

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 04:52:33PM +0100, Hans de Goede wrote:
> On 12/16/22 14:50, Andy Shevchenko wrote:
> > On Fri, Dec 16, 2022 at 12:30:05PM +0100, Hans de Goede wrote:

...

> >> +	/*
> >> +	 * For named LEDs, first look up the name in the "leds-names" property.
> >> +	 * If it cannot be found, then of_parse_phandle() will propagate the error.
> >> +	 */
> >> +	if (name)
> >> +		index = of_property_match_string(np, "leds-names", name);
> > 
> > I can't find this property anywhere in the kernel. Is it new?
> 
> Yes and no, adding a foo-names property for foo[] arrays to be
> able to get members by name is a standard template for devicetree
> bindings. See e.g. the clock bindings:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml
> 
> > If so, where is the bindings?
> 
> As for why not document this, there are currently no devicetree users
> and the devicetree maintainers have repeatedly let me know not to
> submit new bindings for fwnode x86 bindings ...

How above is related to fwnode as you put that? (I do see OF specific code
which is required to have a binding, right?)

> > And why entire code can't be converted
> > to use fwnode for this case?
> 
> This is a trivial change to allow the new functions to work
> with devicetree. Note this series does not introduce any devicetree
> users, hence no bindings. But it is good to have compatibility backed in
> from day 1.

AFAIU the OF properties must be documented from day 1.

> Converting to fwnode APIs would be more involved and I cannot test
> those changes.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 04:54:59PM +0100, Hans de Goede wrote:
> On 12/16/22 14:43, Andy Shevchenko wrote:
> > On Fri, Dec 16, 2022 at 12:30:07PM +0100, Hans de Goede wrote:

...

> >> +EXPORT_SYMBOL_GPL(led_add_lookup);
> > 
> >> +EXPORT_SYMBOL_GPL(led_remove_lookup);
> > 
> > Wondering why we can't make at least those two to be namespaced from day 1,
> 
> switching the LED subsystem code to use module namespaces is
> a RFE which is independent of / orthogonal to this patchset.

Adding new unnamespaced APIs only delays and increases a burden of the work.
We can start already for the logically separated subset of API, no? That's why
I asked about this.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 12/16/22 17:07, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 04:54:59PM +0100, Hans de Goede wrote:
>> On 12/16/22 14:43, Andy Shevchenko wrote:
>>> On Fri, Dec 16, 2022 at 12:30:07PM +0100, Hans de Goede wrote:
> 
> ...
> 
>>>> +EXPORT_SYMBOL_GPL(led_add_lookup);
>>>
>>>> +EXPORT_SYMBOL_GPL(led_remove_lookup);
>>>
>>> Wondering why we can't make at least those two to be namespaced from day 1,
>>
>> switching the LED subsystem code to use module namespaces is
>> a RFE which is independent of / orthogonal to this patchset.
> 
> Adding new unnamespaced APIs only delays and increases a burden of the work.
> We can start already for the logically separated subset of API, no? That's why
> I asked about this.

Sorry, but I really see this as an independent changes and there is
enough discussion around these changes as is without throwing this
into the mix.

Regards,

Hans

Re: [PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Hans de Goede]

Hi,

On 12/16/22 15:02, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote:
>> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
>> for sensors with a privacy LED, rather then having to duplicate this code
>> in all the sensor drivers.
> 
> ...
> 
>> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
>> +{
>> +	if (!sd->dev)
>> +		return;
>> +
>> +	/* Try to get privacy-led once, at first s_stream() */
>> +	if (!sd->privacy_led)
>> +		sd->privacy_led = led_get(sd->dev, "privacy-led");
> 
>> +
> 
> Redundant blank line?

I find this more readable with the blank line between the 2 ifs.
> 
>> +	if (IS_ERR(sd->privacy_led))
>> +		return;
> 
> I'm not sure I have got the logic right. Let's assume we call it with
> _led == NULL. Then in case of error, we feel it with the error pointer.
> If we call again, we check for NULL, and return error pointer.
> 
> So, we won't try the second time. Is it by design? Or should it be

It is by design, there even is a comment which says so:

/* Try to get privacy-led once, at first s_stream() */



Regards,

Hans



> 
> 	struct ... *led;
> 
> 	if (!privacy_led) {
> 		led = ...
> 		if (IS_ERR())
> 			return;
> 		privacy_led = led;
> 	}
> 
> ?
> 
>> +}
> 

Re: [PATCH v3 07/11] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Hans de Goede]

Hi,

On 12/16/22 14:57, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:09PM +0100, Hans de Goede wrote:
>> Add a helper function to map the type returned by the _DSM
>> method to a function name + the default polarity for that function.
>>
>> And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN
>> cases into a single generic case.
>>
>> This is a preparation patch for further GPIO mapping changes.
> 
> ...
> 
>> +static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
> 
> I find a bit strange not making this a function that returns func:
> 
> static const char *int3472_get_func_and_polarity(u8 type, u32 *polarity)

Why make it return func and not polarity ?

Since there are 2 values to return it makes sense to be
consistent and return both by reference.

Also this sort of comments really get into the territory
of bikeshedding which is not helpful IMHO.

Regards,

Hans




> 
>> +{
>> +	switch (type) {
>> +	case INT3472_GPIO_TYPE_RESET:
>> +		*func = "reset";
>> +		*polarity = GPIO_ACTIVE_LOW;
> 
> 		return "reset";
> 
> etc.
> 
>> +		break;
>> +	case INT3472_GPIO_TYPE_POWERDOWN:
>> +		*func = "powerdown";
>> +		*polarity = GPIO_ACTIVE_LOW;
>> +		break;
>> +	case INT3472_GPIO_TYPE_CLK_ENABLE:
>> +		*func = "clk-enable";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>> +	case INT3472_GPIO_TYPE_PRIVACY_LED:
>> +		*func = "privacy-led";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>> +	case INT3472_GPIO_TYPE_POWER_ENABLE:
>> +		*func = "power-enable";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>> +	default:
>> +		*func = "unknown";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>> +	}
>> +}
> 

Re: [PATCH v3 07/11] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 05:15:14PM +0100, Hans de Goede wrote:
> On 12/16/22 14:57, Andy Shevchenko wrote:
> > On Fri, Dec 16, 2022 at 12:30:09PM +0100, Hans de Goede wrote:

...

> >> +static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
> > 
> > I find a bit strange not making this a function that returns func:
> > 
> > static const char *int3472_get_func_and_polarity(u8 type, u32 *polarity)
> 
> Why make it return func and not polarity ?

Because of name "func" and "polarity". And as you read a prototype from left to
right it exactly follows that.

> Since there are 2 values to return it makes sense to be
> consistent and return both by reference.

But this is unneeded complication, no?

> Also this sort of comments really get into the territory
> of bikeshedding which is not helpful IMHO.

I find helpful to have a prototype shorter and the switch-case shorter due to
return vs break. Of course you can think it's unhelpful, but I have another
opinion. All by all you are the maintainer there, your call.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Hans de Goede]

Hi,

On 12/16/22 15:16, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote:
>> On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad
>> X1 Nano gen 2 there is no clock-enable pin, triggering the:
>> "No clk GPIO. The privacy LED won't work" warning and causing the privacy
>> LED to not work.
>>
>> Fix this by modeling the privacy LED as a LED class device rather then
>> integrating it with the registered clock.
>>
>> Note this relies on media subsys changes to actually turn the LED on/off
>> when the sensor's v4l2_subdev's s_stream() operand gets called.
> 
> ...
> 
>> +	struct int3472_pled {
>> +		char name[INT3472_LED_MAX_NAME_LEN];
>> +		struct led_lookup_data lookup;
> 
>> +		struct led_classdev classdev;
> 
> Why not putting this as a first member in the struct, so any container_of()
> against it become no-op at compile time?

Ack will fix for v4.

> 
>> +		struct gpio_desc *gpio;
>> +	} pled;
> 
> ...
> 
>> +	if (IS_ERR(int3472->pled.gpio)) {
>> +		ret = PTR_ERR(int3472->pled.gpio);
>> +		return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n");
> 
> 	return dev_err_probe(...);

That goes over 100 chars.


> 
>> +	}
> 
> ...
> 
>> +	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
>> +	snprintf(int3472->pled.name, sizeof(int3472->pled.name),
>> +		 "%s::privacy_led", acpi_dev_name(int3472->sensor));
> 
>> +	for (i = 0; int3472->pled.name[i]; i++) {
>> +		if (int3472->pled.name[i] == ':') {
>> +			int3472->pled.name[i] = '_';
>> +			break;
>> +		}
>> +	}
> 
> NIH strreplace().

Please look more careful, quoting from the strreplace() docs:

 * strreplace - Replace all occurrences of character in string.

Notice the *all* and we only want to replace the first ':' here,
because the ':' char has a special meaning in LED class-device-names.


> 
> ...
> 
>> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
>> +{
>> +	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
>> +		return;
> 
> This dups the check inside the _unregister() below, right?

Right.

> 
>> +	led_remove_lookup(&int3472->pled.lookup);
> 
> With list_del_init() I believe the above check can be droped.

No it cannot, list_del_init() inside led_remove_lookup() would
protect against double led_remove_lookup() calls.

But here we may have a completely uninitialized list_head on
devices without an INT3472 privacy-led, which will trigger
either __list_del_entry_valid() errors or lead to NULL
pointer derefs.


> 
>> +	led_classdev_unregister(&int3472->pled.classdev);
>> +	gpiod_put(int3472->pled.gpio);
>> +}
> 

Regards.

Hans

Re: [PATCH v3 09/11] platform/x86: int3472/discrete: Move GPIO request to skl_int3472_register_clock()

[Hans de Goede]

Hi,

On 12/16/22 15:20, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:11PM +0100, Hans de Goede wrote:
>> 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 .
> 
> Extra space.

Fixed in my local tree.

> 
> ...
> 
>> +	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(...); ?

That would make the line longer then 100 chars.

Regards,

Hans


> 
>> +		ret = PTR_ERR(int3472->clock.ena_gpio);
>> +		return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n");
>> +	}
> 

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

[Hans de Goede]

Hi,

On 12/16/22 15:57, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede wrote:
>> 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,
> 
> Here and in the code you actually can refer to it as 3rd byte.

3th byte or 4th byte? There is no universal convention for numbering
bytes in a word, so just using Bits 31-24 is unambiguous.

> 
>> 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.
> 
> ...
> 
>> +	/* 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)
> 
> Not sure why you need a temporary variable for this. Just use
> GENMASK()/GENMASK_ULL()?
> 
> 	if (obj->integer.value & GENMASK(31, 24));
> 
> In this case you even don't need to repeat bit numbers in the comment.

These bits contain the value to which the pin should be set when the
sensor is active (on), the active_value helper variable IMHO makes this
a lot more clear then directly checking the mask.

> 
>> +		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");
> 
> Yet another high/low :-)

Nope, this is the same patch as last time (when you were fine with
the other bits...).

Regards,

Hans

Re: [PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Laurent Pinchart]

Hi Hans,

On Fri, Dec 16, 2022 at 04:45:29PM +0100, Hans de Goede wrote:
> On 12/16/22 14:56, Laurent Pinchart wrote:
> > On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote:
> >> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
> >> for sensors with a privacy LED, rather then having to duplicate this code
> >> in all the sensor drivers.
> >>
> >> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++
> >>  include/media/v4l2-subdev.h           |  3 ++
> >>  2 files changed, 43 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >> index 4988a25bd8f4..7344f6cd58b7 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
> >>  	       sd->ops->pad->get_mbus_config(sd, pad, config);
> >>  }
> >>  
> >> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> >> +#include <linux/leds.h>
> > 
> > Can this be moved to the top of the file ? It doesn't have to be
> > conditionally compiled there.
> 
> You mean just the #include right? Ack to that.

Yes, that's what I meant.

> >> +
> >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
> >> +{
> >> +	if (!sd->dev)
> >> +		return;
> >> +
> >> +	/* Try to get privacy-led once, at first s_stream() */
> >> +	if (!sd->privacy_led)
> >> +		sd->privacy_led = led_get(sd->dev, "privacy-led");
> > 
> > I'm not sure I like this much. If the LED provider isn't available
> > (yet), the LED will stay off. That's a privacy concern.
> 
> At first I tried to put the led_get() inside v4l2_async_register_subdev_sensor(),
> which could then return an error like -EPROBE_DEFER if the led_get()
> fails (and nicely limits the led_get() to sensors).
> 
> The problem which I hit is that v4l2-fwnode.c is build according to
> CONFIG_V4L2_FWNODE where as v4l2-subdev.c is build according to
> CONFIG_VIDEO_DEV 
> 
> And e.g. CONFIG_VIDEO_DEV could be builtin while CONFIG_V4L2_FWNODE
> could be a module and then having the #if IS_REACHABLE(CONFIG_LEDS_CLASS)
> spread over the 2 could result in different answers in the different
> files ...
> 
> One solution here could be to change CONFIG_V4L2_FWNODE and V4L2_ASYNC
> to bools and link the (quite small) objects for these 2 into videodev.ko:
> 
> videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o

There's a long overdue simplification of Kconfig symbols in the
subsystem. Another option would be to compile both in a single module,
as they're often used together. I'll let Sakari chime in, I don't have a
strong preference.

> >> +
> >> +	if (IS_ERR(sd->privacy_led))
> >> +		return;
> >> +
> >> +	mutex_lock(&sd->privacy_led->led_access);
> >> +
> >> +	if (enable) {
> >> +		led_sysfs_disable(sd->privacy_led);
> >> +		led_trigger_remove(sd->privacy_led);
> >> +		led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
> >> +	} else {
> >> +		led_set_brightness(sd->privacy_led, 0);
> >> +		led_sysfs_enable(sd->privacy_led);
> > 
> > I don't think you should reenable control through sysfs here. I don't
> > really see a use case, and you've removed the trigger anyway, so the
> > behaviour would be quite inconsistent.
> 
> Hmm, I thought this was a good compromise, this way the LED
> can be used for other purposes when the sensor is off if users
> want to.
> 
> Right if users want to use a trigger then they would need
> to re-attach the trigger after using the camera.
> 
> But this way at least they can use the LED for other purposes
> which since many users don't use their webcam that often
> might be interesting for some users ...

If the privacy LED starts being used for other purposes, users may get
used to seeing it on at random times, which defeats the point of the
privacy LED in the first place.

> And this is consistent with how flash LEDs are handled.
> 
> > Also, I think it would be better if the LED device was marked as "no
> > sysfs" when it is registered.
> 
> If we decide to permanently disallow userspace access then
> yes this is an option. Or maybe better (to keep the LED
> drivers generic), do the disable directly after the led_get() ?

I suppose the small race condition wouldn't be a big issue, but if we
decide that the privacy LED should really not be used for user purposes,
then I'd still rather disable userspace access when registering the LED.

> >> +	}
> >> +
> >> +	mutex_unlock(&sd->privacy_led->led_access);
> >> +}
> >> +#else
> >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {}
> >> +#endif
> >> +
> >>  static int call_s_stream(struct v4l2_subdev *sd, int enable)
> >>  {
> >>  	int ret;
> >>  
> >> +	call_s_stream_update_pled(sd, enable);
> >> +
> >>  	ret = sd->ops->video->s_stream(sd, enable);
> >>  
> >>  	if (!enable && ret < 0) {
> > 
> > You need to turn the LED off when enabling if .s_stream() fails.
> 
> Ack.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Hans de Goede]

Hi,

On 12/16/22 17:44, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Fri, Dec 16, 2022 at 04:45:29PM +0100, Hans de Goede wrote:
>> On 12/16/22 14:56, Laurent Pinchart wrote:
>>> On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote:
>>>> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
>>>> for sensors with a privacy LED, rather then having to duplicate this code
>>>> in all the sensor drivers.
>>>>
>>>> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++
>>>>  include/media/v4l2-subdev.h           |  3 ++
>>>>  2 files changed, 43 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> index 4988a25bd8f4..7344f6cd58b7 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
>>>>  	       sd->ops->pad->get_mbus_config(sd, pad, config);
>>>>  }
>>>>  
>>>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>>>> +#include <linux/leds.h>
>>>
>>> Can this be moved to the top of the file ? It doesn't have to be
>>> conditionally compiled there.
>>
>> You mean just the #include right? Ack to that.
> 
> Yes, that's what I meant.
> 
>>>> +
>>>> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
>>>> +{
>>>> +	if (!sd->dev)
>>>> +		return;
>>>> +
>>>> +	/* Try to get privacy-led once, at first s_stream() */
>>>> +	if (!sd->privacy_led)
>>>> +		sd->privacy_led = led_get(sd->dev, "privacy-led");
>>>
>>> I'm not sure I like this much. If the LED provider isn't available
>>> (yet), the LED will stay off. That's a privacy concern.
>>
>> At first I tried to put the led_get() inside v4l2_async_register_subdev_sensor(),
>> which could then return an error like -EPROBE_DEFER if the led_get()
>> fails (and nicely limits the led_get() to sensors).
>>
>> The problem which I hit is that v4l2-fwnode.c is build according to
>> CONFIG_V4L2_FWNODE where as v4l2-subdev.c is build according to
>> CONFIG_VIDEO_DEV 
>>
>> And e.g. CONFIG_VIDEO_DEV could be builtin while CONFIG_V4L2_FWNODE
>> could be a module and then having the #if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> spread over the 2 could result in different answers in the different
>> files ...
>>
>> One solution here could be to change CONFIG_V4L2_FWNODE and V4L2_ASYNC
>> to bools and link the (quite small) objects for these 2 into videodev.ko:
>>
>> videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
>> videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
> 
> There's a long overdue simplification of Kconfig symbols in the
> subsystem. Another option would be to compile both in a single module,
> as they're often used together. I'll let Sakari chime in, I don't have a
> strong preference.
> 
>>>> +
>>>> +	if (IS_ERR(sd->privacy_led))
>>>> +		return;
>>>> +
>>>> +	mutex_lock(&sd->privacy_led->led_access);
>>>> +
>>>> +	if (enable) {
>>>> +		led_sysfs_disable(sd->privacy_led);
>>>> +		led_trigger_remove(sd->privacy_led);
>>>> +		led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
>>>> +	} else {
>>>> +		led_set_brightness(sd->privacy_led, 0);
>>>> +		led_sysfs_enable(sd->privacy_led);
>>>
>>> I don't think you should reenable control through sysfs here. I don't
>>> really see a use case, and you've removed the trigger anyway, so the
>>> behaviour would be quite inconsistent.
>>
>> Hmm, I thought this was a good compromise, this way the LED
>> can be used for other purposes when the sensor is off if users
>> want to.
>>
>> Right if users want to use a trigger then they would need
>> to re-attach the trigger after using the camera.
>>
>> But this way at least they can use the LED for other purposes
>> which since many users don't use their webcam that often
>> might be interesting for some users ...
> 
> If the privacy LED starts being used for other purposes, users may get
> used to seeing it on at random times, which defeats the point of the
> privacy LED in the first place.

Using it for other purposes it not something which I expect
e.g. distros to do OOTB, so normal users won't see the LED used
in another way.  But it may be useful for tinkerers who do this
as a local modification, in which case they know the LED
behavior.

With that said I'm fine with just disabling the sysfs interface
once at probe / register time.

Regards,

Hans


> 
>> And this is consistent with how flash LEDs are handled.
>>
>>> Also, I think it would be better if the LED device was marked as "no
>>> sysfs" when it is registered.
>>
>> If we decide to permanently disallow userspace access then
>> yes this is an option. Or maybe better (to keep the LED
>> drivers generic), do the disable directly after the led_get() ?
> 
> I suppose the small race condition wouldn't be a big issue, but if we
> decide that the privacy LED should really not be used for user purposes,
> then I'd still rather disable userspace access when registering the LED.
> 
>>>> +	}
>>>> +
>>>> +	mutex_unlock(&sd->privacy_led->led_access);
>>>> +}
>>>> +#else
>>>> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {}
>>>> +#endif
>>>> +
>>>>  static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>>>  {
>>>>  	int ret;
>>>>  
>>>> +	call_s_stream_update_pled(sd, enable);
>>>> +
>>>>  	ret = sd->ops->video->s_stream(sd, enable);
>>>>  
>>>>  	if (!enable && ret < 0) {
>>>
>>> You need to turn the LED off when enabling if .s_stream() fails.
>>
>> Ack.
> 

Re: [PATCH v3 08/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 05:29:13PM +0100, Hans de Goede wrote:
> On 12/16/22 15:16, Andy Shevchenko wrote:
> > On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote:

...

> >> +	if (IS_ERR(int3472->pled.gpio)) {
> >> +		ret = PTR_ERR(int3472->pled.gpio);
> >> +		return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n");
> > 
> > 	return dev_err_probe(...);
> 
> That goes over 100 chars.

The point is you don't need ret to be initialized. Moreover, no-one prevents
you to split the line to two.

> >> +	}

...

> >> +	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
> >> +	snprintf(int3472->pled.name, sizeof(int3472->pled.name),
> >> +		 "%s::privacy_led", acpi_dev_name(int3472->sensor));
> > 
> >> +	for (i = 0; int3472->pled.name[i]; i++) {
> >> +		if (int3472->pled.name[i] == ':') {
> >> +			int3472->pled.name[i] = '_';
> >> +			break;
> >> +		}
> >> +	}
> > 
> > NIH strreplace().
> 
> Please look more careful, quoting from the strreplace() docs:
> 
>  * strreplace - Replace all occurrences of character in string.
> 
> Notice the *all* and we only want to replace the first ':' here,
> because the ':' char has a special meaning in LED class-device-names.

It's still possible to use that, but anyway, the above is still
something NIH.

	char *p;

	p = strchr(name, ':');
	*p = '_';

But either code has an issue if by some reason you need to check if : is ever
present in acpi_dev_name().

The more robust is either to copy acpi_dev_name(), call strreplace(), so you
will be sure that _all_ : from ACPI device name will be covered and then attach
the rest.

...

> >> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
> >> +{
> >> +	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
> >> +		return;
> > 
> > This dups the check inside the _unregister() below, right?
> 
> Right.
> 
> >> +	led_remove_lookup(&int3472->pled.lookup);
> > 
> > With list_del_init() I believe the above check can be droped.
> 
> No it cannot, list_del_init() inside led_remove_lookup() would
> protect against double led_remove_lookup() calls.
> 
> But here we may have a completely uninitialized list_head on
> devices without an INT3472 privacy-led, which will trigger
> either __list_del_entry_valid() errors or lead to NULL
> pointer derefs.

But we can initialize that as well...

> >> +	led_classdev_unregister(&int3472->pled.classdev);
> >> +	gpiod_put(int3472->pled.gpio);
> >> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] platform/x86: int3472/discrete: Move GPIO request to skl_int3472_register_clock()

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 05:35:51PM +0100, Hans de Goede wrote:
> On 12/16/22 15:20, Andy Shevchenko wrote:
> > On Fri, Dec 16, 2022 at 12:30:11PM +0100, Hans de Goede wrote:

...

> >> +	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(...); ?
> 
> That would make the line longer then 100 chars.

Maybe I wasn't clear, the point is to get rid of ret assignment (the rest can
be resplit as you wish).

> >> +		ret = PTR_ERR(int3472->clock.ena_gpio);
> >> +		return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n");
> >> +	}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Dec 16, 2022 at 05:42:08PM +0100, Hans de Goede wrote:
> On 12/16/22 15:57, Andy Shevchenko wrote:
> > On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede wrote:

...

> >> +	/* 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)
> > 
> > Not sure why you need a temporary variable for this. Just use
> > GENMASK()/GENMASK_ULL()?
> > 
> > 	if (obj->integer.value & GENMASK(31, 24));
> > 
> > In this case you even don't need to repeat bit numbers in the comment.
> 
> These bits contain the value to which the pin should be set when the
> sensor is active (on), the active_value helper variable IMHO makes this
> a lot more clear then directly checking the mask.

Mask makes much more clear to understand what bits you are really use without
looking at the type of a temporary variable. (IIUC the value is u64.)

Maybe

	active_value = (obj->integer.value & GENMASK(31, 24)) >> 24;

But yes, this is on the edge of bikeshedding.

> >> +		polarity ^= GPIO_ACTIVE_LOW;

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Fri, Dec 16, 2022 at 12:30 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Add a generic [devm_]led_get() method which can be used on both devicetree
> and non devicetree platforms to get a LED classdev associated with
> a specific function on a specific device, e.g. the privacy LED associated
> with a specific camera sensor.
>
> Note unlike of_led_get() this takes a string describing the function
> rather then an index. This is done because e.g. camera sensors might
> have a privacy LED, or a flash LED, or both and using an index
> approach leaves it unclear what the function of index 0 is if there is
> only 1 LED. The existing of support is extended to also support
> getting a LED by function-name using the standard devicetree pattern
> of adding a -names string array to map names to the indexes.
>
> For non devicetree platforms a lookup-table mechanism is added to
> allow the platform code to map specific LED class_dev-s to specific
> device,function combinations this way.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I see you need to iron out some details but the concept is
clean cut and this is exactly what we want to do.

I think it was Bjorn Andersson who pointed out to me years
and years ago "why can't we do that, like any other
resource?" and here it is.

Yours,
Linus Walleij

Re: [PATCH v3 08/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Hans de Goede]

Hi,

On 12/16/22 18:14, Andy Shevchenko wrote:
> On Fri, Dec 16, 2022 at 05:29:13PM +0100, Hans de Goede wrote:
>> On 12/16/22 15:16, Andy Shevchenko wrote:
>>> On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote:
> 
> ...
> 
>>>> +	if (IS_ERR(int3472->pled.gpio)) {
>>>> +		ret = PTR_ERR(int3472->pled.gpio);
>>>> +		return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n");
>>>
>>> 	return dev_err_probe(...);
>>
>> That goes over 100 chars.
> 
> The point is you don't need ret to be initialized. Moreover, no-one prevents
> you to split the line to two.

The compiler is perfectly capable of optimizing away the store
in ret if that is not necessary; and splitting the line instead
of doing it above will just make the code harder to read.

Also this really is bikeshedding...

> 
>>>> +	}
> 
> ...
> 
>>>> +	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
>>>> +	snprintf(int3472->pled.name, sizeof(int3472->pled.name),
>>>> +		 "%s::privacy_led", acpi_dev_name(int3472->sensor));
>>>
>>>> +	for (i = 0; int3472->pled.name[i]; i++) {
>>>> +		if (int3472->pled.name[i] == ':') {
>>>> +			int3472->pled.name[i] = '_';
>>>> +			break;
>>>> +		}
>>>> +	}
>>>
>>> NIH strreplace().
>>
>> Please look more careful, quoting from the strreplace() docs:
>>
>>  * strreplace - Replace all occurrences of character in string.
>>
>> Notice the *all* and we only want to replace the first ':' here,
>> because the ':' char has a special meaning in LED class-device-names.
> 
> It's still possible to use that, but anyway, the above is still
> something NIH.
> 
> 	char *p;
> 
> 	p = strchr(name, ':');
> 	*p = '_';

Ok, In will switch to this for the next version.

> But either code has an issue if by some reason you need to check if : is ever
> present in acpi_dev_name().

acpi device names are set by this code:

        result = ida_alloc(instance_ida, GFP_KERNEL);
        if (result < 0)
                return result;

        device->pnp.instance_no = result;
        dev_set_name(&device->dev, "%s:%02x", acpi_device_bus_id->bus_id, result);

And the bus_id cannot have a : in it, so there always is a single :.


> 
> The more robust is either to copy acpi_dev_name(), call strreplace(), so you
> will be sure that _all_ : from ACPI device name will be covered and then attach
> the rest.
> 
> ...
> 
>>>> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
>>>> +{
>>>> +	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
>>>> +		return;
>>>
>>> This dups the check inside the _unregister() below, right?
>>
>> Right.
>>
>>>> +	led_remove_lookup(&int3472->pled.lookup);
>>>
>>> With list_del_init() I believe the above check can be droped.
>>
>> No it cannot, list_del_init() inside led_remove_lookup() would
>> protect against double led_remove_lookup() calls.
>>
>> But here we may have a completely uninitialized list_head on
>> devices without an INT3472 privacy-led, which will trigger
>> either __list_del_entry_valid() errors or lead to NULL
>> pointer derefs.
> 
> But we can initialize that as well...

The standard pattern in the kernel is that INIT_LIST_HEAD()
is only used for list_head-s which are actually used as the head
of the list. list_head-s used to track members of the list are
usually not initialized until they are added to the list.

Doing multiple list-init-s in multiple cases, including
one in *subsystem core code* just to drop an if here seems
counter productive.

Also checking that we can move forward with the unregister
is a good idea regardless of all the called functions being
able to run safely if the register never happened, because
future changes to the unregister function might end up
doing something which is unsafe when the LED was never
registered in the first place.

Regards,

Hans




> 
>>>> +	led_classdev_unregister(&int3472->pled.classdev);
>>>> +	gpiod_put(int3472->pled.gpio);
>>>> +}
>