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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aRoVqVtYLJJAPCia@sirena.co.uk>
Date: Sun, 16 Nov 2025 18:19:21 +0000
From: Mark Brown <broonie@...nel.org>
To: Alex Elder <elder@...cstar.com>
Cc: dlan@...too.org, p.zabel@...gutronix.de, linux-spi@...r.kernel.org,
	spacemit@...ts.linux.dev, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/3] spi: spacemit: introduce SpacemiT K1 SPI
 controller driver

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...

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)