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 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); 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. Thanks, -- Jean Delvare SUSE L3 Support
