Re: [PATCH v5 1/5] Documentation: tps65912: Add DT bindings for the TPS65912 PMIC

[Lee Jones <lee.jones@linaro.org>]

On Thu, 29 Oct 2015, Andrew F. Davis wrote:

> The TPS65912 PMIC contains several regulators and a GPIO controller.
> Add bindings for the TPS65912 PMIC.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  .../devicetree/bindings/gpio/gpio-tps65912.txt     | 16 +++++++

Why have you dropped Linus' Review-by?

>  Documentation/devicetree/bindings/mfd/tps65912.txt | 51 ++++++++++++++++++++++
>  .../bindings/regulator/tps65912-regulator.txt      | 28 ++++++++++++
>  3 files changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65912.txt
>  create mode 100644 Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt b/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
> new file mode 100644
> index 0000000..0c5c05c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
> @@ -0,0 +1,16 @@
> +* TPS65912 GPIO Controller bindings
> +
> +Required properties:
> + - compatible : Should be "ti,tps65912-gpio".
> + - gpio-controller : Marks the device node as a GPIO Controller.
> + - #gpio-cells : Should be two.  The first cell is the pin number and

You should add a (See: ../gpio/gpio.txt) here I think.

> +     the second cell is used to specify flags.
> +     See include/dt-bindings/gpio/gpio.h for possible values.

This is a Linuxisum and shouldn't really live in here.

I think it would be better to document them in ../gpio/gpio.txt and
reference that instead.

> +Example:
> +
> +	gpio4: tps65912_gpio {

Device names don't belong in node names.

Just "gpio" will do.

> +		compatible = "ti,tps65912-gpio";
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +	};
> diff --git a/Documentation/devicetree/bindings/mfd/tps65912.txt b/Documentation/devicetree/bindings/mfd/tps65912.txt
> new file mode 100644
> index 0000000..da5804a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/tps65912.txt
> @@ -0,0 +1,51 @@
> +* TPS65912 Power Management Integrated Circuit bindings
> +
> +Required properties:
> + - compatible : Should be "ti,tps65912".
> + - reg : Slave address or chip select number (I2C / SPI).
> + - interrupt-parent : The parent interrupt controller.
> + - interrupts : The interrupt line the device is connected to.
> + - interrupt-controller : Marks the device node as an interrupt controller.
> + - #interrupt-cells: The number of cells to describe an IRQ, this should be 2.
> +     The first cell is the IRQ number.
> +     The second cell is the flags, encoded as the trigger masks from
> +     ../interrupt-controller/interrupts.txt

Nit: We *normally* treat these as bullet-points and not place
full-stops on them:

$ git grep "compatible" -- Documentation/devicetree/bindings/ | grep -v "\.$" | wc -l
5227
$ git grep "compatible.*\.$" -- Documentation/devicetree/bindings/ | wc -l
486

... and I *personally* think it's easier to read the document if it's
formatted as follows:

- compatible			: Should be "ti,tps65912".
- reg				: Slave address or chip select number (I2C / SPI).
- interrupt-parent		: The parent interrupt controller.
- interrupts			: The interrupt line the device is connected to.
- interrupt-controller		: Marks the device node as an interrupt controller.
- #interrupt-cells		: The number of cells to describe an IRQ, this should be 2.
					The first cell is the IRQ number.
					The second cell is the flags, encoded as the trigger masks from
					../interrupt-controller/interrupts.txt

... I hope you agree.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog