[PATCH v1 0/4] ASoC: Codecs: Add TDA7802 codec

[PATCH v1 0/4] ASoC: Codecs: Add TDA7802 codec

[Thomas Preston]

This patch series adds a driver for the ST TDA7802 amplifier.

Thanks,
Thomas

Cc: Patrick Glaser <pglaser@tesla.com>
Cc: Rob Duncan <rduncan@tesla.com>
Cc: Nate Case <ncase@tesla.com>

Thomas Preston (4):
  dt-bindings: ASoC: Add TDA7802 amplifier
  ASoC: Add codec driver for ST TDA7802
  ASoC: tda7802: Add enable device attribute
  ASoC: tda7802: Add speaker-test sysfs

 .../devicetree/bindings/sound/tda7802.txt          |  26 +
 sound/soc/codecs/Kconfig                           |   6 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/tda7802.c                         | 815 +++++++++++++++++++++
 4 files changed, 849 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
 create mode 100644 sound/soc/codecs/tda7802.c

-- 
2.11.0

[PATCH v1 1/4] dt-bindings: ASoC: Add TDA7802 amplifier

[Thomas Preston]

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 = <&amp_enable_reg>;
+};
-- 
2.11.0

[PATCH v1 2/4] ASoC: Add codec driver for ST TDA7802

[Thomas Preston]

Add an I2C based codec driver for ST TDA7802 amplifier. By default, the
amplifier supports 4 audio channels but can support up to 16 with
multiple devices. Input is configurable for I2S or TDM.

The unified device properties API is used to get board-specific config from
device tree / ACPI.

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>
---
 sound/soc/codecs/Kconfig   |   6 +
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/tda7802.c | 580 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 588 insertions(+)
 create mode 100644 sound/soc/codecs/tda7802.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index e730d47ac85b..1d30c2333cb1 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -176,6 +176,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
