Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] gpiolib: acpi: Support GpioInt with active_low polarity
@ 2020-10-28 17:17 Ricardo Ribalda
  2020-10-28 18:27 ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Ricardo Ribalda @ 2020-10-28 17:17 UTC (permalink / raw)
  To: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, linux-acpi, linux-kernel
  Cc: Ricardo Ribalda

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

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;
 };
-- 
2.29.0.rc2.309.g374f81d7ae-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpiolib: acpi: Support GpioInt with active_low polarity
  2020-10-28 17:17 [PATCH] gpiolib: acpi: Support GpioInt with active_low polarity Ricardo Ribalda
@ 2020-10-28 18:27 ` Andy Shevchenko
  2020-10-29 15:34   ` Ricardo Ribalda
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-10-28 18:27 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Mika Westerberg, Linus Walleij, Bartosz Golaszewski, linux-gpio,
	linux-acpi, linux-kernel

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


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpiolib: acpi: Support GpioInt with active_low polarity
  2020-10-28 18:27 ` Andy Shevchenko
@ 2020-10-29 15:34   ` Ricardo Ribalda
  2020-10-29 17:31     ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Ricardo Ribalda @ 2020-10-29 15:34 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mika Westerberg, Linus Walleij, Bartosz Golaszewski, linux-gpio,
	linux-acpi, linux-kernel

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


-- 
Ricardo Ribalda

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpiolib: acpi: Support GpioInt with active_low polarity
  2020-10-29 15:34   ` Ricardo Ribalda
@ 2020-10-29 17:31     ` Andy Shevchenko
  2020-10-29 18:08       ` Ricardo Ribalda
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-10-29 17:31 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij,
	Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	ACPI Devel Maling List, Linux Kernel Mailing List

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.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpiolib: acpi: Support GpioInt with active_low polarity
  2020-10-29 17:31     ` Andy Shevchenko
@ 2020-10-29 18:08       ` Ricardo Ribalda
  2020-10-29 18:14         ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Ricardo Ribalda @ 2020-10-29 18:08 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij,
	Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	ACPI Devel Maling List, Linux Kernel Mailing List, Tomasz Figa

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



-- 
Ricardo Ribalda

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gpiolib: acpi: Support GpioInt with active_low polarity
  2020-10-29 18:08       ` Ricardo Ribalda
@ 2020-10-29 18:14         ` Andy Shevchenko
  0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-10-29 18:14 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Andy Shevchenko, Mika Westerberg, Linus Walleij,
	Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	ACPI Devel Maling List, Linux Kernel Mailing List, Tomasz Figa

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.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-28 17:17 [PATCH] gpiolib: acpi: Support GpioInt with active_low polarity Ricardo Ribalda
2020-10-28 18:27 ` Andy Shevchenko
2020-10-29 15:34   ` Ricardo Ribalda
2020-10-29 17:31     ` Andy Shevchenko
2020-10-29 18:08       ` Ricardo Ribalda
2020-10-29 18:14         ` Andy Shevchenko

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git