[PATCH v2 0/3] ASoC: Codecs: Add TDA7802 codec

[PATCH v2 0/3] ASoC: Codecs: Add TDA7802 codec

[Thomas Preston]

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

Thank you Mark Brown, Charles Keepax, Cezary Rojewski and Kirill
Marinushkin for your time and useful feedback (on IRC too). Sorry for
taking so long in getting back to you, there were quite a lot of
changes!

Please let me know if there's anything else which needs changing.

Many thanks,
Thomas

Thomas Preston (3):
  dt-bindings: ASoC: Add TDA7802 amplifier
  ASoC: Add codec driver for ST TDA7802
  ASoC: TDA7802: Add turn-on diagnostic routine

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

-- 
2.21.0

[PATCH v2 1/3] dt-bindings: ASoC: Add TDA7802 amplifier

[Thomas Preston]

Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
Cc: Patrick Glaser <pglaser@tesla.com>
Cc: Rob Duncan <rduncan@tesla.com>
Cc: Nate Case <ncase@tesla.com>
---
 .../devicetree/bindings/sound/tda7802.txt     | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt

diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt
new file mode 100644
index 000000000000..f80aaf4f1ba0
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/tda7802.txt
@@ -0,0 +1,26 @@
+ST TDA7802 audio processor
+
+This device supports I2C only.
+
+Required properties:
+
+- compatible : "st,tda7802"
+- reg : the I2C address of the device
+- enable-supply : a regulator spec for the PLLen pin
+
+Optional properties:
+
+- st,gain-ch13 : gain for channels 1 and 3 (range: 1-4)
+- st,gain-ch24 : gain for channels 2 and 3 (range: 1-4)
+- st,diagnostic-mode-ch13 : diagnotic mode for channels 1 and 3
+                            values: "Speaker" (default), "Booster"
+- st,diagnostic-mode-ch24 : diagnotic mode for channels 2 and 4
+                            values: "Speaker" (default), "Booster"
+
+Example:
+
+amp: tda7802@6c {
+	compatible = "st,tda7802";
+	reg = <0x6c>;
+	enable-supply = <&amp_enable_reg>;
+};
-- 
2.21.0

Re: [PATCH v2 1/3] dt-bindings: ASoC: Add TDA7802 amplifier

[Charles Keepax]

On Tue, Jul 30, 2019 at 01:09:35PM +0100, Thomas Preston wrote:
> Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
> Cc: Patrick Glaser <pglaser@tesla.com>
> Cc: Rob Duncan <rduncan@tesla.com>
> Cc: Nate Case <ncase@tesla.com>
> ---
>  .../devicetree/bindings/sound/tda7802.txt     | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt
> new file mode 100644
> index 000000000000..f80aaf4f1ba0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/tda7802.txt
> @@ -0,0 +1,26 @@
> +ST TDA7802 audio processor
> +
> +This device supports I2C only.
> +
> +Required properties:
> +
> +- compatible : "st,tda7802"
> +- reg : the I2C address of the device
> +- enable-supply : a regulator spec for the PLLen pin
> +
> +Optional properties:
> +
> +- st,gain-ch13 : gain for channels 1 and 3 (range: 1-4)
> +- st,gain-ch24 : gain for channels 2 and 3 (range: 1-4)

I wouldn't have expected the gains to be available as a device
tree setting.

> +- st,diagnostic-mode-ch13 : diagnotic mode for channels 1 and 3
> +                            values: "Speaker" (default), "Booster"
> +- st,diagnostic-mode-ch24 : diagnotic mode for channels 2 and 4
> +                            values: "Speaker" (default), "Booster"
> +
> +Example:
> +
> +amp: tda7802@6c {
> +	compatible = "st,tda7802";
> +	reg = <0x6c>;
> +	enable-supply = <&amp_enable_reg>;
> +};
> -- 
> 2.21.0
> 

Thanks,
Charles

Re: [PATCH v2 1/3] dt-bindings: ASoC: Add TDA7802 amplifier

[Marco Felsch]

Hi Charles,

sorry for jumping in..

On 19-07-30 13:27, Charles Keepax wrote:
> On Tue, Jul 30, 2019 at 01:09:35PM +0100, Thomas Preston wrote:
> > Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
> > Cc: Patrick Glaser <pglaser@tesla.com>
> > Cc: Rob Duncan <rduncan@tesla.com>
> > Cc: Nate Case <ncase@tesla.com>
> > ---
> >  .../devicetree/bindings/sound/tda7802.txt     | 26 +++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt
> > new file mode 100644
> > index 000000000000..f80aaf4f1ba0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/tda7802.txt
> > @@ -0,0 +1,26 @@
> > +ST TDA7802 audio processor
> > +
> > +This device supports I2C only.
> > +
> > +Required properties:
> > +
> > +- compatible : "st,tda7802"
> > +- reg : the I2C address of the device
> > +- enable-supply : a regulator spec for the PLLen pin

Shouldn't that be a pin called 'pllen-gpios'? IMHO I would not use a
regulator for that.

Regards,
  Marco

> > +
> > +Optional properties:
> > +
> > +- st,gain-ch13 : gain for channels 1 and 3 (range: 1-4)
> > +- st,gain-ch24 : gain for channels 2 and 3 (range: 1-4)
> 
> I wouldn't have expected the gains to be available as a device
> tree setting.
> 
> > +- st,diagnostic-mode-ch13 : diagnotic mode for channels 1 and 3
> > +                            values: "Speaker" (default), "Booster"
> > +- st,diagnostic-mode-ch24 : diagnotic mode for channels 2 and 4
> > +                            values: "Speaker" (default), "Booster"
> > +
> > +Example:
> > +
> > +amp: tda7802@6c {
> > +	compatible = "st,tda7802";
> > +	reg = <0x6c>;
> > +	enable-supply = <&amp_enable_reg>;
> > +};
> > -- 
> > 2.21.0
> > 
> 
> Thanks,
> Charles
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [alsa-devel] [PATCH v2 1/3] dt-bindings: ASoC: Add TDA7802 amplifier

[Thomas Preston]

On 30/07/2019 14:12, Marco Felsch wrote:
> Hi Charles,
> 
> sorry for jumping in..
> 
> On 19-07-30 13:27, Charles Keepax wrote:
>> On Tue, Jul 30, 2019 at 01:09:35PM +0100, Thomas Preston wrote:
>>> Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
>>> Cc: Patrick Glaser <pglaser@tesla.com>
>>> Cc: Rob Duncan <rduncan@tesla.com>
>>> Cc: Nate Case <ncase@tesla.com>
>>> ---
>>>  .../devicetree/bindings/sound/tda7802.txt     | 26 +++++++++++++++++++
>>>  1 file changed, 26 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt
>>> new file mode 100644
>>> index 000000000000..f80aaf4f1ba0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/sound/tda7802.txt
>>> @@ -0,0 +1,26 @@
>>> +ST TDA7802 audio processor
>>> +
>>> +This device supports I2C only.
>>> +
>>> +Required properties:
>>> +
>>> +- compatible : "st,tda7802"
>>> +- reg : the I2C address of the device
>>> +- enable-supply : a regulator spec for the PLLen pin
> 
> Shouldn't that be a pin called 'pllen-gpios'? IMHO I would not use a
> regulator for that.
> 
> Regards,
>   Marco
> 

Hi Marco,
We have multiple amplifiers hooked up in a chain, and all the PLLens
are connected to one GPIO. So we need to use a regulator so that
i2c-TDA7802:00 doesn't turn off the PLLen which i2c-TDA7802:01 still
requires.

This is why we use a regulator. Is there GPIO support for this?

Thanks,
Thomas

Re: [alsa-devel] [PATCH v2 1/3] dt-bindings: ASoC: Add TDA7802 amplifier

[Mark Brown]

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

On Tue, Jul 30, 2019 at 03:12:21PM +0100, Thomas Preston wrote:
> On 30/07/2019 14:12, Marco Felsch wrote:

> >>> +- compatible : "st,tda7802"
> >>> +- reg : the I2C address of the device
> >>> +- enable-supply : a regulator spec for the PLLen pin

> > Shouldn't that be a pin called 'pllen-gpios'? IMHO I would not use a
> > regulator for that.

> Hi Marco,
> We have multiple amplifiers hooked up in a chain, and all the PLLens
> are connected to one GPIO. So we need to use a regulator so that
> i2c-TDA7802:00 doesn't turn off the PLLen which i2c-TDA7802:01 still
> requires.

> This is why we use a regulator. Is there GPIO support for this?

If it's a GPIO not a regulator then it should be a GPIO not a regulator
in the device tree.  The device tree describes the hardware.  There was
some work on helping share GPIOs in the GPIO framework to accomodate
GPIOs for regulator enables, you should be able to do something similar
to what the regulator framework does.

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

Re: [alsa-devel] [PATCH v2 1/3] dt-bindings: ASoC: Add TDA7802 amplifier

[Thomas Preston]

On 30/07/2019 13:27, Charles Keepax wrote:
> On Tue, Jul 30, 2019 at 01:09:35PM +0100, Thomas Preston wrote:
>> Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
>> Cc: Patrick Glaser <pglaser@tesla.com>
>> Cc: Rob Duncan <rduncan@tesla.com>
>> Cc: Nate Case <ncase@tesla.com>
>> ---
>>  .../devicetree/bindings/sound/tda7802.txt     | 26 +++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/sound/tda7802.txt
>>
>> diff --git a/Documentation/devicetree/bindings/sound/tda7802.txt b/Documentation/devicetree/bindings/sound/tda7802.txt
>> new file mode 100644
>> index 000000000000..f80aaf4f1ba0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/tda7802.txt
>> @@ -0,0 +1,26 @@
>> +ST TDA7802 audio processor
>> +
>> +This device supports I2C only.
>> +
>> +Required properties:
>> +
>> +- compatible : "st,tda7802"
>> +- reg : the I2C address of the device
>> +- enable-supply : a regulator spec for the PLLen pin
>> +
>> +Optional properties:
>> +
>> +- st,gain-ch13 : gain for channels 1 and 3 (range: 1-4)
>> +- st,gain-ch24 : gain for channels 2 and 3 (range: 1-4)
> 
> I wouldn't have expected the gains to be available as a device
> tree setting.
> 

Ah, I forgot to update the docs from v1. Thanks

[PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802

[Thomas Preston]

Add an I2C based codec driver for ST TDA7802 amplifier. The amplifier
supports 4 audio channels but can support up to 16 with multiple
devices.

Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
Cc: Patrick Glaser <pglaser@tesla.com>
Cc: Rob Duncan <rduncan@tesla.com>
Cc: Nate Case <ncase@tesla.com>
---
Changes since v1:
- Use ALSA kcontrol interface to expose device controls to userland
	- Gain
	- Channel diagnostic mode
	- Impedance efficiency optimiser. I decided against setting this
	  as a DT property since it seems like something that can be
	  changed on the fly.
- Add regmap default values
	- Channel unmute by default is added in a downstream patch.
	- I'm not sure if I should keep this since they're all zero,
	  although there are other drivers will all-zero reg_defaults.
- I believe the "//" style is used for SPDX headers in normal C source files.
  https://lwn.net/Articles/739183/
- Drop the "enable" sysfs device attribute.
- Don't set TDM format using magic numbers.
- Set sample rate using hw_params.
- Remove unecessary defines.
- Use DAPM to handle AMP_ON.
- Cosmetic fixups

 sound/soc/codecs/Kconfig   |   6 +
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/tda7802.c | 509 +++++++++++++++++++++++++++++++++++++
 3 files changed, 517 insertions(+)
 create mode 100644 sound/soc/codecs/tda7802.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 9f89a5346299..7e3117eab735 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -182,6 +182,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_TAS5720 if I2C
 	select SND_SOC_TAS6424 if I2C
 	select SND_SOC_TDA7419 if I2C
+	select SND_SOC_TDA7802 if I2C
 	select SND_SOC_TFA9879 if I2C
 	select SND_SOC_TLV320AIC23_I2C if I2C
 	select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
@@ -1121,6 +1122,11 @@ config SND_SOC_TDA7419
 	depends on I2C
 	select REGMAP_I2C
 
+config SND_SOC_TDA7802
+	tristate "ST TDA7802 audio processor"
+	depends on I2C
+	select REGMAP_I2C
+
 config SND_SOC_TFA9879
 	tristate "NXP Semiconductors TFA9879 amplifier"
 	depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 5b4bb8cf4325..31dec8930e98 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -194,6 +194,7 @@ snd-soc-tas571x-objs := tas571x.o
 snd-soc-tas5720-objs := tas5720.o
 snd-soc-tas6424-objs := tas6424.o
 snd-soc-tda7419-objs := tda7419.o
+snd-soc-tda7802-objs := tda7802.o
 snd-soc-tfa9879-objs := tfa9879.o
 snd-soc-tlv320aic23-objs := tlv320aic23.o
 snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o
@@ -474,6 +475,7 @@ obj-$(CONFIG_SND_SOC_TAS571X)	+= snd-soc-tas571x.o
 obj-$(CONFIG_SND_SOC_TAS5720)	+= snd-soc-tas5720.o
 obj-$(CONFIG_SND_SOC_TAS6424)	+= snd-soc-tas6424.o
 obj-$(CONFIG_SND_SOC_TDA7419)	+= snd-soc-tda7419.o
+obj-$(CONFIG_SND_SOC_TDA7802)	+= snd-soc-tda7802.o
 obj-$(CONFIG_SND_SOC_TFA9879)	+= snd-soc-tfa9879.o
 obj-$(CONFIG_SND_SOC_TLV320AIC23)	+= snd-soc-tlv320aic23.o
 obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C)	+= snd-soc-tlv320aic23-i2c.o
diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
new file mode 100644
index 000000000000..0f82a88bc1a4
--- /dev/null
+++ b/sound/soc/codecs/tda7802.c
@@ -0,0 +1,509 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * tda7802.c  --  codec driver for ST TDA7802
+ *
+ * Copyright (C) 2016-2019 Tesla Motors, Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/string.h>
+#include <sound/control.h>
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+
+#define ENABLE_DELAY_MS			10
+
+#define TDA7802_IB0			0x00
+#define TDA7802_IB1			0x01
+#define TDA7802_IB2			0x02
+#define TDA7802_IB3			0x03
+#define TDA7802_IB4			0x04
+#define TDA7802_IB5			0x05
+
+#define TDA7802_DB0			0x10
+#define TDA7802_DB5			0x15
+
+#define IB2_DIGITAL_MUTE_DISABLED	(1 << 2)
+
+#define IB3_SAMPLE_RATE_SHIFT		6
+#define IB3_SAMPLE_RATE_MASK		(3 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_SAMPLE_RATE_44_KHZ		(0 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_SAMPLE_RATE_48_KHZ		(1 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_SAMPLE_RATE_96_KHZ		(2 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_SAMPLE_RATE_192_KHZ		(3 << IB3_SAMPLE_RATE_SHIFT)
+#define IB3_FORMAT_SHIFT		3
+#define IB3_FORMAT_MASK			(7 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_I2S			(0 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM4			(1 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM8_DEV1		(2 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM8_DEV2		(3 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM16_DEV1		(4 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM16_DEV2		(5 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM16_DEV3		(6 << IB3_FORMAT_SHIFT)
+#define IB3_FORMAT_TDM16_DEV4		(7 << IB3_FORMAT_SHIFT)
+
+enum tda7802_type {
+	tda7802_base,
+};
+
+struct tda7802_priv {
+	struct i2c_client *i2c;
+	struct regmap *regmap;
+	struct regulator *enable_reg;
+};
+
+static const struct reg_default tda7802_reg[] = {
+	{ TDA7802_IB0, 0x0 },
+	{ TDA7802_IB1, 0x0 },
+	{ TDA7802_IB2, 0x0 },
+	{ TDA7802_IB3, 0x0 },
+	{ TDA7802_IB4, 0x0 },
+	{ TDA7802_IB5, 0x0 },
+};
+
+static bool tda7802_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TDA7802_IB0 ... TDA7802_IB5:
+	case TDA7802_DB0 ... TDA7802_DB5:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool tda7802_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TDA7802_DB0 ... TDA7802_DB5:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool tda7802_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TDA7802_IB0 ... TDA7802_IB5:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config tda7802_regmap_config = {
+	.val_bits = 8,
+	.reg_bits = 8,
+	.max_register = TDA7802_DB5,
+	.use_single_read = 1,
+	.use_single_write = 1,
+
+	.readable_reg = tda7802_readable_reg,
+	.volatile_reg = tda7802_volatile_reg,
+	.writeable_reg = tda7802_writeable_reg,
+
+	.reg_defaults = tda7802_reg,
+	.num_reg_defaults = ARRAY_SIZE(tda7802_reg),
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
+{
+	const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;
+	struct device *dev = dai->dev;
+	int err;
+
+	dev_dbg(dev, "%s mute=%d\n", __func__, mute);
+
+	err = snd_soc_component_update_bits(dai->component, TDA7802_IB2,
+			IB2_DIGITAL_MUTE_DISABLED, mute_disabled);
+	if (err < 0)
+		dev_err(dev, "Cannot mute amp %d\n", err);
+
+	return err;
+}
+
+static int tda7802_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
+		unsigned int rx_mask, int slots, int slot_width)
+{
+	struct device *dev = dai->dev;
+	u8 tdm_format;
+	int ret;
+
+	dev_dbg(dai->dev, "%s tx %x, rx %x, slots %d, slot_width %d\n",
+			__func__, tx_mask, rx_mask, slots, slot_width);
+
+	switch (slots) {
+	case 4:
+		tdm_format = IB3_FORMAT_TDM4;
+		break;
+	case 8:
+		switch (tx_mask) {
+		case 0x000f:
+			tdm_format = IB3_FORMAT_TDM8_DEV1;
+			break;
+		case 0x00f0:
+			tdm_format = IB3_FORMAT_TDM8_DEV2;
+			break;
+		default:
+			dev_err(dev,
+				"Failed to set tdm fmt, slots %d, tx_mask %x\n",
+				slots, tx_mask);
+			return -ENOTSUPP;
+		}
+		break;
+	case 16:
+		switch (tx_mask) {
+		case 0x000f:
+			tdm_format = IB3_FORMAT_TDM16_DEV1;
+			break;
+		case 0x00f0:
+			tdm_format = IB3_FORMAT_TDM16_DEV2;
+			break;
+		case 0x0f00:
+			tdm_format = IB3_FORMAT_TDM16_DEV3;
+			break;
+		case 0xf000:
+			tdm_format = IB3_FORMAT_TDM16_DEV4;
+			break;
+		default:
+			dev_err(dev,
+				"Failed to set tdm fmt, slots %d, tx_mask %x\n",
+				slots, tx_mask);
+			return -ENOTSUPP;
+		}
+		break;
+	default:
+		dev_err(dev, "Failed to set %d slots, supported: 4, 8, 16\n",
+				slots);
+		return -ENOTSUPP;
+	}
+
+	ret = snd_soc_component_update_bits(dai->component, TDA7802_IB3,
+			IB3_FORMAT_MASK, tdm_format);
+	if (ret < 0) {
+		dev_err(dev, "Failed to write IB3 config %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tda7802_hw_params(struct snd_pcm_substream *substream,
+		struct snd_pcm_hw_params *params,
+		struct snd_soc_dai *dai)
+{
+	int err;
+	u8 val;
+
+	dev_dbg(dai->dev, "%s rate %d\n", __func__, params_rate(params));
+
+	switch (params_rate(params)) {
+	case 44100:
+		val = IB3_SAMPLE_RATE_44_KHZ;
+		break;
+	case 48000:
+		val = IB3_SAMPLE_RATE_48_KHZ;
+		break;
+	case 96000:
+		val = IB3_SAMPLE_RATE_96_KHZ;
+		break;
+	case 192000:
+		val = IB3_SAMPLE_RATE_192_KHZ;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	err = snd_soc_component_update_bits(dai->component, TDA7802_IB3,
+			IB3_SAMPLE_RATE_MASK, val);
+	if (err < 0)
+		dev_err(dai->dev, "Could not set hw_params, %d\n", err);
+
+	return err;
+}
+
+static const struct snd_soc_dai_ops tda7802_dai_ops = {
+	.digital_mute = tda7802_digital_mute,
+	.hw_params = tda7802_hw_params,
+	.set_tdm_slot = tda7802_set_tdm_slot,
+};
+
+static struct snd_soc_dai_driver tda7802_dai_driver = {
+	.name = "tda7802",
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 4,
+		.channels_max = 4,
+		.rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
+			SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_192000,
+		.formats = SNDRV_PCM_FMTBIT_S32_LE,
+	},
+	.ops = &tda7802_dai_ops,
+};
+
+static int tda7802_set_bias_level(struct snd_soc_component *component,
+		enum snd_soc_bias_level level)
+{
+	const struct tda7802_priv *tda7802 =
+		snd_soc_component_get_drvdata(component);
+	struct snd_soc_dapm_context *dapm_context =
+			snd_soc_component_get_dapm(component);
+	const enum snd_soc_bias_level oldlevel =
+		snd_soc_dapm_get_bias_level(dapm_context);
+	int err = 0;
+
+	dev_dbg(component->dev, "%s level %d\n", __func__, level);
+
+	switch (level) {
+	case SND_SOC_BIAS_ON:
+		break;
+	case SND_SOC_BIAS_PREPARE:
+		break;
+	case SND_SOC_BIAS_STANDBY:
+		err = regulator_enable(tda7802->enable_reg);
+		if (err < 0) {
+			dev_err(component->dev, "Could not enable.\n");
+			return err;
+		}
+		dev_dbg(component->dev, "Regulator enabled\n");
+		msleep(ENABLE_DELAY_MS);
+
+		if (oldlevel == SND_SOC_BIAS_OFF) {
+			dev_dbg(component->dev, "Syncing regcache\n");
+			err = regcache_sync(component->regmap);
+			if (err < 0)
+				dev_err(component->dev,
+					"Could not sync regcache, %d\n", err);
+		}
+		break;
+	case SND_SOC_BIAS_OFF:
+		regcache_mark_dirty(component->regmap);
+		err = regulator_disable(tda7802->enable_reg);
+		if (err < 0)
+			dev_err(component->dev, "Could not disable.\n");
+		break;
+	}
+
+	return err;
+}
+
+static const char * const amp_mode_str[] = {
+	"High Efficiency",
+	"Standard Class AB",
+};
+
+static SOC_ENUM_SINGLE_DECL(ch1_amp_mode, TDA7802_IB0, 0, amp_mode_str);
+static SOC_ENUM_SINGLE_DECL(ch2_amp_mode, TDA7802_IB0, 1, amp_mode_str);
+static SOC_ENUM_SINGLE_DECL(ch3_amp_mode, TDA7802_IB0, 2, amp_mode_str);
+static SOC_ENUM_SINGLE_DECL(ch4_amp_mode, TDA7802_IB0, 3, amp_mode_str);
+
+static const char * const zopt_str[] = {
+	"2ohm",
+	"4ohm",
+};
+
+static SOC_ENUM_SINGLE_DECL(zopt_ch24, TDA7802_IB1, 7, zopt_str);
+static SOC_ENUM_SINGLE_DECL(zopt_ch13, TDA7802_IB2, 0, zopt_str);
+
+static const char * const diag_timing_str[] = {
+	"default",
+	"x2",
+	"x4",
+	"x8",
+};
+
+static SOC_ENUM_SINGLE_DECL(diag_timing, TDA7802_IB1, 5, diag_timing_str);
+
+static const char * const mute_time_str[] = {
+	"1.45ms",
+	"5.8ms",
+	"11.6ms",
+	"23.2ms",
+	"46.4ms",
+	"92.8ms",
+	"185.6ms",
+	"371.2ms",
+};
+
+static SOC_ENUM_SINGLE_DECL(mute_time, TDA7802_IB2, 5, mute_time_str);
+
+static const char * const automute_threshold_str[] = {
+	"5.3V",
+	"7.3V",
+};
+
+static SOC_ENUM_SINGLE_DECL(automute_threshold, TDA7802_IB2, 1,
+		automute_threshold_str);
+
+static const char * const ac_diag_threshold_str[] = {
+	"High",
+	"Low",
+};
+
+static SOC_ENUM_SINGLE_DECL(ac_diag_threshold, TDA7802_IB3, 4,
+		ac_diag_threshold_str);
+
+static const char * const ch_diag_mode_str[] = {
+	"Speaker",
+	"Boosted",
+};
+
+static SOC_ENUM_SINGLE_DECL(diag_mode_ch13, TDA7802_IB4, 2, ch_diag_mode_str);
+static SOC_ENUM_SINGLE_DECL(diag_mode_ch24, TDA7802_IB4, 1, ch_diag_mode_str);
+
+static const char * const temp_warn_str[] = {
+	"TW1",
+	"TW2",
+	"TW3",
+	"TW4",
+	"None",
+};
+
+static SOC_ENUM_SINGLE_DECL(temp_warn, TDA7802_IB5, 5, temp_warn_str);
+
+static const char * const clip_detect_str[] = {
+	"2%",
+	"5%",
+	"10%",
+	"None",
+};
+
+static SOC_ENUM_SINGLE_DECL(clip_detect_ch13, TDA7802_IB5, 3, clip_detect_str);
+static SOC_ENUM_SINGLE_DECL(clip_detect_ch24, TDA7802_IB5, 1, clip_detect_str);
+
+static const struct snd_kcontrol_new tda7802_snd_controls[] = {
+	SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0),
+	SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0),
+	SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0),
+	SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0),
+	SOC_ENUM("Channel 4 Amplifier Mode", ch4_amp_mode),
+	SOC_ENUM("Channel 3 Amplifier Mode", ch3_amp_mode),
+	SOC_ENUM("Channel 2 Amplifier Mode", ch2_amp_mode),
+	SOC_ENUM("Channel 1 Amplifier Mode", ch1_amp_mode),
+
+	/* Impedance (Z) efficiency optimiser */
+	SOC_ENUM("Z efficiency opt channels 2 & 4", zopt_ch24),
+	SOC_ENUM("Z efficiency opt channels 1 & 3", zopt_ch13),
+
+	SOC_ENUM("Long diag config timing", diag_timing),
+	SOC_SINGLE_RANGE("Gain channels 1 & 3", TDA7802_IB1, 3, 0, 3, 0),
+	SOC_SINGLE_RANGE("Gain channels 2 & 4", TDA7802_IB1, 1, 0, 3, 0),
+	SOC_SINGLE("Digital gain boost +6db", TDA7802_IB1, 0, 1, 0),
+
+	/* Mute settings */
+	SOC_ENUM("Mute time", mute_time),
+	SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0),
+	SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0),
+	SOC_ENUM("Automute threshold", automute_threshold),
+
+	SOC_SINGLE("High pass filter enable", TDA7802_IB3, 0, 1, 0),
+
+	/* Interactive diagnostics */
+	SOC_SINGLE("Noise gating func enable", TDA7802_IB4, 7, 1, 1),
+	SOC_SINGLE("CDdiag: short fault", TDA7802_IB4, 6, 1, 1),
+	SOC_SINGLE("CDdiag: offset", TDA7802_IB4, 5, 1, 1),
+	SOC_ENUM("CDdiag: temperature warning", temp_warn),
+	SOC_ENUM("AC diag current threshold", ac_diag_threshold),
+	SOC_SINGLE("AC diag enable", TDA7802_IB4, 3, 1, 0),
+	SOC_ENUM("Diag mode channels 1 & 3", diag_mode_ch13),
+	SOC_ENUM("Diag mode channels 2 & 4", diag_mode_ch24),
+	SOC_SINGLE("Diag mode enable", TDA7802_IB4, 0, 1, 0),
+
+	SOC_ENUM("Clipping detect channels 1 & 3", clip_detect_ch13),
+	SOC_ENUM("Clipping detect channels 2 & 4", clip_detect_ch24),
+};
+
+static const struct snd_soc_dapm_widget tda7802_dapm_widgets[] = {
+	SND_SOC_DAPM_AIF_IN("AIFIN", NULL, 0, SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_DAC("DAC", NULL, TDA7802_IB5, 0, 0),
+	SND_SOC_DAPM_OUTPUT("SPK"),
+};
+
+static const struct snd_soc_dapm_route tda7802_dapm_routes[] = {
+	{ "AIFIN", NULL, "Playback" },
+	{ "DAC", NULL, "AIFIN" },
+	{ "SPK", NULL, "DAC" },
+};
+
+static const struct snd_soc_component_driver tda7802_component_driver = {
+	.set_bias_level = tda7802_set_bias_level,
+	.idle_bias_on = 1,
+	.suspend_bias_off = 1,
+	.controls = tda7802_snd_controls,
+	.num_controls = ARRAY_SIZE(tda7802_snd_controls),
+	.dapm_widgets = tda7802_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(tda7802_dapm_widgets),
+	.dapm_routes = tda7802_dapm_routes,
+	.num_dapm_routes = ARRAY_SIZE(tda7802_dapm_routes),
+};
+
+static int tda7802_i2c_probe(struct i2c_client *i2c,
+			     const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct tda7802_priv *tda7802;
+	int err;
+
+	dev_dbg(dev, "%s addr=0x%02hx, id %p\n", __func__, i2c->addr, id);
+
+	tda7802 = devm_kmalloc(dev, sizeof(*tda7802), GFP_KERNEL);
+	if (!tda7802)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, tda7802);
+	tda7802->i2c = i2c;
+
+	tda7802->enable_reg = devm_regulator_get(dev, "enable");
+	if (IS_ERR(tda7802->enable_reg)) {
+		dev_err(dev, "Failed to get enable regulator\n");
+		return PTR_ERR(tda7802->enable_reg);
+	}
+
+	tda7802->regmap = devm_regmap_init_i2c(tda7802->i2c,
+			&tda7802_regmap_config);
+	if (IS_ERR(tda7802->regmap))
+		return PTR_ERR(tda7802->regmap);
+
+	err = devm_snd_soc_register_component(dev, &tda7802_component_driver,
+			&tda7802_dai_driver, 1);
+	if (err < 0)
+		dev_err(dev, "Failed to register codec: %d\n", err);
+	return err;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id tda7802_of_match[] = {
+	{ .compatible = "st,tda7802" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tda7802_of_match);
+#endif
+
+static const struct i2c_device_id tda7802_i2c_id[] = {
+	{ "tda7802", tda7802_base },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, tda7802_i2c_id);
+
+static struct i2c_driver tda7802_i2c_driver = {
+	.driver = {
+		.name  = "tda7802-codec",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(tda7802_of_match),
+	},
+	.probe = tda7802_i2c_probe,
+	.id_table = tda7802_i2c_id,
+};
+module_i2c_driver(tda7802_i2c_driver);
+
+MODULE_DESCRIPTION("ASoC ST TDA7802 driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Rob Duncan <rduncan@tesla.com>");
+MODULE_AUTHOR("Thomas Preston <thomas.preston@codethink.co.uk>");
-- 
2.21.0

Re: [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802

[Charles Keepax]

On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
> Add an I2C based codec driver for ST TDA7802 amplifier. The amplifier
> supports 4 audio channels but can support up to 16 with multiple
> devices.
> 
> Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
> Cc: Patrick Glaser <pglaser@tesla.com>
> Cc: Rob Duncan <rduncan@tesla.com>
> Cc: Nate Case <ncase@tesla.com>
> ---
> Changes since v1:
> - Use ALSA kcontrol interface to expose device controls to userland
> 	- Gain
> 	- Channel diagnostic mode
> 	- Impedance efficiency optimiser. I decided against setting this
> 	  as a DT property since it seems like something that can be
> 	  changed on the fly.
> - Add regmap default values
> 	- Channel unmute by default is added in a downstream patch.
> 	- I'm not sure if I should keep this since they're all zero,
> 	  although there are other drivers will all-zero reg_defaults.
> - I believe the "//" style is used for SPDX headers in normal C source files.
>   https://lwn.net/Articles/739183/
> - Drop the "enable" sysfs device attribute.
> - Don't set TDM format using magic numbers.
> - Set sample rate using hw_params.
> - Remove unecessary defines.
> - Use DAPM to handle AMP_ON.
> - Cosmetic fixups
> 
>  sound/soc/codecs/Kconfig   |   6 +
>  sound/soc/codecs/Makefile  |   2 +
>  sound/soc/codecs/tda7802.c | 509 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 517 insertions(+)
>  create mode 100644 sound/soc/codecs/tda7802.c
> 
> +++ b/sound/soc/codecs/tda7802.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * tda7802.c  --  codec driver for ST TDA7802
> + *
> + * Copyright (C) 2016-2019 Tesla Motors, Inc.
> + */

Better to make the whole comment // see something like
sound/soc/codecs/cs47l35.c for an example.

> +static int tda7802_set_bias_level(struct snd_soc_component *component,
> +		enum snd_soc_bias_level level)
> +{
> +	const struct tda7802_priv *tda7802 =
> +		snd_soc_component_get_drvdata(component);
> +	struct snd_soc_dapm_context *dapm_context =
> +			snd_soc_component_get_dapm(component);
> +	const enum snd_soc_bias_level oldlevel =
> +		snd_soc_dapm_get_bias_level(dapm_context);
> +	int err = 0;
> +
> +	dev_dbg(component->dev, "%s level %d\n", __func__, level);
> +
> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +		break;
> +	case SND_SOC_BIAS_PREPARE:
> +		break;
> +	case SND_SOC_BIAS_STANDBY:
> +		err = regulator_enable(tda7802->enable_reg);
> +		if (err < 0) {
> +			dev_err(component->dev, "Could not enable.\n");
> +			return err;
> +		}
> +		dev_dbg(component->dev, "Regulator enabled\n");
> +		msleep(ENABLE_DELAY_MS);
> +
> +		if (oldlevel == SND_SOC_BIAS_OFF) {
> +			dev_dbg(component->dev, "Syncing regcache\n");
> +			err = regcache_sync(component->regmap);
> +			if (err < 0)
> +				dev_err(component->dev,
> +					"Could not sync regcache, %d\n", err);

If your doing a regcache_sync I would probably have expected to
see calls to regcache_cache_only.

If the device needs syncing that implies the hardware registers
have lost state, so there is little point in writing to them
if they are unavailable/about to loose their state.

> +		}
> +		break;
> +	case SND_SOC_BIAS_OFF:
> +		regcache_mark_dirty(component->regmap);
> +		err = regulator_disable(tda7802->enable_reg);
> +		if (err < 0)
> +			dev_err(component->dev, "Could not disable.\n");
> +		break;
> +	}
> +
> +	return err;
> +}

Thanks,
Charles

Re: [alsa-devel] [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802

[Thomas Preston]

On 30/07/2019 13:38, Charles Keepax wrote:
> On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
>> Add an I2C based codec driver for ST TDA7802 amplifier. The amplifier
>> supports 4 audio channels but can support up to 16 with multiple
>> devices.
>>
>> Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
>> Cc: Patrick Glaser <pglaser@tesla.com>
>> Cc: Rob Duncan <rduncan@tesla.com>
>> Cc: Nate Case <ncase@tesla.com>
>> ---
>> Changes since v1:
>> - Use ALSA kcontrol interface to expose device controls to userland
>> 	- Gain
>> 	- Channel diagnostic mode
>> 	- Impedance efficiency optimiser. I decided against setting this
>> 	  as a DT property since it seems like something that can be
>> 	  changed on the fly.
>> - Add regmap default values
>> 	- Channel unmute by default is added in a downstream patch.
>> 	- I'm not sure if I should keep this since they're all zero,
>> 	  although there are other drivers will all-zero reg_defaults.
>> - I believe the "//" style is used for SPDX headers in normal C source files.
>>   https://lwn.net/Articles/739183/
>> - Drop the "enable" sysfs device attribute.
>> - Don't set TDM format using magic numbers.
>> - Set sample rate using hw_params.
>> - Remove unecessary defines.
>> - Use DAPM to handle AMP_ON.
>> - Cosmetic fixups
>>
>>  sound/soc/codecs/Kconfig   |   6 +
>>  sound/soc/codecs/Makefile  |   2 +
>>  sound/soc/codecs/tda7802.c | 509 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 517 insertions(+)
>>  create mode 100644 sound/soc/codecs/tda7802.c
>>
>> +++ b/sound/soc/codecs/tda7802.c
>> @@ -0,0 +1,509 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * tda7802.c  --  codec driver for ST TDA7802
>> + *
>> + * Copyright (C) 2016-2019 Tesla Motors, Inc.
>> + */
> 
> Better to make the whole comment // see something like
> sound/soc/codecs/cs47l35.c for an example.
> 

I will update to "//" style. Is this a new standard? There aren't many
comments like that in 4.14 (my target kernel) - I can see a lot more
in 5.3.

My intention was:

1. Apply the SPDX rules to SPDX bit.     Documentation/process/license-rules.rst
2. Use multi-line comments for the rest. Documentation/process/coding-style.rst

>> +static int tda7802_set_bias_level(struct snd_soc_component *component,
>> +		enum snd_soc_bias_level level)
>> +{
>> +	const struct tda7802_priv *tda7802 =
>> +		snd_soc_component_get_drvdata(component);
>> +	struct snd_soc_dapm_context *dapm_context =
>> +			snd_soc_component_get_dapm(component);
>> +	const enum snd_soc_bias_level oldlevel =
>> +		snd_soc_dapm_get_bias_level(dapm_context);
>> +	int err = 0;
>> +
>> +	dev_dbg(component->dev, "%s level %d\n", __func__, level);
>> +
>> +	switch (level) {
>> +	case SND_SOC_BIAS_ON:
>> +		break;
>> +	case SND_SOC_BIAS_PREPARE:
>> +		break;
>> +	case SND_SOC_BIAS_STANDBY:
>> +		err = regulator_enable(tda7802->enable_reg);
>> +		if (err < 0) {
>> +			dev_err(component->dev, "Could not enable.\n");
>> +			return err;
>> +		}
>> +		dev_dbg(component->dev, "Regulator enabled\n");
>> +		msleep(ENABLE_DELAY_MS);
>> +
>> +		if (oldlevel == SND_SOC_BIAS_OFF) {
>> +			dev_dbg(component->dev, "Syncing regcache\n");
>> +			err = regcache_sync(component->regmap);
>> +			if (err < 0)
>> +				dev_err(component->dev,
>> +					"Could not sync regcache, %d\n", err);
> 
> If your doing a regcache_sync I would probably have expected to
> see calls to regcache_cache_only.
> 
> If the device needs syncing that implies the hardware registers
> have lost state, so there is little point in writing to them
> if they are unavailable/about to loose their state.
> 

Ah, from the comments I thought I only needed to call regcache_mark_dirty...

>> +		}
>> +		break;
>> +	case SND_SOC_BIAS_OFF:
>> +		regcache_mark_dirty(component->regmap);
>> +		err = regulator_disable(tda7802->enable_reg);
>> +		if (err < 0)
>> +			dev_err(component->dev, "Could not disable.\n");
>> +		break;
>> +	}
>> +
>> +	return err;
>> +}

So I think the correct order is:

device_off:
	regcache_cache_only
	power-off (enable)
	regcache_mark_dirty

device_on:
	power-on (enable)
	regcache_sync

I will double-check the register state is actually lost too. Fiddling
with the cache might be completely unnecessary.

Many thanks

Re: [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802

[Mark Brown]

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

On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:

> index 000000000000..0f82a88bc1a4
> --- /dev/null
> +++ b/sound/soc/codecs/tda7802.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * tda7802.c  --  codec driver for ST TDA7802

Please make the entire comment a C++ one so this looks intentional.

> +static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
> +{
> +	const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;

Please write normal conditional statements to make the code easier to
read.

> +	case SND_SOC_BIAS_STANDBY:
> +		err = regulator_enable(tda7802->enable_reg);
> +		if (err < 0) {
> +			dev_err(component->dev, "Could not enable.\n");
> +			return err;
> +		}
> +		dev_dbg(component->dev, "Regulator enabled\n");
> +		msleep(ENABLE_DELAY_MS);

Is this delay needed by the device or is it for the regulator to ramp?
If it's for the regulator to ramp then the regulator should be doing it.

> +	case SND_SOC_BIAS_OFF:
> +		regcache_mark_dirty(component->regmap);

If the regulator is going off you should really be marking the device as
cache only.

> +		err = regulator_disable(tda7802->enable_reg);
> +		if (err < 0)
> +			dev_err(component->dev, "Could not disable.\n");

Any non-zero value from regulator_disable() is an error, there's similar
error checking issues in other places.

> +static const struct snd_kcontrol_new tda7802_snd_controls[] = {
> +	SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0),
> +	SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0),
> +	SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0),
> +	SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0),

These look like simple on/off switches so should have Switch at the end
of the control name.  It's also not clear to me why this is exported to
userspace - why would this change at runtime and won't any changes need
to be coordinated with whatever else is connected to the signal?

> +	SOC_ENUM("Mute time", mute_time),
> +	SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0),
> +	SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0),

