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

Roman Vivchar
2026-05-05T16:24:55Z

On Tuesday, May 5th, 2026 at 11:00 AM, Andy Shevchenko <andriy.shevchenko@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