Re: [PATCH 2/2 RFC] eeprom: at24: Tidy at24_read()

[Jean Delvare <jdelvare@suse.de>]

Hi Bartosz,

On Tue, 18 Aug 2020 15:46:36 +0200, Bartosz Golaszewski wrote:
> On Fri, Aug 7, 2020 at 4:19 PM Jean Delvare <jdelvare@suse.de> wrote:
> > The elegant code in at24_read() has the drawback that we now need
> > to make a copy of all parameters to pass them to the post-processing
> > callback function if there is one. Rewrite the loop in such a way that
> > the parameters are not modified, so saving them is no longer needed.
> >
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > This has the drawback of creating an asymetry with at24_write(), so
> > I'm not 100% if we want to apply this. If anyone has a better idea,
> > please let me know.
> >
> >  drivers/misc/eeprom/at24.c |   13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > --- linux-5.7.orig/drivers/misc/eeprom/at24.c   2020-08-07 14:23:39.882191500 +0200
> > +++ linux-5.7/drivers/misc/eeprom/at24.c        2020-08-07 14:28:39.039360687 +0200
> > (...)
> > @@ -449,15 +446,15 @@ static int at24_read(void *priv, unsigne
> >          */
> >         mutex_lock(&at24->lock);
> >
> > +       i = 0;  
> 
> Hi Jean,
> 
> I think doing:
> 
>     for (i = 0; count; i += ret, count -= ret)
> 
> would be even more elegant, don't you think?

Definitely. I'll change the code that way and send v2 of the patch,
thank you for the suggestion.

> Bartosz
> 
> >         while (count) {
> > -               ret = at24_regmap_read(at24, buf, off, count);
> > +               ret = at24_regmap_read(at24, buf + i, off + i, count);
> >                 if (ret < 0) {
> >                         mutex_unlock(&at24->lock);
> >                         pm_runtime_put(dev);
> >                         return ret;
> >                 }
> > -               buf += ret;
> > -               off += ret;
> > +               i += ret;
> >                 count -= ret;
> >         }
> >

-- 
Jean Delvare
SUSE L3 Support