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

[Lei Wen]

Hi Andy,

On Fri, Apr 29, 2011 at 3:57 PM, Andy Fleming <afleming at gmail.com> wrote:
> 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.

I have submit another round of patch, please review it whether it meet
your expectation.

Thanks,
Lei