[PATCH 0/3] ASoC: tas2783-sdw: volume and firmware load fixes

[PATCH 0/3] ASoC: tas2783-sdw: volume and firmware load fixes

[Matthew Schwartz]

Three tas2783-sdw fixes found while bringing up the ASUS ROG Xbox Ally
X20 [1], whose two TAS2783 amps share a SoundWire link that is powered
off when idle. They don't depend on that series and apply on their own,
but they were tested on the X20 with it applied.

Patch 1 keeps the Amp Volume and Speaker Volume settings in the driver.
The register cache is dropped on every reset, so the controls fail with
-EBUSY while the amp is runtime suspended, and on a link that powers off
when idle a setting is lost on the next wake. It supersedes Lee Mann's
patch for the volume lost across system suspend [2].

Patch 2 loads the firmware synchronously instead of waiting up to three
seconds for an asynchronous callback, which could leave an amp with its
firmware but without its SDCA init writes, or run after an unbind.
Patch 3 then refuses playback, in hw_params and in the port prepare of
a resumed stream, when those init writes failed.

On the X20 the firmware download takes about 1.2 s per amp from cold
and 0.2 s per amp on a wake, when the program RAM is not downloaded
again.

[1] https://lore.kernel.org/r/20261016232416.2628603-1-matthew.schwartz@linux.dev
[2] https://lore.kernel.org/r/20261016102846.22908-1-leepaulmann@gmail.com

Matthew Schwartz (3):
  ASoC: tas2783-sdw: keep the volume settings across resets
  ASoC: tas2783-sdw: load the firmware synchronously
  ASoC: tas2783-sdw: refuse playback when the init writes failed

 sound/soc/codecs/tas2783-sdw.c | 253 ++++++++++++++++++++++++---------
 1 file changed, 188 insertions(+), 65 deletions(-)


base-commit: 2450a9bcd3423612f614ea15ff44966fed189191
-- 
2.56.0

[PATCH 1/3] ASoC: tas2783-sdw: keep the volume settings across resets

[Matthew Schwartz]

The register cache is dropped when the amp attaches uninitialized and
again after the firmware download, so the Amp Volume and Speaker Volume
registers are not cached while the amp is runtime suspended, and every
read or write of those controls fails with -EBUSY. On a platform that
powers the SoundWire link off when idle, such as the ASUS ROG Xbox Ally
X20, the amp is also reset and initialized again on every wake, so a
volume set while it was awake is lost the next time the link idles.

Keep both settings in the driver, as the cache has to be dropped
whenever the reset and the firmware download change registers behind
it. Reads return the kept value, a write reaches the device only while
it is runtime active and initialized, and the settings are programmed
again after every init and resume. Until userspace sets a control, the
value the firmware leaves after each init is adopted instead, so its
tuning stays in effect. Waking the amp for a control access would
instead cost a full init of both X20 amps, about 0.2 s each.

Fixes: b627da430357 ("ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach")
Reported-by: Lee Mann <leepaulmann@gmail.com>
Link: https://lore.kernel.org/r/20261016102846.22908-1-leepaulmann@gmail.com
Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
---
 sound/soc/codecs/tas2783-sdw.c | 159 +++++++++++++++++++++++++++++++--
 1 file changed, 151 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
index a57230e01f75..ebb52efc1edf 100644
--- a/sound/soc/codecs/tas2783-sdw.c
+++ b/sound/soc/codecs/tas2783-sdw.c
@@ -12,6 +12,7 @@
 // Author: Baojun Xu <baojun.xu@ti.com>
 // Author: Kevin Lu <kevin-lu@ti.com>
 
+#include <linux/bitfield.h>
 #include <linux/cleanup.h>
 #include <linux/unaligned.h>
 #include <linux/crc32.h>
