[PATCH v3 1/5] iio: proximity: sx9500: Assign interrupt from GpioIo()

[PATCH v3 1/5] iio: proximity: sx9500: Assign interrupt from GpioIo()

[Andy Shevchenko]

The commit 0f0796509c07

("iio: remove gpio interrupt probing from drivers that use a single interrupt")

removed custom IRQ assignment for the drivers which are enumerated via
ACPI or OF. Unfortunately, some ACPI tables have IRQ line defined as
GpioIo() resource and thus automatic IRQ allocation will fail.

Partially revert the commit 0f0796509c07 to restore original behaviour.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/proximity/sx9500.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index 53c5d653e780..df23dbcc030a 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -869,6 +869,7 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
 static void sx9500_gpio_probe(struct i2c_client *client,
 			      struct sx9500_data *data)
 {
+	struct gpio_desc *gpiod_int;
 	struct device *dev;
 
 	if (!client)
@@ -876,6 +877,14 @@ static void sx9500_gpio_probe(struct i2c_client *client,
 
 	dev = &client->dev;
 
+	if (client->irq <= 0) {
+		gpiod_int = devm_gpiod_get(dev, SX9500_GPIO_INT, GPIOD_IN);
+		if (IS_ERR(gpiod_int))
+			dev_err(dev, "gpio get irq failed\n");
+		else
+			client->irq = gpiod_to_irq(gpiod_int);
+	}
+
 	data->gpiod_rst = devm_gpiod_get(dev, SX9500_GPIO_RESET, GPIOD_OUT_HIGH);
 	if (IS_ERR(data->gpiod_rst)) {
 		dev_warn(dev, "gpio get reset pin failed\n");
-- 
2.14.2

[PATCH v3 2/5] iio: proximity: sx9500: Add GPIO ACPI mapping table

[Andy Shevchenko]

In order to satisfy GPIO ACPI library requirements convert users of
gpiod_get_index() to correctly behave when there no mapping is provided
by firmware.

Here we add explicit mapping between _CRS GpioIo() resources and
their names used in the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/proximity/sx9500.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index df23dbcc030a..eb687b3dd442 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -32,9 +32,6 @@
 #define SX9500_DRIVER_NAME		"sx9500"
 #define SX9500_IRQ_NAME			"sx9500_event"
 
-#define SX9500_GPIO_INT			"interrupt"
-#define SX9500_GPIO_RESET		"reset"
-
 /* Register definitions. */
 #define SX9500_REG_IRQ_SRC		0x00
 #define SX9500_REG_STAT			0x01
@@ -866,26 +863,40 @@ static int sx9500_init_device(struct iio_dev *indio_dev)
 	return sx9500_init_compensation(indio_dev);
 }
 
+static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
+static const struct acpi_gpio_params interrupt_gpios = { 2, 0, false };
+
+static const struct acpi_gpio_mapping acpi_sx9500_gpios[] = {
+	{ "reset-gpios", &reset_gpios, 1 },
+	{ "interrupt-gpios", &interrupt_gpios, 1 },
+	{ },
+};
+
 static void sx9500_gpio_probe(struct i2c_client *client,
 			      struct sx9500_data *data)
 {
 	struct gpio_desc *gpiod_int;
 	struct device *dev;
+	int ret;
 
 	if (!client)
 		return;
 
 	dev = &client->dev;
 
+	ret = devm_acpi_dev_add_driver_gpios(dev, acpi_sx9500_gpios);
+	if (ret)
+		dev_dbg(dev, "Unable to add GPIO mapping table\n");
+
 	if (client->irq <= 0) {
-		gpiod_int = devm_gpiod_get(dev, SX9500_GPIO_INT, GPIOD_IN);
+		gpiod_int = devm_gpiod_get(dev, "interrupt", GPIOD_IN);
 		if (IS_ERR(gpiod_int))
 			dev_err(dev, "gpio get irq failed\n");
 		else
 			client->irq = gpiod_to_irq(gpiod_int);
 	}
 
-	data->gpiod_rst = devm_gpiod_get(dev, SX9500_GPIO_RESET, GPIOD_OUT_HIGH);
+	data->gpiod_rst = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(data->gpiod_rst)) {
 		dev_warn(dev, "gpio get reset pin failed\n");
 		data->gpiod_rst = NULL;
-- 
2.14.2

Re: [PATCH v3 2/5] iio: proximity: sx9500: Add GPIO ACPI mapping table

[Jonathan Cameron]

On Fri, 3 Nov 2017 15:03:37 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> In order to satisfy GPIO ACPI library requirements convert users of
> gpiod_get_index() to correctly behave when there no mapping is
> provided by firmware.
> 
> Here we add explicit mapping between _CRS GpioIo() resources and
> their names used in the driver.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Added cc's as for previous patch.  I guess this makes sense if we
accept that fixes like the previous one should be in drivers at all.

If not the reset part still makes sense I suppose.

> ---
>  drivers/iio/proximity/sx9500.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c
> b/drivers/iio/proximity/sx9500.c index df23dbcc030a..eb687b3dd442
> 100644 --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -32,9 +32,6 @@
>  #define SX9500_DRIVER_NAME		"sx9500"
>  #define SX9500_IRQ_NAME			"sx9500_event"
>  
> -#define SX9500_GPIO_INT			"interrupt"
> -#define SX9500_GPIO_RESET		"reset"
> -
>  /* Register definitions. */
>  #define SX9500_REG_IRQ_SRC		0x00
>  #define SX9500_REG_STAT			0x01
> @@ -866,26 +863,40 @@ static int sx9500_init_device(struct iio_dev
> *indio_dev) return sx9500_init_compensation(indio_dev);
>  }
>  
> +static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
> +static const struct acpi_gpio_params interrupt_gpios = { 2, 0,
> false }; +
> +static const struct acpi_gpio_mapping acpi_sx9500_gpios[] = {
> +	{ "reset-gpios", &reset_gpios, 1 },
> +	{ "interrupt-gpios", &interrupt_gpios, 1 },
> +	{ },
> +};
> +
>  static void sx9500_gpio_probe(struct i2c_client *client,
>  			      struct sx9500_data *data)
>  {
>  	struct gpio_desc *gpiod_int;
>  	struct device *dev;
> +	int ret;
>  
>  	if (!client)
>  		return;
>  
>  	dev = &client->dev;
>  
> +	ret = devm_acpi_dev_add_driver_gpios(dev, acpi_sx9500_gpios);
> +	if (ret)
> +		dev_dbg(dev, "Unable to add GPIO mapping table\n");
> +
>  	if (client->irq <= 0) {
> -		gpiod_int = devm_gpiod_get(dev, SX9500_GPIO_INT,
> GPIOD_IN);
> +		gpiod_int = devm_gpiod_get(dev, "interrupt",
> GPIOD_IN); if (IS_ERR(gpiod_int))
>  			dev_err(dev, "gpio get irq failed\n");
>  		else
>  			client->irq = gpiod_to_irq(gpiod_int);
>  	}
>  
> -	data->gpiod_rst = devm_gpiod_get(dev, SX9500_GPIO_RESET,
> GPIOD_OUT_HIGH);
> +	data->gpiod_rst = devm_gpiod_get(dev, "reset",
> GPIOD_OUT_HIGH); if (IS_ERR(data->gpiod_rst)) {
>  		dev_warn(dev, "gpio get reset pin failed\n");
>  		data->gpiod_rst = NULL;

Re: [PATCH v3 2/5] iio: proximity: sx9500: Add GPIO ACPI mapping table

[Jonathan Cameron]

On Sat, 4 Nov 2017 03:14:27 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Fri, 3 Nov 2017 15:03:37 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > In order to satisfy GPIO ACPI library requirements convert users of
> > gpiod_get_index() to correctly behave when there no mapping is
> > provided by firmware.
> > 
> > Here we add explicit mapping between _CRS GpioIo() resources and
> > their names used in the driver.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>  
> 
> Added cc's as for previous patch.  I guess this makes sense if we
> accept that fixes like the previous one should be in drivers at all.
> 
> If not the reset part still makes sense I suppose.
So, what this description is missing:
* Is this a fix for known problem?
* If so please add a fixes tag.
* If it is because we now have platforms that need this then say that.

I have no idea on the urgency of this patch from the description.

Jonathan
> 
> > ---
> >  drivers/iio/proximity/sx9500.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/proximity/sx9500.c
> > b/drivers/iio/proximity/sx9500.c index df23dbcc030a..eb687b3dd442
> > 100644 --- a/drivers/iio/proximity/sx9500.c
> > +++ b/drivers/iio/proximity/sx9500.c
> > @@ -32,9 +32,6 @@
> >  #define SX9500_DRIVER_NAME		"sx9500"
> >  #define SX9500_IRQ_NAME			"sx9500_event"
> >  
> > -#define SX9500_GPIO_INT			"interrupt"
> > -#define SX9500_GPIO_RESET		"reset"
> > -
> >  /* Register definitions. */
> >  #define SX9500_REG_IRQ_SRC		0x00
> >  #define SX9500_REG_STAT			0x01
> > @@ -866,26 +863,40 @@ static int sx9500_init_device(struct iio_dev
> > *indio_dev) return sx9500_init_compensation(indio_dev);
> >  }
> >  
> > +static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
> > +static const struct acpi_gpio_params interrupt_gpios = { 2, 0,
> > false }; +
> > +static const struct acpi_gpio_mapping acpi_sx9500_gpios[] = {
> > +	{ "reset-gpios", &reset_gpios, 1 },
> > +	{ "interrupt-gpios", &interrupt_gpios, 1 },
> > +	{ },
> > +};
> > +
> >  static void sx9500_gpio_probe(struct i2c_client *client,
> >  			      struct sx9500_data *data)
> >  {
> >  	struct gpio_desc *gpiod_int;
> >  	struct device *dev;
> > +	int ret;
> >  
> >  	if (!client)
> >  		return;
> >  
> >  	dev = &client->dev;
> >  
> > +	ret = devm_acpi_dev_add_driver_gpios(dev, acpi_sx9500_gpios);
> > +	if (ret)
> > +		dev_dbg(dev, "Unable to add GPIO mapping table\n");
> > +
> >  	if (client->irq <= 0) {
> > -		gpiod_int = devm_gpiod_get(dev, SX9500_GPIO_INT,
> > GPIOD_IN);
> > +		gpiod_int = devm_gpiod_get(dev, "interrupt",
> > GPIOD_IN); if (IS_ERR(gpiod_int))
> >  			dev_err(dev, "gpio get irq failed\n");
> >  		else
> >  			client->irq = gpiod_to_irq(gpiod_int);
> >  	}
> >  
> > -	data->gpiod_rst = devm_gpiod_get(dev, SX9500_GPIO_RESET,
> > GPIOD_OUT_HIGH);
> > +	data->gpiod_rst = devm_gpiod_get(dev, "reset",
> > GPIOD_OUT_HIGH); if (IS_ERR(data->gpiod_rst)) {
> >  		dev_warn(dev, "gpio get reset pin failed\n");
> >  		data->gpiod_rst = NULL;  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] iio: proximity: sx9500: Add GPIO ACPI mapping table

[Jonathan Cameron]

On Sun, 19 Nov 2017 15:29:34 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 4 Nov 2017 03:14:27 +0000
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Fri, 3 Nov 2017 15:03:37 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >   
> > > In order to satisfy GPIO ACPI library requirements convert users of
> > > gpiod_get_index() to correctly behave when there no mapping is
> > > provided by firmware.
> > > 
> > > Here we add explicit mapping between _CRS GpioIo() resources and
> > > their names used in the driver.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>    
> > 
> > Added cc's as for previous patch.  I guess this makes sense if we
> > accept that fixes like the previous one should be in drivers at all.
> > 
> > If not the reset part still makes sense I suppose.  
> So, what this description is missing:
> * Is this a fix for known problem?
> * If so please add a fixes tag.
> * If it is because we now have platforms that need this then say that.
> 
> I have no idea on the urgency of this patch from the description.
> 
Hi Andy, any updates on the above?   I probably won't be sending
a fixes pull until next weekend, but would be good to know if this
should be in there or not by then!

> Jonathan
> >   
> > > ---
> > >  drivers/iio/proximity/sx9500.c | 21 ++++++++++++++++-----
> > >  1 file changed, 16 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/iio/proximity/sx9500.c
> > > b/drivers/iio/proximity/sx9500.c index df23dbcc030a..eb687b3dd442
> > > 100644 --- a/drivers/iio/proximity/sx9500.c
> > > +++ b/drivers/iio/proximity/sx9500.c
> > > @@ -32,9 +32,6 @@
> > >  #define SX9500_DRIVER_NAME		"sx9500"
> > >  #define SX9500_IRQ_NAME			"sx9500_event"
> > >  
> > > -#define SX9500_GPIO_INT			"interrupt"
> > > -#define SX9500_GPIO_RESET		"reset"
> > > -
> > >  /* Register definitions. */
> > >  #define SX9500_REG_IRQ_SRC		0x00
> > >  #define SX9500_REG_STAT			0x01
> > > @@ -866,26 +863,40 @@ static int sx9500_init_device(struct iio_dev
> > > *indio_dev) return sx9500_init_compensation(indio_dev);
> > >  }
> > >  
> > > +static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
> > > +static const struct acpi_gpio_params interrupt_gpios = { 2, 0,
> > > false }; +
> > > +static const struct acpi_gpio_mapping acpi_sx9500_gpios[] = {
> > > +	{ "reset-gpios", &reset_gpios, 1 },
> > > +	{ "interrupt-gpios", &interrupt_gpios, 1 },
> > > +	{ },
> > > +};
> > > +
> > >  static void sx9500_gpio_probe(struct i2c_client *client,
> > >  			      struct sx9500_data *data)
> > >  {
> > >  	struct gpio_desc *gpiod_int;
> > >  	struct device *dev;
> > > +	int ret;
> > >  
> > >  	if (!client)
> > >  		return;
> > >  
> > >  	dev = &client->dev;
> > >  
> > > +	ret = devm_acpi_dev_add_driver_gpios(dev, acpi_sx9500_gpios);
> > > +	if (ret)
> > > +		dev_dbg(dev, "Unable to add GPIO mapping table\n");
> > > +
> > >  	if (client->irq <= 0) {
> > > -		gpiod_int = devm_gpiod_get(dev, SX9500_GPIO_INT,
> > > GPIOD_IN);
> > > +		gpiod_int = devm_gpiod_get(dev, "interrupt",
> > > GPIOD_IN); if (IS_ERR(gpiod_int))
> > >  			dev_err(dev, "gpio get irq failed\n");
> > >  		else
> > >  			client->irq = gpiod_to_irq(gpiod_int);
> > >  	}
> > >  
> > > -	data->gpiod_rst = devm_gpiod_get(dev, SX9500_GPIO_RESET,
> > > GPIOD_OUT_HIGH);
> > > +	data->gpiod_rst = devm_gpiod_get(dev, "reset",
> > > GPIOD_OUT_HIGH); if (IS_ERR(data->gpiod_rst)) {
> > >  		dev_warn(dev, "gpio get reset pin failed\n");
> > >  		data->gpiod_rst = NULL;    
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] iio: proximity: sx9500: Add GPIO ACPI mapping table

[Andy Shevchenko]

On Sat, 2017-11-25 at 14:24 +0000, Jonathan Cameron wrote:
> On Sun, 19 Nov 2017 15:29:34 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:

> Hi Andy, any updates on the above?   I probably won't be sending
> a fixes pull until next weekend, but would be good to know if this
> should be in there or not by then!

Yes, as you suggested I tried to add quirks to the core, so, this patch
will be definitely changed.

Thus, let's postpone.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 2/5] iio: proximity: sx9500: Add GPIO ACPI mapping table

[Linus Walleij]

On Mon, Nov 27, 2017 at 4:08 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Sat, 2017-11-25 at 14:24 +0000, Jonathan Cameron wrote:
>> On Sun, 19 Nov 2017 15:29:34 +0000
>> Jonathan Cameron <jic23@kernel.org> wrote:
>
>> Hi Andy, any updates on the above?   I probably won't be sending
>> a fixes pull until next weekend, but would be good to know if this
>> should be in there or not by then!
>
> Yes, as you suggested I tried to add quirks to the core, so, this patch
> will be definitely changed.

The quirks mechanics are merged to the GPIO core and an
immutable branch is available, so Jonathan could "just" pull that in
and apply (elegent) fixes on top. I think.

Yours,
Linus Walleij

Re: [PATCH v3 2/5] iio: proximity: sx9500: Add GPIO ACPI mapping table

[Andy Shevchenko]

On Fri, 2017-12-01 at 11:04 +0100, Linus Walleij wrote:
> On Mon, Nov 27, 2017 at 4:08 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Sat, 2017-11-25 at 14:24 +0000, Jonathan Cameron wrote:
> > > On Sun, 19 Nov 2017 15:29:34 +0000
> > > Jonathan Cameron <jic23@kernel.org> wrote:
> > > Hi Andy, any updates on the above?   I probably won't be sending
> > > a fixes pull until next weekend, but would be good to know if this
> > > should be in there or not by then!
> > 
> > Yes, as you suggested I tried to add quirks to the core, so, this
> > patch
> > will be definitely changed.
> 
> The quirks mechanics are merged to the GPIO core and an
> immutable branch is available, so Jonathan could "just" pull that in
> and apply (elegent) fixes on top. I think.

Unfortunately I didn't see any updates WRT GPIO/pin control in linux-next.
Did you synchronize repositories?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 3/5] iio: proximity: sx9500: Set IRQ pin to direction-input if necessary

[Andy Shevchenko]

With the new more strict ACPI gpio code the DSDT's IoRestriction flags
are honored on gpiod_get(), but in some DSDT's it is wrong, so explicitly
call gpiod_direction_input() on the IRQ GPIO if necessary.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/proximity/sx9500.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index eb687b3dd442..3cf054155779 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -17,6 +17,7 @@
 #include <linux/irq.h>
 #include <linux/acpi.h>
 #include <linux/gpio/consumer.h>
+#include <linux/gpio.h>
 #include <linux/regmap.h>
 #include <linux/pm.h>
 #include <linux/delay.h>
@@ -892,8 +893,13 @@ static void sx9500_gpio_probe(struct i2c_client *client,
 		gpiod_int = devm_gpiod_get(dev, "interrupt", GPIOD_IN);
 		if (IS_ERR(gpiod_int))
 			dev_err(dev, "gpio get irq failed\n");
-		else
+		else {
+			if (gpiod_get_direction(gpiod_int) != GPIOF_DIR_IN) {
+				dev_warn(dev, FW_BUG "IRQ GPIO not in input mode, fixing\n");
+				gpiod_direction_input(gpiod_int);
+			}
 			client->irq = gpiod_to_irq(gpiod_int);
+		}
 	}
 
 	data->gpiod_rst = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
-- 
2.14.2

Re: [PATCH v3 3/5] iio: proximity: sx9500: Set IRQ pin to direction-input if necessary

[Jonathan Cameron]

On Fri, 3 Nov 2017 15:03:38 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> With the new more strict ACPI gpio code the DSDT's IoRestriction flags
> are honored on gpiod_get(), but in some DSDT's it is wrong, so
> explicitly call gpiod_direction_input() on the IRQ GPIO if necessary.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Again, I really really don't like filling driver code with fixes
for broken firmware.  I appreciate we have to cope with this, but
it does rather seem like this should be moved into the core code
for say gpiod_get_irq.

Otherwise we get to fix this hundreds of times in different drivers
as I doubt this is the only driver effected by wrong tables...

Jonathan

> ---
>  drivers/iio/proximity/sx9500.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/proximity/sx9500.c
> b/drivers/iio/proximity/sx9500.c index eb687b3dd442..3cf054155779
> 100644 --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -17,6 +17,7 @@
>  #include <linux/irq.h>
>  #include <linux/acpi.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/gpio.h>
>  #include <linux/regmap.h>
>  #include <linux/pm.h>
>  #include <linux/delay.h>
> @@ -892,8 +893,13 @@ static void sx9500_gpio_probe(struct i2c_client
> *client, gpiod_int = devm_gpiod_get(dev, "interrupt", GPIOD_IN);
>  		if (IS_ERR(gpiod_int))
>  			dev_err(dev, "gpio get irq failed\n");
> -		else
> +		else {
> +			if (gpiod_get_direction(gpiod_int) !=
> GPIOF_DIR_IN) {
> +				dev_warn(dev, FW_BUG "IRQ GPIO not
> in input mode, fixing\n");
> +				gpiod_direction_input(gpiod_int);
> +			}
>  			client->irq = gpiod_to_irq(gpiod_int);
> +		}
>  	}
>  
>  	data->gpiod_rst = devm_gpiod_get(dev, "reset",
> GPIOD_OUT_HIGH);

Re: [PATCH v3 3/5] iio: proximity: sx9500: Set IRQ pin to direction-input if necessary

[Andy Shevchenko]

+Cc: Mika

On Sat, 2017-11-04 at 03:20 +0000, Jonathan Cameron wrote:
> On Fri, 3 Nov 2017 15:03:38 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > With the new more strict ACPI gpio code the DSDT's IoRestriction
> > flags
> > are honored on gpiod_get(), but in some DSDT's it is wrong, so
> > explicitly call gpiod_direction_input() on the IRQ GPIO if
> > necessary.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Again, I really really don't like filling driver code with fixes
> for broken firmware.  I appreciate we have to cope with this, but
> it does rather seem like this should be moved into the core code
> for say gpiod_get_irq.

I would love to fix in general, though it looks not so trivial:

- gpiod_get() doesn't know if GPIO is going to be used as IRQ
- gpiod_to_irq() doesn't know if descriptor in question comes from
GpioIo() ACPI resource

> 
> Otherwise we get to fix this hundreds of times in different drivers
> as I doubt this is the only driver effected by wrong tables...
> 
> Jonathan
> 
> > ---
> >  drivers/iio/proximity/sx9500.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/proximity/sx9500.c
> > b/drivers/iio/proximity/sx9500.c index eb687b3dd442..3cf054155779
> > 100644 --- a/drivers/iio/proximity/sx9500.c
> > +++ b/drivers/iio/proximity/sx9500.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/irq.h>
> >  #include <linux/acpi.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/gpio.h>
> >  #include <linux/regmap.h>
> >  #include <linux/pm.h>
> >  #include <linux/delay.h>
> > @@ -892,8 +893,13 @@ static void sx9500_gpio_probe(struct i2c_client
> > *client, gpiod_int = devm_gpiod_get(dev, "interrupt", GPIOD_IN);
> >  		if (IS_ERR(gpiod_int))
> >  			dev_err(dev, "gpio get irq failed\n");
> > -		else
> > +		else {
> > +			if (gpiod_get_direction(gpiod_int) !=
> > GPIOF_DIR_IN) {
> > +				dev_warn(dev, FW_BUG "IRQ GPIO not
> > in input mode, fixing\n");
> > +				gpiod_direction_input(gpiod_int);
> > +			}
> >  			client->irq = gpiod_to_irq(gpiod_int);
> > +		}
> >  	}
> >  
> >  	data->gpiod_rst = devm_gpiod_get(dev, "reset",
> > GPIOD_OUT_HIGH);
> 
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 3/5] iio: proximity: sx9500: Set IRQ pin to direction-input if necessary

[Mika Westerberg]

On Wed, Nov 08, 2017 at 06:35:46PM +0200, Andy Shevchenko wrote:
> +Cc: Mika
> 
> On Sat, 2017-11-04 at 03:20 +0000, Jonathan Cameron wrote:
> > On Fri, 3 Nov 2017 15:03:38 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > > With the new more strict ACPI gpio code the DSDT's IoRestriction
> > > flags
> > > are honored on gpiod_get(), but in some DSDT's it is wrong, so
> > > explicitly call gpiod_direction_input() on the IRQ GPIO if
> > > necessary.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > Again, I really really don't like filling driver code with fixes
> > for broken firmware.  I appreciate we have to cope with this, but
> > it does rather seem like this should be moved into the core code
> > for say gpiod_get_irq.
> 
> I would love to fix in general, though it looks not so trivial:
> 
> - gpiod_get() doesn't know if GPIO is going to be used as IRQ
> - gpiod_to_irq() doesn't know if descriptor in question comes from
> GpioIo() ACPI resource

One idea is to allow this strict mode to be relaxed by drivers perhaps
by passing quirks through struct acpi_gpio_mapping:

static const struct acpi_gpio_mapping acpi_foo_gpios[] = {
	/*
	 * This platform has a bug in ACPI GPIO description making IRQ
	 * GPIO to be output only. Ask the GPIO core to ignore this
	 * limit.
	 */
	{ "foobar-gpios", &foobar_gpios, 1, ACPI_QUIRK_IGNORE_IO_RESTRICTION },
	{},
};

or something like that. Not sure if I missed something obvious, though.

Re: [PATCH v3 3/5] iio: proximity: sx9500: Set IRQ pin to direction-input if necessary

[Linus Walleij]

On Wed, Nov 8, 2017 at 6:03 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Nov 08, 2017 at 06:35:46PM +0200, Andy Shevchenko wrote:
>> +Cc: Mika
>>
>> On Sat, 2017-11-04 at 03:20 +0000, Jonathan Cameron wrote:
>> > On Fri, 3 Nov 2017 15:03:38 +0200
>> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> >
>> > > With the new more strict ACPI gpio code the DSDT's IoRestriction
>> > > flags
>> > > are honored on gpiod_get(), but in some DSDT's it is wrong, so
>> > > explicitly call gpiod_direction_input() on the IRQ GPIO if
>> > > necessary.
>> > >
>> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> >
>> > Again, I really really don't like filling driver code with fixes
>> > for broken firmware.  I appreciate we have to cope with this, but
>> > it does rather seem like this should be moved into the core code
>> > for say gpiod_get_irq.
>>
>> I would love to fix in general, though it looks not so trivial:
>>
>> - gpiod_get() doesn't know if GPIO is going to be used as IRQ
>> - gpiod_to_irq() doesn't know if descriptor in question comes from
>> GpioIo() ACPI resource
>
> One idea is to allow this strict mode to be relaxed by drivers perhaps
> by passing quirks through struct acpi_gpio_mapping:
>
> static const struct acpi_gpio_mapping acpi_foo_gpios[] = {
>         /*
>          * This platform has a bug in ACPI GPIO description making IRQ
>          * GPIO to be output only. Ask the GPIO core to ignore this
>          * limit.
>          */
>         { "foobar-gpios", &foobar_gpios, 1, ACPI_QUIRK_IGNORE_IO_RESTRICTION },
>         {},
> };
>
> or something like that. Not sure if I missed something obvious, though.

This looks like an elegant solution to me.

Can it be done?

Yours,
Linus Walleij

Re: [PATCH v3 3/5] iio: proximity: sx9500: Set IRQ pin to direction-input if necessary

[Andy Shevchenko]

On Wed, 2017-11-08 at 21:45 +0100, Linus Walleij wrote:
> On Wed, Nov 8, 2017 at 6:03 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Nov 08, 2017 at 06:35:46PM +0200, Andy Shevchenko wrote:

> This looks like an elegant solution to me.
> 
> Can it be done?

I'm on it, though it will require some refactoring and clean ups.
(Business as usual, as I can say)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 3/5] iio: proximity: sx9500: Set IRQ pin to direction-input if necessary

[Andy Shevchenko]

On Wed, 2017-11-08 at 21:45 +0100, Linus Walleij wrote:
> On Wed, Nov 8, 2017 at 6:03 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Nov 08, 2017 at 06:35:46PM +0200, Andy Shevchenko wrote:
> > > +Cc: Mika

> > One idea is to allow this strict mode to be relaxed by drivers
> > perhaps
> > by passing quirks through struct acpi_gpio_mapping:
> > 
> > static const struct acpi_gpio_mapping acpi_foo_gpios[] = {
> >         /*
> >          * This platform has a bug in ACPI GPIO description making
> > IRQ
> >          * GPIO to be output only. Ask the GPIO core to ignore this
> >          * limit.
> >          */
> >         { "foobar-gpios", &foobar_gpios, 1,
> > ACPI_QUIRK_IGNORE_IO_RESTRICTION },
> >         {},
> > };
> > 
> > or something like that. Not sure if I missed something obvious,
> > though.
> 
> This looks like an elegant solution to me.
> 
> Can it be done?

I have sent today a series against GPIO ACPI library to support quirks
in general.

P.S. Just realized I forgot to put Suggested-by tag there.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 3/5] iio: proximity: sx9500: Set IRQ pin to direction-input if necessary

[Andy Shevchenko]

On Wed, 2017-11-08 at 19:03 +0200, Mika Westerberg wrote:
> On Wed, Nov 08, 2017 at 06:35:46PM +0200, Andy Shevchenko wrote:
> > +Cc: Mika
> > 
> > On Sat, 2017-11-04 at 03:20 +0000, Jonathan Cameron wrote:
> > > On Fri, 3 Nov 2017 15:03:38 +0200
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > 
> > > > With the new more strict ACPI gpio code the DSDT's IoRestriction
> > > > flags
> > > > are honored on gpiod_get(), but in some DSDT's it is wrong, so
> > > > explicitly call gpiod_direction_input() on the IRQ GPIO if
> > > > necessary.
> > > > 
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.co
> > > > m>
> > > 
> > > Again, I really really don't like filling driver code with fixes
> > > for broken firmware.  I appreciate we have to cope with this, but
> > > it does rather seem like this should be moved into the core code
> > > for say gpiod_get_irq.
> > 
> > I would love to fix in general, though it looks not so trivial:
> > 
> > - gpiod_get() doesn't know if GPIO is going to be used as IRQ
> > - gpiod_to_irq() doesn't know if descriptor in question comes from
> > GpioIo() ACPI resource
> 
> One idea is to allow this strict mode to be relaxed by drivers perhaps
> by passing quirks through struct acpi_gpio_mapping:
> 
> static const struct acpi_gpio_mapping acpi_foo_gpios[] = {
> 	/*
> 	 * This platform has a bug in ACPI GPIO description making IRQ
> 	 * GPIO to be output only. Ask the GPIO core to ignore this
> 	 * limit.
> 	 */
> 	{ "foobar-gpios", &foobar_gpios, 1,
> ACPI_QUIRK_IGNORE_IO_RESTRICTION },
> 	{},
> };
> 
> or something like that. Not sure if I missed something obvious,
> though.

Patch is sent, though I forgot to add you to Cc list.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 4/5] iio: proximity: sx9500: Add another ACPI ID

[Andy Shevchenko]

Add new ACPI ID for sx9500 as had been found on prototype board.

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

diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index 3cf054155779..640ea08cd3b2 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -1048,6 +1048,7 @@ static const struct dev_pm_ops sx9500_pm_ops = {
 
 static const struct acpi_device_id sx9500_acpi_match[] = {
 	{"SSX9500", 0},
+	{"SASX9500", 0},
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, sx9500_acpi_match);
-- 
2.14.2

Re: [PATCH v3 4/5] iio: proximity: sx9500: Add another ACPI ID

[Jonathan Cameron]

On Fri,  3 Nov 2017 15:03:39 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Add new ACPI ID for sx9500 as had been found on prototype board.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Applied to the togreg branch of iio.git and pushed out
as testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/proximity/sx9500.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
> index 3cf054155779..640ea08cd3b2 100644
> --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -1048,6 +1048,7 @@ static const struct dev_pm_ops sx9500_pm_ops = {
>  
>  static const struct acpi_device_id sx9500_acpi_match[] = {
>  	{"SSX9500", 0},
> +	{"SASX9500", 0},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, sx9500_acpi_match);

[PATCH v3 5/5] iio: magnetometer: ak8975: Add another ACPI ID

[Andy Shevchenko]

Add new ACPI ID for ak9911 as had been found on prototype board.

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

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index c09329069d0a..42a827a66512 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -788,6 +788,7 @@ static const struct acpi_device_id ak_acpi_match[] = {
 	{"AK8975", AK8975},
 	{"AK8963", AK8963},
 	{"INVN6500", AK8963},
+	{"AK009911", AK09911},
 	{"AK09911", AK09911},
 	{"AK09912", AK09912},
 	{ },
-- 
2.14.2

Re: [PATCH v3 1/5] iio: proximity: sx9500: Assign interrupt from GpioIo()

[Jonathan Cameron]

On Fri, 3 Nov 2017 15:03:36 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The commit 0f0796509c07
> 
> ("iio: remove gpio interrupt probing from drivers that use a single
> interrupt")
> 
> removed custom IRQ assignment for the drivers which are enumerated via
> ACPI or OF. Unfortunately, some ACPI tables have IRQ line defined as
> GpioIo() resource and thus automatic IRQ allocation will fail.

I'll ask the obvious question - is this allowed under the ACPI spec?

> 
> Partially revert the commit 0f0796509c07 to restore original
> behaviour.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I really don't like scattering fixes for broken ACPI tables through
drivers...  Is there really no better solution to this?

On patches like this best to pull in ACPI and GPIO on the cc list.

Also cc'd Mika who made the original change to support gpioint.



Jonathan

> ---
>  drivers/iio/proximity/sx9500.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/iio/proximity/sx9500.c
> b/drivers/iio/proximity/sx9500.c index 53c5d653e780..df23dbcc030a
> 100644 --- a/drivers/iio/proximity/sx9500.c
> +++ b/drivers/iio/proximity/sx9500.c
> @@ -869,6 +869,7 @@ static int sx9500_init_device(struct iio_dev
> *indio_dev) static void sx9500_gpio_probe(struct i2c_client *client,
>  			      struct sx9500_data *data)
>  {
> +	struct gpio_desc *gpiod_int;
>  	struct device *dev;
>  
>  	if (!client)
> @@ -876,6 +877,14 @@ static void sx9500_gpio_probe(struct i2c_client
> *client, 
>  	dev = &client->dev;
>  
> +	if (client->irq <= 0) {
> +		gpiod_int = devm_gpiod_get(dev, SX9500_GPIO_INT,
> GPIOD_IN);
> +		if (IS_ERR(gpiod_int))
> +			dev_err(dev, "gpio get irq failed\n");
> +		else
> +			client->irq = gpiod_to_irq(gpiod_int);
> +	}
> +
>  	data->gpiod_rst = devm_gpiod_get(dev, SX9500_GPIO_RESET,
> GPIOD_OUT_HIGH); if (IS_ERR(data->gpiod_rst)) {
>  		dev_warn(dev, "gpio get reset pin failed\n");

Re: [PATCH v3 1/5] iio: proximity: sx9500: Assign interrupt from GpioIo()

[Linus Walleij]

On Sat, Nov 4, 2017 at 4:11 AM, Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
> On Fri, 3 Nov 2017 15:03:36 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
>> The commit 0f0796509c07
>>
>> ("iio: remove gpio interrupt probing from drivers that use a single
>> interrupt")
>>
>> removed custom IRQ assignment for the drivers which are enumerated via
>> ACPI or OF. Unfortunately, some ACPI tables have IRQ line defined as
>> GpioIo() resource and thus automatic IRQ allocation will fail.
>
> I'll ask the obvious question - is this allowed under the ACPI spec?
>
>>
>> Partially revert the commit 0f0796509c07 to restore original
>> behaviour.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> I really don't like scattering fixes for broken ACPI tables through
> drivers...  Is there really no better solution to this?
>
> On patches like this best to pull in ACPI and GPIO on the cc list.
>
> Also cc'd Mika who made the original change to support gpioint.

Andy and Mika are the maintainers of ACPI GPIO, I have only
superficial knowledge of how that actually works. But it's good if
Mika can look at it too.

The only thing that I know for sure about ACPI GPIO is the same
as always : the people who devised it think that they are releaving
the OS authors of a burden by taking stuff over, and end up
creating more work for us since they make mistakes and
deploy them in firmware that "cannot be fixed".

Linus Walleij

Re: [PATCH v3 1/5] iio: proximity: sx9500: Assign interrupt from GpioIo()

[Mika Westerberg]

On Sat, Nov 04, 2017 at 03:11:19AM +0000, Jonathan Cameron wrote:
> On Fri, 3 Nov 2017 15:03:36 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > The commit 0f0796509c07
> > 
> > ("iio: remove gpio interrupt probing from drivers that use a single
> > interrupt")
> > 
> > removed custom IRQ assignment for the drivers which are enumerated via
> > ACPI or OF. Unfortunately, some ACPI tables have IRQ line defined as
> > GpioIo() resource and thus automatic IRQ allocation will fail.
> 
> I'll ask the obvious question - is this allowed under the ACPI spec?

Yes, it is perfectly fine.

> > Partially revert the commit 0f0796509c07 to restore original
> > behaviour.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I really don't like scattering fixes for broken ACPI tables through
> drivers...  Is there really no better solution to this?

This is not about broken ACPI tables. We just currently have
"convenience" stuff in the kernel that translates trivial things like a
single ACPI GpioInt() resource directly to a device interrupt. If the
table has multiple GpioInt()s or uses GpioIo() then it is up to the
driver to handle device specific details.

> On patches like this best to pull in ACPI and GPIO on the cc list.
> 
> Also cc'd Mika who made the original change to support gpioint.

The patch looks fine to me,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v3 1/5] iio: proximity: sx9500: Assign interrupt from GpioIo()

[Jonathan Cameron]

On Mon, 6 Nov 2017 11:35:56 +0200
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Sat, Nov 04, 2017 at 03:11:19AM +0000, Jonathan Cameron wrote:
> > On Fri, 3 Nov 2017 15:03:36 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >   
> > > The commit 0f0796509c07
> > > 
> > > ("iio: remove gpio interrupt probing from drivers that use a single
> > > interrupt")
> > > 
> > > removed custom IRQ assignment for the drivers which are enumerated via
> > > ACPI or OF. Unfortunately, some ACPI tables have IRQ line defined as
> > > GpioIo() resource and thus automatic IRQ allocation will fail.  
> > 
> > I'll ask the obvious question - is this allowed under the ACPI spec?  
> 
> Yes, it is perfectly fine.

I'm unconvinced...

> 
> > > Partially revert the commit 0f0796509c07 to restore original
> > > behaviour.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>  
> > 
> > I really don't like scattering fixes for broken ACPI tables through
> > drivers...  Is there really no better solution to this?  
> 
> This is not about broken ACPI tables. We just currently have
> "convenience" stuff in the kernel that translates trivial things like a
> single ACPI GpioInt() resource directly to a device interrupt. If the
> table has multiple GpioInt()s or uses GpioIo() then it is up to the
> driver to handle device specific details.

I agree on the multiple cases needing hanlding. 
What bothers me is that there is no guarantee at all that a GpioIo
can even do an interrupt.

(table 6.2.17 in the 6.1 spec for example makes it clear that we are
in a mess)  If it is a gpioio lots of the interrupt specific stuff
cannot be supplied (all the stuff in byte 7)

So as I read the ACPI specification any interrupt specified as GpioIO
is simply broken.

> 
> > On patches like this best to pull in ACPI and GPIO on the cc list.
> > 
> > Also cc'd Mika who made the original change to support gpioint.  
> 
> The patch looks fine to me,
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I'll probably take it anyway to support the platforms doing this particular
piece of fun as doubtlessly the chance of fixing the firmware is next
to nothing...

Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] iio: proximity: sx9500: Assign interrupt from GpioIo()

[Mika Westerberg]

On Sun, Nov 19, 2017 at 03:24:11PM +0000, Jonathan Cameron wrote:
> On Mon, 6 Nov 2017 11:35:56 +0200
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > On Sat, Nov 04, 2017 at 03:11:19AM +0000, Jonathan Cameron wrote:
> > > On Fri, 3 Nov 2017 15:03:36 +0200
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > >   
> > > > The commit 0f0796509c07
> > > > 
> > > > ("iio: remove gpio interrupt probing from drivers that use a single
> > > > interrupt")
> > > > 
> > > > removed custom IRQ assignment for the drivers which are enumerated via
> > > > ACPI or OF. Unfortunately, some ACPI tables have IRQ line defined as
> > > > GpioIo() resource and thus automatic IRQ allocation will fail.  
> > > 
> > > I'll ask the obvious question - is this allowed under the ACPI spec?  
> > 
> > Yes, it is perfectly fine.
> 
> I'm unconvinced...
> 
> > 
> > > > Partially revert the commit 0f0796509c07 to restore original
> > > > behaviour.
> > > > 
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>  
> > > 
> > > I really don't like scattering fixes for broken ACPI tables through
> > > drivers...  Is there really no better solution to this?  
> > 
> > This is not about broken ACPI tables. We just currently have
> > "convenience" stuff in the kernel that translates trivial things like a
> > single ACPI GpioInt() resource directly to a device interrupt. If the
> > table has multiple GpioInt()s or uses GpioIo() then it is up to the
> > driver to handle device specific details.
> 
> I agree on the multiple cases needing hanlding. 
> What bothers me is that there is no guarantee at all that a GpioIo
> can even do an interrupt.
> 
> (table 6.2.17 in the 6.1 spec for example makes it clear that we are
> in a mess)  If it is a gpioio lots of the interrupt specific stuff
> cannot be supplied (all the stuff in byte 7)
> 
> So as I read the ACPI specification any interrupt specified as GpioIO
> is simply broken.

Well, it is the same with DT if you just declare GPIOs as in "xxx-gpios"
but still use them to trigger interrupts.

Normally you would use GpioInt in ACPI case but sometimes there might
actually be need to use the GPIO as input/output without interrupts. I
remember there was some I2C connected touchpanel that required some
magic to be done when it was put to low power states. In those cases
GpioIo is more correct IMHO.

It is also possible that the BIOS people just messed this up.

> > > On patches like this best to pull in ACPI and GPIO on the cc list.
> > > 
> > > Also cc'd Mika who made the original change to support gpioint.  
> > 
> > The patch looks fine to me,
> > 
> > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> I'll probably take it anyway to support the platforms doing this particular
> piece of fun as doubtlessly the chance of fixing the firmware is next
> to nothing...

Thanks!

Re: [PATCH v3 1/5] iio: proximity: sx9500: Assign interrupt from GpioIo()

[Jonathan Cameron]

On Mon, 20 Nov 2017 12:30:27 +0200
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Sun, Nov 19, 2017 at 03:24:11PM +0000, Jonathan Cameron wrote:
> > On Mon, 6 Nov 2017 11:35:56 +0200
> > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> >   
> > > On Sat, Nov 04, 2017 at 03:11:19AM +0000, Jonathan Cameron wrote:  
> > > > On Fri, 3 Nov 2017 15:03:36 +0200
> > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > >     
> > > > > The commit 0f0796509c07
> > > > > 
> > > > > ("iio: remove gpio interrupt probing from drivers that use a single
> > > > > interrupt")
> > > > > 
> > > > > removed custom IRQ assignment for the drivers which are enumerated via
> > > > > ACPI or OF. Unfortunately, some ACPI tables have IRQ line defined as
> > > > > GpioIo() resource and thus automatic IRQ allocation will fail.    
> > > > 
> > > > I'll ask the obvious question - is this allowed under the ACPI spec?    
> > > 
> > > Yes, it is perfectly fine.  
> > 
> > I'm unconvinced...
> >   
> > >   
> > > > > Partially revert the commit 0f0796509c07 to restore original
> > > > > behaviour.
> > > > > 
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>    
> > > > 
> > > > I really don't like scattering fixes for broken ACPI tables through
> > > > drivers...  Is there really no better solution to this?    
> > > 
> > > This is not about broken ACPI tables. We just currently have
> > > "convenience" stuff in the kernel that translates trivial things like a
> > > single ACPI GpioInt() resource directly to a device interrupt. If the
> > > table has multiple GpioInt()s or uses GpioIo() then it is up to the
> > > driver to handle device specific details.  
> > 
> > I agree on the multiple cases needing hanlding. 
> > What bothers me is that there is no guarantee at all that a GpioIo
> > can even do an interrupt.
> > 
> > (table 6.2.17 in the 6.1 spec for example makes it clear that we are
> > in a mess)  If it is a gpioio lots of the interrupt specific stuff
> > cannot be supplied (all the stuff in byte 7)
> > 
> > So as I read the ACPI specification any interrupt specified as GpioIO
> > is simply broken.  
> 
> Well, it is the same with DT if you just declare GPIOs as in "xxx-gpios"
> but still use them to trigger interrupts.
> 
> Normally you would use GpioInt in ACPI case but sometimes there might
> actually be need to use the GPIO as input/output without interrupts. I
> remember there was some I2C connected touchpanel that required some
> magic to be done when it was put to low power states. In those cases
> GpioIo is more correct IMHO.
> 
> It is also possible that the BIOS people just messed this up.
> 
> > > > On patches like this best to pull in ACPI and GPIO on the cc list.
> > > > 
> > > > Also cc'd Mika who made the original change to support gpioint.    
> > > 
> > > The patch looks fine to me,
> > > 
> > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>  
> > 
> > I'll probably take it anyway to support the platforms doing this particular
> > piece of fun as doubtlessly the chance of fixing the firmware is next
> > to nothing...  
> 
> Thanks!
Not sure I actually replied to say I had applied this one to the fixes-togreg
branch of iio.git.  I certainly hadn't marked it as such locally and
just tried to apply it again ;)

Thanks,

Jonathan