[U-Boot] [PATCH 3/6] sandbox: Implement host dev [device]

[Simon Glass]

HI Sjoerd,

On 28 February 2015 at 07:00, Sjoerd Simons
<sjoerd.simons at collabora.co.uk> wrote:
> On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
>> Hi Sjoerd,
>>
>> On 19 February 2015 at 15:41, Sjoerd Simons
>> <sjoerd.simons at collabora.co.uk> wrote:
>> > A common pattern to check if a certain device exists (e.g. in
>> > config_distro_bootcmd) is to use: <interface> dev [device]
>> >
>> > Implement host dev [device] so this pattern can be used for sandbox host
>> > devices.
>>
>> I don't see where this actually affects anything? In other words, does
>> it really use the device you select, or just ignore you?
>
> It gets ignored, it's only real usage is to detect whether a device
> exists.
>
> To step back the reason I implemented it here is just to make it simpler
> to integrate with the command boot commands as it's common blockdevice
> macro is:
>
> #define BOOTENV_SHARED_BLKDEV_BODY(devtypel) \
>         "if " #devtypel " dev ${devnum}; then " \
>                 "setenv devtype " #devtypel "; " \
>                 "run scan_dev_for_boot_part; " \
>         "fi\0"
>
> Which follows exactly that pattern.

I see.

>
>> > Signed-off-by: Sjoerd Simons <sjoerd.simons at collabora.co.uk>
>> > ---
>> >  common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 48 insertions(+)
>> >
>> > diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c
>> > index 4286969..7447d43 100644
>> > --- a/common/cmd_sandbox.c
>> > +++ b/common/cmd_sandbox.c
>> > @@ -10,6 +10,8 @@
>> >  #include <sandboxblockdev.h>
>
>> > +       ret = host_get_dev_err(dev, &blk_dev);
>> > +       if (ret) {
>> > +               if (ret == -ENOENT)
>> > +                       puts("Not bound to a backing file\n");
>>
>> Just use printf(), we should avoid puts() now.
>
> ok
>
>> > +               else if (ret == -ENODEV)
>> > +                       puts("Invalid host device number\n");
>> > +
>> > +               return 1;
>> > +       }
>> > +
>> > +       sandbox_curr_device = dev;
>> > +       return 0;
>> > +}
>> > +
>> > +U_BOOT_CMD(
>> > +       host, 3, 2, do_host_dev,
>> > +       "Set or get current host device", "dev [dev] - Set or retrieve the current host device"
>>
>> Can we make this command part of the 'sb' command? Like 'sh host ...'.
>
> In principle, sure, however that breaks the consistency with other
> commands which all use <interface> <interface-commands>, where
> <interface> matches the names used in the fs and partition commands.
> Which is exactly the consistency _distro_bootcmd takes advantage of in
> the macro i mentioned above.
>
> My thinking was that the sb set of commands are used to manage/setup the
> sandbox interface (e.g. setting up the host device bindings) while in
> this case the host commands are for the more standard interaction with
> host interface devices.
>
>
> Another way of addressing this and essentially side-stepping this
> discussion would be to add a a new generic dev command to allow testing
> device existance e.g.
>   dev exists host 0
>   dev exists mmc 0
>
> And switch _distro_bootcmd to that instead of it relying on all block
> interfaces having a dev subcommand with the same semantics.
>
> Opinions ? :)

How about we change the existing 'sb' command to 'host'? Arguably 'sb'
is a bit of a silly name. We could maintain an alias for a while.

Regards,
Simon