Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Fri, Apr 24, 2015 at 06:52:01AM -0700, Kevin Cernekee wrote: > On Fri, Apr 24, 2015 at 2:28 AM, Mark Brown wrote: > > Do you need to work around it? If the register map is being perserved > > you don't need to sync so just don't do it - it's just that the normal > > expectation would be that power down would cause the register map to be > > reset. > How do I tell regcache to write out any updates that happened while > the hardware was inaccessible? I see that regmap->cache_dirty is 1, > but nothing flushes it automatically when exiting cache_only mode. Oh, I see. A sync will be required, yes. Probably resetting the device is easiest. signature.asc Description: Digital signature
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Fri, Apr 24, 2015 at 2:28 AM, Mark Brown wrote: > On Thu, Apr 23, 2015 at 05:47:49PM -0700, Kevin Cernekee wrote: > >> This is mostly working OK, but regcache_sync() assumes that the >> hardware registers have been reset back to the default values. The >> "pdn" GPIO doesn't actually reset the state of the tas571x; it just >> makes I2C inaccessible and inhibits audio output. So if the factory >> default for mute is 0, corner cases like this fail: > > ... > >> Aside from unnecessarily pulsing the reset GPIO when transitioning >> back from SND_SOC_BIAS_OFF or overriding regcache_default_sync(), can >> you think of a way to work around this? > > Do you need to work around it? If the register map is being perserved > you don't need to sync so just don't do it - it's just that the normal > expectation would be that power down would cause the register map to be > reset. How do I tell regcache to write out any updates that happened while the hardware was inaccessible? I see that regmap->cache_dirty is 1, but nothing flushes it automatically when exiting cache_only mode. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Thu, Apr 23, 2015 at 05:47:49PM -0700, Kevin Cernekee wrote: > This is mostly working OK, but regcache_sync() assumes that the > hardware registers have been reset back to the default values. The > "pdn" GPIO doesn't actually reset the state of the tas571x; it just > makes I2C inaccessible and inhibits audio output. So if the factory > default for mute is 0, corner cases like this fail: ... > Aside from unnecessarily pulsing the reset GPIO when transitioning > back from SND_SOC_BIAS_OFF or overriding regcache_default_sync(), can > you think of a way to work around this? Do you need to work around it? If the register map is being perserved you don't need to sync so just don't do it - it's just that the normal expectation would be that power down would cause the register map to be reset. signature.asc Description: Digital signature
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Sat, Apr 18, 2015 at 9:16 AM, Kevin Cernekee wrote: >>> + 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. Hi Mark, I have reworked the driver to do the following: - set appropriate regmap default values on probe - enable idle_bias_off - use regcache_cache_only() to prevent accesses to I2C when in SND_SOC_BIAS_OFF state (pdn asserted) - use regcache_sync() when transitioning from SND_SOC_BIAS_OFF -> SND_SOC_BIAS_STANDBY This is mostly working OK, but regcache_sync() assumes that the hardware registers have been reset back to the default values. The "pdn" GPIO doesn't actually reset the state of the tas571x; it just makes I2C inaccessible and inhibits audio output. So if the factory default for mute is 0, corner cases like this fail: - enter SND_SOC_BIAS_ON (e.g. play a wav file) - set mute to 1 - enter SND_SOC_BIAS_OFF (e.g. playback ends) - set mute to 0 - re-enter SND_SOC_BIAS_ON - regcache_sync() incorrectly assumes that the hardware register is already 0, but in fact it needs to be refreshed from the cache Aside from unnecessarily pulsing the reset GPIO when transitioning back from SND_SOC_BIAS_OFF or overriding regcache_default_sync(), can you think of a way to work around this? Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Mon, Apr 20, 2015 at 01:56:46PM -0700, Kevin Cernekee wrote: > On Thu, Apr 16, 2015 at 5:57 AM, Lars-Peter Clausen wrote: > > On 04/15/2015 11:42 PM, Kevin Cernekee wrote: > > If you want to make the clock optional use > > if (PTR_ERR(priv->mclk) == -ENOENT) > > return PTR_ERR(priv->mclk); > > This makes sure that the case where the clock is specified, but there is a > > error with the specification (e.g. incorrect DT cells) is handled properly. > Did you mean: > > if (PTR_ERR(priv->mclk) != -ENOENT) > > return PTR_ERR(priv->mclk); > I don't see this in other codec drivers, but I do see the explicit > EPROBE_DEFER check in max98090, max98095, pcm512x, and wm8960. That's the most correct way of writing a check for an optional clock, best practice on this stuff is evolving over time (as is standardization in the clock API). signature.asc Description: Digital signature
Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Thu, Apr 16, 2015 at 5:57 AM, Lars-Peter Clausen wrote:
> On 04/15/2015 11:42 PM, Kevin Cernekee wrote:
>>
>> Introduce a new codec driver for the Texas Instruments
>> TAS5711/TAS5717/TAS5719 power amplifier chips. These chips are typically
>> used to take an I2S digital audio input and drive 10-20W into a pair of
>> speakers.
>>
>> Signed-off-by: Kevin Cernekee
>
>
> Looks pretty good. Comments inlune.
Thanks for the review. I'm working on a V2 incorporating the feedback
from you and Mark.
>> +static int tas571x_i2c_probe(struct i2c_client *client,
>> +const struct i2c_device_id *id)
>> +{
>> + struct tas571x_private *priv;
>> + struct device *dev = &client->dev;
>> + int i;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> + i2c_set_clientdata(client, priv);
>> +
>> + priv->mclk = devm_clk_get(dev, "mclk");
>> + if (PTR_ERR(priv->mclk) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>
>
> If you want to make the clock optional use
>
> if (PTR_ERR(priv->mclk) == -ENOENT)
> return PTR_ERR(priv->mclk);
>
> This makes sure that the case where the clock is specified, but there is a
> error with the specification (e.g. incorrect DT cells) is handled properly.
Did you mean:
> if (PTR_ERR(priv->mclk) != -ENOENT)
> return PTR_ERR(priv->mclk);
I don't see this in other codec drivers, but I do see the explicit
EPROBE_DEFER check in max98090, max98095, pcm512x, and wm8960.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Mon, Apr 20, 2015 at 08:12:36AM -0700, Kevin Cernekee wrote: > On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown wrote: > >> The same check shows up in numerous other drivers, including the one > >> for the audio controller on my board. > > Sounds like either that (undisclosed) driver has a problem or you're > > using it inappropriately. > I don't think this driver has been posted on the list yet, but the > checks show up in these two functions: > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#108 > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147 > Any suggestions on what to change? That driver has not been posted previously (and looks like it could use some rework for mainline). If it doesn't care if it set the clock and is just trying for information it should be handling -ENOTSUPP, that's why the core returns that value. signature.asc Description: Digital signature
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
Hi Kevin, On Mon, Apr 20, 2015 at 8:12 AM, Kevin Cernekee wrote: > On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown wrote: >> On Sat, Apr 18, 2015 at 01:07:07PM -0700, Kevin Cernekee wrote: >>> On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown wrote: >> >>> > Someone trying to use the atmel_wm8904 driver with something other than >>> > a wm8904 shouldn't really be expecting a good experince... >> >>> The same check shows up in numerous other drivers, including the one >>> for the audio controller on my board. >> >> Sounds like either that (undisclosed) driver has a problem or you're >> using it inappropriately. > > I don't think this driver has been posted on the list yet, but the > checks show up in these two functions: > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#108 > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147 > > Any suggestions on what to change? I think the card driver should be ignoring a return value of -ENOTSUPP from snd_soc_dai_set_sysclk() in that case. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown wrote: > On Sat, Apr 18, 2015 at 01:07:07PM -0700, Kevin Cernekee wrote: >> On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown wrote: > >> > Someone trying to use the atmel_wm8904 driver with something other than >> > a wm8904 shouldn't really be expecting a good experince... > >> The same check shows up in numerous other drivers, including the one >> for the audio controller on my board. > > Sounds like either that (undisclosed) driver has a problem or you're > using it inappropriately. I don't think this driver has been posted on the list yet, but the checks show up in these two functions: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#108 https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147 Any suggestions on what to change? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Sat, Apr 18, 2015 at 01:07:07PM -0700, Kevin Cernekee wrote: > On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown wrote: > > Someone trying to use the atmel_wm8904 driver with something other than > > a wm8904 shouldn't really be expecting a good experince... > The same check shows up in numerous other drivers, including the one > for the audio controller on my board. Sounds like either that (undisclosed) driver has a problem or you're using it inappropriately. > >> Is there a stub version that I can use instead? Nothing jumped out at > >> me when looking at the other codec drivers. > > No, such a stub would make no sense - why would we put a stub in all the > > drivers rather than just making the core do the right thing? > AFAICT, implementing the set_sysclk callback is mandatory, even if it > is a no-op on the codec side. If I delete the stub function, audio > playback fails. For the reasons I mentioned above having a set_sysclk() function is not mandatory and your driver will not be merged with a stub such as is currently present. As far as I can tell you are trying to bodge around some problem elsewhere in either the code or your usage of it. > Clearing just the LSB would accomplish the same thing, but would be > less obvious IMO. It would also require an extra read, and the code > is less concise. I don't think anyone is going to care about an extra read on system init, and in any case if the driver followed best practice and provided register defaults that read would be satisfied from cache. signature.asc Description: Digital signature
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown wrote:
> On Sat, Apr 18, 2015 at 09:16:36AM -0700, Kevin Cernekee wrote:
>> On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown wrote:
>
>> >> +static int tas571x_set_sysclk(struct snd_soc_dai *dai,
>> >> + int clk_id, unsigned int freq, int dir)
>
>> > 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- }
>
> Someone trying to use the atmel_wm8904 driver with something other than
> a wm8904 shouldn't really be expecting a good experince...
The same check shows up in numerous other drivers, including the one
for the audio controller on my board.
>> Is there a stub version that I can use instead? Nothing jumped out at
>> me when looking at the other codec drivers.
>
> No, such a stub would make no sense - why would we put a stub in all the
> drivers rather than just making the core do the right thing?
AFAICT, implementing the set_sysclk callback is mandatory, even if it
is a no-op on the codec side. If I delete the stub function, audio
playback fails.
>> >> + /*
>> >> + * 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.
>
> It's not entirely clear to me why we need to reset the bit, or why if
> we're just trying to update that one bit we write the entire register
> value rather than use _update_bits(). If the goal is just to change
> that one bit then _update_bits() would be a lot clearer.
The default master volume setting on the TAS5717/5719 is 0x3ff (bits
9:0). We only ever update bits 9:1 when the volume changes, because
the driver is only able to work with 0.25 dB resolution rather than
the hardware's native 0.125 dB resolution. So this register write
sets the master volume to an even number, which we know we can handle.
Clearing just the LSB would accomplish the same thing, but would be
less obvious IMO. It would also require an extra read, and the code
is less concise.
Is there a better way to handle this situation?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Sat, Apr 18, 2015 at 09:16:36AM -0700, Kevin Cernekee wrote:
> On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown wrote:
> >> +static int tas571x_set_sysclk(struct snd_soc_dai *dai,
> >> + int clk_id, unsigned int freq, int dir)
> > 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- }
Someone trying to use the atmel_wm8904 driver with something other than
a wm8904 shouldn't really be expecting a good experince...
> Is there a stub version that I can use instead? Nothing jumped out at
> me when looking at the other codec drivers.
No, such a stub would make no sense - why would we put a stub in all the
drivers rather than just making the core do the right thing?
> >> + /*
> >> + * 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.
It's not entirely clear to me why we need to reset the bit, or why if
we're just trying to update that one bit we write the entire register
value rather than use _update_bits(). If the goal is just to change
that one bit then _update_bits() would be a lot clearer.
signature.asc
Description: Digital signature
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Thu, Apr 16, 2015 at 02:57:49PM +0200, Lars-Peter Clausen wrote: > On 04/15/2015 11:42 PM, Kevin Cernekee wrote: > >+case TAS571X_ID_5711: > >+priv->codec_driver.controls = tas5711_controls; > >+priv->codec_driver.num_controls = ARRAY_SIZE(tas5711_controls); > >+break; > >+case TAS571X_ID_5717: > >+case TAS571X_ID_5719: > >+priv->codec_driver.controls = tas5717_controls; > >+priv->codec_driver.num_controls = ARRAY_SIZE(tas5717_controls); > Typically when a driver supports multiple chips with different control sets > the snd_soc_codec_driver implements a probe callback in which the correct > controls are registered. I'm fine with doing it with tables (though just having two static CODEC driver structures would be a bit cleaner). The pattern with probe() is usually that there's some base set of controls all the devices have which then gets device specific controls/routes/whatever added to it so you get benefits fromm sharing the table but in this case the table is so tiny anyway that I'm not sure it's worth caring. signature.asc Description: Digital signature
Re: [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On Wed, Apr 15, 2015 at 02:42:20PM -0700, Kevin Cernekee wrote:
This looks mostly good but several things below, all of which should be
straightforward to fix.
> +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.
> + val += (clamp(params_width(params), 16, 24) >> 2) - 4;
Please write this more clearly or comment it (preferably the former),
it's hard to tell what it's supposed to do and therefore hard to tell if
it's doing it correctly.
> +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.
> + 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).
> +static const struct snd_kcontrol_new tas5711_controls[] = {
> + SOC_SINGLE_TLV("Master Volume",
> +TAS571X_MVOL_REG,
> +0, 0xff, 1, tas5711_volume_tlv),
All these controls will be brokenn if the I2C access goes away.
> +static const struct snd_soc_codec_driver tas571x_codec = {
> + .set_bias_level = tas571x_set_bias_level,
> + .suspend_bias_off = true,
Why not idle_bias_off? It looks like power up takes no meaningful
time.
> +static int tas571x_register_size(struct tas571x_private *priv, unsigned int
> reg)
> +{
> + switch (reg) {
> + case TAS571X_MVOL_REG:
> + case TAS571X_CH1_VOL_REG:
> + case TAS571X_CH2_VOL_REG:
> + return priv->dev_id == TAS571X_ID_5711 ? 1 : 2;
Nest switch statements please, that way things work better if another
variant turns up.
> + /*
> + * This will fall back to the dummy regulator if nothing is specified
> + * in DT.
> + */
The driver doesn't care, it may not even be on a system using DT.
> + if (devm_regulator_bulk_get(dev, TAS571X_NUM_SUPPLIES,
> + priv->supplies)) {
> + dev_err(dev, "Failed to get supplies\n");
> + return -EINVAL;
> + }
Don't discard error codes from functions you call, log them and provide
them to calllers. The above is broken for probe deferral for example.
> + priv->pdn_gpio = devm_gpiod_get(dev, "pdn");
> + if (!IS_ERR(priv->pdn_gpio)) {
> + gpiod_direction_output(priv->pdn_gpio, 1);
> + } else if (PTR_ERR(priv->pdn_gpio) != -ENOENT &&
> +PTR_ERR(priv->pdn_gpio) != -ENOSYS) {
> + dev_warn(dev, "error requesting pdn_gpio: %ld\n",
> + PTR_ERR(priv->pdn_gpio));
> + }
This should at least be handling probe deferral, it's not clear why it
doesn't just error out in the cases where it gets an error. Similarly
for the reset GPIO.
> + /*
> + * 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?
> +#ifndef _TAS571X_H
> +#define _TAS571X_H
> +
> +#include
Why is this needed in the header?
signature.asc
Description: Digital signature
Re: [alsa-devel] [PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
On 04/15/2015 11:42 PM, Kevin Cernekee wrote:
Introduce a new codec driver for the Texas Instruments
TAS5711/TAS5717/TAS5719 power amplifier chips. These chips are typically
used to take an I2S digital audio input and drive 10-20W into a pair of
speakers.
Signed-off-by: Kevin Cernekee
Looks pretty good. Comments inlune.
[...]
-obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o
Accidentally removed line
+obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o
[...]
+++ b/sound/soc/codecs/tas571x.c
@@ -0,0 +1,456 @@
+/*
+ * TAS571x amplifier audio driver
+ *
+ * Copyright (C) 2015 Google, Inc.
+ * Copyright (c) 2013 Daniel Mack
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
There is no of specific GPIO code in the driver.
[...]
+
+static int tas571x_set_bias_level(struct snd_soc_codec *codec,
+ enum snd_soc_bias_level level)
+{
+ struct tas571x_private *priv = snd_soc_codec_get_drvdata(codec);
+ int ret, assert_pdn = 0;
+
+ if (priv->bias_level == level)
+ return 0;
The core already takes care that this function is only called if there is a
actual change.
+
+ switch (level) {
+ case SND_SOC_BIAS_PREPARE:
+ if (!IS_ERR(priv->mclk)) {
+ ret = clk_prepare_enable(priv->mclk);
+ if (ret) {
+ dev_err(codec->dev,
+ "Failed to enable master clock\n");
+ return ret;
+ }
+ }
+
+ 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;
+
+ if (!IS_ERR(priv->mclk))
+ clk_disable_unprepare(priv->mclk);
+ break;
+ case SND_SOC_BIAS_ON:
+ break;
+ case SND_SOC_BIAS_OFF:
+ /* Note that this kills I2C accesses. */
+ assert_pdn = 1;
+ break;
+ }
+
+ if (!IS_ERR(priv->pdn_gpio))
+ gpiod_set_value(priv->pdn_gpio, !assert_pdn);
+
+ priv->bias_level = level;
This should update codec->dapm.bias_level, otherwise the core will become
confused about the actual level.
+ return 0;
+}
+
[...]
+static const unsigned int tas5711_volume_tlv[] = {
+ TLV_DB_RANGE_HEAD(1),
+ 0, 0xff, TLV_DB_SCALE_ITEM(-10350, 50, 1),
+};
For TLVs with a single item use DECLARE_TLV_DB_SCALE()
+
[...]
+static const unsigned int tas5717_volume_tlv[] = {
+ TLV_DB_RANGE_HEAD(1),
+ 0x000, 0x1ff, TLV_DB_SCALE_ITEM(-10375, 25, 0),
+};
Same here.
[...]
+static int tas571x_i2c_probe(struct i2c_client *client,
+const struct i2c_device_id *id)
+{
+ struct tas571x_private *priv;
+ struct device *dev = &client->dev;
+ int i;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+ i2c_set_clientdata(client, priv);
+
+ priv->mclk = devm_clk_get(dev, "mclk");
+ if (PTR_ERR(priv->mclk) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
If you want to make the clock optional use
if (PTR_ERR(priv->mclk) == -ENOENT)
return PTR_ERR(priv->mclk);
This makes sure that the case where the clock is specified, but there is a
error with the specification (e.g. incorrect DT cells) is handled properly.
+
+ for (i = 0; i < TAS571X_NUM_SUPPLIES; i++)
+ priv->supplies[i].supply = tas571x_supply_names[i];
+
+ /*
+* This will fall back to the dummy regulator if nothing is specified
+* in DT.
+*/
+ if (devm_regulator_bulk_get(dev, TAS571X_NUM_SUPPLIES,
+ priv->supplies)) {
Move the function outside the if condition and also pass the error condition
to the caller. (And print it)
+ dev_err(dev, "Failed to get supplies\n");
+ return -EINVAL;
+ }
+ if (regulator_bulk_enable(TAS571X_NUM_SUPPLIES, priv->supplies)) {
Same here.
+ dev_err(dev, "Failed to enable supplies\n");
+ return -EINVAL;
+ }
+
+ priv->regmap = devm_regmap_init(dev, NULL, client, &tas571x_regmap);
+ if (IS_ERR(priv->regmap))
+ return PTR_ERR(priv->regmap);
+
+ priv->pdn_gpio = devm_gpiod_get(dev, "pdn");
devm_gpiod_get_optional() ?
Using gpiod_get w
[PATCH 2/3] ASoC: tas571x: New driver for TI TAS571x power amplifiers
Introduce a new codec driver for the Texas Instruments
TAS5711/TAS5717/TAS5719 power amplifier chips. These chips are typically
used to take an I2S digital audio input and drive 10-20W into a pair of
speakers.
Signed-off-by: Kevin Cernekee
---
sound/soc/codecs/Kconfig | 5 +
sound/soc/codecs/Makefile | 3 +-
sound/soc/codecs/tas571x.c | 456 +
sound/soc/codecs/tas571x.h | 39
4 files changed, 502 insertions(+), 1 deletion(-)
create mode 100644 sound/soc/codecs/tas571x.c
create mode 100644 sound/soc/codecs/tas571x.h
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index ea9f0e31f9d4..ef8393926607 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -103,6 +103,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
select SND_SOC_TAS2552 if I2C
select SND_SOC_TAS5086 if I2C
+ select SND_SOC_TAS571X if I2C
select SND_SOC_TFA9879 if I2C
select SND_SOC_TLV320AIC23_I2C if I2C
select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
@@ -606,6 +607,10 @@ config SND_SOC_TAS5086
tristate "Texas Instruments TAS5086 speaker amplifier"
depends on I2C
+config SND_SOC_TAS571X
+ tristate "Texas Instruments TAS5711/TAS5717/TAS5719 power amplifiers"
+ depends on I2C
+
config SND_SOC_TFA9879
tristate "NXP Semiconductors TFA9879 amplifier"
depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 69b8666d187a..104112f9a486 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -105,6 +105,7 @@ snd-soc-sta350-objs := sta350.o
snd-soc-sta529-objs := sta529.o
snd-soc-stac9766-objs := stac9766.o
snd-soc-tas5086-objs := tas5086.o
+snd-soc-tas571x-objs := tas571x.o
snd-soc-tfa9879-objs := tfa9879.o
snd-soc-tlv320aic23-objs := tlv320aic23.o
snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o
@@ -283,7 +284,7 @@ obj-$(CONFIG_SND_SOC_STA350) += snd-soc-sta350.o
obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o
obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o
obj-$(CONFIG_SND_SOC_TAS2552) += snd-soc-tas2552.o
-obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o
+obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o
obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o
obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o
obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o
diff --git a/sound/soc/codecs/tas571x.c b/sound/soc/codecs/tas571x.c
new file mode 100644
index ..25c4deb2342d
--- /dev/null
+++ b/sound/soc/codecs/tas571x.c
@@ -0,0 +1,456 @@
+/*
+ * TAS571x amplifier audio driver
+ *
+ * Copyright (C) 2015 Google, Inc.
+ * Copyright (c) 2013 Daniel Mack
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "tas571x.h"
+
+#define TAS571X_NUM_SUPPLIES 2
+static const char * const tas571x_supply_names[TAS571X_NUM_SUPPLIES] = {
+ "VDD",
+ "PVDD",
+};
+
+struct tas571x_private {
+ struct regmap *regmap;
+ struct regulator_bulk_data supplies[TAS571X_NUM_SUPPLIES];
+ struct clk *mclk;
+ unsigned intformat;
+ enum snd_soc_bias_level bias_level;
+ struct gpio_desc*reset_gpio;
+ struct gpio_desc*pdn_gpio;
+ unsigned intdev_id;
+ struct snd_soc_codec_driver codec_driver;
+};
+
+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;
+}
+
+static int tas571x_set_dai_fmt(struct snd_soc_dai *dai, unsigned int format)
+{
+ struct tas571x_private *priv = snd_soc_codec_get_drvdata(dai->codec);
+
+ priv->format = format;
+
+ return 0;
+}
+
+static int tas571x_hw_params(struct snd_pcm_substream *substream,
+struct snd_pcm_hw_params *params,
+struct snd_soc_dai *dai)
+{
+ struct tas571x_private *priv = snd_soc_codec_get_drvdata(dai->codec);
+ u32 val;
+
+ switch (priv->format & SND_SOC_DAIFMT_FORMAT_MASK) {
+ case SND_SOC_DAIFMT_RIGHT_J:
+ val = 0x00;
+ break;
+ case SND_SOC_DAIFMT_I2S:
+ val
