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

[<Manikandan.M@microchip.com>]

Hi Miquel,

Thanks for the review — all valid, I will fix in v9.

On 10/14/26 2:22 PM, Miquel Raynal wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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


-- 
Thanks and Regards,
Manikandan M.