[PATCH v3 2/4] gpiolib: add gpiod_get_array and gpiod_put_array functions

[PATCH v3 2/4] gpiolib: add gpiod_get_array and gpiod_put_array functions

[Rojhalat Ibrahim]

Introduce new functions for conveniently obtaining and disposing of an entire
array of GPIOs with one function call.

Suggested-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
---
Change log:
  v3: - rebase on current linux-gpio devel branch
      - fix ACPI GPIO counting
      - allow for zero-sized arrays
      - make the flags argument mandatory for the new functions
      - clarify documentation
  v2: change interface

 Documentation/gpio/consumer.txt |  33 +++++-
 drivers/gpio/gpiolib.c          | 231 ++++++++++++++++++++++++++++++++++++++++
 include/linux/gpio/consumer.h   |  46 ++++++++
 3 files changed, 307 insertions(+), 3 deletions(-)

diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index d85fbae..85a7e30 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -58,7 +58,6 @@ pattern where a GPIO is optional, the gpiod_get_optional() and
 gpiod_get_index_optional() functions can be used. These functions return NULL
 instead of -ENOENT if no GPIO has been assigned to the requested function:
 
-
 	struct gpio_desc *gpiod_get_optional(struct device *dev,
 					     const char *con_id,
 					     enum gpiod_flags flags)
@@ -68,6 +67,27 @@ instead of -ENOENT if no GPIO has been assigned to the requested function:
 						   unsigned int index,
 						   enum gpiod_flags flags)
 
+For a function using multiple GPIOs all of those can be obtained with one call:
+
+	struct gpio_descs *gpiod_get_array(struct device *dev,
+					   const char *con_id,
+					   enum gpiod_flags flags)
+
+This function returns a struct gpio_descs which contains an array of
+descriptors:
+
+	struct gpio_descs {
+		unsigned int array_size;
+		struct gpio_desc *desc_array[];
+	}
+
+The following function returns NULL instead of -ENOENT if no GPIOs have been
+assigned to the requested function:
+
+	struct gpio_descs *gpiod_get_array_optional(struct device *dev,
+						    const char *con_id,
+						    enum gpiod_flags flags)
+
 Device-managed variants of these functions are also defined:
 
 	struct gpio_desc *devm_gpiod_get(struct device *dev, const char *con_id,
@@ -91,8 +111,15 @@ A GPIO descriptor can be disposed of using the gpiod_put() function:
 
 	void gpiod_put(struct gpio_desc *desc)
 
-It is strictly forbidden to use a descriptor after calling this function. The
-device-managed variant is, unsurprisingly:
+For an array of GPIOs this function can be used:
+
+	void gpiod_put_array(struct gpio_descs *descs)
+
+It is strictly forbidden to use a descriptor after calling these functions.
+It is also not allowed to individually release descriptors (using gpiod_put())
+from an array acquired with gpiod_get_array().
+
+The device-managed variant is, unsurprisingly:
 
 	void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ff2b80d..d4153e0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1804,6 +1804,158 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 	return desc;
 }
 
+static int dt_gpio_count(struct device *dev, const char *con_id)
+{
+	int ret;
+	char propname[32];
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
+		if (con_id)
+			snprintf(propname, sizeof(propname), "%s-%s",
+				 con_id, gpio_suffixes[i]);
+		else
+			snprintf(propname, sizeof(propname), "%s",
+				 gpio_suffixes[i]);
+
+		ret = of_gpio_named_count(dev->of_node, propname);
+		if (ret >= 0)
+			break;
+	}
+	return ret;
+}
+
+#ifdef CONFIG_ACPI
+
+static unsigned 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)
+			count++;
+
+		element++;
+	}
+	return count;
+}
+
+static int acpi_find_gpio_count(struct acpi_resource *ares, void *data)
+{
+	unsigned int *count = data;
+
+	if (ares->type == ACPI_RESOURCE_TYPE_GPIO)
+		*count += ares->data.gpio.pin_table_length;
+
+	return 1;
+}
+
+static int acpi_gpio_count(struct device *dev, const char *con_id)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	const union acpi_object *obj;
+	const struct acpi_gpio_mapping *gm;
+	int count = -ENOENT;
+	int ret;
+	char propname[32];
+	unsigned int i;
+
+	/* Try first from _DSD */
+	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
+		if (con_id && strcmp(con_id, "gpios"))
+			snprintf(propname, sizeof(propname), "%s-%s",
+				 con_id, gpio_suffixes[i]);
+		else
+			snprintf(propname, sizeof(propname), "%s",
+				 gpio_suffixes[i]);
+
+		ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_ANY,
+					    &obj);
+		if (ret == 0) {
+			if (obj->type == ACPI_TYPE_LOCAL_REFERENCE)
+				count = 1;
+			else if (obj->type == ACPI_TYPE_PACKAGE)
+				count = acpi_gpio_package_count(obj);
+		} else if (adev->driver_gpios) {
+			for (gm = adev->driver_gpios; gm->name; gm++)
+				if (strcmp(propname, gm->name) == 0) {
+					count = gm->size;
+					break;
+				}
+		}
+		if (count >= 0)
+			break;
+	}
+
+	/* Then from plain _CRS GPIOs */
+	if (count < 0) {
+		struct list_head resource_list;
+		unsigned int crs_count = 0;
+
+		INIT_LIST_HEAD(&resource_list);
+		acpi_dev_get_resources(adev, &resource_list,
+				       acpi_find_gpio_count, &crs_count);
+		acpi_dev_free_resource_list(&resource_list);
+		if (crs_count > 0)
+			count = crs_count;
+	}
+	return count;
+}
+
+#else
+
+static int acpi_gpio_count(struct device *dev, const char *con_id)
+{
+	return -ENODEV;
+}
+
+#endif
+
+static int lut_gpio_count(struct device *dev, const char *con_id)
+{
+	struct gpiod_lookup_table *table;
+	struct gpiod_lookup *p;
+	unsigned int count = 0;
+
+	table = gpiod_find_lookup_table(dev);
+	if (!table)
+		return -ENOENT;
+
+	for (p = &table->table[0]; p->chip_label; p++) {
+		if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) ||
+		    (!con_id && !p->con_id))
+			count++;
+	}
+	if (!count)
+		return -ENOENT;
+
+	return count;
+}
+
+/**
+ * gpiod_count - return the number of GPIOs associated with a device / function
+ *		or -ENOENT if no GPIO has been assigned to the requested function
+ * @dev:	GPIO consumer, can be NULL for system-global GPIOs
+ * @con_id:	function within the GPIO consumer
+ */
+int gpiod_count(struct device *dev, const char *con_id)
+{
+	int count = -ENOENT;
+
+	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+		count = dt_gpio_count(dev, con_id);
+	else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev))
+		count = acpi_gpio_count(dev, con_id);
+
+	if (count < 0)
+		count = lut_gpio_count(dev, con_id);
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(gpiod_count);
+
 /**
  * gpiod_get - obtain a GPIO for a given GPIO function
  * @dev:	GPIO consumer, can be NULL for system-global GPIOs
@@ -2005,6 +2157,70 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
 EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 
 /**
+ * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
+ * @dev:	GPIO consumer, can be NULL for system-global GPIOs
+ * @con_id:	function within the GPIO consumer
+ * @flags:	optional GPIO initialization flags
+ *
+ * This function acquires all the GPIOs defined under a given function.
+ *
+ * Return a struct gpio_descs containing an array of descriptors, -ENOENT if
+ * no GPIO has been assigned to the requested function, or another IS_ERR()
+ * code if an error occurred while trying to acquire the GPIOs.
+ */
+struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
+						const char *con_id,
+						enum gpiod_flags flags)
+{
+	struct gpio_desc *desc;
+	struct gpio_descs *descs;
+	int count;
+
+	count = gpiod_count(dev, con_id);
+	if (count < 0)
+		return ERR_PTR(count);
+
+	descs = kzalloc(sizeof(*descs) + sizeof(descs->desc_array[0]) * count,
+			GFP_KERNEL);
+
+	for (descs->array_size = 0; descs->array_size < count; ) {
+		desc = gpiod_get_index(dev, con_id, descs->array_size, flags);
+		if (IS_ERR(desc)) {
+			gpiod_put_array(descs);
+			return ERR_PTR(PTR_ERR(desc));
+		}
+		descs->desc_array[descs->array_size] = desc;
+		descs->array_size++;
+	}
+	return descs;
+}
+EXPORT_SYMBOL_GPL(gpiod_get_array);
+
+/**
+ * gpiod_get_array_optional - obtain multiple GPIOs from a multi-index GPIO
+ *                            function
+ * @dev:	GPIO consumer, can be NULL for system-global GPIOs
+ * @con_id:	function within the GPIO consumer
+ * @flags:	optional GPIO initialization flags
+ *
+ * This is equivalent to gpiod_get_array(), except that when no GPIO was
+ * assigned to the requested function it will return NULL.
+ */
+struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
+							const char *con_id,
+							enum gpiod_flags flags)
+{
+	struct gpio_descs *descs;
+
+	descs = gpiod_get_array(dev, con_id, flags);
+	if (IS_ERR(descs) && (PTR_ERR(descs) == -ENOENT))
+		return NULL;
+
+	return descs;
+}
+EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
+
+/**
  * gpiod_put - dispose of a GPIO descriptor
  * @desc:	GPIO descriptor to dispose of
  *
@@ -2016,6 +2232,21 @@ void gpiod_put(struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_put);
 
+/**
+ * gpiod_put_array - dispose of multiple GPIO descriptors
+ * @descs:	struct gpio_descs containing an array of descriptors
+ */
+void gpiod_put_array(struct gpio_descs *descs)
+{
+	unsigned int i;
+
+	for (i = 0; i < descs->array_size; i++)
+		gpiod_put(descs->desc_array[i]);
+
+	kfree(descs);
+}
+EXPORT_SYMBOL_GPL(gpiod_put_array);
+
 #ifdef CONFIG_DEBUG_FS
 
 static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 45afc2d..6b1c389 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -16,6 +16,15 @@ struct device;
  */
 struct gpio_desc;
 
