[PATCH v4 0/2] gpio: Add a managed API

[PATCH v4 0/2] gpio: Add a managed API

[Pratyush Yadav]

Hi,

This is a re-submission of Jean-Jacques' earlier work in October last
year. It can be found at [0]. The goal is to facilitate porting drivers
from the Linux kernel. Most of the series will be about adding managed
APIs to existing infrastructure (GPIO, reset, regmap).

This particular series is about GPIOs. It adds a managed API using the
API as Linux. To make it 100% compatible with linux, there is a small
deviation from u-boot's way of naming the gpio lists: the managed
equivalent of gpio_request_by_name(..,"blabla-gpios", ...) is
devm_gpiod_get_index(..., "blabla", ...)

Travis CI run: https://travis-ci.org/github/prati0100/uboot/builds/725044296

Changes in v4:
- Rebase on latest master and fix merge conflicts.
- Move "another-test" node down the order to make sure
  dm_test_fdt_uclass_seq() continues to work.
- Bump num_devices to 9 in dm_test_bus_children(), dm_test_fdt(), and
  dm_test_uclass_foreach() to fix broken tests.
- s/DM_TESTF/UT_TESTF/g

Changes in v3:
- Add a blank line before return in devm_gpiod_get_index().
- Add Simon's Reviwed-by in both patches.

Changes in v2:
- The original series had a patch that checked for NULL pointers in the
  core GPIO functions. The checks were needed because of the addition of
  devm_gpiod_get_index_optional() which would return NULL when when no
  GPIO was assigned to the requested function. This is convenient for
  drivers that need to handle optional GPIOs.

  Simon argued that those should be behind a Kconfig option because of
  code size concerns. He also argued against implicit return in the
  macro that checked for the optional GPIOs.

  This submission removes the controversial patch so that base
  functionality can get merged.

  We still need to take a stance on who is responsible for the NULL
  check: the driver or the GPIO core? Do we want to trust drivers to
  take care of the NULL checks, or do we want to distrust them and make
  sure they don't send us anything bogus in the GPIO core. I will send a
  separate RFC of the NULL check patch and we can probably discuss the
  issue there.

[0] https://patchwork.ozlabs.org/project/uboot/cover/20191001115130.18886-1-jjhiblot at ti.com/

Jean-Jacques Hiblot (2):
  drivers: gpio: Add a managed API to get a GPIO from the device-tree
  test: gpio: Add tests for the managed API

 arch/sandbox/dts/test.dts  |  10 ++++
 drivers/gpio/gpio-uclass.c |  71 ++++++++++++++++++++++++++
 include/asm-generic/gpio.h |  47 +++++++++++++++++
 test/dm/bus.c              |   2 +-
 test/dm/gpio.c             | 102 +++++++++++++++++++++++++++++++++++++
 test/dm/test-fdt.c         |   6 +--
 6 files changed, 234 insertions(+), 4 deletions(-)

--
2.28.0

[PATCH v4 1/2] drivers: gpio: Add a managed API to get a GPIO from the device-tree

[Pratyush Yadav]

From: Jean-Jacques Hiblot <jjhiblot@ti.com>

Add managed functions to get a gpio from the devce-tree, based on a
property name (minus the '-gpios' suffix) and optionally an index.

When the device is unbound, the GPIO is automatically released and the
data structure is freed.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/gpio/gpio-uclass.c | 71 ++++++++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h | 47 +++++++++++++++++++++++++
 2 files changed, 118 insertions(+)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 9c53299b6a..0c01413b58 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -6,6 +6,8 @@
 #include <common.h>
 #include <dm.h>
 #include <log.h>
+#include <dm/devres.h>
+#include <dm/device_compat.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
 #include <dm/uclass-internal.h>
@@ -1209,6 +1211,75 @@ int gpio_dev_request_index(struct udevice *dev, const char *nodename,
 				 flags, 0, dev);
 }

