Re: [PATCH v7 3/7] mtd: spi-nor: sfdp: expose the SFDP as a read-only NVMEM device

[<Manikandan.M@microchip.com>]

Hi Michael,

On 9/30/26 4:22 PM, Manikandan.M@microchip.com wrote:
> Hi Michael,
> 
> Sorry for the late reply.
> 
> On 9/7/26 6:30 PM, Michael Walle wrote:
>> Hi,
>>
>> On Wed Aug 12, 2026 at 12:49 PM CEST, Manikandan Muralidharan wrote:
>>> The SPI NOR core already reads the SFDP tables during enumeration and
>>> caches them in nor->sfdp->dwords (see spi_nor_parse_sfdp()). Re-expose
>>> that cached data as a read-only NVMEM device, in on-flash byte order,
>>> rooted at the flash's SFDP child node (compatible "jedec,sfdp").
>>>
>>> This lets NVMEM cells reference any SFDP data: a fixed-layout for
>>> parameters at a known offset, or an nvmem-layout parser for vendor data
>>> whose location must be discovered at runtime. The device is only registered
>>> when an "sfdp" node is present in the device tree.
>>>
>>> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
>>> ---
>>>    drivers/mtd/spi-nor/core.c | 78 ++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 78 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index ccf4396cdcd0..0425af6e898f 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -17,6 +17,7 @@
>>>    #include <linux/mtd/mtd.h>
>>>    #include <linux/mtd/spi-nor.h>
>>>    #include <linux/mutex.h>
>>> +#include <linux/nvmem-provider.h>
>>>    #include <linux/of.h>
>>>    #include <linux/regulator/consumer.h>
>>>    #include <linux/sched/task_stack.h>
>>> @@ -3001,6 +3002,75 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
>>>    		nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
>>>    }
>>>    
>>> +static int spi_nor_sfdp_reg_read(void *priv, unsigned int offset,
>>> +				 void *val, size_t bytes)
>>> +{
>>> +	struct spi_nor *nor = priv;
>>> +	struct sfdp *sfdp = nor->sfdp;
>>> +	size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
>>> +
>>> +	if (offset >= sfdp_size || bytes > sfdp_size - offset)
>>> +		return -EINVAL;
>>> +
>>> +	/* The cached SFDP is kept in on-flash (little-endian) byte order. */
>>> +	memcpy(val, (u8 *)sfdp->dwords + offset, bytes);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * spi_nor_register_sfdp_nvmem() - expose the SFDP as a read-only NVMEM device
>>> + * @nor:	pointer to a 'struct spi_nor'
>>> + *
>>> + * Expose the whole SFDP, in on-flash byte order, as a read-only NVMEM device
>>> + * rooted at the flash's SFDP child node (compatible "jedec,sfdp"). This lets
>>> + * generic (fixed-layout) or vendor (nvmem-layout) cells reference any SFDP
>>> + * data. The device is only registered when a child node with the "jedec,sfdp"
>>> + * compatible is described in the device tree.
>>> + *
>>> + * Return: 0 on success or if there is nothing to do, -errno otherwise.
>>> + */
>>> +static int spi_nor_register_sfdp_nvmem(struct spi_nor *nor)
>>> +{
>>> +	struct device *dev = nor->dev;
>>> +	struct nvmem_config config = { };
>>> +	struct nvmem_device *nvmem;
>>> +	struct device_node *np;
>>> +
>>> +	if (!nor->sfdp)
>>> +		return 0;
>>> +
>>> +	np = of_get_compatible_child(dev_of_node(dev), "jedec,sfdp");
>>> +	if (!np)
>>> +		return 0;
>>
>> What if there is no jedec,sfdp node? Should this be added anyways?
>>
if there is no jedec,sfdp node, the /sys/bus/nvmem/devices/sfdp* entry 
will not appear.
Removing this check will silently registering the SFDP NVMEM device on 
every SPI-NOR flash by default.
Do you suggest removing it ?

>>> +
>>> +	config.dev = dev;
>>> +	config.of_node = np;
>>
>> What about just:
>> 	config.of_node = of_get_compatible_child(dev_of_node(dev), "jedec,sfdp");
>>
> will do that, Thanks
>>> +	config.name = "sfdp";
>>> +	config.id = NVMEM_DEVID_AUTO;
>>> +	config.owner = THIS_MODULE;
>>> +	config.read_only = true;
>>> +	config.word_size = 1;
>>> +	config.stride = 1;
>>> +	config.size = (int)(nor->sfdp->num_dwords * sizeof(*nor->sfdp->dwords));
>>> +	config.reg_read = spi_nor_sfdp_reg_read;
>>> +	config.priv = nor;
>>> +
>>> +	nvmem = devm_nvmem_register(dev, &config);
>>> +	of_node_put(np);
>>
>> This now leaves the nvmem device with a dangling pointer. What I
>> meant in my previous reply, was that you'll add an of_node_get() to
>> the nvmem_register(), so the nvmem subsystem will have its own
>> tracking.
>>
> And Sorry, I overlooked that — I moved the of_node_put() instead of
> adding the of_node_get() to nvmem_register().
> I'll fold it into v8 and send it shortly — let me know if you'd rather
> see the core patch posted separately first.
>> -michael
>>
>>> +	if (IS_ERR(nvmem)) {
>>> +		/* NVMEM support is optional. */
>>> +		if (PTR_ERR(nvmem) == -EOPNOTSUPP)
>>> +			return 0;
>>> +		return dev_err_probe(dev, PTR_ERR(nvmem),
>>> +				     "failed to register SFDP NVMEM device\n");
>>> +	}
>>> +
>>> +	dev_dbg(dev, "exposed %d-byte SFDP as an NVMEM device\n", config.size);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    /**
>>>     * spi_nor_late_init_params() - Late initialization of default flash parameters.
>>>     * @nor:	pointer to a 'struct spi_nor'
>>> @@ -3204,6 +3274,14 @@ static int spi_nor_init_params(struct spi_nor *nor)
>>>    		spi_nor_init_params_deprecated(nor);
>>>    	}
>>>    
>>> +	/*
>>> +	 * Expose the SFDP table as an NVMEM device only when
>>> +	 * the flash actually provides one
>>> +	 */
>>> +	ret = spi_nor_register_sfdp_nvmem(nor);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>    	ret = spi_nor_late_init_params(nor);
>>>    	if (ret)
>>>    		return ret;
>>
> 
> 


-- 
Thanks and Regards,
Manikandan M.