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

[Mark Brown <broonie@kernel.org>]

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

On Fri, Nov 14, 2025 at 12:57:43PM -0600, Alex Elder 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.

This looks mostly good but there's a bit of open coding that looks like
the driver could make more use of the core.

> @@ -0,0 +1,966 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SpacemiT K1 SPI controller driver
> + *
> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> + * Copyright (c) 2023, spacemit Corporation.
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +static bool k1_spi_map_dma_buffer(struct k1_spi_io *io, size_t len, void *dummy)
> +{
> +	struct device *dmadev = io->chan->device->dev;
> +	unsigned int nents = DIV_ROUND_UP(len, SZ_2K);
> +	struct sg_table *sgt = &io->sgt;
> +	void *bufp = io->buf ? : dummy;
> +	struct scatterlist *sg;
> +	unsigned int i;

The SPI core can do DMA mapping for you, the only thing this is doing
that's unusual is that it's imposing a fixed 2K limit on block sizes.
If this limit comes from the DMA controller (which looks to be the case
since we feed the entire table into the DMA controller at once?) the
core will already DTRT here, assuming the DMA controller correctly
advertises this restriction.

> +static bool k1_spi_map_dma_buffers(struct k1_spi_driver_data *drv_data)
> +{

...

> +	/* Don't bother with DMA if we can't do even a single burst */
> +	if (drv_data->len < dma_burst_size)
> +		return false;
> +
> +	/* We won't use DMA if the transfer is too big, either */
> +	if (drv_data->len > K1_SPI_MAX_DMA_LEN)
> +		return false;

The core has a can_dma() callback for this.

> +static int k1_spi_transfer_one_message(struct spi_controller *host,
> +					   struct spi_message *message)
> +{

...

> +	/* Hold frame low to avoid losing transferred data */
> +	val = readl(drv_data->base + SSP_TOP_CTRL);
> +	val |= TOP_HOLD_FRAME_LOW;
> +	writel(val, drv_data->base + SSP_TOP_CTRL);

This looks like it should be a set_cs() operation?

> +
> +	list_for_each_entry(transfer, &message->transfers, transfer_list) {
> +		reinit_completion(completion);
> +
> +		/* Issue the next transfer */
> +		if (!k1_spi_transfer_start(drv_data, transfer)) {
> +			message->status = -EIO;
> +			break;
> +		}
> +
> +		k1_spi_transfer_wait(drv_data);
> +
> +		k1_spi_transfer_end(drv_data, transfer);

Why not just implement the transfer_one() callback?  This just looks
like it's duplicating code.

> +static irqreturn_t k1_spi_ssp_isr(int irq, void *dev_id)
> +{

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

This unconditionally acknowledges all interrupts even if we didn't
handle anything...

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