On Sat, Jul 3, 2021 at 2:05 PM Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> wrote: > > Hi Andy, > > On Sat, Jul 3, 2021 at 1:32 PM Andy Shevchenko > <andy.shevchenko@xxxxxxxxx> wrote: > > > > On Sat, Jul 3, 2021 at 2:06 PM Sergio Paracuellos > > <sergio.paracuellos@xxxxxxxxx> wrote: > > > On Fri, Jul 2, 2021 at 1:30 PM Sergio Paracuellos > > > <sergio.paracuellos@xxxxxxxxx> wrote: > > > > ... > > > > > - ret = devprop_gpiochip_set_names(gc); > > > + ret = devprop_gpiochip_set_names(gc, 0); > > > > I had been expecting that this parameter would be in the field of the gpiochip. > > > > ... > > If doing it in that way is preferred, I have no problem at all. But in > that case I think there is no need for a new > 'devprop_gpiochip_set_names_base' and we can assume for all drivers to > be zero and if is set taking it into account directly in > devprop_gpiochip_set_names function? Is this what you mean by having > this field added there?? How about something like this? diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c index 82fb20dca53a..5854a9343491 100644 --- a/drivers/gpio/gpio-mt7621.c +++ b/drivers/gpio/gpio-mt7621.c @@ -241,6 +241,7 @@ mediatek_gpio_bank_probe(struct device *dev, if (!rg->chip.label) return -ENOMEM; + rg->chip.offset = bank * MTK_BANK_WIDTH; rg->irq_chip.name = dev_name(dev); rg->irq_chip.parent_device = dev; rg->irq_chip.irq_unmask = mediatek_gpio_irq_unmask; diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 6e3c4d7a7d14..0587f46b7c22 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -380,10 +380,10 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) return 0; count = device_property_string_array_count(dev, "gpio-line-names"); - if (count < 0) + if (count < 0 || count <= chip->offset) return 0; - if (count > gdev->ngpio) { + if (count > gdev->ngpio && chip->offset == 0) { dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d", count, gdev->ngpio); count = gdev->ngpio; @@ -401,8 +401,9 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip) return ret; } + count = (chip->offset >= count) ? (chip->offset - count) : count; 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 4a7e295c3640..39e0786586f6 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; + int offset; const char *const *names; bool can_sleep; Does this sound reasonable? Best regards, Sergio Paracuellos > > > > > > The problem I see with this approach is that > > > 'devprop_gpiochip_set_names' already trusts in gpio_device already > > > created and this happens in 'gpiochip_add_data_with_key'. So doing in > > > this way force "broken drivers" to call this new > > > 'devprop_gpiochip_set_names_base' function after > > > 'devm_gpiochip_add_data' is called so the core code has already set up > > > the friendly names repeated for all gpio chip banks and the approach > > > would be to "overwrite" those in a second pass which sounds more like > > > a hack than a solution. > > > > > > But maybe I am missing something in what you were pointing out here. > > > > Would the above work? > > The following works for me, but the overwritten part of the > 'gdev->descs[i].name' because this has been already called once by the > core code is hacky and dirty, IMHO :) > > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index 4a7e295c3640..ad145ab0794c 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -537,6 +537,8 @@ extern int gpiochip_add_data_with_key(struct > gpio_chip *gc, void *data, > devm_gpiochip_add_data_with_key(dev, gc, data, NULL, NULL) > #endif /* CONFIG_LOCKDEP */ > > +extern int devprop_gpiochip_set_names_base(struct gpio_chip *gc, int base); > + > static inline int gpiochip_add(struct gpio_chip *gc) > { > return gpiochip_add_data(gc, NULL); > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 6e3c4d7a7d14..f9942d5d2f2a 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -361,13 +361,14 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc) > /* > * devprop_gpiochip_set_names - Set GPIO line names using device properties > * @chip: GPIO chip whose lines should be named, if possible > + * @base: starting index in names array to start copying from > * > * Looks for device property "gpio-line-names" and if it exists assigns > * GPIO line names for the chip. The memory allocated for the assigned > * names belong to the underlying software node and should not be released > * by the caller. > */ > -static int devprop_gpiochip_set_names(struct gpio_chip *chip) > +static int devprop_gpiochip_set_names(struct gpio_chip *chip, int base) > { > struct gpio_device *gdev = chip->gpiodev; > struct device *dev = chip->parent; > @@ -383,12 +384,18 @@ static int devprop_gpiochip_set_names(struct > gpio_chip *chip) > if (count < 0) > return 0; > > - if (count > gdev->ngpio) { > + if (count > gdev->ngpio && base == 0) { > dev_warn(&gdev->dev, "gpio-line-names is length %d but > should be at most length %d", > count, gdev->ngpio); > count = gdev->ngpio; > } > > + if (count <= base) { > + for (i = 0; i < count; i++) > + gdev->descs[i].name = ""; > + return 0; > + } > + > names = kcalloc(count, sizeof(*names), GFP_KERNEL); > if (!names) > return -ENOMEM; > @@ -401,14 +408,21 @@ static int devprop_gpiochip_set_names(struct > gpio_chip *chip) > return ret; > } > > + count = (base >= count) ? (base - count) : count; > for (i = 0; i < count; i++) > - gdev->descs[i].name = names[i]; > + gdev->descs[i].name = names[base + i]; > > kfree(names); > > return 0; > } > > +int devprop_gpiochip_set_names_base(struct gpio_chip *gc, int base) > +{ > + return devprop_gpiochip_set_names(gc, base); > +} > +EXPORT_SYMBOL_GPL(devprop_gpiochip_set_names_base); > + > static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc) > { > unsigned long *p; > @@ -684,7 +698,7 @@ int gpiochip_add_data_with_key(struct gpio_chip > *gc, void *data, > if (gc->names) > ret = gpiochip_set_desc_names(gc); > else > - ret = devprop_gpiochip_set_names(gc); > + ret = devprop_gpiochip_set_names(gc, 0); > if (ret) > goto err_remove_from_list; > > diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c > index 82fb20dca53a..d4f19ab726b2 100644 > --- a/drivers/gpio/gpio-mt7621.c > +++ b/drivers/gpio/gpio-mt7621.c > @@ -282,6 +282,12 @@ mediatek_gpio_bank_probe(struct device *dev, > return ret; > } > > + ret = devprop_gpiochip_set_names_base(&rg->chip, bank * MTK_BANK_WIDTH); > + if (ret) { > + dev_err(dev, "Error setting line names for bank %d", bank); > + return ret; > + } > + > /* set polarity to low for all gpios */ > mtk_gpio_w32(rg, GPIO_REG_POL, 0); > > Best regards, > Sergio Paracuellos > > > > > -- > > With Best Regards, > > Andy Shevchenko