Re: [PATCH v1 2/2 RESEND] ASoC: codecs: Add support for Fortemedia FM34NE DSP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] asoc: fm34ne-dsp: I2C transfer errors are silently masked by unconditional return 0
- [Low] asoc: fm34ne-dsp: mixing goto-based cleanup with guard() scope-based locking
--

commit c702ef2250bbc5a4eaf5b40b9bb43fc3e069825a
Author: Svyatoslav Ryhel <clamor95@gmail.com>

ASoC: codecs: Add support for Fortemedia FM34NE DSP

This commit introduces a new I2C-based ALSA SoC codec driver for the
Fortemedia FM34NE DSP. This chip is primarily used for active noise
suppression on devices like the ASUS Transformers.

> diff --git a/sound/soc/codecs/fm34ne-dsp.c b/sound/soc/codecs/fm34ne-dsp.c
> new file mode 100644
> index 0000000000000..74520e37c1d1a
> --- /dev/null
> +++ b/sound/soc/codecs/fm34ne-dsp.c
> @@ -0,0 +1,388 @@
[ ... ]
> +static int fm34ne_dsp_set_config(struct fm34ne_dsp *dsp, enum dsp_state state)
> +{
> +	struct device *dev = &dsp->client->dev;
> +	int ret;
> +
> +	guard(mutex)(&dsp->lock);
> +
> +	gpiod_set_value_cansleep(dsp->bypass_gpio, 1);
> +	msleep(20);
> +
> +	switch (state) {
> +	case FM34NE_NS_ENABLE:
> +		ret = i2c_master_send(dsp->client, dsp->info->enable_parameter,
> +				      dsp->info->enable_parameter_length);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to set DSP enable %d\n", ret);
> +			goto exit;

[Severity: Low]
Does this mix goto-based error cleanup with scope-based locking?

According to the kernel's cleanup helpers documentation, the expectation
is that the usage of goto labels and scope-based cleanup helpers (like
guard) should not be mixed in the same function. This is to avoid confusing
ownership and lock release semantics. Could this be converted entirely to
scope-based cleanup, or stick to traditional locking?

> +		}
> +
> +		ret = i2c_master_send(dsp->client, dsp->info->enable_noise_suppression,
> +				      dsp->info->enable_ns_length);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to enable DSP noise suppression %d\n", ret);
> +			goto exit;
> +		}
> +
> +		break;
[ ... ]
> +exit:
> +	gpiod_set_value_cansleep(dsp->bypass_gpio, 0);
> +
> +	return 0;

[Severity: Medium]
Does this unconditionally discard the error code returned by
i2c_master_send on the failure paths?

By returning 0 here after jumping to the exit label, callers like
fm34ne_dsp_probe will assume the hardware was successfully configured
even if the underlying I2C transfer failed. Should this propagate the
negative error code stored in ret instead?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013184755.530392-1-clamor95@gmail.com?part=2