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

[Bartosz Golaszewski]

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.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?

...

> +
> +#define WSA885X_FU21_VOL_STEPS 124
> +#define WSA885X_USAGE_MODE_MAX 8
> +#define WSA885X_INIT_TABLE_MAX_ITEMS 256

Add newline.

...

> +
> +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.

> +
> +	if (!wsa885x || !wsa885x->regmap)
> +		return -EINVAL;


You have a lot of these checks but this can't really happen, can it?

> +
> +	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.

> +	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?

...

> +
> +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?

> +	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);
> +	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?

...

Bart