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

Heiko Schocher hs at denx.de
Mon Jun 24 07:15:58 UTC 2019


Hello Simon,

Am 22.06.2019 um 21:09 schrieb Simon Glass:
> Hi Heiko,
> 
> On Wed, 12 Jun 2019 at 05:12, Heiko Schocher <hs at denx.de> 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 at 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
> 
> 
> Sorry for not getting to this earlier.

no problem, thanks for your time!

>> 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
>>   #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.
> 
> This seems a strange name. I suppose this is what linux called it.

Yes.

>> +
>> +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>;
> 
> Should have an example for line-name

Oh, yes of course, I added the "line-name" part later, and forgot the
example.
> 
>> +                };
>> +        };
>> +
>> +For the above Example you can than access the gpio in your boardcode
>> +with:
>> +
>> +        desc = gpio_hog_lookup_name("boot_rescue.gpio-hog");
> 
> Could we drop the .gpio-hog suffix? It doesn't seem necessary, but
> perhaps you are worried about people getting confused otherwise?

Good catch! Since v4 it is already without this suffix, so I correct
the doc here.

> Also this function should be passed a desc struct pointer, not return
> static data.
> 
>> +        if (desc) {
>> +                if (dm_gpio_get_value(desc))
> 
> You should check the error here.
> 
> dm_gpio_get_value() returns -ve on error, including if desc returns an
> invalid GPIO (dev=NULL), so you should be able to do:
> 
> gpio_hog_lookup_name("boot_rescue.gpio-hog", &desc);

Also we should check here the return value.

> if (dm_gpio_get_value(desc) == 1)

Good point. I rework this, thanks!

> So in fact
>> +                        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
> 
> Can we drop the DM_ prefix? At some point DM_GPIO will go away and we
> don't want to rename these sorts of options.

Hmm... we also have the DM_GPIO symbol, as the gpio hog only works with
DM_GPIO enabled, I thought DM_GPIO_HOG is the better choice, but of course
I can change this.

>> +       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;
> 
> As above I don't think you need this.

Hmm.. when probing I need to request the gpio with gpio_dev_request_index()
for which I need to pass a pointer to struct gpio_desc ...

And later I want to have the possibility to access the gpio for example
with dm_gpio_set_value() and need therefore the gpio descriptor, so I must
store it somewhere. Or do I overlook something obvious?

>> +};
>> +
>> +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);
> 
> I don't think you should set the device name unless needed. But why
> not store this in the device data, rather than changing its name? If
> we want to change the name, why not just use that for the node name in
> the .dts?

Because I use the device name for finding the gpio through gpio_hog_lookup_name().

Ok, if there is no line-name property, we do not need to set
the device name, as it has already the correct name, so this can be
changed to:

         nodename = dev_read_string(dev, "line-name");
         if (nodename)
                 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);
> 
> Check error

Yes ... Hmm, while checking, this is not needed anymore, as
gpio_dev_request_index() calls gpio_request_tail() which calls
dm_gpio_set_dir(), so I remove this.

> 
>> +       if (plat->gpiod_flags == GPIOD_IS_OUT)
>> +               dm_gpio_set_value(&priv->gpiod, plat->value);
> 
> check error

done.

>> +
>> +       return 0;
>> +}
>> +
>> +int gpio_hog_probe_all(void)
>> +{
>> +       struct udevice *dev;
>> +       int ret;
>> +
>> +       for (uclass_first_device(UCLASS_NOP, &dev);
> 
> Perhaps you should create a new uclass for this?

Hmm.. yes, I had an approach with a new uclass, but I thought, the new [1]
UCLASS_NOP is valid for this here to?

[1] http://patchwork.ozlabs.org/patch/1098913/

>> +            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)
> 
> This function badly needs a comment now. Also 'dev' is not a great name.

Before my patch I think, it needed a comment ;-)
I try to add one...

Hmm... the name "dev" is the same as in struct gpio_desc ... what is a good name?

parent? gpio_dev?

I tend to name it gpio_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)
> 
> Can you use if (IS_ENABLED(CONFIG....

Yes, changed.

>> +       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);
> 
> Check error? This might run out of memory, perhaps.

done.

>> +               }
>> +       }
>> +#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()
> 
> What if one fails and the others succeed?

Currently I return with error code from device_probe(). So the
rest of the gpio-hogs are not probed.

Hmm.. may it is better to emmit a debug message and continue?

>> + */
>> +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
> 
> What is a node? Can you expand this a bit?

changed to:

  * @nodename:   Name of node for which gpio gets requested, used
  *              for the gpio label name

> 
>> + * @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
> 
> Reference GPIOD_... mask here?

Yep, added.

>> + * @desc:      GPIO descriotor filled from this function
> 
> returns GPIO descriptor...

changed.

>> + * @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
>>    *
>> --
>> 2.21.0
>>
> 
> Regards,
> Simon

Thanks for this review!

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