[U-Boot] [PATCH V2 1/2] cmd_mmc: eliminate device num in the mmc command

[Andy Fleming]

On Fri, Apr 15, 2011 at 9:17 PM, Lei Wen <leiwen at marvell.com> wrote:
> From now on, follow the general rule "mmc dev [dev]" to change the
> mmc command applied device, like ide and usb...
>
> Signed-off-by: Lei Wen <leiwen at marvell.com>
> ---
> Changelog:
> V2: use the "mmc_cur_dev" to replace explicitly specify the dev num
>    in mmc command.


I'd really prefer if there were still the option to specify the device
in the read/write commands. I appreciate the convenience of not having
to specify it every time, but I feel that by doing things this way, we
create an artificial separation between a transaction, and the target
of the transaction. It tends to encourage a notion that a transaction
can only be done to the global-current device, which is fine for
command-line interfaces, but can result in broken programming
interfaces



>  int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
> -       int rc = 0;
> +       if (argc < 2)
> +               return cmd_usage(cmdtp);
>
> -       switch (argc) {
> -       case 3:
> -               if (strcmp(argv[1], "rescan") == 0) {
> -                       int dev = simple_strtoul(argv[2], NULL, 10);
> -                       struct mmc *mmc = find_mmc_device(dev);
> +       if (strcmp(argv[1], "rescan") == 0) {
> +               struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>
> -                       if (!mmc)
> -                               return 1;
> +               if (!mmc)
> +                       return 1;
> +
> +               mmc_init(mmc);
>
> -                       mmc_init(mmc);
> +               return 0;
> +       } else if (strncmp(argv[1], "part", 4) == 0) {
> +               block_dev_desc_t *mmc_dev;
> +               struct mmc *mmc = find_mmc_device(mmc_cur_dev);
>
> +               if (!mmc) {
> +                       puts("no mmc devices available\n");

Technically, this just means that no device is selected. I'm sure that
would only happen if no devices were available, but that's an
assumption carried outside of the vicinity of this code. Someone might
change that assumption (for instance, if something could cause an mmc
device to be removed)

> +                       return 1;
> +               }
> +               mmc_init(mmc);
> +               mmc_dev = mmc_get_dev(mmc_cur_dev);
> +               if (mmc_dev != NULL &&
> +                               mmc_dev->type != DEV_TYPE_UNKNOWN) {
> +                       print_part(mmc_dev);

[...]

> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 6805b33..377f13a 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -34,7 +34,7 @@
>  #include <div64.h>
>
>  static struct list_head mmc_devices;
> -static int cur_dev_num = -1;


Please don't remove this, or conflate this value with the global
"current" pointer. This is used to assign the block dev num.
mmc_cur_dev is a runtime value that will change.  If, for some reason,
someone needs to interact with an mmc device before all of them have
come up, we will utterly break.  This is the sort of reason that I'm
not big on this sort of interaction paradigm.


> +int mmc_cur_dev = -1;


You should make this a pointer to the mmc device, not an integer.
Also, you should keep it in the cmd_mmc file.  The *only* place this
should be used is to declare what the current device for user
interaction is. Board code that wants to interact with MMC devices
should feel free to do so, irrespective of the user (ie, the user's
desired card should not affect which card the board code accesses, and
the board code should not cause the user's selection to change).


>
>  int __board_mmc_getcd(u8 *cd, struct mmc *mmc) {
>        return -1;
> @@ -849,7 +849,7 @@ int mmc_register(struct mmc *mmc)
>  {
>        /* Setup the universal parts of the block interface just once */
>        mmc->block_dev.if_type = IF_TYPE_MMC;
> -       mmc->block_dev.dev = cur_dev_num++;
> +       mmc->block_dev.dev = mmc_cur_dev++;

Make sure to switch this back

>        mmc->block_dev.removable = 1;
>        mmc->block_dev.block_read = mmc_bread;
>        mmc->block_dev.block_write = mmc_bwrite;
> @@ -936,12 +936,20 @@ void print_mmc_devices(char separator)
>
>  int mmc_initialize(bd_t *bis)
>  {
> +       int ret = -1;
>        INIT_LIST_HEAD (&mmc_devices);
> -       cur_dev_num = 0;
> +       mmc_cur_dev = 0;
>
>        if (board_mmc_init(bis) < 0)
> -               cpu_mmc_init(bis);
> +               ret = cpu_mmc_init(bis);
> +       else
> +               ret = 0;
>
> +       /* Put the device 0 as the default */
> +       if (ret < 0)
> +               mmc_cur_dev = -1;
> +       else
> +               mmc_cur_dev = 0;

These seems needlessly awkward. I suggest that we can initialize it
either to zero, or to the last register device, inside mmc_register().
You don't need "ret" at all, then. Because this should be in cmd_mmc,
we would add a function to cmd_mmc, like: mmc_set_current_dev(struct
mmc *)


>        print_mmc_devices(',');
>
>        return 0;
> diff --git a/include/mmc.h b/include/mmc.h
> index fcd0fd1..dd9bf15 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -295,4 +295,7 @@ int atmel_mci_init(void *regs);
>  int mmc_legacy_init(int verbose);
>  #endif
>
> +#ifdef CONFIG_GENERIC_MMC
> +extern int mmc_cur_dev;
> +#endif

And then this isn't necessary, but you can declare mmc_set_current_dev(), here

Andy