All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] ACPI / gpio: Updates to properties
@ 2016-09-23 14:57 Mika Westerberg
  2016-09-23 14:57 ` [PATCH 1/5] ACPI / documentation: Use recommended name in GPIO property names Mika Westerberg
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Mika Westerberg @ 2016-09-23 14:57 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linus Walleij
  Cc: Alexandre Courbot, linux-acpi, linux-kernel, 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).

Mika Westerberg (5):
  ACPI / documentation: Use recommended name in GPIO property names
  ACPI / gpio: Add support for naming GPIOs
  ACPI / gpio: Add hogging support
  ACPI / property: Allow holes in reference properties
  ACPI / gpio: Allow holes in list of GPIOs for a device

 Documentation/acpi/gpio-properties.txt |  72 ++++++++++++++--
 drivers/acpi/property.c                | 116 +++++++++++++++++++-------
 drivers/gpio/gpiolib-acpi.c            | 147 ++++++++++++++++++++++++++++++---
 include/linux/acpi.h                   |   3 +
 4 files changed, 292 insertions(+), 46 deletions(-)

-- 
2.9.3


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

* [PATCH 1/5] ACPI / documentation: Use recommended name in GPIO property names
  2016-09-23 14:57 [PATCH 0/5] ACPI / gpio: Updates to properties Mika Westerberg
@ 2016-09-23 14:57 ` Mika Westerberg
  2016-09-23 14:57 ` [PATCH 2/5] ACPI / gpio: Add support for naming GPIOs Mika Westerberg
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Mika Westerberg @ 2016-09-23 14:57 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linus Walleij
  Cc: Alexandre Courbot, linux-acpi, linux-kernel, Mika Westerberg

The recommended property name for all kinds of GPIOs is to end it with
"-gpios" even if there is only one GPIO. Update the documentation to follow
this fact.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 Documentation/acpi/gpio-properties.txt | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/acpi/gpio-properties.txt b/Documentation/acpi/gpio-properties.txt
index f35dad11f0de..5aafe0b351a1 100644
--- a/Documentation/acpi/gpio-properties.txt
+++ b/Documentation/acpi/gpio-properties.txt
@@ -28,8 +28,8 @@ index, like the ASL example below shows:
           ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
           Package ()
 	  {
-              Package () {"reset-gpio", Package() {^BTH, 1, 1, 0 }},
-              Package () {"shutdown-gpio", Package() {^BTH, 0, 0, 0 }},
+              Package () {"reset-gpios", Package() {^BTH, 1, 1, 0 }},
+              Package () {"shutdown-gpios", Package() {^BTH, 0, 0, 0 }},
           }
       })
   }
@@ -48,7 +48,7 @@ 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.
 
-In our Bluetooth example the "reset-gpio" refers to the second GpioIo()
+In our Bluetooth example the "reset-gpios" refers to the second GpioIo()
 resource, second pin in that resource with the GPIO number of 31.
 
 ACPI GPIO Mappings Provided by Drivers
@@ -83,8 +83,8 @@ static const struct acpi_gpio_params reset_gpio = { 1, 1, false };
 static const struct acpi_gpio_params shutdown_gpio = { 0, 0, false };
 
 static const struct acpi_gpio_mapping bluetooth_acpi_gpios[] = {
-  { "reset-gpio", &reset_gpio, 1 },
-  { "shutdown-gpio", &shutdown_gpio, 1 },
+  { "reset-gpios", &reset_gpio, 1 },
+  { "shutdown-gpios", &shutdown_gpio, 1 },
   { },
 };
 
-- 
2.9.3

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

* [PATCH 2/5] ACPI / gpio: Add support for naming GPIOs
  2016-09-23 14:57 [PATCH 0/5] ACPI / gpio: Updates to properties Mika Westerberg
  2016-09-23 14:57 ` [PATCH 1/5] ACPI / documentation: Use recommended name in GPIO property names Mika Westerberg
@ 2016-09-23 14:57 ` Mika Westerberg
  2016-09-23 14:57 ` [PATCH 3/5] ACPI / gpio: Add hogging support Mika Westerberg
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Mika Westerberg @ 2016-09-23 14:57 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linus Walleij
  Cc: Alexandre Courbot, linux-acpi, linux-kernel, 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


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

* [PATCH 3/5] ACPI / gpio: Add hogging support
  2016-09-23 14:57 [PATCH 0/5] ACPI / gpio: Updates to properties Mika Westerberg
  2016-09-23 14:57 ` [PATCH 1/5] ACPI / documentation: Use recommended name in GPIO property names Mika Westerberg
  2016-09-23 14:57 ` [PATCH 2/5] ACPI / gpio: Add support for naming GPIOs Mika Westerberg
