linux-kernel - Re: [PATCH 2/2] ASoC: Adds support for TAS575x to the pcm512x driver

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: <ZPn4nsypsSXdB3J7@ubuntu>
Date:   Thu, 7 Sep 2023 18:21:50 +0200
From:   Joerg Schambacher <joerg.hifiberry@...il.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     a-krasser@...com, joerg@...iberry.com,
        Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Zhang Qilong <zhangqilong3@...wei.com>,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] ASoC: Adds support for TAS575x to the pcm512x driver

Am 07.09.2023 um 15:21 hat Mark Brown geschrieben:
> On Thu, Sep 07, 2023 at 06:12:05PM +0200, Joerg Schambacher wrote:
> 
> > +		if (pcm512x->tas_device) {
> > +			/* set necessary PLL coeffs for amp
> > +			 * according to spec only 2x and 4x MCLKs
> > +			 * possible
> > +			 */
> > +			ret = regmap_write(pcm512x->regmap,
> > +					   PCM512x_PLL_COEFF_0, 0x01);
> > +			if (mck_rate > 25000000UL)
> > +				ret = regmap_write(pcm512x->regmap,
> > +						   PCM512x_PLL_COEFF_1, 0x02);
> > +			else
> > +				ret = regmap_write(pcm512x->regmap,
> > +						   PCM512x_PLL_COEFF_1, 0x04);
> 
> I would name this quirk something a bit more specific - it seems likely
> that there might be future TASxxxx devices which need different quirks.
> If it's a limited range of MCLK multipliers perhaps something about how
> the PLL is limited, or just make the quirk data being to specify min/max
> for the multiplier?
The spec allows a maximum input Clk of 50MHz. Useful for the concerned
mode are only clocks with 22.5792/45.1584MHz for the 44.1ksps family rates
and 24.576/49.152MHz for the 48ksps. The only thing we need
to make sure is to divide the master clock by 2 in case of a MCLK input
of >25MHz to use the the same settings hereafter.
And yes, we cannot be sure that future devices may require different
settings, but I can hardly imagine anything completely different than
this approach with the usual audio MCLK frequencies.
> 
> > +		if (!pcm512x->tas_device) {
> > +			ret = regmap_update_bits(pcm512x->regmap,
> > +						 PCM512x_PLL_EN, PCM512x_PLLE, 0);
> > +		} else {
> > +			/* leave PLL enabled for amp clocking */
> > +			ret = regmap_write(pcm512x->regmap,
> > +					   PCM512x_PLL_EN, 0x01);
> > +			dev_dbg(component->dev, "Enabling PLL for TAS575x\n");
> > +		}
> 
> This is probably a separate quirk?  I'm a bit concerned about what's
> turning the PLL off here...
The PLL should not be used in this specific case where all divisions are
just divide-by-2's. Your original code does reflect that and turns the
PLL off. It improves jitter and finally audio performance.
But in the case of the TAS-devices we even then need the PLL to
drive the AMP clocks.
My code changes only address the case 'TAS-device and external audio MCLK'.
All other constellations like clock slave mode or e.g. 12MHz MCLK input
require the PLL anyhow. THe 12MHz clock input even requires the FLEX mode
through the PLL using the GPIOs as PLL-in and -out.
I hope I could clarify things a bit.



--