Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1607842 > unrolled thread

[PATCH v1 0/8] gpio: acpi: Make it working

Back to article view | Back to linux.kernel

Contents

  [PATCH v1 0/8] gpio: acpi: Make it working Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-23 20:50 +0100
    [PATCH v1 3/8] gpio: acpi: Do sanity check for GpioInt in acpi_find_gpio() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-23 20:50 +0100
      Re: [PATCH v1 3/8] gpio: acpi: Do sanity check for GpioInt in  acpi_find_gpio() Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-03-23 21:30 +0100
    [PATCH v1 4/8] gpio: acpi: Even more tighten up ACPI GPIO lookups Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-23 20:50 +0100
      Re: [PATCH v1 4/8] gpio: acpi: Even more tighten up ACPI GPIO lookups Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-03-23 21:20 +0100
        Re: [PATCH v1 4/8] gpio: acpi: Even more tighten up ACPI GPIO  lookups Bastien Nocera <hadess@hadess.net> - 2017-03-24 11:50 +0100
    [PATCH v1 2/8] gpio: acpi: Align acpi_find_gpio() with DT version Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-23 20:50 +0100
      Re: [PATCH v1 2/8] gpio: acpi: Align acpi_find_gpio() with DT version Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-03-23 21:20 +0100
    [PATCH v1 6/8] gpio: acpi: Explain how to get GPIO descriptors in ACPI case Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-23 20:50 +0100
      Re: [PATCH v1 6/8] gpio: acpi: Explain how to get GPIO descriptors  in ACPI case Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-03-23 21:30 +0100
    [PATCH v1 5/8] gpio: acpi: Synchronize acpi_find_gpio() and acpi_gpio_count() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-23 20:50 +0100
      Re: [PATCH v1 5/8] gpio: acpi: Synchronize acpi_find_gpio() and  acpi_gpio_count() Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-03-24 17:00 +0100
    [PATCH v1 8/8] gpio: acpi: Override GPIO initialization flags Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-23 20:50 +0100
    Re: [PATCH v1 0/8] gpio: acpi: Make it working Jarkko Nikula <jarkko.nikula@linux.intel.com> - 2017-03-24 15:00 +0100
    Re: [PATCH v1 0/8] gpio: acpi: Make it working Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-03-24 17:10 +0100

#1607842 — [PATCH v1 0/8] gpio: acpi: Make it working

Currently GPIO ACPI library provides an API to get a GPIO resources
(IO or interrupt) from ACPI tables for the individual drivers.

This library has few flaws which makes some devices not working:
 - the library allows to abuse ACPI by using a _CRS fallback mechanism
 - the library neglects flags of the resource

In this series:
 - the _CRS fallback is forbidden
 - the pin configuration follows what firmware wants to
 - the documentation is updated in order to clarify corner cases

After this series it's possible to use GPIO pins for input (interrupt)
which were configured as output by BIOS by some reason. It's a crucial
functionality for IoT open connected boards where user may choose any of
available pin for almost any of available function, including GPIO input
(interrupt).

Current bad behaviour was first reported by Jarkko Nikula few months ago.

Andy Shevchenko (8):
  gpiolib: Export gpiod_configure_flags() to internal users
  gpio: acpi: Align acpi_find_gpio() with DT version
  gpio: acpi: Do sanity check for GpioInt in acpi_find_gpio()
  gpio: acpi: Even more tighten up ACPI GPIO lookups
  gpio: acpi: Synchronize acpi_find_gpio() and acpi_gpio_count()
  gpio: acpi: Explain how to get GPIO descriptors in ACPI case
  gpio: acpi: Factor out acpi_gpio_to_gpiod_flags() helper
  gpio: acpi: Override GPIO initialization flags

 Documentation/acpi/gpio-properties.txt |  60 ++++++++++++
 drivers/gpio/gpiolib-acpi.c            | 162 +++++++++++++++++++--------------
 drivers/gpio/gpiolib.c                 |  10 +-
 drivers/gpio/gpiolib.h                 |  17 +++-
 4 files changed, 176 insertions(+), 73 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1607843 — [PATCH v1 3/8] gpio: acpi: Do sanity check for GpioInt in acpi_find_gpio()

Check that we don't ask for output direction on GpioInt resource in cases with
or without _DSD defined.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 3bda3166d418..21e4930ca2db 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -614,12 +614,12 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
 		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
 		if (IS_ERR(desc))
 			return desc;
+	}
 
-		if ((flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH) &&
-		    info.gpioint) {
-			dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
-			return ERR_PTR(-ENOENT);
-		}
+	if (info.gpioint &&
+	    (flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH)) {
+		dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
+		return ERR_PTR(-ENOENT);
 	}
 
 	if (info.polarity == GPIO_ACTIVE_LOW)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1607876 — Re: [PATCH v1 3/8] gpio: acpi: Do sanity check for GpioInt in acpi_find_gpio()

On Thu, Mar 23, 2017 at 09:46:13PM +0200, Andy Shevchenko wrote:
> Check that we don't ask for output direction on GpioInt resource in cases with
> or without _DSD defined.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/gpio/gpiolib-acpi.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 3bda3166d418..21e4930ca2db 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -614,12 +614,12 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
>  		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
>  		if (IS_ERR(desc))
>  			return desc;
> +	}
>  
> -		if ((flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH) &&
> -		    info.gpioint) {
> -			dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
> -			return ERR_PTR(-ENOENT);
> -		}
> +	if (info.gpioint &&
> +	    (flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH)) {
> +		dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
> +		return ERR_PTR(-ENOENT);
>  	}
>  
>  	if (info.polarity == GPIO_ACTIVE_LOW)
> -- 
> 2.11.0
> 

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1607844 — [PATCH v1 4/8] gpio: acpi: Even more tighten up ACPI GPIO lookups

The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio lookups")
prevents to getting same resource twice if the driver asks twice using same
connection ID.

But the whole idea of fallback might bring some problems. Imagine the case when
we have two versions of BIOS/hardware where in one _DSD is introduced along
with GPIO resources, but the other one uses just plain GPIO resource for
another purpose

Case 1:

    Device (DEVX)
    {
        ...
        Name (_CRS, ResourceTemplate ()
        {
            GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
                    "\\_SB.GPO0", 0, ResourceConsumer) {15}
        })
        Name (_DSD, Package ()
        {
            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
            Package ()
            {
                Package () {"some-gpios", Package() {^DEVX, 0, 0, 0 }},
            }
        })
    }

Case 2:

    Device (DEVX)
    {
        ...
        Name (_CRS, ResourceTemplate ()
        {
            GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
                    "\\_SB.GPO0", 0, ResourceConsumer) {27}
        })
    }

To prevent the possible misconfiguration tighten up even more ACPI GPIO lookups
for case without connection ID provided.

In the past the issue had been triggered by "use mctrl_gpio helpers" series
[1,2].

Besides above, removal of the main logic of acpi_can_fallback_to_crs()
eliminates a potential memory leak when the same device has been unbound and
bound again.

[1] commit 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] https://patchwork.kernel.org/patch/9283745/

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 36 +-----------------------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 21e4930ca2db..e516b7a0cc50 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1121,45 +1121,11 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
 	return count ? count : -ENOENT;
 }
 
-struct acpi_crs_lookup {
-	struct list_head node;
-	struct acpi_device *adev;
-	const char *con_id;
-};
-
-static DEFINE_MUTEX(acpi_crs_lookup_lock);
-static LIST_HEAD(acpi_crs_lookup_list);
-
 bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
 {
-	struct acpi_crs_lookup *l, *lookup = NULL;
-
 	/* Never allow fallback if the device has properties */
 	if (adev->data.properties || adev->driver_gpios)
 		return false;
 
-	mutex_lock(&acpi_crs_lookup_lock);
-
-	list_for_each_entry(l, &acpi_crs_lookup_list, node) {
-		if (l->adev == adev) {
-			lookup = l;
-			break;
-		}
-	}
-
-	if (!lookup) {
-		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
-		if (lookup) {
-			lookup->adev = adev;
-			lookup->con_id = kstrdup(con_id, GFP_KERNEL);
-			list_add_tail(&lookup->node, &acpi_crs_lookup_list);
-		}
-	}
-
-	mutex_unlock(&acpi_crs_lookup_lock);
-
-	return lookup &&
-		((!lookup->con_id && !con_id) ||
-		 (lookup->con_id && con_id &&
-		  strcmp(lookup->con_id, con_id) == 0));
+	return con_id == NULL;
 }
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1607869 — Re: [PATCH v1 4/8] gpio: acpi: Even more tighten up ACPI GPIO lookups

On Thu, Mar 23, 2017 at 09:46:14PM +0200, Andy Shevchenko wrote:
> The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio lookups")
> prevents to getting same resource twice if the driver asks twice using same

s/same/different/

> connection ID.
> 
> But the whole idea of fallback might bring some problems. Imagine the case when
> we have two versions of BIOS/hardware where in one _DSD is introduced along
> with GPIO resources, but the other one uses just plain GPIO resource for
> another purpose
> 
> Case 1:
> 
>     Device (DEVX)
>     {
>         ...
>         Name (_CRS, ResourceTemplate ()
>         {
>             GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
>                     "\\_SB.GPO0", 0, ResourceConsumer) {15}
>         })
>         Name (_DSD, Package ()
>         {
>             ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>             Package ()
>             {
>                 Package () {"some-gpios", Package() {^DEVX, 0, 0, 0 }},
>             }
>         })
>     }
> 
> Case 2:
> 
>     Device (DEVX)
>     {
>         ...
>         Name (_CRS, ResourceTemplate ()
>         {
>             GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
>                     "\\_SB.GPO0", 0, ResourceConsumer) {27}
>         })
>     }
> 
> To prevent the possible misconfiguration tighten up even more ACPI GPIO lookups
> for case without connection ID provided.

I wonder if this will break Goodix. Irina, Bastien?

> 
> In the past the issue had been triggered by "use mctrl_gpio helpers" series
> [1,2].
> 
> Besides above, removal of the main logic of acpi_can_fallback_to_crs()
> eliminates a potential memory leak when the same device has been unbound and
> bound again.

Where? We'll reuse lookup table as ACPI device is still the same.

> 
> [1] commit 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
> [2] https://patchwork.kernel.org/patch/9283745/
> 
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 36 +-----------------------------------
>  1 file changed, 1 insertion(+), 35 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 21e4930ca2db..e516b7a0cc50 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1121,45 +1121,11 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
>  	return count ? count : -ENOENT;
>  }
>  
> -struct acpi_crs_lookup {
> -	struct list_head node;
> -	struct acpi_device *adev;
> -	const char *con_id;
> -};
> -
> -static DEFINE_MUTEX(acpi_crs_lookup_lock);
> -static LIST_HEAD(acpi_crs_lookup_list);
> -
>  bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
>  {
> -	struct acpi_crs_lookup *l, *lookup = NULL;
> -
>  	/* Never allow fallback if the device has properties */
>  	if (adev->data.properties || adev->driver_gpios)
>  		return false;
>  
> -	mutex_lock(&acpi_crs_lookup_lock);
> -
> -	list_for_each_entry(l, &acpi_crs_lookup_list, node) {
> -		if (l->adev == adev) {
> -			lookup = l;
> -			break;
> -		}
> -	}
> -
> -	if (!lookup) {
> -		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
> -		if (lookup) {
> -			lookup->adev = adev;
> -			lookup->con_id = kstrdup(con_id, GFP_KERNEL);
> -			list_add_tail(&lookup->node, &acpi_crs_lookup_list);
> -		}
> -	}
> -
> -	mutex_unlock(&acpi_crs_lookup_lock);
> -
> -	return lookup &&
> -		((!lookup->con_id && !con_id) ||
> -		 (lookup->con_id && con_id &&
> -		  strcmp(lookup->con_id, con_id) == 0));
> +	return con_id == NULL;
>  }
> -- 
> 2.11.0
> 

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1608305 — Re: [PATCH v1 4/8] gpio: acpi: Even more tighten up ACPI GPIO lookups

On Thu, 2017-03-23 at 13:12 -0700, Dmitry Torokhov wrote:
> 
<snip>
> > To prevent the possible misconfiguration tighten up even more ACPI
> > GPIO lookups
> > for case without connection ID provided.
> 
> I wonder if this will break Goodix. Irina, Bastien?

My Goodix tablet has been on the fritz for nearly a year. Hopefully,
32-bit-UEFI support will be available for my preferred distribution
soon, and I'd be able to tell you.

[toc] | [prev] | [next] | [standalone]

#1607845 — [PATCH v1 2/8] gpio: acpi: Align acpi_find_gpio() with DT version

By some reason acpi_find_gpio() and acpi_gpio_count() have compared connection
ID to "gpios" when tries to check if suffix is needed or not.

Don't do any assumptions about what connection ID can be and, when defined, use
it only with suffix as it's done in the device tree version.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 56a6b1be3a17..3bda3166d418 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -593,7 +593,7 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
 
 	/* Try first from _DSD */
 	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
