[PATCH v3 0/2] ASoC: codecs: add support for TAS5720 digital amplifier

[PATCH v3 0/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

Updated driver for TI's TAS5720L/M digital audio amplifiers incorporating
previous feedback (thanks Mark Brown). Biggest change is that the driver is
no longer using interrupts for codec fault reporting, but instead uses a
polling-based approach to check for (and clear) codec fault conditions every
200ms. This is much more graceful and actually lower overhead than trying to
keep the actual TAS5720 fault interrupts (that come in every 300us!) under
control which is what v2 of the driver was doing. Because of the removal of
interrupts the DT doc had to be updated as well, so I did not carry forward
Rob Herring's earlier Acked-by because of said change.

--

Andreas Dannenberg
Texas Instruments Inc


Changes since v2:
- Switched fault handling from using interrupts to polling mode
- Remove interrupt related description from DT bindings doc
- Remove unlikely() to simplify an expression
- Remove unnesseary tas5720_set_dai_sysclk() function stub

Changes since v1:
- Simplified DT interrupt documentation (Thanks Rob Herring)
- Fixed potential race condition during codec probe where deferred work
  that's used by the threaded IRQ handler was setup after the IRQ was
  initialized (would lead to an issue when the TAS5720 was already throwing
  interrupts at the time of probe)

Andreas Dannenberg (2):
  ASoC: codecs: add TA5720 digital amplifier DT bindings
  ASoC: codecs: add support for TAS5720 digital amplifier

 .../devicetree/bindings/sound/tas5720.txt          |  26 +
 sound/soc/codecs/Kconfig                           |  10 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/tas5720.c                         | 638 +++++++++++++++++++++
 sound/soc/codecs/tas5720.h                         |  90 +++
 5 files changed, 766 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/tas5720.txt
 create mode 100644 sound/soc/codecs/tas5720.c
 create mode 100644 sound/soc/codecs/tas5720.h

-- 
2.6.4

[PATCH v3 0/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

Updated driver for TI's TAS5720L/M digital audio amplifiers incorporating
previous feedback (thanks Mark Brown). Biggest change is that the driver is
no longer using interrupts for codec fault reporting, but instead uses a
polling-based approach to check for (and clear) codec fault conditions every
200ms. This is much more graceful and actually lower overhead than trying to
keep the actual TAS5720 fault interrupts (that come in every 300us!) under
control which is what v2 of the driver was doing. Because of the removal of
interrupts the DT doc had to be updated as well, so I did not carry forward
Rob Herring's earlier Acked-by because of said change.

--

Andreas Dannenberg
Texas Instruments Inc


Changes since v2:
- Switched fault handling from using interrupts to polling mode
- Remove interrupt related description from DT bindings doc
- Remove unlikely() to simplify an expression
- Remove unnesseary tas5720_set_dai_sysclk() function stub

Changes since v1:
- Simplified DT interrupt documentation (Thanks Rob Herring)
- Fixed potential race condition during codec probe where deferred work
  that's used by the threaded IRQ handler was setup after the IRQ was
  initialized (would lead to an issue when the TAS5720 was already throwing
  interrupts at the time of probe)

Andreas Dannenberg (2):
  ASoC: codecs: add TA5720 digital amplifier DT bindings
  ASoC: codecs: add support for TAS5720 digital amplifier

 .../devicetree/bindings/sound/tas5720.txt          |  26 +
 sound/soc/codecs/Kconfig                           |  10 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/tas5720.c                         | 638 +++++++++++++++++++++
 sound/soc/codecs/tas5720.h                         |  90 +++
 5 files changed, 766 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/tas5720.txt
 create mode 100644 sound/soc/codecs/tas5720.c
 create mode 100644 sound/soc/codecs/tas5720.h

-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Andreas Dannenberg]

The Texas Instruments TAS5720L/M device is a high-efficiency mono
Class-D audio power amplifier optimized for high transient power
capability to use the dynamic power headroom of small loudspeakers.
Its digital time division multiplexed (TDM) interface enables up to
16 devices to share the same bus.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 .../devicetree/bindings/sound/tas5720.txt          | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/tas5720.txt

diff --git a/Documentation/devicetree/bindings/sound/tas5720.txt b/Documentation/devicetree/bindings/sound/tas5720.txt
new file mode 100644
index 0000000..439451e
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/tas5720.txt
@@ -0,0 +1,26 @@
+Texas Instruments TAS5720 Mono Audio amplifier
+
+The TAS5720 serial control bus communicates through the I2C protocol only. The
+serial bus is also used for periodic codec fault checking/reporting during
+audio playback. For more product information please see the links below:
+
+http://www.ti.com/product/TAS5720L
+http://www.ti.com/product/TAS5720M
+
+Required properties:
+
+- compatible : "ti,tas5720"
+- reg : I2C slave address
+
+Optional properties:
+
+- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
+- pvdd-supply : phandle to a supply used for the Class-D amp and the analog
+
+Example:
+
+tas5720: tas5720@6c {
+	status = "okay";
+	compatible = "ti,tas5720";
+	reg = <0x6c>;
+};
-- 
2.6.4

[PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Andreas Dannenberg]

The Texas Instruments TAS5720L/M device is a high-efficiency mono
Class-D audio power amplifier optimized for high transient power
capability to use the dynamic power headroom of small loudspeakers.
Its digital time division multiplexed (TDM) interface enables up to
16 devices to share the same bus.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 .../devicetree/bindings/sound/tas5720.txt          | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/tas5720.txt

diff --git a/Documentation/devicetree/bindings/sound/tas5720.txt b/Documentation/devicetree/bindings/sound/tas5720.txt
new file mode 100644
index 0000000..439451e
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/tas5720.txt
@@ -0,0 +1,26 @@
+Texas Instruments TAS5720 Mono Audio amplifier
+
+The TAS5720 serial control bus communicates through the I2C protocol only. The
+serial bus is also used for periodic codec fault checking/reporting during
+audio playback. For more product information please see the links below:
+
+http://www.ti.com/product/TAS5720L
+http://www.ti.com/product/TAS5720M
+
+Required properties:
+
+- compatible : "ti,tas5720"
+- reg : I2C slave address
+
+Optional properties:
+
+- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
+- pvdd-supply : phandle to a supply used for the Class-D amp and the analog
+
+Example:
+
+tas5720: tas5720@6c {
+	status = "okay";
+	compatible = "ti,tas5720";
+	reg = <0x6c>;
+};
-- 
2.6.4

[PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

The Texas Instruments TAS5720L/M device is a high-efficiency mono
Class-D audio power amplifier optimized for high transient power
capability to use the dynamic power headroom of small loudspeakers.
Its digital time division multiplexed (TDM) interface enables up to
16 devices to share the same bus.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 sound/soc/codecs/Kconfig   |  10 +
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/tas5720.c | 638 +++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/tas5720.h |  90 +++++++
 4 files changed, 740 insertions(+)
 create mode 100644 sound/soc/codecs/tas5720.c
 create mode 100644 sound/soc/codecs/tas5720.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 290f921..3949dae 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -125,6 +125,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_TAS2552 if I2C
 	select SND_SOC_TAS5086 if I2C
 	select SND_SOC_TAS571X if I2C
+	select SND_SOC_TAS5720 if I2C
 	select SND_SOC_TFA9879 if I2C
 	select SND_SOC_TLV320AIC23_I2C if I2C
 	select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
@@ -748,6 +749,15 @@ config SND_SOC_TAS571X
 	tristate "Texas Instruments TAS5711/TAS5717/TAS5719/TAS5721 power amplifiers"
 	depends on I2C
 
+config SND_SOC_TAS5720
+	tristate "Texas Instruments TAS5720 Mono Audio amplifier"
+	depends on I2C
+	help
+	  Enable support for Texas Instruments TAS5720L/M high-efficiency mono
+	  Class-D audio power amplifiers. The devices use an I2C interface for
+	  setup/control and support an optional GPIO interrupt signal for fault
+	  reporting.
+
 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 448c61d..54433cb 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -130,6 +130,7 @@ snd-soc-stac9766-objs := stac9766.o
 snd-soc-sti-sas-objs := sti-sas.o
 snd-soc-tas5086-objs := tas5086.o
 snd-soc-tas571x-objs := tas571x.o
+snd-soc-tas5720-objs := tas5720.o
 snd-soc-tfa9879-objs := tfa9879.o
 snd-soc-tlv320aic23-objs := tlv320aic23.o
 snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o
@@ -339,6 +340,7 @@ obj-$(CONFIG_SND_SOC_STI_SAS)	+= snd-soc-sti-sas.o
 obj-$(CONFIG_SND_SOC_TAS2552)	+= snd-soc-tas2552.o
 obj-$(CONFIG_SND_SOC_TAS5086)	+= snd-soc-tas5086.o
 obj-$(CONFIG_SND_SOC_TAS571X)	+= snd-soc-tas571x.o
+obj-$(CONFIG_SND_SOC_TAS5720)	+= snd-soc-tas5720.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/tas5720.c b/sound/soc/codecs/tas5720.c
new file mode 100644
index 0000000..1b5366f
--- /dev/null
+++ b/sound/soc/codecs/tas5720.c
@@ -0,0 +1,638 @@
+/*
+ * tas5720.c - ALSA SoC Texas Instruments TAS5720 Mono Audio Amplifier
+ *
+ * Copyright (C)2015-2016 Texas Instruments Incorporated -  http://www.ti.com
+ *
+ * Author: Andreas Dannenberg <dannenberg@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/regulator/consumer.h>
+#include <linux/delay.h>
+
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+#include <sound/tlv.h>
+
+#include "tas5720.h"
+
+/* Define how often to check (and clear) the fault status register (in ms) */
+#define TAS5720_FAULT_CHECK_INTERVAL		200
+
+static const char * const tas5720_supply_names[] = {
+	"dvdd",		/* Digital power supply. Connect to 3.3-V supply. */
+	"pvdd",		/* Class-D amp and analog power supply (connected). */
+};
+
+#define TAS5720_NUM_SUPPLIES	ARRAY_SIZE(tas5720_supply_names)
+
+struct tas5720_data {
+	struct snd_soc_codec *codec;
+	struct regmap *regmap;
+	struct i2c_client *tas5720_client;
+	struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
+	struct delayed_work fault_check_work;
+	unsigned int last_fault;
+};
+
+static int tas5720_hw_params(struct snd_pcm_substream *substream,
+			     struct snd_pcm_hw_params *params,
+			     struct snd_soc_dai *dai)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	int width = params_width(params);
+	unsigned int rate = params_rate(params);
+	bool ssz_ds;
+	int ret;
+
+	switch (width) {
+	case 16:
+	case 18:
+	case 20:
+	case 24:
+		/*
+		 * We only support the different left-justified serial audio
+		 * formats in which case there is nothing to configure in the
+		 * TAS5720.
+		 */
+		break;
+	default:
+		dev_err(codec->dev, "unsupported sample size: %d\n", width);
+		return -EINVAL;
+	}
+
+	switch (rate) {
+	case 44100:
+	case 48000:
+		ssz_ds = false;
+		break;
+	case 88200:
+	case 96000:
+		ssz_ds = true;
+		break;
+	default:
+		dev_err(codec->dev, "unsupported sample rate: %u\n", rate);
+		return -EINVAL;
+	}
+
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL1_REG,
+				  TAS5720_SSZ_DS, ssz_ds);
+	if (ret < 0) {
+		dev_err(codec->dev, "error setting sample rate: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tas5720_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	u8 serial_format;
+	int ret;
+
+	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) {
+		dev_vdbg(codec->dev, "DAI Format master is not found\n");
+		return -EINVAL;
+	}
+
+	switch (fmt & (SND_SOC_DAIFMT_FORMAT_MASK |
+		       SND_SOC_DAIFMT_INV_MASK)) {
+	case (SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF):
+		/* 1st data bit occur one BCLK cycle after the frame sync */
+		serial_format = TAS5720_SAIF_I2S;
+		break;
+	case (SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_NB_NF):
+		/*
+		 * Note that although the TAS5720 does not have a dedicated DSP
+		 * mode it doesn't care about the LRCLK duty cycle during TDM
+		 * operation. Therefore we can use the device's I2S mode with
+		 * its delaying of the 1st data bit to receive DSP_A formatted
+		 * data. See device datasheet for additional details.
+		 */
+		serial_format = TAS5720_SAIF_I2S;
+		break;
+	case (SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_NB_NF):
+		/*
+		 * Similar to DSP_A, we can use the fact that the TAS5720 does
+		 * not care about the LRCLK duty cycle during TDM to receive
+		 * DSP_B formatted data in LEFTJ mode (no delaying of the 1st
+		 * data bit).
+		 */
+		serial_format = TAS5720_SAIF_LEFTJ;
+		break;
+	case (SND_SOC_DAIFMT_LEFT_J | SND_SOC_DAIFMT_NB_NF):
+		/* No delay after the frame sync */
+		serial_format = TAS5720_SAIF_LEFTJ;
+		break;
+	default:
+		dev_vdbg(codec->dev, "DAI Format is not found\n");
+		return -EINVAL;
+	}
+
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL1_REG,
+				  TAS5720_SAIF_FORMAT_MASK,
+				  serial_format);
+	if (ret < 0) {
+		dev_err(codec->dev, "error setting SAIF format: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai,
+				    unsigned int tx_mask, unsigned int rx_mask,
+				    int slots, int slot_width)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	unsigned int first_slot;
+	int ret;
+
+	if (!tx_mask) {
+		dev_err(codec->dev, "tx masks must not be 0\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Determine the first slot that is being requested. We will only
+	 * use the first slot that is found since the TAS5720 is a mono
+	 * amplifier.
+	 */
+	first_slot = __ffs(tx_mask);
+
+	if (first_slot > 7) {
+		dev_err(codec->dev, "slot selection out of bounds (%u)\n",
+			first_slot);
+		return -EINVAL;
+	}
+
+	/* Enable manual TDM slot selection (instead of I2C ID based) */
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL1_REG,
+				  TAS5720_TDM_CFG_SRC, TAS5720_TDM_CFG_SRC);
+	if (ret < 0)
+		goto error_snd_soc_update_bits;
+
+	/* Configure the TDM slot to process audio from */
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL2_REG,
+				  TAS5720_TDM_SLOT_SEL_MASK, first_slot);
+	if (ret < 0)
+		goto error_snd_soc_update_bits;
+
+	return 0;
+
+error_snd_soc_update_bits:
+	dev_err(codec->dev, "error configuring TDM mode: %d\n", ret);
+	return ret;
+}
+
+static int tas5720_mute(struct snd_soc_dai *dai, int mute)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	int ret;
+
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL2_REG,
+				  TAS5720_MUTE, mute ? TAS5720_MUTE : 0);
+	if (ret < 0) {
+		dev_err(codec->dev, "error (un-)muting device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void tas5720_fault_check_work(struct work_struct *work)
+{
+	struct tas5720_data *tas5720 = container_of(work, struct tas5720_data,
+			fault_check_work.work);
+	struct device *dev = tas5720->codec->dev;
+	unsigned int curr_fault;
+	int ret;
+
+	ret = regmap_read(tas5720->regmap, TAS5720_FAULT_REG, &curr_fault);
+	if (ret < 0) {
+		dev_err(dev, "failed to read FAULT register: %d\n", ret);
+		goto out;
+	}
+
+	/* Check/handle all errors except SAIF clock errors */
+	curr_fault &= TAS5720_OCE | TAS5720_DCE | TAS5720_OTE;
+
+	/*
+	 * Only flag errors once for a given occurrence. This is needed as
+	 * the TAS5720 will take time clearing the fault condition internally
+	 * during which we don't want to bombard the system with the same
+	 * error message over and over.
+	 */
+	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
+		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");
+
+	if ((curr_fault & TAS5720_DCE) && !(tas5720->last_fault & TAS5720_DCE))
+		dev_warn(dev, "The Class-D output stage has experienced a DC detect error\n");
+
+	if ((curr_fault & TAS5720_OTE) && !(tas5720->last_fault & TAS5720_OTE))
+		dev_warn(dev, "The Class-D output stage has experienced an over temperature error\n");
+
+	/* Store current fault value so we can detect any changes next time */
+	tas5720->last_fault = curr_fault;
+
+	if (!curr_fault)
+		goto out;
+
+	/*
+	 * Periodically toggle SDZ (shutdown bit) H->L->H to clear any latching
+	 * faults as long as a fault condition persists. Always going through
+	 * the full sequence no matter the first return value to minimizes
+	 * chances for the device to end up in shutdown mode.
+	 */
+	ret = regmap_write_bits(tas5720->regmap, TAS5720_POWER_CTRL_REG,
+				TAS5720_SDZ, 0);
+	if (ret < 0)
+		dev_err(dev, "failed to write POWER_CTRL register: %d\n", ret);
+
+	ret = regmap_write_bits(tas5720->regmap, TAS5720_POWER_CTRL_REG,
+				TAS5720_SDZ, TAS5720_SDZ);
+	if (ret < 0)
+		dev_err(dev, "failed to write POWER_CTRL register: %d\n", ret);
+
+out:
+	/* Schedule the next fault check at the specified interval */
+	schedule_delayed_work(&tas5720->fault_check_work,
+			      msecs_to_jiffies(TAS5720_FAULT_CHECK_INTERVAL));
+}
+
+static int tas5720_codec_probe(struct snd_soc_codec *codec)
+{
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	unsigned int device_id;
+	int ret;
+
+	tas5720->codec = codec;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(tas5720->supplies),
+				    tas5720->supplies);
+	if (ret != 0) {
+		dev_err(codec->dev, "failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_read(tas5720->regmap, TAS5720_DEVICE_ID_REG, &device_id);
+	if (ret < 0) {
+		dev_err(codec->dev, "failed to read device ID register: %d\n",
+			ret);
+		goto probe_fail;
+	}
+
+	if (device_id != TAS5720_DEVICE_ID) {
+		dev_err(codec->dev, "wrong device ID. expected: %u read: %u\n",
+			TAS5720_DEVICE_ID, device_id);
+		ret = -ENODEV;
+		goto probe_fail;
+	}
+
+	/* Set device to mute */
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL2_REG,
+				  TAS5720_MUTE, TAS5720_MUTE);
+	if (ret < 0)
+		goto error_snd_soc_update_bits;
+
+	/*
+	 * Enter shutdown mode - our default when not playing audio - to
+	 * minimize current consumption. On the TAS5720 there is no real down
+	 * side doing so as all device registers are preserved and the wakeup
+	 * of the codec is rather quick which we do using a dapm widget.
+	 */
+	ret = snd_soc_update_bits(codec, TAS5720_POWER_CTRL_REG,
+				  TAS5720_SDZ, 0);
+	if (ret < 0)
+		goto error_snd_soc_update_bits;
+
+	INIT_DELAYED_WORK(&tas5720->fault_check_work, tas5720_fault_check_work);
+
+	return 0;
+
+error_snd_soc_update_bits:
+	dev_err(codec->dev, "error configuring device registers: %d\n", ret);
+
+probe_fail:
+	regulator_bulk_disable(ARRAY_SIZE(tas5720->supplies),
+			       tas5720->supplies);
+	return ret;
+}
+
+static int tas5720_codec_remove(struct snd_soc_codec *codec)
+{
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	cancel_delayed_work_sync(&tas5720->fault_check_work);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(tas5720->supplies),
+				     tas5720->supplies);
+	if (ret < 0)
+		dev_err(codec->dev, "failed to disable supplies: %d\n", ret);
+
+	return ret;
+};
+
+static int tas5720_dac_event(struct snd_soc_dapm_widget *w,
+			     struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	if (event & SND_SOC_DAPM_POST_PMU) {
+		/* Take TAS5720 out of shutdown mode */
+		ret = snd_soc_update_bits(codec, TAS5720_POWER_CTRL_REG,
+					  TAS5720_SDZ, TAS5720_SDZ);
+		if (ret < 0) {
+			dev_err(codec->dev, "error waking codec: %d\n", ret);
+			return ret;
+		}
+
+		/*
+		 * Observe codec shutdown-to-active time. The datasheet only
+		 * lists a nominal value however just use-it as-is without
+		 * additional padding to minimize the delay introduced in
+		 * starting to play audio (actually there is other setup done
+		 * by the ASoC framework that will provide additional delays,
+		 * so we should always be safe).
+		 */
+		msleep(25);
+
+		/* Turn on TAS5720 periodic fault checking/handling */
+		tas5720->last_fault = 0;
+		schedule_delayed_work(&tas5720->fault_check_work,
+				msecs_to_jiffies(TAS5720_FAULT_CHECK_INTERVAL));
+	} else if (event & SND_SOC_DAPM_PRE_PMD) {
+		/* Disable TAS5720 periodic fault checking/handling */
+		cancel_delayed_work_sync(&tas5720->fault_check_work);
+
+		/* Place TAS5720 in shutdown mode to minimize current draw */
+		ret = snd_soc_update_bits(codec, TAS5720_POWER_CTRL_REG,
+					  TAS5720_SDZ, 0);
+		if (ret < 0) {
+			dev_err(codec->dev, "error shutting down codec: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int tas5720_suspend(struct snd_soc_codec *codec)
+{
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	regcache_cache_only(tas5720->regmap, true);
+	regcache_mark_dirty(tas5720->regmap);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(tas5720->supplies),
+				     tas5720->supplies);
+	if (ret < 0)
+		dev_err(codec->dev, "failed to disable supplies: %d\n", ret);
+
+	return ret;
+}
+
+static int tas5720_resume(struct snd_soc_codec *codec)
+{
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(tas5720->supplies),
+				    tas5720->supplies);
+	if (ret < 0) {
+		dev_err(codec->dev, "failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	regcache_cache_only(tas5720->regmap, false);
+
+	ret = regcache_sync(tas5720->regmap);
+	if (ret < 0) {
+		dev_err(codec->dev, "failed to sync regcache: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#else
+#define tas5720_suspend NULL
+#define tas5720_resume NULL
+#endif
+
+static bool tas5720_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TAS5720_DEVICE_ID_REG:
+	case TAS5720_FAULT_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config tas5720_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = TAS5720_MAX_REG,
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = tas5720_is_volatile_reg,
+};
+
+/*
+ * DAC analog gain. There are four discrete values to select from, ranging
+ * from 19.2 dB to 26.3dB.
+ */
+static const DECLARE_TLV_DB_RANGE(dac_analog_tlv,
+	0x0, 0x0, TLV_DB_SCALE_ITEM(1920, 0, 0),
+	0x1, 0x1, TLV_DB_SCALE_ITEM(2070, 0, 0),
+	0x2, 0x2, TLV_DB_SCALE_ITEM(2350, 0, 0),
+	0x3, 0x3, TLV_DB_SCALE_ITEM(2630, 0, 0),
+);
+
+/*
+ * DAC digital volumes. From -103.5 to 24 dB in 0.5 dB steps. Note that
+ * setting the gain below -100 dB (register value <0x7) is effectively a MUTE
+ * as per device datasheet.
+ */
+static DECLARE_TLV_DB_SCALE(dac_tlv, -10350, 50, 0);
+
+static const struct snd_kcontrol_new tas5720_snd_controls[] = {
+	SOC_SINGLE_TLV("Speaker Driver Playback Volume",
+		       TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, dac_tlv),
+	SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG,
+		       TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv),
+};
+
+static const struct snd_soc_dapm_widget tas5720_dapm_widgets[] = {
+	SND_SOC_DAPM_AIF_IN("DAC IN", "Playback", 0, SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_DAC_E("DAC", NULL, SND_SOC_NOPM, 0, 0, tas5720_dac_event,
+			   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
+	SND_SOC_DAPM_OUTPUT("OUT")
+};
+
+static const struct snd_soc_dapm_route tas5720_audio_map[] = {
+	{ "DAC", NULL, "DAC IN" },
+	{ "OUT", NULL, "DAC" },
+};
+
+static struct snd_soc_codec_driver soc_codec_dev_tas5720 = {
+	.probe = tas5720_codec_probe,
+	.remove = tas5720_codec_remove,
+	.suspend = tas5720_suspend,
+	.resume = tas5720_resume,
+
+	.controls = tas5720_snd_controls,
+	.num_controls = ARRAY_SIZE(tas5720_snd_controls),
+	.dapm_widgets = tas5720_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets),
+	.dapm_routes = tas5720_audio_map,
+	.num_dapm_routes = ARRAY_SIZE(tas5720_audio_map),
+};
+
+/* PCM rates supported by the TAS5720 driver */
+#define TAS5720_RATES	(SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\
+			 SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
+
+/* Formats supported by TAS5720 driver */
+#define TAS5720_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S18_3LE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S24_LE)
+
+static struct snd_soc_dai_ops tas5720_speaker_dai_ops = {
+	.hw_params	= tas5720_hw_params,
+	.set_fmt	= tas5720_set_dai_fmt,
+	.set_tdm_slot	= tas5720_set_dai_tdm_slot,
+	.digital_mute	= tas5720_mute,
+};
+
+/*
+ * TAS5720 DAI structure
+ *
+ * Note that were are advertising .playback.channels_max = 2 despite this being
+ * a mono amplifier. The reason for that is that some serial ports such as TI's
+ * McASP module have a minimum number of channels (2) that they can output.
+ * Advertising more channels than we have will allow us to interface with such
+ * a serial port without really any negative side effects as the TAS5720 will
+ * simply ignore any extra channel(s) asides from the one channel that is
+ * configured to be played back.
+ */
+static struct snd_soc_dai_driver tas5720_dai[] = {
+	{
+		.name = "tas5720-amplifier",
+		.playback = {
+			.stream_name = "Playback",
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = TAS5720_RATES,
+			.formats = TAS5720_FORMATS,
+		},
+		.ops = &tas5720_speaker_dai_ops,
+	},
+};
+
+static int tas5720_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct tas5720_data *data;
+	int ret;
+	int i;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->tas5720_client = client;
+	data->regmap = devm_regmap_init_i2c(client, &tas5720_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		ret = PTR_ERR(data->regmap);
+		dev_err(dev, "failed to allocate register map: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(data->supplies); i++)
+		data->supplies[i].supply = tas5720_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
+				      data->supplies);
+	if (ret != 0) {
+		dev_err(dev, "failed to request supplies: %d\n", ret);
+		return ret;
+	}
+
+	dev_set_drvdata(dev, data);
+
+	ret = snd_soc_register_codec(&client->dev,
+				     &soc_codec_dev_tas5720,
+				     tas5720_dai, ARRAY_SIZE(tas5720_dai));
+	if (ret < 0) {
+		dev_err(dev, "failed to register codec: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tas5720_remove(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+
+	snd_soc_unregister_codec(dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id tas5720_id[] = {
+	{ "tas5720", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tas5720_id);
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id tas5720_of_match[] = {
+	{ .compatible = "ti,tas5720", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tas5720_of_match);
+#endif
+
+static struct i2c_driver tas5720_i2c_driver = {
+	.driver = {
+		.name = "tas5720",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(tas5720_of_match),
+	},
+	.probe = tas5720_probe,
+	.remove = tas5720_remove,
+	.id_table = tas5720_id,
+};
+
+module_i2c_driver(tas5720_i2c_driver);
+
+MODULE_AUTHOR("Andreas Dannenberg <dannenberg@ti.com>");
+MODULE_DESCRIPTION("TAS5720 Audio amplifier driver");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/tas5720.h b/sound/soc/codecs/tas5720.h
new file mode 100644
index 0000000..3d077c7
--- /dev/null
+++ b/sound/soc/codecs/tas5720.h
@@ -0,0 +1,90 @@
+/*
+ * tas5720.h - ALSA SoC Texas Instruments TAS5720 Mono Audio Amplifier
+ *
+ * Copyright (C)2015-2016 Texas Instruments Incorporated -  http://www.ti.com
+ *
+ * Author: Andreas Dannenberg <dannenberg@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef __TAS5720_H__
+#define __TAS5720_H__
+
+/* Register Address Map */
+#define TAS5720_DEVICE_ID_REG		0x00
+#define TAS5720_POWER_CTRL_REG		0x01
+#define TAS5720_DIGITAL_CTRL1_REG	0x02
+#define TAS5720_DIGITAL_CTRL2_REG	0x03
+#define TAS5720_VOLUME_CTRL_REG		0x04
+#define TAS5720_ANALOG_CTRL_REG		0x06
+#define TAS5720_FAULT_REG		0x08
+#define TAS5720_DIGITAL_CLIP2_REG	0x10
+#define TAS5720_DIGITAL_CLIP1_REG	0x11
+#define TAS5720_MAX_REG			TAS5720_DIGITAL_CLIP1_REG
+
+/* TAS5720_DEVICE_ID_REG */
+#define TAS5720_DEVICE_ID		0x01
+
+/* TAS5720_POWER_CTRL_REG */
+#define TAS5720_DIG_CLIP_MASK		GENMASK(7, 2)
+#define TAS5720_SLEEP			BIT(1)
+#define TAS5720_SDZ			BIT(0)
+
+/* TAS5720_DIGITAL_CTRL1_REG */
+#define TAS5720_HPF_BYPASS		BIT(7)
+#define TAS5720_TDM_CFG_SRC		BIT(6)
+#define TAS5720_SSZ_DS			BIT(3)
+#define TAS5720_SAIF_RIGHTJ_24BIT	(0x0)
+#define TAS5720_SAIF_RIGHTJ_20BIT	(0x1)
+#define TAS5720_SAIF_RIGHTJ_18BIT	(0x2)
+#define TAS5720_SAIF_RIGHTJ_16BIT	(0x3)
+#define TAS5720_SAIF_I2S		(0x4)
+#define TAS5720_SAIF_LEFTJ		(0x5)
+#define TAS5720_SAIF_FORMAT_MASK	GENMASK(2, 0)
+
+/* TAS5720_DIGITAL_CTRL2_REG */
+#define TAS5720_MUTE			BIT(4)
+#define TAS5720_TDM_SLOT_SEL_MASK	GENMASK(2, 0)
+
+/* TAS5720_ANALOG_CTRL_REG */
+#define TAS5720_PWM_RATE_6_3_FSYNC	(0x0 << 4)
+#define TAS5720_PWM_RATE_8_4_FSYNC	(0x1 << 4)
+#define TAS5720_PWM_RATE_10_5_FSYNC	(0x2 << 4)
+#define TAS5720_PWM_RATE_12_6_FSYNC	(0x3 << 4)
+#define TAS5720_PWM_RATE_14_7_FSYNC	(0x4 << 4)
+#define TAS5720_PWM_RATE_16_8_FSYNC	(0x5 << 4)
+#define TAS5720_PWM_RATE_20_10_FSYNC	(0x6 << 4)
+#define TAS5720_PWM_RATE_24_12_FSYNC	(0x7 << 4)
+#define TAS5720_PWM_RATE_MASK		GENMASK(6, 4)
+#define TAS5720_ANALOG_GAIN_19_2DBV	(0x0 << 2)
+#define TAS5720_ANALOG_GAIN_20_7DBV	(0x1 << 2)
+#define TAS5720_ANALOG_GAIN_23_5DBV	(0x2 << 2)
+#define TAS5720_ANALOG_GAIN_26_3DBV	(0x3 << 2)
+#define TAS5720_ANALOG_GAIN_MASK	GENMASK(3, 2)
+#define TAS5720_ANALOG_GAIN_SHIFT	(0x2)
+
+/* TAS5720_FAULT_REG */
+#define TAS5720_OC_THRESH_100PCT	(0x0 << 4)
+#define TAS5720_OC_THRESH_75PCT		(0x1 << 4)
+#define TAS5720_OC_THRESH_50PCT		(0x2 << 4)
+#define TAS5720_OC_THRESH_25PCT		(0x3 << 4)
+#define TAS5720_OC_THRESH_MASK		GENMASK(5, 4)
+#define TAS5720_CLKE			BIT(3)
+#define TAS5720_OCE			BIT(2)
+#define TAS5720_DCE			BIT(1)
+#define TAS5720_OTE			BIT(0)
+#define TAS5720_FAULT_MASK		GENMASK(3, 0)
+
+/* TAS5720_DIGITAL_CLIP1_REG */
+#define TAS5720_CLIP1_MASK		GENMASK(7, 2)
+#define TAS5720_CLIP1_SHIFT		(0x2)
+
+#endif /* __TAS5720_H__ */
-- 
2.6.4

[PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

The Texas Instruments TAS5720L/M device is a high-efficiency mono
Class-D audio power amplifier optimized for high transient power
capability to use the dynamic power headroom of small loudspeakers.
Its digital time division multiplexed (TDM) interface enables up to
16 devices to share the same bus.

Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
---
 sound/soc/codecs/Kconfig   |  10 +
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/tas5720.c | 638 +++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/tas5720.h |  90 +++++++
 4 files changed, 740 insertions(+)
 create mode 100644 sound/soc/codecs/tas5720.c
 create mode 100644 sound/soc/codecs/tas5720.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 290f921..3949dae 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -125,6 +125,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_TAS2552 if I2C
 	select SND_SOC_TAS5086 if I2C
 	select SND_SOC_TAS571X if I2C
+	select SND_SOC_TAS5720 if I2C
 	select SND_SOC_TFA9879 if I2C
 	select SND_SOC_TLV320AIC23_I2C if I2C
 	select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
@@ -748,6 +749,15 @@ config SND_SOC_TAS571X
 	tristate "Texas Instruments TAS5711/TAS5717/TAS5719/TAS5721 power amplifiers"
 	depends on I2C
 
+config SND_SOC_TAS5720
+	tristate "Texas Instruments TAS5720 Mono Audio amplifier"
+	depends on I2C
+	help
+	  Enable support for Texas Instruments TAS5720L/M high-efficiency mono
+	  Class-D audio power amplifiers. The devices use an I2C interface for
+	  setup/control and support an optional GPIO interrupt signal for fault
+	  reporting.
+
 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 448c61d..54433cb 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -130,6 +130,7 @@ snd-soc-stac9766-objs := stac9766.o
 snd-soc-sti-sas-objs := sti-sas.o
 snd-soc-tas5086-objs := tas5086.o
 snd-soc-tas571x-objs := tas571x.o
+snd-soc-tas5720-objs := tas5720.o
 snd-soc-tfa9879-objs := tfa9879.o
 snd-soc-tlv320aic23-objs := tlv320aic23.o
 snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o
@@ -339,6 +340,7 @@ obj-$(CONFIG_SND_SOC_STI_SAS)	+= snd-soc-sti-sas.o
 obj-$(CONFIG_SND_SOC_TAS2552)	+= snd-soc-tas2552.o
 obj-$(CONFIG_SND_SOC_TAS5086)	+= snd-soc-tas5086.o
 obj-$(CONFIG_SND_SOC_TAS571X)	+= snd-soc-tas571x.o
+obj-$(CONFIG_SND_SOC_TAS5720)	+= snd-soc-tas5720.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/tas5720.c b/sound/soc/codecs/tas5720.c
new file mode 100644
index 0000000..1b5366f
--- /dev/null
+++ b/sound/soc/codecs/tas5720.c
@@ -0,0 +1,638 @@
+/*
+ * tas5720.c - ALSA SoC Texas Instruments TAS5720 Mono Audio Amplifier
+ *
+ * Copyright (C)2015-2016 Texas Instruments Incorporated -  http://www.ti.com
+ *
+ * Author: Andreas Dannenberg <dannenberg@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/regulator/consumer.h>
+#include <linux/delay.h>
+
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+#include <sound/tlv.h>
+
+#include "tas5720.h"
+
+/* Define how often to check (and clear) the fault status register (in ms) */
+#define TAS5720_FAULT_CHECK_INTERVAL		200
+
+static const char * const tas5720_supply_names[] = {
+	"dvdd",		/* Digital power supply. Connect to 3.3-V supply. */
+	"pvdd",		/* Class-D amp and analog power supply (connected). */
+};
+
+#define TAS5720_NUM_SUPPLIES	ARRAY_SIZE(tas5720_supply_names)
+
+struct tas5720_data {
+	struct snd_soc_codec *codec;
+	struct regmap *regmap;
+	struct i2c_client *tas5720_client;
+	struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES];
+	struct delayed_work fault_check_work;
+	unsigned int last_fault;
+};
+
+static int tas5720_hw_params(struct snd_pcm_substream *substream,
+			     struct snd_pcm_hw_params *params,
+			     struct snd_soc_dai *dai)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	int width = params_width(params);
+	unsigned int rate = params_rate(params);
+	bool ssz_ds;
+	int ret;
+
+	switch (width) {
+	case 16:
+	case 18:
+	case 20:
+	case 24:
+		/*
+		 * We only support the different left-justified serial audio
+		 * formats in which case there is nothing to configure in the
+		 * TAS5720.
+		 */
+		break;
+	default:
+		dev_err(codec->dev, "unsupported sample size: %d\n", width);
+		return -EINVAL;
+	}
+
+	switch (rate) {
+	case 44100:
+	case 48000:
+		ssz_ds = false;
+		break;
+	case 88200:
+	case 96000:
+		ssz_ds = true;
+		break;
+	default:
+		dev_err(codec->dev, "unsupported sample rate: %u\n", rate);
+		return -EINVAL;
+	}
+
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL1_REG,
+				  TAS5720_SSZ_DS, ssz_ds);
+	if (ret < 0) {
+		dev_err(codec->dev, "error setting sample rate: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tas5720_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	u8 serial_format;
+	int ret;
+
+	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) {
+		dev_vdbg(codec->dev, "DAI Format master is not found\n");
+		return -EINVAL;
+	}
+
+	switch (fmt & (SND_SOC_DAIFMT_FORMAT_MASK |
+		       SND_SOC_DAIFMT_INV_MASK)) {
+	case (SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF):
+		/* 1st data bit occur one BCLK cycle after the frame sync */
+		serial_format = TAS5720_SAIF_I2S;
+		break;
+	case (SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_NB_NF):
+		/*
+		 * Note that although the TAS5720 does not have a dedicated DSP
+		 * mode it doesn't care about the LRCLK duty cycle during TDM
+		 * operation. Therefore we can use the device's I2S mode with
+		 * its delaying of the 1st data bit to receive DSP_A formatted
+		 * data. See device datasheet for additional details.
+		 */
+		serial_format = TAS5720_SAIF_I2S;
+		break;
+	case (SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_NB_NF):
+		/*
+		 * Similar to DSP_A, we can use the fact that the TAS5720 does
+		 * not care about the LRCLK duty cycle during TDM to receive
+		 * DSP_B formatted data in LEFTJ mode (no delaying of the 1st
+		 * data bit).
+		 */
+		serial_format = TAS5720_SAIF_LEFTJ;
+		break;
+	case (SND_SOC_DAIFMT_LEFT_J | SND_SOC_DAIFMT_NB_NF):
+		/* No delay after the frame sync */
+		serial_format = TAS5720_SAIF_LEFTJ;
+		break;
+	default:
+		dev_vdbg(codec->dev, "DAI Format is not found\n");
+		return -EINVAL;
+	}
+
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL1_REG,
+				  TAS5720_SAIF_FORMAT_MASK,
+				  serial_format);
+	if (ret < 0) {
+		dev_err(codec->dev, "error setting SAIF format: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tas5720_set_dai_tdm_slot(struct snd_soc_dai *dai,
+				    unsigned int tx_mask, unsigned int rx_mask,
+				    int slots, int slot_width)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	unsigned int first_slot;
+	int ret;
+
+	if (!tx_mask) {
+		dev_err(codec->dev, "tx masks must not be 0\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Determine the first slot that is being requested. We will only
+	 * use the first slot that is found since the TAS5720 is a mono
+	 * amplifier.
+	 */
+	first_slot = __ffs(tx_mask);
+
+	if (first_slot > 7) {
+		dev_err(codec->dev, "slot selection out of bounds (%u)\n",
+			first_slot);
+		return -EINVAL;
+	}
+
+	/* Enable manual TDM slot selection (instead of I2C ID based) */
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL1_REG,
+				  TAS5720_TDM_CFG_SRC, TAS5720_TDM_CFG_SRC);
+	if (ret < 0)
+		goto error_snd_soc_update_bits;
+
+	/* Configure the TDM slot to process audio from */
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL2_REG,
+				  TAS5720_TDM_SLOT_SEL_MASK, first_slot);
+	if (ret < 0)
+		goto error_snd_soc_update_bits;
+
+	return 0;
+
+error_snd_soc_update_bits:
+	dev_err(codec->dev, "error configuring TDM mode: %d\n", ret);
+	return ret;
+}
+
+static int tas5720_mute(struct snd_soc_dai *dai, int mute)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	int ret;
+
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL2_REG,
+				  TAS5720_MUTE, mute ? TAS5720_MUTE : 0);
+	if (ret < 0) {
+		dev_err(codec->dev, "error (un-)muting device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void tas5720_fault_check_work(struct work_struct *work)
+{
+	struct tas5720_data *tas5720 = container_of(work, struct tas5720_data,
+			fault_check_work.work);
+	struct device *dev = tas5720->codec->dev;
+	unsigned int curr_fault;
+	int ret;
+
+	ret = regmap_read(tas5720->regmap, TAS5720_FAULT_REG, &curr_fault);
+	if (ret < 0) {
+		dev_err(dev, "failed to read FAULT register: %d\n", ret);
+		goto out;
+	}
+
+	/* Check/handle all errors except SAIF clock errors */
+	curr_fault &= TAS5720_OCE | TAS5720_DCE | TAS5720_OTE;
+
+	/*
+	 * Only flag errors once for a given occurrence. This is needed as
+	 * the TAS5720 will take time clearing the fault condition internally
+	 * during which we don't want to bombard the system with the same
+	 * error message over and over.
+	 */
+	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
+		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");
+
+	if ((curr_fault & TAS5720_DCE) && !(tas5720->last_fault & TAS5720_DCE))
+		dev_warn(dev, "The Class-D output stage has experienced a DC detect error\n");
+
+	if ((curr_fault & TAS5720_OTE) && !(tas5720->last_fault & TAS5720_OTE))
+		dev_warn(dev, "The Class-D output stage has experienced an over temperature error\n");
+
+	/* Store current fault value so we can detect any changes next time */
+	tas5720->last_fault = curr_fault;
+
+	if (!curr_fault)
+		goto out;
+
+	/*
+	 * Periodically toggle SDZ (shutdown bit) H->L->H to clear any latching
+	 * faults as long as a fault condition persists. Always going through
+	 * the full sequence no matter the first return value to minimizes
+	 * chances for the device to end up in shutdown mode.
+	 */
+	ret = regmap_write_bits(tas5720->regmap, TAS5720_POWER_CTRL_REG,
+				TAS5720_SDZ, 0);
+	if (ret < 0)
+		dev_err(dev, "failed to write POWER_CTRL register: %d\n", ret);
+
+	ret = regmap_write_bits(tas5720->regmap, TAS5720_POWER_CTRL_REG,
+				TAS5720_SDZ, TAS5720_SDZ);
+	if (ret < 0)
+		dev_err(dev, "failed to write POWER_CTRL register: %d\n", ret);
+
+out:
+	/* Schedule the next fault check at the specified interval */
+	schedule_delayed_work(&tas5720->fault_check_work,
+			      msecs_to_jiffies(TAS5720_FAULT_CHECK_INTERVAL));
+}
+
+static int tas5720_codec_probe(struct snd_soc_codec *codec)
+{
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	unsigned int device_id;
+	int ret;
+
+	tas5720->codec = codec;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(tas5720->supplies),
+				    tas5720->supplies);
+	if (ret != 0) {
+		dev_err(codec->dev, "failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_read(tas5720->regmap, TAS5720_DEVICE_ID_REG, &device_id);
+	if (ret < 0) {
+		dev_err(codec->dev, "failed to read device ID register: %d\n",
+			ret);
+		goto probe_fail;
+	}
+
+	if (device_id != TAS5720_DEVICE_ID) {
+		dev_err(codec->dev, "wrong device ID. expected: %u read: %u\n",
+			TAS5720_DEVICE_ID, device_id);
+		ret = -ENODEV;
+		goto probe_fail;
+	}
+
+	/* Set device to mute */
+	ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL2_REG,
+				  TAS5720_MUTE, TAS5720_MUTE);
+	if (ret < 0)
+		goto error_snd_soc_update_bits;
+
+	/*
+	 * Enter shutdown mode - our default when not playing audio - to
+	 * minimize current consumption. On the TAS5720 there is no real down
+	 * side doing so as all device registers are preserved and the wakeup
+	 * of the codec is rather quick which we do using a dapm widget.
+	 */
+	ret = snd_soc_update_bits(codec, TAS5720_POWER_CTRL_REG,
+				  TAS5720_SDZ, 0);
+	if (ret < 0)
+		goto error_snd_soc_update_bits;
+
+	INIT_DELAYED_WORK(&tas5720->fault_check_work, tas5720_fault_check_work);
+
+	return 0;
+
+error_snd_soc_update_bits:
+	dev_err(codec->dev, "error configuring device registers: %d\n", ret);
+
+probe_fail:
+	regulator_bulk_disable(ARRAY_SIZE(tas5720->supplies),
+			       tas5720->supplies);
+	return ret;
+}
+
+static int tas5720_codec_remove(struct snd_soc_codec *codec)
+{
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	cancel_delayed_work_sync(&tas5720->fault_check_work);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(tas5720->supplies),
+				     tas5720->supplies);
+	if (ret < 0)
+		dev_err(codec->dev, "failed to disable supplies: %d\n", ret);
+
+	return ret;
+};
+
+static int tas5720_dac_event(struct snd_soc_dapm_widget *w,
+			     struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	if (event & SND_SOC_DAPM_POST_PMU) {
+		/* Take TAS5720 out of shutdown mode */
+		ret = snd_soc_update_bits(codec, TAS5720_POWER_CTRL_REG,
+					  TAS5720_SDZ, TAS5720_SDZ);
+		if (ret < 0) {
+			dev_err(codec->dev, "error waking codec: %d\n", ret);
+			return ret;
+		}
+
+		/*
+		 * Observe codec shutdown-to-active time. The datasheet only
+		 * lists a nominal value however just use-it as-is without
+		 * additional padding to minimize the delay introduced in
+		 * starting to play audio (actually there is other setup done
+		 * by the ASoC framework that will provide additional delays,
+		 * so we should always be safe).
+		 */
+		msleep(25);
+
+		/* Turn on TAS5720 periodic fault checking/handling */
+		tas5720->last_fault = 0;
+		schedule_delayed_work(&tas5720->fault_check_work,
+				msecs_to_jiffies(TAS5720_FAULT_CHECK_INTERVAL));
+	} else if (event & SND_SOC_DAPM_PRE_PMD) {
+		/* Disable TAS5720 periodic fault checking/handling */
+		cancel_delayed_work_sync(&tas5720->fault_check_work);
+
+		/* Place TAS5720 in shutdown mode to minimize current draw */
+		ret = snd_soc_update_bits(codec, TAS5720_POWER_CTRL_REG,
+					  TAS5720_SDZ, 0);
+		if (ret < 0) {
+			dev_err(codec->dev, "error shutting down codec: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int tas5720_suspend(struct snd_soc_codec *codec)
+{
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	regcache_cache_only(tas5720->regmap, true);
+	regcache_mark_dirty(tas5720->regmap);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(tas5720->supplies),
+				     tas5720->supplies);
+	if (ret < 0)
+		dev_err(codec->dev, "failed to disable supplies: %d\n", ret);
+
+	return ret;
+}
+
+static int tas5720_resume(struct snd_soc_codec *codec)
+{
+	struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(tas5720->supplies),
+				    tas5720->supplies);
+	if (ret < 0) {
+		dev_err(codec->dev, "failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	regcache_cache_only(tas5720->regmap, false);
+
+	ret = regcache_sync(tas5720->regmap);
+	if (ret < 0) {
+		dev_err(codec->dev, "failed to sync regcache: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#else
+#define tas5720_suspend NULL
+#define tas5720_resume NULL
+#endif
+
+static bool tas5720_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TAS5720_DEVICE_ID_REG:
+	case TAS5720_FAULT_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config tas5720_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = TAS5720_MAX_REG,
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = tas5720_is_volatile_reg,
+};
+
+/*
+ * DAC analog gain. There are four discrete values to select from, ranging
+ * from 19.2 dB to 26.3dB.
+ */
+static const DECLARE_TLV_DB_RANGE(dac_analog_tlv,
+	0x0, 0x0, TLV_DB_SCALE_ITEM(1920, 0, 0),
+	0x1, 0x1, TLV_DB_SCALE_ITEM(2070, 0, 0),
+	0x2, 0x2, TLV_DB_SCALE_ITEM(2350, 0, 0),
+	0x3, 0x3, TLV_DB_SCALE_ITEM(2630, 0, 0),
+);
+
+/*
+ * DAC digital volumes. From -103.5 to 24 dB in 0.5 dB steps. Note that
+ * setting the gain below -100 dB (register value <0x7) is effectively a MUTE
+ * as per device datasheet.
+ */
+static DECLARE_TLV_DB_SCALE(dac_tlv, -10350, 50, 0);
+
+static const struct snd_kcontrol_new tas5720_snd_controls[] = {
+	SOC_SINGLE_TLV("Speaker Driver Playback Volume",
+		       TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, dac_tlv),
+	SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG,
+		       TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv),
+};
+
+static const struct snd_soc_dapm_widget tas5720_dapm_widgets[] = {
+	SND_SOC_DAPM_AIF_IN("DAC IN", "Playback", 0, SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_DAC_E("DAC", NULL, SND_SOC_NOPM, 0, 0, tas5720_dac_event,
+			   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
+	SND_SOC_DAPM_OUTPUT("OUT")
+};
+
+static const struct snd_soc_dapm_route tas5720_audio_map[] = {
+	{ "DAC", NULL, "DAC IN" },
+	{ "OUT", NULL, "DAC" },
+};
+
+static struct snd_soc_codec_driver soc_codec_dev_tas5720 = {
+	.probe = tas5720_codec_probe,
+	.remove = tas5720_codec_remove,
+	.suspend = tas5720_suspend,
+	.resume = tas5720_resume,
+
+	.controls = tas5720_snd_controls,
+	.num_controls = ARRAY_SIZE(tas5720_snd_controls),
+	.dapm_widgets = tas5720_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets),
+	.dapm_routes = tas5720_audio_map,
+	.num_dapm_routes = ARRAY_SIZE(tas5720_audio_map),
+};
+
+/* PCM rates supported by the TAS5720 driver */
+#define TAS5720_RATES	(SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\
+			 SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
+
+/* Formats supported by TAS5720 driver */
+#define TAS5720_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S18_3LE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S24_LE)
+
+static struct snd_soc_dai_ops tas5720_speaker_dai_ops = {
+	.hw_params	= tas5720_hw_params,
+	.set_fmt	= tas5720_set_dai_fmt,
+	.set_tdm_slot	= tas5720_set_dai_tdm_slot,
+	.digital_mute	= tas5720_mute,
+};
+
+/*
+ * TAS5720 DAI structure
+ *
+ * Note that were are advertising .playback.channels_max = 2 despite this being
+ * a mono amplifier. The reason for that is that some serial ports such as TI's
+ * McASP module have a minimum number of channels (2) that they can output.
+ * Advertising more channels than we have will allow us to interface with such
+ * a serial port without really any negative side effects as the TAS5720 will
+ * simply ignore any extra channel(s) asides from the one channel that is
+ * configured to be played back.
+ */
+static struct snd_soc_dai_driver tas5720_dai[] = {
+	{
+		.name = "tas5720-amplifier",
+		.playback = {
+			.stream_name = "Playback",
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = TAS5720_RATES,
+			.formats = TAS5720_FORMATS,
+		},
+		.ops = &tas5720_speaker_dai_ops,
+	},
+};
+
+static int tas5720_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct tas5720_data *data;
+	int ret;
+	int i;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->tas5720_client = client;
+	data->regmap = devm_regmap_init_i2c(client, &tas5720_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		ret = PTR_ERR(data->regmap);
+		dev_err(dev, "failed to allocate register map: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(data->supplies); i++)
+		data->supplies[i].supply = tas5720_supply_names[i];
+
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
+				      data->supplies);
+	if (ret != 0) {
+		dev_err(dev, "failed to request supplies: %d\n", ret);
+		return ret;
+	}
+
+	dev_set_drvdata(dev, data);
+
+	ret = snd_soc_register_codec(&client->dev,
+				     &soc_codec_dev_tas5720,
+				     tas5720_dai, ARRAY_SIZE(tas5720_dai));
+	if (ret < 0) {
+		dev_err(dev, "failed to register codec: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tas5720_remove(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+
+	snd_soc_unregister_codec(dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id tas5720_id[] = {
+	{ "tas5720", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tas5720_id);
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id tas5720_of_match[] = {
+	{ .compatible = "ti,tas5720", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tas5720_of_match);
+#endif
+
+static struct i2c_driver tas5720_i2c_driver = {
+	.driver = {
+		.name = "tas5720",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(tas5720_of_match),
+	},
+	.probe = tas5720_probe,
+	.remove = tas5720_remove,
+	.id_table = tas5720_id,
+};
+
+module_i2c_driver(tas5720_i2c_driver);
+
+MODULE_AUTHOR("Andreas Dannenberg <dannenberg@ti.com>");
+MODULE_DESCRIPTION("TAS5720 Audio amplifier driver");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/tas5720.h b/sound/soc/codecs/tas5720.h
new file mode 100644
index 0000000..3d077c7
--- /dev/null
+++ b/sound/soc/codecs/tas5720.h
@@ -0,0 +1,90 @@
+/*
+ * tas5720.h - ALSA SoC Texas Instruments TAS5720 Mono Audio Amplifier
+ *
+ * Copyright (C)2015-2016 Texas Instruments Incorporated -  http://www.ti.com
+ *
+ * Author: Andreas Dannenberg <dannenberg@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef __TAS5720_H__
+#define __TAS5720_H__
+
+/* Register Address Map */
+#define TAS5720_DEVICE_ID_REG		0x00
+#define TAS5720_POWER_CTRL_REG		0x01
+#define TAS5720_DIGITAL_CTRL1_REG	0x02
+#define TAS5720_DIGITAL_CTRL2_REG	0x03
+#define TAS5720_VOLUME_CTRL_REG		0x04
+#define TAS5720_ANALOG_CTRL_REG		0x06
+#define TAS5720_FAULT_REG		0x08
+#define TAS5720_DIGITAL_CLIP2_REG	0x10
+#define TAS5720_DIGITAL_CLIP1_REG	0x11
+#define TAS5720_MAX_REG			TAS5720_DIGITAL_CLIP1_REG
+
+/* TAS5720_DEVICE_ID_REG */
+#define TAS5720_DEVICE_ID		0x01
+
+/* TAS5720_POWER_CTRL_REG */
+#define TAS5720_DIG_CLIP_MASK		GENMASK(7, 2)
+#define TAS5720_SLEEP			BIT(1)
+#define TAS5720_SDZ			BIT(0)
+
+/* TAS5720_DIGITAL_CTRL1_REG */
+#define TAS5720_HPF_BYPASS		BIT(7)
+#define TAS5720_TDM_CFG_SRC		BIT(6)
+#define TAS5720_SSZ_DS			BIT(3)
+#define TAS5720_SAIF_RIGHTJ_24BIT	(0x0)
+#define TAS5720_SAIF_RIGHTJ_20BIT	(0x1)
+#define TAS5720_SAIF_RIGHTJ_18BIT	(0x2)
+#define TAS5720_SAIF_RIGHTJ_16BIT	(0x3)
+#define TAS5720_SAIF_I2S		(0x4)
+#define TAS5720_SAIF_LEFTJ		(0x5)
+#define TAS5720_SAIF_FORMAT_MASK	GENMASK(2, 0)
+
+/* TAS5720_DIGITAL_CTRL2_REG */
+#define TAS5720_MUTE			BIT(4)
+#define TAS5720_TDM_SLOT_SEL_MASK	GENMASK(2, 0)
+
+/* TAS5720_ANALOG_CTRL_REG */
+#define TAS5720_PWM_RATE_6_3_FSYNC	(0x0 << 4)
+#define TAS5720_PWM_RATE_8_4_FSYNC	(0x1 << 4)
+#define TAS5720_PWM_RATE_10_5_FSYNC	(0x2 << 4)
+#define TAS5720_PWM_RATE_12_6_FSYNC	(0x3 << 4)
+#define TAS5720_PWM_RATE_14_7_FSYNC	(0x4 << 4)
+#define TAS5720_PWM_RATE_16_8_FSYNC	(0x5 << 4)
+#define TAS5720_PWM_RATE_20_10_FSYNC	(0x6 << 4)
+#define TAS5720_PWM_RATE_24_12_FSYNC	(0x7 << 4)
+#define TAS5720_PWM_RATE_MASK		GENMASK(6, 4)
+#define TAS5720_ANALOG_GAIN_19_2DBV	(0x0 << 2)
+#define TAS5720_ANALOG_GAIN_20_7DBV	(0x1 << 2)
+#define TAS5720_ANALOG_GAIN_23_5DBV	(0x2 << 2)
+#define TAS5720_ANALOG_GAIN_26_3DBV	(0x3 << 2)
+#define TAS5720_ANALOG_GAIN_MASK	GENMASK(3, 2)
+#define TAS5720_ANALOG_GAIN_SHIFT	(0x2)
+
+/* TAS5720_FAULT_REG */
+#define TAS5720_OC_THRESH_100PCT	(0x0 << 4)
+#define TAS5720_OC_THRESH_75PCT		(0x1 << 4)
+#define TAS5720_OC_THRESH_50PCT		(0x2 << 4)
+#define TAS5720_OC_THRESH_25PCT		(0x3 << 4)
+#define TAS5720_OC_THRESH_MASK		GENMASK(5, 4)
+#define TAS5720_CLKE			BIT(3)
+#define TAS5720_OCE			BIT(2)
+#define TAS5720_DCE			BIT(1)
+#define TAS5720_OTE			BIT(0)
+#define TAS5720_FAULT_MASK		GENMASK(3, 0)
+
+/* TAS5720_DIGITAL_CLIP1_REG */
+#define TAS5720_CLIP1_MASK		GENMASK(7, 2)
+#define TAS5720_CLIP1_SHIFT		(0x2)
+
+#endif /* __TAS5720_H__ */
-- 
2.6.4

Re: [PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Mark Brown]

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

On Mon, Apr 25, 2016 at 03:17:35PM -0500, Andreas Dannenberg wrote:

> +Optional properties:
> +
> +- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
> +- pvdd-supply : phandle to a supply used for the Class-D amp and the analog

This suggests that the device doesn't need power...  

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

Re: [PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Mark Brown]

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

On Mon, Apr 25, 2016 at 03:17:35PM -0500, Andreas Dannenberg wrote:

> +Optional properties:
> +
> +- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
> +- pvdd-supply : phandle to a supply used for the Class-D amp and the analog

This suggests that the device doesn't need power...  

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

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Mark Brown]

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

On Mon, Apr 25, 2016 at 03:17:36PM -0500, Andreas Dannenberg wrote:

This looks mostly good, a few small things below.

> +	switch (width) {
> +	case 16:
> +	case 18:
> +	case 20:
> +	case 24:
> +		/*
> +		 * We only support the different left-justified serial audio
> +		 * formats in which case there is nothing to configure in the
> +		 * TAS5720.
> +		 */
> +		break;
> +	default:
> +		dev_err(codec->dev, "unsupported sample size: %d\n", width);
> +		return -EINVAL;
> +	}

If the driver doesn't do anything just remove the code.

> +	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
> +		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");

"Class D over current".  The verbosity is making the line over long and
the phrasing is a bit unclear (and makes it seem less critical than it
really is).  These should probably be dev_crit() or somthing too, over
current and similar events on a speaker output are generally extremely
serious.

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

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Mark Brown]

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

On Mon, Apr 25, 2016 at 03:17:36PM -0500, Andreas Dannenberg wrote:

This looks mostly good, a few small things below.

> +	switch (width) {
> +	case 16:
> +	case 18:
> +	case 20:
> +	case 24:
> +		/*
> +		 * We only support the different left-justified serial audio
> +		 * formats in which case there is nothing to configure in the
> +		 * TAS5720.
> +		 */
> +		break;
> +	default:
> +		dev_err(codec->dev, "unsupported sample size: %d\n", width);
> +		return -EINVAL;
> +	}

If the driver doesn't do anything just remove the code.

> +	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
> +		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");

"Class D over current".  The verbosity is making the line over long and
the phrasing is a bit unclear (and makes it seem less critical than it
really is).  These should probably be dev_crit() or somthing too, over
current and similar events on a speaker output are generally extremely
serious.

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

Re: [PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Andreas Dannenberg]

On Tue, Apr 26, 2016 at 04:37:07PM +0100, Mark Brown wrote:
> On Mon, Apr 25, 2016 at 03:17:35PM -0500, Andreas Dannenberg wrote:
> 
> > +Optional properties:
> > +
> > +- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
> > +- pvdd-supply : phandle to a supply used for the Class-D amp and the analog
> 
> This suggests that the device doesn't need power...  

Hi Mark,
no power, now that would be nice! :)  What this was supposed to mean is
that the properties are optional, the power of course is not. The DT has
really no control over how I wire up my HW and I'd argue most folks just
permanently power the TAS5720 since the shutdown current is just a few
uAs IIRC in order to save some BOM cost. But I suppose the DT description
is such that the Kernel can more intelligently handle things based on
what the driver is doing if somebody choses and implements additional
regulator/power switch HW. Or did I misunderstand your point?

Thanks,
Andreas

Re: [PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Andreas Dannenberg]

On Tue, Apr 26, 2016 at 04:37:07PM +0100, Mark Brown wrote:
> On Mon, Apr 25, 2016 at 03:17:35PM -0500, Andreas Dannenberg wrote:
> 
> > +Optional properties:
> > +
> > +- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
> > +- pvdd-supply : phandle to a supply used for the Class-D amp and the analog
> 
> This suggests that the device doesn't need power...  

Hi Mark,
no power, now that would be nice! :)  What this was supposed to mean is
that the properties are optional, the power of course is not. The DT has
really no control over how I wire up my HW and I'd argue most folks just
permanently power the TAS5720 since the shutdown current is just a few
uAs IIRC in order to save some BOM cost. But I suppose the DT description
is such that the Kernel can more intelligently handle things based on
what the driver is doing if somebody choses and implements additional
regulator/power switch HW. Or did I misunderstand your point?

Thanks,
Andreas

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

Hi Mark, thanks for the continued feedback, please see below...

On Tue, Apr 26, 2016 at 04:43:13PM +0100, Mark Brown wrote:
> On Mon, Apr 25, 2016 at 03:17:36PM -0500, Andreas Dannenberg wrote:
> 
> This looks mostly good, a few small things below.
> 
> > +	switch (width) {
> > +	case 16:
> > +	case 18:
> > +	case 20:
> > +	case 24:
> > +		/*
> > +		 * We only support the different left-justified serial audio
> > +		 * formats in which case there is nothing to configure in the
> > +		 * TAS5720.
> > +		 */
> > +		break;
> > +	default:
> > +		dev_err(codec->dev, "unsupported sample size: %d\n", width);
> > +		return -EINVAL;
> > +	}
> 
> If the driver doesn't do anything just remove the code.

Well it's doing something which is making sure the nobody passes in a
sample size that's not supported. Wouldn't we want to catch this?

> 
> > +	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
> > +		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");
> 
> "Class D over current".  The verbosity is making the line over long and
> the phrasing is a bit unclear (and makes it seem less critical than it
> really is).  These should probably be dev_crit() or somthing too, over
> current and similar events on a speaker output are generally extremely
> serious.

The overlong line goes through checkpatch --strict and looks like an
accepted practice to prevent breaking "grep" for example. The text is
more or less from the datasheet to give people something they can
cross-associate. But I can try to short this a bit.

You made a good point with the dev_crit(), I can certainly promote this
over current condition.  It happens for example if I drive too big of a
load (a heavy-duty jumper wire in my case, simulating a short of the
output stage).  If so, we should also consider the other two fault
conditions.  For the "over temp" error condition (which is actually
really hard to create on the bench, I've to get the EVM up to like 150C
and things start smelling a bit) this should probably be dev_crit() as
well. And then, maybe leave the "DC error" as a warning, since it's less
critical than the other two conditions. Thoughts?

Regards,
Andreas

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

Hi Mark, thanks for the continued feedback, please see below...

On Tue, Apr 26, 2016 at 04:43:13PM +0100, Mark Brown wrote:
> On Mon, Apr 25, 2016 at 03:17:36PM -0500, Andreas Dannenberg wrote:
> 
> This looks mostly good, a few small things below.
> 
> > +	switch (width) {
> > +	case 16:
> > +	case 18:
> > +	case 20:
> > +	case 24:
> > +		/*
> > +		 * We only support the different left-justified serial audio
> > +		 * formats in which case there is nothing to configure in the
> > +		 * TAS5720.
> > +		 */
> > +		break;
> > +	default:
> > +		dev_err(codec->dev, "unsupported sample size: %d\n", width);
> > +		return -EINVAL;
> > +	}
> 
> If the driver doesn't do anything just remove the code.

Well it's doing something which is making sure the nobody passes in a
sample size that's not supported. Wouldn't we want to catch this?

> 
> > +	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
> > +		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");
> 
> "Class D over current".  The verbosity is making the line over long and
> the phrasing is a bit unclear (and makes it seem less critical than it
> really is).  These should probably be dev_crit() or somthing too, over
> current and similar events on a speaker output are generally extremely
> serious.

The overlong line goes through checkpatch --strict and looks like an
accepted practice to prevent breaking "grep" for example. The text is
more or less from the datasheet to give people something they can
cross-associate. But I can try to short this a bit.

You made a good point with the dev_crit(), I can certainly promote this
over current condition.  It happens for example if I drive too big of a
load (a heavy-duty jumper wire in my case, simulating a short of the
output stage).  If so, we should also consider the other two fault
conditions.  For the "over temp" error condition (which is actually
really hard to create on the bench, I've to get the EVM up to like 150C
and things start smelling a bit) this should probably be dev_crit() as
well. And then, maybe leave the "DC error" as a warning, since it's less
critical than the other two conditions. Thoughts?

Regards,
Andreas

Re: [PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Mark Brown]

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

On Tue, Apr 26, 2016 at 11:14:25AM -0500, Andreas Dannenberg wrote:
> On Tue, Apr 26, 2016 at 04:37:07PM +0100, Mark Brown wrote:
> > On Mon, Apr 25, 2016 at 03:17:35PM -0500, Andreas Dannenberg wrote:

> > > +Optional properties:

> > > +- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
> > > +- pvdd-supply : phandle to a supply used for the Class-D amp and the analog

> > This suggests that the device doesn't need power...  

> no power, now that would be nice! :)  What this was supposed to mean is
> that the properties are optional, the power of course is not. The DT has

They should only be optional if they may really be missing.  We may
attempt to be liberal in the DTs we accept but we should not encourage
sloppily written DTs.

> really no control over how I wire up my HW and I'd argue most folks just
> permanently power the TAS5720 since the shutdown current is just a few
> uAs IIRC in order to save some BOM cost. But I suppose the DT description
> is such that the Kernel can more intelligently handle things based on
> what the driver is doing if somebody choses and implements additional
> regulator/power switch HW. Or did I misunderstand your point?

It doesn't really matter if a given system ends up controlling things
actively, we still want to be consistent in describing them since it is
less error prone when it does become relevant.

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

Re: [PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1412 bytes --]

On Tue, Apr 26, 2016 at 11:14:25AM -0500, Andreas Dannenberg wrote:
> On Tue, Apr 26, 2016 at 04:37:07PM +0100, Mark Brown wrote:
> > On Mon, Apr 25, 2016 at 03:17:35PM -0500, Andreas Dannenberg wrote:

> > > +Optional properties:

> > > +- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
> > > +- pvdd-supply : phandle to a supply used for the Class-D amp and the analog

> > This suggests that the device doesn't need power...  

> no power, now that would be nice! :)  What this was supposed to mean is
> that the properties are optional, the power of course is not. The DT has

They should only be optional if they may really be missing.  We may
attempt to be liberal in the DTs we accept but we should not encourage
sloppily written DTs.

> really no control over how I wire up my HW and I'd argue most folks just
> permanently power the TAS5720 since the shutdown current is just a few
> uAs IIRC in order to save some BOM cost. But I suppose the DT description
> is such that the Kernel can more intelligently handle things based on
> what the driver is doing if somebody choses and implements additional
> regulator/power switch HW. Or did I misunderstand your point?

It doesn't really matter if a given system ends up controlling things
actively, we still want to be consistent in describing them since it is
less error prone when it does become relevant.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Andreas Dannenberg]

On Tue, Apr 26, 2016 at 05:50:06PM +0100, Mark Brown wrote:
> On Tue, Apr 26, 2016 at 11:14:25AM -0500, Andreas Dannenberg wrote:
> > On Tue, Apr 26, 2016 at 04:37:07PM +0100, Mark Brown wrote:
> > > On Mon, Apr 25, 2016 at 03:17:35PM -0500, Andreas Dannenberg wrote:
> 
> > > > +Optional properties:
> 
> > > > +- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
> > > > +- pvdd-supply : phandle to a supply used for the Class-D amp and the analog
> 
> > > This suggests that the device doesn't need power...  
> 
> > no power, now that would be nice! :)  What this was supposed to mean is
> > that the properties are optional, the power of course is not. The DT has
> 
> They should only be optional if they may really be missing.  We may
> attempt to be liberal in the DTs we accept but we should not encourage
> sloppily written DTs.

Thanks for the guidance and the background. Will move them to "required"
but I'll wait a few more days before re-submitting in case there is
additional feedback on the patch series.

Regards,
Andreas

> 
> > really no control over how I wire up my HW and I'd argue most folks just
> > permanently power the TAS5720 since the shutdown current is just a few
> > uAs IIRC in order to save some BOM cost. But I suppose the DT description
> > is such that the Kernel can more intelligently handle things based on
> > what the driver is doing if somebody choses and implements additional
> > regulator/power switch HW. Or did I misunderstand your point?
> 
> It doesn't really matter if a given system ends up controlling things
> actively, we still want to be consistent in describing them since it is
> less error prone when it does become relevant.

Re: [PATCH v3 1/2] ASoC: codecs: add TA5720 digital amplifier DT bindings

[Andreas Dannenberg]

On Tue, Apr 26, 2016 at 05:50:06PM +0100, Mark Brown wrote:
> On Tue, Apr 26, 2016 at 11:14:25AM -0500, Andreas Dannenberg wrote:
> > On Tue, Apr 26, 2016 at 04:37:07PM +0100, Mark Brown wrote:
> > > On Mon, Apr 25, 2016 at 03:17:35PM -0500, Andreas Dannenberg wrote:
> 
> > > > +Optional properties:
> 
> > > > +- dvdd-supply : phandle to a 3.3-V supply for the digital circuitry
> > > > +- pvdd-supply : phandle to a supply used for the Class-D amp and the analog
> 
> > > This suggests that the device doesn't need power...  
> 
> > no power, now that would be nice! :)  What this was supposed to mean is
> > that the properties are optional, the power of course is not. The DT has
> 
> They should only be optional if they may really be missing.  We may
> attempt to be liberal in the DTs we accept but we should not encourage
> sloppily written DTs.

Thanks for the guidance and the background. Will move them to "required"
but I'll wait a few more days before re-submitting in case there is
additional feedback on the patch series.

Regards,
Andreas

> 
> > really no control over how I wire up my HW and I'd argue most folks just
> > permanently power the TAS5720 since the shutdown current is just a few
> > uAs IIRC in order to save some BOM cost. But I suppose the DT description
> > is such that the Kernel can more intelligently handle things based on
> > what the driver is doing if somebody choses and implements additional
> > regulator/power switch HW. Or did I misunderstand your point?
> 
> It doesn't really matter if a given system ends up controlling things
> actively, we still want to be consistent in describing them since it is
> less error prone when it does become relevant.

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andrew F. Davis]

On 04/25/2016 03:17 PM, Andreas Dannenberg wrote:
> The Texas Instruments TAS5720L/M device is a high-efficiency mono
> Class-D audio power amplifier optimized for high transient power
> capability to use the dynamic power headroom of small loudspeakers.
> Its digital time division multiplexed (TDM) interface enables up to
> 16 devices to share the same bus.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
>  sound/soc/codecs/Kconfig   |  10 +
>  sound/soc/codecs/Makefile  |   2 +
>  sound/soc/codecs/tas5720.c | 638 +++++++++++++++++++++++++++++++++++++++++++++
>  sound/soc/codecs/tas5720.h |  90 +++++++
>  4 files changed, 740 insertions(+)
>  create mode 100644 sound/soc/codecs/tas5720.c
>  create mode 100644 sound/soc/codecs/tas5720.h
> 
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 290f921..3949dae 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -125,6 +125,7 @@ config SND_SOC_ALL_CODECS
>  	select SND_SOC_TAS2552 if I2C
>  	select SND_SOC_TAS5086 if I2C
>  	select SND_SOC_TAS571X if I2C
> +	select SND_SOC_TAS5720 if I2C
>  	select SND_SOC_TFA9879 if I2C
>  	select SND_SOC_TLV320AIC23_I2C if I2C
>  	select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
> @@ -748,6 +749,15 @@ config SND_SOC_TAS571X
>  	tristate "Texas Instruments TAS5711/TAS5717/TAS5719/TAS5721 power amplifiers"
>  	depends on I2C
>  
> +config SND_SOC_TAS5720
> +	tristate "Texas Instruments TAS5720 Mono Audio amplifier"
> +	depends on I2C
> +	help
> +	  Enable support for Texas Instruments TAS5720L/M high-efficiency mono
> +	  Class-D audio power amplifiers. The devices use an I2C interface for
> +	  setup/control and support an optional GPIO interrupt signal for fault
> +	  reporting.

If fault reporting is no longer interrupt based could this be re-worded?

[...]

> +
> +static struct i2c_driver tas5720_i2c_driver = {
> +	.driver = {
> +		.name = "tas5720",
> +		.owner = THIS_MODULE,

This shouldn't be needed.

Andrew

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andrew F. Davis]

On 04/25/2016 03:17 PM, Andreas Dannenberg wrote:
> The Texas Instruments TAS5720L/M device is a high-efficiency mono
> Class-D audio power amplifier optimized for high transient power
> capability to use the dynamic power headroom of small loudspeakers.
> Its digital time division multiplexed (TDM) interface enables up to
> 16 devices to share the same bus.
> 
> Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
>  sound/soc/codecs/Kconfig   |  10 +
>  sound/soc/codecs/Makefile  |   2 +
>  sound/soc/codecs/tas5720.c | 638 +++++++++++++++++++++++++++++++++++++++++++++
>  sound/soc/codecs/tas5720.h |  90 +++++++
>  4 files changed, 740 insertions(+)
>  create mode 100644 sound/soc/codecs/tas5720.c
>  create mode 100644 sound/soc/codecs/tas5720.h
> 
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 290f921..3949dae 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -125,6 +125,7 @@ config SND_SOC_ALL_CODECS
>  	select SND_SOC_TAS2552 if I2C
>  	select SND_SOC_TAS5086 if I2C
>  	select SND_SOC_TAS571X if I2C
> +	select SND_SOC_TAS5720 if I2C
>  	select SND_SOC_TFA9879 if I2C
>  	select SND_SOC_TLV320AIC23_I2C if I2C
>  	select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
> @@ -748,6 +749,15 @@ config SND_SOC_TAS571X
>  	tristate "Texas Instruments TAS5711/TAS5717/TAS5719/TAS5721 power amplifiers"
>  	depends on I2C
>  
> +config SND_SOC_TAS5720
> +	tristate "Texas Instruments TAS5720 Mono Audio amplifier"
> +	depends on I2C
> +	help
> +	  Enable support for Texas Instruments TAS5720L/M high-efficiency mono
> +	  Class-D audio power amplifiers. The devices use an I2C interface for
> +	  setup/control and support an optional GPIO interrupt signal for fault
> +	  reporting.

If fault reporting is no longer interrupt based could this be re-worded?

[...]

> +
> +static struct i2c_driver tas5720_i2c_driver = {
> +	.driver = {
> +		.name = "tas5720",
> +		.owner = THIS_MODULE,

This shouldn't be needed.

Andrew

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Mark Brown]

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

On Tue, Apr 26, 2016 at 11:22:40AM -0500, Andreas Dannenberg wrote:
> On Tue, Apr 26, 2016 at 04:43:13PM +0100, Mark Brown wrote:

> > If the driver doesn't do anything just remove the code.

> Well it's doing something which is making sure the nobody passes in a
> sample size that's not supported. Wouldn't we want to catch this?

Is the device actually going to mess up if someone sends it something
else or is it just going to ignore the extra bits (given that it's doing
autodetection anyway).

> > > +	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
> > > +		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");

> > "Class D over current".  The verbosity is making the line over long and
> > the phrasing is a bit unclear (and makes it seem less critical than it
> > really is).  These should probably be dev_crit() or somthing too, over
> > current and similar events on a speaker output are generally extremely
> > serious.

> The overlong line goes through checkpatch --strict and looks like an
> accepted practice to prevent breaking "grep" for example. The text is
> more or less from the datasheet to give people something they can
> cross-associate. But I can try to short this a bit.

It's not just the fact that it's wrapping round to the next line, it's
also the fact that it's very weakly phrased for something which might
reasonably indicate an actual fire risk.  I'm pretty sure people will
not struggle excessively to find the reference to over current in the
datasheet.

> conditions.  For the "over temp" error condition (which is actually
> really hard to create on the bench, I've to get the EVM up to like 150C
> and things start smelling a bit) this should probably be dev_crit() as

If the silicon is flagging an over temperature condition that tends to
indicate a catastrophic physical failure in the system, it is likely
that the speaker itself has failed or there's otherwise a short in the
speaker output path and potentially other physical damage especially in
smaller systems where you might find that for example there's thermal
damage to the case (and possibly even the user).

> well. And then, maybe leave the "DC error" as a warning, since it's less
> critical than the other two conditions. Thoughts?

If you're pushing DC through a speaker that will generally mean that you
will shortly see one or both of the over current and over temperature
errors, it's really not something they're designed for.

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

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2495 bytes --]

On Tue, Apr 26, 2016 at 11:22:40AM -0500, Andreas Dannenberg wrote:
> On Tue, Apr 26, 2016 at 04:43:13PM +0100, Mark Brown wrote:

> > If the driver doesn't do anything just remove the code.

> Well it's doing something which is making sure the nobody passes in a
> sample size that's not supported. Wouldn't we want to catch this?

Is the device actually going to mess up if someone sends it something
else or is it just going to ignore the extra bits (given that it's doing
autodetection anyway).

> > > +	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
> > > +		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");

> > "Class D over current".  The verbosity is making the line over long and
> > the phrasing is a bit unclear (and makes it seem less critical than it
> > really is).  These should probably be dev_crit() or somthing too, over
> > current and similar events on a speaker output are generally extremely
> > serious.

> The overlong line goes through checkpatch --strict and looks like an
> accepted practice to prevent breaking "grep" for example. The text is
> more or less from the datasheet to give people something they can
> cross-associate. But I can try to short this a bit.

It's not just the fact that it's wrapping round to the next line, it's
also the fact that it's very weakly phrased for something which might
reasonably indicate an actual fire risk.  I'm pretty sure people will
not struggle excessively to find the reference to over current in the
datasheet.

> conditions.  For the "over temp" error condition (which is actually
> really hard to create on the bench, I've to get the EVM up to like 150C
> and things start smelling a bit) this should probably be dev_crit() as

If the silicon is flagging an over temperature condition that tends to
indicate a catastrophic physical failure in the system, it is likely
that the speaker itself has failed or there's otherwise a short in the
speaker output path and potentially other physical damage especially in
smaller systems where you might find that for example there's thermal
damage to the case (and possibly even the user).

> well. And then, maybe leave the "DC error" as a warning, since it's less
> critical than the other two conditions. Thoughts?

If you're pushing DC through a speaker that will generally mean that you
will shortly see one or both of the over current and over temperature
errors, it's really not something they're designed for.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

On Tue, Apr 26, 2016 at 12:19:05PM -0500, Andrew F. Davis wrote:
> On 04/25/2016 03:17 PM, Andreas Dannenberg wrote:
> > The Texas Instruments TAS5720L/M device is a high-efficiency mono
> > Class-D audio power amplifier optimized for high transient power
> > capability to use the dynamic power headroom of small loudspeakers.
> > Its digital time division multiplexed (TDM) interface enables up to
> > 16 devices to share the same bus.
> > 
> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > ---
> >  sound/soc/codecs/Kconfig   |  10 +
> >  sound/soc/codecs/Makefile  |   2 +
> >  sound/soc/codecs/tas5720.c | 638 +++++++++++++++++++++++++++++++++++++++++++++
> >  sound/soc/codecs/tas5720.h |  90 +++++++
> >  4 files changed, 740 insertions(+)
> >  create mode 100644 sound/soc/codecs/tas5720.c
> >  create mode 100644 sound/soc/codecs/tas5720.h
> > 
> > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> > index 290f921..3949dae 100644
> > --- a/sound/soc/codecs/Kconfig
> > +++ b/sound/soc/codecs/Kconfig
> > @@ -125,6 +125,7 @@ config SND_SOC_ALL_CODECS
> >  	select SND_SOC_TAS2552 if I2C
> >  	select SND_SOC_TAS5086 if I2C
> >  	select SND_SOC_TAS571X if I2C
> > +	select SND_SOC_TAS5720 if I2C
> >  	select SND_SOC_TFA9879 if I2C
> >  	select SND_SOC_TLV320AIC23_I2C if I2C
> >  	select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
> > @@ -748,6 +749,15 @@ config SND_SOC_TAS571X
> >  	tristate "Texas Instruments TAS5711/TAS5717/TAS5719/TAS5721 power amplifiers"
> >  	depends on I2C
> >  
> > +config SND_SOC_TAS5720
> > +	tristate "Texas Instruments TAS5720 Mono Audio amplifier"
> > +	depends on I2C
> > +	help
> > +	  Enable support for Texas Instruments TAS5720L/M high-efficiency mono
> > +	  Class-D audio power amplifiers. The devices use an I2C interface for
> > +	  setup/control and support an optional GPIO interrupt signal for fault
> > +	  reporting.
> 
> If fault reporting is no longer interrupt based could this be re-worded?

Good catch! Thanks Andrew.

> 
> [...]
> 
> > +
> > +static struct i2c_driver tas5720_i2c_driver = {
> > +	.driver = {
> > +		.name = "tas5720",
> > +		.owner = THIS_MODULE,
> 
> This shouldn't be needed.

Yes. I recall your recent mass-optimizations. Will remove.

Regards,
Andreas

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

On Tue, Apr 26, 2016 at 12:19:05PM -0500, Andrew F. Davis wrote:
> On 04/25/2016 03:17 PM, Andreas Dannenberg wrote:
> > The Texas Instruments TAS5720L/M device is a high-efficiency mono
> > Class-D audio power amplifier optimized for high transient power
> > capability to use the dynamic power headroom of small loudspeakers.
> > Its digital time division multiplexed (TDM) interface enables up to
> > 16 devices to share the same bus.
> > 
> > Signed-off-by: Andreas Dannenberg <dannenberg@ti.com>
> > ---
> >  sound/soc/codecs/Kconfig   |  10 +
> >  sound/soc/codecs/Makefile  |   2 +
> >  sound/soc/codecs/tas5720.c | 638 +++++++++++++++++++++++++++++++++++++++++++++
> >  sound/soc/codecs/tas5720.h |  90 +++++++
> >  4 files changed, 740 insertions(+)
> >  create mode 100644 sound/soc/codecs/tas5720.c
> >  create mode 100644 sound/soc/codecs/tas5720.h
> > 
> > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> > index 290f921..3949dae 100644
> > --- a/sound/soc/codecs/Kconfig
> > +++ b/sound/soc/codecs/Kconfig
> > @@ -125,6 +125,7 @@ config SND_SOC_ALL_CODECS
> >  	select SND_SOC_TAS2552 if I2C
> >  	select SND_SOC_TAS5086 if I2C
> >  	select SND_SOC_TAS571X if I2C
> > +	select SND_SOC_TAS5720 if I2C
> >  	select SND_SOC_TFA9879 if I2C
> >  	select SND_SOC_TLV320AIC23_I2C if I2C
> >  	select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
> > @@ -748,6 +749,15 @@ config SND_SOC_TAS571X
> >  	tristate "Texas Instruments TAS5711/TAS5717/TAS5719/TAS5721 power amplifiers"
> >  	depends on I2C
> >  
> > +config SND_SOC_TAS5720
> > +	tristate "Texas Instruments TAS5720 Mono Audio amplifier"
> > +	depends on I2C
> > +	help
> > +	  Enable support for Texas Instruments TAS5720L/M high-efficiency mono
> > +	  Class-D audio power amplifiers. The devices use an I2C interface for
> > +	  setup/control and support an optional GPIO interrupt signal for fault
> > +	  reporting.
> 
> If fault reporting is no longer interrupt based could this be re-worded?

Good catch! Thanks Andrew.

> 
> [...]
> 
> > +
> > +static struct i2c_driver tas5720_i2c_driver = {
> > +	.driver = {
> > +		.name = "tas5720",
> > +		.owner = THIS_MODULE,
> 
> This shouldn't be needed.

Yes. I recall your recent mass-optimizations. Will remove.

Regards,
Andreas

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

On Tue, Apr 26, 2016 at 06:29:36PM +0100, Mark Brown wrote:
> On Tue, Apr 26, 2016 at 11:22:40AM -0500, Andreas Dannenberg wrote:
> > On Tue, Apr 26, 2016 at 04:43:13PM +0100, Mark Brown wrote:
> 
> > > If the driver doesn't do anything just remove the code.
> 
> > Well it's doing something which is making sure the nobody passes in a
> > sample size that's not supported. Wouldn't we want to catch this?
> 
> Is the device actually going to mess up if someone sends it something
> else or is it just going to ignore the extra bits (given that it's doing
> autodetection anyway).

Hi Mark,
well in any of the left-justified modes (which are the only ones the
driver supports) the device takes and processes as many bits as it can
given the clock and divider settings. Any extra bits provided will get
ignored, and the next sync happens on the frame sync signal and not by
counting bits so there is no downside also as confirmed by some bench
testing I did feeding in 32-bit long frames for one channel. This seems
like a case of preferring tolerance over strictly enforcing
datasheet-advertised bit-widths. Will take out the check code.

> 
> > > > +	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
> > > > +		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");
> 
> > > "Class D over current".  The verbosity is making the line over long and
> > > the phrasing is a bit unclear (and makes it seem less critical than it
> > > really is).  These should probably be dev_crit() or somthing too, over
> > > current and similar events on a speaker output are generally extremely
> > > serious.
> 
> > The overlong line goes through checkpatch --strict and looks like an
> > accepted practice to prevent breaking "grep" for example. The text is
> > more or less from the datasheet to give people something they can
> > cross-associate. But I can try to short this a bit.
> 
> It's not just the fact that it's wrapping round to the next line, it's
> also the fact that it's very weakly phrased for something which might
> reasonably indicate an actual fire risk.  I'm pretty sure people will
> not struggle excessively to find the reference to over current in the
> datasheet.

Ok will tweak it.

> > conditions.  For the "over temp" error condition (which is actually
> > really hard to create on the bench, I've to get the EVM up to like 150C
> > and things start smelling a bit) this should probably be dev_crit() as
> 
> If the silicon is flagging an over temperature condition that tends to
> indicate a catastrophic physical failure in the system, it is likely
> that the speaker itself has failed or there's otherwise a short in the
> speaker output path and potentially other physical damage especially in
> smaller systems where you might find that for example there's thermal
> damage to the case (and possibly even the user).

Yes make sense. As suggested promoting this to dev_crit() and making sure
its adequately phrased will help.

> 
> > well. And then, maybe leave the "DC error" as a warning, since it's less
> > critical than the other two conditions. Thoughts?
> 
> If you're pushing DC through a speaker that will generally mean that you
> will shortly see one or both of the over current and over temperature
> errors, it's really not something they're designed for.

I had to actually craft my own DC "audio" file to play with aplay to
trigger this error during testing. So when this happens I'd think it is
likely something userspace is "doing wrong" (of course there could be
other reasons too such a broken-off SAIF DIN/DOUT trace) but let's make
this also for the sake of consistency a dev_crit() too.

Along these lines, earlier as I was rummaging through the existing
drivers looking for a solution I could model after I noticed that
most(?) ASoC codec drivers don't have any type of HW fault checking, at
at least whatever drivers I looked at. Not sure why this is but given
this discussion this seems like a general opportunity to make
improvements.

Regards,
Andreas

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

On Tue, Apr 26, 2016 at 06:29:36PM +0100, Mark Brown wrote:
> On Tue, Apr 26, 2016 at 11:22:40AM -0500, Andreas Dannenberg wrote:
> > On Tue, Apr 26, 2016 at 04:43:13PM +0100, Mark Brown wrote:
> 
> > > If the driver doesn't do anything just remove the code.
> 
> > Well it's doing something which is making sure the nobody passes in a
> > sample size that's not supported. Wouldn't we want to catch this?
> 
> Is the device actually going to mess up if someone sends it something
> else or is it just going to ignore the extra bits (given that it's doing
> autodetection anyway).

Hi Mark,
well in any of the left-justified modes (which are the only ones the
driver supports) the device takes and processes as many bits as it can
given the clock and divider settings. Any extra bits provided will get
ignored, and the next sync happens on the frame sync signal and not by
counting bits so there is no downside also as confirmed by some bench
testing I did feeding in 32-bit long frames for one channel. This seems
like a case of preferring tolerance over strictly enforcing
datasheet-advertised bit-widths. Will take out the check code.

> 
> > > > +	if ((curr_fault & TAS5720_OCE) && !(tas5720->last_fault & TAS5720_OCE))
> > > > +		dev_warn(dev, "The Class-D output stage has experienced an over current event\n");
> 
> > > "Class D over current".  The verbosity is making the line over long and
> > > the phrasing is a bit unclear (and makes it seem less critical than it
> > > really is).  These should probably be dev_crit() or somthing too, over
> > > current and similar events on a speaker output are generally extremely
> > > serious.
> 
> > The overlong line goes through checkpatch --strict and looks like an
> > accepted practice to prevent breaking "grep" for example. The text is
> > more or less from the datasheet to give people something they can
> > cross-associate. But I can try to short this a bit.
> 
> It's not just the fact that it's wrapping round to the next line, it's
> also the fact that it's very weakly phrased for something which might
> reasonably indicate an actual fire risk.  I'm pretty sure people will
> not struggle excessively to find the reference to over current in the
> datasheet.

Ok will tweak it.

> > conditions.  For the "over temp" error condition (which is actually
> > really hard to create on the bench, I've to get the EVM up to like 150C
> > and things start smelling a bit) this should probably be dev_crit() as
> 
> If the silicon is flagging an over temperature condition that tends to
> indicate a catastrophic physical failure in the system, it is likely
> that the speaker itself has failed or there's otherwise a short in the
> speaker output path and potentially other physical damage especially in
> smaller systems where you might find that for example there's thermal
> damage to the case (and possibly even the user).

Yes make sense. As suggested promoting this to dev_crit() and making sure
its adequately phrased will help.

> 
> > well. And then, maybe leave the "DC error" as a warning, since it's less
> > critical than the other two conditions. Thoughts?
> 
> If you're pushing DC through a speaker that will generally mean that you
> will shortly see one or both of the over current and over temperature
> errors, it's really not something they're designed for.

I had to actually craft my own DC "audio" file to play with aplay to
trigger this error during testing. So when this happens I'd think it is
likely something userspace is "doing wrong" (of course there could be
other reasons too such a broken-off SAIF DIN/DOUT trace) but let's make
this also for the sake of consistency a dev_crit() too.

Along these lines, earlier as I was rummaging through the existing
drivers looking for a solution I could model after I noticed that
most(?) ASoC codec drivers don't have any type of HW fault checking, at
at least whatever drivers I looked at. Not sure why this is but given
this discussion this seems like a general opportunity to make
improvements.

Regards,
Andreas

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Mark Brown]

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

On Tue, Apr 26, 2016 at 01:01:05PM -0500, Andreas Dannenberg wrote:
> On Tue, Apr 26, 2016 at 06:29:36PM +0100, Mark Brown wrote:

> > Is the device actually going to mess up if someone sends it something
> > else or is it just going to ignore the extra bits (given that it's doing
> > autodetection anyway).

> well in any of the left-justified modes (which are the only ones the
> driver supports) the device takes and processes as many bits as it can
> given the clock and divider settings. Any extra bits provided will get
> ignored, and the next sync happens on the frame sync signal and not by
> counting bits so there is no downside also as confirmed by some bench
> testing I did feeding in 32-bit long frames for one channel. This seems
> like a case of preferring tolerance over strictly enforcing
> datasheet-advertised bit-widths. Will take out the check code.

OK, accepting extra bits is fine.  You should set sig_bits in the DAI so
userspace can see what's going on if it cares.

> Along these lines, earlier as I was rummaging through the existing
> drivers looking for a solution I could model after I noticed that
> most(?) ASoC codec drivers don't have any type of HW fault checking, at
> at least whatever drivers I looked at. Not sure why this is but given
> this discussion this seems like a general opportunity to make
> improvements.

There are some with over temperature handling (eg, wm8962) but it's
relatively uncommon for observable protection features to be implemented
in silicon and even rarer for the interrupts to be hooked up (and hence
useful to support in software) unless there is also accessory detection
in the device.  On older devices the required digital logic was often
excessively expensive and realistically only relatively high power
speaker drivers have much risk of something going wrong - things like
headphone outputs or smaller speaker drivers end up with protection from
their supplies collapsing well before the device is in physical danger.

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

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

Mark, please see below...

On Fri, May 13, 2016 at 12:55:40PM +0100, Mark Brown wrote:
> On Tue, Apr 26, 2016 at 01:01:05PM -0500, Andreas Dannenberg wrote:
> > On Tue, Apr 26, 2016 at 06:29:36PM +0100, Mark Brown wrote:
> 
> > > Is the device actually going to mess up if someone sends it something
> > > else or is it just going to ignore the extra bits (given that it's doing
> > > autodetection anyway).
> 
> > well in any of the left-justified modes (which are the only ones the
> > driver supports) the device takes and processes as many bits as it can
> > given the clock and divider settings. Any extra bits provided will get
> > ignored, and the next sync happens on the frame sync signal and not by
> > counting bits so there is no downside also as confirmed by some bench
> > testing I did feeding in 32-bit long frames for one channel. This seems
> > like a case of preferring tolerance over strictly enforcing
> > datasheet-advertised bit-widths. Will take out the check code.
> 
> OK, accepting extra bits is fine.  You should set sig_bits in the DAI so
> userspace can see what's going on if it cares.

I just had a quick look to see what sig_bits does, yes this is a good
addition. Will post a follow-up patch based on the driver that you
already accepted into your tree.

(Btw I'm working on another codec driver and I don't stop getting
positively surprised how flexible and powerful the overall ASoC
framework is).

Regards,
Andreas

> > Along these lines, earlier as I was rummaging through the existing
> > drivers looking for a solution I could model after I noticed that
> > most(?) ASoC codec drivers don't have any type of HW fault checking, at
> > at least whatever drivers I looked at. Not sure why this is but given
> > this discussion this seems like a general opportunity to make
> > improvements.
> 
> There are some with over temperature handling (eg, wm8962) but it's
> relatively uncommon for observable protection features to be implemented
> in silicon and even rarer for the interrupts to be hooked up (and hence
> useful to support in software) unless there is also accessory detection
> in the device.  On older devices the required digital logic was often
> excessively expensive and realistically only relatively high power
> speaker drivers have much risk of something going wrong - things like
> headphone outputs or smaller speaker drivers end up with protection from
> their supplies collapsing well before the device is in physical danger.

Re: [PATCH v3 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

[Andreas Dannenberg]

Mark, please see below...

On Fri, May 13, 2016 at 12:55:40PM +0100, Mark Brown wrote:
> On Tue, Apr 26, 2016 at 01:01:05PM -0500, Andreas Dannenberg wrote:
> > On Tue, Apr 26, 2016 at 06:29:36PM +0100, Mark Brown wrote:
> 
> > > Is the device actually going to mess up if someone sends it something
> > > else or is it just going to ignore the extra bits (given that it's doing
> > > autodetection anyway).
> 
> > well in any of the left-justified modes (which are the only ones the
> > driver supports) the device takes and processes as many bits as it can
> > given the clock and divider settings. Any extra bits provided will get
> > ignored, and the next sync happens on the frame sync signal and not by
> > counting bits so there is no downside also as confirmed by some bench
> > testing I did feeding in 32-bit long frames for one channel. This seems
> > like a case of preferring tolerance over strictly enforcing
> > datasheet-advertised bit-widths. Will take out the check code.
> 
> OK, accepting extra bits is fine.  You should set sig_bits in the DAI so
> userspace can see what's going on if it cares.

I just had a quick look to see what sig_bits does, yes this is a good
addition. Will post a follow-up patch based on the driver that you
already accepted into your tree.

(Btw I'm working on another codec driver and I don't stop getting
positively surprised how flexible and powerful the overall ASoC
framework is).

Regards,
Andreas

> > Along these lines, earlier as I was rummaging through the existing
> > drivers looking for a solution I could model after I noticed that
> > most(?) ASoC codec drivers don't have any type of HW fault checking, at
> > at least whatever drivers I looked at. Not sure why this is but given
> > this discussion this seems like a general opportunity to make
> > improvements.
> 
> There are some with over temperature handling (eg, wm8962) but it's
> relatively uncommon for observable protection features to be implemented
> in silicon and even rarer for the interrupts to be hooked up (and hence
> useful to support in software) unless there is also accessory detection
> in the device.  On older devices the required digital logic was often
> excessively expensive and realistically only relatively high power
> speaker drivers have much risk of something going wrong - things like
> headphone outputs or smaller speaker drivers end up with protection from
> their supplies collapsing well before the device is in physical danger.