Re: [PATCH v1 2/2] ASoC: codecs: add Qualcomm WSA885X I2C codec driver

[Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>]

On 6/11/2026 3:19 PM, Bartosz Golaszewski wrote:
> On Wed, 10 Jun 2026 17:57:08 +0200, Prasad Kumpatla
> <prasad.kumpatla@oss.qualcomm.com> 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@oss.qualcomm.com>
>> ---
> ...
>
>> diff --git a/sound/soc/codecs/wsa885x-i2c.c b/sound/soc/codecs/wsa885x-i2c.c
>> 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>
> Can you keep the headers in alphabetical order?

Hi Bart,

Thanks for review the patch and the feedback.

Ack, Will update

>
> ...
>
>> +
>> +#define WSA885X_FU21_VOL_STEPS 124
>> +#define WSA885X_USAGE_MODE_MAX 8
>> +#define WSA885X_INIT_TABLE_MAX_ITEMS 256
> Add newline.
Ack, Will update.
>
> ...
>
>> +
>> +static int wsa885x_apply_init_table(struct wsa885x_i2c_priv *wsa885x)
>> +{
>> +	int i;
>> +	int ret;
> I'd put it on the same line (elsewhere too) but that's personal preference.
Ack, I will make them to a single line.
>
>> +
>> +	if (!wsa885x || !wsa885x->regmap)
>> +		return -EINVAL;
>
> You have a lot of these checks but this can't really happen, can it?
Ack, I will cleanup and remove the all unnecessary checks and update in 
next version
>
>> +
>> +	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);
> Newline.
Ack.
>
>> +	if (regmap_read(wsa885x->regmap,
>> +			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;
> Do we warn about unused arguments in the kernel now?

Right, this is unnecessary. I'll drop the unused parameter cast.

Ack.

>
> ...
>
>> +
>> +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;
>> +
> You seem to be repeating the same sequence in multiple functions just to get
> the address of wsa885x_i2c. Can you factor it out into a separate helper and
> save some lines?
Ack.
>
>> +	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
Ack. will update
>
> ...
>
>> +	ret = devm_add_action_or_reset(dev, wsa885x_gpio_powerdown, wsa885x);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "devm_add_action_or_reset failed\n");
>> +
>> +	i2c_set_clientdata(client, wsa885x);
> I don't see a corresponding i2c_get_clientdata(). Do you really need it?

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