iio: proximity: sx9500: Assign interrupt from GpioIo()

PATCH Applied Andy Shevchenko 9 participants 25 messages Nov 3, 2017 – Feb 26, 2018 3 messages not in the archive

Subsystems (3)

ACPI GPIO IIO

lore

Share & tools

lore

Andy Shevchenko

author

2017-11-03 13:04 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/iio/proximity/sx9500.c | 9 +++++++++
 1 file changed, 9 insertions(+)

drivers/iio/proximity/sx9500.c +9 -0

index 53c5d653e780..df23dbcc030a 100644
@@ -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");

Signature · 1 line

2.14.2

Andy Shevchenko

author

2017-11-03 13:03 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/iio/proximity/sx9500.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

drivers/iio/proximity/sx9500.c +16 -5

index df23dbcc030a..eb687b3dd442 100644
@@ -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;

Signature · 1 line

2.14.2

A message that is not in the archive (sent off-list or not yet received): 20171103130340.42459-2-andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]

Jonathan Cameron

2017-11-04 03:14 UTC #3 lore raw

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

Quoted 8 lines · On Fri, 3 Nov 2017 15:03:37 +0200 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-VuQAYsv1563Yd54FQh9/[email protected]>

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.

Quoted 63 lines

---
 drivers/iio/proximity/sx9500.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

(quoted diff) +0 -0

b/drivers/iio/proximity/sx9500.c index df23dbcc030a..eb687b3dd442 100644 --- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c

drivers/iio/proximity/sx9500.c +2 -3

@@ -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 }; +

drivers/iio/proximity/sx9500.c +11 -1

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

(quoted diff) +1 -0

+		gpiod_int = devm_gpiod_get(dev, "interrupt",

GPIOD_IN); if (IS_ERR(gpiod_int))

drivers/iio/proximity/sx9500.c +0 -1

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

(quoted diff) +1 -0

+	data->gpiod_rst = devm_gpiod_get(dev, "reset",

GPIOD_OUT_HIGH); if (IS_ERR(data->gpiod_rst)) {

drivers/iio/proximity/sx9500.c +0 -0

 		dev_warn(dev, "gpio get reset pin failed\n");
 		data->gpiod_rst = NULL;

Jonathan Cameron

maintainer

2017-11-19 15:29 UTC #4 lore raw

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

Quoted 10 lines · On Sat, 4 Nov 2017 03:14:27 +0000 Jonathan Cameron…

Quoted 7 lines · On Fri, 3 Nov 2017 15:03:37 +0200 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.

Quoted 6 lines

Quoted 1 line
Signed-off-by Andy Shevchenko <[email protected]>
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

Quoted 69 lines

Quoted 62 lines

---
 drivers/iio/proximity/sx9500.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

(quoted diff) +0 -0

b/drivers/iio/proximity/sx9500.c index df23dbcc030a..eb687b3dd442 100644 --- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c

drivers/iio/proximity/sx9500.c +2 -3

@@ -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 }; +

drivers/iio/proximity/sx9500.c +11 -1

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

(quoted diff) +1 -0

+		gpiod_int = devm_gpiod_get(dev, "interrupt",

GPIOD_IN); if (IS_ERR(gpiod_int))

drivers/iio/proximity/sx9500.c +0 -1

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

(quoted diff) +1 -0

