Patchwork [2/2] ARM: kirkwood: extend the kirkwood i2s driver for DT usage

login
register
mail settings
Submitter Jean-Francois Moine
Date March 26, 2013, 6:05 p.m.
Message ID <20130326190539.494d96d9@armhf>
Download mbox | patch
Permalink /patch/231506/
State New
Headers show

Comments

Jean-Francois Moine - March 26, 2013, 6:05 p.m.
The kirkwood i2s 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     |   24 +++++++++++++
 sound/soc/kirkwood/kirkwood-i2s.c                  |   36 ++++++++++++++++++--
 2 files changed, 58 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
Andrew Lunn - March 26, 2013, 6:47 p.m.
On Tue, Mar 26, 2013 at 07:05:39PM +0100, Jean-Francois Moine wrote:
> The kirkwood i2s 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     |   24 +++++++++++++
>  sound/soc/kirkwood/kirkwood-i2s.c                  |   36 ++++++++++++++++++--
>  2 files changed, 58 insertions(+), 2 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..a089504
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
> @@ -0,0 +1,24 @@
> +* Kirkwood I2S controller
> +
> +Required properties:
> +
> +- compatible: "marvell,kirkwood-i2s"

Hi Jean-Francois

We should maybe change this to marvell,mvebu-i2s. Kirkwood, Dove, and
370 have i2s. We know Dove and Kirkwood are compatible, and its likely
370 is also compatible. So a renaming will avoid confusion in the long
run. We can leave the filename of the driver alone for the moment,
since that is not an ABI.

> +- 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: phandle of the internal or external clock.
> +
> +Non-standard property:

There does not appear to be an i2s standard, so saying a property is
non-standard is a bit odd. I would just say Optional Properties, and
drop the (optional) below.