-		if (con_id && strcmp(con_id, "gpios")) {
+		if (con_id) {
 			snprintf(propname, sizeof(propname), "%s-%s",
 				 con_id, gpio_suffixes[i]);
 		} else {
@@ -1081,7 +1081,7 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
 
 	/* Try first from _DSD */
 	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
-		if (con_id && strcmp(con_id, "gpios"))
+		if (con_id)
 			snprintf(propname, sizeof(propname), "%s-%s",
 				 con_id, gpio_suffixes[i]);
 		else
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1607866 — Re: [PATCH v1 2/8] gpio: acpi: Align acpi_find_gpio() with DT version

On Thu, Mar 23, 2017 at 09:46:12PM +0200, Andy Shevchenko wrote:
> By some reason acpi_find_gpio() and acpi_gpio_count() have compared connection
> ID to "gpios" when tries to check if suffix is needed or not.
> 
> Don't do any assumptions about what connection ID can be and, when defined, use
> it only with suffix as it's done in the device tree version.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/gpio/gpiolib-acpi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 56a6b1be3a17..3bda3166d418 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -593,7 +593,7 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
>  
>  	/* Try first from _DSD */
>  	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> -		if (con_id && strcmp(con_id, "gpios")) {
> +		if (con_id) {
>  			snprintf(propname, sizeof(propname), "%s-%s",
>  				 con_id, gpio_suffixes[i]);
>  		} else {
> @@ -1081,7 +1081,7 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
>  
>  	/* Try first from _DSD */
>  	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> -		if (con_id && strcmp(con_id, "gpios"))
> +		if (con_id)
>  			snprintf(propname, sizeof(propname), "%s-%s",
>  				 con_id, gpio_suffixes[i]);
>  		else
> -- 
> 2.11.0
> 

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1607847 — [PATCH v1 6/8] gpio: acpi: Explain how to get GPIO descriptors in ACPI case

Documentation lacks of explanation how we actually use device properties
for GPIO resources.

Add a section to the documentation about that.

Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/acpi/gpio-properties.txt | 60 ++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/Documentation/acpi/gpio-properties.txt b/Documentation/acpi/gpio-properties.txt
index 2aff0349facd..07954b7c3a12 100644
--- a/Documentation/acpi/gpio-properties.txt
+++ b/Documentation/acpi/gpio-properties.txt
@@ -156,3 +156,63 @@ 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.
+
+Using the _CRS fallback
+-----------------------
+
+If a device does not have _DSD or the driver does not create ACPI GPIO
+mapping, the Linux GPIO framework refuses to return any GPIOs. This is
+because the driver does not know what it actually gets. For example if we
+have a device like below:
+
+  Device (BTH)
+  {
+      Name (_HID, ...)
+
+      Name (_CRS, ResourceTemplate () {
+          GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
+                  "\\_SB.GPO0", 0, ResourceConsumer) {15}
+          GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
+                  "\\_SB.GPO0", 0, ResourceConsumer) {27}
+      })
+  }
+
+The driver might expect to get the right GPIO when it does:
+
+  desc = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+
+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).
+
+Getting GPIO descriptor
+-----------------------
+
+There are two main approaches to get GPIO resource from ACPI:
+	desc = gpiod_get(dev, connection_id, flags);
+	desc = gpiod_get_index(dev, connection_id, index, flags);
+
+We may consider two different cases here, i.e. when connection ID is
+provided and otherwise.
+
+Case 1:
+	desc = gpiod_get(dev, "non-null-connection-id", flags);
+	desc = gpiod_get_index(dev, "non-null-connection-id", index, flags);
+
+Case 2:
+	desc = gpiod_get(dev, NULL, flags);
+	desc = gpiod_get_index(dev, NULL, index, flags);
+
+Case 1 assumes that corresponding ACPI device description must have
+defined device properties and will prevent to getting any GPIO resources
+otherwise.
+
+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
+chapter.
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1607872 — Re: [PATCH v1 6/8] gpio: acpi: Explain how to get GPIO descriptors in ACPI case

On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko wrote:
> Documentation lacks of explanation how we actually use device properties
> for GPIO resources.
> 
> Add a section to the documentation about that.
> 
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/acpi/gpio-properties.txt | 60 ++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/Documentation/acpi/gpio-properties.txt b/Documentation/acpi/gpio-properties.txt
> index 2aff0349facd..07954b7c3a12 100644
> --- a/Documentation/acpi/gpio-properties.txt
> +++ b/Documentation/acpi/gpio-properties.txt
> @@ -156,3 +156,63 @@ 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.
> +
> +Using the _CRS fallback
> +-----------------------
> +
> +If a device does not have _DSD or the driver does not create ACPI GPIO
> +mapping, the Linux GPIO framework refuses to return any GPIOs. This is
> +because the driver does not know what it actually gets. For example if we
> +have a device like below:
> +
> +  Device (BTH)
> +  {
> +      Name (_HID, ...)
> +
> +      Name (_CRS, ResourceTemplate () {
> +          GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
> +                  "\\_SB.GPO0", 0, ResourceConsumer) {15}
> +          GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
> +                  "\\_SB.GPO0", 0, ResourceConsumer) {27}
> +      })
> +  }
> +
> +The driver might expect to get the right GPIO when it does:
> +
> +  desc = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +
> +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).

