Re: [PATCH v3 2/4] spi: rockchip-sfc: add rockchip serial flash controller driver

[Mark Brown <broonie@kernel.org>]

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

On Tue, Jun 01, 2021 at 03:10:19PM -0500, Chris Morgan wrote:

This looks mostly good, a few mostly minor comments below:

> @@ -0,0 +1,861 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Rockchip Serial Flash Controller Driver

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

> +static int rockchip_sfc_get_if_type(const struct spi_mem_op *op,
> +				    struct rockchip_sfc *sfc)
> +{
> +	if (op->data.buswidth == 2)
> +		return IF_TYPE_DUAL;
> +	else if (op->data.buswidth == 4)
> +		return IF_TYPE_QUAD;
> +	else if (op->data.buswidth == 1)
> +		return IF_TYPE_STD;
> +
> +	dev_err(sfc->dev, "unsupported SPI read mode\n");
> +
> +	return -EINVAL;
> +}

This would be more idiomatically implemented as a switch statement.

> +static int rockchip_sfc_wait_fifo_ready(struct rockchip_sfc *sfc, int wr, u32 timeout)
> +{
> +	unsigned long deadline = jiffies + timeout;
> +	int level;
> +
> +	while (!(level = rockchip_sfc_get_fifo_level(sfc, wr))) {
> +		if (time_after_eq(jiffies, deadline)) {
> +			dev_warn(sfc->dev, "%s fifo timeout\n", wr ? "write" : "read");
> +			return -ETIMEDOUT;
> +		}
> +		udelay(1);
> +	}
> +
> +	return level;

The use of the assignment in the while conditional makes it hard to tell
if this code is doing what was intended.

> +static int rockchip_sfc_write_fifo(struct rockchip_sfc *sfc, const u8 *buf, int len)
> +{
> +	u8 bytes = len & 0x3;
> +	u32 dwords;
> +	int tx_level;
> +	u32 write_words;
> +	u32 tmp = 0;
> +
> +	dwords = len >> 2;
> +	while (dwords) {
> +		tx_level = rockchip_sfc_wait_fifo_ready(sfc, SFC_CMD_DIR_WR, HZ);
> +		if (tx_level < 0)
> +			return tx_level;
> +		write_words = min_t(u32, tx_level, dwords);
> +		iowrite32_rep(sfc->regbase + SFC_DATA, buf, write_words);
> +		buf += write_words << 2;
> +		dwords -= write_words;
> +		}

Weird indentation on the } here.

> +	/* write the rest non word aligned bytes */
> +	if (bytes) {
> +		tx_level = rockchip_sfc_wait_fifo_ready(sfc, SFC_CMD_DIR_WR, HZ);

It's not the source buffer being aligned that's the issue here, it's the
buffer not being a multiple of word size.

> +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_TRAN_FINISH)
> +		complete(&sfc->cp);
> +
> +	return IRQ_HANDLED;
> +}

This will silently clear any unknown interrupt, and silently claim to
have handled an interrupt even if none happened (eg, due to shared IRQs)
- it would be better to only ack interrupts we handle and return
IRQ_NONE if we didn't handle any.

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