[PATCH v5 3/3] gpio: search for gpio label if gpio is not found through bank name

[Simon Glass]

Hi Heiko,

On Fri, 15 May 2020 at 08:02, Heiko Schocher <hs at denx.de> wrote:
>
> dm_gpio_lookup_name() searches for a gpio through
> the bank name. But we have also gpio labels, and it
> makes sense to search for a gpio also in the labels
> we have defined, if no gpio is found through the
> bank name definition.
>
> This is useful for example if you have a wp pin on
> different gpios on different board versions.
>
> If dm_gpio_lookup_name() searches also for the gpio labels,
> you can give the gpio an unique label name and search
> for this label, and do not need to differ between
> board revisions.
>
> Signed-off-by: Heiko Schocher <hs at denx.de>
> ---
>
> Example on the aristainetos board:
>
> => gpio clear wp_spi_nor.gpio-hog
> gpio: pin wp_spi_nor.gpio-hog (gpio 47) value is 0
> =>
>
> before this patch, you need to know where your
> pin is:
>
> => gpio clear GPIO2_15
> gpio: pin GPIO2_15 (gpio 47) value is 0
> =>
>
> travis build:
>
> Changes in v5: None
> Changes in v4:
> - rebased to current master ac14bc4169
>
> Changes in v3:
> - add comment from Simon Glass
>   make this new function configurable through Kconfig
>   option DM_GPIO_LOOKUP_LABEL
>
> Changes in v2:
> - add comment from Simon Glass
>   move code into seperate function dm_gpio_lookup_label()
>   add test if dm_gpio_lookup_label() works
>
>  drivers/gpio/Kconfig       | 22 ++++++++++++++++++
>  drivers/gpio/gpio-uclass.c | 47 ++++++++++++++++++++++++++++++++++++++
>  test/dm/gpio.c             |  7 ++++++
>  3 files changed, 76 insertions(+)

Reviewed-by: Simon Glass <sjg at chromium.org>

Some optional thoughts below

>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 2081520f42..4a635b905c 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -46,6 +46,28 @@ config GPIO_HOG
>           is a mechanism providing automatic GPIO request and config-
>           uration as part of the gpio-controller's driver probe function.
>
> +config DM_GPIO_LOOKUP_LABEL
> +       bool "Enable searching for gpio labelnames"
> +       depends on DM_GPIO
> +       default y
> +       help
> +         This option enables searching for gpio names in
> +         the defined gpio labels, if the search for the
> +         gpio bank name failed. This makes sense if you use
> +         different gpios on different hardware versions
> +         for the same functionality in board code.
> +
> +config SPL_DM_GPIO_LOOKUP_LABEL
> +       bool "Enable searching for gpio labelnames"
> +       depends on DM_GPIO && SPL_DM && SPL_GPIO_SUPPORT

Perhaps the first term could be DM_GPIO_LOOKUP_LABEL?

> +       default n

Not needed

> +       help
> +         This option enables searching for gpio names in
> +         the defined gpio labels, if the search for the
> +         gpio bank name failed. This makes sense if you use
> +         different gpios on different hardware versions
> +         for the same functionality in board code.
> +
>  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 d241263970..317b486982 100644
> --- a/drivers/gpio/gpio-uclass.c
> +++ b/drivers/gpio/gpio-uclass.c
> @@ -67,6 +67,46 @@ static int gpio_to_device(unsigned int gpio, struct gpio_desc *desc)
>         return ret ? ret : -ENOENT;
>  }
>
> +#if CONFIG_IS_ENABLED(DM_GPIO_LOOKUP_LABEL)
> +/**
> + * dm_gpio_lookup_label() - look for name in gpio device
> + *
> + * search in uc_priv, if there is a gpio with labelname same
> + * as name.
> + *
> + * @name:      name which is searched
> + * @uc_priv:   gpio_dev_priv pointer.
> + * @offset:    gpio offset within the device
> + * @return:    0 if found, -ENOENT if not.
> + */
> +static int

I prefer to have the type on the same line as the function

> +dm_gpio_lookup_label(const char *name, struct gpio_dev_priv *uc_priv,
> +                    ulong *offset)
> +{
> +       int len;
> +       int i;
> +
> +       *offset = -1;
> +       len = strlen(name);
> +       for (i = 0; i < uc_priv->gpio_count; i++) {
> +               if (!uc_priv->name[i])
> +                       continue;
> +               if (!strncmp(name, uc_priv->name[i], len)) {
> +                       *offset = i;
> +                       return 0;
> +               }
> +       }
> +       return -ENOENT;
> +}
> +#else
> +static int
> +dm_gpio_lookup_label(const char *name, struct gpio_dev_priv *uc_priv,
> +                    ulong *offset)
> +{
> +       return -ENOENT;
> +}
> +#endif
> +
>  int dm_gpio_lookup_name(const char *name, struct gpio_desc *desc)
>  {
>         struct gpio_dev_priv *uc_priv = NULL;
> @@ -95,6 +135,13 @@ int dm_gpio_lookup_name(const char *name, struct gpio_desc *desc)
>                         if (!strict_strtoul(name + len, 10, &offset))
>                                 break;
>                 }
> +
> +               /*
> +                * if we did not found a gpio through its bank
> +                * name, we search for a valid gpio label.
> +                */
> +               if (!dm_gpio_lookup_label(name, uc_priv, &offset))
> +                       break;
>         }
>
>         if (!dev)
> diff --git a/test/dm/gpio.c b/test/dm/gpio.c
> index 1443a2db79..e2f147e776 100644
> --- a/test/dm/gpio.c
> +++ b/test/dm/gpio.c
> @@ -131,6 +131,13 @@ static int dm_test_gpio(struct unit_test_state *uts)
>         ut_assertok(dm_gpio_set_value(desc, 0));
>         ut_asserteq(0, dm_gpio_get_value(desc));
>
> +       /* Check if lookup for labels work */
> +       ut_assertok(gpio_lookup_name("hog_input_active_low", &dev, &offset,
> +                                    &gpio));
> +       ut_asserteq_str(dev->name, "base-gpios");
> +       ut_asserteq(0, offset);
> +       ut_asserteq(CONFIG_SANDBOX_GPIO_COUNT + 0, gpio);

Could add a negative test too (where you don't find it).

> +
>         return 0;
>  }
>  DM_TEST(dm_test_gpio, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> --
> 2.24.1
>

Regards,
SImon