| Message ID | 20200520211916.25727-5-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/5] gpiolib: acpi: Introduce opaque data field for quirks | expand |
On Thu, May 21, 2020 at 12:19:16AM +0300, Andy Shevchenko wrote: > ACPI table on Intel Galileo Gen 2 has wrong pin number for IRQ resource > of one of the I²C GPIO expanders. ACPI GPIO library provides a special > quirk which we may use in this case. With help of it, override GpioInt() > pin for the affected platform. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpio-pca953x.c | 44 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c > index 1fca8dd7824f..2014563309be 100644 > --- a/drivers/gpio/gpio-pca953x.c > +++ b/drivers/gpio/gpio-pca953x.c > @@ -10,6 +10,7 @@ > > #include <linux/acpi.h> > #include <linux/bitmap.h> > +#include <linux/dmi.h> > #include <linux/gpio/driver.h> > #include <linux/gpio/consumer.h> > #include <linux/i2c.h> > @@ -113,6 +114,39 @@ static const struct acpi_device_id pca953x_acpi_ids[] = { > }; > MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids); > > +#ifdef CONFIG_GPIO_PCA953X_IRQ > +static const struct acpi_gpio_params pca953x_interrupt_gpios = { 0, 0, true }; > + > +static const struct acpi_gpio_mapping pca953x_acpi_interrupt_gpios[] = { > + { "interrupt-gpios", &pca953x_interrupt_gpios, 1, ACPI_GPIO_QUIRK_FORCE_PIN, 1 }, > + { } > +}; > + > +static int pca953x_acpi_interrupt_get_irq(struct device *dev) > +{ > + struct gpio_desc *desc; > + > + if (devm_acpi_dev_add_driver_gpios(dev, pca953x_acpi_interrupt_gpios)) > + dev_warn(dev, "can't add GPIO ACPI mapping\n"); > + > + desc = devm_gpiod_get(dev, "interrupt", GPIOD_IN); > + if (IS_ERR(desc)) > + return PTR_ERR(desc); > + > + return gpiod_to_irq(desc); > +} > + > +static const struct dmi_system_id pca953x_dmi_acpi_interrupt_info[] = { > + { > + .ident = "Intel Galileo Gen 2", > + .matches = { > + DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"), > + }, > + }, > + {} Since you do everything already in this driver, I think we can live without adding ACPI_GPIO_QUIRK_FORCE_PIN to the core code at all.
On Mon, May 25, 2020 at 12:20:28PM +0300, Mika Westerberg wrote: > On Thu, May 21, 2020 at 12:19:16AM +0300, Andy Shevchenko wrote: > > ACPI table on Intel Galileo Gen 2 has wrong pin number for IRQ resource > > of one of the I²C GPIO expanders. ACPI GPIO library provides a special > > quirk which we may use in this case. With help of it, override GpioInt() > > pin for the affected platform. ... > > +static const struct acpi_gpio_params pca953x_interrupt_gpios = { 0, 0, true }; > > + > > +static const struct acpi_gpio_mapping pca953x_acpi_interrupt_gpios[] = { > > + { "interrupt-gpios", &pca953x_interrupt_gpios, 1, ACPI_GPIO_QUIRK_FORCE_PIN, 1 }, > > + { } > > +}; > > + > > +static int pca953x_acpi_interrupt_get_irq(struct device *dev) > > +{ > > + struct gpio_desc *desc; > > + > > + if (devm_acpi_dev_add_driver_gpios(dev, pca953x_acpi_interrupt_gpios)) > > + dev_warn(dev, "can't add GPIO ACPI mapping\n"); > > + > > + desc = devm_gpiod_get(dev, "interrupt", GPIOD_IN); > > + if (IS_ERR(desc)) > > + return PTR_ERR(desc); > > + > > + return gpiod_to_irq(desc); > > +} > > + > > +static const struct dmi_system_id pca953x_dmi_acpi_interrupt_info[] = { > > + { > > + .ident = "Intel Galileo Gen 2", > > + .matches = { > > + DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"), > > + }, > > + }, > > + {} > > Since you do everything already in this driver, I think we can live > without adding ACPI_GPIO_QUIRK_FORCE_PIN to the core code at all. Hmm... I don't see how (perhaps need morning coffee). Any pointers?
On Mon, May 25, 2020 at 12:31:50PM +0300, Andy Shevchenko wrote: > On Mon, May 25, 2020 at 12:20:28PM +0300, Mika Westerberg wrote: > > On Thu, May 21, 2020 at 12:19:16AM +0300, Andy Shevchenko wrote: > > > ACPI table on Intel Galileo Gen 2 has wrong pin number for IRQ resource > > > of one of the I²C GPIO expanders. ACPI GPIO library provides a special > > > quirk which we may use in this case. With help of it, override GpioInt() > > > pin for the affected platform. > > ... > > > > +static const struct acpi_gpio_params pca953x_interrupt_gpios = { 0, 0, true }; > > > + > > > +static const struct acpi_gpio_mapping pca953x_acpi_interrupt_gpios[] = { > > > + { "interrupt-gpios", &pca953x_interrupt_gpios, 1, ACPI_GPIO_QUIRK_FORCE_PIN, 1 }, > > > + { } > > > +}; > > > + > > > +static int pca953x_acpi_interrupt_get_irq(struct device *dev) > > > +{ > > > + struct gpio_desc *desc; > > > + > > > + if (devm_acpi_dev_add_driver_gpios(dev, pca953x_acpi_interrupt_gpios)) > > > + dev_warn(dev, "can't add GPIO ACPI mapping\n"); > > > + > > > + desc = devm_gpiod_get(dev, "interrupt", GPIOD_IN); > > > + if (IS_ERR(desc)) > > > + return PTR_ERR(desc); > > > + > > > + return gpiod_to_irq(desc); > > > +} > > > + > > > +static const struct dmi_system_id pca953x_dmi_acpi_interrupt_info[] = { > > > + { > > > + .ident = "Intel Galileo Gen 2", > > > + .matches = { > > > + DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"), > > > + }, > > > + }, > > > + {} > > > > Since you do everything already in this driver, I think we can live > > without adding ACPI_GPIO_QUIRK_FORCE_PIN to the core code at all. > > Hmm... I don't see how (perhaps need morning coffee). Any pointers? Well you already know all the details in this driver, no? Why you need to pass any of this information to the core and the back to the same driver?
On Mon, May 25, 2020 at 12:45:53PM +0300, Mika Westerberg wrote: > On Mon, May 25, 2020 at 12:31:50PM +0300, Andy Shevchenko wrote: > > On Mon, May 25, 2020 at 12:20:28PM +0300, Mika Westerberg wrote: > > > On Thu, May 21, 2020 at 12:19:16AM +0300, Andy Shevchenko wrote: > > > > ACPI table on Intel Galileo Gen 2 has wrong pin number for IRQ resource > > > > of one of the I²C GPIO expanders. ACPI GPIO library provides a special > > > > quirk which we may use in this case. With help of it, override GpioInt() > > > > pin for the affected platform. ... > > > > +static const struct acpi_gpio_params pca953x_interrupt_gpios = { 0, 0, true }; > > > > + > > > > +static const struct acpi_gpio_mapping pca953x_acpi_interrupt_gpios[] = { > > > > + { "interrupt-gpios", &pca953x_interrupt_gpios, 1, ACPI_GPIO_QUIRK_FORCE_PIN, 1 }, > > > > + { } > > > > +}; > > > > + > > > > +static int pca953x_acpi_interrupt_get_irq(struct device *dev) > > > > +{ > > > > + struct gpio_desc *desc; > > > > + > > > > + if (devm_acpi_dev_add_driver_gpios(dev, pca953x_acpi_interrupt_gpios)) > > > > + dev_warn(dev, "can't add GPIO ACPI mapping\n"); > > > > + > > > > + desc = devm_gpiod_get(dev, "interrupt", GPIOD_IN); > > > > + if (IS_ERR(desc)) > > > > + return PTR_ERR(desc); > > > > + > > > > + return gpiod_to_irq(desc); > > > > +} > > > > + > > > > +static const struct dmi_system_id pca953x_dmi_acpi_interrupt_info[] = { > > > > + { > > > > + .ident = "Intel Galileo Gen 2", > > > > + .matches = { > > > > + DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"), > > > > + }, > > > > + }, > > > > + {} > > > Since you do everything already in this driver, I think we can live > > > without adding ACPI_GPIO_QUIRK_FORCE_PIN to the core code at all. > > Hmm... I don't see how (perhaps need morning coffee). Any pointers? > Well you already know all the details in this driver, no? Why you need > to pass any of this information to the core and the back to the same > driver? Due to parsing of ACPI tables. I don't want to copy'n'paste 25% of gpiolib-acpi.c in here. I think provided solution is cleaner and (more) flexible in terms of maintenance.
On Mon, May 25, 2020 at 01:13:35PM +0300, Andy Shevchenko wrote: > Due to parsing of ACPI tables. I don't want to copy'n'paste 25% of > gpiolib-acpi.c in here. I think provided solution is cleaner and (more) > flexible in terms of maintenance. Hmm, you seem to pass a hard-coded pin number (1) to the core that then passes it back to the driver. Why you can't simple use that number here directly? You don't need to parse anything. What I'm missing? :-)
On Mon, May 25, 2020 at 02:05:56PM +0300, Mika Westerberg wrote: > On Mon, May 25, 2020 at 01:13:35PM +0300, Andy Shevchenko wrote: > > Due to parsing of ACPI tables. I don't want to copy'n'paste 25% of > > gpiolib-acpi.c in here. I think provided solution is cleaner and (more) > > flexible in terms of maintenance. > > Hmm, you seem to pass a hard-coded pin number (1) to the core that then > passes it back to the driver. Why you can't simple use that number here > directly? You don't need to parse anything. What I'm missing? :-) Okay, so, AFAIU you are proposing something like this: 1) find a GPIO controller by the ACPI path (somehow, I guess by finding a handle followed by physical device behind it); 2) somehow to request a pin from that device by number; 3) convert to IRQ and use. Is it correct?
On Mon, May 25, 2020 at 02:35:51PM +0300, Andy Shevchenko wrote: > On Mon, May 25, 2020 at 02:05:56PM +0300, Mika Westerberg wrote: > > On Mon, May 25, 2020 at 01:13:35PM +0300, Andy Shevchenko wrote: > > > Due to parsing of ACPI tables. I don't want to copy'n'paste 25% of > > > gpiolib-acpi.c in here. I think provided solution is cleaner and (more) > > > flexible in terms of maintenance. > > > > Hmm, you seem to pass a hard-coded pin number (1) to the core that then > > passes it back to the driver. Why you can't simple use that number here > > directly? You don't need to parse anything. What I'm missing? :-) > > Okay, so, AFAIU you are proposing something like this: > > 1) find a GPIO controller by the ACPI path (somehow, I guess by finding a > handle followed by physical device behind it); 2) somehow to request a > pin from that device by number; > 3) convert to IRQ and use. > > Is it correct? Ah, and before all these, to detect properly the IO expander that actually has that resource in the table (something like gpiod_count() or do we have better approach to answer the question "does this device has a GpioInt() resource?").
On Mon, May 25, 2020 at 02:35:51PM +0300, Andy Shevchenko wrote: > On Mon, May 25, 2020 at 02:05:56PM +0300, Mika Westerberg wrote: > > On Mon, May 25, 2020 at 01:13:35PM +0300, Andy Shevchenko wrote: > > > Due to parsing of ACPI tables. I don't want to copy'n'paste 25% of > > > gpiolib-acpi.c in here. I think provided solution is cleaner and (more) > > > flexible in terms of maintenance. > > > > Hmm, you seem to pass a hard-coded pin number (1) to the core that then > > passes it back to the driver. Why you can't simple use that number here > > directly? You don't need to parse anything. What I'm missing? :-) > > Okay, so, AFAIU you are proposing something like this: > > 1) find a GPIO controller by the ACPI path (somehow, I guess by finding a > handle followed by physical device behind it); 2) somehow to request a > pin from that device by number; > 3) convert to IRQ and use. > > Is it correct? Well, no. In the first patch you do this: pin = lookup->info.quirks_data; and this essentially becomes 1 so you know the pin number upfront in the driver. Why not simply get GPIO number 1 in the driver and use it as an interrupt? You know already that this particular board with the matching DMI identifier always uses the this number anyway.
On Mon, May 25, 2020 at 03:21:36PM +0300, Mika Westerberg wrote: > On Mon, May 25, 2020 at 02:35:51PM +0300, Andy Shevchenko wrote: > > On Mon, May 25, 2020 at 02:05:56PM +0300, Mika Westerberg wrote: > > > On Mon, May 25, 2020 at 01:13:35PM +0300, Andy Shevchenko wrote: > > > > Due to parsing of ACPI tables. I don't want to copy'n'paste 25% of > > > > gpiolib-acpi.c in here. I think provided solution is cleaner and (more) > > > > flexible in terms of maintenance. > > > > > > Hmm, you seem to pass a hard-coded pin number (1) to the core that then > > > passes it back to the driver. Why you can't simple use that number here > > > directly? You don't need to parse anything. What I'm missing? :-) > > > > Okay, so, AFAIU you are proposing something like this: > > > > 1) find a GPIO controller by the ACPI path (somehow, I guess by finding a > > handle followed by physical device behind it); 2) somehow to request a > > pin from that device by number; > > 3) convert to IRQ and use. > > > > Is it correct? > > Well, no. In the first patch you do this: > > pin = lookup->info.quirks_data; > > and this essentially becomes 1 so you know the pin number upfront in the > driver. Why not simply get GPIO number 1 in the driver and use it as an > interrupt? You know already that this particular board with the matching > DMI identifier always uses the this number anyway. But GPIO (relative!) number is not enough. We need to understand more, i.e.: 1) from which GPIO controller it comes from (okay, for this particular platform I know it); 2) which expander does have this resource (they all have same ACPI HID). So, second one means to count GpioInt() resources (I don't remember if we have helper for that, probably we can add one). For the first we need to get a GPIO controller something (gpiochip?) And this first one I have no idea how we can perform without talking to the core. Basically, it may be done by reimplementing acpi_dev_gpio_irq_get(), followed by acpi_get_gpiod_by_index(), followed by acpi_gpio_resource_lookup(), followed by acpi_populate_gpio_lookup(), where at last this quirk should be applied. It seems for me like an over duplicated solution. I really don't understand how we can shortcut all these. What am I missing?
On Mon, May 25, 2020 at 04:01:08PM +0300, Andy Shevchenko wrote: > On Mon, May 25, 2020 at 03:21:36PM +0300, Mika Westerberg wrote: > > On Mon, May 25, 2020 at 02:35:51PM +0300, Andy Shevchenko wrote: > > > On Mon, May 25, 2020 at 02:05:56PM +0300, Mika Westerberg wrote: > > > > On Mon, May 25, 2020 at 01:13:35PM +0300, Andy Shevchenko wrote: > > > > > Due to parsing of ACPI tables. I don't want to copy'n'paste 25% of > > > > > gpiolib-acpi.c in here. I think provided solution is cleaner and (more) > > > > > flexible in terms of maintenance. > > > > > > > > Hmm, you seem to pass a hard-coded pin number (1) to the core that then > > > > passes it back to the driver. Why you can't simple use that number here > > > > directly? You don't need to parse anything. What I'm missing? :-) > > > > > > Okay, so, AFAIU you are proposing something like this: > > > > > > 1) find a GPIO controller by the ACPI path (somehow, I guess by finding a > > > handle followed by physical device behind it); 2) somehow to request a > > > pin from that device by number; > > > 3) convert to IRQ and use. > > > > > > Is it correct? > > > > Well, no. In the first patch you do this: > > > > pin = lookup->info.quirks_data; > > > > and this essentially becomes 1 so you know the pin number upfront in the > > driver. Why not simply get GPIO number 1 in the driver and use it as an > > interrupt? You know already that this particular board with the matching > > DMI identifier always uses the this number anyway. > > But GPIO (relative!) number is not enough. We need to understand more, i.e.: > 1) from which GPIO controller it comes from (okay, for this particular platform I know it); > 2) which expander does have this resource (they all have same ACPI HID). > > So, second one means to count GpioInt() resources (I don't remember if we have > helper for that, probably we can add one). For the first we need to get a GPIO > controller something (gpiochip?) And this first one I have no idea how we can > perform without talking to the core. > > Basically, it may be done by reimplementing acpi_dev_gpio_irq_get(), followed > by acpi_get_gpiod_by_index(), followed by acpi_gpio_resource_lookup(), followed > by acpi_populate_gpio_lookup(), where at last this quirk should be applied. > > It seems for me like an over duplicated solution. > > I really don't understand how we can shortcut all these. What am I missing? Why for example following would not work? If it is using global GPIO numbers anyway. static int pca953x_acpi_interrupt_get_irq(struct device *dev) { struct gpio_desc *desc; desc = gpio_to_desc(1); if (!desc) return -ENODEV; return gpiod_to_irq(desc); }
On Mon, May 25, 2020 at 04:47:48PM +0300, Mika Westerberg wrote: > On Mon, May 25, 2020 at 04:01:08PM +0300, Andy Shevchenko wrote: > > On Mon, May 25, 2020 at 03:21:36PM +0300, Mika Westerberg wrote: > > > On Mon, May 25, 2020 at 02:35:51PM +0300, Andy Shevchenko wrote: > > > > On Mon, May 25, 2020 at 02:05:56PM +0300, Mika Westerberg wrote: > > > > > On Mon, May 25, 2020 at 01:13:35PM +0300, Andy Shevchenko wrote: > > > > > > Due to parsing of ACPI tables. I don't want to copy'n'paste 25% of > > > > > > gpiolib-acpi.c in here. I think provided solution is cleaner and (more) > > > > > > flexible in terms of maintenance. > > > > > > > > > > Hmm, you seem to pass a hard-coded pin number (1) to the core that then > > > > > passes it back to the driver. Why you can't simple use that number here > > > > > directly? You don't need to parse anything. What I'm missing? :-) > > > > > > > > Okay, so, AFAIU you are proposing something like this: > > > > > > > > 1) find a GPIO controller by the ACPI path (somehow, I guess by finding a > > > > handle followed by physical device behind it); 2) somehow to request a > > > > pin from that device by number; > > > > 3) convert to IRQ and use. > > > > > > > > Is it correct? > > > > > > Well, no. In the first patch you do this: > > > > > > pin = lookup->info.quirks_data; > > > > > > and this essentially becomes 1 so you know the pin number upfront in the > > > driver. Why not simply get GPIO number 1 in the driver and use it as an > > > interrupt? You know already that this particular board with the matching > > > DMI identifier always uses the this number anyway. > > > > But GPIO (relative!) number is not enough. We need to understand more, i.e.: > > 1) from which GPIO controller it comes from (okay, for this particular platform I know it); > > 2) which expander does have this resource (they all have same ACPI HID). > > > > So, second one means to count GpioInt() resources (I don't remember if we have > > helper for that, probably we can add one). For the first we need to get a GPIO > > controller something (gpiochip?) And this first one I have no idea how we can > > perform without talking to the core. > > > > Basically, it may be done by reimplementing acpi_dev_gpio_irq_get(), followed > > by acpi_get_gpiod_by_index(), followed by acpi_gpio_resource_lookup(), followed > > by acpi_populate_gpio_lookup(), where at last this quirk should be applied. > > > > It seems for me like an over duplicated solution. > > > > I really don't understand how we can shortcut all these. What am I missing? > > Why for example following would not work? If it is using global GPIO numbers > anyway. Because GPIO 1 above and below is not global? Okay, what we have in the table seems global. Let me see if this will fly. > static int pca953x_acpi_interrupt_get_irq(struct device *dev) > { > struct gpio_desc *desc; > > desc = gpio_to_desc(1); > if (!desc) Okay, this seems have no deferred probe support, but I think it's fine, it will be unlikely we will have for the certain platform the SoC's GPIO controller enumerated after the IO expander one. > return -ENODEV; > > return gpiod_to_irq(desc); > }
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index 1fca8dd7824f..2014563309be 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -10,6 +10,7 @@ #include <linux/acpi.h> #include <linux/bitmap.h> +#include <linux/dmi.h> #include <linux/gpio/driver.h> #include <linux/gpio/consumer.h> #include <linux/i2c.h> @@ -113,6 +114,39 @@ static const struct acpi_device_id pca953x_acpi_ids[] = { }; MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids); +#ifdef CONFIG_GPIO_PCA953X_IRQ +static const struct acpi_gpio_params pca953x_interrupt_gpios = { 0, 0, true }; + +static const struct acpi_gpio_mapping pca953x_acpi_interrupt_gpios[] = { + { "interrupt-gpios", &pca953x_interrupt_gpios, 1, ACPI_GPIO_QUIRK_FORCE_PIN, 1 }, + { } +}; + +static int pca953x_acpi_interrupt_get_irq(struct device *dev) +{ + struct gpio_desc *desc; + + if (devm_acpi_dev_add_driver_gpios(dev, pca953x_acpi_interrupt_gpios)) + dev_warn(dev, "can't add GPIO ACPI mapping\n"); + + desc = devm_gpiod_get(dev, "interrupt", GPIOD_IN); + if (IS_ERR(desc)) + return PTR_ERR(desc); + + return gpiod_to_irq(desc); +} + +static const struct dmi_system_id pca953x_dmi_acpi_interrupt_info[] = { + { + .ident = "Intel Galileo Gen 2", + .matches = { + DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"), + }, + }, + {} +}; +#endif + #define MAX_BANK 5 #define BANK_SZ 8 #define MAX_LINE (MAX_BANK * BANK_SZ) @@ -750,8 +784,18 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base) struct irq_chip *irq_chip = &chip->irq_chip; DECLARE_BITMAP(reg_direction, MAX_LINE); DECLARE_BITMAP(irq_stat, MAX_LINE); + const struct dmi_system_id *id; int ret; + id = dmi_first_match(pca953x_dmi_acpi_interrupt_info); + if (id) { + dev_info(&client->dev, "Applying ACPI interrupt quirk\n"); + + ret = pca953x_acpi_interrupt_get_irq(&client->dev); + if (ret > 0) + client->irq = ret; + } + if (!client->irq) return 0;
ACPI table on Intel Galileo Gen 2 has wrong pin number for IRQ resource of one of the I²C GPIO expanders. ACPI GPIO library provides a special quirk which we may use in this case. With help of it, override GpioInt() pin for the affected platform. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpio-pca953x.c | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)