[PATCH v1 1/5] gpiolib: acpi: Introduce opaque data field for quirks

[PATCH v1 1/5] gpiolib: acpi: Introduce opaque data field for quirks

[Andy Shevchenko]

Some quirks may need an additional data to be provided.
Introduce special quirks data field.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c   | 13 +++++++------
 drivers/gpio/gpiolib-acpi.h   |  2 ++
 include/linux/gpio/consumer.h |  1 +
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 9276051663da..3aa976f9ad1a 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -502,7 +502,7 @@ EXPORT_SYMBOL_GPL(devm_acpi_dev_remove_driver_gpios);
 static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
 				      const char *name, int index,
 				      struct fwnode_reference_args *args,
-				      unsigned int *quirks)
+				      struct acpi_gpio_info *info)
 {
 	const struct acpi_gpio_mapping *gm;
 
@@ -519,7 +519,8 @@ static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
 			args->args[2] = par->active_low;
 			args->nargs = 3;
 
-			*quirks = gm->quirks;
+			info->quirks = gm->quirks;
+			info->quirks_data = gm->quirks_data;
 			return true;
 		}
 
@@ -716,7 +717,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 				     struct acpi_gpio_lookup *lookup)
 {
 	struct fwnode_reference_args args;
-	unsigned int quirks = 0;
+	struct acpi_gpio_info info = {};
 	int ret;
 
 	memset(&args, 0, sizeof(args));
@@ -728,8 +729,7 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 		if (!adev)
 			return ret;
 
-		if (!acpi_get_driver_gpio_data(adev, propname, index, &args,
-					       &quirks))
+		if (!acpi_get_driver_gpio_data(adev, propname, index, &args, &info))
 			return ret;
 	}
 	/*
@@ -746,7 +746,8 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 	lookup->active_low = !!args.args[2];
 
 	lookup->info.adev = to_acpi_device_node(args.fwnode);
-	lookup->info.quirks = quirks;
+	lookup->info.quirks = info.quirks;
+	lookup->info.quirks_data = info.quirks_data;
 
 	return 0;
 }
diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
index 1c6d65cf0629..44cf55c9d35d 100644
--- a/drivers/gpio/gpiolib-acpi.h
+++ b/drivers/gpio/gpiolib-acpi.h
@@ -19,6 +19,7 @@ struct acpi_device;
  * @polarity: interrupt polarity as provided by ACPI
  * @triggering: triggering type as provided by ACPI
  * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
+ * @quirks_data: optional data for the specific @quirks
  */
 struct acpi_gpio_info {
 	struct acpi_device *adev;
@@ -28,6 +29,7 @@ struct acpi_gpio_info {
 	int polarity;
 	int triggering;
 	unsigned int quirks;
+	unsigned long quirks_data;
 };
 
 #ifdef CONFIG_ACPI
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 901aab89d025..49743a499fda 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -676,6 +676,7 @@ struct acpi_gpio_mapping {
 #define ACPI_GPIO_QUIRK_ONLY_GPIOIO		BIT(1)
 
 	unsigned int quirks;
+	unsigned long quirks_data;
 };
 
 #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_ACPI)
-- 
2.26.2

[PATCH v1 2/5] gpiolib: acpi: Introduce a quirk to force GpioInt pin

[Andy Shevchenko]

Some ACPI tables have broken GpioInt() pin number, i.e.
Intel Galileo gen 2 board, where it by some reason refers to
the absolute one instead of being relative to the controller.

In order to work around, introduce a new quirk to force this number.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c   | 9 +++++++--
 include/linux/gpio/consumer.h | 6 ++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 3aa976f9ad1a..93f3c833f3c7 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -651,6 +651,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		const struct acpi_resource_gpio *agpio = &ares->data.gpio;
 		bool gpioint = agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
 		int pin_index;
+		u16 pin;
 
 		if (lookup->info.quirks & ACPI_GPIO_QUIRK_ONLY_GPIOIO && gpioint)
 			lookup->index++;
@@ -662,8 +663,12 @@ 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,
-					      agpio->pin_table[pin_index]);
+		if (lookup->info.quirks & ACPI_GPIO_QUIRK_FORCE_PIN)
+			pin = lookup->info.quirks_data;
+		else
+			pin = agpio->pin_table[pin_index];
+
+		lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr, pin);
 		lookup->info.pin_config = agpio->pin_config;
 		lookup->info.gpioint = gpioint;
 
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 49743a499fda..e6bacebcecb7 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -674,6 +674,12 @@ struct acpi_gpio_mapping {
  * get GpioIo type explicitly, this quirk may be used.
  */
 #define ACPI_GPIO_QUIRK_ONLY_GPIOIO		BIT(1)
+/*
+ * Some ACPI tables may have wrong pin defined. Allow to force the pin
+ * number if quirk is provided. New pin number should be provided via
+ * @quirks_data field.
+ */
+#define ACPI_GPIO_QUIRK_FORCE_PIN		BIT(2)
 
 	unsigned int quirks;
 	unsigned long quirks_data;
-- 
2.26.2

[PATCH v1 3/5] gpio: pca953x: Drop unneeded ACPI_PTR()

[Andy Shevchenko]

ACPI_PTR() becomes a no-op when !CONFIG_ACPI. This is not needed since
we always have ID table enabled. Moreover, in the mentioned case compiler
will complain about defined but not used variable.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 4bb3d3524bc7..1fca8dd7824f 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1176,7 +1176,7 @@ static struct i2c_driver pca953x_driver = {
 		.name	= "pca953x",
 		.pm	= &pca953x_pm_ops,
 		.of_match_table = pca953x_dt_ids,
-		.acpi_match_table = ACPI_PTR(pca953x_acpi_ids),
+		.acpi_match_table = pca953x_acpi_ids,
 	},
 	.probe		= pca953x_probe,
 	.remove		= pca953x_remove,
-- 
2.26.2

Re: [PATCH v1 3/5] gpio: pca953x: Drop unneeded ACPI_PTR()

[Andy Shevchenko]

On Thu, May 21, 2020 at 12:19:14AM +0300, Andy Shevchenko wrote:
> ACPI_PTR() becomes a no-op when !CONFIG_ACPI. This is not needed since
> we always have ID table enabled. Moreover, in the mentioned case compiler
> will complain about defined but not used variable.

Bart, are you going to apply this one, or should I resend it as a part of v2?

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-pca953x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 4bb3d3524bc7..1fca8dd7824f 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -1176,7 +1176,7 @@ static struct i2c_driver pca953x_driver = {
>  		.name	= "pca953x",
>  		.pm	= &pca953x_pm_ops,
>  		.of_match_table = pca953x_dt_ids,
> -		.acpi_match_table = ACPI_PTR(pca953x_acpi_ids),
> +		.acpi_match_table = pca953x_acpi_ids,
>  	},
>  	.probe		= pca953x_probe,
>  	.remove		= pca953x_remove,
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/5] gpio: pca953x: Drop unneeded ACPI_PTR()

