[PATCH v4] gpio: mmio: handle "ngpios" properly in bgpio_init()

[PATCH v4] 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().

Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 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..b52a3dd511ca 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 = 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 939c776b9488..1964ec64e356 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -647,6 +647,40 @@ static void gpiochip_setup_devs(void)
 	}
 }
 
+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)
@@ -655,7 +689,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;
 
@@ -704,36 +737,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) {
@@ -903,7 +909,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 b3c2db6eba80..c38cbf1b753b 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -207,6 +207,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

Re: [PATCH v4] gpio: mmio: handle "ngpios" properly in bgpio_init()

[andy.shevchenko]

Mon, Mar 06, 2023 at 04:12:37PM -0500, Asmaa Mnebhi kirjoitti:
> 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().

Thank you!

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Sorry it took a bit longer for us to settle on this.

> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> ---
>  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..b52a3dd511ca 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 = 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 939c776b9488..1964ec64e356 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -647,6 +647,40 @@ static void gpiochip_setup_devs(void)
>  	}
>  }
>  
> +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)
> @@ -655,7 +689,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;
>  
> @@ -704,36 +737,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) {
> @@ -903,7 +909,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 b3c2db6eba80..c38cbf1b753b 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -207,6 +207,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
> 

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4] gpio: mmio: handle "ngpios" properly in bgpio_init()

[Asmaa Mnebhi]

 > Mon, Mar 06, 2023 at 04:12:37PM -0500, Asmaa Mnebhi kirjoitti:
> > 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().
> 
> Thank you!
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Sorry it took a bit longer for us to settle on this.

Thank you Andy!

RE: [PATCH v4] gpio: mmio: handle "ngpios" properly in bgpio_init()

[Asmaa Mnebhi]

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Hi Bart,

Could you please add this patch to the tree?

Thanks.
Asmaa

Re: [PATCH v4] gpio: mmio: handle "ngpios" properly in bgpio_init()

[Bartosz Golaszewski]

On Mon, Jul 10, 2023 at 7:26 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> Hi Bart,
>
> Could you please add this patch to the tree?
>
> Thanks.
> Asmaa

No, because it doesn't apply to v6.5-rc1. Please rebase and resend to
my current email address.

Bartosz

Re: [PATCH v4] gpio: mmio: handle "ngpios" properly in bgpio_init()

[Linus Walleij]

On Mon, Mar 6, 2023 at 10:12 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:

> 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().
>
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v4] gpio: mmio: handle "ngpios" properly in bgpio_init()

[Christian Lamparter]

On 7/16/23 09:42, Linus Walleij wrote:
> On Mon, Mar 6, 2023 at 10:12 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
> 
>> 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().
>>
>> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thank you!

For context:
<https://patchwork.kernel.org/project/linux-arm-kernel/patch/535f785bf6116c0fb6f46afbb77e6d4bd3ef5f60.1462543458.git.chunkeey@googlemail.com/>

Regards,
Christian