eeprom: at24: make spd world-readable again

The integration of the at24 driver into the nvmem framework broke the
world-readability of spd EEPROMs. Fix it.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Fixes: 57d155506dd5 ("eeprom: at24: extend driver to plug into the NVMEM framework")
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 drivers/misc/eeprom/at24.c |    2 +-
 drivers/nvmem/core.c       |   15 +++++++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

pt., 26 lip 2019 o 15:18 Jean Delvare <jdelvare@suse.de> napisał(a):
>
> The integration of the at24 driver into the nvmem framework broke the
> world-readability of spd EEPROMs. Fix it.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Fixes: 57d155506dd5 ("eeprom: at24: extend driver to plug into the NVMEM framework")
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/misc/eeprom/at24.c |    2 +-
>  drivers/nvmem/core.c       |   15 +++++++++++----
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> --- 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);

Hi Jean,

I have a preference for code as readable as possible. Please make it
something like: root_only = flags & AT24_FLAG_IRUGO ? false : true;.

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.

Bart

>         nvmem_config.owner = THIS_MODULE;
>         nvmem_config.compat = true;
>         nvmem_config.base_dev = dev;
> --- linux-5.1.orig/drivers/nvmem/core.c 2019-07-23 19:30:27.630099103 +0200
> +++ linux-5.1/drivers/nvmem/core.c      2019-07-26 14:21:31.002908472 +0200
> @@ -435,10 +435,17 @@ static int nvmem_setup_compat(struct nvm
>         if (!config->base_dev)
>                 return -EINVAL;
>
> -       if (nvmem->read_only)
> -               nvmem->eeprom = bin_attr_ro_root_nvmem;
> -       else
> -               nvmem->eeprom = bin_attr_rw_root_nvmem;
> +       if (nvmem->read_only) {
> +               if (config->root_only)
> +                       nvmem->eeprom = bin_attr_ro_root_nvmem;
> +               else
> +                       nvmem->eeprom = bin_attr_ro_nvmem;
> +       } else {
> +               if (config->root_only)
> +                       nvmem->eeprom = bin_attr_rw_root_nvmem;
> +               else
> +                       nvmem->eeprom = bin_attr_rw_nvmem;
> +       }
>         nvmem->eeprom.attr.name = "eeprom";
>         nvmem->eeprom.size = nvmem->size;
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>
>
> --
> Jean Delvare
> SUSE L3 Support

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

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,

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