On Wed, Jul 8, 2020 at 1:54 AM Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > > On 7/7/20 8:47 PM, Havard Skinnemoen wrote: > > + value = tswap32(nc->disabled_modules); > > + npcm7xx_otp_array_write(&s->fuse_array, &value, 64, sizeof(value)); > > What is magic offset 64 for?
Good point. I'll add some definitions based on https://github.com/Nuvoton-Israel/bootblock/blob/master/Src/fuse_wrapper/fuse_wrapper.h#L23 > > + /* Preserve read-only and write-one-to-clear bits */ > > + value = > > + (value & ~FST_RO_MASK) | (s->regs[NPCM7XX_OTP_FST] & > > FST_RO_MASK); > > Trivial to review as: > > value &= ~FST_RO_MASK; > value |= s->regs[NPCM7XX_OTP_FST] & FST_RO_MASK; You're right, will do. > > +/* Each OTP module holds 8192 bits of one-time programmable storage */ > > +#define NPCM7XX_OTP_ARRAY_BITS (8192) > > +#define NPCM7XX_OTP_ARRAY_BYTES (NPCM7XX_OTP_ARRAY_BITS / 8) > > You could replace 8 by BITS_PER_BYTE. Will do. > > +typedef struct NPCM7xxOTPClass { > > + SysBusDeviceClass parent; > > + > > + const MemoryRegionOps *mmio_ops; > > +} NPCM7xxOTPClass; > > + > > +#define NPCM7XX_OTP_CLASS(klass) \ > > + OBJECT_CLASS_CHECK(NPCM7xxOTPClass, (klass), TYPE_NPCM7XX_OTP) > > +#define NPCM7XX_OTP_GET_CLASS(obj) \ > > + OBJECT_GET_CLASS(NPCM7xxOTPClass, (obj), TYPE_NPCM7XX_OTP) > > If nothing outside of the model implementation requires accessing > the class fields (which is certainly the case here, no code out of > npcm7xx_otp.c should access mmio_ops directly), then I recommend > to keep the class definitions local to the single source file where > it is used. This also makes this header simpler to look at. Good idea. Will do. > Very high code quality, so I just made nitpicking comments. > Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org> Thank you. I'll incorporate your feedback and send out a v5 series shortly. Havard
