| Message ID | 20171103130340.42459-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); -- 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
+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); > >
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. -- 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
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 -- 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
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)
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.
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.
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);
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(-)