If the driver is not platform specific, then it would have no idea about
mapping between _CRS GPIOs and names. All such stuff should be hidden in
platform glue (i.e drivers/platform/x86/platform_crap.c).

> +
> +Getting GPIO descriptor
> +-----------------------
> +
> +There are two main approaches to get GPIO resource from ACPI:
> +	desc = gpiod_get(dev, connection_id, flags);
> +	desc = gpiod_get_index(dev, connection_id, index, flags);
> +
> +We may consider two different cases here, i.e. when connection ID is
> +provided and otherwise.
> +
> +Case 1:
> +	desc = gpiod_get(dev, "non-null-connection-id", flags);
> +	desc = gpiod_get_index(dev, "non-null-connection-id", index, flags);
> +
> +Case 2:
> +	desc = gpiod_get(dev, NULL, flags);
> +	desc = gpiod_get_index(dev, NULL, index, flags);
> +
> +Case 1 assumes that corresponding ACPI device description must have
> +defined device properties and will prevent to getting any GPIO resources
> +otherwise.
> +
> +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
> +chapter.

I think that this wording is too x86-centric. We are talking about
consumers of GPIOs here (i.e. drivers), which need unified behavior
between ACPI, DT, and static board properties, they do not really care
about _CRS or _DSD.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1607848 — [PATCH v1 5/8] gpio: acpi: Synchronize acpi_find_gpio() and acpi_gpio_count()

If we pass connection ID to the both functions and at the same time
acpi_can_fallback_to_crs() returns false we will get different results, i.e.
the number of GPIO resourses returned by acpi_gpio_count() might be not
correct.

Fix this by calling acpi_can_fallback_to_crs() in acpi_gpio_count() before
trying to fallback.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e516b7a0cc50..61518273e360 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1111,6 +1111,9 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
 		struct list_head resource_list;
 		unsigned int crs_count = 0;
 
+		if (!acpi_can_fallback_to_crs(adev, con_id))
+			return count;
+
 		INIT_LIST_HEAD(&resource_list);
 		acpi_dev_get_resources(adev, &resource_list,
 				       acpi_find_gpio_count, &crs_count);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1608596 — Re: [PATCH v1 5/8] gpio: acpi: Synchronize acpi_find_gpio() and acpi_gpio_count()

On Thu, Mar 23, 2017 at 09:46:15PM +0200, Andy Shevchenko wrote:
> If we pass connection ID to the both functions and at the same time
> acpi_can_fallback_to_crs() returns false we will get different results, i.e.
> the number of GPIO resourses returned by acpi_gpio_count() might be not
                     ^^^^^^^^^
resources

[toc] | [prev] | [next] | [standalone]

#1607849 — [PATCH v1 8/8] gpio: acpi: Override GPIO initialization flags

This allows ACPI GPIO code to modify flags based on
ACPI GpioIo() / GpioInt() resources.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 50 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/gpio/gpiolib.c      |  8 ++++++--
 drivers/gpio/gpiolib.h      | 15 ++++++++++++--
 3 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d4de84670c5b..5506b0736aba 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -442,6 +442,34 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
 	}
 }
 
