ASoC: codecs: aw88261: fixes and cleanup

PATCH Superseded Val Packett 3 participants 15 messages May 18 – May 22

Subsystems (2)

Sound ASoC

lore

Share & tools

lore

Val Packett

author

2026-05-18 22:09 UTC #0 lore raw

v2 cover ASoC: codecs: aw88261: fixes and cleanup

The Awinic smart speaker/amp drivers were merged in a very "downstream-brained" state, where configuration was only really determined by the binary "firmware" (register list) file instead of properly participating in the ASoC system. Let's start untangling this mess. This series makes aw88261 actually usable on devices like fairphone-fp5, motorola-dubai and xiaomi-pipa.

v2:
- Add missing sample rates to the switch statement (turns out we do have them)
- Process the conversion from requested values to register ones directly in the ASoC ops to make sure EINVAL is returned early, from the right place
- Do not include untested aw88258 fixes.. (why did I think that some support for it was already present in the tree?)
- Do include TDM support, logspam reduction, a newly discovered mask fix
- Remove misfeatures inherited from downstream: async start, fade-in/fade-out (hopefully this won't be too controversial?)
- Also overhaul volume control to make it actually usable as PlaybackVolume

v1: https://lore.kernel.org/all/[email protected]/

~val

---

Val Packett (7):
ASoC: codecs: aw88261: support changing sample rate and bit width
ASoC: codecs: aw88261: add TDM support
ASoC: codecs: aw88261: reduce log spam
ASoC: codecs: aw88261: remove fade in/out on start/stop
ASoC: codecs: aw88261: remove async start
ASoC: codecs: aw88261: fix incorrect masks for boost regs
ASoC: codecs: aw88261: make volume control usable

 sound/soc/codecs/aw88261.c | 612 +++++++++++++++++++------------------
 sound/soc/codecs/aw88261.h | 179 ++++++++++-
 2 files changed, 486 insertions(+), 305 deletions(-)

Signature · 1 line

2.53.0

Val Packett

author

2026-05-18 22:09 UTC #1 lore raw

PATCH v2 1/7 ASoC: codecs: aw88261: support changing sample rate and bit width

The aw88261 driver only worked with 32-bit 48kHz streams so far due to the lack of a proper PLL initialization sequence. Fix by selecting all the necessary PLL settings based on what was passed to us by the hw_params/set_fmt ops. This replaces the strange downstream routine that tries two divider modes in sequence.

Fixes 028a2ae25691 ("ASoC: codecs: Add aw88261 amplifier driver")

Signed-off-by Val Packett <[email protected]>

---
 sound/soc/codecs/aw88261.c | 247 +++++++++++++++++++++++++++----------
 sound/soc/codecs/aw88261.h | 116 ++++++++++++++++-
 2 files changed, 299 insertions(+), 64 deletions(-)

sound/soc/codecs/aw88261.c +184 ~~-63~~

index a6805d5405cd..2064e72b51af 100644
@@ -13,6 +13,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <sound/soc.h>
+#include <sound/pcm_params.h>
 #include "aw88261.h"
 #include "aw88395/aw88395_data_type.h"
 #include "aw88395/aw88395_device.h"

@@ -158,7 +159,7 @@ static int aw88261_dev_get_iis_status(struct aw_device *aw_dev)
 	return ret;
 }
 
-static int aw88261_dev_check_mode1_pll(struct aw_device *aw_dev)
+static int aw88261_dev_check_pll(struct aw_device *aw_dev)
 {
 	int ret, i;

@@ -175,71 +176,48 @@ static int aw88261_dev_check_mode1_pll(struct aw_device *aw_dev)
 	return -EPERM;
 }
 
-static int aw88261_dev_check_mode2_pll(struct aw_device *aw_dev)
-{
-	unsigned int reg_val;
-	int ret, i;
-
-	ret = regmap_read(aw_dev->regmap, AW88261_PLLCTRL1_REG, &reg_val);
-	if (ret)
-		return ret;
-
-	reg_val &= (~AW88261_CCO_MUX_MASK);
-	if (reg_val == AW88261_CCO_MUX_DIVIDED_VALUE) {
-		dev_dbg(aw_dev->dev, "CCO_MUX is already divider");
-		return -EPERM;
-	}
-
-	/* change mode2 */
-	ret = regmap_update_bits(aw_dev->regmap, AW88261_PLLCTRL1_REG,
-			~AW88261_CCO_MUX_MASK, AW88261_CCO_MUX_DIVIDED_VALUE);
-	if (ret)
-		return ret;
-
-	for (i = 0; i < AW88261_DEV_SYSST_CHECK_MAX; i++) {
-		ret = aw88261_dev_get_iis_status(aw_dev);
-		if (ret) {
-			dev_err(aw_dev->dev, "mode2 iis signal check error");
-			usleep_range(AW88261_2000_US, AW88261_2000_US + 10);
-		} else {
-			break;
-		}
-	}
-
-	/* change mode1 */
-	ret = regmap_update_bits(aw_dev->regmap, AW88261_PLLCTRL1_REG,
-			~AW88261_CCO_MUX_MASK, AW88261_CCO_MUX_BYPASS_VALUE);
-	if (ret == 0) {
-		usleep_range(AW88261_2000_US, AW88261_2000_US + 10);
-		for (i = 0; i < AW88261_DEV_SYSST_CHECK_MAX; i++) {
-			ret = aw88261_dev_check_mode1_pll(aw_dev);
-			if (ret) {
-				dev_err(aw_dev->dev, "mode2 switch to mode1, iis signal check error");
-				usleep_range(AW88261_2000_US, AW88261_2000_US + 10);
-			} else {
-				break;
-			}
-		}
-	}
-
-	return ret;
-}
-
-static int aw88261_dev_check_syspll(struct aw_device *aw_dev)
+static int aw88261_dev_configure_syspll(struct aw88261 *aw88261)
 {
+	struct aw_device *aw_dev = aw88261->aw_pa;
 	int ret;
 
-	ret = aw88261_dev_check_mode1_pll(aw_dev);
-	if (ret) {
-		dev_dbg(aw_dev->dev, "mode1 check iis failed try switch to mode2 check");
-		ret = aw88261_dev_check_mode2_pll(aw_dev);
-		if (ret) {
-			dev_err(aw_dev->dev, "mode2 check iis failed");
-			return ret;
-		}
-	}
+	/* PLL divider must be used for 8/16/32 kHz modes */
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_PLLCTRL1_REG,
+			~AW88261_CCO_MUX_MASK, aw88261->cco_mux_value);
+	if (ret)
+		return ret;
 
-	return ret;
+	/* The word clock (WCK) defines the beginning of a frame */
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCTRL1_REG,
+			~AW88261_I2SSR_MASK, aw88261->sr_value);
+	if (ret)
+		return ret;
+
+	/* The bit clock (BCK) defines the length of a frame */
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCTRL1_REG,
+			~AW88261_I2SBCK_MASK, aw88261->bck_value);
+	if (ret)
+		return ret;
+
+	/* The logical frame size is the width of data for 1 slot */
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCTRL1_REG,
+			~AW88261_I2SFS_MASK, aw88261->fs_value);
+	if (ret)
+		return ret;
+
+	/* The I2S interface mode (Philips standard, LSB/MSB justified) */
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCTRL1_REG,
+			~AW88261_I2SMD_MASK, aw88261->md_value);
+	if (ret)
+		return ret;
+
+	/* The polarity of the bit clock (BCK) */
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL_REG,
+			~AW88261_BCKINV_MASK, aw88261->bck_inv_value);
+	if (ret)
+		return ret;
+
+	return aw88261_dev_check_pll(aw_dev);
 }
 
 static int aw88261_dev_check_sysst(struct aw_device *aw_dev)

@@ -558,7 +536,7 @@ static int aw88261_dev_start(struct aw88261 *aw88261)
 	aw88261_dev_pwd(aw_dev, false);
 	usleep_range(AW88261_2000_US, AW88261_2000_US + 10);
 
