[U-Boot] [PATCH v3] mmc-uclass: correct the device number

[Jaehoon Chung <jh80.chung@samsung.com>]

Hi Simon,

On 07/22/2016 12:21 PM, Simon Glass wrote:
> Hi Kever,
> 
> On 19 July 2016 at 07:28, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Not like the mmc-legacy which the devnum starts from 1, it starts from 0
>> in mmc-uclass, so the device number should be (devnum + 1) in get_mmc_num().
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>> Changes in v3:
>> - apply comments from Jaehoon Chung
>>
>> Changes in v2:
>> - add comment for get_mmc_num() in mmc.h
>> - update mmc_get_next_devnum()
>>
>>  drivers/mmc/mmc-uclass.c | 4 ++--
>>  include/mmc.h            | 6 ++++++
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
>> index 38ced41..d0ca91b 100644
>> --- a/drivers/mmc/mmc-uclass.c
>> +++ b/drivers/mmc/mmc-uclass.c
>> @@ -111,7 +111,7 @@ struct mmc *find_mmc_device(int dev_num)
>>
>>  int get_mmc_num(void)
>>  {
>> -       return max(blk_find_max_devnum(IF_TYPE_MMC), 0);
>> +       return max((blk_find_max_devnum(IF_TYPE_MMC) + 1), 0);
> 
> Sorry to be pendantic, but the problem is that this
> blk_find_max_devnum() can return -ENODEV. You change it to 0 in this
> case, which is correct for get_mmc_num(), but not for
> mmc_get_next_devnum(). I think you should adjust the latter to call
> blk_find_max_devnum() directly, so it can return an error if there is
> one.

You're right, blk_find_max_devnum() can be return -ENODEV.
But get_mmc_num() is returned max(-ENODEV, 0), then it should be always returned 0, if there is no device.
0 means no devices, doesn't?
(get_mmc_num() never returned the error number.)
Well, i didn't find that case until now..Is there case that return -ENODEV from mmc_get_num()?

And mmc_get_next_devnum() is called in mmc_legacy.c.
I didn't find anywhere called mmc_get_next_devnum() in mmc_uclass.c

> 
> I realise that this may not matter in practice, but it is really
> confusing the way you have it.

Hmm, I'm confusing a lot for MMC DM.
It seems that there are three cases..

1. Use the legacy.
- It's just using the existing model.

2. Use DM_MMC and legacy.
- I don't understand why use the combination of DM_MMC and legacy.
- When i see the u-boot-dm repository,

ifdef CONFIG_DM_MMC
obj-$(CONFIG_GENERIC_MMC) += mmc-uclass.o
endif

ifndef CONFIG_BLK
obj-$(CONFIG_GENERIC_MMC) += mmc_legacy.o
endif

It should be conflicted too many things..

3. Use DM_MMC and BLK
- I think this is our best way.

Right? It might be my misunderstanding.
Even if i shouldn't misunderstand something, i want to help you on MMC and block side.
So i will go ahead for fixing and cleaning.

Best Regards,
Jaehoon Chung

> 
>>  }
>>
>>  int mmc_get_next_devnum(void)
>> @@ -122,7 +122,7 @@ int mmc_get_next_devnum(void)
>>         if (ret < 0)
>>                 return ret;
>>
>> -       return ret + 1;
>> +       return ret;
>>  }
>>
>>  struct blk_desc *mmc_get_blk_desc(struct mmc *mmc)
>> diff --git a/include/mmc.h b/include/mmc.h
>> index 8f309f1..dd47f34 100644
>> --- a/include/mmc.h
>> +++ b/include/mmc.h
>> @@ -503,6 +503,12 @@ void mmc_set_clock(struct mmc *mmc, uint clock);
>>  struct mmc *find_mmc_device(int dev_num);
>>  int mmc_set_dev(int dev_num);
>>  void print_mmc_devices(char separator);
>> +
>> +/**
>> + * get_mmc_num() - get the total MMC device number
>> + *
>> + * @return 0 if there is no MMC device, else the number of devices
>> + */
>>  int get_mmc_num(void);
>>  int mmc_hwpart_config(struct mmc *mmc, const struct mmc_hwpart_conf *conf,
>>                       enum mmc_hwpart_conf_mode mode);
>> --
>> 1.9.1
>>
>>
> 
> Regards,
> Simon
> 
> 
>