[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Jean-Francois Moine]

This patch adds the nodes to instantiate the audio devices of the Dove
boards.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 499abad..78227e2 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -573,6 +573,24 @@
 					phy-handle = <&ethphy>;
 				};
 			};
+
+			i2s0: audio-controller@b0000 {
+				compatible = "marvell,mvebu-audio";
+				reg = <0xb0000 0x2210>;
+				interrupts = <19>, <20>;
+				clocks = <&gate_clk 12>;
+				clock-names = "internal";
+				status = "disabled";
+			};
+
+			i2s1: audio-controller@b4000 {
+				compatible = "marvell,mvebu-audio";
+				reg = <0xb4000 0x2210>;
+				interrupts = <21>, <22>;
+				clocks = <&gate_clk 13>;
+				clock-names = "internal";
+				status = "disabled";
+			};
 		};
 	};
 };


-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Jean-Francois Moine]

This patch adds the nodes to instantiate the audio devices of the Dove
boards.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 499abad..78227e2 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -573,6 +573,24 @@
 					phy-handle = <&ethphy>;
 				};
 			};
+
+			i2s0: audio-controller at b0000 {
+				compatible = "marvell,mvebu-audio";
+				reg = <0xb0000 0x2210>;
+				interrupts = <19>, <20>;
+				clocks = <&gate_clk 12>;
+				clock-names = "internal";
+				status = "disabled";
+			};
+
+			i2s1: audio-controller at b4000 {
+				compatible = "marvell,mvebu-audio";
+				reg = <0xb4000 0x2210>;
+				interrupts = <21>, <22>;
+				clocks = <&gate_clk 13>;
+				clock-names = "internal";
+				status = "disabled";
+			};
 		};
 	};
 };


-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/2013 11:34 AM, Jean-Francois Moine wrote:
> This patch adds the nodes to instantiate the audio devices of the Dove
> boards.
>
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>   arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> index 499abad..78227e2 100644
> --- a/arch/arm/boot/dts/dove.dtsi
> +++ b/arch/arm/boot/dts/dove.dtsi
> @@ -573,6 +573,24 @@
>   					phy-handle = <&ethphy>;
>   				};
>   			};
> +
> +			i2s0: audio-controller@b0000 {
> +				compatible = "marvell,mvebu-audio";

[added Gregory to Cc]

Jean-Francois,

as Mark Brown already took the bindings patch for above generic
compatible, how are we going to discriminate different
implementations/features of Dove, Kirkwood, and Armada 370?

Sebastian

> +				reg = <0xb0000 0x2210>;
> +				interrupts = <19>, <20>;
> +				clocks = <&gate_clk 12>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};
> +
> +			i2s1: audio-controller@b4000 {
> +				compatible = "marvell,mvebu-audio";
> +				reg = <0xb4000 0x2210>;
> +				interrupts = <21>, <22>;
> +				clocks = <&gate_clk 13>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};
>   		};
>   	};
>   };
>
>

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/2013 11:34 AM, Jean-Francois Moine wrote:
> This patch adds the nodes to instantiate the audio devices of the Dove
> boards.
>
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>   arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> index 499abad..78227e2 100644
> --- a/arch/arm/boot/dts/dove.dtsi
> +++ b/arch/arm/boot/dts/dove.dtsi
> @@ -573,6 +573,24 @@
>   					phy-handle = <&ethphy>;
>   				};
>   			};
> +
> +			i2s0: audio-controller at b0000 {
> +				compatible = "marvell,mvebu-audio";

[added Gregory to Cc]

Jean-Francois,

as Mark Brown already took the bindings patch for above generic
compatible, how are we going to discriminate different
implementations/features of Dove, Kirkwood, and Armada 370?

Sebastian

> +				reg = <0xb0000 0x2210>;
> +				interrupts = <19>, <20>;
> +				clocks = <&gate_clk 12>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};
> +
> +			i2s1: audio-controller at b4000 {
> +				compatible = "marvell,mvebu-audio";
> +				reg = <0xb4000 0x2210>;
> +				interrupts = <21>, <22>;
> +				clocks = <&gate_clk 13>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};
>   		};
>   	};
>   };
>
>

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/2013 11:34 AM, Jean-Francois Moine wrote:
> This patch adds the nodes to instantiate the audio devices of the Dove
> boards.
>
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>   arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> index 499abad..78227e2 100644
> --- a/arch/arm/boot/dts/dove.dtsi
> +++ b/arch/arm/boot/dts/dove.dtsi
> @@ -573,6 +573,24 @@
>   					phy-handle = <&ethphy>;
>   				};
>   			};
> +
> +			i2s0: audio-controller@b0000 {
> +				compatible = "marvell,mvebu-audio";

[added Gregory to Cc]

Jean-Francois,

as Mark Brown already took the bindings patch for above generic
compatible, how are we going to discriminate different
implementations/features of Dove, Kirkwood, and Armada 370?

Sebastian

> +				reg = <0xb0000 0x2210>;
> +				interrupts = <19>, <20>;
> +				clocks = <&gate_clk 12>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};
> +
> +			i2s1: audio-controller@b4000 {
> +				compatible = "marvell,mvebu-audio";
> +				reg = <0xb4000 0x2210>;
> +				interrupts = <21>, <22>;
> +				clocks = <&gate_clk 13>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};
>   		};
>   	};
>   };
>
>

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Sebastian, Jean-François,

