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

[Mark Brown]

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@xxxxxxxxxx>
> Signed-off-by: Prasad Bolisetty <pbolisetty@xxxxxxxxxx>

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

Attachment: signature.asc
Description: PGP signature