linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks
  2020-08-31 16:48 [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks Robin Gong
@ 2020-08-31 10:53 ` Marco Felsch
  2020-09-01  1:58   ` Robin Gong
  2020-08-31 16:48 ` [PATCH v1 2/2] ARM64: dts: imx8mp-evk: add pca9450 node Robin Gong
  1 sibling, 1 reply; 5+ messages in thread
From: Marco Felsch @ 2020-08-31 10:53 UTC (permalink / raw)
  To: Robin Gong
  Cc: robh+dt, shawnguo, s.hauer, festevam, lgirdwood, broonie,
	Anson.Huang, linux-arm-kernel, devicetree, linux-imx, kernel,
	linux-kernel

Hi Robin,

On 20-09-01 00:48, Robin Gong wrote:
> BuckX enable mode
> 00b = OFF
> 01b = ON by PMIC_ON_REQ = H
> 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L
> 11b = Always ON
> 
> For such enable mode, enable_value should be clearly set in requlator desc,
> 00/11 is not enough, correct it now for different bucks. For example, buck2
> is designed for vddarm which could be off in 'PMIC_STBY_REQ = H' after kernel
> enter suspend, so should be set '10b' as ON, while others is '01b' as ON.
> All are the same as the default setting which means bucks no need to be
> enabled again during kernel boot even if they have been enabled already after
> pmic on.

I wouldn't hard-code the regulator behaviour because the behaviour comes
from the system design which in most cases are comming from our hw-guys.
Till now I saw a few intelligent designs don't following the pmic user
recommendations to save money. I would love to specify the regulator
behaviour/mode within the dt or acpi.

> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
>  drivers/regulator/pca9450-regulator.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
> index eb5822b..79f2a5a 100644
> --- a/drivers/regulator/pca9450-regulator.c
> +++ b/drivers/regulator/pca9450-regulator.c
> @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK1OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK1CTRL,
>  			.enable_mask = BUCK1_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
>  			.vsel_mask = BUCK2OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK2CTRL,
> -			.enable_mask = BUCK1_ENMODE_MASK,
> +			.enable_mask = BUCK2_ENMODE_MASK,

Unrelated change?

> +			.enable_val = BUCK_ENMODE_ONREQ_STBYREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -299,6 +301,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK3OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK3CTRL,
>  			.enable_mask = BUCK3_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -324,6 +327,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK4OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK4CTRL,
>  			.enable_mask = BUCK4_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -342,6 +346,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK5OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK5CTRL,
>  			.enable_mask = BUCK5_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -360,6 +365,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK6OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK6CTRL,
>  			.enable_mask = BUCK6_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -475,6 +481,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_mask = BUCK1OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK1CTRL,
>  			.enable_mask = BUCK1_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -499,7 +506,8 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
>  			.vsel_mask = BUCK2OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK2CTRL,
> -			.enable_mask = BUCK1_ENMODE_MASK,
> +			.enable_mask = BUCK2_ENMODE_MASK,

Unrelated change?

Regards,
  Marco
> +			.enable_val = BUCK_ENMODE_ONREQ_STBYREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -525,6 +533,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_mask = BUCK4OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK4CTRL,
>  			.enable_mask = BUCK4_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -543,6 +552,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_mask = BUCK5OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK5CTRL,
>  			.enable_mask = BUCK5_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -561,6 +571,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_mask = BUCK6OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK6CTRL,
>  			.enable_mask = BUCK6_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks
@ 2020-08-31 16:48 Robin Gong
  2020-08-31 10:53 ` Marco Felsch
  2020-08-31 16:48 ` [PATCH v1 2/2] ARM64: dts: imx8mp-evk: add pca9450 node Robin Gong
  0 siblings, 2 replies; 5+ messages in thread
From: Robin Gong @ 2020-08-31 16:48 UTC (permalink / raw)
  To: robh+dt, shawnguo, s.hauer, festevam, lgirdwood, broonie, Anson.Huang
  Cc: kernel, linux-imx, devicetree, linux-arm-kernel, linux-kernel

BuckX enable mode
00b = OFF
01b = ON by PMIC_ON_REQ = H
10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L
11b = Always ON

For such enable mode, enable_value should be clearly set in requlator desc,
00/11 is not enough, correct it now for different bucks. For example, buck2
is designed for vddarm which could be off in 'PMIC_STBY_REQ = H' after kernel
enter suspend, so should be set '10b' as ON, while others is '01b' as ON.
All are the same as the default setting which means bucks no need to be
enabled again during kernel boot even if they have been enabled already after
pmic on.

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/regulator/pca9450-regulator.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index eb5822b..79f2a5a 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.vsel_mask = BUCK1OUT_DVS0_MASK,
 			.enable_reg = PCA9450_REG_BUCK1CTRL,
 			.enable_mask = BUCK1_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ,
 			.owner = THIS_MODULE,
 			.of_parse_cb = pca9450_set_dvs_levels,
 		},
@@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
 			.vsel_mask = BUCK2OUT_DVS0_MASK,
 			.enable_reg = PCA9450_REG_BUCK2CTRL,
-			.enable_mask = BUCK1_ENMODE_MASK,
+			.enable_mask = BUCK2_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ_STBYREQ,
 			.owner = THIS_MODULE,
 			.of_parse_cb = pca9450_set_dvs_levels,
 		},
@@ -299,6 +301,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.vsel_mask = BUCK3OUT_DVS0_MASK,
 			.enable_reg = PCA9450_REG_BUCK3CTRL,
 			.enable_mask = BUCK3_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ,
 			.owner = THIS_MODULE,
 			.of_parse_cb = pca9450_set_dvs_levels,
 		},
@@ -324,6 +327,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.vsel_mask = BUCK4OUT_MASK,
 			.enable_reg = PCA9450_REG_BUCK4CTRL,
 			.enable_mask = BUCK4_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ,
 			.owner = THIS_MODULE,
 		},
 	},
@@ -342,6 +346,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.vsel_mask = BUCK5OUT_MASK,
 			.enable_reg = PCA9450_REG_BUCK5CTRL,
 			.enable_mask = BUCK5_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ,
 			.owner = THIS_MODULE,
 		},
 	},
@@ -360,6 +365,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.vsel_mask = BUCK6OUT_MASK,
 			.enable_reg = PCA9450_REG_BUCK6CTRL,
 			.enable_mask = BUCK6_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ,
 			.owner = THIS_MODULE,
 		},
 	},
@@ -475,6 +481,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.vsel_mask = BUCK1OUT_DVS0_MASK,
 			.enable_reg = PCA9450_REG_BUCK1CTRL,
 			.enable_mask = BUCK1_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ,
 			.owner = THIS_MODULE,
 			.of_parse_cb = pca9450_set_dvs_levels,
 		},
@@ -499,7 +506,8 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
 			.vsel_mask = BUCK2OUT_DVS0_MASK,
 			.enable_reg = PCA9450_REG_BUCK2CTRL,
-			.enable_mask = BUCK1_ENMODE_MASK,
+			.enable_mask = BUCK2_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ_STBYREQ,
 			.owner = THIS_MODULE,
 			.of_parse_cb = pca9450_set_dvs_levels,
 		},
@@ -525,6 +533,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.vsel_mask = BUCK4OUT_MASK,
 			.enable_reg = PCA9450_REG_BUCK4CTRL,
 			.enable_mask = BUCK4_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ,
 			.owner = THIS_MODULE,
 		},
 	},
@@ -543,6 +552,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.vsel_mask = BUCK5OUT_MASK,
 			.enable_reg = PCA9450_REG_BUCK5CTRL,
 			.enable_mask = BUCK5_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ,
 			.owner = THIS_MODULE,
 		},
 	},
@@ -561,6 +571,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.vsel_mask = BUCK6OUT_MASK,
 			.enable_reg = PCA9450_REG_BUCK6CTRL,
 			.enable_mask = BUCK6_ENMODE_MASK,
+			.enable_val = BUCK_ENMODE_ONREQ,
 			.owner = THIS_MODULE,
 		},
 	},
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v1 2/2] ARM64: dts: imx8mp-evk: add pca9450 node
  2020-08-31 16:48 [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks Robin Gong
  2020-08-31 10:53 ` Marco Felsch
@ 2020-08-31 16:48 ` Robin Gong
  1 sibling, 0 replies; 5+ messages in thread
From: Robin Gong @ 2020-08-31 16:48 UTC (permalink / raw)
  To: robh+dt, shawnguo, s.hauer, festevam, lgirdwood, broonie, Anson.Huang
  Cc: kernel, linux-imx, devicetree, linux-arm-kernel, linux-kernel

Add pca9450c pmic for imx8mp-evk board.

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp-evk.dts | 115 +++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
index 3da1fff..8e000c1 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
@@ -66,6 +66,108 @@
 	};
 };
 
+&i2c1 {
+	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c1>;
+	status = "okay";
+
+	pmic: pca9450@25 {
+		reg = <0x25>;
+		compatible = "nxp,pca9450c";
+		/* PMIC PCA9450 PMIC_nINT GPIO1_IO3 */
+		pinctrl-0 = <&pinctrl_pmic>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <3 GPIO_ACTIVE_LOW>;
+
+		regulators {
+			buck1: BUCK1 {
+				regulator-name = "BUCK1";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <2187500>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-ramp-delay = <3125>;
+			};
+
+			buck2: BUCK2 {
+				regulator-name = "BUCK2";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <2187500>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-ramp-delay = <3125>;
+				nxp,dvs-run-voltage = <950000>;
+				nxp,dvs-standby-voltage = <850000>;
+			};
+
+			buck4: BUCK4{
+				regulator-name = "BUCK4";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			buck5: BUCK5{
+				regulator-name = "BUCK5";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			buck6: BUCK6 {
+				regulator-name = "BUCK6";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo1: LDO1 {
+				regulator-name = "LDO1";
+				regulator-min-microvolt = <1600000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo2: LDO2 {
+				regulator-name = "LDO2";
+				regulator-min-microvolt = <800000>;
+				regulator-max-microvolt = <1150000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo3: LDO3 {
+				regulator-name = "LDO3";
+				regulator-min-microvolt = <800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo4: LDO4 {
+				regulator-name = "LDO4";
+				regulator-min-microvolt = <800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo5: LDO5 {
+				regulator-name = "LDO5";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+	};
+};
+
 &i2c3 {
 	clock-frequency = <400000>;
 	pinctrl-names = "default";
@@ -152,6 +254,13 @@
 		>;
 	};
 
+	pinctrl_i2c1: i2c1grp {
+		fsl,pins = <
+			MX8MP_IOMUXC_I2C1_SCL__I2C1_SCL		0x400001c3
+			MX8MP_IOMUXC_I2C1_SDA__I2C1_SDA		0x400001c3
+		>;
+	};
+
 	pinctrl_i2c3: i2c3grp {
 		fsl,pins = <
 			MX8MP_IOMUXC_I2C3_SCL__I2C3_SCL		0x400001c3
@@ -159,6 +268,12 @@
 		>;
 	};
 
+	pinctrl_pmic: pmicirq {
+		fsl,pins = <
+			MX8MP_IOMUXC_GPIO1_IO03__GPIO1_IO03	0x41
+		>;
+	};
+
 	pinctrl_reg_usdhc2_vmmc: regusdhc2vmmc {
 		fsl,pins = <
 			MX8MP_IOMUXC_SD2_RESET_B__GPIO2_IO19	0x41
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* RE: [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks
  2020-08-31 10:53 ` Marco Felsch
@ 2020-09-01  1:58   ` Robin Gong
  2020-09-29  9:10     ` Robin Gong
  0 siblings, 1 reply; 5+ messages in thread
From: Robin Gong @ 2020-09-01  1:58 UTC (permalink / raw)
  To: Marco Felsch
  Cc: robh+dt, shawnguo, s.hauer, festevam, lgirdwood, broonie,
	Anson Huang, linux-arm-kernel, devicetree, dl-linux-imx, kernel,
	linux-kernel

On 2020/08/31 18:53 Marco Felsch <m.felsch@pengutronix.de> wrote:
> Hi Robin,
> 
> On 20-09-01 00:48, Robin Gong wrote:
> > BuckX enable mode
> > 00b = OFF
> > 01b = ON by PMIC_ON_REQ = H
> > 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L 11b = Always ON
> >
> > For such enable mode, enable_value should be clearly set in requlator
> > desc,
> > 00/11 is not enough, correct it now for different bucks. For example,
> > buck2 is designed for vddarm which could be off in 'PMIC_STBY_REQ = H'
> > after kernel enter suspend, so should be set '10b' as ON, while others is '01b'
> as ON.
> > All are the same as the default setting which means bucks no need to
> > be enabled again during kernel boot even if they have been enabled
> > already after pmic on.
> 
> I wouldn't hard-code the regulator behaviour because the behaviour comes
> from the system design which in most cases are comming from our hw-guys.
> Till now I saw a few intelligent designs don't following the pmic user
> recommendations to save money. I would love to specify the regulator
> behaviour/mode within the dt or acpi.
Well, if so the better way is moving into dts. Will implement it in v2.

> 
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > ---
> >  drivers/regulator/pca9450-regulator.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/regulator/pca9450-regulator.c
> > b/drivers/regulator/pca9450-regulator.c
> > index eb5822b..79f2a5a 100644
> > --- a/drivers/regulator/pca9450-regulator.c
> > +++ b/drivers/regulator/pca9450-regulator.c
> > @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc
> pca9450a_regulators[] = {
> >  			.vsel_mask = BUCK1OUT_DVS0_MASK,
> >  			.enable_reg = PCA9450_REG_BUCK1CTRL,
> >  			.enable_mask = BUCK1_ENMODE_MASK,
> > +			.enable_val = BUCK_ENMODE_ONREQ,
> >  			.owner = THIS_MODULE,
> >  			.of_parse_cb = pca9450_set_dvs_levels,
> >  		},
> > @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc
> pca9450a_regulators[] = {
> >  			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
> >  			.vsel_mask = BUCK2OUT_DVS0_MASK,
> >  			.enable_reg = PCA9450_REG_BUCK2CTRL,
> > -			.enable_mask = BUCK1_ENMODE_MASK,
> > +			.enable_mask = BUCK2_ENMODE_MASK,
> 
> Unrelated change?
Yes, that's just correct it minor literal since that's BUCK2 although
they're the same, will split it anyway.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks
  2020-09-01  1:58   ` Robin Gong
@ 2020-09-29  9:10     ` Robin Gong
  0 siblings, 0 replies; 5+ messages in thread
From: Robin Gong @ 2020-09-29  9:10 UTC (permalink / raw)
  To: Marco Felsch
  Cc: robh+dt, shawnguo, s.hauer, festevam, lgirdwood, broonie,
	Anson Huang, linux-arm-kernel, devicetree, dl-linux-imx, kernel,
	linux-kernel

On 2020/09/01 9:58 Robin Gong <yibin.gong@nxp.com> wrote:
> On 2020/08/31 18:53 Marco Felsch <m.felsch@pengutronix.de> wrote:
> > Hi Robin,
> >
> > On 20-09-01 00:48, Robin Gong wrote:
> > > BuckX enable mode
> > > 00b = OFF
> > > 01b = ON by PMIC_ON_REQ = H
> > > 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L 11b = Always ON
> > >
> > > For such enable mode, enable_value should be clearly set in
> > > requlator desc,
> > > 00/11 is not enough, correct it now for different bucks. For
> > > example,
> > > buck2 is designed for vddarm which could be off in 'PMIC_STBY_REQ = H'
> > > after kernel enter suspend, so should be set '10b' as ON, while others is
> '01b'
> > as ON.
> > > All are the same as the default setting which means bucks no need to
> > > be enabled again during kernel boot even if they have been enabled
> > > already after pmic on.
> >
> > I wouldn't hard-code the regulator behaviour because the behaviour
> > comes from the system design which in most cases are comming from our
> hw-guys.
> > Till now I saw a few intelligent designs don't following the pmic user
> > recommendations to save money. I would love to specify the regulator
> > behaviour/mode within the dt or acpi.
> Well, if so the better way is moving into dts. Will implement it in v2.
Hi Marco,
	Sorry, I'm afraid that big changes have to be made for moving it to dts, because
current pca9450 driver is using common regulator_enable_regmap() which means
rdev->desc is const and 'enable_value' can't be changed runtime with the value in
dts. Regards to pca9450 is the specific pmic designed for i.mx8m family rather than
generic pmic, the power rails defining should be the same as NXP MEK reference
board, especially for the BUCKs which match well with SOC's power. So may I leave it
at this time, and pick it up if the 'unbelievable trouble' really come out in the future?   

> 
> >
> > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > ---
> > >  drivers/regulator/pca9450-regulator.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/regulator/pca9450-regulator.c
> > > b/drivers/regulator/pca9450-regulator.c
> > > index eb5822b..79f2a5a 100644
> > > --- a/drivers/regulator/pca9450-regulator.c
> > > +++ b/drivers/regulator/pca9450-regulator.c
> > > @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc
> > pca9450a_regulators[] = {
> > >  			.vsel_mask = BUCK1OUT_DVS0_MASK,
> > >  			.enable_reg = PCA9450_REG_BUCK1CTRL,
> > >  			.enable_mask = BUCK1_ENMODE_MASK,
> > > +			.enable_val = BUCK_ENMODE_ONREQ,
> > >  			.owner = THIS_MODULE,
> > >  			.of_parse_cb = pca9450_set_dvs_levels,
> > >  		},
> > > @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc
> > pca9450a_regulators[] = {
> > >  			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
> > >  			.vsel_mask = BUCK2OUT_DVS0_MASK,
> > >  			.enable_reg = PCA9450_REG_BUCK2CTRL,
> > > -			.enable_mask = BUCK1_ENMODE_MASK,
> > > +			.enable_mask = BUCK2_ENMODE_MASK,
> >
> > Unrelated change?
> Yes, that's just correct it minor literal since that's BUCK2 although they're the
> same, will split it anyway.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-09-29  9:10 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-31 16:48 [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks Robin Gong
2020-08-31 10:53 ` Marco Felsch
2020-09-01  1:58   ` Robin Gong
2020-09-29  9:10     ` Robin Gong
2020-08-31 16:48 ` [PATCH v1 2/2] ARM64: dts: imx8mp-evk: add pca9450 node Robin Gong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).