linux-kernel - Re: [PATCH v2 2/3] spi: axiado: Add driver for Axiado SPI DB controller

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aNrBx4KTgGT5OE72@finisterre.sirena.org.uk>
Date: Mon, 29 Sep 2025 18:28:39 +0100
From: Mark Brown <broonie@...nel.org>
To: Vladimir Moravcevic <vmoravcevic@...ado.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Harshit Shah <hshah@...ado.com>,
	Tzu-Hao Wei <twei@...ado.com>,
	Axiado Reviewers <linux-maintainer@...ado.com>,
	linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Prasad Bolisetty <pbolisetty@...ado.com>
Subject: Re: [PATCH v2 2/3] spi: axiado: Add driver for Axiado SPI DB
 controller

On Mon, Sep 29, 2025 at 01:58:02AM -0700, Vladimir Moravcevic wrote:
> The Axiado SPI controller is present in AX3000 SoC and Evaluation Board.
> This controller is operating in Host only mode.

This looks mostly good, but the formatting is a bit odd, one small bug
and there are licensing problems.

> Signed-off-by: Vladimir Moravcevic <vmoravcevic@...ado.com>
> Signed-off-by: Prasad Bolisetty <pbolisetty@...ado.com>

The signoff of the person sending the mail should be last.

> +++ b/drivers/spi/spi-axiado.c
> @@ -0,0 +1,1029 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Axiado SPI controller driver (Host mode only)
> + *
> + * Copyright (C) 2022-2025 Axiado Corporation (or its affiliates). All rights reserved.
> + *
> + */

All rights reserved or GPL?

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

> +	/* xspi->clk_rate - AMBA Slave clock frequency
> +	 * transfer->speed_hz - Slave clock required frequency
> +	 * As per data sheet - SCD = (AMBA Slave clock/SCK) - 2
> +	 */

Please use terms like "device" rather than outdated ones like slave.

> +}
> +/**

Blank line between functions please.

> +	/* Process any remaining bytes in the RX FIFO */
> +	u32 avail_bytes = ax_spi_read(xspi, AX_SPI_RX_FBCAR);
> +
> +	// This loop handles bytes that are already staged from a previous word read
> +	while (xspi->bytes_left_in_current_rx_word_for_irq &&

The mix of /* and // seems a bit random, does it mean something?

> +	 * Handle "Message Transfer Complete" interrupt.
> +	 * This means all bytes have been shifted out of the TX FIFO.
> +	 * It's time to harvest the final incoming bytes from the RX FIFO.
> +	 */
> +	if (intr_status & AX_SPI_IVR_MTCV) {

> +		ax_spi_process_rx_and_finalize(ctlr);
> +		return IRQ_HANDLED;
> +	}

> +	if (intr_status & AX_SPI_IVR_RFFV) {

We can't get incoming and outgoing data interrupts simultaneously
asserted?

> +	drain_limit = AX_SPI_RX_FIFO_DRAIN_LIMIT; // Sane limit to prevent infinite loop on HW error
> +	while (ax_spi_read(xspi, AX_SPI_RX_FBCAR) > 0 && drain_limit-- > 0)
> +		(void)ax_spi_read(xspi, AX_SPI_RXFIFO); // Read and discard
> +

If you need the cast to void there something is going wrong.

> +	ax_spi_setup_transfer(spi, transfer);
> +	ax_spi_fill_tx_fifo(xspi);
> +	ax_spi_write(xspi, AX_SPI_CR2, (AX_SPI_CR2_HTE | AX_SPI_CR2_SRD | AX_SPI_CR2_SWD));
> +
> +	spi_transfer_delay_exec(transfer);

The core will implement any per transfer delays, this will double them.

> +	fifo_total_bytes = xspi->tx_fifo_depth;
> +	/* Calculate protocol overhead bytes according to the real operation each time. */
> +	protocol_overhead_bytes = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;

/*
 *
 */


> +	/*Calculate the maximum data payload that can fit into the FIFO. */

/* Calculate

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