Messages in this thread | | | Date | Tue, 21 Oct 2014 11:10:28 +0200 | From | Javier Martinez Canillas <> | Subject | Re: [PATCH 1/2] regulator: max77686: Implement suspend disable for some LDOs |
| |
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
| |