linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes
@ 2020-10-28 20:50 Andy Shevchenko
  2020-10-28 20:51 ` [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo() Andy Shevchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-28 20:50 UTC (permalink / raw)
  To: Mika Westerberg, linux-acpi, Rafael J. Wysocki, linux-gpio
  Cc: Andy Shevchenko

Fix factual mistakes and style issues in GPIO properties document.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../firmware-guide/acpi/gpio-properties.rst   | 29 ++++++++++---------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/Documentation/firmware-guide/acpi/gpio-properties.rst b/Documentation/firmware-guide/acpi/gpio-properties.rst
index bb6d74f23ee0..e6e65ceb2ca1 100644
--- a/Documentation/firmware-guide/acpi/gpio-properties.rst
+++ b/Documentation/firmware-guide/acpi/gpio-properties.rst
@@ -20,9 +20,9 @@ index, like the ASL example below shows::
 
       Name (_CRS, ResourceTemplate ()
       {
-          GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
+          GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,
                   "\\_SB.GPO0", 0, ResourceConsumer) {15}
-          GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
+          GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,
                   "\\_SB.GPO0", 0, ResourceConsumer) {27, 31}
       })
 
@@ -49,7 +49,7 @@ index
 pin
   Pin in the GpioIo()/GpioInt() resource. Typically this is zero.
 active_low
-  If 1 the GPIO is marked as active_low.
+  If 1, the GPIO is marked as active_low.
 
 Since ACPI GpioIo() resource does not have a field saying whether it is
 active low or high, the "active_low" argument can be used here.  Setting
@@ -112,8 +112,8 @@ Example::
   Package () {
       "gpio-line-names",
       Package () {
-          "SPI0_CS_N", "EXP2_INT", "MUX6_IO", "UART0_RXD", "MUX7_IO",
-          "LVL_C_A1", "MUX0_IO", "SPI1_MISO"
+          "SPI0_CS_N", "EXP2_INT", "MUX6_IO", "UART0_RXD",
+          "MUX7_IO", "LVL_C_A1", "MUX0_IO", "SPI1_MISO",
       }
   }
 
@@ -137,7 +137,7 @@ to the GPIO lines it is going to use and provide the GPIO subsystem with a
 mapping between those names and the ACPI GPIO resources corresponding to them.
 
 To do that, the driver needs to define a mapping table as a NULL-terminated
-array of struct acpi_gpio_mapping objects that each contain a name, a pointer
+array of struct acpi_gpio_mapping objects that each contains a name, a pointer
 to an array of line data (struct acpi_gpio_params) objects and the size of that
 array.  Each struct acpi_gpio_params object consists of three fields,
 crs_entry_index, line_index, active_low, representing the index of the target
@@ -154,13 +154,14 @@ question would look like this::
   static const struct acpi_gpio_mapping bluetooth_acpi_gpios[] = {
     { "reset-gpios", &reset_gpio, 1 },
     { "shutdown-gpios", &shutdown_gpio, 1 },
-    { },
+    { }
   };
 
 Next, the mapping table needs to be passed as the second argument to
-acpi_dev_add_driver_gpios() that will register it with the ACPI device object
-pointed to by its first argument.  That should be done in the driver's .probe()
-routine.  On removal, the driver should unregister its GPIO mapping table by
+acpi_dev_add_driver_gpios() or its managed analogue that will
+register it with the ACPI device object pointed to by its first
+argument. That should be done in the driver's .probe() routine.
+On removal, the driver should unregister its GPIO mapping table by
 calling acpi_dev_remove_driver_gpios() on the ACPI device object where that
 table was previously registered.
 
@@ -191,12 +192,12 @@ The driver might expect to get the right GPIO when it does::
 but since there is no way to know the mapping between "reset" and
 the GpioIo() in _CRS desc will hold ERR_PTR(-ENOENT).
 
-The driver author can solve this by passing the mapping explictly
-(the recommended way and documented in the above chapter).
+The driver author can solve this by passing the mapping explicitly
+(this is the recommended way and it's documented in the above chapter).
 
 The ACPI GPIO mapping tables should not contaminate drivers that are not
 knowing about which exact device they are servicing on. It implies that
-the ACPI GPIO mapping tables are hardly linked to ACPI ID and certain
+the ACPI GPIO mapping tables are hardly linked to an ACPI ID and certain
 objects, as listed in the above chapter, of the device in question.
 
 Getting GPIO descriptor
@@ -229,5 +230,5 @@ Case 2 explicitly tells GPIO core to look for resources in _CRS.
 Be aware that gpiod_get_index() in cases 1 and 2, assuming that there
 are two versions of ACPI device description provided and no mapping is
 present in the driver, will return different resources. That's why a
-certain driver has to handle them carefully as explained in previous
+certain driver has to handle them carefully as explained in the previous
 chapter.
-- 
2.28.0


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

* [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-28 20:50 [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes Andy Shevchenko
@ 2020-10-28 20:51 ` Andy Shevchenko
  2020-10-28 21:10   ` Ricardo Ribalda
  2020-10-29  8:13   ` Mika Westerberg
  2020-10-28 20:51 ` [PATCH v1 3/3] Documentation: firmware-guide: gpio-properties: Clarify initial output state Andy Shevchenko
  2020-10-29  8:13 ` [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes Mika Westerberg
  2 siblings, 2 replies; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-28 20:51 UTC (permalink / raw)
  To: Mika Westerberg, linux-acpi, Rafael J. Wysocki, linux-gpio
  Cc: Andy Shevchenko, Ricardo Ribalda

It appears that people may misinterpret active_low field in _DSD
for GpioInt() resource. Add a paragraph to clarify this.

Reported-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/firmware-guide/acpi/gpio-properties.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/firmware-guide/acpi/gpio-properties.rst b/Documentation/firmware-guide/acpi/gpio-properties.rst
index e6e65ceb2ca1..370fe46c6af9 100644
--- a/Documentation/firmware-guide/acpi/gpio-properties.rst
+++ b/Documentation/firmware-guide/acpi/gpio-properties.rst
@@ -55,6 +55,9 @@ Since ACPI GpioIo() resource does not have a field saying whether it is
 active low or high, the "active_low" argument can be used here.  Setting
 it to 1 marks the GPIO as active low.
 
+Note, active_low in _DSD does not make sense for GpioInt() resource and
+must be 0. GpioInt() resource has its own means of defining it.
+
 In our Bluetooth example the "reset-gpios" refers to the second GpioIo()
 resource, second pin in that resource with the GPIO number of 31.
 
-- 
2.28.0


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

* [PATCH v1 3/3] Documentation: firmware-guide: gpio-properties: Clarify initial output state
  2020-10-28 20:50 [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes Andy Shevchenko
  2020-10-28 20:51 ` [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo() Andy Shevchenko
@ 2020-10-28 20:51 ` Andy Shevchenko
  2020-10-29  8:16   ` Mika Westerberg
  2020-10-29  8:13 ` [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes Mika Westerberg
  2 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-28 20:51 UTC (permalink / raw)
  To: Mika Westerberg, linux-acpi, Rafael J. Wysocki, linux-gpio
  Cc: Andy Shevchenko

GpioIo() doesn't provide an explicit state for an output pin.
Linux tries to be smart and uses a common sense based on other
parameters. Document how it looks like in the code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../firmware-guide/acpi/gpio-properties.rst   | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/firmware-guide/acpi/gpio-properties.rst b/Documentation/firmware-guide/acpi/gpio-properties.rst
index 370fe46c6af9..59aad6138b6e 100644
--- a/Documentation/firmware-guide/acpi/gpio-properties.rst
+++ b/Documentation/firmware-guide/acpi/gpio-properties.rst
@@ -61,6 +61,29 @@ must be 0. GpioInt() resource has its own means of defining it.
 In our Bluetooth example the "reset-gpios" refers to the second GpioIo()
 resource, second pin in that resource with the GPIO number of 31.
 
+The GpioIo() resource unfortunately doesn't explicitly provide an initial
+state of the output pin which driver should use during its initialization.
+
+Linux tries to use common sense here and derives the state from the bias
+and polarity settings. The table below shows the expectations:
+
+=========  =============  ==============
+Pull Bias     Polarity     Requested...
+=========  =============  ==============
+Implicit     x            AS IS (assumed firmware configured for us)
+Explicit     x (no _DSD)  as Pull Bias (Up == High, Down == Low),
+                          assuming non-active (Polarity = !Pull Bias)
+Down         Low          as low, assuming active
+Down         High         as low, assuming non-active
+Up           Low          as high, assuming non-active
+Up           High         as high, assuming active
+=========  =============  ==============
+
+That said, for our above example the both GPIOs, since the bias setting
+is explicit and _DSD is present, will be treated as active with a high
+polarity and Linux will configure the pins in this state until a driver
+reprograms them differently.
+
 It is possible to leave holes in the array of GPIOs. This is useful in
 cases like with SPI host controllers where some chip selects may be
 implemented as GPIOs and some as native signals. For example a SPI host
-- 
2.28.0


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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-28 20:51 ` [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo() Andy Shevchenko
@ 2020-10-28 21:10   ` Ricardo Ribalda
  2020-10-29 14:46     ` Andy Shevchenko
  2020-10-29  8:13   ` Mika Westerberg
  1 sibling, 1 reply; 19+ messages in thread
From: Ricardo Ribalda @ 2020-10-28 21:10 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mika Westerberg, linux-acpi, Rafael J. Wysocki, linux-gpio

Hi Andy

Thanks for your patch and super fast response.

I think there are two different concepts here:

1) when the pin has a low value, it is  0 or a 1? =>active_low

2) when do I get an irq, low->high or high->low => irq polarity

When I read the acpi spec for GpioInt()
https://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf page
934, it has the same problem as for GpioIo(), it does not express the
active_low and this is where the _DSD field comes handy.

Without using the active_low, how can we describe  a pin that is
active low and has to trigger an irq on both edges?

Thanks again


On Wed, Oct 28, 2020 at 9:51 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> It appears that people may misinterpret active_low field in _DSD
> for GpioInt() resource. Add a paragraph to clarify this.
>
> Reported-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/firmware-guide/acpi/gpio-properties.rst | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/firmware-guide/acpi/gpio-properties.rst b/Documentation/firmware-guide/acpi/gpio-properties.rst
> index e6e65ceb2ca1..370fe46c6af9 100644
> --- a/Documentation/firmware-guide/acpi/gpio-properties.rst
> +++ b/Documentation/firmware-guide/acpi/gpio-properties.rst
> @@ -55,6 +55,9 @@ Since ACPI GpioIo() resource does not have a field saying whether it is
>  active low or high, the "active_low" argument can be used here.  Setting
>  it to 1 marks the GPIO as active low.
>
> +Note, active_low in _DSD does not make sense for GpioInt() resource and
> +must be 0. GpioInt() resource has its own means of defining it.
> +
>  In our Bluetooth example the "reset-gpios" refers to the second GpioIo()
>  resource, second pin in that resource with the GPIO number of 31.
>
> --
> 2.28.0
>


-- 
Ricardo Ribalda

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

* Re: [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes
  2020-10-28 20:50 [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes Andy Shevchenko
  2020-10-28 20:51 ` [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo() Andy Shevchenko
  2020-10-28 20:51 ` [PATCH v1 3/3] Documentation: firmware-guide: gpio-properties: Clarify initial output state Andy Shevchenko
@ 2020-10-29  8:13 ` Mika Westerberg
  2020-10-29 11:10   ` Andy Shevchenko
  2 siblings, 1 reply; 19+ messages in thread
From: Mika Westerberg @ 2020-10-29  8:13 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-acpi, Rafael J. Wysocki, linux-gpio

On Wed, Oct 28, 2020 at 10:50:59PM +0200, Andy Shevchenko wrote:
> Fix factual mistakes and style issues in GPIO properties document.

Can you clarify here what factual mistakes this fixes :)

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-28 20:51 ` [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo() Andy Shevchenko
  2020-10-28 21:10   ` Ricardo Ribalda
@ 2020-10-29  8:13   ` Mika Westerberg
  1 sibling, 0 replies; 19+ messages in thread
From: Mika Westerberg @ 2020-10-29  8:13 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-acpi, Rafael J. Wysocki, linux-gpio, Ricardo Ribalda

On Wed, Oct 28, 2020 at 10:51:00PM +0200, Andy Shevchenko wrote:
> It appears that people may misinterpret active_low field in _DSD
> for GpioInt() resource. Add a paragraph to clarify this.
> 
> Reported-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

* Re: [PATCH v1 3/3] Documentation: firmware-guide: gpio-properties: Clarify initial output state
  2020-10-28 20:51 ` [PATCH v1 3/3] Documentation: firmware-guide: gpio-properties: Clarify initial output state Andy Shevchenko
@ 2020-10-29  8:16   ` Mika Westerberg
  0 siblings, 0 replies; 19+ messages in thread
From: Mika Westerberg @ 2020-10-29  8:16 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-acpi, Rafael J. Wysocki, linux-gpio

On Wed, Oct 28, 2020 at 10:51:01PM +0200, Andy Shevchenko wrote:
> GpioIo() doesn't provide an explicit state for an output pin.
> Linux tries to be smart and uses a common sense based on other
> parameters. Document how it looks like in the code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

* Re: [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes
  2020-10-29  8:13 ` [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes Mika Westerberg
@ 2020-10-29 11:10   ` Andy Shevchenko
  0 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-29 11:10 UTC (permalink / raw)
  To: Mika Westerberg; +Cc: linux-acpi, Rafael J. Wysocki, linux-gpio

On Thu, Oct 29, 2020 at 10:13:15AM +0200, Mika Westerberg wrote:
> On Wed, Oct 28, 2020 at 10:50:59PM +0200, Andy Shevchenko wrote:
> > Fix factual mistakes and style issues in GPIO properties document.
> 
> Can you clarify here what factual mistakes this fixes :)

The IoRestriction has wrong direction in the examples.
I'll do in v2.

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-28 21:10   ` Ricardo Ribalda
@ 2020-10-29 14:46     ` Andy Shevchenko
  2020-10-29 14:54       ` Ricardo Ribalda
  0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-29 14:46 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Mika Westerberg, linux-acpi, Rafael J. Wysocki, linux-gpio

On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:
> Hi Andy
> 
> Thanks for your patch and super fast response.
> 
> I think there are two different concepts here:
> 
> 1) when the pin has a low value, it is  0 or a 1? =>active_low

I'm not sure I have got it. GpioIo() has no such property and
_DSD active_low is documented as being polarity setting:

	active_low == 1 => active low polarity.

> 2) when do I get an irq, low->high or high->low => irq polarity

IRQ polarity is clearly defined by GpioInt() resource in the ACPI
specification.

> When I read the acpi spec for GpioInt()
> https://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf page
> 934, it has the same problem as for GpioIo(), it does not express the
> active_low and this is where the _DSD field comes handy.

ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
Resource Descriptor Macro).

