Hi Tom,
On Wed, 15 Nov 2023 at 19:07, Tom Rini <tr...@konsulko.com> wrote:
>
> On Wed, Nov 15, 2023 at 06:56:33PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 15 Nov 2023 at 18:47, Tom Rini <tr...@konsulko.com> wrote:
> > >
> > > On Wed, Nov 15, 2023 at 06:42:19PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 15 Nov 2023 at 15:38, Tom Rini <tr...@konsulko.com> wrote:
> > > > >
> > > > > On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote:
> > > > >
> > > > > > Rather than passing it all the command-line args, pass in the pieces
> > > > > > that it needs. These are the image address, the ramdisk address/name
> > > > > > and the FDT address/name.
> > > > > >
> > > > > > Ultimately this will allow usage of this function without being
> > > > > > called
> > > > > > from the command line.
> > > > >
> > > > > OK, so this goal is good.
> > > > >
> > > > > [snip]
> > > > > > + return bootm_find_images(img_addr, argc > 1 ? argv[1]
> > > > > > : NULL,
> > > > > > + argc > 2 ? argv[2] : NULL,
> > > > > > 0, 0);
> > > > >
> > > > > That we repeat this much harder to read test/if/else three times now
> > > > > is
> > > > > less good. Can we find some way to hide the complexity here in the
> > > > > case
> > > > > where it's coming from a command and so we have argc/argv[] ?
> > > >
> > > > I can't really think of one. Ultimately this is coming from the fact
> > > > that the booti and bootz commands directly call bootm_find_images(). I
> > > > haven't got far enough to know whether that will still be true in the
> > > > end, but I hope not.
> > > >
> > > > IMO the correct place for the logic above is in the command-processing
> > > > code, where it decides which arg means what.
> > > >
> > > > I could imagine something like:
> > > >
> > > > static const char *cmd_get_arg(int argc, char *const argv[], int argnum)
> > > > {
> > > > return argc > argnum ? argv[argnum] : NULL;
> > > > }
> > > >
> > > > but I'm not sure that is an improvement.
> > >
> > > I was thinking about this more after I sent this. And I think we might
> > > indeed want an inline / macro to handle this case more generally as I
> > > suspect we'll have similar cases where we need argN-or-NULL as we
> > > refactor other areas of code to split "here is the command" from "here
> > > is the library functionality" of it. Then we'll have:
> > > ulong foo = cmd_arg_one(argc, argv);
> > > ulong bar = cmd_arg_two(argc, argv);
> > >
> > > librarycall(foo, bar, ...);
> >
> > OK I can try something. But note that the one, two terminology becomes
> > confusing. Is the first argument zero or one? Also we often drop an
> > argument in a subcommand to allow things to start from 0 again.
> >
> > It might be better to use first and second, instead?
>
> Yes, please come up with a better naming scheme, I'm terrible about
> stuff like that.
Actually my naming has problems too...hopefully this has been solved
properly elsewhere and someone will reply to my patch.
Regards,
Simon
