Hi Simon, Thank you for the detailed review of v4 — genuinely useful. I'll address the points in v5. The hard-coded 16-bit address in particular sent this somewhere better, and chasing your comment on the non-standard 'size' and 'pagesize' properties turned up the atmel,at25 binding already in tree, which now drives the whole geometry.
Thanks, João On Mon, Jul 13, 2026 03:04 PM, Simon Glass <[email protected]> wrote: > Hi João, > > On 2026-06-15T17:51:16, João Loureiro <[email protected]> wrote: > > spi: Introduce initial EEPROM driver mode support > > > > This patch introduces the initial SPI EEPROM driver mode support > > analogous to the I2C EEPROM driver mode support. The SPI EEPROM > > driver mode support is enabled by default in the sandbox_defconfig. > > > > Signed-off-by: João Loureiro <[email protected]> > > > > MAINTAINERS | 6 ++ > > configs/sandbox_defconfig | 1 + > > drivers/misc/Kconfig | 6 ++ > > drivers/misc/Makefile | 1 + > > drivers/misc/spi_eeprom.c | 185 ++++++++++++++++++++++++++++++ > ++++++++++++++++ > > include/dm/uclass-id.h | 1 + > > include/spi_eeprom.h | 91 +++++++++++++++++++++++ > > 7 files changed, 291 insertions(+) > > Sorry I didn't notice the earlier versions of this patch, but I do > have a few comments. > > > This patch introduces the initial SPI EEPROM driver mode support > > analogous to the I2C EEPROM driver mode support. The SPI EEPROM > > driver mode support is enabled by default in the sandbox_defconfig. > > 'driver model', not 'driver mode' - three times. Please also use the > imperative: 'Introduce initial driver-model support for SPI EEPROMs, > analogous to ...' matches the subject and U-Boot convention. > > Please explain the motivation briefly (why now, what will consume it) > before your current commit msg. > > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > @@ -567,6 +567,12 @@ config I2C_EEPROM > > +config SPI_EEPROM > > + bool "SPI EEPROM support" > > + depends on MISC > > + help > > + Enable support for SPI EEPROM devices. > > + Currently with only read access. > > Please expand the help text - mention that this provides a uclass with > a small AT25-style driver, that only 16-bit-addressed parts are > handled, and that only read is implemented. > > Does this really need 'depends on MISC'? The driver does not use the > misc uclass; it sits on its own UCLASS_SPI_EEPROM. i2c_eeprom does not > depend on MISC either. > > > diff --git a/drivers/misc/spi_eeprom.c b/drivers/misc/spi_eeprom.c > > @@ -0,0 +1,185 @@ > > +/* 1 command byte followed by a 16-bit address */ > > +#define SPI_EEPROM_CMD_SIZE 3 > > This lives in the public header and hard-codes a 16-bit address, but > the uclass is presented as generic. Anything larger than 64 KiB > (AT25M02, AT25512, most 24-bit-addressed parts) will not work. Please > either: > > - move SPI_EEPROM_CMD_SIZE into spi_eeprom.c and derive the address > length from driver-data (an addr_len field), so growing the compatible > list later does not need an ABI change; or > - state explicitly in the commit message and Kconfig help that > initial support is limited to 16-bit-addressed AT25-style parts. > > I'd prefer the first. > > > + if (offset < 0 || size < 0 || offset + size > priv->size) { > > + log_err("Read out of bounds (offset %d, size %d, max > %lu)\n", > > + offset, size, priv->size); > > + return -EINVAL; > > + } > > offset + size is an int and can overflow before the comparison against > the unsigned long priv->size. Please check the two ranges > independently: > > if (offset < 0 || size < 0 || offset >= priv->size || > size > priv->size - offset) > > > + cmd[0] = data->cmd_read_data; > > + cmd[1] = (offset >> 8) & 0xff; > > + cmd[2] = offset & 0xff; > > If you keep the 16-bit assumption, please reject offsets that don't > fit in 16 bits rather than silently truncating. priv->size can be > overridden from DT via the 'size' property with any value. > > > +static int spi_eeprom_std_of_to_plat(struct udevice *dev) > > +{ > > + const struct spi_eeprom_drv_data *data = > > + (const struct spi_eeprom_drv_data > *)dev_get_driver_data(dev); > > + struct spi_eeprom *priv = dev_get_priv(dev); > > + > > + priv->size = dev_read_u32_default(dev, 'size', data->size); > > + priv->pagesize = dev_read_u32_default(dev, 'pagesize', > data->pagesize); > > Two things: > > 'size' and 'pagesize' don't seem to be standard bindings for SPI > EEPROM nodes - the size is normally determined by the compatible. > Please either drop these overrides or point at a binding that > documents them. > > Also, this writes into priv from of_to_plat(). Per driver-model > convention, of_to_plat() should populate plat data (auto_plat) and > probe() should use plat or can put a few things in priv if it likes. > Since there is no write path using pagesize yet, the simpler fix is to > move this into spi_eeprom_std_probe() > > > +static const struct udevice_id spi_eeprom_std_ids[] = { > > + { .compatible = 'microchip,at25160bn', .data = > (ulong)&atmel25_data }, > > + { } > > +}; > > Only one compatible for a generic driver feels thin - any reason not > to add the rest of the AT25 family that shares this command set > (at25010b, at25020b, at25040b, at25080b, at2532b, at25640b etc)? Each > just needs a drv_data entry. > > > diff --git a/include/spi_eeprom.h b/include/spi_eeprom.h > > @@ -0,0 +1,91 @@ > > +/* AT25-style command set */ > > +#define AT25_CMD_READ_DATA 0x03 /* Read data from memory array */ > > +#define AT25_CMD_READ_STATUS 0x05 /* Read status register */ > > + > > +/* 1 command byte followed by a 16-bit address */ > > +#define SPI_EEPROM_CMD_SIZE 3 > > These are only used by drivers/misc/spi_eeprom.c and > drivers/misc/spi_eeprom_emul.c - implementation details rather than > uclass API. Please move them into a private header shared by the > driver and emulator, or duplicate the two opcodes locally. > > > +#if CONFIG_IS_ENABLED(SPI_EEPROM) > > +/* > > + * spi_eeprom_read() - read bytes from an SPI EEPROM chip > > Please switch to kerneldoc (/** rather than /*) - new code should > carry proper kerneldoc even where i2c_eeprom.h has not been updated. > > > +#endif /* SPI_EEPROM */ > > + > > +#endif > > Please add /* __SPI_EEPROM */ after the outer #endif to match the inner > one. > > Regards, > Simon >
