| Message ID | 20200526171222.14835-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2 | expand |
On Tue, May 26, 2020 at 7:12 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > +static int pca953x_acpi_get_irq(struct device *dev) > +{ > + int pin, ret; > + > + pin = pca953x_acpi_find_pin(dev); > + if (pin < 0) > + return pin; > + > + dev_info(dev, "Applying ACPI interrupt quirk (GPIO %d)\n", pin); > + > + if (!gpio_is_valid(pin)) > + return -EINVAL; > + > + ret = gpio_request(pin, "pca953x interrupt"); > + if (ret) > + return ret; So would it work to do gpiochip_request_own_desc() here in some form? I.e. can you figure out the hardware offset number? Yours, Linus Walleij
On Wed, May 27, 2020 at 07:46:55AM +0200, Linus Walleij wrote: > On Tue, May 26, 2020 at 7:12 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > +static int pca953x_acpi_get_irq(struct device *dev) > > +{ > > + int pin, ret; > > + > > + pin = pca953x_acpi_find_pin(dev); > > + if (pin < 0) > > + return pin; > > + > > + dev_info(dev, "Applying ACPI interrupt quirk (GPIO %d)\n", pin); > > + > > + if (!gpio_is_valid(pin)) > > + return -EINVAL; > > + > > + ret = gpio_request(pin, "pca953x interrupt"); > > + if (ret) > > + return ret; > > So would it work to do > gpiochip_request_own_desc() here in some form? It would but it will be wrong. We don't request own pin, we request pin from upper GPIO (IRQ) chip. > I.e. can you figure out the hardware offset number? That's what this quirk basically does, it takes it from ACPI.
On Wed, May 27, 2020 at 12:08:46PM +0300, Andy Shevchenko wrote: > On Wed, May 27, 2020 at 07:46:55AM +0200, Linus Walleij wrote: > > On Tue, May 26, 2020 at 7:12 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > +static int pca953x_acpi_get_irq(struct device *dev) > > > +{ > > > + int pin, ret; > > > + > > > + pin = pca953x_acpi_find_pin(dev); > > > + if (pin < 0) > > > + return pin; > > > + > > > + dev_info(dev, "Applying ACPI interrupt quirk (GPIO %d)\n", pin); > > > + > > > + if (!gpio_is_valid(pin)) > > > + return -EINVAL; > > > + > > > + ret = gpio_request(pin, "pca953x interrupt"); > > > + if (ret) > > > + return ret; > > > > So would it work to do > > gpiochip_request_own_desc() here in some form? > > It would but it will be wrong. We don't request own pin, we request pin from > upper GPIO (IRQ) chip. > > > I.e. can you figure out the hardware offset number? > > That's what this quirk basically does, it takes it from ACPI. Ah, sorry, I meant absolute number. We can, of course, calculate an offset, but then it will have hard coded number no better than hard coded from ACPI table.
On Tue, May 26, 2020 at 08:12:22PM +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. Since we know what that number is and > luckily have GPIO bases fixed for SoC's controllers, we may use a simple > DMI quirk to match the platform and retrieve GpioInt() pin on it for > the expander in question. > > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > v3: used legacy API (Mika) > drivers/gpio/gpio-pca953x.c | 83 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 83 insertions(+) > > diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c > index 1fca8dd7824f..eeb91b27a52f 100644 > --- a/drivers/gpio/gpio-pca953x.c > +++ b/drivers/gpio/gpio-pca953x.c > @@ -10,11 +10,13 @@ > > #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> > #include <linux/init.h> > #include <linux/interrupt.h> > +#include <linux/list.h> > #include <linux/module.h> > #include <linux/of_platform.h> > #include <linux/platform_data/pca953x.h> > @@ -107,6 +109,79 @@ static const struct i2c_device_id pca953x_id[] = { > }; > MODULE_DEVICE_TABLE(i2c, pca953x_id); > > +#ifdef CONFIG_GPIO_PCA953X_IRQ > + > +#include <linux/gpio.h> > + > +static const struct dmi_system_id pca953x_dmi_acpi_irq_info[] = { > + { > + /* > + * On Intel Galileo Gen 2 board the IRQ pin of one of > + * the I²C GPIO expanders, which has GpioInt() resource, > + * is provided as an absolute number instead of being > + * relative. Since first controller (gpio-sch.c) and > + * second (gpio-dwapb.c) are at the fixed bases, we may > + * safely refer to the number in the global space to get > + * an IRQ out of it. > + */ > + .matches = { > + DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"), > + }, > + }, > + {} > +}; > + > +#ifdef CONFIG_ACPI > +static int pca953x_acpi_get_pin(struct acpi_resource *ares, void *data) > +{ > + struct acpi_resource_gpio *agpio; > + int *pin = data; > + > + if (!acpi_gpio_get_irq_resource(ares, &agpio)) > + return 1; > + > + *pin = agpio->pin_table[0]; Writing it like below looks better IMHO: if (acpi_gpio_get_irq_resource(ares, &agpio)) *pin = agpio->pin_table[0]; return 1; > +} > + > +static int pca953x_acpi_find_pin(struct device *dev) > +{ > + struct acpi_device *adev = ACPI_COMPANION(dev); > + LIST_HEAD(r); > + int pin = -ENOENT, ret; Hmm, reverse christmas tree? struct acpi_device *adev = ACPI_COMPANION(dev); int pin = -ENOENT, ret; LIST_HEAD(r); > + > + ret = acpi_dev_get_resources(adev, &r, pca953x_acpi_get_pin, &pin); > + acpi_dev_free_resource_list(&r); > + if (ret < 0) > + return ret; > + > + return pin; Or return ret < 0 ? ret : pin; > +} > +#else > +static inline int pca953x_acpi_find_pin(struct device *dev) { return -ENXIO; } > +#endif > + > +static int pca953x_acpi_get_irq(struct device *dev) > +{ > + int pin, ret; > + > + pin = pca953x_acpi_find_pin(dev); > + if (pin < 0) > + return pin; Since you don't actually check the error value you may also return simply 0 here (invalid IRQ) and other places. > + > + dev_info(dev, "Applying ACPI interrupt quirk (GPIO %d)\n", pin); > + > + if (!gpio_is_valid(pin)) > + return -EINVAL; > + > + ret = gpio_request(pin, "pca953x interrupt"); > + if (ret) > + return ret; > + > + return gpio_to_irq(pin); > +} > +#endif > + > static const struct acpi_device_id pca953x_acpi_ids[] = { > { "INT3491", 16 | PCA953X_TYPE | PCA_LATCH_INT, }, > { } > @@ -750,8 +825,16 @@ 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_irq_info); > + if (id) { Also here since you don't actually use id for anything you can simplify this like: if (dmi_check_system(pca953x_dmi_acpi_irq_info)) { ret = pca953x_acpi_get_irq(&client->dev); if (ret > 0) client->irq = ret; } > + ret = pca953x_acpi_get_irq(&client->dev); > + if (ret > 0) > + client->irq = ret; > + } > + > if (!client->irq) > return 0; > > -- > 2.26.2
On Wed, May 27, 2020 at 02:27:49PM +0300, Mika Westerberg wrote: > On Tue, May 26, 2020 at 08:12:22PM +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. Since we know what that number is and > > luckily have GPIO bases fixed for SoC's controllers, we may use a simple > > DMI quirk to match the platform and retrieve GpioInt() pin on it for > > the expander in question. ... > > +static int pca953x_acpi_get_pin(struct acpi_resource *ares, void *data) > > +{ > > + struct acpi_resource_gpio *agpio; > > + int *pin = data; > > + > > + if (!acpi_gpio_get_irq_resource(ares, &agpio)) > > + return 1; > > + > > + *pin = agpio->pin_table[0]; > > Writing it like below looks better IMHO: > > if (acpi_gpio_get_irq_resource(ares, &agpio)) > *pin = agpio->pin_table[0]; > return 1; Actually this reveals a suboptimal behaviour in comparison to acpi_walk_resources(), i.e. there is no way to stop traversing when (1st) resource is found. But okay, I will rewrite. > > +} ... > > + ret = acpi_dev_get_resources(adev, &r, pca953x_acpi_get_pin, &pin); > > + acpi_dev_free_resource_list(&r); > > + if (ret < 0) > > + return ret; > > + > > + return pin; > > Or > return ret < 0 ? ret : pin; I think former is better to read since it has been grouped appropriately. ... > > +static int pca953x_acpi_get_irq(struct device *dev) > > +{ > > + int pin, ret; > > + > > + pin = pca953x_acpi_find_pin(dev); > > + if (pin < 0) > > + return pin; > > Since you don't actually check the error value you may also return > simply 0 here (invalid IRQ) and other places. I don't think it is a good idea, because... > > + dev_info(dev, "Applying ACPI interrupt quirk (GPIO %d)\n", pin); > > + > > + if (!gpio_is_valid(pin)) > > + return -EINVAL; > > + > > + ret = gpio_request(pin, "pca953x interrupt"); > > + if (ret) > > + return ret; > > + > > + return gpio_to_irq(pin); ... this will become to something like ret = gpio_to_irq(); if (ret < 0) return 0; return ret; > > +} ... I agree on the rest.
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index 1fca8dd7824f..eeb91b27a52f 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -10,11 +10,13 @@ #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> #include <linux/init.h> #include <linux/interrupt.h> +#include <linux/list.h> #include <linux/module.h> #include <linux/of_platform.h> #include <linux/platform_data/pca953x.h> @@ -107,6 +109,79 @@ static const struct i2c_device_id pca953x_id[] = { }; MODULE_DEVICE_TABLE(i2c, pca953x_id); +#ifdef CONFIG_GPIO_PCA953X_IRQ + +#include <linux/gpio.h> + +static const struct dmi_system_id pca953x_dmi_acpi_irq_info[] = { + { + /* + * On Intel Galileo Gen 2 board the IRQ pin of one of + * the I²C GPIO expanders, which has GpioInt() resource, + * is provided as an absolute number instead of being + * relative. Since first controller (gpio-sch.c) and + * second (gpio-dwapb.c) are at the fixed bases, we may + * safely refer to the number in the global space to get + * an IRQ out of it. + */ + .matches = { + DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"), + }, + }, + {} +}; + +#ifdef CONFIG_ACPI +static int pca953x_acpi_get_pin(struct acpi_resource *ares, void *data) +{ + struct acpi_resource_gpio *agpio; + int *pin = data; + + if (!acpi_gpio_get_irq_resource(ares, &agpio)) + return 1; + + *pin = agpio->pin_table[0]; + return 1; +} + +static int pca953x_acpi_find_pin(struct device *dev) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + LIST_HEAD(r); + int pin = -ENOENT, ret; + + ret = acpi_dev_get_resources(adev, &r, pca953x_acpi_get_pin, &pin); + acpi_dev_free_resource_list(&r); + if (ret < 0) + return ret; + + return pin; +} +#else +static inline int pca953x_acpi_find_pin(struct device *dev) { return -ENXIO; } +#endif + +static int pca953x_acpi_get_irq(struct device *dev) +{ + int pin, ret; + + pin = pca953x_acpi_find_pin(dev); + if (pin < 0) + return pin; + + dev_info(dev, "Applying ACPI interrupt quirk (GPIO %d)\n", pin); + + if (!gpio_is_valid(pin)) + return -EINVAL; + + ret = gpio_request(pin, "pca953x interrupt"); + if (ret) + return ret; + + return gpio_to_irq(pin); +} +#endif + static const struct acpi_device_id pca953x_acpi_ids[] = { { "INT3491", 16 | PCA953X_TYPE | PCA_LATCH_INT, }, { } @@ -750,8 +825,16 @@ 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_irq_info); + if (id) { + ret = pca953x_acpi_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. Since we know what that number is and luckily have GPIO bases fixed for SoC's controllers, we may use a simple DMI quirk to match the platform and retrieve GpioInt() pin on it for the expander in question. Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v3: used legacy API (Mika) drivers/gpio/gpio-pca953x.c | 83 +++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)