diff mbox

[v2,1/4] spi/davinci: add DT binding documentation

Message ID 1362401955-9616-2-git-send-email-prakash.pm@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Manjunathappa, Prakash March 4, 2013, 12:59 p.m. UTC
From: Murali Karicheri <m-karicheri2@ti.com>

Get back missed out binding documentation submitted along
with below patch:
"spi/davinci: add OF support for the spi controller"

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Reviewed-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
---
Resubmitting it as it is missed out while merging.

 .../devicetree/bindings/spi/spi-davinci.txt        |   51 ++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-davinci.txt

Comments

Arnd Bergmann March 4, 2013, 4:29 p.m. UTC | #1
On Monday 04 March 2013 18:29:12 Manjunathappa, Prakash wrote:
> diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt
> new file mode 100644
> index 0000000..a62d7a8
> --- /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,dm644x-spi" for SPI used similar to that on DM644x SoC family
> +	- "ti,da8xx-spi" for SPI used similar to that on DA8xx SoC family

In general, you should avoid wildcards in "compatible" properties.
Better use the number of the first chip that introduced the specific
version of the device.

> +- reg: Offset and length of SPI controller register space
> +- num-cs: Number of chip selects
> +- ti,davinci-spi-intr-line: interrupt line used to connect the SPI
> +	IP to the interrupt controller withn the SoC. Possible values
> +	are 0 and 1. Manual says one of the two possible interrupt
> +	lines can be tied to the interrupt controller. Set this
> +	based on a specifc SoC configuration.
> +- interrupts: interrupt number offset at the irq parent

I would not call this an "offset". It is an interrupt descriptor
which may be something other than a simple number.

Unfortunately, there is no way to provide an "invalid" interrupt,
otherwise you could just list both interrupts, out of which at
least one should be valid, and drop the ti,davinci-spi-intr-line
property.

One thing you could do instead though is to use the "interrupt-names"
property to define "irq0" and "irq1" interrupts, and in the
implementation use the first one you find.

	Arnd
Manjunathappa, Prakash March 5, 2013, 12:56 p.m. UTC | #2
Hi Arnd,

On Mon, Mar 04, 2013 at 21:59:16, Arnd Bergmann wrote:
> On Monday 04 March 2013 18:29:12 Manjunathappa, Prakash wrote:
> > diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt
> > new file mode 100644
> > index 0000000..a62d7a8
> > --- /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,dm644x-spi" for SPI used similar to that on DM644x SoC family
> > +	- "ti,da8xx-spi" for SPI used similar to that on DA8xx SoC family
> 
> In general, you should avoid wildcards in "compatible" properties.
> Better use the number of the first chip that introduced the specific
> version of the device.
> 

Correct, will accommodate this change.

> > +- reg: Offset and length of SPI controller register space
> > +- num-cs: Number of chip selects
> > +- ti,davinci-spi-intr-line: interrupt line used to connect the SPI
> > +	IP to the interrupt controller withn the SoC. Possible values
> > +	are 0 and 1. Manual says one of the two possible interrupt
> > +	lines can be tied to the interrupt controller. Set this
> > +	based on a specifc SoC configuration.
> > +- interrupts: interrupt number offset at the irq parent
> 
> I would not call this an "offset". It is an interrupt descriptor
> which may be something other than a simple number.
> 

I am planning to drop from this documentation as it is common property.

> Unfortunately, there is no way to provide an "invalid" interrupt,
> otherwise you could just list both interrupts, out of which at
> least one should be valid, and drop the ti,davinci-spi-intr-line
> property.
> 

This is different from interrupt number, this is used to specifies out
of 2 lines from SPI IP which is tied to INTC. Some discussion about it:
https://lkml.org/lkml/2012/11/16/404
	
> One thing you could do instead though is to use the "interrupt-names"
> property to define "irq0" and "irq1" interrupts, and in the
> implementation use the first one you find.
> 

In IP specification it is mentioned as spi-intr-line, I prefer to retain this
property as davinci-spi-intr-line.

Thanks,
Prakash
Sekhar Nori March 5, 2013, 1:28 p.m. UTC | #3
On 3/5/2013 6:26 PM, Manjunathappa, Prakash wrote:
> Hi Arnd,
> 
> On Mon, Mar 04, 2013 at 21:59:16, Arnd Bergmann wrote:
>> On Monday 04 March 2013 18:29:12 Manjunathappa, Prakash wrote:
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt
>>> new file mode 100644
>>> index 0000000..a62d7a8
>>> --- /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,dm644x-spi" for SPI used similar to that on DM644x SoC family
>>> +	- "ti,da8xx-spi" for SPI used similar to that on DA8xx SoC family
>>
>> In general, you should avoid wildcards in "compatible" properties.
>> Better use the number of the first chip that introduced the specific
>> version of the device.
>>
> 
> Correct, will accommodate this change.

But note that this patch is just documenting bindings already accepted
and part of kernel. You probably need another patch which fixes the
incorrect bindings and this one can then document in fixed bindings.

Thanks,
Sekhar
Manjunathappa, Prakash March 5, 2013, 1:45 p.m. UTC | #4
On Tue, Mar 05, 2013 at 18:58:54, Nori, Sekhar wrote:
> 
> 
> On 3/5/2013 6:26 PM, Manjunathappa, Prakash wrote:
> > Hi Arnd,
> > 
> > On Mon, Mar 04, 2013 at 21:59:16, Arnd Bergmann wrote:
> >> On Monday 04 March 2013 18:29:12 Manjunathappa, Prakash wrote:
> >>> diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt
> >>> new file mode 100644
> >>> index 0000000..a62d7a8
> >>> --- /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,dm644x-spi" for SPI used similar to that on DM644x SoC family
> >>> +	- "ti,da8xx-spi" for SPI used similar to that on DA8xx SoC family
> >>
> >> In general, you should avoid wildcards in "compatible" properties.
> >> Better use the number of the first chip that introduced the specific
> >> version of the device.
> >>
> > 
> > Correct, will accommodate this change.
> 
> But note that this patch is just documenting bindings already accepted
> and part of kernel. You probably need another patch which fixes the
> incorrect bindings and this one can then document in fixed bindings.
> 

Correct Sekhar I meant the same. I will have patch for driver to accommodate
above change and a patch for missing binding document.

Thanks,
Prakash

> Thanks,
> Sekhar
>
Arnd Bergmann March 5, 2013, 7:32 p.m. UTC | #5
On Tuesday 05 March 2013, Manjunathappa, Prakash wrote:
> On Mon, Mar 04, 2013 at 21:59:16, Arnd Bergmann wrote:
> > On Monday 04 March 2013 18:29:12 Manjunathappa, Prakash wrote:
> > > +- reg: Offset and length of SPI controller register space
> > > +- num-cs: Number of chip selects
> > > +- ti,davinci-spi-intr-line: interrupt line used to connect the SPI
> > > +	IP to the interrupt controller withn the SoC. Possible values
> > > +	are 0 and 1. Manual says one of the two possible interrupt
> > > +	lines can be tied to the interrupt controller. Set this
> > > +	based on a specifc SoC configuration.
> > > +- interrupts: interrupt number offset at the irq parent
> > 
> > I would not call this an "offset". It is an interrupt descriptor
> > which may be something other than a simple number.
> > 
> 
> I am planning to drop from this documentation as it is common property.

I think it makese sense to document the fact that there should be exactly
one interrupt listed in the interrupts property, especially since the
hardware has two outputs.

	Arnd
Manjunathappa, Prakash March 11, 2013, 8:53 a.m. UTC | #6
On Wed, Mar 06, 2013 at 01:02:41, Arnd Bergmann wrote:
> On Tuesday 05 March 2013, Manjunathappa, Prakash wrote:
> > On Mon, Mar 04, 2013 at 21:59:16, Arnd Bergmann wrote:
> > > On Monday 04 March 2013 18:29:12 Manjunathappa, Prakash wrote:
> > > > +- reg: Offset and length of SPI controller register space
> > > > +- num-cs: Number of chip selects
> > > > +- ti,davinci-spi-intr-line: interrupt line used to connect the SPI
> > > > +	IP to the interrupt controller withn the SoC. Possible values
> > > > +	are 0 and 1. Manual says one of the two possible interrupt
> > > > +	lines can be tied to the interrupt controller. Set this
> > > > +	based on a specifc SoC configuration.
> > > > +- interrupts: interrupt number offset at the irq parent
> > > 
> > > I would not call this an "offset". It is an interrupt descriptor
> > > which may be something other than a simple number.
> > > 
> > 
> > I am planning to drop from this documentation as it is common property.
> 
> I think it makese sense to document the fact that there should be exactly
> one interrupt listed in the interrupts property, especially since the
> hardware has two outputs.
> 

Agreed, will consider this property for documenting.

Thanks,
Prakash
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt
new file mode 100644
index 0000000..a62d7a8
--- /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,dm644x-spi" for SPI used similar to that on DM644x SoC family
+	- "ti,da8xx-spi" for SPI used similar to that on DA8xx SoC family
+- reg: Offset and length of SPI controller register space
+- num-cs: Number of chip selects
+- ti,davinci-spi-intr-line: interrupt line used to connect the SPI
+	IP to the interrupt controller withn the SoC. Possible values
+	are 0 and 1. Manual says one of the two possible interrupt
+	lines can be tied to the interrupt controller. Set this
+	based on a specifc SoC configuration.
+- interrupts: interrupt number offset at the irq parent
+- clocks: spi clk phandle
+
+Example of a NOR flash slave device (n25q032) connected to DaVinci
+SPI controller device over the SPI bus.
+
+spi0:spi@20BF0000 {
+	#address-cells	 = <1>;
+	#size-cells	 = <0>;
+	compatible	 = "ti,dm644x-spi";
+	reg	 = <0x20BF0000 0x1000>;
+	num-cs	 = <4>;
+	ti,davinci-spi-intr-line	= <0>;
+	interrupts	 = <338>;
+	clocks	 = <&clkspi>;
+
+	flash: n25q032@0 {
+	 #address-cells = <1>;
+	 #size-cells = <1>;
+	 compatible = "st,m25p32";
+	 spi-max-frequency = <25000000>;
+	 reg = <0>;
+
+	 partition@0 {
+	 label = "u-boot-spl";
+	 reg = <0x0 0x80000>;
+	 read-only;
+	 };
+
+	 partition@1 {
+	 label = "test";
+	 reg = <0x80000 0x380000>;
+	 };
+	};
+};