Re: [PATCH v3 14/19] nvmem: microchip-otpc: Expose UID registers as 2nd nvmem device

[Alexander Dahl <ada@thorsis.com>]

Hello Claudiu,

thanks for your feedback on this, see my remarks inline below.

Am Sat, Jan 31, 2026 at 06:11:56PM +0200 schrieb Claudiu Beznea:
> 
> 
> On 1/20/26 17:44, Alexander Dahl wrote:
> > For SAM9X60 the Product UID x Register containing the Unique Product ID
> > is part of the OTPC registers.
> 
> SAMA7G5 as well.
> 
> > We have everything at hand here to just
> > create a trivial nvmem device for those.
> 
> Please massage a bit the patch description to match https://www.kernel.org/doc/html/v6.13/process/submitting-patches.html#describe-your-changes
> 
> > 
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > ---
> > 
> > Notes:
> >      v3:
> >      - no changes
> >      v2:
> >      - Use dev_err_probe() for error reporting (thanks Claudiu)
> >      - Move required register definition over here from removed patch
> > 
> >   drivers/nvmem/microchip-otpc.c | 38 +++++++++++++++++++++++++++++++++-
> >   1 file changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> > index 34c0d7611f3e4..b35ed13b004bb 100644
> > --- a/drivers/nvmem/microchip-otpc.c
> > +++ b/drivers/nvmem/microchip-otpc.c
> > @@ -25,10 +25,14 @@
> >   #define MCHP_OTPC_HR			(0x20)
> >   #define MCHP_OTPC_HR_SIZE		GENMASK(15, 8)
> >   #define MCHP_OTPC_DR			(0x24)
> > +#define MCHP_OTPC_UID0R			(0x60)
> >   #define MCHP_OTPC_NAME			"mchp-otpc"
> >   #define MCHP_OTPC_SIZE			(11 * 1024)
> > +#define MCHP_OTPC_UID_NAME		"mchp-uid"
> > +#define MCHP_OTPC_UID_SIZE		16
> 
> I think you can drop these as they are used (individually) in a single place.

If you don't mind I would rather keep it here together with already
present similar definitions, instead of sprinkling new literals over
this c file.

> 
> > +
> >   /**
> >    * struct mchp_otpc - OTPC private data structure
> >    * @base: base address
> > @@ -230,6 +234,16 @@ static int mchp_otpc_init_packets_list(struct mchp_otpc *otpc, u32 *size)
> >   	return 0;
> >   }
> > +static int mchp_otpc_uid_read(void *priv, unsigned int offset,
> > +			      void *val, size_t bytes)
> > +{
> > +	struct mchp_otpc *otpc = priv;
> > +
> > +	memcpy_fromio(val, otpc->base + MCHP_OTPC_UID0R + offset, bytes);
> 
> Include <asm-generic/logic_io.h> for this one?

Yes, include what you use, I'd add `#include <linux/io.h>` then, seems
the usual way in drivers/nvmem.

> 
> > +
> > +	return 0;
> > +}
> > +
> >   static struct nvmem_config mchp_nvmem_config = {
> >   	.name = MCHP_OTPC_NAME,
> >   	.type = NVMEM_TYPE_OTP,
> > @@ -239,6 +253,15 @@ static struct nvmem_config mchp_nvmem_config = {
> >   	.reg_read = mchp_otpc_read,
> >   };
> > +static struct nvmem_config mchp_otpc_uid_nvmem_config = {
> > +	.name = MCHP_OTPC_UID_NAME,
> 
> Should the type be filled here?

In general I would say yes, but looking at `enum nvmem_type` none of
them seems to match.  IIRC I more or less copied the approach from
drivers/soc/atmel/sfr.c here.

Greets
Alex

> 
> 
> > +	.read_only = true,
> > +	.word_size = 4,
> > +	.stride = 4,
> > +	.size = MCHP_OTPC_UID_SIZE,
> > +	.reg_read = mchp_otpc_uid_read,
> > +};
> > +
> >   static int mchp_otpc_probe(struct platform_device *pdev)
> >   {
> >   	struct nvmem_device *nvmem;
> > @@ -269,8 +292,21 @@ static int mchp_otpc_probe(struct platform_device *pdev)
> >   	mchp_nvmem_config.size = size;
> >   	mchp_nvmem_config.priv = otpc;
> >   	nvmem = devm_nvmem_register(&pdev->dev, &mchp_nvmem_config);
> > +	if (IS_ERR(nvmem)) {
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(nvmem),
> > +				     "Error registering OTP as nvmem device\n");
> > +	}
> > -	return PTR_ERR_OR_ZERO(nvmem);
> > +	mchp_otpc_uid_nvmem_config.dev = otpc->dev;
> > +	mchp_otpc_uid_nvmem_config.priv = otpc;
> > +
> > +	nvmem = devm_nvmem_register(&pdev->dev, &mchp_otpc_uid_nvmem_config);
> > +	if (IS_ERR(nvmem)) {
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(nvmem),
> > +				     "Error registering UIDxR as nvmem device\n");
> > +	}
> > +
> > +	return 0;
> >   }
> >   static const struct of_device_id __maybe_unused mchp_otpc_ids[] = {
>