Re: [RFC v5 4/4] gpio: dt-bindings: add gpio-mmio bindings

[Mark Rutland <mark.rutland@arm.com>]

Hi,

As a general thing, please put the binding earlier in a series than code
implemeting it, as that that makes it easier to review the series
in-order (with context from the binding making code review easier). See
Documentation/devicetree/bindings/submitting-patches.txt

On Fri, Apr 29, 2016 at 02:53:17AM +0200, Christian Lamparter wrote:
> From: Álvaro Fernández Rojas <noltari@gmail.com>
> 
> This patch adds the device tree bindings for the gpio-mmio.
> The gpio-mmio is already part of a the GPIO generic library.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
>  .../devicetree/bindings/gpio/gpio-mmio.txt         | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mmio.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mmio.txt b/Documentation/devicetree/bindings/gpio/gpio-mmio.txt
> new file mode 100644
> index 0000000..cc7f0b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mmio.txt
> @@ -0,0 +1,73 @@
> +Bindings for the generic driver for memory-mapped GPIO controllers.
> +

Bindings should be for hardware (either specific device models, or for
classes), and not for Linux drivers. The latter is subject to arbitrary
changes while the former is not, as old hardware continues to exist and
does not change while drivers get completely reworked.

So please frame this binding document in terms of the class of hardware
you are trying to address. Please provide some introduction to the
assumptions that you are making about said hardware, such that it's
obvious when one needs a more specific binding.

I share the same fears that Rob mentioned in that while this may appear
simple now, the limitations are unclear, and this effectively prevents
us from accurately/specifically describing some hardware.

Regardless, I've taken the time to review the below, and I have a number
of comments.

> +Required properties:
> +	- compatible: should be "linux,gpio-mmio"

Even with the above comments, this binding name would be valid.

> +	- reg-names: must contain
> +		"dat" - data register
> +		may contain
> +		"set" - data set register
> +		"clr" - data clear register
> +		"dirout" - direction output register
> +		"dirin" - direction input register

Below, we mention endianness. Are registers an arbitrary number of bytes
long? In what unit size are they grouped (e.g. {8,16,32,64}-bit)?

What are the expectations for the set/clear/dirin/dirout registers? Are
they write-one to modify, or write-zero to modify?

For the names, don't bother abbreviating (i.e. use "data", "set",
"clear").

These names might clash with the real names a specific HW model applies
to its register sets, which means that this binding is exclusive w.r.t.
a specific binding for a device (i.e. it cannot be extended with
device-specific information).

If we're happy with that, then we must call out the expected limitations
on the use of this binding, or we end up with the not-so-simple-any-more
issues Rob mentioned previously. 

> +	- reg: address + size pairs describing the GPIO register sets;
> +		order must correspond with the order of entries in reg-names
> +	- #gpio-cells = must be set to 2

Where the cells encode what? I'm guessing this is <$idx $flags>, but you
should spell that out explicitly.

> +	- gpio-controller: Marks the device node as a gpio controller.
> +
> +Optional properties:
> +	- ngpio: specifies the number of gpio mapped in the register.
> +	- big-endian: force big endian register accesses.
> +	- big-endian-byte-order: assign GPIOs in reverse order.

I cannot parse this last description. It needs a better wording.

I guess this means that the indices (in the first GPIO cell) are applied
in descending order for ascending chunks of the registers (and that is a
poor description, too).

> +	- unreadable-reg-set: data set register is not readable.
> +	- read-output-reg-set: cache value set for reads.
> +	- unreadable-reg-dir: dirout/dirin register is not readable.
> +	- no-output: GPIOs are read-only.
> +
> +The GPIO generic library provides support for memory-mapped GPIO
> +controllers. The configuration is detected by which resources are present.
> +The simplest form of a GPIO controller that the driver support is just a
> +single "dat" register, where GPIO state can be read and/or written.
> +However, the driver supports far more:
> +	- 8/16/32/64 bits registers. The number of GPIOs is automatically
> +	  determined by the width of the registers.
> +	- GPIO controllers with clear/set registers.
> +	- GPIO controllers with a single "dat" register.
> +	- Big endian bits/GPIOs ordering.

Reword this in terms of the class of hardware you are trying to support,
(rather than the specific library code you are using), and move it to
the introduction at the top of the binding.

Thanks,
Mark.

> +
> +For setting GPIO's there are three configurations:
> +	1. single input/output register resource (named "dat"),
> +	2. set/clear pair (named "set" and "clr"),
> +	3. single output register resource and single input resource
> +	   ("set" and dat").
> +
> +For setting the GPIO direction, there are three configurations:
> +	a. simple bidirectional GPIOs that requires no configuration.
> +	b. an output direction register (named "dirout")
> +	   where a 1 bit indicates the GPIO is an output.
> +	c. an input direction register (named "dirin")
> +	   where a 1 bit indicates the GPIO is an input.
> +
> +Examples:
> +
> +	/* Configuration for single input/output register
> +	 * for eight simple bidirectional GPIOs.
> +	 */
> +	gpio_a_1 {
> +		compatible = "linux,gpio-mmio";
> +		reg = <0x18000000 0x1>;
> +		reg-names = "dat";
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +	};
> +
> +	/* Configuration for set/clear pair registers with
> +	 * 32 output direction register GPIOs.
> +	 */
> +	gpio_b_2 {
> +		compatible = "linux,gpio-mmio";
> +		reg = <0x18000000 0x4>, <0x18000010 0x4>,
> +		      <0x18000004 0x4>, <0x18000008 0x4>;
> +		reg-names = "dat", "set", "clr", "dirout";
> +		#gpio-cells = <2>;
> +		gpio-controller;
> +	};
> -- 
> 2.8.1
>