[PATCH] ASoC: tas27{64,70}: fix PM ops ordering

[PATCH] ASoC: tas27{64,70}: fix PM ops ordering

[James Calligeros]

On both tas2764 and tas2770, a write to PWR_CTRL is attempted
on resume before syncing the regcache to the chip, potentially leaving
it in an undefined state that causes resume to fail. The codec
is then unavailable until the next system reset.

On tas2770 specifically, both suspend and resume ops attempt useless
register writes on unrestored registers. This causes its state to be
desynchronised from what ASoC expects it to be in.

These two codecs are almost identical, so unify their behaviour
and reorder the ops so that the codec is always suspended and
resumed in a known/expected state.

Suggested-by: Martin Povišer <povik+lin@cutebit.org>
Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
---
 sound/soc/codecs/tas2764.c | 11 +++++++----
 sound/soc/codecs/tas2770.c | 40 ++++++++++++++++++++------------------
 2 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/sound/soc/codecs/tas2764.c b/sound/soc/codecs/tas2764.c
index 2e0ed3e68fa5..51c6b3a940c4 100644
--- a/sound/soc/codecs/tas2764.c
+++ b/sound/soc/codecs/tas2764.c
@@ -32,7 +32,7 @@ struct tas2764_priv {
 	struct regmap *regmap;
 	struct device *dev;
 	int irq;
-	
+
 	int v_sense_slot;
 	int i_sense_slot;
 
@@ -157,14 +157,17 @@ static int tas2764_codec_resume(struct snd_soc_component *component)
 		usleep_range(1000, 2000);
 	}
 
-	ret = tas2764_update_pwr_ctrl(tas2764);
+	regcache_cache_only(tas2764->regmap, false);
 
+	ret = regcache_sync(tas2764->regmap);
 	if (ret < 0)
 		return ret;
 
-	regcache_cache_only(tas2764->regmap, false);
+	ret = tas2764_update_pwr_ctrl(tas2764);
+	if (ret < 0)
+		return ret;
 
-	return regcache_sync(tas2764->regmap);
+	return 0;
 }
 #else
 #define tas2764_codec_suspend NULL
diff --git a/sound/soc/codecs/tas2770.c b/sound/soc/codecs/tas2770.c
index 8557759acb1f..5c9e8419b387 100644
--- a/sound/soc/codecs/tas2770.c
+++ b/sound/soc/codecs/tas2770.c
@@ -72,25 +72,21 @@ static int tas2770_codec_suspend(struct snd_soc_component *component)
 	struct tas2770_priv *tas2770 = snd_soc_component_get_drvdata(component);
 	int ret = 0;
 
+	ret = snd_soc_component_update_bits(component, TAS2770_PWR_CTRL,
+						TAS2770_PWR_CTRL_MASK,
+						TAS2770_PWR_CTRL_SHUTDOWN);
+
+	if (ret < 0)
+		return ret;
+
 	regcache_cache_only(tas2770->regmap, true);
-	regcache_mark_dirty(tas2770->regmap);
+	regcache_sync(tas2770->regmap);
 
-	if (tas2770->sdz_gpio) {
+	if (tas2770->sdz_gpio)
 		gpiod_set_value_cansleep(tas2770->sdz_gpio, 0);
-	} else {
-		ret = snd_soc_component_update_bits(component, TAS2770_PWR_CTRL,
-						    TAS2770_PWR_CTRL_MASK,
-						    TAS2770_PWR_CTRL_SHUTDOWN);
-		if (ret < 0) {
-			regcache_cache_only(tas2770->regmap, false);
-			regcache_sync(tas2770->regmap);
-			return ret;
-		}
 
-		ret = 0;
-	}
 
-	return ret;
+	return 0;
 }
 
 static int tas2770_codec_resume(struct snd_soc_component *component)
@@ -98,18 +94,24 @@ static int tas2770_codec_resume(struct snd_soc_component *component)
 	struct tas2770_priv *tas2770 = snd_soc_component_get_drvdata(component);
 	int ret;
 
+
 	if (tas2770->sdz_gpio) {
 		gpiod_set_value_cansleep(tas2770->sdz_gpio, 1);
 		usleep_range(1000, 2000);
-	} else {
-		ret = tas2770_update_pwr_ctrl(tas2770);
-		if (ret < 0)
-			return ret;
 	}
 
 	regcache_cache_only(tas2770->regmap, false);
 
-	return regcache_sync(tas2770->regmap);
+	ret = regcache_sync(tas2770->regmap);
+	if (ret < 0)
+		return ret;
+
+	ret = tas2770_update_pwr_ctrl(tas2770);
+	if (ret < 0)
+		return ret;
+
+
+	return 0;
 }
 #else
 #define tas2770_codec_suspend NULL
-- 
2.38.1

Re: [PATCH] ASoC: tas27{64,70}: fix PM ops ordering

[Martin Povišer]

(Copying in some TI addresses which may be interested.)

> On 2. 12. 2022, at 12:58, James Calligeros <jcalligeros99@gmail.com> wrote:
> 
> On both tas2764 and tas2770, a write to PWR_CTRL is attempted
> on resume before syncing the regcache to the chip, potentially leaving
> it in an undefined state that causes resume to fail. The codec
> is then unavailable until the next system reset.

