| Message ID | 1346926861-534-1-git-send-email-kliu5@marvell.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Sep 06, 2012 at 06:21:01PM +0800, Kevin Liu wrote: > add status check in suspend_set_state to avoid redundant enable or disable. > suspend_finish just restore the status before suspend_prepare to avoid redundant enable. I'm not exactly sure what this commit is supposed to do... the subject line needs to describe the meaning of the change (we can tell from the diff which functions are changed) and even looking at the body of the commit it's not clear how the change achieves its goal - what are we checking for? Please also check your word wrapping. > /* If we have no suspend mode configration don't set anything; > * only warn if the driver implements set_suspend_voltage or > * set_suspend_mode callback. > */ > if (!rstate->enabled && !rstate->disabled) { > - if (rdev->desc->ops->set_suspend_voltage || > - rdev->desc->ops->set_suspend_mode) > + if (ops->set_suspend_voltage || ops->set_suspend_mode) > rdev_warn(rdev, "No configuration\n"); > return 0; > } I don't understand what this hunk is supposed to do, the error message doesn't match up with what is being tested (and isn't very clear, it should at least identify what isn't being configured). In general I'm finding this extremely hard to understand. It also seems likely that this should be split into multiple changes, for example restructuring the code separately to any functional changes. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark, Sorry for unclear expressing. This patch is mainly to avoid redundant enabling during suspend_finish. In current code, all regulators which has been used or always on will be enabled in suspend_finish. This will cost time with i2c pmic. We don't need to do this if the regulator has NOT been disabled during suspend_prepare. This patch has a bug. I will update the patch and send again. Thanks Kevin -----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Thursday, September 06, 2012 6:40 PM To: Kevin Liu Cc: linux-pm@vger.kernel.org; rjw@sisk.pl; Chao Xie; Haojian Zhuang Subject: Re: [PATCH] regulator: core: update suspend_prepare and suspend_finish On Thu, Sep 06, 2012 at 06:21:01PM +0800, Kevin Liu wrote: > add status check in suspend_set_state to avoid redundant enable or disable. > suspend_finish just restore the status before suspend_prepare to avoid redundant enable. I'm not exactly sure what this commit is supposed to do... the subject line needs to describe the meaning of the change (we can tell from the diff which functions are changed) and even looking at the body of the commit it's not clear how the change achieves its goal - what are we checking for? Please also check your word wrapping. > /* If we have no suspend mode configration don't set anything; > * only warn if the driver implements set_suspend_voltage or > * set_suspend_mode callback. > */ > if (!rstate->enabled && !rstate->disabled) { > - if (rdev->desc->ops->set_suspend_voltage || > - rdev->desc->ops->set_suspend_mode) > + if (ops->set_suspend_voltage || ops->set_suspend_mode) > rdev_warn(rdev, "No configuration\n"); > return 0; > } I don't understand what this hunk is supposed to do, the error message doesn't match up with what is being tested (and isn't very clear, it should at least identify what isn't being configured). In general I'm finding this extremely hard to understand. It also seems likely that this should be split into multiple changes, for example restructuring the code separately to any functional changes. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 06, 2012 at 07:48:54PM -0700, Kevin Liu wrote: Don't top post! > Sorry for unclear expressing. > This patch is mainly to avoid redundant enabling during suspend_finish. > In current code, all regulators which has been used or always on will be enabled in suspend_finish. This will cost time with i2c pmic. > We don't need to do this if the regulator has NOT been disabled during suspend_prepare. > This patch has a bug. I will update the patch and send again. The above is still not very clear to me. Why is the code reenabling things and why is it a bad idea to do this? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index f092588..eee22b7 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -656,14 +656,14 @@ static int suspend_set_state(struct regulator_dev *rdev, struct regulator_state *rstate) { int ret = 0; + struct regulator_ops *ops = rdev->desc->ops; /* If we have no suspend mode configration don't set anything; * only warn if the driver implements set_suspend_voltage or * set_suspend_mode callback. */ if (!rstate->enabled && !rstate->disabled) { - if (rdev->desc->ops->set_suspend_voltage || - rdev->desc->ops->set_suspend_mode) + if (ops->set_suspend_voltage || ops->set_suspend_mode) rdev_warn(rdev, "No configuration\n"); return 0; } @@ -673,28 +673,35 @@ static int suspend_set_state(struct regulator_dev *rdev, return -EINVAL; } - if (rstate->enabled && rdev->desc->ops->set_suspend_enable) - ret = rdev->desc->ops->set_suspend_enable(rdev); - else if (rstate->disabled && rdev->desc->ops->set_suspend_disable) - ret = rdev->desc->ops->set_suspend_disable(rdev); - else /* OK if set_suspend_enable or set_suspend_disable is NULL */ + if (rstate->enabled && ops->set_suspend_enable) { + ret = ops->set_suspend_enable(rdev); + if (ret < 0) { + rdev_err(rdev, "failed to enabled\n"); + return ret; + } + rdev->suspend_flag = SUSPEND_ENABLED; + } else if (rstate->disabled && ops->set_suspend_disable) { + ret = ops->set_suspend_disable(rdev); + if (ret < 0) { + rdev_err(rdev, "failed to disabled\n"); + return ret; + } + rdev->suspend_flag = SUSPEND_DISABLED; + } else { + /* OK if set_suspend_enable or set_suspend_disable is NULL */ ret = 0; - - if (ret < 0) { - rdev_err(rdev, "failed to enabled/disable\n"); - return ret; } - if (rdev->desc->ops->set_suspend_voltage && rstate->uV > 0) { - ret = rdev->desc->ops->set_suspend_voltage(rdev, rstate->uV); + if (ops->set_suspend_voltage && rstate->uV > 0) { + ret = ops->set_suspend_voltage(rdev, rstate->uV); if (ret < 0) { rdev_err(rdev, "failed to set voltage\n"); return ret; } } - if (rdev->desc->ops->set_suspend_mode && rstate->mode > 0) { - ret = rdev->desc->ops->set_suspend_mode(rdev, rstate->mode); + if (ops->set_suspend_mode && rstate->mode > 0) { + ret = ops->set_suspend_mode(rdev, rstate->mode); if (ret < 0) { rdev_err(rdev, "failed to set mode\n"); return ret; @@ -3390,31 +3397,20 @@ EXPORT_SYMBOL_GPL(regulator_suspend_prepare); int regulator_suspend_finish(void) { struct regulator_dev *rdev; - int ret = 0, error; + int ret = 0; mutex_lock(®ulator_list_mutex); list_for_each_entry(rdev, ®ulator_list, list) { 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 (error) - ret = error; - } else { - if (!has_full_constraints) - goto unlock; - if (!ops->disable) - goto unlock; - if (!_regulator_is_enabled(rdev)) - goto unlock; - - error = ops->disable(rdev); - if (error) - ret = error; + if ((rdev->suspend_flag == SUSPEND_DISABLED) && ops->enable) { + ret = ops->enable(rdev); + } else if ((rdev->suspend_flag == SUSPEND_ENABLED) && + ops->disable) { + ret = ops->disable(rdev); } -unlock: + rdev->suspend_flag = SUSPEND_NONE; mutex_unlock(&rdev->mutex); } mutex_unlock(®ulator_list_mutex); diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h index bac4c87..d545417 100644 --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -155,6 +155,15 @@ enum regulator_type { REGULATOR_CURRENT, }; +/* + * Regulator can disable, enable or do nothing during suspend + */ +enum regulator_suspend_flag { + SUSPEND_NONE, + SUSPEND_ENABLED, + SUSPEND_DISABLED, +}; + /** * struct regulator_desc - Static regulator descriptor * @@ -253,6 +262,7 @@ struct regulator_dev { int exclusive; u32 use_count; u32 open_count; + enum regulator_suspend_flag suspend_flag; /* lists we belong to */ struct list_head list; /* list of all regulators */
add status check in suspend_set_state to avoid redundant enable or disable. suspend_finish just restore the status before suspend_prepare to avoid redundant enable. Signed-off-by: Kevin Liu <kliu5@marvell.com> --- drivers/regulator/core.c | 62 +++++++++++++++++-------------------- include/linux/regulator/driver.h | 10 ++++++ 2 files changed, 39 insertions(+), 33 deletions(-)