Re: [PATCH 05/13] nvmem: add mt6323 PMIC EFUSE driver

Andy Shevchenko
2026-05-05T08:00:05Z

On Mon, May 04, 2026 at 09:24:57PM +0300, Roman Vivchar via B4 Relay wrote:

> Add support for the EFUSE controller found in the Mediatek MT6323 PMIC.
> The MT6323 EFUSE stores 24 bytes of hardware-related data, such as
> thermal sensor calibration values.

Besides below comments, check for the similar issues that previous patches in
the series have.

...

> +#include <linux/device.h>

Not needed as platform_device.h implies this ("is this good or bad?" is a
different story).

> +#include <linux/io.h>
> +#include <linux/mfd/mt6323/registers.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>

Follow IWYU. At least missing types.h.

...

> +struct mt6323_efuse {
> + struct regmap *regmap;
> +};

Do you really need a custom wrapper data structure? Can't regmap be used
directly?

...

> +static int mt6323_efuse_read(void *context, unsigned int offset, void *val,
> + size_t bytes)
> +{
> + struct mt6323_efuse *efuse = context;
> + u32 tmp;

> + u16 *buf = val;

Really? CPU order all the time?

> + int i, ret;

Why is 'i' signed?

> + for (i = 0; i < bytes; i += 2) {

sizeof()?

And since 'i' is used only inside the loop

for (size_t i = 0; i < bytes; i += sizeof(*buf)) {

> + ret = regmap_read(efuse->regmap,
> + MT6323_EFUSE_DOUT_BASE + offset + i, &tmp);
> + if (ret)
> + return ret;
> + buf[i / 2] = tmp;
> + }

+ blank line.

Isn't this reimplementation of bulk read? Why the latter may not be used?

> + return 0;
> +}

...

> +static const struct of_device_id mt6323_efuse_of_match[] = {
> + { .compatible = "mediatek,mt6323-efuse" },
> + { /* sentinel */ },

Having trailing comma in the terminator is nonsense. Do we expect anything
behind it?

> +};

--
With Best Regards,
Andy Shevchenko