Re: [PATCH 3/4] mmc: sdhci-of-k1: Set clock-frequency of io clock

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Missing check for `clk_set_rate()` return value, masking configuration failures.

Pre-existing issues:
- [High] Breaking probe deferral by obscuring `devm_clk_get_enabled()` error codes with `-EINVAL`.
- [High] Probe deferral is broken for pinctrl due to unconditionally ignoring all errors.
--

commit 9748f01c4f206e03365b12c3c2ceed5bed26c9c0
Author: Long Wan <long.wan@linux.spacemit.com>

mmc: sdhci-of-k1: Set clock-frequency of io clock

Add a clock-frequency property to set the rate of io clock, for SDIO
case, a clock frequency need to propagate down early.

> diff --git a/drivers/mmc/host/sdhci-of-k1.c b/drivers/mmc/host/sdhci-of-k1.c
> index 37b0911e7cf20..e6ab40c9a5551 100644
> --- a/drivers/mmc/host/sdhci-of-k1.c
> +++ b/drivers/mmc/host/sdhci-of-k1.c
> @@ -438,6 +438,8 @@ static inline int spacemit_sdhci_get_clocks(struct device *dev,
>  					    struct sdhci_pltfm_host *pltfm_host)
>  {
>  	struct spacemit_sdhci_host *sdhst = sdhci_pltfm_priv(pltfm_host);
> +	struct device_node *np = dev->of_node;
> +	u32 freq;
>  
>  	sdhst->clk_core = devm_clk_get_enabled(dev, "core");
>  	if (IS_ERR(sdhst->clk_core))
>  		return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but does returning -EINVAL here
break probe deferral? If devm_clk_get_enabled() in spacemit_sdhci_get_clocks()
(drivers/mmc/host/sdhci-of-k1.c) returns -EPROBE_DEFER, returning -EINVAL will
cause the probe to fail permanently. Should this propagate
PTR_ERR(sdhst->clk_core) instead?

>  
>  	sdhst->clk_io = devm_clk_get_enabled(dev, "io");
>  	if (IS_ERR(sdhst->clk_io))
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this also break probe deferral for the
same reason? Should we propagate PTR_ERR(sdhst->clk_io) in
spacemit_sdhci_get_clocks() instead of returning -EINVAL?

>  
> +	if (!of_property_read_u32(np, "clock-frequency", &freq))
> +		clk_set_rate(sdhst->clk_io, freq);

[Severity: Medium]
Does this code silently ignore clock configuration failures? If clk_set_rate()
in spacemit_sdhci_get_clocks() (drivers/mmc/host/sdhci-of-k1.c) fails and
returns an error code (for instance, if the frequency is unsupported or
rejected by CLK_SET_RATE_GATE), the failure is masked. Should we check the
return value and handle potential configuration failures?

> +
>  	pltfm_host->clk = sdhst->clk_io;
>  
>  	return 0;

While reviewing the file, I noticed another pre-existing issue in
spacemit_sdhci_get_pins():

drivers/mmc/host/sdhci-of-k1.c:spacemit_sdhci_get_pins() {
    [ ... ]
	sdhst->pinctrl = devm_pinctrl_get(dev);
	if (IS_ERR(sdhst->pinctrl)) {
		sdhst->pinctrl = NULL;
		dev_dbg(dev, "pinctrl not available, voltage switching will work without it\n");
		return;
	}
    [ ... ]
}

[Severity: High]
This is a pre-existing issue, but does this code break probe deferral for
pinctrl by unconditionally ignoring all errors? If devm_pinctrl_get() in
spacemit_sdhci_get_pins() (drivers/mmc/host/sdhci-of-k1.c) returns
-EPROBE_DEFER, the driver will treat it as if no pin controller is present.
Should we check for and propagate -EPROBE_DEFER?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-07-k3-sdhci-dts-v1-0-4e7909cf4233@kernel.org?part=3