-	ret = aw88261_dev_check_syspll(aw_dev);
+	ret = aw88261_dev_configure_syspll(aw88261);
 	if (ret) {
 		dev_err(aw_dev->dev, "pll check failed cannot start");
 		goto pll_check_fail;

@@ -712,6 +690,140 @@ static void aw88261_start(struct aw88261 *aw88261, bool sync_start)
 			AW88261_START_WORK_DELAY_MS);
 }
 
+static int aw88261_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct snd_soc_component *component = dai->component;
+	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(component);
+
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_NB_NF:
+		aw88261->bck_inv_value = AW88261_BCKINV_NOT_INVERT_VALUE;
+		break;
+	case SND_SOC_DAIFMT_IB_NF:
+		aw88261->bck_inv_value = AW88261_BCKINV_INVERTED_VALUE;
+		break;
+	default:
+		dev_err(aw88261->aw_pa->dev, "unsupported invert mode 0x%x\n",
+			fmt & SND_SOC_DAIFMT_INV_MASK);
+		return -EINVAL;
+	}
+
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+		aw88261->md_value = AW88261_I2SMD_PHILIPS_STANDARD_VALUE;
+		break;
+	case SND_SOC_DAIFMT_MSB:
+		aw88261->md_value = AW88261_I2SMD_MSB_JUSTIFIED_VALUE;
+		break;
+	case SND_SOC_DAIFMT_LSB:
+		aw88261->md_value = AW88261_I2SMD_LSB_JUSTIFIED_VALUE;
+		break;
+	default:
+		dev_err(aw88261->aw_pa->dev, "unsupported DAI format 0x%x\n",
+			fmt & SND_SOC_DAIFMT_FORMAT_MASK);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int aw88261_hw_params(struct snd_pcm_substream *substream,
+	struct snd_pcm_hw_params *params,
+	struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(component);
+
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		return 0;
+
+	aw88261->cco_mux_value = AW88261_CCO_MUX_BYPASS_VALUE;
+	switch (params_rate(params)) {
+	case 8000:
+		aw88261->sr_value = AW88261_I2SSR_8KHZ_VALUE;
+		aw88261->cco_mux_value = AW88261_CCO_MUX_DIVIDED_VALUE;
+		break;
+	case 11025:
+		aw88261->sr_value = AW88261_I2SSR_11P025KHZ_VALUE;
+		break;
+	case 12000:
+		aw88261->sr_value = AW88261_I2SSR_12KHZ_VALUE;
+		break;
+	case 16000:
+		aw88261->sr_value = AW88261_I2SSR_16KHZ_VALUE;
+		aw88261->cco_mux_value = AW88261_CCO_MUX_DIVIDED_VALUE;
+		break;
+	case 22050:
+		aw88261->sr_value = AW88261_I2SSR_22P05KHZ_VALUE;
+		break;
+	case 24000:
+		aw88261->sr_value = AW88261_I2SSR_24KHZ_VALUE;
+		break;
+	case 32000:
+		aw88261->sr_value = AW88261_I2SSR_32KHZ_VALUE;
+		aw88261->cco_mux_value = AW88261_CCO_MUX_DIVIDED_VALUE;
+		break;
+	case 44100:
+		aw88261->sr_value = AW88261_I2SSR_44P1KHZ_VALUE;
+		break;
+	case 48000:
+		aw88261->sr_value = AW88261_I2SSR_48KHZ_VALUE;
+		break;
+	case 96000:
+		aw88261->sr_value = AW88261_I2SSR_96KHZ_VALUE;
+		break;
+	case 192000:
+		aw88261->sr_value = AW88261_I2SSR_192KHZ_VALUE;
+		break;
+	default:
+		dev_err(aw88261->aw_pa->dev, "unsupported sample rate %d\n",
+			params_rate(params));
+		return -EINVAL;
+	}
+
+	switch (params_width(params)) {
+	case 16:
+		aw88261->fs_value = AW88261_I2SFS_16_BITS_VALUE;
+		break;
+	case 20:
+		aw88261->fs_value = AW88261_I2SFS_20_BITS_VALUE;
+		break;
+	case 24:
+		aw88261->fs_value = AW88261_I2SFS_24_BITS_VALUE;
+		break;
+	case 32:
+		aw88261->fs_value = AW88261_I2SFS_32_BITS_VALUE;
+		break;
+	default:
+		dev_err(aw88261->aw_pa->dev, "unsupported bit width %d\n",
+			params_width(params));
+		return -EINVAL;
+	}
+
+	switch (params_physical_width(params)) {
+	case 16:
+		aw88261->bck_value = AW88261_I2SBCK_32FS_VALUE;
+		break;
+	case 24:
+		aw88261->bck_value = AW88261_I2SBCK_48FS_VALUE;
+		break;
+	case 32:
+		aw88261->bck_value = AW88261_I2SBCK_64FS_VALUE;
+		break;
+	default:
+		dev_err(aw88261->aw_pa->dev, "unsupported physical bit width %d\n",
+			params_physical_width(params));
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct snd_soc_dai_ops aw88261_dai_ops = {
+	.set_fmt = aw88261_set_fmt,
+	.hw_params = aw88261_hw_params,
+};
+
 static struct snd_soc_dai_driver aw88261_dai[] = {
 	{
 		.name = "aw88261-aif",

@@ -730,6 +842,7 @@ static struct snd_soc_dai_driver aw88261_dai[] = {
 			.rates = AW88261_RATES,
 			.formats = AW88261_FORMATS,
 		},
+		.ops = &aw88261_dai_ops,
 	},
 };

@@ -1249,6 +1362,14 @@ static int aw88261_i2c_probe(struct i2c_client *i2c)
 	if (!aw88261)
 		return -ENOMEM;
 
+	/* set defaults */
+	aw88261->sr_value = AW88261_I2SSR_48KHZ_VALUE;
+	aw88261->cco_mux_value = AW88261_CCO_MUX_BYPASS_VALUE;
+	aw88261->fs_value = AW88261_I2SFS_24_BITS_VALUE;
+	aw88261->bck_value = AW88261_I2SBCK_64FS_VALUE;
+	aw88261->bck_inv_value = AW88261_BCKINV_NOT_INVERT_VALUE;
+	aw88261->md_value = AW88261_I2SMD_PHILIPS_STANDARD_VALUE;
+
 	mutex_init(&aw88261->lock);
 
 	i2c_set_clientdata(i2c, aw88261);

sound/soc/codecs/aw88261.h +115 -1

141 lines not on this page · raw message

Signature · 1 line

2.53.0

Val Packett

author

2026-05-18 22:09 UTC #2 lore raw

PATCH v2 2/7 ASoC: codecs: aw88261: add TDM support

In reply to Val Packett

This amp supports TDM mode, so implement the set_tdm_slot operation to let the SoC driver configure the TDM slot number, width, and masks.

Signed-off-by Val Packett <[email protected]>

---
 sound/soc/codecs/aw88261.c | 88 ++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/aw88261.h | 53 +++++++++++++++++++++++
 2 files changed, 141 insertions(+)

sound/soc/codecs/aw88261.c +88 -0

index 2064e72b51af..7b0e778f5a25 100644
@@ -10,6 +10,7 @@
 
 #include <linux/i2c.h>
 #include <linux/firmware.h>
+#include <linux/bitops.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <sound/soc.h>

@@ -181,6 +182,30 @@ static int aw88261_dev_configure_syspll(struct aw88261 *aw88261)
 	struct aw_device *aw_dev = aw88261->aw_pa;
 	int ret;
 
+	/* Configure TDM slots (I2S is represented as no slots) */
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCTRL2_REG,
+			~AW88261_SLOT_NUM_MASK, aw88261->slot_num_value);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCTRL2_REG,
+			~AW88261_I2S_TX_SLOTVLD_MASK,
+			aw88261->tx_slotvld_mask);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCTRL2_REG,
+			~AW88261_I2S_RXL_SLOTVLD_MASK,
+			aw88261->rxl_slotvld_mask);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCTRL2_REG,
+			~AW88261_I2S_RXR_SLOTVLD_MASK,
+			aw88261->rxr_slotvld_mask);
+	if (ret)
+		return ret;
+
 	/* PLL divider must be used for 8/16/32 kHz modes */
 	ret = regmap_update_bits(aw_dev->regmap, AW88261_PLLCTRL1_REG,
 			~AW88261_CCO_MUX_MASK, aw88261->cco_mux_value);

