[alsa-devel] [PATCH 0/3] ASoc: TAS6424: Add support for mute, standby, and faster power on
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(-)
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)
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...
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...
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.
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) {
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.
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),
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.
participants (2)
-
Jean-Jacques Hiblot -
Mark Brown