[PATCH 0/2] Untested TAS2562 power setting fixes

[PATCH 0/2] Untested TAS2562 power setting fixes

[Martin Povišer]

The tas2562 driver does the same thing with the setting of PWR_CTRL
field as the tas2764/tas2770 drivers were doing.
Link: https://lore.kernel.org/alsa-devel/20220808141246.5749-1-povik+lin@cutebit.org/T/#t
Link: https://lore.kernel.org/alsa-devel/20220825140241.53963-1-povik+lin@cutebit.org/T/#t

These are blindly written patches without testing since I don't have
the hardware. (I even tried TI's formal sample request program but
was refused there. CCing @ti.com addresses I found on other series
recently submitted.)

Martin Povišer (2):
  ASoC: tas2562: Drop conflicting set_bias_level power setting
  ASoC: tas2562: Fix mute/unmute

 sound/soc/codecs/tas2562.c | 90 ++++++++++++--------------------------
 1 file changed, 29 insertions(+), 61 deletions(-)

-- 
2.33.0

[PATCH 0/2] Untested TAS2562 power setting fixes

[Martin Povišer]

The tas2562 driver does the same thing with the setting of PWR_CTRL
field as the tas2764/tas2770 drivers were doing.
Link: https://lore.kernel.org/alsa-devel/20220808141246.5749-1-povik+lin@cutebit.org/T/#t
Link: https://lore.kernel.org/alsa-devel/20220825140241.53963-1-povik+lin@cutebit.org/T/#t

These are blindly written patches without testing since I don't have
the hardware. (I even tried TI's formal sample request program but
was refused there. CCing @ti.com addresses I found on other series
recently submitted.)

Martin Povišer (2):
  ASoC: tas2562: Drop conflicting set_bias_level power setting
  ASoC: tas2562: Fix mute/unmute

 sound/soc/codecs/tas2562.c | 90 ++++++++++++--------------------------
 1 file changed, 29 insertions(+), 61 deletions(-)

-- 
2.33.0

[PATCH 1/2] ASoC: tas2562: Drop conflicting set_bias_level power setting

[Martin Povišer]

The driver is setting the PWR_CTRL field in both the set_bias_level
callback and on DAPM events of the DAC widget (and also in the
mute_stream method). Drop the set_bias_level callback altogether as the
power setting it does is in conflict with the other code paths.

