RE: [PATCH v2 1/1] gpio: mmio: handle "ngpios" properly in bgpio_init()

[Asmaa Mnebhi]

 > bgpio_init() uses "sz" argument to populate ngpio, which is not accurate.
> Instead, read the "ngpios" property from the DT and if it doesn't exist, use the
> "sz" argument. With this change, drivers no longer need to overwrite the ngpio
> variable after calling bgpio_init().
> 
> If the "ngpios" property is specified, bgpio_bits is calculated as the round up
> value of ngpio. At the moment, the only requirement specified is that the round
> up value must be a multiple of 8 but it should also be a power of 2 because we
> provide accessors based on the bank size in bgpio_setup_accessors().
> 
> Signed-off-by: Asmaa Mnebhi <asmaa@xxxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> ---
> The following 2 patches were approved in March 2023 but didn't make it into
> the tree:
> [PATCH v4] gpio: mmio: handle "ngpios" properly in bgpio_init() [PATCH v1]
> gpio: mmio: fix calculation of bgpio_bits
> 
> They needed a rebase and were combined into a single patch since
> "gpio: mmio: fix calculation of bgpio_bits" fixes a bug in
> "gpio: mmio: handle "ngpios" properly in bgpio_init()"
> 
> v1->v2:
> - Added the tags
> - Updated the changelog
> 
>  drivers/gpio/gpio-mmio.c |  9 +++++-
>  drivers/gpio/gpiolib.c   | 68 ++++++++++++++++++++++------------------
>  drivers/gpio/gpiolib.h   |  1 +
>  3 files changed, 46 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index
> d9dff3dc92ae..74fdf0d87b2c 100644
> --- a/drivers/gpio/gpio-mmio.c
> +++ b/drivers/gpio/gpio-mmio.c
> @@ -60,6 +60,8 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA
> is ,.`
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> 
> +#include "gpiolib.h"
> +
>  static void bgpio_write8(void __iomem *reg, unsigned long data)  {
>  	writeb(data, reg);
> @@ -614,10 +616,15 @@ int bgpio_init(struct gpio_chip *gc, struct device
> *dev,
>  	gc->parent = dev;
>  	gc->label = dev_name(dev);
>  	gc->base = -1;
> -	gc->ngpio = gc->bgpio_bits;
>  	gc->request = bgpio_request;
>  	gc->be_bits = !!(flags & BGPIOF_BIG_ENDIAN);
> 
> +	ret = gpiochip_get_ngpios(gc, dev);
> +	if (ret)
> +		gc->ngpio = gc->bgpio_bits;
> +	else
> +		gc->bgpio_bits = roundup_pow_of_two(round_up(gc->ngpio,
> 8));
> +
>  	ret = bgpio_setup_io(gc, dat, set, clr, flags);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index
> 251c875b5c34..7dac8bb9905a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -700,6 +700,40 @@ void *gpiochip_get_data(struct gpio_chip *gc)  }
> EXPORT_SYMBOL_GPL(gpiochip_get_data);
> 
> +int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev) {
> +	u32 ngpios = gc->ngpio;
> +	int ret;
> +
> +	if (ngpios == 0) {
> +		ret = device_property_read_u32(dev, "ngpios", &ngpios);
> +		if (ret == -ENODATA)
> +			/*
> +			 * -ENODATA means that there is no property found
> and
> +			 * we want to issue the error message to the user.
> +			 * Besides that, we want to return different error code
> +			 * to state that supplied value is not valid.
> +			 */
> +			ngpios = 0;
> +		else if (ret)
> +			return ret;
> +
> +		gc->ngpio = ngpios;
> +	}
> +
> +	if (gc->ngpio == 0) {
> +		chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> +		return -EINVAL;
> +	}
> +
> +	if (gc->ngpio > FASTPATH_NGPIO)
> +		chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n",
> +			gc->ngpio, FASTPATH_NGPIO);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gpiochip_get_ngpios);
> +
>  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  			       struct lock_class_key *lock_key,
>  			       struct lock_class_key *request_key) @@ -707,7
> +741,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  	struct gpio_device *gdev;
>  	unsigned long flags;
>  	unsigned int i;
> -	u32 ngpios = 0;
>  	int base = 0;
>  	int ret = 0;
> 
> @@ -753,36 +786,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc,
> void *data,
>  	else
>  		gdev->owner = THIS_MODULE;
> 
> -	/*
> -	 * Try the device properties if the driver didn't supply the number
> -	 * of GPIO lines.
> -	 */
> -	ngpios = gc->ngpio;
> -	if (ngpios == 0) {
> -		ret = device_property_read_u32(&gdev->dev, "ngpios",
> &ngpios);
> -		if (ret == -ENODATA)
> -			/*
> -			 * -ENODATA means that there is no property found
> and
> -			 * we want to issue the error message to the user.
> -			 * Besides that, we want to return different error code
> -			 * to state that supplied value is not valid.
> -			 */
> -			ngpios = 0;
> -		else if (ret)
> -			goto err_free_dev_name;
> -
> -		gc->ngpio = ngpios;
> -	}
> -
> -	if (gc->ngpio == 0) {
> -		chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> -		ret = -EINVAL;
> +	ret = gpiochip_get_ngpios(gc, &gdev->dev);
> +	if (ret)
>  		goto err_free_dev_name;
> -	}
> -
> -	if (gc->ngpio > FASTPATH_NGPIO)
> -		chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n",
> -			  gc->ngpio, FASTPATH_NGPIO);
> 
>  	gdev->descs = kcalloc(gc->ngpio, sizeof(*gdev->descs), GFP_KERNEL);
>  	if (!gdev->descs) {
> @@ -947,7 +953,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc,
> void *data,
>  	/* failures here can mean systems won't boot... */
>  	if (ret != -EPROBE_DEFER) {
>  		pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n",
> __func__,
> -		       base, base + (int)ngpios - 1,
> +		       base, base + (int)gc->ngpio - 1,
>  		       gc->label ? : "generic", ret);
>  	}
>  	return ret;
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index
> cca81375f127..8de748a16d13 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -217,6 +217,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const
> char *con_id,  int gpio_set_debounce_timeout(struct gpio_desc *desc,
> unsigned int debounce);  int gpiod_hog(struct gpio_desc *desc, const char
> *name,
>  		unsigned long lflags, enum gpiod_flags dflags);
> +int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev);
> 
>  /*
>   * Return the GPIO number of the passed descriptor relative to its chip
> --
> 2.30.1

Hi Bart,

This is the final approved patch by both Linus and Andy. Please discard all others.