Messages in this thread | | | Date | Fri, 5 Jul 2013 12:33:10 -0500 | From | Nishanth Menon <> | Subject | Re: [RFC PATCH V2 1/8] regulator: Introduce OMAP regulator to control PMIC over VC/VP |
| |
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
| |