[PATCH v2 0/4] ACPI / gpio: Updates to properties

[PATCH v2 0/4] ACPI / gpio: Updates to properties

[Mika Westerberg]

Hi,

This series brings couple of useful GPIO related properties from Device
Tree to ACPI _DSD device properties:

  - Names for GPIO lines
  - GPIO hogging
  - Holes in GPIO property lists

We are using these to get Intel Galileo better supported in the mainline
kernel (but these may be useful for other boards as well). For example SPI
chip select on Galileo is connected to a GPIO line so we need to be able to
describe it in ACPI, and at the same time allow native chip selects.

GPIO hogging can be used to set initial state of certain GPIOs available on
the headers regardless of the BIOS settings (which may be wrong as it knows
nothing about which devices have been connected).

The previous version can be found here:

  http://www.spinics.net/lists/linux-acpi/msg69385.html

Changes from v1:
  - Drop patch [1/5] as it has been applied already.
  - Move patch [4/5] to be the first.
  - Rename acpi_data_get_property_reference() to __acpi_node_get_property_reference().
  - Drop acpi_node_get_property_reference() as it is not necessary anymore.
  - Add static inline wrapper acpi_node_get_property_reference() that
    calls the previous passing MAX_ACPI_REFERENCE_ARGS to support existing
    drivers.

Mika Westerberg (4):
  ACPI / property: Allow holes in reference properties
  ACPI / gpio: Add support for naming GPIOs
  ACPI / gpio: Add hogging support
  ACPI / gpio: Allow holes in list of GPIOs for a device

 Documentation/acpi/gpio-properties.txt |  62 ++++++++++++++
 drivers/acpi/property.c                | 117 +++++++++++++++-----------
 drivers/gpio/gpiolib-acpi.c            | 147 ++++++++++++++++++++++++++++++---
 include/linux/acpi.h                   |  22 ++++-
 4 files changed, 285 insertions(+), 63 deletions(-)

-- 
2.9.3

[PATCH v2 1/4] ACPI / property: Allow holes in reference properties

[Mika Westerberg]

DT allows holes or empty phandles for references. This is used for example
in SPI subsystem where some chip selects are native and others are regular
GPIOs. In ACPI _DSD we currently do not support this but instead the
preceding reference consumes all following integer arguments.

For example we would like to support something like the below ASL fragment
for SPI:

  Package () {
      "cs-gpios",
      Package () {
          ^GPIO, 19, 0, 0, // GPIO CS0
          0,               // Native CS
          ^GPIO, 20, 0, 0, // GPIO CS1
      }
  }

The zero in the middle means "no entry" or NULL reference. To support this
we change acpi_data_get_property_reference() to take firmware node and
num_args as argument and rename it to __acpi_node_get_property_reference().
The function returns -ENOENT if the given index resolves to "no entry"
reference and -ENODATA when there are no more entries in the property.

We then add static inline wrapper acpi_node_get_property_reference() that
passes MAX_ACPI_REFERENCE_ARGS as num_args to support the existing
behaviour which some drivers have been relying on.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/property.c | 117 ++++++++++++++++++++++++++++--------------------
 include/linux/acpi.h    |  22 +++++++--
 2 files changed, 87 insertions(+), 52 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index f2fd3fee588a..03f5ec11ab31 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -468,10 +468,11 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
 }
 
 /**
- * acpi_data_get_property_reference - returns handle to the referenced object
- * @data: ACPI device data object containing the property
+ * __acpi_node_get_property_reference - returns handle to the referenced object
+ * @fwnode: Firmware node to get the property from
  * @propname: Name of the property
  * @index: Index of the reference to return
+ * @num_args: Maximum number of arguments after each reference
  * @args: Location to store the returned reference with optional arguments
  *
  * Find property with @name, verifify that it is a package containing at least
@@ -482,17 +483,40 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
  * If there's more than one reference in the property value package, @index is
  * used to select the one to return.
  *
+ * It is possible to leave holes in the property value set like in the
+ * example below:
+ *
+ * Package () {
+ *     "cs-gpios",
+ *     Package () {
+ *        ^GPIO, 19, 0, 0,
+ *        ^GPIO, 20, 0, 0,
+ *        0,
+ *        ^GPIO, 21, 0, 0,
+ *     }
+ * }
+ *
+ * Calling this function with index %2 return %-ENOENT and with index %3
+ * returns the last entry. If the property does not contain any more values
+ * %-ENODATA is returned. The NULL entry must be single integer and
+ * preferably contain value %0.
+ *
  * Return: %0 on success, negative error code on failure.
  */
