Re: [PATCH 1/2] regulator: act8945: Implement PM functionalities
On 07.11.2018 16:53, Mark Brown wrote:
> On Fri, Oct 26, 2018 at 04:19:48PM +, claudiu.bez...@microchip.com wrote:
>
>> +static unsigned int act8945a_of_map_mode(unsigned int mode)
>> +{
>> +if (mode == ACT8945A_DCDC_MODE_POWER_SAVING)
>> +return REGULATOR_MODE_STANDBY;
>> +
>> +return REGULATOR_MODE_NORMAL;
>> +}
>
> This should error out if it gets an unknown value rather than silently
> mapping it to normal - we don't know what the user intended to set here.
> There should also be some binding documentation updates saying what the
> values that can be set are.
Sure, I will update it on next version.
>
>> +static void act8945a_pmic_shutdown(struct platform_device *pdev)
>> +{
>> +struct act8945a_pmic *act8945a = platform_get_drvdata(pdev);
>> +struct regmap *regmap = act8945a->regmap;
>> +
>> +/*
>> + * Ask the PMIC to shutdown everything on the next PWRHLD transition.
>> + */
>> +regmap_write(regmap, ACT8945A_SYS_CTRL, 0x0);
>> }
>>
>
> This shutdown function appears to be independant of the mode setting and
> would be better split out as a separate patch (you could have one patch
> adding the regmap stuff, one for this and one for the mode setting).
> It makes review a lot simpler if each patch does a minimal set of
> changes.
Ok, I will split them in next version.
Thank you,
Claudiu Beznea
>
Re: [PATCH 1/2] regulator: act8945: Implement PM functionalities
On 07.11.2018 16:53, Mark Brown wrote:
> On Fri, Oct 26, 2018 at 04:19:48PM +, claudiu.bez...@microchip.com wrote:
>
>> +static unsigned int act8945a_of_map_mode(unsigned int mode)
>> +{
>> +if (mode == ACT8945A_DCDC_MODE_POWER_SAVING)
>> +return REGULATOR_MODE_STANDBY;
>> +
>> +return REGULATOR_MODE_NORMAL;
>> +}
>
> This should error out if it gets an unknown value rather than silently
> mapping it to normal - we don't know what the user intended to set here.
> There should also be some binding documentation updates saying what the
> values that can be set are.
Sure, I will update it on next version.
>
>> +static void act8945a_pmic_shutdown(struct platform_device *pdev)
>> +{
>> +struct act8945a_pmic *act8945a = platform_get_drvdata(pdev);
>> +struct regmap *regmap = act8945a->regmap;
>> +
>> +/*
>> + * Ask the PMIC to shutdown everything on the next PWRHLD transition.
>> + */
>> +regmap_write(regmap, ACT8945A_SYS_CTRL, 0x0);
>> }
>>
>
> This shutdown function appears to be independant of the mode setting and
> would be better split out as a separate patch (you could have one patch
> adding the regmap stuff, one for this and one for the mode setting).
> It makes review a lot simpler if each patch does a minimal set of
> changes.
Ok, I will split them in next version.
Thank you,
Claudiu Beznea
>
Re: [PATCH 1/2] regulator: act8945: Implement PM functionalities
On Fri, Oct 26, 2018 at 04:19:48PM +, claudiu.bez...@microchip.com wrote:
> +static unsigned int act8945a_of_map_mode(unsigned int mode)
> +{
> + if (mode == ACT8945A_DCDC_MODE_POWER_SAVING)
> + return REGULATOR_MODE_STANDBY;
> +
> + return REGULATOR_MODE_NORMAL;
> +}
This should error out if it gets an unknown value rather than silently
mapping it to normal - we don't know what the user intended to set here.
There should also be some binding documentation updates saying what the
values that can be set are.
> +static void act8945a_pmic_shutdown(struct platform_device *pdev)
> +{
> + struct act8945a_pmic *act8945a = platform_get_drvdata(pdev);
> + struct regmap *regmap = act8945a->regmap;
> +
> + /*
> + * Ask the PMIC to shutdown everything on the next PWRHLD transition.
> + */
> + regmap_write(regmap, ACT8945A_SYS_CTRL, 0x0);
> }
>
This shutdown function appears to be independant of the mode setting and
would be better split out as a separate patch (you could have one patch
adding the regmap stuff, one for this and one for the mode setting).
It makes review a lot simpler if each patch does a minimal set of
changes.
signature.asc
Description: PGP signature
Re: [PATCH 1/2] regulator: act8945: Implement PM functionalities
On Fri, Oct 26, 2018 at 04:19:48PM +, claudiu.bez...@microchip.com wrote:
> +static unsigned int act8945a_of_map_mode(unsigned int mode)
> +{
> + if (mode == ACT8945A_DCDC_MODE_POWER_SAVING)
> + return REGULATOR_MODE_STANDBY;
> +
> + return REGULATOR_MODE_NORMAL;
> +}
This should error out if it gets an unknown value rather than silently
mapping it to normal - we don't know what the user intended to set here.
There should also be some binding documentation updates saying what the
values that can be set are.
> +static void act8945a_pmic_shutdown(struct platform_device *pdev)
> +{
> + struct act8945a_pmic *act8945a = platform_get_drvdata(pdev);
> + struct regmap *regmap = act8945a->regmap;
> +
> + /*
> + * Ask the PMIC to shutdown everything on the next PWRHLD transition.
> + */
> + regmap_write(regmap, ACT8945A_SYS_CTRL, 0x0);
> }
>
This shutdown function appears to be independant of the mode setting and
would be better split out as a separate patch (you could have one patch
adding the regmap stuff, one for this and one for the mode setting).
It makes review a lot simpler if each patch does a minimal set of
changes.
signature.asc
Description: PGP signature
[PATCH 1/2] regulator: act8945: Implement PM functionalities
From: Boris Brezillon
The regulator supports a dedicated suspend mode.
Implement the appropriate ->set_suspend_xx() hooks, add support for
->set_mode(), and provide basic PM ops functionalities to setup the
regulator in a suspend state when the system is entering suspend.
We also implement the ->shutdown() method to make sure the PMIC will
not enter the suspend state when the system is shutdown.
Signed-off-by: Boris Brezillon
[claudiu.bez...@microchip.com: remove dev_pm_ops, fix checkpatch warking, adapt
commit message]
Signed-off-by: Claudiu Beznea
---
drivers/regulator/act8945a-regulator.c | 195 -
1 file changed, 193 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/act8945a-regulator.c
b/drivers/regulator/act8945a-regulator.c
index 43fda8b4455a..41bf7d224abb 100644
--- a/drivers/regulator/act8945a-regulator.c
+++ b/drivers/regulator/act8945a-regulator.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -23,23 +24,36 @@
*/
#define ACT8945A_SYS_MODE 0x00
#define ACT8945A_SYS_CTRL 0x01
+#define ACT8945A_SYS_UNLK_REGS 0x0b
#define ACT8945A_DCDC1_VSET1 0x20
#define ACT8945A_DCDC1_VSET2 0x21
#define ACT8945A_DCDC1_CTRL0x22
+#define ACT8945A_DCDC1_SUS 0x24
#define ACT8945A_DCDC2_VSET1 0x30
#define ACT8945A_DCDC2_VSET2 0x31
#define ACT8945A_DCDC2_CTRL0x32
+#define ACT8945A_DCDC2_SUS 0x34
#define ACT8945A_DCDC3_VSET1 0x40
#define ACT8945A_DCDC3_VSET2 0x41
#define ACT8945A_DCDC3_CTRL0x42
+#define ACT8945A_DCDC3_SUS 0x44
+
+#define ACT8945A_DCDC_MODE_MSK BIT(5)
+#define ACT8945A_DCDC_MODE_FIXED BIT(5)
+#define ACT8945A_DCDC_MODE_POWER_SAVING(0)
+
#define ACT8945A_LDO1_VSET 0x50
#define ACT8945A_LDO1_CTRL 0x51
+#define ACT8945A_LDO1_SUS 0x52
#define ACT8945A_LDO2_VSET 0x54
#define ACT8945A_LDO2_CTRL 0x55
+#define ACT8945A_LDO2_SUS 0x56
#define ACT8945A_LDO3_VSET 0x60
#define ACT8945A_LDO3_CTRL 0x61
+#define ACT8945A_LDO3_SUS 0x62
#define ACT8945A_LDO4_VSET 0x64
#define ACT8945A_LDO4_CTRL 0x65
+#define ACT8945A_LDO4_SUS 0x66
/**
* Field Definitions.
@@ -63,12 +77,154 @@ enum {
ACT8945A_REG_NUM,
};
+struct act8945a_pmic {
+ struct regulator_dev *rdevs[ACT8945A_REG_NUM];
+ struct regmap *regmap;
+};
+
static const struct regulator_linear_range act8945a_voltage_ranges[] = {
REGULATOR_LINEAR_RANGE(60, 0, 23, 25000),
REGULATOR_LINEAR_RANGE(120, 24, 47, 5),
REGULATOR_LINEAR_RANGE(240, 48, 63, 10),
};
+static int act8945a_set_suspend_state(struct regulator_dev *rdev, bool enable)
+{
+ struct regmap *regmap = rdev->regmap;
+ int id = rdev->desc->id, ret, reg, val;
+
+ switch (id) {
+ case ACT8945A_ID_DCDC1:
+ reg = ACT8945A_DCDC1_SUS;
+ val = 0xa8;
+ break;
+ case ACT8945A_ID_DCDC2:
+ reg = ACT8945A_DCDC2_SUS;
+ val = 0xa8;
+ break;
+ case ACT8945A_ID_DCDC3:
+ reg = ACT8945A_DCDC3_SUS;
+ val = 0xa8;
+ break;
+ case ACT8945A_ID_LDO1:
+ reg = ACT8945A_LDO1_SUS;
+ val = 0xe8;
+ break;
+ case ACT8945A_ID_LDO2:
+ reg = ACT8945A_LDO2_SUS;
+ val = 0xe8;
+ break;
+ case ACT8945A_ID_LDO3:
+ reg = ACT8945A_LDO3_SUS;
+ val = 0xe8;
+ break;
+ case ACT8945A_ID_LDO4:
+ reg = ACT8945A_LDO4_SUS;
+ val = 0xe8;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (enable)
+ val |= BIT(4);
+
+ /*
+* Ask the PMIC to enable/disable this output when entering hibernate
+* mode.
+*/
+ ret = regmap_write(regmap, reg, val);
+ if (ret < 0)
+ return ret;
+
+ /*
+* Ask the PMIC to enter the suspend mode on the next PWRHLD
+* transition.
+*/
+ return regmap_write(regmap, ACT8945A_SYS_CTRL, 0x42);
+}
+
+static int act8945a_set_suspend_enable(struct regulator_dev *rdev)
+{
+ return act8945a_set_suspend_state(rdev, true);
+}
+
+static int act8945a_set_suspend_disable(struct regulator_dev *rdev)
+{
+ return act8945a_set_suspend_state(rdev, false);
+}
+
+static unsigned int act8945a_of_map_mode(unsigned int mode)
+{
+ if (mode == ACT8945A_DCDC_MODE_POWER_SAVING)
+ return REGULATOR_MODE_STANDBY;
+
+ return REGULATOR_MODE_NORMAL;
+}
+
+static int act8945a_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+ struct regmap *regmap = rdev->regmap;
+ int id = rdev->desc->id;
+ int reg, val;
+
+ switch (mode) {
+ case REGULATOR_MODE_STANDBY:
+ val = ACT8945A_DCDC_MODE_POWER_SAVING;
+
[PATCH 1/2] regulator: act8945: Implement PM functionalities
From: Boris Brezillon
The regulator supports a dedicated suspend mode.
Implement the appropriate ->set_suspend_xx() hooks, add support for
->set_mode(), and provide basic PM ops functionalities to setup the
regulator in a suspend state when the system is entering suspend.
We also implement the ->shutdown() method to make sure the PMIC will
not enter the suspend state when the system is shutdown.
Signed-off-by: Boris Brezillon
[claudiu.bez...@microchip.com: remove dev_pm_ops, fix checkpatch warking, adapt
commit message]
Signed-off-by: Claudiu Beznea
---
drivers/regulator/act8945a-regulator.c | 195 -
1 file changed, 193 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/act8945a-regulator.c
b/drivers/regulator/act8945a-regulator.c
index 43fda8b4455a..41bf7d224abb 100644
--- a/drivers/regulator/act8945a-regulator.c
+++ b/drivers/regulator/act8945a-regulator.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -23,23 +24,36 @@
*/
#define ACT8945A_SYS_MODE 0x00
#define ACT8945A_SYS_CTRL 0x01
+#define ACT8945A_SYS_UNLK_REGS 0x0b
#define ACT8945A_DCDC1_VSET1 0x20
#define ACT8945A_DCDC1_VSET2 0x21
#define ACT8945A_DCDC1_CTRL0x22
+#define ACT8945A_DCDC1_SUS 0x24
#define ACT8945A_DCDC2_VSET1 0x30
#define ACT8945A_DCDC2_VSET2 0x31
#define ACT8945A_DCDC2_CTRL0x32
+#define ACT8945A_DCDC2_SUS 0x34
#define ACT8945A_DCDC3_VSET1 0x40
#define ACT8945A_DCDC3_VSET2 0x41
#define ACT8945A_DCDC3_CTRL0x42
+#define ACT8945A_DCDC3_SUS 0x44
+
+#define ACT8945A_DCDC_MODE_MSK BIT(5)
+#define ACT8945A_DCDC_MODE_FIXED BIT(5)
+#define ACT8945A_DCDC_MODE_POWER_SAVING(0)
+
#define ACT8945A_LDO1_VSET 0x50
#define ACT8945A_LDO1_CTRL 0x51
+#define ACT8945A_LDO1_SUS 0x52
#define ACT8945A_LDO2_VSET 0x54
#define ACT8945A_LDO2_CTRL 0x55
+#define ACT8945A_LDO2_SUS 0x56
#define ACT8945A_LDO3_VSET 0x60
#define ACT8945A_LDO3_CTRL 0x61
+#define ACT8945A_LDO3_SUS 0x62
#define ACT8945A_LDO4_VSET 0x64
#define ACT8945A_LDO4_CTRL 0x65
+#define ACT8945A_LDO4_SUS 0x66
/**
* Field Definitions.
@@ -63,12 +77,154 @@ enum {
ACT8945A_REG_NUM,
};
+struct act8945a_pmic {
+ struct regulator_dev *rdevs[ACT8945A_REG_NUM];
+ struct regmap *regmap;
+};
+
static const struct regulator_linear_range act8945a_voltage_ranges[] = {
REGULATOR_LINEAR_RANGE(60, 0, 23, 25000),
REGULATOR_LINEAR_RANGE(120, 24, 47, 5),
REGULATOR_LINEAR_RANGE(240, 48, 63, 10),
};
+static int act8945a_set_suspend_state(struct regulator_dev *rdev, bool enable)
+{
+ struct regmap *regmap = rdev->regmap;
+ int id = rdev->desc->id, ret, reg, val;
+
+ switch (id) {
+ case ACT8945A_ID_DCDC1:
+ reg = ACT8945A_DCDC1_SUS;
+ val = 0xa8;
+ break;
+ case ACT8945A_ID_DCDC2:
+ reg = ACT8945A_DCDC2_SUS;
+ val = 0xa8;
+ break;
+ case ACT8945A_ID_DCDC3:
+ reg = ACT8945A_DCDC3_SUS;
+ val = 0xa8;
+ break;
+ case ACT8945A_ID_LDO1:
+ reg = ACT8945A_LDO1_SUS;
+ val = 0xe8;
+ break;
+ case ACT8945A_ID_LDO2:
+ reg = ACT8945A_LDO2_SUS;
+ val = 0xe8;
+ break;
+ case ACT8945A_ID_LDO3:
+ reg = ACT8945A_LDO3_SUS;
+ val = 0xe8;
+ break;
+ case ACT8945A_ID_LDO4:
+ reg = ACT8945A_LDO4_SUS;
+ val = 0xe8;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (enable)
+ val |= BIT(4);
+
+ /*
+* Ask the PMIC to enable/disable this output when entering hibernate
+* mode.
+*/
+ ret = regmap_write(regmap, reg, val);
+ if (ret < 0)
+ return ret;
+
+ /*
+* Ask the PMIC to enter the suspend mode on the next PWRHLD
+* transition.
+*/
+ return regmap_write(regmap, ACT8945A_SYS_CTRL, 0x42);
+}
+
+static int act8945a_set_suspend_enable(struct regulator_dev *rdev)
+{
+ return act8945a_set_suspend_state(rdev, true);
+}
+
+static int act8945a_set_suspend_disable(struct regulator_dev *rdev)
+{
+ return act8945a_set_suspend_state(rdev, false);
+}
+
+static unsigned int act8945a_of_map_mode(unsigned int mode)
+{
+ if (mode == ACT8945A_DCDC_MODE_POWER_SAVING)
+ return REGULATOR_MODE_STANDBY;
+
+ return REGULATOR_MODE_NORMAL;
+}
+
+static int act8945a_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+ struct regmap *regmap = rdev->regmap;
+ int id = rdev->desc->id;
+ int reg, val;
+
+ switch (mode) {
+ case REGULATOR_MODE_STANDBY:
+ val = ACT8945A_DCDC_MODE_POWER_SAVING;
+
