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

[Lubomir Popov]

Hi Tom,

On 05/06/13 16:45, Tom Rini wrote:
> On Wed, Jun 05, 2013 at 11:03:26AM +0300, Lubomir Popov wrote:
> 
>> Hi Tom,
>>
>> On 05/06/13 00:06, Tom Rini wrote:
>>> On Mon, Jun 03, 2013 at 10:58:27PM +0300, 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 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]
>>>>>>> +	/* 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.
>>>>
>>>> Tom, your opinion?
>>>
>>> OK, so at the time it was "nothing will really use this code except test
>>> functions".  Looks like we have a use for mmc1_ldo9 code at least, so
>>> lets rework the first patch for adding that + cleanups wrt constants.
>> Well, I'm not quite sure that this LDO9 function would be the only one
>> used (or LDO1 on the DRA7xx board). Judging from omapboot for the OMAP5
>> boards for example, SMPS7 (it delivers the common 1.8 V I/O supply) is
>> set to 'Forced PWM' mode in order to reduce board noise - there sure has
>> been a reason to do so and sacrifice converter efficiency. Therefore I
>> added similar functionality in my patch to the Palmas driver (and am
>> explicitly calling it in my board init).
>> The option to bypass LDO9 on OMAP5+TWL603x boards seems quite mandatory
>> as well, if hardware is designed such that the SD card socket has a
>> separate fixed 3.3 V supply which also powers the LDO9 input (the
>> uEVM for example). On the DRA7xx+TPS659038 board the power scheme is
>> different and this does not apply.
> 
> OK, lets see.  That so lets keep your patch as-is, since we've now got
> -ffunction-sections/-fdata-sections/--gc-sections on ARM for main
> U-Boot, these small things won't hurt like they used to.
> 
OK, but then I would like to do some cleanup first - remove the audio
power stuff (shall have it in my board file), as well as either sort out
the function naming:

- Those functions that are specific to a SoC+PMIC combination are
named e.g. twl603x_... or tps659038_... so that they explicitly
indicate the hardware that they are working with (actually almost all
functions are such). This is however sort of regression, and requires
fixes in the files calling these functions;

or, alternatively:

- Introduce generic functions with fixed names, palmas_bla_bla(),
sort of wrappers, which in their bodies perform the appropriate action
based on the #ifdefs defining the platform hardware (where we could also
define the particular LDO which for example a palmas_mmc1_poweron_ldo()
generic function would manipulate). Drawback: again #ifdefs.
Advantage: single place where this stuff is located, and where other
PMIC/LDO combinations can be added without affecting other code.
And this generic palmas_mmc1_poweron_ldo() function would be called
by another generic function, e.g. omap_sdmmc_poweron(), located in
the board file, only if needed by the particular hardware.
omap_sdmmc_poweron(), on its hand, is the function that is to be called
from within the pbias routines in omap_hsmmc.c, and not the hardware-
dependant functions directly. So we get the abstraction.

What do you think? Lokesh, your opinion?

Regards,
Lubo