Re: [PATCH v8 5/8] nvmem: layouts: add Microchip/SST SFDP EUI layout driver

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi,

On 14/10/2026 at 12:10:09 +0530, Manikandan Muralidharan <manikandan.m@microchip.com> wrote:

> Add an NVMEM layout that exposes the factory-programmed EUI-48 identifier
> from the Microchip/SST vendor SFDP parameter table (e.g. SST26VF064BEUI)
> as a "mac-address" cell, for use as a network MAC address. The vendor
> table is located at runtime via the SFDP NVMEM device (no offset in DT),
> and a read_post_process callback reverses the LSB-first bytes into
> canonical MAC order. Binds to an "nvmem-layout" node with compatible
> "microchip,sst26vf-sfdp-eui".
>
> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>

Mostly minor nits, see below.

> --- a/drivers/nvmem/layouts/Makefile
> +++ b/drivers/nvmem/layouts/Makefile
> @@ -6,4 +6,5 @@
>  obj-$(CONFIG_NVMEM_LAYOUTS) += fixed-layout.o
>  obj-$(CONFIG_NVMEM_LAYOUT_SL28_VPD) += sl28vpd.o
>  obj-$(CONFIG_NVMEM_LAYOUT_ONIE_TLV) += onie-tlv.o
> +obj-$(CONFIG_NVMEM_LAYOUT_SST26VF_SFDP_EUI) += sst26vf-sfdp-eui.o
>  obj-$(CONFIG_NVMEM_LAYOUT_U_BOOT_ENV) += u-boot-env.o

Not sure this is ordered alphabetically, so in general add it at the end.

> +/* SFDP header and parameter header, as laid out on the flash. */
> +struct sfdp_header {
> +	u8 signature[4];
> +	u8 minor;
> +	u8 major;
> +	u8 nph;
> +	u8 unused;
> +};
> +
> +struct sfdp_parameter_header {
> +	u8 id_lsb;
> +	u8 minor;
> +	u8 major;
> +	u8 length;
> +	u8 parameter_table_pointer[3];
> +	u8 id_msb;
> +};
> +
> +#define SFDP_SIGNATURE			0x50444653U
> +
> +#define SFDP_PARAM_HEADER_ID(h)		(((h)->id_msb << 8) | (h)->id_lsb)
> +#define SFDP_PARAM_HEADER_PTP(h)	get_unaligned_le24((h)->parameter_table_pointer)
> +
> +/* Microchip (vendor) parameter table identifier: id_msb << 8 | id_lsb. */

This is not relevant, we saw it just above         ^^^^^^^^^^^^^^^^^^^^^^^

> +#define SFDP_MCHP_VENDOR_ID		0x01bf
> +
> +#define SFDP_MCHP_EUI48_MARKER_OFFSET	0x60
> +#define SFDP_MCHP_EUI48_MARKER		0x30
> +#define SFDP_MCHP_EUI48_OFFSET		0x61
> +
> +static int sfdp_eui_read_post_process(void *priv, const char *id, int index,
> +				      unsigned int offset, void *buf,
> +				      size_t bytes)
> +{
> +	u8 *data = buf;
> +	int i;
> +
I believe the if against the length is wrong and should be inverted and
placed here:

        if (bytes != ETH_ALEN)
                return -EINVAL;

Since lenght does not change, prefer to bail out early.

> +	/* SFDP stores the address least-significant octet first; reverse it. */
> +	for (i = 0; i < bytes / 2; i++)
> +		swap(data[i], data[bytes - 1 - i]);
> +
> +	if (bytes == ETH_ALEN && !is_valid_ether_addr(buf))

Is bytes != ETH_ALEN a valid use case?

> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int sfdp_eui_find_vendor_table(struct nvmem_device *nvmem, u32 *ptp)
> +{
> +	struct sfdp_parameter_header ph;
> +	struct sfdp_header hdr;
> +	int nph, i, ret;
> +
> +	ret = nvmem_device_read(nvmem, 0, sizeof(hdr), &hdr);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (get_unaligned_le32(hdr.signature) != SFDP_SIGNATURE)
> +		return -EINVAL;
> +
> +	/* The number of parameter headers (NPH) field is zero-based. */
> +	nph = hdr.nph;

nph = hdr.nph + 1 ?

> +
> +	for (i = 0; i <= nph; i++) {

and here the usual i < nph?

> +		ret = nvmem_device_read(nvmem, sizeof(hdr) + i * sizeof(ph),
> +					sizeof(ph), &ph);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (SFDP_PARAM_HEADER_ID(&ph) != SFDP_MCHP_VENDOR_ID)
> +			continue;
> +
> +		*ptp = SFDP_PARAM_HEADER_PTP(&ph);
> +		return 0;
> +	}
> +
> +	return -ENOENT;
> +}

...

> +	info.name = "mac-address";
> +	info.offset = base + SFDP_MCHP_EUI48_OFFSET;
> +	info.bytes = ETH_ALEN;
> +	info.np = of_get_child_by_name(layout_np, "mac-address");
> +	info.read_post_process = sfdp_eui_read_post_process;
> +
> +	ret = nvmem_add_one_cell(nvmem, &info);
> +	if (ret)
> +		of_node_put(info.np);
> +	else
> +		dev_dbg(dev, "exposed EUI-48 at SFDP offset 0x%x\n", info.offset);

This one is not really useful anymore.

> +
> +	of_node_put(layout_np);
> +
> +	return ret;
> +}
> +

Thanks,
Miquèl