@@ -1078,6 +1079,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 aa7720a7a0aa..fc3fc672bc4b 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -187,6 +187,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
@@ -460,6 +461,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..38ca52de85f0
--- /dev/null
+++ b/sound/soc/codecs/tda7802.c
@@ -0,0 +1,580 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * tda7802.c  --  codec driver for ST TDA7802
+ *
+ * Copyright (C) 2016-2019 Tesla Motors, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ */
+
+#include <linux/regulator/consumer.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <sound/soc.h>
+
+enum tda7802_type {
+	tda7802_base,
+};
+
+#define I2C_BUS				   4
+
+#define CHANNELS_PER_AMP		   4
+#define MAX_NUM_AMPS			   4
+
+#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_DB1			   0x11
+#define TDA7802_DB2			   0x12
+#define TDA7802_DB3			   0x13
+#define TDA7802_DB4			   0x14
+#define TDA7802_DB5			   0x15
+
+#define IB0_CH4_TRISTATE_ON		   (1 << 7)
+#define IB0_CH3_TRISTATE_ON		   (1 << 6)
+#define IB0_CH2_TRISTATE_ON		   (1 << 5)
+#define IB0_CH1_TRISTATE_ON		   (1 << 4)
+#define IB0_CH4_HIGH_EFF		   (0 << 3)
+#define IB0_CH4_CLASS_AB		   (1 << 3)
+#define IB0_CH3_HIGH_EFF		   (0 << 2)
+#define IB0_CH3_CLASS_AB		   (1 << 2)
+#define IB0_CH2_HIGH_EFF		   (0 << 0)
+#define IB0_CH2_CLASS_AB		   (1 << 1)
+#define IB0_CH1_HIGH_EFF		   (0 << 0)
+#define IB0_CH1_CLASS_AB		   (1 << 0)
+
+#define IB1_REAR_IMPEDANCE_2_OHM	   (0 << 7)
+#define IB1_REAR_IMPEDANCE_4_OHM	   (1 << 7)
+#define IB1_LONG_DIAG_TIMING_x1		   (0 << 5)
+#define IB1_LONG_DIAG_TIMING_x2		   (1 << 5)
+#define IB1_LONG_DIAG_TIMING_x4		   (2 << 5)
+#define IB1_LONG_DIAG_TIMING_x8		   (3 << 5)
+#define IB1_GAIN_CH13_GV1		   (0 << 3)
+#define IB1_GAIN_CH13_GV2		   (1 << 3)
+#define IB1_GAIN_CH13_GV3		   (2 << 3)
+#define IB1_GAIN_CH13_GV4		   (3 << 3)
+#define IB1_GAIN_CH24_GV1		   (0 << 1)
+#define IB1_GAIN_CH24_GV2		   (1 << 1)
+#define IB1_GAIN_CH24_GV3		   (2 << 1)
+#define IB1_GAIN_CH24_GV4		   (3 << 1)
+#define IB1_DIGITAL_GAIN_BOOST		   (1 << 0)
+
+#define IB2_MUTE_TIME_1_MS		   (0 << 5)
+#define IB2_MUTE_TIME_5_MS		   (1 << 5)
+#define IB2_MUTE_TIME_11_MS		   (2 << 5)
+#define IB2_MUTE_TIME_23_MS		   (3 << 5)
+#define IB2_MUTE_TIME_46_MS		   (4 << 5)
+#define IB2_MUTE_TIME_92_MS		   (5 << 5)
+#define IB2_MUTE_TIME_185_MS		   (6 << 5)
+#define IB2_MUTE_TIME_371_MS		   (7 << 5)
+#define IB2_CH13_UNMUTED		   (1 << 4)
+#define IB2_CH24_UNMUTED		   (1 << 3)
+#define IB2_DIGITAL_MUTE_DISABLED	   (1 << 2)
+#define IB2_AUTOMUTE_THRESHOLD_5V3	   (0 << 1)
+#define IB2_AUTOMUTE_THRESHOLD_7V3	   (1 << 1)
+#define IB2_FRONT_IMPEDANCE_2_OHM	   (0 << 0)
+#define IB2_FRONT_IMPEDANCE_4_OHM	   (1 << 0)
+
+#define IB3_SAMPLE_RATE_44_KHZ		   (0 << 6)
+#define IB3_SAMPLE_RATE_48_KHZ		   (1 << 6)
+#define IB3_SAMPLE_RATE_96_KHZ		   (2 << 6)
+#define IB3_SAMPLE_RATE_192_KHZ		   (3 << 6)
+#define IB3_FORMAT_I2S			   (0 << 3)
+#define IB3_FORMAT_TDM4			   (1 << 3)
+#define IB3_FORMAT_TDM8_DEV1		   (2 << 3)
+#define IB3_FORMAT_TDM8_DEV2		   (3 << 3)
+#define IB3_FORMAT_TDM16_DEV1		   (4 << 3)
+#define IB3_FORMAT_TDM16_DEV2		   (5 << 3)
+#define IB3_FORMAT_TDM16_DEV3		   (6 << 3)
+#define IB3_FORMAT_TDM16_DEV4		   (7 << 3)
+#define IB3_I2S_FRAME_PERIOD_64		   (0 << 2)
+#define IB3_I2S_FRAME_PERIOD_50		   (1 << 2)
+#define IB3_PLL_CLOCK_DITHER_ENABLE	   (1 << 1)
+#define IB3_HIGH_PASS_FILTER_ENABLE	   (1 << 0)
+
+static const u8 IB3_FORMAT[4][4] = {
+	/* 1x amp, 4 channels */
+	{ IB3_FORMAT_TDM4 },
+	/* 2x amp, 8 channels */
+	{ IB3_FORMAT_TDM8_DEV1,
+		IB3_FORMAT_TDM8_DEV2 },
+	/* 3x amp not supported */
+	{ },
+	/* 4x amp, 16 channels */
+	{ IB3_FORMAT_TDM16_DEV1,
+		IB3_FORMAT_TDM16_DEV2,
+		IB3_FORMAT_TDM16_DEV3,
+		IB3_FORMAT_TDM16_DEV4 },
+};
+
+#define IB4_NOISE_GATE_DISABLE		   (1 << 7)
+#define IB4_SHORT_FAULT_ON_CDDIAG_YES	   (0 << 6)
+#define IB4_SHORT_FAULT_ON_CDDIAG_NO	   (1 << 6)
+#define IB4_OFFSET_ON_CDDIAG_YES	   (0 << 5)
+#define IB4_OFFSET_ON_CDDIAG_NO		   (1 << 5)
+#define IB4_AC_DIAG_CURRENT_THRESHOLD_HIGH (0 << 4)
+#define IB4_AC_DIAG_CURRENT_THRESHOLD_LOW  (1 << 4)
+#define IB4_AC_DIAG_ENABLE		   (1 << 3)
+#define IB4_CH13_DIAG_MODE_SPEAKER	   (0 << 2)
+#define IB4_CH13_DIAG_MODE_BOOSTER	   (1 << 2)
+#define IB4_CH24_DIAG_MODE_SPEAKER	   (0 << 1)
+#define IB4_CH24_DIAG_MODE_BOOSTER	   (1 << 1)
+#define IB4_DIAG_MODE_ENABLE		   (1 << 0)
+
+#define IB5_TEMP_WARNING_ON_CDDIAG_TW1	   (0 << 5)
+#define IB5_TEMP_WARNING_ON_CDDIAG_TW2	   (1 << 5)
+#define IB5_TEMP_WARNING_ON_CDDIAG_TW3	   (2 << 5)
+#define IB5_TEMP_WARNING_ON_CDDIAG_TW4	   (3 << 5)
+#define IB5_TEMP_WARNING_ON_CDDIAG_NONE	   (4 << 5)
+#define IB5_CLIP_DETECT_FRONT_1PC	   (0 << 3)
+#define IB5_CLIP_DETECT_FRONT_5PC	   (1 << 3)
+#define IB5_CLIP_DETECT_FRONT_10PC	   (2 << 3)
+#define IB5_CLIP_DETECT_FRONT_NONE	   (3 << 3)
+#define IB5_CLIP_DETECT_REAR_1PC	   (0 << 1)
+#define IB5_CLIP_DETECT_REAR_5PC	   (1 << 1)
+#define IB5_CLIP_DETECT_REAR_10PC	   (2 << 1)
+#define IB5_CLIP_DETECT_REAR_NONE	   (3 << 1)
+#define IB5_AMPLIFIER_ON		   (1 << 0)
+
+#define DB0_STARTUP_DIAG_STATUS		   0x40
+
+#define DUMP_NUM_BLOCK			   6
+#define DUMP_NUM_REGS			   (DUMP_NUM_BLOCK * 2)
+
+struct tda7802_priv {
+	struct i2c_client *i2c;
+	struct regmap *regmap;
+	struct regulator *enable_reg;
+	const char *diag_mode_ch13, *diag_mode_ch24;
+	u8 gain_ch13, gain_ch24;
+};
+
+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_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,
+	.writeable_reg = tda7802_writeable_reg,
+};
+
+#ifdef DEBUG
+static int tda7802_dump_regs(struct tda7802_priv *tda7802)
+{
+	u8 regs[DUMP_NUM_REGS];
+	const char *prefix;
+	int err = 0;
+
+	prefix = kasprintf(GFP_KERNEL, "%s ", dev_name(&tda7802->i2c->dev));
+	if (!prefix)
+		return -ENOMEM;
+
+	err = regmap_bulk_read(tda7802->regmap, TDA7802_IB0, &regs[0],
+			DUMP_NUM_BLOCK);
+	if (err < 0)
+		goto cleanup_dump_regs;
+
+	err = regmap_bulk_read(tda7802->regmap, TDA7802_DB0,
+			&regs[DUMP_NUM_BLOCK], DUMP_NUM_BLOCK);
+	if (err < 0)
+		goto cleanup_dump_regs;
+
+	print_hex_dump_bytes(prefix, DUMP_PREFIX_NONE, &regs[0], DUMP_NUM_REGS);
+
+cleanup_dump_regs:
+	kfree(prefix);
+	return err;
+}
+#else
+static int tda7802_dump_regs(struct tda7802_priv *tda7802)
+{
+	return 0;
+}
+#endif
+
+static int tda7802_dai_startup(struct snd_pcm_substream *substream,
+			       struct snd_soc_dai *dai)
+{
+	struct tda7802_priv *tda7802 =
+		snd_soc_component_get_drvdata(dai->component);
+	struct device *dev = dai->dev;
+	u8 data[6] = { 0 };
+	int err;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	/* enable the device */
+	err = regulator_enable(tda7802->enable_reg);
+	if (err < 0) {
+		dev_err(dev, "Could not enable (startup).\n");
+		return err;
+	}
+	msleep(ENABLE_DELAY_MS);
+
+	/* All channels out of tri-state mode, all channels in Standard Class
+	 * AB mode (not High-efficiency)
+	 */
+	data[0] = IB0_CH4_CLASS_AB | IB0_CH3_CLASS_AB | IB0_CH2_CLASS_AB |
+		IB0_CH1_CLASS_AB;
+
+	/* Rear channel load impedance set to 2-Ohm, default diagnostic timing,
+	 * GV1 gain on all channels (default), no digital gain increase
+	 */
+	data[1] = IB1_REAR_IMPEDANCE_2_OHM | IB1_LONG_DIAG_TIMING_x1;
+	switch (tda7802->gain_ch13) {
+	case 4:
+		data[1] |= IB1_GAIN_CH13_GV4;
+		break;
+	case 3:
+		data[1] |= IB1_GAIN_CH13_GV3;
+		break;
+	case 2:
+		data[1] |= IB1_GAIN_CH13_GV2;
+		break;
+	default:
+		dev_err(dev, "Unknown gain for channel 1,3 %d, setting to 1\n",
+				tda7802->gain_ch13);
+	case 1:
+		data[1] |= IB1_GAIN_CH13_GV1;
+		break;
+	}
+	switch (tda7802->gain_ch24) {
+	case 4:
+		data[1] |= IB1_GAIN_CH24_GV4;
+		break;
+	case 3:
+		data[1] |= IB1_GAIN_CH24_GV3;
+		break;
+	case 2:
+		data[1] |= IB1_GAIN_CH24_GV2;
+		break;
+	default:
+		dev_err(dev, "Unknown gain for channel 2,4 %d, setting to 1\n",
+				tda7802->gain_ch24);
+	case 1:
+		data[1] |= IB1_GAIN_CH24_GV1;
+		break;
+	}
+
+	/* Mute timing 1.45ms, all channels un-muted, digital mute enabled,
+	 * 5.3V undervoltage threshold, front-channel load impedance set to
+	 * 2-Ohms
+	 */
+	data[2] = IB2_MUTE_TIME_1_MS | IB2_CH13_UNMUTED | IB2_CH24_UNMUTED |
+		IB2_AUTOMUTE_THRESHOLD_5V3 | IB2_FRONT_IMPEDANCE_2_OHM;
+
+	/* Don't set IB3 here, we should set it in set_tdm_slot
+	 * data[3] = 0;
+	 */
+
+	/* Noise gating enabled, short and offset info on CD-Diag (fault) pin,
+	 * diagnostics disabled. Default to speaker.
+	 */
+	if (strcmp(tda7802->diag_mode_ch13, "Booster") == 0)
+		data[4] |= IB4_CH13_DIAG_MODE_BOOSTER;
+	else
+		data[4] |= IB4_CH13_DIAG_MODE_SPEAKER;
+
+	if (strcmp(tda7802->diag_mode_ch24, "Booster") == 0)
+		data[4] |= IB4_CH24_DIAG_MODE_BOOSTER;
+	else
+		data[4] |= IB4_CH24_DIAG_MODE_SPEAKER;
+
+	/* Temperature warning on diag pin set to TW1 (highest setting), clip
+	 * detection set to 1% on all channels
+	 */
+	data[5] = IB5_TEMP_WARNING_ON_CDDIAG_TW1 | IB5_CLIP_DETECT_FRONT_1PC |
+		IB5_CLIP_DETECT_REAR_1PC,
+
+	err = regmap_bulk_write(tda7802->regmap, TDA7802_IB0, data,
+			ARRAY_SIZE(data));
+	if (err < 0) {
+		dev_err(dev, "Cannot configure amp %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static void tda7802_dai_shutdown(struct snd_pcm_substream *substream,
+				 struct snd_soc_dai *dai)
+{
+	struct tda7802_priv *tda7802 =
+		snd_soc_component_get_drvdata(dai->component);
+	struct device *dev = dai->dev;
+	int err;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	err = regulator_disable(tda7802->enable_reg);
+	if (err < 0)
+		dev_err(dev, "Could not disable (shutdown)\n");
+}
+
+static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
+{
+	struct device *dev = dai->dev;
+	int err;
+
+	dev_dbg(dev, "%s mute=%d\n", __func__, mute);
+
+	if (mute) {
+		/* digital mute */
+		err = snd_soc_component_update_bits(dai->component, TDA7802_IB2,
+				IB2_DIGITAL_MUTE_DISABLED,
+				~IB2_DIGITAL_MUTE_DISABLED);
+		if (err < 0)
+			dev_err(dev, "Cannot mute amp %d\n", err);
+
+		/* amp off */
+		err = snd_soc_component_update_bits(dai->component, TDA7802_IB5,
+				IB5_AMPLIFIER_ON, ~IB5_AMPLIFIER_ON);
+		if (err < 0)
+			dev_err(dev, "Cannot amp off %d\n", err);
+
+	} else {
+		/* amp on */
+		err = snd_soc_component_update_bits(dai->component, TDA7802_IB5,
+				IB5_AMPLIFIER_ON, IB5_AMPLIFIER_ON);
+		if (err < 0)
+			dev_err(dev, "Cannot amp on %d\n", err);
+
+		/* digital unmute */
+		err = snd_soc_component_update_bits(dai->component, TDA7802_IB2,
+				IB2_DIGITAL_MUTE_DISABLED,
+				IB2_DIGITAL_MUTE_DISABLED);
+		if (err < 0)
+			dev_err(dev, "Cannot unmute amp %d\n", err);
+	}
+
+	return 0;
+}
+
+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 tda7802_priv *tda7802 =
+		snd_soc_component_get_drvdata(dai->component);
+	int width_index, slot_index, ret;
+	struct device *dev = dai->dev;
+	u8 data;
+
+	if (!(slots == 4 || slots == 8 || slots == 16)) {
+		dev_err(dev, "Failed to set %d slots, supported: 4, 8, 16\n",
+				slots);
+		return -ENOTSUPP;
+	}
+	width_index = (slots / 4) - 1;
+
+	switch (tx_mask) {
+	case 0x000f:
+		slot_index = 0;
+		break;
+	case 0x00f0:
+		slot_index = 1;
+		break;
+	case 0x0f00:
+		slot_index = 2;
+		break;
+	case 0xf000:
+		slot_index = 3;
+		break;
+	default:
+		/* must be contigious nibble */
+		dev_err(dev, "Failed to set tx_mask %08x\n", tx_mask);
+		return -ENOTSUPP;
+	}
+
+	/* 48kHz sample rate, TDM configuration, 64-bit I2S frame period, PLL
+	 * clock dither disabled, high-pass filter enabled (blocks DC output)
+	 */
+	data = IB3_SAMPLE_RATE_48_KHZ | IB3_FORMAT[width_index][slot_index] |
+		IB3_I2S_FRAME_PERIOD_64 | IB3_HIGH_PASS_FILTER_ENABLE;
+	ret = snd_soc_component_write(dai->component, TDA7802_IB3, data);
+	if (ret < 0) {
+		dev_err(dev, "Failed to write IB3 config %d\n", ret);
+		return ret;
+	}
+
+	tda7802_dump_regs(tda7802);
+
+	return 0;
+}
+
+static const struct snd_soc_dai_ops tda7802_dai_ops = {
+	.startup      = tda7802_dai_startup,
+	.shutdown     = tda7802_dai_shutdown,
+	.digital_mute = tda7802_digital_mute,
+	.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_48000,
+		.formats      = SNDRV_PCM_FMTBIT_S32_LE,
+	},
+	.ops = &tda7802_dai_ops,
+};
+
+/* read device tree or ACPI properties from device */
+static int tda7802_read_properties(struct tda7802_priv *tda7802)
+{
+	struct device *dev = &tda7802->i2c->dev;
+	int err = 0;
+
+	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);
+	}
+
+	err = device_property_read_u8(dev, "st,gain-ch13", &tda7802->gain_ch13);
+	if (err < 0)
+		dev_err(dev, "Failed to read gain, channel 1,3 %d\n", err);
+
+	err = device_property_read_u8(dev, "st,gain-ch24", &tda7802->gain_ch24);
+	if (err < 0)
+		dev_err(dev, "Failed to read gain, channel 2,4 %d\n", err);
+
+	err = device_property_read_string(dev, "st,diagnostic-mode-ch13",
+			&tda7802->diag_mode_ch13);
+	if (err < 0)
+		dev_err(dev, "Failed to read diagnostic mode, channel 1,3 %d\n",
+				err);
+
+	err = device_property_read_string(dev, "st,diagnostic-mode-ch24",
+			&tda7802->diag_mode_ch24);
+	if (err < 0)
+		dev_err(dev, "Failed to read diagnostic mode, channel 2,4 %d\n",
+				err);
+
+	return err;
+}
+
+static const struct snd_soc_component_driver tda7802_component_driver;
+
+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 status, err, err2;
+
+	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;
+
+	err = tda7802_read_properties(tda7802);
+	if (err < 0)
+		return err;
+
+	tda7802->regmap = devm_regmap_init_i2c(tda7802->i2c,
+			&tda7802_regmap_config);
+	if (IS_ERR(tda7802->regmap))
+		return PTR_ERR(tda7802->regmap);
+
+	/* Enable and verify the connection */
+	err = regulator_enable(tda7802->enable_reg);
+	if (err < 0) {
+		dev_err(dev, "Failed to enable (init)\n");
+		return err;
+	}
+	msleep(ENABLE_DELAY_MS);
+
+	err = regmap_read(tda7802->regmap, TDA7802_DB1, &status);
+
+	/* always disable the regulator, even if the read fails */
+	err2 = regulator_disable(tda7802->enable_reg);
+	if (err2 < 0) {
+		dev_err(dev, "Failed to disable (init)\n");
+		return err2;
+	}
+	/* now check for regmap_read errors */
+	if (err < 0) {
+		dev_err(dev, "Could not read from device\n");
+		return -ENODEV;
+	}
+
+	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>");
-- 
2.11.0