On Wed, 28 Aug 2013 12:13:07 +0200, Sebastian Hesselbarth wrote:
> On 08/28/2013 11:34 AM, Jean-Francois Moine wrote:
> > This patch adds the nodes to instantiate the audio devices of the Dove
> > boards.
> >
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > ---
> >   arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> > index 499abad..78227e2 100644
> > --- a/arch/arm/boot/dts/dove.dtsi
> > +++ b/arch/arm/boot/dts/dove.dtsi
> > @@ -573,6 +573,24 @@
> >   					phy-handle = <&ethphy>;
> >   				};
> >   			};
> > +
> > +			i2s0: audio-controller@b0000 {
> > +				compatible = "marvell,mvebu-audio";
> 
> [added Gregory to Cc]
> 
> Jean-Francois,
> 
> as Mark Brown already took the bindings patch for above generic
> compatible, how are we going to discriminate different
> implementations/features of Dove, Kirkwood, and Armada 370?

I agree that mvebu-audio is not a really good compatible string. It
should use the first SoC that introduced the IP block, so that if
future SOCs have variations, we can introduce separate compatible
strings.

So for now, the compatible string should be kirkwood-audio.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Sebastian, Jean-Fran?ois,

On Wed, 28 Aug 2013 12:13:07 +0200, Sebastian Hesselbarth wrote:
> On 08/28/2013 11:34 AM, Jean-Francois Moine wrote:
> > This patch adds the nodes to instantiate the audio devices of the Dove
> > boards.
> >
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > ---
> >   arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> > index 499abad..78227e2 100644
> > --- a/arch/arm/boot/dts/dove.dtsi
> > +++ b/arch/arm/boot/dts/dove.dtsi
> > @@ -573,6 +573,24 @@
> >   					phy-handle = <&ethphy>;
> >   				};
> >   			};
> > +
> > +			i2s0: audio-controller at b0000 {
> > +				compatible = "marvell,mvebu-audio";
> 
> [added Gregory to Cc]
> 
> Jean-Francois,
> 
> as Mark Brown already took the bindings patch for above generic
> compatible, how are we going to discriminate different
> implementations/features of Dove, Kirkwood, and Armada 370?

I agree that mvebu-audio is not a really good compatible string. It
should use the first SoC that introduced the IP block, so that if
future SOCs have variations, we can introduce separate compatible
strings.

So for now, the compatible string should be kirkwood-audio.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Sebastian, Jean-François,

On Wed, 28 Aug 2013 12:13:07 +0200, Sebastian Hesselbarth wrote:
> On 08/28/2013 11:34 AM, Jean-Francois Moine wrote:
> > This patch adds the nodes to instantiate the audio devices of the Dove
> > boards.
> >
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > ---
> >   arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> > index 499abad..78227e2 100644
> > --- a/arch/arm/boot/dts/dove.dtsi
> > +++ b/arch/arm/boot/dts/dove.dtsi
> > @@ -573,6 +573,24 @@
> >   					phy-handle = <&ethphy>;
> >   				};
> >   			};
> > +
> > +			i2s0: audio-controller@b0000 {
> > +				compatible = "marvell,mvebu-audio";
> 
> [added Gregory to Cc]
> 
> Jean-Francois,
> 
> as Mark Brown already took the bindings patch for above generic
> compatible, how are we going to discriminate different
> implementations/features of Dove, Kirkwood, and Armada 370?

I agree that mvebu-audio is not a really good compatible string. It
should use the first SoC that introduced the IP block, so that if
future SOCs have variations, we can introduce separate compatible
strings.

So for now, the compatible string should be kirkwood-audio.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/2013 12:19 PM, Thomas Petazzoni wrote:
> Sebastian, Jean-François,
>
> On Wed, 28 Aug 2013 12:13:07 +0200, Sebastian Hesselbarth wrote:
>> On 08/28/2013 11:34 AM, Jean-Francois Moine wrote:
>>> This patch adds the nodes to instantiate the audio devices of the Dove
>>> boards.
>>>
>>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>>> ---
>>>    arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
>>>    1 file changed, 18 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
>>> index 499abad..78227e2 100644
>>> --- a/arch/arm/boot/dts/dove.dtsi
>>> +++ b/arch/arm/boot/dts/dove.dtsi
>>> @@ -573,6 +573,24 @@
>>>    					phy-handle = <&ethphy>;
>>>    				};
>>>    			};
>>> +
>>> +			i2s0: audio-controller@b0000 {
>>> +				compatible = "marvell,mvebu-audio";
>>
>> [added Gregory to Cc]
>>
>> Jean-Francois,
>>
>> as Mark Brown already took the bindings patch for above generic
>> compatible, how are we going to discriminate different
>> implementations/features of Dove, Kirkwood, and Armada 370?
>
> I agree that mvebu-audio is not a really good compatible string. It
> should use the first SoC that introduced the IP block, so that if
> future SOCs have variations, we can introduce separate compatible
> strings.
>
> So for now, the compatible string should be kirkwood-audio.

Unfortunately, mvebu-audio has already been taken by Mark. Also, we
know the differences for the three SoCs now and should have a compatible
for each (and maybe mvebu-audio for fallback).

Also, we'll need to distinguish between the different audio controllers
on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
base passed.

Sebastian

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/2013 12:19 PM, Thomas Petazzoni wrote:
> Sebastian, Jean-Fran?ois,
>
> On Wed, 28 Aug 2013 12:13:07 +0200, Sebastian Hesselbarth wrote:
>> On 08/28/2013 11:34 AM, Jean-Francois Moine wrote:
>>> This patch adds the nodes to instantiate the audio devices of the Dove
>>> boards.
>>>
>>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>>> ---
>>>    arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
>>>    1 file changed, 18 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
>>> index 499abad..78227e2 100644
>>> --- a/arch/arm/boot/dts/dove.dtsi
>>> +++ b/arch/arm/boot/dts/dove.dtsi
>>> @@ -573,6 +573,24 @@
>>>    					phy-handle = <&ethphy>;
>>>    				};
>>>    			};
>>> +
>>> +			i2s0: audio-controller at b0000 {
>>> +				compatible = "marvell,mvebu-audio";
>>
>> [added Gregory to Cc]
>>
>> Jean-Francois,
>>
>> as Mark Brown already took the bindings patch for above generic
>> compatible, how are we going to discriminate different
>> implementations/features of Dove, Kirkwood, and Armada 370?
>
> I agree that mvebu-audio is not a really good compatible string. It
> should use the first SoC that introduced the IP block, so that if
> future SOCs have variations, we can introduce separate compatible
> strings.
>
> So for now, the compatible string should be kirkwood-audio.

Unfortunately, mvebu-audio has already been taken by Mark. Also, we
know the differences for the three SoCs now and should have a compatible
for each (and maybe mvebu-audio for fallback).

Also, we'll need to distinguish between the different audio controllers
on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
base passed.

Sebastian

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/2013 12:19 PM, Thomas Petazzoni wrote:
> Sebastian, Jean-François,
>
> On Wed, 28 Aug 2013 12:13:07 +0200, Sebastian Hesselbarth wrote:
>> On 08/28/2013 11:34 AM, Jean-Francois Moine wrote:
>>> This patch adds the nodes to instantiate the audio devices of the Dove
>>> boards.
>>>
>>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>>> ---
>>>    arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
>>>    1 file changed, 18 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
>>> index 499abad..78227e2 100644
>>> --- a/arch/arm/boot/dts/dove.dtsi
>>> +++ b/arch/arm/boot/dts/dove.dtsi
>>> @@ -573,6 +573,24 @@
>>>    					phy-handle = <&ethphy>;
>>>    				};
>>>    			};
>>> +
>>> +			i2s0: audio-controller@b0000 {
>>> +				compatible = "marvell,mvebu-audio";
>>
>> [added Gregory to Cc]
>>
>> Jean-Francois,
>>
>> as Mark Brown already took the bindings patch for above generic
>> compatible, how are we going to discriminate different
>> implementations/features of Dove, Kirkwood, and Armada 370?
>
> I agree that mvebu-audio is not a really good compatible string. It
> should use the first SoC that introduced the IP block, so that if
> future SOCs have variations, we can introduce separate compatible
> strings.
>
> So for now, the compatible string should be kirkwood-audio.

Unfortunately, mvebu-audio has already been taken by Mark. Also, we
know the differences for the three SoCs now and should have a compatible
for each (and maybe mvebu-audio for fallback).

Also, we'll need to distinguish between the different audio controllers
on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
base passed.

Sebastian

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Dear Sebastian Hesselbarth,

On Wed, 28 Aug 2013 12:26:31 +0200, Sebastian Hesselbarth wrote:

> >> as Mark Brown already took the bindings patch for above generic
> >> compatible, how are we going to discriminate different
> >> implementations/features of Dove, Kirkwood, and Armada 370?
> >
> > I agree that mvebu-audio is not a really good compatible string. It
> > should use the first SoC that introduced the IP block, so that if
> > future SOCs have variations, we can introduce separate compatible
> > strings.
> >
> > So for now, the compatible string should be kirkwood-audio.
> 
> Unfortunately, mvebu-audio has already been taken by Mark. Also, we
> know the differences for the three SoCs now and should have a compatible
> for each (and maybe mvebu-audio for fallback).

For 3.12, right? So 3.12 hasn't been released yet, so it's still time
to fix this.

> Also, we'll need to distinguish between the different audio controllers
> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
> base passed.

For what reason does the driver needs to know whether it's the instance
0 or instance 1 ? If it's needed for some specific reason, then there
should probably be something like marvell,i2s-channel-id = <0> and
marvell,i2s-channel-id = <1>.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Dear Sebastian Hesselbarth,

On Wed, 28 Aug 2013 12:26:31 +0200, Sebastian Hesselbarth wrote:

> >> as Mark Brown already took the bindings patch for above generic
> >> compatible, how are we going to discriminate different
> >> implementations/features of Dove, Kirkwood, and Armada 370?
> >
> > I agree that mvebu-audio is not a really good compatible string. It
> > should use the first SoC that introduced the IP block, so that if
> > future SOCs have variations, we can introduce separate compatible
> > strings.
> >
> > So for now, the compatible string should be kirkwood-audio.
> 
> Unfortunately, mvebu-audio has already been taken by Mark. Also, we
> know the differences for the three SoCs now and should have a compatible
> for each (and maybe mvebu-audio for fallback).

For 3.12, right? So 3.12 hasn't been released yet, so it's still time
to fix this.

> Also, we'll need to distinguish between the different audio controllers
> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
> base passed.

For what reason does the driver needs to know whether it's the instance
0 or instance 1 ? If it's needed for some specific reason, then there
should probably be something like marvell,i2s-channel-id = <0> and
marvell,i2s-channel-id = <1>.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/13 13:15, Thomas Petazzoni wrote:
> On Wed, 28 Aug 2013 12:26:31 +0200, Sebastian Hesselbarth wrote:
>>>> as Mark Brown already took the bindings patch for above generic
>>>> compatible, how are we going to discriminate different
>>>> implementations/features of Dove, Kirkwood, and Armada 370?
>>>
>>> I agree that mvebu-audio is not a really good compatible string. It
>>> should use the first SoC that introduced the IP block, so that if
>>> future SOCs have variations, we can introduce separate compatible
>>> strings.
>>>
>>> So for now, the compatible string should be kirkwood-audio.
>>
>> Unfortunately, mvebu-audio has already been taken by Mark. Also, we
>> know the differences for the three SoCs now and should have a compatible
>> for each (and maybe mvebu-audio for fallback).
>
> For 3.12, right? So 3.12 hasn't been released yet, so it's still time
> to fix this.

I guess, yes.

>> Also, we'll need to distinguish between the different audio controllers
>> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
>> base passed.
>
> For what reason does the driver needs to know whether it's the instance
> 0 or instance 1 ? If it's needed for some specific reason, then there
> should probably be something like marvell,i2s-channel-id = <0> and
> marvell,i2s-channel-id = <1>.

On Dove, audio1 has SPDIF out, audio0 hasn't. Russell also mentioned to
get rid of "i2s" and use "audio" instead. Most SoC's controllers are
i2s only but as soon as SPDIF comes into play, it is a different
interface protocol.

I am fine with having a "marvell,channel-id" (no "i2s") to discriminate
the instances, although reg offset should be sufficient.

Jean-Francois: Can you please also rename the DT nodes to "audio0" and
"audio1" instead?

Sebastian

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/13 13:15, Thomas Petazzoni wrote:
> On Wed, 28 Aug 2013 12:26:31 +0200, Sebastian Hesselbarth wrote:
>>>> as Mark Brown already took the bindings patch for above generic
>>>> compatible, how are we going to discriminate different
>>>> implementations/features of Dove, Kirkwood, and Armada 370?
>>>
>>> I agree that mvebu-audio is not a really good compatible string. It
>>> should use the first SoC that introduced the IP block, so that if
>>> future SOCs have variations, we can introduce separate compatible
>>> strings.
>>>
>>> So for now, the compatible string should be kirkwood-audio.
>>
>> Unfortunately, mvebu-audio has already been taken by Mark. Also, we
>> know the differences for the three SoCs now and should have a compatible
>> for each (and maybe mvebu-audio for fallback).
>
> For 3.12, right? So 3.12 hasn't been released yet, so it's still time
> to fix this.

I guess, yes.

>> Also, we'll need to distinguish between the different audio controllers
>> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
>> base passed.
>
> For what reason does the driver needs to know whether it's the instance
> 0 or instance 1 ? If it's needed for some specific reason, then there
> should probably be something like marvell,i2s-channel-id = <0> and
> marvell,i2s-channel-id = <1>.

On Dove, audio1 has SPDIF out, audio0 hasn't. Russell also mentioned to
get rid of "i2s" and use "audio" instead. Most SoC's controllers are
i2s only but as soon as SPDIF comes into play, it is a different
interface protocol.

I am fine with having a "marvell,channel-id" (no "i2s") to discriminate
the instances, although reg offset should be sufficient.

Jean-Francois: Can you please also rename the DT nodes to "audio0" and
"audio1" instead?

Sebastian

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Dear Sebastian Hesselbarth,

On Wed, 28 Aug 2013 13:44:51 +0200, Sebastian Hesselbarth wrote:

> > For 3.12, right? So 3.12 hasn't been released yet, so it's still time
> > to fix this.
> 
> I guess, yes.

Jean-François, could you cook and submit a patch to change the
compatible string?

> >> Also, we'll need to distinguish between the different audio controllers
> >> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
> >> base passed.
> >
> > For what reason does the driver needs to know whether it's the instance
> > 0 or instance 1 ? If it's needed for some specific reason, then there
> > should probably be something like marvell,i2s-channel-id = <0> and
> > marvell,i2s-channel-id = <1>.
> 
> On Dove, audio1 has SPDIF out, audio0 hasn't. Russell also mentioned to
> get rid of "i2s" and use "audio" instead. Most SoC's controllers are
> i2s only but as soon as SPDIF comes into play, it is a different
> interface protocol.
> 
> I am fine with having a "marvell,channel-id" (no "i2s") to discriminate
> the instances, although reg offset should be sufficient.

Well, the reg offset is a possibility, but it's not really nice, and
would have to be adapted to each and every SoC even if the reset of the
audio IP is the same.

Though, if the difference between the two units is the availability of
SPDIF support, then we shouldn't encode the channel number, but instead
the availability of SPDIF, i.e:

	audio0 {
		reg = <... ...>;
		compatible = "marvell,kirkwood-audio";
		marvell,has-spdif;
	};

	audio1 {
		reg = <... ...>;
		compatible = "marvell,kirkwood-audio";
	};

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Dear Sebastian Hesselbarth,

On Wed, 28 Aug 2013 13:44:51 +0200, Sebastian Hesselbarth wrote:

> > For 3.12, right? So 3.12 hasn't been released yet, so it's still time
> > to fix this.
> 
> I guess, yes.

Jean-Fran?ois, could you cook and submit a patch to change the
compatible string?

> >> Also, we'll need to distinguish between the different audio controllers
> >> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
> >> base passed.
> >
> > For what reason does the driver needs to know whether it's the instance
> > 0 or instance 1 ? If it's needed for some specific reason, then there
> > should probably be something like marvell,i2s-channel-id = <0> and
> > marvell,i2s-channel-id = <1>.
> 
> On Dove, audio1 has SPDIF out, audio0 hasn't. Russell also mentioned to
> get rid of "i2s" and use "audio" instead. Most SoC's controllers are
> i2s only but as soon as SPDIF comes into play, it is a different
> interface protocol.
> 
> I am fine with having a "marvell,channel-id" (no "i2s") to discriminate
> the instances, although reg offset should be sufficient.

Well, the reg offset is a possibility, but it's not really nice, and
would have to be adapted to each and every SoC even if the reset of the
audio IP is the same.

Though, if the difference between the two units is the availability of
SPDIF support, then we shouldn't encode the channel number, but instead
the availability of SPDIF, i.e:

	audio0 {
		reg = <... ...>;
		compatible = "marvell,kirkwood-audio";
		marvell,has-spdif;
	};

	audio1 {
		reg = <... ...>;
		compatible = "marvell,kirkwood-audio";
	};

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Wed, Aug 28, 2013 at 01:58:27PM +0200, Thomas Petazzoni wrote:
> Dear Sebastian Hesselbarth,
> 
> On Wed, 28 Aug 2013 13:44:51 +0200, Sebastian Hesselbarth wrote:
> 
> > > For 3.12, right? So 3.12 hasn't been released yet, so it's still time
> > > to fix this.
> > 
> > I guess, yes.
> 
> Jean-François, could you cook and submit a patch to change the
> compatible string?

I don't think this is a good idea.  The configuration of this IP is
not based on the SoC as a single SoC can have a mixture of different
configurations.

I think marvell,mvebu-audio is a reasonable compatible string for this,
and that the different configurations should be described by properties
indicating which inputs and outputs have been implemented.

For instance, on the Dove, there are two of these blocks.  One has I2S
in and out only, but the other block has I2S in and out, and SPDIF out.
On some other Marvell devices, this block has I2S in and out and SPDIF
in and out.

Otherwise, they're functionally the same.

> Though, if the difference between the two units is the availability of
> SPDIF support, then we shouldn't encode the channel number, but instead
> the availability of SPDIF, i.e:
> 
> 	audio0 {
> 		reg = <... ...>;
> 		compatible = "marvell,kirkwood-audio";
> 		marvell,has-spdif;
> 	};
> 
> 	audio1 {
> 		reg = <... ...>;
> 		compatible = "marvell,kirkwood-audio";
> 	};

... which means there's no problem with using marvell,mvebu-audio as the
compatible string if you're going to use properties to describe what
facilities are available.

In any case "marvell,has-spdif" is too generic - as I've indicated above,
there's versions with spdif out, and other versions with spdif in and
out.

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Wed, Aug 28, 2013 at 01:58:27PM +0200, Thomas Petazzoni wrote:
> Dear Sebastian Hesselbarth,
> 
> On Wed, 28 Aug 2013 13:44:51 +0200, Sebastian Hesselbarth wrote:
> 
> > > For 3.12, right? So 3.12 hasn't been released yet, so it's still time
> > > to fix this.
> > 
> > I guess, yes.
> 
> Jean-Fran?ois, could you cook and submit a patch to change the
> compatible string?

I don't think this is a good idea.  The configuration of this IP is
not based on the SoC as a single SoC can have a mixture of different
configurations.

I think marvell,mvebu-audio is a reasonable compatible string for this,
and that the different configurations should be described by properties
indicating which inputs and outputs have been implemented.

For instance, on the Dove, there are two of these blocks.  One has I2S
in and out only, but the other block has I2S in and out, and SPDIF out.
On some other Marvell devices, this block has I2S in and out and SPDIF
in and out.

Otherwise, they're functionally the same.

> Though, if the difference between the two units is the availability of
> SPDIF support, then we shouldn't encode the channel number, but instead
> the availability of SPDIF, i.e:
> 
> 	audio0 {
> 		reg = <... ...>;
> 		compatible = "marvell,kirkwood-audio";
> 		marvell,has-spdif;
> 	};
> 
> 	audio1 {
> 		reg = <... ...>;
> 		compatible = "marvell,kirkwood-audio";
> 	};

... which means there's no problem with using marvell,mvebu-audio as the
compatible string if you're going to use properties to describe what
facilities are available.

In any case "marvell,has-spdif" is too generic - as I've indicated above,
there's versions with spdif out, and other versions with spdif in and
out.

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Dear Russell King - ARM Linux,

On Wed, 28 Aug 2013 13:13:20 +0100, Russell King - ARM Linux wrote:

> > > I guess, yes.
> > 
> > Jean-François, could you cook and submit a patch to change the
> > compatible string?
> 
> I don't think this is a good idea.  The configuration of this IP is
> not based on the SoC as a single SoC can have a mixture of different
> configurations.

Using the name of the oldest SoC in the family that had the IP block is
the norm, because it's really what "compatible" means: the IP block in
Dove is *compatible* with the one that was originally introduced in
Kirkwood.

See what Rob Herring (one of the DT maintainer) says in
http://lists.infradead.org/pipermail/linux-mtd/2012-March/040417.html:

"""
There is no reason all machines can't use "st,spear600-smi" in their
dts. It doesn't have to be a spear600, just compatible with it. Really
you want the string to be the oldest SOC the block is in and then newer
SOCs can claim compatibility with the old version.
"""

The thread was precisely about replacing a SoC-specific compatible
string "st,spear600-smi" by a more generic "st,spear-smi" and Rob
Herring (above) was opposing to that.

> I think marvell,mvebu-audio is a reasonable compatible string for this,
> and that the different configurations should be described by properties
> indicating which inputs and outputs have been implemented.
> 
> For instance, on the Dove, there are two of these blocks.  One has I2S
> in and out only, but the other block has I2S in and out, and SPDIF out.
> On some other Marvell devices, this block has I2S in and out and SPDIF
> in and out.
> 
> Otherwise, they're functionally the same.

Right, that's why they can both use "kirkwood-audio" as the compatible
string.

> > Though, if the difference between the two units is the availability of
> > SPDIF support, then we shouldn't encode the channel number, but instead
> > the availability of SPDIF, i.e:
> > 
> > 	audio0 {
> > 		reg = <... ...>;
> > 		compatible = "marvell,kirkwood-audio";
> > 		marvell,has-spdif;
> > 	};
> > 
> > 	audio1 {
> > 		reg = <... ...>;
> > 		compatible = "marvell,kirkwood-audio";
> > 	};
> 
> ... which means there's no problem with using marvell,mvebu-audio as the
> compatible string if you're going to use properties to describe what
> facilities are available.

I disagree, because how do you know if a future "mvebu" SOC such as
Armada 370, or one that doesn't exist yet, will not have a different
audio IP block? It will still be audio, it will still be mvebu, but it
will not be able to use a "marvell,mvebu-audio" driver. Or maybe it can
use the same driver, but with a few variations, so a different
compatible string will be needed to identify the original IP
("marvell,kirkwood-audio", used on Kirkwood/Dove) and slightly newer
versions of the IP ("marvell,some-funky-soc-audio").

> In any case "marvell,has-spdif" is too generic - as I've indicated above,
> there's versions with spdif out, and other versions with spdif in and
> out.

Right, the above was just an example to illustrate that we can have
additional properties to encode the differences between each instance
of the audio devices.

For example, for XOR engines, we have:

                        xor@60900 {
                                compatible = "marvell,orion-xor";
                                reg = <0x60900 0x100
                                       0x60b00 0x100>;
                                clocks = <&gateclk 22>;
                                status = "okay";

                                xor10 {
                                        interrupts = <51>;
                                        dmacap,memcpy;
                                        dmacap,xor;
                                };
                                xor11 {
                                        interrupts = <52>;
                                        dmacap,memcpy;
                                        dmacap,xor;
                                        dmacap,memset;
                                };
                        };

because the first channel of each XOR engine has only memcpy and xor
capabilities, while the second channel has memcpy, xor and memset
capabilities.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Dear Russell King - ARM Linux,

On Wed, 28 Aug 2013 13:13:20 +0100, Russell King - ARM Linux wrote:

> > > I guess, yes.
> > 
> > Jean-Fran?ois, could you cook and submit a patch to change the
> > compatible string?
> 
> I don't think this is a good idea.  The configuration of this IP is
> not based on the SoC as a single SoC can have a mixture of different
> configurations.

Using the name of the oldest SoC in the family that had the IP block is
the norm, because it's really what "compatible" means: the IP block in
Dove is *compatible* with the one that was originally introduced in
Kirkwood.

See what Rob Herring (one of the DT maintainer) says in
http://lists.infradead.org/pipermail/linux-mtd/2012-March/040417.html:

"""
There is no reason all machines can't use "st,spear600-smi" in their
dts. It doesn't have to be a spear600, just compatible with it. Really
you want the string to be the oldest SOC the block is in and then newer
SOCs can claim compatibility with the old version.
"""

The thread was precisely about replacing a SoC-specific compatible
string "st,spear600-smi" by a more generic "st,spear-smi" and Rob
Herring (above) was opposing to that.

> I think marvell,mvebu-audio is a reasonable compatible string for this,
> and that the different configurations should be described by properties
> indicating which inputs and outputs have been implemented.
> 
> For instance, on the Dove, there are two of these blocks.  One has I2S
> in and out only, but the other block has I2S in and out, and SPDIF out.
> On some other Marvell devices, this block has I2S in and out and SPDIF
> in and out.
> 
> Otherwise, they're functionally the same.

Right, that's why they can both use "kirkwood-audio" as the compatible
string.

> > Though, if the difference between the two units is the availability of
> > SPDIF support, then we shouldn't encode the channel number, but instead
> > the availability of SPDIF, i.e:
> > 
> > 	audio0 {
> > 		reg = <... ...>;
> > 		compatible = "marvell,kirkwood-audio";
> > 		marvell,has-spdif;
> > 	};
> > 
> > 	audio1 {
> > 		reg = <... ...>;
> > 		compatible = "marvell,kirkwood-audio";
> > 	};
> 
> ... which means there's no problem with using marvell,mvebu-audio as the
> compatible string if you're going to use properties to describe what
> facilities are available.

I disagree, because how do you know if a future "mvebu" SOC such as
Armada 370, or one that doesn't exist yet, will not have a different
audio IP block? It will still be audio, it will still be mvebu, but it
will not be able to use a "marvell,mvebu-audio" driver. Or maybe it can
use the same driver, but with a few variations, so a different
compatible string will be needed to identify the original IP
("marvell,kirkwood-audio", used on Kirkwood/Dove) and slightly newer
versions of the IP ("marvell,some-funky-soc-audio").

> In any case "marvell,has-spdif" is too generic - as I've indicated above,
> there's versions with spdif out, and other versions with spdif in and
> out.

Right, the above was just an example to illustrate that we can have
additional properties to encode the differences between each instance
of the audio devices.

For example, for XOR engines, we have:

                        xor at 60900 {
                                compatible = "marvell,orion-xor";
                                reg = <0x60900 0x100
                                       0x60b00 0x100>;
                                clocks = <&gateclk 22>;
                                status = "okay";

                                xor10 {
                                        interrupts = <51>;
                                        dmacap,memcpy;
                                        dmacap,xor;
                                };
                                xor11 {
                                        interrupts = <52>;
                                        dmacap,memcpy;
                                        dmacap,xor;
                                        dmacap,memset;
                                };
                        };

because the first channel of each XOR engine has only memcpy and xor
capabilities, while the second channel has memcpy, xor and memset
capabilities.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Wed, Aug 28, 2013 at 02:29:09PM +0200, Thomas Petazzoni wrote:
> Dear Russell King - ARM Linux,
> 
> On Wed, 28 Aug 2013 13:13:20 +0100, Russell King - ARM Linux wrote:
> 
> > > > I guess, yes.
> > > 
> > > Jean-François, could you cook and submit a patch to change the
> > > compatible string?
> > 
> > I don't think this is a good idea.  The configuration of this IP is
> > not based on the SoC as a single SoC can have a mixture of different
> > configurations.
> 
> Using the name of the oldest SoC in the family that had the IP block is
> the norm, because it's really what "compatible" means: the IP block in
> Dove is *compatible* with the one that was originally introduced in
> Kirkwood.
> 
> See what Rob Herring (one of the DT maintainer) says in
> http://lists.infradead.org/pipermail/linux-mtd/2012-March/040417.html:
> 
> """
> There is no reason all machines can't use "st,spear600-smi" in their
> dts. It doesn't have to be a spear600, just compatible with it. Really
> you want the string to be the oldest SOC the block is in and then newer
> SOCs can claim compatibility with the old version.
> """
> 
> The thread was precisely about replacing a SoC-specific compatible
> string "st,spear600-smi" by a more generic "st,spear-smi" and Rob
> Herring (above) was opposing to that.

We're not talking about replacing a pre-existing string, we're talking
about adding one, which is a different situation.

> > I think marvell,mvebu-audio is a reasonable compatible string for this,
> > and that the different configurations should be described by properties
> > indicating which inputs and outputs have been implemented.
> > 
> > For instance, on the Dove, there are two of these blocks.  One has I2S
> > in and out only, but the other block has I2S in and out, and SPDIF out.
> > On some other Marvell devices, this block has I2S in and out and SPDIF
> > in and out.
> > 
> > Otherwise, they're functionally the same.
> 
> Right, that's why they can both use "kirkwood-audio" as the compatible
> string.
> 
> > > Though, if the difference between the two units is the availability of
> > > SPDIF support, then we shouldn't encode the channel number, but instead
> > > the availability of SPDIF, i.e:
> > > 
> > > 	audio0 {
> > > 		reg = <... ...>;
> > > 		compatible = "marvell,kirkwood-audio";
> > > 		marvell,has-spdif;
> > > 	};
> > > 
> > > 	audio1 {
> > > 		reg = <... ...>;
> > > 		compatible = "marvell,kirkwood-audio";
> > > 	};
> > 
> > ... which means there's no problem with using marvell,mvebu-audio as the
> > compatible string if you're going to use properties to describe what
> > facilities are available.
> 
> I disagree, because how do you know if a future "mvebu" SOC such as
> Armada 370, or one that doesn't exist yet, will not have a different
> audio IP block?

The Dove already contains _three_ audio blocks, two of which are this
one, and another which is block for driving an AC'97 codec (which doesn't
have a driver.)  That's no problem because you won't call that one an
"audio" block but an AC'97 block.  So...

