[PATCH v6 0/5] int3472/media privacy LED support

[PATCH v6 0/5] int3472/media privacy LED support

[Hans de Goede]

Hi All,

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

Changes in v6:
- The LED lookup series has been merged, immutable branch pull-req here:
  https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/
- Rework the media-core changes to correctly free the LED reference,
  this rework also allows dropping the patch to merge the async + fwnode
  code into videodev.ko
- Drop the patch merging the async + fwnode code into videodev.ko

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

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

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

Patch 1 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 add privacy-LED code to all the
camera sensor drivers separately (as requested by Sakari).

Patches 2-5 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.

This depends on the just merged LED lookup code from:
https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/

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 (5):
  media: v4l2-core: Make the v4l2-core code enable/disable the privacy
    LED if present
  platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
  platform/x86: int3472/discrete: Create a LED class device for the
    privacy LED
  platform/x86: int3472/discrete: Move GPIO request to
    skl_int3472_register_clock()
  platform/x86: int3472/discrete: Get the polarity from the _DSM entry

 drivers/media/v4l2-core/v4l2-async.c          |   4 +
 drivers/media/v4l2-core/v4l2-fwnode.c         |   7 ++
 drivers/media/v4l2-core/v4l2-subdev-priv.h    |  14 +++
 drivers/media/v4l2-core/v4l2-subdev.c         |  44 ++++++++
 drivers/platform/x86/intel/int3472/Makefile   |   2 +-
 .../x86/intel/int3472/clk_and_regulator.c     |  34 ++++--
 drivers/platform/x86/intel/int3472/common.h   |  18 +++-
 drivers/platform/x86/intel/int3472/discrete.c | 100 ++++++++----------
 drivers/platform/x86/intel/int3472/led.c      |  74 +++++++++++++
 include/media/v4l2-subdev.h                   |   3 +
 10 files changed, 234 insertions(+), 66 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-subdev-priv.h
 create mode 100644 drivers/platform/x86/intel/int3472/led.c

-- 
2.39.1

[PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[Hans de Goede]

Make v4l2_async_register_subdev_sensor() try to get a privacy LED
associated with the sensor and extend the call_s_stream() wrapper to
enable/disable the privacy LED if found.

This makes the core handle privacy LED control, rather then having to
duplicate this code in all the sensor drivers.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v6:
- Add v4l2_subdev_privacy_led_get()/_put() helpers
- At least the _put helper is necessary for cleanup on errors later on in
  v4l2_async_register_subdev_sensor()
- This puts all the LED related coded into a single file (v4l2-subdev.c)
  removing the need to build the async + fwnode code into videodev.ko,
  so that patch is dropped
- Move the (non-error-exit) cleanup from v4l2_subdev_cleanup() to
   v4l2_async_unregister_subdev()

Changes in v4 (requested by Laurent Pinchart):
- Move the led_get() call to v4l2_async_register_subdev_sensor() and
  make errors other then -ENOENT fail the register() call.
- Move the led_disable_sysfs() call to be done at led_get() time, instead
  of only disabling the sysfs interface when the sensor is streaming.
---
 drivers/media/v4l2-core/v4l2-async.c       |  4 ++
 drivers/media/v4l2-core/v4l2-fwnode.c      |  7 ++++
 drivers/media/v4l2-core/v4l2-subdev-priv.h | 14 +++++++
 drivers/media/v4l2-core/v4l2-subdev.c      | 44 ++++++++++++++++++++++
 include/media/v4l2-subdev.h                |  3 ++
 5 files changed, 72 insertions(+)
 create mode 100644 drivers/media/v4l2-core/v4l2-subdev-priv.h

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 2f1b718a9189..d7e9ffc7aa23 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -24,6 +24,8 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
+#include "v4l2-subdev-priv.h"
+
 static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
 				    struct v4l2_subdev *subdev,
 				    struct v4l2_async_subdev *asd)
@@ -822,6 +824,8 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 	if (!sd->async_list.next)
 		return;
 
+	v4l2_subdev_put_privacy_led(sd);
+
 	mutex_lock(&list_lock);
 
 	__v4l2_async_nf_unregister(sd->subdev_notifier);
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 3d9533c1b202..049c2f2001ea 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -28,6 +28,8 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
+#include "v4l2-subdev-priv.h"
+
 static const struct v4l2_fwnode_bus_conv {
 	enum v4l2_fwnode_bus_type fwnode_bus_type;
 	enum v4l2_mbus_type mbus_type;
@@ -1302,6 +1304,10 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
 
 	v4l2_async_nf_init(notifier);
 
+	ret = v4l2_subdev_get_privacy_led(sd);
+	if (ret < 0)
+		goto out_cleanup;
+
 	ret = v4l2_async_nf_parse_fwnode_sensor(sd->dev, notifier);
 	if (ret < 0)
 		goto out_cleanup;
@@ -1322,6 +1328,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
 	v4l2_async_nf_unregister(notifier);
 
 out_cleanup:
+	v4l2_subdev_put_privacy_led(sd);
 	v4l2_async_nf_cleanup(notifier);
 	kfree(notifier);
 
diff --git a/drivers/media/v4l2-core/v4l2-subdev-priv.h b/drivers/media/v4l2-core/v4l2-subdev-priv.h
new file mode 100644
index 000000000000..52391d6d8ab7
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-subdev-priv.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * V4L2 sub-device pivate header.
+ *
+ * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
+ */
+
+#ifndef _V4L2_SUBDEV_PRIV_H_
+#define _V4L2_SUBDEV_PRIV_H_
+
+int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd);
+void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd);
+
+#endif
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 4988a25bd8f4..9fd183628285 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/ioctl.h>
+#include <linux/leds.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -23,6 +24,8 @@
 #include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
 
+#include "v4l2-subdev-priv.h"
+
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 {
@@ -322,6 +325,14 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	int ret;
 
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
+	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
+		if (enable)
+			led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
+		else
+			led_set_brightness(sd->privacy_led, 0);
+	}
+#endif
 	ret = sd->ops->video->s_stream(sd, enable);
 
 	if (!enable && ret < 0) {
@@ -1090,6 +1101,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;
@@ -1105,3 +1117,35 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
 	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
+
+int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
+{
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
+	sd->privacy_led = led_get(sd->dev, "privacy-led");
+	if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
+		return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
+
+	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
+		mutex_lock(&sd->privacy_led->led_access);
+		led_sysfs_disable(sd->privacy_led);
+		led_trigger_remove(sd->privacy_led);
+		led_set_brightness(sd->privacy_led, 0);
+		mutex_unlock(&sd->privacy_led->led_access);
+	}
+#endif
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
+
+void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd)
+{
+#if IS_REACHABLE(CONFIG_LEDS_CLASS)
+	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
+		mutex_lock(&sd->privacy_led->led_access);
+		led_sysfs_enable(sd->privacy_led);
+		mutex_unlock(&sd->privacy_led->led_access);
+		led_put(sd->privacy_led);
+	}
+#endif
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_put_privacy_led);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b15fa9930f30..0547313f98cc 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -38,6 +38,7 @@ struct v4l2_subdev;
 struct v4l2_subdev_fh;
 struct tuner_setup;
 struct v4l2_mbus_frame_desc;
