Re: [pm_wip/voltdm_nm][PATCH v2 2/3] OMAP4: PM: VC: introduce concept of default channel

["Menon, Nishanth" <nm@ti.com>]

On Thu, Jun 2, 2011 at 19:10, Kevin Hilman <khilman@ti.com> wrote:
> "Menon, Nishanth" <nm@ti.com> writes:
>
>> On Thu, Jun 2, 2011 at 18:45, Kevin Hilman <khilman@ti.com> wrote:
>>> Nishanth Menon <nm@ti.com> writes:
>>>
>>>> Due to a TRM bug, the current code assumes that channel0(core) is the default
>>>> channel. With the additional explanation provided by the hardware team, it is
>>>> clear that PRM_VC_CFG_CHANNEL register allows for flexibility of configuring
>>>> for various PMIC configurations. For example, the I2C slave address on TWL6030
>>>> when using 4430 configuration could potentially use the same slave address for
>>>> all domains, while in 4460 configuration, we drive MPU using TPS; Core and IVA
>>>> using TWL6030. To allow this flexibility, we state in existing parameters using
>>>> a flag to indicate usage of default channel.
>>>>
>>>> In addition, the TRM update clears up the confusion on the fact that MPU is
>>>> infact the default channel on OMAP4.
>>>>
>>>> We update the same here.
>>>>
>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>
>>> There's too much going on in this patch not described in the changelog
>>> (extra error checking, volt & cmd reg checking etc.)
>>>
>>> Also, I don't like the USE_DEFAULT_CHANNEL_I2C_PARAM, mainly because it
>>> adds clutter without any obvious value.  To me, zero is a perfectly good
>>> value to signify "use default channel value", especially since that's
>>> the value used by the hardware.
>> The reason is that 0 is a valid i2c register address. 8 bits is used
>> in VC and I wanted one bit which was further away, hence the 16 bit.
>> The usage could be that in .volt_reg_addr or in .cmd_reg_addr I could
>> set this up with the macro. and bingo, we use the default domain's
>> configuration.
>>
>> This is esp useful if we had a single pmic reg for all domains.. I
>> mean the VC design allows for it, even though we dont use it
>> currently.
>
> OK.
>
> See, it's easy to convince me that something is needed/useful.  Of
> course, I prefer to be conviced by the changelog. :)
>
> Please make this feature into a dedicated patch with an appropriately
> descriptive changelog.  Thanks.

Yes, I agree. it probably is a better idea to break this thing up - I
guess I was a bit lazyish considering these were targetted to be
squashed.. not in the next series :)
>
>>>
>>> Incidentally, adding this doesn't actually change current behavior.
>>> Currently, I use zero (an unconfigured value in the VC settings) to
>>> signify it should use default settings.  In your patch, you defined
>>> USE_DEFAULT_CHANNEL_I2C_PARAM as 0x10000 (17 bits) but assign it to 16
>>> bit fields, which means they are just set to zero. :)
>>
>>
>>> So rather than take this patch, I'm just going to fold the following
>>> diff into "OMAP3+: VC: abstract out channel configuration" in voltdm_b.
>>> I'll also update the changelog noting that the TRM is wrong here in that
>>> it describes CORE as the default channel when it's in fact MPU.
>>
>> I intend to post a new series including my PMIC changes as well early
>> next week(mondayish hopefully). Can we hold off review of any further
>> of my voltage fixes patches till then?
>>
>
> Sure.  It's the first time anyone as asked me not to review patches. :)
> I'll gladly comply.
>
> I've already folded the minimal change I proposed into the original
> patch locally, and will push updated voltdm_* branches by the end of
> today.

:) Thanks. I will rework the patches and line up a series from cpufreq
and voltage domain perspective in the upcoming days..

Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html