+/**
+ * Struct containing an array of descriptors that can be obtained using
+ * gpiod_get_array().
+ */
+struct gpio_descs {
+	unsigned int array_size;
+	struct gpio_desc *desc_array[];
+};
+
 #define GPIOD_FLAGS_BIT_DIR_SET		BIT(0)
 #define GPIOD_FLAGS_BIT_DIR_OUT		BIT(1)
 #define GPIOD_FLAGS_BIT_DIR_VAL		BIT(2)
@@ -34,6 +43,9 @@ enum gpiod_flags {
 
 #ifdef CONFIG_GPIOLIB
 
+/* Return the number of GPIOs associated with a device / function */
+int gpiod_count(struct device *dev, const char *con_id);
+
 /* Acquire and dispose GPIOs */
 struct gpio_desc *__must_check __gpiod_get(struct device *dev,
 					 const char *con_id,
@@ -49,7 +61,14 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
 							const char *con_id,
 							unsigned int index,
 							enum gpiod_flags flags);
+struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
+						const char *con_id,
+						enum gpiod_flags flags);
+struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
+							const char *con_id,
+							enum gpiod_flags flags);
 void gpiod_put(struct gpio_desc *desc);
+void gpiod_put_array(struct gpio_descs *descs);
 
 struct gpio_desc *__must_check __devm_gpiod_get(struct device *dev,
 					      const char *con_id,
@@ -113,6 +132,11 @@ struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
 					    struct fwnode_handle *child);
 #else /* CONFIG_GPIOLIB */
 
+static inline int gpiod_count(struct device *dev, const char *con_id)
+{
+	return 0;
+}
+
 static inline struct gpio_desc *__must_check __gpiod_get(struct device *dev,
 						const char *con_id,
 						enum gpiod_flags flags)
@@ -142,6 +166,20 @@ __gpiod_get_index_optional(struct device *dev, const char *con_id,
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct gpio_descs *__must_check
+gpiod_get_array(struct device *dev, const char *con_id,
+		enum gpiod_flags flags)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline struct gpio_descs *__must_check
+gpiod_get_array_optional(struct device *dev, const char *con_id,
+			 enum gpiod_flags flags)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline void gpiod_put(struct gpio_desc *desc)
 {
 	might_sleep();
@@ -150,6 +188,14 @@ static inline void gpiod_put(struct gpio_desc *desc)
 	WARN_ON(1);
 }
 
+static inline void gpiod_put_array(struct gpio_descs *descs)
+{
+	might_sleep();
+
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+}
+
 static inline struct gpio_desc *__must_check
 __devm_gpiod_get(struct device *dev,
 		 const char *con_id,
-- 
2.0.5

Re: [PATCH v3 2/4] gpiolib: add gpiod_get_array and gpiod_put_array functions

[Alexandre Courbot]

On Tue, Feb 10, 2015 at 2:07 AM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> Introduce new functions for conveniently obtaining and disposing of an entire
> array of GPIOs with one function call.
>
> Suggested-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
> ---
> Change log:
>   v3: - rebase on current linux-gpio devel branch
>       - fix ACPI GPIO counting
>       - allow for zero-sized arrays
>       - make the flags argument mandatory for the new functions
>       - clarify documentation
>   v2: change interface
>
>  Documentation/gpio/consumer.txt |  33 +++++-
>  drivers/gpio/gpiolib.c          | 231 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/gpio/consumer.h   |  46 ++++++++
>  3 files changed, 307 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> index d85fbae..85a7e30 100644
> --- a/Documentation/gpio/consumer.txt
> +++ b/Documentation/gpio/consumer.txt
> @@ -58,7 +58,6 @@ pattern where a GPIO is optional, the gpiod_get_optional() and
>  gpiod_get_index_optional() functions can be used. These functions return NULL
>  instead of -ENOENT if no GPIO has been assigned to the requested function:
>
> -
>         struct gpio_desc *gpiod_get_optional(struct device *dev,
>                                              const char *con_id,
>                                              enum gpiod_flags flags)
> @@ -68,6 +67,27 @@ instead of -ENOENT if no GPIO has been assigned to the requested function:
>                                                    unsigned int index,
>                                                    enum gpiod_flags flags)
>
> +For a function using multiple GPIOs all of those can be obtained with one call:
> +
> +       struct gpio_descs *gpiod_get_array(struct device *dev,
> +                                          const char *con_id,
> +                                          enum gpiod_flags flags)
> +
> +This function returns a struct gpio_descs which contains an array of
> +descriptors:
> +
> +       struct gpio_descs {
> +               unsigned int array_size;
> +               struct gpio_desc *desc_array[];

I missed this in the previous revision, but how about having shorter
names for these members? Like

array_size -> ndescs
desc_array -> desc

These would be more comfortable to work with I believe.

> +       }
> +
> +The following function returns NULL instead of -ENOENT if no GPIOs have been
> +assigned to the requested function:
> +
> +       struct gpio_descs *gpiod_get_array_optional(struct device *dev,
> +                                                   const char *con_id,
> +                                                   enum gpiod_flags flags)
> +
>  Device-managed variants of these functions are also defined:
>
>         struct gpio_desc *devm_gpiod_get(struct device *dev, const char *con_id,
> @@ -91,8 +111,15 @@ A GPIO descriptor can be disposed of using the gpiod_put() function:
>
>         void gpiod_put(struct gpio_desc *desc)
>
> -It is strictly forbidden to use a descriptor after calling this function. The
> -device-managed variant is, unsurprisingly:
> +For an array of GPIOs this function can be used:
> +
> +       void gpiod_put_array(struct gpio_descs *descs)
> +
> +It is strictly forbidden to use a descriptor after calling these functions.
> +It is also not allowed to individually release descriptors (using gpiod_put())
> +from an array acquired with gpiod_get_array().
> +
> +The device-managed variant is, unsurprisingly:
>
>         void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index ff2b80d..d4153e0 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1804,6 +1804,158 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>         return desc;
>  }
>
> +static int dt_gpio_count(struct device *dev, const char *con_id)
> +{
> +       int ret;
> +       char propname[32];
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> +               if (con_id)
> +                       snprintf(propname, sizeof(propname), "%s-%s",
> +                                con_id, gpio_suffixes[i]);
> +               else
> +                       snprintf(propname, sizeof(propname), "%s",
> +                                gpio_suffixes[i]);
> +
> +               ret = of_gpio_named_count(dev->of_node, propname);
> +               if (ret >= 0)
> +                       break;
> +       }
> +       return ret;
> +}
> +
> +#ifdef CONFIG_ACPI
> +
> +static unsigned 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)
> +                       count++;
> +
> +               element++;
> +       }
> +       return count;
> +}
> +
> +static int acpi_find_gpio_count(struct acpi_resource *ares, void *data)
> +{
> +       unsigned int *count = data;
> +
> +       if (ares->type == ACPI_RESOURCE_TYPE_GPIO)
> +               *count += ares->data.gpio.pin_table_length;
> +
> +       return 1;
> +}
> +
> +static int acpi_gpio_count(struct device *dev, const char *con_id)
> +{
> +       struct acpi_device *adev = ACPI_COMPANION(dev);
> +       const union acpi_object *obj;
> +       const struct acpi_gpio_mapping *gm;
> +       int count = -ENOENT;
> +       int ret;
> +       char propname[32];
> +       unsigned int i;
> +
> +       /* Try first from _DSD */
> +       for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> +               if (con_id && strcmp(con_id, "gpios"))
> +                       snprintf(propname, sizeof(propname), "%s-%s",
> +                                con_id, gpio_suffixes[i]);
> +               else
> +                       snprintf(propname, sizeof(propname), "%s",
> +                                gpio_suffixes[i]);
> +
> +               ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_ANY,
> +                                           &obj);
> +               if (ret == 0) {
> +                       if (obj->type == ACPI_TYPE_LOCAL_REFERENCE)
> +                               count = 1;
> +                       else if (obj->type == ACPI_TYPE_PACKAGE)
> +                               count = acpi_gpio_package_count(obj);
> +               } else if (adev->driver_gpios) {
> +                       for (gm = adev->driver_gpios; gm->name; gm++)
> +                               if (strcmp(propname, gm->name) == 0) {
> +                                       count = gm->size;
> +                                       break;
> +                               }
> +               }
> +               if (count >= 0)
> +                       break;
> +       }
> +
> +       /* Then from plain _CRS GPIOs */
> +       if (count < 0) {
> +               struct list_head resource_list;
> +               unsigned int crs_count = 0;
> +
> +               INIT_LIST_HEAD(&resource_list);
> +               acpi_dev_get_resources(adev, &resource_list,
> +                                      acpi_find_gpio_count, &crs_count);
> +               acpi_dev_free_resource_list(&resource_list);
> +               if (crs_count > 0)
> +                       count = crs_count;
> +       }
> +       return count;
> +}

