All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2
@ 2020-05-26 17:12 Andy Shevchenko
  2020-05-27  5:46 ` Linus Walleij
  2020-05-27 11:27 ` Mika Westerberg
  0 siblings, 2 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-05-26 17:12 UTC (permalink / raw)
  To: linux-gpio, Linus Walleij, Bartosz Golaszewski
  Cc: Andy Shevchenko, Mika Westerberg

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];
+	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;
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2
  2020-05-26 17:12 [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2 Andy Shevchenko
@ 2020-05-27  5:46 ` Linus Walleij
  2020-05-27  9:08   ` Andy Shevchenko
  2020-05-27 11:27 ` Mika Westerberg
  1 sibling, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2020-05-27  5:46 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Mika Westerberg

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2
  2020-05-27  5:46 ` Linus Walleij
@ 2020-05-27  9:08   ` Andy Shevchenko
  2020-05-27  9:13     ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-05-27  9:08 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Mika Westerberg

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.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2
  2020-05-27  9:08   ` Andy Shevchenko
@ 2020-05-27  9:13     ` Andy Shevchenko
  0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-05-27  9:13 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Mika Westerberg

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.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2
  2020-05-26 17:12 [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2 Andy Shevchenko
  2020-05-27  5:46 ` Linus Walleij
@ 2020-05-27 11:27 ` Mika Westerberg
  2020-05-27 13:28   ` Andy Shevchenko
  1 sibling, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2020-05-27 11:27 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-gpio, Linus Walleij, Bartosz Golaszewski

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2
  2020-05-27 11:27 ` Mika Westerberg
@ 2020-05-27 13:28   ` Andy Shevchenko
  0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-05-27 13:28 UTC (permalink / raw)
  To: Mika Westerberg; +Cc: linux-gpio, Linus Walleij, Bartosz Golaszewski

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.


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-05-27 13:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-26 17:12 [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2 Andy Shevchenko
2020-05-27  5:46 ` Linus Walleij
2020-05-27  9:08   ` Andy Shevchenko
2020-05-27  9:13     ` Andy Shevchenko
2020-05-27 11:27 ` Mika Westerberg
2020-05-27 13:28   ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.