-static int acpi_data_get_property_reference(struct acpi_device_data *data,
-					    const char *propname, size_t index,
-					    struct acpi_reference_args *args)
+int __acpi_node_get_property_reference(struct fwnode_handle *fwnode,
+	const char *propname, size_t index, size_t num_args,
+	struct acpi_reference_args *args)
 {
 	const union acpi_object *element, *end;
 	const union acpi_object *obj;
+	struct acpi_device_data *data;
 	struct acpi_device *device;
 	int ret, idx = 0;
 
+	data = acpi_device_data_of_node(fwnode);
+	if (!data)
+		return -EINVAL;
+
 	ret = acpi_data_get_property(data, propname, ACPI_TYPE_ANY, &obj);
 	if (ret)
 		return ret;
@@ -532,59 +556,54 @@ static int acpi_data_get_property_reference(struct acpi_device_data *data,
 	while (element < end) {
 		u32 nargs, i;
 
-		if (element->type != ACPI_TYPE_LOCAL_REFERENCE)
-			return -EPROTO;
-
-		ret = acpi_bus_get_device(element->reference.handle, &device);
-		if (ret)
-			return -ENODEV;
-
-		element++;
-		nargs = 0;
-
-		/* assume following integer elements are all args */
-		for (i = 0; element + i < end; i++) {
-			int type = element[i].type;
+		if (element->type == ACPI_TYPE_LOCAL_REFERENCE) {
+			ret = acpi_bus_get_device(element->reference.handle,
+						  &device);
+			if (ret)
+				return -ENODEV;
+
+			nargs = 0;
+			element++;
+
+			/* assume following integer elements are all args */
+			for (i = 0; element + i < end && i < num_args; i++) {
+				int type = element[i].type;
+
+				if (type == ACPI_TYPE_INTEGER)
+					nargs++;
+				else if (type == ACPI_TYPE_LOCAL_REFERENCE)
+					break;
+				else
+					return -EPROTO;
+			}
 
-			if (type == ACPI_TYPE_INTEGER)
-				nargs++;
-			else if (type == ACPI_TYPE_LOCAL_REFERENCE)
-				break;
-			else
+			if (nargs > MAX_ACPI_REFERENCE_ARGS)
 				return -EPROTO;
-		}
 
-		if (idx++ == index) {
-			args->adev = device;
-			args->nargs = nargs;
-			for (i = 0; i < nargs; i++)
-				args->args[i] = element[i].integer.value;
+			if (idx == index) {
+				args->adev = device;
+				args->nargs = nargs;
+				for (i = 0; i < nargs; i++)
+					args->args[i] = element[i].integer.value;
 
-			return 0;
+				return 0;
+			}
+
+			element += nargs;
+		} else if (element->type == ACPI_TYPE_INTEGER) {
+			if (idx == index)
+				return -ENOENT;
+			element++;
+		} else {
+			return -EPROTO;
 		}
 
-		element += nargs;
+		idx++;
 	}
 
-	return -EPROTO;
-}
-
-/**
- * acpi_node_get_property_reference - get a handle to the referenced object.
- * @fwnode: Firmware node to get the property from.
- * @propname: Name of the property.
- * @index: Index of the reference to return.
- * @args: Location to store the returned reference with optional arguments.
- */
-int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
-				     const char *name, size_t index,
-				     struct acpi_reference_args *args)
-{
-	struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
-
-	return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
+	return -ENODATA;
 }
-EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);
+EXPORT_SYMBOL_GPL(__acpi_node_get_property_reference);
 
 static int acpi_data_prop_read_single(struct acpi_device_data *data,
 				      const char *propname,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index c5eaf2f80a4c..54460ad3a7a6 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -927,9 +927,17 @@ struct acpi_reference_args {
 #ifdef CONFIG_ACPI
 int acpi_dev_get_property(struct acpi_device *adev, const char *name,
 			  acpi_object_type type, const union acpi_object **obj);
-int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
-				     const char *name, size_t index,
-				     struct acpi_reference_args *args);
+int __acpi_node_get_property_reference(struct fwnode_handle *fwnode,
+				const char *name, size_t index, size_t num_args,
+				struct acpi_reference_args *args);
+
+static inline int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
+				const char *name, size_t index,
+				struct acpi_reference_args *args)
+{
+	return __acpi_node_get_property_reference(fwnode, name, index,
+		MAX_ACPI_REFERENCE_ARGS, args);
+}
 
 int acpi_node_prop_get(struct fwnode_handle *fwnode, const char *propname,
 		       void **valptr);