+	data->gpiod_rst = devm_gpiod_get(dev, "reset",

GPIOD_OUT_HIGH); if (IS_ERR(data->gpiod_rst)) {

drivers/iio/proximity/sx9500.c static int sx9500_init_device(struct iio_dev *indio_dev) · line 902

Quoted 2 lines

drivers/iio/proximity/sx9500.c +0 -0

 		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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jonathan Cameron

2017-11-25 14:24 UTC #5 lore raw

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

Quoted 18 lines · On Sun, 19 Nov 2017 15:29:34 +0000 Jonathan Cameron…

Quoted 15 lines · On Sat, 4 Nov 2017 03:14:27 +0000 Jonathan Cameron…
Quoted 8 lines · On Fri, 3 Nov 2017 15:03:37 +0200 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-VuQAYsv1563Yd54FQh9/[email protected]>
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.

Quoted 8 lines

Quoted 1 line
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!

Quoted 75 lines

Jonathan
Quoted 58 lines
Quoted 58 lines
  
---
 drivers/iio/proximity/sx9500.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
(quoted diff) +0 -0
b/drivers/iio/proximity/sx9500.c index df23dbcc030a..eb687b3dd442 100644 --- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
drivers/iio/proximity/sx9500.c +2 -3
@@ -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 }; +
drivers/iio/proximity/sx9500.c +11 -1
+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);
(quoted diff) +1 -0
+		gpiod_int = devm_gpiod_get(dev, "interrupt",
GPIOD_IN); if (IS_ERR(gpiod_int))
drivers/iio/proximity/sx9500.c +0 -0
 			dev_err(dev, "gpio get irq failed\n");
 		else
 			client->irq = gpiod_to_irq(gpiod_int);
 	}
Quoted 11 lines
drivers/iio/proximity/sx9500.c static int sx9500_init_device(struct iio_dev *indio_dev) · line 902
Quoted 6 lines
(quoted diff) +0 -1
-	data->gpiod_rst = devm_gpiod_get(dev, SX9500_GPIO_RESET,
GPIOD_OUT_HIGH);
(quoted diff) +1 -0
+	data->gpiod_rst = devm_gpiod_get(dev, "reset",
GPIOD_OUT_HIGH); if (IS_ERR(data->gpiod_rst)) {
drivers/iio/proximity/sx9500.c +0 -0
 		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 [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Andy Shevchenko

author

2017-11-27 15:08 UTC #6 lore raw

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

Quoted 6 lines · Jonathan Cameron

On Sun, 19 Nov 2017 15:29:34 +0000
Jonathan Cameron <[email protected]> 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.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

A message that is not in the archive (sent off-list or not yet received): 1511795292.25007.454.camel-VuQAYsv1563Yd54FQh9/[email protected]

Linus Walleij

2017-12-01 10:04 UTC #8 lore raw

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

Quoted 10 lines · Andy Shevchenko

Quoted 6 lines · Jonathan Cameron
On Sun, 19 Nov 2017 15:29:34 +0000
Jonathan Cameron <[email protected]> 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

Andy Shevchenko

author

2017-12-01 12:36 UTC #9 lore raw

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

Quoted 9 lines · Linus Walleij

Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · Jonathan Cameron
On Sun, 19 Nov 2017 15:29:34 +0000
Jonathan Cameron <[email protected]> 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!

Quoted 7 lines

Quoted 3 lines
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?

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Andy Shevchenko

author

2017-11-03 13:03 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/iio/proximity/sx9500.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/iio/proximity/sx9500.c +7 -1

index eb687b3dd442..3cf054155779 100644
@@ -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);

Signature · 1 line

2.14.2

Jonathan Cameron

2017-11-04 03:21 UTC #11 lore raw

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

Quoted 5 lines · On Fri, 3 Nov 2017 15:03:38 +0200 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 <[email protected]>

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

Quoted 34 lines

---
 drivers/iio/proximity/sx9500.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/iio/proximity/sx9500.c index eb687b3dd442..3cf054155779 100644 --- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c

drivers/iio/proximity/sx9500.c +7 -1

@@ -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

author

2017-11-08 16:37 UTC #12 lore raw

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

+Cc: Mika

Quoted 9 lines · Jonathan Cameron

Quoted 6 lines · On Fri, 3 Nov 2017 15:03:38 +0200 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.

Quoted 6 lines

Quoted 1 line
Signed-off-by Andy Shevchenko <[email protected]>
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

Quoted 42 lines

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
Quoted 34 lines
---
 drivers/iio/proximity/sx9500.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/iio/proximity/sx9500.c index eb687b3dd442..3cf054155779 100644 --- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
drivers/iio/proximity/sx9500.c +7 -1
@@ -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);

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Mika Westerberg

