[U-Boot] [PATCH V2 09/12] mmc: omap_hsmmc: add mmc1 pbias, ldo1

[Lubomir Popov]

Hi Lokesh,

On 30/05/13 16:19, Lokesh Vutla wrote:
> From: Balaji T K <balajitk at ti.com>
> 
> add dra mmc pbias support and ldo1 power on
> 
> Signed-off-by: Balaji T K <balajitk at ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
> ---
>  arch/arm/include/asm/arch-omap5/omap.h |    3 ++-
>  drivers/mmc/omap_hsmmc.c               |   26 ++++++++++++++------------
>  drivers/power/palmas.c                 |   25 ++++++++++++++++++++++++-
>  include/configs/omap5_common.h         |    4 ++++
>  include/configs/omap5_uevm.h           |    5 -----
>  include/palmas.h                       |    6 +++++-
>  6 files changed, 49 insertions(+), 20 deletions(-)
> 
[snip]
>  
> diff --git a/drivers/power/palmas.c b/drivers/power/palmas.c
> index 09c832d..1bcff52 100644
> --- a/drivers/power/palmas.c
> +++ b/drivers/power/palmas.c
> @@ -28,7 +28,7 @@ void palmas_init_settings(void)
>  	return;
>  }
>  
> -int palmas_mmc1_poweron_ldo(void)
> +int palmas_mmc1_poweron_ldo9(void)
>  {
>  	u8 val = 0;
>  
> @@ -50,3 +50,26 @@ int palmas_mmc1_poweron_ldo(void)
>  
>  	return 0;
>  }
> +
> +int palmas_mmc1_poweron_ldo1(void)
> +{
> +	u8 val = 0;
> +
> +	/* set LDO9 TWL6035 to 3V */
LDO9? TWL6035? If this function is used on the DRA7xx boards only (with
TPS659038), you should add some comment above.

> +	val = 0x2b; /* (3 - 0.9) * 20 + 1 */
Why not use definitions for the voltage? You could take them from
http://patchwork.ozlabs.org/patch/244103/ where some values are
defined.

> +
> +	if (palmas_i2c_write_u8(TPS659038_CHIP_ADDR, LDO1_VOLTAGE, val)) {
> +		printf("tps659038: could not set LDO1 voltage\n");
> +		return 1;
> +	}
> +
> +	/* TURN ON LDO9 */
LDO9?

> +	val = LDO_ON | LDO_MODE_SLEEP | LDO_MODE_ACTIVE;
Bit LDO_ON in all LDOx_CTRL Palmas registers is Read-Only (and reflects the
current status of the LDO). While it makes no harm to try writing to it, this
may be misleading about actual LDO operation, and anyway has no sense.

> +
> +	if (palmas_i2c_write_u8(TPS659038_CHIP_ADDR, LDO1_CTRL, val)) {
> +		printf("tps659038: could not turn on LDO1\n");
> +		return 1;
> +	}
> +
[snip]
>  /* I2C chip addresses */
>  #define PALMAS_CHIP_ADDR	0x48
> +#define TPS659038_CHIP_ADDR	0x58
Now we have a mess again. The files were recently renamed from twl6035.x
to palmas.x, implying that palmas is the generic family name of a series
of PMICs. Having TPS659038_CHIP_ADDR above is OK, but then we should have
TWL603X_CHIP_ADDR instead of PALMAS_CHIP_ADDR.

Best regards,
Lubomir