+struct led_classdev;
 
 /**
  * struct v4l2_decode_vbi_line - used to decode_vbi_line
@@ -982,6 +983,8 @@ struct v4l2_subdev {
 	 * appropriate functions.
 	 */
 
+	struct led_classdev *privacy_led;
+
 	/*
 	 * TODO: active_state should most likely be changed from a pointer to an
 	 * embedded field. For the time being it's kept as a pointer to more
-- 
2.39.1

[PATCH v6 2/5] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Hans de Goede]

Add a helper function to map the type returned by the _DSM
method to a function name + the default polarity for that function.

And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN
cases into a single generic case.

This is a preparation patch for further GPIO mapping changes.

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

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

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

[PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Hans de Goede]

On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad
X1 Nano gen 2 there is no clock-enable pin, triggering the:
"No clk GPIO. The privacy LED won't work" warning and causing the privacy
LED to not work.

Fix this by modeling the privacy LED as a LED class device rather then
integrating it with the registered clock.

Note this relies on media subsys changes to actually turn the LED on/off
when the sensor's v4l2_subdev's s_stream() operand gets called.

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

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

[PATCH v6 4/5] platform/x86: int3472/discrete: Move GPIO request to skl_int3472_register_clock()

[Hans de Goede]

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

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

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

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

[PATCH v6 5/5] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

[Hans de Goede]

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

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

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

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

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

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

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

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

Re: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[kernel test robot]

Hi Hans,

I love your patch! Yet something to improve:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
patch link:    https://lore.kernel.org/r/20230127203729.10205-4-hdegoede%40redhat.com
patch subject: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED
config: i386-randconfig-r004-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281537.fKVHsgf4-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
        git checkout d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/platform/

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

All errors (new ones prefixed by >>):

   In file included from drivers/platform/x86/intel/int3472/discrete.c:17:
>> drivers/platform/x86/intel/int3472/common.h:107:40: error: field 'lookup' has incomplete type
     107 |                 struct led_lookup_data lookup;
         |                                        ^~~~~~
--
   In file included from drivers/platform/x86/intel/int3472/led.c:7:
>> drivers/platform/x86/intel/int3472/common.h:107:40: error: field 'lookup' has incomplete type
     107 |                 struct led_lookup_data lookup;
         |                                        ^~~~~~
   drivers/platform/x86/intel/int3472/led.c: In function 'skl_int3472_register_pled':
>> drivers/platform/x86/intel/int3472/led.c:57:9: error: implicit declaration of function 'led_add_lookup'; did you mean 'd_can_lookup'? [-Werror=implicit-function-declaration]
      57 |         led_add_lookup(&int3472->pled.lookup);
         |         ^~~~~~~~~~~~~~
         |         d_can_lookup
   drivers/platform/x86/intel/int3472/led.c: In function 'skl_int3472_unregister_pled':
>> drivers/platform/x86/intel/int3472/led.c:71:9: error: implicit declaration of function 'led_remove_lookup' [-Werror=implicit-function-declaration]
      71 |         led_remove_lookup(&int3472->pled.lookup);
         |         ^~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/lookup +107 drivers/platform/x86/intel/int3472/common.h

    80	
    81	struct int3472_discrete_device {
    82		struct acpi_device *adev;
    83		struct device *dev;
    84		struct acpi_device *sensor;
    85		const char *sensor_name;
    86	
    87		const struct int3472_sensor_config *sensor_config;
    88	
    89		struct int3472_gpio_regulator {
    90			char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
    91			char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
    92			struct gpio_desc *gpio;
    93			struct regulator_dev *rdev;
    94			struct regulator_desc rdesc;
    95		} regulator;
    96	
    97		struct int3472_gpio_clock {
    98			struct clk *clk;
    99			struct clk_hw clk_hw;
   100			struct clk_lookup *cl;
   101			struct gpio_desc *ena_gpio;
   102			u32 frequency;
   103		} clock;
   104	
   105		struct int3472_pled {
   106			struct led_classdev classdev;
 > 107			struct led_lookup_data lookup;
   108			char name[INT3472_LED_MAX_NAME_LEN];
   109			struct gpio_desc *gpio;
   110		} pled;
   111	
   112		unsigned int ngpios; /* how many GPIOs have we seen */
   113		unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
   114		struct gpiod_lookup_table gpios;
   115	};
   116	

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

Re: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[kernel test robot]

Hi Hans,

I love your patch! Yet something to improve:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
config: riscv-randconfig-r026-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281534.9Z8xRsrX-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
        git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/media/v4l2-core/

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

All errors (new ones prefixed by >>):

>> drivers/media/v4l2-core/v4l2-subdev.c:1124:20: error: call to undeclared function 'led_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           sd->privacy_led = led_get(sd->dev, "privacy-led");
                             ^
>> drivers/media/v4l2-core/v4l2-subdev.c:1124:18: error: incompatible integer to pointer conversion assigning to 'struct led_classdev *' from 'int' [-Wint-conversion]
           sd->privacy_led = led_get(sd->dev, "privacy-led");
                           ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   2 errors generated.


vim +/led_get +1124 drivers/media/v4l2-core/v4l2-subdev.c

  1120	
  1121	int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
  1122	{
  1123	#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> 1124		sd->privacy_led = led_get(sd->dev, "privacy-led");
  1125		if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
  1126			return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
  1127	
  1128		if (!IS_ERR_OR_NULL(sd->privacy_led)) {
  1129			mutex_lock(&sd->privacy_led->led_access);
  1130			led_sysfs_disable(sd->privacy_led);
  1131			led_trigger_remove(sd->privacy_led);
  1132			led_set_brightness(sd->privacy_led, 0);
  1133			mutex_unlock(&sd->privacy_led->led_access);
  1134		}
  1135	#endif
  1136		return 0;
  1137	}
  1138	EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
  1139	

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

Re: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[kernel test robot]

Hi Hans,