2017-11-08 17:03 UTC #13 lore raw

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

Quoted 19 lines · Andy Shevchenko

+Cc: Mika
Quoted 9 lines · Jonathan Cameron
Quoted 6 lines · On Fri, 3 Nov 2017 15:03:38 +0200 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.
Quoted 6 lines
Quoted 1 line
Signed-off-by Andy Shevchenko <[email protected]>
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.

Quoted 5 lines

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.

Linus Walleij

2017-11-08 20:45 UTC #14 lore raw

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

Quoted 39 lines · Mika Westerberg

Quoted 19 lines · Andy Shevchenko
+Cc: Mika
Quoted 9 lines · Jonathan Cameron
Quoted 6 lines · On Fri, 3 Nov 2017 15:03:38 +0200 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.
Quoted 6 lines
Quoted 1 line
Signed-off-by Andy Shevchenko <[email protected]>
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.
Quoted 5 lines
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 }, {},
};

Quoted 1 line

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

Andy Shevchenko

author

2017-11-08 20:52 UTC #15 lore raw

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

Quoted 7 lines · Linus Walleij

Quoted 1 line · Mika Westerberg
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)

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Andy Shevchenko

author

2017-11-10 18:13 UTC #16 lore raw

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

In reply to Linus Walleij

Quoted 21 lines · Linus Walleij

Quoted 19 lines · Mika Westerberg
Quoted 1 line · Andy Shevchenko
+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 },
        {},
};

Quoted 6 lines

Quoted 2 lines
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.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Andy Shevchenko

author

2018-02-26 19:51 UTC #17 lore raw

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

In reply to Mika Westerberg

Quoted 41 lines · Mika Westerberg

Quoted 20 lines · Andy Shevchenko
+Cc: Mika
Quoted 9 lines · Jonathan Cameron
Quoted 6 lines · On Fri, 3 Nov 2017 15:03:38 +0200 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.
Quoted 7 lines
Quoted 2 lines
Signed-off-by Andy Shevchenko <[email protected]
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.
Quoted 5 lines
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 },
	{},
};

Quoted 2 lines

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

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

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Andy Shevchenko

author

2017-11-03 13:03 UTC #18 lore raw

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

In reply to Andy Shevchenko

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

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/iio/proximity/sx9500.c | 1 +
 1 file changed, 1 insertion(+)

drivers/iio/proximity/sx9500.c +1 -0

index 3cf054155779..640ea08cd3b2 100644
@@ -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);

Signature · 1 line

2.14.2

Jonathan Cameron

maintainer Applied

2017-11-19 15:57 UTC #19 lore raw

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

Quoted 3 lines · On Fri, 3 Nov 2017 15:03:39 +0200 Andy Shevchenko…

Add new ACPI ID for sx9500 as had been found on prototype board.
Signed-off-by Andy Shevchenko <[email protected]>

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

Thanks,

Jonathan

Quoted 16 lines

---
 drivers/iio/proximity/sx9500.c | 1 +
 1 file changed, 1 insertion(+)

drivers/iio/proximity/sx9500.c +1 -0

index 3cf054155779..640ea08cd3b2 100644
@@ -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);

Andy Shevchenko

author

2017-11-03 13:03 UTC #20 lore raw

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

In reply to Andy Shevchenko

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

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/iio/magnetometer/ak8975.c | 1 +
 1 file changed, 1 insertion(+)

drivers/iio/magnetometer/ak8975.c +1 -0

index c09329069d0a..42a827a66512 100644
@@ -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},
 	{ },

Signature · 1 line

2.14.2

Jonathan Cameron

2017-11-04 03:11 UTC #21 lore raw

In reply to Andy Shevchenko

Quoted 8 lines · On Fri, 3 Nov 2017 15:03:36 +0200 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.

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

Quoted 5 lines

