| Message ID | 1413879912-26606-2-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Krzysztof, On 10/21/2014 10:25 AM, 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 You should also document what each regulator support in the max77686 DT binding doc, which btw I see is in Documentation/devicetree/bindings/mfd/max77686.txt while I think it should be in Documentation/devicetree/bindings/regulators/ I just posted a patch to add this information to the max77802 DT binding doc [0] so maybe you can add something along those lines? > 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. > > Implement a set_suspend_disable function for regulators supporting this. > This helps reducing energy consumption during system sleep. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > drivers/regulator/max77686.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c > index ef1af2debbd2..84603051f24d 100644 > --- a/drivers/regulator/max77686.c > +++ b/drivers/regulator/max77686.c > @@ -123,6 +123,24 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev, > return 0; > } > > +static int max77686_ldo_set_suspend_disable(struct regulator_dev *rdev, > + unsigned int mode) > +{ > + unsigned int val; > + struct max77686_data *max77686 = rdev_get_drvdata(rdev); > + int ret; > + > + val = 0x1 << MAX77686_OPMODE_SHIFT; Maybe instead of using magic numbers you can document what 0x1 means here. I don't have access to the max77686 data-sheet but I do have for the max77802 so my educated guess is that 0x1 means "Output ON/OFF Controlled by PWRREQ" and that the Exynos 4412 XPWRRGTON pin is connected to the max77686 PWRREQ in the Trats2 board. > + > + ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, > + rdev->desc->enable_mask, val); > + if (ret) > + return ret; > + > + max77686->opmode[rdev_get_id(rdev)] = val; > + return 0; > +} > + There is already a max77686_buck_set_suspend_disable() that is used by BUCK 1-4. At least in the max77802, all regulators expect LDO 1, 3, 20 and 21 support OFF by PWRREQ so with some refactoring (using a function to get the opmode shift) you can use the same function for all regulators that support this instead of having separate .set_suspend_disable function handlers. > /* Some LDOs supports LPM-ON/OFF/Normal-ON mode during suspend state */ > static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev, > unsigned int mode) > @@ -212,6 +230,7 @@ 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_ldo_set_suspend_disable, You probably want add .set_suspend_enable as well. > }; > > static struct regulator_ops max77686_buck1_ops = { > In general, could you please take a look to the latest patches I posted for the max77802? They are in a topic branch in Mark's regulator tree [1]. I would like both 77686 and 77802 drivers to handle things similarly even though I know that the PMICs don't behave identically and that's why we decided to have different drivers (although maybe that was a mistake and makes more sense to merge them). Thanks a lot and best regards, Javier [0]: https://lkml.org/lkml/2014/10/20/320 [1]: https://git.kernel.org/cgit/linux/kernel/git/broonie/regulator.git/log/?h=topic/max77802 -- 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 wto, 2014-10-21 at 11:10 +0200, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 10/21/2014 10:25 AM, 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 > > You should also document what each regulator support in the max77686 DT binding > doc, which btw I see is in Documentation/devicetree/bindings/mfd/max77686.txt > while I think it should be in Documentation/devicetree/bindings/regulators/ > I just posted a patch to add this information to the max77802 DT binding doc [0] > so maybe you can add something along those lines? Sure, I'll document this. > > 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. > > > > Implement a set_suspend_disable function for regulators supporting this. > > This helps reducing energy consumption during system sleep. > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > --- > > drivers/regulator/max77686.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c > > index ef1af2debbd2..84603051f24d 100644 > > --- a/drivers/regulator/max77686.c > > +++ b/drivers/regulator/max77686.c > > @@ -123,6 +123,24 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev, > > return 0; > > } > > > > +static int max77686_ldo_set_suspend_disable(struct regulator_dev *rdev, > > + unsigned int mode) > > +{ > > + unsigned int val; > > + struct max77686_data *max77686 = rdev_get_drvdata(rdev); > > + int ret; > > + > > + val = 0x1 << MAX77686_OPMODE_SHIFT; > > Maybe instead of using magic numbers you can document what 0x1 means here. > I don't have access to the max77686 data-sheet but I do have for the max77802 > so my educated guess is that 0x1 means "Output ON/OFF Controlled by PWRREQ" > and that the Exynos 4412 XPWRRGTON pin is connected to the max77686 PWRREQ in > the Trats2 board. OK. > > > + > > + ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, > > + rdev->desc->enable_mask, val); > > + if (ret) > > + return ret; > > + > > + max77686->opmode[rdev_get_id(rdev)] = val; > > + return 0; > > +} > > + > > There is already a max77686_buck_set_suspend_disable() that is used by BUCK 1-4. > > At least in the max77802, all regulators expect LDO 1, 3, 20 and 21 support OFF > by PWRREQ so with some refactoring (using a function to get the opmode shift) > you can use the same function for all regulators that support this instead of > having separate .set_suspend_disable function handlers. OK, I can merge this into one suspend_disable. > > > /* Some LDOs supports LPM-ON/OFF/Normal-ON mode during suspend state */ > > static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev, > > unsigned int mode) > > @@ -212,6 +230,7 @@ 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_ldo_set_suspend_disable, > > You probably want add .set_suspend_enable as well. Yes, I'll add it. > > > }; > > > > static struct regulator_ops max77686_buck1_ops = { > > > > In general, could you please take a look to the latest patches I posted for the > max77802? They are in a topic branch in Mark's regulator tree [1]. I would like > both 77686 and 77802 drivers to handle things similarly even though I know that > the PMICs don't behave identically and that's why we decided to have different > drivers (although maybe that was a mistake and makes more sense to merge them). I looked at them. In this patchset I didn't want to implement fully the whole suspend-mode-thing. Just the first, easier part - set suspend disable. In general I would like to do it similar to your solution but that is work for future. Especially as I would like to wait for merging your "Add max77802 regulator operating mode support". Best regards, Krzysztof > > Thanks a lot and best regards, > Javier > > [0]: https://lkml.org/lkml/2014/10/20/320 > [1]: https://git.kernel.org/cgit/linux/kernel/git/broonie/regulator.git/log/?h=topic/max77802 -- 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/21/2014 11:36 AM, Krzysztof Kozlowski wrote: > On wto, 2014-10-21 at 11:10 +0200, Javier Martinez Canillas wrote: >> >> In general, could you please take a look to the latest patches I posted for the >> max77802? They are in a topic branch in Mark's regulator tree [1]. I would like >> both 77686 and 77802 drivers to handle things similarly even though I know that >> the PMICs don't behave identically and that's why we decided to have different >> drivers (although maybe that was a mistake and makes more sense to merge them). > > I looked at them. In this patchset I didn't want to implement fully the > whole suspend-mode-thing. Just the first, easier part - set suspend > disable. > Agreed, incremental support makes sense to me as well. I also want to configure the regulator modes in suspend for the Snow Chromebook using the same modes that are in the downstream Chrome OS tree and I think your changes will be enough for that too. > In general I would like to do it similar to your solution but that is > work for future. Especially as I would like to wait for merging your > "Add max77802 regulator operating mode support". > Perfect, thanks a lot for working on this! > 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
diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c index ef1af2debbd2..84603051f24d 100644 --- a/drivers/regulator/max77686.c +++ b/drivers/regulator/max77686.c @@ -123,6 +123,24 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev, return 0; } +static int max77686_ldo_set_suspend_disable(struct regulator_dev *rdev, + unsigned int mode) +{ + unsigned int val; + struct max77686_data *max77686 = rdev_get_drvdata(rdev); + int ret; + + val = 0x1 << MAX77686_OPMODE_SHIFT; + + ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, + rdev->desc->enable_mask, val); + if (ret) + return ret; + + max77686->opmode[rdev_get_id(rdev)] = val; + return 0; +} + /* Some LDOs supports LPM-ON/OFF/Normal-ON mode during suspend state */ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev, unsigned int mode) @@ -212,6 +230,7 @@ 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_ldo_set_suspend_disable, }; static struct regulator_ops max77686_buck1_ops = {
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. Implement a set_suspend_disable function for regulators supporting this. This helps reducing energy consumption during system sleep. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/regulator/max77686.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)