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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Manual snd_soc_component_exit_regmap() call on a devm-managed regmap causes a double free and potential Use-After-Free.
- [High] Volume control changes do not take effect dynamically during active playback.
- [Medium] ALSA Kcontrol .put callbacks unconditionally return 0 instead of 1 when values are successfully changed.
- [Medium] Shared configuration variables are modified by ALSA kcontrols and read by DAI callbacks without locking.
- [Low] Passing 'int *' to regmap_read() causes incompatible pointer type compiler warnings.
--

commit e03e869e67ca57141b83b27db8520ee9f2d72031
Author: Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>
Date:   Wed Jun 10 15:57:08 2026 +0000

ASoC: codecs: add Qualcomm WSA885X I2C codec driver

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.

> diff --git a/sound/soc/codecs/wsa885x-i2c.c b/sound/soc/codecs/wsa885x-i2c.c
> new file mode 100644
> index 0000000000000..a7d8f8d48a21c
> --- /dev/null
> +++ b/sound/soc/codecs/wsa885x-i2c.c
[ ... ]
> +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);

[Severity: Low]
Does this trigger a compiler warning? regmap_read() expects an unsigned
int * for its third argument, but act_ps is declared as an int. Should
act_ps and clock_valid be declared as unsigned int to match the API
signature?

[ ... ]
> +static int wsa885x_codec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
> +{
[ ... ]
> +	} else {
> +		regmap_multi_reg_write(wsa885x->regmap, unmute_prep_head_regs,
> +				       ARRAY_SIZE(unmute_prep_head_regs));
> +		regmap_write(wsa885x->regmap, WSA885X_SMP_AMP_CTRL_STEREO_OT23_USAGE,
> +			     wsa885x->usage_mode);
> +		regmap_multi_reg_write(wsa885x->regmap, unmute_prep_tail_regs,
> +				       ARRAY_SIZE(unmute_prep_tail_regs));
> +		regmap_write(wsa885x->regmap,
> +			     WSA885X_SMP_AMP_CTRL_STEREO_FU21_CH_VOL_CH2X0_MSB,
> +			     (s8)wsa885x->stereo_vol_db);
> +		regmap_write(wsa885x->regmap,
> +			     WSA885X_SMP_AMP_CTRL_STEREO_FU21_CH_VOL_CH2X1_MSB,
> +			     (s8)wsa885x->stereo_vol_db);

[Severity: Medium]
Can this lead to a race condition where mismatched channel volumes are
applied? The stereo_vol_db is read twice sequentially here without lock
protection.

If a concurrent call to wsa885x_stereo_gain_offset_put() occurs, could it
update stereo_vol_db exactly between these two regmap writes?

[ ... ]
> +static void wsa885x_component_remove(struct snd_soc_component *component)
> +{
> +	if (!component)
> +		return;
> +
> +	snd_soc_component_exit_regmap(component);
> +}

[Severity: Critical]
Is it safe to manually exit the regmap here? Since the regmap was allocated
using devm_regmap_init_i2c(), devres will attempt to free it automatically.
Calling snd_soc_component_exit_regmap() here causes an early free, which
can result in a double free.

Additionally, because the interrupt handler is registered via
devm_request_threaded_irq() before the component is registered, the IRQ
remains active after this component removal. If the interrupt fires in this
window, will wsa885x_interrupt_handler() trigger a use-after-free when
accessing wsa885x->regmap?

[ ... ]
> +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;
> +}

[Severity: High]
Will this update take effect immediately during playback? The cached
volume is updated, but it doesn't look like the new value is written to the
hardware registers here. Are users expected to pause and restart the stream
for the volume change to apply?

[Severity: Medium]
Should this return 1 instead of 0 when the value actually changes?
ALSA kcontrol .put callbacks are expected to return 1 if the control was
modified so the core can generate a state change event for userspace.

[ ... ]
> +static int wsa885x_i2c_usage_modes_put(struct snd_kcontrol *kcontrol,
> +				       struct snd_ctl_elem_value *ucontrol)
> +{
[ ... ]
> +	val = ucontrol->value.integer.value[0];
> +
> +	if (val < 0 || val > WSA885X_USAGE_MODE_MAX)
> +		return -EINVAL;
> +
> +	wsa885x_i2c->usage_mode = val;
> +
> +	return 0;
> +}

[Severity: Medium]
Does this need to return 1 when the usage_mode changes so ALSA generates
a state change event, similar to the volume control above?

[ ... ]
> +static int wsa885x_i2c_rx_slot_mask_put(struct snd_kcontrol *kcontrol,
> +					struct snd_ctl_elem_value *ucontrol)
> +{
[ ... ]
> +	mask = ucontrol->value.integer.value[0];
> +
> +	if (!wsa885x_is_valid_rx_slot_mask(mask))
> +		return -EINVAL;
> +
> +	wsa885x_i2c->rx_slot_mask = mask;
> +
> +	return 0;
> +}

[Severity: Medium]
Does this also need to return 1 when the mask changes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610155708.151067-1-prasad.kumpatla@oss.qualcomm.com?part=2