Hi,
On 3/5/21 12:26 PM, Shawn Guo wrote:
> 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.
Not really it is still pointing to a wrong pin, the full identifier
for a pin in linux (see the userspace iocontrol interface also) is
<gpio-chip-name> + <index> so if we add support for remapping we should
add support for specifying the full pair. But your new approach with
the consumer is also interesting.
Maybe a generic override mechanism should use the following
inputs (indexing into the override map) and outputs:
In: consumer-dev-name + consumer-gpio-pin-name (e.g. "power", etc.)
Out: gpio-chip-name> + index + flags
That would also work in cases where GPIOs are just completely missing
from the DSDTs.
And to circle back to Andy's point about the current fix for his
case requiring duplicating lots of gpiolib-acpi code.
Maybe we need a gpiod_get_with_lookup_table() or some such
where the driver doing the gpiod_get call can specify a lookup
table to do the overriding of the broken DSDT.
I think we should even be able to make this non ACPI specific
this way.
This will allow drivers to avoid having to code all
the lookup code themselves, while still pushing the
responsibility of actually maintaining the lookup-overrides
inside the individual drivers.
Regards,
Hans
>
> 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;
>