[PATCH v1 3/4] ASoC: tda7802: Add enable device attribute

[Thomas Preston]

Add a device attribute to control the enable regulator. Write 1 to
enable, 0 to disable (ref-count minus one), or -1 to force disable the
physical pin.

To disable a set of amplifiers wired to the same enable gpio, each
device must be disabled. For example:

	echo 0 > /sys/devices/.../device:00/enable
	echo 0 > /sys/devices/.../device:01/enable

In an emergency, we can force disable from any device:

	echo -1 > /sys/devices/.../device:00/enable

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>
---
 sound/soc/codecs/tda7802.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
index 38ca52de85f0..62aae011d9f1 100644
--- a/sound/soc/codecs/tda7802.c
+++ b/sound/soc/codecs/tda7802.c
@@ -458,6 +458,42 @@ static struct snd_soc_dai_driver tda7802_dai_driver = {
 	.ops = &tda7802_dai_ops,
 };
 
+static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	struct tda7802_priv *tda7802 = dev_get_drvdata(dev);
+	int enabled = regulator_is_enabled(tda7802->enable_reg) ? 1 : 0;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", enabled);
+}
+
+static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct tda7802_priv *tda7802 = dev_get_drvdata(dev);
+	int err;
+
+	if (sysfs_streq(buf, "1")) {
+		err = regulator_enable(tda7802->enable_reg);
+		if (err < 0)
+			dev_err(dev, "Could not enable (sysfs)\n");
+	} else if (sysfs_streq(buf, "0")) {
+		err = regulator_disable(tda7802->enable_reg);
+		if (err < 0)
+			dev_err(dev, "Could not disable (sysfs)\n");
+	} else if (sysfs_streq(buf, "-1")) {
+		err = regulator_force_disable(tda7802->enable_reg);
+		if (err < 0)
+			dev_err(dev, "Could not force disable (sysfs)\n");
+	} else {
+		return -EINVAL;
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(enable);
+
 /* read device tree or ACPI properties from device */
 static int tda7802_read_properties(struct tda7802_priv *tda7802)
 {
@@ -493,7 +529,34 @@ static int tda7802_read_properties(struct tda7802_priv *tda7802)
 	return err;
 }
 
-static const struct snd_soc_component_driver tda7802_component_driver;
+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;
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	err = device_create_file(dev, &dev_attr_enable);
+	if (err < 0) {
+		dev_err(dev, "Could not create enable attr\n");
+		return err;
+	}
+
+	return err;
+}
+
+static void tda7802_remove(struct snd_soc_component *component)
+{
+	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
+
+	device_remove_file(&tda7802->i2c->dev, &dev_attr_enable);
+}
+
+static const struct snd_soc_component_driver tda7802_component_driver = {
+	.probe = tda7802_probe,
+	.remove = tda7802_remove,
+};
 
 static int tda7802_i2c_probe(struct i2c_client *i2c,
 			     const struct i2c_device_id *id)
-- 
2.11.0

[PATCH v1 4/4] ASoC: tda7802: Add speaker-test sysfs

[Thomas Preston]

Add speaker_test device attribute. When the speaker-test node is read
the hardware speaker test is started.

	$ cat /sys/devices/.../device:00/speaker_test
	04 04 04 04

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>
---
 sound/soc/codecs/tda7802.c | 172 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 172 insertions(+)

diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
index 62aae011d9f1..edfa752c0c9f 100644
--- a/sound/soc/codecs/tda7802.c
+++ b/sound/soc/codecs/tda7802.c
@@ -157,6 +157,7 @@ static const u8 IB3_FORMAT[4][4] = {
 #define DUMP_NUM_REGS			   (DUMP_NUM_BLOCK * 2)
 
 struct tda7802_priv {
+	struct mutex mutex;
 	struct i2c_client *i2c;
 	struct regmap *regmap;
 	struct regulator *enable_reg;
@@ -458,6 +459,159 @@ static struct snd_soc_dai_driver tda7802_dai_driver = {
 	.ops = &tda7802_dai_ops,
 };
 
+/* The speaker test is triggered by reading a sysfs attribute file attached to
+ * the I2C device. The user space thread that's doing the reading is blocked
+ * until the test completes (or times out). We only permit one test to be in
+ * progress at a time.
+ */
+
+static int speaker_test_start(struct regmap *regmap)
+{
+	int err;
+
+	err = regmap_update_bits(regmap, TDA7802_IB5, IB5_AMPLIFIER_ON, 0);
+	if (err < 0) {
+		dev_err(regmap_get_device(regmap),
+			"Cannot disable amp for speaker test (%d)\n", err);
+		return err;
+	}
+
+	err = regmap_update_bits(regmap, TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0);
+	if (err < 0) {
+		dev_err(regmap_get_device(regmap),
+			"Cannot disable diag mode before speaker test (%d)\n",
+			err);
+		return err;
+	}
+
+	/* Seem to need at least a 15 ms delay here before the rising
+	 * edge. Otherwise the diagnostics never complete (perhaps
+	 * they never start).
+	 */
+	msleep(20);
+
+	err = regmap_update_bits(regmap, TDA7802_IB4,
+				 IB4_DIAG_MODE_ENABLE, IB4_DIAG_MODE_ENABLE);
+	if (err < 0) {
+		dev_err(regmap_get_device(regmap),
+			"Cannot enable diag mode for speaker test (%d)\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int speaker_test_stop(struct regmap *regmap)
+{
+	int err;
+
+	err = regmap_update_bits(regmap, TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0);
+	if (err < 0)
+		dev_err(regmap_get_device(regmap),
+			"Cannot disable diag mode after speaker test (%d)\n",
+			err);
+	return err;
+}
+
+/* We poll the test completion bit, letting the current thread sleep
+ * until we're done. These values are not critical.
+ */
+#define SPEAKER_TEST_DONE_POLL_PERIOD_US 5000
+#define SPEAKER_TEST_DONE_TIMEOUT_US	 5000000
+
+static int speaker_test_check(struct tda7802_priv *tda7802,
+		unsigned int *status)
+{
+	struct regmap *regmap = tda7802->regmap;
+	struct device *dev = &tda7802->i2c->dev;
+	int reg_err = 0, err = 0, i, amp_on;
+	unsigned int val;
+
+	reg_err = regulator_enable(tda7802->enable_reg);
+	if (reg_err < 0) {
+		dev_err(dev, "Could not enable (speaker-test).\n");
+		return reg_err;
+	}
+	msleep(ENABLE_DELAY_MS);
+
+	/* we should return amp on state when speaker-test is done */
+	err = regmap_read(regmap, TDA7802_IB5, &amp_on);
+	if (err < 0) {
+		dev_err(dev, "Could not read amp on state (speaker-test)\n");
+		goto speaker_test_disable;
+	}
+
+	for (i = 0; i < CHANNELS_PER_AMP; ++i)
+		status[i] = 0xff;
+
+	err = speaker_test_start(regmap);
+	if (err < 0)
+		goto speaker_test_restore;
+
+	/* Wait until DB0_STARTUP_DIAG_STATUS is set */
+	err = regmap_read_poll_timeout(regmap, TDA7802_DB0, val,
+			val & DB0_STARTUP_DIAG_STATUS,
+			SPEAKER_TEST_DONE_POLL_PERIOD_US,
+			SPEAKER_TEST_DONE_TIMEOUT_US);
+	if (err < 0) {
+		if (err == -ENODATA)
+			dev_err(dev,
+				"Speaker diagnostic test did not complete\n");
+		speaker_test_stop(regmap);
+		goto speaker_test_restore;
+	}
+
+	err = speaker_test_stop(regmap);
+	if (err < 0)
+		goto speaker_test_restore;
+
+	for (i = 0; i < CHANNELS_PER_AMP; ++i) {
+		err = regmap_read(regmap, TDA7802_DB1 + i, status + i);
+		if (err < 0) {
+			dev_err(dev,
+				"Cannot get speaker %d status (%d)\n", i, err);
+			goto speaker_test_restore;
+		}
+	}
+
+speaker_test_restore:
+	err = regmap_update_bits(regmap, TDA7802_IB5, IB5_AMPLIFIER_ON,
+			amp_on);
+	if (err < 0)
+		dev_err(dev, "Could not restore amp on state (speaker-test)\n");
+
+speaker_test_disable:
+	reg_err = regulator_disable(tda7802->enable_reg);
+	if (reg_err < 0) {
+		dev_err(dev, "Could not disable (speaker-test).\n");
+		return reg_err;
+	}
+	return err;
+}
+
+static ssize_t speaker_test_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct tda7802_priv *tda7802 = dev_get_drvdata(dev);
+	unsigned int channel_status[CHANNELS_PER_AMP];
+	char *str = buf;
+	int ret, i;
+
+	mutex_lock(&tda7802->mutex);
+	ret = speaker_test_check(tda7802, channel_status);
+	mutex_unlock(&tda7802->mutex);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < CHANNELS_PER_AMP; i++)
+		str += sprintf(str, "%02x ", channel_status[i]);
+	str += sprintf(str, "\n");
+
+	return str - buf;
+}
+
+static DEVICE_ATTR_RO(speaker_test);
+
 static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
 		char *buf)
 {
@@ -537,12 +691,28 @@ static int tda7802_probe(struct snd_soc_component *component)
 
 	dev_dbg(dev, "%s\n", __func__);
 
+	/* Device is ready, now we should create sysfs attributes.
+	 * Rememer to cleanup
+	 */
 	err = device_create_file(dev, &dev_attr_enable);
 	if (err < 0) {
 		dev_err(dev, "Could not create enable attr\n");
 		return err;
 	}
 
+	mutex_init(&tda7802->mutex);
+	err = device_create_file(dev, &dev_attr_speaker_test);
+	if (err < 0) {
+		dev_err(dev, "Could not create speaker_test attr\n");
+		goto cleanup_speaker_test;
+	}
+
+	return 0;
+
+cleanup_speaker_test:
+	mutex_destroy(&tda7802->mutex);
+	device_remove_file(&tda7802->i2c->dev, &dev_attr_enable);
+
 	return err;
 }
 
@@ -550,6 +720,8 @@ static void tda7802_remove(struct snd_soc_component *component)
 {
 	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
 
+	device_remove_file(&tda7802->i2c->dev, &dev_attr_speaker_test);
+	mutex_destroy(&tda7802->mutex);
 	device_remove_file(&tda7802->i2c->dev, &dev_attr_enable);
 }
 
-- 
2.11.0

Re: [PATCH v1 1/4] dt-bindings: ASoC: Add TDA7802 amplifier

[Charles Keepax]

On Tue, Jun 11, 2019 at 06:49:06PM +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)

Does it make sense to have the gains in device tree? Are these
expected to be fixed by the system design, normally the gain
would be controlled through an ALSA control.

Thanks,
Charles

Re: [PATCH v1 3/4] ASoC: tda7802: Add enable device attribute

[Cezary Rojewski]

On 2019-06-11 19:49, Thomas Preston wrote:
> Add a device attribute to control the enable regulator. Write 1 to
> enable, 0 to disable (ref-count minus one), or -1 to force disable the
> physical pin.
> 
> To disable a set of amplifiers wired to the same enable gpio, each
> device must be disabled. For example:
> 
> 	echo 0 > /sys/devices/.../device:00/enable
> 	echo 0 > /sys/devices/.../device:01/enable
> 
> In an emergency, we can force disable from any device:
> 
> 	echo -1 > /sys/devices/.../device:00/enable
> 
> 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>
> ---
>   sound/soc/codecs/tda7802.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
> index 38ca52de85f0..62aae011d9f1 100644
> --- a/sound/soc/codecs/tda7802.c
> +++ b/sound/soc/codecs/tda7802.c
> @@ -458,6 +458,42 @@ static struct snd_soc_dai_driver tda7802_dai_driver = {
>   	.ops = &tda7802_dai_ops,
>   };
>   
> +static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct tda7802_priv *tda7802 = dev_get_drvdata(dev);
> +	int enabled = regulator_is_enabled(tda7802->enable_reg) ? 1 : 0;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", enabled);
> +}
> +
> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t count)
> +{
> +	struct tda7802_priv *tda7802 = dev_get_drvdata(dev);
> +	int err;
> +
> +	if (sysfs_streq(buf, "1")) {
> +		err = regulator_enable(tda7802->enable_reg);
> +		if (err < 0)
> +			dev_err(dev, "Could not enable (sysfs)\n");
> +	} else if (sysfs_streq(buf, "0")) {
> +		err = regulator_disable(tda7802->enable_reg);
> +		if (err < 0)
> +			dev_err(dev, "Could not disable (sysfs)\n");
> +	} else if (sysfs_streq(buf, "-1")) {
> +		err = regulator_force_disable(tda7802->enable_reg);
> +		if (err < 0)
> +			dev_err(dev, "Could not force disable (sysfs)\n");
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(enable);
> +
>   /* read device tree or ACPI properties from device */
>   static int tda7802_read_properties(struct tda7802_priv *tda7802)
>   {
> @@ -493,7 +529,34 @@ static int tda7802_read_properties(struct tda7802_priv *tda7802)
>   	return err;
>   }
>   
> -static const struct snd_soc_component_driver tda7802_component_driver;
> +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;
> +
> +	dev_dbg(dev, "%s\n", __func__);

Function name alone ain't very informational. Is this intended?

> +
> +	err = device_create_file(dev, &dev_attr_enable);
> +	if (err < 0) {
> +		dev_err(dev, "Could not create enable attr\n");
> +		return err;
> +	}

Regardless of outcome, you'll be returning err here. Consider leaving 
error message alone within if-statement. Remove redundant brackets if 
you decide to do so.

> +
> +	return err;
> +}
> +
> +static void tda7802_remove(struct snd_soc_component *component)
> +{
> +	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
> +
> +	device_remove_file(&tda7802->i2c->dev, &dev_attr_enable);
> +}
> +
> +static const struct snd_soc_component_driver tda7802_component_driver = {
> +	.probe = tda7802_probe,
> +	.remove = tda7802_remove,
> +};
>   
>   static int tda7802_i2c_probe(struct i2c_client *i2c,
>   			     const struct i2c_device_id *id)
> 

Re: [PATCH v1 4/4] ASoC: tda7802: Add speaker-test sysfs

[Cezary Rojewski]

On 2019-06-11 19:49, Thomas Preston wrote:
> Add speaker_test device attribute. When the speaker-test node is read
> the hardware speaker test is started.
> 
> 	$ cat /sys/devices/.../device:00/speaker_test
> 	04 04 04 04
> 
> 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>
> ---
>   sound/soc/codecs/tda7802.c | 172 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 172 insertions(+)
> 
> diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
> index 62aae011d9f1..edfa752c0c9f 100644
> --- a/sound/soc/codecs/tda7802.c
> +++ b/sound/soc/codecs/tda7802.c
> @@ -157,6 +157,7 @@ static const u8 IB3_FORMAT[4][4] = {
>   #define DUMP_NUM_REGS			   (DUMP_NUM_BLOCK * 2)
>   
>   struct tda7802_priv {
> +	struct mutex mutex;
>   	struct i2c_client *i2c;
>   	struct regmap *regmap;
>   	struct regulator *enable_reg;
> @@ -458,6 +459,159 @@ static struct snd_soc_dai_driver tda7802_dai_driver = {
>   	.ops = &tda7802_dai_ops,
>   };
>   
> +/* The speaker test is triggered by reading a sysfs attribute file attached to
> + * the I2C device. The user space thread that's doing the reading is blocked
> + * until the test completes (or times out). We only permit one test to be in
> + * progress at a time.
> + */
> +
> +static int speaker_test_start(struct regmap *regmap)
> +{
> +	int err;
> +
> +	err = regmap_update_bits(regmap, TDA7802_IB5, IB5_AMPLIFIER_ON, 0);
> +	if (err < 0) {
> +		dev_err(regmap_get_device(regmap),
> +			"Cannot disable amp for speaker test (%d)\n", err);
> +		return err;
> +	}
> +
> +	err = regmap_update_bits(regmap, TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0);
> +	if (err < 0) {
> +		dev_err(regmap_get_device(regmap),
> +			"Cannot disable diag mode before speaker test (%d)\n",
> +			err);
> +		return err;
> +	}
> +
> +	/* Seem to need at least a 15 ms delay here before the rising
> +	 * edge. Otherwise the diagnostics never complete (perhaps
> +	 * they never start).
> +	 */
> +	msleep(20);
> +
> +	err = regmap_update_bits(regmap, TDA7802_IB4,
> +				 IB4_DIAG_MODE_ENABLE, IB4_DIAG_MODE_ENABLE);
> +	if (err < 0) {
> +		dev_err(regmap_get_device(regmap),
> +			"Cannot enable diag mode for speaker test (%d)\n", err);
> +		return err;
> +	}
> +
> +	return 0;

You may want to follow path set by speaker_test_stop: replace "return 0" 
with "return err" and have only error msg found within preceding 
if-statement.

> +}
> +
> +static int speaker_test_stop(struct regmap *regmap)
> +{
> +	int err;
> +
> +	err = regmap_update_bits(regmap, TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0);
> +	if (err < 0)
> +		dev_err(regmap_get_device(regmap),
> +			"Cannot disable diag mode after speaker test (%d)\n",
> +			err);
> +	return err;
> +}
> +
> +/* We poll the test completion bit, letting the current thread sleep
> + * until we're done. These values are not critical.
> + */
> +#define SPEAKER_TEST_DONE_POLL_PERIOD_US 5000
> +#define SPEAKER_TEST_DONE_TIMEOUT_US	 5000000
> +
> +static int speaker_test_check(struct tda7802_priv *tda7802,
> +		unsigned int *status)
> +{
> +	struct regmap *regmap = tda7802->regmap;
> +	struct device *dev = &tda7802->i2c->dev;
> +	int reg_err = 0, err = 0, i, amp_on;

Both reg_err and err are initialized unnecessarily.

> +	unsigned int val;
> +
> +	reg_err = regulator_enable(tda7802->enable_reg);
> +	if (reg_err < 0) {
> +		dev_err(dev, "Could not enable (speaker-test).\n");
> +		return reg_err;
> +	}
> +	msleep(ENABLE_DELAY_MS);
> +
> +	/* we should return amp on state when speaker-test is done */
> +	err = regmap_read(regmap, TDA7802_IB5, &amp_on);
> +	if (err < 0) {
> +		dev_err(dev, "Could not read amp on state (speaker-test)\n");
> +		goto speaker_test_disable;
> +	}
> +
> +	for (i = 0; i < CHANNELS_PER_AMP; ++i)
> +		status[i] = 0xff;
> +
> +	err = speaker_test_start(regmap);
> +	if (err < 0)
> +		goto speaker_test_restore;
> +
> +	/* Wait until DB0_STARTUP_DIAG_STATUS is set */
> +	err = regmap_read_poll_timeout(regmap, TDA7802_DB0, val,
> +			val & DB0_STARTUP_DIAG_STATUS,
> +			SPEAKER_TEST_DONE_POLL_PERIOD_US,
> +			SPEAKER_TEST_DONE_TIMEOUT_US);
> +	if (err < 0) {
> +		if (err == -ENODATA)
> +			dev_err(dev,
> +				"Speaker diagnostic test did not complete\n");
> +		speaker_test_stop(regmap);
> +		goto speaker_test_restore;
> +	}
> +
> +	err = speaker_test_stop(regmap);
> +	if (err < 0)
> +		goto speaker_test_restore;

This sequence looks weird and as if it could be simplified but I might 
be just plain wrong. speaker_test_stop gets invoked whether or not 
regmap_read_poll_timeout fails, though only in "happy" path its return 
is account for. If it's not too much to ask, could you clarify?

> +
> +	for (i = 0; i < CHANNELS_PER_AMP; ++i) {
> +		err = regmap_read(regmap, TDA7802_DB1 + i, status + i);
> +		if (err < 0) {
> +			dev_err(dev,
> +				"Cannot get speaker %d status (%d)\n", i, err);
> +			goto speaker_test_restore;
> +		}
> +	}
> +
> +speaker_test_restore:
> +	err = regmap_update_bits(regmap, TDA7802_IB5, IB5_AMPLIFIER_ON,
> +			amp_on);
> +	if (err < 0)
> +		dev_err(dev, "Could not restore amp on state (speaker-test)\n");
> +
> +speaker_test_disable:
> +	reg_err = regulator_disable(tda7802->enable_reg);
> +	if (reg_err < 0) {
> +		dev_err(dev, "Could not disable (speaker-test).\n");
> +		return reg_err;
> +	}
> +	return err;
> +}
> +
> +static ssize_t speaker_test_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct tda7802_priv *tda7802 = dev_get_drvdata(dev);
> +	unsigned int channel_status[CHANNELS_PER_AMP];
> +	char *str = buf;
> +	int ret, i;
> +
> +	mutex_lock(&tda7802->mutex);
> +	ret = speaker_test_check(tda7802, channel_status);
> +	mutex_unlock(&tda7802->mutex);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < CHANNELS_PER_AMP; i++)
> +		str += sprintf(str, "%02x ", channel_status[i]);
> +	str += sprintf(str, "\n");
> +
> +	return str - buf;
> +}
> +
> +static DEVICE_ATTR_RO(speaker_test);
> +
>   static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
>   		char *buf)
>   {
> @@ -537,12 +691,28 @@ static int tda7802_probe(struct snd_soc_component *component)
>   
>   	dev_dbg(dev, "%s\n", __func__);
>   
> +	/* Device is ready, now we should create sysfs attributes.
> +	 * Rememer to cleanup
> +	 */
>   	err = device_create_file(dev, &dev_attr_enable);
>   	if (err < 0) {
>   		dev_err(dev, "Could not create enable attr\n");
>   		return err;
>   	}
>   
> +	mutex_init(&tda7802->mutex);
> +	err = device_create_file(dev, &dev_attr_speaker_test);
> +	if (err < 0) {
> +		dev_err(dev, "Could not create speaker_test attr\n");
> +		goto cleanup_speaker_test;
> +	}
> +
> +	return 0;
> +
> +cleanup_speaker_test:
> +	mutex_destroy(&tda7802->mutex);
> +	device_remove_file(&tda7802->i2c->dev, &dev_attr_enable);
> +
>   	return err;
>   }
>   
> @@ -550,6 +720,8 @@ static void tda7802_remove(struct snd_soc_component *component)
>   {
>   	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
>   
> +	device_remove_file(&tda7802->i2c->dev, &dev_attr_speaker_test);
> +	mutex_destroy(&tda7802->mutex);
>   	device_remove_file(&tda7802->i2c->dev, &dev_attr_enable);
>   }
>   
> 