+static void devm_gpiod_release(struct udevice *dev, void *res)
+{
+	dm_gpio_free(dev, res);
+}
+
+static int devm_gpiod_match(struct udevice *dev, void *res, void *data)
+{
+	return res == data;
+}
+
+struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id,
+				       unsigned int index, int flags)
+{
+	int rc;
+	struct gpio_desc *desc;
+	char *propname;
+	static const char suffix[] = "-gpios";
+
+	propname = malloc(strlen(id) + sizeof(suffix));
+	if (!propname) {
+		rc = -ENOMEM;
+		goto end;
+	}
+
+	strcpy(propname, id);
+	strcat(propname, suffix);
+
+	desc = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc),
+			    __GFP_ZERO);
+	if (unlikely(!desc)) {
+		rc = -ENOMEM;
+		goto end;
+	}
+
+	rc = gpio_request_by_name(dev, propname, index, desc, flags);
+
+end:
+	if (propname)
+		free(propname);
+
+	if (rc)
+		return ERR_PTR(rc);
+
+	devres_add(dev, desc);
+
+	return desc;
+}
+
+struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev,
+						const char *id,
+						unsigned int index,
+						int flags)
+{
+	struct gpio_desc *desc = devm_gpiod_get_index(dev, id, index, flags);
+
+	if (IS_ERR(desc))
+		return NULL;
+
+	return desc;
+}
+
+void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc)
+{
+	int rc;
+
+	rc = devres_release(dev, devm_gpiod_release, devm_gpiod_match, desc);
+	WARN_ON(rc);
+}
+
 static int gpio_post_bind(struct udevice *dev)
 {
 	struct udevice *child;
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index a57dd2665c..ad6979a8ce 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -701,4 +701,51 @@ int gpio_get_number(const struct gpio_desc *desc);
  */
 int gpio_get_acpi(const struct gpio_desc *desc, struct acpi_gpio *gpio);

+/**
+ * devm_gpiod_get_index - Resource-managed gpiod_get()
+ * @dev:	GPIO consumer
+ * @con_id:	function within the GPIO consumer
+ * @index:	index of the GPIO to obtain in the consumer
+ * @flags:	optional GPIO initialization flags
+ *
+ * Managed gpiod_get(). GPIO descriptors returned from this function are
+ * automatically disposed on driver detach.
+ * Return the GPIO descriptor corresponding to the function con_id of device
+ * dev, -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 GPIO.
+ */
+struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id,
+				       unsigned int index, int flags);
+
+#define devm_gpiod_get(dev, id, flags) devm_gpiod_get_index(dev, id, 0, flags)
+/**
+ * gpiod_get_optional - obtain an optional GPIO for a given GPIO function
+ * @dev: GPIO consumer, can be NULL for system-global GPIOs
+ * @con_id: function within the GPIO consumer
+ * @index:	index of the GPIO to obtain in the consumer
+ * @flags: optional GPIO initialization flags
+ *
+ * This is equivalent to devm_gpiod_get(), except that when no GPIO was
+ * assigned to the requested function it will return NULL. This is convenient
+ * for drivers that need to handle optional GPIOs.
+ */
+struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev,
+						const char *id,
+						unsigned int index,
+						int flags);
+
+#define devm_gpiod_get_optional(dev, id, flags) \
+	devm_gpiod_get_index_optional(dev, id, 0, flags)
+
+/**
+ * devm_gpiod_put - Resource-managed gpiod_put()
+ * @dev:	GPIO consumer
+ * @desc:	GPIO descriptor to dispose of
+ *
+ * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
+ * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
+ * will be disposed of by the resource management code.
+ */
+void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc);
+
 #endif	/* _ASM_GENERIC_GPIO_H_ */
--
2.28.0

[PATCH v4 2/2] test: gpio: Add tests for the managed API

[Pratyush Yadav]

From: Jean-Jacques Hiblot <jjhiblot@ti.com>

Add a test to verify that GPIOs can be acquired/released using the managed
API. Also check that the GPIOs are released when the consumer device is
removed.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 arch/sandbox/dts/test.dts |  10 ++++
 test/dm/bus.c             |   2 +-
 test/dm/gpio.c            | 102 ++++++++++++++++++++++++++++++++++++++
 test/dm/test-fdt.c        |   6 +--
 4 files changed, 116 insertions(+), 4 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 9f45c48e4e..d35aa6d1f5 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -121,6 +121,9 @@
 			<&gpio_c 5 GPIO_IN>,
 			<&gpio_c 6 (GPIO_ACTIVE_LOW|GPIO_OUT|GPIO_OPEN_DRAIN)>,
 			<&gpio_c 7 (GPIO_ACTIVE_LOW|GPIO_OUT|GPIO_OPEN_SOURCE)>;
+		test4-gpios = <&gpio_a 14>, <&gpio_b 4 1 3 2 1>;
+		test5-gpios = <&gpio_a 19>;
+
 		int-value = <1234>;
 		uint-value = <(-1234)>;
 		int64-value = /bits/ 64 <0x1111222233334444>;
@@ -270,6 +273,13 @@
 		compatible = "denx,u-boot-devres-test";
 	};