> Without using the active_low, how can we describe  a pin that is
> active low and has to trigger an irq on both edges?

This is nonsense.
What does it mean?

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 14:46     ` Andy Shevchenko
@ 2020-10-29 14:54       ` Ricardo Ribalda
  2020-10-29 17:17         ` Andy Shevchenko
  0 siblings, 1 reply; 19+ messages in thread
From: Ricardo Ribalda @ 2020-10-29 14:54 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mika Westerberg, linux-acpi, Rafael J. Wysocki, linux-gpio

Hi Andy

On Thu, Oct 29, 2020 at 3:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:
> > Hi Andy
> >
> > Thanks for your patch and super fast response.
> >
> > I think there are two different concepts here:
> >
> > 1) when the pin has a low value, it is  0 or a 1? =>active_low
>
> I'm not sure I have got it. GpioIo() has no such property and
> _DSD active_low is documented as being polarity setting:
>
>         active_low == 1 => active low polarity.
>
> > 2) when do I get an irq, low->high or high->low => irq polarity
>
> IRQ polarity is clearly defined by GpioInt() resource in the ACPI
> specification.
>
> > When I read the acpi spec for GpioInt()
> > https://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf page
> > 934, it has the same problem as for GpioIo(), it does not express the
> > active_low and this is where the _DSD field comes handy.
>
> ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
> Resource Descriptor Macro).
>
> > Without using the active_low, how can we describe  a pin that is
> > active low and has to trigger an irq on both edges?
>
> This is nonsense.
> What does it mean?

Let me try to explain myself again:

I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both

The problem is that the value of that pin is inverted: Low means 1 and
high means 0.

How can I describe that the pin "is inverted" without using the _DSD field?

Best regards

>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Ricardo Ribalda

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 14:54       ` Ricardo Ribalda
@ 2020-10-29 17:17         ` Andy Shevchenko
  2020-10-29 17:20           ` Andy Shevchenko
  0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-29 17:17 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Andy Shevchenko, Mika Westerberg, ACPI Devel Maling List,
	Rafael J. Wysocki, open list:GPIO SUBSYSTEM

