| Message ID | 20201028171757.765866-1-ribalda@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: acpi: Support GpioInt with active_low polarity | expand |
On Wed, Oct 28, 2020 at 06:17:57PM +0100, Ricardo Ribalda wrote: > On the current implementation we only support active_high polarity for > GpioInt. > > There can be cases where a GPIO has active_low polarity and it is also a > IRQ source. > > De-couple the irq_polarity and active_low fields instead of re-use it. > > With this patch we support ACPI devices such as: Is it real device on the market?! This table is broken. _DSD GPIO active_low is only for GpioIo(). If it is a ChromeBook, please fix the firmware. > Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings > { > GpioInt (Edge, ActiveBoth, Exclusive, PullDefault, 0x0000, > "\\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , > ) > { // Pin list > 0x0064 > } > }) > Name (_DSD, Package (0x02) // _DSD: Device-Specific Data > { > ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, > Package (0x01) > { > Package (0x02) > { > "privacy-gpio", > Package (0x04) > { > \_SB.PCI0.XHCI.RHUB.HS07, > Zero, > Zero, > One > } > } > } > })
Hi Andy On Thu, Oct 29, 2020 at 3:38 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Oct 28, 2020 at 06:17:57PM +0100, Ricardo Ribalda wrote: > > On the current implementation we only support active_high polarity for > > GpioInt. > > > > There can be cases where a GPIO has active_low polarity and it is also a > > IRQ source. > > > > De-couple the irq_polarity and active_low fields instead of re-use it. > > > > With this patch we support ACPI devices such as: > > Is it real device on the market?! Yes, it is a chromebook. > > This table is broken. _DSD GPIO active_low is only for GpioIo(). AFAIK the format of the _DSD is not in the ACPI standard. We have decided its fields. (please correct me if I am wrong here) On the other mail I have described why we need to make use of the active_low on a GpioInt() If there is another way of describing ActiveBoth + inverted polarity please let me know and I will go that way. Thanks > > If it is a ChromeBook, please fix the firmware. > > > Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings > > { > > GpioInt (Edge, ActiveBoth, Exclusive, PullDefault, 0x0000, > > "\\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , > > ) > > { // Pin list > > 0x0064 > > } > > }) > > Name (_DSD, Package (0x02) // _DSD: Device-Specific Data > > { > > ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, > > Package (0x01) > > { > > Package (0x02) > > { > > "privacy-gpio", > > Package (0x04) > > { > > \_SB.PCI0.XHCI.RHUB.HS07, > > Zero, > > Zero, > > One > > } > > } > > } > > }) > > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Oct 29, 2020 at 5:37 PM Ricardo Ribalda <ribalda@google.com> wrote: > On Thu, Oct 29, 2020 at 3:38 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Wed, Oct 28, 2020 at 06:17:57PM +0100, Ricardo Ribalda wrote: > > > On the current implementation we only support active_high polarity for > > > GpioInt. > > > > > > There can be cases where a GPIO has active_low polarity and it is also a > > > IRQ source. > > > > > > De-couple the irq_polarity and active_low fields instead of re-use it. > > > > > > With this patch we support ACPI devices such as: > > > > Is it real device on the market?! > > Yes, it is a chromebook. You mean it's already on sale with this broken table?! > > This table is broken. _DSD GPIO active_low is only for GpioIo(). > > AFAIK the format of the _DSD is not in the ACPI standard. We have > decided its fields. (please correct me if I am wrong here) _DSD is a concept that is part of the spec, but each UUID and its application is out of scope indeed. GPIO application to _DSD is described in the in-kernel documentation. Thanks for pointing out the issues it has. > On the other mail I have described why we need to make use of the > active_low on a GpioInt() > > If there is another way of describing ActiveBoth + inverted polarity > please let me know and I will go that way. I answered it there, please, continue this topic there. NAK to the proposed change. > > If it is a ChromeBook, please fix the firmware.
Adding Tomasz in CC in case he wants to share more info about the device. On Thu, Oct 29, 2020 at 6:31 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Oct 29, 2020 at 5:37 PM Ricardo Ribalda <ribalda@google.com> wrote: > > On Thu, Oct 29, 2020 at 3:38 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Wed, Oct 28, 2020 at 06:17:57PM +0100, Ricardo Ribalda wrote: > > > > On the current implementation we only support active_high polarity for > > > > GpioInt. > > > > > > > > There can be cases where a GPIO has active_low polarity and it is also a > > > > IRQ source. > > > > > > > > De-couple the irq_polarity and active_low fields instead of re-use it. > > > > > > > > With this patch we support ACPI devices such as: > > > > > > Is it real device on the market?! > > > > Yes, it is a chromebook. > > You mean it's already on sale with this broken table?! I do not agree that it is broken. It follows the current standard ;) > > > > This table is broken. _DSD GPIO active_low is only for GpioIo(). > > > > AFAIK the format of the _DSD is not in the ACPI standard. We have > > decided its fields. (please correct me if I am wrong here) > > _DSD is a concept that is part of the spec, but each UUID and its > application is out of scope indeed. > > GPIO application to _DSD is described in the in-kernel documentation. > Thanks for pointing out the issues it has. > > > On the other mail I have described why we need to make use of the > > active_low on a GpioInt() > > > > If there is another way of describing ActiveBoth + inverted polarity > > please let me know and I will go that way. > > I answered it there, please, continue this topic there. > NAK to the proposed change. > > > > If it is a ChromeBook, please fix the firmware. Lets agree what is the best way to describe in acpi my usecase and I will implement it. > > -- > With Best Regards, > Andy Shevchenko
On Thu, Oct 29, 2020 at 8:09 PM Ricardo Ribalda <ribalda@google.com> wrote:
> Adding Tomasz in CC in case he wants to share more info about the device.
OK, but let's discuss in another thread.
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 834a12f3219e..bc33c1056391 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -624,7 +624,7 @@ int acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags, break; } - if (info->polarity == GPIO_ACTIVE_LOW) + if (info->active_low) *lookupflags |= GPIO_ACTIVE_LOW; return 0; @@ -665,6 +665,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data) agpio->pin_table[pin_index]); lookup->info.pin_config = agpio->pin_config; lookup->info.gpioint = gpioint; + lookup->info.active_low = !!lookup->active_low; /* * Polarity and triggering are only specified for GpioInt @@ -675,11 +676,10 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data) */ if (lookup->info.gpioint) { lookup->info.flags = GPIOD_IN; - lookup->info.polarity = agpio->polarity; + lookup->info.irq_polarity = agpio->polarity; lookup->info.triggering = agpio->triggering; } else { lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio); - lookup->info.polarity = lookup->active_low; } } @@ -958,7 +958,7 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index) return ret; irq_flags = acpi_dev_get_irq_type(info.triggering, - info.polarity); + info.irq_polarity); /* Set type if specified and different than the current one */ if (irq_flags != IRQ_TYPE_NONE && diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h index 1c6d65cf0629..816a2d7a21ed 100644 --- a/drivers/gpio/gpiolib-acpi.h +++ b/drivers/gpio/gpiolib-acpi.h @@ -16,7 +16,8 @@ struct acpi_device; * @flags: GPIO initialization flags * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo * @pin_config: pin bias as provided by ACPI - * @polarity: interrupt polarity as provided by ACPI + * @irq_polarity: interrupt polarity as provided by ACPI + * @active_low: pin polarity as provided by ACPI * @triggering: triggering type as provided by ACPI * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping */ @@ -25,7 +26,8 @@ struct acpi_gpio_info { enum gpiod_flags flags; bool gpioint; int pin_config; - int polarity; + int irq_polarity; + bool active_low; int triggering; unsigned int quirks; };
On the current implementation we only support active_high polarity for GpioInt. There can be cases where a GPIO has active_low polarity and it is also a IRQ source. De-couple the irq_polarity and active_low fields instead of re-use it. With this patch we support ACPI devices such as: Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { GpioInt (Edge, ActiveBoth, Exclusive, PullDefault, 0x0000, "\\_SB.PCI0.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0064 } }) Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, Package (0x01) { Package (0x02) { "privacy-gpio", Package (0x04) { \_SB.PCI0.XHCI.RHUB.HS07, Zero, Zero, One } } } }) Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/gpio/gpiolib-acpi.c | 8 ++++---- drivers/gpio/gpiolib-acpi.h | 6 ++++-- 2 files changed, 8 insertions(+), 6 deletions(-)