On Tue, 8 Sep 2020 08:51:11 +0200, Wolfram Sang wrote: > On Thu, Sep 03, 2020 at 11:00:54AM +0200, Jean Delvare wrote: > > Short writes used "daddress" for the value, but the masking code did > > not expect that, and instead applied the mask to a variable that was > > never used. > > > > So change short writes to use "value" for the value, as all other > > commands do. Adjust all code paths accordingly. > > > > Reported by David Jedynak. > > > > Signed-off-by: Jean Delvare <jdelvare@xxxxxxx> > > Reviewed-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> > Tested-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> > > Passed my test as well. One question: Thank you very much for testing! I'll commit that now. > > @@ -140,7 +140,7 @@ static int confirm(const char *filename, > > } else > > fprintf(stderr, "data 0x%02x%s, mode %s.\n", value, > > vmask ? " (masked)" : "", > > - size == I2C_SMBUS_BYTE_DATA ? "byte" : "word"); > > + size == I2C_SMBUS_WORD_DATA ? "word" : "byte"); > > Does this really change something or is it a refactoring leftover? It does change something. Before the changes, size could only have two values here, I2C_SMBUS_BYTE_DATA and I2C_SMBUS_WORD_DATA, so the result would indeed be the same. But after the changes, size can also have value I2C_SMBUS_BYTE, for which we want to display "mode byte", not "mode word". Which is why we test explicitly for I2C_SMBUS_WORD_DATA, else default to "byte", rather than the other way around. Note that this also aligns nicely with the rest of the code in this file, which does the same in various places. Funny story, while I only posted this last week, I wrote the fix several months ago, so last week I actually got to review my own code with fresh eyes, and when I stumbled upon that specific change, first thing that came to my mind was "this is a useless change, why did I do that ?" Then I scrolled up, checked the other changes in that function, noticed the removed "else" and said OK, I'm not that stupid after all ;-) -- Jean Delvare SUSE L3 Support
