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

Simon Glass sjg at chromium.org
Fri Jul 28 03:52:04 CEST 2023


Hi Marek,

On Thu, 27 Jul 2023 at 09:50, Marek Vasut <marex at 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 at denx.de>
> ---
> Cc: Michal Suchanek <msuchanek at suse.de>
> Cc: Patrice Chotard <patrice.chotard at foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
> Cc: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> Cc: Samuel Holland <samuel at sholland.org>
> Cc: Simon Glass <sjg at 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


More information about the U-Boot mailing list