Re: [PATCH 3/3] devicetree: ad5064: Added devicetree bindings documentation

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

On Wed, Feb 12, 2014 at 12:01:58PM +0000, Paul Cercueil wrote:
> Signed-off-by: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  .../devicetree/bindings/iio/dac/ad5064.txt         |   49 ++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/ad5064.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/ad5064.txt b/Documentation/devicetree/bindings/iio/dac/ad5064.txt
> new file mode 100644
> index 0000000..e5997b0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/ad5064.txt
> @@ -0,0 +1,49 @@
> +Analog Devices AD5064 DAC device driver

It would be nice to have a short description here, with useful facts
(e.g. this is an SPI device).

Is there any public documentation? It might not need to go in the
document, but having a link really helps with review.

> +
> +Required properties:
> +	- compatible: Must be one of:
> +		* "adi,ad5024"
> +		* "adi,ad5025"
> +		* "adi,ad5044"
> +		* "adi,ad5045"
> +		* "adi,ad5064"
> +		* "adi,ad5064-1"
> +		* "adi,ad5065"
> +		* "adi,ad5628-1"
> +		* "adi,ad5628-2"
> +		* "adi,ad5648-1"
> +		* "adi,ad5648-2"
> +		* "adi,ad5666-1"
> +		* "adi,ad5666-2"
> +		* "adi,ad5668-1"
> +		* "adi,ad5668-2"
> +		* "adi,ad5668-3"

How do these differ? Are these just different revisions of the same
chip?

Are particular strings expected to be used as fallbacks in the
compatible list?

> +	- reg: SPI chip select number for the device
> +	- spi-max-frequency: Max SPI frequency to use (< 30000000)
> +	- vrefA-supply, vrefB-supply: phandles to external reference voltage
> +	  supplies for channels 0 and 1 respectively.
> +	  This property must be present for ad5024, ad5025, ad5044, ad5045,
> +	  ad5064, ad5065.

Does ad5064 also imply ad5604-1 here?

> +	- vrefC-supply, vrefD-supply: phandles to external reference voltage
> +	  supplies for channels 2 and 3 respectively.
> +	  This property must be present for ad5024, ad5044, ad5064.

Likewise.

These seem oddly named given the description. Are these named A B C D in
the documentation? If not, vref-channel-X-supply would seem to be
clearer.

Why are these grouped in pairs rather than listed individually?

> +
> +Optional properties:
> +	- adi,use-external-reference: If set, the external reference voltage
> +	  supply is used. This should only be set if there is an external
> +	  reference voltage connected to the vref or vref[A-D] pins.
> +	  If the property is not set, the internal reference voltage supply
> +	  is used if present, or an error is issued by the driver.

When would you want to do this, and when would you want to use the
internal reference? Could the driver not choose to do this based on
whether a supply is listed in the dt?

Strip the part about the error, that's not a description of the device
and not a matter for the binding.

> +	- vref-supply: phandle to the external reference voltage supply.
> +	  This property can be used with ad5064-1, ad5628-1, ad5628-2, ad5648-1,
> +	  ad5648-2, ad5666-1, ad5666-2, ad5668-1, ad5668-2, ad5668-3.

Just to check, on some versions (e.g. ad5668-3), this can work without
any supply listed at all, yes?

Cheers,
Mark.