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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 23 Jul 2013 14:59:06 +0200
From:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
To:	Mark Brown <broonie@...nel.org>
CC:	Jean-Francois Moine <moinejf@...e.fr>, alsa-devel@...a-project.org,
	Takashi Iwai <tiwai@...e.de>, linux-kernel@...r.kernel.org,
	Liam Girdwood <lgirdwood@...il.com>,
	Rob Herring <rob.herring@...xeda.com>,
	devicetree-discuss@...r.kernel.org,
	Russell King <rmk+kernel@....linux.org.uk>
Subject: Re: [alsa-devel] [PATCH v3] ARM: kirkwood: extend the kirkwood i2s
 driver for DT usage

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@...00 {
	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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/