Re: [PATCH v2 2/2] ASoC: cs35l45: Adds support for SYNC bus

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] cs35l45_sync_en_put() incorrectly returns 0 instead of 1 when the control value is successfully changed.
- [Medium] The driver ignores hardware update failures in cs35l45_sync_en_put(), potentially desynchronizing software and hardware state.
--

--- Patch [2]: [PATCH v2 2/2] ASoC: cs35l45: Adds support for SYNC bus ---
commit 6cfd479679d81732828972b84bd776a3f10f10e2
Author: Ricardo Rivera-Matos <[email protected]>

ASoC: cs35l45: Adds support for SYNC bus

Adds support for the multidevice synchronization (SYNC) bus. Initial
support only includes software (SW) data packets.

> diff --git a/sound/soc/codecs/cs35l45.c b/sound/soc/codecs/cs35l45.c
> index a032bb23b4ac7..9f0cdfe9fb92e 100644
> --- a/sound/soc/codecs/cs35l45.c
> +++ b/sound/soc/codecs/cs35l45.c

[ ... ]

> +static int cs35l45_sync_en_put(struct snd_kcontrol *kcontrol,
> +				      struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +			snd_soc_kcontrol_component(kcontrol);
> +	struct cs35l45_private *cs35l45 =
> +			snd_soc_component_get_drvdata(component);
> +
> +	if (!cs35l45->sync_pin_set)
> +		return -EPERM;
> +
> +	if (ucontrol->value.integer.value[0] == cs35l45->sync_en)
> +		return 0;
> +
> +	if ((bool)ucontrol->value.integer.value[0])
> +		regmap_set_bits(cs35l45->regmap, CS35L45_BLOCK_ENABLES2, CS35L45_SYNC_EN_MASK);
> +	else
> +		regmap_clear_bits(cs35l45->regmap, CS35L45_BLOCK_ENABLES2, CS35L45_SYNC_EN_MASK);
> +
> +	cs35l45->sync_en = ucontrol->value.integer.value[0];

[Severity: Medium]
Does this code need to check the return value of regmap_set_bits() and
regmap_clear_bits() before updating the local software cache?

If the I2C/SPI bus transaction fails, cs35l45->sync_en will be updated even
though the hardware state remains unchanged, potentially desynchronizing
software and hardware state.

> +
> +	return 0;

[Severity: Medium]
Should this return 1 instead of 0 when the value is actually changed?

In the ALSA kcontrol API, put callbacks must return 1 when the hardware or
cached state is successfully changed, so that ALSA mixer UIs and audio servers
receive a SNDRV_CTL_EVENT_MASK_VALUE notification. Returning 0 here might lead
to state desynchronization in userspace.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2