Mika, does the ACPI part look good to you?

> +
> +#else
> +
> +static int acpi_gpio_count(struct device *dev, const char *con_id)
> +{
> +       return -ENODEV;
> +}
> +
> +#endif
> +
> +static int lut_gpio_count(struct device *dev, const char *con_id)

I did not reply to your previous comment, but I'd prefer if this was
named platform_gpio_count, for clarity.

These are my last nits - after they are fixed, please feel free to add my

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

to the next revision.

Re: [PATCH v3 2/4] gpiolib: add gpiod_get_array and gpiod_put_array functions

[Mika Westerberg]

[+Rafael]

On Mon, Feb 09, 2015 at 06:07:10PM +0100, Rojhalat Ibrahim wrote:
> Introduce new functions for conveniently obtaining and disposing of an entire
> array of GPIOs with one function call.
> 
> Suggested-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>

Can you please next time CC me the whole series? Now it fails with:

drivers/gpio/gpiolib.c: In function ‘dt_gpio_count’:
drivers/gpio/gpiolib.c:1812:29: error: ‘gpio_suffixes’ undeclared (first
use in this function)
  for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
                             ^
include/linux/kernel.h:54:33: note: in definition of macro ‘ARRAY_SIZE’
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))

and so on.

> ---
> Change log:
>   v3: - rebase on current linux-gpio devel branch
>       - fix ACPI GPIO counting
>       - allow for zero-sized arrays
>       - make the flags argument mandatory for the new functions
>       - clarify documentation
>   v2: change interface
> 
>  Documentation/gpio/consumer.txt |  33 +++++-
>  drivers/gpio/gpiolib.c          | 231 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/gpio/consumer.h   |  46 ++++++++
>  3 files changed, 307 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> index d85fbae..85a7e30 100644
> --- a/Documentation/gpio/consumer.txt
> +++ b/Documentation/gpio/consumer.txt
> @@ -58,7 +58,6 @@ pattern where a GPIO is optional, the gpiod_get_optional() and
>  gpiod_get_index_optional() functions can be used. These functions return NULL
>  instead of -ENOENT if no GPIO has been assigned to the requested function:
>  
> -
>  	struct gpio_desc *gpiod_get_optional(struct device *dev,
>  					     const char *con_id,
>  					     enum gpiod_flags flags)
> @@ -68,6 +67,27 @@ instead of -ENOENT if no GPIO has been assigned to the requested function:
>  						   unsigned int index,
>  						   enum gpiod_flags flags)
>  
> +For a function using multiple GPIOs all of those can be obtained with one call:
> +
> +	struct gpio_descs *gpiod_get_array(struct device *dev,
> +					   const char *con_id,
> +					   enum gpiod_flags flags)
> +
> +This function returns a struct gpio_descs which contains an array of
> +descriptors:
> +
> +	struct gpio_descs {
> +		unsigned int array_size;
> +		struct gpio_desc *desc_array[];
> +	}
> +
> +The following function returns NULL instead of -ENOENT if no GPIOs have been
> +assigned to the requested function:
> +
> +	struct gpio_descs *gpiod_get_array_optional(struct device *dev,
> +						    const char *con_id,
> +						    enum gpiod_flags flags)
> +
>  Device-managed variants of these functions are also defined:
>  
>  	struct gpio_desc *devm_gpiod_get(struct device *dev, const char *con_id,
> @@ -91,8 +111,15 @@ A GPIO descriptor can be disposed of using the gpiod_put() function:
>  
>  	void gpiod_put(struct gpio_desc *desc)
>  
> -It is strictly forbidden to use a descriptor after calling this function. The
> -device-managed variant is, unsurprisingly:
> +For an array of GPIOs this function can be used:
> +
> +	void gpiod_put_array(struct gpio_descs *descs)
> +
> +It is strictly forbidden to use a descriptor after calling these functions.
> +It is also not allowed to individually release descriptors (using gpiod_put())
> +from an array acquired with gpiod_get_array().
> +
> +The device-managed variant is, unsurprisingly:
>  
>  	void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
>  
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index ff2b80d..d4153e0 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1804,6 +1804,158 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>  	return desc;
>  }
>  
> +static int dt_gpio_count(struct device *dev, const char *con_id)
> +{
> +	int ret;
> +	char propname[32];
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> +		if (con_id)
> +			snprintf(propname, sizeof(propname), "%s-%s",
> +				 con_id, gpio_suffixes[i]);
> +		else
> +			snprintf(propname, sizeof(propname), "%s",
> +				 gpio_suffixes[i]);
> +
> +		ret = of_gpio_named_count(dev->of_node, propname);
> +		if (ret >= 0)
> +			break;
> +	}
> +	return ret;
> +}
> +
> +#ifdef CONFIG_ACPI
> +
> +static unsigned 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)
> +			count++;
> +
> +		element++;
> +	}
> +	return count;
> +}
> +
> +static int acpi_find_gpio_count(struct acpi_resource *ares, void *data)
> +{
> +	unsigned int *count = data;
> +
> +	if (ares->type == ACPI_RESOURCE_TYPE_GPIO)
> +		*count += ares->data.gpio.pin_table_length;
> +
> +	return 1;
> +}
> +
> +static int acpi_gpio_count(struct device *dev, const char *con_id)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);

Should you check that the ACPI companion device actually exists here or
is it checked somewhere before already?

> +	const union acpi_object *obj;
> +	const struct acpi_gpio_mapping *gm;
> +	int count = -ENOENT;
> +	int ret;
> +	char propname[32];
> +	unsigned int i;
> +
> +	/* Try first from _DSD */
> +	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> +		if (con_id && strcmp(con_id, "gpios"))
> +			snprintf(propname, sizeof(propname), "%s-%s",
> +				 con_id, gpio_suffixes[i]);

I'm assuming some previous patch added gpio_suffixes[].

> +		else
> +			snprintf(propname, sizeof(propname), "%s",
> +				 gpio_suffixes[i]);
> +
> +		ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_ANY,
> +					    &obj);
> +		if (ret == 0) {
> +			if (obj->type == ACPI_TYPE_LOCAL_REFERENCE)
> +				count = 1;
> +			else if (obj->type == ACPI_TYPE_PACKAGE)
> +				count = acpi_gpio_package_count(obj);
> +		} else if (adev->driver_gpios) {
> +			for (gm = adev->driver_gpios; gm->name; gm++)
> +				if (strcmp(propname, gm->name) == 0) {
> +					count = gm->size;
> +					break;
> +				}
> +		}
> +		if (count >= 0)
> +			break;
> +	}
> +
> +	/* Then from plain _CRS GPIOs */
> +	if (count < 0) {
> +		struct list_head resource_list;
> +		unsigned int crs_count = 0;
> +
> +		INIT_LIST_HEAD(&resource_list);
> +		acpi_dev_get_resources(adev, &resource_list,
> +				       acpi_find_gpio_count, &crs_count);
> +		acpi_dev_free_resource_list(&resource_list);
> +		if (crs_count > 0)
> +			count = crs_count;
> +	}
> +	return count;
> +}

Otherwise the ACPI parts look good to me. I still want to test this,
though.

> +
> +#else
> +
> +static int acpi_gpio_count(struct device *dev, const char *con_id)
> +{
> +	return -ENODEV;
> +}
> +
> +#endif
> +
> +static int lut_gpio_count(struct device *dev, const char *con_id)
> +{
> +	struct gpiod_lookup_table *table;
> +	struct gpiod_lookup *p;
> +	unsigned int count = 0;
> +
> +	table = gpiod_find_lookup_table(dev);
> +	if (!table)
> +		return -ENOENT;
> +
> +	for (p = &table->table[0]; p->chip_label; p++) {
> +		if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) ||
> +		    (!con_id && !p->con_id))
> +			count++;
> +	}
> +	if (!count)
> +		return -ENOENT;
> +
> +	return count;
> +}
> +
> +/**
> + * gpiod_count - return the number of GPIOs associated with a device / function
> + *		or -ENOENT if no GPIO has been assigned to the requested function
> + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> + * @con_id:	function within the GPIO consumer
> + */
> +int gpiod_count(struct device *dev, const char *con_id)
> +{
> +	int count = -ENOENT;
> +
> +	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> +		count = dt_gpio_count(dev, con_id);
> +	else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev))
> +		count = acpi_gpio_count(dev, con_id);