@ 2016-09-23 14:57 ` Mika Westerberg
  2016-09-23 14:57 ` [PATCH 4/5] ACPI / property: Allow holes in reference properties Mika Westerberg
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Mika Westerberg @ 2016-09-23 14:57 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linus Walleij
  Cc: Alexandre Courbot, linux-acpi, linux-kernel, 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

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

* [PATCH 4/5] ACPI / property: Allow holes in reference properties
  2016-09-23 14:57 [PATCH 0/5] ACPI / gpio: Updates to properties Mika Westerberg
                   ` (2 preceding siblings ...)
  2016-09-23 14:57 ` [PATCH 3/5] ACPI / gpio: Add hogging support Mika Westerberg
@ 2016-09-23 14:57 ` Mika Westerberg
  2016-09-28 21:36   ` Rafael J. Wysocki
  2016-09-23 14:57 ` [PATCH 5/5] ACPI / gpio: Allow holes in list of GPIOs for a device Mika Westerberg
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Mika Westerberg @ 2016-09-23 14:57 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linus Walleij
  Cc: Alexandre Courbot, linux-acpi, linux-kernel, 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 add a new function acpi_node_get_property_reference_fixed_args() that
takes number of expected arguments as parameter. Function returns -ENOENT
if the corresponding index resolves to the "no entry" reference.

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

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index f2fd3fee588a..b255c4442a05 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -472,6 +472,7 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
  * @data: ACPI device data object containing the property
  * @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
@@ -485,8 +486,8 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
  * 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)
+	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;
@@ -532,41 +533,52 @@ 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;
+	return -ENODATA;
 }
 
 /**
@@ -582,10 +594,54 @@ int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
 {
 	struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
 
-	return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
+	if (!data)
+		return -EINVAL;
+
+	return acpi_data_get_property_reference(data, name, index,
+		MAX_ACPI_REFERENCE_ARGS, args);
 }
 EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);
 
+/**
+ * acpi_node_get_property_reference_fixed_args - get a reference from property
+ * @fwnode: Firmware node to get the property from.
+ * @propname: Name of the property.
+ * @index: Index of the reference to return.
+ * @num_args: Expected number of arguments following each reference.
+ * @args: Location to store the returned reference with optional arguments.
+ *
+ * Using this function it is possible to leave "holes" in the property
+ * value set like in an 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.
+ */
+int acpi_node_get_property_reference_fixed_args(struct fwnode_handle *fwnode,
+	const char *name, size_t index, size_t num_args,
+	struct acpi_reference_args *args)
+{
+	struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
+
+	if (!data)
+		return -EINVAL;
+
+	return acpi_data_get_property_reference(data, name, index, num_args,
+						args);
+}
+EXPORT_SYMBOL_GPL(acpi_node_get_property_reference_fixed_args);
+
 static int acpi_data_prop_read_single(struct acpi_device_data *data,
 				      const char *propname,
 				      enum dev_prop_type proptype, void *val)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index c5eaf2f80a4c..c5d7c503843f 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -930,6 +930,9 @@ int acpi_dev_get_property(struct acpi_device *adev, const char *name,
 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_fixed_args(struct fwnode_handle *fwnode,
+				const char *name, size_t index, size_t num_args,
+				struct acpi_reference_args *args);
 
 int acpi_node_prop_get(struct fwnode_handle *fwnode, const char *propname,
 		       void **valptr);
-- 
2.9.3

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

* [PATCH 5/5] ACPI / gpio: Allow holes in list of GPIOs for a device
  2016-09-23 14:57 [PATCH 0/5] ACPI / gpio: Updates to properties Mika Westerberg
                   ` (3 preceding siblings ...)
  2016-09-23 14:57 ` [PATCH 4/5] ACPI / property: Allow holes in reference properties Mika Westerberg
@ 2016-09-23 14:57 ` Mika Westerberg
  2016-09-28 21:32 ` [PATCH 0/5] ACPI / gpio: Updates to properties Rafael J. Wysocki
  2016-10-19 12:41 ` Mika Westerberg
  6 siblings, 0 replies; 19+ messages in thread
