[PATCH v4 2/3] spi: Add Amlogic SPISG driver

[Mark Brown]

On Fri, Jul 04, 2025 at 10:59:33AM +0800, Xianwei Zhao via B4 Relay wrote:

> Introduced support for the new SPI IP (SPISG) driver. The SPISG is
> a communication-oriented SPI controller from Amlogic,supporting
> three operation modes: PIO, block DMA, and scatter-gather DMA.

This looks good, a few small things below but nothing major.

> +static bool aml_spisg_can_dma(struct spi_controller *ctlr,
> +			      struct spi_device *spi,
> +			      struct spi_transfer *xfer)
> +{
> +	return true;
> +}

Is it worth having a copybreak such that smaller transfers are done
using PIO?  With a lot of controllers that increases performance due to
the extra overhead of setting up DMA, talking to the DMA and interrupt
controllers can be as expensive as directly accessing the FIFOs.

> +static irqreturn_t aml_spisg_irq(int irq, void *data)
> +{
> +	struct spisg_device *spisg = (void *)data;
> +	u32 sts;
> +
> +	spisg->status = 0;
> +	regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts);
> +	regmap_write(spisg->map, SPISG_REG_IRQ_STS, sts);
> +	if (sts & (IRQ_RCH_DESC_INVALID |
> +		   IRQ_RCH_DESC_RESP |
> +		   IRQ_RCH_DATA_RESP |
> +		   IRQ_WCH_DESC_INVALID |
> +		   IRQ_WCH_DESC_RESP |
> +		   IRQ_WCH_DATA_RESP |
> +		   IRQ_DESC_ERR))
> +		spisg->status = sts;
> +
> +	complete(&spisg->completion);
> +
> +	return IRQ_HANDLED;

It'd be better to check if there's an interrupt actually flagged and
return IRQ_NONE if not, as well as supporting sharing that means that
the interrupt core can handle any errors that cause the interrupt to
latch on.

> +	ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg);
> +	if (ret) {
> +		dev_err(&pdev->dev, "irq request failed\n");
> +		goto out_controller;
> +	}
> +
> +	ret = aml_spisg_clk_init(spisg, base);
> +	if (ret)
> +		goto out_controller;

Do we need the clocks for register access - if so what happens if the
interrupt fires as soon as it is registered?  I'd have expected
requesting the interrupt to be one of the last things done.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20250707/dc9911d8/attachment.sig>