> It will still be audio, it will still be mvebu, but it
> will not be able to use a "marvell,mvebu-audio" driver. Or maybe it can
> use the same driver, but with a few variations, so a different
> compatible string will be needed to identify the original IP
> ("marvell,kirkwood-audio", used on Kirkwood/Dove) and slightly newer
> versions of the IP ("marvell,some-funky-soc-audio").

I don't think this really applies.

> > In any case "marvell,has-spdif" is too generic - as I've indicated above,
> > there's versions with spdif out, and other versions with spdif in and
> > out.
> 
> Right, the above was just an example to illustrate that we can have
> additional properties to encode the differences between each instance
> of the audio devices.

I think this is a mistake too: these properties will just tell us what
may be possible, and the driver will take no real action on them.  I
suppose that a property specifying whether there is a SPDIF output could
be used to control whether the IEC958 channel status controls are
registered.  However...

What's more important is which outputs are actually wired up, and
therefore which bits of this hardware are enabled.  Even then, we
wouldn't want to expose (eg) the IEC958 channel status controls if
the SPDIF output isn't wired.  So all in all, I don't see any point
to a set of properties saying "we have SPDIF" etc.  That information
should come solely from whether the SPDIF output has been "wired up".

Let me put that another way: we _can_ provide those properties to
indicate what facilities the hardware has, we just wouldn't use them
at all - and to provide them seems like over-design to me.

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Wed, Aug 28, 2013 at 02:29:09PM +0200, Thomas Petazzoni wrote:
> Dear Russell King - ARM Linux,
> 
> On Wed, 28 Aug 2013 13:13:20 +0100, Russell King - ARM Linux wrote:
> 
> > > > I guess, yes.
> > > 
> > > Jean-Fran?ois, could you cook and submit a patch to change the
> > > compatible string?
> > 
> > I don't think this is a good idea.  The configuration of this IP is
> > not based on the SoC as a single SoC can have a mixture of different
> > configurations.
> 
> Using the name of the oldest SoC in the family that had the IP block is
> the norm, because it's really what "compatible" means: the IP block in
> Dove is *compatible* with the one that was originally introduced in
> Kirkwood.
> 
> See what Rob Herring (one of the DT maintainer) says in
> http://lists.infradead.org/pipermail/linux-mtd/2012-March/040417.html:
> 
> """
> There is no reason all machines can't use "st,spear600-smi" in their
> dts. It doesn't have to be a spear600, just compatible with it. Really
> you want the string to be the oldest SOC the block is in and then newer
> SOCs can claim compatibility with the old version.
> """
> 
> The thread was precisely about replacing a SoC-specific compatible
> string "st,spear600-smi" by a more generic "st,spear-smi" and Rob
> Herring (above) was opposing to that.

