(Copying in some TI addresses which may be interested.)
> On 2. 12. 2022, at 12:58, James Calligeros <jcalligeros99@xxxxxxxxx> 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@xxxxxxxxxxx>
> Signed-off-by: James Calligeros <jcalligeros99@xxxxxxxxx>
> ---
> 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
>
