ASoC: add support for TAS5805M digital amplifier
PATCH 1/2 ASoC: add support for TAS5805M digital amplifier
The Texas Instruments TAS5805M is a class D audio amplifier with an integrated DSP. DSP configuration is expected to be supplied via a device-tree attribute. See the bindings file for more details.
--- sound/soc/codecs/Kconfig | 9 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tas5805m.c | 534 ++++++++++++++++++++++++++++++++++++ 3 files changed, 545 insertions(+) create mode 100644 sound/soc/codecs/tas5805m.c
sound/soc/codecs/Kconfig +9 -0
@@ -1485,6 +1485,15 @@ config SND_SOC_TAS5720 Enable support for Texas Instruments TAS5720L/M high-efficiency mono Class-D audio power amplifiers. +config SND_SOC_TAS5805M + tristate "Texas Instruments TAS5805M speaker amplifier" + depends on I2C + help + Enable support for Texas Instruments TAS5805M Class-D + amplifiers. This is a speaker amplifier with an integrated + DSP. DSP configuration for each instance needs to be supplied + via a device-tree attribute. + config SND_SOC_TAS6424 tristate "Texas Instruments TAS6424 Quad-Channel Audio amplifier" depends on I2C
sound/soc/codecs/Makefile +2 -0
@@ -236,6 +236,7 @@ snd-soc-sti-sas-objs := sti-sas.o snd-soc-tas5086-objs := tas5086.o snd-soc-tas571x-objs := tas571x.o snd-soc-tas5720-objs := tas5720.o +snd-soc-tas5805m-objs := tas5805m.o snd-soc-tas6424-objs := tas6424.o snd-soc-tda7419-objs := tda7419.o snd-soc-tas2770-objs := tas2770.o
@@ -574,6 +575,7 @@ obj-$(CONFIG_SND_SOC_TAS2764) += snd-soc-tas2764.o obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o obj-$(CONFIG_SND_SOC_TAS5720) += snd-soc-tas5720.o +obj-$(CONFIG_SND_SOC_TAS5805M) += snd-soc-tas5805m.o obj-$(CONFIG_SND_SOC_TAS6424) += snd-soc-tas6424.o obj-$(CONFIG_SND_SOC_TDA7419) += snd-soc-tda7419.o obj-$(CONFIG_SND_SOC_TAS2770) += snd-soc-tas2770.o
sound/soc/codecs/tas5805m.c new file +534 -0
534 lines not on this page · raw message
Signature · 1 line
2.30.2
Mark Brown
maintainersound/soc/codecs/tas5805m.c line 4
Quoted 6 lines · Daniel Beer
+++
b/sound/soc/codecs/tas5805m.csound/soc/codecs/tas5805m.c new file +4
-0@@ -0,0 +1,534 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Driver for the TAS5805M Audio Amplifier +*
Please make the entire comment a C++ one so things look more intentional.
sound/soc/codecs/tas5805m.c line 182
Quoted 11 lines
sound/soc/codecs/tas5805m.c new file +10
-0+static void tas5805m_refresh_unlocked (struct snd_soc_component *component) +{ +struct tas5805m_priv *tas5805m = +snd_soc_component_get_drvdata (component); +uint8_t v[4 ]; +unsigned int i; +uint32_t x; +snd_soc_component_write (component,REG_PAGE ,0x00 ); +snd_soc_component_write (component,REG_BOOK ,0x8c ); +snd_soc_component_write (component,REG_PAGE ,0x2a );
This isn't using the regmap paging support and I'm not seeing anything that resets the page here.
sound/soc/codecs/tas5805m.c line 187
Quoted 4 lines
sound/soc/codecs/tas5805m.c new file +4
-0+for (i =0 ; i <4 ; i++) +snd_soc_component_write (component,0x24 + i, v[i]); +for (i =0 ; i <4 ; i++) +snd_soc_component_write (component,0x28 + i, v[i]);
This looks like it's potentially a stereo control but we're not allowing the two channels to be configured separately?
sound/soc/codecs/tas5805m.c line 191
Quoted 3 lines
sound/soc/codecs/tas5805m.c new file +3
-0+/* Volume controls */ +snd_soc_component_write (component,REG_DEVICE_CTRL_2 , + tas5805m->is_muted ?0x0b :0x03 );
This comment doesn't seem terribly descriptive and this is very much a magic number.
sound/soc/codecs/tas5805m.c line 215
Quoted 11 lines
sound/soc/codecs/tas5805m.c new file +11
-0+static int tas5805m_vol_get (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol) +{ +struct snd_soc_component *component = +snd_soc_kcontrol_component (kcontrol); +struct tas5805m_priv *tas5805m = +snd_soc_component_get_drvdata (component); + +mutex_lock (&tas5805m->lock); + ucontrol->value.integer.value[0 ] = tas5805m->vol; +mutex_unlock (&tas5805m->lock);
What is this lock intended to protect? We take the lock, do a read of a single value and then unlock which doesn't seem like it's adding a huge amount.
sound/soc/codecs/tas5805m.c line 230
Quoted 4 lines
sound/soc/codecs/tas5805m.c new file +4
-0+ +mutex_lock (&tas5805m->lock); + tas5805m->vol =clamp ((int )ucontrol->value.integer.value[0 ], +TAS5805M_VOLUME_MIN ,TAS5805M_VOLUME_MAX );
It would be better to reject out of bounds values with an error.
sound/soc/codecs/tas5805m.c line 261
Quoted 7 lines
sound/soc/codecs/tas5805m.c new file +7
-0+static void send_cfg (struct snd_soc_component *component, +const uint8_t *s,unsigned int len) +{ +unsigned int i; + +for (i =0 ; i +1 < len; i +=2 ) +snd_soc_component_write (component, s[i], s[i +1 ]);
This looks like the driver might be happier using regmap directly, this looks like an open coded _multi_reg_write().
Quoted 7 lines
+/* The TAS5805M can't be configured or brought out of power-down without
+ * an I2S clock. In power-down, registers are reset.
+ *
+ * We rely on DAPM not powering up the DAC widget until the source for
+ * it is ready, which we think implies that the I2S clock is present and
+ * stable.
+ */
That's not true, we run DAPM in prepare() prior to calling trigger() and controllers might not start clocking the bus until trigger() has been called. There was some other device with similar issues which was scheduling things from the trigger() callback IIRC, if you search around in the CODEC directory you should be able to find something.
sound/soc/codecs/tas5805m.c line 434
Quoted 7 lines
sound/soc/codecs/tas5805m.c new file +7
-0+dev_set_drvdata (dev, tas5805m); + tas5805m->regmap = regmap; + tas5805m->gpio_pdn_n =of_get_named_gpio (dev->of_node,"pdn-gpio" ,0 ); +if (!gpio_is_valid (tas5805m->gpio_pdn_n)) { +dev_err (dev,"power-down GPIO not specified\n" ); +return -EINVAL ; + }
Use the gpiod APIs, you shouldn't need to do anything specific to OF here. Also GPIO properties need to have names ending -gpios even if they're for a single GPIO.
Quoted 6 lines
sound/soc/codecs/tas5805m.c new file +6
-0+ ret =devm_snd_soc_register_component (dev, &soc_codec_dev_tas5805m, + &tas5805m_dai,1 ); +if (ret <0 ) { +dev_err (dev,"unable to register codec: %d\n" , ret); +return ret; + }
sound/soc/codecs/tas5805m.c line 490
Quoted 10 lines
sound/soc/codecs/tas5805m.c new file +10
-0+ + ret =regulator_enable (tas5805m->pvdd); +if (ret <0 ) { +dev_err (dev,"failed to enable pvdd: %d\n" , ret); +return ret; + } + +usleep_range (100000 ,150000 ); +gpio_set_value (tas5805m->gpio_pdn_n,1 ); +usleep_range (10000 ,15000 );
This seems broken - the card might be instantiated and userspace start using it as soon as the component is registered but we don't power on the device until after registering it and there's no runtime power up or down. For power management like this you need to complete the initial power on prior to registering the component.
sound/soc/codecs/tas5805m.c line 501
Quoted 7 lines
sound/soc/codecs/tas5805m.c new file +7
-0+static int tas5805m_i2c_remove (struct i2c_client *i2c) +{ +struct tas5805m_priv *tas5805m =dev_get_drvdata (&i2c->dev); + +gpio_set_value (tas5805m->gpio_pdn_n,0 ); +usleep_range (10000 ,15000 ); +regulator_disable (tas5805m->pvdd);
This will power down the device prior to unregistering the component so userspace could still be running. You need to not use devm_ for component registration and manually unregister here to avoid that issue (or register a custom devm action).
Quoted 8 lines · Mark Brown
Quoted 7 lines
sound/soc/codecs/tas5805m.c new file +7
-0+static void tas5805m_refresh_unlocked (struct snd_soc_component *component) +{ +struct tas5805m_priv *tas5805m = +snd_soc_component_get_drvdata (component); +uint8_t v[4 ]; +unsigned int i; +uint32_t x;
Quoted 6 lines
sound/soc/codecs/tas5805m.c line 182
Quoted 3 lines
sound/soc/codecs/tas5805m.c new file +3
-0+snd_soc_component_write (component,REG_PAGE ,0x00 ); +snd_soc_component_write (component,REG_BOOK ,0x8c ); +snd_soc_component_write (component,REG_PAGE ,0x2a );This isn't using the regmap paging support and I'm not seeing anything that resets the page here.
Hi Mark,
The blob of register writes we get given from PPC3 contains a whole lot of explicit page and book changes, and there's not an easy way to tell regmap about this, as far as I know. Do you think it's acceptable to stick with explicit paging for this reason, or is there a way to make this work with regmap's paging?
I'll go ahead and address your other comments before resubmitting.
Cheers,
Daniel
Signature · 4 lines
Daniel Beer Firmware Engineer at Igor Institute [email protected] or +64-27-420-8101 Offices in Seattle, San Francisco, and Vancouver BC or (206) 494-3312
Mark Brown
maintainerQuoted 5 lines · Daniel Beer
The blob of register writes we get given from PPC3 contains a whole lot of explicit page and book changes, and there's not an easy way to tell regmap about this, as far as I know. Do you think it's acceptable to stick with explicit paging for this reason, or is there a way to make this work with regmap's paging?
That's probably fine. It's *really* hard to get enthusiastic about this system design TBH, just injecting a stream of unverified register writes is going to make the driver very fragile against changes but I'm not sure you're going to much better there.