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

[Lokesh Vutla <lokeshvutla@ti.com>]

Hi Lubomir,,
On Tuesday 04 June 2013 01:28 AM, Lubomir Popov wrote:
> Hi Lokesh,
>
>> 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@ti.com>
>>>>
>>>> add dra mmc pbias support and ldo1 power on
>>>>
>>>> Signed-off-by: Balaji T K <balajitk@ti.com>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@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]
>>>> +	/* 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.
> Please be aware that my above mentioned patch has not been reviewed/
> tested/acked/nacked/whatever by nobody (except possibly a quick look by
> Nishanth Menon, who had some objections). I wrote it when bringing up a
> custom OMAP5 board, and most probably it shall not go into mainline in
> its current form, if ever. I gave it only as an example of how things
> could be done cleaner. Feel free to use the code as you wish, but I'm
> afraid that applying it as a patch to your tree and basing upon it might
> run you into problems when you later sync with mainline.
Ahh sorry, I was in a dilemma whether to ask this or not. Since it is 
posted I assumed
that the patch ll get merged. I have already posted a patch on top of 
your patch.
Ill wait for Tom to comment.
>
> Tom, your opinion?
>
>>>
>>>> +
>>>> +	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.
> But are you sure that the TPS659038 has the same LDOx_CTRL register layout
> as the TWL6035/37? It belongs to the family, yes, but I don't have a
> Register Manual for this chip... Hope you have checked.
Yes, TPS659038 has same LDOx_CTRL register layout.

Thanks,
Lokesh
>>
>> Thanks
>> Lokesh
>>>
> [snip]
>
> Best regards,
> Lubo
>