+	another-test {
+		reg = <0 2>;
+		compatible = "denx,u-boot-fdt-test";
+		test4-gpios = <&gpio_a 14>, <&gpio_b 4 1 3 2 1>;
+		test5-gpios = <&gpio_a 19>;
+	};
+
 	acpi_test1: acpi-test {
 		compatible = "denx,u-boot-acpi-test";
 		acpi-ssdt-test-data = "ab";
diff --git a/test/dm/bus.c b/test/dm/bus.c
index 865e8bd9fb..27b7266645 100644
--- a/test/dm/bus.c
+++ b/test/dm/bus.c
@@ -120,7 +120,7 @@ UCLASS_DRIVER(testbus) = {
 /* Test that we can probe for children */
 static int dm_test_bus_children(struct unit_test_state *uts)
 {
-	int num_devices = 8;
+	int num_devices = 9;
 	struct udevice *bus;
 	struct uclass *uc;

diff --git a/test/dm/gpio.c b/test/dm/gpio.c
index 4848152644..54e960b185 100644
--- a/test/dm/gpio.c
+++ b/test/dm/gpio.c
@@ -10,6 +10,7 @@
 #include <malloc.h>
 #include <acpi/acpi_device.h>
 #include <asm/gpio.h>
+#include <dm/device-internal.h>
 #include <dm/root.h>
 #include <dm/test.h>
 #include <dm/util.h>
@@ -480,3 +481,104 @@ static int dm_test_gpio_get_acpi_irq(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_gpio_get_acpi_irq, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+/* Test that we can get/release GPIOs using managed API */
+static int dm_test_gpio_devm(struct unit_test_state *uts)
+{
+	static const u32 flags = GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE;
+	struct gpio_desc *desc1, *desc2, *desc3, *desc_err;
+	struct udevice *dev;
+	struct udevice *dev2;
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_TEST_FDT, "a-test",
+					      &dev));
+	ut_assertok(uclass_get_device_by_name(UCLASS_TEST_FDT, "another-test",
+					      &dev2));
+
+	/* Get 3 GPIOs from 'a-test' dev */
+	desc1 = devm_gpiod_get_index(dev, "test4", 0, flags);
+	ut_assert(!IS_ERR(desc1));
+	desc2 = devm_gpiod_get_index(dev, "test4", 1, flags);
+	ut_assert(!IS_ERR(desc2));
+	desc3 = devm_gpiod_get_index_optional(dev, "test5", 0, flags);
+	ut_assert(!IS_ERR(desc3));
+	ut_assert(desc3);
+
+	/*
+	 * Try get the same 3 GPIOs from 'a-test' and 'another-test' devices.
+	 * check that it fails
+	 */
+	desc_err = devm_gpiod_get_index(dev, "test4", 0, flags);
+	ut_asserteq(-EBUSY, PTR_ERR(desc_err));
+	desc_err = devm_gpiod_get_index(dev2, "test4", 0, flags);
+	ut_asserteq(-EBUSY, PTR_ERR(desc_err));
+	desc_err = devm_gpiod_get_index(dev, "test4", 1, flags);
+	ut_asserteq(-EBUSY, PTR_ERR(desc_err));
+	desc_err = devm_gpiod_get_index(dev2, "test4", 1, flags);
+	ut_asserteq(-EBUSY, PTR_ERR(desc_err));
+	desc_err = devm_gpiod_get_index_optional(dev, "test5", 0, flags);
+	ut_asserteq_ptr(NULL, desc_err);
+	desc_err = devm_gpiod_get_index_optional(dev2, "test5", 0, flags);
+	ut_asserteq_ptr(NULL, desc_err);
+
+	/* Try get GPIOs outside of the list */
+	desc_err = devm_gpiod_get_index(dev, "test4", 2, flags);
+	ut_assert(IS_ERR(desc_err));
+	desc_err = devm_gpiod_get_index_optional(dev, "test5", 1, flags);
+	ut_asserteq_ptr(NULL, desc_err);
+
+	/* Manipulate the GPIOs */
+	ut_assertok(dm_gpio_set_value(desc1, 1));
+	ut_asserteq(1, dm_gpio_get_value(desc1));
+	ut_assertok(dm_gpio_set_value(desc1, 0));
+	ut_asserteq(0, dm_gpio_get_value(desc1));
+
+	ut_assertok(dm_gpio_set_value(desc2, 1));
+	ut_asserteq(1, dm_gpio_get_value(desc2));
+	ut_assertok(dm_gpio_set_value(desc2, 0));
+	ut_asserteq(0, dm_gpio_get_value(desc2));
+
+	ut_assertok(dm_gpio_set_value(desc3, 1));
+	ut_asserteq(1, dm_gpio_get_value(desc3));
+	ut_assertok(dm_gpio_set_value(desc3, 0));
+	ut_asserteq(0, dm_gpio_get_value(desc3));
+
+	/* Check that the GPIO cannot be owned by more than one device */
+	desc_err = devm_gpiod_get_index(dev2, "test4", 0, flags);
+	ut_asserteq(-EBUSY, PTR_ERR(desc_err));
+	desc_err = devm_gpiod_get_index(dev2, "test4", 1, flags);
+	ut_asserteq(-EBUSY, PTR_ERR(desc_err));
+	desc_err = devm_gpiod_get_index_optional(dev2, "test5", 0, flags);
+	ut_asserteq_ptr(NULL, desc_err);
+
+	/*
+	 * Release one GPIO and check that we can get it back using
+	 * 'another-test' and then 'a-test'
+	 */
+	devm_gpiod_put(dev, desc2);
+	desc2 = devm_gpiod_get_index(dev2, "test4", 1, flags);
+	ut_assert(!IS_ERR(desc2));
+
+	devm_gpiod_put(dev2, desc2);
+	desc2 = devm_gpiod_get_index(dev, "test4", 1, flags);
+	ut_assert(!IS_ERR(desc2));
+
+	/* Release one GPIO before removing the 'a-test' dev. */
+	devm_gpiod_put(dev, desc2);
+	device_remove(dev, DM_REMOVE_NORMAL);
+
+	/* All the GPIOs must have been freed. We should be able to claim
+	 * them with the 'another-test' device.
+	 */
+	desc1 = devm_gpiod_get_index(dev2, "test4", 0, flags);
+	ut_assert(!IS_ERR(desc1));
+	desc2 = devm_gpiod_get_index(dev2, "test4", 1, flags);
+	ut_assert(!IS_ERR(desc2));
+	desc3 = devm_gpiod_get_index_optional(dev2, "test5", 0, flags);
+	ut_assert(!IS_ERR(desc3));
+	ut_assert(desc3);
+
+	device_remove(dev2, DM_REMOVE_NORMAL);
+	return 0;
+}
+DM_TEST(dm_test_gpio_devm, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index 04802deb7f..26d57f40d1 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -251,7 +251,7 @@ int dm_check_devices(struct unit_test_state *uts, int num_devices)
 /* Test that FDT-based binding works correctly */
 static int dm_test_fdt(struct unit_test_state *uts)
 {
-	const int num_devices = 8;
+	const int num_devices = 9;
 	struct udevice *dev;
 	struct uclass *uc;
 	int ret;
@@ -473,12 +473,12 @@ static int dm_test_uclass_foreach(struct unit_test_state *uts)
 	count = 0;
 	uclass_id_foreach_dev(UCLASS_TEST_FDT, dev, uc)
 		count++;
-	ut_asserteq(8, count);
+	ut_asserteq(9, count);

 	count = 0;
 	uclass_foreach_dev(dev, uc)
 		count++;
-	ut_asserteq(8, count);
+	ut_asserteq(9, count);

 	return 0;
 }
--
2.28.0

[PATCH v4 1/2] drivers: gpio: Add a managed API to get a GPIO from the device-tree

[Heinrich Schuchardt]

On 08.09.20 07:40, Pratyush Yadav wrote:
> From: Jean-Jacques Hiblot <jjhiblot@ti.com>
>
> Add managed functions to get a gpio from the devce-tree, based on a
> property name (minus the '-gpios' suffix) and optionally an index.
>
> When the device is unbound, the GPIO is automatically released and the
> data structure is freed.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/gpio/gpio-uclass.c | 71 ++++++++++++++++++++++++++++++++++++++
>  include/asm-generic/gpio.h | 47 +++++++++++++++++++++++++
>  2 files changed, 118 insertions(+)
>
> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> index 9c53299b6a..0c01413b58 100644
> --- a/drivers/gpio/gpio-uclass.c
> +++ b/drivers/gpio/gpio-uclass.c
> @@ -6,6 +6,8 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <log.h>
> +#include <dm/devres.h>
> +#include <dm/device_compat.h>
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
>  #include <dm/uclass-internal.h>
> @@ -1209,6 +1211,75 @@ int gpio_dev_request_index(struct udevice *dev, const char *nodename,
>  				 flags, 0, dev);
>  }
>
> +static void devm_gpiod_release(struct udevice *dev, void *res)
> +{
> +	dm_gpio_free(dev, res);
> +}
> +
> +static int devm_gpiod_match(struct udevice *dev, void *res, void *data)
> +{
> +	return res == data;
> +}
> +
> +struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id,
> +				       unsigned int index, int flags)
> +{
> +	int rc;
> +	struct gpio_desc *desc;
> +	char *propname;
> +	static const char suffix[] = "-gpios";
> +
> +	propname = malloc(strlen(id) + sizeof(suffix));
> +	if (!propname) {
> +		rc = -ENOMEM;
> +		goto end;
> +	}
> +
> +	strcpy(propname, id);
> +	strcat(propname, suffix);
> +
> +	desc = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc),
> +			    __GFP_ZERO);
> +	if (unlikely(!desc)) {
> +		rc = -ENOMEM;
> +		goto end;
> +	}
> +
> +	rc = gpio_request_by_name(dev, propname, index, desc, flags);
> +
> +end:
> +	if (propname)
> +		free(propname);
> +
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	devres_add(dev, desc);
> +
> +	return desc;
> +}
> +
> +struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev,
> +						const char *id,
> +						unsigned int index,
> +						int flags)
> +{
> +	struct gpio_desc *desc = devm_gpiod_get_index(dev, id, index, flags);
> +
> +	if (IS_ERR(desc))
> +		return NULL;
> +
> +	return desc;
> +}
> +
> +void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc)
> +{
> +	int rc;
> +
> +	rc = devres_release(dev, devm_gpiod_release, devm_gpiod_match, desc);
> +	WARN_ON(rc);
> +}
> +
>  static int gpio_post_bind(struct udevice *dev)
>  {
>  	struct udevice *child;
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index a57dd2665c..ad6979a8ce 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -701,4 +701,51 @@ int gpio_get_number(const struct gpio_desc *desc);
>   */
>  int gpio_get_acpi(const struct gpio_desc *desc, struct acpi_gpio *gpio);
>
> +/**
> + * devm_gpiod_get_index - Resource-managed gpiod_get()
> + * @dev:	GPIO consumer
> + * @con_id:	function within the GPIO consumer
> + * @index:	index of the GPIO to obtain in the consumer
> + * @flags:	optional GPIO initialization flags
> + *
> + * Managed gpiod_get(). GPIO descriptors returned from this function are
> + * automatically disposed on driver detach.

You pass in a device but write "driver detach". Shouldn't the object be
"device" in the description as in your commit message?

Devices have methods remove() and unbind() but no detach. In the commit
message you speak of unbind(). Please, don't use alternative language.

Please, include the API in the HTML documentation created by 'make
htmldocs'.

Why did you choose the unbind() and not the remove() event for releasing
the GPIOs?

Best regards

Heinrich

> + * Return the GPIO descriptor corresponding to the function con_id of device
> + * dev, -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 GPIO.
> + */
> +struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id,
> +				       unsigned int index, int flags);
> +
> +#define devm_gpiod_get(dev, id, flags) devm_gpiod_get_index(dev, id, 0, flags)
> +/**
> + * gpiod_get_optional - obtain an optional GPIO for a given GPIO function
> + * @dev: GPIO consumer, can be NULL for system-global GPIOs
> + * @con_id: function within the GPIO consumer
> + * @index:	index of the GPIO to obtain in the consumer
> + * @flags: optional GPIO initialization flags
> + *
> + * This is equivalent to devm_gpiod_get(), except that when no GPIO was
> + * assigned to the requested function it will return NULL. This is convenient
> + * for drivers that need to handle optional GPIOs.
> + */
> +struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev,
> +						const char *id,
> +						unsigned int index,
> +						int flags);
> +
> +#define devm_gpiod_get_optional(dev, id, flags) \
> +	devm_gpiod_get_index_optional(dev, id, 0, flags)
> +
> +/**
> + * devm_gpiod_put - Resource-managed gpiod_put()
> + * @dev:	GPIO consumer
> + * @desc:	GPIO descriptor to dispose of
> + *
> + * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
> + * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
> + * will be disposed of by the resource management code.
> + */
> +void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc);
> +
>  #endif	/* _ASM_GENERIC_GPIO_H_ */
> --
> 2.28.0
>

[PATCH v4 1/2] drivers: gpio: Add a managed API to get a GPIO from the device-tree

[Pratyush Yadav]

On 08/09/20 04:12PM, Heinrich Schuchardt wrote:
> On 08.09.20 07:40, Pratyush Yadav wrote:
> > From: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >
> > Add managed functions to get a gpio from the devce-tree, based on a
> > property name (minus the '-gpios' suffix) and optionally an index.
> >
> > When the device is unbound, the GPIO is automatically released and the
> > data structure is freed.
> >
> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/gpio/gpio-uclass.c | 71 ++++++++++++++++++++++++++++++++++++++
> >  include/asm-generic/gpio.h | 47 +++++++++++++++++++++++++
> >  2 files changed, 118 insertions(+)
> >
> > diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> > index 9c53299b6a..0c01413b58 100644
> > --- a/drivers/gpio/gpio-uclass.c
> > +++ b/drivers/gpio/gpio-uclass.c
> > @@ -6,6 +6,8 @@
> >  #include <common.h>
> >  #include <dm.h>
> >  #include <log.h>
> > +#include <dm/devres.h>
> > +#include <dm/device_compat.h>
> >  #include <dm/device-internal.h>
> >  #include <dm/lists.h>
> >  #include <dm/uclass-internal.h>
> > @@ -1209,6 +1211,75 @@ int gpio_dev_request_index(struct udevice *dev, const char *nodename,
> >  				 flags, 0, dev);
> >  }
> >
> > +static void devm_gpiod_release(struct udevice *dev, void *res)
> > +{
> > +	dm_gpio_free(dev, res);
> > +}
> > +
> > +static int devm_gpiod_match(struct udevice *dev, void *res, void *data)
> > +{
> > +	return res == data;
> > +}
> > +
> > +struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id,
> > +				       unsigned int index, int flags)
> > +{
> > +	int rc;
> > +	struct gpio_desc *desc;
> > +	char *propname;
> > +	static const char suffix[] = "-gpios";
> > +
> > +	propname = malloc(strlen(id) + sizeof(suffix));
> > +	if (!propname) {
> > +		rc = -ENOMEM;
> > +		goto end;
> > +	}
> > +
> > +	strcpy(propname, id);
> > +	strcat(propname, suffix);
> > +
> > +	desc = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc),
> > +			    __GFP_ZERO);
> > +	if (unlikely(!desc)) {
> > +		rc = -ENOMEM;
> > +		goto end;
> > +	}
> > +
> > +	rc = gpio_request_by_name(dev, propname, index, desc, flags);
> > +
> > +end:
> > +	if (propname)
> > +		free(propname);
> > +
> > +	if (rc)
> > +		return ERR_PTR(rc);
> > +
> > +	devres_add(dev, desc);
> > +
> > +	return desc;
> > +}
> > +
> > +struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev,
> > +						const char *id,
> > +						unsigned int index,
> > +						int flags)
> > +{
> > +	struct gpio_desc *desc = devm_gpiod_get_index(dev, id, index, flags);
> > +
> > +	if (IS_ERR(desc))
> > +		return NULL;
> > +
> > +	return desc;
> > +}
> > +
> > +void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc)
> > +{
> > +	int rc;
> > +
> > +	rc = devres_release(dev, devm_gpiod_release, devm_gpiod_match, desc);
> > +	WARN_ON(rc);
> > +}
> > +
> >  static int gpio_post_bind(struct udevice *dev)
> >  {
> >  	struct udevice *child;
> > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> > index a57dd2665c..ad6979a8ce 100644
> > --- a/include/asm-generic/gpio.h
> > +++ b/include/asm-generic/gpio.h
> > @@ -701,4 +701,51 @@ int gpio_get_number(const struct gpio_desc *desc);
> >   */
> >  int gpio_get_acpi(const struct gpio_desc *desc, struct acpi_gpio *gpio);
> >
> > +/**
> > + * devm_gpiod_get_index - Resource-managed gpiod_get()
> > + * @dev:	GPIO consumer
> > + * @con_id:	function within the GPIO consumer
> > + * @index:	index of the GPIO to obtain in the consumer
> > + * @flags:	optional GPIO initialization flags
> > + *
> > + * Managed gpiod_get(). GPIO descriptors returned from this function are
> > + * automatically disposed on driver detach.
> 
> You pass in a device but write "driver detach". Shouldn't the object be
> "device" in the description as in your commit message?

Yes, it should be device.
 
> Devices have methods remove() and unbind() but no detach. In the commit
> message you speak of unbind(). Please, don't use alternative language.

Ok.
 
> Please, include the API in the HTML documentation created by 'make
> htmldocs'.

I tried searching for the GPIO API documentation under doc/ but I can't 
find anything. README.gpio doesn't mention it anywhere and doc/api/ has 
no file for gpio. Where do I document the newly added APIs then?
 
> Why did you choose the unbind() and not the remove() event for releasing
> the GPIOs?

I did not. Whoever added the devres API did (git blames Masahiro 
Yamada). device_unbind() calls devres_release_all() so as a consequence 
GPIO is released when the device is unbound.
 
> Best regards
> 
> Heinrich
> 
> > + * Return the GPIO descriptor corresponding to the function con_id of device
> > + * dev, -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 GPIO.
> > + */
> > +struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const char *id,
> > +				       unsigned int index, int flags);
> > +
> > +#define devm_gpiod_get(dev, id, flags) devm_gpiod_get_index(dev, id, 0, flags)
> > +/**
> > + * gpiod_get_optional - obtain an optional GPIO for a given GPIO function
> > + * @dev: GPIO consumer, can be NULL for system-global GPIOs
> > + * @con_id: function within the GPIO consumer
> > + * @index:	index of the GPIO to obtain in the consumer
> > + * @flags: optional GPIO initialization flags
> > + *
> > + * This is equivalent to devm_gpiod_get(), except that when no GPIO was
> > + * assigned to the requested function it will return NULL. This is convenient
> > + * for drivers that need to handle optional GPIOs.
> > + */
> > +struct gpio_desc *devm_gpiod_get_index_optional(struct udevice *dev,
> > +						const char *id,
> > +						unsigned int index,
> > +						int flags);
> > +
> > +#define devm_gpiod_get_optional(dev, id, flags) \
> > +	devm_gpiod_get_index_optional(dev, id, 0, flags)
> > +
> > +/**
> > + * devm_gpiod_put - Resource-managed gpiod_put()
> > + * @dev:	GPIO consumer
> > + * @desc:	GPIO descriptor to dispose of
> > + *
> > + * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
> > + * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
> > + * will be disposed of by the resource management code.
> > + */
> > +void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc);
> > +
> >  #endif	/* _ASM_GENERIC_GPIO_H_ */
> > --
> > 2.28.0
> >
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH v4 1/2] drivers: gpio: Add a managed API to get a GPIO from the device-tree

