| Message ID | 1392577256-20475-1-git-send-email-mpa@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Feb 16, 2014 at 08:00:56PM +0100, Markus Pargmann wrote: Please use more standard subject lines, don't do things like "core bugfix", just write a normal changelog. > +static int _regulator_do_enable_no_delay(struct regulator_dev *rdev) > +{ > + int ret; > + > + if (rdev->ena_pin) { > + ret = regulator_ena_gpio_ctrl(rdev, true); > + if (ret < 0) > + return ret; > + rdev->ena_gpio_state = 1; > + } else if (rdev->desc->ops->enable) { > + ret = rdev->desc->ops->enable(rdev); > + } else { > + ret = -EINVAL; > + } > + > + return ret; > +} I don't understand this. Why is this called _no_delay() and why don't we want to delay when applying constraints? We don't want to ever be in a position where we think a supply is enabled but it has in fact not finished ramping, and of course enable() may in fact be blocking anyway. The use of the common code to do enable is good fix but this just seems odd.
On Tue, Feb 18, 2014 at 09:14:20AM +0900, Mark Brown wrote: > On Sun, Feb 16, 2014 at 08:00:56PM +0100, Markus Pargmann wrote: > > Please use more standard subject lines, don't do things like "core > bugfix", just write a normal changelog. Okay, will fix. > > > +static int _regulator_do_enable_no_delay(struct regulator_dev *rdev) > > +{ > > + int ret; > > + > > + if (rdev->ena_pin) { > > + ret = regulator_ena_gpio_ctrl(rdev, true); > > + if (ret < 0) > > + return ret; > > + rdev->ena_gpio_state = 1; > > + } else if (rdev->desc->ops->enable) { > > + ret = rdev->desc->ops->enable(rdev); > > + } else { > > + ret = -EINVAL; > > + } > > + > > + return ret; > > +} > > I don't understand this. Why is this called _no_delay() and why don't > we want to delay when applying constraints? We don't want to ever be in > a position where we think a supply is enabled but it has in fact not > finished ramping, and of course enable() may in fact be blocking anyway. I tried not to modify the current behaviour of the core driver for non-gpio regulators. Before this patch only ops->enable() was called which also didn't have a delay. So I seperated the non-delay enable function to have the same behaviour for normal regulators. Also the constraints are applied when registering a new regulator. For "boot-on" we should not delay because this regulator is already on by definition. But I am not sure what to do with always-on regulators? Thanks, Markus
On Tue, Feb 18, 2014 at 10:40:07PM +0100, Markus Pargmann wrote: > On Tue, Feb 18, 2014 at 09:14:20AM +0900, Mark Brown wrote: > > I don't understand this. Why is this called _no_delay() and why don't > > we want to delay when applying constraints? We don't want to ever be in > > a position where we think a supply is enabled but it has in fact not > > finished ramping, and of course enable() may in fact be blocking anyway. > I tried not to modify the current behaviour of the core driver for > non-gpio regulators. Before this patch only ops->enable() was called > which also didn't have a delay. So I seperated the non-delay enable > function to have the same behaviour for normal regulators. No, that's not good. The fact that it wasn't applying delays is going to be a bug - it should've been doing that. > Also the constraints are applied when registering a new regulator. For > "boot-on" we should not delay because this regulator is already on by > definition. But I am not sure what to do with always-on regulators? I'd just always apply a delay, it's simpler and more robust.
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 16a309e..8469244 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -953,6 +953,8 @@ static int machine_constraints_current(struct regulator_dev *rdev, return 0; } +static int _regulator_do_enable_no_delay(struct regulator_dev *rdev); + /** * set_machine_constraints - sets regulator constraints * @rdev: regulator source @@ -1013,9 +1015,8 @@ static int set_machine_constraints(struct regulator_dev *rdev, /* If the constraints say the regulator should be on at this point * and we have control then make sure it is enabled. */ - if ((rdev->constraints->always_on || rdev->constraints->boot_on) && - ops->enable) { - ret = ops->enable(rdev); + if (rdev->constraints->always_on || rdev->constraints->boot_on) { + ret = _regulator_do_enable_no_delay(rdev); if (ret < 0) { rdev_err(rdev, "failed to enable\n"); goto out; @@ -1745,6 +1746,24 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable) return 0; } +static int _regulator_do_enable_no_delay(struct regulator_dev *rdev) +{ + int ret; + + if (rdev->ena_pin) { + ret = regulator_ena_gpio_ctrl(rdev, true); + if (ret < 0) + return ret; + rdev->ena_gpio_state = 1; + } else if (rdev->desc->ops->enable) { + ret = rdev->desc->ops->enable(rdev); + } else { + ret = -EINVAL; + } + + return ret; +} + static int _regulator_do_enable(struct regulator_dev *rdev) { int ret, delay; @@ -1760,18 +1779,9 @@ static int _regulator_do_enable(struct regulator_dev *rdev) trace_regulator_enable(rdev_get_name(rdev)); - if (rdev->ena_pin) { - ret = regulator_ena_gpio_ctrl(rdev, true); - if (ret < 0) - return ret; - rdev->ena_gpio_state = 1; - } else if (rdev->desc->ops->enable) { - ret = rdev->desc->ops->enable(rdev); - if (ret < 0) - return ret; - } else { - return -EINVAL; - } + ret = _regulator_do_enable_no_delay(rdev); + if (ret) + return ret; /* Allow the regulator to ramp; it would be useful to extend * this for bulk operations so that the regulators can ramp @@ -3633,9 +3643,8 @@ int regulator_suspend_finish(void) struct regulator_ops *ops = rdev->desc->ops; mutex_lock(&rdev->mutex); - if ((rdev->use_count > 0 || rdev->constraints->always_on) && - ops->enable) { - error = ops->enable(rdev); + if (rdev->use_count > 0 || rdev->constraints->always_on) { + error = _regulator_do_enable_no_delay(rdev); if (error) ret = error; } else {
always_on regulators are currently enabled at registration using the enable function of struct regulator_ops. The regulator framework does support gpios to enable/disable regulators. gpios are not handled through struct regulator_ops functions as they are a basic component of the framework. Instead they are handled within _regulator_do_enable(). Unfortunately always-on regulator do not use this function, to gpio-controlled always-on regulators which are not enabled on boot. Furthermore they are disabled the whole time because regulator_enable() does not call _regulator_do_enable() for always-on regulator. This patch seperates _regulator_do_enable_no_delay from the _regulator_do_enable function to be able to call the enable function without delays. It then fixes the always_on behaviour by using this function. Cc: stable@vger.kernel.org Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- drivers/regulator/core.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)