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

[PATCH v2 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
API to existing infrastructure (GPIO, reset, regmap (already
submitted)).

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", ...)

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 unblocked.

  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. For now the
  responsibility lies on the drivers by default. 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 |  70 +++++++++++++++++++++++++
 include/asm-generic/gpio.h |  47 +++++++++++++++++
 test/dm/gpio.c             | 102 +++++++++++++++++++++++++++++++++++++
 4 files changed, 229 insertions(+)

--
2.26.2

[PATCH v2 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>
---
 drivers/gpio/gpio-uclass.c | 70 ++++++++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h | 47 +++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 9eeab22eef..fd868608fc 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>
@@ -1141,6 +1143,74 @@ 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 e16c2f31d9..76e0e902c4 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -674,4 +674,51 @@ int dm_gpio_get_dir_flags(struct gpio_desc *desc, ulong *flags);
  */
 int gpio_get_number(const struct gpio_desc *desc);
 
+/**
+ * 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.26.2

[PATCH v2 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>
---
 arch/sandbox/dts/test.dts |  10 ++++
 test/dm/gpio.c            | 102 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 5ce5e28476..a8618ccade 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -107,6 +107,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>;
@@ -114,6 +117,13 @@
 		interrupts-extended = <&irq 3 0>;
 	};
 
+	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>;
+	};
+
 	junk {
 		reg = <1 1>;
 		compatible = "not,compatible";
diff --git a/test/dm/gpio.c b/test/dm/gpio.c
index b5ee4e4f87..40bea32b13 100644
--- a/test/dm/gpio.c
+++ b/test/dm/gpio.c
@@ -8,6 +8,7 @@
 #include <dm.h>
 #include <log.h>
 #include <malloc.h>
+#include <dm/device-internal.h>
 #include <dm/root.h>
 #include <dm/test.h>
 #include <dm/util.h>
@@ -385,3 +386,104 @@ static int dm_test_gpio_get_dir_flags(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_gpio_get_dir_flags, DM_TESTF_SCAN_PDATA | DM_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, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
-- 
2.26.2

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

[Simon Glass]

Hi Pratyush,

On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> 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
> API to existing infrastructure (GPIO, reset, regmap (already
> submitted)).
>
> 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", ...)
>
> 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 unblocked.
>
>   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. For now the
>   responsibility lies on the drivers by default. 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 |  70 +++++++++++++++++++++++++
>  include/asm-generic/gpio.h |  47 +++++++++++++++++
>  test/dm/gpio.c             | 102 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 229 insertions(+)
>
> --
> 2.26.2
>

The first question I have is why do you want to allocate the gpio_desc
and return it? Doesn't the caller have a place for that in its private
struct?

Regards,
Simon

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

[Pratyush Yadav]

On 31/05/20 08:08AM, Simon Glass wrote:
> Hi Pratyush,
> 
> On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > 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
> > API to existing infrastructure (GPIO, reset, regmap (already
> > submitted)).
> >
> > 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", ...)
> >
> > 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 unblocked.
> >
> >   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. For now the
> >   responsibility lies on the drivers by default. 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 |  70 +++++++++++++++++++++++++
> >  include/asm-generic/gpio.h |  47 +++++++++++++++++
> >  test/dm/gpio.c             | 102 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 229 insertions(+)
> >
> > --
> > 2.26.2
> >
> 
> The first question I have is why do you want to allocate the gpio_desc
> and return it? Doesn't the caller have a place for that in its private
> struct?

Ask the Linux folks that ;-)

The main aim of this series is to make it easier to port and maintain 
drivers from Linux. The less changes we have to make when porting a 
driver, the easier it is to port future fixes and features.

Linux drivers (like the TI J721E WIZ [0] for which this effort is mainly 
being made) use these APIs. FWIW, the docs in Linux say the optional 
wrappers to the functions are added as a convenience for drivers that 
need to handle optional GPIOs.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/ti/phy-j721e-wiz.c

-- 
Regards,
Pratyush Yadav
Texas Instruments India

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

[Simon Glass]

Hi Pratyush,

On Mon, 1 Jun 2020 at 05:22, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> On 31/05/20 08:08AM, Simon Glass wrote:
> > Hi Pratyush,
> >
> > On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote:
> > >
> > > 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
> > > API to existing infrastructure (GPIO, reset, regmap (already
> > > submitted)).
> > >
> > > 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", ...)
> > >
> > > 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 unblocked.
> > >
> > >   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. For now the
> > >   responsibility lies on the drivers by default. 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 |  70 +++++++++++++++++++++++++
> > >  include/asm-generic/gpio.h |  47 +++++++++++++++++
> > >  test/dm/gpio.c             | 102 +++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 229 insertions(+)
> > >
> > > --
> > > 2.26.2
> > >
> >
> > The first question I have is why do you want to allocate the gpio_desc
> > and return it? Doesn't the caller have a place for that in its private
> > struct?
>
> Ask the Linux folks that ;-)
>
> The main aim of this series is to make it easier to port and maintain
> drivers from Linux. The less changes we have to make when porting a
> driver, the easier it is to port future fixes and features.
>
> Linux drivers (like the TI J721E WIZ [0] for which this effort is mainly
> being made) use these APIs. FWIW, the docs in Linux say the optional
> wrappers to the functions are added as a convenience for drivers that
> need to handle optional GPIOs.

U-Boot already supports optional GPIOs.

Can we put this behind a CONFIG_LINUX_COMPAT_GPIO flag perhaps, so
people know they are trading off code / memory size for compatibility?

>
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/ti/phy-j721e-wiz.c
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments India

Regards,
Simon

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

[Pratyush Yadav]

Hi Simon,

On 01/06/20 08:45AM, Simon Glass wrote:
> Hi Pratyush,
> 
> On Mon, 1 Jun 2020 at 05:22, Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > On 31/05/20 08:08AM, Simon Glass wrote:
> > > Hi Pratyush,
> > >
> > > On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote:
> > > >
> > > > 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
> > > > API to existing infrastructure (GPIO, reset, regmap (already
> > > > submitted)).
> > > >
> > > > 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", ...)
> > > >
> > > > 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 unblocked.
> > > >
> > > >   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. For now the
> > > >   responsibility lies on the drivers by default. 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 |  70 +++++++++++++++++++++++++
> > > >  include/asm-generic/gpio.h |  47 +++++++++++++++++
> > > >  test/dm/gpio.c             | 102 +++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 229 insertions(+)
> > > >
> > > > --
> > > > 2.26.2
> > > >
> > >
> > > The first question I have is why do you want to allocate the gpio_desc
> > > and return it? Doesn't the caller have a place for that in its private
> > > struct?
> >
> > Ask the Linux folks that ;-)
> >
> > The main aim of this series is to make it easier to port and maintain
> > drivers from Linux. The less changes we have to make when porting a
> > driver, the easier it is to port future fixes and features.
> >
> > Linux drivers (like the TI J721E WIZ [0] for which this effort is mainly
> > being made) use these APIs. FWIW, the docs in Linux say the optional
> > wrappers to the functions are added as a convenience for drivers that
> > need to handle optional GPIOs.
> 
> U-Boot already supports optional GPIOs.

I seem to have mixed up some things when sending the reply. Sorry. This 
patchset doesn't really have all that much to do with optional GPIOs. It 
is about adding managed counterparts to the GPIO APIs.
 
> Can we put this behind a CONFIG_LINUX_COMPAT_GPIO flag perhaps, so
> people know they are trading off code / memory size for compatibility?

The decision to put these behind a config depends on what U-Boot's 
stance is on the "devm_*" functions in general. I see that there are 
devm_clk_*() functions in mainline already and they aren't beind a 
Kconfig option. A quick search tells me as of now no other subsystem has 
devm_* functions in mainline, though you did drop your Reviewed-by on 
the regmap patch that adds devm_regmap_init() [0] and it isn't guarded 
by a Kconfig option.

So as of now I see the stance is to add devm_* functions without 
wrapping them in a compile time switch. If we don't want to make 
building them mandatory (at the added cognitive cost of code that harder 
to reason about because of the multiple ways you can build it), that can 
be done but IMO we should be consistent about what we put behind a 
config.

One note I have about devm_gpiod_get_index() (which is the main source 
of new code) is that I don't think it is possible to have a dummy 
function in its stead since it is not a direct call to a lower level 
function and instead it plays around with the propname suffix. So if we 
do make it optional, boards with drivers using it should always make 
sure the config is enabled or their build will fail.

[0] https://patchwork.ozlabs.org/project/uboot/patch/20200605203025.15466-2-p.yadav at ti.com/

-- 
Regards,
Pratyush Yadav
Texas Instruments India

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

[Simon Glass]

Hi Pratyush,

On Fri, 29 May 2020 at 15:38, Pratyush Yadav <p.yadav@ti.com> 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>
> ---
>  drivers/gpio/gpio-uclass.c | 70 ++++++++++++++++++++++++++++++++++++++
>  include/asm-generic/gpio.h | 47 +++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)

Is there a change log for this one?

Regards,
Simon

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

[Simon Glass]

On Fri, 29 May 2020 at 15:38, 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>
> ---
>  arch/sandbox/dts/test.dts |  10 ++++
>  test/dm/gpio.c            | 102 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+)

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

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

[Simon Glass]

Hi Pratyush,

On Mon, 8 Jun 2020 at 12:05, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Simon,
>
> On 01/06/20 08:45AM, Simon Glass wrote:
> > Hi Pratyush,
> >
> > On Mon, 1 Jun 2020 at 05:22, Pratyush Yadav <p.yadav@ti.com> wrote:
> > >
> > > On 31/05/20 08:08AM, Simon Glass wrote:
> > > > Hi Pratyush,
> > > >
> > > > On Fri, 29 May 2020 at 15:39, Pratyush Yadav <p.yadav@ti.com> wrote:
> > > > >
> > > > > 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
> > > > > API to existing infrastructure (GPIO, reset, regmap (already
> > > > > submitted)).
> > > > >
> > > > > 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", ...)
> > > > >
> > > > > 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 unblocked.
> > > > >
> > > > >   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. For now the
> > > > >   responsibility lies on the drivers by default. 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 |  70 +++++++++++++++++++++++++
> > > > >  include/asm-generic/gpio.h |  47 +++++++++++++++++
> > > > >  test/dm/gpio.c             | 102 +++++++++++++++++++++++++++++++++++++
> > > > >  4 files changed, 229 insertions(+)
> > > > >
> > > > > --
> > > > > 2.26.2
> > > > >
> > > >
> > > > The first question I have is why do you want to allocate the gpio_desc
> > > > and return it? Doesn't the caller have a place for that in its private
> > > > struct?
> > >
> > > Ask the Linux folks that ;-)
> > >
> > > The main aim of this series is to make it easier to port and maintain
> > > drivers from Linux. The less changes we have to make when porting a
> > > driver, the easier it is to port future fixes and features.
> > >
> > > Linux drivers (like the TI J721E WIZ [0] for which this effort is mainly
> > > being made) use these APIs. FWIW, the docs in Linux say the optional
> > > wrappers to the functions are added as a convenience for drivers that
> > > need to handle optional GPIOs.
> >
> > U-Boot already supports optional GPIOs.
>
> I seem to have mixed up some things when sending the reply. Sorry. This
> patchset doesn't really have all that much to do with optional GPIOs. It
> is about adding managed counterparts to the GPIO APIs.
>
> > Can we put this behind a CONFIG_LINUX_COMPAT_GPIO flag perhaps, so
> > people know they are trading off code / memory size for compatibility?
>
> The decision to put these behind a config depends on what U-Boot's
> stance is on the "devm_*" functions in general. I see that there are
> devm_clk_*() functions in mainline already and they aren't beind a
> Kconfig option. A quick search tells me as of now no other subsystem has
> devm_* functions in mainline, though you did drop your Reviewed-by on
> the regmap patch that adds devm_regmap_init() [0] and it isn't guarded
> by a Kconfig option.

It is OK to add new functionality that doesn't increase size if it
isn't used. But I worry with this API that we are changing the
semantics of functions and adding more checks, which does increase
code size, and sow confusion.

>
> So as of now I see the stance is to add devm_* functions without
> wrapping them in a compile time switch. If we don't want to make
> building them mandatory (at the added cognitive cost of code that harder
> to reason about because of the multiple ways you can build it), that can
> be done but IMO we should be consistent about what we put behind a
> config.
>
> One note I have about devm_gpiod_get_index() (which is the main source
> of new code) is that I don't think it is possible to have a dummy
> function in its stead since it is not a direct call to a lower level
> function and instead it plays around with the propname suffix. So if we
> do make it optional, boards with drivers using it should always make
> sure the config is enabled or their build will fail.

Yes indeed. Boards need to choose to use this API and enable it. But
that is the case with lots of U-Boot features so I think it makes
sense.

>
> [0] https://patchwork.ozlabs.org/project/uboot/patch/20200605203025.15466-2-p.yadav at ti.com/
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments India

Regards,
Simon

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

[Pratyush Yadav]

On 16/06/20 05:37PM, Simon Glass wrote:
> Hi Pratyush,
> 
> On Fri, 29 May 2020 at 15:38, Pratyush Yadav <p.yadav@ti.com> 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>
> > ---
> >  drivers/gpio/gpio-uclass.c | 70 ++++++++++++++++++++++++++++++++++++++
> >  include/asm-generic/gpio.h | 47 +++++++++++++++++++++++++
> >  2 files changed, 117 insertions(+)
> 
> Is there a change log for this one?

No changes to this patch from v1 other than adding a couple #include to 
fix compilation errors.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

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

[Simon Glass]

On Fri, 29 May 2020 at 15:38, Pratyush Yadav <p.yadav@ti.com> 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>
> ---
>  drivers/gpio/gpio-uclass.c | 70 ++++++++++++++++++++++++++++++++++++++
>  include/asm-generic/gpio.h | 47 +++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
>

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

nit below

> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> index 9eeab22eef..fd868608fc 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>
> @@ -1141,6 +1143,74 @@ 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);

blank line here

> +       return desc;
> +}
> +
Regards,
Simon