Re: [PATCH v8 1/8] ARM: dt: Binding documentation for imx25 ADC/TSC

Lee Jones
2015-11-23T14:59:52Z

On Mon, 16 Nov 2015, Markus Pargmann wrote:

> This documentation describes the devicetree bindings for the
> ADC/Touchscreen unit of the i.MX25 SoC.
>
> Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>
> Notes:
> Changes in v6:
> - Removed adc-ref property and replaced it with refp and refn for positive and
> negative references. The properties are optional now as the default
> behaviour is a positive internal reference voltage and ADC GND as negative
> reference.
>
> .../devicetree/bindings/mfd/fsl-imx25-tsadc.txt | 46 ++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt b/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
> new file mode 100644
> index 000000000000..a857af0eb68c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/fsl-imx25-tsadc.txt
> @@ -0,0 +1,46 @@
> +Freescale mx25 ADC/TSC multifunction device

Prefer "MX25" and "MultiFunction Device (MFD)"

> +This device combines two general purpose conversion queues one used for general
> +ADC and the other used for touchscreens.
> +
> +Required properties:
> + - compatible: Should be "fsl,imx25-tsadc".
> + - reg: Memory range of the device.

Not exactly. More like start location and size.

> + - interrupts: Interrupt for this device as described in
> + interrupts/interrupts.txt

No such file. In any case, I'd prefer:

"(See: ../interrupt-controller/interrupts.txt)".

> + - clocks: An 'ipg' clock defined as described in clocks/clock.txt

Didn't you check any of what you've written?

"(See: ../clock/clock-bindings.txt)"

> + - interrupt-controller: This device is an interrupt controller. It controls
> + the interrupts of both conversion queues.
> + - #interrupt-cells: Should be '<1>'.
> + - #address-cells: Should be '<1>'.
> + - #size-cells: Should be '<1>'.
> + - ranges

?

Also, don't think think that this looks better/easier to read?

> +Required properties:
> + - compatible: Should be "fsl,imx25-tsadc".
> + - reg: Memory range of the device.
> + - interrupts: Interrupt for this device as described in
> + interrupts/interrupts.txt
> + - clocks: An 'ipg' clock defined as described in clocks/clock.txt
> + - interrupt-controller: This device is an interrupt controller. It controls
> + the interrupts of both conversion queues.
> + - #interrupt-cells: Should be '<1>'.
> + - #address-cells: Should be '<1>'.
> + - #size-cells: Should be '<1>'.

[...]

> +This device includes two conversion queues which can be added as subnodes.
> +The first queue is for the touchscreen, the second for general purpose ADC.
> +
> +Example:
> + tscadc: tscadc@50030000 {
> + compatible = "fsl,imx25-tsadc";
> + reg = <0x50030000 0xc>;
> + interrupts = <46>;
> + clocks = <&clks 119>;
> + clock-names = "ipg";

I don't think this is required, if you only have a single clock.

> + interrupt-controller;
> + #interrupt-cells = <1>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + tsc: tcq@50030400 {
> + compatible = "fsl,imx25-tcq";
> + reg = <0x50030400 0x60>;
> + ...
> + };
> +
> + adc: gcq@50030800 {
> + compatible = "fsl,imx25-gcq";
> + reg = <0x50030800 0x60>;
> + ...
> + };
> + };

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