Re: [RFC PATCH V2 1/8] regulator: Introduce OMAP regulator to control PMIC over VC/VP

[Nishanth Menon <nm@ti.com>]

On 07/05/2013 11:52 AM, Mark Brown wrote:
> On Fri, Jul 05, 2013 at 09:50:34AM -0500, Nishanth Menon wrote:
>> On 07/05/2013 09:08 AM, Mark Brown wrote:
>
>>>> option 1) we just bypass get_voltage/set_voltage to point through to
>>>> this function. result will be something similar to what we got here[1]
>
>>> I don't really know what you mean when you say "bypass get_voltage/set_voltage
>>> so it's kind of hard to comment...  the link you posted appears to be a
>>> link to the code I'm reviewing so it's not terribly enlightening.
>
>> :) it is similar, yes. by bypass get/set_voltage, I mean something like [1]
>
> No, that's not a good idea.

I agree.

>
>>> What makes you think that the existing regulator drivers need to be
>>> modified?
>
>> data path difference - Almost all standard regulators use i2c
>> (standard i2c APIs) for every other SMPS/LDO except for the ones
>> controlled by OMAP custom i2c path(vc/vp), the
>> set_voltage/get_voltage needs a different implementation when it
>> comes to using the vc/vp path.
>
>>> They already have all the data exported to the core (or
>>> should do)...
>
>> I see that twl-regulator does not indeed do it, but, assuming the
>> regulators have all the data exported to the core, the data is
>> hidden in struct regulator_desc which is private to the device and
>> driver. lets go through these:
>
> That's just a simple matter of programming to fix, and any regulator
> which can work with this sort of table based stuff you're looking at
> here must also be able to be converted to work with the equivalent code
> in the regulator core so open coding is a deficiency in the driver.

OK, conceptually, I am a bit lost here (may be I thinking about "how the 
heck am I supposed to implement this?") - Hoping for your patience in 
trying to get through to my thick skull :)

Taking an example of twl-regulator and omap_pmic, are you suggesting 
omap_pmic to be a user twl-regulator using 
include/linux/regulator/consumer.h? or are you suggesting that omap_pmic 
should not be a regulator at all?

Could you suggest what you have in your mind here, I can see how we can 
make that happen. I am not averse to writing new code ofcourse.

>
>>> +	.cmd_reg_addr = 0x00,
>
>> command register is used for sending low power state commands -
>> which is not the same as voltage register or vsel_reg as used in
>> depicted in regulator_desc.	
>
> There's no information about how to use this register in your
> bindings...  but anyway, can't be too hard to add this if it's actually
> used.

Yes it is, and also happens to be how OMAPs achieve maximum power 
savings - when low power modes are achieved in OMAP(automatic hardware 
assisted commands are send to the specific command registers in PMIC and 
viceversa on wakeup) - but this also happens to be very specific to OMAP 
way of handling things. I can refer to the Reference Manual as to how it 
actually works, but that'd be an overkill, I will try to expand on the 
bindings a little more, I guess.

>
>>> +	.i2c_timeout_us = 200,
>
>> yep, does not match up.
>
> Like I say I just don't see why this is even specified per device.
>
>>> +	.max_uV = 1450000,
>
>> can be used with constraints, but most regulator drivers seem to
>> store this internally.
>
> Or just trivially calculate it as we do currently.
>
>>> +	.voltage_selector_offset = 0,
>>> +	.voltage_selector_mask = 0x7F,
>>> +	.voltage_selector_setbits = 0x0,
>>> +	.voltage_selector_zero = false,
>
>> to an extent we can map these to vsel_mask, linear_min_sel etc.
>> (again assuming the regulator driver has populated it - most that
>> implement custom set_voltage, get_voltage do not do that.
>
> Anything that implements a custom set_voltage() won't work with your
> data structure either...

It would not work with OMAP either ;). But that said, drivers do freely 
implement custom set_voltage/get_voltage primarily because there are 
ranges in supported voltages that are non-linear and try to be generic 
to work on non-OMAP platforms as well. However, within the supported 
range, only the linear ranges are used with OMAP.

>
>> Other option is to make rdev available to omap_pmic regulator -
>> which again implies change in existing regulator driver?
>
> Why would any of the drivers need to change to do this?  They're already
> exporting the information.
I am thinking here: two regulator drivers, using same rdev/desc for 
getting the data. probably makes no sense, I agree.

>
>>> the only thing that's missing is the timeouts and it's
>>> not at all obvious why those need to be tuned per device.
>
>> OMAP VC hardware has no idea about how long to wait before giving up
>> on an ongoing i2c transaction. This may depend on PMIC and what it
>> does before acking on i2c.
>
> So pick a high number (it's only for error cases...)?
>
from hardware perspective yeah, if it does not lockup (based on erratas 
on specific devices ;) ). it also controls in part, the latency of 
response to Voltage processor from Voltage controller also needed for 
computing SmartReflex latencies (as it should consider worst case 
conditions)

-- 
Regards,
Nishanth Menon