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

[Simon Glass]

Hi Jaehoon,

On 21 July 2016 at 22:14, Jaehoon Chung <jh80.chung at samsung.com> wrote:
> 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 at 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 at 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.

Yes it is confusing, I agree. My point is that -ENODEV is actually an
error. Still, returning 0 is OK. for get_mmc_num() I suppose. But we
cannot just add 1. Adding 1 to -ENODEV does not yield 0.

>
> 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,

I allows the mmc driver to be probed from device tree. It was the
first stage of the conversion.

>
> 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.

Yes that is exactly right. I have been trying to push things towards
case 3, as per my recently applied changes. But more work is needed.

And in fact there is a 4th case - DM_MMC_OPS. That is where we really
need to end up. There is quite a bit of code to convert and I could
use more help!

>
> Best Regards,
> Jaehoon Chung
>
>>

Regards,
Simon