Re: [PATCH 1/2] regulator: max77686: Implement suspend disable for some LDOs

[Javier Martinez Canillas <javier.martinez@collabora.co.uk>]

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