@@ -710,9 +735,11 @@ static int aw88261_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_I2S:
+	case SND_SOC_DAIFMT_DSP_A:
 		aw88261->md_value = AW88261_I2SMD_PHILIPS_STANDARD_VALUE;
 		break;
 	case SND_SOC_DAIFMT_MSB:
+	case SND_SOC_DAIFMT_DSP_B:
 		aw88261->md_value = AW88261_I2SMD_MSB_JUSTIFIED_VALUE;
 		break;
 	case SND_SOC_DAIFMT_LSB:

@@ -819,9 +846,68 @@ static int aw88261_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int aw88261_set_tdm_slot(struct snd_soc_dai *dai,
+	unsigned int tx_mask, unsigned int rx_mask, int slots, int slot_width)
+{
+	struct snd_soc_component *component = dai->component;
+	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(component);
+	int bit;
+
+	switch (slots) {
+	case 0:
+		aw88261->slot_num_value = AW88261_SLOT_NUM_I2S_MODE_VALUE;
+		break;
+	case 1:
+		aw88261->slot_num_value = AW88261_SLOT_NUM_TDM1S_VALUE;
+		break;
+	case 2:
+		aw88261->slot_num_value = AW88261_SLOT_NUM_TDM2S_VALUE;
+		break;
+	case 4:
+		aw88261->slot_num_value = AW88261_SLOT_NUM_TDM4S_VALUE;
+		break;
+	case 6:
+		aw88261->slot_num_value = AW88261_SLOT_NUM_TDM6S_VALUE;
+		break;
+	case 8:
+		aw88261->slot_num_value = AW88261_SLOT_NUM_TDM8S_VALUE;
+		break;
+	case 16:
+		aw88261->slot_num_value = AW88261_SLOT_NUM_TDM16S_VALUE;
+		break;
+	default:
+		dev_err(aw88261->aw_pa->dev, "unsupported slot count %d\n", slots);
+		return -EINVAL;
+	}
+
+	if (tx_mask != 0) {
+		if ((bit = __ffs(tx_mask)) > 15)
+			return -EINVAL;
+
+		aw88261->tx_slotvld_mask = bit << AW88261_I2S_TX_SLOTVLD_START_BIT;
+	}
+
+	if (rx_mask != 0) {
+		if ((bit = __ffs(rx_mask)) > 15)
+			return -EINVAL;
+
+		aw88261->rxl_slotvld_mask = bit << AW88261_I2S_RXL_SLOTVLD_START_BIT;
+	}
+
+	if ((rx_mask & ~BIT(bit)) != 0) {
+		if ((bit = __ffs(rx_mask & ~BIT(bit))) > 15)
+			return -EINVAL;
+
+		aw88261->rxr_slotvld_mask = bit << AW88261_I2S_RXR_SLOTVLD_START_BIT;
+	}
+
+	return 0;
+}
+
 static const struct snd_soc_dai_ops aw88261_dai_ops = {
 	.set_fmt = aw88261_set_fmt,
 	.hw_params = aw88261_hw_params,
+	.set_tdm_slot	= aw88261_set_tdm_slot,
 };
 
 static struct snd_soc_dai_driver aw88261_dai[] = {

@@ -1363,12 +1449,14 @@ static int aw88261_i2c_probe(struct i2c_client *i2c)
 		return -ENOMEM;
 
 	/* set defaults */
+	aw88261->slot_num_value = AW88261_SLOT_NUM_I2S_MODE_VALUE;
 	aw88261->sr_value = AW88261_I2SSR_48KHZ_VALUE;
 	aw88261->cco_mux_value = AW88261_CCO_MUX_BYPASS_VALUE;
 	aw88261->fs_value = AW88261_I2SFS_24_BITS_VALUE;
 	aw88261->bck_value = AW88261_I2SBCK_64FS_VALUE;
 	aw88261->bck_inv_value = AW88261_BCKINV_NOT_INVERT_VALUE;
 	aw88261->md_value = AW88261_I2SMD_PHILIPS_STANDARD_VALUE;
+	aw88261->rxr_slotvld_mask = 1 << AW88261_I2S_RXR_SLOTVLD_START_BIT;
 
 	mutex_init(&aw88261->lock);

sound/soc/codecs/aw88261.h +53 -0

65 lines not on this page · raw message

Signature · 1 line

2.53.0

Mark Brown

maintainer

2026-05-19 16:06 UTC #3 lore raw

Re: [PATCH v2 2/7] ASoC: codecs: aw88261: add TDM support

sound/soc/codecs/aw88261.c static int aw88261_hw_params(struct snd_pcm_substream *substream, · line 851

Quoted 6 lines · Val Packett

This amp supports TDM mode, so implement the set_tdm_slot operation to let the SoC driver configure the TDM slot number, width, and masks.
sound/soc/codecs/aw88261.c +3 -0
+static int aw88261_set_tdm_slot(struct snd_soc_dai *dai,
+	unsigned int tx_mask, unsigned int rx_mask, int slots, int slot_width)
+{

In spite of the changelog this doesn't actually seem to use slot_width? It should.

Note also that we should disable TDM if slots == 0.

Val Packett

author

2026-05-19 22:32 UTC #4 lore raw

Re: [PATCH v2 2/7] ASoC: codecs: aw88261: add TDM support

Quoted 8 lines · Mark Brown

sound/soc/codecs/aw88261.c static int aw88261_hw_params(struct snd_pcm_substream *substream, · line 851
Quoted 5 lines · Val Packett
This amp supports TDM mode, so implement the set_tdm_slot operation to let the SoC driver configure the TDM slot number, width, and masks.
sound/soc/codecs/aw88261.c +3 -0
+static int aw88261_set_tdm_slot(struct snd_soc_dai *dai,
+	unsigned int tx_mask, unsigned int rx_mask, int slots, int slot_width)
+{
In spite of the changelog this doesn't actually seem to use slot_width? It should.

Right, I hadn't exactly figured out where that slots in (ha)..

The datasheet seems to say that I2SBCK is slot width, but there are only 3 options for it (32/48/64) and I currently select the one that matches 2*physical_width.

Grepping the tree I'm seeing that slot_width is basically the same thing as physical width. Should slot_width from set_tdm_slot take precedence over the params_physical_width() from hw_params? Should I just have both callbacks set the variable and whichever was called last "wins"?

Quoted 1 line

Note also that we should disable TDM if slots == 0.

This is pretty much already the case.. but I guess I should also reset the masks specifically to the i2s default (2nd channel to R). Are masks also zero when slots == 0?

Thanks,
~val

Mark Brown

maintainer

2026-05-20 12:18 UTC #5 lore raw

Re: [PATCH v2 2/7] ASoC: codecs: aw88261: add TDM support

Quoted 7 lines · Val Packett

Quoted 2 lines · Mark Brown
In spite of the changelog this doesn't actually seem to use slot_width? It should.
Right, I hadn't exactly figured out where that slots in (ha)..

Quoted 8 lines

The datasheet seems to say that I2SBCK is slot width, but there are only 3 options for it (32/48/64) and I currently select the one that matches 2*physical_width.
Grepping the tree I'm seeing that slot_width is basically the same thing as physical width. Should slot_width from set_tdm_slot take precedence over the params_physical_width() from hw_params? Should I just have both callbacks set the variable and whichever was called last "wins"?

If TDM is configured then hw_params should not cause the number of bits on the bus to vary at all, the clocking of the bus is fixed by the TDM configuration.

Quoted 5 lines

Quoted 1 line
Note also that we should disable TDM if slots == 0.
This is pretty much already the case.. but I guess I should also reset the masks specifically to the i2s default (2nd channel to R). Are masks also zero when slots == 0?

They should be.

Val Packett

author

2026-05-18 22:09 UTC #6 lore raw

PATCH v2 3/7 ASoC: codecs: aw88261: reduce log spam

In reply to Val Packett

This driver would create a wall of logspam during initialization due to e.g. the PLL not being ready while waiting for it to stabilize. Change intermediate dev_err() calls to dev_dbg() to reduce the noise.

While here, log the detected chip ID when that check fails.

Signed-off-by Val Packett <[email protected]>

---
 sound/soc/codecs/aw88261.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

sound/soc/codecs/aw88261.c +14 ~~-10~~

index 7b0e778f5a25..d1fc3403cb5a 100644
@@ -153,7 +153,7 @@ static int aw88261_dev_get_iis_status(struct aw_device *aw_dev)
 	if (ret)
 		return ret;
 	if ((reg_val & AW88261_BIT_PLL_CHECK) != AW88261_BIT_PLL_CHECK) {
-		dev_err(aw_dev->dev, "check pll lock fail,reg_val:0x%04x", reg_val);
+		dev_dbg(aw_dev->dev, "check pll lock fail,reg_val:0x%04x", reg_val);
 		return -EINVAL;
 	}

@@ -167,7 +167,7 @@ static int aw88261_dev_check_pll(struct aw_device *aw_dev)
 	for (i = 0; i < AW88261_DEV_SYSST_CHECK_MAX; i++) {
 		ret = aw88261_dev_get_iis_status(aw_dev);
 		if (ret) {
-			dev_err(aw_dev->dev, "mode1 iis signal check error");
+			dev_dbg(aw_dev->dev, "mode1 iis signal check error");
 			usleep_range(AW88261_2000_US, AW88261_2000_US + 10);
 		} else {
 			return ret;

@@ -259,7 +259,7 @@ static int aw88261_dev_check_sysst(struct aw_device *aw_dev)
 		check_val = reg_val & (~AW88261_BIT_SYSST_CHECK_MASK)
 							& AW88261_BIT_SYSST_CHECK;
 		if (check_val != AW88261_BIT_SYSST_CHECK) {
-			dev_err(aw_dev->dev, "check sysst fail, reg_val=0x%04x, check:0x%x",
+			dev_dbg(aw_dev->dev, "check sysst fail, reg_val=0x%04x, check:0x%x",
 				reg_val, AW88261_BIT_SYSST_CHECK);
 			usleep_range(AW88261_2000_US, AW88261_2000_US + 10);
 		} else {

@@ -553,7 +553,7 @@ static int aw88261_dev_start(struct aw88261 *aw88261)
 	int ret;
 
 	if (aw_dev->status == AW88261_DEV_PW_ON) {
-		dev_info(aw_dev->dev, "already power on");
+		dev_dbg(aw_dev->dev, "already power on");
 		return 0;
 	}

@@ -563,7 +563,7 @@ static int aw88261_dev_start(struct aw88261 *aw88261)
 
 	ret = aw88261_dev_configure_syspll(aw88261);
 	if (ret) {
-		dev_err(aw_dev->dev, "pll check failed cannot start");
+		dev_dbg(aw_dev->dev, "pll check failed");
 		goto pll_check_fail;
 	}

@@ -574,7 +574,7 @@ static int aw88261_dev_start(struct aw88261 *aw88261)
 	/* check i2s status */
 	ret = aw88261_dev_check_sysst(aw_dev);
 	if (ret) {
-		dev_err(aw_dev->dev, "sysst check failed");
+		dev_dbg(aw_dev->dev, "sysst check failed");
 		goto sysst_check_fail;
 	}

@@ -675,18 +675,22 @@ static void aw88261_start_pa(struct aw88261 *aw88261)
 	for (i = 0; i < AW88261_START_RETRIES; i++) {
 		ret = aw88261_reg_update(aw88261, aw88261->phase_sync);
 		if (ret) {
-			dev_err(aw88261->aw_pa->dev, "fw update failed, cnt:%d\n", i);
+			dev_dbg(aw88261->aw_pa->dev,
+				"aw88261_reg_update failed, cnt:%d, ret:%d\n", i, ret);
 			continue;
 		}
 		ret = aw88261_dev_start(aw88261);
 		if (ret) {
-			dev_err(aw88261->aw_pa->dev, "aw88261 device start failed. retry = %d", i);
+			dev_dbg(aw88261->aw_pa->dev,
+				"aw88261_dev_start failed, cnt:%d, ret:%d\n", i, ret);
 			continue;
 		} else {
-			dev_info(aw88261->aw_pa->dev, "start success\n");
+			dev_dbg(aw88261->aw_pa->dev, "start success\n");
 			break;
 		}
 	}
+	if (ret != 0)
+		dev_err(aw88261->aw_pa->dev, "start failure (%d)\n", ret);
 }
 
 static void aw88261_startup_work(struct work_struct *work)

@@ -1407,7 +1411,7 @@ static int aw88261_init(struct aw88261 *aw88261, struct i2c_client *i2c, struct
 		return ret;
 	}
 	if (chip_id != AW88261_CHIP_ID) {
-		dev_err(&i2c->dev, "unsupported device");
+		dev_err(&i2c->dev, "unsupported device id = %x", chip_id);
 		return -ENXIO;
 	}

Signature · 1 line

2.53.0

Val Packett

author

2026-05-18 22:10 UTC #7 lore raw

PATCH v2 4/7 ASoC: codecs: aw88261: remove fade in/out on start/stop

In reply to Val Packett

This "feature" was copied from downstream, but it does not belong in the kernel at all. Remove it to simplify the driver.

Signed-off-by Val Packett <[email protected]>

---
 sound/soc/codecs/aw88261.c | 158 +------------------------------------
 sound/soc/codecs/aw88261.h |   3 -
 2 files changed, 2 insertions(+), 159 deletions(-)

sound/soc/codecs/aw88261.c +2 ~~-156~~

index d1fc3403cb5a..ba2a0b0fc296 100644
@@ -45,50 +45,6 @@ static void aw88261_dev_set_volume(struct aw_device *aw_dev, unsigned int value)
 	regmap_write(aw_dev->regmap, AW88261_SYSCTRL2_REG, real_value);
 }
 
-static void aw88261_dev_fade_in(struct aw_device *aw_dev)
-{
-	struct aw_volume_desc *desc = &aw_dev->volume_desc;
-	int fade_in_vol = desc->ctl_volume;
-	int fade_step = aw_dev->fade_step;
-	int i;
-
-	if (fade_step == 0 || aw_dev->fade_in_time == 0) {
-		aw88261_dev_set_volume(aw_dev, fade_in_vol);
-		return;
-	}
-
-	for (i = AW88261_MUTE_VOL; i >= fade_in_vol; i -= fade_step) {
-		aw88261_dev_set_volume(aw_dev, i);
-		usleep_range(aw_dev->fade_in_time,
-					aw_dev->fade_in_time + 10);
-	}
-
-	if (i != fade_in_vol)
-		aw88261_dev_set_volume(aw_dev, fade_in_vol);
-}
-
-static void aw88261_dev_fade_out(struct aw_device *aw_dev)
-{
-	struct aw_volume_desc *desc = &aw_dev->volume_desc;
-	int fade_step = aw_dev->fade_step;
-	int i;
-
-	if (fade_step == 0 || aw_dev->fade_out_time == 0) {
-		aw88261_dev_set_volume(aw_dev, AW88261_MUTE_VOL);
-		return;
-	}
-
-	for (i = desc->ctl_volume; i <= AW88261_MUTE_VOL; i += fade_step) {
-		aw88261_dev_set_volume(aw_dev, i);
-		usleep_range(aw_dev->fade_out_time, aw_dev->fade_out_time + 10);
-	}
-
-	if (i != AW88261_MUTE_VOL) {
-		aw88261_dev_set_volume(aw_dev, AW88261_MUTE_VOL);
-		usleep_range(aw_dev->fade_out_time, aw_dev->fade_out_time + 10);
-	}
-}
-
 static void aw88261_dev_i2s_tx_enable(struct aw_device *aw_dev, bool flag)
 {
 	if (flag)

@@ -122,13 +78,13 @@ static void aw88261_dev_amppd(struct aw_device *aw_dev, bool amppd)
 static void aw88261_dev_mute(struct aw_device *aw_dev, bool is_mute)
 {
 	if (is_mute) {
-		aw88261_dev_fade_out(aw_dev);
+		aw88261_dev_set_volume(aw_dev, AW88261_MUTE_VOL);
 		regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL_REG,
 				~AW88261_HMUTE_MASK, AW88261_HMUTE_ENABLE_VALUE);
 	} else {
 		regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL_REG,
 				~AW88261_HMUTE_MASK, AW88261_HMUTE_DISABLE_VALUE);
-		aw88261_dev_fade_in(aw_dev);
+		aw88261_dev_set_volume(aw_dev, aw_dev->volume_desc.ctl_volume);
 	}
 }

@@ -936,75 +892,6 @@ static struct snd_soc_dai_driver aw88261_dai[] = {
 	},
 };
 
-static int aw88261_get_fade_in_time(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
-	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(component);
-	struct aw_device *aw_dev = aw88261->aw_pa;
-
-	ucontrol->value.integer.value[0] = aw_dev->fade_in_time;
-
-	return 0;
-}
-
-static int aw88261_set_fade_in_time(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
-	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(component);
-	struct soc_mixer_control *mc =
-		(struct soc_mixer_control *)kcontrol->private_value;
-	struct aw_device *aw_dev = aw88261->aw_pa;
-	int time;
-
-	time = ucontrol->value.integer.value[0];
-
-	if (time < mc->min || time > mc->max)
-		return -EINVAL;
-
-	if (time != aw_dev->fade_in_time) {
-		aw_dev->fade_in_time = time;
-		return 1;
-	}
-
-	return 0;
-}
-
-static int aw88261_get_fade_out_time(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
-	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(component);
-	struct aw_device *aw_dev = aw88261->aw_pa;
-
-	ucontrol->value.integer.value[0] = aw_dev->fade_out_time;
-
-	return 0;
-}
-
-static int aw88261_set_fade_out_time(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
-	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(component);
-	struct soc_mixer_control *mc =
-		(struct soc_mixer_control *)kcontrol->private_value;
-	struct aw_device *aw_dev = aw88261->aw_pa;
-	int time;
-
-	time = ucontrol->value.integer.value[0];
-	if (time < mc->min || time > mc->max)
-		return -EINVAL;
-
-	if (time != aw_dev->fade_out_time) {
-		aw_dev->fade_out_time = time;
-		return 1;
-	}
-
-	return 0;
-}
-
 static int aw88261_dev_set_profile_index(struct aw_device *aw_dev, int index)
 {
 	/* check the index whether is valid */

@@ -1128,48 +1015,10 @@ static int aw88261_volume_set(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
-static int aw88261_get_fade_step(struct snd_kcontrol *kcontrol,
-				struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_component *codec = snd_kcontrol_chip(kcontrol);
-	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(codec);
-
-	ucontrol->value.integer.value[0] = aw88261->aw_pa->fade_step;
-
-	return 0;
-}
-
-static int aw88261_set_fade_step(struct snd_kcontrol *kcontrol,
-				struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_component *codec = snd_kcontrol_chip(kcontrol);
-	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(codec);
-	struct soc_mixer_control *mc =
-		(struct soc_mixer_control *)kcontrol->private_value;
-	int value;
-
-	value = ucontrol->value.integer.value[0];
-	if (value < mc->min || value > mc->max)
-		return -EINVAL;
-
-	if (aw88261->aw_pa->fade_step != value) {
-		aw88261->aw_pa->fade_step = value;
-		return 1;
-	}
-
-	return 0;
-}
-
 static const struct snd_kcontrol_new aw88261_controls[] = {
 	SOC_SINGLE_EXT("PCM Playback Volume", AW88261_SYSCTRL2_REG,
 		6, AW88261_MUTE_VOL, 0, aw88261_volume_get,
 		aw88261_volume_set),
-	SOC_SINGLE_EXT("Fade Step", 0, 0, AW88261_MUTE_VOL, 0,
-		aw88261_get_fade_step, aw88261_set_fade_step),
-	SOC_SINGLE_EXT("Volume Ramp Up Step", 0, 0, FADE_TIME_MAX, FADE_TIME_MIN,
-		aw88261_get_fade_in_time, aw88261_set_fade_in_time),
-	SOC_SINGLE_EXT("Volume Ramp Down Step", 0, 0, FADE_TIME_MAX, FADE_TIME_MIN,
-		aw88261_get_fade_out_time, aw88261_set_fade_out_time),
 	AW88261_PROFILE_EXT("Profile Set", aw88261_profile_info,
 		aw88261_profile_get, aw88261_profile_set),
 };

@@ -1260,8 +1109,6 @@ static int aw88261_dev_init(struct aw88261 *aw88261, struct aw_container *aw_cfg
 	if (ret)
 		return ret;
 
-	aw_dev->fade_in_time = AW88261_500_US;
-	aw_dev->fade_out_time = AW88261_500_US;
 	aw_dev->prof_cur = AW88261_INIT_PROFILE;
 	aw_dev->prof_index = AW88261_INIT_PROFILE;

@@ -1432,7 +1279,6 @@ static int aw88261_init(struct aw88261 *aw88261, struct i2c_client *i2c, struct
 	aw_dev->prof_info.prof_type = AW88395_DEV_NONE_TYPE_ID;
 	aw_dev->channel = 0;
 	aw_dev->fw_status = AW88261_DEV_FW_FAILED;
-	aw_dev->fade_step = AW88261_VOLUME_STEP_DB;
 	aw_dev->volume_desc.ctl_volume = AW88261_VOL_DEFAULT_VALUE;
 	aw_dev->volume_desc.mute_volume = AW88261_MUTE_VOL;
 	aw88261_parse_channel_dt(aw88261);

sound/soc/codecs/aw88261.h +0 -3

index 02820795d434..12c98f01fcfe 100644
@@ -531,9 +531,6 @@
 			SNDRV_PCM_FMTBIT_S24_LE | \
 			SNDRV_PCM_FMTBIT_S32_LE)
 
-#define FADE_TIME_MAX			100000
-#define FADE_TIME_MIN			0
-
 #define AW88261_DEV_DEFAULT_CH		(0)
 #define AW88261_ACF_FILE		"aw88261_acf.bin"
 #define AW88261_DEV_SYSST_CHECK_MAX	(10)

Signature · 1 line

2.53.0

Val Packett

author

2026-05-18 22:10 UTC #8 lore raw

PATCH v2 5/7 ASoC: codecs: aw88261: remove async start

In reply to Val Packett

Codec drivers are not supposed to do anything like this. The result was that the first second or so of playback was essentially inaudible, and very short alert sounds could be missed entirely. Let's not do this.

Signed-off-by Val Packett <[email protected]>

---
 sound/soc/codecs/aw88261.c | 33 ++++-----------------------------
 sound/soc/codecs/aw88261.h |  6 ------
 2 files changed, 4 insertions(+), 35 deletions(-)

sound/soc/codecs/aw88261.c +4 ~~-29~~

index ba2a0b0fc296..e0644fb445e5 100644
@@ -649,17 +649,7 @@ static void aw88261_start_pa(struct aw88261 *aw88261)
 		dev_err(aw88261->aw_pa->dev, "start failure (%d)\n", ret);
 }
 
-static void aw88261_startup_work(struct work_struct *work)
-{
-	struct aw88261 *aw88261 =
-		container_of(work, struct aw88261, start_work.work);
-
-	mutex_lock(&aw88261->lock);
-	aw88261_start_pa(aw88261);
-	mutex_unlock(&aw88261->lock);
-}
-
-static void aw88261_start(struct aw88261 *aw88261, bool sync_start)
+static void aw88261_start(struct aw88261 *aw88261)
 {
 	if (aw88261->aw_pa->fw_status != AW88261_DEV_FW_OK)
 		return;

@@ -667,12 +657,7 @@ static void aw88261_start(struct aw88261 *aw88261, bool sync_start)
 	if (aw88261->aw_pa->status == AW88261_DEV_PW_ON)
 		return;
 
-	if (sync_start == AW88261_SYNC_START)
-		aw88261_start_pa(aw88261);
-	else
-		queue_delayed_work(system_dfl_wq,
-			&aw88261->start_work,
-			AW88261_START_WORK_DELAY_MS);
+	aw88261_start_pa(aw88261);
 }
 
 static int aw88261_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)

@@ -970,7 +955,7 @@ static int aw88261_profile_set(struct snd_kcontrol *kcontrol,
 
 	if (aw88261->aw_pa->status) {
 		aw88261_dev_stop(aw88261->aw_pa);
-		aw88261_start(aw88261, AW88261_SYNC_START);
+		aw88261_start(aw88261);
 	}
 
 	mutex_unlock(&aw88261->lock);

@@ -1032,7 +1017,7 @@ static int aw88261_playback_event(struct snd_soc_dapm_widget *w,
 	mutex_lock(&aw88261->lock);
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
-		aw88261_start(aw88261, AW88261_ASYNC_START);
+		aw88261_start(aw88261);
 		break;
 	case SND_SOC_DAPM_POST_PMD:
 		aw88261_dev_stop(aw88261->aw_pa);

@@ -1192,8 +1177,6 @@ static int aw88261_codec_probe(struct snd_soc_component *component)
 	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(component);
 	int ret;
 
-	INIT_DELAYED_WORK(&aw88261->start_work, aw88261_startup_work);
-
 	ret = aw88261_request_firmware_file(aw88261);
 	if (ret)
 		return dev_err_probe(aw88261->aw_pa->dev, ret,

@@ -1217,16 +1200,8 @@ static int aw88261_codec_probe(struct snd_soc_component *component)
 	return ret;
 }
 
-static void aw88261_codec_remove(struct snd_soc_component *aw_codec)
-{
-	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(aw_codec);
-
-	cancel_delayed_work_sync(&aw88261->start_work);
-}
-
 static const struct snd_soc_component_driver soc_codec_dev_aw88261 = {
 	.probe = aw88261_codec_probe,
-	.remove = aw88261_codec_remove,
 };
 
 static void aw88261_parse_channel_dt(struct aw88261 *aw88261)

sound/soc/codecs/aw88261.h +0 -6

index 12c98f01fcfe..1b1beba6a26b 100644
@@ -555,11 +555,6 @@
 	.put = profile_set, \
 }
 
-enum {
-	AW88261_SYNC_START = 0,
-	AW88261_ASYNC_START,
-};
-
 enum aw88261_id {
 	AW88261_CHIP_ID = 0x2113,
 };

@@ -594,7 +589,6 @@ struct aw88261 {
 	struct aw_device *aw_pa;
 	struct mutex lock;
 	struct gpio_desc *reset_gpio;
-	struct delayed_work start_work;
 	struct regmap *regmap;
 	struct aw_container *aw_cfg;

Signature · 1 line

2.53.0

Val Packett

author

2026-05-18 22:10 UTC #9 lore raw

PATCH v2 6/7 ASoC: codecs: aw88261: fix incorrect masks for boost regs

In reply to Val Packett

The boost-related register fields used in aw88261_reg_force_set use the exact same definitions as the rest of the fields, where the mask must be inverted when passing it to regmap_update_bits, but they weren't inverted here.

Fixes 028a2ae25691 ("ASoC: codecs: Add aw88261 amplifier driver")

Signed-off-by Val Packett <[email protected]>

---
 sound/soc/codecs/aw88261.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

sound/soc/codecs/aw88261.c +6 -6

index e0644fb445e5..adc728e45f57 100644
@@ -243,22 +243,22 @@ static void aw88261_reg_force_set(struct aw88261 *aw88261)
 	if (aw88261->frcset_en == AW88261_FRCSET_ENABLE) {
 		/* set FORCE_PWM */
 		regmap_update_bits(aw88261->regmap, AW88261_BSTCTRL3_REG,
-				AW88261_FORCE_PWM_MASK, AW88261_FORCE_PWM_FORCEMINUS_PWM_VALUE);
+				~AW88261_FORCE_PWM_MASK, AW88261_FORCE_PWM_FORCEMINUS_PWM_VALUE);
 		/* set BOOST_OS_WIDTH */
 		regmap_update_bits(aw88261->regmap, AW88261_BSTCTRL5_REG,
-				AW88261_BST_OS_WIDTH_MASK, AW88261_BST_OS_WIDTH_50NS_VALUE);
+				~AW88261_BST_OS_WIDTH_MASK, AW88261_BST_OS_WIDTH_50NS_VALUE);
 		/* set BURST_LOOPR */
 		regmap_update_bits(aw88261->regmap, AW88261_BSTCTRL6_REG,
-				AW88261_BST_LOOPR_MASK, AW88261_BST_LOOPR_340K_VALUE);
+				~AW88261_BST_LOOPR_MASK, AW88261_BST_LOOPR_340K_VALUE);
 		/* set RSQN_DLY */
 		regmap_update_bits(aw88261->regmap, AW88261_BSTCTRL7_REG,
-				AW88261_RSQN_DLY_MASK, AW88261_RSQN_DLY_35NS_VALUE);
+				~AW88261_RSQN_DLY_MASK, AW88261_RSQN_DLY_35NS_VALUE);
 		/* set BURST_SSMODE */
 		regmap_update_bits(aw88261->regmap, AW88261_BSTCTRL8_REG,
-				AW88261_BURST_SSMODE_MASK, AW88261_BURST_SSMODE_FAST_VALUE);
+				~AW88261_BURST_SSMODE_MASK, AW88261_BURST_SSMODE_FAST_VALUE);
 		/* set BST_BURST */
 		regmap_update_bits(aw88261->regmap, AW88261_BSTCTRL9_REG,
-				AW88261_BST_BURST_MASK, AW88261_BST_BURST_30MA_VALUE);
+				~AW88261_BST_BURST_MASK, AW88261_BST_BURST_30MA_VALUE);
 	} else {
 		dev_dbg(aw88261->aw_pa->dev, "needn't set reg value");
 	}

Signature · 1 line

2.53.0

Val Packett

author

2026-05-18 22:10 UTC #10 lore raw

PATCH v2 7/7 ASoC: codecs: aw88261: make volume control usable

In reply to Val Packett

- Invert the value to match userspace expectations (in the hardware, positive numbers represent negative dB attenuation)
- Provide TLV metadata for the dB scale (and divide the raw values by 2 as the excessive precision used by HW is not representable in TLV)
- Do not unnecessarily reset the volume while switching profiles
- Simplify aw88261_dev_set_volume using regmap_update_bits
- Do not add the initial volume from the profile to the requested volume as that would throw off the dB mapping (if a lower max limit is desired, it can be set in the UCM profile in userspace)

With this change, it's actually possible to use this hardware volume control as PlaybackVolume in an ALSA UCM profile.

Fixes 028a2ae25691 ("ASoC: codecs: Add aw88261 amplifier driver")

Signed-off-by Val Packett <[email protected]>

---
 sound/soc/codecs/aw88261.c | 52 +++++++++++++++-----------------------
 sound/soc/codecs/aw88261.h |  1 -
 2 files changed, 21 insertions(+), 32 deletions(-)

sound/soc/codecs/aw88261.c +21 ~~-31~~

index adc728e45f57..0e6b2dfe5db9 100644
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
 #include <sound/soc.h>
 #include <sound/pcm_params.h>
+#include <sound/tlv.h>
 #include "aw88261.h"
 #include "aw88395/aw88395_data_type.h"
 #include "aw88395/aw88395_device.h"

@@ -29,20 +30,10 @@ static const struct regmap_config aw88261_remap_config = {
 
 static void aw88261_dev_set_volume(struct aw_device *aw_dev, unsigned int value)
 {
-	struct aw_volume_desc *vol_desc = &aw_dev->volume_desc;
-	unsigned int real_value, volume;
-	unsigned int reg_value;
+	unsigned int volume = min(value, (unsigned int)AW88261_MUTE_VOL);
 
-	volume = min((value + vol_desc->init_volume), (unsigned int)AW88261_MUTE_VOL);
-	real_value = DB_TO_REG_VAL(volume);
-
-	regmap_read(aw_dev->regmap, AW88261_SYSCTRL2_REG, &reg_value);
-
-	real_value = (real_value | (reg_value & AW88261_VOL_START_MASK));
-
-	dev_dbg(aw_dev->dev, "value 0x%x , real_value:0x%x", value, real_value);
-
-	regmap_write(aw_dev->regmap, AW88261_SYSCTRL2_REG, real_value);
+	regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL2_REG,
+		~AW88261_VOL_MASK, DB_TO_REG_VAL(volume));
 }
 
 static void aw88261_dev_i2s_tx_enable(struct aw_device *aw_dev, bool flag)

@@ -424,17 +415,7 @@ static int aw88261_dev_reg_update(struct aw88261 *aw88261,
 			break;
 	}
 
-	ret = aw88261_dev_set_vcalb(aw_dev);
-	if (ret)
-		return ret;
-
-	if (aw_dev->prof_cur != aw_dev->prof_index)
-		vol_desc->ctl_volume = 0;
-
-	/* keep min volume */
-	aw88261_dev_set_volume(aw_dev, vol_desc->mute_volume);
-
-	return ret;
+	return aw88261_dev_set_vcalb(aw_dev);
 }
 
 static int aw88261_dev_get_prof_name(struct aw_device *aw_dev, int index, char **prof_name)

@@ -970,7 +951,8 @@ static int aw88261_volume_get(struct snd_kcontrol *kcontrol,
 	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(codec);
 	struct aw_volume_desc *vol_desc = &aw88261->aw_pa->volume_desc;
 
-	ucontrol->value.integer.value[0] = vol_desc->ctl_volume;
+	ucontrol->value.integer.value[0] =
+		(AW88261_MUTE_VOL - vol_desc->ctl_volume) / 2;
 
 	return 0;
 }

@@ -983,13 +965,13 @@ static int aw88261_volume_set(struct snd_kcontrol *kcontrol,
 	struct aw_volume_desc *vol_desc = &aw88261->aw_pa->volume_desc;
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
-	int value;
-
-	value = ucontrol->value.integer.value[0];
+	int value = ucontrol->value.integer.value[0];
 
 	if (value < mc->min || value > mc->max)
 		return -EINVAL;
 
+	value = AW88261_MUTE_VOL - (value * 2);
+
 	if (vol_desc->ctl_volume != value) {
 		vol_desc->ctl_volume = value;
 		aw88261_dev_set_volume(aw88261->aw_pa, vol_desc->ctl_volume);

@@ -1000,10 +982,18 @@ static int aw88261_volume_set(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+/*
+ * The field contains 4 bits in units of 6dB + 6 bits in units of 0.125dB
+ * which is too precise for TLV (!) so we have to multiply the scale by 2.
+ *
+ * The range is clamped at -90dB to prevent overflowing the 4-bit part.
+ */
+static const DECLARE_TLV_DB_SCALE(volume_tlv, -9000, 25, 0);
+
 static const struct snd_kcontrol_new aw88261_controls[] = {
-	SOC_SINGLE_EXT("PCM Playback Volume", AW88261_SYSCTRL2_REG,
-		6, AW88261_MUTE_VOL, 0, aw88261_volume_get,
-		aw88261_volume_set),
+	SOC_SINGLE_EXT_TLV("PCM Playback Volume", AW88261_SYSCTRL2_REG,
+		6, AW88261_MUTE_VOL / 2, 1,
+		aw88261_volume_get, aw88261_volume_set, volume_tlv),
 	AW88261_PROFILE_EXT("Profile Set", aw88261_profile_info,
 		aw88261_profile_get, aw88261_profile_set),
 };

sound/soc/codecs/aw88261.h +0 -1

index 1b1beba6a26b..76ef47236bb7 100644
@@ -536,7 +536,6 @@
 #define AW88261_DEV_SYSST_CHECK_MAX	(10)
 #define AW88261_SOFT_RESET_VALUE	(0x55aa)
 #define AW88261_REG_TO_DB		(0x3f)
-#define AW88261_VOL_START_MASK		(0xfc00)
 #define AW88261_INIT_PROFILE		(0)
 
 #define REG_VAL_TO_DB(value)		((((value) >> AW88261_VOL_6DB_START) * \

Signature · 1 line

2.53.0

Mark Brown

maintainer

2026-05-19 16:20 UTC #11 lore raw

Re: [PATCH v2 7/7] ASoC: codecs: aw88261: make volume control usable

Quoted 7 lines · Val Packett

sound/soc/codecs/aw88261.c +0 -3

@@ -424,17 +415,7 @@ static int aw88261_dev_reg_update(struct aw88261 *aw88261,
 			break;
 	}
 
-	ret = aw88261_dev_set_vcalb(aw_dev);
-	if (ret)
-		return ret;

sound/soc/codecs/aw88261.c static int aw88261_dev_reg_update(struct aw88261 *aw88261, · line 419

Quoted 10 lines

sound/soc/codecs/aw88261.c +1 -8

-
-	if (aw_dev->prof_cur != aw_dev->prof_index)
-		vol_desc->ctl_volume = 0;
-
-	/* keep min volume */
-	aw88261_dev_set_volume(aw_dev, vol_desc->mute_volume);
-
-	return ret;
+	return aw88261_dev_set_vcalb(aw_dev);
 }

This looks like it was intentional in order to ensure that things are muted until we power up (there's an unmute in _dev_start())?

Quoted 4 lines

sound/soc/codecs/aw88261.c +0 -0

@@ -983,13 +965,13 @@ static int aw88261_volume_set(struct snd_kcontrol *kcontrol,
 	struct aw_volume_desc *vol_desc = &aw88261->aw_pa->volume_desc;
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;

sound/soc/codecs/aw88261.c static int aw88261_volume_set(struct snd_kcontrol *kcontrol, · line 974

Quoted 10 lines

sound/soc/codecs/aw88261.c +3 -3

-	int value;
-
-	value = ucontrol->value.integer.value[0];
+	int value = ucontrol->value.integer.value[0];
 
 	if (value < mc->min || value > mc->max)
 		return -EINVAL;
 
+	value = AW88261_MUTE_VOL - (value * 2);
+

We're doing the invert of the value scale before we store it, currently the default value is defined as 0 in the header so we'll default to maximum volume which is likely to lead to some distress at some point. The default should be updated as well.

Val Packett

author

2026-05-19 22:36 UTC #12 lore raw

Re: [PATCH v2 7/7] ASoC: codecs: aw88261: make volume control usable

Quoted 6 lines · Mark Brown

Quoted 4 lines · Val Packett

(quoted diff) +0 -0

@@ -424,17 +415,7 @@ static int aw88261_dev_reg_update(struct aw88261 *aw88261,
  			break;
  	}

Quoted 15 lines

sound/soc/codecs/aw88261.c static int aw88261_dev_reg_update(struct aw88261 *aw88261, · line 419
Quoted 13 lines
sound/soc/codecs/aw88261.c +1 ~~-11~~
-	ret = aw88261_dev_set_vcalb(aw_dev);
-	if (ret)
-		return ret;
-
-	if (aw_dev->prof_cur != aw_dev->prof_index)
-		vol_desc->ctl_volume = 0;
-
-	/* keep min volume */
-	aw88261_dev_set_volume(aw_dev, vol_desc->mute_volume);
-
-	return ret;
+	return aw88261_dev_set_vcalb(aw_dev);
  }
This looks like it was intentional in order to ensure that things are muted until we power up (there's an unmute in _dev_start())?

Sure.. that doesn't seem required but I can keep it to err on the side of safety / keeping what works.

Quoted 12 lines

sound/soc/codecs/aw88261.c +1 -3

@@ -983,13 +965,13 @@ static int aw88261_volume_set(struct snd_kcontrol *kcontrol,
  	struct aw_volume_desc *vol_desc = &aw88261->aw_pa->volume_desc;
  	struct soc_mixer_control *mc =
  		(struct soc_mixer_control *)kcontrol->private_value;
-	int value;
-
-	value = ucontrol->value.integer.value[0];
+	int value = ucontrol->value.integer.value[0];
 
  	if (value < mc->min || value > mc->max)
  		return -EINVAL;

Quoted 6 lines

sound/soc/codecs/aw88261.c static int aw88261_volume_set(struct snd_kcontrol *kcontrol, · line 974
Quoted 2 lines
sound/soc/codecs/aw88261.c +2 -0
+	value = AW88261_MUTE_VOL - (value * 2);
+
We're doing the invert of the value scale before we store it, currently the default value is defined as 0 in the header so we'll default to maximum volume which is likely to lead to some distress at some point. The default should be updated as well.

Ack

~val

Luca Weiss

2026-05-22 12:39 UTC #13 lore raw

Re: [PATCH v2 7/7] ASoC: codecs: aw88261: make volume control usable

In reply to Val Packett

Hi Val,

Quoted 62 lines · Val Packett

- Invert the value to match userspace expectations (in the hardware, positive numbers represent negative dB attenuation)
- Provide TLV metadata for the dB scale (and divide the raw values by 2 as the excessive precision used by HW is not representable in TLV)
- Do not unnecessarily reset the volume while switching profiles
- Simplify aw88261_dev_set_volume using regmap_update_bits
- Do not add the initial volume from the profile to the requested volume as that would throw off the dB mapping (if a lower max limit is desired, it can be set in the UCM profile in userspace)
With this change, it's actually possible to use this hardware volume control as PlaybackVolume in an ALSA UCM profile.
Fixes 028a2ae25691 ("ASoC: codecs: Add aw88261 amplifier driver")
Signed-off-by Val Packett <[email protected]>
---
 sound/soc/codecs/aw88261.c | 52 +++++++++++++++-----------------------
 sound/soc/codecs/aw88261.h |  1 -
 2 files changed, 21 insertions(+), 32 deletions(-)
sound/soc/codecs/aw88261.c +4 ~~-15~~
index adc728e45f57..0e6b2dfe5db9 100644
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
 #include <sound/soc.h>
 #include <sound/pcm_params.h>
+#include <sound/tlv.h>
 #include "aw88261.h"
 #include "aw88395/aw88395_data_type.h"
 #include "aw88395/aw88395_device.h"
@@ -29,20 +30,10 @@ static const struct regmap_config aw88261_remap_config = {
 
 static void aw88261_dev_set_volume(struct aw_device *aw_dev, unsigned int value)
 {
-	struct aw_volume_desc *vol_desc = &aw_dev->volume_desc;
-	unsigned int real_value, volume;
-	unsigned int reg_value;
+	unsigned int volume = min(value, (unsigned int)AW88261_MUTE_VOL);
 
-	volume = min((value + vol_desc->init_volume), (unsigned int)AW88261_MUTE_VOL);
-	real_value = DB_TO_REG_VAL(volume);
-
-	regmap_read(aw_dev->regmap, AW88261_SYSCTRL2_REG, &reg_value);
-
-	real_value = (real_value | (reg_value & AW88261_VOL_START_MASK));
-
-	dev_dbg(aw_dev->dev, "value 0x%x , real_value:0x%x", value, real_value);
-
-	regmap_write(aw_dev->regmap, AW88261_SYSCTRL2_REG, real_value);
+	regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL2_REG,
+		~AW88261_VOL_MASK, DB_TO_REG_VAL(volume));
 }
 
 static void aw88261_dev_i2s_tx_enable(struct aw_device *aw_dev, bool flag)
@@ -424,17 +415,7 @@ static int aw88261_dev_reg_update(struct aw88261 *aw88261,
 			break;
 	}
 
-	ret = aw88261_dev_set_vcalb(aw_dev);
-	if (ret)

sound/soc/codecs/aw88261.c static int aw88261_dev_reg_update(struct aw88261 *aw88261, · line 418

Quoted 10 lines

sound/soc/codecs/aw88261.c +1 -9

-		return ret;
-
-	if (aw_dev->prof_cur != aw_dev->prof_index)
-		vol_desc->ctl_volume = 0;
-
-	/* keep min volume */
-	aw88261_dev_set_volume(aw_dev, vol_desc->mute_volume);
-
-	return ret;
+	return aw88261_dev_set_vcalb(aw_dev);

These removals cause the following compile warning, when applied on linux-next:

sound/soc/codecs/aw88261.c: In function 'aw88261_dev_reg_update':
sound/soc/codecs/aw88261.c:347:32: warning: unused variable 'vol_desc' [-Wunused-variable]
347 | struct aw_volume_desc *vol_desc = &aw_dev->volume_desc;
| ^~~~~~~~

Regards
Luca

Luca Weiss

Tested-by on the v2 series

2026-05-22 12:36 UTC #14 lore raw

In reply to Val Packett

Hi Val,

Quoted 6 lines · Val Packett

The Awinic smart speaker/amp drivers were merged in a very "downstream-brained" state, where configuration was only really determined by the binary "firmware" (register list) file instead of properly participating in the ASoC system. Let's start untangling this mess. This series makes aw88261 actually usable on devices like fairphone-fp5, motorola-dubai and xiaomi-pipa.

Thanks a lot for working on this!

With this series, and simply hooking up the two amplifiers in dts, speakers seem to work without any issues, getting rid of the hacks that I've had in the tree before!

Tested-by Luca Weiss <[email protected]> # qcm6490-fairphone-fp5

Regards
Luca

Quoted 30 lines

v2:
- Add missing sample rates to the switch statement (turns out we do have them)
- Process the conversion from requested values to register ones directly in the ASoC ops to make sure EINVAL is returned early, from the right place
- Do not include untested aw88258 fixes.. (why did I think that some support for it was already present in the tree?)
- Do include TDM support, logspam reduction, a newly discovered mask fix
- Remove misfeatures inherited from downstream: async start, fade-in/fade-out (hopefully this won't be too controversial?)
- Also overhaul volume control to make it actually usable as PlaybackVolume
v1: https://lore.kernel.org/all/[email protected]/
~val
---
Val Packett (7):
ASoC: codecs: aw88261: support changing sample rate and bit width
ASoC: codecs: aw88261: add TDM support
ASoC: codecs: aw88261: reduce log spam
ASoC: codecs: aw88261: remove fade in/out on start/stop
ASoC: codecs: aw88261: remove async start
ASoC: codecs: aw88261: fix incorrect masks for boost regs
ASoC: codecs: aw88261: make volume control usable
 sound/soc/codecs/aw88261.c | 612 +++++++++++++++++++------------------
 sound/soc/codecs/aw88261.h | 179 ++++++++++-
 2 files changed, 486 insertions(+), 305 deletions(-)