linux-kernel - Re: [PATCH v2 1/2] ARM: tegra: apalis/colibri t30: integrate audio support

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]

Message-ID: <5410C222.2020602@wwwdotorg.org>
Date:	Wed, 10 Sep 2014 15:26:58 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Marcel Ziswiler <marcel@...wiler.com>, alsa-devel@...a-project.org
CC:	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>, linux@....linux.org.uk,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
	stefan@...er.ch, Fabio Estevam <festevam@...il.com>
Subject: Re: [PATCH v2 1/2] ARM: tegra: apalis/colibri t30: integrate audio
 support

On 09/10/2014 02:54 PM, Marcel Ziswiler wrote:
> Integrate Freescale SGTL5000 analogue audio codec support.
>
> Signed-off-by: Marcel Ziswiler <marcel@...wiler.com>
> Reviewed-by: Stephen Warren <swarren@...dia.com>

No, definitely not; this patch has significant semantic changes since I 
reviewed it.

> diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi b/arch/arm/boot/dts/tegra30-apalis.dtsi

> +	sound {
> +		compatible = "simple-audio-card";
...
> +		simple-audio-card,codec {
> +			bitclock-master;
> +			clocks = <&tegra_car TEGRA30_CLK_EXTERN1>;
> +			frame-master;
> +			sound-dai = <&sgtl5000>;
> +		};
> +
> +		simple-audio-card,cpu {
> +			bitclock-master;
> +			clocks = <&tegra_car TEGRA30_CLK_PLL_A>,
> +				 <&tegra_car TEGRA30_CLK_PLL_A_OUT0>;
> +			frame-master;
> +			master-clkdir-out;
> +			sound-dai = <&tegra_i2s2>;
> +		};
> +	};

I'm not sure how this can work. Certainly all 3 clocks that are required 
for audio are mentioned here. However, the PLL_A clock (which then 
trickles down to the other 2) needs to have its rate changed based on 
whether the sample rate is 44.1KHz- or 48KHz-based. Semantically, this 
can't be something that "simple-audio-card" can imply, since 
"simple-audio-card" is something agnostic to any HW, whereas this 
clocking requirement is something specific to Tegra HW.

To see the problem, try encoding some audio stream as both 44.1KHz and 
48KHz, then play a few seconds of each and keep switching back and 
forth. You'll likely find either the playback pitch is wrong, or you get 
drop-outs or stuttering.

I also wonder how "simple-audio-card" can know what rate the EXTERN1 
clock to the CODEC should be set to; each CODEC has a different 
requirement for the minimum clock input and the Fs multiple usually 
varies for different sample rates. See for example 
sound/soc/tegra/tegra_wm8903.c:tegra_wm8903_hw_params(). This could be a 
solved problem though: Perhaps some API has been added to CODECs to 
report this information (or perhaps CODEC drivers now clk_set_rate() on 
their input clocks themselves) since I last looked.

--
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/