On Thu, Oct 29, 2020 at 4:55 PM Ricardo Ribalda <ribalda@google.com> wrote:
> On Thu, Oct 29, 2020 at 3:45 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:

...

> > ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
> > Resource Descriptor Macro).
> >
> > > Without using the active_low, how can we describe  a pin that is
> > > active low and has to trigger an irq on both edges?
> >
> > This is nonsense.
> > What does it mean?
>
> Let me try to explain myself again:
>
> I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both
>
> The problem is that the value of that pin is inverted: Low means 1 and
> high means 0.
>
> How can I describe that the pin "is inverted" without using the _DSD field?

"Both edges" and "inverted" or "polarity low" in one sentence make no sense.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 17:17         ` Andy Shevchenko
@ 2020-10-29 17:20           ` Andy Shevchenko
  2020-10-29 17:25             ` Andy Shevchenko
  0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-29 17:20 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Andy Shevchenko, Mika Westerberg, ACPI Devel Maling List,
	Rafael J. Wysocki, open list:GPIO SUBSYSTEM

On Thu, Oct 29, 2020 at 7:17 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Oct 29, 2020 at 4:55 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > On Thu, Oct 29, 2020 at 3:45 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:
>
> ...
>
> > > ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
> > > Resource Descriptor Macro).
> > >
> > > > Without using the active_low, how can we describe  a pin that is
> > > > active low and has to trigger an irq on both edges?
> > >
> > > This is nonsense.
> > > What does it mean?
> >
> > Let me try to explain myself again:
> >
> > I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both
> >
> > The problem is that the value of that pin is inverted: Low means 1 and
> > high means 0.
> >
> > How can I describe that the pin "is inverted" without using the _DSD field?
>
> "Both edges" and "inverted" or "polarity low" in one sentence make no sense.

To be on the constructive side, I can *imagine* so badly designed
hardware that uses level and edge at the same time, but before I go to
conclusions, can you share relevant (pieces of) datasheet?


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 17:20           ` Andy Shevchenko
@ 2020-10-29 17:25             ` Andy Shevchenko
  2020-10-29 17:32               ` Ricardo Ribalda
  0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-29 17:25 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Andy Shevchenko, Mika Westerberg, ACPI Devel Maling List,
	Rafael J. Wysocki, open list:GPIO SUBSYSTEM

