[PATCH 4/4] ASoc: kirkwood: add DT support

[PATCH 4/4] ASoc: kirkwood: add DT support

[Jean-Francois Moine]

The kirkwood audio driver is used without DT in the Kirkwood machine.
This patch adds a DT compatible definition for use in other Marvell
machines as the Armada 88AP510 (Dove).

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../devicetree/bindings/sound/kirkwood-i2s.txt     | 29 ++++++++++++++++++
 sound/soc/kirkwood/kirkwood-i2s.c                  | 34 +++++++++++++++-------
 2 files changed, 52 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/kirkwood-i2s.txt

diff --git a/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
new file mode 100644
index 0000000..015a769
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
@@ -0,0 +1,29 @@
+* mvebu (Kirkwood, Dove, Armada 370) I2S controller
+
+Required properties:
+
+- compatible: "marvell,mvebu-i2s"
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- interrupts: list of two irq numbers.
+  The first irq is used for data flow and the second one is used for errors.
+
+- clocks: one or two phandles.
+  The first one is mandatory and defines the internal clock.
+  The second one is optional and defines an external clock.
+
+- clock-names: names associated to the clocks:
+	"internal" for the internal clock
+	"external" for the external clock
+
+Example:
+
+i2s1: audio-controller@b4000 {
+	compatible = "marvell,mvebu-i2s";
+	reg = <0xb4000 0x2210>;
+	interrupts = <21>, <22>;
+	clocks = <&gate_clk 13>;
+	clock-names = "internal";
+};
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 3517784..fbe5150 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -12,7 +12,6 @@
 
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/mbus.h>
@@ -22,6 +21,8 @@
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <linux/platform_data/asoc-kirkwood.h>
+#include <linux/of.h>
+
 #include "kirkwood.h"
 
 #define DRV_NAME	"kirkwood-i2s"
@@ -497,16 +498,18 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	if (!data) {
-		dev_err(&pdev->dev, "no platform data ?!\n");
-		return -EINVAL;
-	}
-
-	priv->burst = data->burst;
+	if (np) {
+		priv->burst = 128;		/* might be 32 or 128 */
+	} else if (data) {
+		priv->burst = data->burst;
+	} else {
+		dev_err(&pdev->dev, "no DT nor platform data ?!\n");
+ 		return -EINVAL;
+ 	}
 
-	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	priv->clk = devm_clk_get(&pdev->dev, "internal");
 	if (IS_ERR(priv->clk)) {
-		dev_err(&pdev->dev, "no clock\n");
+		dev_err(&pdev->dev, "no internal clock\n");
 		return PTR_ERR(priv->clk);
 	}
 
@@ -514,7 +517,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	if (err < 0)
 		return err;
 
