| Message ID | 20230711151248.4750-1-asmaa@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 55b2395e4e92adc492c6b30ac109eb78250dcd9d |
| Headers | show |
| Series | [v2,1/1] gpio: mmio: handle "ngpios" properly in bgpio_init() | expand |
On Tue, Jul 11, 2023 at 6:13 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(). > > 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@nvidia.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > 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()" And hence Linus' tag had been dropped. LGTM now, thank you for pursuing this! I hope Linus can review it again and Bart will be okay with the result to be applied. > v1->v2: > - Added the tags > - Updated the changelog
On Tue, Jul 11, 2023 at 5:13 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(). > > 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@nvidia.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- I'm confused. Is this the final version after all? Bart
On Tue, Jul 18, 2023 at 9:21 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Tue, Jul 11, 2023 at 5:13 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote: ... > I'm confused. Is this the final version after all? Seems to me so.
> 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@nvidia.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > 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.
On Tue, Jul 18, 2023 at 8:38 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(). > > > > 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@nvidia.com> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > --- > > 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. > Ok, I applied this one but you need to get your patch versioning in order next time. Bart
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