* [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