-	priv->extclk = clk_get(&pdev->dev, "extclk");
+	priv->extclk = clk_get(&pdev->dev, "external");
 	if (!IS_ERR(priv->extclk)) {
 		if (priv->extclk == priv->clk) {
 			clk_put(priv->extclk);
@@ -531,7 +534,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	priv->ctl_rec = KIRKWOOD_RECCTL_SIZE_24;
 
 	/* Select the burst size */
-	if (data->burst == 32) {
+	if (priv->burst == 32) {
 		priv->ctl_play |= KIRKWOOD_PLAYCTL_BURST_32;
 		priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_32;
 	} else {
@@ -556,12 +559,21 @@ fail:
 	return err;
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id kirkwood_i2s_of_match[] = {
+	{ .compatible = "marvell,mvebu-i2s" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, kirkwood_i2s_of_match);
+#endif
+
 static struct platform_driver kirkwood_i2s_driver = {
 	.probe  = kirkwood_i2s_dev_probe,
 	.remove = kirkwood_i2s_dev_remove,
 	.driver = {
 		.name = DRV_NAME,
 		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(kirkwood_i2s_of_match),
 	},
 };
 
-- 
1.8.3.2



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

Re: [PATCH 4/4] ASoc: kirkwood: add DT support

[Mark Brown]

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

On Thu, Jul 25, 2013 at 11:14:59AM +0200, Jean-Francois Moine wrote:

> +	if (np) {
> +		priv->burst = 128;		/* might be 32 or 128 */
> +	} else if (data) {

When you posted this before I queried how and why the value might vary -
I see the code is the same and I don't recall a reply.

> -	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	priv->clk = devm_clk_get(&pdev->dev, "internal");
>  	if (IS_ERR(priv->clk)) {
> -		dev_err(&pdev->dev, "no clock\n");
> +		dev_err(&pdev->dev, "no internal clock\n");
>  		return PTR_ERR(priv->clk);
>  	}

Does the code providing it already name the clock?  If not are updates
needed to do that?

> -	priv->extclk = clk_get(&pdev->dev, "extclk");
> +	priv->extclk = clk_get(&pdev->dev, "external");

Is the clock actually called extclk in the datasheet and so on?  If so
it seems better to stick with that name.  Do any boards need updates for
the new name?

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

Re: [PATCH 4/4] ASoc: kirkwood: add DT support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 898 bytes --]

On Thu, Jul 25, 2013 at 11:14:59AM +0200, Jean-Francois Moine wrote:

> +	if (np) {
> +		priv->burst = 128;		/* might be 32 or 128 */
> +	} else if (data) {

When you posted this before I queried how and why the value might vary -
I see the code is the same and I don't recall a reply.

> -	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	priv->clk = devm_clk_get(&pdev->dev, "internal");
>  	if (IS_ERR(priv->clk)) {
> -		dev_err(&pdev->dev, "no clock\n");
> +		dev_err(&pdev->dev, "no internal clock\n");
>  		return PTR_ERR(priv->clk);
>  	}

Does the code providing it already name the clock?  If not are updates
needed to do that?

> -	priv->extclk = clk_get(&pdev->dev, "extclk");
> +	priv->extclk = clk_get(&pdev->dev, "external");

Is the clock actually called extclk in the datasheet and so on?  If so
it seems better to stick with that name.  Do any boards need updates for
the new name?

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 4/4] ASoc: kirkwood: add DT support

[Russell King - ARM Linux]

On Thu, Jul 25, 2013 at 08:19:05PM +0100, Mark Brown wrote:
> On Thu, Jul 25, 2013 at 11:14:59AM +0200, Jean-Francois Moine wrote:
> 
> > +	if (np) {
> > +		priv->burst = 128;		/* might be 32 or 128 */
> > +	} else if (data) {
> 
> When you posted this before I queried how and why the value might vary -
> I see the code is the same and I don't recall a reply.

This is the DMA burst size, and can be either 32 or 128 bytes according
to the docs.  Everyone seems to pass this as 128 bytes in their platform
data to date, which I guess is why its ended up being hard coded as 128.

However, whether it needs to be configurable or not is debatable - obviously
the hardware allows it, but that doesn't mean it has to be exposed.  If
ALSA has some kind of way of specifying a "low latency" mode where 128
byte vs 32 byte fetches would make a significant difference, then it may
be something to look at.

I'll retract my previous comment on this (about it being a DT property) -
given what it does, it isn't describing the hardware capabilities as I
first thought it was.

> > -	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > +	priv->clk = devm_clk_get(&pdev->dev, "internal");
> >  	if (IS_ERR(priv->clk)) {
> > -		dev_err(&pdev->dev, "no clock\n");
> > +		dev_err(&pdev->dev, "no internal clock\n");
> >  		return PTR_ERR(priv->clk);
> >  	}
> 
> Does the code providing it already name the clock?  If not are updates
> needed to do that?
> 
> > -	priv->extclk = clk_get(&pdev->dev, "extclk");
> > +	priv->extclk = clk_get(&pdev->dev, "external");
> 
> Is the clock actually called extclk in the datasheet and so on?  If so
> it seems better to stick with that name.  Do any boards need updates for
> the new name?

"AU_EXTCLK" is the exact name (pasted out of the documentation).
I don't see any purpose to this name changing.

Re: [PATCH 4/4] ASoc: kirkwood: add DT support

[Russell King - ARM Linux]

On Thu, Jul 25, 2013 at 08:19:05PM +0100, Mark Brown wrote:
> On Thu, Jul 25, 2013 at 11:14:59AM +0200, Jean-Francois Moine wrote:
> 
> > +	if (np) {
> > +		priv->burst = 128;		/* might be 32 or 128 */
> > +	} else if (data) {
> 
> When you posted this before I queried how and why the value might vary -
> I see the code is the same and I don't recall a reply.

This is the DMA burst size, and can be either 32 or 128 bytes according
to the docs.  Everyone seems to pass this as 128 bytes in their platform
data to date, which I guess is why its ended up being hard coded as 128.

However, whether it needs to be configurable or not is debatable - obviously
the hardware allows it, but that doesn't mean it has to be exposed.  If
ALSA has some kind of way of specifying a "low latency" mode where 128
byte vs 32 byte fetches would make a significant difference, then it may
be something to look at.

I'll retract my previous comment on this (about it being a DT property) -
given what it does, it isn't describing the hardware capabilities as I
first thought it was.

> > -	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > +	priv->clk = devm_clk_get(&pdev->dev, "internal");
> >  	if (IS_ERR(priv->clk)) {
> > -		dev_err(&pdev->dev, "no clock\n");
> > +		dev_err(&pdev->dev, "no internal clock\n");
> >  		return PTR_ERR(priv->clk);
> >  	}
> 
> Does the code providing it already name the clock?  If not are updates
> needed to do that?
> 
> > -	priv->extclk = clk_get(&pdev->dev, "extclk");
> > +	priv->extclk = clk_get(&pdev->dev, "external");
> 
> Is the clock actually called extclk in the datasheet and so on?  If so
> it seems better to stick with that name.  Do any boards need updates for
> the new name?

"AU_EXTCLK" is the exact name (pasted out of the documentation).
I don't see any purpose to this name changing.

Re: [PATCH 4/4] ASoc: kirkwood: add DT support

[Jean-Francois Moine]

On Fri, 26 Jul 2013 00:05:33 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Jul 25, 2013 at 08:19:05PM +0100, Mark Brown wrote:
> > On Thu, Jul 25, 2013 at 11:14:59AM +0200, Jean-Francois Moine wrote:
	[snip]
> > > -	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > > +	priv->clk = devm_clk_get(&pdev->dev, "internal");
> > >  	if (IS_ERR(priv->clk)) {
> > > -		dev_err(&pdev->dev, "no clock\n");
> > > +		dev_err(&pdev->dev, "no internal clock\n");
> > >  		return PTR_ERR(priv->clk);
> > >  	}
> > 
> > Does the code providing it already name the clock?  If not are updates
> > needed to do that?

I don't know. I will reset the clock name to NULL when no DT, so it
will be compatible.

> > > -	priv->extclk = clk_get(&pdev->dev, "extclk");
> > > +	priv->extclk = clk_get(&pdev->dev, "external");
> > 
> > Is the clock actually called extclk in the datasheet and so on?  If so
> > it seems better to stick with that name.  Do any boards need updates for
> > the new name?
> 
> "AU_EXTCLK" is the exact name (pasted out of the documentation).
> I don't see any purpose to this name changing.

On Tue, Jul 23, 2013 at 02:59:06PM +0200, Sebastian Hesselbarth wrote:
> As Sascha Hauer pointed out, clocks should be distinguished by names
> (clock-names property) instead of position and then use
> devm_clk_get(&pdev->dev, "internal") and
> devm_clk_get(&pdev->dev, "external") respectively.  
> 
> This will possibly also require to update platform_data and legacy
> users of kirkwood-i2s or have different setup functions for non-DT
> and DT.  

The A510 documentation uses the names "DCO PLL" for the internal clock
and "AU_EXTCLK" for the external clock. So, what about "dcopll" and
"extclk"?


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

Re: [PATCH 4/4] ASoc: kirkwood: add DT support

[Russell King - ARM Linux]

On Fri, Jul 26, 2013 at 11:09:13AM +0200, Jean-Francois Moine wrote:
> The A510 documentation uses the names "DCO PLL" for the internal clock
> and "AU_EXTCLK" for the external clock. So, what about "dcopll" and
> "extclk"?

Stop naming them according to their source.  Their _consumer_ names
not _source_ names.

Re: [PATCH 4/4] ASoc: kirkwood: add DT support

[Russell King - ARM Linux]

On Fri, Jul 26, 2013 at 11:09:13AM +0200, Jean-Francois Moine wrote:
> The A510 documentation uses the names "DCO PLL" for the internal clock
> and "AU_EXTCLK" for the external clock. So, what about "dcopll" and
> "extclk"?

Stop naming them according to their source.  Their _consumer_ names
not _source_ names.

Re: [PATCH 4/4] ASoc: kirkwood: add DT support

[Jean-Francois Moine]

On Fri, 26 Jul 2013 10:21:56 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Fri, Jul 26, 2013 at 11:09:13AM +0200, Jean-Francois Moine wrote:
> > The A510 documentation uses the names "DCO PLL" for the internal clock
> > and "AU_EXTCLK" for the external clock. So, what about "dcopll" and
> > "extclk"?
> 
> Stop naming them according to their source.  Their _consumer_ names
> not _source_ names.

I have no preference. What do you propose?

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

Re: [PATCH 4/4] ASoc: kirkwood: add DT support

[Mark Brown]

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

On Fri, Jul 26, 2013 at 12:05:33AM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 25, 2013 at 08:19:05PM +0100, Mark Brown wrote:
> > On Thu, Jul 25, 2013 at 11:14:59AM +0200, Jean-Francois Moine wrote:

> > > +	if (np) {
> > > +		priv->burst = 128;		/* might be 32 or 128 */
> > > +	} else if (data) {

> > When you posted this before I queried how and why the value might vary -
> > I see the code is the same and I don't recall a reply.

> This is the DMA burst size, and can be either 32 or 128 bytes according
> to the docs.  Everyone seems to pass this as 128 bytes in their platform
> data to date, which I guess is why its ended up being hard coded as 128.

OK.

> However, whether it needs to be configurable or not is debatable - obviously
> the hardware allows it, but that doesn't mean it has to be exposed.  If
> ALSA has some kind of way of specifying a "low latency" mode where 128
> byte vs 32 byte fetches would make a significant difference, then it may
> be something to look at.

This sort of configuration would normally be keyed off the period size
and so vary at runtime depending on what the application does when it
opens a stream.  Sounds like something someone can worry about if they
have the requirement.

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