Re: [PATCH v3 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks per device
From: Gregory Fong <hidden>
Date: 2021-07-27 23:15:41
Also in:
lkml
On Tue, Jul 27, 2021 at 8:20 AM Sergio Paracuellos [off-list ref] 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 <redacted>
One minor comment below, then this looks great: Reviewed-by: Gregory Fong <redacted>
quoted hunk ↗ jump to hunk
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