Re: [PATCH v2 3/5] spi: atmel-quadspi: use init callback for gclk variants

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The `.init` callback invocation in `atmel_qspi_resume` is strictly coupled to `has_gclk`, contradicting the commit message claim.

Pre-existing issues:
- [High] Runtime PM usage counter leak permanently prevents runtime suspend for gclk variants.
- [High] System resume accesses memory-mapped hardware registers with the peripheral clock disabled, causing a bus fault.
--

--- Patch [3]: [PATCH v2 3/5] spi: atmel-quadspi: use init callback for gclk variants ---
commit ce20548a48abc8b47ba02b69140b1f7ba51f52af
Author: Robert Marko <robert.marko@sartura.hr>

spi: atmel-quadspi: use init callback for gclk variants
    
Assign the existing SAMA7G5 initialization routine to every generic
clock variant and dispatch initialization exclusively through the
capability callback.

> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index 62ea84d234d02..d1aec14e39789 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c

[ ... ]

> @@ -1576,11 +1573,8 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
>  		return ret;
>  	}
>  
> -	if (aq->caps->init)
> -		return aq->caps->init(aq);
> -
>  	if (aq->caps->has_gclk)
> -		return atmel_qspi_sama7g5_init(aq);
> +		return aq->caps->init(aq);

[Severity: High]
This is a pre-existing issue, but does this early return cause a runtime PM
usage counter leak?

In atmel_qspi_suspend(), the usage counter is incremented but not balanced
for has_gclk variants:

    ret = pm_runtime_resume_and_get(dev);
    ...
    if (aq->caps->has_gclk) {
        ret = atmel_qspi_sama7g5_suspend(aq);
        clk_disable_unprepare(aq->pclk);
        return ret;
    }

By returning early here in atmel_qspi_resume(), the
pm_runtime_put_autosuspend(dev) call at the end of the function is completely
bypassed. Will this permanently inflate the usage counter on every system
suspend/resume cycle and prevent the device from entering runtime suspend?

[Severity: High]
This is also a pre-existing issue, but does this early return lead to accessing
hardware registers with the peripheral clock disabled?

During atmel_qspi_suspend(), the peripheral clock is explicitly disabled for
has_gclk variants:

    clk_disable_unprepare(aq->pclk);
    return ret;

In atmel_qspi_resume(), clk_prepare() is called, but clk_enable() is never
invoked before the early return:

    ret = clk_prepare(aq->pclk);
    ...
    if (aq->caps->has_gclk)
        return aq->caps->init(aq);

Since pm_runtime_force_resume() is bypassed by this early return, the runtime
resume callback is not triggered to enable the clock. Will this cause a bus
fault when aq->caps->init() performs register writes (e.g., to QSPI_CR)?

[Severity: Medium]
The commit message states that this change "keeps hardware capabilities
separate from initialization selection".

However, by removing the generic check for aq->caps->init and gating it
strictly behind aq->caps->has_gclk, doesn't this couple the init callback
invocation to the has_gclk capability?

If a non-has_gclk variant defined an init callback, wouldn't it now be
ignored during system resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709112006.390742-1-robert.marko@sartura.hr?part=3