Re: [PATCH] gpiolib: acpi: support override broken GPIO number in ACPI table

[Shawn Guo <shawn.guo@xxxxxxxxxx>]

On Fri, Mar 05, 2021 at 10:10:50AM +0100, Hans de Goede wrote:
> > So we reach a consensus that this is not the right solution for Lenovo
> > Flex 5G. But what about for Andy's Galileo Gen 2 case, where the GPIO
> > number in ACPI is truly broken?
> 
> Well if the ACPI table truely simply has a wrong number in it, like in
> this case, then we clearly need a workaround.
> 
> >   ba8c90c61847 ("gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2")
> 
> And we have one in place, so I'm not sure what the question is?
> 
> I guess the question is of your generic GPIO renumber patch would not
> be a better answer to that ?
> 
> IMHO no, we want to keep quirks out of the core as much as possible,
> for example the code which Andy added a quirk to is build as a module
> in the generic Fedora distro kernel, so for most users the code will
> not be loaded into memory. Where as if we add it to the core it would
> use up extra memory for everyone.

Fair point. I did not really think of it, because there is already
gpiolib_acpi_quirks[] in core code.  And on the other side, if there are
more drivers need such workaround, having each of these drivers copy the
same code is not ideal either.

> 
> Also if, in the future, we were to ever add a generic GPIO renumber quirk
> mechanism to the core, then your code would need more work. Because to be
> truely generic it would need to remap one gpiochip-name:pin-number on
> another gpiochip-name:pin-number pair. There might very well be a case
> with multiple gpiochips with pin number 32 being referenced in the DSDT
> and where we need to remap one of those 32-s to a different number
> (or possibly even to a different chip + number).

Yeah, I had already have v2 of my patch, just did not post it as the
overall direction is not agreed on.  I attach it here for discussion.
I think with the GPIO consumer specified, it should be good enough to
locate the broken GPIO number that needs override.  If gpiochip is
wrong, that means "\\_SB.GIO0" of GpioInt needs an override.  That's
a different issue.

GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
    "\\_SB.GIO0", 0x00, ResourceConsumer, ,
    )
    {   // Pin list
	0x0280
    }

Shawn


[PATCH v2] gpiolib: acpi: support override broken GPIO number in ACPI

Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
not working.  That's because the GpioInt number of TCPD node in ACPI
table is simply wrong, and the number even exceeds the maximum GPIO
lines.  As the touchpad works fine with Windows on the same machine,
presumably this is something Windows-ism.  Although it's obviously
a specification violation, believe of that Microsoft will fix this in
the near future is not really realistic.

It adds the support of overriding broken GPIO number in ACPI table
on particular machines, which are matched using DMI info.  Such
mechanism for fixing up broken firmware and ACPI table is not uncommon
in kernel.  And hopefully it can be useful for other machines that get
broken GPIO number coded in ACPI table.

The signature of acpi_get_gpiod() gets updated to pass over acpi_device
pointer of consumer device, so that the broken pin can be matched
precisely with consumer fwnode name.

Changes for v2:
- Match broken pin with additional consumer fwnode name comparison.

Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
---
 drivers/gpio/gpiolib-acpi.c | 79 +++++++++++++++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e37a57d0a2f0..fed045d64a26 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -93,6 +93,72 @@ static DEFINE_MUTEX(acpi_gpio_deferred_req_irqs_lock);
 static LIST_HEAD(acpi_gpio_deferred_req_irqs_list);
 static bool acpi_gpio_deferred_req_irqs_done;
 
+struct acpi_gpio_pin_fixup {
+	const char *consumer;
+	int pin_broken;
+	int pin_correct;
+};
+
+struct acpi_gpio_pin_override {
+	const struct acpi_gpio_pin_fixup *fixups;
+	int num;
+};
+
+static const struct acpi_gpio_pin_fixup lenovo_flex_5g_fixups[] = {
+	{
+		/* GpioInt of Touchpad */
+		.consumer = "\\_SB.I2C8.TCPD",
+		.pin_broken = 0x0280,
+		.pin_correct = 0x0018,
+	},
+};
+
+static const struct acpi_gpio_pin_override lenovo_flex_5g_override = {
+	.fixups = lenovo_flex_5g_fixups,
+	.num = ARRAY_SIZE(lenovo_flex_5g_fixups),
+};
+
+static const struct dmi_system_id acpi_gpio_pin_override_table[] = {
+	{
+		.ident = "Lenovo Flex 5G",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_FAMILY, "Flex 5G 14Q8CX05"),
+		},
+		.driver_data = (void *)&lenovo_flex_5g_override,
+	},
+	{ } /* terminator */
+};
+
+static int acpi_gpio_pin_override(struct acpi_device *adev, int pin)
+{
+	const struct acpi_gpio_pin_override *override;
+	const struct dmi_system_id *system_id;
+	char *fwname;
+	int ret = pin;
+	int i;
+
+	system_id = dmi_first_match(acpi_gpio_pin_override_table);
+	if (!system_id)
+		return ret;
+
+	fwname = kasprintf(GFP_KERNEL, "%pfwf", &adev->fwnode);
+	override = system_id->driver_data;
+
+	for (i = 0; i < override->num; i++) {
+		const struct acpi_gpio_pin_fixup *f = &override->fixups[i];
+
+		if (!strcmp(f->consumer, fwname) && pin == f->pin_broken) {
+			ret = f->pin_correct;
+			goto done;
+		}
+	}
+
+done:
+	kfree(fwname);
+	return ret;
+}
+
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
 	if (!gc->parent)
@@ -103,6 +169,7 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 
 /**
  * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
+ * @adev:	ACPI device that consumes the GPIO
  * @path:	ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
  * @pin:	ACPI GPIO pin number (0-based, controller-relative)
  *
@@ -111,7 +178,8 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
  * controller does not have GPIO chip registered at the moment. This is to
  * support probe deferral.
  */
-static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
+static struct gpio_desc *acpi_get_gpiod(struct acpi_device *adev,
+					char *path, int pin)
 {
 	struct gpio_chip *chip;
 	acpi_handle handle;
@@ -125,7 +193,11 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
 	if (!chip)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	return gpiochip_get_desc(chip, pin);
+	/*
+	 * Give it a chance to correct the broken GPIO pin number in ACPI
+	 * table of particular machines.
+	 */
+	return gpiochip_get_desc(chip, acpi_gpio_pin_override(adev, pin));
 }
 
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
@@ -689,7 +761,8 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		if (pin_index >= agpio->pin_table_length)
 			return 1;
 
-		lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
+		lookup->desc = acpi_get_gpiod(lookup->info.adev,
+					      agpio->resource_source.string_ptr,
 					      agpio->pin_table[pin_index]);
 		lookup->info.pin_config = agpio->pin_config;
 		lookup->info.debounce = agpio->debounce_timeout;
-- 
2.17.1