On Thu, Oct 29, 2020 at 7:20 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 29, 2020 at 7:17 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Oct 29, 2020 at 4:55 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > > On Thu, Oct 29, 2020 at 3:45 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:
> >
> > ...
> >
> > > > ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
> > > > Resource Descriptor Macro).
> > > >
> > > > > Without using the active_low, how can we describe  a pin that is
> > > > > active low and has to trigger an irq on both edges?
> > > >
> > > > This is nonsense.
> > > > What does it mean?
> > >
> > > Let me try to explain myself again:
> > >
> > > I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both
> > >
> > > The problem is that the value of that pin is inverted: Low means 1 and
> > > high means 0.
> > >
> > > How can I describe that the pin "is inverted" without using the _DSD field?
> >
> > "Both edges" and "inverted" or "polarity low" in one sentence make no sense.
>
> To be on the constructive side, I can *imagine* so badly designed
> hardware that uses level and edge at the same time, but before I go to
> conclusions, can you share relevant (pieces of) datasheet?

The [1] is a real example of how GPIO is being used to detect changing
of current level of the signal.
Note, ACPI tables for that device have problems [2], but I guess you
may get the idea.


[1]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L138
[2]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L45

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 17:25             ` Andy Shevchenko
@ 2020-10-29 17:32               ` Ricardo Ribalda
  2020-10-29 18:09                 ` Ricardo Ribalda
  2020-10-29 18:10                 ` Andy Shevchenko
  0 siblings, 2 replies; 19+ messages in thread
