[PATCH 0/3] ASoc: TAS6424: Add support for mute, standby, and faster power on

[PATCH 0/3] ASoc: TAS6424: Add support for mute, standby, and faster power on

[Jean-Jacques Hiblot]

mute and standby pins are available on the codec. If they are connected, they
should be managed by the driver, instead of relying on gpio hogs or on the
initial state of the GPIOs.

This series also includes a patch to improve the start-up time of the channels
by disabling built-in DC diagnostics. Those diagnosdtics basically serve to
detect :
- wires shorted together
- wire shorted to ground or vbat
- wire disconnected
This is not useful for all platforms and the addition to the startup time is
quite noticeable (230ms).


Jean-Jacques Hiblot (3):
  ASoC: tas6424: Allow disabling auto diagnostics for faster power-on
  ASoC: tas6424: Add support for the standby pin
  ASoC: tas6424: Add support for the mute pin

 .../devicetree/bindings/sound/ti,tas6424.txt       |  4 ++
 sound/soc/codecs/tas6424.c                         | 74 +++++++++++++++++++++-
 sound/soc/codecs/tas6424.h                         |  3 +
 3 files changed, 79 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH 1/3] ASoC: tas6424: Allow disabling auto diagnostics for faster power-on

[Jean-Jacques Hiblot]

The TAS6424 incorporates both DC-load and AC-load diagnostics which are
used to determine the status of the load. The DC diagnostics runs when any
channel is directed to leave the Hi-Z state and enter the MUTE or PLAY
state.
The DC diagnostics are turned on by default but if a fast startup without
diagnostics is required the DC diagnostics can be bypassed by adding the
property "disable-auto-diagnostics".

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 .../devicetree/bindings/sound/ti,tas6424.txt       |  2 ++
 sound/soc/codecs/tas6424.c                         | 22 +++++++++++++++++++++-
 sound/soc/codecs/tas6424.h                         |  3 +++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/ti,tas6424.txt b/Documentation/devicetree/bindings/sound/ti,tas6424.txt
index 1c4ada0..bf2ff98 100644
--- a/Documentation/devicetree/bindings/sound/ti,tas6424.txt
+++ b/Documentation/devicetree/bindings/sound/ti,tas6424.txt
@@ -6,6 +6,8 @@ Required properties:
 	- compatible: "ti,tas6424" - TAS6424
 	- reg: I2C slave address
 	- sound-dai-cells: must be equal to 0
+	- disable-auto-diagnostics: disable DC auto diagnostics (faster power
+	  on, but less safe as shortage won't be detected)
 
 Example:
 
diff --git a/sound/soc/codecs/tas6424.c b/sound/soc/codecs/tas6424.c
index 4f3a16c..5cee400 100644
--- a/sound/soc/codecs/tas6424.c
+++ b/sound/soc/codecs/tas6424.c
@@ -43,6 +43,7 @@ struct tas6424_data {
 	unsigned int last_fault1;
 	unsigned int last_fault2;
 	unsigned int last_warn;
+	bool no_auto_diags;
 };
 
 /*
@@ -308,7 +309,8 @@ static int tas6424_power_on(struct snd_soc_component *component)
 	/* any time we come out of HIZ, the output channels automatically run DC
 	 * load diagnostics, wait here until this completes
 	 */
-	msleep(230);
+	if (!tas6424->no_auto_diags)
+		msleep(230);
 
 	return 0;
 }
@@ -627,6 +629,9 @@ static int tas6424_i2c_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	tas6424->no_auto_diags = of_property_read_bool(dev->of_node,
+						"disable-auto-diagnostics");
+
 	for (i = 0; i < ARRAY_SIZE(tas6424->supplies); i++)
 		tas6424->supplies[i].supply = tas6424_supply_names[i];
 	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(tas6424->supplies),
