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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux