Re: [PATCH v4 4/4] spi: qcom-geni: Enable SPI on SA8255p Qualcomm platforms

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incomplete abstraction in `setup_gsi_xfer()` breaks GPI DMA transfers for SA8255p.
- [High] `spi_geni_runtime_resume()` hardcodes legacy OPP clock restore, breaking performance state restoration on SA8255p.
- [Low] Compilation fails due to undeclared identifiers `geni_se_domain_attach` and `geni_se_set_perf_opp`.

Pre-existing issues:
- [High] Ignored `pm_runtime_get_sync()` return value in `spi_geni_init()` causes kernel panic on initialization failure.
--

commit 41d8ba8fbc50744c00c196e2484d690b4592cc0a
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
Subject: spi: qcom-geni: Enable SPI on SA8255p Qualcomm platforms

This commit adds support for the SA8255p SoC to the qcom-geni SPI driver.
It abstracts resources initialization and rate setting into a new
geni_spi_desc structure to handle SCMI-based power and performance protocols
required by this platform.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 025165c4b9527..88ac0833351c0 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c

[ ... ]

> @@ -825,7 +834,7 @@ static int setup_se_xfer(struct spi_transfer *xfer,
>  	}
>  
>  	/* Speed and bits per word can be overridden per transfer */
> -	ret = geni_spi_set_clock_and_bw(mas, xfer->speed_hz);
> +	ret = mas->dev_data->set_rate(&mas->se, xfer->speed_hz);
>  	if (ret)
>  		return ret;

[Severity: High]
Since setup_se_xfer() now uses the new set_rate callback, should
setup_gsi_xfer() be updated as well?

Currently, setup_gsi_xfer() appears to hardcode a call to the legacy
get_spi_clk_cfg() function, which bypasses mas->dev_data->set_rate.
Could this break GPI DMA transfers for the new SA8255p platform by leaving
the hardware misconfigured?

[ ... ]

> @@ -1075,7 +1084,11 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	mas->se.wrapper = dev_get_drvdata(dev->parent);
>  	mas->se.base = base;
>  
> -	ret = geni_se_resources_init(&mas->se);
> +	mas->dev_data = device_get_match_data(&pdev->dev);
> +	if (!mas->dev_data)
> +		return -EINVAL;
> +
> +	ret = mas->dev_data->resources_init(&mas->se);
>  	if (ret)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does spi_geni_init() safely handle power
domain or clock enablement failures?

In spi_geni_init(), which is called shortly after this probe setup, the
return value of pm_runtime_get_sync() is ignored before accessing hardware:

    pm_runtime_get_sync(mas->dev);
    proto = geni_se_read_proto(se);

If pm_runtime_get_sync() fails, could the subsequent read from the
GENI_FW_REVISION_RO register via geni_se_read_proto() cause an asynchronous
external abort and panic the kernel?

[ ... ]

> @@ -1140,9 +1154,11 @@ static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
>  	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
>  	int ret;
>  
> -	ret = geni_se_resources_activate(&mas->se);
> -	if (ret)
> -		return ret;
> +	if (mas->dev_data->power_on) {
> +		ret = mas->dev_data->power_on(&mas->se);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (mas->se.has_opp)
>  		return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);

[Severity: High]
Does this hardcoded OPP restore bypass the new set_rate abstraction for the
SA8255p platform?

For SA8255p, mas->dev_data->set_rate is geni_se_set_perf_opp(), which
does not update the legacy mas->cur_sclk_hz variable. If has_opp is true,
could this accidentally set the clock rate to 0 and stall the device?
Alternatively, if has_opp is false, could this fail to restore the
performance state entirely?

[ ... ]

> @@ -1188,8 +1204,21 @@ static const struct dev_pm_ops spi_geni_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(spi_geni_suspend, spi_geni_resume)
>  };
>  
> +static const struct geni_spi_desc geni_spi = {
> +	.resources_init = geni_se_resources_init,
> +	.set_rate = geni_spi_set_clock_and_bw,
> +	.power_on = geni_se_resources_activate,
> +	.power_off = geni_se_resources_deactivate,
> +};
> +
> +static const struct geni_spi_desc sa8255p_geni_spi = {
> +	.resources_init = geni_se_domain_attach,
> +	.set_rate = geni_se_set_perf_opp,
> +};

[Severity: Low]
Are geni_se_domain_attach and geni_se_set_perf_opp declared elsewhere?

It looks like these identifiers are not defined in this file or the included
headers, which might cause a build failure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-enable-spi-on-sa8255p-v4-0-f5b5067e7e1e@oss.qualcomm.com?part=4