| Message ID | 20200324135653.6676-4-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpio: Add GPIO Aggregator | expand |
On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven <geert+renesas@glider.be> wrote: > Currently a GPIO lookup table can only refer to a specific GPIO by a > tuple, consisting of a GPIO controller label and a GPIO offset inside > the controller. > > However, a GPIO may also carry a line name, defined by DT or ACPI. > If present, the line name is the most use-centric way to refer to a > GPIO. Hence add support for looking up GPIOs by line name. > > Implement this by reusing the existing gpiod_lookup infrastructure. > 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 kind of like this approach, however there are things here that need to be considered: the line name is in no way globally unique, and I think there are already quite a few GPIO chips that have the same line names assigned for every instance of that chip. gpiochip_set_desc_names() only warns if there is a line with the same name on the same gpio_chip. I suppose we need to document that the line name look-up will be on a first-come-first-served basis: whatever line we find first with this name is what you will get a reference to, no matter what chip it is on, and it is possible albeit not recommended that some other chip has a line with the same name. Yours, Linus Walleij
Hi Linus, On Thu, Mar 26, 2020 at 10:18 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven > <geert+renesas@glider.be> wrote: > > Currently a GPIO lookup table can only refer to a specific GPIO by a > > tuple, consisting of a GPIO controller label and a GPIO offset inside > > the controller. > > > > However, a GPIO may also carry a line name, defined by DT or ACPI. > > If present, the line name is the most use-centric way to refer to a > > GPIO. Hence add support for looking up GPIOs by line name. > > > > Implement this by reusing the existing gpiod_lookup infrastructure. > > 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 kind of like this approach, however there are things here that > need to be considered: the line name is in no way globally unique, > and I think there are already quite a few GPIO chips that > have the same line names assigned for every instance of that > chip. > > gpiochip_set_desc_names() only warns if there is a line with > the same name on the same gpio_chip. on a _different_ gpio chip. > I suppose we need to document that the line name look-up > will be on a first-come-first-served basis: whatever line > we find first with this name is what you will get a reference > to, no matter what chip it is on, and it is possible albeit > not recommended that some other chip has a line with the > same name. Agreed. Gr{oetje,eeting}s, Geert
diff --git a/Documentation/driver-api/gpio/board.rst b/Documentation/driver-api/gpio/board.rst index ce91518bf9f48ded..0ad1f8cacf5e5d26 100644 --- 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. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 8d7366f4451fe695..c756602e249c052e 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -4643,7 +4643,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 */ @@ -4654,18 +4654,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); } @@ -4696,7 +4708,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, \