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

Maxime Ripard
2018-01-05T09:27:59Z

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>