[Heinrich Schuchardt]

Am 8. September 2020 17:15:25 MESZ schrieb Pratyush Yadav <p.yadav@ti.com>:
>On 08/09/20 04:12PM, Heinrich Schuchardt wrote:
>> On 08.09.20 07:40, Pratyush Yadav wrote:
>> > From: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> >
>> > Add managed functions to get a gpio from the devce-tree, based on a
>> > property name (minus the '-gpios' suffix) and optionally an index.
>> >
>> > When the device is unbound, the GPIO is automatically released and
>the
>> > data structure is freed.
>> >
>> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> > Reviewed-by: Simon Glass <sjg@chromium.org>
>> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
>> > ---
>> >  drivers/gpio/gpio-uclass.c | 71
>++++++++++++++++++++++++++++++++++++++
>> >  include/asm-generic/gpio.h | 47 +++++++++++++++++++++++++
>> >  2 files changed, 118 insertions(+)
>> >
>> > diff --git a/drivers/gpio/gpio-uclass.c
>b/drivers/gpio/gpio-uclass.c
>> > index 9c53299b6a..0c01413b58 100644
>> > --- a/drivers/gpio/gpio-uclass.c
>> > +++ b/drivers/gpio/gpio-uclass.c
>> > @@ -6,6 +6,8 @@
>> >  #include <common.h>
>> >  #include <dm.h>
>> >  #include <log.h>
>> > +#include <dm/devres.h>
>> > +#include <dm/device_compat.h>
>> >  #include <dm/device-internal.h>
>> >  #include <dm/lists.h>
>> >  #include <dm/uclass-internal.h>
>> > @@ -1209,6 +1211,75 @@ int gpio_dev_request_index(struct udevice
>*dev, const char *nodename,
>> >  				 flags, 0, dev);
>> >  }
>> >
>> > +static void devm_gpiod_release(struct udevice *dev, void *res)
>> > +{
>> > +	dm_gpio_free(dev, res);
>> > +}
>> > +
>> > +static int devm_gpiod_match(struct udevice *dev, void *res, void
>*data)
>> > +{
>> > +	return res == data;
>> > +}
>> > +
>> > +struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const
>char *id,
>> > +				       unsigned int index, int flags)
>> > +{
>> > +	int rc;
>> > +	struct gpio_desc *desc;
>> > +	char *propname;
>> > +	static const char suffix[] = "-gpios";
>> > +
>> > +	propname = malloc(strlen(id) + sizeof(suffix));
>> > +	if (!propname) {
>> > +		rc = -ENOMEM;
>> > +		goto end;
>> > +	}
>> > +
>> > +	strcpy(propname, id);
>> > +	strcat(propname, suffix);
>> > +
>> > +	desc = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc),
>> > +			    __GFP_ZERO);
>> > +	if (unlikely(!desc)) {
>> > +		rc = -ENOMEM;
>> > +		goto end;
>> > +	}
>> > +
>> > +	rc = gpio_request_by_name(dev, propname, index, desc, flags);
>> > +
>> > +end:
>> > +	if (propname)
>> > +		free(propname);
>> > +
>> > +	if (rc)
>> > +		return ERR_PTR(rc);
>> > +
>> > +	devres_add(dev, desc);
>> > +
>> > +	return desc;
>> > +}
>> > +
>> > +struct gpio_desc *devm_gpiod_get_index_optional(struct udevice
>*dev,
>> > +						const char *id,
>> > +						unsigned int index,
>> > +						int flags)
>> > +{
>> > +	struct gpio_desc *desc = devm_gpiod_get_index(dev, id, index,
>flags);
>> > +
>> > +	if (IS_ERR(desc))
>> > +		return NULL;
>> > +
>> > +	return desc;
>> > +}
>> > +
>> > +void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc)
>> > +{
>> > +	int rc;
>> > +
>> > +	rc = devres_release(dev, devm_gpiod_release, devm_gpiod_match,
>desc);
>> > +	WARN_ON(rc);
>> > +}
>> > +
>> >  static int gpio_post_bind(struct udevice *dev)
>> >  {
>> >  	struct udevice *child;
>> > diff --git a/include/asm-generic/gpio.h
>b/include/asm-generic/gpio.h
>> > index a57dd2665c..ad6979a8ce 100644
>> > --- a/include/asm-generic/gpio.h
>> > +++ b/include/asm-generic/gpio.h
>> > @@ -701,4 +701,51 @@ int gpio_get_number(const struct gpio_desc
>*desc);
>> >   */
>> >  int gpio_get_acpi(const struct gpio_desc *desc, struct acpi_gpio
>*gpio);
>> >
>> > +/**
>> > + * devm_gpiod_get_index - Resource-managed gpiod_get()
>> > + * @dev:	GPIO consumer
>> > + * @con_id:	function within the GPIO consumer
>> > + * @index:	index of the GPIO to obtain in the consumer
>> > + * @flags:	optional GPIO initialization flags
>> > + *
>> > + * Managed gpiod_get(). GPIO descriptors returned from this
>function are
>> > + * automatically disposed on driver detach.
>> 
>> You pass in a device but write "driver detach". Shouldn't the object
>be
>> "device" in the description as in your commit message?
>
>Yes, it should be device.
> 
>> Devices have methods remove() and unbind() but no detach. In the
>commit
>> message you speak of unbind(). Please, don't use alternative
>language.
>
>Ok.
> 
>> Please, include the API in the HTML documentation created by 'make
>> htmldocs'.
>
>I tried searching for the GPIO API documentation under doc/ but I can't
>
>find anything. README.gpio doesn't mention it anywhere and doc/api/ has
>
>no file for gpio. Where do I document the newly added APIs then?

Please, add a new file doc/api/gpio.rst and include your include there. Add gpio.rst to doc/api/index.rst.

Check that make htmldocs does not show warnings.

The documentation will be generated in doc/output.

Best regards

Heinrich


> 
>> Why did you choose the unbind() and not the remove() event for
>releasing
>> the GPIOs?
>
>I did not. Whoever added the devres API did (git blames Masahiro 
>Yamada). device_unbind() calls devres_release_all() so as a consequence
>
>GPIO is released when the device is unbound.
> 
>> Best regards
>> 
>> Heinrich
>> 
>> > + * Return the GPIO descriptor corresponding to the function con_id
>of device
>> > + * dev, -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 GPIO.
>> > + */
>> > +struct gpio_desc *devm_gpiod_get_index(struct udevice *dev, const
>char *id,
>> > +				       unsigned int index, int flags);
>> > +
>> > +#define devm_gpiod_get(dev, id, flags) devm_gpiod_get_index(dev,
>id, 0, flags)
>> > +/**
>> > + * gpiod_get_optional - obtain an optional GPIO for a given GPIO
>function
>> > + * @dev: GPIO consumer, can be NULL for system-global GPIOs
>> > + * @con_id: function within the GPIO consumer
>> > + * @index:	index of the GPIO to obtain in the consumer
>> > + * @flags: optional GPIO initialization flags
>> > + *
>> > + * This is equivalent to devm_gpiod_get(), except that when no
>GPIO was
>> > + * assigned to the requested function it will return NULL. This is
>convenient
>> > + * for drivers that need to handle optional GPIOs.
>> > + */
>> > +struct gpio_desc *devm_gpiod_get_index_optional(struct udevice
>*dev,
>> > +						const char *id,
>> > +						unsigned int index,
>> > +						int flags);
>> > +
>> > +#define devm_gpiod_get_optional(dev, id, flags) \
>> > +	devm_gpiod_get_index_optional(dev, id, 0, flags)
>> > +
>> > +/**
>> > + * devm_gpiod_put - Resource-managed gpiod_put()
>> > + * @dev:	GPIO consumer
>> > + * @desc:	GPIO descriptor to dispose of
>> > + *
>> > + * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
>> > + * devm_gpiod_get_index(). Normally this function will not be
>called as the GPIO
>> > + * will be disposed of by the resource management code.
>> > + */
>> > +void devm_gpiod_put(struct udevice *dev, struct gpio_desc *desc);
>> > +
>> >  #endif	/* _ASM_GENERIC_GPIO_H_ */
>> > --
>> > 2.28.0
>> >
>> 

