Re: [PATCH v2 3/3] Documentation: gpio: Add APM X-Gene SoC GPIO controller DTS binding

[Alexandre Courbot <gnurou@xxxxxxxxx>]

On Fri, May 30, 2014 at 4:23 AM, Feng Kan <fkan@xxxxxxx> wrote:
> Documentation for APM X-Gene SoC GPIO controller DTS binding.

This patch should come before 2/3. You want the binding to be visible
before adding entries following it.

>
> Signed-off-by: Feng Kan <fkan@xxxxxxx>
> ---
>  .../devicetree/bindings/gpio/gpio-xgene.txt        | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> new file mode 100644
> index 0000000..7219575
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> @@ -0,0 +1,37 @@
> +APM X-Gene SoC GPIO controller bindings
> +
> +This is a gpio controller that is part of the flash controller.
> +All registers are 32bit and in little endian format.

Mmm, does your driver take care of endianness issue if this controller
is used on big-endian architectures?

> +
> +There are three flash controller gpio banks, each bank have 16
> +gpio pins.
> +
> +Required properties:
> +- compatible: "apm,xgene-gpio" for X-Gene GPIO controller
> +- reg: Physical base address and size of the controller's registers
> +- #gpio-cells: Should be two.
> +       - first cell is the pin number
> +       - second cell is used to specify optional parameters (unused)
> +- gpio-controller: Marks the device node as a GPIO controller.
> +
> +Example:
> +       gpio0: gpio0@1701c00c {
> +               compatible = "apm,xgene-gpio";
> +               reg = <0x0 0x1701c00c 0x0 0x30>;
> +               gpio-controller;
> +               #gpio-cells = <2>;
> +       };
> +
> +       gpio1: gpio1@1701c018 {
> +               compatible = "apm,xgene-gpio";
> +               reg = <0x0 0x1701c018 0x0 0x30>;
> +               gpio-controller;
> +               #gpio-cells = <2>;
> +       };
> +
> +       gpio2: gpio2@1701c024 {
> +               compatible = "apm,xgene-gpio";
> +               reg = <0x0 0x1701c024 0x0 0x30>;
> +               gpio-controller;
> +               #gpio-cells = <2>;
> +       };

While I like this better than the first version, I wonder if we should
not have this as a single device, with each bank having an entry in
the reg property. Something like:

gpio@1701c00c {
        compatible = "apm,xgene-gpio";
        reg = <0x0 0x1701c00c 0x0 0x30
               0x0 0x1701c018 0x0 0x30
               0x0 0x1701c024 0x0 0x30>;
        gpio-controller;
        #gpio-cells = <2>;
};

>From the reg property your driver can know how many banks there are.
Since the banks do not have properties of their own (like number of
GPIOs which is now fixed), they don't need additional description.
Then you can end up with a single device which represents the reality
better, while still being flexible wrt. the sparse register layout.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html