[U-Boot] [PATCH 03/14] env: Pass additional parameters to the env lookup function

[Maxime Ripard]

Hi Simon,

On Thu, Dec 28, 2017 at 08:13:23PM -0700, Simon Glass wrote:
> > -static struct env_driver *env_driver_lookup(void)
> > +static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
> 
> Can you please document the function args? The operation of prio needs
> to be described somewhere.

Will do.

> >  {
> > -       enum env_location loc = env_get_location();
> > +       enum env_location loc = env_get_location(op, prio);
> >         struct env_driver *drv;
> >
> > +       if (loc == ENVL_UNKNOWN)
> > +               return NULL;
> 
> Why is this needed? Shouldn't _env_driver_lookup() return NULL in this
> case anyway?

I just thought that making it obvious and explicit (especially when
the behaviour of _env_driver_lookup might change) would be a good
thing, but I can remove it if you want.

> > -       if (!drv)
> > -               return -ENODEV;
> > -       if (!drv->save)
> > -               return -ENOSYS;
> > -
> > -       printf("Saving Environment to %s...\n", drv->name);
> > -       ret = drv->save();
> > -       if (ret) {
> > -               debug("%s: Environment failed to save (err=%d)\n", __func__,
> > -                     ret);
> > -               return ret;
> > +       for (prio = 0; (drv = env_driver_lookup(ENVO_SAVE, prio)); prio++) {
> > +               int ret;
> > +
> > +               if (!drv->save)
> > +                       continue;
> > +
> > +               printf("Saving Environment to %s...\n", drv->name);
> > +               ret = drv->save();
> > +               if (!ret)
> > +                       return 0;
> 
> So we get no advice of error? I think it should print a message here,
> like ...failed or ...done.

This is addressed in the next patch

> Also if no driver succeeded I suggest returning one of the errors
> returned from drv-save().

And this was one of the comments made in this version, so it's going
to be addressed in the next iteration.

> > --- a/include/environment.h
> > +++ b/include/environment.h
> > @@ -215,6 +215,13 @@ enum env_location {
> >         ENVL_UNKNOWN,
> >  };
> >
> 
> comment here
> 
> > +enum env_operation {
> > +       ENVO_GET_CHAR,
> 
> I think ENVOP would be a better prefix.

That works for me. Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180105/bceecf4a/attachment.sig>