I think we need to split this into separate tas2764 and tas2770 changes.
So, concentrating on tas2764 first:

The issue here isn’t that a write is attempted before the device is synced
and while the regcache is in cache-only state. That’s on its own OK.
The issue here is that all registers including PWR_CTRL are restored in
one go, and that can cause issues since we need the device properly
configured before raising its power state.

> On tas2770 specifically, both suspend and resume ops attempt useless
> register writes on unrestored registers. This causes its state to be
> desynchronised from what ASoC expects it to be in.
> 
> These two codecs are almost identical, so unify their behaviour
> and reorder the ops so that the codec is always suspended and
> resumed in a known/expected state.

I suggest we make the first commit fix up tas2764 suspend/resume
code to a state that’s OK, then second commit copies that over
to tas2770 to replace what’s there now. (Pointing out some of the
things that’s wrong with the old code.)

> Suggested-by: Martin Povišer <povik+lin@cutebit.org>
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
> ---
> sound/soc/codecs/tas2764.c | 11 +++++++----
> sound/soc/codecs/tas2770.c | 40 ++++++++++++++++++++------------------
> 2 files changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/sound/soc/codecs/tas2764.c b/sound/soc/codecs/tas2764.c
> index 2e0ed3e68fa5..51c6b3a940c4 100644
> --- a/sound/soc/codecs/tas2764.c
> +++ b/sound/soc/codecs/tas2764.c
> @@ -32,7 +32,7 @@ struct tas2764_priv {
> 	struct regmap *regmap;
> 	struct device *dev;
> 	int irq;
> -	
> +

Stray whiteline change here

> 	int v_sense_slot;
> 	int i_sense_slot;
> 
> @@ -157,14 +157,17 @@ static int tas2764_codec_resume(struct snd_soc_component *component)
> 		usleep_range(1000, 2000);
> 	}
> 
> -	ret = tas2764_update_pwr_ctrl(tas2764);
> +	regcache_cache_only(tas2764->regmap, false);
> 
> +	ret = regcache_sync(tas2764->regmap);
> 	if (ret < 0)
> 		return ret;
> 
> -	regcache_cache_only(tas2764->regmap, false);
> +	ret = tas2764_update_pwr_ctrl(tas2764);
> +	if (ret < 0)
> +		return ret;
> 
> -	return regcache_sync(tas2764->regmap);
> +	return 0;
> }
> #else
> #define tas2764_codec_suspend NULL
> diff --git a/sound/soc/codecs/tas2770.c b/sound/soc/codecs/tas2770.c
> index 8557759acb1f..5c9e8419b387 100644
> --- a/sound/soc/codecs/tas2770.c
> +++ b/sound/soc/codecs/tas2770.c
> @@ -72,25 +72,21 @@ static int tas2770_codec_suspend(struct snd_soc_component *component)
> 	struct tas2770_priv *tas2770 = snd_soc_component_get_drvdata(component);
> 	int ret = 0;
> 
> +	ret = snd_soc_component_update_bits(component, TAS2770_PWR_CTRL,
> +						TAS2770_PWR_CTRL_MASK,
> +						TAS2770_PWR_CTRL_SHUTDOWN);
> +
> +	if (ret < 0)
> +		return ret;
> +
> 	regcache_cache_only(tas2770->regmap, true);
> -	regcache_mark_dirty(tas2770->regmap);
> +	regcache_sync(tas2770->regmap);
> 
> -	if (tas2770->sdz_gpio) {
> +	if (tas2770->sdz_gpio)
> 		gpiod_set_value_cansleep(tas2770->sdz_gpio, 0);
> -	} else {
> -		ret = snd_soc_component_update_bits(component, TAS2770_PWR_CTRL,
> -						    TAS2770_PWR_CTRL_MASK,
> -						    TAS2770_PWR_CTRL_SHUTDOWN);
> -		if (ret < 0) {
> -			regcache_cache_only(tas2770->regmap, false);
> -			regcache_sync(tas2770->regmap);
> -			return ret;
> -		}
> 
> -		ret = 0;
> -	}
> 
> -	return ret;
> +	return 0;
> }
> 
> static int tas2770_codec_resume(struct snd_soc_component *component)
> @@ -98,18 +94,24 @@ static int tas2770_codec_resume(struct snd_soc_component *component)
> 	struct tas2770_priv *tas2770 = snd_soc_component_get_drvdata(component);
> 	int ret;
> 
> +
> 	if (tas2770->sdz_gpio) {
> 		gpiod_set_value_cansleep(tas2770->sdz_gpio, 1);
> 		usleep_range(1000, 2000);
> -	} else {
> -		ret = tas2770_update_pwr_ctrl(tas2770);
> -		if (ret < 0)
> -			return ret;
> 	}
> 
> 	regcache_cache_only(tas2770->regmap, false);
> 
> -	return regcache_sync(tas2770->regmap);
> +	ret = regcache_sync(tas2770->regmap);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tas2770_update_pwr_ctrl(tas2770);
> +	if (ret < 0)
> +		return ret;
> +
> +
> +	return 0;
> }
> #else
> #define tas2770_codec_suspend NULL
> -- 
> 2.38.1
>