Re: [linux-keystone] [PATCH v1 1/1] spi: davinci: add OF support for the spi controller

[Grant Likely <grant.likely@secretlab.ca>]

On Mon, 3 Dec 2012 13:38:38 -0500, Murali Karicheri <m-karicheri2@ti.com> wrote:
> This adds OF support to DaVinci SPI controller to configure platform
> data through device bindings. Also replaces clk_enable() with
> of clk_prepare_enable() as well as clk_disable() with
> clk_disable_unprepare().
> 

Hi Murali.

Thanks for the patch. Comments below...

> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Reviewed-by : Grant Likely <grant.likely@secretlab.ca>
> ---
>  .../devicetree/bindings/spi/spi-davinci.txt        |   51 ++++++++++
>  drivers/spi/spi-davinci.c                          |  102 +++++++++++++++++---
>  2 files changed, 139 insertions(+), 14 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-davinci.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt
> new file mode 100644
> index 0000000..a2746c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-davinci.txt
> @@ -0,0 +1,51 @@
> +Davinci SPI controller device bindings
> +
> +Required properties:
> +- #address-cells: number of cells required to define a chip select
> +	address on the SPI bus. Should be set to 1.
> +- #size-cells: should be zero.
> +- compatible:
> +	- "ti,davinci-spi-v1" for SPI version 1.0
> +	- "ti,davinci-spi-v2" for SPI version 2.0

It's better to use a specific chip name than some anonymous "v1" or "v2"
(unless you can point me at some hardware documentation that actually
uses those names).

> +- reg: Offset and length of SPI controller register space
> +- ti,davinci-spi-num-cs: Number of chip selects

Simply "num-cs" is sufficient here. There is now a common binding for
the number of chip select lines.

Otherwise the patch and the binding look fine to me.

g.