These are also Switch controls.  There are *lots* of problems with
control names, see control-names.rst.

> +static const struct snd_soc_component_driver tda7802_component_driver = {
> +	.set_bias_level = tda7802_set_bias_level,
> +	.idle_bias_on = 1,

Any reason to keep the device powered up?  It looks like the power on
process is just powering things up and writing the register cache out
and there's not that many registers so the delay is trivial.

> +	tda7802->enable_reg = devm_regulator_get(dev, "enable");
> +	if (IS_ERR(tda7802->enable_reg)) {
> +		dev_err(dev, "Failed to get enable regulator\n");

It's better to print error codes if you have them and are printing a
diagnostic so people have more to go on when debugging problems.

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

Re: [alsa-devel] [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802

[Thomas Preston]

On 30/07/2019 15:58, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
> 
>> index 000000000000..0f82a88bc1a4
>> --- /dev/null
>> +++ b/sound/soc/codecs/tda7802.c
>> @@ -0,0 +1,509 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * tda7802.c  --  codec driver for ST TDA7802
> 
> Please make the entire comment a C++ one so this looks intentional.
> 

Ok thanks.

>> +static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
>> +{
>> +	const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;
> 
> Please write normal conditional statements to make the code easier to
> read.
> 

On it.

>> +	case SND_SOC_BIAS_STANDBY:
>> +		err = regulator_enable(tda7802->enable_reg);
>> +		if (err < 0) {
>> +			dev_err(component->dev, "Could not enable.\n");
>> +			return err;
>> +		}
>> +		dev_dbg(component->dev, "Regulator enabled\n");
>> +		msleep(ENABLE_DELAY_MS);
> 
> Is this delay needed by the device or is it for the regulator to ramp?
> If it's for the regulator to ramp then the regulator should be doing it.
> 

According to the datasheet the device itself takes 10ms to rise from 0V
after PLLen is enabled. There are additional rise times but they are
negligible with default capacitor configuration (which we have).

Good to know about the regulator rising configuration though. Thanks.

>> +	case SND_SOC_BIAS_OFF:
>> +		regcache_mark_dirty(component->regmap);
> 
> If the regulator is going off you should really be marking the device as
> cache only.
> 

Got it, thanks.

>> +		err = regulator_disable(tda7802->enable_reg);
>> +		if (err < 0)
>> +			dev_err(component->dev, "Could not disable.\n");
> 
> Any non-zero value from regulator_disable() is an error, there's similar
> error checking issues in other places.
> 

I return the error at the end of the function, but I'll bring it back here
for consistency.

>> +static const struct snd_kcontrol_new tda7802_snd_controls[] = {
>> +	SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0),
>> +	SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0),
>> +	SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0),
>> +	SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0),
> 
> These look like simple on/off switches so should have Switch at the end
> of the control name.  It's also not clear to me why this is exported to
> userspace - why would this change at runtime and won't any changes need
> to be coordinated with whatever else is connected to the signal?
> 
>> +	SOC_ENUM("Mute time", mute_time),
>> +	SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0),
>> +	SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0),
> 
> These are also Switch controls.  There are *lots* of problems with
> control names, see control-names.rst.
> 