Aha, so you here you checked for the ACPI compation, all right then.

> +
> +	if (count < 0)
> +		count = lut_gpio_count(dev, con_id);
> +
> +	return count;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_count);
> +
>  /**
>   * gpiod_get - obtain a GPIO for a given GPIO function
>   * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> @@ -2005,6 +2157,70 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
>  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>  
>  /**
> + * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
> + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> + * @con_id:	function within the GPIO consumer
> + * @flags:	optional GPIO initialization flags
> + *
> + * This function acquires all the GPIOs defined under a given function.
> + *
> + * Return a struct gpio_descs containing an array of descriptors, -ENOENT if
> + * no GPIO has been assigned to the requested function, or another IS_ERR()
> + * code if an error occurred while trying to acquire the GPIOs.
> + */
> +struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
> +						const char *con_id,
> +						enum gpiod_flags flags)
> +{
> +	struct gpio_desc *desc;
> +	struct gpio_descs *descs;
> +	int count;
> +
> +	count = gpiod_count(dev, con_id);
> +	if (count < 0)
> +		return ERR_PTR(count);
> +
> +	descs = kzalloc(sizeof(*descs) + sizeof(descs->desc_array[0]) * count,
> +			GFP_KERNEL);

Should you return ERR_PTR(-ENOMEM) here if the allocation fails?

> +
> +	for (descs->array_size = 0; descs->array_size < count; ) {
                                                              ^
                                                              |
                                                              +-- extra whitespace

> +		desc = gpiod_get_index(dev, con_id, descs->array_size, flags);
> +		if (IS_ERR(desc)) {
> +			gpiod_put_array(descs);
> +			return ERR_PTR(PTR_ERR(desc));

Uh, why not

	return desc

?

> +		}
> +		descs->desc_array[descs->array_size] = desc;
> +		descs->array_size++;
> +	}
> +	return descs;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_get_array);
> +
> +/**
> + * gpiod_get_array_optional - obtain multiple GPIOs from a multi-index GPIO
> + *                            function
> + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> + * @con_id:	function within the GPIO consumer
> + * @flags:	optional GPIO initialization flags
> + *
> + * This is equivalent to gpiod_get_array(), except that when no GPIO was
> + * assigned to the requested function it will return NULL.
> + */
> +struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
> +							const char *con_id,
> +							enum gpiod_flags flags)
> +{
> +	struct gpio_descs *descs;
> +
> +	descs = gpiod_get_array(dev, con_id, flags);
> +	if (IS_ERR(descs) && (PTR_ERR(descs) == -ENOENT))
> +		return NULL;

The caller still needs to handle other error codes (than -ENOENT), so
would it be better to just have only gpiod_get_array()?

> +
> +	return descs;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
> +
> +/**
>   * gpiod_put - dispose of a GPIO descriptor
>   * @desc:	GPIO descriptor to dispose of
>   *
> @@ -2016,6 +2232,21 @@ void gpiod_put(struct gpio_desc *desc)
>  }
>  EXPORT_SYMBOL_GPL(gpiod_put);
>  
> +/**
> + * gpiod_put_array - dispose of multiple GPIO descriptors
> + * @descs:	struct gpio_descs containing an array of descriptors
> + */
> +void gpiod_put_array(struct gpio_descs *descs)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < descs->array_size; i++)
> +		gpiod_put(descs->desc_array[i]);
> +
> +	kfree(descs);
> +}
> +EXPORT_SYMBOL_GPL(gpiod_put_array);
> +
>  #ifdef CONFIG_DEBUG_FS
>  
>  static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index 45afc2d..6b1c389 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -16,6 +16,15 @@ struct device;
>   */
>  struct gpio_desc;
>  
> +/**
> + * Struct containing an array of descriptors that can be obtained using
> + * gpiod_get_array().
> + */
> +struct gpio_descs {
> +	unsigned int array_size;
> +	struct gpio_desc *desc_array[];
> +};
> +
>  #define GPIOD_FLAGS_BIT_DIR_SET		BIT(0)
>  #define GPIOD_FLAGS_BIT_DIR_OUT		BIT(1)
>  #define GPIOD_FLAGS_BIT_DIR_VAL		BIT(2)
> @@ -34,6 +43,9 @@ enum gpiod_flags {
>  
>  #ifdef CONFIG_GPIOLIB
>  
> +/* Return the number of GPIOs associated with a device / function */
> +int gpiod_count(struct device *dev, const char *con_id);
> +
>  /* Acquire and dispose GPIOs */
>  struct gpio_desc *__must_check __gpiod_get(struct device *dev,
>  					 const char *con_id,
> @@ -49,7 +61,14 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
>  							const char *con_id,
>  							unsigned int index,
>  							enum gpiod_flags flags);
> +struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
> +						const char *con_id,
> +						enum gpiod_flags flags);
> +struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
> +							const char *con_id,
> +							enum gpiod_flags flags);
>  void gpiod_put(struct gpio_desc *desc);
> +void gpiod_put_array(struct gpio_descs *descs);
>  
>  struct gpio_desc *__must_check __devm_gpiod_get(struct device *dev,
>  					      const char *con_id,
> @@ -113,6 +132,11 @@ struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
>  					    struct fwnode_handle *child);
>  #else /* CONFIG_GPIOLIB */
>  
> +static inline int gpiod_count(struct device *dev, const char *con_id)
> +{
> +	return 0;
> +}
> +
>  static inline struct gpio_desc *__must_check __gpiod_get(struct device *dev,
>  						const char *con_id,
>  						enum gpiod_flags flags)
> @@ -142,6 +166,20 @@ __gpiod_get_index_optional(struct device *dev, const char *con_id,
>  	return ERR_PTR(-ENOSYS);
>  }
>  
> +static inline struct gpio_descs *__must_check
> +gpiod_get_array(struct device *dev, const char *con_id,
> +		enum gpiod_flags flags)
> +{
> +	return ERR_PTR(-ENOSYS);

That should be -ENXIO.

> +}
> +
> +static inline struct gpio_descs *__must_check
> +gpiod_get_array_optional(struct device *dev, const char *con_id,
> +			 enum gpiod_flags flags)
> +{
> +	return ERR_PTR(-ENOSYS);

Ditto.

> +}
> +
>  static inline void gpiod_put(struct gpio_desc *desc)
>  {
>  	might_sleep();
> @@ -150,6 +188,14 @@ static inline void gpiod_put(struct gpio_desc *desc)
>  	WARN_ON(1);
>  }
>  
> +static inline void gpiod_put_array(struct gpio_descs *descs)
> +{
> +	might_sleep();
> +
> +	/* GPIO can never have been requested */
> +	WARN_ON(1);
> +}
> +
>  static inline struct gpio_desc *__must_check
>  __devm_gpiod_get(struct device *dev,
>  		 const char *con_id,
> -- 
> 2.0.5
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] gpiolib: add gpiod_get_array and gpiod_put_array functions

[Rojhalat Ibrahim]

On Tuesday 10 February 2015 12:40:34 Mika Westerberg wrote:
> [+Rafael]
> 
> On Mon, Feb 09, 2015 at 06:07:10PM +0100, Rojhalat Ibrahim wrote:
> > Introduce new functions for conveniently obtaining and disposing of an entire
> > array of GPIOs with one function call.
> > 
> > Suggested-by: Alexandre Courbot <acourbot@nvidia.com>
> > Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
> 
> Can you please next time CC me the whole series? Now it fails with:
> 

Sure. Sorry about that.

