diff mbox series

[3/6] platform/x86: int3472/discrete: Treat privacy LED as regular GPIO

Message ID 20221129231149.697154-4-hdegoede@redhat.com (mailing list archive)
State Changes Requested, archived
Headers show
Series ov5693/int3472: Privacy LED handling changes + IPU6 compatibility | expand

Commit Message

Hans de Goede Nov. 29, 2022, 11:11 p.m. UTC 
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 treating the privacy LED as a regular GPIO rather then
integrating it with the registered clock.

Note this relies on the ov5693 driver change to support an (optional)
privacy-led GPIO to avoid the front cam privacy LED regressing on some
models.

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

Comments

Andy Shevchenko Nov. 30, 2022, 9:54 a.m. UTC | #1 
On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> 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 treating the privacy LED as a regular GPIO rather then
> integrating it with the registered clock.
>
> Note this relies on the ov5693 driver change to support an (optional)
> privacy-led GPIO to avoid the front cam privacy LED regressing on some
> models.

...

> -       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));
>
> -               int3472->clock.led_gpio = gpio;
> -               break;

I'm not sure how the previous patch makes this one work without
regressions. We have a "privacy-led" GPIO name there and here it used
to be with a prefix. Maybe I'm missing something...
Hans de Goede Nov. 30, 2022, 10:34 a.m. UTC | #2 
Hi,

On 11/30/22 10:54, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> 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 treating the privacy LED as a regular GPIO rather then
>> integrating it with the registered clock.
>>
>> Note this relies on the ov5693 driver change to support an (optional)
>> privacy-led GPIO to avoid the front cam privacy LED regressing on some
>> models.
> 
> ...
> 
>> -       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));
>>
>> -               int3472->clock.led_gpio = gpio;
>> -               break;
> 
> I'm not sure how the previous patch makes this one work without
> regressions. We have a "privacy-led" GPIO name there and here it used
> to be with a prefix. Maybe I'm missing something...

The GPIO used to be controlled as part of the clk-provider,
and the "int3472,privacy-led" name was the name of the consumer
of the GPIO shown in /sys/kernel/debug/gpio. The "int3472,privacy-led"
name has no lookup meaning since the pin is directly looked up by
GPIO chip ACPI path + pin offset here.

Since not all devices with a privacy LED also have a clk-enable GPIO
and thus a clk provider this did not work anywhere.

So this patch removes the code which controls the privacy LED
through the clk-provider (which used the "int3472,privacy-led"
and instead now adds an entry to the GPIO lookup table attached
to the sensor. That new GPIO lookup table entry uses the name
"privacy-led" since the LED no now longer is controlled by
the INT3472 code (*).  The matching sensor driver patch
(patch 1/6) to make the sensor driver directly control the
privacy-led also uses "privacy-led" when calling gpiod_get()
for it.

I hope this helps explain.

Regards,

Hans


*) all the INT3472 code now does is add the lookup table entry
gpio lookup table
Andy Shevchenko Nov. 30, 2022, 11:04 a.m. UTC | #3 
On Wed, Nov 30, 2022 at 11:34:57AM +0100, Hans de Goede wrote:
> On 11/30/22 10:54, Andy Shevchenko wrote:
> > On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> 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 treating the privacy LED as a regular GPIO rather then
> >> integrating it with the registered clock.
> >>
> >> Note this relies on the ov5693 driver change to support an (optional)
> >> privacy-led GPIO to avoid the front cam privacy LED regressing on some
> >> models.
> > 
> > ...
> > 
> >> -       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));
> >>
> >> -               int3472->clock.led_gpio = gpio;
> >> -               break;
> > 
> > I'm not sure how the previous patch makes this one work without
> > regressions. We have a "privacy-led" GPIO name there and here it used
> > to be with a prefix. Maybe I'm missing something...
> 
> The GPIO used to be controlled as part of the clk-provider,
> and the "int3472,privacy-led" name was the name of the consumer
> of the GPIO shown in /sys/kernel/debug/gpio. The "int3472,privacy-led"
> name has no lookup meaning since the pin is directly looked up by
> GPIO chip ACPI path + pin offset here.
> 
> Since not all devices with a privacy LED also have a clk-enable GPIO
> and thus a clk provider this did not work anywhere.
> 
> So this patch removes the code which controls the privacy LED
> through the clk-provider (which used the "int3472,privacy-led"
> and instead now adds an entry to the GPIO lookup table attached
> to the sensor. That new GPIO lookup table entry uses the name
> "privacy-led" since the LED no now longer is controlled by
> the INT3472 code (*).  The matching sensor driver patch
> (patch 1/6) to make the sensor driver directly control the
> privacy-led also uses "privacy-led" when calling gpiod_get()
> for it.
> 
> I hope this helps explain.

Definitely, thanks!

> *) all the INT3472 code now does is add the lookup table entry
> gpio lookup table
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 1cf958983e86..e61119b17677 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -23,8 +23,6 @@  static int skl_int3472_clk_prepare(struct clk_hw *hw)
 	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
 
 	gpiod_set_value_cansleep(clk->ena_gpio, 1);
-	gpiod_set_value_cansleep(clk->led_gpio, 1);
-
 	return 0;
 }
 
@@ -33,7 +31,6 @@  static void skl_int3472_clk_unprepare(struct clk_hw *hw)
 	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
 
 	gpiod_set_value_cansleep(clk->ena_gpio, 0);
-	gpiod_set_value_cansleep(clk->led_gpio, 0);
 }
 
 static int skl_int3472_clk_enable(struct clk_hw *hw)
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 53270d19c73a..c31321a586d4 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -96,7 +96,6 @@  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;
 
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 1eb053d13353..7887c6a4035e 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -155,33 +155,19 @@  static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 }
 
 static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
-				       struct acpi_resource_gpio *agpio, u8 type)
+				       struct acpi_resource_gpio *agpio)
 {
 	char *path = agpio->resource_source.string_ptr;
 	u16 pin = agpio->pin_table[0];
 	struct gpio_desc *gpio;
 
-	switch (type) {
-	case INT3472_GPIO_TYPE_CLK_ENABLE:
-		gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
-		if (IS_ERR(gpio))
-			return (PTR_ERR(gpio));
-
-		int3472->clock.ena_gpio = gpio;
-		break;
-	case INT3472_GPIO_TYPE_PRIVACY_LED:
-		gpio = acpi_get_and_request_gpiod(path, pin, "int3472,privacy-led");
-		if (IS_ERR(gpio))
-			return (PTR_ERR(gpio));
+	gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
+	if (IS_ERR(gpio))
+		return (PTR_ERR(gpio));
 
-		int3472->clock.led_gpio = gpio;
-		break;
-	default:
-		dev_err(int3472->dev, "Invalid GPIO type 0x%02x for clock\n", type);
-		break;
-	}
+	int3472->clock.ena_gpio = gpio;
 
-	return 0;
+	return skl_int3472_register_clock(int3472);
 }
 
 static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
@@ -282,14 +268,14 @@  static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
 	case INT3472_GPIO_TYPE_POWERDOWN:
+	case INT3472_GPIO_TYPE_PRIVACY_LED:
 		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
 		if (ret)
 			err_msg = "Failed to map GPIO pin to sensor\n";
 
 		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";
 
@@ -336,21 +322,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);
 
@@ -367,7 +338,6 @@  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_regulator(int3472);