@@ -1005,6 +1013,14 @@ static inline int acpi_dev_get_property(struct acpi_device *adev,
 	return -ENXIO;
 }
 
+static inline int
+__acpi_node_get_property_reference(struct fwnode_handle *fwnode,
+				const char *name, size_t index, size_t num_args,
+				struct acpi_reference_args *args)
+{
+	return -ENXIO;
+}
+
 static inline int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
 				const char *name, size_t index,
 				struct acpi_reference_args *args)
-- 
2.9.3

Re: [PATCH v2 1/4] ACPI / property: Allow holes in reference properties

[Rafael J. Wysocki]

On Thu, Sep 29, 2016 at 3:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> DT allows holes or empty phandles for references. This is used for example
> in SPI subsystem where some chip selects are native and others are regular
> GPIOs. In ACPI _DSD we currently do not support this but instead the
> preceding reference consumes all following integer arguments.
>
> For example we would like to support something like the below ASL fragment
> for SPI:
>
>   Package () {
>       "cs-gpios",
>       Package () {
>           ^GPIO, 19, 0, 0, // GPIO CS0
>           0,               // Native CS
>           ^GPIO, 20, 0, 0, // GPIO CS1
>       }
>   }
>
> The zero in the middle means "no entry" or NULL reference. To support this
> we change acpi_data_get_property_reference() to take firmware node and
> num_args as argument and rename it to __acpi_node_get_property_reference().
> The function returns -ENOENT if the given index resolves to "no entry"
> reference and -ENODATA when there are no more entries in the property.
>
> We then add static inline wrapper acpi_node_get_property_reference() that
> passes MAX_ACPI_REFERENCE_ARGS as num_args to support the existing
> behaviour which some drivers have been relying on.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

The patch looks good to me and I don't see any reason to defer it, so
I'm queuing it up for the next ACPI pull request.

Thanks,
Rafael

Re: [PATCH v2 1/4] ACPI / property: Allow holes in reference properties

[Linus Walleij]

On Thu, Sep 29, 2016 at 3:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> DT allows holes or empty phandles for references. This is used for example
> in SPI subsystem where some chip selects are native and others are regular
> GPIOs. In ACPI _DSD we currently do not support this but instead the
> preceding reference consumes all following integer arguments.
>
> For example we would like to support something like the below ASL fragment
> for SPI:
>
>   Package () {
>       "cs-gpios",
>       Package () {
>           ^GPIO, 19, 0, 0, // GPIO CS0
>           0,               // Native CS
>           ^GPIO, 20, 0, 0, // GPIO CS1
>       }
>   }
>
> The zero in the middle means "no entry" or NULL reference. To support this
> we change acpi_data_get_property_reference() to take firmware node and
> num_args as argument and rename it to __acpi_node_get_property_reference().
> The function returns -ENOENT if the given index resolves to "no entry"
> reference and -ENODATA when there are no more entries in the property.
>
> We then add static inline wrapper acpi_node_get_property_reference() that
> passes MAX_ACPI_REFERENCE_ARGS as num_args to support the existing
> behaviour which some drivers have been relying on.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 2/4] ACPI / gpio: Add support for naming GPIOs

[Mika Westerberg]

DT has property 'gpio-line-names' to name GPIO lines the controller has if
present. Use this very same property in ACPI as well to provide nice names
for the GPIOS.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 Documentation/acpi/gpio-properties.txt | 21 +++++++++++++++++
 drivers/gpio/gpiolib-acpi.c            | 43 ++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/Documentation/acpi/gpio-properties.txt b/Documentation/acpi/gpio-properties.txt
index 5aafe0b351a1..7875974823ae 100644
--- a/Documentation/acpi/gpio-properties.txt
+++ b/Documentation/acpi/gpio-properties.txt
@@ -51,6 +51,27 @@ it to 1 marks the GPIO as active low.
 In our Bluetooth example the "reset-gpios" refers to the second GpioIo()
 resource, second pin in that resource with the GPIO number of 31.
 
+Other supported properties
+--------------------------
+
+Following Device Tree compatible device properties are also supported by
+_DSD device properties for GPIO controllers:
+
+- gpio-line-names
+
+Example:
+
+  Package () {
+      "gpio-line-names",
+      Package () {
+          "SPI0_CS_N", "EXP2_INT", "MUX6_IO", "UART0_RXD", "MUX7_IO",
+          "LVL_C_A1", "MUX0_IO", "SPI1_MISO"
+      }
+  }
+
+See Documentation/devicetree/bindings/gpio/gpio.tx for more information
+about these properties.
+
 ACPI GPIO Mappings Provided by Drivers
 --------------------------------------
 
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index af514618d7fb..202cf1b842de 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -805,6 +805,46 @@ static void acpi_gpiochip_free_regions(struct acpi_gpio_chip *achip)
 	}
 }
 
