Add an I2C based codec driver for ST TDA7802 amplifier. The amplifier
supports 4 audio channels but can support up to 16 with multiple
devices.
Signed-off-by: Thomas Preston <[email protected]>
Cc: Patrick Glaser <[email protected]>
Cc: Rob Duncan <[email protected]>
Cc: Nate Case <[email protected]>
---
Changes since v1:
- Use ALSA kcontrol interface to expose device controls to userland
- Gain
- Channel diagnostic mode
- Impedance efficiency optimiser. I decided against setting this
as a DT property since it seems like something that can be
changed on the fly.
- Add regmap default values
- Channel unmute by default is added in a downstream patch.
- I'm not sure if I should keep this since they're all zero,
although there are other drivers will all-zero reg_defaults.
- I believe the "//" style is used for SPDX headers in normal C source files.
https://lwn.net/Articles/739183/
- Drop the "enable" sysfs device attribute.
- Don't set TDM format using magic numbers.
- Set sample rate using hw_params.
- Remove unecessary defines.
- Use DAPM to handle AMP_ON.
- Cosmetic fixups
sound/soc/codecs/Kconfig | 6 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/tda7802.c | 509 +++++++++++++++++++++++++++++++++++++
3 files changed, 517 insertions(+)
create mode 100644 sound/soc/codecs/tda7802.c
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 9f89a5346299..7e3117eab735 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -182,6 +182,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_TAS5720 if I2C
select SND_SOC_TAS6424 if I2C
select SND_SOC_TDA7419 if I2C
+ select SND_SOC_TDA7802 if I2C
select SND_SOC_TFA9879 if I2C
select SND_SOC_TLV320AIC23_I2C if I2C
select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
@@ -1121,6 +1122,11 @@ config SND_SOC_TDA7419
depends on I2C
select REGMAP_I2C
+config SND_SOC_TDA7802
+ tristate "ST TDA7802 audio processor"
+ depends on I2C
+ select REGMAP_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 5b4bb8cf4325..31dec8930e98 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -194,6 +194,7 @@ snd-soc-tas571x-objs := tas571x.o
snd-soc-tas5720-objs := tas5720.o
snd-soc-tas6424-objs := tas6424.o
snd-soc-tda7419-objs := tda7419.o
+snd-soc-tda7802-objs := tda7802.o
snd-soc-tfa9879-objs := tfa9879.o
snd-soc-tlv320aic23-objs := tlv320aic23.o
snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o
@@ -474,6 +475,7 @@ obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o
obj-$(CONFIG_SND_SOC_TAS5720) += snd-soc-tas5720.o
obj-$(CONFIG_SND_SOC_TAS6424) += snd-soc-tas6424.o
obj-$(CONFIG_SND_SOC_TDA7419) += snd-soc-tda7419.o
+obj-$(CONFIG_SND_SOC_TDA7802) += snd-soc-tda7802.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/tda7802.c b/sound/soc/codecs/tda7802.c
new file mode 100644
index 000000000000..0f82a88bc1a4
--- /dev/null
+++ b/sound/soc/codecs/tda7802.c
@@ -0,0 +1,509 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * tda7802.c -- codec driver for ST TDA7802
+ *
+ * Copyright (C) 2016-2019 Tesla Motors, Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/string.h>
+#include <sound/control.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+
+#define ENABLE_DELAY_MS 10
+
+#define TDA7802_IB0 0x00
+#define TDA7802_IB1 0x01
+#define TDA7802_IB2 0x02
+#define TDA7802_IB3 0x03
+#define TDA7802_IB4 0x04
+#define TDA7802_IB5 0x05
+
+#define TDA7802_DB0 0x10
+#define TDA7802_DB5 0x15
+
+#define IB2_DIGITAL_MUTE_DISABLED (1 << 2)
+
+#define IB3_SAMPLE_RATE_SHIFT 6
+#define IB3_SAMPLE_RATE_MASK (3 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_SAMPLE_RATE_44_KHZ (0 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_SAMPLE_RATE_48_KHZ (1 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_SAMPLE_RATE_96_KHZ (2 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_SAMPLE_RATE_192_KHZ (3 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_FORMAT_SHIFT 3
+#define IB3_FORMAT_MASK (7 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_I2S (0 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM4 (1 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM8_DEV1 (2 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM8_DEV2 (3 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM16_DEV1 (4 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM16_DEV2 (5 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM16_DEV3 (6 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM16_DEV4 (7 << IB3_FORMAT_SHIFT)
+
+enum tda7802_type {
+ tda7802_base,
+};
+
+struct tda7802_priv {
+ struct i2c_client *i2c;
+ struct regmap *regmap;
+ struct regulator *enable_reg;
+};
+
+static const struct reg_default tda7802_reg[] = {
+ { TDA7802_IB0, 0x0 },
+ { TDA7802_IB1, 0x0 },
+ { TDA7802_IB2, 0x0 },
+ { TDA7802_IB3, 0x0 },
+ { TDA7802_IB4, 0x0 },
+ { TDA7802_IB5, 0x0 },
+};
+
+static bool tda7802_readable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case TDA7802_IB0 ... TDA7802_IB5:
+ case TDA7802_DB0 ... TDA7802_DB5:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool tda7802_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case TDA7802_DB0 ... TDA7802_DB5:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool tda7802_writeable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case TDA7802_IB0 ... TDA7802_IB5:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static const struct regmap_config tda7802_regmap_config = {
+ .val_bits = 8,
+ .reg_bits = 8,
+ .max_register = TDA7802_DB5,
+ .use_single_read = 1,
+ .use_single_write = 1,
+
+ .readable_reg = tda7802_readable_reg,
+ .volatile_reg = tda7802_volatile_reg,
+ .writeable_reg = tda7802_writeable_reg,
+
+ .reg_defaults = tda7802_reg,
+ .num_reg_defaults = ARRAY_SIZE(tda7802_reg),
+ .cache_type = REGCACHE_RBTREE,
+};
+
+static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
+{
+ const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;
+ struct device *dev = dai->dev;
+ int err;
+
+ dev_dbg(dev, "%s mute=%d\n", __func__, mute);
+
+ err = snd_soc_component_update_bits(dai->component, TDA7802_IB2,
+ IB2_DIGITAL_MUTE_DISABLED, mute_disabled);
+ if (err < 0)
+ dev_err(dev, "Cannot mute amp %d\n", err);
+
+ return err;
+}
+
+static int tda7802_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
+ unsigned int rx_mask, int slots, int slot_width)
+{
+ struct device *dev = dai->dev;
+ u8 tdm_format;
+ int ret;
+
+ dev_dbg(dai->dev, "%s tx %x, rx %x, slots %d, slot_width %d\n",
+ __func__, tx_mask, rx_mask, slots, slot_width);
+
+ switch (slots) {
+ case 4:
+ tdm_format = IB3_FORMAT_TDM4;
+ break;
+ case 8:
+ switch (tx_mask) {
+ case 0x000f:
+ tdm_format = IB3_FORMAT_TDM8_DEV1;
+ break;
+ case 0x00f0:
+ tdm_format = IB3_FORMAT_TDM8_DEV2;
+ break;
+ default:
+ dev_err(dev,
+ "Failed to set tdm fmt, slots %d, tx_mask %x\n",
+ slots, tx_mask);
+ return -ENOTSUPP;
+ }
+ break;
+ case 16:
+ switch (tx_mask) {
+ case 0x000f:
+ tdm_format = IB3_FORMAT_TDM16_DEV1;
+ break;
+ case 0x00f0:
+ tdm_format = IB3_FORMAT_TDM16_DEV2;
+ break;
+ case 0x0f00:
+ tdm_format = IB3_FORMAT_TDM16_DEV3;
+ break;
+ case 0xf000:
+ tdm_format = IB3_FORMAT_TDM16_DEV4;
+ break;
+ default:
+ dev_err(dev,
+ "Failed to set tdm fmt, slots %d, tx_mask %x\n",
+ slots, tx_mask);
+ return -ENOTSUPP;
+ }
+ break;
+ default:
+ dev_err(dev, "Failed to set %d slots, supported: 4, 8, 16\n",
+ slots);
+ return -ENOTSUPP;
+ }
+
+ ret = snd_soc_component_update_bits(dai->component, TDA7802_IB3,
+ IB3_FORMAT_MASK, tdm_format);
+ if (ret < 0) {
+ dev_err(dev, "Failed to write IB3 config %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int tda7802_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ int err;
+ u8 val;
+
+ dev_dbg(dai->dev, "%s rate %d\n", __func__, params_rate(params));
+
+ switch (params_rate(params)) {
+ case 44100:
+ val = IB3_SAMPLE_RATE_44_KHZ;
+ break;
+ case 48000:
+ val = IB3_SAMPLE_RATE_48_KHZ;
+ break;
+ case 96000:
+ val = IB3_SAMPLE_RATE_96_KHZ;
+ break;
+ case 192000:
+ val = IB3_SAMPLE_RATE_192_KHZ;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ err = snd_soc_component_update_bits(dai->component, TDA7802_IB3,
+ IB3_SAMPLE_RATE_MASK, val);
+ if (err < 0)
+ dev_err(dai->dev, "Could not set hw_params, %d\n", err);
+
+ return err;
+}
+
+static const struct snd_soc_dai_ops tda7802_dai_ops = {
+ .digital_mute = tda7802_digital_mute,
+ .hw_params = tda7802_hw_params,
+ .set_tdm_slot = tda7802_set_tdm_slot,
+};
+
+static struct snd_soc_dai_driver tda7802_dai_driver = {
+ .name = "tda7802",
+ .playback = {
+ .stream_name = "Playback",
+ .channels_min = 4,
+ .channels_max = 4,
+ .rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
+ SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_192000,
+ .formats = SNDRV_PCM_FMTBIT_S32_LE,
+ },
+ .ops = &tda7802_dai_ops,
+};
+
+static int tda7802_set_bias_level(struct snd_soc_component *component,
+ enum snd_soc_bias_level level)
+{
+ const struct tda7802_priv *tda7802 =
+ snd_soc_component_get_drvdata(component);
+ struct snd_soc_dapm_context *dapm_context =
+ snd_soc_component_get_dapm(component);
+ const enum snd_soc_bias_level oldlevel =
+ snd_soc_dapm_get_bias_level(dapm_context);
+ int err = 0;
+
+ dev_dbg(component->dev, "%s level %d\n", __func__, level);
+
+ switch (level) {
+ case SND_SOC_BIAS_ON:
+ break;
+ case SND_SOC_BIAS_PREPARE:
+ break;
+ case SND_SOC_BIAS_STANDBY:
+ err = regulator_enable(tda7802->enable_reg);
+ if (err < 0) {
+ dev_err(component->dev, "Could not enable.\n");
+ return err;
+ }
+ dev_dbg(component->dev, "Regulator enabled\n");
+ msleep(ENABLE_DELAY_MS);
+
+ if (oldlevel == SND_SOC_BIAS_OFF) {
+ dev_dbg(component->dev, "Syncing regcache\n");
+ err = regcache_sync(component->regmap);
+ if (err < 0)
+ dev_err(component->dev,
+ "Could not sync regcache, %d\n", err);
+ }
+ break;
+ case SND_SOC_BIAS_OFF:
+ regcache_mark_dirty(component->regmap);
+ err = regulator_disable(tda7802->enable_reg);
+ if (err < 0)
+ dev_err(component->dev, "Could not disable.\n");
+ break;
+ }
+
+ return err;
+}
+
+static const char * const amp_mode_str[] = {
+ "High Efficiency",
+ "Standard Class AB",
+};
+
+static SOC_ENUM_SINGLE_DECL(ch1_amp_mode, TDA7802_IB0, 0, amp_mode_str);
+static SOC_ENUM_SINGLE_DECL(ch2_amp_mode, TDA7802_IB0, 1, amp_mode_str);
+static SOC_ENUM_SINGLE_DECL(ch3_amp_mode, TDA7802_IB0, 2, amp_mode_str);
+static SOC_ENUM_SINGLE_DECL(ch4_amp_mode, TDA7802_IB0, 3, amp_mode_str);
+
+static const char * const zopt_str[] = {
+ "2ohm",
+ "4ohm",
+};
+
+static SOC_ENUM_SINGLE_DECL(zopt_ch24, TDA7802_IB1, 7, zopt_str);
+static SOC_ENUM_SINGLE_DECL(zopt_ch13, TDA7802_IB2, 0, zopt_str);
+
+static const char * const diag_timing_str[] = {
+ "default",
+ "x2",
+ "x4",
+ "x8",
+};
+
+static SOC_ENUM_SINGLE_DECL(diag_timing, TDA7802_IB1, 5, diag_timing_str);
+
+static const char * const mute_time_str[] = {
+ "1.45ms",
+ "5.8ms",
+ "11.6ms",
+ "23.2ms",
+ "46.4ms",
+ "92.8ms",
+ "185.6ms",
+ "371.2ms",
+};
+
+static SOC_ENUM_SINGLE_DECL(mute_time, TDA7802_IB2, 5, mute_time_str);
+
+static const char * const automute_threshold_str[] = {
+ "5.3V",
+ "7.3V",
+};
+
+static SOC_ENUM_SINGLE_DECL(automute_threshold, TDA7802_IB2, 1,
+ automute_threshold_str);
+
+static const char * const ac_diag_threshold_str[] = {
+ "High",
+ "Low",
+};
+
+static SOC_ENUM_SINGLE_DECL(ac_diag_threshold, TDA7802_IB3, 4,
+ ac_diag_threshold_str);
+
+static const char * const ch_diag_mode_str[] = {
+ "Speaker",
+ "Boosted",
+};
+
+static SOC_ENUM_SINGLE_DECL(diag_mode_ch13, TDA7802_IB4, 2, ch_diag_mode_str);
+static SOC_ENUM_SINGLE_DECL(diag_mode_ch24, TDA7802_IB4, 1, ch_diag_mode_str);
+
+static const char * const temp_warn_str[] = {
+ "TW1",
+ "TW2",
+ "TW3",
+ "TW4",
+ "None",
+};
+
+static SOC_ENUM_SINGLE_DECL(temp_warn, TDA7802_IB5, 5, temp_warn_str);
+
+static const char * const clip_detect_str[] = {
+ "2%",
+ "5%",
+ "10%",
+ "None",
+};
+
+static SOC_ENUM_SINGLE_DECL(clip_detect_ch13, TDA7802_IB5, 3, clip_detect_str);
+static SOC_ENUM_SINGLE_DECL(clip_detect_ch24, TDA7802_IB5, 1, clip_detect_str);
+
+static const struct snd_kcontrol_new tda7802_snd_controls[] = {
+ SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0),
+ SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0),
+ SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0),
+ SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0),
+ SOC_ENUM("Channel 4 Amplifier Mode", ch4_amp_mode),
+ SOC_ENUM("Channel 3 Amplifier Mode", ch3_amp_mode),
+ SOC_ENUM("Channel 2 Amplifier Mode", ch2_amp_mode),
+ SOC_ENUM("Channel 1 Amplifier Mode", ch1_amp_mode),
+
+ /* Impedance (Z) efficiency optimiser */
+ SOC_ENUM("Z efficiency opt channels 2 & 4", zopt_ch24),
+ SOC_ENUM("Z efficiency opt channels 1 & 3", zopt_ch13),
+
+ SOC_ENUM("Long diag config timing", diag_timing),
+ SOC_SINGLE_RANGE("Gain channels 1 & 3", TDA7802_IB1, 3, 0, 3, 0),
+ SOC_SINGLE_RANGE("Gain channels 2 & 4", TDA7802_IB1, 1, 0, 3, 0),
+ SOC_SINGLE("Digital gain boost +6db", TDA7802_IB1, 0, 1, 0),
+
+ /* Mute settings */
+ SOC_ENUM("Mute time", mute_time),
+ SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0),
+ SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0),
+ SOC_ENUM("Automute threshold", automute_threshold),
+
+ SOC_SINGLE("High pass filter enable", TDA7802_IB3, 0, 1, 0),
+
+ /* Interactive diagnostics */
+ SOC_SINGLE("Noise gating func enable", TDA7802_IB4, 7, 1, 1),
+ SOC_SINGLE("CDdiag: short fault", TDA7802_IB4, 6, 1, 1),
+ SOC_SINGLE("CDdiag: offset", TDA7802_IB4, 5, 1, 1),
+ SOC_ENUM("CDdiag: temperature warning", temp_warn),
+ SOC_ENUM("AC diag current threshold", ac_diag_threshold),
+ SOC_SINGLE("AC diag enable", TDA7802_IB4, 3, 1, 0),
+ SOC_ENUM("Diag mode channels 1 & 3", diag_mode_ch13),
+ SOC_ENUM("Diag mode channels 2 & 4", diag_mode_ch24),
+ SOC_SINGLE("Diag mode enable", TDA7802_IB4, 0, 1, 0),
+
+ SOC_ENUM("Clipping detect channels 1 & 3", clip_detect_ch13),
+ SOC_ENUM("Clipping detect channels 2 & 4", clip_detect_ch24),
+};
+
+static const struct snd_soc_dapm_widget tda7802_dapm_widgets[] = {
+ SND_SOC_DAPM_AIF_IN("AIFIN", NULL, 0, SND_SOC_NOPM, 0, 0),
+ SND_SOC_DAPM_DAC("DAC", NULL, TDA7802_IB5, 0, 0),
+ SND_SOC_DAPM_OUTPUT("SPK"),
+};
+
+static const struct snd_soc_dapm_route tda7802_dapm_routes[] = {
+ { "AIFIN", NULL, "Playback" },
+ { "DAC", NULL, "AIFIN" },
+ { "SPK", NULL, "DAC" },
+};
+
+static const struct snd_soc_component_driver tda7802_component_driver = {
+ .set_bias_level = tda7802_set_bias_level,
+ .idle_bias_on = 1,
+ .suspend_bias_off = 1,
+ .controls = tda7802_snd_controls,
+ .num_controls = ARRAY_SIZE(tda7802_snd_controls),
+ .dapm_widgets = tda7802_dapm_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(tda7802_dapm_widgets),
+ .dapm_routes = tda7802_dapm_routes,
+ .num_dapm_routes = ARRAY_SIZE(tda7802_dapm_routes),
+};
+
+static int tda7802_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &i2c->dev;
+ struct tda7802_priv *tda7802;
+ int err;
+
+ dev_dbg(dev, "%s addr=0x%02hx, id %p\n", __func__, i2c->addr, id);
+
+ tda7802 = devm_kmalloc(dev, sizeof(*tda7802), GFP_KERNEL);
+ if (!tda7802)
+ return -ENOMEM;
+
+ i2c_set_clientdata(i2c, tda7802);
+ tda7802->i2c = i2c;
+
+ tda7802->enable_reg = devm_regulator_get(dev, "enable");
+ if (IS_ERR(tda7802->enable_reg)) {
+ dev_err(dev, "Failed to get enable regulator\n");
+ return PTR_ERR(tda7802->enable_reg);
+ }
+
+ tda7802->regmap = devm_regmap_init_i2c(tda7802->i2c,
+ &tda7802_regmap_config);
+ if (IS_ERR(tda7802->regmap))
+ return PTR_ERR(tda7802->regmap);
+
+ err = devm_snd_soc_register_component(dev, &tda7802_component_driver,
+ &tda7802_dai_driver, 1);
+ if (err < 0)
+ dev_err(dev, "Failed to register codec: %d\n", err);
+ return err;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id tda7802_of_match[] = {
+ { .compatible = "st,tda7802" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, tda7802_of_match);
+#endif
+
+static const struct i2c_device_id tda7802_i2c_id[] = {
+ { "tda7802", tda7802_base },
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, tda7802_i2c_id);
+
+static struct i2c_driver tda7802_i2c_driver = {
+ .driver = {
+ .name = "tda7802-codec",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(tda7802_of_match),
+ },
+ .probe = tda7802_i2c_probe,
+ .id_table = tda7802_i2c_id,
+};
+module_i2c_driver(tda7802_i2c_driver);
+
+MODULE_DESCRIPTION("ASoC ST TDA7802 driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Rob Duncan <[email protected]>");
+MODULE_AUTHOR("Thomas Preston <[email protected]>");
--
2.21.0
On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
> Add an I2C based codec driver for ST TDA7802 amplifier. The amplifier
> supports 4 audio channels but can support up to 16 with multiple
> devices.
>
> Signed-off-by: Thomas Preston <[email protected]>
> Cc: Patrick Glaser <[email protected]>
> Cc: Rob Duncan <[email protected]>
> Cc: Nate Case <[email protected]>
> ---
> Changes since v1:
> - Use ALSA kcontrol interface to expose device controls to userland
> - Gain
> - Channel diagnostic mode
> - Impedance efficiency optimiser. I decided against setting this
> as a DT property since it seems like something that can be
> changed on the fly.
> - Add regmap default values
> - Channel unmute by default is added in a downstream patch.
> - I'm not sure if I should keep this since they're all zero,
> although there are other drivers will all-zero reg_defaults.
> - I believe the "//" style is used for SPDX headers in normal C source files.
> https://lwn.net/Articles/739183/
> - Drop the "enable" sysfs device attribute.
> - Don't set TDM format using magic numbers.
> - Set sample rate using hw_params.
> - Remove unecessary defines.
> - Use DAPM to handle AMP_ON.
> - Cosmetic fixups
>
> sound/soc/codecs/Kconfig | 6 +
> sound/soc/codecs/Makefile | 2 +
> sound/soc/codecs/tda7802.c | 509 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 517 insertions(+)
> create mode 100644 sound/soc/codecs/tda7802.c
>
> +++ b/sound/soc/codecs/tda7802.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * tda7802.c -- codec driver for ST TDA7802
> + *
> + * Copyright (C) 2016-2019 Tesla Motors, Inc.
> + */
Better to make the whole comment // see something like
sound/soc/codecs/cs47l35.c for an example.
> +static int tda7802_set_bias_level(struct snd_soc_component *component,
> + enum snd_soc_bias_level level)
> +{
> + const struct tda7802_priv *tda7802 =
> + snd_soc_component_get_drvdata(component);
> + struct snd_soc_dapm_context *dapm_context =
> + snd_soc_component_get_dapm(component);
> + const enum snd_soc_bias_level oldlevel =
> + snd_soc_dapm_get_bias_level(dapm_context);
> + int err = 0;
> +
> + dev_dbg(component->dev, "%s level %d\n", __func__, level);
> +
> + switch (level) {
> + case SND_SOC_BIAS_ON:
> + break;
> + case SND_SOC_BIAS_PREPARE:
> + break;
> + case SND_SOC_BIAS_STANDBY:
> + err = regulator_enable(tda7802->enable_reg);
> + if (err < 0) {
> + dev_err(component->dev, "Could not enable.\n");
> + return err;
> + }
> + dev_dbg(component->dev, "Regulator enabled\n");
> + msleep(ENABLE_DELAY_MS);
> +
> + if (oldlevel == SND_SOC_BIAS_OFF) {
> + dev_dbg(component->dev, "Syncing regcache\n");
> + err = regcache_sync(component->regmap);
> + if (err < 0)
> + dev_err(component->dev,
> + "Could not sync regcache, %d\n", err);
If your doing a regcache_sync I would probably have expected to
see calls to regcache_cache_only.
If the device needs syncing that implies the hardware registers
have lost state, so there is little point in writing to them
if they are unavailable/about to loose their state.
> + }
> + break;
> + case SND_SOC_BIAS_OFF:
> + regcache_mark_dirty(component->regmap);
> + err = regulator_disable(tda7802->enable_reg);
> + if (err < 0)
> + dev_err(component->dev, "Could not disable.\n");
> + break;
> + }
> +
> + return err;
> +}
Thanks,
Charles
On 30/07/2019 13:38, Charles Keepax wrote:
> On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
>> Add an I2C based codec driver for ST TDA7802 amplifier. The amplifier
>> supports 4 audio channels but can support up to 16 with multiple
>> devices.
>>
>> Signed-off-by: Thomas Preston <[email protected]>
>> Cc: Patrick Glaser <[email protected]>
>> Cc: Rob Duncan <[email protected]>
>> Cc: Nate Case <[email protected]>
>> ---
>> Changes since v1:
>> - Use ALSA kcontrol interface to expose device controls to userland
>> - Gain
>> - Channel diagnostic mode
>> - Impedance efficiency optimiser. I decided against setting this
>> as a DT property since it seems like something that can be
>> changed on the fly.
>> - Add regmap default values
>> - Channel unmute by default is added in a downstream patch.
>> - I'm not sure if I should keep this since they're all zero,
>> although there are other drivers will all-zero reg_defaults.
>> - I believe the "//" style is used for SPDX headers in normal C source files.
>> https://lwn.net/Articles/739183/
>> - Drop the "enable" sysfs device attribute.
>> - Don't set TDM format using magic numbers.
>> - Set sample rate using hw_params.
>> - Remove unecessary defines.
>> - Use DAPM to handle AMP_ON.
>> - Cosmetic fixups
>>
>> sound/soc/codecs/Kconfig | 6 +
>> sound/soc/codecs/Makefile | 2 +
>> sound/soc/codecs/tda7802.c | 509 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 517 insertions(+)
>> create mode 100644 sound/soc/codecs/tda7802.c
>>
>> +++ b/sound/soc/codecs/tda7802.c
>> @@ -0,0 +1,509 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * tda7802.c -- codec driver for ST TDA7802
>> + *
>> + * Copyright (C) 2016-2019 Tesla Motors, Inc.
>> + */
>
> Better to make the whole comment // see something like
> sound/soc/codecs/cs47l35.c for an example.
>
I will update to "//" style. Is this a new standard? There aren't many
comments like that in 4.14 (my target kernel) - I can see a lot more
in 5.3.
My intention was:
1. Apply the SPDX rules to SPDX bit. Documentation/process/license-rules.rst
2. Use multi-line comments for the rest. Documentation/process/coding-style.rst
>> +static int tda7802_set_bias_level(struct snd_soc_component *component,
>> + enum snd_soc_bias_level level)
>> +{
>> + const struct tda7802_priv *tda7802 =
>> + snd_soc_component_get_drvdata(component);
>> + struct snd_soc_dapm_context *dapm_context =
>> + snd_soc_component_get_dapm(component);
>> + const enum snd_soc_bias_level oldlevel =
>> + snd_soc_dapm_get_bias_level(dapm_context);
>> + int err = 0;
>> +
>> + dev_dbg(component->dev, "%s level %d\n", __func__, level);
>> +
>> + switch (level) {
>> + case SND_SOC_BIAS_ON:
>> + break;
>> + case SND_SOC_BIAS_PREPARE:
>> + break;
>> + case SND_SOC_BIAS_STANDBY:
>> + err = regulator_enable(tda7802->enable_reg);
>> + if (err < 0) {
>> + dev_err(component->dev, "Could not enable.\n");
>> + return err;
>> + }
>> + dev_dbg(component->dev, "Regulator enabled\n");
>> + msleep(ENABLE_DELAY_MS);
>> +
>> + if (oldlevel == SND_SOC_BIAS_OFF) {
>> + dev_dbg(component->dev, "Syncing regcache\n");
>> + err = regcache_sync(component->regmap);
>> + if (err < 0)
>> + dev_err(component->dev,
>> + "Could not sync regcache, %d\n", err);
>
> If your doing a regcache_sync I would probably have expected to
> see calls to regcache_cache_only.
>
> If the device needs syncing that implies the hardware registers
> have lost state, so there is little point in writing to them
> if they are unavailable/about to loose their state.
>
Ah, from the comments I thought I only needed to call regcache_mark_dirty...
>> + }
>> + break;
>> + case SND_SOC_BIAS_OFF:
>> + regcache_mark_dirty(component->regmap);
>> + err = regulator_disable(tda7802->enable_reg);
>> + if (err < 0)
>> + dev_err(component->dev, "Could not disable.\n");
>> + break;
>> + }
>> +
>> + return err;
>> +}
So I think the correct order is:
device_off:
regcache_cache_only
power-off (enable)
regcache_mark_dirty
device_on:
power-on (enable)
regcache_sync
I will double-check the register state is actually lost too. Fiddling
with the cache might be completely unnecessary.
Many thanks
On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
> index 000000000000..0f82a88bc1a4
> --- /dev/null
> +++ b/sound/soc/codecs/tda7802.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * tda7802.c -- codec driver for ST TDA7802
Please make the entire comment a C++ one so this looks intentional.
> +static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
> +{
> + const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;
Please write normal conditional statements to make the code easier to
read.
> + case SND_SOC_BIAS_STANDBY:
> + err = regulator_enable(tda7802->enable_reg);
> + if (err < 0) {
> + dev_err(component->dev, "Could not enable.\n");
> + return err;
> + }
> + dev_dbg(component->dev, "Regulator enabled\n");
> + msleep(ENABLE_DELAY_MS);
Is this delay needed by the device or is it for the regulator to ramp?
If it's for the regulator to ramp then the regulator should be doing it.
> + case SND_SOC_BIAS_OFF:
> + regcache_mark_dirty(component->regmap);
If the regulator is going off you should really be marking the device as
cache only.
> + err = regulator_disable(tda7802->enable_reg);
> + if (err < 0)
> + dev_err(component->dev, "Could not disable.\n");
Any non-zero value from regulator_disable() is an error, there's similar
error checking issues in other places.
> +static const struct snd_kcontrol_new tda7802_snd_controls[] = {
> + SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0),
> + SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0),
> + SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0),
> + SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0),
These look like simple on/off switches so should have Switch at the end
of the control name. It's also not clear to me why this is exported to
userspace - why would this change at runtime and won't any changes need
to be coordinated with whatever else is connected to the signal?
> + SOC_ENUM("Mute time", mute_time),
> + SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0),
> + SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0),
These are also Switch controls. There are *lots* of problems with
control names, see control-names.rst.
> +static const struct snd_soc_component_driver tda7802_component_driver = {
> + .set_bias_level = tda7802_set_bias_level,
> + .idle_bias_on = 1,
Any reason to keep the device powered up? It looks like the power on
process is just powering things up and writing the register cache out
and there's not that many registers so the delay is trivial.
> + tda7802->enable_reg = devm_regulator_get(dev, "enable");
> + if (IS_ERR(tda7802->enable_reg)) {
> + dev_err(dev, "Failed to get enable regulator\n");
It's better to print error codes if you have them and are printing a
diagnostic so people have more to go on when debugging problems.
On 30/07/2019 15:58, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
>
>> index 000000000000..0f82a88bc1a4
>> --- /dev/null
>> +++ b/sound/soc/codecs/tda7802.c
>> @@ -0,0 +1,509 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * tda7802.c -- codec driver for ST TDA7802
>
> Please make the entire comment a C++ one so this looks intentional.
>
Ok thanks.
>> +static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
>> +{
>> + const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;
>
> Please write normal conditional statements to make the code easier to
> read.
>
On it.
>> + case SND_SOC_BIAS_STANDBY:
>> + err = regulator_enable(tda7802->enable_reg);
>> + if (err < 0) {
>> + dev_err(component->dev, "Could not enable.\n");
>> + return err;
>> + }
>> + dev_dbg(component->dev, "Regulator enabled\n");
>> + msleep(ENABLE_DELAY_MS);
>
> Is this delay needed by the device or is it for the regulator to ramp?
> If it's for the regulator to ramp then the regulator should be doing it.
>
According to the datasheet the device itself takes 10ms to rise from 0V
after PLLen is enabled. There are additional rise times but they are
negligible with default capacitor configuration (which we have).
Good to know about the regulator rising configuration though. Thanks.
>> + case SND_SOC_BIAS_OFF:
>> + regcache_mark_dirty(component->regmap);
>
> If the regulator is going off you should really be marking the device as
> cache only.
>
Got it, thanks.
>> + err = regulator_disable(tda7802->enable_reg);
>> + if (err < 0)
>> + dev_err(component->dev, "Could not disable.\n");
>
> Any non-zero value from regulator_disable() is an error, there's similar
> error checking issues in other places.
>
I return the error at the end of the function, but I'll bring it back here
for consistency.
>> +static const struct snd_kcontrol_new tda7802_snd_controls[] = {
>> + SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0),
>> + SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0),
>> + SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0),
>> + SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0),
>
> These look like simple on/off switches so should have Switch at the end
> of the control name. It's also not clear to me why this is exported to
> userspace - why would this change at runtime and won't any changes need
> to be coordinated with whatever else is connected to the signal?
>
>> + SOC_ENUM("Mute time", mute_time),
>> + SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0),
>> + SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0),
>
> These are also Switch controls. There are *lots* of problems with
> control names, see control-names.rst.
>
Ok thanks, I didn't know about the "Switch" suffix, I will read
control-names.rst.
I will move Tristate to DT properties. I was also unsure about the
Impedance Efficiency Optimiser but the datasheet doesn't go into much
detail about this so I left it exposed.
They both seemed like user configurable options in the context of a
test rig, but I agree - who knows what this output might be connected
to in other systems. I will lock them down in DT. Thanks.
>> +static const struct snd_soc_component_driver tda7802_component_driver = {
>> + .set_bias_level = tda7802_set_bias_level,
>> + .idle_bias_on = 1,
>
> Any reason to keep the device powered up? It looks like the power on
> process is just powering things up and writing the register cache out
> and there's not that many registers so the delay is trivial.
>
Ah no, I think that's a mistake. I want the PLLen to switch off in
idle/suspend and the device should restore on wake.
>> + tda7802->enable_reg = devm_regulator_get(dev, "enable");
>> + if (IS_ERR(tda7802->enable_reg)) {
>> + dev_err(dev, "Failed to get enable regulator\n");
>
> It's better to print error codes if you have them and are printing a
> diagnostic so people have more to go on when debugging problems.
Yep on it.
Many thanks, I appreciate the feedback.
Hi Thomas,
again sorry for jumping in..
On 19-07-30 18:26, Thomas Preston wrote:
> On 30/07/2019 15:58, Mark Brown wrote:
> > On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
> >
> >> index 000000000000..0f82a88bc1a4
> >> --- /dev/null
> >> +++ b/sound/soc/codecs/tda7802.c
> >> @@ -0,0 +1,509 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * tda7802.c -- codec driver for ST TDA7802
> >
> > Please make the entire comment a C++ one so this looks intentional.
> >
>
> Ok thanks.
>
> >> +static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
> >> +{
> >> + const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;
> >
> > Please write normal conditional statements to make the code easier to
> > read.
> >
>
> On it.
>
> >> + case SND_SOC_BIAS_STANDBY:
> >> + err = regulator_enable(tda7802->enable_reg);
> >> + if (err < 0) {
> >> + dev_err(component->dev, "Could not enable.\n");
> >> + return err;
> >> + }
> >> + dev_dbg(component->dev, "Regulator enabled\n");
> >> + msleep(ENABLE_DELAY_MS);
> >
> > Is this delay needed by the device or is it for the regulator to ramp?
> > If it's for the regulator to ramp then the regulator should be doing it.
> >
>
> According to the datasheet the device itself takes 10ms to rise from 0V
> after PLLen is enabled. There are additional rise times but they are
> negligible with default capacitor configuration (which we have).
>
> Good to know about the regulator rising configuration though. Thanks.
Isn't it the regulator we mentioned to not use that because it is a
GPIO?
Regards,
Marco
> >> + case SND_SOC_BIAS_OFF:
> >> + regcache_mark_dirty(component->regmap);
> >
> > If the regulator is going off you should really be marking the device as
> > cache only.
> >
>
> Got it, thanks.
>
> >> + err = regulator_disable(tda7802->enable_reg);
> >> + if (err < 0)
> >> + dev_err(component->dev, "Could not disable.\n");
> >
> > Any non-zero value from regulator_disable() is an error, there's similar
> > error checking issues in other places.
> >
>
> I return the error at the end of the function, but I'll bring it back here
> for consistency.
>
> >> +static const struct snd_kcontrol_new tda7802_snd_controls[] = {
> >> + SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0),
> >> + SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0),
> >> + SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0),
> >> + SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0),
> >
> > These look like simple on/off switches so should have Switch at the end
> > of the control name. It's also not clear to me why this is exported to
> > userspace - why would this change at runtime and won't any changes need
> > to be coordinated with whatever else is connected to the signal?
> >
> >> + SOC_ENUM("Mute time", mute_time),
> >> + SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0),
> >> + SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0),
> >
> > These are also Switch controls. There are *lots* of problems with
> > control names, see control-names.rst.
> >
>
> Ok thanks, I didn't know about the "Switch" suffix, I will read
> control-names.rst.
>
> I will move Tristate to DT properties. I was also unsure about the
> Impedance Efficiency Optimiser but the datasheet doesn't go into much
> detail about this so I left it exposed.
>
> They both seemed like user configurable options in the context of a
> test rig, but I agree - who knows what this output might be connected
> to in other systems. I will lock them down in DT. Thanks.
>
> >> +static const struct snd_soc_component_driver tda7802_component_driver = {
> >> + .set_bias_level = tda7802_set_bias_level,
> >> + .idle_bias_on = 1,
> >
> > Any reason to keep the device powered up? It looks like the power on
> > process is just powering things up and writing the register cache out
> > and there's not that many registers so the delay is trivial.
> >
>
> Ah no, I think that's a mistake. I want the PLLen to switch off in
> idle/suspend and the device should restore on wake.
>
> >> + tda7802->enable_reg = devm_regulator_get(dev, "enable");
> >> + if (IS_ERR(tda7802->enable_reg)) {
> >> + dev_err(dev, "Failed to get enable regulator\n");
> >
> > It's better to print error codes if you have them and are printing a
> > diagnostic so people have more to go on when debugging problems.
>
> Yep on it.
>
> Many thanks, I appreciate the feedback.
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 31/07/2019 07:06, Marco Felsch wrote:
> Hi Thomas,
>
> again sorry for jumping in..
>
Np!
> On 19-07-30 18:26, Thomas Preston wrote:
>> On 30/07/2019 15:58, Mark Brown wrote:
>>> On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
>>>> + case SND_SOC_BIAS_STANDBY:
>>>> + err = regulator_enable(tda7802->enable_reg);
>>>> + if (err < 0) {
>>>> + dev_err(component->dev, "Could not enable.\n");
>>>> + return err;
>>>> + }
>>>> + dev_dbg(component->dev, "Regulator enabled\n");
>>>> + msleep(ENABLE_DELAY_MS);
>>>
>>> Is this delay needed by the device or is it for the regulator to ramp?
>>> If it's for the regulator to ramp then the regulator should be doing it.
>>>
>>
>> According to the datasheet the device itself takes 10ms to rise from 0V
>> after PLLen is enabled. There are additional rise times but they are
>> negligible with default capacitor configuration (which we have).
>>
>> Good to know about the regulator rising configuration though. Thanks.
>
> Isn't it the regulator we mentioned to not use that because it is a
> GPIO?
>
Yeah it is - I intend to switch PLLen to gpio API.