[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-i2c
Subject:    Re: [PATCH] eeprom: at24: make spd world-readable again
From:       Bartosz Golaszewski <brgl () bgdev ! pl>
Date:       2019-07-28 10:36:45
Message-ID: CAMRc=Me8HqgfxwUh+Z7=2L-tbwX3gLZ=eFS=sMg4FToY732eGQ () mail ! gmail ! com
[Download RAW message or body]

sob., 27 lip 2019 o 17:50 Jean Delvare <jdelvare@suse.de> 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@suse.de> 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
[prev in list] [next in list] [prev in thread] [next in thread]