Hi Marek, On Fri, 28 Jul 2023 at 11:17, Marek Vasut <ma...@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 <ma...@denx.de> > --- > Cc: Michal Suchanek <msucha...@suse.de> > Cc: Patrice Chotard <patrice.chot...@foss.st.com> > Cc: Patrick Delaunay <patrick.delau...@foss.st.com> > Cc: Rasmus Villemoes <rasmus.villem...@prevas.dk> > Cc: Samuel Holland <sam...@sholland.org> > Cc: Simon Glass <s...@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