Re: [PATCH v2 2/7] ASoC: loongson: Add Loongson-2K0300 I2S controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The hardware reset sequence is executed during probe but is missing from the resume path, which will cause the device to malfunction after waking up from sleep.
- [Low] Platform configuration structures are not marked as `const`.
--

commit 33b4276799f09ecdd4e5a93ded6b7cc2b6154e10
Author: Binbin Zhou <zhoubinbin@loongson.cn>

ASoC: loongson: Add Loongson-2K0300 I2S controller support
    
This commit adds support for the Loongson-2K0300 I2S controller. It introduces
new match data structures and applies an extra hardware reset sequence during
probe to work around a controller design flaw.

> diff --git a/sound/soc/loongson/loongson_i2s_plat.c b/sound/soc/loongson/loongson_i2s_plat.c
> index ac054b6ce6322..b4d807ee7f8f3 100644
> --- a/sound/soc/loongson/loongson_i2s_plat.c
> +++ b/sound/soc/loongson/loongson_i2s_plat.c

[ ... ]

> @@ -47,8 +53,18 @@ static int loongson_i2s_apbdma_config(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static struct loongson_i2s_plat_config ls2k1000_i2s_plat_config = {
> +	.rev_id = 0,
> +	.i2s_dma_config = loongson_i2s_apbdma_config,
> +};
> +
> +static struct loongson_i2s_plat_config ls2k0300_i2s_plat_config = {
> +	.rev_id = 1,
> +};
> +

[Severity: Low]
Should these platform configuration structures be marked as const?

Since they are statically allocated and used exclusively as read-only match
data, omitting const places them in the mutable .data section instead of
.rodata. Marking them as const would save RAM and reduce the mutable attack
surface.

>  static int loongson_i2s_plat_probe(struct platform_device *pdev)
>  {

[ ... ]

> @@ -87,11 +108,17 @@ static int loongson_i2s_plat_probe(struct platform_device *pdev)
>  	if (IS_ERR(i2s_clk))
>  		return dev_err_probe(dev, PTR_ERR(i2s_clk), "clock property invalid\n");
>  	i2s->clk_rate = clk_get_rate(i2s_clk);
> +	i2s->rev_id = plat_config->rev_id;
>  
>  	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>  	dev_set_name(dev, LS_I2S_DRVNAME);
>  	dev_set_drvdata(dev, i2s);
>  
> +	if (i2s->rev_id == 1) {
> +		regmap_update_bits(i2s->regmap, LS_I2S_CTRL, I2S_CTRL_RESET, I2S_CTRL_RESET);
> +		fsleep(200);
> +	}
> +

[Severity: High]
Will the device malfunction after waking up from sleep because this reset
sequence is missing from the resume path?

If the device loses power during system suspend, it will wake up in its
default flawed state. The shared PM operations only perform a
regcache_sync() during resume:

sound/soc/loongson/loongson_i2s.c:i2s_resume() {
	struct loongson_i2s *i2s = dev_get_drvdata(dev);
	regcache_cache_only(i2s->regmap, false);
	return regcache_sync(i2s->regmap);
}

Because LS_I2S_CTRL is a volatile register, regcache_sync() skips it.
Without reapplying this reset sequence in i2s_resume(), does the controller
remain in an unstable state after resuming?

>  	ret = devm_snd_soc_register_component(dev, &loongson_i2s_edma_component,
>  					      &loongson_i2s_dai, 1);
>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780538113.git.zhoubinbin@loongson.cn?part=2