On Wed, Jul 07, 2021 at 05:08:02PM +0800, Jon Lin wrote:

This looks pretty nice, a few small issues below but nothing major:

> +/* Maximum clock values from datasheet suggest keeping clock value under
> + * 150MHz. No minimum or average value is suggested, but the U-boot BSP driver
> + * has a minimum of 10MHz and a default of 80MHz which seems reasonable.
> + */

It's OK to just not specify a minimum if the hardware doesn't have one,
and AFAICT the driver doesn't actually use the default speed (the SPI
stack will generally try to go as fast as possible by default, the board
can configure the maximum speed and should be doing that if there's
issues).

> +static irqreturn_t rockchip_sfc_irq_handler(int irq, void *dev_id)
> +{
> +	struct rockchip_sfc *sfc = dev_id;
> +	u32 reg;
> +
> +	reg = readl(sfc->regbase + SFC_RISR);
> +
> +	/* Clear interrupt */
> +	writel_relaxed(reg, sfc->regbase + SFC_ICLR);
> +
> +	if (reg & SFC_RISR_DMA)
> +		complete(&sfc->cp);
> +
> +	return IRQ_HANDLED;
> +}

This will unconditionally ack any interrupt that is flagged, and doesn't
verify that there were any at all.  This won't work if the interrupt
ever gets shared and will mean that if something goes wrong and an
unexpected interrupt happens we'll at best just ignore it, at worst
we'll end up with a screaming interrupt constantly firing.  It'd be
better to at least return IRQ_NONE if we got anything unexpected (I
guess just if SFC_RISR_DMA isn't set, assuming there's nothing else
we're intentionally ignoring).

> +	master->mode_bits = SPI_TX_QUAD | SPI_TX_DUAL | SPI_RX_QUAD | SPI_RX_DUAL;

Should it also do SPI_HALF_DUPLEX?

> +	ret = devm_request_irq(dev, ret, rockchip_sfc_irq_handler,
> +			       0, pdev->name, sfc);
> +	if (ret) {
> +		dev_err(dev, "Failed to request irq\n");
> +
> +		return ret;
> +	}

Should we have a call to _irq_mask() before this to make sure that the
mask is set up correctly in the hardware, just in case?