Hi Martin,
On Thu, 24 Apr 2025 at 06:43, Martin Schwan <m.sch...@phytec.de> wrote:
>
> Hi Simon,
>
> thanks for the review and sorry for my delayed reply.
>
> On Mon, 2025-04-14 at 05:32 -0600, Simon Glass wrote:
> > Hi Martin,
> >
> > On Wed, 29 Jan 2025 at 07:25, Martin Schwan <m.sch...@phytec.de>
> > wrote:
> > >
> > > Add a bootmeth driver which supports booting A/B system with RAUC
> > > as
> > > their update client.
> > >
> > > Signed-off-by: Martin Schwan <m.sch...@phytec.de>
> > > ---
> > > boot/Kconfig | 11 ++
> > > boot/Makefile | 1 +
> > > boot/bootmeth_rauc.c | 408
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 420 insertions(+)
> >
> > This looks fine to me. Just some nits below.
> >
[..]
> > > +
> > > + partitions = env_get("rauc_partitions");
> > > + if (!partitions)
> > > + return log_msg_ret("env", -ENOENT);
> > > + partitions_list = str_to_list(partitions);
> > > +
> > > + slots = env_get("rauc_slots");
> > > + if (!slots)
> > > + return log_msg_ret("env", -ENOENT);
> > > + slots_list = str_to_list(slots);
> > > +
> > > + boot_order = env_get("BOOT_ORDER");
> >
> > Environment variables should be in lower case.
>
> Unfortunately, we are bound to the naming RAUC uses for these
> environment variables, which is upper case. See
> https://rauc.readthedocs.io/en/latest/integration.html#set-up-u-boot-boot-script-for-rauc
Can't you change it, since you are introducing new support here?
If not, then you could perhaps set the env variables to lowercase when
you find uppercase variables? I suppose that would require updating
the client to do the same thing too?
It would be very confusing for a user to have upper-case variables for
some things.
>
> >
> > > + if (!boot_order) {
> > > + if (env_set("BOOT_ORDER", slots))
> > > + return log_msg_ret("env", -EPERM);
> > > + }
> > > + priv->boot_order = strdup(boot_order);
> > > +
> > > + default_tries = env_get_ulong("rauc_slot_default_tries",
> > > 10, 3);
> > > + for (i = 0; slots_list[i] != NULL; i++) {
> > > + sprintf(boot_left, "BOOT_%s_LEFT", slots_list[i]);
> >
> > same here
[..]
Regards,
Simon