From: Ricardo Ribalda @ 2020-10-29 17:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Mika Westerberg, ACPI Devel Maling List,
	Rafael J. Wysocki, open list:GPIO SUBSYSTEM

Hi Andy

On Thu, Oct 29, 2020 at 6:26 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 29, 2020 at 7:20 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Thu, Oct 29, 2020 at 7:17 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Oct 29, 2020 at 4:55 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > > > On Thu, Oct 29, 2020 at 3:45 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:
> > >
> > > ...
> > >
> > > > > ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
> > > > > Resource Descriptor Macro).
> > > > >
> > > > > > Without using the active_low, how can we describe  a pin that is
> > > > > > active low and has to trigger an irq on both edges?
> > > > >
> > > > > This is nonsense.
> > > > > What does it mean?
> > > >
> > > > Let me try to explain myself again:
> > > >
> > > > I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both
> > > >
> > > > The problem is that the value of that pin is inverted: Low means 1 and
> > > > high means 0.
> > > >
> > > > How can I describe that the pin "is inverted" without using the _DSD field?
> > >
> > > "Both edges" and "inverted" or "polarity low" in one sentence make no sense.
> >
> > To be on the constructive side, I can *imagine* so badly designed
> > hardware that uses level and edge at the same time, but before I go to
> > conclusions, can you share relevant (pieces of) datasheet?
>
> The [1] is a real example of how GPIO is being used to detect changing
> of current level of the signal.
> Note, ACPI tables for that device have problems [2], but I guess you
> may get the idea.


This is exactly what I need to do. Get an IRQ whenever the value
changes. But the pin is "inverted"

This is the "schematic" :  https://ibb.co/f8GMBbP . I want to pass to
userspace a "1" when the switch is closed and "0"  when it is open.



>
>
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L138
> [2]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L45
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
Ricardo Ribalda

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 17:32               ` Ricardo Ribalda
@ 2020-10-29 18:09                 ` Ricardo Ribalda
  2020-10-29 18:13                   ` Andy Shevchenko
  2020-10-29 18:10                 ` Andy Shevchenko
  1 sibling, 1 reply; 19+ messages in thread
From: Ricardo Ribalda @ 2020-10-29 18:09 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Mika Westerberg, ACPI Devel Maling List,
	Rafael J. Wysocki, open list:GPIO SUBSYSTEM, Tomasz Figa

(clicked on reply instead of reply all sorry)

