Re: [PATCH] gpio: Use separate bitfield array to indicate GPIO is claimed

Hi Marek,

On Thu, 27 Jul 2023 at 09:50, Marek Vasut marex@denx.de wrote:

The current gpio-uclass design uses name field in struct gpio_dev_priv as an indicator that GPIO is claimed by consumer. This overloads the function of name field and does not work well for named pins not configured as GPIO pins.

Introduce separate bitfield array as the claim indicator.

This unbreaks dual-purpose AF and GPIO operation on STM32MP since commit 2c38f7c31806 ("pinctrl: pinctrl_stm32: Populate uc_priv->name[] with pinmux node's name") where any pin which has already been configured as AF could no longer be claimed as dual-purpose GPIO. This is important for pins like STM32 MMCI st,cmd-gpios .

Signed-off-by: Marek Vasut marex@denx.de

Cc: Michal Suchanek msuchanek@suse.de Cc: Patrice Chotard patrice.chotard@foss.st.com Cc: Patrick Delaunay patrick.delaunay@foss.st.com Cc: Rasmus Villemoes rasmus.villemoes@prevas.dk Cc: Samuel Holland samuel@sholland.org Cc: Simon Glass sjg@chromium.org

---

drivers/gpio/gpio-uclass.c | 35 ++++++++++++++++++++++++++++++----- include/asm-generic/gpio.h | 2 ++ 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 712119c3415..25873f07bd4 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -75,6 +75,20 @@ static int gpio_to_device(unsigned int gpio, struct gpio_desc *desc) return -ENOENT; }

+/**

• • gpio_is_claimed() - Test whether GPIO is claimed by consumer
• • Test whether GPIO is claimed by consumer already.
• • @uc_priv: gpio_dev_priv pointer.
• • @offset: gpio offset within the device
• • @return: true if claimed, false if not claimed
• */

+static bool gpio_is_claimed(struct gpio_dev_priv *uc_priv, unsigned int offset) +{

•   return !!(uc_priv->claimed[offset / 32] & BIT(offset % 32));
  

+}

Can you add a helper to set is_claimed as well? That avoids open-coding it in two places below.

#if CONFIG_IS_ENABLED(DM_GPIO_LOOKUP_LABEL) /**

• dm_gpio_lookup_label() - look for name in gpio device

@@ -94,7 +108,7 @@ static int dm_gpio_lookup_label(const char *name,

    *offset = -1;
    for (i = 0; i < uc_priv->gpio_count; i++) {

•           if (!uc_priv->name[i])
  

•           if (!gpio_is_claimed(uc_priv, i))
                    continue;
            if (!strcmp(name, uc_priv->name[i])) {
                    *offset = i;
  

@@ -350,7 +364,7 @@ int dm_gpio_request(struct gpio_desc *desc, const char *label) int ret;

    uc_priv = dev_get_uclass_priv(dev);

•   if (uc_priv->name[desc->offset])
  

•   if (gpio_is_claimed(uc_priv, desc->offset))
            return -EBUSY;
    str = strdup(label);
    if (!str)
  

@@ -362,6 +376,8 @@ int dm_gpio_request(struct gpio_desc *desc, const char *label) return ret; } }

•   uc_priv->claimed[desc->offset / 32] |= BIT(desc->offset % 32);
    uc_priv->name[desc->offset] = str;
  
    return 0;
  

@@ -438,7 +454,7 @@ int _dm_gpio_free(struct udevice *dev, uint offset) int ret;

    uc_priv = dev_get_uclass_priv(dev);

•   if (!uc_priv->name[offset])
  

•   if (!gpio_is_claimed(uc_priv, offset))
            return -ENXIO;
    if (ops->rfree) {
            ret = ops->rfree(dev, offset);
  

@@ -446,6 +462,7 @@ int _dm_gpio_free(struct udevice *dev, uint offset) return ret; }

•   uc_priv->claimed[offset / 32] &= ~BIT(offset % 32);
    free(uc_priv->name[offset]);
    uc_priv->name[offset] = NULL;
  
  

@@ -480,7 +497,7 @@ static int check_reserved(const struct gpio_desc *desc, const char *func) return -ENOENT;

    uc_priv = dev_get_uclass_priv(desc->dev);

•   if (!uc_priv->name[desc->offset]) {
  

•   if (!gpio_is_claimed(uc_priv, desc->offset)) {
            printf("%s: %s: error: gpio %s%d not reserved\n",
                   desc->dev->name, func,
                   uc_priv->bank_name ? uc_priv->bank_name : "",
  

@@ -826,7 +843,7 @@ static int get_function(struct udevice *dev, int offset, bool skip_unused, return -EINVAL; if (namep) *namep = uc_priv->name[offset];

•   if (skip_unused && !uc_priv->name[offset])
  

•   if (skip_unused && !gpio_is_claimed(uc_priv, offset))
            return GPIOF_UNUSED;
    if (ops->get_function) {
            int ret;
  

@@ -1341,6 +1358,13 @@ static int gpio_post_probe(struct udevice *dev) if (!uc_priv->name) return -ENOMEM;

•   uc_priv->claimed = calloc(DIV_ROUND_UP(uc_priv->gpio_count, 32),
  

•                             sizeof(*uc_priv->claimed));
  

•   if (!uc_priv->claimed) {
  

•           free(uc_priv->name);
  

•           return -ENOMEM;
  

•   }
  

We already have the name[] array which holds the name for each GPIO. What do you think about a struct for each GPIO, with both the name and the bool in it. It is messy to have two separate, mirrored arrays.

•   return gpio_renumber(NULL);
  

}

@@ -1353,6 +1377,7 @@ static int gpio_pre_remove(struct udevice *dev) if (uc_priv->name[i]) free(uc_priv->name[i]); }

•   free(uc_priv->claimed);
    free(uc_priv->name);
  
    return gpio_renumber(dev);
  

diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index c4a7fd28439..a21c606f2b8 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -414,6 +414,7 @@ struct dm_gpio_ops {

• @gpio_base: Base GPIO number for this device. For the first active device
• this will be 0; the numbering for others will follow sequentially so that
• @gpio_base for device 1 will equal the number of GPIOs in device 0.

• • @claimed: Array of bits indicating which GPIOs in the bank are claimed.
  • @name: Array of pointers to the name for each GPIO in this bank. The
  • value of the pointer will be NULL if the GPIO has not been claimed.
  */

@@ -421,6 +422,7 @@ struct gpio_dev_priv { const char *bank_name; unsigned gpio_count; unsigned gpio_base;

•   u32 *claimed;
    char **name;
  

};

-- 2.40.1

Regards, Simon