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

Hi Heiko,

On Fri, 15 May 2020 at 08:02, Heiko Schocher hs@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@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@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