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

Heiko Schocher hs at denx.de
Tue Jun 4 16:04:38 UTC 2019


Hello Patrick,

Am 04.06.2019 um 13:49 schrieb Patrick DELAUNAY:
> Hi Heiko,
> 
>>
>> 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 at denx.de>
> 
> It seens the hog function can be called several times, for gpio bank probed in pre-reloc phasis:
> 	
> - before recocation : gpio_post_probe()  => gpio_hog() .... gpio_hogs_probed = 0
> - after relocation :
>   	gpio_post_probe()  => gpio_hog()... gpio_hogs_probed = 0
>   	gpio_hog_probe_all() => gpio_post_probe()  => gpio_hog()... gpio_hogs_probed = 1
> 	
> I don't known if it is expected behavior:
> 	hog configuration 2 times in Uboot = before and after relocation

No, not expected.

>> ---
>> clean travis build, see result:
>> https://travis-ci.org/hsdenx/u-boot-test/builds/538734780
>>
>> 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             | 217 ++++++++++++++++++++++---
>>   include/asm-generic/gpio.h             |  32 ++++
>>   5 files changed, 301 insertions(+), 19 deletions(-)

[...]
>> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index
>> da5e9ba6e5..4ecff115f1 100644
>> --- a/drivers/gpio/gpio-uclass.c
>> +++ b/drivers/gpio/gpio-uclass.c
>> @@ -5,6 +5,7 @@
>>
>>   #include <common.h>
>>   #include <dm.h>
>> +#include <dm/device-internal.h>
>>   #include <dt-bindings/gpio/gpio.h>
>>   #include <errno.h>
>>   #include <fdtdec.h>
>> @@ -15,6 +16,18 @@
>>
>>   DECLARE_GLOBAL_DATA_PTR;
>>
>> +#if defined(CONFIG_DM_GPIO_HOG)
>> +struct gpio_priv_one_hog {
>> +	struct list_head list;
>> +	char *name;
>> +	struct gpio_desc gpiod;
>> +};
>> +
>> +static struct list_head hogs;
>> +static int list_init;
>> +static int gpio_hogs_probed;
>> +#endif
>> +
>>   /**
>>    * gpio_to_device() - Convert global GPIO number to device, number
>>    *
>> @@ -141,6 +154,147 @@ 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)
>> +int gpio_hog_probe_all(void)
>> +{
>> +	struct udevice *dev;
>> +	ofnode node;
>> +	int ret;
>> +
>> +	/*
>> +	 * We need to probe all gpio-hog devices
>> +	 * at some point, as we cannot be sure,
>> +	 * that all gpio devices, which contain a
>> +	 * gpio-hog definition are probed.
>> +	 *
>> +	 */
>> +	if (gpio_hogs_probed)
>> +		return 0;
>> +
>> +	for (uclass_first_device(UCLASS_GPIO, &dev);
>> +	     dev;
>> +	     uclass_next_device(&dev)) {
> 
> uclass_next_device automatically call probe for the class (in uclass_get_device_tail)
> in this context, I think that uclass_find_next_device is better

The idea was to only probe gpio devices with gpio-hog subnodes...

>> +		dev_for_each_subnode(node, dev) {
>> +			if (ofnode_read_bool(node, "gpio-hog")) {
>> +				ret = device_probe(dev);
>> +				if (ret)
>> +					return ret;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +	gpio_hogs_probed = 1;
>> +
>> +	return 0;
>> +}
>> +
>> +struct gpio_desc *gpio_hog_lookup_name(const char *name) {
>> +	struct list_head *entry;
>> +	struct gpio_priv_one_hog *cur;
>> +
>> +	/* be sure, all gpio devices with gpio-hogs are probed */
>> +	gpio_hog_probe_all();
>> +	list_for_each(entry, &hogs) {
>> +		cur = list_entry(entry, struct gpio_priv_one_hog, list);
>> +		if (strcmp(cur->name, name) == 0)
>> +			return &cur->gpiod;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static int gpio_hog(struct udevice *dev) {
>> +	ofnode node;
>> +	int found = 0;
>> +	int ret;
>> +	struct gpio_dev_priv *uc_priv = NULL;
>> +
>> +	if (!list_init) {
>> +		INIT_LIST_HEAD(&hogs);
>> +		list_init = 1;
>> +	}
>> +	dev_for_each_subnode(node, dev) {
>> +		if (ofnode_read_bool(node, "gpio-hog")) {
>> +			found = 1;
>> +			break;
>> +		}
>> +	}
> 
> This check is really needed, as it is already done in the main loop ?
> I think you can remove this first  'found' loop.

Yes if we only setup the gpio-hogs from main loop.

>> +
>> +	if (!found)
>> +		return 0;
>> +
>> +	uc_priv = dev_get_uclass_priv(dev);
>> +	if (!uc_priv) {
>> +		printf("%s: missing private data.\n", __func__);
>> +		return 0;
>> +	}
> 
> It is prossible to have uc_priv = 0 ?
> by construction, is is not allowed in driver mode, so this test can be remove I think.

Yes, just panic check. I remove this.

>> +
>> +	/* scan for gpio-hog subnodes */
>> +	dev_for_each_subnode(node, dev) {
>> +		u32 val[2];
>> +		int value = 0;
>> +		int gpiod_flags;
>> +		struct gpio_priv_one_hog *new;
>> +		const char *nodename;
>> +
>> +		if (!ofnode_read_bool(node, "gpio-hog"))
>> +			continue;
>> +
>> +		if (ofnode_read_bool(node, "input")) {
>> +			gpiod_flags = GPIOD_IS_IN;
>> +		} else if (ofnode_read_bool(node, "output-high")) {
>> +			value = 1;
>> +			gpiod_flags = GPIOD_IS_OUT;
>> +		} else if (ofnode_read_bool(node, "output-low")) {
>> +			gpiod_flags = GPIOD_IS_OUT;
>> +		} else {
>> +			printf("%s: missing gpio-hog state.\n", __func__);
>> +			return -EINVAL;
>> +		}
>> +
>> +		ret = ofnode_read_u32_array(node, "gpios", val, 2);
>> +		if (ret) {
>> +			printf("%s: wrong gpios property, 2 values needed, ret:
>> %d\n", __func__, ret);
>> +			return ret;
>> +		}
>> +
>> +		new = calloc(1, sizeof(struct gpio_priv_one_hog));
>> +		nodename = ofnode_read_string(node, "line-name");
>> +		if (!nodename)
>> +			nodename = ofnode_get_name(node);
>> +		ret = gpio_dev_request_index(dev, nodename, "gpio-hog",
>> +					     val[0], gpiod_flags, val[1],
>> +					     &new->gpiod);
>> +		if (ret < 0) {
>> +			debug("%s: node %s could not get gpio.\n", __func__,
>> +			      ofnode_get_name(node));
>> +			free(new);
>> +			return ret;
>> +		}
>> +
>> +		new->name = uc_priv->name[new->gpiod.offset];
>> +		list_add_tail(&new->list, &hogs);
>> +		dm_gpio_set_dir(&new->gpiod);
>> +		if (gpiod_flags == GPIOD_IS_OUT)
>> +			dm_gpio_set_value(&new->gpiod, value);
>> +	}
>> +
>> +	return 0;
>> +}
>> +#else
>> +static int gpio_hog(struct udevice *dev) {
>> +	return 0;
>> +}
>> +
>> +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;
>> @@ -149,8 +303,9 @@ int dm_gpio_request(struct gpio_desc *desc, const char
>> *label)
>>   	int ret;
>>
>>   	uc_priv = dev_get_uclass_priv(dev);
>> -	if (uc_priv->name[desc->offset])
>> -		return -EBUSY;
>> +	if (uc_priv)
>> +		if (uc_priv->name[desc->offset])
>> +			return -EBUSY;
>>   	str = strdup(label);
>>   	if (!str)
>>   		return -ENOMEM;
>> @@ -640,22 +795,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 +821,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 +835,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 +849,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 +864,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, @@ -
>> 832,12 +990,17 @@ int gpio_get_number(const struct gpio_desc *desc)  static int
>> gpio_post_probe(struct udevice *dev)  {
>>   	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>> +	int ret;
>>
>>   	uc_priv->name = calloc(uc_priv->gpio_count, sizeof(char *));
>>   	if (!uc_priv->name)
>>   		return -ENOMEM;
>>
>> -	return gpio_renumber(NULL);
>> +	ret = gpio_renumber(NULL);
>> +	if (ret)
>> +		return ret;
>> +
> 
> It is strange for me to have many device tree parsing in probe function....
> 
> For me
> => hog detection + DT parsing in gpio_post_bind() .... info saved in platdata
> => gpio configuration for hog (request and set_dir) => probe function

Yes, this approach is much more cleaner.

>> +	return gpio_hog(dev);
>>   }
> 
> 
> One other solution to do it is to manage a hog with uclass  (using the new UCLASS_NOP or misc for example)
> http://patchwork.ozlabs.org/patch/1098913/

Ah, thanks for this hint, wasn;t aware of it.

> Ps: we can also create a new UCLASS_GPIOHOG

Heh, I had a version (never posted) with such an approach. Thought this
is to much overhead ... I try first with UCLASS_NOP.

> And we to bind this class during GPIO binding; the parsing of DT is done only one time
> and manage privdata and platdata as expected by driver model....

Yes, cleaner.

> For example something like (not tested)
[...]

Thanks for your input! I try to rework this soon.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de


More information about the U-Boot mailing list