> +
> +- marvell,burst-size (optional): burst size which can be only 32 or 128.
> +  Default is 128.
> +
> +Example:
> +
> +i2s1: audio-controller@b4000 {
> +	compatible = "marvell,kirkwood-i2s";
> +	reg = <0xb4000 0x4000>;

I've not looked, but 0x4000 seem rather large for an register range.

> +	interrupts = <21>, <22>;
> +	clocks = <&gate_clk 13>;
> +};
> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> index afca1ec..dceb9f8 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"
> @@ -485,10 +486,30 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
>  		return -ENXIO;
>  	}
>  
> +#ifdef CONFIG_OF
> +	if (!data) {
> +		struct device_node *np = pdev->dev.of_node;
> +		u32 val;
> +
> +		if (!np
> +		 || of_property_read_u32(np, "marvell,burst-size", &val)) {

It is normal to put the || on the previous line.

> +			val = 128;
> +		} else if (val != 32 && val != 128) {
> +			dev_warn(&pdev->dev,
> +				"'marvell,burst-size' can be only 32 or 128"
> +				 "- value forced to 128\n");

I would prefer to return EINVAL, or some other error code. Invalid DT
contents will get noticed faster that way.

> +			val = 128;
> +		}
> +		priv->burst = val;
> +	} else {
> +		priv->burst = data->burst;
> +	}
> +#else
>  	if (!data) {
>  		dev_err(&pdev->dev, "no platform data ?!\n");
>  		return -EINVAL;
>  	}
> +#endif
>  
>  	priv->burst = data->burst;
>  
> @@ -519,7 +540,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 +577,23 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static struct of_device_id kirkwood_i2s_of_match[] = {
> +	{ .compatible = "marvell,kirkwood-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,
> +#ifdef CONFIG_OF
> +		.of_match_table??=??kirkwood_i2s_of_match,
> +#endif

You should be able to skip all these #ifdefs.

    Andrew
Sebastian Hesselbarth - March 26, 2013, 7:37 p.m.
On 03/26/2013 07:05 PM, Jean-Francois Moine wrote:
> The kirkwood i2s 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     |   24 +++++++++++++
>   sound/soc/kirkwood/kirkwood-i2s.c                  |   36 ++++++++++++++++++--
>   2 files changed, 58 insertions(+), 2 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..a089504
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
> @@ -0,0 +1,24 @@
> +* Kirkwood I2S controller
> +
> +Required properties:
> +
> +- compatible: "marvell,kirkwood-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: phandle of the internal or external clock.

Jean-Francois,

have you tested the driver with external clock set instead of internal clock?

I guess it will hang on access, as internal clock comes from a clock gate that
needs to be enabled prior use of i2s core. I suggest to have two clocks, 0 for
internal and optional (1) for external clock. You can get them with
of_clk_get(np, 0) and of_clk_get(np, 1) respectively.

> +
> +Non-standard property:
> +
> +- marvell,burst-size (optional): burst size which can be only 32 or 128.
> +  Default is 128.

Do we really want to expose this to the device node? If this is set to different
values for kirkwood, dove, and a370, we can still have it set by corresponding
compatible strings and match data. (See below)

> +
> +Example:
> +
> +i2s1: audio-controller@b4000 {
> +	compatible = "marvell,kirkwood-i2s";
> +	reg =<0xb4000 0x4000>;
> +	interrupts =<21>,<22>;
> +	clocks =<&gate_clk 13>;
> +};
> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> index afca1ec..dceb9f8 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"
> @@ -485,10 +486,30 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
>   		return -ENXIO;
>   	}
>
> +#ifdef CONFIG_OF
> +	if (!data) {

You can remove the #ifdef and check for pdev->dev.of_node instead. All of_xx functions
should have no-op counterparts if CONFIG_OF is not set.

> +		struct device_node *np = pdev->dev.of_node;
> +		u32 val;
> +
> +		if (!np
> +		 || of_property_read_u32(np, "marvell,burst-size",&val)) {
> +			val = 128;
> +		} else if (val != 32&&  val != 128) {
> +			dev_warn(&pdev->dev,
> +				"'marvell,burst-size' can be only 32 or 128"
> +				 "- value forced to 128\n");
> +			val = 128;
> +		}

If you initialize val with 128 you can just use of_property_read as it will
only modify val on success. You will have to check for valid values of course later on.

> +		priv->burst = val;
> +	} else {
> +		priv->burst = data->burst;
> +	}
> +#else
>   	if (!data) {
>   		dev_err(&pdev->dev, "no platform data ?!\n");
>   		return -EINVAL;
>   	}
> +#endif
>
>   	priv->burst = data->burst;
>
> @@ -519,7 +540,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 +577,23 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev)
>   	return 0;
>   }
>
> +#ifdef CONFIG_OF
> +static struct of_device_id kirkwood_i2s_of_match[] = {
> +	{ .compatible = "marvell,kirkwood-i2s" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, kirkwood_i2s_of_match);
> +#endif

If there is SoC specific data (e.g. burst_size) you can use
.data to pass it according to the compatible string:

static struct of_device_id kirkwood_i2s_of_match[] = {
	{ .compatible = "marvell,kirkwood-i2s", .data = (void *)32 },
	{ .compatible = "marvell,dove-i2s", .data = (void *)128 },
	{ .compatible = "marvell,armada-370-i2s", .data = (void *)128 },
	{ }
};

Or have some SoC specific struct i2s_soc_data passed with .data,
if there is more than one value you need to pass.

> +
>   static struct platform_driver kirkwood_i2s_driver = {
>   	.probe  = kirkwood_i2s_dev_probe,
>   	.remove = kirkwood_i2s_dev_remove,
>   	.driver = {
>   		.name = DRV_NAME,
>   		.owner = THIS_MODULE,
> +#ifdef CONFIG_OF
> +		.of_match_table = kirkwood_i2s_of_match,
> +#endif

Remove the $ifdef and have .of_match_table = of_match_ptr(kirkwood_i2s_of_match),

>   	},
>   };
>

Sebastian
Jean-Francois Moine - March 26, 2013, 7:54 p.m.
On Tue, 26 Mar 2013 19:47:58 +0100
Andrew Lunn <andrew@lunn.ch> wrote:
	[snip]
> > diff --git a/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
> > new file mode 100644
> > index 0000000..a089504
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
> > @@ -0,0 +1,24 @@
> > +* Kirkwood I2S controller
> > +
> > +Required properties:
> > +
> > +- compatible: "marvell,kirkwood-i2s"
> 
> Hi Jean-Francois
> 
> We should maybe change this to marvell,mvebu-i2s. Kirkwood, Dove, and
> 370 have i2s. We know Dove and Kirkwood are compatible, and its likely
> 370 is also compatible. So a renaming will avoid confusion in the long
> run. We can leave the filename of the driver alone for the moment,
> since that is not an ABI.

OK. I will do an other version of the dove DT extension.

> > +- 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: phandle of the internal or external clock.
> > +
> > +Non-standard property:
> 
> There does not appear to be an i2s standard, so saying a property is
> non-standard is a bit odd. I would just say Optional Properties, and
> drop the (optional) below.

No problem. I just did a copy/yank from an other binding.

> > +
> > +- marvell,burst-size (optional): burst size which can be only 32 or 128.
> > +  Default is 128.
> > +
> > +Example:
> > +
> > +i2s1: audio-controller@b4000 {
> > +	compatible = "marvell,kirkwood-i2s";
> > +	reg = <0xb4000 0x4000>;
> 
> I've not looked, but 0x4000 seem rather large for an register range.

I got the value from kirkwood/common.c (SZ_16K). But, yes, in the
88AP510 doc, it seems that the last register is 2208. I will put 2210.

> > +	interrupts = <21>, <22>;
> > +	clocks = <&gate_clk 13>;
> > +};
> > diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> > index afca1ec..dceb9f8 100644
> > --- a/sound/soc/kirkwood/kirkwood-i2s.c
> > +++ b/sound/soc/kirkwood/kirkwood-i2s.c
	[snip]
> > @@ -485,10 +486,30 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
> >  		return -ENXIO;
> >  	}
> >  
> > +#ifdef CONFIG_OF
> > +	if (!data) {
> > +		struct device_node *np = pdev->dev.of_node;
> > +		u32 val;
> > +
> > +		if (!np
> > +		 || of_property_read_u32(np, "marvell,burst-size", &val)) {
> 
> It is normal to put the || on the previous line.

I find || and && are clearer on the next line when the condition is
somewhat complex. I may change that, but I did not see any constraint
in the CodingStyle...

> > +			val = 128;
> > +		} else if (val != 32 && val != 128) {
> > +			dev_warn(&pdev->dev,
> > +				"'marvell,burst-size' can be only 32 or 128"
> > +				 "- value forced to 128\n");
> 
> I would prefer to return EINVAL, or some other error code. Invalid DT
> contents will get noticed faster that way.

OK.

	[snip]
> > @@ -556,12 +577,23 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_OF
> > +static struct of_device_id kirkwood_i2s_of_match[] = {
> > +	{ .compatible = "marvell,kirkwood-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,
> > +#ifdef CONFIG_OF
> > +		.of_match_table??=??kirkwood_i2s_of_match,
> > +#endif
> 
> You should be able to skip all these #ifdefs.

OK.
Jean-Francois Moine - March 27, 2013, 10:21 a.m.
On Tue, 26 Mar 2013 20:37:34 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> On 03/26/2013 07:05 PM, Jean-Francois Moine wrote:
> > The kirkwood i2s 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>
> > ---
	[snip]
> > diff --git a/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
> > new file mode 100644
> > index 0000000..a089504
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
> > @@ -0,0 +1,24 @@
> > +* Kirkwood I2S controller
> > +
> > +Required properties:
> > +
> > +- compatible: "marvell,kirkwood-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: phandle of the internal or external clock.
> 
> Jean-Francois,
> 
> have you tested the driver with external clock set instead of internal clock?

Hi Sebastian,

No, I don't know yet how to install and start the audio subsystem.

> I guess it will hang on access, as internal clock comes from a clock gate that
> needs to be enabled prior use of i2s core. I suggest to have two clocks, 0 for
> internal and optional (1) for external clock. You can get them with
> of_clk_get(np, 0) and of_clk_get(np, 1) respectively.

You are right.

Trying to code this, I came across an other (but surely known) problem:
the synchronization of the module initialization.

If you want to use some external clock given by, say, the si5351
module, as all modules are loaded at the same time, the module
kirkwood-i2s may be initialized before the si5351 and, so, the clock
will not be seen as available.

Question: is there a module synchronization mechanism for the DT?
(late_initcall does not work for modules)

There is an other problem, tied to the si5351 module unload: how can
the kirkwood-i2s know that the clock driver is unloaded, or how can it
do to lock this driver? (the module of the clock driver is not seen
from the clock user)

A response to this problem could simply be: increase the use count of
the clock module (try_module_get) when a clock is used (clk prepare or
enable).

> > +
> > +Non-standard property:
> > +
> > +- marvell,burst-size (optional): burst size which can be only 32 or 128.
> > +  Default is 128.
> 
> Do we really want to expose this to the device node? If this is set to different
> values for kirkwood, dove, and a370, we can still have it set by corresponding
> compatible strings and match data. (See below)

	[snip]
> > diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> > index afca1ec..dceb9f8 100644
> > --- a/sound/soc/kirkwood/kirkwood-i2s.c
> > +++ b/sound/soc/kirkwood/kirkwood-i2s.c
	[snip]
> > @@ -556,12 +577,23 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev)
> >   	return 0;
> >   }
> >
> > +#ifdef CONFIG_OF
> > +static struct of_device_id kirkwood_i2s_of_match[] = {
> > +	{ .compatible = "marvell,kirkwood-i2s" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, kirkwood_i2s_of_match);
> > +#endif
> 
> If there is SoC specific data (e.g. burst_size) you can use
> .data to pass it according to the compatible string:
> 
> static struct of_device_id kirkwood_i2s_of_match[] = {
> 	{ .compatible = "marvell,kirkwood-i2s", .data = (void *)32 },
> 	{ .compatible = "marvell,dove-i2s", .data = (void *)128 },
> 	{ .compatible = "marvell,armada-370-i2s", .data = (void *)128 },
> 	{ }
> };

I don't like this solution: the burst value is hidden and hard coded.
If a new machine uses this same driver, it is easier to change the DT
instead of recompiling a whole kernel.

> Or have some SoC specific struct i2s_soc_data passed with .data,
> if there is more than one value you need to pass.
> 
> > +
> >   static struct platform_driver kirkwood_i2s_driver = {
> >   	.probe  = kirkwood_i2s_dev_probe,
> >   	.remove = kirkwood_i2s_dev_remove,
> >   	.driver = {
> >   		.name = DRV_NAME,
> >   		.owner = THIS_MODULE,
> > +#ifdef CONFIG_OF
> > +		.of_match_table = kirkwood_i2s_of_match,
> > +#endif
> 
> Remove the $ifdef and have .of_match_table = of_match_ptr(kirkwood_i2s_of_match),
> 
> >   	},
> >   };
> >
> 
> Sebastian

OK.
Andrew Lunn - March 27, 2013, 12:01 p.m.
> If you want to use some external clock given by, say, the si5351
> module, as all modules are loaded at the same time, the module
> kirkwood-i2s may be initialized before the si5351 and, so, the clock
> will not be seen as available.

If the DT says the clock should exist, and of_clk_get() fails, return
-EDEFER in the probe. The kernel will try again later once more
modules have been loaded.

> > If there is SoC specific data (e.g. burst_size) you can use
> > .data to pass it according to the compatible string:
> > 
> > static struct of_device_id kirkwood_i2s_of_match[] = {
> > 	{ .compatible = "marvell,kirkwood-i2s", .data = (void *)32 },
> > 	{ .compatible = "marvell,dove-i2s", .data = (void *)128 },
> > 	{ .compatible = "marvell,armada-370-i2s", .data = (void *)128 },
> > 	{ }
> > };
> 
> I don't like this solution: the burst value is hidden and hard coded.
> If a new machine uses this same driver, it is easier to change the DT
> instead of recompiling a whole kernel.

If its a new SoC family, its not a change to DT, its a whole new DT,
for that SoC family. There is no inheritance between SoC
families. There is no sharing between dove, kirkwood, 370 for .dts or
dtsi files. As far as i can see, burst is a SoC family property. So
will not be expected to change from board to board, which is the
typical use case for DT.

	Andrew
Jean-Francois Moine - March 27, 2013, 12:18 p.m.
OOn Wed, 27 Mar 2013 13:01:19 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > If you want to use some external clock given by, say, the si5351
> > module, as all modules are loaded at the same time, the module
> > kirkwood-i2s may be initialized before the si5351 and, so, the clock
> > will not be seen as available.
> 
> If the DT says the clock should exist, and of_clk_get() fails, return
> -EDEFER in the probe. The kernel will try again later once more
> modules have been loaded.

Hi Andrew,

I will try that.

> > > If there is SoC specific data (e.g. burst_size) you can use
> > > .data to pass it according to the compatible string:
> > > 
> > > static struct of_device_id kirkwood_i2s_of_match[] = {
> > > 	{ .compatible = "marvell,kirkwood-i2s", .data = (void *)32 },
> > > 	{ .compatible = "marvell,dove-i2s", .data = (void *)128 },
> > > 	{ .compatible = "marvell,armada-370-i2s", .data = (void *)128 },
> > > 	{ }
> > > };
> > 
> > I don't like this solution: the burst value is hidden and hard coded.
> > If a new machine uses this same driver, it is easier to change the DT
> > instead of recompiling a whole kernel.
> 
> If its a new SoC family, its not a change to DT, its a whole new DT,
> for that SoC family. There is no inheritance between SoC
> families. There is no sharing between dove, kirkwood, 370 for .dts or
> dtsi files. As far as i can see, burst is a SoC family property. So
> will not be expected to change from board to board, which is the
> typical use case for DT.

Maybe I did not explain clearly.

Suppose Marvell creates a new machine, say "toto", which includes the
same audio controller. In its DT, there would be something as
"marvell,toto-i2s". So, to use it, you should add a line:

	{ .compatible = "marvell,toto-i2s", .data = (void *)128 },

in the kirkwood-i2s.c.

While, if the 'burst' value is defined in the DT, there would be no
change in this file kirkwood-i2s.c, and, as you proposed, the
of_device_id would be simply:

	{ .compatible = "marvell,mvebu-i2s" },

Patch

diff --git a/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
new file mode 100644
index 0000000..a089504
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt
@@ -0,0 +1,24 @@ 
+* Kirkwood I2S controller
+
+Required properties:
+
+- compatible: "marvell,kirkwood-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: phandle of the internal or external clock.
+
+Non-standard property:
+
+- marvell,burst-size (optional): burst size which can be only 32 or 128.
+  Default is 128.
+
+Example:
+
+i2s1: audio-controller@b4000 {
+	compatible = "marvell,kirkwood-i2s";
+	reg = <0xb4000 0x4000>;
+	interrupts = <21>, <22>;
+	clocks = <&gate_clk 13>;
+};
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index afca1ec..dceb9f8 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"
@@ -485,10 +486,30 @@  static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
+#ifdef CONFIG_OF
+	if (!data) {
+		struct device_node *np = pdev->dev.of_node;
+		u32 val;
+
+		if (!np
+		 || of_property_read_u32(np, "marvell,burst-size", &val)) {
+			val = 128;
+		} else if (val != 32 && val != 128) {
+			dev_warn(&pdev->dev,
+				"'marvell,burst-size' can be only 32 or 128"
+				 "- value forced to 128\n");
+			val = 128;
+		}
+		priv->burst = val;
+	} else {
+		priv->burst = data->burst;
+	}
+#else
 	if (!data) {
 		dev_err(&pdev->dev, "no platform data ?!\n");
 		return -EINVAL;
 	}
+#endif
 
 	priv->burst = data->burst;
 
@@ -519,7 +540,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 +577,23 @@  static int kirkwood_i2s_dev_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id kirkwood_i2s_of_match[] = {
+	{ .compatible = "marvell,kirkwood-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,
+#ifdef CONFIG_OF
+		.of_match_table = kirkwood_i2s_of_match,
+#endif
 	},
 };