| Message ID | 1414411911-5539-4-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Krzysztof, On 10/27/2014 01:11 PM, Krzysztof Kozlowski wrote: > Some LDOs of Maxim 77686 PMIC support disabling during system suspend > (LDO{2,6,7,8,10,11,12,14,15,16}). This was already implemented as part > of set_suspend_mode function. In that case the mode was one of: > - disable, > - normal mode, > - low power mode. > However there are no bindings for setting the mode during suspend. > > Add suspend disable for LDO regulators supporting this. Re-use existing > max77686_buck_set_suspend_disable() function. This helps reducing > energy consumption during system sleep. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > drivers/regulator/max77686.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c > index 2ebc4257425b..c6bf6f79bd2a 100644 > --- a/drivers/regulator/max77686.c > +++ b/drivers/regulator/max77686.c Looks good to me. Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 27, 2014 at 01:11:49PM +0100, Krzysztof Kozlowski wrote: > @@ -247,6 +250,8 @@ static struct regulator_ops max77686_ldo_ops = { > .set_voltage_sel = regulator_set_voltage_sel_regmap, > .set_voltage_time_sel = regulator_set_voltage_time_sel, > .set_suspend_mode = max77686_ldo_set_suspend_mode, > + .set_suspend_disable = max77686_set_suspend_disable, > + .set_suspend_enable = max77686_enable, This looks wrong, you're using the regular enable operation as suspend enable. How does that work without disrupting the current runtime state?
On wto, 2014-10-28 at 22:31 +0000, Mark Brown wrote: > On Mon, Oct 27, 2014 at 01:11:49PM +0100, Krzysztof Kozlowski wrote: > > > @@ -247,6 +250,8 @@ static struct regulator_ops max77686_ldo_ops = { > > .set_voltage_sel = regulator_set_voltage_sel_regmap, > > .set_voltage_time_sel = regulator_set_voltage_time_sel, > > .set_suspend_mode = max77686_ldo_set_suspend_mode, > > + .set_suspend_disable = max77686_set_suspend_disable, > > + .set_suspend_enable = max77686_enable, > > This looks wrong, you're using the regular enable operation as suspend > enable. How does that work without disrupting the current runtime > state? Currently it shouldn't disrupt state of regulator because during runtime it may only be only: on (0x3) or off (0x0). Suspend enable in max77686 writes 0x3 to the register which means - always on. If regulator was disabled before suspend then it has to be enabled during suspend_enable() call which is exactly what max77686_enable does. If it was enabled then nothing happens. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 10:20:13AM +0100, Krzysztof Kozlowski wrote: > On wto, 2014-10-28 at 22:31 +0000, Mark Brown wrote: > > This looks wrong, you're using the regular enable operation as suspend > > enable. How does that work without disrupting the current runtime > > state? > Currently it shouldn't disrupt state of regulator because during runtime > it may only be only: on (0x3) or off (0x0). Suspend enable in max77686 > writes 0x3 to the register which means - always on. > If regulator was disabled before suspend then it has to be enabled > during suspend_enable() call which is exactly what max77686_enable does. > If it was enabled then nothing happens. No, this isn't suspend enable control - this is normal, standard enable control and the device has no suspend enable control.
On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote: > On Wed, Oct 29, 2014 at 10:20:13AM +0100, Krzysztof Kozlowski wrote: > > On wto, 2014-10-28 at 22:31 +0000, Mark Brown wrote: > > > > This looks wrong, you're using the regular enable operation as suspend > > > enable. How does that work without disrupting the current runtime > > > state? > > > Currently it shouldn't disrupt state of regulator because during runtime > > it may only be only: on (0x3) or off (0x0). Suspend enable in max77686 > > writes 0x3 to the register which means - always on. > > > If regulator was disabled before suspend then it has to be enabled > > during suspend_enable() call which is exactly what max77686_enable does. > > If it was enabled then nothing happens. > > No, this isn't suspend enable control - this is normal, standard enable > control and the device has no suspend enable control. You mean that for such regulator the driver shouldn't implement suspend_enable()? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 11:18:54AM +0100, Krzysztof Kozlowski wrote: > On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote: > > No, this isn't suspend enable control - this is normal, standard enable > > control and the device has no suspend enable control. > You mean that for such regulator the driver shouldn't implement > suspend_enable()? Yes, if there is no separate control of suspend mode in hardware then of course the driver shouldn't implement operations for things it doesn't have.
On ?ro, 2014-10-29 at 10:31 +0000, Mark Brown wrote: > On Wed, Oct 29, 2014 at 11:18:54AM +0100, Krzysztof Kozlowski wrote: > > On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote: > > > > No, this isn't suspend enable control - this is normal, standard enable > > > control and the device has no suspend enable control. > > > You mean that for such regulator the driver shouldn't implement > > suspend_enable()? > > Yes, if there is no separate control of suspend mode in hardware then of > course the driver shouldn't implement operations for things it doesn't > have. Oh, thanks! I'll send fixed patch. This means that probably the max77802 ("mirrored" driver) should be fixed... Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Krzysztof, On 10/29/2014 11:44 AM, Krzysztof Kozlowski wrote: > On ?ro, 2014-10-29 at 10:31 +0000, Mark Brown wrote: >> On Wed, Oct 29, 2014 at 11:18:54AM +0100, Krzysztof Kozlowski wrote: >> > On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote: >> >> > > No, this isn't suspend enable control - this is normal, standard enable >> > > control and the device has no suspend enable control. >> >> > You mean that for such regulator the driver shouldn't implement >> > suspend_enable()? >> >> Yes, if there is no separate control of suspend mode in hardware then of >> course the driver shouldn't implement operations for things it doesn't >> have. > > Oh, thanks! I'll send fixed patch. > > This means that probably the max77802 ("mirrored" driver) should be > fixed... > Indeed, I had the same confusion that you had. Just to avoid duplicating work, do you want me to send a fix or are you going to include one on your series? > Best regards, > Krzysztof > > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On ?ro, 2014-10-29 at 11:51 +0100, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 10/29/2014 11:44 AM, Krzysztof Kozlowski wrote: > > On ?ro, 2014-10-29 at 10:31 +0000, Mark Brown wrote: > >> On Wed, Oct 29, 2014 at 11:18:54AM +0100, Krzysztof Kozlowski wrote: > >> > On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote: > >> > >> > > No, this isn't suspend enable control - this is normal, standard enable > >> > > control and the device has no suspend enable control. > >> > >> > You mean that for such regulator the driver shouldn't implement > >> > suspend_enable()? > >> > >> Yes, if there is no separate control of suspend mode in hardware then of > >> course the driver shouldn't implement operations for things it doesn't > >> have. > > > > Oh, thanks! I'll send fixed patch. > > > > This means that probably the max77802 ("mirrored" driver) should be > > fixed... > > > > Indeed, I had the same confusion that you had. Just to avoid duplicating work, > do you want me to send a fix or are you going to include one on your series? I'll send a patch for max77802 also. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/max77686.c b/drivers/regulator/max77686.c index 2ebc4257425b..c6bf6f79bd2a 100644 --- a/drivers/regulator/max77686.c +++ b/drivers/regulator/max77686.c @@ -99,8 +99,8 @@ static unsigned int max77686_get_opmode_shift(int id) } } -/* Some BUCKS supports Normal[ON/OFF] mode during suspend */ -static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev) +/* Some BUCKs and LDOs supports Normal[ON/OFF] mode during suspend */ +static int max77686_set_suspend_disable(struct regulator_dev *rdev) { unsigned int val, shift; struct max77686_data *max77686 = rdev_get_drvdata(rdev); @@ -195,6 +195,9 @@ static int max77686_enable(struct regulator_dev *rdev) shift = max77686_get_opmode_shift(id); + if (max77686->opmode[id] == MAX77686_OFF_PWRREQ) + max77686->opmode[id] = MAX77686_NORMAL; + return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, rdev->desc->enable_mask, max77686->opmode[id] << shift); @@ -247,6 +250,8 @@ static struct regulator_ops max77686_ldo_ops = { .set_voltage_sel = regulator_set_voltage_sel_regmap, .set_voltage_time_sel = regulator_set_voltage_time_sel, .set_suspend_mode = max77686_ldo_set_suspend_mode, + .set_suspend_disable = max77686_set_suspend_disable, + .set_suspend_enable = max77686_enable, }; static struct regulator_ops max77686_buck1_ops = { @@ -258,7 +263,8 @@ static struct regulator_ops max77686_buck1_ops = { .get_voltage_sel = regulator_get_voltage_sel_regmap, .set_voltage_sel = regulator_set_voltage_sel_regmap, .set_voltage_time_sel = regulator_set_voltage_time_sel, - .set_suspend_disable = max77686_buck_set_suspend_disable, + .set_suspend_disable = max77686_set_suspend_disable, + .set_suspend_enable = max77686_enable, }; static struct regulator_ops max77686_buck_dvs_ops = { @@ -271,7 +277,8 @@ static struct regulator_ops max77686_buck_dvs_ops = { .set_voltage_sel = regulator_set_voltage_sel_regmap, .set_voltage_time_sel = regulator_set_voltage_time_sel, .set_ramp_delay = max77686_set_ramp_delay, - .set_suspend_disable = max77686_buck_set_suspend_disable, + .set_suspend_disable = max77686_set_suspend_disable, + .set_suspend_enable = max77686_enable, }; #define regulator_desc_ldo(num) { \
Some LDOs of Maxim 77686 PMIC support disabling during system suspend (LDO{2,6,7,8,10,11,12,14,15,16}). This was already implemented as part of set_suspend_mode function. In that case the mode was one of: - disable, - normal mode, - low power mode. However there are no bindings for setting the mode during suspend. Add suspend disable for LDO regulators supporting this. Re-use existing max77686_buck_set_suspend_disable() function. This helps reducing energy consumption during system sleep. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/regulator/max77686.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)