@@ -89,6 +90,15 @@ struct tas2783_prv {
 	struct mutex calib_lock;
 	/* pde and firmware download */
 	struct mutex pde_lock;
+	/* volume settings, kept across resets */
+	struct mutex vol_lock;
+	unsigned int amp_level;
+	unsigned int dvc_level;
+	/* set by userspace, so program it instead of adopting it */
+	bool amp_set;
+	bool dvc_set;
+	/* runtime active, so volume writes reach the device */
+	bool vol_live;
 	struct regmap *regmap;
 	struct device *dev;
 	struct class *class;
@@ -530,36 +540,151 @@ static const struct regmap_sdw_mbq_cfg tas2783_mbq_cfg = {
 	.mbq_size = tas2783_sdca_mbq_size,
 };
 
+#define TAS2783_AMP_LEVEL_MAX	20
+#define TAS2783_DVC_LVL_MAX	200
+
+/* forced, as a failed write leaves the new value in the cache */
+static s32 tas2783_write_amp_level(struct tas2783_prv *tas_dev,
+				   unsigned int level)
+{
+	return regmap_write_bits(tas_dev->regmap, TAS2783_AMP_LEVEL,
+				 TAS2783_AMP_LEVEL_MASK,
+				 FIELD_PREP(TAS2783_AMP_LEVEL_MASK, level));
+}
+
+static s32 tas2783_write_dvc_level(struct tas2783_prv *tas_dev,
+				   unsigned int level)
+{
+	return regmap_write(tas_dev->regmap, TAS2783_DVC_LVL,
+			    TAS2783_DVC_LVL_MAX - level);
+}
+
+/* program the volume settings userspace set, adopt the firmware ones */
+static s32 tas2783_sync_volume(struct tas2783_prv *tas_dev)
+{
+	unsigned int val;
+	s32 ret;
+
+	lockdep_assert_held(&tas_dev->vol_lock);
+
+	if (tas_dev->amp_set) {
+		ret = tas2783_write_amp_level(tas_dev, tas_dev->amp_level);
+	} else {
+		ret = regmap_read(tas_dev->regmap, TAS2783_AMP_LEVEL, &val);
+		if (!ret) {
+			val = FIELD_GET(TAS2783_AMP_LEVEL_MASK, val);
+			tas_dev->amp_level = umin(val, TAS2783_AMP_LEVEL_MAX);
+		}
+	}
+	if (ret)
+		return ret;
+
+	if (tas_dev->dvc_set)
+		return tas2783_write_dvc_level(tas_dev, tas_dev->dvc_level);
+
+	ret = regmap_read(tas_dev->regmap, TAS2783_DVC_LVL, &val);
+	if (!ret)
+		tas_dev->dvc_level = TAS2783_DVC_LVL_MAX -
+				     umin(val, TAS2783_DVC_LVL_MAX);
+
+	return ret;
+}
+
+static bool tas2783_vol_valid(struct snd_kcontrol *kcontrol, long val)
+{
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+
+	if (mc->platform_max && val > mc->platform_max)
+		return false;
+
+	return val >= 0 && val <= mc->max;
+}
+
 static s32 tas2783_digital_getvol(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_value *ucontrol)
 {
-	return snd_soc_get_volsw(kcontrol, ucontrol);
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
+
+	guard(mutex)(&tas_dev->vol_lock);
+	ucontrol->value.integer.value[0] = tas_dev->dvc_level;
+
+	return 0;
 }
 
 static s32 tas2783_digital_putvol(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_value *ucontrol)
 {
-	return snd_soc_put_volsw(kcontrol, ucontrol);
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
+	long val = ucontrol->value.integer.value[0];
+	s32 ret;
+
+	if (!tas2783_vol_valid(kcontrol, val))
+		return -EINVAL;
+
+	guard(mutex)(&tas_dev->vol_lock);
+	if (tas_dev->vol_live && tas_dev->hw_init) {
+		ret = tas2783_write_dvc_level(tas_dev, val);
+		if (ret)
+			return ret;
+	}
+
+	tas_dev->dvc_set = true;
+	if (val == tas_dev->dvc_level)
+		return 0;
+
+	tas_dev->dvc_level = val;
+
+	return 1;
 }
 
 static s32 tas2783_amp_getvol(struct snd_kcontrol *kcontrol,
 			      struct snd_ctl_elem_value *ucontrol)
 {
-	return snd_soc_get_volsw(kcontrol, ucontrol);
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
+
+	guard(mutex)(&tas_dev->vol_lock);
+	ucontrol->value.integer.value[0] = tas_dev->amp_level;
+
+	return 0;
 }
 
 static s32 tas2783_amp_putvol(struct snd_kcontrol *kcontrol,
 			      struct snd_ctl_elem_value *ucontrol)
 {
-	return snd_soc_put_volsw(kcontrol, ucontrol);
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
+	long val = ucontrol->value.integer.value[0];
+	s32 ret;
+
+	if (!tas2783_vol_valid(kcontrol, val))
+		return -EINVAL;
+
+	guard(mutex)(&tas_dev->vol_lock);
+	if (tas_dev->vol_live && tas_dev->hw_init) {
+		ret = tas2783_write_amp_level(tas_dev, val);
+		if (ret)
+			return ret;
+	}
+
+	tas_dev->amp_set = true;
+	if (val == tas_dev->amp_level)
+		return 0;
+
+	tas_dev->amp_level = val;
+
+	return 1;
 }
 
 static const struct snd_kcontrol_new tas2783_snd_controls[] = {
 	SOC_SINGLE_RANGE_EXT_TLV("Amp Volume", TAS2783_AMP_LEVEL,
-				 1, 0, 20, 0, tas2783_amp_getvol,
+				 1, 0, TAS2783_AMP_LEVEL_MAX, 0, tas2783_amp_getvol,
 				 tas2783_amp_putvol, tas2781_amp_tlv),
 	SOC_SINGLE_RANGE_EXT_TLV("Speaker Volume", TAS2783_DVC_LVL,
-				 0, 0, 200, 1, tas2783_digital_getvol,
+				 0, 0, TAS2783_DVC_LVL_MAX, 1, tas2783_digital_getvol,
 				 tas2783_digital_putvol, tas2781_dvc_tlv),
 };
 
@@ -1116,6 +1241,9 @@ static s32 tas2783_sdca_dev_suspend(struct device *dev)
 {
 	struct tas2783_prv *tas_dev = dev_get_drvdata(dev);
 
+	scoped_guard(mutex, &tas_dev->vol_lock)
+		tas_dev->vol_live = false;
+
 	if (!tas_dev->hw_init)
 		return 0;
 
@@ -1148,6 +1276,14 @@ static s32 tas2783_sdca_dev_resume(struct device *dev)
 		return ret;
 	}
 
+	guard(mutex)(&tas_dev->vol_lock);
+	if (tas_dev->hw_init) {
+		ret = tas2783_sync_volume(tas_dev);
+		if (ret)
+			return ret;
+	}
+	tas_dev->vol_live = true;
+
 	return 0;
 }
 
@@ -1245,11 +1381,14 @@ static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
 
 		/* Re-active AMP after resume. */
 		regmap_write(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 2), 0);
+		if (!ret) {
+			guard(mutex)(&tas_dev->vol_lock);
+			ret = tas2783_sync_volume(tas_dev);
+			tas_dev->hw_init = !ret;
+		}
 		if (ret)
 			dev_err(tas_dev->dev,
 				"init writes failed, err=%d", ret);
-		else
-			tas_dev->hw_init = true;
 	}
 
 	return ret;
@@ -1427,6 +1566,9 @@ static s32 tas_sdw_probe(struct sdw_slave *peripheral,
 	tas_dev->fw_version = 0;
 	mutex_init(&tas_dev->calib_lock);
 	mutex_init(&tas_dev->pde_lock);
+	mutex_init(&tas_dev->vol_lock);
+	tas_dev->amp_level = TAS2783_AMP_LEVEL_MAX;
+	tas_dev->dvc_level = TAS2783_DVC_LVL_MAX;
 
 	init_waitqueue_head(&tas_dev->fw_wait);
 	dev_set_drvdata(dev, tas_dev);
@@ -1452,6 +1594,7 @@ static void tas_sdw_remove(struct sdw_slave *peripheral)
 	tas_remove(tas_dev);
 	mutex_destroy(&tas_dev->calib_lock);
 	mutex_destroy(&tas_dev->pde_lock);
+	mutex_destroy(&tas_dev->vol_lock);
 	dev_set_drvdata(&peripheral->dev, NULL);
 }
 
-- 
2.56.0

Re: [PATCH 1/3] ASoC: tas2783-sdw: keep the volume settings across resets

[Pierre-Louis Bossart]

On 10/18/26 22:00, Matthew Schwartz wrote:
> The register cache is dropped when the amp attaches uninitialized and
> again after the firmware download, so the Amp Volume and Speaker Volume
> registers are not cached while the amp is runtime suspended, and every
> read or write of those controls fails with -EBUSY. On a platform that
> powers the SoundWire link off when idle, such as the ASUS ROG Xbox Ally
> X20, the amp is also reset and initialized again on every wake, so a
> volume set while it was awake is lost the next time the link idles.
> 
> Keep both settings in the driver, as the cache has to be dropped
> whenever the reset and the firmware download change registers behind
> it. Reads return the kept value, a write reaches the device only while
> it is runtime active and initialized, and the settings are programmed
> again after every init and resume. Until userspace sets a control, the
> value the firmware leaves after each init is adopted instead, so its
> tuning stays in effect. Waking the amp for a control access would
> instead cost a full init of both X20 amps, about 0.2 s each.
> 
> Fixes: b627da430357 ("ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach")

Erm, not sure what to think about this fix of a fix.

This commit b627da430357 introduced the regcache_drop_region() for all
kinds of reasons, and now this new patch introduces a manual management
of the volume registers - but why limit this management to volume
settings only? Shouldn't other features such as e.g. bass boost or EQ
also be restored?

It really feels like the problem is that the regmap is not properly
restored...at the end of the day, this driver is missing a
regcache_sync(), no?

