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

[Andy Fleming]

On Wed, Apr 27, 2011 at 10:44 PM, Lei Wen <adrian.wenl at gmail.com> wrote:

>>> -                       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)
>
> If that device be removed, I think the following mmc_init could check it out,
> wouldn't it? For this check, my understanding is to see whether that slot has
> been register or not, then following init would double check device whether
> function.

Well, here I'm talking about some piece of code *programmatically*
removing a registered device. Right now, we don't support that, and I
don't suspect we will anytime soon, but my point was that the error
message should state what the actual problem was.  At best, this code
can *guess* that no mmc devices are available, but it doesn't know
that.  It only knows that it didn't find mmc_cur_dev. Also, by telling
the user that you didn't find mmc_cur_dev, the user can try to figure
out whether mmc_cur_dev got corrupted, or if he accidentally set it to
the wrong value.


>
>>
>>> +                       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).
>
> I agree with your concern.
> The main reason I mmc_cur_dev global is for cmd_mmc cannot get the info
> of how many slots has been enabled mmc.c
>
> I think make a function in mmc.c like mmc_get_num() which would also fulfill
> my need.


I'm not that concerned about where the variable sits. If it's in
mmc.c, that's fine.  But it must not be used for any purpose other
than to record what device was last selected for use by the mmc
commands. Other clients (such as early boot code) should have no sense
of that state.


Andy