+static void acpi_gpiochip_set_names(struct acpi_gpio_chip *achip)
+{
+	struct gpio_chip *chip = achip->chip;
+	struct gpio_device *gdev = chip->gpiodev;
+	const char **names;
+	int ret, i;
+
+	ret = device_property_read_string_array(chip->parent, "gpio-line-names",
+						NULL, 0);
+	if (ret < 0)
+		return;
+
+	if (ret != gdev->ngpio) {
+		dev_warn(chip->parent,
+			 "names %d do not match number of GPIOs %d\n", ret,
+			 gdev->ngpio);
+		return;
+	}
+
+	names = kcalloc(gdev->ngpio, sizeof(*names), GFP_KERNEL);
+	if (!names)
+		return;
+
+	ret = device_property_read_string_array(chip->parent, "gpio-line-names",
+						names, gdev->ngpio);
+	if (ret < 0) {
+		dev_warn(chip->parent, "Failed to read GPIO line names\n");
+		return;
+	}
+
+	/*
+	 * It is fine to assign the name, it will be allocated as long as
+	 * the ACPI device exists.
+	 */
+	for (i = 0; i < gdev->ngpio; i++)
+		gdev->descs[i].name = names[i];
+
+	kfree(names);
+}
+
 void acpi_gpiochip_add(struct gpio_chip *chip)
 {
 	struct acpi_gpio_chip *acpi_gpio;
@@ -835,6 +875,9 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
 		return;
 	}
 
+	if (!chip->names)
+		acpi_gpiochip_set_names(acpi_gpio);
+
 	acpi_gpiochip_request_regions(acpi_gpio);
 	acpi_walk_dep_device_list(handle);
 }
-- 
2.9.3

Re: [PATCH v2 2/4] ACPI / gpio: Add support for naming GPIOs

[Andy Shevchenko]

On Thu, Sep 29, 2016 at 4:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> DT has property 'gpio-line-names' to name GPIO lines the controller has if
> present. Use this very same property in ACPI as well to provide nice names
> for the GPIOS.

One nit below.

> @@ -835,6 +875,9 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
>                 return;
>         }
>
> +       if (!chip->names)
> +               acpi_gpiochip_set_names(acpi_gpio);
> +

I'm okay with this, though wouldn't be better to call it
unconditionally like it's done for below call and move check inside?

>         acpi_gpiochip_request_regions(acpi_gpio);
>         acpi_walk_dep_device_list(handle);
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] ACPI / gpio: Add support for naming GPIOs

[Mika Westerberg]

On Fri, Oct 07, 2016 at 08:05:14PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 29, 2016 at 4:39 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > DT has property 'gpio-line-names' to name GPIO lines the controller has if
> > present. Use this very same property in ACPI as well to provide nice names
> > for the GPIOS.
> 
> One nit below.
> 
> > @@ -835,6 +875,9 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
> >                 return;
> >         }
> >
> > +       if (!chip->names)
> > +               acpi_gpiochip_set_names(acpi_gpio);
> > +
> 
> I'm okay with this, though wouldn't be better to call it
> unconditionally like it's done for below call and move check inside?

DT does it like this. I can move the check inside the function as well.

> 
> >         acpi_gpiochip_request_regions(acpi_gpio);
> >         acpi_walk_dep_device_list(handle);
> >  }
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 2/4] ACPI / gpio: Add support for naming GPIOs

[Andy Shevchenko]

On Sun, Oct 9, 2016 at 6:01 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Oct 07, 2016 at 08:05:14PM +0300, Andy Shevchenko wrote:
>> On Thu, Sep 29, 2016 at 4:39 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:

>> > +       if (!chip->names)
>> > +               acpi_gpiochip_set_names(acpi_gpio);
>> > +
>>
>> I'm okay with this, though wouldn't be better to call it
>> unconditionally like it's done for below call and move check inside?
>
> DT does it like this. I can move the check inside the function as well.

Up to you. If it even worth to change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] ACPI / gpio: Add support for naming GPIOs

[Linus Walleij]

On Thu, Sep 29, 2016 at 3:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> DT has property 'gpio-line-names' to name GPIO lines the controller has if
> present. Use this very same property in ACPI as well to provide nice names
> for the GPIOS.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Looks OK to me, is this patch independent so I can merge it into the
GPIO tree or does it need to go in with patch 1/4 to the ACPI tree?