I love your patch! Perhaps something to improve:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230128/202301281654.sPeSxigX-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
        git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/media/v4l2-core/

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

All warnings (new ones prefixed by >>):

   drivers/media/v4l2-core/v4l2-subdev.c: In function 'v4l2_subdev_get_privacy_led':
   drivers/media/v4l2-core/v4l2-subdev.c:1124:27: error: implicit declaration of function 'led_get'; did you mean 'key_get'? [-Werror=implicit-function-declaration]
    1124 |         sd->privacy_led = led_get(sd->dev, "privacy-led");
         |                           ^~~~~~~
         |                           key_get
>> drivers/media/v4l2-core/v4l2-subdev.c:1124:25: warning: assignment to 'struct led_classdev *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1124 |         sd->privacy_led = led_get(sd->dev, "privacy-led");
         |                         ^
   cc1: some warnings being treated as errors


vim +1124 drivers/media/v4l2-core/v4l2-subdev.c

  1120	
  1121	int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
  1122	{
  1123	#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> 1124		sd->privacy_led = led_get(sd->dev, "privacy-led");
  1125		if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
  1126			return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
  1127	
  1128		if (!IS_ERR_OR_NULL(sd->privacy_led)) {
  1129			mutex_lock(&sd->privacy_led->led_access);
  1130			led_sysfs_disable(sd->privacy_led);
  1131			led_trigger_remove(sd->privacy_led);
  1132			led_set_brightness(sd->privacy_led, 0);
  1133			mutex_unlock(&sd->privacy_led->led_access);
  1134		}
  1135	#endif
  1136		return 0;
  1137	}
  1138	EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
  1139	

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

Re: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[Hans de Goede]

Hi,

On 1/28/23 08:35, kernel test robot wrote:
> Hi Hans,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v6.2-rc5]
> [cannot apply to media-tree/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
> patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
> patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
> config: riscv-randconfig-r026-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281534.9Z8xRsrX-lkp@intel.com/config)
> compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install riscv cross compiling tool for clang build
>         # apt-get install binutils-riscv64-linux-gnu
>         # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
>         git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/media/v4l2-core/
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>>> drivers/media/v4l2-core/v4l2-subdev.c:1124:20: error: call to undeclared function 'led_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>            sd->privacy_led = led_get(sd->dev, "privacy-led");
>                              ^
>>> drivers/media/v4l2-core/v4l2-subdev.c:1124:18: error: incompatible integer to pointer conversion assigning to 'struct led_classdev *' from 'int' [-Wint-conversion]
>            sd->privacy_led = led_get(sd->dev, "privacy-led");
>                            ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    2 errors generated.

As mentioned in the cover-letter this series depends on this immutable-branch:

https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/

That branch not being present in the base used by LKP is what is causing this
error.

Regards,

Hans

Re: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Hans de Goede]

Hi,

On 1/28/23 08:24, kernel test robot wrote:
> Hi Hans,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v6.2-rc5]
> [cannot apply to media-tree/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
> patch link:    https://lore.kernel.org/r/20230127203729.10205-4-hdegoede%40redhat.com
> patch subject: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED
> config: i386-randconfig-r004-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281537.fKVHsgf4-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/intel-lab-lkp/linux/commit/d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
>         git checkout d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         make W=1 O=build_dir ARCH=i386 olddefconfig
>         make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/platform/
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from drivers/platform/x86/intel/int3472/discrete.c:17:
>>> drivers/platform/x86/intel/int3472/common.h:107:40: error: field 'lookup' has incomplete type
>      107 |                 struct led_lookup_data lookup;
>          |                                        ^~~~~~
> --
>    In file included from drivers/platform/x86/intel/int3472/led.c:7:
>>> drivers/platform/x86/intel/int3472/common.h:107:40: error: field 'lookup' has incomplete type
>      107 |                 struct led_lookup_data lookup;
>          |                                        ^~~~~~
>    drivers/platform/x86/intel/int3472/led.c: In function 'skl_int3472_register_pled':
>>> drivers/platform/x86/intel/int3472/led.c:57:9: error: implicit declaration of function 'led_add_lookup'; did you mean 'd_can_lookup'? [-Werror=implicit-function-declaration]
>       57 |         led_add_lookup(&int3472->pled.lookup);
>          |         ^~~~~~~~~~~~~~
>          |         d_can_lookup
>    drivers/platform/x86/intel/int3472/led.c: In function 'skl_int3472_unregister_pled':
>>> drivers/platform/x86/intel/int3472/led.c:71:9: error: implicit declaration of function 'led_remove_lookup' [-Werror=implicit-function-declaration]
>       71 |         led_remove_lookup(&int3472->pled.lookup);
>          |         ^~~~~~~~~~~~~~~~~
>    cc1: some warnings being treated as errors

As mentioned in the cover-letter this series depends on this immutable-branch:

https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/

That branch not being present in the base used by LKP is what is causing this
error.

Regards,

Hans

Re: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[kernel test robot]

Hi Hans,

I love your patch! Yet something to improve:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
patch link:    https://lore.kernel.org/r/20230127203729.10205-4-hdegoede%40redhat.com
patch subject: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED
config: x86_64-randconfig-a012-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281800.sm8woBKh-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
        git checkout d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/platform/

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

All errors (new ones prefixed by >>):

   In file included from drivers/platform/x86/intel/int3472/discrete.c:17:
>> drivers/platform/x86/intel/int3472/common.h:107:26: error: field has incomplete type 'struct led_lookup_data'
                   struct led_lookup_data lookup;
                                          ^
   drivers/platform/x86/intel/int3472/common.h:107:10: note: forward declaration of 'struct led_lookup_data'
                   struct led_lookup_data lookup;
                          ^
   1 error generated.
--
   In file included from drivers/platform/x86/intel/int3472/led.c:7:
>> drivers/platform/x86/intel/int3472/common.h:107:26: error: field has incomplete type 'struct led_lookup_data'
                   struct led_lookup_data lookup;
                                          ^
   drivers/platform/x86/intel/int3472/common.h:107:10: note: forward declaration of 'struct led_lookup_data'
                   struct led_lookup_data lookup;
                          ^
>> drivers/platform/x86/intel/int3472/led.c:57:2: error: implicit declaration of function 'led_add_lookup' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           led_add_lookup(&int3472->pled.lookup);
           ^
>> drivers/platform/x86/intel/int3472/led.c:71:2: error: implicit declaration of function 'led_remove_lookup' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           led_remove_lookup(&int3472->pled.lookup);
           ^
   3 errors generated.


vim +107 drivers/platform/x86/intel/int3472/common.h

    80	
    81	struct int3472_discrete_device {
    82		struct acpi_device *adev;
    83		struct device *dev;
    84		struct acpi_device *sensor;
    85		const char *sensor_name;
    86	
    87		const struct int3472_sensor_config *sensor_config;
    88	
    89		struct int3472_gpio_regulator {
    90			char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
    91			char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
    92			struct gpio_desc *gpio;
    93			struct regulator_dev *rdev;
    94			struct regulator_desc rdesc;
    95		} regulator;
    96	
    97		struct int3472_gpio_clock {
    98			struct clk *clk;
    99			struct clk_hw clk_hw;
   100			struct clk_lookup *cl;
   101			struct gpio_desc *ena_gpio;
   102			u32 frequency;
   103		} clock;
   104	
   105		struct int3472_pled {
   106			struct led_classdev classdev;
 > 107			struct led_lookup_data lookup;
   108			char name[INT3472_LED_MAX_NAME_LEN];
   109			struct gpio_desc *gpio;
   110		} pled;
   111	
   112		unsigned int ngpios; /* how many GPIOs have we seen */
   113		unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
   114		struct gpiod_lookup_table gpios;
   115	};
   116	

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

Re: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[Laurent Pinchart]

Hi Hans,

On Sat, Jan 28, 2023 at 10:41:15AM +0100, Hans de Goede wrote:
> On 1/28/23 08:35, kernel test robot wrote:
> > Hi Hans,
> > 
> > I love your patch! Yet something to improve:
> > 
> > [auto build test ERROR on linus/master]
> > [also build test ERROR on v6.2-rc5]
> > [cannot apply to media-tree/master]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
> > patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
> > patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
> > config: riscv-randconfig-r026-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281534.9Z8xRsrX-lkp@intel.com/config)
> > compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # install riscv cross compiling tool for clang build
> >         # apt-get install binutils-riscv64-linux-gnu
> >         # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
> >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> >         git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
> >         git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/media/v4l2-core/
> > 
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <lkp@intel.com>
> > 
> > All errors (new ones prefixed by >>):
> > 
> >>> drivers/media/v4l2-core/v4l2-subdev.c:1124:20: error: call to undeclared function 'led_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> >            sd->privacy_led = led_get(sd->dev, "privacy-led");
> >                              ^
> >>> drivers/media/v4l2-core/v4l2-subdev.c:1124:18: error: incompatible integer to pointer conversion assigning to 'struct led_classdev *' from 'int' [-Wint-conversion]
> >            sd->privacy_led = led_get(sd->dev, "privacy-led");
> >                            ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    2 errors generated.
> 
> As mentioned in the cover-letter this series depends on this immutable-branch:
> 
> https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/
> 
> That branch not being present in the base used by LKP is what is causing this
> error.

