Re: [PATCH v6 08/11] ASoC: renesas: fsi: refactor clock initialization

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional clock initialization during probe breaks the driver on slave-only platforms due to clkdev fallback.

Pre-existing issues:
- [High] Logic error in fsi_clk_enable() prevents concurrent playback and capture streams.
--

commit 83020abc14f73339100e18d0f7451918e1591b37
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: renesas: fsi: refactor clock initialization

Move fsi_clk_init() from set_fmt() to the probe path to ensure that clock
resources are acquired only once during device initialization.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index e29be2bcf952c..43bc77ebcca39 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
[ ... ]
> @@ -970,35 +980,35 @@ static int fsi_clk_set_rate_cpg(struct device *dev,
>  	return ret;
>  }
>  
> -static int fsi_clk_init(struct device *dev,
> -			struct fsi_priv *fsi,
> -			int xck,
> -			int ick,
> -			int div,
> -			int (*set_rate)(struct device *dev,
> -					struct fsi_priv *fsi))
> +static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
>  {
[ ... ]
>  	/* external clock */
>  	if (xck) {
>  		clock->xck = devm_clk_get_optional(dev, is_porta ? "xcka" : "xckb");
> -		if (IS_ERR(clock->xck)) {
> -			dev_err(dev, "can't get xck clock\n");
> -			return -EINVAL;
> -		}
> +		if (IS_ERR(clock->xck))
> +			return dev_err_probe(dev, PTR_ERR(clock->xck), "Can't get xck clock\n");
>  		if (clock->xck == clock->own) {
>  			dev_err(dev, "cpu doesn't support xck clock\n");
>  			return -EINVAL;
>  		}
[ ... ]
> @@ -1672,15 +1678,6 @@ static int fsi_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>  		break;
>  	}
>  
> -	if (fsi_is_clk_master(fsi)) {
> -		if (fsi->clk_cpg)
> -			fsi_clk_init(dai->dev, fsi, 0, 1, 1,
> -				     fsi_clk_set_rate_cpg);
> -		else
> -			fsi_clk_init(dai->dev, fsi, 1, 1, 0,
> -				     fsi_clk_set_rate_external);
> -	}
> -
>  	/* set format */
>  	if (fsi_is_spdif(fsi))
>  		ret = fsi_set_fmt_spdif(fsi);
[ ... ]
> @@ -1978,6 +1975,11 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->master	= master;
>  	fsi_port_info_init(fsi, &info.port_a);
>  	fsi_handler_init(fsi, &info.port_a);
> +	ret = fsi_clk_init(&pdev->dev, fsi);
> +	if (ret) {
> +		dev_err(&pdev->dev, "FSIA clk init failed\n");
> +		return ret;
> +	}

[Severity: High]
By removing the fsi_is_clk_master() check from fsi_dai_set_fmt() and calling
fsi_clk_init() unconditionally here during probe(), does this break the
driver on slave-only platforms?

On older platforms using clkdev, looking up missing optional clocks like
xckb or ickb can trigger a fuzzy-match fallback to the device's main
functional clock. When devm_clk_get_optional() returns this fallback clock
instead of NULL, the check in fsi_clk_init() for (clock->xck == clock->own)
evaluates to true and returns -EINVAL.

Since fsi_clk_init() is now executed unconditionally for all ports, will this
cause legitimately configured slave ports (which lack explicit master clocks)
to fail probe entirely?

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the clock
handling, I noticed a pre-existing issue in fsi_clk_enable() that prevents
concurrent streams:

sound/soc/renesas/fsi.c:fsi_clk_enable() {
        int ret = -EINVAL;

        if (!fsi_clk_is_valid(fsi))
                return ret;

        if (0 == clock->count) {
                ...
        }

        ...
        return ret;
}

When a second concurrent stream starts (for example, capture during
playback), the function skips the if (0 == clock->count) block. Because
ret is initialized to -EINVAL, the function incorrectly returns -EINVAL
instead of 0.

Does this cause fsi_hw_startup() to fail and abort the stream trigger,
permanently preventing full-duplex operation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609113836.45079-1-phucduc.bui@gmail.com?part=8