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

[Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>]

Mark,

On 8/28/26 10:57 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Hardware I/O errors in cs35l45_sync_en_put cause permanent state desynchronization.
> --
> 
> commit 81445881eb3cef5b79a02d7516346532c3f3f907
> Author: Ricardo Rivera-Matos <rriveram@opensource.cirrus.com>
> ASoC: cs35l45: Adds support for SYNC bus
> 
> Adds support for the multidevice synchronization (SYNC) bus. The SYNC
> bus is an opaque one wire bus that is shared between Cirrus devices.
> 
>> diff --git a/sound/soc/codecs/cs35l45.c b/sound/soc/codecs/cs35l45.c
>> index a032bb23b4ac7..763cd83b5d6d8 100644
>> --- a/sound/soc/codecs/cs35l45.c
>> +++ b/sound/soc/codecs/cs35l45.c
> [ ... ]
>> @@ -195,6 +196,46 @@ static int cs35l45_activate_ctl(struct snd_soc_component *component,
> [ ... ]
>> +static int cs35l45_sync_en_put(struct snd_kcontrol *kcontrol,
>> +				      struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>> +	struct cs35l45_private *cs35l45 =
>> +			snd_soc_component_get_drvdata(component);
>> +	struct snd_soc_dapm_context *dapm =
>> +			snd_soc_component_to_dapm(component);
>> +
>> +	snd_soc_dapm_mutex_lock(dapm);
>> +
>> +	if ((bool)ucontrol->value.integer.value[0] == cs35l45->sync_en) {
>> +		snd_soc_dapm_mutex_unlock(dapm);
>> +		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 = (bool)ucontrol->value.integer.value[0];
> 
> [Severity: Medium]
> Could this lead to permanent state desynchronization if the hardware I/O fails?
> The regmap API calls can fail (e.g., returning -EIO on I2C/SPI errors), but the
> error is ignored here and the software cache (cs35l45->sync_en) is updated
> unconditionally.
> 
> If this happens, the ALSA control state becomes desynchronized from the hardware.
> Because of the fast-path check at the beginning of this function:
> 
>      if ((bool)ucontrol->value.integer.value[0] == cs35l45->sync_en) {
>          ...
> 
> Any subsequent attempts from userspace to set the correct value will be dropped,
> making the failure permanent until explicitly toggled to the opposite state and
> back again. Should the return values of regmap_set_bits() and regmap_clear_bits()
> be checked before updating the cached state?

Are you expecting this comment to be addressed? I am not opposed, but 
the suggestion does not fit the style of the rest of the subsystem.

Ultimately, your call.

Ricardo