We're not talking about replacing a pre-existing string, we're talking
about adding one, which is a different situation.

> > I think marvell,mvebu-audio is a reasonable compatible string for this,
> > and that the different configurations should be described by properties
> > indicating which inputs and outputs have been implemented.
> > 
> > For instance, on the Dove, there are two of these blocks.  One has I2S
> > in and out only, but the other block has I2S in and out, and SPDIF out.
> > On some other Marvell devices, this block has I2S in and out and SPDIF
> > in and out.
> > 
> > Otherwise, they're functionally the same.
> 
> Right, that's why they can both use "kirkwood-audio" as the compatible
> string.
> 
> > > Though, if the difference between the two units is the availability of
> > > SPDIF support, then we shouldn't encode the channel number, but instead
> > > the availability of SPDIF, i.e:
> > > 
> > > 	audio0 {
> > > 		reg = <... ...>;
> > > 		compatible = "marvell,kirkwood-audio";
> > > 		marvell,has-spdif;
> > > 	};
> > > 
> > > 	audio1 {
> > > 		reg = <... ...>;
> > > 		compatible = "marvell,kirkwood-audio";
> > > 	};
> > 
> > ... which means there's no problem with using marvell,mvebu-audio as the
> > compatible string if you're going to use properties to describe what
> > facilities are available.
> 
> I disagree, because how do you know if a future "mvebu" SOC such as
> Armada 370, or one that doesn't exist yet, will not have a different
> audio IP block?

The Dove already contains _three_ audio blocks, two of which are this
one, and another which is block for driving an AC'97 codec (which doesn't
have a driver.)  That's no problem because you won't call that one an
"audio" block but an AC'97 block.  So...

> It will still be audio, it will still be mvebu, but it
> will not be able to use a "marvell,mvebu-audio" driver. Or maybe it can
> use the same driver, but with a few variations, so a different
> compatible string will be needed to identify the original IP
> ("marvell,kirkwood-audio", used on Kirkwood/Dove) and slightly newer
> versions of the IP ("marvell,some-funky-soc-audio").

I don't think this really applies.

> > In any case "marvell,has-spdif" is too generic - as I've indicated above,
> > there's versions with spdif out, and other versions with spdif in and
> > out.
> 
> Right, the above was just an example to illustrate that we can have
> additional properties to encode the differences between each instance
> of the audio devices.

I think this is a mistake too: these properties will just tell us what
may be possible, and the driver will take no real action on them.  I
suppose that a property specifying whether there is a SPDIF output could
be used to control whether the IEC958 channel status controls are
registered.  However...

What's more important is which outputs are actually wired up, and
therefore which bits of this hardware are enabled.  Even then, we
wouldn't want to expose (eg) the IEC958 channel status controls if
the SPDIF output isn't wired.  So all in all, I don't see any point
to a set of properties saying "we have SPDIF" etc.  That information
should come solely from whether the SPDIF output has been "wired up".

Let me put that another way: we _can_ provide those properties to
indicate what facilities the hardware has, we just wouldn't use them
at all - and to provide them seems like over-design to me.

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Dear Russell King - ARM Linux,

On Wed, 28 Aug 2013 13:42:55 +0100, Russell King - ARM Linux wrote:

> > Using the name of the oldest SoC in the family that had the IP block is
> > the norm, because it's really what "compatible" means: the IP block in
> > Dove is *compatible* with the one that was originally introduced in
> > Kirkwood.
> > 
> > See what Rob Herring (one of the DT maintainer) says in
> > http://lists.infradead.org/pipermail/linux-mtd/2012-March/040417.html:
> > 
> > """
> > There is no reason all machines can't use "st,spear600-smi" in their
> > dts. It doesn't have to be a spear600, just compatible with it. Really
> > you want the string to be the oldest SOC the block is in and then newer
> > SOCs can claim compatibility with the old version.
> > """
> > 
> > The thread was precisely about replacing a SoC-specific compatible
> > string "st,spear600-smi" by a more generic "st,spear-smi" and Rob
> > Herring (above) was opposing to that.
> 
> We're not talking about replacing a pre-existing string, we're talking
> about adding one, which is a different situation.

I don't see how this makes this a different situation. See for example
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/161065.html
and
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/087702.html
where Arnd also said using the oldest SoC that has the same IP block as
the compatible string was the right thing to do.

> > > ... which means there's no problem with using marvell,mvebu-audio as the
> > > compatible string if you're going to use properties to describe what
> > > facilities are available.
> > 
> > I disagree, because how do you know if a future "mvebu" SOC such as
> > Armada 370, or one that doesn't exist yet, will not have a different
> > audio IP block?
> 
> The Dove already contains _three_ audio blocks, two of which are this
> one, and another which is block for driving an AC'97 codec (which doesn't
> have a driver.)  That's no problem because you won't call that one an
> "audio" block but an AC'97 block.  So...

And? If that's a different IP block, it'll have a different compatible
string, that's it.

That doesn't change my point: using "marvell,mvebu-audio" as the
compatible string is stupid, because you have absolutely no idea what
the future of audio in mvebu SOCs will be. However, you do know,
*today* that Kirkwood and Dove have compatible IP blocks for audio, and
that they were first introduced with Kirkwood.

> > It will still be audio, it will still be mvebu, but it
> > will not be able to use a "marvell,mvebu-audio" driver. Or maybe it can
> > use the same driver, but with a few variations, so a different
> > compatible string will be needed to identify the original IP
> > ("marvell,kirkwood-audio", used on Kirkwood/Dove) and slightly newer
> > versions of the IP ("marvell,some-funky-soc-audio").
> 
> I don't think this really applies.

It does. We're exactly in this situation, as I will soon be working on
Armada 370 audio support, and while the IP looks similar, I have
checked all the details to see if it's exactly identical.

And Armada 370 is really a mvebu architecture: it's even supported in
mach-mvebu/, while Kirkwood and Dove are not (yet).


> > > In any case "marvell,has-spdif" is too generic - as I've indicated above,
> > > there's versions with spdif out, and other versions with spdif in and
> > > out.
> > 
> > Right, the above was just an example to illustrate that we can have
> > additional properties to encode the differences between each instance
> > of the audio devices.
> 
> I think this is a mistake too: these properties will just tell us what
> may be possible, and the driver will take no real action on them.  I
> suppose that a property specifying whether there is a SPDIF output could
> be used to control whether the IEC958 channel status controls are
> registered.  However...
> 
> What's more important is which outputs are actually wired up, and
> therefore which bits of this hardware are enabled.  Even then, we
> wouldn't want to expose (eg) the IEC958 channel status controls if
> the SPDIF output isn't wired.  So all in all, I don't see any point
> to a set of properties saying "we have SPDIF" etc.  That information
> should come solely from whether the SPDIF output has been "wired up".
> 
> Let me put that another way: we _can_ provide those properties to
> indicate what facilities the hardware has, we just wouldn't use them
> at all - and to provide them seems like over-design to me.

I am not arguing about the properties, as I haven't looked at the
specific problem that needs to be solved. By suggesting properties, I
was merely suggesting one possible solution to the problem that
Sebastian was raising, where the different instances of the IP block
don't have the same capabilities.

What I am however strongly arguing on is the choice of the compatible
string. marvell,mvebu-audio is a wrong choice.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Thomas Petazzoni]

Dear Russell King - ARM Linux,

On Wed, 28 Aug 2013 13:42:55 +0100, Russell King - ARM Linux wrote:

> > Using the name of the oldest SoC in the family that had the IP block is
> > the norm, because it's really what "compatible" means: the IP block in
> > Dove is *compatible* with the one that was originally introduced in
> > Kirkwood.
> > 
> > See what Rob Herring (one of the DT maintainer) says in
> > http://lists.infradead.org/pipermail/linux-mtd/2012-March/040417.html:
> > 
> > """
> > There is no reason all machines can't use "st,spear600-smi" in their
> > dts. It doesn't have to be a spear600, just compatible with it. Really
> > you want the string to be the oldest SOC the block is in and then newer
> > SOCs can claim compatibility with the old version.
> > """
> > 
> > The thread was precisely about replacing a SoC-specific compatible
> > string "st,spear600-smi" by a more generic "st,spear-smi" and Rob
> > Herring (above) was opposing to that.
> 
> We're not talking about replacing a pre-existing string, we're talking
> about adding one, which is a different situation.

I don't see how this makes this a different situation. See for example
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/161065.html
and
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/087702.html
where Arnd also said using the oldest SoC that has the same IP block as
the compatible string was the right thing to do.

> > > ... which means there's no problem with using marvell,mvebu-audio as the
> > > compatible string if you're going to use properties to describe what
> > > facilities are available.
> > 
> > I disagree, because how do you know if a future "mvebu" SOC such as
> > Armada 370, or one that doesn't exist yet, will not have a different
> > audio IP block?
> 
> The Dove already contains _three_ audio blocks, two of which are this
> one, and another which is block for driving an AC'97 codec (which doesn't
> have a driver.)  That's no problem because you won't call that one an
> "audio" block but an AC'97 block.  So...

