[UPSTREAM - RFC] pm_bt_use_regulator_api.patch
Balaji Rao
balaji at raobalaji.com
Wed Oct 8 18:52:22 CEST 2008
On Wed, 8 Oct 2008 16:51:45 +0100
Mark Brown <broonie at opensource.wolfsonmicro.com> wrote:
> On Wed, Oct 08, 2008 at 07:41:26PM +0530, Balaji Rao wrote:
>
> > +static struct gta01_pm_bt_platform_data gta01_pm_bt_dev_pdata = {
> > + .voltage_supply = "BT_3V2",
> > +};
>
> This should ideally be done by requesting a fixed supply name together
> and supplying the dev - the mapping is set up by the platform data in
> the current for-linus regulator API, or with
> regulator_set_device_supply() in the original version (but see
> below!).
>
> Requesting fixed names like this still works, it's just not the
> preferred idiom since it scatters code and data for each regulator
> about rather than keeping it in a single kernel.
>
> > /* Regulation constraints */
> > @@ -585,6 +594,8 @@ static void
> > gta02_pcf50633_attach_child_devices(struct device *parent_device)
> > regulator_set_machine_constraints("ldo4", &ldo4_constraint);
> > regulator_set_machine_constraints("ldo5", &ldo5_constraint);
> > + regulator_set_device_supply("ldo4", >a01_pm_bt_dev.dev,
> > "BT_3V2"); +
>
> You're already doing what I suggested - in that case you shouldn't
> need to pass the name of the supply via platform data at all since
> the name will be mapped here and the driver can just hard code the
> name it wants.
>
You mean I can use BT_3V2 directly in the driver ? Fine.
> > + if (on) {
> > + regulator_set_voltage(regulator,
> > 3200000, 3200000);
>
> I forget from the platform data, is this regulator allowed to vary?
> If not then just set this in the platform data and let the core do
> this for you - you only ever set it to one value anyway.
>
> regulator_set_voltage() is mostly intended to be used when varying the
> voltage.
>
I didn't get this part. You mean to say that I should set the voltage
directly and that it should never be required for us to use it
set_voltage here ?
> > + if
> > (!regulator_is_enabled(regulator))
> > +
> > regulator_enable(regulator);
> > + } else {
> > + if
> > (regulator_is_enabled(regulator))
> > +
> > regulator_disable(regulator);
> > + }
> > +
> > + vol = regulator_get_voltage(regulator);
>
> There's misssing error checking here (but it wasn't there in the
> orignal either so no loss there). The checks for
> regulator_is_enabled() look a bit odd, though it looks like this is
> because there's not enough state maintained in the driver to know the
> state locally. If the regulator isn't shared it shouldn't make much
> odds.
OK. But what if the device is tried to be powered on even when it's
on ? I found that I get a warning that the regulator is already enabled.
One more thing I wanted to ask is about the suspend/resume handling
within the regulator API. Is it mainly about saving state before going
to suspend ? I'm yet to to figure out how to use it.
- Balaji
More information about the openmoko-kernel
mailing list