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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ASoC: fm34ne-dsp: goto mixing and stuck bypass_gpio in fm34ne_dsp_set_config()
- [Medium] ASoC: fm34ne-dsp: conflicting DAPM widget events clobber DSP state
- [Low] ASoC: fm34ne-dsp: reset GPIO left deasserted when DSP is disabled
--

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

ASoC: codecs: Add support for Fortemedia FM34NE DSP

This commit adds an ASoC codec driver for the Fortemedia FM34NE digital
sound processing chip used for active noise suppression on ASUS
Transformer devices.

> diff --git a/sound/soc/codecs/fm34ne-dsp.c b/sound/soc/codecs/fm34ne-dsp.c
> new file mode 100644
> index 0000000000000..608b94e6ea0ca
> --- /dev/null
> +++ b/sound/soc/codecs/fm34ne-dsp.c

[ ... ]

> +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;
> +		}
> +
> +		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;
> +
> +	case FM34NE_NS_DISABLE:
> +		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 with %d\n", ret);
> +			goto exit;
> +		}
> +
> +		ret = i2c_master_send(dsp->client, dsp->info->disable_noise_suppression,
> +				      dsp->info->disable_ns_length);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to disable DSP noise suppression with %d\n", ret);
> +			goto exit;
> +		}
> +
> +		break;
> +
> +	case FM34NE_BYPASS:
> +	default:
> +		ret = i2c_master_send(dsp->client, bypass_parameter, sizeof(bypass_parameter));
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to set DSP bypass with %d\n", ret);
> +			goto exit;
> +		}
> +
> +		break;
> +	}
> +
> +exit:
> +	if (ret < 0)
> +		return ret;
> +
> +	gpiod_set_value_cansleep(dsp->bypass_gpio, 0);
> +
> +	return 0;
> +}

[Severity: Medium]
In sound/soc/codecs/fm34ne-dsp.c, fm34ne_dsp_set_config() mixes
guard(mutex)(&dsp->lock) with goto exit labels, which goes against the
scope-based cleanup guidelines in include/linux/cleanup.h.

Also, if any i2c_master_send() call fails during a DAPM widget event
(via fm34ne_dsp_int_state_control() or fm34ne_dsp_ext_state_control()),
the exit label returns early when ret < 0 before resetting
dsp->bypass_gpio back to 0:

exit:
	if (ret < 0)
		return ret;

	gpiod_set_value_cansleep(dsp->bypass_gpio, 0);

Should fm34ne_dsp_set_config() deassert dsp->bypass_gpio before
returning on error so the DSP is not left stuck in programming mode, and
avoid mixing guard(mutex) with goto labels (as well as explicitly
including <linux/cleanup.h> and <linux/mutex.h>)?

