On 9/27/22 14:23, Mark Brown wrote:
Hi Mark,
Thanks for reviewing.
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
In both cases, because SPi boot flash is the only way to boot this SoC
that I know of. However as you say this may not be the case on different
SoCs, and indeed I believe this SPI core is in lots of stuff already.
So I'll fix this, and also address your other comments.
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.
--
Bert Vermeulen
bert@xxxxxxxx