On Thu, Oct 29, 2020 at 6:32 PM Ricardo Ribalda <ribalda@google.com> wrote:
>
> Hi Andy
>
> On Thu, Oct 29, 2020 at 6:26 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Thu, Oct 29, 2020 at 7:20 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Thu, Oct 29, 2020 at 7:17 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Oct 29, 2020 at 4:55 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > > > > On Thu, Oct 29, 2020 at 3:45 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:
> > > >
> > > > ...
> > > >
> > > > > > ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
> > > > > > Resource Descriptor Macro).
> > > > > >
> > > > > > > Without using the active_low, how can we describe  a pin that is
> > > > > > > active low and has to trigger an irq on both edges?
> > > > > >
> > > > > > This is nonsense.
> > > > > > What does it mean?
> > > > >
> > > > > Let me try to explain myself again:
> > > > >
> > > > > I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both
> > > > >
> > > > > The problem is that the value of that pin is inverted: Low means 1 and
> > > > > high means 0.
> > > > >
> > > > > How can I describe that the pin "is inverted" without using the _DSD field?
> > > >
> > > > "Both edges" and "inverted" or "polarity low" in one sentence make no sense.
> > >
> > > To be on the constructive side, I can *imagine* so badly designed
> > > hardware that uses level and edge at the same time, but before I go to
> > > conclusions, can you share relevant (pieces of) datasheet?
> >
> > The [1] is a real example of how GPIO is being used to detect changing
> > of current level of the signal.
> > Note, ACPI tables for that device have problems [2], but I guess you
> > may get the idea.
>
>
> This is exactly what I need to do. Get an IRQ whenever the value
> changes. But the pin is "inverted"
>
> This is the "schematic" :  https://ibb.co/f8GMBbP . I want to pass to
> userspace a "1" when the switch is closed and "0"  when it is open.
>
And there are also other devices where the swith works the other way
around, so the acpi should be verbose enough to describe both
situations.

With my proposal (use the same active_low field as with GpioIO) we
cover both usecases.

>
>
> >
> >
> > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L138
> > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L45
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 17:32               ` Ricardo Ribalda
  2020-10-29 18:09                 ` Ricardo Ribalda
@ 2020-10-29 18:10                 ` Andy Shevchenko
  1 sibling, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-29 18:10 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Andy Shevchenko, Mika Westerberg, ACPI Devel Maling List,
	Rafael J. Wysocki, open list:GPIO SUBSYSTEM

On Thu, Oct 29, 2020 at 7:32 PM Ricardo Ribalda <ribalda@google.com> wrote:
> On Thu, Oct 29, 2020 at 6:26 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Oct 29, 2020 at 7:20 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Oct 29, 2020 at 7:17 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Oct 29, 2020 at 4:55 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > > > > On Thu, Oct 29, 2020 at 3:45 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:
> > > >
> > > > ...
> > > >
> > > > > > ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
> > > > > > Resource Descriptor Macro).
> > > > > >
> > > > > > > Without using the active_low, how can we describe  a pin that is
> > > > > > > active low and has to trigger an irq on both edges?
> > > > > >
> > > > > > This is nonsense.
> > > > > > What does it mean?
> > > > >
> > > > > Let me try to explain myself again:
> > > > >
> > > > > I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both
> > > > >
> > > > > The problem is that the value of that pin is inverted: Low means 1 and
> > > > > high means 0.
> > > > >
> > > > > How can I describe that the pin "is inverted" without using the _DSD field?
> > > >
> > > > "Both edges" and "inverted" or "polarity low" in one sentence make no sense.
> > >
> > > To be on the constructive side, I can *imagine* so badly designed
> > > hardware that uses level and edge at the same time, but before I go to
> > > conclusions, can you share relevant (pieces of) datasheet?
> >
> > The [1] is a real example of how GPIO is being used to detect changing
> > of current level of the signal.
> > Note, ACPI tables for that device have problems [2], but I guess you
> > may get the idea.
>
>
> This is exactly what I need to do. Get an IRQ whenever the value
> changes. But the pin is "inverted"

Yes. It's fine. The above is using GpioIo() resource where polarity is
part of the _DSD (okay, the actual ACPI table doesn't have it, but you
can do it).

> This is the "schematic" :  https://ibb.co/f8GMBbP . I want to pass to
> userspace a "1" when the switch is closed and "0"  when it is open.

> > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L138
> > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L45

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 18:09                 ` Ricardo Ribalda
@ 2020-10-29 18:13                   ` Andy Shevchenko
  2020-10-29 18:58                     ` Ricardo Ribalda
  0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-29 18:13 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Andy Shevchenko, Mika Westerberg, ACPI Devel Maling List,
	Rafael J. Wysocki, open list:GPIO SUBSYSTEM, Tomasz Figa