The --base argument to git-format-patch will record the base commit ID
in the cover letter, that can possibly help bots getting it right.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[Hans de Goede]

Hi,

On 1/28/23 14:42, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Sat, Jan 28, 2023 at 10:41:15AM +0100, Hans de Goede wrote:
>> On 1/28/23 08:35, kernel test robot wrote:
>>> Hi Hans,
>>>
>>> I love your patch! Yet something to improve:
>>>
>>> [auto build test ERROR on linus/master]
>>> [also build test ERROR on v6.2-rc5]
>>> [cannot apply to media-tree/master]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented in
>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>
>>> url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
>>> patch link:    https://lore.kernel.org/r/20230127203729.10205-2-hdegoede%40redhat.com
>>> patch subject: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present
>>> config: riscv-randconfig-r026-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281534.9Z8xRsrX-lkp@intel.com/config)
>>> compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
>>> reproduce (this is a W=1 build):
>>>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>>         chmod +x ~/bin/make.cross
>>>         # install riscv cross compiling tool for clang build
>>>         # apt-get install binutils-riscv64-linux-gnu
>>>         # https://github.com/intel-lab-lkp/linux/commit/000ccec1824b3256e3fc1a94079bb953f19faab5
>>>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>>>         git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
>>>         git checkout 000ccec1824b3256e3fc1a94079bb953f19faab5
>>>         # save the config file
>>>         mkdir build_dir && cp config build_dir/.config
>>>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
>>>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/media/v4l2-core/
>>>
>>> If you fix the issue, kindly add following tag where applicable
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> drivers/media/v4l2-core/v4l2-subdev.c:1124:20: error: call to undeclared function 'led_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>>>            sd->privacy_led = led_get(sd->dev, "privacy-led");
>>>                              ^
>>>>> drivers/media/v4l2-core/v4l2-subdev.c:1124:18: error: incompatible integer to pointer conversion assigning to 'struct led_classdev *' from 'int' [-Wint-conversion]
>>>            sd->privacy_led = led_get(sd->dev, "privacy-led");
>>>                            ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>    2 errors generated.
>>
>> As mentioned in the cover-letter this series depends on this immutable-branch:
>>
>> https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/
>>
>> That branch not being present in the base used by LKP is what is causing this
>> error.
> 
> The --base argument to git-format-patch will record the base commit ID
> in the cover letter, that can possibly help bots getting it right.

Thanks I was not aware of the --base argument, that is useful to know.

Regards,

Hans

Re: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Sakari Ailus]

Hi Hans,

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

While I suppose ACPI device names generally are shorter than
sizeof(int3472->pled.name), it'd be nice to still check p is non-NULL here,
just to be sure.

> +
> +	int3472->pled.classdev.name = int3472->pled.name;
> +	int3472->pled.classdev.max_brightness = 1;
> +	int3472->pled.classdev.brightness_set_blocking = int3472_pled_set;
> +
> +	ret = led_classdev_register(int3472->dev, &int3472->pled.classdev);
> +	if (ret)
> +		goto err_free_gpio;
> +
> +	int3472->pled.lookup.provider = int3472->pled.name;
> +	int3472->pled.lookup.dev_id = int3472->sensor_name;
> +	int3472->pled.lookup.con_id = "privacy-led";
> +	led_add_lookup(&int3472->pled.lookup);
> +
> +	return 0;
> +
> +err_free_gpio:
> +	gpiod_put(int3472->pled.gpio);
> +	return ret;
> +}
> +
> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
> +{
> +	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
> +		return;
> +
> +	led_remove_lookup(&int3472->pled.lookup);
> +	led_classdev_unregister(&int3472->pled.classdev);
> +	gpiod_put(int3472->pled.gpio);
> +}

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[Sakari Ailus]

Hi Hans,

