[PATCH v4 1/2] spi: Introduce initial EEPROM driver mode support

[Simon Glass]

Hi João,

On 2026-06-15T17:51:16, João Loureiro <joaofl at gmail.com> 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 <joaofl at gmail.com>
>
> 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