And? If that's a different IP block, it'll have a different compatible
string, that's it.

That doesn't change my point: using "marvell,mvebu-audio" as the
compatible string is stupid, because you have absolutely no idea what
the future of audio in mvebu SOCs will be. However, you do know,
*today* that Kirkwood and Dove have compatible IP blocks for audio, and
that they were first introduced with Kirkwood.

> > It will still be audio, it will still be mvebu, but it
> > will not be able to use a "marvell,mvebu-audio" driver. Or maybe it can
> > use the same driver, but with a few variations, so a different
> > compatible string will be needed to identify the original IP
> > ("marvell,kirkwood-audio", used on Kirkwood/Dove) and slightly newer
> > versions of the IP ("marvell,some-funky-soc-audio").
> 
> I don't think this really applies.

It does. We're exactly in this situation, as I will soon be working on
Armada 370 audio support, and while the IP looks similar, I have
checked all the details to see if it's exactly identical.

And Armada 370 is really a mvebu architecture: it's even supported in
mach-mvebu/, while Kirkwood and Dove are not (yet).


> > > In any case "marvell,has-spdif" is too generic - as I've indicated above,
> > > there's versions with spdif out, and other versions with spdif in and
> > > out.
> > 
> > Right, the above was just an example to illustrate that we can have
> > additional properties to encode the differences between each instance
> > of the audio devices.
> 
> I think this is a mistake too: these properties will just tell us what
> may be possible, and the driver will take no real action on them.  I
> suppose that a property specifying whether there is a SPDIF output could
> be used to control whether the IEC958 channel status controls are
> registered.  However...
> 
> What's more important is which outputs are actually wired up, and
> therefore which bits of this hardware are enabled.  Even then, we
> wouldn't want to expose (eg) the IEC958 channel status controls if
> the SPDIF output isn't wired.  So all in all, I don't see any point
> to a set of properties saying "we have SPDIF" etc.  That information
> should come solely from whether the SPDIF output has been "wired up".
> 
> Let me put that another way: we _can_ provide those properties to
> indicate what facilities the hardware has, we just wouldn't use them
> at all - and to provide them seems like over-design to me.

I am not arguing about the properties, as I haven't looked at the
specific problem that needs to be solved. By suggesting properties, I
was merely suggesting one possible solution to the problem that
Sebastian was raising, where the different instances of the IP block
don't have the same capabilities.

What I am however strongly arguing on is the choice of the compatible
string. marvell,mvebu-audio is a wrong choice.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Wed, Aug 28, 2013 at 02:51:51PM +0200, Thomas Petazzoni wrote:
> I don't see how this makes this a different situation. See for example
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/161065.html
> and
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/087702.html
> where Arnd also said using the oldest SoC that has the same IP block as
> the compatible string was the right thing to do.

Well, I have a different opinion, and that's all there is to it.  However,
differing opinions aren't tolerated within the kernel community even if
they are equally valid, so I'll just shut up about after this mail.

> And? If that's a different IP block, it'll have a different compatible
> string, that's it.

There you go, you've just said why there's no problem with using a generic
compatible string.

> It does. We're exactly in this situation, as I will soon be working on
> Armada 370 audio support, and while the IP looks similar, I have
> checked all the details to see if it's exactly identical.

You seem to have missed something in that paragraph.  If it's exactly
identical, there isn't a problem. :)

> And Armada 370 is really a mvebu architecture: it's even supported in
> mach-mvebu/, while Kirkwood and Dove are not (yet).

Maybe it should be mach-kirkwood and not mach-mvebu by your reasoning
then!

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Wed, Aug 28, 2013 at 02:51:51PM +0200, Thomas Petazzoni wrote:
> I don't see how this makes this a different situation. See for example
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/161065.html
> and
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/087702.html
> where Arnd also said using the oldest SoC that has the same IP block as
> the compatible string was the right thing to do.

Well, I have a different opinion, and that's all there is to it.  However,
differing opinions aren't tolerated within the kernel community even if
they are equally valid, so I'll just shut up about after this mail.

> And? If that's a different IP block, it'll have a different compatible
> string, that's it.

There you go, you've just said why there's no problem with using a generic
compatible string.

> It does. We're exactly in this situation, as I will soon be working on
> Armada 370 audio support, and while the IP looks similar, I have
> checked all the details to see if it's exactly identical.

You seem to have missed something in that paragraph.  If it's exactly
identical, there isn't a problem. :)

> And Armada 370 is really a mvebu architecture: it's even supported in
> mach-mvebu/, while Kirkwood and Dove are not (yet).

Maybe it should be mach-kirkwood and not mach-mvebu by your reasoning
then!

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/13 13:58, Thomas Petazzoni wrote:
> On Wed, 28 Aug 2013 13:44:51 +0200, Sebastian Hesselbarth wrote:
>>>> Also, we'll need to distinguish between the different audio controllers
>>>> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
>>>> base passed.
>>>
>>> For what reason does the driver needs to know whether it's the instance
>>> 0 or instance 1 ? If it's needed for some specific reason, then there
>>> should probably be something like marvell,i2s-channel-id = <0> and
>>> marvell,i2s-channel-id = <1>.
>>
>> On Dove, audio1 has SPDIF out, audio0 hasn't. Russell also mentioned to
>> get rid of "i2s" and use "audio" instead. Most SoC's controllers are
>> i2s only but as soon as SPDIF comes into play, it is a different
>> interface protocol.
>>
>> I am fine with having a "marvell,channel-id" (no "i2s") to discriminate
>> the instances, although reg offset should be sufficient.
>
> Well, the reg offset is a possibility, but it's not really nice, and
> would have to be adapted to each and every SoC even if the reset of the
> audio IP is the same.
>
> Though, if the difference between the two units is the availability of
> SPDIF support, then we shouldn't encode the channel number, but instead
> the availability of SPDIF, i.e:
>
> 	audio0 {
> 		reg = <... ...>;
> 		compatible = "marvell,kirkwood-audio";
> 		marvell,has-spdif;

Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out"
Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi
for the one audio controller available. Can't tell for Armada 370.

BTW, you might have followed some of the DT discussions with Mark
before; as he insists on having a separate sound card node, he might
argue that above property should be part of that node instead.

Last patch discussion [1] I followed on some spdif sound nodes, took the
patch up to v11 or so. Mainly, because the author updated it too
quickly, but looks like audio bindings are (still) worth a lot of
discussion.

[1] http://comments.gmane.org/gmane.linux.alsa.devel/112004

Sebastian

> 	};
>
> 	audio1 {
> 		reg = <... ...>;
> 		compatible = "marvell,kirkwood-audio";
> 	};

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/28/13 13:58, Thomas Petazzoni wrote:
> On Wed, 28 Aug 2013 13:44:51 +0200, Sebastian Hesselbarth wrote:
>>>> Also, we'll need to distinguish between the different audio controllers
>>>> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
>>>> base passed.
>>>
>>> For what reason does the driver needs to know whether it's the instance
>>> 0 or instance 1 ? If it's needed for some specific reason, then there
>>> should probably be something like marvell,i2s-channel-id = <0> and
>>> marvell,i2s-channel-id = <1>.
>>
>> On Dove, audio1 has SPDIF out, audio0 hasn't. Russell also mentioned to
>> get rid of "i2s" and use "audio" instead. Most SoC's controllers are
>> i2s only but as soon as SPDIF comes into play, it is a different
>> interface protocol.
>>
>> I am fine with having a "marvell,channel-id" (no "i2s") to discriminate
>> the instances, although reg offset should be sufficient.
>
> Well, the reg offset is a possibility, but it's not really nice, and
> would have to be adapted to each and every SoC even if the reset of the
> audio IP is the same.
>
> Though, if the difference between the two units is the availability of
> SPDIF support, then we shouldn't encode the channel number, but instead
> the availability of SPDIF, i.e:
>
> 	audio0 {
> 		reg = <... ...>;
> 		compatible = "marvell,kirkwood-audio";
> 		marvell,has-spdif;

Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out"
Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi
for the one audio controller available. Can't tell for Armada 370.

BTW, you might have followed some of the DT discussions with Mark
before; as he insists on having a separate sound card node, he might
argue that above property should be part of that node instead.

Last patch discussion [1] I followed on some spdif sound nodes, took the
patch up to v11 or so. Mainly, because the author updated it too
quickly, but looks like audio bindings are (still) worth a lot of
discussion.

[1] http://comments.gmane.org/gmane.linux.alsa.devel/112004

Sebastian

> 	};
>
> 	audio1 {
> 		reg = <... ...>;
> 		compatible = "marvell,kirkwood-audio";
> 	};

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Jean-Francois Moine]

On Wed, 28 Aug 2013 14:16:32 +0200
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

	[snip]
> > Though, if the difference between the two units is the availability of
> > SPDIF support, then we shouldn't encode the channel number, but instead
> > the availability of SPDIF, i.e:
> >
> > 	audio0 {
> > 		reg = <... ...>;
> > 		compatible = "marvell,kirkwood-audio";
> > 		marvell,has-spdif;  
> 
> Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out"
> Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi
> for the one audio controller available. Can't tell for Armada 370.
> 
> BTW, you might have followed some of the DT discussions with Mark
> before; as he insists on having a separate sound card node, he might
> argue that above property should be part of that node instead.

Yes. For the Cubox, the card will be described by something like:

	sound {
		compatible = "simple-audio";
		audio-controller = <&audio1>;
		audio-codec = <&spdif>;
		codec-dai-name = "dit-hifi";
	};

with:

	spdif: spdif {
 		compatible = "linux,spdif-dit";
	};

Then, the audio driver will know about s/pdif on the first open.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Jean-Francois Moine]

On Wed, 28 Aug 2013 14:16:32 +0200
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

	[snip]
