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

Mark Brown
2025-09-29T17:28:56Z

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

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

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