Re: [PATCH 2/2] spi: qcom-geni: Use GPIO to notify master of SPI target activity

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The probe function incorrectly handles errors from devm_gpiod_get_index_optional(), breaking probe deferral.
- [High] A race condition between spi_geni_transfer_one() and geni_spi_isr() can leave the target readiness GPIO stuck permanently asserted.
- [Medium] The target readiness GPIO is toggled per spi_transfer rather than per spi_message, causing spurious dropouts mid-transaction.
--

--- Patch [2]: [PATCH 2/2] spi: qcom-geni: Use GPIO to notify master of SPI target activity ---
commit c41b3184af4cdb8d8cf0321da6d2ab29ec44d0c2
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
spi: qcom-geni: Use GPIO to notify master of SPI target activity

This commit adds support for an optional GPIO to signal target availability
to the SPI master in the GENI SPI controller driver. The GPIO is asserted
when a transfer begins and deasserted upon completion, abort, or timeout.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 6566975eb24f2..6083725a20aa4 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c

[ ... ]

> @@ -938,8 +943,12 @@ static int spi_geni_transfer_one(struct spi_controller *spi,
>  	if (mas->cur_xfer_mode == GENI_SE_FIFO || mas->cur_xfer_mode == GENI_SE_DMA) {
>  		ret = setup_se_xfer(xfer, mas, slv->mode, spi);
>  		/* SPI framework expects +ve ret code to wait for transfer complete */
> -		if (!ret)
> +		if (!ret) {
> +			if (spi->target && mas->target_gpio)
> +				gpiod_set_value(mas->target_gpio, 1);

[Severity: High]
Does asserting the GPIO here introduce a race condition?

setup_se_xfer() unlocks mas->lock before returning. If the SPI master
immediately clocks data, the transfer could complete on the hardware and
trigger geni_spi_isr() on another CPU.

geni_spi_isr() would then deassert the GPIO to 0 before this thread
asserts it to 1, leaving the GPIO permanently stuck asserted and breaking
the handshake protocol for subsequent transfers.

[Severity: Medium]
Is the target readiness GPIO toggled per spi_transfer instead of per
spi_message here?

If a multi-transfer message is processed, this will cause the signal to
drop between fragments when geni_spi_isr() sets the GPIO to 0, potentially
causing the master to incorrectly interpret it as a target abort or failure.

>  			ret = 1;
> +		}
> +
>  		return ret;
>  	}

[ ... ]

> @@ -1131,8 +1144,16 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	init_completion(&mas->rx_reset_done);
>  	spin_lock_init(&mas->lock);
>  
> -	if (spi->target)
> +	if (spi->target) {
>  		spi->target_abort = spi_geni_target_abort;
> +		mas->target_gpio = devm_gpiod_get_index_optional(dev, "device", 0,
> +								 GPIOD_OUT_LOW);
> +		if (IS_ERR(mas->target_gpio)) {
> +			dev_err(dev, "Failed to request GPIO: %ld\n",
> +				PTR_ERR(mas->target_gpio));
> +			mas->target_gpio = NULL;
> +		}
> +	}

[Severity: High]
Could this error handling block probe deferral?

If the GPIO provider has not probed yet, devm_gpiod_get_index_optional()
will return -EPROBE_DEFER. By catching this error and setting
mas->target_gpio to NULL, the driver will silently continue without the
requested GPIO instead of deferring the probe.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-use_gpio_to_notify_master_of_spi_target_activity-v1-0-c363a9b5df69@oss.qualcomm.com?part=2