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 <[email protected]>");
> > +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