+int
+acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
+{
+	int ret = 0;
+
+	/*
+	 * Check if the BIOS has IoRestriction with explicitly set direction
+	 * and update @flags accordingly. Otherwise use whatever caller asked
+	 * for.
+	 */
+	if (update & GPIOD_FLAGS_BIT_DIR_SET) {
+		enum gpiod_flags diff = *flags ^ update;
+
+		/*
+		 * Check if caller supplied incompatible GPIO initialization
+		 * flags.
+		 *
+		 * Return %-EINVAL to notify that firmware has different
+		 * settings and we are going to use them.
+		 */
+		if (((*flags & GPIOD_FLAGS_BIT_DIR_SET) && (diff & GPIOD_FLAGS_BIT_DIR_OUT)) ||
+		    ((*flags & GPIOD_FLAGS_BIT_DIR_OUT) && (diff & GPIOD_FLAGS_BIT_DIR_VAL)))
+			ret = -EINVAL;
+		*flags = update;
+	}
+	return ret;
+}
+
 struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
 	int index;
@@ -479,8 +507,11 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		 * - ACPI_ACTIVE_HIGH == GPIO_ACTIVE_HIGH
 		 */
 		if (lookup->info.gpioint) {
+			lookup->info.flags = GPIOD_IN;
 			lookup->info.polarity = agpio->polarity;
 			lookup->info.triggering = agpio->triggering;
+		} else {
+			lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio);
 		}
 
 	}
@@ -607,13 +638,14 @@ static struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
 struct gpio_desc *acpi_find_gpio(struct device *dev,
 				 const char *con_id,
 				 unsigned int idx,
-				 enum gpiod_flags flags,
+				 enum gpiod_flags *dflags,
 				 enum gpio_lookup_flags *lookupflags)
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
 	struct acpi_gpio_info info;
 	struct gpio_desc *desc;
 	char propname[32];
+	int err;
 	int i;
 
 	/* Try first from _DSD */
@@ -642,7 +674,7 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
 	}
 
 	if (info.gpioint &&
-	    (flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH)) {
+	    (*dflags == GPIOD_OUT_LOW || *dflags == GPIOD_OUT_HIGH)) {
 		dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
 		return ERR_PTR(-ENOENT);
 	}
@@ -650,6 +682,10 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
 	if (info.polarity == GPIO_ACTIVE_LOW)
 		*lookupflags |= GPIO_ACTIVE_LOW;
 
+	err = acpi_gpio_update_gpiod_flags(dflags, info.flags);
+	if (err)
+		dev_dbg(dev, "Override GPIO initialization flags\n");
+
 	return desc;
 }
 
@@ -703,12 +739,16 @@ struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
  * used to translate from the GPIO offset in the resource to the Linux IRQ
  * number.
  *