> drivers/gpio/gpiolib.c: In function ‘dt_gpio_count’:
> drivers/gpio/gpiolib.c:1812:29: error: ‘gpio_suffixes’ undeclared (first
> use in this function)
>   for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
>                              ^
> include/linux/kernel.h:54:33: note: in definition of macro ‘ARRAY_SIZE’
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
> 
> and so on.
> 
> > ---
> > Change log:
> >   v3: - rebase on current linux-gpio devel branch
> >       - fix ACPI GPIO counting
> >       - allow for zero-sized arrays
> >       - make the flags argument mandatory for the new functions
> >       - clarify documentation
> >   v2: change interface
> > 
> >  Documentation/gpio/consumer.txt |  33 +++++-
> >  drivers/gpio/gpiolib.c          | 231 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/gpio/consumer.h   |  46 ++++++++
> >  3 files changed, 307 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> > index d85fbae..85a7e30 100644
> > --- a/Documentation/gpio/consumer.txt
> > +++ b/Documentation/gpio/consumer.txt
> > @@ -58,7 +58,6 @@ pattern where a GPIO is optional, the gpiod_get_optional() and
> >  gpiod_get_index_optional() functions can be used. These functions return NULL
> >  instead of -ENOENT if no GPIO has been assigned to the requested function:
> >  
> > -
> >  	struct gpio_desc *gpiod_get_optional(struct device *dev,
> >  					     const char *con_id,
> >  					     enum gpiod_flags flags)
> > @@ -68,6 +67,27 @@ instead of -ENOENT if no GPIO has been assigned to the requested function:
> >  						   unsigned int index,
> >  						   enum gpiod_flags flags)
> >  
> > +For a function using multiple GPIOs all of those can be obtained with one call:
> > +
> > +	struct gpio_descs *gpiod_get_array(struct device *dev,
> > +					   const char *con_id,
> > +					   enum gpiod_flags flags)
> > +
> > +This function returns a struct gpio_descs which contains an array of
> > +descriptors:
> > +
> > +	struct gpio_descs {
> > +		unsigned int array_size;
> > +		struct gpio_desc *desc_array[];
> > +	}
> > +
> > +The following function returns NULL instead of -ENOENT if no GPIOs have been
> > +assigned to the requested function:
> > +
> > +	struct gpio_descs *gpiod_get_array_optional(struct device *dev,
> > +						    const char *con_id,
> > +						    enum gpiod_flags flags)
> > +
> >  Device-managed variants of these functions are also defined:
> >  
> >  	struct gpio_desc *devm_gpiod_get(struct device *dev, const char *con_id,
> > @@ -91,8 +111,15 @@ A GPIO descriptor can be disposed of using the gpiod_put() function:
> >  
> >  	void gpiod_put(struct gpio_desc *desc)
> >  
> > -It is strictly forbidden to use a descriptor after calling this function. The
> > -device-managed variant is, unsurprisingly:
> > +For an array of GPIOs this function can be used:
> > +
> > +	void gpiod_put_array(struct gpio_descs *descs)
> > +
> > +It is strictly forbidden to use a descriptor after calling these functions.
> > +It is also not allowed to individually release descriptors (using gpiod_put())
> > +from an array acquired with gpiod_get_array().
> > +
> > +The device-managed variant is, unsurprisingly:
> >  
> >  	void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
> >  
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index ff2b80d..d4153e0 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1804,6 +1804,158 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
> >  	return desc;
> >  }
> >  
> > +static int dt_gpio_count(struct device *dev, const char *con_id)
> > +{
> > +	int ret;
> > +	char propname[32];
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> > +		if (con_id)
> > +			snprintf(propname, sizeof(propname), "%s-%s",
> > +				 con_id, gpio_suffixes[i]);
> > +		else
> > +			snprintf(propname, sizeof(propname), "%s",
> > +				 gpio_suffixes[i]);
> > +
> > +		ret = of_gpio_named_count(dev->of_node, propname);
> > +		if (ret >= 0)
> > +			break;
> > +	}
> > +	return ret;
> > +}
> > +
> > +#ifdef CONFIG_ACPI
> > +
> > +static unsigned 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)
> > +			count++;
> > +
> > +		element++;
> > +	}
> > +	return count;
> > +}
> > +
> > +static int acpi_find_gpio_count(struct acpi_resource *ares, void *data)
> > +{
> > +	unsigned int *count = data;
> > +
> > +	if (ares->type == ACPI_RESOURCE_TYPE_GPIO)
> > +		*count += ares->data.gpio.pin_table_length;
> > +
> > +	return 1;
> > +}
> > +
> > +static int acpi_gpio_count(struct device *dev, const char *con_id)
> > +{
> > +	struct acpi_device *adev = ACPI_COMPANION(dev);
> 
> Should you check that the ACPI companion device actually exists here or
> is it checked somewhere before already?
> 
> > +	const union acpi_object *obj;
> > +	const struct acpi_gpio_mapping *gm;
> > +	int count = -ENOENT;
> > +	int ret;
> > +	char propname[32];
> > +	unsigned int i;
> > +
> > +	/* Try first from _DSD */
> > +	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> > +		if (con_id && strcmp(con_id, "gpios"))
> > +			snprintf(propname, sizeof(propname), "%s-%s",
> > +				 con_id, gpio_suffixes[i]);
> 
> I'm assuming some previous patch added gpio_suffixes[].
> 
> > +		else
> > +			snprintf(propname, sizeof(propname), "%s",
> > +				 gpio_suffixes[i]);
> > +
> > +		ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_ANY,
> > +					    &obj);
> > +		if (ret == 0) {
> > +			if (obj->type == ACPI_TYPE_LOCAL_REFERENCE)
> > +				count = 1;
> > +			else if (obj->type == ACPI_TYPE_PACKAGE)
> > +				count = acpi_gpio_package_count(obj);
> > +		} else if (adev->driver_gpios) {
> > +			for (gm = adev->driver_gpios; gm->name; gm++)
> > +				if (strcmp(propname, gm->name) == 0) {
> > +					count = gm->size;
> > +					break;
> > +				}
> > +		}
> > +		if (count >= 0)
> > +			break;
> > +	}
> > +
> > +	/* Then from plain _CRS GPIOs */
> > +	if (count < 0) {
> > +		struct list_head resource_list;
> > +		unsigned int crs_count = 0;
> > +
> > +		INIT_LIST_HEAD(&resource_list);
> > +		acpi_dev_get_resources(adev, &resource_list,
> > +				       acpi_find_gpio_count, &crs_count);
> > +		acpi_dev_free_resource_list(&resource_list);
> > +		if (crs_count > 0)
> > +			count = crs_count;
> > +	}
> > +	return count;
> > +}
> 
> Otherwise the ACPI parts look good to me. I still want to test this,
> though.
> 

Great.