On Fri, Jan 27, 2023 at 09:37:25PM +0100, Hans de Goede wrote:
> Make v4l2_async_register_subdev_sensor() try to get a privacy LED
> associated with the sensor and extend the call_s_stream() wrapper to
> enable/disable the privacy LED if found.
> 
> This makes the core handle privacy LED control, rather then having to
> duplicate this code in all the sensor drivers.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Please wrap the lines over 80, unless there are tangible reasons to keep
them as-is.

For this patch:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

On my behalf it can be merged via another tree, I don't expect conflicts.
Also cc Hans Verkuil.

And the rest:

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Please also see my comment on the 3rd patch.

> ---
> Changes in v6:
> - Add v4l2_subdev_privacy_led_get()/_put() helpers
> - At least the _put helper is necessary for cleanup on errors later on in
>   v4l2_async_register_subdev_sensor()
> - This puts all the LED related coded into a single file (v4l2-subdev.c)
>   removing the need to build the async + fwnode code into videodev.ko,
>   so that patch is dropped
> - Move the (non-error-exit) cleanup from v4l2_subdev_cleanup() to
>    v4l2_async_unregister_subdev()
> 
> Changes in v4 (requested by Laurent Pinchart):
> - Move the led_get() call to v4l2_async_register_subdev_sensor() and
>   make errors other then -ENOENT fail the register() call.
> - Move the led_disable_sysfs() call to be done at led_get() time, instead
>   of only disabling the sysfs interface when the sensor is streaming.
> ---
>  drivers/media/v4l2-core/v4l2-async.c       |  4 ++
>  drivers/media/v4l2-core/v4l2-fwnode.c      |  7 ++++
>  drivers/media/v4l2-core/v4l2-subdev-priv.h | 14 +++++++
>  drivers/media/v4l2-core/v4l2-subdev.c      | 44 ++++++++++++++++++++++
>  include/media/v4l2-subdev.h                |  3 ++
>  5 files changed, 72 insertions(+)
>  create mode 100644 drivers/media/v4l2-core/v4l2-subdev-priv.h
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 2f1b718a9189..d7e9ffc7aa23 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -24,6 +24,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "v4l2-subdev-priv.h"
> +
>  static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
>  				    struct v4l2_subdev *subdev,
>  				    struct v4l2_async_subdev *asd)
> @@ -822,6 +824,8 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  	if (!sd->async_list.next)
>  		return;
>  
> +	v4l2_subdev_put_privacy_led(sd);
> +
>  	mutex_lock(&list_lock);
>  
>  	__v4l2_async_nf_unregister(sd->subdev_notifier);
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3d9533c1b202..049c2f2001ea 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -28,6 +28,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "v4l2-subdev-priv.h"
> +
>  static const struct v4l2_fwnode_bus_conv {
>  	enum v4l2_fwnode_bus_type fwnode_bus_type;
>  	enum v4l2_mbus_type mbus_type;
> @@ -1302,6 +1304,10 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  
>  	v4l2_async_nf_init(notifier);
>  
> +	ret = v4l2_subdev_get_privacy_led(sd);
> +	if (ret < 0)
> +		goto out_cleanup;
> +
>  	ret = v4l2_async_nf_parse_fwnode_sensor(sd->dev, notifier);
>  	if (ret < 0)
>  		goto out_cleanup;
> @@ -1322,6 +1328,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  	v4l2_async_nf_unregister(notifier);
>  
>  out_cleanup:
> +	v4l2_subdev_put_privacy_led(sd);
>  	v4l2_async_nf_cleanup(notifier);
>  	kfree(notifier);
>  
> diff --git a/drivers/media/v4l2-core/v4l2-subdev-priv.h b/drivers/media/v4l2-core/v4l2-subdev-priv.h
> new file mode 100644
> index 000000000000..52391d6d8ab7
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-subdev-priv.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * V4L2 sub-device pivate header.
> + *
> + * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
> + */
> +
> +#ifndef _V4L2_SUBDEV_PRIV_H_
> +#define _V4L2_SUBDEV_PRIV_H_
> +
> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd);
> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd);
> +
> +#endif
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4988a25bd8f4..9fd183628285 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/ioctl.h>
> +#include <linux/leds.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> @@ -23,6 +24,8 @@
>  #include <media/v4l2-fh.h>
>  #include <media/v4l2-event.h>
>  
> +#include "v4l2-subdev-priv.h"
> +
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>  static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>  {
> @@ -322,6 +325,14 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	int ret;
>  
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		if (enable)
> +			led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
> +		else
> +			led_set_brightness(sd->privacy_led, 0);
> +	}
> +#endif
>  	ret = sd->ops->video->s_stream(sd, enable);
>  
>  	if (!enable && ret < 0) {
> @@ -1090,6 +1101,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;
> @@ -1105,3 +1117,35 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>  	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
> +
> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
> +{
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	sd->privacy_led = led_get(sd->dev, "privacy-led");
> +	if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
> +		return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
> +
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		mutex_lock(&sd->privacy_led->led_access);
> +		led_sysfs_disable(sd->privacy_led);
> +		led_trigger_remove(sd->privacy_led);
> +		led_set_brightness(sd->privacy_led, 0);
> +		mutex_unlock(&sd->privacy_led->led_access);
> +	}
> +#endif
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
> +
> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd)
> +{
> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
> +		mutex_lock(&sd->privacy_led->led_access);
> +		led_sysfs_enable(sd->privacy_led);
> +		mutex_unlock(&sd->privacy_led->led_access);
> +		led_put(sd->privacy_led);
> +	}
> +#endif
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_put_privacy_led);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index b15fa9930f30..0547313f98cc 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -38,6 +38,7 @@ struct v4l2_subdev;
>  struct v4l2_subdev_fh;
>  struct tuner_setup;
>  struct v4l2_mbus_frame_desc;
> +struct led_classdev;
>  
>  /**
>   * struct v4l2_decode_vbi_line - used to decode_vbi_line
> @@ -982,6 +983,8 @@ struct v4l2_subdev {
>  	 * appropriate functions.
>  	 */
>  
> +	struct led_classdev *privacy_led;
> +
>  	/*
>  	 * TODO: active_state should most likely be changed from a pointer to an
>  	 * embedded field. For the time being it's kept as a pointer to more

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Hans de Goede]

Hi,

On 1/30/23 11:12, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Jan 27, 2023 at 09:37:27PM +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.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v4:
>> - Make struct led_classdev the first member of the pled struct
>> - Use strchr to replace the : with _ in the acpi_dev_name()
>> ---
>>  drivers/platform/x86/intel/int3472/Makefile   |  2 +-
>>  .../x86/intel/int3472/clk_and_regulator.c     |  3 -
>>  drivers/platform/x86/intel/int3472/common.h   | 15 +++-
>>  drivers/platform/x86/intel/int3472/discrete.c | 58 ++++-----------
>>  drivers/platform/x86/intel/int3472/led.c      | 74 +++++++++++++++++++
>>  5 files changed, 105 insertions(+), 47 deletions(-)
>>  create mode 100644 drivers/platform/x86/intel/int3472/led.c
>>
>> diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile
>> index cfec7784c5c9..9f16cb514397 100644
>> --- a/drivers/platform/x86/intel/int3472/Makefile
>> +++ b/drivers/platform/x86/intel/int3472/Makefile
>> @@ -1,4 +1,4 @@
>>  obj-$(CONFIG_INTEL_SKL_INT3472)		+= intel_skl_int3472_discrete.o \
>>  					   intel_skl_int3472_tps68470.o
>> -intel_skl_int3472_discrete-y		:= discrete.o clk_and_regulator.o common.o
>> +intel_skl_int3472_discrete-y		:= discrete.o clk_and_regulator.o led.o common.o
>>  intel_skl_int3472_tps68470-y		:= tps68470.o tps68470_board_data.o common.o
>> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> index 74dc2cff799e..e3b597d93388 100644
>> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> @@ -23,8 +23,6 @@ static int skl_int3472_clk_prepare(struct clk_hw *hw)
>>  	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>>  
>>  	gpiod_set_value_cansleep(clk->ena_gpio, 1);
>> -	gpiod_set_value_cansleep(clk->led_gpio, 1);
>> -
>>  	return 0;
>>  }
>>  
>> @@ -33,7 +31,6 @@ static void skl_int3472_clk_unprepare(struct clk_hw *hw)
>>  	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>>  
>>  	gpiod_set_value_cansleep(clk->ena_gpio, 0);
>> -	gpiod_set_value_cansleep(clk->led_gpio, 0);
>>  }
>>  
>>  static int skl_int3472_clk_enable(struct clk_hw *hw)
>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
>> index 53270d19c73a..82dc37e08882 100644
>> --- a/drivers/platform/x86/intel/int3472/common.h
>> +++ b/drivers/platform/x86/intel/int3472/common.h
>> @@ -6,6 +6,7 @@
>>  
>>  #include <linux/clk-provider.h>
>>  #include <linux/gpio/machine.h>
>> +#include <linux/leds.h>
>>  #include <linux/regulator/driver.h>
>>  #include <linux/regulator/machine.h>
>>  #include <linux/types.h>
>> @@ -28,6 +29,8 @@
>>  #define GPIO_REGULATOR_NAME_LENGTH				21
>>  #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH			9
>>  
>> +#define INT3472_LED_MAX_NAME_LEN				32
>> +
>>  #define CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET			86
>>  
>>  #define INT3472_REGULATOR(_name, _supply, _ops)			\
>> @@ -96,10 +99,16 @@ struct int3472_discrete_device {
>>  		struct clk_hw clk_hw;
>>  		struct clk_lookup *cl;
>>  		struct gpio_desc *ena_gpio;
>> -		struct gpio_desc *led_gpio;
>>  		u32 frequency;
>>  	} clock;
>>  
>> +	struct int3472_pled {
>> +		struct led_classdev classdev;
>> +		struct led_lookup_data lookup;
>> +		char name[INT3472_LED_MAX_NAME_LEN];
>> +		struct gpio_desc *gpio;
>> +	} pled;
>> +
>>  	unsigned int ngpios; /* how many GPIOs have we seen */
>>  	unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
>>  	struct gpiod_lookup_table gpios;
>> @@ -119,4 +128,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>>  				   struct acpi_resource_gpio *agpio);
>>  void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472);
>>  
>> +int skl_int3472_register_pled(struct int3472_discrete_device *int3472,
>> +			      struct acpi_resource_gpio *agpio, u32 polarity);
>> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472);
>> +
>>  #endif
>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>> index 708d51f9b41d..38b1372e0745 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -155,37 +155,21 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
>>  }
>>  
>>  static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
>> -				       struct acpi_resource_gpio *agpio, u8 type)
>> +				       struct acpi_resource_gpio *agpio)
>>  {
>>  	char *path = agpio->resource_source.string_ptr;
>>  	u16 pin = agpio->pin_table[0];
>>  	struct gpio_desc *gpio;
>>  
>> -	switch (type) {
>> -	case INT3472_GPIO_TYPE_CLK_ENABLE:
>> -		gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
>> -		if (IS_ERR(gpio))
>> -			return (PTR_ERR(gpio));
>> -
>> -		int3472->clock.ena_gpio = gpio;
>> -		/* Ensure the pin is in output mode and non-active state */
>> -		gpiod_direction_output(int3472->clock.ena_gpio, 0);
>> -		break;
>> -	case INT3472_GPIO_TYPE_PRIVACY_LED:
>> -		gpio = acpi_get_and_request_gpiod(path, pin, "int3472,privacy-led");
>> -		if (IS_ERR(gpio))
>> -			return (PTR_ERR(gpio));
>> +	gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
>> +	if (IS_ERR(gpio))
>> +		return (PTR_ERR(gpio));
>>  
>> -		int3472->clock.led_gpio = gpio;
>> -		/* Ensure the pin is in output mode and non-active state */
>> -		gpiod_direction_output(int3472->clock.led_gpio, 0);
>> -		break;
>> -	default:
>> -		dev_err(int3472->dev, "Invalid GPIO type 0x%02x for clock\n", type);
>> -		break;
>> -	}
>> +	int3472->clock.ena_gpio = gpio;
>> +	/* Ensure the pin is in output mode and non-active state */
>> +	gpiod_direction_output(int3472->clock.ena_gpio, 0);
>>  
>> -	return 0;
>> +	return skl_int3472_register_clock(int3472);
>>  }
>>  
>>  static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
>> @@ -293,11 +277,16 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>>  
>>  		break;
>>  	case INT3472_GPIO_TYPE_CLK_ENABLE:
>> -	case INT3472_GPIO_TYPE_PRIVACY_LED:
>> -		ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type);
>> +		ret = skl_int3472_map_gpio_to_clk(int3472, agpio);
>>  		if (ret)
>>  			err_msg = "Failed to map GPIO to clock\n";
>>  
>> +		break;
>> +	case INT3472_GPIO_TYPE_PRIVACY_LED:
>> +		ret = skl_int3472_register_pled(int3472, agpio, polarity);
>> +		if (ret)
>> +			err_msg = "Failed to register LED\n";
>> +
>>  		break;
>>  	case INT3472_GPIO_TYPE_POWER_ENABLE:
>>  		ret = skl_int3472_register_regulator(int3472, agpio);
>> @@ -341,21 +330,6 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
>>  
>>  	acpi_dev_free_resource_list(&resource_list);
>>  
>> -	/*
>> -	 * If we find no clock enable GPIO pin then the privacy LED won't work.
>> -	 * We've never seen that situation, but it's possible. Warn the user so
>> -	 * it's clear what's happened.
>> -	 */
>> -	if (int3472->clock.ena_gpio) {
>> -		ret = skl_int3472_register_clock(int3472);
>> -		if (ret)
>> -			return ret;
>> -	} else {
>> -		if (int3472->clock.led_gpio)
>> -			dev_warn(int3472->dev,
>> -				 "No clk GPIO. The privacy LED won't work\n");
>> -	}
>> -
>>  	int3472->gpios.dev_id = int3472->sensor_name;
>>  	gpiod_add_lookup_table(&int3472->gpios);
>>  
>> @@ -372,8 +346,8 @@ static int skl_int3472_discrete_remove(struct platform_device *pdev)
>>  		skl_int3472_unregister_clock(int3472);
>>  
>>  	gpiod_put(int3472->clock.ena_gpio);
>> -	gpiod_put(int3472->clock.led_gpio);
>>  
>> +	skl_int3472_unregister_pled(int3472);
>>  	skl_int3472_unregister_regulator(int3472);
>>  
>>  	return 0;
>> diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
>> new file mode 100644
>> index 000000000000..251c6524458e
>> --- /dev/null
>> +++ b/drivers/platform/x86/intel/int3472/led.c
>> @@ -0,0 +1,74 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Author: Hans de Goede <hdegoede@redhat.com> */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/leds.h>
>> +#include "common.h"
>> +
>> +static int int3472_pled_set(struct led_classdev *led_cdev,
>> +				     enum led_brightness brightness)
>> +{
>> +	struct int3472_discrete_device *int3472 =
>> +		container_of(led_cdev, struct int3472_discrete_device, pled.classdev);
>> +
>> +	gpiod_set_value_cansleep(int3472->pled.gpio, brightness);
>> +	return 0;
>> +}
>> +
>> +int skl_int3472_register_pled(struct int3472_discrete_device *int3472,
>> +			      struct acpi_resource_gpio *agpio, u32 polarity)
>> +{
>> +	char *p, *path = agpio->resource_source.string_ptr;
>> +	int ret;
>> +
>> +	if (int3472->pled.classdev.dev)
>> +		return -EBUSY;
>> +
>> +	int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
>> +							     "int3472,privacy-led");
>> +	if (IS_ERR(int3472->pled.gpio))
>> +		return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio),
>> +				     "getting privacy LED GPIO\n");
>> +
>> +	if (polarity == GPIO_ACTIVE_LOW)
>> +		gpiod_toggle_active_low(int3472->pled.gpio);
>> +
>> +	/* Ensure the pin is in output mode and non-active state */
>> +	gpiod_direction_output(int3472->pled.gpio, 0);
>> +
>> +	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
>> +	snprintf(int3472->pled.name, sizeof(int3472->pled.name),
>> +		 "%s::privacy_led", acpi_dev_name(int3472->sensor));
>> +	p = strchr(int3472->pled.name, ':');
>> +	*p = '_';
> 
> While I suppose ACPI device names generally are shorter than
> sizeof(int3472->pled.name), it'd be nice to still check p is non-NULL here,
> just to be sure.