> > Though, if the difference between the two units is the availability of
> > SPDIF support, then we shouldn't encode the channel number, but instead
> > the availability of SPDIF, i.e:
> >
> > 	audio0 {
> > 		reg = <... ...>;
> > 		compatible = "marvell,kirkwood-audio";
> > 		marvell,has-spdif;  
> 
> Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out"
> Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi
> for the one audio controller available. Can't tell for Armada 370.
> 
> BTW, you might have followed some of the DT discussions with Mark
> before; as he insists on having a separate sound card node, he might
> argue that above property should be part of that node instead.

Yes. For the Cubox, the card will be described by something like:

	sound {
		compatible = "simple-audio";
		audio-controller = <&audio1>;
		audio-codec = <&spdif>;
		codec-dai-name = "dit-hifi";
	};

with:

	spdif: spdif {
 		compatible = "linux,spdif-dit";
	};

Then, the audio driver will know about s/pdif on the first open.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Thu, Aug 29, 2013 at 12:07:04PM +0200, Jean-Francois Moine wrote:
> On Wed, 28 Aug 2013 14:16:32 +0200
> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
> 
> 	[snip]
> > > Though, if the difference between the two units is the availability of
> > > SPDIF support, then we shouldn't encode the channel number, but instead
> > > the availability of SPDIF, i.e:
> > >
> > > 	audio0 {
> > > 		reg = <... ...>;
> > > 		compatible = "marvell,kirkwood-audio";
> > > 		marvell,has-spdif;  
> > 
> > Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out"
> > Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi
> > for the one audio controller available. Can't tell for Armada 370.
> > 
> > BTW, you might have followed some of the DT discussions with Mark
> > before; as he insists on having a separate sound card node, he might
> > argue that above property should be part of that node instead.
> 
> Yes. For the Cubox, the card will be described by something like:
> 
> 	sound {
> 		compatible = "simple-audio";
> 		audio-controller = <&audio1>;
> 		audio-codec = <&spdif>;
> 		codec-dai-name = "dit-hifi";
> 	};
> 
> with:
> 
> 	spdif: spdif {
>  		compatible = "linux,spdif-dit";
> 	};
> 
> Then, the audio driver will know about s/pdif on the first open.

I can tell that neither of you have taken notice of what I said about
the "has" stuff - it's completely useless to the driver, it conveys no
useful information.

Moreover, the above isn't going to be the answer to this.  With DPCM
you need:

1. two DAI links to be setup:
1a. one to connect the CPU DAI to the dummy codec
1b. one to connect the dummy platform/CPU DAI to the SPDIF codec
2. DAPM routes to connect the CPU DAI audio stream(s) to the Codec stream(s)

If you have both I2S and SPDIF, then you need another DAI link and a
few more DAPM routes.  Even if you have just one codec connected, you
will need this structure so that the CPU DAI knows which audio outputs
are to be enabled.  If no DAPM routes exist, the CPU DAI will not enable
any outputs.

Or at least that's the theory when ASoC DPCM eventually works.

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Thu, Aug 29, 2013 at 12:07:04PM +0200, Jean-Francois Moine wrote:
> On Wed, 28 Aug 2013 14:16:32 +0200
> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
> 
> 	[snip]
> > > Though, if the difference between the two units is the availability of
> > > SPDIF support, then we shouldn't encode the channel number, but instead
> > > the availability of SPDIF, i.e:
> > >
> > > 	audio0 {
> > > 		reg = <... ...>;
> > > 		compatible = "marvell,kirkwood-audio";
> > > 		marvell,has-spdif;  
> > 
> > Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out"
> > Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi
> > for the one audio controller available. Can't tell for Armada 370.
> > 
> > BTW, you might have followed some of the DT discussions with Mark
> > before; as he insists on having a separate sound card node, he might
> > argue that above property should be part of that node instead.
> 
> Yes. For the Cubox, the card will be described by something like:
> 
> 	sound {
> 		compatible = "simple-audio";
> 		audio-controller = <&audio1>;
> 		audio-codec = <&spdif>;
> 		codec-dai-name = "dit-hifi";
> 	};
> 
> with:
> 
> 	spdif: spdif {
>  		compatible = "linux,spdif-dit";
> 	};
> 
> Then, the audio driver will know about s/pdif on the first open.

I can tell that neither of you have taken notice of what I said about
the "has" stuff - it's completely useless to the driver, it conveys no
useful information.

Moreover, the above isn't going to be the answer to this.  With DPCM
you need:

1. two DAI links to be setup:
1a. one to connect the CPU DAI to the dummy codec
1b. one to connect the dummy platform/CPU DAI to the SPDIF codec
2. DAPM routes to connect the CPU DAI audio stream(s) to the Codec stream(s)

If you have both I2S and SPDIF, then you need another DAI link and a
few more DAPM routes.  Even if you have just one codec connected, you
will need this structure so that the CPU DAI knows which audio outputs
are to be enabled.  If no DAPM routes exist, the CPU DAI will not enable
any outputs.

Or at least that's the theory when ASoC DPCM eventually works.

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/29/13 12:13, Russell King - ARM Linux wrote:
> On Thu, Aug 29, 2013 at 12:07:04PM +0200, Jean-Francois Moine wrote:
>> On Wed, 28 Aug 2013 14:16:32 +0200
>> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
>>
>> 	[snip]
>>>> Though, if the difference between the two units is the availability of
>>>> SPDIF support, then we shouldn't encode the channel number, but instead
>>>> the availability of SPDIF, i.e:
>>>>
>>>> 	audio0 {
>>>> 		reg = <... ...>;
>>>> 		compatible = "marvell,kirkwood-audio";
>>>> 		marvell,has-spdif;
>>>
>>> Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out"
>>> Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi
>>> for the one audio controller available. Can't tell for Armada 370.
>>>
>>> BTW, you might have followed some of the DT discussions with Mark
>>> before; as he insists on having a separate sound card node, he might
>>> argue that above property should be part of that node instead.
>>
>> Yes. For the Cubox, the card will be described by something like:
>>
>> 	sound {
>> 		compatible = "simple-audio";
>> 		audio-controller = <&audio1>;
>> 		audio-codec = <&spdif>;
>> 		codec-dai-name = "dit-hifi";
>> 	};
>>
>> with:
>>
>> 	spdif: spdif {
>>   		compatible = "linux,spdif-dit";
>> 	};
>>
>> Then, the audio driver will know about s/pdif on the first open.
>
> I can tell that neither of you have taken notice of what I said about
> the "has" stuff - it's completely useless to the driver, it conveys no
> useful information.

You are right, of course, that has-spdif-foo properties are useless,
for the driver and the DT. For a working audio setup, we need to link
the codecs anyway and we can put that information in there.

> Moreover, the above isn't going to be the answer to this.  With DPCM
> you need:
>
> 1. two DAI links to be setup:
> 1a. one to connect the CPU DAI to the dummy codec
> 1b. one to connect the dummy platform/CPU DAI to the SPDIF codec

Taking the sound node above:

sound {
	compatible = "whatever-audio";
	...
	audio-codec = <&rt1234 1>, <&spdif 0>;
	audio-codec-names = "i2s", "spdifo";
	...
};

Each audio-codec phandle with arg links to one specific "port" at some
"codec". The audio-codec-names property allows the ASoC driver to
determine the local "ports" where the above codecs are connected.
The dummy codecs are ASoC specific and must be added by the driver.

Basically, it is the same approach as we have for clocks, maybe the 
property names are not best suited, but it should give an impression of
how it _could_ be done.

Unfortunately, the fsl spdif discussion I referenced earlier ended up
with
a) a special machine driver and its own DT binding
b) using that "has" properties approach

> 2. DAPM routes to connect the CPU DAI audio stream(s) to the Codec stream(s)

I am not yet sure how that could be solved easily with DT, nor if it
should go in there at all. But I have not dug into ASoC and DAPM enough.

Can you point out, given the above linking, what you think is missing
to allow the driver to find all it needs wrt to DAIs and DAPM?

> If you have both I2S and SPDIF, then you need another DAI link and a
> few more DAPM routes.  Even if you have just one codec connected, you
> will need this structure so that the CPU DAI knows which audio outputs
> are to be enabled.  If no DAPM routes exist, the CPU DAI will not enable
> any outputs.
>
> Or at least that's the theory when ASoC DPCM eventually works.
>

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/29/13 12:13, Russell King - ARM Linux wrote:
> On Thu, Aug 29, 2013 at 12:07:04PM +0200, Jean-Francois Moine wrote:
>> On Wed, 28 Aug 2013 14:16:32 +0200
>> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
>>
>> 	[snip]
>>>> Though, if the difference between the two units is the availability of
>>>> SPDIF support, then we shouldn't encode the channel number, but instead
>>>> the availability of SPDIF, i.e:
>>>>
>>>> 	audio0 {
>>>> 		reg = <... ...>;
>>>> 		compatible = "marvell,kirkwood-audio";
>>>> 		marvell,has-spdif;
>>>
>>> Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out"
>>> Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi
>>> for the one audio controller available. Can't tell for Armada 370.
>>>
>>> BTW, you might have followed some of the DT discussions with Mark
>>> before; as he insists on having a separate sound card node, he might
>>> argue that above property should be part of that node instead.
>>
>> Yes. For the Cubox, the card will be described by something like:
>>
>> 	sound {
>> 		compatible = "simple-audio";
>> 		audio-controller = <&audio1>;
>> 		audio-codec = <&spdif>;
>> 		codec-dai-name = "dit-hifi";
>> 	};
>>
>> with:
>>
>> 	spdif: spdif {
>>   		compatible = "linux,spdif-dit";
>> 	};
>>
>> Then, the audio driver will know about s/pdif on the first open.
>
> I can tell that neither of you have taken notice of what I said about
> the "has" stuff - it's completely useless to the driver, it conveys no
> useful information.

You are right, of course, that has-spdif-foo properties are useless,
for the driver and the DT. For a working audio setup, we need to link
the codecs anyway and we can put that information in there.

> Moreover, the above isn't going to be the answer to this.  With DPCM
> you need:
>
> 1. two DAI links to be setup:
> 1a. one to connect the CPU DAI to the dummy codec
> 1b. one to connect the dummy platform/CPU DAI to the SPDIF codec

Taking the sound node above:

sound {
	compatible = "whatever-audio";
	...
	audio-codec = <&rt1234 1>, <&spdif 0>;
	audio-codec-names = "i2s", "spdifo";
	...
};

Each audio-codec phandle with arg links to one specific "port" at some
"codec". The audio-codec-names property allows the ASoC driver to
determine the local "ports" where the above codecs are connected.
The dummy codecs are ASoC specific and must be added by the driver.

Basically, it is the same approach as we have for clocks, maybe the 
property names are not best suited, but it should give an impression of
how it _could_ be done.

Unfortunately, the fsl spdif discussion I referenced earlier ended up
with
a) a special machine driver and its own DT binding
b) using that "has" properties approach

> 2. DAPM routes to connect the CPU DAI audio stream(s) to the Codec stream(s)

I am not yet sure how that could be solved easily with DT, nor if it
should go in there at all. But I have not dug into ASoC and DAPM enough.

Can you point out, given the above linking, what you think is missing
to allow the driver to find all it needs wrt to DAIs and DAPM?

> If you have both I2S and SPDIF, then you need another DAI link and a
> few more DAPM routes.  Even if you have just one codec connected, you
> will need this structure so that the CPU DAI knows which audio outputs
> are to be enabled.  If no DAPM routes exist, the CPU DAI will not enable
> any outputs.
>
> Or at least that's the theory when ASoC DPCM eventually works.
>

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Thu, Aug 29, 2013 at 01:01:23PM +0200, Sebastian Hesselbarth wrote:
> Taking the sound node above:
>
> sound {
> 	compatible = "whatever-audio";
> 	...
> 	audio-codec = <&rt1234 1>, <&spdif 0>;
> 	audio-codec-names = "i2s", "spdifo";
> 	...
> };
>
> Each audio-codec phandle with arg links to one specific "port" at some
> "codec". The audio-codec-names property allows the ASoC driver to
> determine the local "ports" where the above codecs are connected.
> The dummy codecs are ASoC specific and must be added by the driver.

I think it's slightly more complicated than that.  Here's what needs to
be setup with DPCM - when it eventually works:

1. Two DAI links need to be setup:
1a:             .name = "S/PDIF1",
                .stream_name = "Audio Playback",
                .platform_name = "mvebu-audio.1",
                .cpu_dai_name = "mvebu-audio.1",
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dynamic = 1,

1b:             .name = "Codec",
                .stream_name = "IEC958 Playback",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_dai_name = "dit-hifi",
                .codec_name = "spdif-dit",

   This separates the CPU DAI from the codec DAI - the important thing
   seems to be that the first entry (which connects the CPU DAI to the
   dummy codec) breaks the automatic DAPM routes between the CPU streams
   and the codec streams.

2. DAPM routes need to be created between the CPU DAI and Codec DAIs to
   wire the back together:

	static const struct snd_soc_dapm_route routes[] = {
	        { "spdif-Playback", NULL, "spdifdo" },
	};

"spdif-Playback" is the stream name in the spdif-dit codec (I had to change
the name from merely "Playback" as otherwise the route gets bound to the
dummy codec instead.)  "spdifdo" is the audio interface widget name in the
CPU DAI, which is connected to the CPU DAI playback stream.

So, if we wanted two codecs, one spdif and one i2s, we would need something
like this:

DAI links:
                .name = "CPU",
                .stream_name = "Audio Playback",
                .platform_name = "mvebu-audio.1",
                .cpu_dai_name = "mvebu-audio.1",
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dynamic = 1,

                .name = "I2S",
                .stream_name = "PCM Playback",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_dai_name = "i2s-codec-dai-name",
                .codec_name = "i2s-codec-name",
                .ops = &..._ops,

                .name = "SPDIF",
                .stream_name = "IEC958 Playback",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_dai_name = "dit-hifi",
                .codec_name = "spdif-dit",

with routes like this:

static const struct snd_soc_dapm_route routes[] = {
	{ "i2sdi", NULL, "i2s-codec-Capture" },
	{ "i2s-codec-Playback", NULL, "i2sdo" },
        { "spdif-Playback", NULL, "spdifdo" },
};

If we were to add support for SPDIF recording mode, then we'd need to add
an additional route for that.

The pitfalls here are:
1. if we are going to refer to codec streams in DAPM routes, we need all
   codec implementations to have unique names, or some way for DAPM routes
   to refer to specific codec streams.

2. being able to effectively represent this in DT.

I think we need a flag in DT to indicate a DPCM configuration, which would
trigger the creation of the first DAI link.  The second and (optionally)
third can be generated from knowing the codec DAI name and the codec
name.  I think we also need a way to specify arbitary DAPM routes in DT
as well.

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Thu, Aug 29, 2013 at 01:01:23PM +0200, Sebastian Hesselbarth wrote:
> Taking the sound node above:
>
> sound {
> 	compatible = "whatever-audio";
> 	...
> 	audio-codec = <&rt1234 1>, <&spdif 0>;
> 	audio-codec-names = "i2s", "spdifo";
> 	...
> };
>
> Each audio-codec phandle with arg links to one specific "port" at some
> "codec". The audio-codec-names property allows the ASoC driver to
> determine the local "ports" where the above codecs are connected.
> The dummy codecs are ASoC specific and must be added by the driver.

I think it's slightly more complicated than that.  Here's what needs to
be setup with DPCM - when it eventually works:

1. Two DAI links need to be setup:
1a:             .name = "S/PDIF1",
                .stream_name = "Audio Playback",
                .platform_name = "mvebu-audio.1",
                .cpu_dai_name = "mvebu-audio.1",
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dynamic = 1,

1b:             .name = "Codec",
                .stream_name = "IEC958 Playback",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_dai_name = "dit-hifi",
                .codec_name = "spdif-dit",

   This separates the CPU DAI from the codec DAI - the important thing
   seems to be that the first entry (which connects the CPU DAI to the
   dummy codec) breaks the automatic DAPM routes between the CPU streams
   and the codec streams.

2. DAPM routes need to be created between the CPU DAI and Codec DAIs to
   wire the back together:

	static const struct snd_soc_dapm_route routes[] = {
	        { "spdif-Playback", NULL, "spdifdo" },
	};

"spdif-Playback" is the stream name in the spdif-dit codec (I had to change
the name from merely "Playback" as otherwise the route gets bound to the
dummy codec instead.)  "spdifdo" is the audio interface widget name in the
CPU DAI, which is connected to the CPU DAI playback stream.

So, if we wanted two codecs, one spdif and one i2s, we would need something
like this:

DAI links:
                .name = "CPU",
                .stream_name = "Audio Playback",
                .platform_name = "mvebu-audio.1",
                .cpu_dai_name = "mvebu-audio.1",
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dynamic = 1,

                .name = "I2S",
                .stream_name = "PCM Playback",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_dai_name = "i2s-codec-dai-name",
                .codec_name = "i2s-codec-name",
                .ops = &..._ops,

                .name = "SPDIF",
                .stream_name = "IEC958 Playback",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_dai_name = "dit-hifi",
                .codec_name = "spdif-dit",

with routes like this:

static const struct snd_soc_dapm_route routes[] = {
	{ "i2sdi", NULL, "i2s-codec-Capture" },
	{ "i2s-codec-Playback", NULL, "i2sdo" },
        { "spdif-Playback", NULL, "spdifdo" },
};

If we were to add support for SPDIF recording mode, then we'd need to add
an additional route for that.

The pitfalls here are:
1. if we are going to refer to codec streams in DAPM routes, we need all
   codec implementations to have unique names, or some way for DAPM routes
   to refer to specific codec streams.

2. being able to effectively represent this in DT.

I think we need a flag in DT to indicate a DPCM configuration, which would
trigger the creation of the first DAI link.  The second and (optionally)
third can be generated from knowing the codec DAI name and the codec
name.  I think we also need a way to specify arbitary DAPM routes in DT
as well.

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Jean-Francois Moine]