> > +
> > +#else
> > +
> > +static int acpi_gpio_count(struct device *dev, const char *con_id)
> > +{
> > +	return -ENODEV;
> > +}
> > +
> > +#endif
> > +
> > +static int lut_gpio_count(struct device *dev, const char *con_id)
> > +{
> > +	struct gpiod_lookup_table *table;
> > +	struct gpiod_lookup *p;
> > +	unsigned int count = 0;
> > +
> > +	table = gpiod_find_lookup_table(dev);
> > +	if (!table)
> > +		return -ENOENT;
> > +
> > +	for (p = &table->table[0]; p->chip_label; p++) {
> > +		if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) ||
> > +		    (!con_id && !p->con_id))
> > +			count++;
> > +	}
> > +	if (!count)
> > +		return -ENOENT;
> > +
> > +	return count;
> > +}
> > +
> > +/**
> > + * gpiod_count - return the number of GPIOs associated with a device / function
> > + *		or -ENOENT if no GPIO has been assigned to the requested function
> > + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> > + * @con_id:	function within the GPIO consumer
> > + */
> > +int gpiod_count(struct device *dev, const char *con_id)
> > +{
> > +	int count = -ENOENT;
> > +
> > +	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> > +		count = dt_gpio_count(dev, con_id);
> > +	else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev))
> > +		count = acpi_gpio_count(dev, con_id);
> 
> Aha, so you here you checked for the ACPI compation, all right then.
> 
> > +
> > +	if (count < 0)
> > +		count = lut_gpio_count(dev, con_id);
> > +
> > +	return count;
> > +}
> > +EXPORT_SYMBOL_GPL(gpiod_count);
> > +
> >  /**
> >   * gpiod_get - obtain a GPIO for a given GPIO function
> >   * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> > @@ -2005,6 +2157,70 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
> >  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> >  
> >  /**
> > + * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
> > + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> > + * @con_id:	function within the GPIO consumer
> > + * @flags:	optional GPIO initialization flags
> > + *
> > + * This function acquires all the GPIOs defined under a given function.
> > + *
> > + * Return a struct gpio_descs containing an array of descriptors, -ENOENT if
> > + * no GPIO has been assigned to the requested function, or another IS_ERR()
> > + * code if an error occurred while trying to acquire the GPIOs.
> > + */
> > +struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
> > +						const char *con_id,
> > +						enum gpiod_flags flags)
> > +{
> > +	struct gpio_desc *desc;
> > +	struct gpio_descs *descs;
> > +	int count;
> > +
> > +	count = gpiod_count(dev, con_id);
> > +	if (count < 0)
> > +		return ERR_PTR(count);
> > +
> > +	descs = kzalloc(sizeof(*descs) + sizeof(descs->desc_array[0]) * count,
> > +			GFP_KERNEL);
> 
> Should you return ERR_PTR(-ENOMEM) here if the allocation fails?
> 

Of course. I'll put it in the next revision.

> > +
> > +	for (descs->array_size = 0; descs->array_size < count; ) {
>                                                               ^
>                                                               |
>                                                               +-- extra whitespace
> 
> > +		desc = gpiod_get_index(dev, con_id, descs->array_size, flags);
> > +		if (IS_ERR(desc)) {
> > +			gpiod_put_array(descs);
> > +			return ERR_PTR(PTR_ERR(desc));
> 
> Uh, why not
> 
> 	return desc
> 
> ?

Because of the different pointer type. desc is a pointer to a GPIO descriptor
whereas the return type of the function is a pointer to struct gpio_descs.

> 
> > +		}
> > +		descs->desc_array[descs->array_size] = desc;
> > +		descs->array_size++;
> > +	}
> > +	return descs;
> > +}
> > +EXPORT_SYMBOL_GPL(gpiod_get_array);
> > +
> > +/**
> > + * gpiod_get_array_optional - obtain multiple GPIOs from a multi-index GPIO
> > + *                            function
> > + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> > + * @con_id:	function within the GPIO consumer
> > + * @flags:	optional GPIO initialization flags
> > + *
> > + * This is equivalent to gpiod_get_array(), except that when no GPIO was
> > + * assigned to the requested function it will return NULL.
> > + */
> > +struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
> > +							const char *con_id,
> > +							enum gpiod_flags flags)
> > +{
> > +	struct gpio_descs *descs;
> > +
> > +	descs = gpiod_get_array(dev, con_id, flags);
> > +	if (IS_ERR(descs) && (PTR_ERR(descs) == -ENOENT))
> > +		return NULL;
> 
> The caller still needs to handle other error codes (than -ENOENT), so
> would it be better to just have only gpiod_get_array()?
> 

This follows the same pattern as gpiod_get_optional() and
gpiod_get_index_optional()

From Documentation/gpio/consumer.txt:

This is useful to discriminate between mere
errors and an absence of GPIO for optional GPIO parameters. For the common
pattern where a GPIO is optional, the gpiod_get_optional() and
gpiod_get_index_optional() functions can be used. These functions return NULL
instead of -ENOENT if no GPIO has been assigned to the requested function

> > +
> > +	return descs;
> > +}
> > +EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
> > +
> > +/**
> >   * gpiod_put - dispose of a GPIO descriptor
> >   * @desc:	GPIO descriptor to dispose of
> >   *
> > @@ -2016,6 +2232,21 @@ void gpiod_put(struct gpio_desc *desc)
> >  }
> >  EXPORT_SYMBOL_GPL(gpiod_put);
> >  
> > +/**
> > + * gpiod_put_array - dispose of multiple GPIO descriptors
> > + * @descs:	struct gpio_descs containing an array of descriptors
> > + */
> > +void gpiod_put_array(struct gpio_descs *descs)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < descs->array_size; i++)
> > +		gpiod_put(descs->desc_array[i]);
> > +
> > +	kfree(descs);
> > +}
> > +EXPORT_SYMBOL_GPL(gpiod_put_array);
> > +
> >  #ifdef CONFIG_DEBUG_FS
> >  
> >  static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> > index 45afc2d..6b1c389 100644
> > --- a/include/linux/gpio/consumer.h
> > +++ b/include/linux/gpio/consumer.h
> > @@ -16,6 +16,15 @@ struct device;
> >   */
> >  struct gpio_desc;
> >  
> > +/**
> > + * Struct containing an array of descriptors that can be obtained using
> > + * gpiod_get_array().
> > + */
> > +struct gpio_descs {
> > +	unsigned int array_size;
> > +	struct gpio_desc *desc_array[];
> > +};
> > +
> >  #define GPIOD_FLAGS_BIT_DIR_SET		BIT(0)
> >  #define GPIOD_FLAGS_BIT_DIR_OUT		BIT(1)
> >  #define GPIOD_FLAGS_BIT_DIR_VAL		BIT(2)
> > @@ -34,6 +43,9 @@ enum gpiod_flags {
> >  
> >  #ifdef CONFIG_GPIOLIB
> >  
> > +/* Return the number of GPIOs associated with a device / function */
> > +int gpiod_count(struct device *dev, const char *con_id);
> > +
> >  /* Acquire and dispose GPIOs */
> >  struct gpio_desc *__must_check __gpiod_get(struct device *dev,
> >  					 const char *con_id,
> > @@ -49,7 +61,14 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
> >  							const char *con_id,
> >  							unsigned int index,
> >  							enum gpiod_flags flags);
> > +struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
> > +						const char *con_id,
> > +						enum gpiod_flags flags);
> > +struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
> > +							const char *con_id,
> > +							enum gpiod_flags flags);
> >  void gpiod_put(struct gpio_desc *desc);
> > +void gpiod_put_array(struct gpio_descs *descs);
> >  
> >  struct gpio_desc *__must_check __devm_gpiod_get(struct device *dev,
> >  					      const char *con_id,
> > @@ -113,6 +132,11 @@ struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
> >  					    struct fwnode_handle *child);
> >  #else /* CONFIG_GPIOLIB */
> >  
> > +static inline int gpiod_count(struct device *dev, const char *con_id)
> > +{
> > +	return 0;
> > +}
> > +
> >  static inline struct gpio_desc *__must_check __gpiod_get(struct device *dev,
> >  						const char *con_id,
> >  						enum gpiod_flags flags)
> > @@ -142,6 +166,20 @@ __gpiod_get_index_optional(struct device *dev, const char *con_id,
> >  	return ERR_PTR(-ENOSYS);
> >  }
> >  
> > +static inline struct gpio_descs *__must_check
> > +gpiod_get_array(struct device *dev, const char *con_id,
> > +		enum gpiod_flags flags)
> > +{
> > +	return ERR_PTR(-ENOSYS);
> 
> That should be -ENXIO.
> 

Why? All the other functions in this section return -ENOSYS.

According to include/uapi/asm-generic/errno.h
ENOSYS means "Function not implemented"
and in include/uapi/asm-generic/errno-base.h it says
ENXIO stands for "No such device or address".

If ENXIO is more correct here (and I don't see why),
then it should at least be consistent for all functions, right?

> > +}
> > +
> > +static inline struct gpio_descs *__must_check
> > +gpiod_get_array_optional(struct device *dev, const char *con_id,
> > +			 enum gpiod_flags flags)
> > +{
> > +	return ERR_PTR(-ENOSYS);
> 
> Ditto.
> 
> > +}
> > +
> >  static inline void gpiod_put(struct gpio_desc *desc)
> >  {
> >  	might_sleep();
> > @@ -150,6 +188,14 @@ static inline void gpiod_put(struct gpio_desc *desc)
> >  	WARN_ON(1);
> >  }
> >  
> > +static inline void gpiod_put_array(struct gpio_descs *descs)
> > +{
> > +	might_sleep();
> > +
> > +	/* GPIO can never have been requested */
> > +	WARN_ON(1);
> > +}
> > +
> >  static inline struct gpio_desc *__must_check
> >  __devm_gpiod_get(struct device *dev,
> >  		 const char *con_id,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] gpiolib: add gpiod_get_array and gpiod_put_array functions

[Mika Westerberg]

On Tue, Feb 10, 2015 at 01:21:19PM +0100, Rojhalat Ibrahim wrote:
> On Tuesday 10 February 2015 12:40:34 Mika Westerberg wrote:
> > [+Rafael]
> > 
> > On Mon, Feb 09, 2015 at 06:07:10PM +0100, Rojhalat Ibrahim wrote:
> > > Introduce new functions for conveniently obtaining and disposing of an entire
> > > array of GPIOs with one function call.
> > > 
> > > Suggested-by: Alexandre Courbot <acourbot@nvidia.com>
> > > Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
> > 
> > Can you please next time CC me the whole series? Now it fails with:
> > 
> 
> Sure. Sorry about that.
> 
> > drivers/gpio/gpiolib.c: In function ‘dt_gpio_count’:
> > drivers/gpio/gpiolib.c:1812:29: error: ‘gpio_suffixes’ undeclared (first
> > use in this function)
> >   for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> >                              ^
> > include/linux/kernel.h:54:33: note: in definition of macro ‘ARRAY_SIZE’
> >  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> > __must_be_array(arr))
> > 
> > and so on.
> > 
> > > ---
> > > Change log:
> > >   v3: - rebase on current linux-gpio devel branch
> > >       - fix ACPI GPIO counting
> > >       - allow for zero-sized arrays
> > >       - make the flags argument mandatory for the new functions
> > >       - clarify documentation
> > >   v2: change interface
> > > 
> > >  Documentation/gpio/consumer.txt |  33 +++++-
> > >  drivers/gpio/gpiolib.c          | 231 ++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/gpio/consumer.h   |  46 ++++++++
> > >  3 files changed, 307 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> > > index d85fbae..85a7e30 100644
> > > --- a/Documentation/gpio/consumer.txt
> > > +++ b/Documentation/gpio/consumer.txt
> > > @@ -58,7 +58,6 @@ pattern where a GPIO is optional, the gpiod_get_optional() and
> > >  gpiod_get_index_optional() functions can be used. These functions return NULL
> > >  instead of -ENOENT if no GPIO has been assigned to the requested function:
> > >  
> > > -
> > >  	struct gpio_desc *gpiod_get_optional(struct device *dev,
> > >  					     const char *con_id,
> > >  					     enum gpiod_flags flags)
> > > @@ -68,6 +67,27 @@ instead of -ENOENT if no GPIO has been assigned to the requested function:
> > >  						   unsigned int index,
> > >  						   enum gpiod_flags flags)
> > >  
> > > +For a function using multiple GPIOs all of those can be obtained with one call:
> > > +
> > > +	struct gpio_descs *gpiod_get_array(struct device *dev,
> > > +					   const char *con_id,
> > > +					   enum gpiod_flags flags)
> > > +
> > > +This function returns a struct gpio_descs which contains an array of
> > > +descriptors:
> > > +
> > > +	struct gpio_descs {
> > > +		unsigned int array_size;
> > > +		struct gpio_desc *desc_array[];
> > > +	}
> > > +
> > > +The following function returns NULL instead of -ENOENT if no GPIOs have been
> > > +assigned to the requested function:
> > > +
> > > +	struct gpio_descs *gpiod_get_array_optional(struct device *dev,
> > > +						    const char *con_id,
> > > +						    enum gpiod_flags flags)
> > > +
> > >  Device-managed variants of these functions are also defined:
> > >  
> > >  	struct gpio_desc *devm_gpiod_get(struct device *dev, const char *con_id,
> > > @@ -91,8 +111,15 @@ A GPIO descriptor can be disposed of using the gpiod_put() function:
> > >  
> > >  	void gpiod_put(struct gpio_desc *desc)
> > >  
> > > -It is strictly forbidden to use a descriptor after calling this function. The
> > > -device-managed variant is, unsurprisingly:
> > > +For an array of GPIOs this function can be used:
> > > +
> > > +	void gpiod_put_array(struct gpio_descs *descs)
> > > +
> > > +It is strictly forbidden to use a descriptor after calling these functions.
> > > +It is also not allowed to individually release descriptors (using gpiod_put())
> > > +from an array acquired with gpiod_get_array().
> > > +
> > > +The device-managed variant is, unsurprisingly:
> > >  
> > >  	void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
> > >  
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index ff2b80d..d4153e0 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -1804,6 +1804,158 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
> > >  	return desc;
> > >  }
> > >  
> > > +static int dt_gpio_count(struct device *dev, const char *con_id)
> > > +{
> > > +	int ret;
> > > +	char propname[32];
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> > > +		if (con_id)
> > > +			snprintf(propname, sizeof(propname), "%s-%s",
> > > +				 con_id, gpio_suffixes[i]);
> > > +		else
> > > +			snprintf(propname, sizeof(propname), "%s",
> > > +				 gpio_suffixes[i]);
> > > +
> > > +		ret = of_gpio_named_count(dev->of_node, propname);
> > > +		if (ret >= 0)
> > > +			break;
> > > +	}
> > > +	return ret;
> > > +}
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +
> > > +static unsigned 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)
> > > +			count++;
> > > +
> > > +		element++;
> > > +	}
> > > +	return count;
> > > +}
> > > +
> > > +static int acpi_find_gpio_count(struct acpi_resource *ares, void *data)
> > > +{
> > > +	unsigned int *count = data;
> > > +
> > > +	if (ares->type == ACPI_RESOURCE_TYPE_GPIO)
> > > +		*count += ares->data.gpio.pin_table_length;
> > > +
> > > +	return 1;
> > > +}
> > > +
> > > +static int acpi_gpio_count(struct device *dev, const char *con_id)
> > > +{
> > > +	struct acpi_device *adev = ACPI_COMPANION(dev);
> > 
> > Should you check that the ACPI companion device actually exists here or
> > is it checked somewhere before already?
> > 
> > > +	const union acpi_object *obj;
> > > +	const struct acpi_gpio_mapping *gm;
> > > +	int count = -ENOENT;
> > > +	int ret;
> > > +	char propname[32];
> > > +	unsigned int i;
> > > +
> > > +	/* Try first from _DSD */
> > > +	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> > > +		if (con_id && strcmp(con_id, "gpios"))
> > > +			snprintf(propname, sizeof(propname), "%s-%s",
> > > +				 con_id, gpio_suffixes[i]);
> > 
> > I'm assuming some previous patch added gpio_suffixes[].
> > 
> > > +		else
> > > +			snprintf(propname, sizeof(propname), "%s",
> > > +				 gpio_suffixes[i]);
> > > +
> > > +		ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_ANY,
> > > +					    &obj);
> > > +		if (ret == 0) {
> > > +			if (obj->type == ACPI_TYPE_LOCAL_REFERENCE)
> > > +				count = 1;
> > > +			else if (obj->type == ACPI_TYPE_PACKAGE)
> > > +				count = acpi_gpio_package_count(obj);
> > > +		} else if (adev->driver_gpios) {
> > > +			for (gm = adev->driver_gpios; gm->name; gm++)
> > > +				if (strcmp(propname, gm->name) == 0) {
> > > +					count = gm->size;
> > > +					break;
> > > +				}
> > > +		}
> > > +		if (count >= 0)
> > > +			break;
> > > +	}
> > > +
> > > +	/* Then from plain _CRS GPIOs */
> > > +	if (count < 0) {
> > > +		struct list_head resource_list;
> > > +		unsigned int crs_count = 0;
> > > +
> > > +		INIT_LIST_HEAD(&resource_list);
> > > +		acpi_dev_get_resources(adev, &resource_list,
> > > +				       acpi_find_gpio_count, &crs_count);
> > > +		acpi_dev_free_resource_list(&resource_list);
> > > +		if (crs_count > 0)
> > > +			count = crs_count;
> > > +	}
> > > +	return count;
> > > +}
> > 
> > Otherwise the ACPI parts look good to me. I still want to test this,
> > though.
> > 
> 
> Great.
> 
> > > +
> > > +#else
> > > +
> > > +static int acpi_gpio_count(struct device *dev, const char *con_id)
> > > +{
> > > +	return -ENODEV;
> > > +}
> > > +
> > > +#endif
> > > +
> > > +static int lut_gpio_count(struct device *dev, const char *con_id)
> > > +{
> > > +	struct gpiod_lookup_table *table;
> > > +	struct gpiod_lookup *p;
> > > +	unsigned int count = 0;
> > > +
> > > +	table = gpiod_find_lookup_table(dev);
> > > +	if (!table)
> > > +		return -ENOENT;
> > > +
> > > +	for (p = &table->table[0]; p->chip_label; p++) {
> > > +		if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) ||
> > > +		    (!con_id && !p->con_id))
> > > +			count++;
> > > +	}
> > > +	if (!count)
> > > +		return -ENOENT;
> > > +
> > > +	return count;
> > > +}
> > > +
> > > +/**
> > > + * gpiod_count - return the number of GPIOs associated with a device / function
> > > + *		or -ENOENT if no GPIO has been assigned to the requested function
> > > + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> > > + * @con_id:	function within the GPIO consumer
> > > + */
> > > +int gpiod_count(struct device *dev, const char *con_id)
> > > +{
> > > +	int count = -ENOENT;
> > > +
> > > +	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> > > +		count = dt_gpio_count(dev, con_id);
> > > +	else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev))
> > > +		count = acpi_gpio_count(dev, con_id);
> > 
> > Aha, so you here you checked for the ACPI compation, all right then.
> > 
> > > +
> > > +	if (count < 0)
> > > +		count = lut_gpio_count(dev, con_id);
> > > +
> > > +	return count;
> > > +}
> > > +EXPORT_SYMBOL_GPL(gpiod_count);
> > > +
> > >  /**
> > >   * gpiod_get - obtain a GPIO for a given GPIO function
> > >   * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> > > @@ -2005,6 +2157,70 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
> > >  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> > >  
> > >  /**
> > > + * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
> > > + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> > > + * @con_id:	function within the GPIO consumer
> > > + * @flags:	optional GPIO initialization flags
> > > + *
> > > + * This function acquires all the GPIOs defined under a given function.
> > > + *
> > > + * Return a struct gpio_descs containing an array of descriptors, -ENOENT if
> > > + * no GPIO has been assigned to the requested function, or another IS_ERR()
> > > + * code if an error occurred while trying to acquire the GPIOs.
> > > + */
> > > +struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
> > > +						const char *con_id,
> > > +						enum gpiod_flags flags)
> > > +{
> > > +	struct gpio_desc *desc;
> > > +	struct gpio_descs *descs;
> > > +	int count;
> > > +
> > > +	count = gpiod_count(dev, con_id);
> > > +	if (count < 0)
> > > +		return ERR_PTR(count);
> > > +
> > > +	descs = kzalloc(sizeof(*descs) + sizeof(descs->desc_array[0]) * count,
> > > +			GFP_KERNEL);
> > 
> > Should you return ERR_PTR(-ENOMEM) here if the allocation fails?
> > 
> 
> Of course. I'll put it in the next revision.
> 
> > > +
> > > +	for (descs->array_size = 0; descs->array_size < count; ) {
> >                                                               ^
> >                                                               |
> >                                                               +-- extra whitespace
> > 
> > > +		desc = gpiod_get_index(dev, con_id, descs->array_size, flags);
> > > +		if (IS_ERR(desc)) {
> > > +			gpiod_put_array(descs);
> > > +			return ERR_PTR(PTR_ERR(desc));
> > 
> > Uh, why not
> > 
> > 	return desc
> > 
> > ?
> 
> Because of the different pointer type. desc is a pointer to a GPIO descriptor
> whereas the return type of the function is a pointer to struct gpio_descs.

Right. I wonder if ERR_CAST() may be used here.

> 
> > 
> > > +		}
> > > +		descs->desc_array[descs->array_size] = desc;
> > > +		descs->array_size++;
> > > +	}
> > > +	return descs;
> > > +}
> > > +EXPORT_SYMBOL_GPL(gpiod_get_array);
> > > +
> > > +/**
> > > + * gpiod_get_array_optional - obtain multiple GPIOs from a multi-index GPIO
> > > + *                            function
> > > + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> > > + * @con_id:	function within the GPIO consumer
> > > + * @flags:	optional GPIO initialization flags
> > > + *
> > > + * This is equivalent to gpiod_get_array(), except that when no GPIO was
> > > + * assigned to the requested function it will return NULL.
> > > + */
> > > +struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
> > > +							const char *con_id,
> > > +							enum gpiod_flags flags)
> > > +{
> > > +	struct gpio_descs *descs;
> > > +
> > > +	descs = gpiod_get_array(dev, con_id, flags);
> > > +	if (IS_ERR(descs) && (PTR_ERR(descs) == -ENOENT))
> > > +		return NULL;
> > 
> > The caller still needs to handle other error codes (than -ENOENT), so
> > would it be better to just have only gpiod_get_array()?
> > 
> 
> This follows the same pattern as gpiod_get_optional() and
> gpiod_get_index_optional()
> 
> >From Documentation/gpio/consumer.txt:
> 
> This is useful to discriminate between mere
> errors and an absence of GPIO for optional GPIO parameters. For the common
> pattern where a GPIO is optional, the gpiod_get_optional() and
> gpiod_get_index_optional() functions can be used. These functions return NULL
> instead of -ENOENT if no GPIO has been assigned to the requested function

Fair enough.

> > > +
> > > +	return descs;
> > > +}
> > > +EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
> > > +
> > > +/**
> > >   * gpiod_put - dispose of a GPIO descriptor
> > >   * @desc:	GPIO descriptor to dispose of
> > >   *
> > > @@ -2016,6 +2232,21 @@ void gpiod_put(struct gpio_desc *desc)
> > >  }
> > >  EXPORT_SYMBOL_GPL(gpiod_put);
> > >  
> > > +/**
> > > + * gpiod_put_array - dispose of multiple GPIO descriptors
> > > + * @descs:	struct gpio_descs containing an array of descriptors
> > > + */
> > > +void gpiod_put_array(struct gpio_descs *descs)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < descs->array_size; i++)
> > > +		gpiod_put(descs->desc_array[i]);
> > > +
> > > +	kfree(descs);
> > > +}
> > > +EXPORT_SYMBOL_GPL(gpiod_put_array);
> > > +
> > >  #ifdef CONFIG_DEBUG_FS
> > >  
> > >  static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> > > index 45afc2d..6b1c389 100644
> > > --- a/include/linux/gpio/consumer.h
> > > +++ b/include/linux/gpio/consumer.h
> > > @@ -16,6 +16,15 @@ struct device;
> > >   */
> > >  struct gpio_desc;
> > >  
> > > +/**
> > > + * Struct containing an array of descriptors that can be obtained using
> > > + * gpiod_get_array().
> > > + */
> > > +struct gpio_descs {
> > > +	unsigned int array_size;
> > > +	struct gpio_desc *desc_array[];
> > > +};
> > > +
> > >  #define GPIOD_FLAGS_BIT_DIR_SET		BIT(0)
> > >  #define GPIOD_FLAGS_BIT_DIR_OUT		BIT(1)
> > >  #define GPIOD_FLAGS_BIT_DIR_VAL		BIT(2)
> > > @@ -34,6 +43,9 @@ enum gpiod_flags {
> > >  
> > >  #ifdef CONFIG_GPIOLIB
> > >  
> > > +/* Return the number of GPIOs associated with a device / function */
> > > +int gpiod_count(struct device *dev, const char *con_id);
> > > +
> > >  /* Acquire and dispose GPIOs */
> > >  struct gpio_desc *__must_check __gpiod_get(struct device *dev,
> > >  					 const char *con_id,
> > > @@ -49,7 +61,14 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
> > >  							const char *con_id,
> > >  							unsigned int index,
> > >  							enum gpiod_flags flags);
> > > +struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
> > > +						const char *con_id,
> > > +						enum gpiod_flags flags);
> > > +struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
> > > +							const char *con_id,
> > > +							enum gpiod_flags flags);
> > >  void gpiod_put(struct gpio_desc *desc);
> > > +void gpiod_put_array(struct gpio_descs *descs);
> > >  
> > >  struct gpio_desc *__must_check __devm_gpiod_get(struct device *dev,
> > >  					      const char *con_id,
> > > @@ -113,6 +132,11 @@ struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
> > >  					    struct fwnode_handle *child);
> > >  #else /* CONFIG_GPIOLIB */
> > >  
> > > +static inline int gpiod_count(struct device *dev, const char *con_id)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > >  static inline struct gpio_desc *__must_check __gpiod_get(struct device *dev,
> > >  						const char *con_id,
> > >  						enum gpiod_flags flags)
> > > @@ -142,6 +166,20 @@ __gpiod_get_index_optional(struct device *dev, const char *con_id,
> > >  	return ERR_PTR(-ENOSYS);
> > >  }
> > >  
> > > +static inline struct gpio_descs *__must_check
> > > +gpiod_get_array(struct device *dev, const char *con_id,
> > > +		enum gpiod_flags flags)
> > > +{
> > > +	return ERR_PTR(-ENOSYS);
> > 
> > That should be -ENXIO.
> > 
> 
> Why? All the other functions in this section return -ENOSYS.

It means system call not implemented and should only be returned from
those.

> According to include/uapi/asm-generic/errno.h
> ENOSYS means "Function not implemented"
> and in include/uapi/asm-generic/errno-base.h it says
> ENXIO stands for "No such device or address".
> 
> If ENXIO is more correct here (and I don't see why),

IIRC it was decided (perhaps in last Kernel Summit) that in these cases
we return -ENXIO, not -ENOSYS. But...

> then it should at least be consistent for all functions, right?

...you are right here. It should be consistent accross the file.

> > > +}
> > > +
> > > +static inline struct gpio_descs *__must_check
> > > +gpiod_get_array_optional(struct device *dev, const char *con_id,
> > > +			 enum gpiod_flags flags)
> > > +{
> > > +	return ERR_PTR(-ENOSYS);
> > 
> > Ditto.
> > 
> > > +}
> > > +
> > >  static inline void gpiod_put(struct gpio_desc *desc)
> > >  {
> > >  	might_sleep();
> > > @@ -150,6 +188,14 @@ static inline void gpiod_put(struct gpio_desc *desc)
> > >  	WARN_ON(1);
> > >  }
> > >  
> > > +static inline void gpiod_put_array(struct gpio_descs *descs)
> > > +{
> > > +	might_sleep();
> > > +
> > > +	/* GPIO can never have been requested */
> > > +	WARN_ON(1);
> > > +}
> > > +
> > >  static inline struct gpio_desc *__must_check
> > >  __devm_gpiod_get(struct device *dev,
> > >  		 const char *con_id,
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] gpiolib: add gpiod_get_array and gpiod_put_array functions

[Rojhalat Ibrahim]

On Tuesday 10 February 2015 14:41:18 Mika Westerberg wrote:
> On Tue, Feb 10, 2015 at 01:21:19PM +0100, Rojhalat Ibrahim wrote:
> > On Tuesday 10 February 2015 12:40:34 Mika Westerberg wrote:
> > 
> > > > +
> > > > +	for (descs->array_size = 0; descs->array_size < count; ) {
> > >                                                               ^
> > >                                                               |
> > >                                                               +-- extra whitespace
> > > 
> > > > +		desc = gpiod_get_index(dev, con_id, descs->array_size, flags);
> > > > +		if (IS_ERR(desc)) {
> > > > +			gpiod_put_array(descs);
> > > > +			return ERR_PTR(PTR_ERR(desc));
> > > 
> > > Uh, why not
> > > 
> > > 	return desc
> > > 
> > > ?
> > 
> > Because of the different pointer type. desc is a pointer to a GPIO descriptor
> > whereas the return type of the function is a pointer to struct gpio_descs.
> 
> Right. I wonder if ERR_CAST() may be used here.
> 

Didn't know about that one. I'll use it in the next revision.

Thanks for looking at this.