@@ -651,6 +656,21 @@ static int tas6424_i2c_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	if (tas6424->no_auto_diags) {
+		/*
+		 * Disable DC auto-diagnostics to save time when channel leave
+		 * Hi-Z state
+		 */
+		ret = regmap_update_bits(tas6424->regmap,
+					 TAS6424_DC_DIAG_CTRL1,
+					  0xff, TAS6424_LDGBYPASS);
+		if (ret) {
+			dev_err(dev, "failed to disable auto-diags: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	INIT_DELAYED_WORK(&tas6424->fault_check_work, tas6424_fault_check_work);
 
 	ret = devm_snd_soc_register_component(dev, &soc_codec_dev_tas6424,
diff --git a/sound/soc/codecs/tas6424.h b/sound/soc/codecs/tas6424.h
index 4305883..3aea0f7 100644
--- a/sound/soc/codecs/tas6424.h
+++ b/sound/soc/codecs/tas6424.h
@@ -111,6 +111,9 @@
 					 TAS6424_CH3_STATE_DIAG | \
 					 TAS6424_CH4_STATE_DIAG)
 
+/* TAS6424_DC_DIAG_CTRL1 */
+#define TAS6424_LDGBYPASS		BIT(0)
+
 /* TAS6424_GLOB_FAULT1_REG */
 #define TAS6424_FAULT_CLOCK		BIT(4)
 #define TAS6424_FAULT_PVDD_OV		BIT(3)
-- 
2.7.4

Re: [PATCH 1/3] ASoC: tas6424: Allow disabling auto diagnostics for faster power-on

[Mark Brown]

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

On Fri, Apr 20, 2018 at 12:04:42PM +0200, Jean-Jacques Hiblot wrote:
> The TAS6424 incorporates both DC-load and AC-load diagnostics which are
> used to determine the status of the load. The DC diagnostics runs when any
> channel is directed to leave the Hi-Z state and enter the MUTE or PLAY
> state.
> The DC diagnostics are turned on by default but if a fast startup without
> diagnostics is required the DC diagnostics can be bypassed by adding the
> property "disable-auto-diagnostics".

This is making me think we should be smarter here and either only run
the diagnostics once during boot or provide a user control for the
diagnostics.  It seems like something that is more of a runtime software
decision than something that's fixed in hardware design, is there
anything about the hardware design that'd make it impossible to run
diagnostics?

> +	if (tas6424->no_auto_diags) {
> +		/*
> +		 * Disable DC auto-diagnostics to save time when channel leave
> +		 * Hi-Z state
> +		 */
> +		ret = regmap_update_bits(tas6424->regmap,
> +					 TAS6424_DC_DIAG_CTRL1,
> +					  0xff, TAS6424_LDGBYPASS);

This could just be exposed to userspace as a simple switch control
couldn't it?  I do note that it masks more bits than it sets though...

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

Re: [PATCH 1/3] ASoC: tas6424: Allow disabling auto diagnostics for faster power-on

[Jean-Jacques Hiblot]

On 23/04/2018 17:44, Mark Brown wrote:
> On Fri, Apr 20, 2018 at 12:04:42PM +0200, Jean-Jacques Hiblot wrote:
>> The TAS6424 incorporates both DC-load and AC-load diagnostics which are
>> used to determine the status of the load. The DC diagnostics runs when any
>> channel is directed to leave the Hi-Z state and enter the MUTE or PLAY
>> state.
>> The DC diagnostics are turned on by default but if a fast startup without
>> diagnostics is required the DC diagnostics can be bypassed by adding the
>> property "disable-auto-diagnostics".
> This is making me think we should be smarter here and either only run
> the diagnostics once during boot or provide a user control for the
> diagnostics.  It seems like something that is more of a runtime software
> decision than something that's fixed in hardware design, is there
> anything about the hardware design that'd make it impossible to run
> diagnostics?
I can't think of anything that would make it unusable.
This auto diagnostic is really useful in cars where wires can get 
disconnected or shorted to ground.
In quieter environment it may not be as useful, and a test at startup 
may be sufficient.
Diagnostics can also be triggered at will.
Do you think a sysfs interface would be better suited to disable the 
autodiag ?

Thanks for your comment on the series.

JJ

>
>> +	if (tas6424->no_auto_diags) {
>> +		/*
>> +		 * Disable DC auto-diagnostics to save time when channel leave
>> +		 * Hi-Z state
>> +		 */
>> +		ret = regmap_update_bits(tas6424->regmap,
>> +					 TAS6424_DC_DIAG_CTRL1,
>> +					  0xff, TAS6424_LDGBYPASS);
> This could just be exposed to userspace as a simple switch control
> couldn't it?  I do note that it masks more bits than it sets though...

Re: [PATCH 1/3] ASoC: tas6424: Allow disabling auto diagnostics for faster power-on

[Mark Brown]

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

On Mon, Apr 23, 2018 at 09:00:18PM +0200, Jean-Jacques Hiblot wrote:

> I can't think of anything that would make it unusable.
> This auto diagnostic is really useful in cars where wires can get
> disconnected or shorted to ground.
> In quieter environment it may not be as useful, and a test at startup may be
> sufficient.
> Diagnostics can also be triggered at will.
> Do you think a sysfs interface would be better suited to disable the
> autodiag ?

I'd just make it an ALSA control TBH.  sysfs should work as well though.

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

[PATCH 2/3] ASoC: tas6424: Add support for the standby pin

[Jean-Jacques Hiblot]

The standby pin can be connected to a GPIO. In that case we have to drive
it to the correct values for the TAS6424 to operate properly.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 .../devicetree/bindings/sound/ti,tas6424.txt       |  1 +
 sound/soc/codecs/tas6424.c                         | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/ti,tas6424.txt b/Documentation/devicetree/bindings/sound/ti,tas6424.txt
index bf2ff98..82c6d48 100644
--- a/Documentation/devicetree/bindings/sound/ti,tas6424.txt
+++ b/Documentation/devicetree/bindings/sound/ti,tas6424.txt
@@ -8,6 +8,7 @@ Required properties:
 	- sound-dai-cells: must be equal to 0
 	- disable-auto-diagnostics: disable DC auto diagnostics (faster power
 	  on, but less safe as shortage won't be detected)
+	- standby-gpio: GPIO used to shut the TAS6424 down.
 
 Example:
 
diff --git a/sound/soc/codecs/tas6424.c b/sound/soc/codecs/tas6424.c
index 5cee400..926259a 100644
--- a/sound/soc/codecs/tas6424.c
+++ b/sound/soc/codecs/tas6424.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/regulator/consumer.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -44,6 +45,7 @@ struct tas6424_data {
 	unsigned int last_fault2;
 	unsigned int last_warn;
 	bool no_auto_diags;
+	struct gpio_desc *standby_gpio;
 };
 
 /*
@@ -632,6 +634,22 @@ static int tas6424_i2c_probe(struct i2c_client *client,
 	tas6424->no_auto_diags = of_property_read_bool(dev->of_node,
 						"disable-auto-diagnostics");
 
+	/*
+	 * Get control of the standby pin and set it LOW to take the codec
+	 * out of the stand-by mode.
+	 * Note: The actual pin polarity is taken care of in the GPIO lib
+	 * according the polarity specified in the DTS.
+	 */
+	tas6424->standby_gpio = devm_gpiod_get_optional(dev, "standby",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(tas6424->standby_gpio)) {
+		if (PTR_ERR(tas6424->standby_gpio) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_info(dev, "failed to get standvby GPIO: %ld\n",
+			PTR_ERR(tas6424->standby_gpio));
+		tas6424->standby_gpio = NULL;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(tas6424->supplies); i++)
 		tas6424->supplies[i].supply = tas6424_supply_names[i];
 	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(tas6424->supplies),
@@ -691,6 +709,10 @@ static int tas6424_i2c_remove(struct i2c_client *client)
 
 	cancel_delayed_work_sync(&tas6424->fault_check_work);
 
+	/* put the codec in stand-by */
+	if (tas6424->standby_gpio)
+		gpiod_set_value_cansleep(tas6424->standby_gpio, 1);
+
 	ret = regulator_bulk_disable(ARRAY_SIZE(tas6424->supplies),
 				     tas6424->supplies);
 	if (ret < 0) {
-- 
2.7.4

Re: [PATCH 2/3] ASoC: tas6424: Add support for the standby pin

[Mark Brown]

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

On Fri, Apr 20, 2018 at 12:04:43PM +0200, Jean-Jacques Hiblot wrote:

> +	- standby-gpio: GPIO used to shut the TAS6424 down.

All GPIO property names are supposed to be called -gpios regardless of
if there can ever be more than one GPIO (though the framework does
support the singular, I wonder if it's not more helpful to just get the
spec relaxed here).

> +		dev_info(dev, "failed to get standvby GPIO: %ld\n",

Typo.

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

[PATCH 3/3] ASoC: tas6424: Add support for the mute pin

[Jean-Jacques Hiblot]

mute can be connected to GPIO. In that case we have to drive it to the
correct value

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 .../devicetree/bindings/sound/ti,tas6424.txt       |  1 +
 sound/soc/codecs/tas6424.c                         | 37 +++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/ti,tas6424.txt b/Documentation/devicetree/bindings/sound/ti,tas6424.txt
index 82c6d48..527e356 100644
--- a/Documentation/devicetree/bindings/sound/ti,tas6424.txt
+++ b/Documentation/devicetree/bindings/sound/ti,tas6424.txt
@@ -9,6 +9,7 @@ Required properties:
 	- disable-auto-diagnostics: disable DC auto diagnostics (faster power
 	  on, but less safe as shortage won't be detected)
 	- standby-gpio: GPIO used to shut the TAS6424 down.
+	- mute-gpio: GPIO used to mute all the outputs
 
 Example:
 
diff --git a/sound/soc/codecs/tas6424.c b/sound/soc/codecs/tas6424.c
index 926259a..cf84b1c 100644
--- a/sound/soc/codecs/tas6424.c
+++ b/sound/soc/codecs/tas6424.c
@@ -46,6 +46,7 @@ struct tas6424_data {
 	unsigned int last_warn;
 	bool no_auto_diags;
 	struct gpio_desc *standby_gpio;
+	struct gpio_desc *mute_gpio;
 };
 
 /*
@@ -252,10 +253,16 @@ static int tas6424_set_dai_tdm_slot(struct snd_soc_dai *dai,
 static int tas6424_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct snd_soc_component *component = dai->component;
+	struct tas6424_data *tas6424 = snd_soc_component_get_drvdata(component);
 	unsigned int val;
 
 	dev_dbg(component->dev, "%s() mute=%d\n", __func__, mute);
 
+	if (tas6424->mute_gpio) {
+		gpiod_set_value_cansleep(tas6424->mute_gpio, mute ? 1 : 0);
+		return 0;
+	}
+
 	if (mute)
 		val = TAS6424_ALL_STATE_MUTE;
 	else
@@ -290,6 +297,7 @@ static int tas6424_power_on(struct snd_soc_component *component)
 {
 	struct tas6424_data *tas6424 = snd_soc_component_get_drvdata(component);
 	int ret;
+	u8 chan_states;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(tas6424->supplies),
 				    tas6424->supplies);
@@ -306,7 +314,18 @@ static int tas6424_power_on(struct snd_soc_component *component)
 		return ret;
 	}
 
-	snd_soc_component_write(component, TAS6424_CH_STATE_CTRL, TAS6424_ALL_STATE_MUTE);
+	if (tas6424->mute_gpio) {
+		gpiod_set_value_cansleep(tas6424->mute_gpio, 0);
+		/*
+		 * channels are muted via the mute pin, don't also. Don't also
+		 * mute them via the registers so that subsequent register
+		 * access is not necessary to un-mute the channels
+		 */
+		chan_states = TAS6424_ALL_STATE_PLAY;
+	} else {
+		chan_states = TAS6424_ALL_STATE_MUTE;
+	}
+	snd_soc_component_write(component, TAS6424_CH_STATE_CTRL, chan_states);
 
 	/* any time we come out of HIZ, the output channels automatically run DC
 	 * load diagnostics, wait here until this completes
@@ -650,6 +669,22 @@ static int tas6424_i2c_probe(struct i2c_client *client,
 		tas6424->standby_gpio = NULL;
 	}
 
+	/*
+	 * Get control of the mute pin and set it HIGH in order to start with
+	 * all the output muted.
+	 * Note: The actual pin polarity is taken care of in the GPIO lib
+	 * according the polarity specified in the DTS.
+	 */
+	tas6424->mute_gpio = devm_gpiod_get_optional(dev, "mute",
+						      GPIOD_OUT_HIGH);
+	if (IS_ERR(tas6424->mute_gpio)) {
+		if (PTR_ERR(tas6424->mute_gpio) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_info(dev, "failed to get nmute GPIO: %ld\n",
+			PTR_ERR(tas6424->mute_gpio));
+		tas6424->mute_gpio = NULL;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(tas6424->supplies); i++)
 		tas6424->supplies[i].supply = tas6424_supply_names[i];
 	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(tas6424->supplies),
-- 
2.7.4

Re: [PATCH 3/3] ASoC: tas6424: Add support for the mute pin

[Mark Brown]

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

On Fri, Apr 20, 2018 at 12:04:44PM +0200, Jean-Jacques Hiblot wrote:

> +	- mute-gpio: GPIO used to mute all the outputs

Same thing with the plural here.

> +	if (tas6424->mute_gpio) {
> +		gpiod_set_value_cansleep(tas6424->mute_gpio, mute ? 1 : 0);
> +		return 0;
> +	}

Just use mute directly, the ternery operator is doing nothing for
legibility here and C does the integer to boolean thing for you.

> +	if (tas6424->mute_gpio) {
> +		gpiod_set_value_cansleep(tas6424->mute_gpio, 0);
> +		/*
> +		 * channels are muted via the mute pin, don't also. Don't also
> +		 * mute them via the registers so that subsequent register
> +		 * access is not necessary to un-mute the channels

Extra "don't also" in there.

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