From: Mika Westerberg @ 2016-09-23 14:57 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linus Walleij
  Cc: Alexandre Courbot, linux-acpi, linux-kernel, 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_fixed_args()
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..65c457b9541c 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_fixed_args(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


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

* Re: [PATCH 0/5] ACPI / gpio: Updates to properties
  2016-09-23 14:57 [PATCH 0/5] ACPI / gpio: Updates to properties Mika Westerberg
                   ` (4 preceding siblings ...)
  2016-09-23 14:57 ` [PATCH 5/5] ACPI / gpio: Allow holes in list of GPIOs for a device Mika Westerberg
@ 2016-09-28 21:32 ` Rafael J. Wysocki
  2016-09-29  7:00   ` Mika Westerberg
  2016-10-19 12:41 ` Mika Westerberg
  6 siblings, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2016-09-28 21:32 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

Hi,

On Fri, Sep 23, 2016 at 4:57 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).
>
> Mika Westerberg (5):
>   ACPI / documentation: Use recommended name in GPIO property names
>   ACPI / gpio: Add support for naming GPIOs
>   ACPI / gpio: Add hogging support
>   ACPI / property: Allow holes in reference properties
>   ACPI / gpio: Allow holes in list of GPIOs for a device
>
>  Documentation/acpi/gpio-properties.txt |  72 ++++++++++++++--
>  drivers/acpi/property.c                | 116 +++++++++++++++++++-------
>  drivers/gpio/gpiolib-acpi.c            | 147 ++++++++++++++++++++++++++++++---
>  include/linux/acpi.h                   |   3 +
>  4 files changed, 292 insertions(+), 46 deletions(-)

I would like to tackle patches [1/5] and [4/5] first, as they are core
patches and the rest is against gpiolib-acpi.

Moreover, the documentation patch [1/5] looks like immediately applicable to me.

Thanks,
Rafael

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

* Re: [PATCH 4/5] ACPI / property: Allow holes in reference properties
  2016-09-23 14:57 ` [PATCH 4/5] ACPI / property: Allow holes in reference properties Mika Westerberg
@ 2016-09-28 21:36   ` Rafael J. Wysocki
  2016-09-29  6:48     ` Mika Westerberg
  0 siblings, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2016-09-28 21:36 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Fri, Sep 23, 2016 at 4:57 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 add a new function acpi_node_get_property_reference_fixed_args() that
> takes number of expected arguments as parameter. Function returns -ENOENT
> if the corresponding index resolves to the "no entry" reference.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/acpi/property.c | 116 +++++++++++++++++++++++++++++++++++-------------
>  include/linux/acpi.h    |   3 ++
>  2 files changed, 89 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index f2fd3fee588a..b255c4442a05 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -472,6 +472,7 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
>   * @data: ACPI device data object containing the property
>   * @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
> @@ -485,8 +486,8 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
>   * 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)
> +       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;
> @@ -532,41 +533,52 @@ 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;
> +       return -ENODATA;
>  }
>
>  /**
> @@ -582,10 +594,54 @@ int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
>  {
>         struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
>
> -       return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
> +       if (!data)
> +               return -EINVAL;
> +
> +       return acpi_data_get_property_reference(data, name, index,
> +               MAX_ACPI_REFERENCE_ARGS, args);
>  }
>  EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);

gpiolib-acpi is the only user of this AFAICS, so why don't we change
the definition of acpi_node_get_property_reference() to take one extra
argument and update the caller to pass MAX_ACPI_REFERENCE_ARGS?

We'd avoid having to define the thing below then.

>
> +/**
> + * acpi_node_get_property_reference_fixed_args - get a reference from property
> + * @fwnode: Firmware node to get the property from.
> + * @propname: Name of the property.
> + * @index: Index of the reference to return.
> + * @num_args: Expected number of arguments following each reference.
> + * @args: Location to store the returned reference with optional arguments.
> + *
> + * Using this function it is possible to leave "holes" in the property
> + * value set like in an 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.
> + */
> +int acpi_node_get_property_reference_fixed_args(struct fwnode_handle *fwnode,
> +       const char *name, size_t index, size_t num_args,
> +       struct acpi_reference_args *args)
> +{
> +       struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
> +
> +       if (!data)
> +               return -EINVAL;
> +
> +       return acpi_data_get_property_reference(data, name, index, num_args,
> +                                               args);
> +}
> +EXPORT_SYMBOL_GPL(acpi_node_get_property_reference_fixed_args);
> +
>  static int acpi_data_prop_read_single(struct acpi_device_data *data,
>                                       const char *propname,
>                                       enum dev_prop_type proptype, void *val)
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index c5eaf2f80a4c..c5d7c503843f 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -930,6 +930,9 @@ int acpi_dev_get_property(struct acpi_device *adev, const char *name,
>  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_fixed_args(struct fwnode_handle *fwnode,
> +                               const char *name, size_t index, size_t num_args,
> +                               struct acpi_reference_args *args);
>
>  int acpi_node_prop_get(struct fwnode_handle *fwnode, const char *propname,
>                        void **valptr);
> --

Thanks,
Rafael

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

* Re: [PATCH 4/5] ACPI / property: Allow holes in reference properties
  2016-09-28 21:36   ` Rafael J. Wysocki
@ 2016-09-29  6:48     ` Mika Westerberg
  2016-09-29 11:53       ` Rafael J. Wysocki
  0 siblings, 1 reply; 19+ messages in thread
From: Mika Westerberg @ 2016-09-29  6:48 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Wed, Sep 28, 2016 at 11:36:10PM +0200, Rafael J. Wysocki wrote:
> On Fri, Sep 23, 2016 at 4:57 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 add a new function acpi_node_get_property_reference_fixed_args() that
> > takes number of expected arguments as parameter. Function returns -ENOENT
> > if the corresponding index resolves to the "no entry" reference.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/acpi/property.c | 116 +++++++++++++++++++++++++++++++++++-------------
> >  include/linux/acpi.h    |   3 ++
> >  2 files changed, 89 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index f2fd3fee588a..b255c4442a05 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -472,6 +472,7 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> >   * @data: ACPI device data object containing the property
> >   * @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
> > @@ -485,8 +486,8 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> >   * 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)
> > +       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;
> > @@ -532,41 +533,52 @@ 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;
> > +       return -ENODATA;
> >  }
> >
> >  /**
> > @@ -582,10 +594,54 @@ int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
> >  {
> >         struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
> >
> > -       return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
> > +       if (!data)
> > +               return -EINVAL;
> > +
> > +       return acpi_data_get_property_reference(data, name, index,
> > +               MAX_ACPI_REFERENCE_ARGS, args);
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);
> 
> gpiolib-acpi is the only user of this AFAICS, so why don't we change
> the definition of acpi_node_get_property_reference() to take one extra
> argument and update the caller to pass MAX_ACPI_REFERENCE_ARGS?

Unfortunately there are these (which I think should not call that
function directly anyway):

drivers/net/ethernet/apm/xgene/xgene_enet_hw.c: status = acpi_node_get_property_reference(fw_node, "phy-handle", 0,
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c:      rc = acpi_node_get_property_reference(
drivers/net/ethernet/hisilicon/hns/hns_enet.c:          ret = acpi_node_get_property_reference(dev->fwnode,

So we would need to update those as well. Not a big deal, though.

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

* Re: [PATCH 0/5] ACPI / gpio: Updates to properties
  2016-09-28 21:32 ` [PATCH 0/5] ACPI / gpio: Updates to properties Rafael J. Wysocki
@ 2016-09-29  7:00   ` Mika Westerberg
  2016-09-29 12:26     ` Rafael J. Wysocki
  0 siblings, 1 reply; 19+ messages in thread
From: Mika Westerberg @ 2016-09-29  7:00 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Wed, Sep 28, 2016 at 11:32:46PM +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> On Fri, Sep 23, 2016 at 4:57 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).
> >
> > Mika Westerberg (5):
> >   ACPI / documentation: Use recommended name in GPIO property names
> >   ACPI / gpio: Add support for naming GPIOs
> >   ACPI / gpio: Add hogging support
> >   ACPI / property: Allow holes in reference properties
> >   ACPI / gpio: Allow holes in list of GPIOs for a device
> >
> >  Documentation/acpi/gpio-properties.txt |  72 ++++++++++++++--
> >  drivers/acpi/property.c                | 116 +++++++++++++++++++-------
> >  drivers/gpio/gpiolib-acpi.c            | 147 ++++++++++++++++++++++++++++++---
> >  include/linux/acpi.h                   |   3 +
> >  4 files changed, 292 insertions(+), 46 deletions(-)
> 
> I would like to tackle patches [1/5] and [4/5] first, as they are core
> patches and the rest is against gpiolib-acpi.