Re: [PATCH v1 3/4] ASoC: tda7802: Add enable device attribute

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

On Tue, Jun 11, 2019 at 06:49:08PM +0100, Thomas Preston wrote:
> Add a device attribute to control the enable regulator. Write 1 to
> enable, 0 to disable (ref-count minus one), or -1 to force disable the
> physical pin.

> To disable a set of amplifiers wired to the same enable gpio, each
> device must be disabled. For example:

> 	echo 0 > /sys/devices/.../device:00/enable
> 	echo 0 > /sys/devices/.../device:01/enable

This is adding a new ABI completely outstide the standard ALSA and power
management frameworks and ABIs with no explanation as to why or
integration with the rest of the driver.  This is obviously not in the
slightest bit OK.  If there's something missing from the frameworks
extend the frameworks, don't just ignore them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/4] ASoC: Add codec driver for ST TDA7802

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4581 bytes --]

On Tue, Jun 11, 2019 at 06:49:07PM +0100, Thomas Preston wrote:

> +++ b/sound/soc/codecs/tda7802.c
> @@ -0,0 +1,580 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * tda7802.c  --  codec driver for ST TDA7802
> + *

Make the entire comment a C++ one so this looks more intentional.

> + * Copyright (C) 2016-2019 Tesla Motors, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.