Yours,
Linus Walleij

Re: [PATCH v2 2/4] ACPI / gpio: Add support for naming GPIOs

[Mika Westerberg]

On Thu, Oct 20, 2016 at 02:02:34PM +0200, Linus Walleij wrote:
> On Thu, Sep 29, 2016 at 3:39 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > DT has property 'gpio-line-names' to name GPIO lines the controller has if
> > present. Use this very same property in ACPI as well to provide nice names
> > for the GPIOS.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Looks OK to me, is this patch independent so I can merge it into the
> GPIO tree or does it need to go in with patch 1/4 to the ACPI tree?

This (2/4) and the 3/4 are independent of patch 1/4 so they can go via
any tree.

Re: [PATCH v2 2/4] ACPI / gpio: Add support for naming GPIOs

[Linus Walleij]

On Thu, Sep 29, 2016 at 3:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> DT has property 'gpio-line-names' to name GPIO lines the controller has if
> present. Use this very same property in ACPI as well to provide nice names
> for the GPIOS.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Oh wait:

> +static void acpi_gpiochip_set_names(struct acpi_gpio_chip *achip)
> +{
> +       struct gpio_chip *chip = achip->chip;
> +       struct gpio_device *gdev = chip->gpiodev;
> +       const char **names;
> +       int ret, i;
> +
> +       ret = device_property_read_string_array(chip->parent, "gpio-line-names",
> +                                               NULL, 0);
> +       if (ret < 0)
> +               return;
> +
> +       if (ret != gdev->ngpio) {
> +               dev_warn(chip->parent,
> +                        "names %d do not match number of GPIOs %d\n", ret,
> +                        gdev->ngpio);
> +               return;
> +       }
> +
> +       names = kcalloc(gdev->ngpio, sizeof(*names), GFP_KERNEL);
> +       if (!names)
> +               return;
> +
> +       ret = device_property_read_string_array(chip->parent, "gpio-line-names",
> +                                               names, gdev->ngpio);
> +       if (ret < 0) {
> +               dev_warn(chip->parent, "Failed to read GPIO line names\n");
> +               return;
> +       }
> +
> +       /*
> +        * It is fine to assign the name, it will be allocated as long as
> +        * the ACPI device exists.
> +        */
> +       for (i = 0; i < gdev->ngpio; i++)
> +               gdev->descs[i].name = names[i];
> +
> +       kfree(names);
> +}

Wouldn't this entire function work just as fine on device tree?

So should this snippet using device_property_* be moved into
a file like gpiolib-devprop.c+gpiolib.h signature and get used from
both gpiolib-of.c and gpiolib-acpi.c, replacing the DT-specific
code in gpiolib-of.c?

Yours,
Linus Walleij

Re: [PATCH v2 2/4] ACPI / gpio: Add support for naming GPIOs

[Mika Westerberg]

On Thu, Oct 20, 2016 at 02:06:42PM +0200, Linus Walleij wrote:
> On Thu, Sep 29, 2016 at 3:39 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > DT has property 'gpio-line-names' to name GPIO lines the controller has if
> > present. Use this very same property in ACPI as well to provide nice names
> > for the GPIOS.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Oh wait:
> 
> > +static void acpi_gpiochip_set_names(struct acpi_gpio_chip *achip)
> > +{
> > +       struct gpio_chip *chip = achip->chip;
> > +       struct gpio_device *gdev = chip->gpiodev;
> > +       const char **names;
> > +       int ret, i;
> > +
> > +       ret = device_property_read_string_array(chip->parent, "gpio-line-names",
> > +                                               NULL, 0);
> > +       if (ret < 0)
> > +               return;
> > +
> > +       if (ret != gdev->ngpio) {
> > +               dev_warn(chip->parent,
> > +                        "names %d do not match number of GPIOs %d\n", ret,
> > +                        gdev->ngpio);
> > +               return;
> > +       }
> > +
> > +       names = kcalloc(gdev->ngpio, sizeof(*names), GFP_KERNEL);
> > +       if (!names)
> > +               return;
> > +
> > +       ret = device_property_read_string_array(chip->parent, "gpio-line-names",
> > +                                               names, gdev->ngpio);
> > +       if (ret < 0) {
> > +               dev_warn(chip->parent, "Failed to read GPIO line names\n");
> > +               return;
> > +       }
> > +
> > +       /*
> > +        * It is fine to assign the name, it will be allocated as long as
> > +        * the ACPI device exists.
> > +        */
> > +       for (i = 0; i < gdev->ngpio; i++)
> > +               gdev->descs[i].name = names[i];
> > +
> > +       kfree(names);
> > +}
> 
> Wouldn't this entire function work just as fine on device tree?