[Bartosz Golaszewski]

pon., 25 maj 2020 o 19:58 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Thu, May 21, 2020 at 12:19:14AM +0300, Andy Shevchenko wrote:
> > ACPI_PTR() becomes a no-op when !CONFIG_ACPI. This is not needed since
> > we always have ID table enabled. Moreover, in the mentioned case compiler
> > will complain about defined but not used variable.
>
> Bart, are you going to apply this one, or should I resend it as a part of v2?
>

Ugh, I already sent my last PRs to Linus both for v5.7 fixes & v5.8
updates. I'll let Linus pick it up once he pulls from my tree.

Bart

Re: [PATCH v1 3/5] gpio: pca953x: Drop unneeded ACPI_PTR()

[Andy Shevchenko]

On Wed, May 27, 2020 at 03:13:11PM +0200, Bartosz Golaszewski wrote:
> pon., 25 maj 2020 o 19:58 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > On Thu, May 21, 2020 at 12:19:14AM +0300, Andy Shevchenko wrote:
> > > ACPI_PTR() becomes a no-op when !CONFIG_ACPI. This is not needed since
> > > we always have ID table enabled. Moreover, in the mentioned case compiler
> > > will complain about defined but not used variable.
> >
> > Bart, are you going to apply this one, or should I resend it as a part of v2?
> >
> 
> Ugh, I already sent my last PRs to Linus both for v5.7 fixes & v5.8
> updates. I'll let Linus pick it up once he pulls from my tree.

