[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", &gta01_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