+ * The function is idempotent, though each time it runs it will configure GPIO
+ * pin direction according to the flags in GpioInt resource.
+ *
  * Return: Linux IRQ number (>%0) on success, negative errno on failure.
  */
 int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 {
 	int idx, i;
 	unsigned int irq_flags;
+	int ret;
 
 	for (i = 0, idx = 0; idx <= index; i++) {
 		struct acpi_gpio_info info;
@@ -721,6 +761,7 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 			return PTR_ERR(desc);
 
 		if (info.gpioint && idx++ == index) {
+			char label[32];
 			int irq;
 
 			if (IS_ERR(desc))
@@ -730,6 +771,11 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 			if (irq < 0)
 				return irq;
 
+			snprintf(label, sizeof(label), "GpioInt() %d", index);
+			ret = gpiod_configure_flags(desc, label, 0, info.flags);
+			if (ret < 0)
+				return ret;
+
 			irq_flags = acpi_dev_get_irq_type(info.triggering,
 							  info.polarity);
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3752785d31ab..6110fd62277a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3272,7 +3272,7 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 			desc = of_find_gpio(dev, con_id, idx, &lookupflags);
 		} else if (ACPI_COMPANION(dev)) {
 			dev_dbg(dev, "using ACPI for GPIO lookup\n");
-			desc = acpi_find_gpio(dev, con_id, idx, flags, &lookupflags);
+			desc = acpi_find_gpio(dev, con_id, idx, &flags, &lookupflags);
 		}
 	}
 
@@ -3351,8 +3351,12 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 		struct acpi_gpio_info info;
 
 		desc = acpi_node_get_gpiod(fwnode, propname, index, &info);
-		if (!IS_ERR(desc))
+		if (!IS_ERR(desc)) {
 			active_low = info.polarity == GPIO_ACTIVE_LOW;
+			ret = acpi_gpio_update_gpiod_flags(&dflags, info.flags);
+			if (ret)
+				pr_debug("Override GPIO initialization flags\n");
+		}
 	}
 
 	if (IS_ERR(desc))
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index e36a0bdc7740..cff398cbb545 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -75,11 +75,13 @@ struct gpio_device {
 
 /**
  * struct acpi_gpio_info - ACPI GPIO specific information
+ * @flags: GPIO initialization flags
  * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
  * @polarity: interrupt polarity as provided by ACPI
  * @triggering: triggering type as provided by ACPI
  */
 struct acpi_gpio_info {
+	enum gpiod_flags flags;
 	bool gpioint;
 	int polarity;
 	int triggering;
@@ -121,10 +123,13 @@ void acpi_gpiochip_remove(struct gpio_chip *chip);
 void acpi_gpiochip_request_interrupts(struct gpio_chip *chip);
 void acpi_gpiochip_free_interrupts(struct gpio_chip *chip);
 
+int acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags,
+				 enum gpiod_flags update);
+
 struct gpio_desc *acpi_find_gpio(struct device *dev,
 				 const char *con_id,
 				 unsigned int idx,
-				 enum gpiod_flags flags,
+				 enum gpiod_flags *dflags,
 				 enum gpio_lookup_flags *lookupflags);
 struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
 				      const char *propname, int index,
@@ -143,9 +148,15 @@ acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { }
 static inline void
 acpi_gpiochip_free_interrupts(struct gpio_chip *chip) { }
 
+static inline int
+acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
+{
+	return 0;
+}
+
 static inline struct gpio_desc *
 acpi_find_gpio(struct device *dev, const char *con_id,
-	       unsigned int idx, enum gpiod_flags flags,
+	       unsigned int idx, enum gpiod_flags *dflags,
 	       enum gpio_lookup_flags *lookupflags)
 {
 	return ERR_PTR(-ENOENT);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1608406

On 03/23/17 21:46, Andy Shevchenko wrote:
> After this series it's possible to use GPIO pins for input (interrupt)
> which were configured as output by BIOS by some reason. It's a crucial
> functionality for IoT open connected boards where user may choose any of
> available pin for almost any of available function, including GPIO input
> (interrupt).
>
> Current bad behaviour was first reported by Jarkko Nikula few months ago.
>
It was actually due HW reset default is input buffer disabled and BIOS 
didn't touch that pin at all if I recall correctly.

For the set:
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[toc] | [prev] | [next] | [standalone]

#1608605

On Thu, Mar 23, 2017 at 09:46:10PM +0200, Andy Shevchenko wrote:
> Currently GPIO ACPI library provides an API to get a GPIO resources
> (IO or interrupt) from ACPI tables for the individual drivers.
> 
> This library has few flaws which makes some devices not working:
>  - the library allows to abuse ACPI by using a _CRS fallback mechanism
>  - the library neglects flags of the resource
> 
> In this series:
>  - the _CRS fallback is forbidden
>  - the pin configuration follows what firmware wants to
>  - the documentation is updated in order to clarify corner cases
> 
> After this series it's possible to use GPIO pins for input (interrupt)
> which were configured as output by BIOS by some reason. It's a crucial
> functionality for IoT open connected boards where user may choose any of
> available pin for almost any of available function, including GPIO input
> (interrupt).
> 
> Current bad behaviour was first reported by Jarkko Nikula few months ago.
> 
> Andy Shevchenko (8):
>   gpiolib: Export gpiod_configure_flags() to internal users
>   gpio: acpi: Align acpi_find_gpio() with DT version
>   gpio: acpi: Do sanity check for GpioInt in acpi_find_gpio()
>   gpio: acpi: Even more tighten up ACPI GPIO lookups
>   gpio: acpi: Synchronize acpi_find_gpio() and acpi_gpio_count()
>   gpio: acpi: Explain how to get GPIO descriptors in ACPI case
>   gpio: acpi: Factor out acpi_gpio_to_gpiod_flags() helper
>   gpio: acpi: Override GPIO initialization flags
> 
>  Documentation/acpi/gpio-properties.txt |  60 ++++++++++++
>  drivers/gpio/gpiolib-acpi.c            | 162 +++++++++++++++++++--------------
>  drivers/gpio/gpiolib.c                 |  10 +-
>  drivers/gpio/gpiolib.h                 |  17 +++-
>  4 files changed, 176 insertions(+), 73 deletions(-)

Really nice work!

For the whole series,

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web