No problem, thanks!

P.S. The patch is pretty much independent and can be applied even before that
(I suppose), but let Linus decide what is better for him.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/5] gpio: pca953x: Drop unneeded ACPI_PTR()

[Linus Walleij]

On Wed, May 27, 2020 at 3:13 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> pon., 25 maj 2020 o 19:58 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > On Thu, May 21, 2020 at 12:19:14AM +0300, Andy Shevchenko wrote:
> > > ACPI_PTR() becomes a no-op when !CONFIG_ACPI. This is not needed since
> > > we always have ID table enabled. Moreover, in the mentioned case compiler
> > > will complain about defined but not used variable.
> >
> > Bart, are you going to apply this one, or should I resend it as a part of v2?
> >
>
> Ugh, I already sent my last PRs to Linus both for v5.7 fixes & v5.8
> updates. I'll let Linus pick it up once he pulls from my tree.

I cherry-picked this one (patch 3) on top of my devel branch.

Yours,
Linus Walleij

[PATCH v1 4/5] gpio: pca935x: Allow IRQ support for driver built as a module

[Andy Shevchenko]

Perhaps by some historical reasons the IRQ support has been allowed
only for built-in driver. However, there is nothing prevents us
to build it as module an use as IRQ chip.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 03c01f4aa316..eff454eed9a7 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -952,7 +952,7 @@ config GPIO_PCA953X
 
 config GPIO_PCA953X_IRQ
 	bool "Interrupt controller support for PCA953x"
-	depends on GPIO_PCA953X=y
+	depends on GPIO_PCA953X
 	select GPIOLIB_IRQCHIP
 	help
 	  Say yes here to enable the pca953x to be used as an interrupt
-- 
2.26.2

Re: [PATCH v1 4/5] gpio: pca935x: Allow IRQ support for driver built as a module

[Bartosz Golaszewski]

śr., 20 maj 2020 o 23:19 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> Perhaps by some historical reasons the IRQ support has been allowed
> only for built-in driver. However, there is nothing prevents us
> to build it as module an use as IRQ chip.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This may have been a stand-alone patch as well, so I went ahead and
applied it for next.

Bart

Re: [PATCH v1 4/5] gpio: pca935x: Allow IRQ support for driver built as a module

[Andy Shevchenko]

On Mon, May 25, 2020 at 11:38:32AM +0200, Bartosz Golaszewski wrote:
> śr., 20 maj 2020 o 23:19 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > Perhaps by some historical reasons the IRQ support has been allowed
> > only for built-in driver. However, there is nothing prevents us
> > to build it as module an use as IRQ chip.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> This may have been a stand-alone patch as well,

Yes.

>	so I went ahead and applied it for next.

Thanks!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Andy Shevchenko]

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"),
+		},
+	},
+	{}
+};
+#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;
 
-- 
2.26.2

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Mika Westerberg]

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.

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Mika Westerberg]

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?

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Mika Westerberg]

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? :-)

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Andy Shevchenko]

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?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Andy Shevchenko]

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?").

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Mika Westerberg]

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.

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Mika Westerberg]

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);
}

Re: [PATCH v1 5/5] gpio: pca953x: Override GpioInt() pin for Intel Galileo Gen 2

[Andy Shevchenko]

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);
> }

-- 
With Best Regards,
Andy Shevchenko