On 25/02/2025 23:36, Linus Walleij wrote:
On Tue, Feb 25, 2025 at 8:01 AM Matti Vaittinen
<mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx>
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
