lkml.org 
[lkml]   [2015]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Apr 15, 2015 at 02:42:20PM -0700, Kevin Cernekee wrote:
>> +static int tas571x_set_sysclk(struct snd_soc_dai *dai,
>> + int clk_id, unsigned int freq, int dir)
>> +{
>> + /*
>> + * TAS5717 datasheet pg 21: "The DAP can autodetect and set the
>> + * internal clock-control logic to the appropriate settings for all
>> + * supported clock rates as defined in the clock control register."
>> + */
>> + return 0;
>> +}
>
> Remove empty functions, at best they waste space at worst they break
> things.

Without the empty function, we run into problems with drivers that
abort when they get -ENOTSUPP here:

sound/soc/atmel/atmel_wm8904.c: ret =
snd_soc_dai_set_sysclk(codec_dai, WM8904_CLK_FLL,
sound/soc/atmel/atmel_wm8904.c- 0, SND_SOC_CLOCK_IN);
sound/soc/atmel/atmel_wm8904.c- if (ret < 0) {
sound/soc/atmel/atmel_wm8904.c- pr_err("%s -failed to set
wm8904 SYSCLK\n", __func__);
sound/soc/atmel/atmel_wm8904.c- return ret;
sound/soc/atmel/atmel_wm8904.c- }

Is there a stub version that I can use instead? Nothing jumped out at
me when looking at the other codec drivers.

>> +static int tas571x_set_shutdown(struct tas571x_private *priv, bool is_shutdown)
>> +{
>> + return regmap_update_bits(priv->regmap, TAS571X_SYS_CTRL_2_REG,
>> + TAS571X_SYS_CTRL_2_SDN_MASK,
>> + is_shutdown ? TAS571X_SYS_CTRL_2_SDN_MASK : 0);
>> +}
>
>> + ret = tas571x_set_shutdown(priv, false);
>> + if (ret)
>> + return ret;
>> + break;
>> + case SND_SOC_BIAS_STANDBY:
>> + ret = tas571x_set_shutdown(priv, true);
>> + if (ret)
>> + return ret;
>
> This looks like it'd be clearer just as direct register updates, I'm not
> sure a function to set a single bit is addinng much.

It might be useful if another tas571x variant put the bit somewhere
else, but that hasn't happened yet so I can nuke the helper function
for now.

>> + case SND_SOC_BIAS_OFF:
>> + /* Note that this kills I2C accesses. */
>> + assert_pdn = 1;
>
> No, the GPIO set associated with it kills I2C access. I'd also expect
> to see the regmap being marked cache only before we do this and a resync
> of the register map when we power back up (assuming that is actually a
> power down).

Hmm, not sure if this actually resets the registers back to power-on
defaults, but I'll check.

>> + /*
>> + * The master volume defaults to 0x3ff (mute), but we ignore
>> + * (zero) the LSB because the hardware step size is 0.125 dB
>> + * and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB.
>> + */
>> + if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe))
>> + return -EIO;
>
> I don't understand this - is the LSB a mute bit or sommething?

The 10-bit master volume field on 5717/5719 works like:

0x3ff: MUTE (power-on default)
0x3fe: -103.750 dB
0x3fd: -103.625 dB
[lots more options, in 0.125 dB increments]
0x001: 23.875 dB
0x000: 24.000 dB

Since we only have a resolution of 0.01 dB, the driver forces the LSB
to 0 and uses 0.25 dB increments instead of 0.125 dB. Mute is handled
through the dedicated per-channel soft mute register bits instead of
the 0x3ff volume setting.


\
 
 \ /
  Last update: 2015-04-18 18:21    [W:0.110 / U:0.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site