[PATCH v4 2/2] test: gpio: Add tests for the managed API

[Simon Glass]

On Mon, 7 Sep 2020 at 23:40, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> From: Jean-Jacques Hiblot <jjhiblot@ti.com>
>
> Add a test to verify that GPIOs can be acquired/released using the managed
> API. Also check that the GPIOs are released when the consumer device is
> removed.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  arch/sandbox/dts/test.dts |  10 ++++
>  test/dm/bus.c             |   2 +-
>  test/dm/gpio.c            | 102 ++++++++++++++++++++++++++++++++++++++
>  test/dm/test-fdt.c        |   6 +--
>  4 files changed, 116 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Although I don't see a change log, so this is a bit blind.

[PATCH v4 2/2] test: gpio: Add tests for the managed API

[Pratyush Yadav]

On 08/09/20 05:56PM, Simon Glass wrote:
> On Mon, 7 Sep 2020 at 23:40, Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > From: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >
> > Add a test to verify that GPIOs can be acquired/released using the managed
> > API. Also check that the GPIOs are released when the consumer device is
> > removed.
> >
> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  arch/sandbox/dts/test.dts |  10 ++++
> >  test/dm/bus.c             |   2 +-
> >  test/dm/gpio.c            | 102 ++++++++++++++++++++++++++++++++++++++
> >  test/dm/test-fdt.c        |   6 +--
> >  4 files changed, 116 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks.
 
