Re: [PATCH 15/28] ASoC: tas2764: Set up V/ISENSE on codec probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASoC: tas2764: flawed V/I sense disabled logic causes inadvertent enable
--

commit ff9219a20060d38cf8689dacfe0f48323c8c8566
Author: James Calligeros <jcalligeros99@gmail.com>

ASoC: tas2764: Set up V/ISENSE on codec probe

This commit moves the setup of V/ISENSE to the codec probe rather than
configuring it on every hw_params or TDM slot call. This ensures it is
configured at the same time as the rest of the codec.

> diff --git a/sound/soc/codecs/tas2764.c b/sound/soc/codecs/tas2764.c
> index 6654100ac91d1..39372d0446d06 100644
> --- a/sound/soc/codecs/tas2764.c
> +++ b/sound/soc/codecs/tas2764.c

[ ... ]

> @@ -499,15 +470,26 @@ static int tas2764_set_dai_tdm_slot(struct snd_soc_dai *dai,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = snd_soc_component_update_bits(component, TAS2764_TDM_CFG5,
> +	return 0;
> +}
> +
> +static int tas2764_set_visense(struct tas2764_priv *tas2764, int vslot, int islot)
> +{
> +	int ret;
> +
> +	ret = snd_soc_component_update_bits(tas2764->component, TAS2764_TDM_CFG5,
> +					    TAS2764_TDM_CFG5_VSNS_ENABLE |
>  					    TAS2764_TDM_CFG5_50_MASK,
> -					    tas2764->v_sense_slot);
> +					    TAS2764_TDM_CFG5_VSNS_ENABLE |
> +					    vslot);

[Severity: Medium]
Does this code inadvertently enable the sense slot and assign it to the maximum
value if vslot is -1?

A bitwise OR with a -1 value yields 0xffffffff. When applied with the bitmask
in snd_soc_component_update_bits(), this will turn on the enable bit and set
the slot to the maximum value, which can lead to a TDM bus collision and
audio corruption.

>  	if (ret < 0)
>  		return ret;
>  
> -	ret = snd_soc_component_update_bits(component, TAS2764_TDM_CFG6,
> +	ret = snd_soc_component_update_bits(tas2764->component, TAS2764_TDM_CFG6,
> +					    TAS2764_TDM_CFG6_ISNS_ENABLE |
>  					    TAS2764_TDM_CFG6_50_MASK,
> -					    tas2764->i_sense_slot);
> +					    TAS2764_TDM_CFG6_ISNS_ENABLE |
> +					    islot);
>  	if (ret < 0)
>  		return ret;

[ ... ]

> @@ -821,15 +803,12 @@ static int tas2764_codec_probe(struct snd_soc_component *component)
>  			dev_warn(tas2764->dev, "failed to request IRQ: %d\n", ret);
>  	}
>  
> -	ret = snd_soc_component_update_bits(tas2764->component, TAS2764_TDM_CFG5,
> -					    TAS2764_TDM_CFG5_VSNS_ENABLE, 0);
> -	if (ret < 0)
> -		return ret;
> +	if (tas2764->v_sense_slot != -1 || tas2764->i_sense_slot != -1) {
> +		ret = tas2764_set_visense(tas2764, tas2764->v_sense_slot, tas2764->i_sense_slot);

[Severity: Medium]
If a user configures one of the sense slots to -1 in the device tree to disable
it while keeping the other enabled, will this logical OR evaluate to true and
cause the disabled slot value (-1) to be passed to tas2764_set_visense() above?

> +		if (ret < 0)
> +			return ret;
> +	}
>  
> -	ret = snd_soc_component_update_bits(tas2764->component, TAS2764_TDM_CFG6,
> -					    TAS2764_TDM_CFG6_ISNS_ENABLE, 0);
> -	if (ret < 0)
> -		return ret;
>  
>  	switch (tas2764->devid) {
>  	case DEVID_SN012776:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920-macaudio-v1-0-741cc20a74e5@gmail.com?part=15