gpiolib: acpi: Respect bias settings for GpioInt() resource
PATCH 1/3 gpiolib: acpi: Respect bias settings for GpioInt() resource
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.
---
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/[email protected]/T/
drivers/gpio/gpiolib-acpi.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/gpio/gpiolib-acpi.c +8 -4
@@ -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);
Signature · 1 line
2.28.0
PATCH 2/3 gpiolib: acpi: Use named item for enum gpiod_flags variable
Use named item instead of plain integer for enum gpiod_flags to make it clear that even 0 has its own meaning.
--- drivers/gpio/gpiolib-acpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib-acpi.c +1 -1
@@ -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,
Signature · 1 line
2.28.0
Re: [PATCH v1 2/3] gpiolib: acpi: Use named item for enum gpiod_flags variable
Quoted 4 lines · Andy Shevchenko
Use named item instead of plain integer for enum
gpiod_flagsto make it clear that even 0 has its own meaning.Signed-off-by Andy Shevchenko <[email protected]>
PATCH 3/3 gpiolib: of: Use named item for enum gpiod_flags variable
In reply to Andy ShevchenkoUse named item instead of plain integer for enum gpiod_flags to make it clear that even 0 has its own meaning.
--- drivers/gpio/gpiolib-of.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib-of.c +1 -1
@@ -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)
Signature · 1 line
2.28.0
Quoted 41 lines · Andy Shevchenko
In some cases the
GpioInt()resource is coming with bias settings which may affect system functioning. Respect bias settings forGpioInt()resource by calling acpi_gpio_update_gpiod_*flags() API inacpi_dev_gpio_irq_get().While at it, refactor to configure flags first and, only when succeeded, map the IRQ descriptor.
Reported-by Jamie McClymont <[email protected]>Signed-off-by Andy Shevchenko <[email protected]>---
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/[email protected]/T/
drivers/gpio/gpiolib-acpi.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)drivers/gpio/gpiolib-acpi.c +3
-3@@ -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);
drivers/gpio/gpiolib-acpi.c int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) · line 962
Quoted 10 lines
drivers/gpio/gpiolib-acpi.c +4
-1snprintf (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,
Quoted 8 lines · Mika Westerberg
Quoted 7 lines · Andy Shevchenko
In some cases the
GpioInt()resource is coming with bias settings which may affect system functioning. Respect bias settings forGpioInt()resource by calling acpi_gpio_update_gpiod_*flags() API inacpi_dev_gpio_irq_get().While at it, refactor to configure flags first and, only when succeeded, map the IRQ descriptor.
...
Quoted 12 lines
Quoted 12 lines
drivers/gpio/gpiolib-acpi.c +3
-4- 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;
Quoted 5 lines
drivers/gpio/gpiolib-acpi.c int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) · line 962
Quoted 3 lines
drivers/gpio/gpiolib-acpi.c +3
-0+ 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?
Quoted 3 lines
In any case looks good so,
Reviewed-by Mika Westerberg <[email protected]>
Thanks!
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 39 lines · Andy Shevchenko
Quoted 8 lines · Mika Westerberg
Quoted 7 lines · Andy Shevchenko
In some cases the
GpioInt()resource is coming with bias settings which may affect system functioning. Respect bias settings forGpioInt()resource by calling acpi_gpio_update_gpiod_*flags() API inacpi_dev_gpio_irq_get().While at it, refactor to configure flags first and, only when succeeded, map the IRQ descriptor.
...
Quoted 12 lines
Quoted 12 lines
drivers/gpio/gpiolib-acpi.c +3
-4- 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;Quoted 5 lines
drivers/gpio/gpiolib-acpi.c int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) · line 962
Quoted 3 lines
drivers/gpio/gpiolib-acpi.c +3
-0+ 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?
Quoted 4 lines
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 :-)
Hi,
Quoted 39 lines · Andy Shevchenko
Quoted 8 lines · Mika Westerberg
Quoted 7 lines · Andy Shevchenko
In some cases the
GpioInt()resource is coming with bias settings which may affect system functioning. Respect bias settings forGpioInt()resource by calling acpi_gpio_update_gpiod_*flags() API inacpi_dev_gpio_irq_get().While at it, refactor to configure flags first and, only when succeeded, map the IRQ descriptor.
...
Quoted 12 lines
Quoted 12 lines
drivers/gpio/gpiolib-acpi.c +3
-4- 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;Quoted 5 lines
drivers/gpio/gpiolib-acpi.c int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) · line 962
Quoted 3 lines
drivers/gpio/gpiolib-acpi.c +3
-0+ 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?
Quoted 4 lines
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
Quoted 45 lines · Hans de Goede
Quoted 39 lines · Andy Shevchenko
Quoted 8 lines · Mika Westerberg
Quoted 7 lines · Andy Shevchenko
In some cases the
GpioInt()resource is coming with bias settings which may affect system functioning. Respect bias settings forGpioInt()resource by calling acpi_gpio_update_gpiod_*flags() API inacpi_dev_gpio_irq_get().While at it, refactor to configure flags first and, only when succeeded, map the IRQ descriptor.
...
Quoted 12 lines
Quoted 12 lines
drivers/gpio/gpiolib-acpi.c +3
-4- 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;Quoted 5 lines
drivers/gpio/gpiolib-acpi.c int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) · line 962
Quoted 3 lines
drivers/gpio/gpiolib-acpi.c +3
-0+ 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?Quoted 4 lines
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?
Quoted 7 lines
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...
Signature · 2 lines
With Best Regards, Andy Shevchenko