Commit 8015443e authored by Matti Vaittinen's avatar Matti Vaittinen Committed by Bartosz Golaszewski
Browse files

gpio: Hide valid_mask from direct assignments 



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
the valid_mask was hidden in the struct gpio_device instead of being a
visible member of the struct gpio_chip.

Help the next developer who decides to directly populate the valid_mask
in struct gpio_chip by hiding the valid_mask in struct gpio_device and
keep it internal to the GPIO core.

Suggested-by: default avatarLinus Walleij <linus.walleij@linaro.org>
Signed-off-by: default avatarMatti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: default avatarLinus Walleij <linus.walleij@linaro.org>
Link: https://lore.kernel.org/r/4547ca90d910d60cab3d56d864d59ddde47a5e93.1741180097.git.mazziesaccount@gmail.com


Signed-off-by: default avatarBartosz Golaszewski <bartosz.golaszewski@linaro.org>
parent 43b665c9

drivers/gpio/gpiolib.c

+8 −8
Original line number Diff line number Diff line
@@ -672,7 +672,7 @@ static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc) 
		if (start >= gc->ngpio || start + count > gc->ngpio)

			continue;



		bitmap_clear(gc->valid_mask, start, count);

		bitmap_clear(gc->gpiodev->valid_mask, start, count);

	}



	kfree(ranges);
@@ -686,8 +686,8 @@ static int gpiochip_init_valid_mask(struct gpio_chip *gc) 
	if (!(gpiochip_count_reserved_ranges(gc) || gc->init_valid_mask))

		return 0;



	gc->valid_mask = gpiochip_allocate_mask(gc);

	if (!gc->valid_mask)

	gc->gpiodev->valid_mask = gpiochip_allocate_mask(gc);

	if (!gc->gpiodev->valid_mask)

		return -ENOMEM;



	ret = gpiochip_apply_reserved_ranges(gc);
@@ -696,7 +696,7 @@ static int gpiochip_init_valid_mask(struct gpio_chip *gc) 


	if (gc->init_valid_mask)

		return gc->init_valid_mask(gc,

					   gc->valid_mask,

					   gc->gpiodev->valid_mask,

					   gc->ngpio);



	return 0;
@@ -704,7 +704,7 @@ static int gpiochip_init_valid_mask(struct gpio_chip *gc) 


static void gpiochip_free_valid_mask(struct gpio_chip *gc)

{

	gpiochip_free_mask(&gc->valid_mask);

	gpiochip_free_mask(&gc->gpiodev->valid_mask);

}



static int gpiochip_add_pin_ranges(struct gpio_chip *gc)
@@ -735,7 +735,7 @@ static int gpiochip_add_pin_ranges(struct gpio_chip *gc) 
 */

const unsigned long *gpiochip_query_valid_mask(const struct gpio_chip *gc)

{

	return gc->valid_mask;

	return gc->gpiodev->valid_mask;

}

EXPORT_SYMBOL_GPL(gpiochip_query_valid_mask);
@@ -743,9 +743,9 @@ bool gpiochip_line_is_valid(const struct gpio_chip *gc, 
				unsigned int offset)

{

	/* No mask means all valid */

	if (likely(!gc->valid_mask))

	if (likely(!gc->gpiodev->valid_mask))

		return true;

	return test_bit(offset, gc->valid_mask);

	return test_bit(offset, gc->gpiodev->valid_mask);

}

EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);

drivers/gpio/gpiolib.h

+3 −0
Original line number Diff line number Diff line
@@ -32,6 +32,8 @@ 
 * @chip: pointer to the corresponding gpiochip, holding static

 * data for this device

 * @descs: array of ngpio descriptors.

 * @valid_mask: If not %NULL, holds bitmask of GPIOs which are valid to be

 * used from the chip.

 * @desc_srcu: ensures consistent state of GPIO descriptors exposed to users

 * @ngpio: the number of GPIO lines on this GPIO device, equal to the size

 * of the @descs array.
@@ -65,6 +67,7 @@ struct gpio_device { 
	struct module		*owner;

	struct gpio_chip __rcu	*chip;

	struct gpio_desc	*descs;

	unsigned long		*valid_mask;

	struct srcu_struct	desc_srcu;

	unsigned int		base;

	u16			ngpio;

include/linux/gpio/driver.h

+0 −8
Original line number Diff line number Diff line
@@ -514,14 +514,6 @@ struct gpio_chip { 
	struct gpio_irq_chip irq;

#endif /* CONFIG_GPIOLIB_IRQCHIP */



	/**

	 * @valid_mask:

	 *

	 * If not %NULL, holds bitmask of GPIOs which are valid to be used

	 * from the chip.

	 */

	unsigned long *valid_mask;



#if defined(CONFIG_OF_GPIO)

	/*

	 * If CONFIG_OF_GPIO is enabled, then all GPIO controllers described in