Re: [PATCH v2 5/9] gpiolib: Get rid of ARCH_NR_GPIOS

From: Andy Shevchenko
Date: Fri Sep 02 2022 - 11:26:19 EST

On Fri, Sep 2, 2022 at 4:57 PM Christophe Leroy
<christophe.leroy@xxxxxxxxxx> wrote:
>
> Since commit 14e85c0e69d5 ("gpio: remove gpio_descs global array")
> there is no limitation on the number of GPIOs that can be allocated
> in the system since the allocation is fully dynamic.
>
> ARCH_NR_GPIOS is today only used in order to provide downwards
> gpiobase allocation from that value, while static allocation is
> performed upwards from 0. However that has the disadvantage of
> limiting the number of GPIOs that can be registered in the system.
>
> To overcome this limitation without requiring each and every
> platform to provide its 'best-guess' maximum number, rework the
> allocation to allocate upwards, allowing approx 2 millions of
> GPIOs.
>
> In order to still allow static allocation for legacy drivers, define
> GPIO_DYNAMIC_BASE with the value 512 as the start for dynamic
> allocation. The 512 value is chosen because it is the end of
> the current default range so all current static allocations are
> expected to be below that value. Of course that's just a rough
> estimate based on the default value, but assuming static
> allocations come first, even if there are more static allocations
> it should fit under the 512 value.
>
> In the future, it is expected that all static allocations go away
> and then dynamic allocation will be patched to start at 0.

Eventually we have to get rid of gpio_is_valid() completely...
But this is another story.
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> ---
> v2: Enhanced commit description and change from 256 to 512.
> ---
> arch/arm/include/asm/gpio.h | 1 -
> drivers/gpio/gpiolib.c | 10 +++----
> include/asm-generic/gpio.h | 55 ++++++++++++++-----------------------
> 3 files changed, 26 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm/include/asm/gpio.h b/arch/arm/include/asm/gpio.h
> index f3bb8a2bf788..4ebbb58f06ea 100644
> --- a/arch/arm/include/asm/gpio.h
> +++ b/arch/arm/include/asm/gpio.h
> @@ -2,7 +2,6 @@
> #ifndef _ARCH_ARM_GPIO_H
> #define _ARCH_ARM_GPIO_H
>
> -/* Note: this may rely upon the value of ARCH_NR_GPIOS set in mach/gpio.h */
> #include <asm-generic/gpio.h>
>
> /* The trivial gpiolib dispatchers */
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4e2fcb7b0a01..1846f24971e3 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -183,14 +183,14 @@ EXPORT_SYMBOL_GPL(gpiod_to_chip);
> static int gpiochip_find_base(int ngpio)
> {
> struct gpio_device *gdev;
> - int base = ARCH_NR_GPIOS - ngpio;
> + int base = GPIO_DYNAMIC_BASE;
>
> - list_for_each_entry_reverse(gdev, &gpio_devices, list) {
> + list_for_each_entry(gdev, &gpio_devices, list) {
> /* found a free space? */
> - if (gdev->base + gdev->ngpio <= base)
> + if (gdev->base >= base + ngpio)
> break;
> - /* nope, check the space right before the chip */
> - base = gdev->base - ngpio;
> + /* nope, check the space right after the chip */
> + base = gdev->base + gdev->ngpio;
> }
>
> if (gpio_is_valid(base)) {
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index aea9aee1f3e9..a7752cf152ce 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -11,40 +11,18 @@
> #include <linux/gpio/driver.h>
> #include <linux/gpio/consumer.h>
>
> -/* Platforms may implement their GPIO interface with library code,
> +/*
> + * Platforms may implement their GPIO interface with library code,
> * at a small performance cost for non-inlined operations and some
> * extra memory (for code and for per-GPIO table entries).
> - *
> - * While the GPIO programming interface defines valid GPIO numbers
> - * to be in the range 0..MAX_INT, this library restricts them to the
> - * smaller range 0..ARCH_NR_GPIOS-1.
> - *
> - * ARCH_NR_GPIOS is somewhat arbitrary; it usually reflects the sum of
> - * builtin/SoC GPIOs plus a number of GPIOs on expanders; the latter is
> - * actually an estimate of a board-specific value.
> */
>
> -#ifndef ARCH_NR_GPIOS
> -#if defined(CONFIG_ARCH_NR_GPIO) && CONFIG_ARCH_NR_GPIO > 0
> -#define ARCH_NR_GPIOS CONFIG_ARCH_NR_GPIO
> -#else
> -#define ARCH_NR_GPIOS 512
> -#endif
> -#endif
> -
> /*
> - * "valid" GPIO numbers are nonnegative and may be passed to
> - * setup routines like gpio_request(). only some valid numbers
> - * can successfully be requested and used.
> - *
> - * Invalid GPIO numbers are useful for indicating no-such-GPIO in
> - * platform data and other tables.
> + * At the end we want all GPIOs to be dynamically allocated from 0.
> + * However, some legacy drivers still perform fixed allocation.
> + * Until they are all fixed, leave 0-512 space for them.
> */
> -
> -static inline bool gpio_is_valid(int number)
> -{
> - return number >= 0 && number < ARCH_NR_GPIOS;
> -}
> +#define GPIO_DYNAMIC_BASE 512
>
> struct device;
> struct gpio;
> @@ -140,12 +118,6 @@ static inline void gpio_unexport(unsigned gpio)
>
> #include <linux/kernel.h>
>
> -static inline bool gpio_is_valid(int number)
> -{
> - /* only non-negative numbers are valid */
> - return number >= 0;
> -}
> -
> /* platforms that don't directly support access to GPIOs through I2C, SPI,
> * or other blocking infrastructure can use these wrappers.
> */
> @@ -169,4 +141,19 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
>
> #endif /* !CONFIG_GPIOLIB */
>
> +/*
> + * "valid" GPIO numbers are nonnegative and may be passed to
> + * setup routines like gpio_request(). only some valid numbers
> + * can successfully be requested and used.
> + *
> + * Invalid GPIO numbers are useful for indicating no-such-GPIO in
> + * platform data and other tables.
> + */
> +
> +static inline bool gpio_is_valid(int number)
> +{
> + /* only non-negative numbers are valid */
> + return number >= 0;
> +}
> +
> #endif /* _ASM_GENERIC_GPIO_H */
> --
> 2.37.1
>


--
With Best Regards,
Andy Shevchenko