> Reported-by: Lee Mann <leepaulmann@gmail.com>
> Link: https://lore.kernel.org/r/20261016102846.22908-1-leepaulmann@gmail.com
> Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> ---
>  sound/soc/codecs/tas2783-sdw.c | 159 +++++++++++++++++++++++++++++++--
>  1 file changed, 151 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
> index a57230e01f75..ebb52efc1edf 100644
> --- a/sound/soc/codecs/tas2783-sdw.c
> +++ b/sound/soc/codecs/tas2783-sdw.c
> @@ -12,6 +12,7 @@
>  // Author: Baojun Xu <baojun.xu@ti.com>
>  // Author: Kevin Lu <kevin-lu@ti.com>
>  
> +#include <linux/bitfield.h>
>  #include <linux/cleanup.h>
>  #include <linux/unaligned.h>
>  #include <linux/crc32.h>
> @@ -89,6 +90,15 @@ struct tas2783_prv {
>  	struct mutex calib_lock;
>  	/* pde and firmware download */
>  	struct mutex pde_lock;
> +	/* volume settings, kept across resets */
> +	struct mutex vol_lock;
> +	unsigned int amp_level;
> +	unsigned int dvc_level;
> +	/* set by userspace, so program it instead of adopting it */
> +	bool amp_set;
> +	bool dvc_set;
> +	/* runtime active, so volume writes reach the device */
> +	bool vol_live;
>  	struct regmap *regmap;
>  	struct device *dev;
>  	struct class *class;
> @@ -530,36 +540,151 @@ static const struct regmap_sdw_mbq_cfg tas2783_mbq_cfg = {
>  	.mbq_size = tas2783_sdca_mbq_size,
>  };
>  
> +#define TAS2783_AMP_LEVEL_MAX	20
> +#define TAS2783_DVC_LVL_MAX	200
> +
> +/* forced, as a failed write leaves the new value in the cache */
> +static s32 tas2783_write_amp_level(struct tas2783_prv *tas_dev,
> +				   unsigned int level)
> +{
> +	return regmap_write_bits(tas_dev->regmap, TAS2783_AMP_LEVEL,
> +				 TAS2783_AMP_LEVEL_MASK,
> +				 FIELD_PREP(TAS2783_AMP_LEVEL_MASK, level));
> +}
> +
> +static s32 tas2783_write_dvc_level(struct tas2783_prv *tas_dev,
> +				   unsigned int level)
> +{
> +	return regmap_write(tas_dev->regmap, TAS2783_DVC_LVL,
> +			    TAS2783_DVC_LVL_MAX - level);
> +}
> +
> +/* program the volume settings userspace set, adopt the firmware ones */
> +static s32 tas2783_sync_volume(struct tas2783_prv *tas_dev)
> +{
> +	unsigned int val;
> +	s32 ret;
> +
> +	lockdep_assert_held(&tas_dev->vol_lock);
> +
> +	if (tas_dev->amp_set) {
> +		ret = tas2783_write_amp_level(tas_dev, tas_dev->amp_level);
> +	} else {
> +		ret = regmap_read(tas_dev->regmap, TAS2783_AMP_LEVEL, &val);
> +		if (!ret) {
> +			val = FIELD_GET(TAS2783_AMP_LEVEL_MASK, val);
> +			tas_dev->amp_level = umin(val, TAS2783_AMP_LEVEL_MAX);
> +		}
> +	}
> +	if (ret)
> +		return ret;
> +
> +	if (tas_dev->dvc_set)
> +		return tas2783_write_dvc_level(tas_dev, tas_dev->dvc_level);
> +
> +	ret = regmap_read(tas_dev->regmap, TAS2783_DVC_LVL, &val);
> +	if (!ret)
> +		tas_dev->dvc_level = TAS2783_DVC_LVL_MAX -
> +				     umin(val, TAS2783_DVC_LVL_MAX);
> +
> +	return ret;
> +}
> +
> +static bool tas2783_vol_valid(struct snd_kcontrol *kcontrol, long val)
> +{
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +
> +	if (mc->platform_max && val > mc->platform_max)
> +		return false;
> +
> +	return val >= 0 && val <= mc->max;
> +}
> +
>  static s32 tas2783_digital_getvol(struct snd_kcontrol *kcontrol,
>  				  struct snd_ctl_elem_value *ucontrol)
>  {
> -	return snd_soc_get_volsw(kcontrol, ucontrol);
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
> +
> +	guard(mutex)(&tas_dev->vol_lock);
> +	ucontrol->value.integer.value[0] = tas_dev->dvc_level;
> +
> +	return 0;
>  }
>  
>  static s32 tas2783_digital_putvol(struct snd_kcontrol *kcontrol,
>  				  struct snd_ctl_elem_value *ucontrol)
>  {
> -	return snd_soc_put_volsw(kcontrol, ucontrol);
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
> +	long val = ucontrol->value.integer.value[0];
> +	s32 ret;
> +
> +	if (!tas2783_vol_valid(kcontrol, val))
> +		return -EINVAL;
> +
> +	guard(mutex)(&tas_dev->vol_lock);
> +	if (tas_dev->vol_live && tas_dev->hw_init) {
> +		ret = tas2783_write_dvc_level(tas_dev, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	tas_dev->dvc_set = true;
> +	if (val == tas_dev->dvc_level)
> +		return 0;
> +
> +	tas_dev->dvc_level = val;
> +
> +	return 1;
>  }
>  
>  static s32 tas2783_amp_getvol(struct snd_kcontrol *kcontrol,
>  			      struct snd_ctl_elem_value *ucontrol)
>  {
> -	return snd_soc_get_volsw(kcontrol, ucontrol);
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
> +
> +	guard(mutex)(&tas_dev->vol_lock);
> +	ucontrol->value.integer.value[0] = tas_dev->amp_level;
> +
> +	return 0;
>  }
>  
>  static s32 tas2783_amp_putvol(struct snd_kcontrol *kcontrol,
>  			      struct snd_ctl_elem_value *ucontrol)
>  {
> -	return snd_soc_put_volsw(kcontrol, ucontrol);
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
> +	long val = ucontrol->value.integer.value[0];
> +	s32 ret;
> +
> +	if (!tas2783_vol_valid(kcontrol, val))
> +		return -EINVAL;
> +
> +	guard(mutex)(&tas_dev->vol_lock);
> +	if (tas_dev->vol_live && tas_dev->hw_init) {
> +		ret = tas2783_write_amp_level(tas_dev, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	tas_dev->amp_set = true;
> +	if (val == tas_dev->amp_level)
> +		return 0;
> +
> +	tas_dev->amp_level = val;
> +
> +	return 1;
>  }
>  
>  static const struct snd_kcontrol_new tas2783_snd_controls[] = {
>  	SOC_SINGLE_RANGE_EXT_TLV("Amp Volume", TAS2783_AMP_LEVEL,
> -				 1, 0, 20, 0, tas2783_amp_getvol,
> +				 1, 0, TAS2783_AMP_LEVEL_MAX, 0, tas2783_amp_getvol,
>  				 tas2783_amp_putvol, tas2781_amp_tlv),
>  	SOC_SINGLE_RANGE_EXT_TLV("Speaker Volume", TAS2783_DVC_LVL,
> -				 0, 0, 200, 1, tas2783_digital_getvol,
> +				 0, 0, TAS2783_DVC_LVL_MAX, 1, tas2783_digital_getvol,
>  				 tas2783_digital_putvol, tas2781_dvc_tlv),
>  };
>  
> @@ -1116,6 +1241,9 @@ static s32 tas2783_sdca_dev_suspend(struct device *dev)
>  {
>  	struct tas2783_prv *tas_dev = dev_get_drvdata(dev);
>  
> +	scoped_guard(mutex, &tas_dev->vol_lock)
> +		tas_dev->vol_live = false;
> +
>  	if (!tas_dev->hw_init)
>  		return 0;
>  
> @@ -1148,6 +1276,14 @@ static s32 tas2783_sdca_dev_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	guard(mutex)(&tas_dev->vol_lock);
> +	if (tas_dev->hw_init) {
> +		ret = tas2783_sync_volume(tas_dev);
> +		if (ret)
> +			return ret;
> +	}
> +	tas_dev->vol_live = true;
> +
>  	return 0;
>  }
>  
> @@ -1245,11 +1381,14 @@ static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
>  
>  		/* Re-active AMP after resume. */
>  		regmap_write(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 2), 0);
> +		if (!ret) {
> +			guard(mutex)(&tas_dev->vol_lock);
> +			ret = tas2783_sync_volume(tas_dev);
> +			tas_dev->hw_init = !ret;
> +		}
>  		if (ret)
>  			dev_err(tas_dev->dev,
>  				"init writes failed, err=%d", ret);
> -		else
> -			tas_dev->hw_init = true;
>  	}
>  
>  	return ret;
> @@ -1427,6 +1566,9 @@ static s32 tas_sdw_probe(struct sdw_slave *peripheral,
>  	tas_dev->fw_version = 0;
>  	mutex_init(&tas_dev->calib_lock);
>  	mutex_init(&tas_dev->pde_lock);
> +	mutex_init(&tas_dev->vol_lock);
> +	tas_dev->amp_level = TAS2783_AMP_LEVEL_MAX;
> +	tas_dev->dvc_level = TAS2783_DVC_LVL_MAX;
>  
>  	init_waitqueue_head(&tas_dev->fw_wait);
>  	dev_set_drvdata(dev, tas_dev);
> @@ -1452,6 +1594,7 @@ static void tas_sdw_remove(struct sdw_slave *peripheral)
>  	tas_remove(tas_dev);
>  	mutex_destroy(&tas_dev->calib_lock);
>  	mutex_destroy(&tas_dev->pde_lock);
> +	mutex_destroy(&tas_dev->vol_lock);
>  	dev_set_drvdata(&peripheral->dev, NULL);
>  }
>  

Re: [PATCH 1/3] ASoC: tas2783-sdw: keep the volume settings across resets

[Matthew Schwartz]

On 10/19/26 8:56 AM, Pierre-Louis Bossart wrote:
> On 10/18/26 22:00, Matthew Schwartz wrote:
>> The register cache is dropped when the amp attaches uninitialized and
>> again after the firmware download, so the Amp Volume and Speaker Volume
>> registers are not cached while the amp is runtime suspended, and every
>> read or write of those controls fails with -EBUSY. On a platform that
>> powers the SoundWire link off when idle, such as the ASUS ROG Xbox Ally
>> X20, the amp is also reset and initialized again on every wake, so a
>> volume set while it was awake is lost the next time the link idles.
>>
>> Keep both settings in the driver, as the cache has to be dropped
>> whenever the reset and the firmware download change registers behind
>> it. Reads return the kept value, a write reaches the device only while
>> it is runtime active and initialized, and the settings are programmed
>> again after every init and resume. Until userspace sets a control, the
>> value the firmware leaves after each init is adopted instead, so its
>> tuning stays in effect. Waking the amp for a control access would
>> instead cost a full init of both X20 amps, about 0.2 s each.
>>
>> Fixes: b627da430357 ("ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach")
> 
> Erm, not sure what to think about this fix of a fix.
> 
> This commit b627da430357 introduced the regcache_drop_region() for all
> kinds of reasons, and now this new patch introduces a manual management
> of the volume registers - but why limit this management to volume
> settings only? Shouldn't other features such as e.g. bass boost or EQ
> also be restored?
> 
> It really feels like the problem is that the regmap is not properly
> restored...at the end of the day, this driver is missing a
> regcache_sync(), no?

The driver only has the two volume controls, there's no bass boost or
EQ control. The rest of the tuning comes from the firmware, which
tas_io_init() writes again on every init, apart from a retained PRAM.

A regcache_sync() can't restore these two because the firmware
download writes registers with sdw_nwrite_no_pm(), behind the cache,
and that includes AMP_LEVEL. Of the 72 TAS2783 files in linux-firmware,
44 set it to something other than the 0x28 in tas2783_reg_default[],
mostly levels 9 to 16, and two set DVC to 0x04 instead of 0x00.
Syncing the default values would replace that per-platform tuning with
full gain. So the driver has to know whether userspace set a value,
which needs restoring, or left it alone, in which case the firmware's
value should stay, and regmap doesn't track that per register.

The volume was also lost before b627da430357, since the sync ran
before the reset wiped it. The Fixes tag is for the -EBUSY that commit
added when reading an uncached volume register while the amp is
runtime suspended.

Certainly open to suggestions for improvement here if there's a simpler
way to fix this.

> 
>> Reported-by: Lee Mann <leepaulmann@gmail.com>
>> Link: https://lore.kernel.org/r/20261016102846.22908-1-leepaulmann@gmail.com
>> Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>> ---
>>  sound/soc/codecs/tas2783-sdw.c | 159 +++++++++++++++++++++++++++++++--
>>  1 file changed, 151 insertions(+), 8 deletions(-)
>>
>> diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
>> index a57230e01f75..ebb52efc1edf 100644
>> --- a/sound/soc/codecs/tas2783-sdw.c
>> +++ b/sound/soc/codecs/tas2783-sdw.c
>> @@ -12,6 +12,7 @@
>>  // Author: Baojun Xu <baojun.xu@ti.com>
>>  // Author: Kevin Lu <kevin-lu@ti.com>
>>  
>> +#include <linux/bitfield.h>
>>  #include <linux/cleanup.h>
>>  #include <linux/unaligned.h>
>>  #include <linux/crc32.h>
>> @@ -89,6 +90,15 @@ struct tas2783_prv {
>>  	struct mutex calib_lock;
>>  	/* pde and firmware download */
>>  	struct mutex pde_lock;
>> +	/* volume settings, kept across resets */
>> +	struct mutex vol_lock;
>> +	unsigned int amp_level;
>> +	unsigned int dvc_level;
>> +	/* set by userspace, so program it instead of adopting it */
>> +	bool amp_set;
>> +	bool dvc_set;
>> +	/* runtime active, so volume writes reach the device */
>> +	bool vol_live;
>>  	struct regmap *regmap;
>>  	struct device *dev;
>>  	struct class *class;
>> @@ -530,36 +540,151 @@ static const struct regmap_sdw_mbq_cfg tas2783_mbq_cfg = {
>>  	.mbq_size = tas2783_sdca_mbq_size,
>>  };
>>  
>> +#define TAS2783_AMP_LEVEL_MAX	20
>> +#define TAS2783_DVC_LVL_MAX	200
>> +
>> +/* forced, as a failed write leaves the new value in the cache */
>> +static s32 tas2783_write_amp_level(struct tas2783_prv *tas_dev,
>> +				   unsigned int level)
>> +{
>> +	return regmap_write_bits(tas_dev->regmap, TAS2783_AMP_LEVEL,
>> +				 TAS2783_AMP_LEVEL_MASK,
>> +				 FIELD_PREP(TAS2783_AMP_LEVEL_MASK, level));
>> +}
>> +
>> +static s32 tas2783_write_dvc_level(struct tas2783_prv *tas_dev,
>> +				   unsigned int level)
>> +{
>> +	return regmap_write(tas_dev->regmap, TAS2783_DVC_LVL,
>> +			    TAS2783_DVC_LVL_MAX - level);
>> +}
>> +
>> +/* program the volume settings userspace set, adopt the firmware ones */
>> +static s32 tas2783_sync_volume(struct tas2783_prv *tas_dev)
>> +{
>> +	unsigned int val;
>> +	s32 ret;
>> +
>> +	lockdep_assert_held(&tas_dev->vol_lock);
>> +
>> +	if (tas_dev->amp_set) {
>> +		ret = tas2783_write_amp_level(tas_dev, tas_dev->amp_level);
>> +	} else {
>> +		ret = regmap_read(tas_dev->regmap, TAS2783_AMP_LEVEL, &val);
>> +		if (!ret) {
>> +			val = FIELD_GET(TAS2783_AMP_LEVEL_MASK, val);
>> +			tas_dev->amp_level = umin(val, TAS2783_AMP_LEVEL_MAX);
>> +		}
>> +	}
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (tas_dev->dvc_set)
>> +		return tas2783_write_dvc_level(tas_dev, tas_dev->dvc_level);
>> +
>> +	ret = regmap_read(tas_dev->regmap, TAS2783_DVC_LVL, &val);
>> +	if (!ret)
>> +		tas_dev->dvc_level = TAS2783_DVC_LVL_MAX -
>> +				     umin(val, TAS2783_DVC_LVL_MAX);
>> +
>> +	return ret;
>> +}
>> +
>> +static bool tas2783_vol_valid(struct snd_kcontrol *kcontrol, long val)
>> +{
>> +	struct soc_mixer_control *mc =
>> +		(struct soc_mixer_control *)kcontrol->private_value;
>> +
>> +	if (mc->platform_max && val > mc->platform_max)
>> +		return false;
>> +
>> +	return val >= 0 && val <= mc->max;
>> +}
>> +
>>  static s32 tas2783_digital_getvol(struct snd_kcontrol *kcontrol,
>>  				  struct snd_ctl_elem_value *ucontrol)
>>  {
>> -	return snd_soc_get_volsw(kcontrol, ucontrol);
>> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>> +	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
>> +
>> +	guard(mutex)(&tas_dev->vol_lock);
>> +	ucontrol->value.integer.value[0] = tas_dev->dvc_level;
>> +
>> +	return 0;
>>  }
>>  
>>  static s32 tas2783_digital_putvol(struct snd_kcontrol *kcontrol,
>>  				  struct snd_ctl_elem_value *ucontrol)
>>  {
>> -	return snd_soc_put_volsw(kcontrol, ucontrol);
>> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>> +	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
>> +	long val = ucontrol->value.integer.value[0];
>> +	s32 ret;
>> +
>> +	if (!tas2783_vol_valid(kcontrol, val))
>> +		return -EINVAL;
>> +
>> +	guard(mutex)(&tas_dev->vol_lock);
>> +	if (tas_dev->vol_live && tas_dev->hw_init) {
>> +		ret = tas2783_write_dvc_level(tas_dev, val);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	tas_dev->dvc_set = true;
>> +	if (val == tas_dev->dvc_level)
>> +		return 0;
>> +
>> +	tas_dev->dvc_level = val;
>> +
>> +	return 1;
>>  }
>>  
>>  static s32 tas2783_amp_getvol(struct snd_kcontrol *kcontrol,
>>  			      struct snd_ctl_elem_value *ucontrol)
>>  {
>> -	return snd_soc_get_volsw(kcontrol, ucontrol);
>> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>> +	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
>> +
>> +	guard(mutex)(&tas_dev->vol_lock);
>> +	ucontrol->value.integer.value[0] = tas_dev->amp_level;
>> +
>> +	return 0;
>>  }
>>  
>>  static s32 tas2783_amp_putvol(struct snd_kcontrol *kcontrol,
>>  			      struct snd_ctl_elem_value *ucontrol)
>>  {
>> -	return snd_soc_put_volsw(kcontrol, ucontrol);
>> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>> +	struct tas2783_prv *tas_dev = snd_soc_component_get_drvdata(component);
>> +	long val = ucontrol->value.integer.value[0];
>> +	s32 ret;
>> +
>> +	if (!tas2783_vol_valid(kcontrol, val))
>> +		return -EINVAL;
>> +
>> +	guard(mutex)(&tas_dev->vol_lock);
>> +	if (tas_dev->vol_live && tas_dev->hw_init) {
>> +		ret = tas2783_write_amp_level(tas_dev, val);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	tas_dev->amp_set = true;
>> +	if (val == tas_dev->amp_level)
>> +		return 0;
>> +
>> +	tas_dev->amp_level = val;
>> +
>> +	return 1;
>>  }
>>  
>>  static const struct snd_kcontrol_new tas2783_snd_controls[] = {
>>  	SOC_SINGLE_RANGE_EXT_TLV("Amp Volume", TAS2783_AMP_LEVEL,
>> -				 1, 0, 20, 0, tas2783_amp_getvol,
>> +				 1, 0, TAS2783_AMP_LEVEL_MAX, 0, tas2783_amp_getvol,
>>  				 tas2783_amp_putvol, tas2781_amp_tlv),
>>  	SOC_SINGLE_RANGE_EXT_TLV("Speaker Volume", TAS2783_DVC_LVL,
>> -				 0, 0, 200, 1, tas2783_digital_getvol,
>> +				 0, 0, TAS2783_DVC_LVL_MAX, 1, tas2783_digital_getvol,
>>  				 tas2783_digital_putvol, tas2781_dvc_tlv),
>>  };
>>  
>> @@ -1116,6 +1241,9 @@ static s32 tas2783_sdca_dev_suspend(struct device *dev)
>>  {
>>  	struct tas2783_prv *tas_dev = dev_get_drvdata(dev);
>>  
>> +	scoped_guard(mutex, &tas_dev->vol_lock)
>> +		tas_dev->vol_live = false;
>> +
>>  	if (!tas_dev->hw_init)
>>  		return 0;
>>  
>> @@ -1148,6 +1276,14 @@ static s32 tas2783_sdca_dev_resume(struct device *dev)
>>  		return ret;
>>  	}
>>  
>> +	guard(mutex)(&tas_dev->vol_lock);
>> +	if (tas_dev->hw_init) {
>> +		ret = tas2783_sync_volume(tas_dev);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	tas_dev->vol_live = true;
>> +
>>  	return 0;
>>  }
>>  
>> @@ -1245,11 +1381,14 @@ static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
>>  
>>  		/* Re-active AMP after resume. */
>>  		regmap_write(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 2), 0);
>> +		if (!ret) {
>> +			guard(mutex)(&tas_dev->vol_lock);
>> +			ret = tas2783_sync_volume(tas_dev);
>> +			tas_dev->hw_init = !ret;
>> +		}
>>  		if (ret)
>>  			dev_err(tas_dev->dev,
>>  				"init writes failed, err=%d", ret);
>> -		else
>> -			tas_dev->hw_init = true;
>>  	}
>>  
>>  	return ret;
>> @@ -1427,6 +1566,9 @@ static s32 tas_sdw_probe(struct sdw_slave *peripheral,
>>  	tas_dev->fw_version = 0;
>>  	mutex_init(&tas_dev->calib_lock);
>>  	mutex_init(&tas_dev->pde_lock);
>> +	mutex_init(&tas_dev->vol_lock);
>> +	tas_dev->amp_level = TAS2783_AMP_LEVEL_MAX;
>> +	tas_dev->dvc_level = TAS2783_DVC_LVL_MAX;
>>  
>>  	init_waitqueue_head(&tas_dev->fw_wait);
>>  	dev_set_drvdata(dev, tas_dev);
>> @@ -1452,6 +1594,7 @@ static void tas_sdw_remove(struct sdw_slave *peripheral)
>>  	tas_remove(tas_dev);
>>  	mutex_destroy(&tas_dev->calib_lock);
>>  	mutex_destroy(&tas_dev->pde_lock);
>> +	mutex_destroy(&tas_dev->vol_lock);
>>  	dev_set_drvdata(&peripheral->dev, NULL);
>>  }
>>  
> 

[PATCH 2/3] ASoC: tas2783-sdw: load the firmware synchronously

[Matthew Schwartz]

tas_fw_load() requests the firmware asynchronously and gives up waiting
for the callback after three seconds. tas_io_init() then returns without
the SDCA function initialization writes, while the callback carries on
downloading, sets fw_dl_success and so allows playback on an amp that
never got them. Nothing cancels the callback when the driver is unbound
either, so it can run on freed driver data. The download takes about
1.2 s per amp on the ASUS ROG Xbox Ally X20, so it fits there, but
nothing bounds it.

Request the firmware with request_firmware_direct() and download it in
the caller, as rt1320 does from its io_init, which removes the wait, the
timeout and the callback. The download now returns its result, so a
missing or bad firmware file fails tas_io_init() before the init writes
instead of running them on an amp that refuses playback anyway.

Fixes: 4cc9bd8d7b32 ("ASoc: tas2783A: Add soundwire based codec driver")
Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
---
 sound/soc/codecs/tas2783-sdw.c | 83 +++++++++++-----------------------
 1 file changed, 26 insertions(+), 57 deletions(-)

diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
index ebb52efc1edf..8e7376889b6d 100644
--- a/sound/soc/codecs/tas2783-sdw.c
+++ b/sound/soc/codecs/tas2783-sdw.c
@@ -25,7 +25,6 @@
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
-#include <linux/wait.h>
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_registers.h>
 #include <linux/soundwire/sdw_type.h>
@@ -41,7 +40,6 @@
 
 #include "tas2783.h"
 
-#define TIMEOUT_FW_DL_MS (3000)
 #define FW_DL_OFFSET	84 /* binary file information */
 #define FW_FL_HDR	20 /* minimum number of bytes in one chunk */
 #define TAS2783_PROBE_TIMEOUT 5000
@@ -108,9 +106,7 @@ struct tas2783_prv {
 	u8 dev_name[32];
 	bool hw_init;
 	unsigned int fw_version;
-	/* wq for firmware download */
-	wait_queue_head_t fw_wait;
-	bool fw_dl_task_done;
+	/* firmware download result */
 	bool fw_dl_success;
 	/* use fallback fw name */
 	bool fw_use_fallback;
@@ -866,10 +862,9 @@ static s32 tas_fw_get_next_file(const u8 *data, struct tas_fw_file *file)
 	return file->length + sizeof(u32) * 5;
 }
 
-static void tas2783_fw_ready(const struct firmware *fmw, void *context)
+static s32 tas2783_fw_download(struct tas2783_prv *tas_dev,
+			       const struct firmware *fmw)
 {
-	struct tas2783_prv *tas_dev =
-		(struct tas2783_prv *)context;
 	const u8 *buf = NULL;
 	s32  img_sz, ret = 0, cur_file = 0;
 	s32 offset = 0;
@@ -877,41 +872,20 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
 
 	struct tas_fw_hdr *hdr __free(kfree) = kzalloc_obj(*hdr);
 	struct tas_fw_file *file __free(kfree) = kzalloc_obj(*file);
-	if (!file || !hdr) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	/* firmware binary not found*/
-	if (!fmw || !fmw->data) {
-		if (!tas_dev->fw_use_fallback) {
-			tas_dev->fw_use_fallback = true;
-			dev_info(tas_dev->dev,
-				"Failed to read preferred fw binary: %s, attempting fallback binary load\n",
-				tas_dev->rca_binaryname);
-		} else {
-			dev_err(tas_dev->dev,
-				"Failed to read fallback fw binary %s\n",
-				tas_dev->rca_binaryname);
-		}
-
-		ret = -EINVAL;
-		goto out;
-	}
+	if (!file || !hdr)
+		return -ENOMEM;
 
 	img_sz = fmw->size;
 	buf = fmw->data;
 	offset += tas_fw_read_hdr(buf, hdr);
 	if (hdr->size != img_sz) {
-		ret = -EINVAL;
 		dev_err(tas_dev->dev, "firmware size mismatch with header");
-		goto out;
+		return -EINVAL;
 	}
 
 	if (img_sz < FW_DL_OFFSET) {
-		ret = -EINVAL;
 		dev_err(tas_dev->dev, "unexpected size, size is too small");
-		goto out;
+		return -EINVAL;
 	}
 
 	mutex_lock(&tas_dev->pde_lock);
@@ -972,13 +946,7 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
 				       (val[2] << 8) | val[3];
 	}
 
-out:
-	if (!ret)
-		tas_dev->fw_dl_success = true;
-	tas_dev->fw_dl_task_done = true;
-	wake_up(&tas_dev->fw_wait);
-	if (fmw)
-		release_firmware(fmw);
+	return ret;
 }
 
 static inline s32 tas_clear_latch(struct tas2783_prv *priv)
@@ -1322,31 +1290,32 @@ static void tas_generate_fw_name(struct sdw_slave *slave, char *name, size_t siz
 
 static s32 tas_fw_load(struct tas2783_prv *tas_dev, struct sdw_slave *slave)
 {
+	const struct firmware *fmw;
 	s32 ret;
-	u8 unique_id = tas_dev->sdw_peripheral->id.unique_id;
 
 	tas_generate_fw_name(slave, tas_dev->rca_binaryname,
 			     sizeof(tas_dev->rca_binaryname));
 
-	tas_dev->fw_dl_task_done = false;
-	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
-				      tas_dev->rca_binaryname, tas_dev->dev,
-				      GFP_KERNEL, tas_dev, tas2783_fw_ready);
+	ret = request_firmware_direct(&fmw, tas_dev->rca_binaryname,
+				      tas_dev->dev);
 	if (ret) {
-		dev_err(tas_dev->dev,
-			"firmware request failed for uid=%d, ret=%d\n",
-			unique_id, ret);
+		if (!tas_dev->fw_use_fallback) {
+			tas_dev->fw_use_fallback = true;
+			dev_info(tas_dev->dev,
+				 "Failed to read preferred fw binary: %s, attempting fallback binary load\n",
+				 tas_dev->rca_binaryname);
+		} else {
+			dev_err(tas_dev->dev,
+				"Failed to read fallback fw binary %s\n",
+				tas_dev->rca_binaryname);
+		}
 		return ret;
 	}
 
-	ret = wait_event_timeout(tas_dev->fw_wait, tas_dev->fw_dl_task_done,
-				 msecs_to_jiffies(TIMEOUT_FW_DL_MS));
-	if (!ret) {
-		dev_err(tas_dev->dev, "fw request, wait_event timeout\n");
-		return -EAGAIN;
-	}
+	ret = tas2783_fw_download(tas_dev, fmw);
+	release_firmware(fmw);
 
-	return 0;
+	return ret;
 }
 
 static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
@@ -1368,8 +1337,9 @@ static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
 
 	tas_dev->fw_use_fallback = false;
 	ret = tas_fw_load(tas_dev, slave);
-	if (!ret && tas_dev->fw_use_fallback)
+	if (ret && tas_dev->fw_use_fallback)
 		ret = tas_fw_load(tas_dev, slave);
+	tas_dev->fw_dl_success = !ret;
 
 	if (!ret) {
 		if (tas_dev->sa_func_data)
@@ -1570,7 +1540,6 @@ static s32 tas_sdw_probe(struct sdw_slave *peripheral,
 	tas_dev->amp_level = TAS2783_AMP_LEVEL_MAX;
 	tas_dev->dvc_level = TAS2783_DVC_LVL_MAX;
 
-	init_waitqueue_head(&tas_dev->fw_wait);
 	dev_set_drvdata(dev, tas_dev);
 	regmap = devm_regmap_init_sdw_mbq_cfg(&peripheral->dev,
 					      peripheral,
-- 
2.56.0

Re: [PATCH 2/3] ASoC: tas2783-sdw: load the firmware synchronously

[Pierre-Louis Bossart]

On 10/18/26 22:00, Matthew Schwartz wrote:
> tas_fw_load() requests the firmware asynchronously and gives up waiting
> for the callback after three seconds. tas_io_init() then returns without
> the SDCA function initialization writes, while the callback carries on
> downloading, sets fw_dl_success and so allows playback on an amp that
> never got them. Nothing cancels the callback when the driver is unbound
> either, so it can run on freed driver data. The download takes about
> 1.2 s per amp on the ASUS ROG Xbox Ally X20, so it fits there, but
> nothing bounds it.
> 
> Request the firmware with request_firmware_direct() and download it in
> the caller, as rt1320 does from its io_init, which removes the wait, the
> timeout and the callback. The download now returns its result, so a
> missing or bad firmware file fails tas_io_init() before the init writes
> instead of running them on an amp that refuses playback anyway.

The changes in this patch look ok, but this commit description doesn't
really say *why* or *how often* the firmware download fails.
Is this really a problem on actual devices?
Is this defensive programming?

Re: [PATCH 2/3] ASoC: tas2783-sdw: load the firmware synchronously

[Matthew Schwartz]

On 10/19/26 9:04 AM, Pierre-Louis Bossart wrote:
> On 10/18/26 22:00, Matthew Schwartz wrote:
>> tas_fw_load() requests the firmware asynchronously and gives up waiting
>> for the callback after three seconds. tas_io_init() then returns without
>> the SDCA function initialization writes, while the callback carries on
>> downloading, sets fw_dl_success and so allows playback on an amp that
>> never got them. Nothing cancels the callback when the driver is unbound
>> either, so it can run on freed driver data. The download takes about
>> 1.2 s per amp on the ASUS ROG Xbox Ally X20, so it fits there, but
>> nothing bounds it.
>>
>> Request the firmware with request_firmware_direct() and download it in
>> the caller, as rt1320 does from its io_init, which removes the wait, the
>> timeout and the callback. The download now returns its result, so a
>> missing or bad firmware file fails tas_io_init() before the init writes
>> instead of running them on an amp that refuses playback anyway.
> 
> The changes in this patch look ok, but this commit description doesn't
> really say *why* or *how often* the firmware download fails.
> Is this really a problem on actual devices?
> Is this defensive programming?

This one is defensive, yeah. I haven't seen the timeout fire. On the
X20 the download takes about 1.2 s per amp against the 3 s limit, and
every TAS2783 file in linux-firmware is the same size. But if it does
fire, tas_io_init() returns without the init writes while the callback
can still go on to set fw_dl_success, so playback is allowed, and
nothing cancels the callback on unbind. tas_fw_load() waits for the
callback anyway, so the asynchronous request gains nothing. I'll
clarify that in v2 and drop the Fixes tag, or I can drop the patch from
this series if we'd rather wait to see if this actually causes issues in
practice.

[PATCH 3/3] ASoC: tas2783-sdw: refuse playback when the init writes failed

[Matthew Schwartz]

When the SDCA function initialization writes fail, tas_io_init() leaves
hw_init false, but the firmware download has already set fw_dl_success,
so tas_sdw_hw_params() goes on to play through an amp that is missing
part of its setup. On the ASUS ROG Xbox Ally X20 those writes come from
the ACPI DisCo initialization table and include the posture, which the
BIOS sets differently on each amp.

Refuse playback until the init writes have completed as well, and log
which of the two steps is missing. A stream that is only prepared again
after a resume does not go through hw_params, so check in the port
prepare as well.

Fixes: ca8f3611dcf8 ("ASoC: tas2783A: use acpi initialisation table")
Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
---
 sound/soc/codecs/tas2783-sdw.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
index 8e7376889b6d..6c4cc8868415 100644
--- a/sound/soc/codecs/tas2783-sdw.c
+++ b/sound/soc/codecs/tas2783-sdw.c
@@ -1055,6 +1055,11 @@ static s32 tas_sdw_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
+	if (!tas_dev->hw_init) {
+		dev_err(tas_dev->dev, "error playback without init writes");
+		return -EINVAL;
+	}
+
 	sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
 	if (!sdw_stream)
 		return -EINVAL;
@@ -1427,6 +1432,12 @@ static int tas_port_prep(struct sdw_slave *slave, struct sdw_prepare_ch *prep_ch
 	addr = SDW_DPN_PREPARECTRL(prep_ch->num);
 	switch (pre_ops) {
 	case SDW_OPS_PORT_PRE_PREP:
+		if (!tas_dev->hw_init) {
+			dev_err(dev, "error port %d prepare without init writes\n",
+				prep_ch->num);
+			return -EINVAL;
+		}
+
 		/*
 		 * The Function has to be powered before the port can complete
 		 * channel preparation.  hw_params() does that when a stream is
-- 
2.56.0

Re: [PATCH 3/3] ASoC: tas2783-sdw: refuse playback when the init writes failed

[Pierre-Louis Bossart]

On 10/18/26 22:00, Matthew Schwartz wrote:
> When the SDCA function initialization writes fail, tas_io_init() leaves
> hw_init false, but the firmware download has already set fw_dl_success,
> so tas_sdw_hw_params() goes on to play through an amp that is missing
> part of its setup. On the ASUS ROG Xbox Ally X20 those writes come from
> the ACPI DisCo initialization table and include the posture, which the
> BIOS sets differently on each amp.
> 
> Refuse playback until the init writes have completed as well, and log
> which of the two steps is missing. A stream that is only prepared again
> after a resume does not go through hw_params, so check in the port
> prepare as well.

Same comment as the previous patch. This patch logs an error message for
a critical error, which seems fine, but what does it take to make the
init writes succeed in the first place?
IOW, this patch flags an incorrect setup without giving information on
what possible corrective actions could be taken.

> Fixes: ca8f3611dcf8 ("ASoC: tas2783A: use acpi initialisation table")
> Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> ---
>  sound/soc/codecs/tas2783-sdw.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
> index 8e7376889b6d..6c4cc8868415 100644
> --- a/sound/soc/codecs/tas2783-sdw.c
> +++ b/sound/soc/codecs/tas2783-sdw.c
> @@ -1055,6 +1055,11 @@ static s32 tas_sdw_hw_params(struct snd_pcm_substream *substream,
>  		return -EINVAL;
>  	}
>  
> +	if (!tas_dev->hw_init) {
> +		dev_err(tas_dev->dev, "error playback without init writes");
> +		return -EINVAL;
> +	}
> +
>  	sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
>  	if (!sdw_stream)
>  		return -EINVAL;
> @@ -1427,6 +1432,12 @@ static int tas_port_prep(struct sdw_slave *slave, struct sdw_prepare_ch *prep_ch
>  	addr = SDW_DPN_PREPARECTRL(prep_ch->num);
>  	switch (pre_ops) {
>  	case SDW_OPS_PORT_PRE_PREP:
> +		if (!tas_dev->hw_init) {
> +			dev_err(dev, "error port %d prepare without init writes\n",
> +				prep_ch->num);
> +			return -EINVAL;
> +		}
> +
>  		/*
>  		 * The Function has to be powered before the port can complete
>  		 * channel preparation.  hw_params() does that when a stream is

Re: [PATCH 3/3] ASoC: tas2783-sdw: refuse playback when the init writes failed

[Matthew Schwartz]

On 10/19/26 9:10 AM, Pierre-Louis Bossart wrote:
> On 10/18/26 22:00, Matthew Schwartz wrote:
>> When the SDCA function initialization writes fail, tas_io_init() leaves
>> hw_init false, but the firmware download has already set fw_dl_success,
>> so tas_sdw_hw_params() goes on to play through an amp that is missing
>> part of its setup. On the ASUS ROG Xbox Ally X20 those writes come from
>> the ACPI DisCo initialization table and include the posture, which the
>> BIOS sets differently on each amp.
>>
>> Refuse playback until the init writes have completed as well, and log
>> which of the two steps is missing. A stream that is only prepared again
>> after a resume does not go through hw_params, so check in the port
>> prepare as well.
> 
> Same comment as the previous patch. This patch logs an error message for
> a critical error, which seems fine, but what does it take to make the
> init writes succeed in the first place?
> IOW, this patch flags an incorrect setup without giving information on
> what possible corrective actions could be taken.

Same as the last one, I haven't seen this fail on the X20. The init
writes come from the DisCo table on the X20, or from tas2783_init_seq
on platforms without one, so they'd fail either on a bus error while
the amp attaches or on an entry that the regmap or the amp rejects. A
bus error can clear up, since hw_init stays false and the init runs
again on the next attach, which on the X20 is the next idle wake. A
rejected entry fails every time, and fixing it means knowing which
entry it was, which the log doesn't tell you today, as
sdca_regmap_write_init() just returns the first error. I'll log the
failing address and value in v2 so a report says what needs fixing.

> 
>> Fixes: ca8f3611dcf8 ("ASoC: tas2783A: use acpi initialisation table")
>> Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>> ---
>>  sound/soc/codecs/tas2783-sdw.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
>> index 8e7376889b6d..6c4cc8868415 100644
>> --- a/sound/soc/codecs/tas2783-sdw.c
>> +++ b/sound/soc/codecs/tas2783-sdw.c
>> @@ -1055,6 +1055,11 @@ static s32 tas_sdw_hw_params(struct snd_pcm_substream *substream,
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (!tas_dev->hw_init) {
>> +		dev_err(tas_dev->dev, "error playback without init writes");
>> +		return -EINVAL;
>> +	}
>> +
>>  	sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
>>  	if (!sdw_stream)
>>  		return -EINVAL;
>> @@ -1427,6 +1432,12 @@ static int tas_port_prep(struct sdw_slave *slave, struct sdw_prepare_ch *prep_ch
>>  	addr = SDW_DPN_PREPARECTRL(prep_ch->num);
>>  	switch (pre_ops) {
>>  	case SDW_OPS_PORT_PRE_PREP:
>> +		if (!tas_dev->hw_init) {
>> +			dev_err(dev, "error port %d prepare without init writes\n",
>> +				prep_ch->num);
>> +			return -EINVAL;
>> +		}
>> +
>>  		/*
>>  		 * The Function has to be powered before the port can complete
>>  		 * channel preparation.  hw_params() does that when a stream is
> 

Re: [PATCH 0/3] ASoC: tas2783-sdw: volume and firmware load fixes

[Lee Mann]

On Sun, 18 Oct 2026 at 21:00, Matthew Schwartz
<matthew.schwartz@linux.dev> wrote:
>
> Three tas2783-sdw fixes found while bringing up the ASUS ROG Xbox Ally
> X20 [1], whose two TAS2783 amps share a SoundWire link that is powered
> off when idle.

Tested on an ASUS ProArt PX13 HN7306EA (two TAS2783 on one AMD ACP
link), with the series on broonie/sound for-next (686b3eff60) plus
173cb3147ba2 ("ASoC: tas2783-sdw: split a stereo stream across the two
mono amps"), which this machine needs for stereo and which for-next
does not have yet:

- stereo playback after boot, no ASoC or -EBUSY errors in the log
- volume changes while both amps are runtime suspended work, no errors
- after ~2.5 min of s2idle both amps come back at the level set before
  suspend, with nothing done in userspace

The PX13 firmware has no usable SDCA function type, so it takes the
tas2783_init_seq path; the ACPI init table case in patch 3 is not
exercised here.

Tested-by: Lee Mann <leepaulmann@gmail.com>

One thing you may want to check on the X20: on the PX13, applying the
ACPI init table (via the fallback in thesofproject/linux PR #5904)
together with 173cb3147ba2 left the second amp silent, because the
table puts it on Right while the stereo split hands it a one-channel
payload. If the X20 table also sets a different posture per amp, it
may hit the same once for-next picks up 173cb3147ba2.

Thanks,
Lee