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

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 26/02/2025 12:18, Linus Walleij wrote:
> On Wed, Feb 26, 2025 at 7:09 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>> On 25/02/2025 23:36, Linus Walleij wrote:
>>> 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 :)
> 
> That's easy to check with some git grep valid_mask

True. I just tried. It seems mostly Ok, but...
For example the drivers/gpio/gpio-rcar.c uses the contents of the 
'valid_mask' in it's set_multiple callback to disallow setting the value 
of masked GPIOs.

For uneducated person like me, it feels this check should be done and 
enforced by the gpiolib and not left for untrustworthy driver writers 
like me! (I am working on BD79124 driver and it didn't occur to me I 
should check for the valid_mask in driver :) If gpiolib may call the 
driver's set_multiple() with masked lines - then the bd79124 driver just 
had one unknown bug less :rolleyes:) )

I tried looking at the gpiolib to see how this works... It seems to me:

gpio_chip_set_multiple() does not seem to check for valid_mask. This is 
called from the gpiod_set_array_value_complex() - which gave me a 
headache as it is, as name says, complex. Well, I didn't spot valid_mask 
check but I may have missed a thing or 2...

If someone remembers straight away how this is supposed to work - I 
appreciate any guidance. If not, then I try doing some testing when I 
wire the BD79124 to my board for the next version of the BD79124 series.

> and intuition. I think all calls actually changing the valid_mask
> are in the init_valid_mask() callback as they should be.
> 
>> 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.
> 
> Oh it was. First we just had .valid_mask and then it was
> manipulated directly.

I still can't decide if hiding the valid_mask is the right thing to do, 
or if we should just respect it if it is set by driver (as it was 
originally intended).

> Then we introduced init_valid_mask() and all users switched over
> to using that.
> 
> So evolution, not intelligent design...

Like anything we actually get done ^_^;

Yours,
	-- Matti