Re: [PATCH 06/16] ASoC: twl4030-codec: Allow setting APLL rate through the .set_sysclk() interface

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing cleanup on write failure in `twl4030_perform_writes()` leaves the codec permanently disabled.
- [Medium] `twl4030_hw_params()` completely ignores the return value of batch register writes, masking hardware configuration failures.
--

--- Patch [6]: [PATCH 06/16] ASoC: twl4030-codec: Allow setting APLL rate through the .set_sysclk() interface ---
commit db9973d614eb8b37cb4d8a6cf9cada8e9b6d1cf8
Author: Grond <[email protected]>
Subject: ASoC: twl4030-codec: Allow setting APLL rate through the .set_sysclk() interface

This patch allows setting the APLL rate through the .set_sysclk() interface
for devices where the APLL drives audio clocks, but playback bypasses the
standard .hw_params() path. It extracts the APLL rate calculation into a
helper and introduces batch register write functions.

> diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
> index 9476cdfd4dde9..ff36679166e20 100644
> --- a/sound/soc/codecs/twl4030.c
> +++ b/sound/soc/codecs/twl4030.c
> @@ -198,6 +198,38 @@ static void twl4030_codec_enable(struct snd_soc_component *component, int enable
>  	udelay(10);
>  }
>  
> +static int twl4030_perform_writes(struct snd_soc_component *component,
> +				  const unsigned int *regs,
> +				  const unsigned int *vals,
> +				  unsigned int n)
> +{
> +	struct twl4030_priv *twl4030 = snd_soc_component_get_drvdata(component);
> +	int reboot_codec = twl4030->codec_powered;
> +	unsigned int i;
> +	int ret;
> +
> +	if (reboot_codec)
> +		twl4030_codec_enable(component, 0);
> +
> +	for (i = 0; i < n; ++i) {
> +		ret = twl4030_write(component, regs[i], vals[i]);
> +		if (ret)
> +			return ret;

[Severity: High]
If twl4030_write() fails and returns early, will it leave the codec permanently
disabled because we skip the twl4030_codec_enable(component, 1) call? This
seems to cause a hardware state mismatch where the ALSA core considers the
codec powered on, but it remains physically powered off.

> +	}
> +
> +	if (reboot_codec)
> +		twl4030_codec_enable(component, 1);
> +
> +	return 0;
> +}

[ ... ]

> @@ -1775,20 +1824,12 @@ static int twl4030_hw_params(struct snd_pcm_substream *substream,
>  		return -EINVAL;
>  	}
>  
> -	if (format != old_format || mode != old_mode) {
> -		if (twl4030->codec_powered) {
> -			/*
> -			 * If the codec is powered, than we need to toggle the
> -			 * codec power.
> -			 */
> -			twl4030_codec_enable(component, 0);
> -			twl4030_write(component, TWL4030_REG_CODEC_MODE, mode);
> -			twl4030_write(component, TWL4030_REG_AUDIO_IF, format);
> -			twl4030_codec_enable(component, 1);
> -		} else {
> -			twl4030_write(component, TWL4030_REG_CODEC_MODE, mode);
> -			twl4030_write(component, TWL4030_REG_AUDIO_IF, format);
> -		}
> +	if (format != old_format || s) {
> +		regs[0] = TWL4030_REG_CODEC_MODE;
> +		vals[0] = mode;
> +		regs[1] = TWL4030_REG_AUDIO_IF;
> +		vals[1] = format;
> +		twl4030_perform_writes(component, regs, vals, ARRAY_SIZE(regs));

[Severity: Medium]
Does this code ignore the return value of twl4030_perform_writes()?
If the batch register writes fail, it seems the hardware remains unconfigured
but the function will still return 0 (success) and falsely advance the stream
state.

>  	}
>  
>  	/* Store the important parameters for the DAI configuration and set

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