Hi Heiko, On 2026-10-04T04:06:44, Heiko Schocher <[email protected]> wrote: > mtd: add driver for the Microchip 23LCV1024 SPI SRAM > > The 23LCV1024 is a 1 Mbit serial SRAM with battery backup, sitting on > an SPI bus and used as non volatile scratch storage. Register it as an > MTD device of type MTD_RAM, so the mtd command reaches it. > > The device powers up in byte operation, where every command carries > exactly one data byte. Select sequential operation once during probe,
Just to check, is this right? My understanding is that the 23LC1024/23LCV1024 data sheet gives sequential mode (01) as the MODE register default; byte mode is the default only on the older 23K256. The Linux mchp23k256 driver never writes the MODE register. Setting it explicitly is still a fine defensive step, since something else could have changed the mode and the part keeps it while on battery. Please can you reword the commit message and the comment in spi_ram_probe() to match? > so that a read or a write of any length is served by a single command > and the bus is claimed once per transfer. > > Writing needs no erase. The driver therefore implements no erase > operation and says MTD_NO_ERASE in the MTD flags. > > Signed-off-by: Stefan Bigler <[email protected]> > Signed-off-by: Heiko Schocher <[email protected]> > > MAINTAINERS | 5 ++ > configs/sandbox_defconfig | 1 + > drivers/mtd/Kconfig | 14 +++ > drivers/mtd/Makefile | 1 + > drivers/mtd/spi_ram.c | 222 > ++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 243 insertions(+) > diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig > @@ -194,6 +194,20 @@ config ALTERA_QSPI > +config SPI_RAM > + bool "Microchip 23LCV1024 SPI SRAM" > + depends on DM_SPI && DM_MTD SPI_RAM, spi_ram.c and the spi_ram driver name are very generic for a driver that handles one Microchip part. Linux already supports this chip in drivers/mtd/devices/mchp23k256.c (CONFIG_MTD_MCHP23K256), and the binding in dts/upstream/Bindings/mtd/microchip,mchp23k256.yaml lists both microchip,mchp23k256 and microchip,mchp23lcv1024. How about using the Linux names, here and in MAINTAINERS? > diff --git a/drivers/mtd/spi_ram.c b/drivers/mtd/spi_ram.c > @@ -0,0 +1,222 @@ > +static int spi_ram_xfer(struct spi_slave *spi, const u8 *cmd, size_t cmd_len, > + const void *data_out, void *data_in, size_t data_len) > +{ > + unsigned long flags = SPI_XFER_BEGIN; > + int ret; > + > + if (!data_len) > + flags |= SPI_XFER_END; This duplicates spi_write_then_read() in drivers/spi/spi-uclass.c, which has the same flag handling and debug messages. You could drop this function and use that one instead? > diff --git a/drivers/mtd/spi_ram.c b/drivers/mtd/spi_ram.c > @@ -0,0 +1,222 @@ > + ret = spi_claim_bus(spi); > + if (ret) { > + dev_dbg(dev, "failed to claim SPI bus: %d\n", ret); > + return ret; > + } > + > + ret = spi_ram_xfer(spi, cmd, sizeof(cmd), data_out, data_in, len); A read or write of the whole 128KB goes out as a single spi_xfer(). This fails on controllers that only implement spi-mem ops or that limit the transfer size, where the SPI NAND driver you are modelling this on would work. Using spi_mem_exec_op() with spi_mem_adjust_op_size() in a loop would cover both cases. I'm not sure how much that matters for your boards, so I'm just mentioning it. In any case: Reviewed-by: Simon Glass <[email protected]> Regards, Simon