Sure, I've added a check for this while merging this.

Regards,

Hans



> 
>> +
>> +	int3472->pled.classdev.name = int3472->pled.name;
>> +	int3472->pled.classdev.max_brightness = 1;
>> +	int3472->pled.classdev.brightness_set_blocking = int3472_pled_set;
>> +
>> +	ret = led_classdev_register(int3472->dev, &int3472->pled.classdev);
>> +	if (ret)
>> +		goto err_free_gpio;
>> +
>> +	int3472->pled.lookup.provider = int3472->pled.name;
>> +	int3472->pled.lookup.dev_id = int3472->sensor_name;
>> +	int3472->pled.lookup.con_id = "privacy-led";
>> +	led_add_lookup(&int3472->pled.lookup);
>> +
>> +	return 0;
>> +
>> +err_free_gpio:
>> +	gpiod_put(int3472->pled.gpio);
>> +	return ret;
>> +}
>> +
>> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
>> +{
>> +	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
>> +		return;
>> +
>> +	led_remove_lookup(&int3472->pled.lookup);
>> +	led_classdev_unregister(&int3472->pled.classdev);
>> +	gpiod_put(int3472->pled.gpio);
>> +}
> 

Re: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[Hans de Goede]

Hi,

On 1/30/23 11:17, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Jan 27, 2023 at 09:37:25PM +0100, Hans de Goede wrote:
>> Make v4l2_async_register_subdev_sensor() try to get a privacy LED
>> associated with the sensor and extend the call_s_stream() wrapper to
>> enable/disable the privacy LED if found.
>>
>> This makes the core handle privacy LED control, rather then having to
>> duplicate this code in all the sensor drivers.
>>
>> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Please wrap the lines over 80, unless there are tangible reasons to keep
> them as-is.
> 
> For this patch:
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> On my behalf it can be merged via another tree, I don't expect conflicts.
> Also cc Hans Verkuil.