You've got a SPDX header, you don't also need GPL boilerplate (this
doesn't match your SPDX header either...).

> +enum tda7802_type {
> +	tda7802_base,
> +};
> +
> +#define I2C_BUS				   4
> +
> +#define CHANNELS_PER_AMP		   4
> +#define MAX_NUM_AMPS			   4
> +

These are concerning but also unused.

> +
> +static const u8 IB3_FORMAT[4][4] = {
> +	/* 1x amp, 4 channels */
> +	{ IB3_FORMAT_TDM4 },
> +	/* 2x amp, 8 channels */
> +	{ IB3_FORMAT_TDM8_DEV1,
> +		IB3_FORMAT_TDM8_DEV2 },
> +	/* 3x amp not supported */
> +	{ },
> +	/* 4x amp, 16 channels */
> +	{ IB3_FORMAT_TDM16_DEV1,
> +		IB3_FORMAT_TDM16_DEV2,
> +		IB3_FORMAT_TDM16_DEV3,
> +		IB3_FORMAT_TDM16_DEV4 },
> +};

There are indentation issues here and this is also rather magic
numberish.  I *think* you should be implement set_tdm_slot().

> +#ifdef DEBUG
> +static int tda7802_dump_regs(struct tda7802_priv *tda7802)
> +{

There's already facilities for viewing the register map in regmap, no
need to open code anything.

> +	/* All channels out of tri-state mode, all channels in Standard Class
> +	 * AB mode (not High-efficiency)
> +	 */
> +	data[0] = IB0_CH4_CLASS_AB | IB0_CH3_CLASS_AB | IB0_CH2_CLASS_AB |
> +		IB0_CH1_CLASS_AB;

The AB mode selection should be a user visible control not hard coded.

> +	/* Rear channel load impedance set to 2-Ohm, default diagnostic timing,
> +	 * GV1 gain on all channels (default), no digital gain increase
> +	 */
> +	data[1] = IB1_REAR_IMPEDANCE_2_OHM | IB1_LONG_DIAG_TIMING_x1;
> +	switch (tda7802->gain_ch13) {

The impedance should be a DT property, the gains should be user
controllable.

> +	case 2:
> +		data[1] |= IB1_GAIN_CH13_GV2;
> +		break;
> +	default:
> +		dev_err(dev, "Unknown gain for channel 1,3 %d, setting to 1\n",
> +				tda7802->gain_ch13);
> +	case 1:
> +		data[1] |= IB1_GAIN_CH13_GV1;
> +		break;

It would be more normal to have the default case as the last thing in the
switch statement and the fall through is obviously a bug, if the driver
doesn't know how to handle the configuration it should return an error.

> +	}
> +	switch (tda7802->gain_ch24) {

Blank line please.

> +	/* Mute timing 1.45ms, all channels un-muted, digital mute enabled,
> +	 * 5.3V undervoltage threshold, front-channel load impedance set to
> +	 * 2-Ohms
> +	 */
> +	data[2] = IB2_MUTE_TIME_1_MS | IB2_CH13_UNMUTED | IB2_CH24_UNMUTED |
> +		IB2_AUTOMUTE_THRESHOLD_5V3 | IB2_FRONT_IMPEDANCE_2_OHM;

More stuff that shouldn't be hard coded in the driver.

> +	if (mute) {
> +		/* digital mute */
> +		err = snd_soc_component_update_bits(dai->component, TDA7802_IB2,
> +				IB2_DIGITAL_MUTE_DISABLED,
> +				~IB2_DIGITAL_MUTE_DISABLED);
> +		if (err < 0)
> +			dev_err(dev, "Cannot mute amp %d\n", err);
> +
> +		/* amp off */
> +		err = snd_soc_component_update_bits(dai->component, TDA7802_IB5,
> +				IB5_AMPLIFIER_ON, ~IB5_AMPLIFIER_ON);
> +		if (err < 0)
> +			dev_err(dev, "Cannot amp off %d\n", err);

Turning off the amplifier is clearly power management and should be
handled via DAPM.

> +static int tda7802_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
> +		unsigned int rx_mask, int slots, int slot_width)
> +{

> +	/* 48kHz sample rate, TDM configuration, 64-bit I2S frame period, PLL
> +	 * clock dither disabled, high-pass filter enabled (blocks DC output)
> +	 */
> +	data = IB3_SAMPLE_RATE_48_KHZ | IB3_FORMAT[width_index][slot_index] |

The sample rate should be set using hw_params(), the high pass filter
should be controllable via a control.

I've stopped reading here, it's fairly clear from what I've seen up to
here that the code is not well integrated with the framework with lots
of hard coding and things scattered around in places where I'd not
expect to find them.  A lot of things should be moved out of hard coding
into ALSA controls, stream configuration should be done via hw_params()
and power management via DAPM - the hardware looks fairly simple so I'd
expect this to all be fairly straightforward to fix.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/4] ASoC: Add codec driver for ST TDA7802

[Kirill Marinushkin]

On 06/11/19 19:49, Thomas Preston wrote:
> Add an I2C based codec driver for ST TDA7802 amplifier. By default, the
> amplifier supports 4 audio channels but can support up to 16 with
> multiple devices. Input is configurable for I2S or TDM.
> 
> The unified device properties API is used to get board-specific config from
> device tree / ACPI.
> 
> 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>
> ---
>  sound/soc/codecs/Kconfig   |   6 +
>  sound/soc/codecs/Makefile  |   2 +
>  sound/soc/codecs/tda7802.c | 580 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 588 insertions(+)
>  create mode 100644 sound/soc/codecs/tda7802.c
> 
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index e730d47ac85b..1d30c2333cb1 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -176,6 +176,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
> @@ -1078,6 +1079,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 aa7720a7a0aa..fc3fc672bc4b 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -187,6 +187,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
> @@ -460,6 +461,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..38ca52de85f0
> --- /dev/null
> +++ b/sound/soc/codecs/tda7802.c
> @@ -0,0 +1,580 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * tda7802.c  --  codec driver for ST TDA7802
> + *
> + * Copyright (C) 2016-2019 Tesla Motors, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + */
> +
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>
> +#include <sound/soc.h>
> +
> +enum tda7802_type {
> +	tda7802_base,
> +};
> +
> +#define I2C_BUS				   4
> +
> +#define CHANNELS_PER_AMP		   4
> +#define MAX_NUM_AMPS			   4
> +
> +#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_DB1			   0x11
> +#define TDA7802_DB2			   0x12
> +#define TDA7802_DB3			   0x13
> +#define TDA7802_DB4			   0x14
> +#define TDA7802_DB5			   0x15
> +
> +#define IB0_CH4_TRISTATE_ON		   (1 << 7)
> +#define IB0_CH3_TRISTATE_ON		   (1 << 6)
> +#define IB0_CH2_TRISTATE_ON		   (1 << 5)
> +#define IB0_CH1_TRISTATE_ON		   (1 << 4)
> +#define IB0_CH4_HIGH_EFF		   (0 << 3)
> +#define IB0_CH4_CLASS_AB		   (1 << 3)
> +#define IB0_CH3_HIGH_EFF		   (0 << 2)
> +#define IB0_CH3_CLASS_AB		   (1 << 2)
> +#define IB0_CH2_HIGH_EFF		   (0 << 0)
> +#define IB0_CH2_CLASS_AB		   (1 << 1)
> +#define IB0_CH1_HIGH_EFF		   (0 << 0)
> +#define IB0_CH1_CLASS_AB		   (1 << 0)
> +
> +#define IB1_REAR_IMPEDANCE_2_OHM	   (0 << 7)
> +#define IB1_REAR_IMPEDANCE_4_OHM	   (1 << 7)
> +#define IB1_LONG_DIAG_TIMING_x1		   (0 << 5)
> +#define IB1_LONG_DIAG_TIMING_x2		   (1 << 5)
> +#define IB1_LONG_DIAG_TIMING_x4		   (2 << 5)
> +#define IB1_LONG_DIAG_TIMING_x8		   (3 << 5)
> +#define IB1_GAIN_CH13_GV1		   (0 << 3)
> +#define IB1_GAIN_CH13_GV2		   (1 << 3)
> +#define IB1_GAIN_CH13_GV3		   (2 << 3)
> +#define IB1_GAIN_CH13_GV4		   (3 << 3)
> +#define IB1_GAIN_CH24_GV1		   (0 << 1)
> +#define IB1_GAIN_CH24_GV2		   (1 << 1)
> +#define IB1_GAIN_CH24_GV3		   (2 << 1)
> +#define IB1_GAIN_CH24_GV4		   (3 << 1)
> +#define IB1_DIGITAL_GAIN_BOOST		   (1 << 0)
> +
> +#define IB2_MUTE_TIME_1_MS		   (0 << 5)
> +#define IB2_MUTE_TIME_5_MS		   (1 << 5)
> +#define IB2_MUTE_TIME_11_MS		   (2 << 5)
> +#define IB2_MUTE_TIME_23_MS		   (3 << 5)
> +#define IB2_MUTE_TIME_46_MS		   (4 << 5)
> +#define IB2_MUTE_TIME_92_MS		   (5 << 5)
> +#define IB2_MUTE_TIME_185_MS		   (6 << 5)
> +#define IB2_MUTE_TIME_371_MS		   (7 << 5)
> +#define IB2_CH13_UNMUTED		   (1 << 4)
> +#define IB2_CH24_UNMUTED		   (1 << 3)
> +#define IB2_DIGITAL_MUTE_DISABLED	   (1 << 2)
> +#define IB2_AUTOMUTE_THRESHOLD_5V3	   (0 << 1)
> +#define IB2_AUTOMUTE_THRESHOLD_7V3	   (1 << 1)
> +#define IB2_FRONT_IMPEDANCE_2_OHM	   (0 << 0)
> +#define IB2_FRONT_IMPEDANCE_4_OHM	   (1 << 0)
> +
> +#define IB3_SAMPLE_RATE_44_KHZ		   (0 << 6)
> +#define IB3_SAMPLE_RATE_48_KHZ		   (1 << 6)
> +#define IB3_SAMPLE_RATE_96_KHZ		   (2 << 6)
> +#define IB3_SAMPLE_RATE_192_KHZ		   (3 << 6)
> +#define IB3_FORMAT_I2S			   (0 << 3)
> +#define IB3_FORMAT_TDM4			   (1 << 3)
> +#define IB3_FORMAT_TDM8_DEV1		   (2 << 3)
> +#define IB3_FORMAT_TDM8_DEV2		   (3 << 3)
> +#define IB3_FORMAT_TDM16_DEV1		   (4 << 3)
> +#define IB3_FORMAT_TDM16_DEV2		   (5 << 3)
> +#define IB3_FORMAT_TDM16_DEV3		   (6 << 3)
> +#define IB3_FORMAT_TDM16_DEV4		   (7 << 3)
> +#define IB3_I2S_FRAME_PERIOD_64		   (0 << 2)
> +#define IB3_I2S_FRAME_PERIOD_50		   (1 << 2)
> +#define IB3_PLL_CLOCK_DITHER_ENABLE	   (1 << 1)
> +#define IB3_HIGH_PASS_FILTER_ENABLE	   (1 << 0)

More unused defines - do you want them?

> +
> +static const u8 IB3_FORMAT[4][4] = {
> +	/* 1x amp, 4 channels */
> +	{ IB3_FORMAT_TDM4 },
> +	/* 2x amp, 8 channels */
> +	{ IB3_FORMAT_TDM8_DEV1,
> +		IB3_FORMAT_TDM8_DEV2 },
> +	/* 3x amp not supported */
> +	{ },
> +	/* 4x amp, 16 channels */
> +	{ IB3_FORMAT_TDM16_DEV1,
> +		IB3_FORMAT_TDM16_DEV2,
> +		IB3_FORMAT_TDM16_DEV3,
> +		IB3_FORMAT_TDM16_DEV4 },
> +};
> +
> +#define IB4_NOISE_GATE_DISABLE		   (1 << 7)
> +#define IB4_SHORT_FAULT_ON_CDDIAG_YES	   (0 << 6)
> +#define IB4_SHORT_FAULT_ON_CDDIAG_NO	   (1 << 6)
> +#define IB4_OFFSET_ON_CDDIAG_YES	   (0 << 5)
> +#define IB4_OFFSET_ON_CDDIAG_NO		   (1 << 5)
> +#define IB4_AC_DIAG_CURRENT_THRESHOLD_HIGH (0 << 4)
> +#define IB4_AC_DIAG_CURRENT_THRESHOLD_LOW  (1 << 4)
> +#define IB4_AC_DIAG_ENABLE		   (1 << 3)
> +#define IB4_CH13_DIAG_MODE_SPEAKER	   (0 << 2)
> +#define IB4_CH13_DIAG_MODE_BOOSTER	   (1 << 2)
> +#define IB4_CH24_DIAG_MODE_SPEAKER	   (0 << 1)
> +#define IB4_CH24_DIAG_MODE_BOOSTER	   (1 << 1)
> +#define IB4_DIAG_MODE_ENABLE		   (1 << 0)
> +
> +#define IB5_TEMP_WARNING_ON_CDDIAG_TW1	   (0 << 5)
> +#define IB5_TEMP_WARNING_ON_CDDIAG_TW2	   (1 << 5)
> +#define IB5_TEMP_WARNING_ON_CDDIAG_TW3	   (2 << 5)
> +#define IB5_TEMP_WARNING_ON_CDDIAG_TW4	   (3 << 5)
> +#define IB5_TEMP_WARNING_ON_CDDIAG_NONE	   (4 << 5)
> +#define IB5_CLIP_DETECT_FRONT_1PC	   (0 << 3)
> +#define IB5_CLIP_DETECT_FRONT_5PC	   (1 << 3)
> +#define IB5_CLIP_DETECT_FRONT_10PC	   (2 << 3)
> +#define IB5_CLIP_DETECT_FRONT_NONE	   (3 << 3)
> +#define IB5_CLIP_DETECT_REAR_1PC	   (0 << 1)
> +#define IB5_CLIP_DETECT_REAR_5PC	   (1 << 1)
> +#define IB5_CLIP_DETECT_REAR_10PC	   (2 << 1)
> +#define IB5_CLIP_DETECT_REAR_NONE	   (3 << 1)
> +#define IB5_AMPLIFIER_ON		   (1 << 0)
> +
> +#define DB0_STARTUP_DIAG_STATUS		   0x40
> +
> +#define DUMP_NUM_BLOCK			   6
> +#define DUMP_NUM_REGS			   (DUMP_NUM_BLOCK * 2)
> +
> +struct tda7802_priv {
> +	struct i2c_client *i2c;
> +	struct regmap *regmap;
> +	struct regulator *enable_reg;
> +	const char *diag_mode_ch13, *diag_mode_ch24;
> +	u8 gain_ch13, gain_ch24;
> +};
> +
> +static bool tda7802_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TDA7802_IB0 ... TDA7802_IB5:`
Oh, I didn't know it is possible to use such `...` syntax in switch cases. This
is interesting!

> +	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,
> +	.writeable_reg = tda7802_writeable_reg,

Maybe add default values?

> +};
> +
> +#ifdef DEBUG
> +static int tda7802_dump_regs(struct tda7802_priv *tda7802)
> +{
> +	u8 regs[DUMP_NUM_REGS];
> +	const char *prefix;
> +	int err = 0;
> +
> +	prefix = kasprintf(GFP_KERNEL, "%s ", dev_name(&tda7802->i2c->dev));
> +	if (!prefix)
> +		return -ENOMEM;
> +
> +	err = regmap_bulk_read(tda7802->regmap, TDA7802_IB0, &regs[0],
> +			DUMP_NUM_BLOCK);
> +	if (err < 0)
> +		goto cleanup_dump_regs;
> +
> +	err = regmap_bulk_read(tda7802->regmap, TDA7802_DB0,
> +			&regs[DUMP_NUM_BLOCK], DUMP_NUM_BLOCK);
> +	if (err < 0)
> +		goto cleanup_dump_regs;
> +
> +	print_hex_dump_bytes(prefix, DUMP_PREFIX_NONE, &regs[0], DUMP_NUM_REGS);
> +
> +cleanup_dump_regs:
> +	kfree(prefix);
> +	return err;
> +}
> +#else
> +static int tda7802_dump_regs(struct tda7802_priv *tda7802)
> +{
> +	return 0;
> +}
> +#endif
> +
> +static int tda7802_dai_startup(struct snd_pcm_substream *substream,
> +			       struct snd_soc_dai *dai)
> +{
> +	struct tda7802_priv *tda7802 =
> +		snd_soc_component_get_drvdata(dai->component);
> +	struct device *dev = dai->dev;
> +	u8 data[6] = { 0 };
> +	int err;
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	/* enable the device */
> +	err = regulator_enable(tda7802->enable_reg);
> +	if (err < 0) {
> +		dev_err(dev, "Could not enable (startup).\n");
> +		return err;
> +	}
> +	msleep(ENABLE_DELAY_MS);
> +
> +	/* All channels out of tri-state mode, all channels in Standard Class
> +	 * AB mode (not High-efficiency)
> +	 */
> +	data[0] = IB0_CH4_CLASS_AB | IB0_CH3_CLASS_AB | IB0_CH2_CLASS_AB |
> +		IB0_CH1_CLASS_AB;
> +
> +	/* Rear channel load impedance set to 2-Ohm, default diagnostic timing,
> +	 * GV1 gain on all channels (default), no digital gain increase
> +	 */
> +	data[1] = IB1_REAR_IMPEDANCE_2_OHM | IB1_LONG_DIAG_TIMING_x1;
> +	switch (tda7802->gain_ch13) {
> +	case 4:
> +		data[1] |= IB1_GAIN_CH13_GV4;
> +		break;
> +	case 3:
> +		data[1] |= IB1_GAIN_CH13_GV3;
> +		break;
> +	case 2:
> +		data[1] |= IB1_GAIN_CH13_GV2;
> +		break;
> +	default:
> +		dev_err(dev, "Unknown gain for channel 1,3 %d, setting to 1\n",
> +				tda7802->gain_ch13);
> +	case 1:
> +		data[1] |= IB1_GAIN_CH13_GV1;
> +		break;
> +	}
> +	switch (tda7802->gain_ch24) {
> +	case 4:
> +		data[1] |= IB1_GAIN_CH24_GV4;
> +		break;
> +	case 3:
> +		data[1] |= IB1_GAIN_CH24_GV3;
> +		break;
> +	case 2:
> +		data[1] |= IB1_GAIN_CH24_GV2;
> +		break;
> +	default:
> +		dev_err(dev, "Unknown gain for channel 2,4 %d, setting to 1\n",
> +				tda7802->gain_ch24);
> +	case 1:
> +		data[1] |= IB1_GAIN_CH24_GV1;
> +		break;
> +	}
> +
> +	/* Mute timing 1.45ms, all channels un-muted, digital mute enabled,
> +	 * 5.3V undervoltage threshold, front-channel load impedance set to
> +	 * 2-Ohms
> +	 */
> +	data[2] = IB2_MUTE_TIME_1_MS | IB2_CH13_UNMUTED | IB2_CH24_UNMUTED |
> +		IB2_AUTOMUTE_THRESHOLD_5V3 | IB2_FRONT_IMPEDANCE_2_OHM> +
> +	/* Don't set IB3 here, we should set it in set_tdm_slot
> +	 * data[3] = 0;
> +	 */
> +
> +	/* Noise gating enabled, short and offset info on CD-Diag (fault) pin,
> +	 * diagnostics disabled. Default to speaker.
> +	 */
> +	if (strcmp(tda7802->diag_mode_ch13, "Booster") == 0)
> +		data[4] |= IB4_CH13_DIAG_MODE_BOOSTER;
> +	else
> +		data[4] |= IB4_CH13_DIAG_MODE_SPEAKER;
> +
> +	if (strcmp(tda7802->diag_mode_ch24, "Booster") == 0)
> +		data[4] |= IB4_CH24_DIAG_MODE_BOOSTER;
> +	else
> +		data[4] |= IB4_CH24_DIAG_MODE_SPEAKER;
> +
> +	/* Temperature warning on diag pin set to TW1 (highest setting), clip
> +	 * detection set to 1% on all channels
> +	 */
> +	data[5] = IB5_TEMP_WARNING_ON_CDDIAG_TW1 | IB5_CLIP_DETECT_FRONT_1PC |
> +		IB5_CLIP_DETECT_REAR_1PC,

Would be nicer to have `;` in the end of the line, instead of `,`

> +
> +	err = regmap_bulk_write(tda7802->regmap, TDA7802_IB0, data,
> +			ARRAY_SIZE(data));
> +	if (err < 0) {
> +		dev_err(dev, "Cannot configure amp %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static void tda7802_dai_shutdown(struct snd_pcm_substream *substream,
> +				 struct snd_soc_dai *dai)
> +{
> +	struct tda7802_priv *tda7802 =
> +		snd_soc_component_get_drvdata(dai->component);
> +	struct device *dev = dai->dev;
> +	int err;
> +
> +	dev_dbg(dev, "%s\n", __func__);
> +
> +	err = regulator_disable(tda7802->enable_reg);
> +	if (err < 0)
> +		dev_err(dev, "Could not disable (shutdown)\n");
> +}
> +
> +static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
> +{
> +	struct device *dev = dai->dev;
> +	int err;
> +
> +	dev_dbg(dev, "%s mute=%d\n", __func__, mute);
> +
> +	if (mute) {
> +		/* digital mute */
> +		err = snd_soc_component_update_bits(dai->component, TDA7802_IB2,
> +				IB2_DIGITAL_MUTE_DISABLED,
> +				~IB2_DIGITAL_MUTE_DISABLED);

In such situations, the last argument is usually `0`, not `~(mask)`

> +		if (err < 0)
> +			dev_err(dev, "Cannot mute amp %d\n", err);

I think you want to `return err` here, right?

> +
> +		/* amp off */
> +		err = snd_soc_component_update_bits(dai->component, TDA7802_IB5,
> +				IB5_AMPLIFIER_ON, ~IB5_AMPLIFIER_ON);
> +		if (err < 0)
> +			dev_err(dev, "Cannot amp off %d\n", err);
> +
> +	} else {
> +		/* amp on */
> +		err = snd_soc_component_update_bits(dai->component, TDA7802_IB5,
> +				IB5_AMPLIFIER_ON, IB5_AMPLIFIER_ON);
> +		if (err < 0)
> +			dev_err(dev, "Cannot amp on %d\n", err);
> +
> +		/* digital unmute */
> +		err = snd_soc_component_update_bits(dai->component, TDA7802_IB2,
> +				IB2_DIGITAL_MUTE_DISABLED,
> +				IB2_DIGITAL_MUTE_DISABLED);
> +		if (err < 0)
> +			dev_err(dev, "Cannot unmute amp %d\n", err);
> +	}
> +
> +	return 0;
> +}
> +
> +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 tda7802_priv *tda7802 =
> +		snd_soc_component_get_drvdata(dai->component);
> +	int width_index, slot_index, ret;
> +	struct device *dev = dai->dev;
> +	u8 data;
> +
> +	if (!(slots == 4 || slots == 8 || slots == 16)) {
> +		dev_err(dev, "Failed to set %d slots, supported: 4, 8, 16\n",
> +				slots);
> +		return -ENOTSUPP;
> +	}
> +	width_index = (slots / 4) - 1;
> +
> +	switch (tx_mask) {
> +	case 0x000f:
> +		slot_index = 0;
> +		break;
> +	case 0x00f0:
> +		slot_index = 1;
> +		break;
> +	case 0x0f00:
> +		slot_index = 2;
> +		break;
> +	case 0xf000:
> +		slot_index = 3;
> +		break;
> +	default:
> +		/* must be contigious nibble */
> +		dev_err(dev, "Failed to set tx_mask %08x\n", tx_mask);
> +		return -ENOTSUPP;
> +	}
> +
> +	/* 48kHz sample rate, TDM configuration, 64-bit I2S frame period, PLL
> +	 * clock dither disabled, high-pass filter enabled (blocks DC output)
> +	 */
> +	data = IB3_SAMPLE_RATE_48_KHZ | IB3_FORMAT[width_index][slot_index] |
> +		IB3_I2S_FRAME_PERIOD_64 | IB3_HIGH_PASS_FILTER_ENABLE;
> +	ret = snd_soc_component_write(dai->component, TDA7802_IB3, data);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to write IB3 config %d\n", ret);
> +		return ret;
> +	}
> +
> +	tda7802_dump_regs(tda7802);
> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_dai_ops tda7802_dai_ops = {
> +	.startup      = tda7802_dai_startup,
> +	.shutdown     = tda7802_dai_shutdown,
> +	.digital_mute = tda7802_digital_mute,
> +	.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_48000,
> +		.formats      = SNDRV_PCM_FMTBIT_S32_LE,
> +	},
> +	.ops = &tda7802_dai_ops,
> +};
> +
> +/* read device tree or ACPI properties from device */
> +static int tda7802_read_properties(struct tda7802_priv *tda7802)
> +{
> +	struct device *dev = &tda7802->i2c->dev;
> +	int err = 0;
> +
> +	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);
> +	}
> +
> +	err = device_property_read_u8(dev, "st,gain-ch13", &tda7802->gain_ch13);
> +	if (err < 0)
> +		dev_err(dev, "Failed to read gain, channel 1,3 %d\n", err);
> +
> +	err = device_property_read_u8(dev, "st,gain-ch24", &tda7802->gain_ch24);
> +	if (err < 0)
> +		dev_err(dev, "Failed to read gain, channel 2,4 %d\n", err);
> +
> +	err = device_property_read_string(dev, "st,diagnostic-mode-ch13",
> +			&tda7802->diag_mode_ch13);

Why is diagnostic mode in dtb? Looks like it should be configurable on the
run-time, so a module parameter would be a more proper place for it

> +	if (err < 0)
> +		dev_err(dev, "Failed to read diagnostic mode, channel 1,3 %d\n",
> +				err);
> +
> +	err = device_property_read_string(dev, "st,diagnostic-mode-ch24",
> +			&tda7802->diag_mode_ch24);
> +	if (err < 0)
> +		dev_err(dev, "Failed to read diagnostic mode, channel 2,4 %d\n",
> +				err);
> +
> +	return err;
> +}
> +
> +static const struct snd_soc_component_driver tda7802_component_driver;
> +
> +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 status, err, err2;
> +
> +	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;
> +
> +	err = tda7802_read_properties(tda7802);
> +	if (err < 0)
> +		return err;
> +
> +	tda7802->regmap = devm_regmap_init_i2c(tda7802->i2c,
> +			&tda7802_regmap_config);
> +	if (IS_ERR(tda7802->regmap))
> +		return PTR_ERR(tda7802->regmap);
> +
> +	/* Enable and verify the connection */
> +	err = regulator_enable(tda7802->enable_reg);
> +	if (err < 0) {
> +		dev_err(dev, "Failed to enable (init)\n");
> +		return err;
> +	}
> +	msleep(ENABLE_DELAY_MS);
> +
> +	err = regmap_read(tda7802->regmap, TDA7802_DB1, &status);
> +
> +	/* always disable the regulator, even if the read fails */
> +	err2 = regulator_disable(tda7802->enable_reg);
> +	if (err2 < 0) {
> +		dev_err(dev, "Failed to disable (init)\n");
> +		return err2;
> +	}
> +	/* now check for regmap_read errors */
> +	if (err < 0) {
> +		dev_err(dev, "Could not read from device\n");
> +		return -ENODEV;
> +	}
> +
> +	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>");
>