* [U-Boot] [PATCH v3] mmc-uclass: correct the device number
@ 2016-07-19 13:28 ` Kever Yang
2016-07-21 2:04 ` Jaehoon Chung
2016-07-22 3:21 ` Simon Glass
0 siblings, 2 replies; 5+ messages in thread
From: Kever Yang @ 2016-07-19 13:28 UTC (permalink / raw)
To: u-boot
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);
}
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v3] mmc-uclass: correct the device number
2016-07-19 13:28 ` [U-Boot] [PATCH v3] mmc-uclass: correct the device number Kever Yang
@ 2016-07-21 2:04 ` Jaehoon Chung
2016-07-22 3:21 ` Simon Glass
1 sibling, 0 replies; 5+ messages in thread
From: Jaehoon Chung @ 2016-07-21 2:04 UTC (permalink / raw)
To: u-boot
Hi Kever,
On 07/19/2016 10:28 PM, Kever Yang 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().
Looks good to me. I had already sent the similar patch for this.
(http://patchwork.ozlabs.org/patch/643921/)
But I think this patch is better than mine.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
Tested-by: Jaehoon Chung <jh80.chung@samsung.com>
(On Exynos4 SoCs with eMMC/SD interface.)
Best Regards,
Jaehoon Chung
> ---
>
> 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);
> }
>
> 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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v3] mmc-uclass: correct the device number
2016-07-19 13:28 ` [U-Boot] [PATCH v3] mmc-uclass: correct the device number Kever Yang
2016-07-21 2:04 ` Jaehoon Chung
@ 2016-07-22 3:21 ` Simon Glass
2016-07-22 4:14 ` Jaehoon Chung
1 sibling, 1 reply; 5+ messages in thread
From: Simon Glass @ 2016-07-22 3:21 UTC (permalink / raw)
To: u-boot
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.
I realise that this may not matter in practice, but it is really
confusing the way you have it.
> }
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v3] mmc-uclass: correct the device number
2016-07-22 3:21 ` Simon Glass
@ 2016-07-22 4:14 ` Jaehoon Chung
2016-08-01 2:21 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Jaehoon Chung @ 2016-07-22 4:14 UTC (permalink / raw)
To: u-boot
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
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v3] mmc-uclass: correct the device number
2016-07-22 4:14 ` Jaehoon Chung
@ 2016-08-01 2:21 ` Simon Glass
0 siblings, 0 replies; 5+ messages in thread
From: Simon Glass @ 2016-08-01 2:21 UTC (permalink / raw)
To: u-boot
Hi Jaehoon,
On 21 July 2016 at 22:14, Jaehoon Chung <jh80.chung@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@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.
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-01 2:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20160719132909epcas1p11d39c6c29cb951947503ed580b1b61a0@epcas1p1.samsung.com>
2016-07-19 13:28 ` [U-Boot] [PATCH v3] mmc-uclass: correct the device number Kever Yang
2016-07-21 2:04 ` Jaehoon Chung
2016-07-22 3:21 ` Simon Glass
2016-07-22 4:14 ` Jaehoon Chung
2016-08-01 2:21 ` Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox