Re: [PATCH V2] OMAP3: PM: Set/reset T2 bit for Smartreflex on TWL.

[Kevin Hilman <khilman@ti.com>]

Nishanth Menon <nm@ti.com> writes:

> Kevin Hilman wrote, on 02/03/2011 03:09 AM:
>> Nishanth Menon<nm@ti.com>  writes:
>>
>>> Kevin Hilman wrote, on 02/02/2011 04:11 AM:
>>>> Shweta Gulati<shweta.gulati@ti.com>   writes:
>>>>
>>>>> From: Thara Gopinath<thara@ti.com>
>>>>>
>>>>> The smartreflex bit on twl4030 needs to be enabled by default irrespective
>>>>> of whether smartreflex module is enabled on the OMAP side or not.
>>>>> This is because without this bit enabled the voltage scaling through
>>>>> vp forceupdate does not function properly on OMAP3.API added
>>>>> 'omap3_twl_set_sr_bit' with parameter to set/clear SR bit. It is cleared
>>>>> for platforms where voltage is not scaled using vpforceupdate
>>>>> or vc_bypass Method. In those cases 'omap3_twl_set_sr_bit' is called
>>>>> from board file, to make sure this bit is not overwritten in
>>>>> 'omap3_twl_init', a flag 'twl_sr_enable'
>>>>> is added.
>>>>
>>>> As Sanjeev pointed out, the use of 'irrespective' above is confusing, in
>>>> fact the whole changelog is kind of confusing.
>>>>
>>>> The changelog states that it has to always be enabled, but then goes on
>>>> to describe the situation(s) where it would be disabled.
>>>>
>>>> Here's my rephrasing of how I understand the above changelog
>>>>
>>>> - enable: *always* be enabled
>>>> - enable: needed for VP force update
>>>> - disable: platforms using VP forced update or VP bypass
>>>>
>>>> -ECONFUSED
>>>>
>>>> Kevin
>>>
>>> How about this as the commit log?
>>>
>>> The smartreflex bit on twl4030 specifies if the setting of voltage
>>> is done over the I2C_SR path. Given that there are platforms that
>>> do not use I2C_SR path for voltage scaling, a new function
>>> 'omap3_twl_set_sr_bit' with parameter to set/clear SR bit has been
>>> provided for flexibility.
>>
>> So far so good.
>>
>>> It is called with appropriate param
>>> for platforms where voltage is not scaled using I2C_SR path
>>> from board file, to make sure this bit is not overwritten in
>>> 'omap3_twl_init'.
>>
>> -ENOPARSE
> k, How about this:
> Voltage control on TWL can be done using VMODE/I2C1/I2C_SR. Since
> almost all platforms use I2C_SR on omap3, omap3_twl_init by default
> defaults expects that OMAP's I2C_SR is plugged in to TWL's I2C and
> calls omap3_twl_set_sr_bit. On platforms where I2C_SR is not
> connected, the board files are expected to call
> omap3_twl_set_sr_bit(false) to ensure that I2C_SR path is not set for
> voltage control and prevent the default behavior of omap3_twl_init.

Nice, thanks.

>>
>>>
>>>
>>>>
>>>>> This patch is based on LO PM Branch and Smartreflex has been
>>>>> tested on OMAP3430 SDP, OMAP3630 SDP and boot tested on
>>>>> OMAP2430 SDP.
>>>
>>> this belongs into the diffstat.
>>>
>>> Attached is a modified version of this patch - i'vent tested it
>>> though.. but basically improves the logic a little:
>>>
>>> *) made the comments more generic to ensure that this is more of
>>> I2C_SR path as far as TWL is concerned(yes, from OMAP perspective it
>>> is vp forceupdate/bypass), but it is more of an OMAP problem than
>>> omap_twl.c problem.
>>> *) modified the function call sequences to prevent rentry even if
>>> board file calls with various other params
>>> *) shifted to using bool
>>> *) use init and initdata to free up the space once we are done with
>>> init sequence
>>
>> All good changes, but I don't think they're incorporated in V3.
>
> could you be more clear inline on v3?

Will look closer at v4.

Kevin