Works for me.

I can re-arrange the series so that the two core patches (1,4/5) are
first and then the GPIO parts.

> Moreover, the documentation patch [1/5] looks like immediately applicable to me.

Yes, it just fixes the names to match what is preferred.

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

* Re: [PATCH 4/5] ACPI / property: Allow holes in reference properties
  2016-09-29  6:48     ` Mika Westerberg
@ 2016-09-29 11:53       ` Rafael J. Wysocki
  2016-09-29 11:56         ` Mika Westerberg
  0 siblings, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2016-09-29 11:53 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Thursday, September 29, 2016 09:48:29 AM Mika Westerberg wrote:
> On Wed, Sep 28, 2016 at 11:36:10PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Sep 23, 2016 at 4:57 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

One thing just occurred to me: what if this value is not 0?

> > >           ^GPIO, 20, 0, 0, // GPIO CS1
> > >       }
> > >   }
> > >
> > > The zero in the middle means "no entry" or NULL reference. To support this
> > > we add a new function acpi_node_get_property_reference_fixed_args() that
> > > takes number of expected arguments as parameter. Function returns -ENOENT
> > > if the corresponding index resolves to the "no entry" reference.
> > >
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/acpi/property.c | 116 +++++++++++++++++++++++++++++++++++-------------
> > >  include/linux/acpi.h    |   3 ++
> > >  2 files changed, 89 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index f2fd3fee588a..b255c4442a05 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -472,6 +472,7 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> > >   * @data: ACPI device data object containing the property
> > >   * @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
> > > @@ -485,8 +486,8 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> > >   * 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)
> > > +       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;
> > > @@ -532,41 +533,52 @@ 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;
> > > +       return -ENODATA;
> > >  }
> > >
> > >  /**
> > > @@ -582,10 +594,54 @@ int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
> > >  {
> > >         struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
> > >
> > > -       return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
> > > +       if (!data)
> > > +               return -EINVAL;
> > > +
> > > +       return acpi_data_get_property_reference(data, name, index,
> > > +               MAX_ACPI_REFERENCE_ARGS, args);
> > >  }
> > >  EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);
> > 
> > gpiolib-acpi is the only user of this AFAICS, so why don't we change
> > the definition of acpi_node_get_property_reference() to take one extra
> > argument and update the caller to pass MAX_ACPI_REFERENCE_ARGS?
> 
> Unfortunately there are these (which I think should not call that
> function directly anyway):

Right.

It looks like exactly the same usage pattern that should better be
implemented as a library function, ie. workarounds for something missing.

> drivers/net/ethernet/apm/xgene/xgene_enet_hw.c: status = acpi_node_get_property_reference(fw_node, "phy-handle", 0,
> drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c:      rc = acpi_node_get_property_reference(
> drivers/net/ethernet/hisilicon/hns/hns_enet.c:          ret = acpi_node_get_property_reference(dev->fwnode,
> 
> So we would need to update those as well. Not a big deal, though.

Or rename it to __acpi_node_get_property_reference() and define
acpi_node_get_property_reference() as a static inline wrapper passing
MAX_ACPI_REFERENCE_ARGS to it.

Thanks,
Rafael


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

* Re: [PATCH 4/5] ACPI / property: Allow holes in reference properties
  2016-09-29 11:53       ` Rafael J. Wysocki
