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

[Nishanth Menon]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/