On Fri, 16 Nov 2012 11:32:46 -0500, Murali Karicheri [off-list ref] wrote:

On 11/15/2012 11:20 AM, Grant Likely wrote:

quoted

On Mon, 12 Nov 2012 16:28:22 -0500, Murali Karicheri [off-list ref] wrote:

quoted

This adds OF support to DaVinci SPI controller to configure platform
data through device bindings.

Signed-off-by: Murali Karicheri <redacted>

Hi Murali,

Comments below...

quoted

---
  .../devicetree/bindings/spi/spi-davinci.txt        |   50 ++++++++++++
  drivers/spi/spi-davinci.c                          |   80 +++++++++++++++++++-
  2 files changed, 126 insertions(+), 4 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..0369369
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-davinci.txt

@@ -0,0 +1,50 @@
+Davinci SPI controller device bindings
+
+Required properties:
+- #address-cells: number of cells required to define a chip select
+	address on the SPI bus.

Will this *ever* be something other than '1'?

Will add "should be set to 1" as only once cell is used for this.

quoted

quoted

+- #size-cells: should be zero.
+- compatible:
+	- "ti,davinci-spi"

Please use the actual model of the chip here. Don't try to be generic
with the compatible string. A driver can bind against more than one
compatible value and new devices can claim compatiblity with old by
including the old compatible string in this list.

quoted

+- reg: Offset and length of SPI controller register space
+- ti,davinci-spi-version: SPI version :- "1.0" or "2.0"

Usually this is determined from the compatible value directly (which is
why compatible strings shouldn't be generic). Don't use a separate
property for it.

Ok. Based on the ablve two comments, I think I will remove 
davinci-spi-version property. So driver will add two compatibility 
strings "ti,davinci-spi1" and "ti,davinci-spi2" amd match data for 
ti,davinci-spi2 will have the version set to 2 so that driver can use it 
to behave differently. This way DTS file for a board can set the 
compatibility string to use different version of the IP for the driver. 
Do you think I got you right?

Mostly, but what would be better is something like:

"ti,davinci(model)-spi" and "ti,keystone(model)-spi" replacing (model)
with the actual chip part number.

It is always better to use real part names than to make up meaninless
'1', '2' numbers. Newer devices can include the older compatible part
in the compatible property. For example:

	compatible = "ti,keystone-gen2-spi", "ti,keystone-original-spi";

(I'm making up part names here, but you get the idea). So a driver that
binds against "ti,keystone-original-spi" will work with a newer
keystone gen2.

quoted

quoted

+- ti,davinci-spi-num-cs: Number of chip selects
+- ti,davinci-spi-intr-line: interrupt line used to connect the SPI
+	IP to the ARM interrupt controller withn the SoC. Possible values
+	are 0 and 1.

? Isn't that what the interrupts property is for? I don't understand why
this is needed from the description.

Based on the IP manual, there are two interrupt lines coming from the IP 
and only one of them is tied to the interrupt controller in a specific 
SoC. There is a register to program which interrupt line is used based 
on the SoC configuration. So this is different from interrupts.

Okay.

g.