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

Hi Heiko,

On Wed, 12 Jun 2019 at 05:12, Heiko Schocher hs@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@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.

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.

+Example:

•    tca6416@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

•            };
  

•    };
  

+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?

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); if (dm_gpio_get_value(desc) == 1)

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.

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

+};

+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?

•   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

•   if (plat->gpiod_flags == GPIOD_IS_OUT)
  

•           dm_gpio_set_value(&priv->gpiod, plat->value);
  

check error

•   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?

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

{

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

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

•           }
  

•   }
  

+#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?

• */

+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?

• • @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?

• • @desc: GPIO descriotor filled from this function

returns GPIO descriptor...

• • @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