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
