[PATCH v4 2/5] nvmem: Add the Raspberry Pi OTP driver

[Gregor Herburger]

On Tue, May 19, 2026 at 11:14:28AM +0200, Thomas Weißschuh wrote:
> On Fri, May 08, 2026 at 04:42:45PM +0200, Gregor Herburger wrote:
> > +config NVMEM_RASPBERRYPI_OTP
> > +	tristate "Raspberry Pi OTP support"
> > +	depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> 
> The '&& !RASPBERRYPI_FIRMWARE' clause looks weird, is it really necessary?

Yes it does looks weird but I think it is necessary. Without this it would be
possible to build RASPBERRYPI_FIRMWARE=m and NVMEM_RASPBERRYPI_OTP=y which
results in linker errors.
> 
> > +	help
> > +	  This driver provides access to the Raspberry Pi OTP memory via the
> > +	  nvmem subsystem. The driver supports the customer OTP as well as the
> > +	  device specific private key OTP (BCM2712 only).
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called raspberrypi-otp.
> 
> While we are here: The empty line here is now missing.

Oh. Thanks will add it again.
> 
> > +++ b/drivers/nvmem/raspberrypi-otp.c
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include <linux/overflow.h>
> 
> This looks unused.
> 
> Instead maybe add linux/types.h, linux/align.h
> 
Thanks will drop it and add the suggested headers.

> > +struct rpi_otp_priv {
> > +	struct rpi_firmware *fw;
> > +	struct device *dev;
> 
> This looks unused.
> 
Thansk will remove.

> > +static int rpi_otp_read(void *context, unsigned int offset, void *buf, size_t bytes)
> > +{
> > +	struct rpi_otp_priv *priv = context;
> > +	struct rpi_otp_header *fwbuf;
> > +	u32 count;
> > +	int ret;
> > +
> > +	if (!IS_ALIGNED(offset, 4) || !IS_ALIGNED(bytes, 4))
> > +		return -EINVAL;
> 
> Isn't this already enforced by the nvmem core?

Only for sysfs access through bin_attr_nvmem_read/bin_attr_nvmem_write. But
there is an in-kernel API nvmem_device_read/nvmem_device_write which does not
have alignment checks. So I added the check to be more defensive here.

> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	data = dev_get_platdata(dev);
> > +	if (!data)
> > +		return -ENODEV;
> 
> I would do this before the devm_kzalloc().

Yes will change.

> > +module_platform_driver(raspberry_otp_driver);
> > +
> > +MODULE_AUTHOR("Gregor Herburger <gregor.herburger at linutronix.de>");
> > +MODULE_DESCRIPTION("Raspberry Pi OTP driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:raspberrypi-otp");
> 
> Instead of the manual module alias here and the implicit matching of the
> platform driver this should use an explicit matching table:
> 
> static const struct platform_device_id foo_id[] = {
> 	{ "raspberrypi-otp" },
> 	{}
> };
> MODULE_DEVICE_TABLE(platform, foo_id);
> 
> static struct platform_driver raspberry_otp_driver = {
> 	...
> 	.id_table = foo_id,
> };
> module_platform_driver(raspberry_otp_driver);
> 

Sounds right will change it.

Regards
Gregor