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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

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?

> +		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...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] ASoC: Adds support for TAS575x to the pcm512x driver

[Joerg Schambacher]

Enables the existing pcm512x driver to control the almost
compatible TAS5754 and -76 amplifers. Both amplifiers support
only an I2C interface and the internal PLL must be always
on to provide necessary clocks to the amplifier section.
Tested on TAS5756 with support from Andreas Arbesser-Krasser
from Texas Instruments <a-krasser@ti.com>

Signed-off-by: Joerg Schambacher <joerg.hifiberry@gmail.com>
---
 sound/soc/codecs/pcm512x-i2c.c |  4 ++++
 sound/soc/codecs/pcm512x.c     | 34 +++++++++++++++++++++++++++++++---
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/pcm512x-i2c.c b/sound/soc/codecs/pcm512x-i2c.c
index 5cd2b64b9337..4be476a280e1 100644
--- a/sound/soc/codecs/pcm512x-i2c.c
+++ b/sound/soc/codecs/pcm512x-i2c.c
@@ -39,6 +39,8 @@ static const struct i2c_device_id pcm512x_i2c_id[] = {
 	{ "pcm5122", },
 	{ "pcm5141", },
 	{ "pcm5142", },
+	{ "tas5754", },
+	{ "tas5756", },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pcm512x_i2c_id);
@@ -49,6 +51,8 @@ static const struct of_device_id pcm512x_of_match[] = {
 	{ .compatible = "ti,pcm5122", },
 	{ .compatible = "ti,pcm5141", },
 	{ .compatible = "ti,pcm5142", },
+	{ .compatible = "ti,tas5754", },
+	{ .compatible = "ti,tas5756", },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pcm512x_of_match);
diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 89059a673cf0..9aa9be2dbdb2 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -48,6 +48,7 @@ struct pcm512x_priv {
 	int mute;
 	struct mutex mutex;
 	unsigned int bclk_ratio;
+	int tas_device;
 };
 
 /*
@@ -927,6 +928,20 @@ static int pcm512x_set_dividers(struct snd_soc_dai *dai,
 		bclk_div = DIV_ROUND_CLOSEST(sck_rate, bclk_rate);
 
 		mck_rate = sck_rate;
+		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);
+		}
 	} else {
 		ret = snd_soc_params_to_bclk(params);
 		if (ret < 0) {
@@ -1258,10 +1273,18 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream,
 			return ret;
 		}
 
-		ret = regmap_update_bits(pcm512x->regmap, PCM512x_PLL_EN,
-					 PCM512x_PLLE, 0);
+		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");
+		}
+
 		if (ret != 0) {
-			dev_err(component->dev, "Failed to disable pll: %d\n", ret);
+			dev_err(component->dev, "Failed to set pll mode: %d\n", ret);
 			return ret;
 		}
 	}
@@ -1659,6 +1682,11 @@ int pcm512x_probe(struct device *dev, struct regmap *regmap)
 			ret = -EINVAL;
 			goto err_pm;
 		}
+
+		if (!strcmp(np->name, "tas5756") ||
+		    !strcmp(np->name, "tas5754"))
+			pcm512x->tas_device = 1;
+		dev_dbg(dev, "Device ID: %s\n", np->name);
 	}
 #endif
 
-- 
2.34.1

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

[Joerg Schambacher]

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.



-- 

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

On Thu, Sep 07, 2023 at 06:21:50PM +0200, Joerg Schambacher wrote:

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

They may for example be restricted and just not the the incoming MCLK
divider, or require a higher frequency for some fancy processing.  In
any case tas_device is just an obviously bad name here - there should be
a flag per quirk, not a flag per entire class of devices.

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

That's definitely a separate quirk, and does sound like it should be
driven from the bias management or DAPM more than hw_params.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Joerg Schambacher]

Am 07.09.2023 um 17:28 hat Mark Brown geschrieben:
> On Thu, Sep 07, 2023 at 06:21:50PM +0200, Joerg Schambacher wrote:
> 
> > 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.
> 
> They may for example be restricted and just not the the incoming MCLK
> divider, or require a higher frequency for some fancy processing.  In
> any case tas_device is just an obviously bad name here - there should be
> a flag per quirk, not a flag per entire class of devices.
> 
Ok, I see your point and completely agree. I tend for now to leave that
part out of the patch. This leaves the PLL divider programmings
completely 'untouched'. Nevertheless, I'll continue with testing here
on the hardware.
> > > 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.
> 
> That's definitely a separate quirk, and does sound like it should be
> driven from the bias management or DAPM more than hw_params.

Then it makes sense to use a DT-param 'force_pll_on' and even
remove the compatible string fixes from the patch series.
Still, I think, this is the best part of the code to act on the PLL.
Simply instead of checking 'do we need it or not' just let it run.
What do you think?


-- 

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Mon, Sep 11, 2023 at 01:59:46PM +0200, Joerg Schambacher wrote:
> Am 07.09.2023 um 17:28 hat Mark Brown geschrieben:
> > On Thu, Sep 07, 2023 at 06:21:50PM +0200, Joerg Schambacher wrote:

> > > But in the case of the TAS-devices we even then need the PLL to
> > > drive the AMP clocks.

> > That's definitely a separate quirk, and does sound like it should be
> > driven from the bias management or DAPM more than hw_params.

> Then it makes sense to use a DT-param 'force_pll_on' and even
> remove the compatible string fixes from the patch series.

If this device always needs the PLL then we should just figure it out
from the compatible rather than requiring a DT property which every
system with the device is going to need to set.

> Still, I think, this is the best part of the code to act on the PLL.
> Simply instead of checking 'do we need it or not' just let it run.
> What do you think?

It's probably fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]