Now that you mentioned it, yes, I think it should work with DT as well.

> So should this snippet using device_property_* be moved into
> a file like gpiolib-devprop.c+gpiolib.h signature and get used from
> both gpiolib-of.c and gpiolib-acpi.c, replacing the DT-specific
> code in gpiolib-of.c?

Works for me :)

[PATCH v2 3/4] ACPI / gpio: Add hogging support

[Mika Westerberg]

GPIO hogging means that the GPIO controller can "hog" and configure certain
GPIOs without need for a driver or userspace to do that. This is useful in
open-connected boards where BIOS cannot possibly know beforehand which
devices will be connected to the board.

This adds GPIO hogging mechanism to ACPI analogous to Device Tree.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 Documentation/acpi/gpio-properties.txt | 26 ++++++++++++
 drivers/gpio/gpiolib-acpi.c            | 72 ++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/Documentation/acpi/gpio-properties.txt b/Documentation/acpi/gpio-properties.txt
index 7875974823ae..46df91c3512b 100644
--- a/Documentation/acpi/gpio-properties.txt
+++ b/Documentation/acpi/gpio-properties.txt
@@ -69,6 +69,32 @@ Example:
       }
   }
 
+- gpio-hog
+- output-high
+- output-low
+- input
+- line-name
+
+Example:
+
+  Name (_DSD, Package () {
+      // _DSD Hierarchical Properties Extension UUID
+      ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+      Package () {
+          Package () {"hog-gpio8", "G8PU"}
+      }
+  })
+
+  Name (G8PU, Package () {
+      ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+      Package () {
+          Package () {"gpio-hog", 1},
+          Package () {"gpios", Package () {8, 0}},
+          Package () {"output-high", 1},
+          Package () {"line-name", "gpio8-pullup"},
+      }
+  })
+
 See Documentation/devicetree/bindings/gpio/gpio.tx for more information
 about these properties.
 
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 202cf1b842de..4e50f8f5d597 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -14,6 +14,7 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/interrupt.h>
@@ -845,6 +846,76 @@ static void acpi_gpiochip_set_names(struct acpi_gpio_chip *achip)
 	kfree(names);
 }
 
+struct gpio_desc *acpi_gpiochip_parse_own_gpio(struct acpi_gpio_chip *achip,
+	struct fwnode_handle *fwnode, const char **name, unsigned int *lflags,
+	unsigned int *dflags)
+{
+	struct gpio_chip *chip = achip->chip;
+	struct gpio_desc *desc;
+	u32 gpios[2];
+	int ret;
+
+	ret = fwnode_property_read_u32_array(fwnode, "gpios", gpios,
+					     ARRAY_SIZE(gpios));
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	ret = acpi_gpiochip_pin_to_gpio_offset(chip->gpiodev, gpios[0]);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	desc = gpiochip_get_desc(chip, ret);
+	if (IS_ERR(desc))
+		return desc;
+
+	*lflags = 0;
+	*dflags = 0;
+	*name = NULL;
+
+	if (gpios[1])
+		*lflags |= GPIO_ACTIVE_LOW;
+
+	if (fwnode_property_present(fwnode, "input"))
+		*dflags |= GPIOD_IN;
+	else if (fwnode_property_present(fwnode, "output-low"))
+		*dflags |= GPIOD_OUT_LOW;
+	else if (fwnode_property_present(fwnode, "output-high"))
+		*dflags |= GPIOD_OUT_HIGH;
+	else
+		return ERR_PTR(-EINVAL);
+
+	fwnode_property_read_string(fwnode, "line-name", name);
+
+	return desc;
+}
+
+static void acpi_gpiochip_scan_gpios(struct acpi_gpio_chip *achip)
+{
+	struct gpio_chip *chip = achip->chip;
+	struct fwnode_handle *fwnode;
+
+	device_for_each_child_node(chip->parent, fwnode) {
+		unsigned int lflags, dflags;
+		struct gpio_desc *desc;
+		const char *name;
+		int ret;
+
+		if (!fwnode_property_present(fwnode, "gpio-hog"))
+			continue;
+
+		desc = acpi_gpiochip_parse_own_gpio(achip, fwnode, &name,
+						    &lflags, &dflags);
+		if (IS_ERR(desc))
+			continue;
+
+		ret = gpiod_hog(desc, name, lflags, dflags);
+		if (ret) {
+			dev_err(chip->parent, "Failed to hog GPIO\n");
+			return;
+		}
+	}
+}
+
 void acpi_gpiochip_add(struct gpio_chip *chip)
 {
 	struct acpi_gpio_chip *acpi_gpio;
@@ -879,6 +950,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
 		acpi_gpiochip_set_names(acpi_gpio);
 
 	acpi_gpiochip_request_regions(acpi_gpio);
+	acpi_gpiochip_scan_gpios(acpi_gpio);
 	acpi_walk_dep_device_list(handle);
 }
 
