On 10 December 2014 at 22:26, Greg Bellows <greg.bell...@linaro.org> wrote: > > > On 9 December 2014 at 13:46, Peter Maydell <peter.mayd...@linaro.org> wrote: >> +static bool raw_accessors_valid(const ARMCPRegInfo *ri) >> +{ >> + /* Return true if a raw access on this register is OK (ie will not >> + * fall into the assert in raw_read() or raw_write()) >> + */ > > > I believe this comment is somewhat misleading as there are registers that > would return true from this function yet still hit the aforementioned > asserts.
Really? I think it is misleading (really it will return false if a raw access is definitely not valid, but may return true even if a raw access is still a bad idea), but I don't think there are any cases that would return true and then hit the assert. >> >> + if (ri->type & ARM_CP_CONST) { >> + return true; >> + } > > > I realize CONST registers would not likely go through the raw functions but > these too make the above comment incorrect. No, read_raw_cp_reg and write_raw_cp_reg both handle CONST, so it's correct to return true here. >> + return (ri->raw_writefn || ri->writefn || ri->fieldoffset) && >> + (ri->raw_readfn || ri->readfn || ri->fieldoffset); >> >> +} >> + > > > Unless we are going to use this function elsewhere, wouldn't it be better to > just inline the code? The name is otherwise misleading and the comment may > cause this to be incorrectly used elsewhere in the future. Maybe instead > just update the call location with something like: > > if (!(r2->type & (ARM_CP_NO_RAW | ARM_CP_CONST)) { > assert((ri->raw_writefn || ri->writefn || ri->fieldoffset) && > (ri->raw_readfn || ri->readfn || ri->fieldoffset); > } I wanted to keep the logic checking for validity next to the logic that it matches in read/write_raw_cp_reg(). Otherwise the two are a long way apart in the file and it's not obvious why we check what we do. (I guess it wasn't obvious anyway, so the comment needs work.) -- PMM