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

[Lokesh Vutla]

Hi Lubomir,
On Thursday 30 May 2013 07:56 PM, Lubomir Popov wrote:
> 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.
Ok ll add the comment.
>
>> +	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.
Yes, Ill rebase this patch on top of your patch and use those defines.
>
>> +
>> +	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.
Yes, I see a similar update in your patch for LDO9. ll do the same for 
LDO1 also.

Thanks
Lokesh
>
>> +
>> +	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
>