(This mirrors commit c8a6ae3fe1c8 ("ASoC: tas2770: Drop conflicting
set_bias_level power setting") which was a fix to the tas2770 driver.)

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 sound/soc/codecs/tas2562.c | 35 -----------------------------------
 1 file changed, 35 deletions(-)

diff --git a/sound/soc/codecs/tas2562.c b/sound/soc/codecs/tas2562.c
index dc088a1c6721..2b0cdb6d1600 100644
--- a/sound/soc/codecs/tas2562.c
+++ b/sound/soc/codecs/tas2562.c
@@ -63,39 +63,6 @@ enum tas256x_model {
 	TAS2110,
 };
 
-static int tas2562_set_bias_level(struct snd_soc_component *component,
-				 enum snd_soc_bias_level level)
-{
-	struct tas2562_data *tas2562 =
-			snd_soc_component_get_drvdata(component);
-
-	switch (level) {
-	case SND_SOC_BIAS_ON:
-		snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK, TAS2562_ACTIVE);
-		break;
-	case SND_SOC_BIAS_STANDBY:
-	case SND_SOC_BIAS_PREPARE:
-		snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK, TAS2562_MUTE);
-		break;
-	case SND_SOC_BIAS_OFF:
-		snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK, TAS2562_SHUTDOWN);
-		break;
-
-	default:
-		dev_err(tas2562->dev,
-				"wrong power level setting %d\n", level);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int tas2562_set_samplerate(struct tas2562_data *tas2562, int samplerate)
 {
 	int samp_rate;
@@ -579,7 +546,6 @@ static const struct snd_soc_component_driver soc_component_dev_tas2110 = {
 	.probe			= tas2562_codec_probe,
 	.suspend		= tas2562_suspend,
 	.resume			= tas2562_resume,
-	.set_bias_level		= tas2562_set_bias_level,
 	.controls		= tas2562_snd_controls,
 	.num_controls		= ARRAY_SIZE(tas2562_snd_controls),
 	.dapm_widgets		= tas2110_dapm_widgets,
@@ -618,7 +584,6 @@ static const struct snd_soc_component_driver soc_component_dev_tas2562 = {
 	.probe			= tas2562_codec_probe,
 	.suspend		= tas2562_suspend,
 	.resume			= tas2562_resume,
-	.set_bias_level		= tas2562_set_bias_level,
 	.controls		= tas2562_snd_controls,
 	.num_controls		= ARRAY_SIZE(tas2562_snd_controls),
 	.dapm_widgets		= tas2562_dapm_widgets,
-- 
2.33.0

[PATCH 1/2] ASoC: tas2562: Drop conflicting set_bias_level power setting

[Martin Povišer]

The driver is setting the PWR_CTRL field in both the set_bias_level
callback and on DAPM events of the DAC widget (and also in the
mute_stream method). Drop the set_bias_level callback altogether as the
power setting it does is in conflict with the other code paths.

(This mirrors commit c8a6ae3fe1c8 ("ASoC: tas2770: Drop conflicting
set_bias_level power setting") which was a fix to the tas2770 driver.)

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 sound/soc/codecs/tas2562.c | 35 -----------------------------------
 1 file changed, 35 deletions(-)

diff --git a/sound/soc/codecs/tas2562.c b/sound/soc/codecs/tas2562.c
index dc088a1c6721..2b0cdb6d1600 100644
--- a/sound/soc/codecs/tas2562.c
+++ b/sound/soc/codecs/tas2562.c
@@ -63,39 +63,6 @@ enum tas256x_model {
 	TAS2110,
 };
 
-static int tas2562_set_bias_level(struct snd_soc_component *component,
-				 enum snd_soc_bias_level level)
-{
-	struct tas2562_data *tas2562 =
-			snd_soc_component_get_drvdata(component);
-
-	switch (level) {
-	case SND_SOC_BIAS_ON:
-		snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK, TAS2562_ACTIVE);
-		break;
-	case SND_SOC_BIAS_STANDBY:
-	case SND_SOC_BIAS_PREPARE:
-		snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK, TAS2562_MUTE);
-		break;
-	case SND_SOC_BIAS_OFF:
-		snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK, TAS2562_SHUTDOWN);
-		break;
-
-	default:
-		dev_err(tas2562->dev,
-				"wrong power level setting %d\n", level);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int tas2562_set_samplerate(struct tas2562_data *tas2562, int samplerate)
 {
 	int samp_rate;
@@ -579,7 +546,6 @@ static const struct snd_soc_component_driver soc_component_dev_tas2110 = {
 	.probe			= tas2562_codec_probe,
 	.suspend		= tas2562_suspend,
 	.resume			= tas2562_resume,
-	.set_bias_level		= tas2562_set_bias_level,
 	.controls		= tas2562_snd_controls,
 	.num_controls		= ARRAY_SIZE(tas2562_snd_controls),
 	.dapm_widgets		= tas2110_dapm_widgets,
@@ -618,7 +584,6 @@ static const struct snd_soc_component_driver soc_component_dev_tas2562 = {
 	.probe			= tas2562_codec_probe,
 	.suspend		= tas2562_suspend,
 	.resume			= tas2562_resume,
-	.set_bias_level		= tas2562_set_bias_level,
 	.controls		= tas2562_snd_controls,
 	.num_controls		= ARRAY_SIZE(tas2562_snd_controls),
 	.dapm_widgets		= tas2562_dapm_widgets,
-- 
2.33.0

[PATCH 2/2] ASoC: tas2562: Fix mute/unmute

[Martin Povišer]

Because the PWR_CTRL field is modeled as the power state of the DAC
widget, and at the same time it is used to implement mute/unmute, we
need some additional book-keeping to have the right end result no matter
the sequence of calls. Without this fix, one permanently mutes an
ongoing stream by toggling the associated speaker pin control.

(This mirrors commit 1e5907bcb3a3 ("ASoC: tas2770: Fix handling of
mute/unmute") which was a fix to the tas2770 driver.)

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 sound/soc/codecs/tas2562.c | 55 ++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/sound/soc/codecs/tas2562.c b/sound/soc/codecs/tas2562.c
index 2b0cdb6d1600..66149055aba9 100644
--- a/sound/soc/codecs/tas2562.c
+++ b/sound/soc/codecs/tas2562.c
@@ -54,6 +54,8 @@ struct tas2562_data {
 	int i_sense_slot;
 	int volume_lvl;
 	int model_id;
+	bool dac_powered;
+	bool unmuted;
 };
 
 enum tas256x_model {
@@ -351,30 +353,43 @@ static int tas2562_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	return 0;
 }
 
+static int tas2562_update_pwr_ctrl(struct tas2562_data *tas2562)
+{
+	struct snd_soc_component *component = tas2562->component;
+	unsigned int val;
+	int ret;
+
+	if (tas2562->dac_powered)
+		val = tas2562->unmuted ?
+			TAS2562_ACTIVE : TAS2562_MUTE;
+	else
+		val = TAS2562_SHUTDOWN;
+
+	ret = snd_soc_component_update_bits(component, TAS2562_PWR_CTRL,
+					    TAS2562_MODE_MASK, val);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int tas2562_mute(struct snd_soc_dai *dai, int mute, int direction)
 {
-	struct snd_soc_component *component = dai->component;
+	struct tas2562_data *tas2562 = snd_soc_component_get_drvdata(dai->component);
 
-	return snd_soc_component_update_bits(component, TAS2562_PWR_CTRL,
-					     TAS2562_MODE_MASK,
-					     mute ? TAS2562_MUTE : 0);
+	tas2562->unmuted = !mute;
+	return tas2562_update_pwr_ctrl(tas2562);
 }
 
 static int tas2562_codec_probe(struct snd_soc_component *component)
 {
 	struct tas2562_data *tas2562 = snd_soc_component_get_drvdata(component);
-	int ret;
 
 	tas2562->component = component;
 
 	if (tas2562->sdz_gpio)
 		gpiod_set_value_cansleep(tas2562->sdz_gpio, 1);
 
-	ret = snd_soc_component_update_bits(component, TAS2562_PWR_CTRL,
-					    TAS2562_MODE_MASK, TAS2562_MUTE);
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
@@ -428,30 +443,18 @@ static int tas2562_dac_event(struct snd_soc_dapm_widget *w,
 
 	switch (event) {
 	case SND_SOC_DAPM_POST_PMU:
-		ret = snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK,
-			TAS2562_MUTE);
-		if (ret)
-			goto end;
+		tas2562->dac_powered = true;
+		ret = tas2562_update_pwr_ctrl(tas2562);
 		break;
 	case SND_SOC_DAPM_PRE_PMD:
-		ret = snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK,
-			TAS2562_SHUTDOWN);
-		if (ret)
-			goto end;
+		tas2562->dac_powered = false;
+		ret = tas2562_update_pwr_ctrl(tas2562);
 		break;
 	default:
 		dev_err(tas2562->dev, "Not supported evevt\n");
 		return -EINVAL;
 	}
 
-end:
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
-- 
2.33.0

[PATCH 2/2] ASoC: tas2562: Fix mute/unmute

[Martin Povišer]

Because the PWR_CTRL field is modeled as the power state of the DAC
widget, and at the same time it is used to implement mute/unmute, we
need some additional book-keeping to have the right end result no matter
the sequence of calls. Without this fix, one permanently mutes an
ongoing stream by toggling the associated speaker pin control.

(This mirrors commit 1e5907bcb3a3 ("ASoC: tas2770: Fix handling of
mute/unmute") which was a fix to the tas2770 driver.)

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 sound/soc/codecs/tas2562.c | 55 ++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/sound/soc/codecs/tas2562.c b/sound/soc/codecs/tas2562.c
index 2b0cdb6d1600..66149055aba9 100644
--- a/sound/soc/codecs/tas2562.c
+++ b/sound/soc/codecs/tas2562.c
@@ -54,6 +54,8 @@ struct tas2562_data {
 	int i_sense_slot;
 	int volume_lvl;
 	int model_id;
+	bool dac_powered;
+	bool unmuted;
 };
 
 enum tas256x_model {
@@ -351,30 +353,43 @@ static int tas2562_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	return 0;
 }
 
+static int tas2562_update_pwr_ctrl(struct tas2562_data *tas2562)
+{
+	struct snd_soc_component *component = tas2562->component;
+	unsigned int val;
+	int ret;
+
+	if (tas2562->dac_powered)
+		val = tas2562->unmuted ?
+			TAS2562_ACTIVE : TAS2562_MUTE;
+	else
+		val = TAS2562_SHUTDOWN;
+
+	ret = snd_soc_component_update_bits(component, TAS2562_PWR_CTRL,
+					    TAS2562_MODE_MASK, val);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int tas2562_mute(struct snd_soc_dai *dai, int mute, int direction)
 {
-	struct snd_soc_component *component = dai->component;
+	struct tas2562_data *tas2562 = snd_soc_component_get_drvdata(dai->component);
 
-	return snd_soc_component_update_bits(component, TAS2562_PWR_CTRL,
-					     TAS2562_MODE_MASK,
-					     mute ? TAS2562_MUTE : 0);
+	tas2562->unmuted = !mute;
+	return tas2562_update_pwr_ctrl(tas2562);
 }
 
 static int tas2562_codec_probe(struct snd_soc_component *component)
 {
 	struct tas2562_data *tas2562 = snd_soc_component_get_drvdata(component);
-	int ret;
 
 	tas2562->component = component;
 
 	if (tas2562->sdz_gpio)
 		gpiod_set_value_cansleep(tas2562->sdz_gpio, 1);
 
-	ret = snd_soc_component_update_bits(component, TAS2562_PWR_CTRL,
-					    TAS2562_MODE_MASK, TAS2562_MUTE);
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
@@ -428,30 +443,18 @@ static int tas2562_dac_event(struct snd_soc_dapm_widget *w,
 
 	switch (event) {
 	case SND_SOC_DAPM_POST_PMU:
-		ret = snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK,
-			TAS2562_MUTE);
-		if (ret)
-			goto end;
+		tas2562->dac_powered = true;
+		ret = tas2562_update_pwr_ctrl(tas2562);
 		break;
 	case SND_SOC_DAPM_PRE_PMD:
-		ret = snd_soc_component_update_bits(component,
-			TAS2562_PWR_CTRL,
-			TAS2562_MODE_MASK,
-			TAS2562_SHUTDOWN);
-		if (ret)
-			goto end;
+		tas2562->dac_powered = false;
+		ret = tas2562_update_pwr_ctrl(tas2562);
 		break;
 	default:
 		dev_err(tas2562->dev, "Not supported evevt\n");
 		return -EINVAL;
 	}
 
-end:
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
-- 
2.33.0

Re: [PATCH 0/2] Untested TAS2562 power setting fixes

[Mark Brown]

On Thu, 25 Aug 2022 16:22:24 +0200, Martin Povišer wrote:
> The tas2562 driver does the same thing with the setting of PWR_CTRL
> field as the tas2764/tas2770 drivers were doing.
> Link: https://lore.kernel.org/alsa-devel/20220808141246.5749-1-povik+lin@cutebit.org/T/#t
> Link: https://lore.kernel.org/alsa-devel/20220825140241.53963-1-povik+lin@cutebit.org/T/#t
> 
> These are blindly written patches without testing since I don't have
> the hardware. (I even tried TI's formal sample request program but
> was refused there. CCing @ti.com addresses I found on other series
> recently submitted.)
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] ASoC: tas2562: Drop conflicting set_bias_level power setting
      commit: b6b55b232564ade5cd91e9b9e2228b49f230d67f
[2/2] ASoC: tas2562: Fix mute/unmute
      commit: 2848d34c3ba1fc6f1ece0736a4faa16c6277f4d3

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 0/2] Untested TAS2562 power setting fixes

[Martin Povišer]

> On 5. 9. 2022, at 17:38, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, 25 Aug 2022 16:22:24 +0200, Martin Povišer wrote:
>> The tas2562 driver does the same thing with the setting of PWR_CTRL
>> field as the tas2764/tas2770 drivers were doing.
>> Link: https://lore.kernel.org/alsa-devel/20220808141246.5749-1-povik+lin@cutebit.org/T/#t
>> Link: https://lore.kernel.org/alsa-devel/20220825140241.53963-1-povik+lin@cutebit.org/T/#t
>> 
>> These are blindly written patches without testing since I don't have
>> the hardware. (I even tried TI's formal sample request program but
>> was refused there. CCing @ti.com addresses I found on other series
>> recently submitted.)
>> 
>> [...]
> 
> Applied to
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
> 
> Thanks!

Texas Instruments is shipping me samples of the affected
codecs, so I should be able to test the changes on hardware soon.

Should I find regressions, I will report back.

Martin

Re: [PATCH 0/2] Untested TAS2562 power setting fixes

[Martin Povišer]

> On 5. 9. 2022, at 17:38, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, 25 Aug 2022 16:22:24 +0200, Martin Povišer wrote:
>> The tas2562 driver does the same thing with the setting of PWR_CTRL
>> field as the tas2764/tas2770 drivers were doing.
>> Link: https://lore.kernel.org/alsa-devel/20220808141246.5749-1-povik+lin@cutebit.org/T/#t
>> Link: https://lore.kernel.org/alsa-devel/20220825140241.53963-1-povik+lin@cutebit.org/T/#t
>> 
>> These are blindly written patches without testing since I don't have
>> the hardware. (I even tried TI's formal sample request program but
>> was refused there. CCing @ti.com addresses I found on other series
>> recently submitted.)
>> 
>> [...]
> 
> Applied to
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
> 
> Thanks!

Texas Instruments is shipping me samples of the affected
codecs, so I should be able to test the changes on hardware soon.

Should I find regressions, I will report back.

Martin

Re: [PATCH 0/2] Untested TAS2562 power setting fixes

[Mark Brown]

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

On Mon, Sep 05, 2022 at 05:45:09PM +0200, Martin Povišer wrote:

> Texas Instruments is shipping me samples of the affected
> codecs, so I should be able to test the changes on hardware soon.

> Should I find regressions, I will report back.

Ah, that's great news.

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

Re: [PATCH 0/2] Untested TAS2562 power setting fixes

[Mark Brown]

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

On Mon, Sep 05, 2022 at 05:45:09PM +0200, Martin Povišer wrote:

> Texas Instruments is shipping me samples of the affected
> codecs, so I should be able to test the changes on hardware soon.

> Should I find regressions, I will report back.

Ah, that's great news.

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