Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 25/02/2025 23:36, Linus Walleij wrote:
> On Tue, Feb 25, 2025 at 8:01 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
> 
>> The valid_mask member of the struct gpio_chip is unconditionally written
>> by the GPIO core at driver registration. Current documentation does not
>> mention this but just says the valid_mask is used if it's not NULL. This
>> lured me to try populating it directly in the GPIO driver probe instead
>> of using the init_valid_mask() callback. It took some retries with
>> different bitmaps and eventually a bit of code-reading to understand why
>> the valid_mask was not obeyed. I could've avoided this trial and error if
>> it was mentioned in the documentation.
>>
>> Help the next developer who decides to directly populate the valid_mask
>> in struct gpio_chip by documenting the valid_mask as internal to the
>> GPIO core.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> Ah typical.
> 
>>           * If not %NULL, holds bitmask of GPIOs which are valid to be used
>> -        * from the chip.
>> +        * from the chip. Internal to GPIO core. Chip drivers should populate
>> +        * init_valid_mask instead.
>>           */
>>          unsigned long *valid_mask;
> 
> Actually if we want to protect this struct member from being manipulated
> outside of gpiolib, we can maybe move it to struct gpio_device in
> drivers/gpio/gpiolib.h?
> 
> This struct exist for every gpio_chip but is entirely gpiolib-internal.
> 
> Then it becomes impossible to do it wrong...

True. I can try seeing what it'd require to do that. But ... If there 
are any drivers out there altering the valid_mask _after_ registering 
the driver to the gpio-core ... Then it may be a can of worms and I may 
just keep the lid closed :)

Furthermore, I was not 100% sure the valid_mask was not intended to be 
used directly by the drivers. I hoped you and Bart have an opinion on that.

We can also try:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ca2f58a2cd45..68fc0c6e2ed3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1047,9 +1047,11 @@ int gpiochip_add_data_with_key(struct gpio_chip 
*gc, void *data,
         if (ret)
                 goto err_cleanup_desc_srcu;

-       ret = gpiochip_init_valid_mask(gc);
-       if (ret)
-               goto err_cleanup_desc_srcu;
+       if (!gc->valid_mask) {
+               ret = gpiochip_init_valid_mask(gc);
+               if (ret)
+                       goto err_cleanup_desc_srcu;
+       }

         for (desc_index = 0; desc_index < gc->ngpio; desc_index++) {
                 struct gpio_desc *desc = &gdev->descs[desc_index];

if you think this is safe enough.

For example the BD79124 driver digs the pin config (GPO or ADC-input) 
during the probe. It needs this to decide which ADC channels to 
register, and also to configure the pinmux. So, the BD79124 could just 
populate the bitmask directly to the valid_mask and omit the 
init_valid_mask() - which might be a tiny simplification in driver. 
Still, I'm not sure if it's worth having two mechanisms to populate 
valid masks - I suppose supporting only the init_valid_mask() might be 
simpler for the gpiolib maintainers ;)

Yours,
	-- Matti