Re: [PATCH v1 2/2] ASoC: codecs: add Qualcomm WSA885X I2C codec driver
From: Prasad Kumpatla
Date: Tue Jun 23 2026 - 05:25:02 EST
On 6/11/2026 3:19 PM, Bartosz Golaszewski wrote:
On Wed, 10 Jun 2026 17:57:08 +0200, Prasad Kumpatla
<prasad.kumpatla@xxxxxxxxxxxxxxxx> said:
Add an ASoC codec driver for the Qualcomm WSA885X smart speaker...
amplifier accessed over I2C.
The driver provides the control-side support needed for playback
bring-up, including register programming, serial interface setup, clock
handling, mute and gain control, reset handling and interrupt support.
Program the init table during codec initialization and reapply it only
after an explicit device reset so the static device configuration is
not rewritten on every playback start. Also program the TDM control
slot-count field from the runtime slot configuration so the same codec
path can be used with 2-slot, 4-slot, or 8-slot Audio IF backends.
Keep the stream-time power-state sequencing in the DAI callbacks and
use normal regmap access for the control path.
Signed-off-by: Prasad Kumpatla <prasad.kumpatla@xxxxxxxxxxxxxxxx>
---
diff --git a/sound/soc/codecs/wsa885x-i2c.c b/sound/soc/codecs/wsa885x-i2c.cCan you keep the headers in alphabetical order?
new file mode 100644
index 000000000..a7d8f8d48
--- /dev/null
+++ b/sound/soc/codecs/wsa885x-i2c.c
@@ -0,0 +1,1643 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+/* WSA885X I2C codec driver */
+
+#include <linux/gpio/consumer.h>
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc-dapm.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+#include <linux/interrupt.h>
Hi Bart,
Thanks for review the patch and the feedback.
Ack, Will update
Ack, Will update.
...
+Add newline.
+#define WSA885X_FU21_VOL_STEPS 124
+#define WSA885X_USAGE_MODE_MAX 8
+#define WSA885X_INIT_TABLE_MAX_ITEMS 256
Ack, I will make them to a single line.
...
+I'd put it on the same line (elsewhere too) but that's personal preference.
+static int wsa885x_apply_init_table(struct wsa885x_i2c_priv *wsa885x)
+{
+ int i;
+ int ret;
Ack, I will cleanup and remove the all unnecessary checks and update in next version
+
+ if (!wsa885x || !wsa885x->regmap)
+ return -EINVAL;
You have a lot of these checks but this can't really happen, can it?
Ack.
+Newline.
+ if (!wsa885x->init_table_size)
+ return 0;
+
+ if (!wsa885x->init_table)
+ return -EINVAL;
+
+ for (i = 0; i < wsa885x->init_table_size / 2; i++) {
+ u32 reg = wsa885x->init_table[2 * i];
+ u32 val = wsa885x->init_table[2 * i + 1];
+
+ if (wsa885x->batt_conf == WSA885X_BATT_2S && reg == WSA885X_SPK_TOP_LF_CH1_CTRL11)
+ continue;
+
+ if (wsa885x->batt_conf == WSA885X_BATT_2S && reg == WSA885X_SPK_TOP_LF_CH2_CTRL11)
+ continue;
+
+ ret = regmap_write(wsa885x->regmap, reg, val);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int wsa885x_hw_init(struct wsa885x_i2c_priv *wsa885x)
+{
+ static const struct reg_sequence regs[] = {
+ { WSA885X_DIG_CTRL1_SPMI_PAD_GPIO2_CTL, 0x2e },
+ { WSA885X_DIG_CTRL1_INTR_MODE, 0x01 },
+ { WSA885X_DIG_CTRL1_PIN_CT, 0x04 },
+ };
+ int ret;
+
+ if (!wsa885x || !wsa885x->regmap)
+ return -EINVAL;
+
+ ret = wsa885x_apply_init_table(wsa885x);
+ if (ret)
+ return ret;
+
+ if (wsa885x->batt_conf == WSA885X_BATT_2S) {
+ ret = wsa885x_2s_conf(wsa885x);
+ if (ret)
+ return ret;
+ }
+
+ return regmap_multi_reg_write(wsa885x->regmap, regs, ARRAY_SIZE(regs));
+}
+
+static int wsa885x_unmask_interrupts(struct wsa885x_i2c_priv *wsa885x)
+{
+ static const struct reg_sequence regs[] = {
+ { WSA885X_INTR_MASK0, 0x00 },
+ { WSA885X_INTR_MASK0 + 1, 0x00 },
+ { WSA885X_INTR_MASK0 + 2, 0xf8 },
+ };
+
+ if (!wsa885x || !wsa885x->regmap)
+ return -EINVAL;
+
+ return regmap_multi_reg_write(wsa885x->regmap, regs, ARRAY_SIZE(regs));
+}
+
+static int wsa885x_wait_for_pde_state(struct wsa885x_i2c_priv *wsa885x, int ps)
+{
+ int act_ps = -1, cnt = 0, clock_valid = -1;
+ int rc = 0;
+
+ if (!wsa885x || !wsa885x->regmap)
+ return -EINVAL;
+
+ if (ps < 0 || ps > 3)
+ return -EINVAL;
+
+ do {
+ usleep_range(1000, 1500);
+ rc = regmap_read(wsa885x->regmap,
+ WSA885X_SMP_AMP_CTRL_STEREO_PDE23_ACT_PS,
+ &act_ps);
+ if (rc) {
+ dev_err(wsa885x->dev, "PDE state read failed: %d\n", rc);
+ return rc;
+ }
+ if (act_ps == ps)
+ return 0;
+ } while (++cnt < 5);
+ if (regmap_read(wsa885x->regmap,Do we warn about unused arguments in the kernel now?
+ WSA885X_SMP_AMP_CTRL_STEREO_CS21_CLOCK_VALID,
+ &clock_valid))
+ dev_err(wsa885x->dev,
+ "PDE power state %d request failed, actual_ps %d, clock_valid read failed\n",
+ ps, act_ps);
+ else
+ dev_err(wsa885x->dev,
+ "PDE power state %d request failed, actual_ps %d, clock_valid:%d\n",
+ ps, act_ps, clock_valid);
+
+ return -ETIMEDOUT;
+}
+
+static int wsa885x_codec_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct wsa885x_i2c_priv *wsa885x;
+ u8 pcm_rate, cs21_sample_rate_idx, cs24_sample_rate_idx;
+
+ (void)substream;
Right, this is unnecessary. I'll drop the unused parameter cast.
Ack.
Ack.
...
+You seem to be repeating the same sequence in multiple functions just to get
+static int wsa885x_stereo_gain_offset_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct snd_soc_component *component;
+ struct wsa885x_i2c_priv *wsa885x;
+ int val;
+
+ if (!kcontrol || !ucontrol)
+ return -EINVAL;
+
+ component = snd_kcontrol_chip(kcontrol);
+ if (!component)
+ return -EINVAL;
+
+ wsa885x = snd_soc_component_get_drvdata(component);
+ if (!wsa885x)
+ return -EINVAL;
+
+ val = wsa885x->stereo_vol_db + 84;
+ if (val < 0 || val > WSA885X_FU21_VOL_STEPS)
+ return -ERANGE;
+
+ ucontrol->value.integer.value[0] = val;
+ return 0;
+}
+
+static int wsa885x_stereo_gain_offset_put(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct snd_soc_component *component;
+ struct wsa885x_i2c_priv *wsa885x;
+ long val;
+
+ if (!kcontrol || !ucontrol)
+ return -EINVAL;
+
+ component = snd_kcontrol_chip(kcontrol);
+ if (!component)
+ return -EINVAL;
+
+ wsa885x = snd_soc_component_get_drvdata(component);
+ if (!wsa885x)
+ return -EINVAL;
+
+ val = ucontrol->value.integer.value[0];
+
+ if (val < 0 || val > WSA885X_FU21_VOL_STEPS) {
+ dev_err(component->dev, "%s: Invalid range, Val: %ld\n", __func__, val);
+ return -EINVAL;
+ }
+ wsa885x->stereo_vol_db = (int)val - 84;
+ return 0;
+}
+
+static int wsa885x_i2c_usage_modes_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct snd_soc_component *component;
+ struct wsa885x_i2c_priv *wsa885x_i2c;
+
+ if (!kcontrol || !ucontrol)
+ return -EINVAL;
+
+ component = snd_kcontrol_chip(kcontrol);
+ if (!component)
+ return -EINVAL;
+
+ wsa885x_i2c = snd_soc_component_get_drvdata(component);
+ if (!wsa885x_i2c)
+ return -EINVAL;
+
+ if (wsa885x_i2c->usage_mode > WSA885X_USAGE_MODE_MAX)
+ return -ERANGE;
+
+ ucontrol->value.integer.value[0] = wsa885x_i2c->usage_mode;
+
+ return 0;
+}
+
+static int wsa885x_i2c_usage_modes_put(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct snd_soc_component *component;
+ struct wsa885x_i2c_priv *wsa885x_i2c;
+ long val;
+
+ if (!kcontrol || !ucontrol)
+ return -EINVAL;
+
+ component = snd_kcontrol_chip(kcontrol);
+ if (!component)
+ return -EINVAL;
+
+ wsa885x_i2c = snd_soc_component_get_drvdata(component);
+ if (!wsa885x_i2c)
+ return -EINVAL;
+
the address of wsa885x_i2c. Can you factor it out into a separate helper and
save some lines?
Ack. will update
+ val = ucontrol->value.integer.value[0];...
+
+ if (val < 0 || val > WSA885X_USAGE_MODE_MAX)
+ return -EINVAL;
+
+ wsa885x_i2c->usage_mode = val;
+
+ return 0;
+}
+
+static int wsa885x_i2c_rx_slot_mask_get(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct snd_soc_component *component;
+ struct wsa885x_i2c_priv *wsa885x_i2c;
+ u32 mask;
+
+ if (!kcontrol || !ucontrol)
+ return -EINVAL;
+
+ component = snd_kcontrol_chip(kcontrol);
+ if (!component)
+ return -EINVAL;
+
+ wsa885x_i2c = snd_soc_component_get_drvdata(component);
+ if (!wsa885x_i2c)
+ return -EINVAL;
+
+ mask = wsa885x_i2c->rx_slot_mask;
+ if (!wsa885x_is_valid_rx_slot_mask(mask))
+ return -ERANGE;
+
+ ucontrol->value.integer.value[0] = mask;
+
+ return 0;
+}
+
+static int wsa885x_i2c_rx_slot_mask_put(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct snd_soc_component *component;
+ struct wsa885x_i2c_priv *wsa885x_i2c;
+ long mask;
+
+ if (!kcontrol || !ucontrol)
+ return -EINVAL;
+
+ component = snd_kcontrol_chip(kcontrol);
+ if (!component)
+ return -EINVAL;
+
+ wsa885x_i2c = snd_soc_component_get_drvdata(component);
+ if (!wsa885x_i2c)
+ return -EINVAL;
+
+ mask = ucontrol->value.integer.value[0];
+
+ if (!wsa885x_is_valid_rx_slot_mask(mask))
+ return -EINVAL;
+
+ wsa885x_i2c->rx_slot_mask = mask;
+
+ return 0;
+}
+
+ /* INTR_CLEAR registers are write-only; use regmap_write/*
+ * instead of regmap_update_bits to avoid the read-modify-write
+ * that regmap_update_bits performs on non-readable registers.
+ */
*/
style comments please
...
+ ret = devm_add_action_or_reset(dev, wsa885x_gpio_powerdown, wsa885x);I don't see a corresponding i2c_get_clientdata(). Do you really need it?
+ if (ret)
+ return dev_err_probe(dev, ret, "devm_add_action_or_reset failed\n");
+
+ i2c_set_clientdata(client, wsa885x);
It is currently not being used, so storing the client data is unnecessary.
I'll either remove it or add it together with the code that requires i2c_get_clientdata() in a future versions.
Thanks,
Prasad
...
Bart