| Message ID | 20200218151812.7816-3-geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: Add GPIO Aggregator | expand |
On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven <geert+renesas@glider.be> wrote: > Currently GPIOs can only be referred to by GPIO controller and offset in > GPIO lookup tables. > > Add support for looking them up by line name. > Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear > that this field can have two meanings, and update the kerneldoc and > GPIO_LOOKUP*() macros. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu> > Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com> > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- a/include/linux/gpio/machine.h > +++ b/include/linux/gpio/machine.h > @@ -20,8 +20,9 @@ enum gpio_lookup_flags { > > /** > * struct gpiod_lookup - lookup table > - * @chip_label: name of the chip the GPIO belongs to > - * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO > + * @key: either the name of the chip the GPIO belongs to, or the GPIO line name > + * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO, or > + * U16_MAX to indicate that @key is a GPIO line name > * @con_id: name of the GPIO from the device's point of view > * @idx: index of the GPIO in case several GPIOs share the same name > * @flags: bitmask of gpio_lookup_flags GPIO_* values > @@ -30,7 +31,7 @@ enum gpio_lookup_flags { > * functions using platform data. > */ > struct gpiod_lookup { > - const char *chip_label; > + const char *key; > u16 chip_hwnum; > const char *con_id; > unsigned int idx; This needs an update in the documentation: --- a/Documentation/driver-api/gpio/board.rst +++ b/Documentation/driver-api/gpio/board.rst @@ -113,13 +113,15 @@ files that desire to do so need to include the following header:: GPIOs are mapped by the means of tables of lookups, containing instances of the gpiod_lookup structure. Two macros are defined to help declaring such mappings:: - GPIO_LOOKUP(chip_label, chip_hwnum, con_id, flags) - GPIO_LOOKUP_IDX(chip_label, chip_hwnum, con_id, idx, flags) + GPIO_LOOKUP(key, chip_hwnum, con_id, flags) + GPIO_LOOKUP_IDX(key, chip_hwnum, con_id, idx, flags) where - - chip_label is the label of the gpiod_chip instance providing the GPIO - - chip_hwnum is the hardware number of the GPIO within the chip + - key is either the label of the gpiod_chip instance providing the GPIO, or + the GPIO line name + - chip_hwnum is the hardware number of the GPIO within the chip, or U16_MAX + to indicate that key is a GPIO line name - con_id is the name of the GPIO function from the device point of view. It can be NULL, in which case it will match any function. - idx is the index of the GPIO within the function. Furthermore, a few drivers populate the gpiod_lookup members directly, instead of using the convenience macros: arch/arm/mach-integrator/impd1.c drivers/i2c/busses/i2c-i801.c drivers/mfd/sm501.c Either they have to be updated s/chip_label/key/, or start using the macros, e.g. --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1444,9 +1444,9 @@ static int i801_add_mux(struct i801_priv *priv) return -ENOMEM; lookup->dev_id = "i2c-mux-gpio"; for (i = 0; i < mux_config->n_gpios; i++) { - lookup->table[i].chip_label = mux_config->gpio_chip; - lookup->table[i].chip_hwnum = mux_config->gpios[i]; - lookup->table[i].con_id = "mux"; + lookup->table[i] = (struct gpiod_lookup) + GPIO_LOOKUP(mux_config->gpio_chip, + mux_config->gpios[i], "mux", 0); } gpiod_add_lookup_table(lookup); priv->lookup = lookup; Do you have any preference? Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, I'm sorry for the slow review, it's a large patch set and takes some time to sit down and review, and see whether my earlier comments have been addressed. On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven <geert+renesas@glider.be> wrote: > Currently GPIOs can only be referred to by GPIO controller and offset in > GPIO lookup tables. > > Add support for looking them up by line name. > Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear > that this field can have two meanings, and update the kerneldoc and > GPIO_LOOKUP*() macros. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu> > Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com> > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com> I will try to understand why this change is necessary to implement the gpio aggregator (probablt I will comment that on the other patches like "aha now I see it" or so, but it would help a lot if the commit message would state the technical reason to why we need to do this change, like what it is that you want to do and why you cannot do it without this change. Yours, Linus Walleij
Hi Linus, On Thu, Mar 12, 2020 at 3:21 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven > <geert+renesas@glider.be> wrote: > > Currently GPIOs can only be referred to by GPIO controller and offset in > > GPIO lookup tables. > > > > Add support for looking them up by line name. > > Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear > > that this field can have two meanings, and update the kerneldoc and > > GPIO_LOOKUP*() macros. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu> > > Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > I will try to understand why this change is necessary to implement > the gpio aggregator (probablt I will comment that on the other > patches like "aha now I see it" or so, but it would help a lot if the > commit message > would state the technical reason to why we need to do this change, > like what it is that you want to do and why you cannot do it without > this change. It's very simple: how do you want the user to refer to a specific GPIO line? Currently he can only do so by gpiochip label and index. However, there exists another stable reference: the (optional) line name, which can be attached using "gpio-line-names" in DT or ACPI. As this is the most use-centric way to refer to a GPIO, it makes sense to support lookup based on that, too. Will reword to make this clearer. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 200c2d2be4b78043..24c02167f9e5472f 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -4453,7 +4453,7 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, if (!table) return desc; - for (p = &table->table[0]; p->chip_label; p++) { + for (p = &table->table[0]; p->key; p++) { struct gpio_chip *chip; /* idx must always match exactly */ @@ -4464,18 +4464,30 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, if (p->con_id && (!con_id || strcmp(p->con_id, con_id))) continue; - chip = find_chip_by_name(p->chip_label); + if (p->chip_hwnum == U16_MAX) { + desc = gpio_name_to_desc(p->key); + if (desc) { + *flags = p->flags; + return desc; + } + + dev_warn(dev, "cannot find GPIO line %s, deferring\n", + p->key); + return ERR_PTR(-EPROBE_DEFER); + } + + chip = find_chip_by_name(p->key); if (!chip) { /* * As the lookup table indicates a chip with - * p->chip_label should exist, assume it may + * p->key should exist, assume it may * still appear later and let the interested * consumer be probed again or let the Deferred * Probe infrastructure handle the error. */ dev_warn(dev, "cannot find GPIO chip %s, deferring\n", - p->chip_label); + p->key); return ERR_PTR(-EPROBE_DEFER); } @@ -4506,7 +4518,7 @@ static int platform_gpio_count(struct device *dev, const char *con_id) if (!table) return -ENOENT; - for (p = &table->table[0]; p->chip_label; p++) { + for (p = &table->table[0]; p->key; p++) { if ((con_id && p->con_id && !strcmp(con_id, p->con_id)) || (!con_id && !p->con_id)) count++; diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h index 1ebe5be05d5f81fa..84c66fbf54fd5811 100644 --- a/include/linux/gpio/machine.h +++ b/include/linux/gpio/machine.h @@ -20,8 +20,9 @@ enum gpio_lookup_flags { /** * struct gpiod_lookup - lookup table - * @chip_label: name of the chip the GPIO belongs to - * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO + * @key: either the name of the chip the GPIO belongs to, or the GPIO line name + * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO, or + * U16_MAX to indicate that @key is a GPIO line name * @con_id: name of the GPIO from the device's point of view * @idx: index of the GPIO in case several GPIOs share the same name * @flags: bitmask of gpio_lookup_flags GPIO_* values @@ -30,7 +31,7 @@ enum gpio_lookup_flags { * functions using platform data. */ struct gpiod_lookup { - const char *chip_label; + const char *key; u16 chip_hwnum; const char *con_id; unsigned int idx; @@ -63,17 +64,17 @@ struct gpiod_hog { /* * Simple definition of a single GPIO under a con_id */ -#define GPIO_LOOKUP(_chip_label, _chip_hwnum, _con_id, _flags) \ - GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _con_id, 0, _flags) +#define GPIO_LOOKUP(_key, _chip_hwnum, _con_id, _flags) \ + GPIO_LOOKUP_IDX(_key, _chip_hwnum, _con_id, 0, _flags) /* * Use this macro if you need to have several GPIOs under the same con_id. * Each GPIO needs to use a different index and can be accessed using * gpiod_get_index() */ -#define GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _con_id, _idx, _flags) \ +#define GPIO_LOOKUP_IDX(_key, _chip_hwnum, _con_id, _idx, _flags) \ { \ - .chip_label = _chip_label, \ + .key = _key, \ .chip_hwnum = _chip_hwnum, \ .con_id = _con_id, \ .idx = _idx, \