[U-Boot] [PATCH v4] gpio: add gpio-hog support

[Patrick DELAUNAY <patrick.delaunay@st.com>]

Hi Mickael

> Subject: Re: [PATCH v4] gpio: add gpio-hog support
> Importance: High
> 
> On 12. 06. 19 6:11, Heiko Schocher wrote:
> > add gpio-hog support. GPIO hogging is a mechanism providing automatic
> > GPIO request and configuration as part of the gpio-controller's driver
> > probe function.
> >
> > for more infos see:
> > doc/device-tree-bindings/gpio/gpio.txt
> >
> > Signed-off-by: Heiko Schocher <hs@denx.de>
> >
> > ---
> > clean travis build, see result:
> > https://travis-ci.org/hsdenx/u-boot-test/builds/544040419
> >
> > Changes in v4:
> > - rework as suggested by Patrick
> >   based on patch:
> >   http://patchwork.ozlabs.org/patch/1098913/
> >   use new UCLASS_NOP
> >   hog detection + DT parsing in gpio_post_bind() .... info saved in platdata
> >   gpio configuration for hog (request and set_dir) => probe function
> >
> > Changes in v3:
> > - probe now all gpio devices with gpio-hogs
> >   from board_r before board_late_init() call
> >   or if someone wants to access gpio-hog
> >   based gpios with gpio_hog_lookup_name()
> >   This is needed, because we cannot be sure,
> >   if a gpio device which contains gpio-hogs
> >   is probed.
> > - add line-name property as Michal recommended
> > - renamed gpio_priv_one into gpio_priv_one_hog
> >   and protected through CONFIG_DM_GPIO_HOG
> >
> > Changes in v2:
> > - move gpio_hog() call from post_probe() to post_bind()
> >   call. Check if current gpio device has gpio-hog
> >   definitions, if so, probe it.
> >
> >  common/board_r.c                       |   6 +
> >  doc/device-tree-bindings/gpio/gpio.txt |  55 ++++++++
> >  drivers/gpio/Kconfig                   |  10 ++
> >  drivers/gpio/gpio-uclass.c             | 181 ++++++++++++++++++++++---
> >  include/asm-generic/gpio.h             |  32 +++++
> >  5 files changed, 268 insertions(+), 16 deletions(-)
> >
> > diff --git a/common/board_r.c b/common/board_r.c index
> > 150e8cd424..507da4c74a 100644
> > --- a/common/board_r.c
> > +++ b/common/board_r.c
> > @@ -49,6 +49,9 @@
> >  #include <linux/err.h>
> >  #include <efi_loader.h>
> >  #include <wdt.h>
> > +#if defined(CONFIG_DM_GPIO_HOG)
> > +#include <asm/gpio.h>
> > +#endif
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -796,6 +799,9 @@ static init_fnc_t init_sequence_r[] = {  #ifdef
> > CONFIG_CMD_NET
> >  	initr_ethaddr,
> >  #endif
> > +#if defined(CONFIG_DM_GPIO_HOG)
> > +	gpio_hog_probe_all,
> > +#endif

Any reason to have hog at this place ? 
Before board_late_init....

I expect more before board_init() for my future use-case, 
to replace the current (dirty)  implementation with pinctrl-0

#if defined(CONFIG_WDT)
	initr_watchdog,
#endif
+#if defined(CONFIG_DM_GPIO_HOG)
+	gpio_hog_probe_all,
+#endif
#if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV) || \
	defined(CONFIG_SANDBOX)
	board_init,	/* Setup chipselects */
#endif


In my device tree I have :

	stmfx: stmfx at 42 {
		compatible = "st,stmfx-0300";
		reg = <0x42>;
		interrupts = <8 IRQ_TYPE_EDGE_RISING>;
		interrupt-parent = <&gpioi>;
		vdd-supply = <&v3v3>;

		stmfx_pinctrl: stmfx-pin-controller {
			compatible = "st,stmfx-0300-pinctrl";
			gpio-controller;
			#gpio-cells = <2>;
			interrupt-controller;
			#interrupt-cells = <2>;
			gpio-ranges = <&stmfx_pinctrl 0 0 24>;
			pinctrl-names = "default";
			pinctrl-0 = <&hog_pins>;

			hog_pins: hog {
				pins = "gpio14";
				drive-push-pull;
				bias-pull-down;
			};
	};


And in board, I force probe to configure HOG....before to initialize the DISPLAY, so in 

./board/st/stm32mp1/stm32mp1.c : board _init()

/* board dependent setup after realloc */
int board_init(void)
{
.....
	/* probe all PINCTRL for hog */
	for (uclass_first_device(UCLASS_PINCTRL, &dev);
	     dev;
	     uclass_next_device(&dev)) {
		pr_debug("probe pincontrol = %s\n", dev->name);
	}


Or you can let each board choose when call the function
as it is already done for other function as regulators_enable_boot_on()
So  don't add it in init_sequence_r[]  / board_r.c ?...

Anyway it is not blocking for me, it just a question.

> >  #ifdef CONFIG_BOARD_LATE_INIT
> >  	board_late_init,
> >  #endif
> > diff --git a/doc/device-tree-bindings/gpio/gpio.txt
> > b/doc/device-tree-bindings/gpio/gpio.txt
> > index f7a158d858..e774439369 100644
> > --- a/doc/device-tree-bindings/gpio/gpio.txt
> > +++ b/doc/device-tree-bindings/gpio/gpio.txt
> > @@ -210,3 +210,58 @@ Example 2:
> >  Here, three GPIO ranges are defined wrt. two pin controllers.
> > pinctrl1 GPIO  ranges are defined using pin numbers whereas the GPIO
> > ranges wrt. pinctrl2  are named "foo" and "bar".
> > +
> > +3) GPIO hog definitions
> > +-----------------------
> > +
> > +The GPIO chip may contain GPIO hog definitions. GPIO hogging is a
> > +mechanism providing automatic GPIO request and configuration as part
> > +of the gpio-controller's driver probe function.
> > +
> > +Each GPIO hog definition is represented as a child node of the GPIO
> controller.
> > +Required properties:
> > +- gpio-hog:   A property specifying that this child node represents a GPIO hog.
> > +- gpios:      Store the GPIO information (id, flags) for the GPIO to
> > +	      affect.
> > +
> > +              ! Not yet support more than one gpio !
> > +
> > +Only one of the following properties scanned in the order shown below.
> > +- input:      A property specifying to set the GPIO direction as input.
> > +- output-low  A property specifying to set the GPIO direction as output with
> > +	      the value low.
> > +- output-high A property specifying to set the GPIO direction as output with
> > +	      the value high.
> > +
> > +Optional properties:
> > +- line-name:  The GPIO label name. If not present the node name is used.
> > +
> > +Example:
> > +
> > +        tca6416 at 20 {
> > +                compatible = "ti,tca6416";
> > +                reg = <0x20>;
> > +                #gpio-cells = <2>;
> > +                gpio-controller;
> > +
> > +                env_reset {
> > +                        gpio-hog;
> > +                        input;
> > +                        gpios = <6 GPIO_ACTIVE_LOW>;
> > +                };
> > +                boot_rescue {
> > +                        gpio-hog;
> > +                        input;
> > +                        gpios = <7 GPIO_ACTIVE_LOW>;
> > +                };
> > +        };
> > +
> > +For the above Example you can than access the gpio in your boardcode
> > +with:
> > +
> > +        desc = gpio_hog_lookup_name("boot_rescue.gpio-hog");
> > +        if (desc) {
> > +                if (dm_gpio_get_value(desc))
> > +                        printf("\nBooting into Rescue System\n");
> > +		else
> > +			printf("\nBoot normal\n");
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index
> > e36a8abc42..fa1c99700f 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -14,6 +14,16 @@ config DM_GPIO
> >  	  particular GPIOs that they provide. The uclass interface
> >  	  is defined in include/asm-generic/gpio.h.
> >
> > +config DM_GPIO_HOG
> > +	bool "Enable GPIO hog support"
> > +	depends on DM_GPIO
> > +	default n
> > +	help
> > +	  Enable gpio hog support
> > +	  The GPIO chip may contain GPIO hog definitions. GPIO hogging
> > +	  is a mechanism providing automatic GPIO request and config-
> > +	  uration as part of the gpio-controller's driver probe function.
> > +
> >  config ALTERA_PIO
> >  	bool "Altera PIO driver"
> >  	depends on DM_GPIO
> > diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> > index da5e9ba6e5..308d0863ad 100644
> > --- a/drivers/gpio/gpio-uclass.c
> > +++ b/drivers/gpio/gpio-uclass.c
> > @@ -5,6 +5,9 @@
> >
> >  #include <common.h>
> >  #include <dm.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> > +#include <dm/uclass-internal.h>
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <errno.h>
> >  #include <fdtdec.h>
> > @@ -141,6 +144,118 @@ static int gpio_find_and_xlate(struct gpio_desc
> *desc,
> >  		return gpio_xlate_offs_flags(desc->dev, desc, args);  }
> >
> > +#if defined(CONFIG_DM_GPIO_HOG)
> > +
> > +struct gpio_hog_priv {
> > +	struct gpio_desc gpiod;
> > +};
> > +
> > +struct gpio_hog_data {
> > +	int gpiod_flags;
> > +	int value;
> > +	u32 val[2];
> > +};
> > +
> > +static int gpio_hog_ofdata_to_platdata(struct udevice *dev) {
> > +	struct gpio_hog_data *plat = dev_get_platdata(dev);
> > +	const char *nodename;
> > +	int ret;
> > +
> > +	plat->value = 0;
> > +	if (dev_read_bool(dev, "input")) {
> > +		plat->gpiod_flags = GPIOD_IS_IN;
> > +	} else if (dev_read_bool(dev, "output-high")) {
> > +		plat->value = 1;
> > +		plat->gpiod_flags = GPIOD_IS_OUT;
> > +	} else if (dev_read_bool(dev, "output-low")) {
> > +		plat->gpiod_flags = GPIOD_IS_OUT;
> > +	} else {
> > +		printf("%s: missing gpio-hog state.\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +	ret = dev_read_u32_array(dev, "gpios", plat->val, 2);
> > +	if (ret) {
> > +		printf("%s: wrong gpios property, 2 values needed %d\n",
> > +		       __func__, ret);
> > +		return ret;
> > +	}
> > +	nodename = dev_read_string(dev, "line-name");
> > +	if (!nodename)
> > +		nodename = dev_read_name(dev);
> > +	device_set_name(dev, nodename);
> > +
> > +	return 0;
> > +}
> > +
> > +static int gpio_hog_probe(struct udevice *dev) {
> > +	struct gpio_hog_data *plat = dev_get_platdata(dev);
> > +	struct gpio_hog_priv *priv = dev_get_priv(dev);
> > +	int ret;
> > +
> > +	ret = gpio_dev_request_index(dev->parent, dev->name, "gpio-hog",
> > +				     plat->val[0], plat->gpiod_flags,
> > +				     plat->val[1], &priv->gpiod);
> > +	if (ret < 0) {
> > +		debug("%s: node %s could not get gpio.\n", __func__,
> > +		      dev->name);
> > +		return ret;
> > +	}
> > +	dm_gpio_set_dir(&priv->gpiod);
> > +	if (plat->gpiod_flags == GPIOD_IS_OUT)
> > +		dm_gpio_set_value(&priv->gpiod, plat->value);
> > +
> > +	return 0;
> > +}
> > +
> > +int gpio_hog_probe_all(void)
> > +{
> > +	struct udevice *dev;
> > +	int ret;
> > +
> > +	for (uclass_first_device(UCLASS_NOP, &dev);
> > +	     dev;
> > +	     uclass_find_next_device(&dev)) {
> > +		if (dev->driver == DM_GET_DRIVER(gpio_hog)) {
> > +			ret = device_probe(dev);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +struct gpio_desc *gpio_hog_lookup_name(const char *name) {
> > +	struct udevice *dev;
> > +
> > +	gpio_hog_probe_all();
> > +	if (!uclass_get_device_by_name(UCLASS_NOP, name, &dev)) {
> > +		struct gpio_hog_priv *priv = dev_get_priv(dev);
> > +
> > +		return &priv->gpiod;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +U_BOOT_DRIVER(gpio_hog) = {
> > +	.name	= "gpio_hog",
> > +	.id	= UCLASS_NOP,
> > +	.ofdata_to_platdata = gpio_hog_ofdata_to_platdata,
> > +	.probe = gpio_hog_probe,
> > +	.priv_auto_alloc_size = sizeof(struct gpio_hog_priv),
> > +	.platdata_auto_alloc_size = sizeof(struct gpio_hog_data), }; #else
> > +struct gpio_desc *gpio_hog_lookup_name(const char *name) {
> > +	return NULL;
> > +}
> > +#endif
> > +
> >  int dm_gpio_request(struct gpio_desc *desc, const char *label)  {
> >  	struct udevice *dev = desc->dev;
> > @@ -640,22 +755,25 @@ int dm_gpio_get_values_as_int(const struct
> gpio_desc *desc_list, int count)
> >  	return vector;
> >  }
> >
> > -static int gpio_request_tail(int ret, ofnode node,
> > +static int gpio_request_tail(int ret, const char *nodename,
> >  			     struct ofnode_phandle_args *args,
> >  			     const char *list_name, int index,
> > -			     struct gpio_desc *desc, int flags, bool add_index)
> > +			     struct gpio_desc *desc, int flags,
> > +			     bool add_index, struct udevice *dev)
> >  {
> > -	desc->dev = NULL;
> > +	desc->dev = dev;
> >  	desc->offset = 0;
> >  	desc->flags = 0;
> >  	if (ret)
> >  		goto err;
> >
> > -	ret = uclass_get_device_by_ofnode(UCLASS_GPIO, args->node,
> > -					  &desc->dev);
> > -	if (ret) {
> > -		debug("%s: uclass_get_device_by_ofnode failed\n", __func__);
> > -		goto err;
> > +	if (!desc->dev) {
> > +		ret = uclass_get_device_by_ofnode(UCLASS_GPIO, args->node,
> > +						  &desc->dev);
> > +		if (ret) {
> > +			debug("%s: uclass_get_device_by_ofnode failed\n",
> __func__);
> > +			goto err;
> > +		}
> >  	}
> >  	ret = gpio_find_and_xlate(desc, args);
> >  	if (ret) {
> > @@ -663,8 +781,7 @@ static int gpio_request_tail(int ret, ofnode node,
> >  		goto err;
> >  	}
> >  	ret = dm_gpio_requestf(desc, add_index ? "%s.%s%d" : "%s.%s",
> > -			       ofnode_get_name(node),
> > -			       list_name, index);
> > +			       nodename, list_name, index);
> >  	if (ret) {
> >  		debug("%s: dm_gpio_requestf failed\n", __func__);
> >  		goto err;
> > @@ -678,7 +795,7 @@ static int gpio_request_tail(int ret, ofnode node,
> >  	return 0;
> >  err:
> >  	debug("%s: Node '%s', property '%s', failed to request GPIO index %d:
> %d\n",
> > -	      __func__, ofnode_get_name(node), list_name, index, ret);
> > +	      __func__, nodename, list_name, index, ret);
> >  	return ret;
> >  }
> >
> > @@ -692,8 +809,8 @@ static int _gpio_request_by_name_nodev(ofnode node,
> const char *list_name,
> >  	ret = ofnode_parse_phandle_with_args(node, list_name, "#gpio-cells", 0,
> >  					     index, &args);
> >
> > -	return gpio_request_tail(ret, node, &args, list_name, index, desc,
> > -				 flags, add_index);
> > +	return gpio_request_tail(ret, ofnode_get_name(node), &args, list_name,
> > +				 index, desc, flags, add_index, NULL);
> >  }
> >
> >  int gpio_request_by_name_nodev(ofnode node, const char *list_name,
> > int index, @@ -707,13 +824,14 @@ int gpio_request_by_name(struct udevice
> *dev, const char *list_name, int index,
> >  			 struct gpio_desc *desc, int flags)  {
> >  	struct ofnode_phandle_args args;
> > +	ofnode node;
> >  	int ret;
> >
> >  	ret = dev_read_phandle_with_args(dev, list_name, "#gpio-cells", 0,
> >  					 index, &args);
> > -
> > -	return gpio_request_tail(ret, dev_ofnode(dev), &args, list_name,
> > -				 index, desc, flags, index > 0);
> > +	node = dev_ofnode(dev);
> > +	return gpio_request_tail(ret, ofnode_get_name(node), &args, list_name,
> > +				 index, desc, flags, index > 0, NULL);
> >  }
> >
> >  int gpio_request_list_by_name_nodev(ofnode node, const char
> > *list_name, @@ -854,8 +972,28 @@ static int gpio_pre_remove(struct udevice
> *dev)
> >  	return gpio_renumber(dev);
> >  }
> >
> > +int gpio_dev_request_index(struct udevice *dev, const char *nodename,
> > +			   char *list_name, int index, int flags,
> > +			   int dtflags, struct gpio_desc *desc) {
> > +	struct ofnode_phandle_args args;
> > +
> > +	args.node =  ofnode_null();
> > +	args.args_count = 2;
> > +	args.args[0] = index;
> > +	args.args[1] = dtflags;
> > +
> > +	return gpio_request_tail(0, nodename, &args, list_name, index, desc,
> > +				 flags, 0, dev);
> > +}
> > +
> >  static int gpio_post_bind(struct udevice *dev)  {
> > +#if defined(CONFIG_DM_GPIO_HOG)
> > +	struct udevice *child;
> > +	ofnode node;
> > +#endif
> > +
> >  #if defined(CONFIG_NEEDS_MANUAL_RELOC)
> >  	struct dm_gpio_ops *ops = (struct dm_gpio_ops *)device_get_ops(dev);
> >  	static int reloc_done;
> > @@ -885,6 +1023,17 @@ static int gpio_post_bind(struct udevice *dev)
> >  		reloc_done++;
> >  	}
> >  #endif
> > +
> > +#if defined(CONFIG_DM_GPIO_HOG)
> > +	dev_for_each_subnode(node, dev) {
> > +		if (ofnode_read_bool(node, "gpio-hog")) {
> > +			const char *name = ofnode_get_name(node);
> > +
> > +			device_bind_driver_to_node(dev, "gpio_hog", name,
> > +						   node, &child);
> > +		}
> > +	}
> > +#endif
> >  	return 0;
> >  }
> >
> > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> > index d03602696f..37f71e5708 100644
> > --- a/include/asm-generic/gpio.h
> > +++ b/include/asm-generic/gpio.h
> > @@ -348,6 +348,22 @@ const char *gpio_get_bank_info(struct udevice *dev,
> int *offset_count);
> >   */
> >  int dm_gpio_lookup_name(const char *name, struct gpio_desc *desc);
> >
> > +/**
> > + * gpio_hog_lookup_name() - Look up a named GPIO and return the gpio
> descr.
> > + *
> > + * @name:	Name to look up
> > + * @return:	Returns gpio_desc for gpio
> > + */
> > +struct gpio_desc *gpio_hog_lookup_name(const char *name);
> > +
> > +/**
> > + * gpio_hog_probe_all() - probe all gpio devices with
> > + * gpio-hog subnodes.
> > + *
> > + * @return:	Returns return value from device_probe()
> > + */
> > +int gpio_hog_probe_all(void);
> > +
> >  /**
> >   * gpio_lookup_name - Look up a GPIO name and return its details
> >   *
> > @@ -503,6 +519,22 @@ int gpio_request_list_by_name_nodev(ofnode node,
> const char *list_name,
> >  				    struct gpio_desc *desc_list, int max_count,
> >  				    int flags);
> >
> > +/**
> > + * gpio_dev_request_index() - request single GPIO from gpio device
> > + *
> > + * @dev:	GPIO device
> > + * @nodename:	Name of node
> > + * @list_name:	Name of GPIO list (e.g. "board-id-gpios")
> > + * @index:	Index number of the GPIO in that list use request (0=first)
> > + * @flags:	GPIOD_* flags
> > + * @dtflags:	GPIO flags read from DT
> > + * @desc:	GPIO descriotor filled from this function
> > + * @return:	return value from gpio_request_tail()
> > + */
> > +int gpio_dev_request_index(struct udevice *dev, const char *nodename,
> > +			   char *list_name, int index, int flags,
> > +			   int dtflags, struct gpio_desc *desc);
> > +
> >  /**
> >   * dm_gpio_free() - Free a single GPIO
> >   *
> >
> 
> Tested-by: Michal Simek <michal.simek@xilinx.com> (zcu102)
> 
> Thanks,
> Michal


For the patch Review and tested on stm32mp1 board.

Tested-by: Patrick Delaunay <patrick.delaunay@st.com>

Regards
PAtrick