From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Mon, 24 Jun 2019 09:15:58 +0200 Subject: [U-Boot] [PATCH v4] gpio: add gpio-hog support In-Reply-To: References: <20190612041146.53854-1-hs@denx.de> Message-ID: <80897b0d-c5ed-4580-86a6-9c9bfbed4b49@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, Am 22.06.2019 um 21:09 schrieb Simon Glass: > Hi Heiko, > > On Wed, 12 Jun 2019 at 05:12, 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 >> >> --- >> 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 >> #include >> #include >> +#if defined(CONFIG_DM_GPIO_HOG) >> +#include >> +#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 >> #include >> +#include >> +#include >> +#include >> #include >> #include >> #include >> @@ -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