sob., 27 lip 2019 o 17:50 Jean Delvare <jdelvare@xxxxxxx> napisał(a): > > Hi Bartosz, > > Thanks for your swift review. > > On Sat, 27 Jul 2019 13:15:03 +0200, Bartosz Golaszewski wrote: > > pt., 26 lip 2019 o 15:18 Jean Delvare <jdelvare@xxxxxxx> napisał(a): > > > --- linux-5.1.orig/drivers/misc/eeprom/at24.c 2019-05-06 02:42:58.000000000 +0200 > > > +++ linux-5.1/drivers/misc/eeprom/at24.c 2019-07-26 13:56:37.612197390 +0200 > > > @@ -719,7 +719,7 @@ static int at24_probe(struct i2c_client > > > nvmem_config.name = dev_name(dev); > > > nvmem_config.dev = dev; > > > nvmem_config.read_only = !writable; > > > - nvmem_config.root_only = true; > > > + nvmem_config.root_only = !(flags & AT24_FLAG_IRUGO); > > > > I have a preference for code as readable as possible. Please make it > > something like: root_only = flags & AT24_FLAG_IRUGO ? false : true;. > > I think this is the first time someone asks me to use the ternary > operator in the name of readability :-D > As I said - it's personal preference: figuring out the outcome of !(flags & AT24_FLAG_IRUGO) took me a couple seconds longer than flags & AT24_FLAG_IRUGO ? false : true. > FWIW the !(flags & FLAG) construct is very popular among kernel > developers, with over 8500 occurrences in the kernel tree, and I > personally find it more readable. As a matter of fact, the very at24 > driver already uses that construct a few lines before I do: > > writable = !(flags & AT24_FLAG_READONLY); You're right. In that case let's keep the code consistent. Don't change that. > > which is why I did the same. I tend to think that consistency matters > when it comes to code readability. > > That being said, you are maintaining the at24 driver, so I'll do as you > prefer. > > > Also: AFAICT these changes can easily be split into two separate > > patches, which would make pushing them upstream easier as at24 and > > nvmem go through different branches. > > OK, makes sense. There is no dependency so they can take their own > route to upstream and land there in whatever order. However the bug > won't be fixed until both are committed. > Sure. Bart > Thanks, > -- > Jean Delvare > SUSE L3 Support
