gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2
PATCH v3 1/1 gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2
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.
---
v3: used legacy API (Mika)
drivers/gpio/gpio-pca953x.c | 83 +++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)
drivers/gpio/gpio-pca953x.c +83 -0
@@ -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 ;
Signature · 1 line
2.26.2
Quoted 6 lines · Andy Shevchenko
drivers/gpio/gpio-pca953x.c +6
-0+static int pca953x_acpi_get_irq (struct device *dev) +{ +int pin, ret; + + pin =pca953x_acpi_find_pin (dev); +if (pin <0 )
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 179
Quoted 10 lines
drivers/gpio/gpio-pca953x.c +10
-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
Quoted 14 lines · Linus Walleij
Quoted 11 lines · Andy Shevchenko
drivers/gpio/gpio-pca953x.c +11
-0+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))
Quoted 8 lines
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 179
Quoted 5 lines
drivers/gpio/gpio-pca953x.c +5
-0+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.
Quoted 1 line
I.e. can you figure out the hardware offset number?
That's what this quirk basically does, it takes it from ACPI.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 24 lines · Andy Shevchenko
Quoted 14 lines · Linus Walleij
Quoted 11 lines · Andy Shevchenko
drivers/gpio/gpio-pca953x.c +11
-0+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))Quoted 8 lines
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 179
Quoted 5 lines
drivers/gpio/gpio-pca953x.c +5
-0+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?
Quoted 6 lines
It would but it will be wrong. We don't request own pin, we request pin from upper GPIO (IRQ) chip.
Quoted 1 line
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 57 lines · Andy Shevchenko
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 <[email protected]>Signed-off-by Andy Shevchenko <[email protected]>---
v3: used legacy API (Mika)drivers/gpio/gpio-pca953x.c | 83 +++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)drivers/gpio/gpio-pca953x.c +24
-0@@ -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" ), + }, + }, + {} +}; +
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 143
Quoted 10 lines
drivers/gpio/gpio-pca953x.c +10
-0+#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;
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 151
Quoted 7 lines
drivers/gpio/gpio-pca953x.c +7
-0+} + +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);
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 158
Quoted 7 lines
drivers/gpio/gpio-pca953x.c +7
-0+ + 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;
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 170
Quoted 12 lines
drivers/gpio/gpio-pca953x.c +12
-0+} +#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.
Quoted 18 lines
drivers/gpio/gpio-pca953x.c +14
-0+ +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 , }, { }
drivers/gpio/gpio-pca953x.c static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base) · line 832
Quoted 8 lines
drivers/gpio/gpio-pca953x.c +3
-0@@ -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;
}Quoted 10 lines
drivers/gpio/gpio-pca953x.c +5
-0+ ret =pca953x_acpi_get_irq (&client->dev); +if (ret >0 ) + client->irq = ret; + } +if (!client->irq)return 0 ;Signature · 1 line
2.26.2
Quoted 6 lines · Mika Westerberg
Quoted 5 lines · Andy Shevchenko
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.
...
Quoted 12 lines
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 143
Quoted 9 lines
drivers/gpio/gpio-pca953x.c +9
-0+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:
Quoted 3 lines
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.
Quoted 1 line
Quoted 1 line
+}
...
Quoted 9 lines
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 158
Quoted 6 lines
drivers/gpio/gpio-pca953x.c +6
-0+ 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.
...
Quoted 10 lines
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 170
Quoted 7 lines
drivers/gpio/gpio-pca953x.c +7
-0+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...
Quoted 10 lines
drivers/gpio/gpio-pca953x.c static const struct i2c_device_id pca953x_id[] = { · line 181
Quoted 10 lines
drivers/gpio/gpio-pca953x.c +10
-0+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;
Quoted 1 line
Quoted 1 line
+}
...
I agree on the rest.
Signature · 2 lines
With Best Regards, Andy Shevchenko