On 26/02/2025 13:42, Matti Vaittinen wrote:
On 26/02/2025 12:18, Linus Walleij wrote:
On Wed, Feb 26, 2025 at 7:09 AM Matti Vaittinen
<mazziesaccount@xxxxxxxxx> 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.
I did some quick testing. I used:
adc: adc@10 {
...
channel@0 {
reg = <0>;
};
channel@1 {
reg = <1>;
};
/* ... up to the channel@6. */
gpio-controller;
};
which left GPIO0 ... GPIO6 masked (pins used for ADC) and only GPIO7
unmasked.
Then I added:
gpiotst {
compatible = "rohm,foo-bd72720-gpio";
rohm,dvs-vsel-gpios = <&adc 5 0>, <&adc 6 0>;
};
and a dummy driver which does:
gpio_array = devm_gpiod_get_array(&pdev->dev, "rohm,dvs-vsel",
GPIOD_OUT_LOW);
...
ret = gpiod_set_array_value_cansleep(gpio_array->ndescs,
gpio_array->desc, gpio_array->info, values);
As a result the bd79124 gpio driver got it's set_multiple called with
masked pins. (Oh, and I had accidentally prepared to handle this as I
had added a sanity check for pinmux register in the set_multiple()).
I suppose one can think this is a result of invalid DT and that drivers
shouldn't need to be prepared for that. But ... After supporting
customers who try to integrate IC drivers to their products ... I think
it's still better to be prepared. I definitely wouldn't blame the rcar
driver authors for their valid_mask sanity check :)
After all this babbling, my point is that having the valid mask visible
for drivers is useful. Especially because there are cases where the
'valid_mask' can be directly compared to the 'mask' parameter in the
set_multiple. It's clear and efficient check, and I could assume the
set_multiple() is an optimized call, and thus being efficient makes sense.
So... Long story short - I would still suggest keeping the valid_mask
visible and either taking just the doc update (as was done in the
original patch) - or skipping the valid_mask initialization in gpiolib
if driver provides non NULL value.
What do you think?
Yours,
-- Matti
