[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 at 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 at ti.com>
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> > Signed-off-by: Pratyush Yadav <p.yadav at 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