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

[Roman Vivchar]

On Tuesday, May 5th, 2026 at 11:00 AM, Andy Shevchenko <andriy.shevchenko at intel.com> wrote:

> 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?

Yes, but the PWRAP (the regmap which the driver uses) doesn't support read
callback, only reg_read is implemented in the driver.
It hits the map->cache_type == REGCACHE_NONE in the regmap_read_raw, and falls
for the !map->read check, so regmap_bulk_read neither the regmap_read_raw
can be used.

Technically the PMIC is not continuous address space, but rather non-MMIO FSM,
so it makes sense to not implement the read callback. I guess the custom
implementation is fine then?

All other suggestions will be applied for v2.

> 
> > +	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
> 
> 
>

Best regards,
Roman