> Although I don't see a change log, so this is a bit blind.

The changelog is in the cover letter [0]. Copy-pasing the parts relevant 
to this patch:

- Move "another-test" node down the order to make sure
  dm_test_fdt_uclass_seq() continues to work.
- Bump num_devices to 9 in dm_test_bus_children(), dm_test_fdt(), and
  dm_test_uclass_foreach() to fix broken tests.
- s/DM_TESTF/UT_TESTF/g

[0] I haven't figured out a workflow that lets me easily record the
changelog on a per-patch basis. Maybe I can write some scripts around 
git-notes for that? Dunno.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH v4 2/2] test: gpio: Add tests for the managed API

[Simon Glass]

Hi Pratyush,

On Tue, 8 Sep 2020 at 21:53, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> On 08/09/20 05:56PM, Simon Glass wrote:
> > On Mon, 7 Sep 2020 at 23:40, Pratyush Yadav <p.yadav@ti.com> wrote:
> > >
> > > From: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > >
> > > Add a test to verify that GPIOs can be acquired/released using the managed
> > > API. Also check that the GPIOs are released when the consumer device is
> > > removed.
> > >
> > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > > ---
> > >  arch/sandbox/dts/test.dts |  10 ++++
> > >  test/dm/bus.c             |   2 +-
> > >  test/dm/gpio.c            | 102 ++++++++++++++++++++++++++++++++++++++
> > >  test/dm/test-fdt.c        |   6 +--
> > >  4 files changed, 116 insertions(+), 4 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Thanks.
>
> > Although I don't see a change log, so this is a bit blind.
>
> The changelog is in the cover letter [0]. Copy-pasing the parts relevant
> to this patch:
>
> - Move "another-test" node down the order to make sure
>   dm_test_fdt_uclass_seq() continues to work.
> - Bump num_devices to 9 in dm_test_bus_children(), dm_test_fdt(), and
>   dm_test_uclass_foreach() to fix broken tests.
> - s/DM_TESTF/UT_TESTF/g

OK ta. I don't see it if it is in the cover letter. It's nice to have
it in both places.

>
> [0] I haven't figured out a workflow that lets me easily record the
> changelog on a per-patch basis. Maybe I can write some scripts around
> git-notes for that? Dunno.

You can use patman which is conveniently in the U-Boot tree and does
this along with automation of various other steps for emailing
patches.

Regards,
Simon