Re: [PATCH 1/3] ASoC: spacemit: rename clock inputs to match binding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing spinlock in trigger and hw_params callbacks for shared registers and state variables.
- [High] Unchecked stream capabilities lead to sound card probe failure if a DMA channel is missing from the Device Tree.
--

--- Patch [1]: [PATCH 1/3] ASoC: spacemit: rename clock inputs to match binding ---
commit 3f9179a8e0f739a34984ca4b3cbbfefcad4578c2
Author: Troy Mitchell <troy.mitchell@linux.spacemit.com>
Date:   Wed Jul 22 02:35:55 2026 +0000

    ASoC: spacemit: rename clock inputs to match binding

    The driver requests the per-controller SSPA bus and functional clocks
    as "sspa_bus" and "sspa", but the device tree binding specifies them as
    "bus" and "func". This patch renames the clock inputs in the driver to
    match the binding, and updates the internal struct member for consistency.

> diff --git a/sound/soc/spacemit/k1_i2s.c b/sound/soc/spacemit/k1_i2s.c
> index 8871fc15b29cc..4247d8d3a6370 100644
> --- a/sound/soc/spacemit/k1_i2s.c
> +++ b/sound/soc/spacemit/k1_i2s.c

[ ... ]

> @@ -221,7 +221,7 @@ static int spacemit_i2s_hw_params(struct snd_pcm_substream *substream,
>  	if (ret)
>  		return ret;
>  

[Severity: High]
This is a pre-existing issue, but missing a spinlock in the hw_params and
trigger callbacks might lead to hardware state corruption.

The spacemit_i2s_hw_params() and spacemit_i2s_trigger() callbacks can be
invoked concurrently on different CPUs for playback and capture streams on a
full-duplex setup.

Both functions perform unprotected read-modify-write operations on the shared
SSCR register. For example, in spacemit_i2s_hw_params():

spacemit_i2s_hw_params() {
    ...
    val = readl(i2s->base + SSCR);
    if (val & SSCR_SSE)
        return 0;
    val &= ~SSCR_DW_32BYTE;
    val |= data_width;
    writel(val, i2s->base + SSCR);
    ...
}

Could concurrent execution cause the hw_params callback to overwrite the
SSCR_SSE enable bit set by the trigger callback, or corrupt the
i2s->started_count reference? Should a driver-level spinlock be added to
protect these shared registers and state variables?

> -	return clk_set_rate(i2s->sspa_clk, bclk_rate);
> +	return clk_set_rate(i2s->func_clk, bclk_rate);
>  }

[ ... ]

> @@ -445,14 +445,14 @@ static int spacemit_i2s_probe(struct platform_device *pdev)
>  	if (IS_ERR(i2s->bclk))
>  		return dev_err_probe(i2s->dev, PTR_ERR(i2s->bclk), "failed to enable bit clock\n");
>  
> -	clk = devm_clk_get_enabled(i2s->dev, "sspa_bus");
> +	clk = devm_clk_get_enabled(i2s->dev, "bus");
>  	if (IS_ERR(clk))
> -		return dev_err_probe(i2s->dev, PTR_ERR(clk), "failed to enable sspa_bus clock\n");
> +		return dev_err_probe(i2s->dev, PTR_ERR(clk), "failed to enable bus clock\n");
>  
> -	i2s->sspa_clk = devm_clk_get_enabled(i2s->dev, "sspa");
> -	if (IS_ERR(i2s->sspa_clk))
> -		return dev_err_probe(i2s->dev, PTR_ERR(i2s->sspa_clk),
> -				     "failed to enable sspa clock\n");
> +	i2s->func_clk = devm_clk_get_enabled(i2s->dev, "func");
> +	if (IS_ERR(i2s->func_clk))
> +		return dev_err_probe(i2s->dev, PTR_ERR(i2s->func_clk),
> +				     "failed to enable func clock\n");

[Severity: High]
This is a pre-existing issue, but unchecked stream capabilities might cause
the sound card probe to fail if a DMA channel is missing from the Device Tree.

When spacemit_i2s_probe() calls spacemit_i2s_init_dai(), the driver
allocates the dai driver struct by duplicating spacemit_i2s_dai, which
statically advertises both playback and capture capabilities with
channels_min = 1.

If the Device Tree lacks a DMA channel (for example, the rx channel), the
driver correctly sets has_capture to false but fails to clear
dai->capture.channels_min.

Would this cause the ALSA core to instantiate an unsupported capture
substream, leading dmaengine_pcm_new() to fail and abort the sound card
probe when it requests the missing DMA channel? Should the capabilities for
unsupported streams be explicitly cleared when has_playback or has_capture
is false?

>  	i2s->sysclk_div = devm_clk_get_optional_enabled(i2s->dev, "sysclk_div");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-kx-i2s-dts-v1-0-d22cb6cfaab5@linux.spacemit.com?part=1