[PATCH 2/2] ASoC: Adds support for TAS575x to the pcm512x driver
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
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);elseret = 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...
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);elseret = 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.
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.
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?
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.
participants (2)
-
Joerg Schambacher -
Mark Brown