Thanks.

I've merged the entire series into my pdx86/review-hans branch now:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
(with the lines in this patch shortened to 80 chars).

Once the builders have had some time to play with this branch
(and once I've run some tests to make sure the patches still
work as expected) I will push to platform-drivers-x86/for-next .

Regards,

Hans





> 
> And the rest:
> 
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Please also see my comment on the 3rd patch.
> 
>> ---
>> Changes in v6:
>> - Add v4l2_subdev_privacy_led_get()/_put() helpers
>> - At least the _put helper is necessary for cleanup on errors later on in
>>   v4l2_async_register_subdev_sensor()
>> - This puts all the LED related coded into a single file (v4l2-subdev.c)
>>   removing the need to build the async + fwnode code into videodev.ko,
>>   so that patch is dropped
>> - Move the (non-error-exit) cleanup from v4l2_subdev_cleanup() to
>>    v4l2_async_unregister_subdev()
>>
>> Changes in v4 (requested by Laurent Pinchart):
>> - Move the led_get() call to v4l2_async_register_subdev_sensor() and
>>   make errors other then -ENOENT fail the register() call.
>> - Move the led_disable_sysfs() call to be done at led_get() time, instead
>>   of only disabling the sysfs interface when the sensor is streaming.
>> ---
>>  drivers/media/v4l2-core/v4l2-async.c       |  4 ++
>>  drivers/media/v4l2-core/v4l2-fwnode.c      |  7 ++++
>>  drivers/media/v4l2-core/v4l2-subdev-priv.h | 14 +++++++
>>  drivers/media/v4l2-core/v4l2-subdev.c      | 44 ++++++++++++++++++++++
>>  include/media/v4l2-subdev.h                |  3 ++
>>  5 files changed, 72 insertions(+)
>>  create mode 100644 drivers/media/v4l2-core/v4l2-subdev-priv.h
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index 2f1b718a9189..d7e9ffc7aa23 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -24,6 +24,8 @@
>>  #include <media/v4l2-fwnode.h>
>>  #include <media/v4l2-subdev.h>
>>  
>> +#include "v4l2-subdev-priv.h"
>> +
>>  static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
>>  				    struct v4l2_subdev *subdev,
>>  				    struct v4l2_async_subdev *asd)
>> @@ -822,6 +824,8 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>>  	if (!sd->async_list.next)
>>  		return;
>>  
>> +	v4l2_subdev_put_privacy_led(sd);
>> +
>>  	mutex_lock(&list_lock);
>>  
>>  	__v4l2_async_nf_unregister(sd->subdev_notifier);
>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>> index 3d9533c1b202..049c2f2001ea 100644
>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>> @@ -28,6 +28,8 @@
>>  #include <media/v4l2-fwnode.h>
>>  #include <media/v4l2-subdev.h>
>>  
>> +#include "v4l2-subdev-priv.h"
>> +
>>  static const struct v4l2_fwnode_bus_conv {
>>  	enum v4l2_fwnode_bus_type fwnode_bus_type;
>>  	enum v4l2_mbus_type mbus_type;
>> @@ -1302,6 +1304,10 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>>  
>>  	v4l2_async_nf_init(notifier);
>>  
>> +	ret = v4l2_subdev_get_privacy_led(sd);
>> +	if (ret < 0)
>> +		goto out_cleanup;
>> +
>>  	ret = v4l2_async_nf_parse_fwnode_sensor(sd->dev, notifier);
>>  	if (ret < 0)
>>  		goto out_cleanup;
>> @@ -1322,6 +1328,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>>  	v4l2_async_nf_unregister(notifier);
>>  
>>  out_cleanup:
>> +	v4l2_subdev_put_privacy_led(sd);
>>  	v4l2_async_nf_cleanup(notifier);
>>  	kfree(notifier);
>>  
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev-priv.h b/drivers/media/v4l2-core/v4l2-subdev-priv.h
>> new file mode 100644
>> index 000000000000..52391d6d8ab7
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-subdev-priv.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * V4L2 sub-device pivate header.
>> + *
>> + * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
>> + */
>> +
>> +#ifndef _V4L2_SUBDEV_PRIV_H_
>> +#define _V4L2_SUBDEV_PRIV_H_
>> +
>> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd);
>> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd);
>> +
>> +#endif
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 4988a25bd8f4..9fd183628285 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -9,6 +9,7 @@
>>   */
>>  
>>  #include <linux/ioctl.h>
>> +#include <linux/leds.h>
>>  #include <linux/mm.h>
>>  #include <linux/module.h>
>>  #include <linux/slab.h>
>> @@ -23,6 +24,8 @@
>>  #include <media/v4l2-fh.h>
>>  #include <media/v4l2-event.h>
>>  
>> +#include "v4l2-subdev-priv.h"
>> +
>>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>>  static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>>  {
>> @@ -322,6 +325,14 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
>>  {
>>  	int ret;
>>  
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> +		if (enable)
>> +			led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
>> +		else
>> +			led_set_brightness(sd->privacy_led, 0);
>> +	}
>> +#endif
>>  	ret = sd->ops->video->s_stream(sd, enable);
>>  
>>  	if (!enable && ret < 0) {
>> @@ -1090,6 +1101,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;
>> @@ -1105,3 +1117,35 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
>>  	v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
>> +
>> +int v4l2_subdev_get_privacy_led(struct v4l2_subdev *sd)
>> +{
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	sd->privacy_led = led_get(sd->dev, "privacy-led");
>> +	if (IS_ERR(sd->privacy_led) && PTR_ERR(sd->privacy_led) != -ENOENT)
>> +		return dev_err_probe(sd->dev, PTR_ERR(sd->privacy_led), "getting privacy LED\n");
>> +
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> +		mutex_lock(&sd->privacy_led->led_access);
>> +		led_sysfs_disable(sd->privacy_led);
>> +		led_trigger_remove(sd->privacy_led);
>> +		led_set_brightness(sd->privacy_led, 0);
>> +		mutex_unlock(&sd->privacy_led->led_access);
>> +	}
>> +#endif
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_privacy_led);
>> +
>> +void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd)
>> +{
>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
>> +	if (!IS_ERR_OR_NULL(sd->privacy_led)) {
>> +		mutex_lock(&sd->privacy_led->led_access);
>> +		led_sysfs_enable(sd->privacy_led);
>> +		mutex_unlock(&sd->privacy_led->led_access);
>> +		led_put(sd->privacy_led);
>> +	}
>> +#endif
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_subdev_put_privacy_led);
>> 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
> 

Re: [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present

[Linus Walleij]

On Mon, Jan 30, 2023 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 1/30/23 11:17, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Fri, Jan 27, 2023 at 09:37:25PM +0100, Hans de Goede wrote:
> >> Make v4l2_async_register_subdev_sensor() try to get a privacy LED
> >> associated with the sensor and extend the call_s_stream() wrapper to
> >> enable/disable the privacy LED if found.
> >>
> >> This makes the core handle privacy LED control, rather then having to
> >> duplicate this code in all the sensor drivers.
> >>
> >> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >
> > Please wrap the lines over 80, unless there are tangible reasons to keep
> > them as-is.
> >
> > For this patch:
> >
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > On my behalf it can be merged via another tree, I don't expect conflicts.
> > Also cc Hans Verkuil.
>
> Thanks.
>
> I've merged the entire series into my pdx86/review-hans branch now:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> (with the lines in this patch shortened to 80 chars).
>
> Once the builders have had some time to play with this branch
> (and once I've run some tests to make sure the patches still
> work as expected) I will push to platform-drivers-x86/for-next .

Excellent progress with this Hans, thanks for investing so heavily
in getting this right after my initial complaints, the result is extremely
appealing an useful.

Yours,
Linus Walleij