-- 
2.9.3

Re: [PATCH v2 3/4] ACPI / gpio: Add hogging support

[Linus Walleij]

On Thu, Sep 29, 2016 at 3:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> GPIO hogging means that the GPIO controller can "hog" and configure certain
> GPIOs without need for a driver or userspace to do that. This is useful in
> open-connected boards where BIOS cannot possibly know beforehand which
> devices will be connected to the board.
>
> This adds GPIO hogging mechanism to ACPI analogous to Device Tree.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Overall nice to have the hogs in ACPI too!

Again here my comment is to break out device_property() and fwnode_*
accessing code to a file shared between ACPI and OF.

If it makes sense.

Because the idea was not to invent those abstractions in order to
just use them with ACPI I hope.

The problem is that some of the code seems to be ACPI-specific,
so if it doesn't work out, I'm also OK to keep it like this.

Yours,
Linus Walleij

Re: [PATCH v2 3/4] ACPI / gpio: Add hogging support

[Mika Westerberg]

On Thu, Oct 20, 2016 at 02:09:30PM +0200, Linus Walleij wrote:
> On Thu, Sep 29, 2016 at 3:39 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > GPIO hogging means that the GPIO controller can "hog" and configure certain
> > GPIOs without need for a driver or userspace to do that. This is useful in
> > open-connected boards where BIOS cannot possibly know beforehand which
> > devices will be connected to the board.
> >
> > This adds GPIO hogging mechanism to ACPI analogous to Device Tree.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Overall nice to have the hogs in ACPI too!
> 
> Again here my comment is to break out device_property() and fwnode_*
> accessing code to a file shared between ACPI and OF.
> 
> If it makes sense.
> 
> Because the idea was not to invent those abstractions in order to
> just use them with ACPI I hope.
> 
> The problem is that some of the code seems to be ACPI-specific,
> so if it doesn't work out, I'm also OK to keep it like this.

Yeah, this one differs between ACPI and DT and I'm not sure if it can be
easily generalized using device_property APIs.

[PATCH v2 4/4] ACPI / gpio: Allow holes in list of GPIOs for a device

[Mika Westerberg]

Make it possible to have an empty GPIOs in a GPIO list for device. For
example a SPI master may use both GPIOs and native pins as chip selects and
we need to be able to distinguish between the two.

This makes it mandatory to have exactly 3 arguments for GPIOs and then
converts gpiolib to use of __acpi_node_get_property_reference() instead. In
addition we make acpi_gpio_package_count() to handle holes as well (this
matches the DT version).

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 Documentation/acpi/gpio-properties.txt | 15 +++++++++++++++
 drivers/gpio/gpiolib-acpi.c            | 32 +++++++++++++++++++++-----------
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/Documentation/acpi/gpio-properties.txt b/Documentation/acpi/gpio-properties.txt
index 46df91c3512b..2ccb70893122 100644
--- a/Documentation/acpi/gpio-properties.txt
+++ b/Documentation/acpi/gpio-properties.txt
@@ -51,6 +51,21 @@ it to 1 marks the GPIO as active low.
 In our Bluetooth example the "reset-gpios" refers to the second GpioIo()
 resource, second pin in that resource with the GPIO number of 31.
 
+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
+controller can have chip selects 0 and 2 implemented as GPIOs and 1 as
+native:
+
+  Package () {
+      "cs-gpios",
+      Package () {
+          ^GPIO, 19, 0, 0, // chip select 0: GPIO
+          0,               // chip select 1: native signal
+          ^GPIO, 20, 0, 0, // chip select 2: GPIO
+      }
+  }
+
 Other supported properties
 --------------------------
 
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 4e50f8f5d597..096a94ee96e7 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -467,7 +467,8 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 	int ret;
 
 	memset(&args, 0, sizeof(args));
