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:
signature.asc
Description: PGP signature
