* [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource @ 2020-10-14 13:31 Andy Shevchenko 2020-10-14 13:31 ` [PATCH v1 2/3] gpiolib: acpi: Use named item for enum gpiod_flags variable Andy Shevchenko ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Andy Shevchenko @ 2020-10-14 13:31 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Mika Westerberg, Hans de Goede Cc: Andy Shevchenko, Jamie McClymont In some cases the GpioInt() resource is coming with bias settings which may affect system functioning. Respect bias settings for GpioInt() resource by calling acpi_gpio_update_gpiod_*flags() API in acpi_dev_gpio_irq_get(). While at it, refactor to configure flags first and, only when succeeded, map the IRQ descriptor. Reported-by: Jamie McClymont <jamie@kwiius.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- This one highly depends on Intel pin control driver changes (for now [1], but might be more), so it's probably not supposed to be backported (at least right now). [1]: https://lore.kernel.org/linux-gpio/20201014104638.84043-1-andriy.shevchenko@linux.intel.com/T/ drivers/gpio/gpiolib-acpi.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 834a12f3219e..52b961673f16 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -942,21 +942,25 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) if (info.gpioint && idx++ == index) { unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT; + enum gpiod_flags dflags = info.flags; char label[32]; int irq; if (IS_ERR(desc)) return PTR_ERR(desc); - irq = gpiod_to_irq(desc); - if (irq < 0) - return irq; + acpi_gpio_update_gpiod_flags(&dflags, &info); + acpi_gpio_update_gpiod_lookup_flags(&lflags, &info); snprintf(label, sizeof(label), "GpioInt() %d", index); - ret = gpiod_configure_flags(desc, label, lflags, info.flags); + ret = gpiod_configure_flags(desc, label, lflags, dflags); if (ret < 0) return ret; + irq = gpiod_to_irq(desc); + if (irq < 0) + return irq; + irq_flags = acpi_dev_get_irq_type(info.triggering, info.polarity); -- 2.28.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v1 2/3] gpiolib: acpi: Use named item for enum gpiod_flags variable 2020-10-14 13:31 [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource Andy Shevchenko @ 2020-10-14 13:31 ` Andy Shevchenko 2020-10-21 9:59 ` Mika Westerberg 2020-10-14 13:31 ` [PATCH v1 3/3] gpiolib: of: " Andy Shevchenko 2020-10-21 9:58 ` [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource Mika Westerberg 2 siblings, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2020-10-14 13:31 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Mika Westerberg, Hans de Goede Cc: Andy Shevchenko Use named item instead of plain integer for enum gpiod_flags to make it clear that even 0 has its own meaning. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-acpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 52b961673f16..1b372f0dfe63 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -1136,7 +1136,7 @@ acpi_gpiochip_parse_own_gpio(struct acpi_gpio_chip *achip, int ret; *lflags = GPIO_LOOKUP_FLAGS_DEFAULT; - *dflags = 0; + *dflags = GPIOD_ASIS; *name = NULL; ret = fwnode_property_read_u32_array(fwnode, "gpios", gpios, -- 2.28.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/3] gpiolib: acpi: Use named item for enum gpiod_flags variable 2020-10-14 13:31 ` [PATCH v1 2/3] gpiolib: acpi: Use named item for enum gpiod_flags variable Andy Shevchenko @ 2020-10-21 9:59 ` Mika Westerberg 0 siblings, 0 replies; 9+ messages in thread From: Mika Westerberg @ 2020-10-21 9:59 UTC (permalink / raw) To: Andy Shevchenko Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Hans de Goede On Wed, Oct 14, 2020 at 04:31:53PM +0300, Andy Shevchenko wrote: > Use named item instead of plain integer for enum gpiod_flags > to make it clear that even 0 has its own meaning. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 3/3] gpiolib: of: Use named item for enum gpiod_flags variable 2020-10-14 13:31 [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource Andy Shevchenko 2020-10-14 13:31 ` [PATCH v1 2/3] gpiolib: acpi: Use named item for enum gpiod_flags variable Andy Shevchenko @ 2020-10-14 13:31 ` Andy Shevchenko 2020-10-21 9:58 ` [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource Mika Westerberg 2 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2020-10-14 13:31 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Mika Westerberg, Hans de Goede Cc: Andy Shevchenko Use named item instead of plain integer for enum gpiod_flags to make it clear that even 0 has its own meaning. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-of.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 2f895a2b8411..892a513b7a64 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -593,7 +593,7 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np, xlate_flags = 0; *lflags = GPIO_LOOKUP_FLAGS_DEFAULT; - *dflags = 0; + *dflags = GPIOD_ASIS; ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp); if (ret) -- 2.28.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource 2020-10-14 13:31 [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource Andy Shevchenko 2020-10-14 13:31 ` [PATCH v1 2/3] gpiolib: acpi: Use named item for enum gpiod_flags variable Andy Shevchenko 2020-10-14 13:31 ` [PATCH v1 3/3] gpiolib: of: " Andy Shevchenko @ 2020-10-21 9:58 ` Mika Westerberg 2020-10-21 16:38 ` Andy Shevchenko 2 siblings, 1 reply; 9+ messages in thread From: Mika Westerberg @ 2020-10-21 9:58 UTC (permalink / raw) To: Andy Shevchenko Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Hans de Goede, Jamie McClymont On Wed, Oct 14, 2020 at 04:31:52PM +0300, Andy Shevchenko wrote: > In some cases the GpioInt() resource is coming with bias settings > which may affect system functioning. Respect bias settings for > GpioInt() resource by calling acpi_gpio_update_gpiod_*flags() API > in acpi_dev_gpio_irq_get(). > > While at it, refactor to configure flags first and, only when succeeded, > map the IRQ descriptor. > > Reported-by: Jamie McClymont <jamie@kwiius.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > > This one highly depends on Intel pin control driver changes (for now [1], but > might be more), so it's probably not supposed to be backported (at least right > now). > > [1]: https://lore.kernel.org/linux-gpio/20201014104638.84043-1-andriy.shevchenko@linux.intel.com/T/ > > drivers/gpio/gpiolib-acpi.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > index 834a12f3219e..52b961673f16 100644 > --- a/drivers/gpio/gpiolib-acpi.c > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -942,21 +942,25 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) > > if (info.gpioint && idx++ == index) { > unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT; > + enum gpiod_flags dflags = info.flags; > char label[32]; > int irq; > > if (IS_ERR(desc)) > return PTR_ERR(desc); > > - irq = gpiod_to_irq(desc); > - if (irq < 0) > - return irq; > + acpi_gpio_update_gpiod_flags(&dflags, &info); > + acpi_gpio_update_gpiod_lookup_flags(&lflags, &info); > > snprintf(label, sizeof(label), "GpioInt() %d", index); > - ret = gpiod_configure_flags(desc, label, lflags, info.flags); > + ret = gpiod_configure_flags(desc, label, lflags, dflags); > if (ret < 0) > return ret; > > + irq = gpiod_to_irq(desc); > + if (irq < 0) > + return irq; Should the above be undone if the conversion here fails? In any case looks good so, Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource 2020-10-21 9:58 ` [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource Mika Westerberg @ 2020-10-21 16:38 ` Andy Shevchenko 2020-10-22 6:51 ` Mika Westerberg 2020-10-22 9:05 ` Hans de Goede 0 siblings, 2 replies; 9+ messages in thread From: Andy Shevchenko @ 2020-10-21 16:38 UTC (permalink / raw) To: Mika Westerberg Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Hans de Goede, Jamie McClymont On Wed, Oct 21, 2020 at 12:58:54PM +0300, Mika Westerberg wrote: > On Wed, Oct 14, 2020 at 04:31:52PM +0300, Andy Shevchenko wrote: > > In some cases the GpioInt() resource is coming with bias settings > > which may affect system functioning. Respect bias settings for > > GpioInt() resource by calling acpi_gpio_update_gpiod_*flags() API > > in acpi_dev_gpio_irq_get(). > > > > While at it, refactor to configure flags first and, only when succeeded, > > map the IRQ descriptor. ... > > - irq = gpiod_to_irq(desc); > > - if (irq < 0) > > - return irq; > > + acpi_gpio_update_gpiod_flags(&dflags, &info); > > + acpi_gpio_update_gpiod_lookup_flags(&lflags, &info); > > > > snprintf(label, sizeof(label), "GpioInt() %d", index); > > - ret = gpiod_configure_flags(desc, label, lflags, info.flags); > > + ret = gpiod_configure_flags(desc, label, lflags, dflags); > > if (ret < 0) > > return ret; > > > > + irq = gpiod_to_irq(desc); > > + if (irq < 0) > > + return irq; > > Should the above be undone if the conversion here fails? But wouldn't it be not good if we changed direction, for example, and then change it back? (IRQ requires input, which is safer, right?) This makes me think what gpiod_to_irq() may do for physical state of the pin. On the brief search it seems there is no side effect on the pin with that function, so, perhaps the original order has that in mind to not shuffle with line if mapping can't be established. But if setting flags fail, we may got into the state which is not equal to the initial one, right? So, in either case I see no good way to roll back the physical pin state changes. But I can return ordering of the calls in next version. What do you think? > In any case looks good so, > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> Thanks! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource 2020-10-21 16:38 ` Andy Shevchenko @ 2020-10-22 6:51 ` Mika Westerberg 2020-10-22 9:05 ` Hans de Goede 1 sibling, 0 replies; 9+ messages in thread From: Mika Westerberg @ 2020-10-22 6:51 UTC (permalink / raw) To: Andy Shevchenko Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Hans de Goede, Jamie McClymont On Wed, Oct 21, 2020 at 07:38:44PM +0300, Andy Shevchenko wrote: > On Wed, Oct 21, 2020 at 12:58:54PM +0300, Mika Westerberg wrote: > > On Wed, Oct 14, 2020 at 04:31:52PM +0300, Andy Shevchenko wrote: > > > In some cases the GpioInt() resource is coming with bias settings > > > which may affect system functioning. Respect bias settings for > > > GpioInt() resource by calling acpi_gpio_update_gpiod_*flags() API > > > in acpi_dev_gpio_irq_get(). > > > > > > While at it, refactor to configure flags first and, only when succeeded, > > > map the IRQ descriptor. > > ... > > > > - irq = gpiod_to_irq(desc); > > > - if (irq < 0) > > > - return irq; > > > + acpi_gpio_update_gpiod_flags(&dflags, &info); > > > + acpi_gpio_update_gpiod_lookup_flags(&lflags, &info); > > > > > > snprintf(label, sizeof(label), "GpioInt() %d", index); > > > - ret = gpiod_configure_flags(desc, label, lflags, info.flags); > > > + ret = gpiod_configure_flags(desc, label, lflags, dflags); > > > if (ret < 0) > > > return ret; > > > > > > + irq = gpiod_to_irq(desc); > > > + if (irq < 0) > > > + return irq; > > > > Should the above be undone if the conversion here fails? > > But wouldn't it be not good if we changed direction, for example, and then > change it back? (IRQ requires input, which is safer, right?) > > This makes me think what gpiod_to_irq() may do for physical state of the pin. > On the brief search it seems there is no side effect on the pin with that > function, so, perhaps the original order has that in mind to not shuffle with > line if mapping can't be established. But if setting flags fail, we may got > into the state which is not equal to the initial one, right? > > So, in either case I see no good way to roll back the physical pin state > changes. But I can return ordering of the calls in next version. > > What do you think? If there is no good way rolling back to the previous state then I think this ordering is as good as the original, so up to you :-) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource 2020-10-21 16:38 ` Andy Shevchenko 2020-10-22 6:51 ` Mika Westerberg @ 2020-10-22 9:05 ` Hans de Goede 2020-10-22 10:43 ` Andy Shevchenko 1 sibling, 1 reply; 9+ messages in thread From: Hans de Goede @ 2020-10-22 9:05 UTC (permalink / raw) To: Andy Shevchenko, Mika Westerberg Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Jamie McClymont Hi, On 10/21/20 6:38 PM, Andy Shevchenko wrote: > On Wed, Oct 21, 2020 at 12:58:54PM +0300, Mika Westerberg wrote: >> On Wed, Oct 14, 2020 at 04:31:52PM +0300, Andy Shevchenko wrote: >>> In some cases the GpioInt() resource is coming with bias settings >>> which may affect system functioning. Respect bias settings for >>> GpioInt() resource by calling acpi_gpio_update_gpiod_*flags() API >>> in acpi_dev_gpio_irq_get(). >>> >>> While at it, refactor to configure flags first and, only when succeeded, >>> map the IRQ descriptor. > > ... > >>> - irq = gpiod_to_irq(desc); >>> - if (irq < 0) >>> - return irq; >>> + acpi_gpio_update_gpiod_flags(&dflags, &info); >>> + acpi_gpio_update_gpiod_lookup_flags(&lflags, &info); >>> >>> snprintf(label, sizeof(label), "GpioInt() %d", index); >>> - ret = gpiod_configure_flags(desc, label, lflags, info.flags); >>> + ret = gpiod_configure_flags(desc, label, lflags, dflags); >>> if (ret < 0) >>> return ret; >>> >>> + irq = gpiod_to_irq(desc); >>> + if (irq < 0) >>> + return irq; >> >> Should the above be undone if the conversion here fails? > > But wouldn't it be not good if we changed direction, for example, and then > change it back? (IRQ requires input, which is safer, right?) > > This makes me think what gpiod_to_irq() may do for physical state of the pin. > On the brief search it seems there is no side effect on the pin with that > function, so, perhaps the original order has that in mind to not shuffle with > line if mapping can't be established. But if setting flags fail, we may got > into the state which is not equal to the initial one, right? > > So, in either case I see no good way to roll back the physical pin state > changes. But I can return ordering of the calls in next version. > > What do you think? I think it would be good to do a new version where you keep the original ordering. Also if you decide to keep the ordering change, that really should be in a separate commit and not squashed into this one, so that e.g. a bisect can determine the difference between the ordering change or the flags changes causing any issues. Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource 2020-10-22 9:05 ` Hans de Goede @ 2020-10-22 10:43 ` Andy Shevchenko 0 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2020-10-22 10:43 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Jamie McClymont On Thu, Oct 22, 2020 at 12:32 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 10/21/20 6:38 PM, Andy Shevchenko wrote: > > On Wed, Oct 21, 2020 at 12:58:54PM +0300, Mika Westerberg wrote: > >> On Wed, Oct 14, 2020 at 04:31:52PM +0300, Andy Shevchenko wrote: > >>> In some cases the GpioInt() resource is coming with bias settings > >>> which may affect system functioning. Respect bias settings for > >>> GpioInt() resource by calling acpi_gpio_update_gpiod_*flags() API > >>> in acpi_dev_gpio_irq_get(). > >>> > >>> While at it, refactor to configure flags first and, only when succeeded, > >>> map the IRQ descriptor. > > > > ... > > > >>> - irq = gpiod_to_irq(desc); > >>> - if (irq < 0) > >>> - return irq; > >>> + acpi_gpio_update_gpiod_flags(&dflags, &info); > >>> + acpi_gpio_update_gpiod_lookup_flags(&lflags, &info); > >>> > >>> snprintf(label, sizeof(label), "GpioInt() %d", index); > >>> - ret = gpiod_configure_flags(desc, label, lflags, info.flags); > >>> + ret = gpiod_configure_flags(desc, label, lflags, dflags); > >>> if (ret < 0) > >>> return ret; > >>> > >>> + irq = gpiod_to_irq(desc); > >>> + if (irq < 0) > >>> + return irq; > >> > >> Should the above be undone if the conversion here fails? > > > > But wouldn't it be not good if we changed direction, for example, and then > > change it back? (IRQ requires input, which is safer, right?) > > > > This makes me think what gpiod_to_irq() may do for physical state of the pin. > > On the brief search it seems there is no side effect on the pin with that > > function, so, perhaps the original order has that in mind to not shuffle with > > line if mapping can't be established. But if setting flags fail, we may got > > into the state which is not equal to the initial one, right? > > > > So, in either case I see no good way to roll back the physical pin state > > changes. But I can return ordering of the calls in next version. > > > > What do you think? > > I think it would be good to do a new version where you keep the original > ordering. > > Also if you decide to keep the ordering change, that really should be > in a separate commit and not squashed into this one, so that e.g. a bisect > can determine the difference between the ordering change or the flags > changes causing any issues. Ack. Thanks Hans, Mika for your comments! I'll revert that piece of change. I dunno what I had in mind when I did it in the first place... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-22 10:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-14 13:31 [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource Andy Shevchenko 2020-10-14 13:31 ` [PATCH v1 2/3] gpiolib: acpi: Use named item for enum gpiod_flags variable Andy Shevchenko 2020-10-21 9:59 ` Mika Westerberg 2020-10-14 13:31 ` [PATCH v1 3/3] gpiolib: of: " Andy Shevchenko 2020-10-21 9:58 ` [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource Mika Westerberg 2020-10-21 16:38 ` Andy Shevchenko 2020-10-22 6:51 ` Mika Westerberg 2020-10-22 9:05 ` Hans de Goede 2020-10-22 10:43 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).