-	ret = acpi_node_get_property_reference(fwnode, propname, index, &args);
+	ret = __acpi_node_get_property_reference(fwnode, propname, index, 3,
+						 &args);
 	if (ret) {
 		struct acpi_device *adev = to_acpi_device_node(fwnode);
 
@@ -482,13 +483,13 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
 	 * on returned args.
 	 */
 	lookup->adev = args.adev;
-	if (args.nargs >= 2) {
-		lookup->index = args.args[0];
-		lookup->pin_index = args.args[1];
-		/* 3rd argument, if present is used to specify active_low. */
-		if (args.nargs >= 3)
-			lookup->active_low = !!args.args[2];
-	}
+	if (args.nargs != 3)
+		return -EPROTO;
+
+	lookup->index = args.args[0];
+	lookup->pin_index = args.args[1];
+	lookup->active_low = !!args.args[2];
+
 	return 0;
 }
 
@@ -979,18 +980,27 @@ void acpi_gpiochip_remove(struct gpio_chip *chip)
 	kfree(acpi_gpio);
 }
 
-static unsigned int acpi_gpio_package_count(const union acpi_object *obj)
+static int acpi_gpio_package_count(const union acpi_object *obj)
 {
 	const union acpi_object *element = obj->package.elements;
 	const union acpi_object *end = element + obj->package.count;
 	unsigned int count = 0;
 
 	while (element < end) {
-		if (element->type == ACPI_TYPE_LOCAL_REFERENCE)
+		switch (element->type) {
+		case ACPI_TYPE_LOCAL_REFERENCE:
+			element += 3;
+			/* Fallthrough */
+		case ACPI_TYPE_INTEGER:
+			element++;
 			count++;
+			break;
 
-		element++;
+		default:
+			return -EPROTO;
+		}
 	}
+
 	return count;
 }
 
-- 
2.9.3

Re: [PATCH v2 4/4] ACPI / gpio: Allow holes in list of GPIOs for a device

[Linus Walleij]

On Thu, Sep 29, 2016 at 3:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> Make it possible to have an empty GPIOs in a GPIO list for device. For
> example a SPI master may use both GPIOs and native pins as chip selects and
> we need to be able to distinguish between the two.
>
> This makes it mandatory to have exactly 3 arguments for GPIOs and then
> converts gpiolib to use of __acpi_node_get_property_reference() instead. In
> addition we make acpi_gpio_package_count() to handle holes as well (this
> matches the DT version).
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Looks OK.

Do I need someone's ACK to merge stuff under
Documentation/acpi/gpio-properties.txt?
Like Rafael?

Yours,
Linus Walleij

Re: [PATCH v2 0/4] ACPI / gpio: Updates to properties

[Andy Shevchenko]

On Thu, Sep 29, 2016 at 4:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Hi,
>
> This series brings couple of useful GPIO related properties from Device
> Tree to ACPI _DSD device properties:
>
>   - Names for GPIO lines
>   - GPIO hogging
>   - Holes in GPIO property lists
>
> We are using these to get Intel Galileo better supported in the mainline
> kernel (but these may be useful for other boards as well). For example SPI
> chip select on Galileo is connected to a GPIO line so we need to be able to
> describe it in ACPI, and at the same time allow native chip selects.
>
> GPIO hogging can be used to set initial state of certain GPIOs available on
> the headers regardless of the BIOS settings (which may be wrong as it knows
> nothing about which devices have been connected).

Awesome!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> The previous version can be found here:
>
>   http://www.spinics.net/lists/linux-acpi/msg69385.html
>
> Changes from v1:
>   - Drop patch [1/5] as it has been applied already.
>   - Move patch [4/5] to be the first.
>   - Rename acpi_data_get_property_reference() to __acpi_node_get_property_reference().
>   - Drop acpi_node_get_property_reference() as it is not necessary anymore.
>   - Add static inline wrapper acpi_node_get_property_reference() that
>     calls the previous passing MAX_ACPI_REFERENCE_ARGS to support existing
>     drivers.
>
> Mika Westerberg (4):
>   ACPI / property: Allow holes in reference properties
>   ACPI / gpio: Add support for naming GPIOs
>   ACPI / gpio: Add hogging support
>   ACPI / gpio: Allow holes in list of GPIOs for a device
>
>  Documentation/acpi/gpio-properties.txt |  62 ++++++++++++++
>  drivers/acpi/property.c                | 117 +++++++++++++++-----------
>  drivers/gpio/gpiolib-acpi.c            | 147 ++++++++++++++++++++++++++++++---
>  include/linux/acpi.h                   |  22 ++++-
>  4 files changed, 285 insertions(+), 63 deletions(-)
>
> --
> 2.9.3
>



-- 
With Best Regards,
Andy Shevchenko