Partially revert the commit 0f0796509c07 to restore original behaviour.
Signed-off-by Andy Shevchenko <[email protected]>

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

Quoted 32 lines

---
 drivers/iio/proximity/sx9500.c | 9 +++++++++
 1 file changed, 9 insertions(+)

(quoted diff) +0 -0

b/drivers/iio/proximity/sx9500.c index 53c5d653e780..df23dbcc030a 100644 --- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c

drivers/iio/proximity/sx9500.c +9 -0

@@ -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");

Linus Walleij

2017-11-04 10:43 UTC #22 lore raw

Quoted 18 lines · Jonathan Cameron

Quoted 8 lines · On Fri, 3 Nov 2017 15:03:36 +0200 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.
I'll ask the obvious question - is this allowed under the ACPI spec?
Quoted 4 lines
Partially revert the commit 0f0796509c07 to restore original behaviour.

Quoted 8 lines

Quoted 1 line
Signed-off-by Andy Shevchenko <[email protected]>
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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Mika Westerberg

Acked-by

2017-11-06 09:35 UTC #24 lore raw

Quoted 5 lines · Jonathan Cameron

Quoted 2 lines · On Fri, 3 Nov 2017 15:03:36 +0200 Andy Shevchenko…
The commit 0f0796509c07

Quoted 8 lines

Quoted 6 lines
("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.

Quoted 7 lines

Quoted 4 lines
Partially revert the commit 0f0796509c07 to restore original behaviour.
Signed-off-by Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]>
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.

Quoted 3 lines

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-VuQAYsv1563Yd54FQh9/[email protected]>

Jonathan Cameron

maintainer

2017-11-19 15:24 UTC #25 lore raw

Quoted 9 lines · On Mon, 6 Nov 2017 11:35:56 +0200 Mika Westerberg…

Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 3 Nov 2017 15:03:36 +0200 Andy Shevchenko…
The commit 0f0796509c07
("iio: remove gpio interrupt probing from drivers that use a single interrupt")

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
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...

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
Partially revert the commit 0f0796509c07 to restore original behaviour.
Signed-off-by Andy Shevchenko <[email protected]>

Quoted 8 lines

Quoted 2 lines
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.

Quoted 8 lines

Quoted 4 lines
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 <[email protected]>

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

Quoted 4 lines

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

Mika Westerberg

2017-11-20 10:30 UTC #26 lore raw

Quoted 41 lines · Jonathan Cameron

Quoted 9 lines · On Mon, 6 Nov 2017 11:35:56 +0200 Mika Westerberg…
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 3 Nov 2017 15:03:36 +0200 Andy Shevchenko…
The commit 0f0796509c07
("iio: remove gpio interrupt probing from drivers that use a single interrupt")
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
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...
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Partially revert the commit 0f0796509c07 to restore original behaviour.
Signed-off-by Andy Shevchenko <[email protected]>
Quoted 8 lines
Quoted 2 lines
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.

Quoted 6 lines

(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.

Quoted 11 lines

Quoted 7 lines
Quoted 3 lines
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 <[email protected]>
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!

Jonathan Cameron

maintainer

2017-11-25 14:28 UTC #27 lore raw

Quoted 66 lines · On Mon, 20 Nov 2017 12:30:27 +0200 Mika Westerberg…

Quoted 41 lines · Jonathan Cameron
Quoted 9 lines · On Mon, 6 Nov 2017 11:35:56 +0200 Mika Westerberg…
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 3 Nov 2017 15:03:36 +0200 Andy Shevchenko…
The commit 0f0796509c07
("iio: remove gpio interrupt probing from drivers that use a single interrupt")
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
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...
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Partially revert the commit 0f0796509c07 to restore original behaviour.
Signed-off-by Andy Shevchenko <[email protected]>
Quoted 8 lines
Quoted 2 lines
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.
Quoted 6 lines
(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.
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
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,

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Acked-by Mika Westerberg <[email protected]>
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