Re: [PATCH v3] gpio: of: make it possible to name GPIO lines

[Markus Pargmann <mpa@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7207 bytes --]

Hi Linus,

Thanks for creating this. Some minor comments inline.

On Thursday 21 April 2016 13:08:21 Linus Walleij wrote:
> Make it possible to name the producer side of a GPIO line using
> a "gpio-line-names" property array, modeled on the
> "clock-output-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-line-names attribute is completely optional.
> 
> Example output from lsgpio on a patched Snowball tree:
> 
> GPIO chip: gpiochip6, "8000e180.gpio", 32 GPIO lines
>         line  0: unnamed unused
>         line  1: "AP_GPIO161" "extkb3" [kernel]
>         line  2: "AP_GPIO162" "extkb4" [kernel]
>         line  3: "ACCELEROMETER_INT1_RDY" unused [kernel]
>         line  4: "ACCELEROMETER_INT2" unused
>         line  5: "MAG_DRDY" unused [kernel]
>         line  6: "GYRO_DRDY" unused [kernel]
>         line  7: "RSTn_MLC" unused
>         line  8: "RSTn_SLC" unused
>         line  9: "GYRO_INT" unused
>         line 10: "UART_WAKE" unused
>         line 11: "GBF_RESET" unused
>         line 12: unnamed unused
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Amit Kucheria <amit.kucheria@linaro.org>
> Cc: David Mandala <david.mandala@linaro.org>
> Cc: Lee Campbell <leecam@google.com>
> Cc: devicetree@vger.kernel.org
> Reviewed-by: Michael Welling <mwelling@ieee.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v2->v3:
> - Swap "gpio-names" for "gpio-line-names" as "gpio-names" indicate
>   a consumer endpoint in DT terminology.
> - Index to either:
>   (A) The end of the gpio-names array or
>   (B) ngpios
>   So we don't risk going out of bounds on either
> ChangeLog v1->v2:
> - Make the naming function return void: we continue at all times
>   and always return 0 anyway.
> - Fix a return value check.
> 
> 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..68d28f62a6f4 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-line-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:

"GPIO0" may not be a good name, but there are plenty of boards out there
that use just this line name in their schematic for GPIOs that are not
used otherwise. For example the riotboard has line-names "GPIO_1",
"GPIO4_31", "GPIO5_05", ...

> +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-line-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";

As many GPIO controllers have something like 32 GPIO lines, it can be
difficult to read which GPIO number the line "reset" is on.
But I think you can get around that by writing one name per file-line or
something similar. Also it should be immediately visible by listing all
gpio names in the userspace.

>  }
>  
>  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..d43eaca803e3 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -196,6 +196,45 @@ 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 void 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-line-names" property */
> +	if (!of_property_read_bool(np, "gpio-line-names"))
> +		return;
> +
> +	nstrings = of_property_count_strings(np, "gpio-line-names");
> +	/*
> +	 * Make sure to not index beyond either the end of the
> +	 * "gpio-names" array nor the number of descriptors of
> +	 * the GPIO device.
> +	 */
> +	for (i = 0; i < nstrings && i < gdev->ngpio; i++) {
> +		const char *name;
> +		int ret;
> +
> +		ret = of_property_read_string_index(np,
> +						    "gpio-line-names",
> +						    i,
> +						    &name);
> +		if (!ret)
> +			gdev->descs[i].name = name;
> +
> +		/* Empty strings are OK */
> +		if (ret < 0 && ret != -ENODATA)
> +			dev_err(&gdev->dev, "unable to name line %d\n",
> +				i);
> +	}

Maybe a warning for nstrings > ngpio here?

Otherwise this looks good.

Best Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]