| Message ID | 1461073157-26574-1-git-send-email-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 19, 2016 at 03:39:17PM +0200, Linus Walleij wrote: > Make it possible to name the producer side of a GPIO line using > a "gpio-names" property array, modeled on the "clock-names" > property from the clock bindings. > > This naming is especially useful for: > > - Debugging: lines are named after function, not just opaque > offset numbers. > > - Exploration: systems where some or all GPIO lines are available > to end users, such as prototyping, one-off's "makerspace usecases" > users are helped by the names of the GPIO lines when tinkering. > This usecase has been surfacing recently. > > The gpio-names attribute is completely optional. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: devicetree@vger.kernel.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > This has been discussed at some length now. > > Why we are not using hogs: these are consumer side, not producer > side. The gpio-controller in DT (gpio_chip in Linux) is a > producer, not a consumer. > > This patch is not about assigning initial values to GPIO lines. > That is an orthogonal usecase. This is just about naming lines. > --- > Documentation/devicetree/bindings/gpio/gpio.txt | 19 +++++++++++ > drivers/gpio/gpiolib-of.c | 43 +++++++++++++++++++++++++ > 2 files changed, 62 insertions(+) > > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt > index c88d2ccb05ca..6b371ab6098e 100644 > --- a/Documentation/devicetree/bindings/gpio/gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt > @@ -152,6 +152,21 @@ additional bitmask is needed to specify which GPIOs are actually in use, > and which are dummies. The bindings for this case has not yet been > specified, but should be specified if/when such hardware appears. > > +Optionally, a GPIO controller may have a "gpio-names" property. This is > +an array of strings defining the names of the GPIO lines going out of the > +GPIO controller. This name should be the most meaningful producer name > +for the system, such as a rail name indicating the usage. Package names > +such as pin name are discouraged: such lines have opaque names (since they > +are by definition generic purpose) and such names are usually not very > +helpful. For example "MMC-CD", "Red LED Vdd" and "ethernet reset" are > +reasonable line names as they describe what the line is used for. "GPIO0" > +is not a good name to give to a GPIO line. Placeholders are discouraged: > +rather use the "" (blank string) if the use of the GPIO line is undefined > +in your design. The names are assigned starting from line offset 0 from > +left to right from the passed array. An incomplete array (where the number > +of passed named are less than ngpios) will still be used up until the last > +provided valid line index. > + > Example: > > gpio-controller@00000000 { > @@ -160,6 +175,10 @@ gpio-controller@00000000 { > gpio-controller; > #gpio-cells = <2>; > ngpios = <18>; > + gpio-names = "MMC-CD", "MMC-WP", "VDD eth", "RST eth", "LED R", > + "LED G", "LED B", "Col A", "Col B", "Col C", "Col D", > + "Row A", "Row B", "Row C", "Row D", "NMI button", > + "poweroff", "reset"; > } > > The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index d81dbd8e90d9..b4e3a42e7aae 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -196,6 +196,42 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np, > } > > /** > + * of_gpiochip_set_names() - set up the names of the lines > + * @chip: GPIO chip whose lines should be named, if possible > + */ > +static int of_gpiochip_set_names(struct gpio_chip *gc) > +{ > + struct gpio_device *gdev = gc->gpiodev; > + struct device_node *np = gc->of_node; > + int i; > + int nstrings; > + > + /* Do we even have the "gpio-names" property */ > + if (!of_property_read_bool(np, "gpio-names")) > + return 0; > + > + nstrings = of_property_count_strings(np, "gpio-names"); > + for (i = 0; i < nstrings; i++) { > + const char *name; > + int ret; > + > + ret = of_property_read_string_index(np, > + "gpio-names", > + i, > + &name); > + if (!ret) > + gdev->descs[i].name = name; > + > + /* Empty strings are OK */ > + if (ret != -ENODATA) > + dev_err(&gdev->dev, "unable to name line %d\n", > + i); Linus, This above if statement does not make sense. Sure empty strings are okay but it seems that an error would be printed even when the name is set above when ret is 0. Using else if perhaps would make more sense. > + } > + > + return 0; > +} > + > +/** > * of_gpiochip_scan_gpios - Scan gpio-controller for gpio definitions > * @chip: gpio chip to act on > * > @@ -445,6 +481,13 @@ int of_gpiochip_add(struct gpio_chip *chip) > if (status) > return status; > > + /* If the chip defines names itself, these take precedence */ > + if (!chip->names) { > + status = of_gpiochip_set_names(chip); Looking at the above of_gpiochip_set_names function it always returns 0. Not sure if that was the intention but it renders this if statement below useless. Regards, Michael > + if (status) > + return status; > + } > + > of_node_get(chip->of_node); > > return of_gpiochip_scan_gpios(chip); > -- > 2.4.11 > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 19, 2016 at 10:39 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > Make it possible to name the producer side of a GPIO line using > a "gpio-names" property array, modeled on the "clock-names" > property from the clock bindings. > > This naming is especially useful for: > > - Debugging: lines are named after function, not just opaque > offset numbers. > > - Exploration: systems where some or all GPIO lines are available > to end users, such as prototyping, one-off's "makerspace usecases" > users are helped by the names of the GPIO lines when tinkering. > This usecase has been surfacing recently. > > The gpio-names attribute is completely optional. Been a little bit late on this discussion, but I like the idea. Acked-by: Alexandre Courbot <acourbot@nvidia.com> One nit below... > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: devicetree@vger.kernel.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > This has been discussed at some length now. > > Why we are not using hogs: these are consumer side, not producer > side. The gpio-controller in DT (gpio_chip in Linux) is a > producer, not a consumer. > > This patch is not about assigning initial values to GPIO lines. > That is an orthogonal usecase. This is just about naming lines. > --- > Documentation/devicetree/bindings/gpio/gpio.txt | 19 +++++++++++ > drivers/gpio/gpiolib-of.c | 43 +++++++++++++++++++++++++ > 2 files changed, 62 insertions(+) > > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt > index c88d2ccb05ca..6b371ab6098e 100644 > --- a/Documentation/devicetree/bindings/gpio/gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt > @@ -152,6 +152,21 @@ additional bitmask is needed to specify which GPIOs are actually in use, > and which are dummies. The bindings for this case has not yet been > specified, but should be specified if/when such hardware appears. > > +Optionally, a GPIO controller may have a "gpio-names" property. This is > +an array of strings defining the names of the GPIO lines going out of the > +GPIO controller. This name should be the most meaningful producer name > +for the system, such as a rail name indicating the usage. Package names > +such as pin name are discouraged: such lines have opaque names (since they > +are by definition generic purpose) and such names are usually not very > +helpful. For example "MMC-CD", "Red LED Vdd" and "ethernet reset" are > +reasonable line names as they describe what the line is used for. "GPIO0" > +is not a good name to give to a GPIO line. Placeholders are discouraged: > +rather use the "" (blank string) if the use of the GPIO line is undefined > +in your design. The names are assigned starting from line offset 0 from > +left to right from the passed array. An incomplete array (where the number > +of passed named are less than ngpios) will still be used up until the last > +provided valid line index. > + > Example: > > gpio-controller@00000000 { > @@ -160,6 +175,10 @@ gpio-controller@00000000 { > gpio-controller; > #gpio-cells = <2>; > ngpios = <18>; > + gpio-names = "MMC-CD", "MMC-WP", "VDD eth", "RST eth", "LED R", > + "LED G", "LED B", "Col A", "Col B", "Col C", "Col D", > + "Row A", "Row B", "Row C", "Row D", "NMI button", > + "poweroff", "reset"; > } > > The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index d81dbd8e90d9..b4e3a42e7aae 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -196,6 +196,42 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np, > } > > /** > + * of_gpiochip_set_names() - set up the names of the lines > + * @chip: GPIO chip whose lines should be named, if possible > + */ > +static int of_gpiochip_set_names(struct gpio_chip *gc) > +{ > + struct gpio_device *gdev = gc->gpiodev; > + struct device_node *np = gc->of_node; > + int i; > + int nstrings; > + > + /* Do we even have the "gpio-names" property */ > + if (!of_property_read_bool(np, "gpio-names")) > + return 0; > + > + nstrings = of_property_count_strings(np, "gpio-names"); > + for (i = 0; i < nstrings; i++) { > + const char *name; > + int ret; > + > + ret = of_property_read_string_index(np, > + "gpio-names", > + i, > + &name); > + if (!ret) > + gdev->descs[i].name = name; Shouldn't we check for name collision (by calling gpio_name_to_desc() as gpiochip_set_desc_names() does) here? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 20, 2016 at 3:10 AM, Alexandre Courbot <gnurou@gmail.com> wrote: >> + if (!ret) >> + gdev->descs[i].name = name; > > Shouldn't we check for name collision (by calling gpio_name_to_desc() > as gpiochip_set_desc_names() does) here? That check is there to avoid getting the same name twice in sysfs (which would fail), and this is not for sysfs, it is for the chardev. And I don't think so: atleast we should not look globally like that thing does. It need not be a unique name for the system, but I don't know if we should even enforce it to be a unique name for the chip. What do people think? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 21, 2016 at 7:21 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Apr 20, 2016 at 3:10 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > >>> + if (!ret) >>> + gdev->descs[i].name = name; >> >> Shouldn't we check for name collision (by calling gpio_name_to_desc() >> as gpiochip_set_desc_names() does) here? > > That check is there to avoid getting the same name twice in > sysfs (which would fail), and this is not for sysfs, it is for the chardev. > > And I don't think so: atleast we should not look globally like that thing > does. It need not be a unique name for the system, but I don't know > if we should even enforce it to be a unique name for the chip. > > What do people think? As long as we don't need to do things like requesting a GPIO by (chip, name) tuple, then there should indeed be no issue with having several lines have the same name. On the other hand, we want lines to be named precisely, and two lines with the same name would indicate a lack of precision (they cannot have *exactly* the same function, can they). So I'm a bit torn as to whether we should enforce this or not. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt index c88d2ccb05ca..6b371ab6098e 100644 --- a/Documentation/devicetree/bindings/gpio/gpio.txt +++ b/Documentation/devicetree/bindings/gpio/gpio.txt @@ -152,6 +152,21 @@ additional bitmask is needed to specify which GPIOs are actually in use, and which are dummies. The bindings for this case has not yet been specified, but should be specified if/when such hardware appears. +Optionally, a GPIO controller may have a "gpio-names" property. This is +an array of strings defining the names of the GPIO lines going out of the +GPIO controller. This name should be the most meaningful producer name +for the system, such as a rail name indicating the usage. Package names +such as pin name are discouraged: such lines have opaque names (since they +are by definition generic purpose) and such names are usually not very +helpful. For example "MMC-CD", "Red LED Vdd" and "ethernet reset" are +reasonable line names as they describe what the line is used for. "GPIO0" +is not a good name to give to a GPIO line. Placeholders are discouraged: +rather use the "" (blank string) if the use of the GPIO line is undefined +in your design. The names are assigned starting from line offset 0 from +left to right from the passed array. An incomplete array (where the number +of passed named are less than ngpios) will still be used up until the last +provided valid line index. + Example: gpio-controller@00000000 { @@ -160,6 +175,10 @@ gpio-controller@00000000 { gpio-controller; #gpio-cells = <2>; ngpios = <18>; + gpio-names = "MMC-CD", "MMC-WP", "VDD eth", "RST eth", "LED R", + "LED G", "LED B", "Col A", "Col B", "Col C", "Col D", + "Row A", "Row B", "Row C", "Row D", "NMI button", + "poweroff", "reset"; } The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index d81dbd8e90d9..b4e3a42e7aae 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -196,6 +196,42 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np, } /** + * of_gpiochip_set_names() - set up the names of the lines + * @chip: GPIO chip whose lines should be named, if possible + */ +static int of_gpiochip_set_names(struct gpio_chip *gc) +{ + struct gpio_device *gdev = gc->gpiodev; + struct device_node *np = gc->of_node; + int i; + int nstrings; + + /* Do we even have the "gpio-names" property */ + if (!of_property_read_bool(np, "gpio-names")) + return 0; + + nstrings = of_property_count_strings(np, "gpio-names"); + for (i = 0; i < nstrings; i++) { + const char *name; + int ret; + + ret = of_property_read_string_index(np, + "gpio-names", + i, + &name); + if (!ret) + gdev->descs[i].name = name; + + /* Empty strings are OK */ + if (ret != -ENODATA) + dev_err(&gdev->dev, "unable to name line %d\n", + i); + } + + return 0; +} + +/** * of_gpiochip_scan_gpios - Scan gpio-controller for gpio definitions * @chip: gpio chip to act on * @@ -445,6 +481,13 @@ int of_gpiochip_add(struct gpio_chip *chip) if (status) return status; + /* If the chip defines names itself, these take precedence */ + if (!chip->names) { + status = of_gpiochip_set_names(chip); + if (status) + return status; + } + of_node_get(chip->of_node); return of_gpiochip_scan_gpios(chip);
Make it possible to name the producer side of a GPIO line using a "gpio-names" property array, modeled on the "clock-names" property from the clock bindings. This naming is especially useful for: - Debugging: lines are named after function, not just opaque offset numbers. - Exploration: systems where some or all GPIO lines are available to end users, such as prototyping, one-off's "makerspace usecases" users are helped by the names of the GPIO lines when tinkering. This usecase has been surfacing recently. The gpio-names attribute is completely optional. Cc: Rob Herring <robh+dt@kernel.org> Cc: Grant Likely <grant.likely@linaro.org> Cc: devicetree@vger.kernel.org Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- This has been discussed at some length now. Why we are not using hogs: these are consumer side, not producer side. The gpio-controller in DT (gpio_chip in Linux) is a producer, not a consumer. This patch is not about assigning initial values to GPIO lines. That is an orthogonal usecase. This is just about naming lines. --- Documentation/devicetree/bindings/gpio/gpio.txt | 19 +++++++++++ drivers/gpio/gpiolib-of.c | 43 +++++++++++++++++++++++++ 2 files changed, 62 insertions(+)