| Message ID | 20210727152026.31019-2-sergio.paracuellos@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks per device | expand |
On Tue, Jul 27, 2021 at 8:20 AM Sergio Paracuellos <sergio.paracuellos@gmail.com> wrote: > > The default gpiolib-of implementation does not work with the multiple > gpiochip banks per device structure used for example by the gpio-mt7621 > and gpio-brcmstb drivers. To fix these kind of situations driver code > is forced to fill the names to avoid the gpiolib code to set names > repeated along the banks. Instead of continue with that antipattern > fix the gpiolib core function to get expected behaviour for every > single situation adding a field 'offset' in the gpiochip structure. > Doing in this way, we can assume this offset will be zero for normal > driver code where only one gpiochip bank per device is used but > can be set explicitly in those drivers that really need more than > one gpiochip. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> One minor comment below, then this looks great: Reviewed-by: Gregory Fong <gregory.0xf0@gmail.com> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > --- > drivers/gpio/gpiolib.c | 32 +++++++++++++++++++++++++++----- > include/linux/gpio/driver.h | 4 ++++ > 2 files changed, 31 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 27c07108496d..84ed4b73fa3e 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -382,10 +382,18 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) > if (count < 0) > return 0; > > - if (count > gdev->ngpio) { > - dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d", > - count, gdev->ngpio); > - count = gdev->ngpio; > + /* > + * When offset is set in the driver side we assume the driver internally > + * is using more than one gpiochip per the same device. We have to stop > + * setting friendly names if the specified ones with 'gpio-line-names' > + * are less than the offset in the device itself. This means all the > + * lines are not present for every single pin within all the internal > + * gpiochips. > + */ > + if (count <= chip->offset) { > + dev_warn(&gdev->dev, "gpio-line-names too short (length %d) cannot map names for the gpiochip at offset %u\n", nit: there should be some punctuation after "(length %d)", otherwise with parentheticals removed it reads as "gpio-line-names too short cannot map names ..." but we need to provide a space between these thoughts for clarity. A comma should be ok: "gpio-line-names too short (length %d), cannot map names ..." Best regards, Gregory
On Wed, Jul 28, 2021 at 1:15 AM Gregory Fong <gregory.0xf0@gmail.com> wrote: > > On Tue, Jul 27, 2021 at 8:20 AM Sergio Paracuellos > <sergio.paracuellos@gmail.com> wrote: > > > > The default gpiolib-of implementation does not work with the multiple > > gpiochip banks per device structure used for example by the gpio-mt7621 > > and gpio-brcmstb drivers. To fix these kind of situations driver code > > is forced to fill the names to avoid the gpiolib code to set names > > repeated along the banks. Instead of continue with that antipattern > > fix the gpiolib core function to get expected behaviour for every > > single situation adding a field 'offset' in the gpiochip structure. > > Doing in this way, we can assume this offset will be zero for normal > > driver code where only one gpiochip bank per device is used but > > can be set explicitly in those drivers that really need more than > > one gpiochip. > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > One minor comment below, then this looks great: > Reviewed-by: Gregory Fong <gregory.0xf0@gmail.com> > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > > --- > > drivers/gpio/gpiolib.c | 32 +++++++++++++++++++++++++++----- > > include/linux/gpio/driver.h | 4 ++++ > > 2 files changed, 31 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index 27c07108496d..84ed4b73fa3e 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -382,10 +382,18 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) > > if (count < 0) > > return 0; > > > > - if (count > gdev->ngpio) { > > - dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d", > > - count, gdev->ngpio); > > - count = gdev->ngpio; > > + /* > > + * When offset is set in the driver side we assume the driver internally > > + * is using more than one gpiochip per the same device. We have to stop > > + * setting friendly names if the specified ones with 'gpio-line-names' > > + * are less than the offset in the device itself. This means all the > > + * lines are not present for every single pin within all the internal > > + * gpiochips. > > + */ > > + if (count <= chip->offset) { > > + dev_warn(&gdev->dev, "gpio-line-names too short (length %d) cannot map names for the gpiochip at offset %u\n", > > nit: there should be some punctuation after "(length %d)", otherwise > with parentheticals removed it reads as > > "gpio-line-names too short cannot map names ..." > > but we need to provide a space between these thoughts for clarity. A > comma should be ok: > > "gpio-line-names too short (length %d), cannot map names ..." Will add it, thanks. Best regards, Sergio Paracuellos > > Best regards, > Gregory
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 27c07108496d..84ed4b73fa3e 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -382,10 +382,18 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) if (count < 0) return 0; - if (count > gdev->ngpio) { - dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d", - count, gdev->ngpio); - count = gdev->ngpio; + /* + * When offset is set in the driver side we assume the driver internally + * is using more than one gpiochip per the same device. We have to stop + * setting friendly names if the specified ones with 'gpio-line-names' + * are less than the offset in the device itself. This means all the + * lines are not present for every single pin within all the internal + * gpiochips. + */ + if (count <= chip->offset) { + dev_warn(&gdev->dev, "gpio-line-names too short (length %d) cannot map names for the gpiochip at offset %u\n", + count, chip->offset); + return 0; } names = kcalloc(count, sizeof(*names), GFP_KERNEL); @@ -400,8 +408,22 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) return ret; } + /* + * When more that one gpiochip per device is used, 'count' can + * contain at most number gpiochips x chip->ngpio. We have to + * correctly distribute all defined lines taking into account + * chip->offset as starting point from where we will assign + * the names to pins from the 'names' array. Since property + * 'gpio-line-names' cannot contains gaps, we have to be sure + * we only assign those pins that really exists since chip->ngpio + * can be different of the chip->offset. + */ + count = (count > chip->offset) ? count - chip->offset : count; + if (count > chip->ngpio) + count = chip->ngpio; + for (i = 0; i < count; i++) - gdev->descs[i].name = names[i]; + gdev->descs[i].name = names[chip->offset + i]; kfree(names); diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 3a268781fcec..a0f9901dcae6 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -312,6 +312,9 @@ struct gpio_irq_chip { * get rid of the static GPIO number space in the long run. * @ngpio: the number of GPIOs handled by this controller; the last GPIO * handled is (base + ngpio - 1). + * @offset: when multiple gpio chips belong to the same device this + * can be used as offset within the device so friendly names can + * be properly assigned. * @names: if set, must be an array of strings to use as alternative * names for the GPIOs in this chip. Any entry in the array * may be NULL if there is no alias for the GPIO, however the @@ -398,6 +401,7 @@ struct gpio_chip { int base; u16 ngpio; + u16 offset; const char *const *names; bool can_sleep;