@ 2016-09-29 11:56         ` Mika Westerberg
  2016-09-29 12:33           ` Rafael J. Wysocki
  0 siblings, 1 reply; 19+ messages in thread
From: Mika Westerberg @ 2016-09-29 11:56 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Thu, Sep 29, 2016 at 01:53:50PM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 29, 2016 09:48:29 AM Mika Westerberg wrote:
> > On Wed, Sep 28, 2016 at 11:36:10PM +0200, Rafael J. Wysocki wrote:
> > > On Fri, Sep 23, 2016 at 4:57 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
> 
> One thing just occurred to me: what if this value is not 0?

It can be any integer value actually. We allow it but 0 is the preferred
value. I did not find anything in DT saying what it should be either.

Alternatively I can add a check that it must be 0.

> > > >           ^GPIO, 20, 0, 0, // GPIO CS1
> > > >       }
> > > >   }
> > > >
> > > > The zero in the middle means "no entry" or NULL reference. To support this
> > > > we add a new function acpi_node_get_property_reference_fixed_args() that
> > > > takes number of expected arguments as parameter. Function returns -ENOENT
> > > > if the corresponding index resolves to the "no entry" reference.
> > > >
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > ---
> > > >  drivers/acpi/property.c | 116 +++++++++++++++++++++++++++++++++++-------------
> > > >  include/linux/acpi.h    |   3 ++
> > > >  2 files changed, 89 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > index f2fd3fee588a..b255c4442a05 100644
> > > > --- a/drivers/acpi/property.c
> > > > +++ b/drivers/acpi/property.c
> > > > @@ -472,6 +472,7 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> > > >   * @data: ACPI device data object containing the property
> > > >   * @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
> > > > @@ -485,8 +486,8 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> > > >   * 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)
> > > > +       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;
> > > > @@ -532,41 +533,52 @@ 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;
> > > > +       return -ENODATA;
> > > >  }
> > > >
> > > >  /**
> > > > @@ -582,10 +594,54 @@ int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
> > > >  {
> > > >         struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
> > > >
> > > > -       return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
> > > > +       if (!data)
> > > > +               return -EINVAL;
> > > > +
> > > > +       return acpi_data_get_property_reference(data, name, index,
> > > > +               MAX_ACPI_REFERENCE_ARGS, args);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);
> > > 
> > > gpiolib-acpi is the only user of this AFAICS, so why don't we change
> > > the definition of acpi_node_get_property_reference() to take one extra
> > > argument and update the caller to pass MAX_ACPI_REFERENCE_ARGS?
> > 
> > Unfortunately there are these (which I think should not call that
> > function directly anyway):
> 
> Right.
> 
> It looks like exactly the same usage pattern that should better be
> implemented as a library function, ie. workarounds for something missing.
> 
> > drivers/net/ethernet/apm/xgene/xgene_enet_hw.c: status = acpi_node_get_property_reference(fw_node, "phy-handle", 0,
> > drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c:      rc = acpi_node_get_property_reference(
> > drivers/net/ethernet/hisilicon/hns/hns_enet.c:          ret = acpi_node_get_property_reference(dev->fwnode,
> > 
> > So we would need to update those as well. Not a big deal, though.
> 
> Or rename it to __acpi_node_get_property_reference() and define
> acpi_node_get_property_reference() as a static inline wrapper passing
> MAX_ACPI_REFERENCE_ARGS to it.

OK, I'll do this.

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

* Re: [PATCH 0/5] ACPI / gpio: Updates to properties
  2016-09-29  7:00   ` Mika Westerberg
@ 2016-09-29 12:26     ` Rafael J. Wysocki
  0 siblings, 0 replies; 19+ messages in thread
From: Rafael J. Wysocki @ 2016-09-29 12:26 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Thursday, September 29, 2016 10:00:15 AM Mika Westerberg wrote:
> On Wed, Sep 28, 2016 at 11:32:46PM +0200, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Fri, Sep 23, 2016 at 4:57 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).
> > >
> > > Mika Westerberg (5):
> > >   ACPI / documentation: Use recommended name in GPIO property names
> > >   ACPI / gpio: Add support for naming GPIOs
> > >   ACPI / gpio: Add hogging support
> > >   ACPI / property: Allow holes in reference properties
> > >   ACPI / gpio: Allow holes in list of GPIOs for a device
> > >
> > >  Documentation/acpi/gpio-properties.txt |  72 ++++++++++++++--
> > >  drivers/acpi/property.c                | 116 +++++++++++++++++++-------
> > >  drivers/gpio/gpiolib-acpi.c            | 147 ++++++++++++++++++++++++++++++---
> > >  include/linux/acpi.h                   |   3 +
> > >  4 files changed, 292 insertions(+), 46 deletions(-)
> > 
> > I would like to tackle patches [1/5] and [4/5] first, as they are core
> > patches and the rest is against gpiolib-acpi.
> 
> Works for me.
> 
> I can re-arrange the series so that the two core patches (1,4/5) are
> first and then the GPIO parts.
> 
> > Moreover, the documentation patch [1/5] looks like immediately applicable to me.
> 
> Yes, it just fixes the names to match what is preferred.

OK, I'll apply it then, no need to resend.

Thanks,
Rafael


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

* Re: [PATCH 4/5] ACPI / property: Allow holes in reference properties
  2016-09-29 11:56         ` Mika Westerberg
@ 2016-09-29 12:33           ` Rafael J. Wysocki
  0 siblings, 0 replies; 19+ messages in thread
From: Rafael J. Wysocki @ 2016-09-29 12:33 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Thursday, September 29, 2016 02:56:57 PM Mika Westerberg wrote:
> On Thu, Sep 29, 2016 at 01:53:50PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, September 29, 2016 09:48:29 AM Mika Westerberg wrote:
> > > On Wed, Sep 28, 2016 at 11:36:10PM +0200, Rafael J. Wysocki wrote:
> > > > On Fri, Sep 23, 2016 at 4:57 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
> > 
> > One thing just occurred to me: what if this value is not 0?
> 
> It can be any integer value actually. We allow it but 0 is the preferred
> value. I did not find anything in DT saying what it should be either.
> 
> Alternatively I can add a check that it must be 0.

No need, I just was wondering. :-)

I think it's best to follow DT exactly here.

> > > > >           ^GPIO, 20, 0, 0, // GPIO CS1
> > > > >       }
> > > > >   }
> > > > >
> > > > > The zero in the middle means "no entry" or NULL reference. To support this
> > > > > we add a new function acpi_node_get_property_reference_fixed_args() that
> > > > > takes number of expected arguments as parameter. Function returns -ENOENT
> > > > > if the corresponding index resolves to the "no entry" reference.
> > > > >
> > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > ---
> > > > >  drivers/acpi/property.c | 116 +++++++++++++++++++++++++++++++++++-------------
> > > > >  include/linux/acpi.h    |   3 ++
> > > > >  2 files changed, 89 insertions(+), 30 deletions(-)
> > > > >
> > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > index f2fd3fee588a..b255c4442a05 100644
> > > > > --- a/drivers/acpi/property.c
> > > > > +++ b/drivers/acpi/property.c
> > > > > @@ -472,6 +472,7 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> > > > >   * @data: ACPI device data object containing the property
> > > > >   * @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
> > > > > @@ -485,8 +486,8 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> > > > >   * 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)
> > > > > +       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;
> > > > > @@ -532,41 +533,52 @@ 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;
> > > > > +       return -ENODATA;
> > > > >  }
> > > > >
> > > > >  /**
> > > > > @@ -582,10 +594,54 @@ int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
> > > > >  {
> > > > >         struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
> > > > >
> > > > > -       return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
> > > > > +       if (!data)
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       return acpi_data_get_property_reference(data, name, index,
> > > > > +               MAX_ACPI_REFERENCE_ARGS, args);
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);
> > > > 
> > > > gpiolib-acpi is the only user of this AFAICS, so why don't we change
> > > > the definition of acpi_node_get_property_reference() to take one extra
> > > > argument and update the caller to pass MAX_ACPI_REFERENCE_ARGS?
> > > 
> > > Unfortunately there are these (which I think should not call that
> > > function directly anyway):
> > 
> > Right.
> > 
> > It looks like exactly the same usage pattern that should better be
> > implemented as a library function, ie. workarounds for something missing.
> > 
> > > drivers/net/ethernet/apm/xgene/xgene_enet_hw.c: status = acpi_node_get_property_reference(fw_node, "phy-handle", 0,
> > > drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c:      rc = acpi_node_get_property_reference(
> > > drivers/net/ethernet/hisilicon/hns/hns_enet.c:          ret = acpi_node_get_property_reference(dev->fwnode,
> > > 
> > > So we would need to update those as well. Not a big deal, though.
> > 
> > Or rename it to __acpi_node_get_property_reference() and define
> > acpi_node_get_property_reference() as a static inline wrapper passing
> > MAX_ACPI_REFERENCE_ARGS to it.
> 
> OK, I'll do this.

Thanks!

Rafael


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

* Re: [PATCH 0/5] ACPI / gpio: Updates to properties
  2016-09-23 14:57 [PATCH 0/5] ACPI / gpio: Updates to properties Mika Westerberg
                   ` (5 preceding siblings ...)
  2016-09-28 21:32 ` [PATCH 0/5] ACPI / gpio: Updates to properties Rafael J. Wysocki
@ 2016-10-19 12:41 ` Mika Westerberg
  2016-10-19 21:22   ` Rafael J. Wysocki
  6 siblings, 1 reply; 19+ messages in thread
From: Mika Westerberg @ 2016-10-19 12:41 UTC (permalink / raw)
  To: Rafael J. Wysocki, Linus Walleij
  Cc: Alexandre Courbot, linux-acpi, linux-kernel

On Fri, Sep 23, 2016 at 05:57:05PM +0300, Mika Westerberg 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).
> 
> Mika Westerberg (5):
>   ACPI / documentation: Use recommended name in GPIO property names
>   ACPI / gpio: Add support for naming GPIOs
>   ACPI / gpio: Add hogging support
>   ACPI / property: Allow holes in reference properties
>   ACPI / gpio: Allow holes in list of GPIOs for a device

Any comments on the GPIO patches in this series?

At least the last one does not add any new bindings so it should be fine
to apply.

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

* Re: [PATCH 0/5] ACPI / gpio: Updates to properties
  2016-10-19 12:41 ` Mika Westerberg
@ 2016-10-19 21:22   ` Rafael J. Wysocki
  2016-10-20  7:46     ` Mika Westerberg
  2016-10-20 12:17     ` Linus Walleij
  0 siblings, 2 replies; 19+ messages in thread
From: Rafael J. Wysocki @ 2016-10-19 21:22 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Wed, Oct 19, 2016 at 2:41 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Sep 23, 2016 at 05:57:05PM +0300, Mika Westerberg 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).
>>
>> Mika Westerberg (5):
>>   ACPI / documentation: Use recommended name in GPIO property names
>>   ACPI / gpio: Add support for naming GPIOs
>>   ACPI / gpio: Add hogging support
>>   ACPI / property: Allow holes in reference properties
>>   ACPI / gpio: Allow holes in list of GPIOs for a device
>
> Any comments on the GPIO patches in this series?
>
> At least the last one does not add any new bindings so it should be fine
> to apply.

I have no comments, but I'd suggest resending them as a new series on
top of 4.9-rc1 (which includes the generic patches from this series).

You can add my ACK to them if that helps.

Thanks,
Rafael

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

* Re: [PATCH 0/5] ACPI / gpio: Updates to properties
  2016-10-19 21:22   ` Rafael J. Wysocki
@ 2016-10-20  7:46     ` Mika Westerberg
  2016-10-20 12:17     ` Linus Walleij
  1 sibling, 0 replies; 19+ messages in thread
From: Mika Westerberg @ 2016-10-20  7:46 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Linus Walleij, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Wed, Oct 19, 2016 at 11:22:03PM +0200, Rafael J. Wysocki wrote:
> On Wed, Oct 19, 2016 at 2:41 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Fri, Sep 23, 2016 at 05:57:05PM +0300, Mika Westerberg 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).
> >>
> >> Mika Westerberg (5):
> >>   ACPI / documentation: Use recommended name in GPIO property names
> >>   ACPI / gpio: Add support for naming GPIOs
> >>   ACPI / gpio: Add hogging support
> >>   ACPI / property: Allow holes in reference properties
> >>   ACPI / gpio: Allow holes in list of GPIOs for a device
> >
> > Any comments on the GPIO patches in this series?
> >
> > At least the last one does not add any new bindings so it should be fine
> > to apply.
> 
> I have no comments, but I'd suggest resending them as a new series on
> top of 4.9-rc1 (which includes the generic patches from this series).
> 
> You can add my ACK to them if that helps.

OK, I will do that. Thanks!

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

* Re: [PATCH 0/5] ACPI / gpio: Updates to properties
  2016-10-19 21:22   ` Rafael J. Wysocki
  2016-10-20  7:46     ` Mika Westerberg
@ 2016-10-20 12:17     ` Linus Walleij
  2016-10-20 12:35       ` Mika Westerberg
  1 sibling, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2016-10-20 12:17 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Mika Westerberg, Rafael J. Wysocki, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Wed, Oct 19, 2016 at 11:22 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Oct 19, 2016 at 2:41 PM, Mika Westerberg

>> Any comments on the GPIO patches in this series?
>>
>> At least the last one does not add any new bindings so it should be fine
>> to apply.
>
> I have no comments, but I'd suggest resending them as a new series on
> top of 4.9-rc1 (which includes the generic patches from this series).
>
> You can add my ACK to them if that helps.

OK I had some comments so please adress these but resend on top
of v4.9-rc1 indeed. If they are already higher up in my mailbox then
look into my comments and post again, hehe.

Thanks!

Yours,
Linus Walleij

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

* Re: [PATCH 0/5] ACPI / gpio: Updates to properties
  2016-10-20 12:17     ` Linus Walleij
@ 2016-10-20 12:35       ` Mika Westerberg
  0 siblings, 0 replies; 19+ messages in thread
From: Mika Westerberg @ 2016-10-20 12:35 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Alexandre Courbot,
	ACPI Devel Maling List, Linux Kernel Mailing List

On Thu, Oct 20, 2016 at 02:17:40PM +0200, Linus Walleij wrote:
> On Wed, Oct 19, 2016 at 11:22 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Oct 19, 2016 at 2:41 PM, Mika Westerberg
> 
> >> Any comments on the GPIO patches in this series?
> >>
> >> At least the last one does not add any new bindings so it should be fine
> >> to apply.
> >
> > I have no comments, but I'd suggest resending them as a new series on
> > top of 4.9-rc1 (which includes the generic patches from this series).
> >
> > You can add my ACK to them if that helps.
> 
> OK I had some comments so please adress these but resend on top
> of v4.9-rc1 indeed. If they are already higher up in my mailbox then
> look into my comments and post again, hehe.

I will, thanks!

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

end of thread, other threads:[~2016-10-20 12:35 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-23 14:57 [PATCH 0/5] ACPI / gpio: Updates to properties Mika Westerberg
2016-09-23 14:57 ` [PATCH 1/5] ACPI / documentation: Use recommended name in GPIO property names Mika Westerberg
2016-09-23 14:57 ` [PATCH 2/5] ACPI / gpio: Add support for naming GPIOs Mika Westerberg
2016-09-23 14:57 ` [PATCH 3/5] ACPI / gpio: Add hogging support Mika Westerberg
2016-09-23 14:57 ` [PATCH 4/5] ACPI / property: Allow holes in reference properties Mika Westerberg
2016-09-28 21:36   ` Rafael J. Wysocki
2016-09-29  6:48     ` Mika Westerberg
2016-09-29 11:53       ` Rafael J. Wysocki
2016-09-29 11:56         ` Mika Westerberg
2016-09-29 12:33           ` Rafael J. Wysocki
2016-09-23 14:57 ` [PATCH 5/5] ACPI / gpio: Allow holes in list of GPIOs for a device Mika Westerberg
2016-09-28 21:32 ` [PATCH 0/5] ACPI / gpio: Updates to properties Rafael J. Wysocki
2016-09-29  7:00   ` Mika Westerberg
2016-09-29 12:26     ` Rafael J. Wysocki
2016-10-19 12:41 ` Mika Westerberg
2016-10-19 21:22   ` Rafael J. Wysocki
2016-10-20  7:46     ` Mika Westerberg
2016-10-20 12:17     ` Linus Walleij
2016-10-20 12:35       ` Mika Westerberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.