From mboxrd@z Thu Jan  1 00:00:00 1970
From: Igor Grinberg <grinberg@compulab.co.il>
Date: Sun, 02 Nov 2014 15:40:00 +0200
Subject: [U-Boot] [PATCH v3 2/2] omap_hsmmc: Board-specific TWL4030 MMC
	power initializations
In-Reply-To: <1414839138-6587-3-git-send-email-contact@paulk.fr>
References: <1414839138-6587-1-git-send-email-contact@paulk.fr>
	<1414839138-6587-3-git-send-email-contact@paulk.fr>
Message-ID: <54563430.30309@compulab.co.il>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Paul,

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?

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

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.

>  #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);
> 

-- 
Regards,
Igor.