Re: [PATCH v8 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]

On Fri, Apr 10, 2026 at 11:04:21PM -0400, Guodong Xu wrote:
> 
> This patch introduces the driver for the SPI controller found in the
> SpacemiT K1 SoC.  Currently the driver supports master mode only.
> The SPI hardware implements RX and TX FIFOs, 32 entries each, and
> supports both PIO and DMA mode transfers.

> +static struct dma_async_tx_descriptor *
> +k1_spi_dma_prep(struct k1_spi_driver_data *drv_data,
> +		struct spi_transfer *transfer, bool tx)
> +{
> +	phys_addr_t addr = drv_data->base_addr + SSP_DATAR;
> +	u32 burst_size = K1_SPI_THRESH * drv_data->bytes;
> +	struct dma_slave_config cfg = { };
> +	enum dma_transfer_direction dir;
> +	enum dma_slave_buswidth width;
> +	struct dma_chan *chan;
> +	struct sg_table *sgt;
> +
> +	width = drv_data->bytes == 1 ? DMA_SLAVE_BUSWIDTH_1_BYTE :
> +		drv_data->bytes == 2 ? DMA_SLAVE_BUSWIDTH_2_BYTES :
> +		/* bytes == 4 */       DMA_SLAVE_BUSWIDTH_4_BYTES;

Please use normal conditional statements (in this case a case statement)
to keep the code legible.

> +static irqreturn_t k1_spi_ssp_isr(int irq, void *dev_id)
> +{
> +	struct k1_spi_driver_data *drv_data = dev_id;
> +	u32 val;

> +	/* Return immediately if we're not expecting any interrupts */
> +	if (!drv_data->transfer)
> +		return IRQ_NONE;

That does't mean the hardware agrees!

> +	/* Get status and clear pending interrupts; all are handled below */
> +	val = readl(drv_data->base + SSP_STATUS);
> +	writel(val, drv_data->base + SSP_STATUS);

Nothing after here can report IRQ_NONE, even if SSP_STATUS didn't flag
anything.  I'd just move the checks for transfer to when we're handling
FIFOs and have the IRQ_NONE report be based on there being something set
in the ISR.

> +	/*
> +	 * For SPI, bytes are transferred in both directions equally, and
> +	 * RX always follows TX.  Start by writing if there is anything to
> +	 * write, then read.  Once there's no more to read, we're done.
> +	 */
> +	if (drv_data->tx_resid && (val & SSP_STATUS_TNF)) {
> +		/* If we finish writing, disable TX interrupts */
> +		if (k1_spi_write(drv_data, val)) {
> +			val = SSP_INT_EN_RX | SSP_INT_EN_ERROR;
> +			writel(val, drv_data->base + SSP_INT_EN);
> +		}
> +	}

This overwrites val...

> +
> +	/* We're not done unless we've read all that was requested */
> +	if (drv_data->rx_resid) {
> +		/* Read more if there FIFO is not empty */
> +		if (val & SSP_STATUS_RNE)
> +			if (k1_spi_read(drv_data, val))
> +				goto done;

...so the read won't see that there's data to read and we'll need
another interrupt.  I would suggest using a more meaingful name for the
actual interrupt status.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]