Re: [PATCH v6 1/2] iio:as3935: Add DT binding docs for AS3935 driver

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

On Mon, Feb 10, 2014 at 04:27:30AM +0000, Matt Ranostay wrote:
> Document compatible string, required and optional DT properties for
> AS3935 chipset driver.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
>  .../devicetree/bindings/iio/proximity/as3935.txt   | 27 ++++++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt        |  1 +
>  2 files changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/as3935.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/as3935.txt b/Documentation/devicetree/bindings/iio/proximity/as3935.txt
> new file mode 100644
> index 0000000..0453254
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/as3935.txt
> @@ -0,0 +1,27 @@
> +Austrian Microsystems AS3935 Franklin lightning sensor device driver
> +
> +Required properties:
> +	- compatible: must be "ams,as3935"
> +	- reg: SPI chip select number for the device
> +	- spi-cpha: SPI Mode 1. Refer to spi/spi-bus.txt for generic SPI
> +	slave node bindings.
> +	- interrupt-parent : should be the phandle for the interrupt controller
> +	- interrupts : interrupt mapping

Nit: Please don't use the term "interrupt mapping", as it's undefined
and clashes with the "interrupt-map" property on an interrupt nexus.
There are a lot of bindings using it, and it would be nice to get them
fixed up too.

As interrupts can be described in a couple of ways it probably doesn't
make sense to state the exact format here, but could this be something
like the following instead:

- interrupts: the sole interrupt generated by the device.

> +
> +	Refer to interrupt-controller/interrupts.txt for generic
> +	interrupt client node bindings.
> +
> +Optional properties:
> +	- ams,tune-cap: Calibration tuning capacitor stepping value 0 - 120pF.
> +	  This will require using the calibration data from the manufacturer.

I think "tune-cap" is a little terse. Also "cap" is used as an
abbreviation for "capability" elsewhere, and it would be nice to make
it clear that in this case it means "capacitor".

Could this be "ams,tuning-capacitor-pf" instead? That would make it
clear at a glance what the value is and the units (though people would
still ahve to refer to documentation to figure out precisely what this
means).

> +
> +Example:
> +
> +as3935@0 {
> +	compatible = "ams,as3935";
> +	reg = <0>;
> +	spi-cpha;
> +	interrupt-parent = <&gpio1>;
> +	interrupts = <16 1>;
> +	ams,tune-cap = <80>;
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index e9d19e2..03e50ff 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -11,6 +11,7 @@ ak	Asahi Kasei Corp.
>  allwinner	Allwinner Technology Co., Ltd.
>  altr	Altera Corp.
>  amcc	Applied Micro Circuits Corporation (APM, formally AMCC)
> +ams	AMS AG

The vendor-prefix addition looks fine to me.	

Thanks,
Mark.