On Wed, 28 Aug 2013 13:15:48 +0200
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> For what reason does the driver needs to know whether it's the instance
> 0 or instance 1 ? If it's needed for some specific reason, then there
> should probably be something like marvell,i2s-channel-id = <0> and
> marvell,i2s-channel-id = <1>.

It seems simpler to use aliases.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Jean-Francois Moine]

On Wed, 28 Aug 2013 13:15:48 +0200
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> For what reason does the driver needs to know whether it's the instance
> 0 or instance 1 ? If it's needed for some specific reason, then there
> should probably be something like marvell,i2s-channel-id = <0> and
> marvell,i2s-channel-id = <1>.

It seems simpler to use aliases.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

On Wed, Aug 28, 2013 at 12:26:31PM +0200, Sebastian Hesselbarth wrote:

> Unfortunately, mvebu-audio has already been taken by Mark. Also, we
> know the differences for the three SoCs now and should have a compatible
> for each (and maybe mvebu-audio for fallback).

We're not at the merge window yet so we can always change it.  We can
also add additional compatible strings to the drivers at any point - the
device can list as many as it likes, this is useful if you've got new
controllers which are supersets of old ones for example.

> Also, we'll need to distinguish between the different audio controllers
> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
> base passed.

Why is this required - ideally this would have been mentioned in some of
the previous reviews...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Mark Brown]

On Wed, Aug 28, 2013 at 12:26:31PM +0200, Sebastian Hesselbarth wrote:

> Unfortunately, mvebu-audio has already been taken by Mark. Also, we
> know the differences for the three SoCs now and should have a compatible
> for each (and maybe mvebu-audio for fallback).

We're not at the merge window yet so we can always change it.  We can
also add additional compatible strings to the drivers at any point - the
device can list as many as it likes, this is useful if you've got new
controllers which are supersets of old ones for example.

> Also, we'll need to distinguish between the different audio controllers
> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
> base passed.

Why is this required - ideally this would have been mentioned in some of
the previous reviews...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130829/c7c8411d/attachment-0001.sig>

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Thu, Aug 29, 2013 at 05:12:17PM +0100, Mark Brown wrote:
> Why is this required - ideally this would have been mentioned in some of
> the previous reviews...

I've mentioned the differences between the blocks to you repeatedly in
our massive thread, including that some contain the block with different
configs, but it seems you've been completely closed to everything
technical that I've ever said.

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Thu, Aug 29, 2013 at 05:12:17PM +0100, Mark Brown wrote:
> Why is this required - ideally this would have been mentioned in some of
> the previous reviews...

I've mentioned the differences between the blocks to you repeatedly in
our massive thread, including that some contain the block with different
configs, but it seems you've been completely closed to everything
technical that I've ever said.

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

On Thu, Aug 29, 2013 at 05:33:58PM +0100, Russell King - ARM Linux wrote:
> On Thu, Aug 29, 2013 at 05:12:17PM +0100, Mark Brown wrote:
> > On Wed, Aug 28, 2013 at 12:26:31PM +0200, Sebastian Hesselbarth wrote:

> > > Also, we'll need to distinguish between the different audio controllers
> > > on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
> > > base passed.

> > Why is this required - ideally this would have been mentioned in some of
> > the previous reviews...

> I've mentioned the differences between the blocks to you repeatedly in
> our massive thread, including that some contain the block with different

You have described some additional features which will require
additional driver support.  I would expect that the device tree bindings
for these features would be added as the features are added and the DTS
files updated, for example by listing additional compatible strings if
that was the binding update, as is the normal practice.  Obviously any
hardware which is not compatible with the current binding should not be
being registered using the current binding.

It is not clear from the above comment by Sebastian if he is referring
to the same set of hardware differences or something new - doing things
based on device address is highly unusual, it sounds like something to
do with the integration into the SoC rather than to do with the IP.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Mark Brown]

On Thu, Aug 29, 2013 at 05:33:58PM +0100, Russell King - ARM Linux wrote:
> On Thu, Aug 29, 2013 at 05:12:17PM +0100, Mark Brown wrote:
> > On Wed, Aug 28, 2013 at 12:26:31PM +0200, Sebastian Hesselbarth wrote:

> > > Also, we'll need to distinguish between the different audio controllers
> > > on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
> > > base passed.

> > Why is this required - ideally this would have been mentioned in some of
> > the previous reviews...

> I've mentioned the differences between the blocks to you repeatedly in
> our massive thread, including that some contain the block with different

You have described some additional features which will require
additional driver support.  I would expect that the device tree bindings
for these features would be added as the features are added and the DTS
files updated, for example by listing additional compatible strings if
that was the binding update, as is the normal practice.  Obviously any
hardware which is not compatible with the current binding should not be
being registered using the current binding.

It is not clear from the above comment by Sebastian if he is referring
to the same set of hardware differences or something new - doing things
based on device address is highly unusual, it sounds like something to
do with the integration into the SoC rather than to do with the IP.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130829/45a367c1/attachment.sig>

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/29/13 19:12, Mark Brown wrote:
> On Thu, Aug 29, 2013 at 05:33:58PM +0100, Russell King - ARM Linux wrote:
>> On Thu, Aug 29, 2013 at 05:12:17PM +0100, Mark Brown wrote:
>>> On Wed, Aug 28, 2013 at 12:26:31PM +0200, Sebastian Hesselbarth wrote:
>
>>>> Also, we'll need to distinguish between the different audio controllers
>>>> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
>>>> base passed.
>
>>> Why is this required - ideally this would have been mentioned in some of
>>> the previous reviews...
>
>> I've mentioned the differences between the blocks to you repeatedly in
>> our massive thread, including that some contain the block with different
>
> You have described some additional features which will require
> additional driver support.  I would expect that the device tree bindings
> for these features would be added as the features are added and the DTS
> files updated, for example by listing additional compatible strings if
> that was the binding update, as is the normal practice.  Obviously any
> hardware which is not compatible with the current binding should not be
> being registered using the current binding.
>
> It is not clear from the above comment by Sebastian if he is referring
> to the same set of hardware differences or something new - doing things
> based on device address is highly unusual, it sounds like something to
> do with the integration into the SoC rather than to do with the IP.
>

Mark,

it is referring the same differences Russell already mentioned. But I
already came to the conclusion, that we don't need the information in
the binding. For example, if you use that controller on Dove and you
hook it up for SPDIF-in (which it hasn't), than I consider this a
DT bug. No need to double-check that in the driver. From that p-o-v,
please just let the current binding as is.

Thomas Petazzoni mentioned earlier, that the _usual_ procedure to
name the compatibles is to pick the SoC that the IP appeared in first.
But I am also fine with "marvell,mvebu-audio" and adding compatibles
for dove or kirkwood _if_ we will ever need them.

Please, just stop fighting over this again - it is not getting anything
any further.

Sebastian

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sebastian Hesselbarth]

On 08/29/13 19:12, Mark Brown wrote:
> On Thu, Aug 29, 2013 at 05:33:58PM +0100, Russell King - ARM Linux wrote:
>> On Thu, Aug 29, 2013 at 05:12:17PM +0100, Mark Brown wrote:
>>> On Wed, Aug 28, 2013 at 12:26:31PM +0200, Sebastian Hesselbarth wrote:
>
>>>> Also, we'll need to distinguish between the different audio controllers
>>>> on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg
>>>> base passed.
>
>>> Why is this required - ideally this would have been mentioned in some of
>>> the previous reviews...
>
>> I've mentioned the differences between the blocks to you repeatedly in
>> our massive thread, including that some contain the block with different
>
> You have described some additional features which will require
> additional driver support.  I would expect that the device tree bindings
> for these features would be added as the features are added and the DTS
> files updated, for example by listing additional compatible strings if
> that was the binding update, as is the normal practice.  Obviously any
> hardware which is not compatible with the current binding should not be
> being registered using the current binding.
>
> It is not clear from the above comment by Sebastian if he is referring
> to the same set of hardware differences or something new - doing things
> based on device address is highly unusual, it sounds like something to
> do with the integration into the SoC rather than to do with the IP.
>

