[alsa-devel] [PATCH 4/4] ASoc: kirkwood: add DT support
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), }, };
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");
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?
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");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.
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");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"?
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.
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?
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.
participants (3)
-
Jean-Francois Moine -
Mark Brown -
Russell King - ARM Linux