Ok thanks, I didn't know about the "Switch" suffix, I will read
control-names.rst.

I will move Tristate to DT properties. I was also unsure about the
Impedance Efficiency Optimiser but the datasheet doesn't go into much
detail about this so I left it exposed.

They both seemed like user configurable options in the context of a
test rig, but I agree - who knows what this output might be connected
to in other systems. I will lock them down in DT. Thanks.

>> +static const struct snd_soc_component_driver tda7802_component_driver = {
>> +	.set_bias_level = tda7802_set_bias_level,
>> +	.idle_bias_on = 1,
> 
> Any reason to keep the device powered up?  It looks like the power on
> process is just powering things up and writing the register cache out
> and there's not that many registers so the delay is trivial.
> 

Ah no, I think that's a mistake. I want the PLLen to switch off in
idle/suspend and the device should restore on wake.

>> +	tda7802->enable_reg = devm_regulator_get(dev, "enable");
>> +	if (IS_ERR(tda7802->enable_reg)) {
>> +		dev_err(dev, "Failed to get enable regulator\n");
> 
> It's better to print error codes if you have them and are printing a
> diagnostic so people have more to go on when debugging problems.

Yep on it.

Many thanks, I appreciate the feedback.

Re: [alsa-devel] [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802

[Marco Felsch]

Hi Thomas,

again sorry for jumping in..

On 19-07-30 18:26, Thomas Preston wrote:
> On 30/07/2019 15:58, Mark Brown wrote:
> > On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
> > 
> >> index 000000000000..0f82a88bc1a4
> >> --- /dev/null
> >> +++ b/sound/soc/codecs/tda7802.c
> >> @@ -0,0 +1,509 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * tda7802.c  --  codec driver for ST TDA7802
> > 
> > Please make the entire comment a C++ one so this looks intentional.
> > 
> 
> Ok thanks.
> 
> >> +static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
> >> +{
> >> +	const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;
> > 
> > Please write normal conditional statements to make the code easier to
> > read.
> > 
> 
> On it.
> 
> >> +	case SND_SOC_BIAS_STANDBY:
> >> +		err = regulator_enable(tda7802->enable_reg);
> >> +		if (err < 0) {
> >> +			dev_err(component->dev, "Could not enable.\n");
> >> +			return err;
> >> +		}
> >> +		dev_dbg(component->dev, "Regulator enabled\n");
> >> +		msleep(ENABLE_DELAY_MS);
> > 
> > Is this delay needed by the device or is it for the regulator to ramp?
> > If it's for the regulator to ramp then the regulator should be doing it.
> > 
> 
> According to the datasheet the device itself takes 10ms to rise from 0V
> after PLLen is enabled. There are additional rise times but they are
> negligible with default capacitor configuration (which we have).
> 
> Good to know about the regulator rising configuration though. Thanks.

Isn't it the regulator we mentioned to not use that because it is a
GPIO?

Regards,
  Marco

> >> +	case SND_SOC_BIAS_OFF:
> >> +		regcache_mark_dirty(component->regmap);
> > 
> > If the regulator is going off you should really be marking the device as
> > cache only.
> > 
> 
> Got it, thanks.
> 
> >> +		err = regulator_disable(tda7802->enable_reg);
> >> +		if (err < 0)
> >> +			dev_err(component->dev, "Could not disable.\n");
> > 
> > Any non-zero value from regulator_disable() is an error, there's similar
> > error checking issues in other places.
> > 
> 
> I return the error at the end of the function, but I'll bring it back here
> for consistency.
> 
> >> +static const struct snd_kcontrol_new tda7802_snd_controls[] = {
> >> +	SOC_SINGLE("Channel 4 Tristate", TDA7802_IB0, 7, 1, 0),
> >> +	SOC_SINGLE("Channel 3 Tristate", TDA7802_IB0, 6, 1, 0),
> >> +	SOC_SINGLE("Channel 2 Tristate", TDA7802_IB0, 5, 1, 0),
> >> +	SOC_SINGLE("Channel 1 Tristate", TDA7802_IB0, 4, 1, 0),
> > 
> > These look like simple on/off switches so should have Switch at the end
> > of the control name.  It's also not clear to me why this is exported to
> > userspace - why would this change at runtime and won't any changes need
> > to be coordinated with whatever else is connected to the signal?
> > 
> >> +	SOC_ENUM("Mute time", mute_time),
> >> +	SOC_SINGLE("Unmute channels 1 & 3", TDA7802_IB2, 4, 1, 0),
> >> +	SOC_SINGLE("Unmute channels 2 & 4", TDA7802_IB2, 3, 1, 0),
> > 
> > These are also Switch controls.  There are *lots* of problems with
> > control names, see control-names.rst.
> > 
> 
> Ok thanks, I didn't know about the "Switch" suffix, I will read
> control-names.rst.
> 
> I will move Tristate to DT properties. I was also unsure about the
> Impedance Efficiency Optimiser but the datasheet doesn't go into much
> detail about this so I left it exposed.
> 
> They both seemed like user configurable options in the context of a
> test rig, but I agree - who knows what this output might be connected
> to in other systems. I will lock them down in DT. Thanks.
> 
> >> +static const struct snd_soc_component_driver tda7802_component_driver = {
> >> +	.set_bias_level = tda7802_set_bias_level,
> >> +	.idle_bias_on = 1,
> > 
> > Any reason to keep the device powered up?  It looks like the power on
> > process is just powering things up and writing the register cache out
> > and there's not that many registers so the delay is trivial.
> > 
> 
> Ah no, I think that's a mistake. I want the PLLen to switch off in
> idle/suspend and the device should restore on wake.
> 
> >> +	tda7802->enable_reg = devm_regulator_get(dev, "enable");
> >> +	if (IS_ERR(tda7802->enable_reg)) {
> >> +		dev_err(dev, "Failed to get enable regulator\n");
> > 
> > It's better to print error codes if you have them and are printing a
> > diagnostic so people have more to go on when debugging problems.
> 
> Yep on it.
> 
> Many thanks, I appreciate the feedback.
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [alsa-devel] [PATCH v2 2/3] ASoC: Add codec driver for ST TDA7802

[Thomas Preston]

On 31/07/2019 07:06, Marco Felsch wrote:
> Hi Thomas,
> 
> again sorry for jumping in..
> 

Np!

> On 19-07-30 18:26, Thomas Preston wrote:
>> On 30/07/2019 15:58, Mark Brown wrote:
>>> On Tue, Jul 30, 2019 at 01:09:36PM +0100, Thomas Preston wrote:
>>>> +	case SND_SOC_BIAS_STANDBY:
>>>> +		err = regulator_enable(tda7802->enable_reg);
>>>> +		if (err < 0) {
>>>> +			dev_err(component->dev, "Could not enable.\n");
>>>> +			return err;
>>>> +		}
>>>> +		dev_dbg(component->dev, "Regulator enabled\n");
>>>> +		msleep(ENABLE_DELAY_MS);
>>>
>>> Is this delay needed by the device or is it for the regulator to ramp?
>>> If it's for the regulator to ramp then the regulator should be doing it.
>>>
>>
>> According to the datasheet the device itself takes 10ms to rise from 0V
>> after PLLen is enabled. There are additional rise times but they are
>> negligible with default capacitor configuration (which we have).
>>
>> Good to know about the regulator rising configuration though. Thanks.
> 
> Isn't it the regulator we mentioned to not use that because it is a
> GPIO?
> 

Yeah it is - I intend to switch PLLen to gpio API.

[PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Thomas Preston]

Add a debugfs device node which initiates the turn-on diagnostic routine
feature of the TDA7802 amplifier. The four status registers (one per
channel) are returned.

Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
---
Changes since v1:
- Rename speaker-test to (turn-on) diagnostics
- Move turn-on diagnostic to debugfs as there is no standard ALSA
  interface for this kind of routine.

 sound/soc/codecs/tda7802.c | 186 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 185 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
index 0f82a88bc1a4..74436212241d 100644
--- a/sound/soc/codecs/tda7802.c
+++ b/sound/soc/codecs/tda7802.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2016-2019 Tesla Motors, Inc.
  */
 
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
@@ -26,6 +27,7 @@
 #define TDA7802_IB5			0x05
 
 #define TDA7802_DB0			0x10
+#define TDA7802_DB1			0x11
 #define TDA7802_DB5			0x15
 
 #define IB2_DIGITAL_MUTE_DISABLED	(1 << 2)
@@ -47,6 +49,17 @@
 #define IB3_FORMAT_TDM16_DEV3		(6 << IB3_FORMAT_SHIFT)
 #define IB3_FORMAT_TDM16_DEV4		(7 << IB3_FORMAT_SHIFT)
 
+#define IB4_DIAG_MODE_ENABLE		(1 << 0)
+
+#define IB5_AMPLIFIER_ON		(1 << 0)
+
+#define DB0_STARTUP_DIAG_STATUS		(1 << 6)
+
+#define DIAGNOSTIC_POLL_PERIOD_US	5000
+#define DIAGNOSTIC_TIMEOUT_US		5000000
+#define DIAGNOSTIC_SETTLE_MS		20
+#define NUM_IB				6
+
 enum tda7802_type {
 	tda7802_base,
 };
@@ -55,6 +68,12 @@ struct tda7802_priv {
 	struct i2c_client *i2c;
 	struct regmap *regmap;
 	struct regulator *enable_reg;
+	struct dentry *debugfs;
+	struct mutex diagnostic_mutex;
+};
+
+struct reg_update {
+	unsigned int reg, mask, val;
 };
 
 static const struct reg_default tda7802_reg[] = {
@@ -113,6 +132,19 @@ static const struct regmap_config tda7802_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+/* The following bits need to be updated before diagnostics mode is set. */
+static const struct reg_update diagnostic_state[NUM_IB] = {
+	{ TDA7802_IB0, 0, 0 },
+	{ TDA7802_IB1, 0, 0 },
+	{ TDA7802_IB2,
+		IB2_CH13_UNMUTED | IB2_CH24_UNMUTED | IB2_DIGITAL_MUTE_DISABLED,
+		IB2_CH13_UNMUTED | IB2_CH24_UNMUTED | IB2_DIGITAL_MUTE_DISABLED,
+	},
+	{ TDA7802_IB3, 0, 0 },
+	{ TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0 },
+	{ TDA7802_IB5, IB5_AMPLIFIER_ON, 0 },
+};
+
 static int tda7802_digital_mute(struct snd_soc_dai *dai, int mute)
 {
 	const u8 mute_disabled = mute ? 0 : IB2_DIGITAL_MUTE_DISABLED;
@@ -414,7 +446,6 @@ static const struct snd_kcontrol_new tda7802_snd_controls[] = {
 	SOC_SINGLE("AC diag enable", TDA7802_IB4, 3, 1, 0),
 	SOC_ENUM("Diag mode channels 1 & 3", diag_mode_ch13),
 	SOC_ENUM("Diag mode channels 2 & 4", diag_mode_ch24),
-	SOC_SINGLE("Diag mode enable", TDA7802_IB4, 0, 1, 0),
 
 	SOC_ENUM("Clipping detect channels 1 & 3", clip_detect_ch13),
 	SOC_ENUM("Clipping detect channels 2 & 4", clip_detect_ch24),
@@ -432,7 +463,160 @@ static const struct snd_soc_dapm_route tda7802_dapm_routes[] = {
 	{ "SPK", NULL, "DAC" },
 };
 
+static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
+		size_t update_count)
+{
+	int i, err;
+
+	for (i = 0; i < update_count; i++) {
+		err = regmap_update_bits(map, update[i].reg, update[i].mask,
+				update[i].val);
+		if (err < 0)
+			return err;
+	}
+
+	return i;
+}
+
+static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status)
+{
+	struct device *dev = &tda7802->i2c->dev;
+	int err_status, err;
+	unsigned int val;
+	u8 state[NUM_IB];
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	/* Save state and prepare for diagnostic */
+	err = regmap_bulk_read(tda7802->regmap, TDA7802_IB0, state,
+			ARRAY_SIZE(state));
+	if (err < 0) {
+		dev_err(dev, "Could not save device state, %d\n", err);
+		return err;
+	}
+
+	err = tda7802_bulk_update(tda7802->regmap, diagnostic_state,
+			ARRAY_SIZE(diagnostic_state));
+	if (err < 0) {
+		dev_err(dev, "Could not prepare for diagnostics, %d\n", err);
+		goto diagnostic_restore;
+	}
+
+	/* We must wait 20ms for device to settle, otherwise diagnostics will
+	 * not start and regmap poll will timeout.
+	 */
+	msleep(DIAGNOSTIC_SETTLE_MS);
+
+	/* Turn on diagnostic */
+	err = regmap_update_bits(tda7802->regmap, TDA7802_IB4,
+			IB4_DIAG_MODE_ENABLE, IB4_DIAG_MODE_ENABLE);
+	if (err < 0) {
+		dev_err(dev, "Could not enable diagnostic mode, %d\n", err);
+		goto diagnostic_restore;
+	}
+
+	/* Wait until DB0_STARTUP_DIAG_STATUS is set, then read status */
+	err_status = regmap_read_poll_timeout(tda7802->regmap, TDA7802_DB0, val,
+			val & DB0_STARTUP_DIAG_STATUS,
+			DIAGNOSTIC_POLL_PERIOD_US,
+			DIAGNOSTIC_TIMEOUT_US);
+	if (err_status < 0) {
+		dev_err(dev, "Diagnostic did not complete, err %d, reg %x",
+				err_status, val);
+		goto diagnostic_restore;
+	}
+
+	err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4);
+	if (err < 0) {
+		dev_err(dev, "Could not read channel status, %d\n", err);
+		goto diagnostic_restore;
+	}
+
+diagnostic_restore:
+	err = regmap_bulk_write(tda7802->regmap, TDA7802_IB0, state,
+			ARRAY_SIZE(state));
+	if (err < 0)
+		dev_err(dev, "Could not restore state, %d\n", err);
+
+	if (err_status < 0)
+		return err_status;
+	else
+		return err;
+}
+
+static int tda7802_diagnostic_show(struct seq_file *f, void *p)
+{
+	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	struct tda7802_priv *tda7802 = f->private;
+	u8 status[4] = { 0 };
+	int i, err;
+
+	mutex_lock(&tda7802->diagnostic_mutex);
+	err = run_turn_on_diagnostic(tda7802, status);
+	mutex_unlock(&tda7802->diagnostic_mutex);
+	if (err < 0)
+		return err;
+
+	for (i = 0; i < ARRAY_SIZE(status); i++)
+		seq_printf(f, "%02x: %02x\n", TDA7802_DB1+i, status[i]);
+
+	return 0;
+}
+
+static int tda7802_diagnostic_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, tda7802_diagnostic_show, inode->i_private);
+}
+
+static const struct file_operations tda7802_diagnostic_fops = {
+	.open = tda7802_diagnostic_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+static int tda7802_probe(struct snd_soc_component *component)
+{
+	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
+	struct device *dev = &tda7802->i2c->dev;
+	int err;
+
+	tda7802->debugfs = debugfs_create_dir(dev_name(dev), NULL);
+	if (IS_ERR_OR_NULL(tda7802->debugfs)) {
+		dev_info(dev,
+			"Failed to create debugfs node, err %ld\n",
+			PTR_ERR(tda7802->debugfs));
+		return 0;
+	}
+
+	mutex_init(&tda7802->diagnostic_mutex);
+	err = debugfs_create_file("diagnostic", 0444, tda7802->debugfs, tda7802,
+			&tda7802_diagnostic_fops);
+	if (err < 0) {
+		dev_err(dev,
+			"debugfs: Failed to create diagnostic node, err %d\n",
+			err);
+		goto cleanup_diagnostic;
+	}
+
+	return 0;
+
+cleanup_diagnostic:
+	mutex_destroy(&tda7802->diagnostic_mutex);
+	return err;
+}
+
+static void tda7802_remove(struct snd_soc_component *component)
+{
+	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
+
+	debugfs_remove_recursive(tda7802->debugfs);
+	mutex_destroy(&tda7802->diagnostic_mutex);
+}
+
 static const struct snd_soc_component_driver tda7802_component_driver = {
+	.probe = tda7802_probe,
+	.remove = tda7802_remove,
 	.set_bias_level = tda7802_set_bias_level,
 	.idle_bias_on = 1,
 	.suspend_bias_off = 1,
-- 
2.21.0

Re: [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Charles Keepax]

On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
> Add a debugfs device node which initiates the turn-on diagnostic routine
> feature of the TDA7802 amplifier. The four status registers (one per
> channel) are returned.
> 
> Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
> ---
> Changes since v1:
> - Rename speaker-test to (turn-on) diagnostics
> - Move turn-on diagnostic to debugfs as there is no standard ALSA
>   interface for this kind of routine.
> 
>  sound/soc/codecs/tda7802.c | 186 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 185 insertions(+), 1 deletion(-)
> 
> +static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
> +		size_t update_count)
> +{
> +	int i, err;
> +
> +	for (i = 0; i < update_count; i++) {
> +		err = regmap_update_bits(map, update[i].reg, update[i].mask,
> +				update[i].val);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	return i;
> +}

This could probably be removed using regmap_multi_reg_write.

> +static int tda7802_probe(struct snd_soc_component *component)
> +{
> +	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
> +	struct device *dev = &tda7802->i2c->dev;
> +	int err;
> +
> +	tda7802->debugfs = debugfs_create_dir(dev_name(dev), NULL);
> +	if (IS_ERR_OR_NULL(tda7802->debugfs)) {
> +		dev_info(dev,
> +			"Failed to create debugfs node, err %ld\n",
> +			PTR_ERR(tda7802->debugfs));
> +		return 0;
> +	}
> +
> +	mutex_init(&tda7802->diagnostic_mutex);
> +	err = debugfs_create_file("diagnostic", 0444, tda7802->debugfs, tda7802,
> +			&tda7802_diagnostic_fops);
> +	if (err < 0) {
> +		dev_err(dev,
> +			"debugfs: Failed to create diagnostic node, err %d\n",
> +			err);
> +		goto cleanup_diagnostic;
> +	}

You shouldn't be failing the driver probe if debugfs fails, it
should be purely optional.

Thanks,
Charles

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Thomas Preston]

Hi,
Thanks for getting back to me so quickly.

On 30/07/2019 13:41, Charles Keepax wrote:
> On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
>> Add a debugfs device node which initiates the turn-on diagnostic routine
>> feature of the TDA7802 amplifier. The four status registers (one per
>> channel) are returned.
>>
>> Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
>> ---
>> Changes since v1:
>> - Rename speaker-test to (turn-on) diagnostics
>> - Move turn-on diagnostic to debugfs as there is no standard ALSA
>>   interface for this kind of routine.
>>
>>  sound/soc/codecs/tda7802.c | 186 ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 185 insertions(+), 1 deletion(-)
>>
>> +static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
>> +		size_t update_count)
>> +{
>> +	int i, err;
>> +
>> +	for (i = 0; i < update_count; i++) {
>> +		err = regmap_update_bits(map, update[i].reg, update[i].mask,
>> +				update[i].val);
>> +		if (err < 0)
>> +			return err;
>> +	}
>> +
>> +	return i;
>> +}
> 
> This could probably be removed using regmap_multi_reg_write.
> 

The problem is that I want to retain the state of the other bits in those
registers. Maybe I should make a copy of the backed up state, set the bits
I want to off-device, then either:

1. Write the changes with regmap_multi_reg_write
2. Write all six regs again (if my device doesn't support the multi_reg)

>> +static int tda7802_probe(struct snd_soc_component *component)
>> +{
>> +	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
>> +	struct device *dev = &tda7802->i2c->dev;
>> +	int err;
>> +
>> +	tda7802->debugfs = debugfs_create_dir(dev_name(dev), NULL);
>> +	if (IS_ERR_OR_NULL(tda7802->debugfs)) {
>> +		dev_info(dev,
>> +			"Failed to create debugfs node, err %ld\n",
>> +			PTR_ERR(tda7802->debugfs));
>> +		return 0;
>> +	}
>> +
>> +	mutex_init(&tda7802->diagnostic_mutex);
>> +	err = debugfs_create_file("diagnostic", 0444, tda7802->debugfs, tda7802,
>> +			&tda7802_diagnostic_fops);
>> +	if (err < 0) {
>> +		dev_err(dev,
>> +			"debugfs: Failed to create diagnostic node, err %d\n",
>> +			err);
>> +		goto cleanup_diagnostic;
>> +	}
> 
> You shouldn't be failing the driver probe if debugfs fails, it
> should be purely optional.
> 

Got it, thanks.

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Charles Keepax]

On Tue, Jul 30, 2019 at 03:04:19PM +0100, Thomas Preston wrote:
> On 30/07/2019 13:41, Charles Keepax wrote:
> >> +static int tda7802_bulk_update(struct regmap *map, struct reg_update *update,
> >> +		size_t update_count)
> >> +{
> >> +	int i, err;
> >> +
> >> +	for (i = 0; i < update_count; i++) {
> >> +		err = regmap_update_bits(map, update[i].reg, update[i].mask,
> >> +				update[i].val);
> >> +		if (err < 0)
> >> +			return err;
> >> +	}
> >> +
> >> +	return i;
> >> +}
> > 
> > This could probably be removed using regmap_multi_reg_write.
> > 
> 
> The problem is that I want to retain the state of the other bits in those
> registers. Maybe I should make a copy of the backed up state, set the bits
> I want to off-device, then either:
> 
> 1. Write the changes with regmap_multi_reg_write
> 2. Write all six regs again (if my device doesn't support the multi_reg)
> 

Nah sorry my bad you are probably better off they way you are.

Thanks,
Charles

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Mark Brown]

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

On Tue, Jul 30, 2019 at 03:04:19PM +0100, Thomas Preston wrote:
> On 30/07/2019 13:41, Charles Keepax wrote:

> > This could probably be removed using regmap_multi_reg_write.

> The problem is that I want to retain the state of the other bits in those
> registers. Maybe I should make a copy of the backed up state, set the bits
> I want to off-device, then either:

> 1. Write the changes with regmap_multi_reg_write
> 2. Write all six regs again (if my device doesn't support the multi_reg)

Or make this a regmap function, there's nothing device specific about
it.

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

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Thomas Preston]

On 30/07/2019 15:20, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 03:04:19PM +0100, Thomas Preston wrote:
>> On 30/07/2019 13:41, Charles Keepax wrote:
> 
>>> This could probably be removed using regmap_multi_reg_write.
> 
>> The problem is that I want to retain the state of the other bits in those
>> registers. Maybe I should make a copy of the backed up state, set the bits
>> I want to off-device, then either:
> 
>> 1. Write the changes with regmap_multi_reg_write
>> 2. Write all six regs again (if my device doesn't support the multi_reg)
> 
> Or make this a regmap function, there's nothing device specific about
> it.
> 

I did wonder why regmap didn't have an multi-update function. If appropriate,
I will add this in.

Re: [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Mark Brown]

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

On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:

> +	struct dentry *debugfs;
> +	struct mutex diagnostic_mutex;
> +};

It is unclear what this mutex usefully protects, it only gets taken when
writing to the debugfs file to trigger this diagnostic mode but doesn't
do anything to control interactions with any other code path in the
driver.

> +static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status)
> +{
> +	struct device *dev = &tda7802->i2c->dev;
> +	int err_status, err;
> +	unsigned int val;
> +	u8 state[NUM_IB];

> +	/* We must wait 20ms for device to settle, otherwise diagnostics will
> +	 * not start and regmap poll will timeout.
> +	 */
> +	msleep(DIAGNOSTIC_SETTLE_MS);

The comment and define might go out of sync...

> +	err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4);
> +	if (err < 0) {
> +		dev_err(dev, "Could not read channel status, %d\n", err);
> +		goto diagnostic_restore;
> +	}

...but here we use a magic number for the array size :(

> +static int tda7802_diagnostic_show(struct seq_file *f, void *p)
> +{
> +	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);

We neither use nor free buf?

> +static int tda7802_probe(struct snd_soc_component *component)
> +{
> +	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
> +	struct device *dev = &tda7802->i2c->dev;
> +	int err;

Why is this done at the component level?

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

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Thomas Preston]

On 30/07/2019 15:19, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 01:09:37PM +0100, Thomas Preston wrote:
> 
>> +	struct dentry *debugfs;
>> +	struct mutex diagnostic_mutex;
>> +};
> 
> It is unclear what this mutex usefully protects, it only gets taken when
> writing to the debugfs file to trigger this diagnostic mode but doesn't
> do anything to control interactions with any other code path in the
> driver.
> 

If another process reads the debugfs node "diagnostic" while the turn-on 
diagnostic mode is running, this mutex prevents the second process
restarting the diagnostics.

This is redundant if debugfs reads are atomic, but I don't think they are.


>> +static int run_turn_on_diagnostic(struct tda7802_priv *tda7802, u8 *status)
>> +{
>> +	struct device *dev = &tda7802->i2c->dev;
>> +	int err_status, err;
>> +	unsigned int val;
>> +	u8 state[NUM_IB];
> 
>> +	/* We must wait 20ms for device to settle, otherwise diagnostics will
>> +	 * not start and regmap poll will timeout.
>> +	 */
>> +	msleep(DIAGNOSTIC_SETTLE_MS);
> 
> The comment and define might go out of sync...
> 

Thanks, I will remove the 20ms but keep the comment here.

>> +	err = regmap_bulk_read(tda7802->regmap, TDA7802_DB1, status, 4);
>> +	if (err < 0) {
>> +		dev_err(dev, "Could not read channel status, %d\n", err);
>> +		goto diagnostic_restore;
>> +	}
> 
> ...but here we use a magic number for the array size :(
> 

Thanks, will update.

>> +static int tda7802_diagnostic_show(struct seq_file *f, void *p)
>> +{
>> +	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> 
> We neither use nor free buf?
> 
>> +static int tda7802_probe(struct snd_soc_component *component)
>> +{
>> +	struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
>> +	struct device *dev = &tda7802->i2c->dev;
>> +	int err;
> 
> Why is this done at the component level?
> 

Argh my bad, a previous iteration required the buf and component. I missed
this, sorry for the noise.

Thanks for feedback, I'll go back and tend to all of this.

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Mark Brown]

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

On Tue, Jul 30, 2019 at 04:25:56PM +0100, Thomas Preston wrote:
> On 30/07/2019 15:19, Mark Brown wrote:

> > It is unclear what this mutex usefully protects, it only gets taken when
> > writing to the debugfs file to trigger this diagnostic mode but doesn't
> > do anything to control interactions with any other code path in the
> > driver.

> If another process reads the debugfs node "diagnostic" while the turn-on 
> diagnostic mode is running, this mutex prevents the second process
> restarting the diagnostics.

> This is redundant if debugfs reads are atomic, but I don't think they are.

Like I say it's not just debugfs though, there's the standard driver
interface too.

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

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Thomas Preston]

On 30/07/2019 16:50, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 04:25:56PM +0100, Thomas Preston wrote:
>> On 30/07/2019 15:19, Mark Brown wrote:
> 
>>> It is unclear what this mutex usefully protects, it only gets taken when
>>> writing to the debugfs file to trigger this diagnostic mode but doesn't
>>> do anything to control interactions with any other code path in the
>>> driver.
> 
>> If another process reads the debugfs node "diagnostic" while the turn-on 
>> diagnostic mode is running, this mutex prevents the second process
>> restarting the diagnostics.
> 
>> This is redundant if debugfs reads are atomic, but I don't think they are.
> 
> Like I say it's not just debugfs though, there's the standard driver
> interface too.
> 

Ah right, I understand. So if we run the turn-on diagnostics routine, there's
nothing stopping anyone from interacting with the device in other ways.

I guess there's no way to share that mutex with ALSA? In that case, it doesn't
matter if this mutex is there or not - this feature is incompatible. How
compatible do debugfs interfaces have to be? I was under the impression anything
goes. I would argue that the debugfs is better off for having the mutex so
that no one re-reads "diagnostic" within the 5s poll timeout.

Alternatively, this diagnostic feature could be handled with an external-handler
kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.

What would be acceptable?

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Charles Keepax]

On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
> On 30/07/2019 16:50, Mark Brown wrote:
> > On Tue, Jul 30, 2019 at 04:25:56PM +0100, Thomas Preston wrote:
> >> On 30/07/2019 15:19, Mark Brown wrote:
> > 
> >>> It is unclear what this mutex usefully protects, it only gets taken when
> >>> writing to the debugfs file to trigger this diagnostic mode but doesn't
> >>> do anything to control interactions with any other code path in the
> >>> driver.
> > 
> >> If another process reads the debugfs node "diagnostic" while the turn-on 
> >> diagnostic mode is running, this mutex prevents the second process
> >> restarting the diagnostics.
> > 
> >> This is redundant if debugfs reads are atomic, but I don't think they are.
> > 
> > Like I say it's not just debugfs though, there's the standard driver
> > interface too.
> > 
> 
> Ah right, I understand. So if we run the turn-on diagnostics routine, there's
> nothing stopping anyone from interacting with the device in other ways.
> 
> I guess there's no way to share that mutex with ALSA? In that case, it doesn't
> matter if this mutex is there or not - this feature is incompatible. How
> compatible do debugfs interfaces have to be? I was under the impression anything
> goes. I would argue that the debugfs is better off for having the mutex so
> that no one re-reads "diagnostic" within the 5s poll timeout.
> 
> Alternatively, this diagnostic feature could be handled with an external-handler
> kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
> 
> What would be acceptable?

You could take the DAPM mutex in your debugfs handler that would
prevent any changes to the cards power state whilst your debug
stuff is running.

Thanks,
Charles

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Mark Brown]

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

On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
> On 30/07/2019 16:50, Mark Brown wrote:

> > Like I say it's not just debugfs though, there's the standard driver
> > interface too.

> Ah right, I understand. So if we run the turn-on diagnostics routine, there's
> nothing stopping anyone from interacting with the device in other ways.

> I guess there's no way to share that mutex with ALSA? In that case, it doesn't
> matter if this mutex is there or not - this feature is incompatible. How
> compatible do debugfs interfaces have to be? I was under the impression anything
> goes. I would argue that the debugfs is better off for having the mutex so
> that no one re-reads "diagnostic" within the 5s poll timeout.

It's not really something that's supported; like Charles says the DAPM
mutex is exposed but if the regular controls would still be able to do
stuff.  It is kind of a "you broke it, you fix it" thing but on the
other hand it's better to make things safer if we can since it might not
be obvious later on why things are broken.

> Alternatively, this diagnostic feature could be handled with an external-handler
> kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
> 
> What would be acceptable?

Yes, that's definitely doable - we've got some other drivers with
similar things like calibration triggers exposed that way.

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

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Thomas Preston]

On 02/08/2019 00:42, Mark Brown wrote:
> On Tue, Jul 30, 2019 at 05:28:11PM +0100, Thomas Preston wrote:
>> On 30/07/2019 16:50, Mark Brown wrote:
> 
>>> Like I say it's not just debugfs though, there's the standard driver
>>> interface too.
> 
>> Ah right, I understand. So if we run the turn-on diagnostics routine, there's
>> nothing stopping anyone from interacting with the device in other ways.
> 
>> I guess there's no way to share that mutex with ALSA? In that case, it doesn't
>> matter if this mutex is there or not - this feature is incompatible. How
>> compatible do debugfs interfaces have to be? I was under the impression anything
>> goes. I would argue that the debugfs is better off for having the mutex so
>> that no one re-reads "diagnostic" within the 5s poll timeout.
> 
> It's not really something that's supported; like Charles says the DAPM
> mutex is exposed but if the regular controls would still be able to do
> stuff.  It is kind of a "you broke it, you fix it" thing but on the
> other hand it's better to make things safer if we can since it might not
> be obvious later on why things are broken.
> 
>> Alternatively, this diagnostic feature could be handled with an external-handler
>> kcontrol SOC_SINGLE_EXT? I'm not sure if this is an atomic interface either.
>>
>> What would be acceptable?
> 
> Yes, that's definitely doable - we've got some other drivers with
> similar things like calibration triggers exposed that way.
> 

One problem with using a kcontrol as a trigger for the turn-on diagnostic
is that the diagnostic routine has a "return value".

It goes like this:
- Bring device to low-quiescent state
- Initiate diagnostics
- Poll for diagnostics-complete bit
- Read the four channel status registers

The final read clears the status registers, so this isn't something I
can just do with regmap.

One idea I had was to initiate the turn-on diagnostics using a kcontrol,
whose handler saves the four channel status registers and an epoch in
tda7802_priv. Then this can be read from debugfs. But it seems strange
to have to turn on this control over here, then go over there and read
this value.

Hm, maybe a better idea is to have the turn on diagnostic only run on
device probe (as its name suggests!), and print something to dmesg:

	modprobe tda7802 turn_on_diagnostic=1

	tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04

Kirill Marinushkin mentioned this in the first review [0], it just didn't
really sink in until now!

[0] https://lkml.org/lkml/2019/6/14/1344

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Mark Brown]

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

On Fri, Aug 02, 2019 at 09:32:17AM +0100, Thomas Preston wrote:
> On 02/08/2019 00:42, Mark Brown wrote:

> > Yes, that's definitely doable - we've got some other drivers with
> > similar things like calibration triggers exposed that way.

> One problem with using a kcontrol as a trigger for the turn-on diagnostic
> is that the diagnostic routine has a "return value".

You can use a read only control for the readback, or just have it be
triggered by overwriting the readback value.  You can cache the result.

> Hm, maybe a better idea is to have the turn on diagnostic only run on
> device probe (as its name suggests!), and print something to dmesg:

> 	modprobe tda7802 turn_on_diagnostic=1

> 	tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04

> Kirill Marinushkin mentioned this in the first review [0], it just didn't
> really sink in until now!

You could do that too, yeah.  Depends on what this is diagnosing and if
that'd be useful.

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

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Thomas Preston]

On 02/08/2019 12:10, Mark Brown wrote:
> On Fri, Aug 02, 2019 at 09:32:17AM +0100, Thomas Preston wrote:
>> On 02/08/2019 00:42, Mark Brown wrote:
> 
>>> Yes, that's definitely doable - we've got some other drivers with
>>> similar things like calibration triggers exposed that way.
> 
>> One problem with using a kcontrol as a trigger for the turn-on diagnostic
>> is that the diagnostic routine has a "return value".
> 
> You can use a read only control for the readback, or just have it be
> triggered by overwriting the readback value.  You can cache the result.
> 

Keeping the trigger and result together like that would be better I think,
although the routine isn't supposed to run mid way through playback. If
we're mid playback the debugfs routine has to turn off AMP_ON, take the
device back to a known state, run diagnostics, then restore. Which causes
a gap in the audible sound.

>> Hm, maybe a better idea is to have the turn on diagnostic only run on
>> device probe (as its name suggests!), and print something to dmesg:
> 
>> 	modprobe tda7802 turn_on_diagnostic=1
> 
>> 	tda7802-codec i2c-TDA7802:00: Turn on diagnostic 04 04 04 04
> 
>> Kirill Marinushkin mentioned this in the first review [0], it just didn't
>> really sink in until now!
> 
> You could do that too, yeah.  Depends on what this is diagnosing and if
> that'd be useful.
> 

The diagnostic status bits describe situations such as:
- open load (no speaker connected)
- short to GND
- short to VCC
- etc

The intention is to test if all the speakers are connected. So, one might 
have a self test which runs the diagnostic and verifies it outputs:

	00 00 00 00

For example, on my test rig there is only one speaker connected. So it
reads:

	04 04 00 04

Where the second bit is "open load". So this would fail the test.

So in the kcontrol case the test would be something like:

	amixer sset "AMP1 turn on diagnostic" on
	amixer sget "AMP1 diagnostic"

And the module parameter case:

	rmmod tda7802
	modprobe tda7802 turn_on_diagnostic=1
	dmesg | grep "Turn on diagnostic 04 04 04 04"
	rmmod tda7802
	modprobe tda7802

I think the module parameter method is more appropriate for a
"Turn-on diagnostic", even though I don't really like grepping dmesg
for the result. I'll go ahead and implement that unless anyone has a
particular preference for the kcontrol-trigger.

Thanks

Re: [alsa-devel] [PATCH v2 3/3] ASoC: TDA7802: Add turn-on diagnostic routine

[Mark Brown]

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

On Fri, Aug 02, 2019 at 03:51:09PM +0100, Thomas Preston wrote:
> On 02/08/2019 12:10, Mark Brown wrote:

> > You can use a read only control for the readback, or just have it be
> > triggered by overwriting the readback value.  You can cache the result.

> Keeping the trigger and result together like that would be better I think,
> although the routine isn't supposed to run mid way through playback. If
> we're mid playback the debugfs routine has to turn off AMP_ON, take the
> device back to a known state, run diagnostics, then restore. Which causes
> a gap in the audible sound.

Whatever method is used to do the triggering can always return -EBUSY
when you someone tries to do so during playback.

> >> Kirill Marinushkin mentioned this in the first review [0], it just didn't
> >> really sink in until now!

> > You could do that too, yeah.  Depends on what this is diagnosing and if
> > that'd be useful.

> The diagnostic status bits describe situations such as:
> - open load (no speaker connected)
> - short to GND
> - short to VCC
> - etc

> The intention is to test if all the speakers are connected. So, one might 
> have a self test which runs the diagnostic and verifies it outputs:

...

> I think the module parameter method is more appropriate for a
> "Turn-on diagnostic", even though I don't really like grepping dmesg
> for the result. I'll go ahead and implement that unless anyone has a
> particular preference for the kcontrol-trigger.

Right.  It's not ideal for use in production systems for example but
perhaps fine for support techs or whoever.  Up to you anyway.

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