Hi Igor, thanks for the review.
On 11/01/14 12:52, Paul Kocialkowski wrote:
Boards using the TWL4030 regulator may not all use the LDOs the same way (e.g. MMC2 power can be controlled by another LDO than VMMC2). This delegates TWL4030 MMC power initializations to board-specific functions, that may still call twl4030_power_mmc_init for the default behavior.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
This mostly looks good, several suggestions below though.
board/comelit/dig297/dig297.c | 9 +++++++++ board/compulab/cm_t35/cm_t35.c | 11 +++++++++++ board/corscience/tricorder/tricorder.c | 11 +++++++++++ board/isee/igep00x0/igep00x0.c | 11 +++++++++++ board/logicpd/omap3som/omap3logic.c | 11 +++++++++++ board/logicpd/zoom1/zoom1.c | 9 +++++++++ board/matrix_vision/mvblx/mvblx.c | 9 +++++++++ board/nokia/rx51/rx51.c | 9 +++++++++ board/overo/overo.c | 11 +++++++++++ board/pandora/pandora.c | 9 +++++++++ board/technexion/tao3530/tao3530.c | 11 +++++++++++ board/ti/beagle/beagle.c | 11 +++++++++++ board/ti/evm/evm.c | 11 +++++++++++ board/ti/sdp3430/sdp.c | 9 +++++++++ board/timll/devkit8000/devkit8000.c | 11 +++++++++++ drivers/mmc/omap_hsmmc.c | 7 +------ 16 files changed, 154 insertions(+), 6 deletions(-)
diff --git a/board/comelit/dig297/dig297.c b/board/comelit/dig297/dig297.c index 2b826df..784483b 100644 --- a/board/comelit/dig297/dig297.c +++ b/board/comelit/dig297/dig297.c @@ -133,6 +133,15 @@ int board_mmc_init(bd_t *bis) { return omap_mmc_init(0, 0, 0, -1, -1); }
+int board_mmc_power_init(void) +{ +#if defined(CONFIG_TWL4030_POWER)
- twl4030_power_mmc_init();
- mdelay(100); /* ramp-up delay from Linux code */
I guess all twl4030 based boards have to have this ramp up delay, right? If so, why don't we want to hide it inside the twl4030_power_mmc_init() function and not spread it across all boards?
That makes perfect sense, thanks for the suggestion. Will do in the next version.
+#endif
- return 0;
+}
Also, what do you think of the below suggestion: Leave the twl4030_power_mmc_init() call inside the omap_hsmmc.c (as it seems that many boards want it) as it currently is. This way you will not need to patch each board file, yet a board has the ability to control that call via the CONFIG_TWL4030_POWER and have a board specific callback should it need one.
One of the initial intents of this patch set was also to remove the implication of CONFIG_TWL4030_POWER on twl4030_power_mmc_init, because my device (LG Optimus Black P970) uses TWL4030 regulators in a way that doesn't match other boards or any reference design (so I need CONFIG_TWL4030_POWER but not twl4030_power_mmc_init).
I would also change envelope the call to twl4030_power_mmc_init() to something like CONFIG_TWL4030_MMC_POWER instead of CONFIG_TWL4030_POWER, so it will be more flexible, but that has little to do with this patch.
That would be acceptable for me, but it would mean having two places where the same thing happens: the new board_mmc_power_init function and internally on omap_hsmmc. That's the sort of lack of consistance I definitely do not like, but if you think it's the best way to do it, I don't see any further issue with that.
Patching all the boards isn't really that big a deal (identifying the boards is already done now anyway) and it makes similar sense to having multiple board_mmc_init that all look very much alike.
In addition, perhaps I should add a dev_index parameter to twl4030_power_mmc_init (in a separate patch) so that only the relevant LDO is enabled (see my previous patch enabling VMMC2 too, that was accepted): most devices only use one MMC controller, so there is no need to enable all the MMC LDOs.
That would give some legitimacy to having one twl4030_power_mmc_init per board, since it would reflect whether to enable one or both MMC LDOs.
What do you think?
#endif
#ifdef CONFIG_CMD_NET
[...]
diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index ef2cbf9..6fb78b3 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -135,12 +135,7 @@ static unsigned char mmc_board_init(struct mmc *mmc) pbias_lite = readl(&t2_base->pbias_lite); pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0); writel(pbias_lite, &t2_base->pbias_lite); -#endif -#if defined(CONFIG_TWL4030_POWER)
- twl4030_power_mmc_init();
- mdelay(100); /* ramp-up delay from Linux code */
-#endif -#if defined(CONFIG_OMAP34XX)
- writel(pbias_lite | PBIASLITEPWRDNZ1 | PBIASSPEEDCTRL0 | PBIASLITEPWRDNZ0, &t2_base->pbias_lite);