On Tue, Sep 27, 2022 at 01:32:28PM +0200, Bert Vermeulen wrote: > > +config SPI_AIROHA_EN7523 > + bool "Airoha EN7523 SPI controller support" Why not tristate? > + depends on ARCH_AIROHA I don't see a reason we couldn't have an || COMPILE_TEST here to improve coverage? > + default ARCH_AIROHA It's unusual to default a SPI controller on, they tend not to be ultra critical like a clock driver or similar can be? > +static void __iomem *iobase; This should be driver data rather than a global, your current SoC might only have one controller but some other model might build two and it's fairly trivial to do. > +static void opfifo_write(u32 cmd, u32 len) > +{ > + u32 tmp = ((cmd & 0x1f) << 9) | (len & 0x1ff); > + > + writel(tmp, REG(ENSPI_MANUAL_OPFIFO_WDATA)); > + > + /* Wait for room in OPFIFO */ > + while (readl(REG(ENSPI_MANUAL_OPFIFO_FULL))) > + cpu_relax(); > + Some sort of timeout would be good with these loops, if things go wrong we'll just lock up which isn't good. > + ret = clk_prepare_enable(clk); > + if (ret) > + return ret; Nothing ever reverses this unless clk_set_rate() fails. > + ret = clk_set_rate(clk, 40000000); > + if (ret) { > + clk_disable_unprepare(clk); > + return ret; > + } Could this be pushed into DT via the clock bindings? The hard coded number might need to vary by SoC. > +static int xfer_read(struct spi_transfer *xfer) > +{ > + int opcode; > + uint8_t *buf = xfer->rx_buf; > + > + switch (xfer->rx_nbits) { > + case SPI_NBITS_SINGLE: > + opcode = OP_INS; > + break; > + case SPI_NBITS_DUAL: > + opcode = OP_IND; > + break; > + } This should have a default case that returns an error. > +static int transfer_one_message(struct spi_controller *ctrl, struct spi_message *msg) > +{ > + struct spi_transfer *xfer; > + int next_xfer_is_rx = 0; > + > + manual_begin_cmd(); > + set_cs(0); The driver should not be setting chip select itself, it should just provide the set_cs() operation to the core and let the core worry about when to call it. > + ctrl->transfer_one_message = transfer_one_message; > + err = devm_spi_register_controller(&pdev->dev, ctrl); > + if (err) { > + dev_err(&pdev->dev, "Could not register SPI controller\n"); > + return -ENODEV; > + } Don't discard the error code that registeration returned, include it in the log message and pass it back to the caller.
Attachment:
signature.asc
Description: PGP signature