Re: [alsa-devel] [PATCH v3] ARM: kirkwood: extend the kirkwood i2s driver for DT usage

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

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