[alsa-devel] [PATCH v2 0/3] ASoC: Codecs: Add TDA7802 codec
Hi, This patch series adds a driver for the ST TDA7802 amplifier.
Thank you Mark Brown, Charles Keepax, Cezary Rojewski and Kirill Marinushkin for your time and useful feedback (on IRC too). Sorry for taking so long in getting back to you, there were quite a lot of changes!
Please let me know if there's anything else which needs changing.
Many thanks, Thomas
Thomas Preston (3): dt-bindings: ASoC: Add TDA7802 amplifier ASoC: Add codec driver for ST TDA7802 ASoC: TDA7802: Add turn-on diagnostic routine
.../devicetree/bindings/sound/tda7802.txt | 26 + sound/soc/codecs/Kconfig | 6 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tda7802.c | 693 ++++++++++++++++++ 4 files changed, 727 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt create mode 100644 sound/soc/codecs/tda7802.c
Signed-off-by: Thomas Preston thomas.preston@codethink.co.uk Cc: Patrick Glaser pglaser@tesla.com Cc: Rob Duncan rduncan@tesla.com Cc: Nate Case ncase@tesla.com --- .../devicetree/bindings/sound/tda7802.txt | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt new file mode 100644 index 000000000000..f80aaf4f1ba0 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tda7802.txt @@ -0,0 +1,26 @@ +ST TDA7802 audio processor + +This device supports I2C only. + +Required properties: + +- compatible : "st,tda7802" +- reg : the I2C address of the device +- enable-supply : a regulator spec for the PLLen pin + +Optional properties: + +- st,gain-ch13 : gain for channels 1 and 3 (range: 1-4) +- st,gain-ch24 : gain for channels 2 and 3 (range: 1-4) +- st,diagnostic-mode-ch13 : diagnotic mode for channels 1 and 3 + values: "Speaker" (default), "Booster" +- st,diagnostic-mode-ch24 : diagnotic mode for channels 2 and 4 + values: "Speaker" (default), "Booster" + +Example: + +amp: tda7802@6c { + compatible = "st,tda7802"; + reg = <0x6c>; + enable-supply = <&_enable_reg>; +};
On Tue, Jul 30, 2019 at 01:09:35PM +0100, Thomas Preston wrote:
Signed-off-by: Thomas Preston thomas.preston@codethink.co.uk Cc: Patrick Glaser pglaser@tesla.com Cc: Rob Duncan rduncan@tesla.com Cc: Nate Case ncase@tesla.com
.../devicetree/bindings/sound/tda7802.txt | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt new file mode 100644 index 000000000000..f80aaf4f1ba0 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tda7802.txt @@ -0,0 +1,26 @@ +ST TDA7802 audio processor
+This device supports I2C only.
+Required properties:
+- compatible : "st,tda7802" +- reg : the I2C address of the device +- enable-supply : a regulator spec for the PLLen pin
+Optional properties:
+- st,gain-ch13 : gain for channels 1 and 3 (range: 1-4) +- st,gain-ch24 : gain for channels 2 and 3 (range: 1-4)
I wouldn't have expected the gains to be available as a device tree setting.
+- st,diagnostic-mode-ch13 : diagnotic mode for channels 1 and 3
values: "Speaker" (default), "Booster"+- st,diagnostic-mode-ch24 : diagnotic mode for channels 2 and 4
values: "Speaker" (default), "Booster"+Example:
+amp: tda7802@6c {
- compatible = "st,tda7802";
- reg = <0x6c>;
- enable-supply = <&_enable_reg>;
+};
2.21.0
Thanks, Charles
Hi Charles,
sorry for jumping in..
On 19-07-30 13:27, Charles Keepax wrote:
On Tue, Jul 30, 2019 at 01:09:35PM +0100, Thomas Preston wrote:
Signed-off-by: Thomas Preston thomas.preston@codethink.co.uk Cc: Patrick Glaser pglaser@tesla.com Cc: Rob Duncan rduncan@tesla.com Cc: Nate Case ncase@tesla.com
.../devicetree/bindings/sound/tda7802.txt | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt new file mode 100644 index 000000000000..f80aaf4f1ba0 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tda7802.txt @@ -0,0 +1,26 @@ +ST TDA7802 audio processor
+This device supports I2C only.
+Required properties:
+- compatible : "st,tda7802" +- reg : the I2C address of the device +- enable-supply : a regulator spec for the PLLen pin
Shouldn't that be a pin called 'pllen-gpios'? IMHO I would not use a regulator for that.
Regards, Marco
+Optional properties:
+- st,gain-ch13 : gain for channels 1 and 3 (range: 1-4) +- st,gain-ch24 : gain for channels 2 and 3 (range: 1-4)
I wouldn't have expected the gains to be available as a device tree setting.
+- st,diagnostic-mode-ch13 : diagnotic mode for channels 1 and 3
values: "Speaker" (default), "Booster"+- st,diagnostic-mode-ch24 : diagnotic mode for channels 2 and 4
values: "Speaker" (default), "Booster"+Example:
+amp: tda7802@6c {
- compatible = "st,tda7802";
- reg = <0x6c>;
- enable-supply = <&_enable_reg>;
+};
2.21.0
Thanks, Charles
On 30/07/2019 14:12, Marco Felsch wrote:
Hi Charles,
sorry for jumping in..
On 19-07-30 13:27, Charles Keepax wrote:
On Tue, Jul 30, 2019 at 01:09:35PM +0100, Thomas Preston wrote:
Signed-off-by: Thomas Preston thomas.preston@codethink.co.uk Cc: Patrick Glaser pglaser@tesla.com Cc: Rob Duncan rduncan@tesla.com Cc: Nate Case ncase@tesla.com
.../devicetree/bindings/sound/tda7802.txt | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt new file mode 100644 index 000000000000..f80aaf4f1ba0 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tda7802.txt @@ -0,0 +1,26 @@ +ST TDA7802 audio processor
+This device supports I2C only.
+Required properties:
+- compatible : "st,tda7802" +- reg : the I2C address of the device +- enable-supply : a regulator spec for the PLLen pin
Shouldn't that be a pin called 'pllen-gpios'? IMHO I would not use a regulator for that.
Regards, Marco
Hi Marco, We have multiple amplifiers hooked up in a chain, and all the PLLens are connected to one GPIO. So we need to use a regulator so that i2c-TDA7802:00 doesn't turn off the PLLen which i2c-TDA7802:01 still requires.
This is why we use a regulator. Is there GPIO support for this?
Thanks, Thomas
On Tue, Jul 30, 2019 at 03:12:21PM +0100, Thomas Preston wrote:
On 30/07/2019 14:12, Marco Felsch wrote:
+- compatible : "st,tda7802" +- reg : the I2C address of the device +- enable-supply : a regulator spec for the PLLen pin
Shouldn't that be a pin called 'pllen-gpios'? IMHO I would not use a regulator for that.
Hi Marco, We have multiple amplifiers hooked up in a chain, and all the PLLens are connected to one GPIO. So we need to use a regulator so that i2c-TDA7802:00 doesn't turn off the PLLen which i2c-TDA7802:01 still requires.
This is why we use a regulator. Is there GPIO support for this?
If it's a GPIO not a regulator then it should be a GPIO not a regulator in the device tree. The device tree describes the hardware. There was some work on helping share GPIOs in the GPIO framework to accomodate GPIOs for regulator enables, you should be able to do something similar to what the regulator framework does.
On 30/07/2019 13:27, Charles Keepax wrote:
On Tue, Jul 30, 2019 at 01:09:35PM +0100, Thomas Preston wrote:
Signed-off-by: Thomas Preston thomas.preston@codethink.co.uk Cc: Patrick Glaser pglaser@tesla.com Cc: Rob Duncan rduncan@tesla.com Cc: Nate Case ncase@tesla.com
.../devicetree/bindings/sound/tda7802.txt | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt new file mode 100644 index 000000000000..f80aaf4f1ba0 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tda7802.txt @@ -0,0 +1,26 @@ +ST TDA7802 audio processor
+This device supports I2C only.
+Required properties:
+- compatible : "st,tda7802" +- reg : the I2C address of the device +- enable-supply : a regulator spec for the PLLen pin
+Optional properties:
+- st,gain-ch13 : gain for channels 1 and 3 (range: 1-4) +- st,gain-ch24 : gain for channels 2 and 3 (range: 1-4)
I wouldn't have expected the gains to be available as a device tree setting.
Ah, I forgot to update the docs from v1. Thanks
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 thomas.preston@codethink.co.uk Cc: Patrick Glaser pglaser@tesla.com Cc: Rob Duncan rduncan@tesla.com Cc: Nate Case ncase@tesla.com --- 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 rduncan@tesla.com"); +MODULE_AUTHOR("Thomas Preston thomas.preston@codethink.co.uk");
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 thomas.preston@codethink.co.uk Cc: Patrick Glaser pglaser@tesla.com Cc: Rob Duncan rduncan@tesla.com Cc: Nate Case ncase@tesla.com
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 thomas.preston@codethink.co.uk Cc: Patrick Glaser pglaser@tesla.com Cc: Rob Duncan rduncan@tesla.com Cc: Nate Case ncase@tesla.com
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.
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.
Add a debugfs device node which initiates the turn-on diagnostic routine feature of the TDA7802 amplifier. The four status registers (one per channel) are returned.
Signed-off-by: Thomas Preston thomas.preston@codethink.co.uk --- Changes since v1: - Rename speaker-test to (turn-on) diagnostics - Move turn-on diagnostic to debugfs as there is no standard ALSA interface for this kind of routine.
sound/soc/codecs/tda7802.c | 186 ++++++++++++++++++++++++++++++++++++- 1 file changed, 185 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c index 0f82a88bc1a4..74436212241d 100644 --- a/sound/soc/codecs/tda7802.c +++ b/sound/soc/codecs/tda7802.c @@ -5,6 +5,7 @@ * Copyright (C) 2016-2019 Tesla Motors, Inc. */
+#include <linux/debugfs.h> #include <linux/delay.h> #include <linux/i2c.h> #include <linux/module.h> @@ -26,6 +27,7 @@ #define TDA7802_IB5 0x05
#define TDA7802_DB0 0x10 +#define TDA7802_DB1 0x11 #define TDA7802_DB5 0x15
#define IB2_DIGITAL_MUTE_DISABLED (1 << 2) @@ -47,6 +49,17 @@ #define IB3_FORMAT_TDM16_DEV3 (6 << IB3_FORMAT_SHIFT) #define IB3_FORMAT_TDM16_DEV4 (7 << IB3_FORMAT_SHIFT)
+#define IB4_DIAG_MODE_ENABLE (1 << 0) + +#define IB5_AMPLIFIER_ON (1 << 0) + +#define DB0_STARTUP_DIAG_STATUS (1 << 6) + +#define DIAGNOSTIC_POLL_PERIOD_US 5000 +#define DIAGNOSTIC_TIMEOUT_US 5000000 +#define DIAGNOSTIC_SETTLE_MS 20 +#define NUM_IB 6 + enum tda7802_type { tda7802_base, }; @@ -55,6 +68,12 @@ struct tda7802_priv { struct i2c_client *i2c; struct regmap *regmap; struct regulator *enable_reg; + struct dentry *debugfs; + struct mutex diagnostic_mutex; +}; + +struct reg_update { + unsigned int reg, mask, val; };
static const struct reg_default tda7802_reg[] = { @@ -113,6 +132,19 @@ static const struct regmap_config tda7802_regmap_config = { .cache_type = REGCACHE_RBTREE, };
+/* The following bits need to be updated before diagnostics mode is set. */ +static const struct reg_update diagnostic_state[NUM_IB] = { + { TDA7802_IB0, 0, 0 }, + { TDA7802_IB1, 0, 0 }, + { TDA7802_IB2, + IB2_CH13_UNMUTED | IB2_CH24_UNMUTED | IB2_DIGITAL_MUTE_DISABLED, + IB2_CH13_UNMUTED | IB2_CH24_UNMUTED | IB2_DIGITAL_MUTE_DISABLED, + }, + { TDA7802_IB3, 0, 0 }, + { TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0 }, + { TDA7802_IB5, IB5_AMPLIFIER_ON, 0 }, +}; + static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute) { const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED; @@ -414,7 +446,6 @@ static const struct snd_kcontrol_new tda7802_snd_controls[] = { 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), @@ -432,7 +463,160 @@ static const struct snd_soc_dapm_route tda7802_dapm_routes[] = { { "SPK", NULL, "DAC" }, };
+static int tda7802_bulk_update(struct regmap *map, struct reg_update *update, + size_t update_count) +{ + int i, err; + + for (i = 0; i < update_count; i++) { + err = regmap_update_bits(map, update[i].reg, update[i].mask, + update[i].val); + if (err < 0) + return err; + } + + return i; +} + +static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status) +{ + struct device *dev = &tda7802->i2c->dev; + int err_status, err; + unsigned int val; + u8 state[NUM_IB]; + + dev_dbg(dev, "%s\n", __func__); + + /* Save state and prepare for diagnostic */ + err = regmap_bulk_read(tda7802->regmap, TDA7802_IB0, state, + ARRAY_SIZE(state)); + if (err < 0) { + dev_err(dev, "Could not save device state, %d\n", err); + return err; + } + + err = tda7802_bulk_update(tda7802->regmap, diagnostic_state, + ARRAY_SIZE(diagnostic_state)); + if (err < 0) { + dev_err(dev, "Could not prepare for diagnostics, %d\n", err); + goto diagnostic_restore; + } + + /* We must wait 20ms for device to settle, otherwise diagnostics will + * not start and regmap poll will timeout. + */ + msleep(DIAGNOSTIC_SETTLE_MS); + + /* Turn on diagnostic */ + err = regmap_update_bits(tda7802->regmap, TDA7802_IB4, + IB4_DIAG_MODE_ENABLE, IB4_DIAG_MODE_ENABLE); + if (err < 0) { + dev_err(dev, "Could not enable diagnostic mode, %d\n", err); + goto diagnostic_restore; + } + + /* Wait until DB0_STARTUP_DIAG_STATUS is set, then read status */ + err_status = regmap_read_poll_timeout(tda7802->regmap, TDA7802_DB0, val, + val & DB0_STARTUP_DIAG_STATUS, + DIAGNOSTIC_POLL_PERIOD_US, + DIAGNOSTIC_TIMEOUT_US); + if (err_status < 0) { + dev_err(dev, "Diagnostic did not complete, err %d, reg %x", + err_status, val); + goto diagnostic_restore; + } + + err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4); + if (err < 0) { + dev_err(dev, "Could not read channel status, %d\n", err); + goto diagnostic_restore; + } + +diagnostic_restore: + err = regmap_bulk_write(tda7802->regmap, TDA7802_IB0, state, + ARRAY_SIZE(state)); + if (err < 0) + dev_err(dev, "Could not restore state, %d\n", err); + + if (err_status < 0) + return err_status; + else + return err; +} + +static int tda7802_diagnostic_show(struct seq_file *f, void *p) +{ + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL); + struct tda7802_priv *tda7802 = f->private; + u8 status[4] = { 0 }; + int i, err; + + mutex_lock(&tda7802->diagnostic_mutex); + err = run_turn_on_diagnostic(tda7802, status); + mutex_unlock(&tda7802->diagnostic_mutex); + if (err < 0) + return err; + + for (i = 0; i < ARRAY_SIZE(status); i++) + seq_printf(f, "%02x: %02x\n", TDA7802_DB1+i, status[i]); + + return 0; +} + +static int tda7802_diagnostic_open(struct inode *inode, struct file *file) +{ + return single_open(file, tda7802_diagnostic_show, inode->i_private); +} + +static const struct file_operations tda7802_diagnostic_fops = { + .open = tda7802_diagnostic_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +static int tda7802_probe(struct snd_soc_component *component) +{ + struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component); + struct device *dev = &tda7802->i2c->dev; + int err; + + tda7802->debugfs = debugfs_create_dir(dev_name(dev), NULL); + if (IS_ERR_OR_NULL(tda7802->debugfs)) { + dev_info(dev, + "Failed to create debugfs node, err %ld\n", + PTR_ERR(tda7802->debugfs)); + return 0; + } + + mutex_init(&tda7802->diagnostic_mutex); + err = debugfs_create_file("diagnostic", 0444, tda7802->debugfs, tda7802, + &tda7802_diagnostic_fops); + if (err < 0) { + dev_err(dev, + "debugfs: Failed to create diagnostic node, err %d\n", + err); + goto cleanup_diagnostic; + } + + return 0; + +cleanup_diagnostic: + mutex_destroy(&tda7802->diagnostic_mutex); + return err; +} + +static void tda7802_remove(struct snd_soc_component *component) +{ + struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component); + + debugfs_remove_recursive(tda7802->debugfs); + mutex_destroy(&tda7802->diagnostic_mutex); +} + static const struct snd_soc_component_driver tda7802_component_driver = { + .probe = tda7802_probe, + .remove = tda7802_remove, .set_bias_level = tda7802_set_bias_level, .idle_bias_on = 1, .suspend_bias_off = 1,
On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
Add a debugfs device node which initiates the turn-on diagnostic routine feature of the TDA7802 amplifier. The four status registers (one per channel) are returned.
Signed-off-by: Thomas Preston thomas.preston@codethink.co.uk
Changes since v1:
- Rename speaker-test to (turn-on) diagnostics
- Move turn-on diagnostic to debugfs as there is no standard ALSA interface for this kind of routine.
sound/soc/codecs/tda7802.c | 186 ++++++++++++++++++++++++++++++++++++- 1 file changed, 185 insertions(+), 1 deletion(-)
+static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
size_t update_count)+{
- int i, err;
- for (i = 0; i < update_count; i++) {
err = regmap_update_bits(map, update[i].reg, update[i].mask,update[i].val);if (err < 0)return err;- }
- return i;
+}
This could probably be removed using regmap_multi_reg_write.
+static int tda7802_probe(struct snd_soc_component *component) +{
- struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
- struct device *dev = &tda7802->i2c->dev;
- int err;
- tda7802->debugfs = debugfs_create_dir(dev_name(dev), NULL);
- if (IS_ERR_OR_NULL(tda7802->debugfs)) {
dev_info(dev,"Failed to create debugfs node, err %ld\n",PTR_ERR(tda7802->debugfs));return 0;- }
- mutex_init(&tda7802->diagnostic_mutex);
- err = debugfs_create_file("diagnostic", 0444, tda7802->debugfs, tda7802,
&tda7802_diagnostic_fops);- if (err < 0) {
dev_err(dev,"debugfs: Failed to create diagnostic node, err %d\n",err);goto cleanup_diagnostic;- }
You shouldn't be failing the driver probe if debugfs fails, it should be purely optional.
Thanks, Charles
Hi, Thanks for getting back to me so quickly.
On 30/07/2019 13:41, Charles Keepax wrote:
On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
Add a debugfs device node which initiates the turn-on diagnostic routine feature of the TDA7802 amplifier. The four status registers (one per channel) are returned.
Signed-off-by: Thomas Preston thomas.preston@codethink.co.uk
Changes since v1:
- Rename speaker-test to (turn-on) diagnostics
- Move turn-on diagnostic to debugfs as there is no standard ALSA interface for this kind of routine.
sound/soc/codecs/tda7802.c | 186 ++++++++++++++++++++++++++++++++++++- 1 file changed, 185 insertions(+), 1 deletion(-)
+static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
size_t update_count)+{
- int i, err;
- for (i = 0; i < update_count; i++) {
err = regmap_update_bits(map, update[i].reg, update[i].mask,update[i].val);if (err < 0)return err;- }
- return i;
+}
This could probably be removed using regmap_multi_reg_write.
The problem is that I want to retain the state of the other bits in those registers. Maybe I should make a copy of the backed up state, set the bits I want to off-device, then either:
1. Write the changes with regmap_multi_reg_write 2. Write all six regs again (if my device doesn't support the multi_reg)
+static int tda7802_probe(struct snd_soc_component *component) +{
- struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
- struct device *dev = &tda7802->i2c->dev;
- int err;
- tda7802->debugfs = debugfs_create_dir(dev_name(dev), NULL);
- if (IS_ERR_OR_NULL(tda7802->debugfs)) {
dev_info(dev,"Failed to create debugfs node, err %ld\n",PTR_ERR(tda7802->debugfs));return 0;- }
- mutex_init(&tda7802->diagnostic_mutex);
- err = debugfs_create_file("diagnostic", 0444, tda7802->debugfs, tda7802,
&tda7802_diagnostic_fops);- if (err < 0) {
dev_err(dev,"debugfs: Failed to create diagnostic node, err %d\n",err);goto cleanup_diagnostic;- }
You shouldn't be failing the driver probe if debugfs fails, it should be purely optional.
Got it, thanks.
On Tue, Jul 30, 2019 at 03:04:19PM +0100, Thomas Preston wrote:
On 30/07/2019 13:41, Charles Keepax wrote:
+static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
size_t update_count)+{
- int i, err;
- for (i = 0; i < update_count; i++) {
err = regmap_update_bits(map, update[i].reg, update[i].mask,update[i].val);if (err < 0)return err;- }
- return i;
+}
This could probably be removed using regmap_multi_reg_write.
The problem is that I want to retain the state of the other bits in those registers. Maybe I should make a copy of the backed up state, set the bits I want to off-device, then either:
- Write the changes with regmap_multi_reg_write
- Write all six regs again (if my device doesn't support the multi_reg)
Nah sorry my bad you are probably better off they way you are.
Thanks, Charles
On Tue, Jul 30, 2019 at 03:04:19PM +0100, Thomas Preston wrote:
On 30/07/2019 13:41, Charles Keepax wrote:
This could probably be removed using regmap_multi_reg_write.
The problem is that I want to retain the state of the other bits in those registers. Maybe I should make a copy of the backed up state, set the bits I want to off-device, then either:
- Write the changes with regmap_multi_reg_write
- Write all six regs again (if my device doesn't support the multi_reg)
Or make this a regmap function, there's nothing device specific about it.
On 30/07/2019 15:20, Mark Brown wrote:
On Tue, Jul 30, 2019 at 03:04:19PM +0100, Thomas Preston wrote:
On 30/07/2019 13:41, Charles Keepax wrote:
This could probably be removed using regmap_multi_reg_write.
The problem is that I want to retain the state of the other bits in those registers. Maybe I should make a copy of the backed up state, set the bits I want to off-device, then either:
- Write the changes with regmap_multi_reg_write
- Write all six regs again (if my device doesn't support the multi_reg)
Or make this a regmap function, there's nothing device specific about it.
I did wonder why regmap didn't have an multi-update function. If appropriate, I will add this in.
On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
- struct dentry *debugfs;
- struct mutex diagnostic_mutex;
+};
It is unclear what this mutex usefully protects, it only gets taken when writing to the debugfs file to trigger this diagnostic mode but doesn't do anything to control interactions with any other code path in the driver.
+static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status) +{
- struct device *dev = &tda7802->i2c->dev;
- int err_status, err;
- unsigned int val;
- u8 state[NUM_IB];
- /* We must wait 20ms for device to settle, otherwise diagnostics will
* not start and regmap poll will timeout.*/- msleep(DIAGNOSTIC_SETTLE_MS);
The comment and define might go out of sync...
- err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4);
- if (err < 0) {
dev_err(dev, "Could not read channel status, %d\n", err);goto diagnostic_restore;- }
...but here we use a magic number for the array size :(
+static int tda7802_diagnostic_show(struct seq_file *f, void *p) +{
- char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
We neither use nor free buf?
+static int tda7802_probe(struct snd_soc_component *component) +{
- struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
- struct device *dev = &tda7802->i2c->dev;
- int err;
Why is this done at the component level?
On 30/07/2019 15:19, Mark Brown wrote:
On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
- struct dentry *debugfs;
- struct mutex diagnostic_mutex;
+};
It is unclear what this mutex usefully protects, it only gets taken when writing to the debugfs file to trigger this diagnostic mode but doesn't do anything to control interactions with any other code path in the driver.
If another process reads the debugfs node "diagnostic" while the turn-on diagnostic mode is running, this mutex prevents the second process restarting the diagnostics.
This is redundant if debugfs reads are atomic, but I don't think they are.
+static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status) +{
- struct device *dev = &tda7802->i2c->dev;
- int err_status, err;
- unsigned int val;
- u8 state[NUM_IB];
- /* We must wait 20ms for device to settle, otherwise diagnostics will
* not start and regmap poll will timeout.*/- msleep(DIAGNOSTIC_SETTLE_MS);
The comment and define might go out of sync...
Thanks, I will remove the 20ms but keep the comment here.
- err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4);
- if (err < 0) {
dev_err(dev, "Could not read channel status, %d\n", err);goto diagnostic_restore;- }
...but here we use a magic number for the array size :(
Thanks, will update.
+static int tda7802_diagnostic_show(struct seq_file *f, void *p) +{
- char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
We neither use nor free buf?
+static int tda7802_probe(struct snd_soc_component *component) +{
- struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
- struct device *dev = &tda7802->i2c->dev;
- int err;
Why is this done at the component level?
Argh my bad, a previous iteration required the buf and component. I missed this, sorry for the noise.
Thanks for feedback, I'll go back and tend to all of this.
On Tue, Jul 30, 2019 at 04:25:56PM +0100, Thomas Preston wrote:
On 30/07/2019 15:19, Mark Brown wrote:
It is unclear what this mutex usefully protects, it only gets taken when writing to the debugfs file to trigger this diagnostic mode but doesn't do anything to control interactions with any other code path in the driver.
If another process reads the debugfs node "diagnostic" while the turn-on diagnostic mode is running, this mutex prevents the second process restarting the diagnostics.
This is redundant if debugfs reads are atomic, but I don't think they are.
Like I say it's not just debugfs though, there's the standard driver interface too.
On 30/07/2019 16:50, Mark Brown wrote:
On Tue, Jul 30, 2019 at 04:25:56PM +0100, Thomas Preston wrote:
On 30/07/2019 15:19, Mark Brown wrote:
It is unclear what this mutex usefully protects, it only gets taken when writing to the debugfs file to trigger this diagnostic mode but doesn't do anything to control interactions with any other code path in the driver.
If another process reads the debugfs node "diagnostic" while the turn-on diagnostic mode is running, this mutex prevents the second process restarting the diagnostics.
This is redundant if debugfs reads are atomic, but I don't think they are.
Like I say it's not just debugfs though, there's the standard driver interface too.
Ah right, I understand. So if we run the turn-on diagnostics routine, there's nothing stopping anyone from interacting with the device in other ways.
I guess there's no way to share that mutex with ALSA? In that case, it doesn't matter if this mutex is there or not - this feature is incompatible. How compatible do debugfs interfaces have to be? I was under the impression anything goes. I would argue that the debugfs is better off for having the mutex so that no one re-reads "diagnostic" within the 5s poll timeout.
Alternatively, this diagnostic feature could be handled with an external-handler kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
What would be acceptable?
On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
On 30/07/2019 16:50, Mark Brown wrote:
On Tue, Jul 30, 2019 at 04:25:56PM +0100, Thomas Preston wrote:
On 30/07/2019 15:19, Mark Brown wrote:
It is unclear what this mutex usefully protects, it only gets taken when writing to the debugfs file to trigger this diagnostic mode but doesn't do anything to control interactions with any other code path in the driver.
If another process reads the debugfs node "diagnostic" while the turn-on diagnostic mode is running, this mutex prevents the second process restarting the diagnostics.
This is redundant if debugfs reads are atomic, but I don't think they are.
Like I say it's not just debugfs though, there's the standard driver interface too.
Ah right, I understand. So if we run the turn-on diagnostics routine, there's nothing stopping anyone from interacting with the device in other ways.
I guess there's no way to share that mutex with ALSA? In that case, it doesn't matter if this mutex is there or not - this feature is incompatible. How compatible do debugfs interfaces have to be? I was under the impression anything goes. I would argue that the debugfs is better off for having the mutex so that no one re-reads "diagnostic" within the 5s poll timeout.
Alternatively, this diagnostic feature could be handled with an external-handler kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
What would be acceptable?
You could take the DAPM mutex in your debugfs handler that would prevent any changes to the cards power state whilst your debug stuff is running.
Thanks, Charles
On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
On 30/07/2019 16:50, Mark Brown wrote:
Like I say it's not just debugfs though, there's the standard driver interface too.
Ah right, I understand. So if we run the turn-on diagnostics routine, there's nothing stopping anyone from interacting with the device in other ways.
I guess there's no way to share that mutex with ALSA? In that case, it doesn't matter if this mutex is there or not - this feature is incompatible. How compatible do debugfs interfaces have to be? I was under the impression anything goes. I would argue that the debugfs is better off for having the mutex so that no one re-reads "diagnostic" within the 5s poll timeout.
It's not really something that's supported; like Charles says the DAPM mutex is exposed but if the regular controls would still be able to do stuff. It is kind of a "you broke it, you fix it" thing but on the other hand it's better to make things safer if we can since it might not be obvious later on why things are broken.
Alternatively, this diagnostic feature could be handled with an external-handler kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
What would be acceptable?
Yes, that's definitely doable - we've got some other drivers with similar things like calibration triggers exposed that way.
On 02/08/2019 00:42, Mark Brown wrote:
On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
On 30/07/2019 16:50, Mark Brown wrote:
Like I say it's not just debugfs though, there's the standard driver interface too.
Ah right, I understand. So if we run the turn-on diagnostics routine, there's nothing stopping anyone from interacting with the device in other ways.
I guess there's no way to share that mutex with ALSA? In that case, it doesn't matter if this mutex is there or not - this feature is incompatible. How compatible do debugfs interfaces have to be? I was under the impression anything goes. I would argue that the debugfs is better off for having the mutex so that no one re-reads "diagnostic" within the 5s poll timeout.
It's not really something that's supported; like Charles says the DAPM mutex is exposed but if the regular controls would still be able to do stuff. It is kind of a "you broke it, you fix it" thing but on the other hand it's better to make things safer if we can since it might not be obvious later on why things are broken.
Alternatively, this diagnostic feature could be handled with an external-handler kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
What would be acceptable?
Yes, that's definitely doable - we've got some other drivers with similar things like calibration triggers exposed that way.
One problem with using a kcontrol as a trigger for the turn-on diagnostic is that the diagnostic routine has a "return value".
It goes like this: - Bring device to low-quiescent state - Initiate diagnostics - Poll for diagnostics-complete bit - Read the four channel status registers
The final read clears the status registers, so this isn't something I can just do with regmap.
One idea I had was to initiate the turn-on diagnostics using a kcontrol, whose handler saves the four channel status registers and an epoch in tda7802_priv. Then this can be read from debugfs. But it seems strange to have to turn on this control over here, then go over there and read this value.
Hm, maybe a better idea is to have the turn on diagnostic only run on device probe (as its name suggests!), and print something to dmesg:
modprobe tda7802 turn_on_diagnostic=1
tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04
Kirill Marinushkin mentioned this in the first review [0], it just didn't really sink in until now!
On Fri, Aug 02, 2019 at 09:32:17AM +0100, Thomas Preston wrote:
On 02/08/2019 00:42, Mark Brown wrote:
Yes, that's definitely doable - we've got some other drivers with similar things like calibration triggers exposed that way.
One problem with using a kcontrol as a trigger for the turn-on diagnostic is that the diagnostic routine has a "return value".
You can use a read only control for the readback, or just have it be triggered by overwriting the readback value. You can cache the result.
Hm, maybe a better idea is to have the turn on diagnostic only run on device probe (as its name suggests!), and print something to dmesg:
modprobe tda7802 turn_on_diagnostic=1
tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04
Kirill Marinushkin mentioned this in the first review [0], it just didn't really sink in until now!
You could do that too, yeah. Depends on what this is diagnosing and if that'd be useful.
On 02/08/2019 12:10, Mark Brown wrote:
On Fri, Aug 02, 2019 at 09:32:17AM +0100, Thomas Preston wrote:
On 02/08/2019 00:42, Mark Brown wrote:
Yes, that's definitely doable - we've got some other drivers with similar things like calibration triggers exposed that way.
One problem with using a kcontrol as a trigger for the turn-on diagnostic is that the diagnostic routine has a "return value".
You can use a read only control for the readback, or just have it be triggered by overwriting the readback value. You can cache the result.
Keeping the trigger and result together like that would be better I think, although the routine isn't supposed to run mid way through playback. If we're mid playback the debugfs routine has to turn off AMP_ON, take the device back to a known state, run diagnostics, then restore. Which causes a gap in the audible sound.
Hm, maybe a better idea is to have the turn on diagnostic only run on device probe (as its name suggests!), and print something to dmesg:
modprobe tda7802 turn_on_diagnostic=1
tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04
Kirill Marinushkin mentioned this in the first review [0], it just didn't really sink in until now!
You could do that too, yeah. Depends on what this is diagnosing and if that'd be useful.
The diagnostic status bits describe situations such as: - open load (no speaker connected) - short to GND - short to VCC - etc
The intention is to test if all the speakers are connected. So, one might have a self test which runs the diagnostic and verifies it outputs:
00 00 00 00
For example, on my test rig there is only one speaker connected. So it reads:
04 04 00 04
Where the second bit is "open load". So this would fail the test.
So in the kcontrol case the test would be something like:
amixer sset "AMP1 turn on diagnostic" on amixer sget "AMP1 diagnostic"
And the module parameter case:
rmmod tda7802 modprobe tda7802 turn_on_diagnostic=1 dmesg | grep "Turn on diagnostic 04 04 04 04" rmmod tda7802 modprobe tda7802
I think the module parameter method is more appropriate for a "Turn-on diagnostic", even though I don't really like grepping dmesg for the result. I'll go ahead and implement that unless anyone has a particular preference for the kcontrol-trigger.
Thanks
On Fri, Aug 02, 2019 at 03:51:09PM +0100, Thomas Preston wrote:
On 02/08/2019 12:10, Mark Brown wrote:
You can use a read only control for the readback, or just have it be triggered by overwriting the readback value. You can cache the result.
Keeping the trigger and result together like that would be better I think, although the routine isn't supposed to run mid way through playback. If we're mid playback the debugfs routine has to turn off AMP_ON, take the device back to a known state, run diagnostics, then restore. Which causes a gap in the audible sound.
Whatever method is used to do the triggering can always return -EBUSY when you someone tries to do so during playback.
Kirill Marinushkin mentioned this in the first review [0], it just didn't really sink in until now!
You could do that too, yeah. Depends on what this is diagnosing and if that'd be useful.
The diagnostic status bits describe situations such as:
- open load (no speaker connected)
- short to GND
- short to VCC
- etc
The intention is to test if all the speakers are connected. So, one might have a self test which runs the diagnostic and verifies it outputs:
...
I think the module parameter method is more appropriate for a "Turn-on diagnostic", even though I don't really like grepping dmesg for the result. I'll go ahead and implement that unless anyone has a particular preference for the kcontrol-trigger.
Right. It's not ideal for use in production systems for example but perhaps fine for support techs or whoever. Up to you anyway.
participants (4)
-
Charles Keepax -
Marco Felsch -
Mark Brown -
Thomas Preston