[UPSTREAM - RFC] pm_bt_use_regulator_api.patch

[Mark Brown]

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.

> +			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.

> +				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.