Mark,

it is referring the same differences Russell already mentioned. But I
already came to the conclusion, that we don't need the information in
the binding. For example, if you use that controller on Dove and you
hook it up for SPDIF-in (which it hasn't), than I consider this a
DT bug. No need to double-check that in the driver. From that p-o-v,
please just let the current binding as is.

Thomas Petazzoni mentioned earlier, that the _usual_ procedure to
name the compatibles is to pick the SoC that the IP appeared in first.
But I am also fine with "marvell,mvebu-audio" and adding compatibles
for dove or kirkwood _if_ we will ever need them.

Please, just stop fighting over this again - it is not getting anything
any further.

Sebastian

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On Thu, Aug 29, 2013 at 08:02:27PM +0200, Sebastian Hesselbarth wrote:
> 
> it is referring the same differences Russell already mentioned. But I
> already came to the conclusion, that we don't need the information in
> the binding. For example, if you use that controller on Dove and you
> hook it up for SPDIF-in (which it hasn't), than I consider this a
> DT bug. No need to double-check that in the driver. From that p-o-v,
> please just let the current binding as is.

OK, great - none of these devices have any differences which are visible
only within the controller, they're all extra external interfaces?

> Thomas Petazzoni mentioned earlier, that the _usual_ procedure to
> name the compatibles is to pick the SoC that the IP appeared in first.
> But I am also fine with "marvell,mvebu-audio" and adding compatibles
> for dove or kirkwood _if_ we will ever need them.

Yeah, it doesn't make much difference either way so long as the base
name isn't utterly confusing.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Mark Brown]

On Thu, Aug 29, 2013 at 08:02:27PM +0200, Sebastian Hesselbarth wrote:
> 
> it is referring the same differences Russell already mentioned. But I
> already came to the conclusion, that we don't need the information in
> the binding. For example, if you use that controller on Dove and you
> hook it up for SPDIF-in (which it hasn't), than I consider this a
> DT bug. No need to double-check that in the driver. From that p-o-v,
> please just let the current binding as is.

OK, great - none of these devices have any differences which are visible
only within the controller, they're all extra external interfaces?

> Thomas Petazzoni mentioned earlier, that the _usual_ procedure to
> name the compatibles is to pick the SoC that the IP appeared in first.
> But I am also fine with "marvell,mvebu-audio" and adding compatibles
> for dove or kirkwood _if_ we will ever need them.

Yeah, it doesn't make much difference either way so long as the base
name isn't utterly confusing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130829/45a71d28/attachment.sig>

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Thu, Aug 29, 2013 at 07:20:11PM +0100, Mark Brown wrote:
> On Thu, Aug 29, 2013 at 08:02:27PM +0200, Sebastian Hesselbarth wrote:
> > 
> > it is referring the same differences Russell already mentioned. But I
> > already came to the conclusion, that we don't need the information in
> > the binding. For example, if you use that controller on Dove and you
> > hook it up for SPDIF-in (which it hasn't), than I consider this a
> > DT bug. No need to double-check that in the driver. From that p-o-v,
> > please just let the current binding as is.
> 
> OK, great - none of these devices have any differences which are visible
> only within the controller, they're all extra external interfaces?

Dove Audio 0:
	- I2S in
	- I2S out
	- No SPDIF

Dove Audio 1:
	- I2S in
	- I2S out
	- SPDIF out only

Both these variants occur on the same SoC.

Kirkwood:
	- I2S in
	- SPDIF in
	- I2S out
	- SPDIF out

Preconditions:
1. Only one input can be used at any one time.
2. One output can be used or both if enabled simultaneously.

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Russell King - ARM Linux]

On Thu, Aug 29, 2013 at 07:20:11PM +0100, Mark Brown wrote:
> On Thu, Aug 29, 2013 at 08:02:27PM +0200, Sebastian Hesselbarth wrote:
> > 
> > it is referring the same differences Russell already mentioned. But I
> > already came to the conclusion, that we don't need the information in
> > the binding. For example, if you use that controller on Dove and you
> > hook it up for SPDIF-in (which it hasn't), than I consider this a
> > DT bug. No need to double-check that in the driver. From that p-o-v,
> > please just let the current binding as is.
> 
> OK, great - none of these devices have any differences which are visible
> only within the controller, they're all extra external interfaces?

Dove Audio 0:
	- I2S in
	- I2S out
	- No SPDIF

Dove Audio 1:
	- I2S in
	- I2S out
	- SPDIF out only

Both these variants occur on the same SoC.

Kirkwood:
	- I2S in
	- SPDIF in
	- I2S out
	- SPDIF out

Preconditions:
1. Only one input can be used at any one time.
2. One output can be used or both if enabled simultaneously.

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sergei Shtylyov]

Hello.

On 08/28/2013 01:34 PM, Jean-Francois Moine wrote:

> This patch adds the nodes to instantiate the audio devices of the Dove
> boards.

> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>   arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)

> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> index 499abad..78227e2 100644
> --- a/arch/arm/boot/dts/dove.dtsi
> +++ b/arch/arm/boot/dts/dove.dtsi
> @@ -573,6 +573,24 @@
>   					phy-handle = <&ethphy>;
>   				};
>   			};
> +
> +			i2s0: audio-controller@b0000 {

     According to ePAPR [1] the node should be called "sound", not 
"audio-controller".

> +				compatible = "marvell,mvebu-audio";
> +				reg = <0xb0000 0x2210>;
> +				interrupts = <19>, <20>;
> +				clocks = <&gate_clk 12>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};
> +
> +			i2s1: audio-controller@b4000 {

    Same comment.

> +				compatible = "marvell,mvebu-audio";
> +				reg = <0xb4000 0x2210>;
> +				interrupts = <21>, <22>;
> +				clocks = <&gate_clk 13>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};

[1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf

WBR, Sergei

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sergei Shtylyov]

Hello.

On 08/28/2013 01:34 PM, Jean-Francois Moine wrote:

> This patch adds the nodes to instantiate the audio devices of the Dove
> boards.

> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>   arch/arm/boot/dts/dove.dtsi                  | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)

> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> index 499abad..78227e2 100644
> --- a/arch/arm/boot/dts/dove.dtsi
> +++ b/arch/arm/boot/dts/dove.dtsi
> @@ -573,6 +573,24 @@
>   					phy-handle = <&ethphy>;
>   				};
>   			};
> +
> +			i2s0: audio-controller at b0000 {

     According to ePAPR [1] the node should be called "sound", not 
"audio-controller".

> +				compatible = "marvell,mvebu-audio";
> +				reg = <0xb0000 0x2210>;
> +				interrupts = <19>, <20>;
> +				clocks = <&gate_clk 12>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};
> +
> +			i2s1: audio-controller at b4000 {

    Same comment.

> +				compatible = "marvell,mvebu-audio";
> +				reg = <0xb4000 0x2210>;
> +				interrupts = <21>, <22>;
> +				clocks = <&gate_clk 13>;
> +				clock-names = "internal";
> +				status = "disabled";
> +			};

[1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf

WBR, Sergei

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Jean-Francois Moine]

On Wed, 28 Aug 2013 23:49:12 +0400
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:

> > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> > index 499abad..78227e2 100644
> > --- a/arch/arm/boot/dts/dove.dtsi
> > +++ b/arch/arm/boot/dts/dove.dtsi
> > @@ -573,6 +573,24 @@
> >   					phy-handle = <&ethphy>;
> >   				};
> >   			};
> > +
> > +			i2s0: audio-controller@b0000 {  
> 
>      According to ePAPR [1] the node should be called "sound", not 
> "audio-controller".
> 
> > +				compatible = "marvell,mvebu-audio";
> > +				reg = <0xb0000 0x2210>;
> > +				interrupts = <19>, <20>;
> > +				clocks = <&gate_clk 12>;
> > +				clock-names = "internal";
> > +				status = "disabled";
> > +			};

AFAIK, "sound" describes the global audio subsystem. For the Cubox,
this will be done by something like:

	sound {
		compatible = "simple-audio";
		audio-controller = <&i2s1>;
		audio-codec = <&spdif>;
		codec-dai-name = "dit-hifi";
	};

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Jean-Francois Moine]

On Wed, 28 Aug 2013 23:49:12 +0400
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:

> > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> > index 499abad..78227e2 100644
> > --- a/arch/arm/boot/dts/dove.dtsi
> > +++ b/arch/arm/boot/dts/dove.dtsi
> > @@ -573,6 +573,24 @@
> >   					phy-handle = <&ethphy>;
> >   				};
> >   			};
> > +
> > +			i2s0: audio-controller at b0000 {  
> 
>      According to ePAPR [1] the node should be called "sound", not 
> "audio-controller".
> 
> > +				compatible = "marvell,mvebu-audio";
> > +				reg = <0xb0000 0x2210>;
> > +				interrupts = <19>, <20>;
> > +				clocks = <&gate_clk 12>;
> > +				clock-names = "internal";
> > +				status = "disabled";
> > +			};

AFAIK, "sound" describes the global audio subsystem. For the Cubox,
this will be done by something like:

	sound {
		compatible = "simple-audio";
		audio-controller = <&i2s1>;
		audio-codec = <&spdif>;
		codec-dai-name = "dit-hifi";
	};

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sergei Shtylyov]

Hello.

On 29-08-2013 13:38, Jean-Francois Moine wrote:

>>> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
>>> index 499abad..78227e2 100644
>>> --- a/arch/arm/boot/dts/dove.dtsi
>>> +++ b/arch/arm/boot/dts/dove.dtsi
>>> @@ -573,6 +573,24 @@
>>>    					phy-handle = <&ethphy>;
>>>    				};
>>>    			};
>>> +
>>> +			i2s0: audio-controller@b0000 {

>>       According to ePAPR [1] the node should be called "sound", not
>> "audio-controller".

>>> +				compatible = "marvell,mvebu-audio";
>>> +				reg = <0xb0000 0x2210>;
>>> +				interrupts = <19>, <20>;
>>> +				clocks = <&gate_clk 12>;
>>> +				clock-names = "internal";
>>> +				status = "disabled";
>>> +			};

> AFAIK, "sound" describes the global audio subsystem. For the Cubox,
> this will be done by something like:

> 	sound {
> 		compatible = "simple-audio";
> 		audio-controller = <&i2s1>;
> 		audio-codec = <&spdif>;
> 		codec-dai-name = "dit-hifi";
> 	};

    Well, then "sound-controller" sounds somewhat more appropriate.

WBR, Sergei

[PATCH 1/2] ARM: Dove: Add the audio devices in DT

[Sergei Shtylyov]

Hello.

On 29-08-2013 13:38, Jean-Francois Moine wrote:

>>> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
>>> index 499abad..78227e2 100644
>>> --- a/arch/arm/boot/dts/dove.dtsi
>>> +++ b/arch/arm/boot/dts/dove.dtsi
>>> @@ -573,6 +573,24 @@
>>>    					phy-handle = <&ethphy>;
>>>    				};
>>>    			};
>>> +
>>> +			i2s0: audio-controller at b0000 {

>>       According to ePAPR [1] the node should be called "sound", not
>> "audio-controller".

>>> +				compatible = "marvell,mvebu-audio";
>>> +				reg = <0xb0000 0x2210>;
>>> +				interrupts = <19>, <20>;
>>> +				clocks = <&gate_clk 12>;
>>> +				clock-names = "internal";
>>> +				status = "disabled";
>>> +			};

> AFAIK, "sound" describes the global audio subsystem. For the Cubox,
> this will be done by something like:

> 	sound {
> 		compatible = "simple-audio";
> 		audio-controller = <&i2s1>;
> 		audio-codec = <&spdif>;
> 		codec-dai-name = "dit-hifi";
> 	};

    Well, then "sound-controller" sounds somewhat more appropriate.

WBR, Sergei