[PATCH v2] gpio: Use separate bitfield array to indicate GPIO is claimed
Simon Glass
sjg at chromium.org
Fri Jul 28 19:32:40 CEST 2023
Hi Marek,
On Fri, 28 Jul 2023 at 11:17, 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>
> ---
> V2: Add set/clear helpers
> ---
> drivers/gpio/gpio-uclass.c | 61 ++++++++++++++++++++++++++++++++++----
> include/asm-generic/gpio.h | 2 ++
> 2 files changed, 58 insertions(+), 5 deletions(-)
I'm OK with a separate bitfield thing if you really want it.
>
> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> index 712119c3415..68c079a3ccd 100644
> --- a/drivers/gpio/gpio-uclass.c
> +++ b/drivers/gpio/gpio-uclass.c
> @@ -75,6 +75,46 @@ 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));
> +}
> +
> +/**
> + * gpio_set_claim() - Set GPIO claimed by consumer
> + *
> + * Set a bit which indicate the GPIO is claimed by consumer
> + *
> + * @uc_priv: gpio_dev_priv pointer.
> + * @offset: gpio offset within the device
> + */
> +static void gpio_set_claim(struct gpio_dev_priv *uc_priv, unsigned int offset)
> +{
> + uc_priv->claimed[offset / 32] |= BIT(offset % 32);
> +}
> +
> +/**
> + * gpio_clear_claim() - Clear GPIO claimed by consumer
> + *
> + * Clear a bit which indicate the GPIO is claimed by consumer
> + *
> + * @uc_priv: gpio_dev_priv pointer.
> + * @offset: gpio offset within the device
> + */
> +static void gpio_clear_claim(struct gpio_dev_priv *uc_priv, unsigned int offset)
> +{
> + uc_priv->claimed[offset / 32] &= ~BIT(offset % 32);
> +}
> +
> #if CONFIG_IS_ENABLED(DM_GPIO_LOOKUP_LABEL)
> /**
> * dm_gpio_lookup_label() - look for name in gpio device
> @@ -94,7 +134,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 +390,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 +402,8 @@ int dm_gpio_request(struct gpio_desc *desc, const char *label)
> return ret;
> }
> }
> +
> + gpio_set_claim(uc_priv, desc->offset);
> uc_priv->name[desc->offset] = str;
>
> return 0;
> @@ -438,7 +480,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 +488,7 @@ int _dm_gpio_free(struct udevice *dev, uint offset)
> return ret;
> }
>
> + gpio_clear_claim(uc_priv, offset);
> free(uc_priv->name[offset]);
> uc_priv->name[offset] = NULL;
>
> @@ -480,7 +523,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 +869,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 +1384,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));
This seems strange to me. Firstly I think the '32' needs to be a
const, like GPIO_ALLOC_BITS, since you have 32 all over the place
here.
Don't you want something like:
calloc(DIV_ROUND_UP(uc_priv->gpio_count, GPIO_ALLOC_BITS), GPIO_ALLOC_BITS / 8)
Because your expression is getting the number of 32-bit words, but
then you are using the size of a pointer, which could be 4 or 8 bytes.
> + if (!uc_priv->claimed) {
> + free(uc_priv->name);
> + return -ENOMEM;
> + }
> +
> return gpio_renumber(NULL);
> }
>
> @@ -1353,6 +1403,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