Messages in this thread |  | | Date | Tue, 5 May 2026 10:59:57 +0300 | | From | Andy Shevchenko <> | | Subject | Re: [PATCH 05/13] nvmem: add mt6323 PMIC EFUSE driver |
| |
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
|  |