On Thu, Oct 29, 2020 at 8:10 PM Ricardo Ribalda <ribalda@google.com> wrote:
> On Thu, Oct 29, 2020 at 6:32 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > On Thu, Oct 29, 2020 at 6:26 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Oct 29, 2020 at 7:20 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Oct 29, 2020 at 7:17 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Thu, Oct 29, 2020 at 4:55 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > > > > > On Thu, Oct 29, 2020 at 3:45 PM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
> > > > > > > Resource Descriptor Macro).
> > > > > > >
> > > > > > > > Without using the active_low, how can we describe  a pin that is
> > > > > > > > active low and has to trigger an irq on both edges?
> > > > > > >
> > > > > > > This is nonsense.
> > > > > > > What does it mean?
> > > > > >
> > > > > > Let me try to explain myself again:
> > > > > >
> > > > > > I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both
> > > > > >
> > > > > > The problem is that the value of that pin is inverted: Low means 1 and
> > > > > > high means 0.
> > > > > >
> > > > > > How can I describe that the pin "is inverted" without using the _DSD field?
> > > > >
> > > > > "Both edges" and "inverted" or "polarity low" in one sentence make no sense.
> > > >
> > > > To be on the constructive side, I can *imagine* so badly designed
> > > > hardware that uses level and edge at the same time, but before I go to
> > > > conclusions, can you share relevant (pieces of) datasheet?
> > >
> > > The [1] is a real example of how GPIO is being used to detect changing
> > > of current level of the signal.
> > > Note, ACPI tables for that device have problems [2], but I guess you
> > > may get the idea.
> >
> >
> > This is exactly what I need to do. Get an IRQ whenever the value
> > changes. But the pin is "inverted"
> >
> > This is the "schematic" :  https://ibb.co/f8GMBbP . I want to pass to
> > userspace a "1" when the switch is closed and "0"  when it is open.
> >
> And there are also other devices where the swith works the other way
> around, so the acpi should be verbose enough to describe both
> situations.
>
> With my proposal (use the same active_low field as with GpioIO) we
> cover both usecases.

Even without your proposal it's feasible.
You see, the problem here is that if you describe GPIO as Interrupt,
the edge and level together make complete nonsense.

Solution: do *not* describe it as Interrupt.

> > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L138
> > > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L45


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 18:13                   ` Andy Shevchenko
@ 2020-10-29 18:58                     ` Ricardo Ribalda
  2020-10-29 19:11                       ` Andy Shevchenko
  0 siblings, 1 reply; 19+ messages in thread
From: Ricardo Ribalda @ 2020-10-29 18:58 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Mika Westerberg, ACPI Devel Maling List,
	Rafael J. Wysocki, open list:GPIO SUBSYSTEM, Tomasz Figa

On Thu, Oct 29, 2020 at 7:13 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 29, 2020 at 8:10 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > On Thu, Oct 29, 2020 at 6:32 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > > On Thu, Oct 29, 2020 at 6:26 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Oct 29, 2020 at 7:20 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Thu, Oct 29, 2020 at 7:17 PM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Thu, Oct 29, 2020 at 4:55 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > > > > > > On Thu, Oct 29, 2020 at 3:45 PM Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > On Wed, Oct 28, 2020 at 10:10:42PM +0100, Ricardo Ribalda wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > > ActiveLevel field is described in 19.6.55 GpioInt (GPIO Interrupt Connection
> > > > > > > > Resource Descriptor Macro).
> > > > > > > >
> > > > > > > > > Without using the active_low, how can we describe  a pin that is
> > > > > > > > > active low and has to trigger an irq on both edges?
> > > > > > > >
> > > > > > > > This is nonsense.
> > > > > > > > What does it mean?
> > > > > > >
> > > > > > > Let me try to explain myself again:
> > > > > > >
> > > > > > > I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both
> > > > > > >
> > > > > > > The problem is that the value of that pin is inverted: Low means 1 and
> > > > > > > high means 0.
> > > > > > >
> > > > > > > How can I describe that the pin "is inverted" without using the _DSD field?
> > > > > >
> > > > > > "Both edges" and "inverted" or "polarity low" in one sentence make no sense.
> > > > >
> > > > > To be on the constructive side, I can *imagine* so badly designed
> > > > > hardware that uses level and edge at the same time, but before I go to
> > > > > conclusions, can you share relevant (pieces of) datasheet?
> > > >
> > > > The [1] is a real example of how GPIO is being used to detect changing
> > > > of current level of the signal.
> > > > Note, ACPI tables for that device have problems [2], but I guess you
> > > > may get the idea.
> > >
> > >
> > > This is exactly what I need to do. Get an IRQ whenever the value
> > > changes. But the pin is "inverted"
> > >
> > > This is the "schematic" :  https://ibb.co/f8GMBbP . I want to pass to
> > > userspace a "1" when the switch is closed and "0"  when it is open.
> > >
> > And there are also other devices where the swith works the other way
> > around, so the acpi should be verbose enough to describe both
> > situations.
> >
> > With my proposal (use the same active_low field as with GpioIO) we
> > cover both usecases.
>
> Even without your proposal it's feasible.
> You see, the problem here is that if you describe GPIO as Interrupt,
> the edge and level together make complete nonsense.
>
> Solution: do *not* describe it as Interrupt.

Now I get my mistake:

I thought that gpiod_to_irq will not work unless it was a GpioInt()
but it works fine. So in this case I will just convert it to that.

Could we say that doing gpiod_get_value() from a GpioInt() is always
wrong? Can we modify the code to avoid it?

Sorry for the confusion and thanks for your help.


>
> > > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L138
> > > > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L45
>
>
> --
> With Best Regards,
> Andy Shevchenko



--
Ricardo Ribalda

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

* Re: [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo()
  2020-10-29 18:58                     ` Ricardo Ribalda
