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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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