> +static int fm34ne_dsp_ext_state_control(struct snd_soc_dapm_widget *w,
> +					struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +	struct fm34ne_dsp *dsp = snd_soc_component_get_drvdata(component);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		return fm34ne_dsp_set_config(dsp, FM34NE_BYPASS);
> +
> +	default:
> +		WARN(1, "Unexpected event");
> +		return -EINVAL;
> +	}
> +}
> +
> +static int fm34ne_dsp_int_state_control(struct snd_soc_dapm_widget *w,
> +					struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +	struct fm34ne_dsp *dsp = snd_soc_component_get_drvdata(component);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		return fm34ne_dsp_set_config(dsp, FM34NE_NS_ENABLE);
> +
> +	case SND_SOC_DAPM_PRE_PMD:
> +		return fm34ne_dsp_set_config(dsp, FM34NE_NS_DISABLE);
> +
> +	default:
> +		WARN(1, "Unexpected event");
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct snd_soc_dapm_widget fm34ne_dsp_dapm_widgets[] = {
> +	SND_SOC_DAPM_INPUT("DSP DMIC"),
> +	SND_SOC_DAPM_INPUT("DSP AMIC"),
> +
> +	SND_SOC_DAPM_OUT_DRV_E("DSP Processing Engine INT", SND_SOC_NOPM,
> +			       0, 0, NULL, 0, fm34ne_dsp_int_state_control,
> +			       SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
> +	SND_SOC_DAPM_OUT_DRV_E("DSP Processing Engine EXT", SND_SOC_NOPM,
> +			       0, 0, NULL, 0, fm34ne_dsp_ext_state_control,
> +			       SND_SOC_DAPM_POST_PMU),
> +
> +	SND_SOC_DAPM_OUTPUT("DSP Digital Mic"),
> +	SND_SOC_DAPM_OUTPUT("DSP Analog Mic"),
> +};

[Severity: Medium]
Can transitions between "DSP Processing Engine INT" and
"DSP Processing Engine EXT" clobber the shared DSP state?

Both widgets call fm34ne_dsp_set_config() to update the global hardware
state without tracking which engine is active:

1. If "DSP Processing Engine EXT" powers up (SND_SOC_DAPM_POST_PMU ->
FM34NE_BYPASS) and "DSP Processing Engine INT" subsequently powers down
(SND_SOC_DAPM_PRE_PMD -> FM34NE_NS_DISABLE), fm34ne_dsp_int_state_control()
sends dsp->info->enable_parameter and dsp->info->disable_noise_suppression,
taking the DSP out of bypass mode while the external analog mic path is
still active.

2. Conversely, "DSP Processing Engine EXT" only handles
SND_SOC_DAPM_POST_PMU and has no PMD handler. If both paths are active
and "DSP Processing Engine EXT" powers down while
"DSP Processing Engine INT" remains powered on, the DSP stays in
FM34NE_BYPASS instead of restoring FM34NE_NS_ENABLE.

Should the driver coordinate state across these widgets (or model the
routing so the active path's configuration is preserved)?

[ ... ]

> +static void fm34ne_dsp_reset(struct fm34ne_dsp *dsp)
> +{
> +	gpiod_set_value_cansleep(dsp->reset_gpio, 1);
> +	msleep(20);
> +	gpiod_set_value_cansleep(dsp->reset_gpio, 0);
> +	msleep(100);
> +}
> +
> +static void fm34ne_dsp_disable_hw(struct fm34ne_dsp *dsp)
> +{
> +	gpiod_set_value_cansleep(dsp->bypass_gpio, 0);
> +	regulator_disable(dsp->vdd_supply);
> +	clk_disable_unprepare(dsp->dap_mclk);
> +}
> +
> +static int fm34ne_dsp_probe(struct snd_soc_component *component)
> +{

[ ... ]

> +exit:
> +	fm34ne_dsp_reset(dsp);
> +	fm34ne_dsp_disable_hw(dsp);
> +
> +	return ret;
> +}
> +
> +static void fm34ne_dsp_remove(struct snd_soc_component *component)
> +{
> +	struct fm34ne_dsp *dsp = snd_soc_component_get_drvdata(component);
> +
> +	fm34ne_dsp_reset(dsp);
> +	fm34ne_dsp_disable_hw(dsp);
> +}

[ ... ]

> +static int fm34ne_probe(struct i2c_client *client)
> +{

[ ... ]

> +	dsp->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(dsp->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(dsp->reset_gpio),
> +				     "Failed to get reset GPIO\n");

[Severity: Low]
Is the reset_gpio state inverted during probe and hardware disable?

In fm34ne_probe(), dsp->reset_gpio is requested with GPIOD_OUT_LOW
(deasserted) before vdd_supply and dap_mclk are enabled in
fm34ne_dsp_enable_hw().

In addition, both fm34ne_dsp_remove() and the error path in
fm34ne_dsp_probe() call fm34ne_dsp_reset() right before
fm34ne_dsp_disable_hw():

	fm34ne_dsp_reset(dsp);
	fm34ne_dsp_disable_hw(dsp);

fm34ne_dsp_reset() asserts reset_gpio (1) for 20 ms, then deasserts
reset_gpio (0) and sleeps 100 ms waiting for the chip to boot, only for
fm34ne_dsp_disable_hw() to immediately turn off vdd_supply and dap_mclk
with reset_gpio left deasserted.

Should fm34ne_probe() request reset_gpio with GPIOD_OUT_HIGH, and should
fm34ne_dsp_disable_hw() assert reset_gpio (1) instead of calling
fm34ne_dsp_reset() before power-down?

[ ... ]

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