[alsa-devel] [PATCH v3] ARM: kirkwood: extend the kirkwood i2s driver for DT usage
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 --- v3 - change compatible from kirkwood to mvebu (Andrew Lunn) v2 - this patch replaces the previous [PATCH 2/2] ARM: kirkwood: extend the kirkwood i2s driver for DT usage - 2 possible clocks (Sebastian Hesselbarth) - shorter io mapping (Andrew Lunn) - less #ifdef's --- Documentation/devicetree/bindings/sound/kirkwood-i2s.txt | 24 +++++++++++ sound/soc/kirkwood/kirkwood-i2s.c | 58 ++++++++++++++++++++------ 2 files changed, 70 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt new file mode 100644 index 0000000..8f1e534 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/kirkwood-i2s.txt @@ -0,0 +1,24 @@ +* 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. + +Example: + +i2s1: audio-controller@b4000 { + compatible = "marvell,mvebu-i2s"; + reg = <0xb4000 0x2210>; + interrupts = <21>, <22>; + clocks = <&gate_clk 13>; +}; diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 4c9dad3..019e340 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" @@ -461,6 +462,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) struct snd_soc_dai_driver *soc_dai = &kirkwood_i2s_dai; struct kirkwood_dma_data *priv; struct resource *mem; + struct device_node *np; int err;
priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); @@ -481,24 +483,45 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) return -ENXIO; }
- if (!data) { - dev_err(&pdev->dev, "no platform data ?!\n"); - return -EINVAL; + /* get the DT or static parameters */ + np = pdev->dev.of_node; + if (np) { + struct of_phandle_args clkspec; + + priv->burst = 128; /* might be 32 or 128 */ + priv->clk = of_clk_get(np, 0); /* internal clock */ + err = of_parse_phandle_with_args(np, + "clocks", "#clock-cells", 1, + &clkspec); + if (err) { + priv->extclk = ERR_PTR(-EINVAL); /* no external clock */ + } else { + priv->extclk = of_clk_get(np, 1); + if (IS_ERR(priv->extclk)) { + err = -EPROBE_DEFER; + goto fail; + } + } + } else { + if (!data) { + dev_err(&pdev->dev, "no platform data ?!\n"); + return -EINVAL; + } + priv->burst = data->burst; + priv->clk = clk_get(&pdev->dev, NULL); + priv->extclk = clk_get(&pdev->dev, "extclk"); }
- priv->burst = data->burst; - - priv->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(priv->clk)) { dev_err(&pdev->dev, "no clock\n"); - return PTR_ERR(priv->clk); + err = PTR_ERR(priv->clk); + goto fail; }
err = clk_prepare_enable(priv->clk); if (err < 0) - return err; + goto fail;
- priv->extclk = clk_get(&pdev->dev, "extclk"); if (!IS_ERR(priv->extclk)) { if (priv->extclk == priv->clk) { clk_put(priv->extclk); @@ -515,7 +538,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 { @@ -528,12 +551,13 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) if (!err) return 0; dev_err(&pdev->dev, "snd_soc_register_component failed\n"); - +fail: if (!IS_ERR(priv->extclk)) { clk_disable_unprepare(priv->extclk); clk_put(priv->extclk); } clk_disable_unprepare(priv->clk); + clk_put(priv->clk);
return err; } @@ -549,16 +573,26 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev) clk_put(priv->extclk); } clk_disable_unprepare(priv->clk); + clk_put(priv->clk);
return 0; }
+#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 Tue, Jul 23, 2013 at 10:46:15AM +0200, 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).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of devm_clk_get() to of_clk_get() necessitates a clk_put().
Can someone please provide a devm_of_clk_get() function before we accept any more patches doing this kind of thing?
The other issue is that kirkwood-dma.c and kirkwood-i2s.c should be one driver; they're not separate hardware on these platforms, so that needs fixing before coming up with any kind of DT model for this stuff.
On 07/23/2013 10:53 AM, Russell King - ARM Linux wrote:
On Tue, Jul 23, 2013 at 10:46:15AM +0200, 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).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of devm_clk_get() to of_clk_get() necessitates a clk_put().
That's just a very poor usage of the API in this patch. devm_clk_get() works fine on DT and non-DT platforms. The driver shouldn't need any changes in that regard.
- Lars
On Tue, Jul 23, 2013 at 09:53:46AM +0100, Russell King - ARM Linux wrote:
On Tue, Jul 23, 2013 at 10:46:15AM +0200, 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).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of devm_clk_get() to of_clk_get() necessitates a clk_put().
Can someone please provide a devm_of_clk_get() function before we accept any more patches doing this kind of thing?
The question is why isn't regular [devm_]clk_get used here? I don't know what the Marvell stuff is doing here, but if the clk stuff is implemented correctly then
np = pdev->dev.of_node; priv->clk = of_clk_get(np, 0);
and
devm_clk_get(&pdev->dev, "label");
should both work
Sascha
On Tue, Jul 23, 2013 at 11:08:20AM +0200, Sascha Hauer wrote:
On Tue, Jul 23, 2013 at 09:53:46AM +0100, Russell King - ARM Linux wrote:
On Tue, Jul 23, 2013 at 10:46:15AM +0200, 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).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of devm_clk_get() to of_clk_get() necessitates a clk_put().
Can someone please provide a devm_of_clk_get() function before we accept any more patches doing this kind of thing?
The question is why isn't regular [devm_]clk_get used here? I don't know what the Marvell stuff is doing here, but if the clk stuff is implemented correctly then
np = pdev->dev.of_node; priv->clk = of_clk_get(np, 0);
and
devm_clk_get(&pdev->dev, "label");
should both work
You tell me - I keep seeing patches on this list converting drivers from using devm_clk_get() to using of_clk_get(). Maybe of_clk_get() should be marked deprecated to stop people using it?
On Tue, Jul 23, 2013 at 10:39:50AM +0100, Russell King - ARM Linux wrote:
On Tue, Jul 23, 2013 at 11:08:20AM +0200, Sascha Hauer wrote:
On Tue, Jul 23, 2013 at 09:53:46AM +0100, Russell King - ARM Linux wrote:
On Tue, Jul 23, 2013 at 10:46:15AM +0200, 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).
Yet again, this illustrates why converting to DT causes backwards steps in drivers: the conversion of devm_clk_get() to of_clk_get() necessitates a clk_put().
Can someone please provide a devm_of_clk_get() function before we accept any more patches doing this kind of thing?
The question is why isn't regular [devm_]clk_get used here? I don't know what the Marvell stuff is doing here, but if the clk stuff is implemented correctly then
np = pdev->dev.of_node; priv->clk = of_clk_get(np, 0);
and
devm_clk_get(&pdev->dev, "label");
should both work
You tell me - I keep seeing patches on this list converting drivers from using devm_clk_get() to using of_clk_get(). Maybe of_clk_get() should be marked deprecated to stop people using it?
I don't think that's possible. There are some valid usecases for of_clk_get() when there's no struct device * available, like in many clocksource drivers.
Sascha
On Tue, Jul 23, 2013 at 10:46:15AM +0200, Jean-Francois Moine wrote:
- np = pdev->dev.of_node;
- if (np) {
struct of_phandle_args clkspec;priv->burst = 128; /* might be 32 or 128 */
The comment says this needs to be variable (depending on what?) but it's hard coded.
priv->clk = of_clk_get(np, 0); /* internal clock */err = of_parse_phandle_with_args(np,"clocks", "#clock-cells", 1,&clkspec);
As others have pointed out if you need to change the clock get code there's something wrong here, DT should be handled transparently by the clock API.
On 07/23/13 14:34, Mark Brown wrote:
On Tue, Jul 23, 2013 at 10:46:15AM +0200, Jean-Francois Moine wrote:
- np = pdev->dev.of_node;
- if (np) {
struct of_phandle_args clkspec;priv->burst = 128; /* might be 32 or 128 */The comment says this needs to be variable (depending on what?) but it's hard coded.
priv->clk = of_clk_get(np, 0); /* internal clock */err = of_parse_phandle_with_args(np,"clocks", "#clock-cells", 1,&clkspec);As others have pointed out if you need to change the clock get code there's something wrong here, DT should be handled transparently by the clock API.
IMHO the reason why of_clk_get() was/is mis-used in that way is mostly compatibility with legacy platform_data based setup.
Kirkwood-i2s never knew about anything else than internal clock, then Dove allows additional external clock input, aso. All changes are incremental and more or less sane. But now is a good opportunity to clean up this.
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.
Also, while ASoC API separates the audio-controller into cpu-side and codec-side parts, the DT should not. IIRC and as Russell repeated again, we mentioned to merge kirkwood-i2s.c and kirkwood-dma.c into a single file, didn't we?
I know we didn't care that much in the past, but one last thing that I catched while reading another thead about compatible strings:
We should really be more careful about those. The correct usage of compatible strings should be "marvell,mvebu-i2s" as common fallback, but also "marvell,dove-i2s" and "marvell,kirkwood-i2s" for the SoC dtsi files. We do not need to have all possible compatible strings in the _driver's_ of_device_id table but the dtsi should contain them.
Finally, I2S DT node will end up as:
i2s1: audio-controller@b4000 { compatible = "marvell,dove-i2s", "marvell,mvebu-i2s"; reg = <0xb4000 0x2210>; interrupts = <21>, <22>; clocks = <&gate_clk 13>, <&si5351a 1>; clock-names = "internal", "external"; };
Jean-Francois, can you re-spin your patches with the comments made by others and the above summary? I really like to see i2s for DT soon, although we will not be able to support multiple codecs per DAI, yet.
Sebastian
On Tue, Jul 23, 2013 at 02:59:06PM +0200, Sebastian Hesselbarth wrote:
On 07/23/13 14:34, Mark Brown wrote:
As others have pointed out if you need to change the clock get code there's something wrong here, DT should be handled transparently by the clock API.
IMHO the reason why of_clk_get() was/is mis-used in that way is mostly compatibility with legacy platform_data based setup.
I'm sorry, but this doesn't make a great deal of sense to me. Can you be more specific?
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.
Why would this be required? The driver is already asking for multiple clocks...
Also, while ASoC API separates the audio-controller into cpu-side and codec-side parts, the DT should not. IIRC and as Russell repeated
You mean DAI and DMA here? I already commented on that in my review of the DMA binding.
again, we mentioned to merge kirkwood-i2s.c and kirkwood-dma.c into a single file, didn't we?
That's been discussed several times but nobody's actually done it.
On 07/23/13 15:20, Mark Brown wrote:
On Tue, Jul 23, 2013 at 02:59:06PM +0200, Sebastian Hesselbarth wrote:
On 07/23/13 14:34, Mark Brown wrote:
As others have pointed out if you need to change the clock get code there's something wrong here, DT should be handled transparently by the clock API.
IMHO the reason why of_clk_get() was/is mis-used in that way is mostly compatibility with legacy platform_data based setup.
I'm sorry, but this doesn't make a great deal of sense to me. Can you be more specific?
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.
Why would this be required? The driver is already asking for multiple clocks...
The driver is asking for multiple *DT based* clocks. Legacy platform_data has never been updated to reflect that. Mainly because multiple clocks are only supported on Dove, which has no active non-DT board in mainline.
Also, while ASoC API separates the audio-controller into cpu-side and codec-side parts, the DT should not. IIRC and as Russell repeated
You mean DAI and DMA here? I already commented on that in my review of the DMA binding.
Yes.
again, we mentioned to merge kirkwood-i2s.c and kirkwood-dma.c into a single file, didn't we?
That's been discussed several times but nobody's actually done it.
Correct, that is why I repeated that request to Jean-Francois.
Sebastian
On Tue, Jul 23, 2013 at 03:30:57PM +0200, Sebastian Hesselbarth wrote:
On 07/23/13 15:20, Mark Brown wrote:
again, we mentioned to merge kirkwood-i2s.c and kirkwood-dma.c into a single file, didn't we?
That's been discussed several times but nobody's actually done it.
Correct, that is why I repeated that request to Jean-Francois.
On that, I have some initial patches which start doing that, but I've not had time to really complete the effort - the problem is working with one tree (mainline) while testing on Rabeeh's kernel tree to get a fully working Cubox platform is quite a hinderance.
Not only that, but the lack of assistance from ASoC people on the multiple substream stuff really doesn't help one iota when it comes to making forward progress with SPDIF support. I've explained to them the problem (SPDIF and I2S if used together must be enabled simultaneously or only one is permitted at any one time); they've suggested using the multiple PCM substream support, and then it's a case that "oh it's undocumented" "oh there's no examples yet".
And then there's the problem of understanding the ASoC DAPM stuff "oh it's just a graph walk, you don't need to know the internals" is what I get from ASoC people. Well, yes I do need to know the internals, so I can find out whether it's going to startup a second substream while a first one is active, and end up with SPDIF enabled non-concurrently with I2S.
On Tue, Jul 23, 2013 at 02:50:00PM +0100, Russell King - ARM Linux wrote:
Not only that, but the lack of assistance from ASoC people on the multiple substream stuff really doesn't help one iota when it comes to making forward progress with SPDIF support. I've explained to them the
It'd probably help if the people looking at this hardware started to contribute some of the work that's being done here; this should help move things forward by looking at concrete code rather than the very general discussion that's happening at the minute.
problem (SPDIF and I2S if used together must be enabled simultaneously or only one is permitted at any one time); they've suggested using the multiple PCM substream support, and then it's a case that "oh it's undocumented" "oh there's no examples yet".
As previously mentioned the OMAP4 and Qualcomm stuff is there, it's just out of tree - there's also Haswell but I'm not sure if that's been made public yet (Liam?).
And then there's the problem of understanding the ASoC DAPM stuff "oh it's just a graph walk, you don't need to know the internals" is what I get from ASoC people. Well, yes I do need to know the internals, so I can find out whether it's going to startup a second substream while a first one is active, and end up with SPDIF enabled non-concurrently with I2S.
As ever the answer is that DAPM will enable any connected audio path and not enable other ones so it is possible to enable only one of multiple paths (as you'd expect, really).
Can you be more specific about the problems you're having following the code? It should be relatively simple to trace through what's going on from dapm_power_widgets()...
On Tue, Jul 23, 2013 at 03:30:57PM +0200, Sebastian Hesselbarth wrote:
On 07/23/13 15:20, Mark Brown wrote:
Why would this be required? The driver is already asking for multiple clocks...
The driver is asking for multiple *DT based* clocks. Legacy platform_data has never been updated to reflect that. Mainly because multiple clocks are only supported on Dove, which has no active non-DT board in mainline.
Why would platform data have anything to do with this? To repeat again the way the clocks are mapped should be totally transparent to the driver requesting them, if it isn't then the driver is not using the API properly.
On Tue, Jul 23, 2013 at 04:01:50PM +0100, Mark Brown wrote:
On Tue, Jul 23, 2013 at 03:30:57PM +0200, Sebastian Hesselbarth wrote:
On 07/23/13 15:20, Mark Brown wrote:
Why would this be required? The driver is already asking for multiple clocks...
The driver is asking for multiple *DT based* clocks. Legacy platform_data has never been updated to reflect that. Mainly because multiple clocks are only supported on Dove, which has no active non-DT board in mainline.
Why would platform data have anything to do with this? To repeat again the way the clocks are mapped should be totally transparent to the driver requesting them, if it isn't then the driver is not using the API properly.
Total rubbish. Of course the driver needs to know what the clocks are, so that it can program its hardware accordingly.
What you have here is an audio block which has two clock inputs. One clock is the system clock, whose rate can be adjusted but has a massive impact on the rest of the system. The other clock input is via an external pin.
Internally, in one of the audio blocks registers is a set of control bits which select which clock is to be used to generate the internal timing for the block, and a divisor on that input.
From that description, anyone can see that it is absolutely required for
the driver to know which clock is which, so it can program the clock input selection bit appropriately.
In this case, it has always been the rule with the clock API that it shall be used as:
clk_get(device, "internal");
to get the internal clock, and:
clk_get(device, "external");
to get the external clock - or whatever names are appropriate to name the clock _inputs_.
On Tue, Jul 23, 2013 at 04:19:56PM +0100, Russell King - ARM Linux wrote:
On Tue, Jul 23, 2013 at 04:01:50PM +0100, Mark Brown wrote:
Why would platform data have anything to do with this? To repeat again the way the clocks are mapped should be totally transparent to the driver requesting them, if it isn't then the driver is not using the API properly.
Total rubbish. Of course the driver needs to know what the clocks are, so that it can program its hardware accordingly.
Of course, but why through platform data? Though reading your mail I think we're talking at cross purposes here...
In this case, it has always been the rule with the clock API that it shall be used as:
clk_get(device, "internal");
to get the internal clock, and:
clk_get(device, "external");
to get the external clock - or whatever names are appropriate to name the clock _inputs_.
This is exactly what I'd expect, yes - but it doesn't need the driver to have platform data as far as I can see. As you say the driver should be requesting the two clock inputs the IP has by name which doesn't need it to have any platform data, it just needs code like you have above. The board specific configuration is all handled within the clk_get() calls.
On Tue, 23 Jul 2013 15:30:57 +0200 Sebastian Hesselbarth sebastian.hesselbarth@gmail.com wrote:
again, we mentioned to merge kirkwood-i2s.c and kirkwood-dma.c into a single file, didn't we?
That's been discussed several times but nobody's actually done it.
Correct, that is why I repeated that request to Jean-Francois.
Thanks for all your remarks.
I am testing a new version, with mainly: - "internal" and "external" clocks - kirkwood-i2s and kirkwood-dma in one module - no S/PDIF (I have not yet tried i2s with the tda998x)
participants (6)
-
Jean-Francois Moine -
Lars-Peter Clausen -
Mark Brown -
Russell King - ARM Linux -
Sascha Hauer -
Sebastian Hesselbarth