@ 2020-10-29 19:11                       ` Andy Shevchenko
  0 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2020-10-29 19:11 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Mika Westerberg, ACPI Devel Maling List, Rafael J. Wysocki,
	open list:GPIO SUBSYSTEM, Tomasz Figa

On Thu, Oct 29, 2020 at 07:58:39PM +0100, Ricardo Ribalda wrote:
> On Thu, Oct 29, 2020 at 7:13 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Oct 29, 2020 at 8:10 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > > On Thu, Oct 29, 2020 at 6:32 PM Ricardo Ribalda <ribalda@google.com> wrote:
> > > > On Thu, Oct 29, 2020 at 6:26 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Thu, Oct 29, 2020 at 7:20 PM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Thu, Oct 29, 2020 at 7:17 PM Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com> wrote:

...

> > > > > > > > Let me try to explain myself again:
> > > > > > > >
> > > > > > > > I have a gpio pin that produces IRQs on both edges. so ActiveLevel is Both
> > > > > > > >
> > > > > > > > The problem is that the value of that pin is inverted: Low means 1 and
> > > > > > > > high means 0.
> > > > > > > >
> > > > > > > > How can I describe that the pin "is inverted" without using the _DSD field?
> > > > > > >
> > > > > > > "Both edges" and "inverted" or "polarity low" in one sentence make no sense.
> > > > > >
> > > > > > To be on the constructive side, I can *imagine* so badly designed
> > > > > > hardware that uses level and edge at the same time, but before I go to
> > > > > > conclusions, can you share relevant (pieces of) datasheet?
> > > > >
> > > > > The [1] is a real example of how GPIO is being used to detect changing
> > > > > of current level of the signal.
> > > > > Note, ACPI tables for that device have problems [2], but I guess you
> > > > > may get the idea.
> > > >
> > > >
> > > > This is exactly what I need to do. Get an IRQ whenever the value
> > > > changes. But the pin is "inverted"
> > > >
> > > > This is the "schematic" :  https://ibb.co/f8GMBbP . I want to pass to
> > > > userspace a "1" when the switch is closed and "0"  when it is open.
> > > >
> > > And there are also other devices where the swith works the other way
> > > around, so the acpi should be verbose enough to describe both
> > > situations.
> > >
> > > With my proposal (use the same active_low field as with GpioIO) we
> > > cover both usecases.
> >
> > Even without your proposal it's feasible.
> > You see, the problem here is that if you describe GPIO as Interrupt,
> > the edge and level together make complete nonsense.
> >
> > Solution: do *not* describe it as Interrupt.
> 
> Now I get my mistake:
> 
> I thought that gpiod_to_irq will not work unless it was a GpioInt()
> but it works fine. So in this case I will just convert it to that.

It's actually that gpio_to_irq() is solely for GPIOs which initially are not IRQs.

> Could we say that doing gpiod_get_value() from a GpioInt() is always
> wrong?

But it's not wrong. Some cases simply make little or no sense, but in principal
why not? Yes, it may be fragile or too much customized.

> Can we modify the code to avoid it?

GpioInt() is orthogonal to GPIO APIs in Linux kernel. It close to be
impossible. Also see above.

> Sorry for the confusion and thanks for your help.

No problem, you're welcome, it's good that you started a discussion!

> > > > > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L138
> > > > > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/extcon/extcon-intel-int3496.c#L45

-- 
With Best Regards,
Andy Shevchenko



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

end of thread, other threads:[~2020-10-29 19:10 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-28 20:50 [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes Andy Shevchenko
2020-10-28 20:51 ` [PATCH v1 2/3] Documentation: firmware-guide: gpio-properties: active_low only for GpioIo() Andy Shevchenko
2020-10-28 21:10   ` Ricardo Ribalda
2020-10-29 14:46     ` Andy Shevchenko
2020-10-29 14:54       ` Ricardo Ribalda
2020-10-29 17:17         ` Andy Shevchenko
2020-10-29 17:20           ` Andy Shevchenko
2020-10-29 17:25             ` Andy Shevchenko
2020-10-29 17:32               ` Ricardo Ribalda
2020-10-29 18:09                 ` Ricardo Ribalda
2020-10-29 18:13                   ` Andy Shevchenko
2020-10-29 18:58                     ` Ricardo Ribalda
2020-10-29 19:11                       ` Andy Shevchenko
2020-10-29 18:10                 ` Andy Shevchenko
2020-10-29  8:13   ` Mika Westerberg
2020-10-28 20:51 ` [PATCH v1 3/3] Documentation: firmware-guide: gpio-properties: Clarify initial output state Andy Shevchenko
2020-10-29  8:16   ` Mika Westerberg
2020-10-29  8:13 ` [PATCH v1 1/3] Documentation: firmware-guide: gpio-properties: Fix factual mistakes Mika Westerberg
2020-10-29 11:10   ` 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).