Message-ID: <51BA3BC1.3090109@wwwdotorg.org>
Date:	Thu, 13 Jun 2013 15:38:09 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Christian Ruppert <christian.ruppert@...lis.com>
CC:	Linus Walleij <linus.walleij@...aro.org>,
	Patrice CHOTARD <patrice.chotard@...com>,
	linux-kernel@...r.kernel.org,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>,
	Sascha Leuenberger <sascha.leuenberger@...lis.com>,
	Pierrick Hascoet <pierrick.hascoet@...lis.com>,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	Alexandre Courbot <acourbot@...dia.com>
Subject: Re: [PATCH 2/2] Make non-linear GPIO ranges accesible from gpiolib

On 06/13/2013 06:55 AM, Christian Ruppert wrote:
> This patch adds the infrastructure required to register non-linear gpio
> ranges through gpiolib and the standard GPIO device tree bindings.

That's not exactly true. The existing gpio-ranges property already
allows non-linear ranges to be represented quite easily; each entry in
the gpio-ranges list is <gpio-base> <pinctrl-base> <count>, so you can
piece together any mapping you want.

The potential advantage of this patch is that the pinctrl-side of the
mapping can be a group name rather than pin IDs, which might reduce the
size of the mapping list if you have an extremely sparse or non-linear
mapping /and/ parts of that mapping just happen to align with the pin
groups in the pin controller HW, since each entry in the gpio-ranges
property can be sparse/non-linear, rather than being a small linear
chunk of the mapping.

As an aside, for Tegra I solved this differently: In the pinctrl driver,
I simply defined the pin IDs to exactly match the GPIO IDs in order, so
the mapping is 100% linear. Of course, this only works if your pinctrl
HW documentation doesn't define any kind of numbering/ordering for your
pins, so you can pick any order you want for the pinctrl binding/driver.
This was true for Tegra20, since the HW only used groups for muxing.
Given more recent Tegra SoCs have moved to per-pin muxing, that might
not have been the best decision though, since now the HW registers at
least do have a defined ordering/ID for each pin. If only we'd started
with Tegra30 support not Tegra20:-)

> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt

> +In addition, named groups of pins can be mapped to pin groups of a given
> +pin controller:
> +
> +	gpio_pio_g: gpio-controller@...0 {
> +		#gpio-cells = <2>;
> +		compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
> +		reg = <0x1480 0x18>;
> +		gpio-controller;
> +		gpio-pingrps = <&pinctrl1 0>, <&pinctrl2 3>;
> +		gpio-pingrp-names = "pctl1_gpio_g", "pctl2_gpio_g";

A few thoughts here:

"gpio-pingrps" doesn't sound very similar to the existing "gpio-ranges".
Can we make their equivalent purpose more obvious by renaming this
"gpio-group-ranges"?

I'm not actually even sure we need a new property for this, except for
the string group names. We could fold the new gpio-pingrps into the
existing gpio-ranges, whose entries have the format:

<gpio-base> <pinctrl-base> <count>

... by saying that if count==0, it means to use a group name instead of
a pinctrl-base value. We can insist that pinctrl-base==0 in this case
too. If we go made, count==0 could mean "special" and pinctrl-base==0
could mean "by pinctrl group name", and other values of pinctrl-base
could be added later to mean other things!

gpio-ranges =
	<&pinctrl1 0 20 10>, /* Existing numeric style */
	<&pinctrl2 10 0 0>,  /* Count==0, so group name style */
	<&pinctrl1 0 20 10>, /* Existing numeric style */
gpio-ranges-group-names =
	"",    /* No group name required for entry 0 */
	"gr1", /* Group name for entry 1 */
	"";    /* No group name required for entry 2 */

Does this seem better?

> +		gpio-pingrp-names = "pctl1_gpio_g", "pctl2_gpio_g";

I'm slightly worried that those example group names appear to be
globally scoped. I would hope the group names are interpreted
relative-to or within the pin controller that they are associated with.
I wouldn't expect the pin controllers to include their own name in the
names of the pin groups they expose. In other words, I'd expect that
example to be more like:

> +		gpio-pingrp-names = "foo", "bar";

...
> +The pinctrl node must have a "#gpio-pingrp-cells" property set to one to
> +define the number of arguments to pass with the phandle.

This shouldn't be required.

Such properties are useful when one node references a second node, and
that second node dictates the format of the reference. However, that is
not the case here; the definition of gpio-pingrp-names itself always
dictates its format entirely, and hence the value #gpio-pingrp-cells
must always be 1, and hence there is no point requiring any referenced
node to include this property.

I realize this issue was inherited from the existing gpio-ranges
documentation/implementation, but I've posted patches to solve that,
triggered by thinking about this patch:

https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/035462.html
(the rest of the series will make it more clear, i.e. the new of_ API)

> +Both methods can be combined in the same GPIO controller, e.g.

In the proposal I have above, combining the mechanisms is slightly more
cohesive, I think.

> +	gpio_pio_i: gpio-controller@...0 {
> +		#gpio-cells = <2>;
> +		compatible = "fsl,qe-pario-bank-e", "fsl,qe-pario-bank";
> +		reg = <0x1480 0x18>;
> +		gpio-controller;
> +		gpio-ranges = <&pinctrl1 0 20 10>;
> +		gpio-pingrps = <&pinctrl2 10>;
> +		gpio-pingrp-names = "gpio_g_pins";
> +	};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists