[RFC 0/4] regulator: lp87565: ignore ENx pins and add LP87524-Q1

[RFC 0/4] regulator: lp87565: ignore ENx pins and add LP87524-Q1

[Luca Ceresoli]

Hi,

the first patch in this series is a small but significant variation in how
the lp87565 driver enables the output rails, allow the kernel to always
know when it is enabling an output. However it can change existing
behaviour (depending on the hardware setup) and thus it should be carefully
evaluated.

The following patches are a fairly straightforward addition of a new chip
variant along with some DT bindings cleanup.

Luca

Luca Ceresoli (4):
  regulator: lp87565: enable voltage regardless of ENx pin
  regulator: lp87565: dt: remove duplicated section
  regulator: lp87565: dt: add LP87524-Q1 variant
  regulator: lp87565: add LP87524-Q1 variant

 .../devicetree/bindings/mfd/lp87565.txt       | 66 ++++++++++++++-----
 drivers/mfd/lp87565.c                         |  4 ++
 drivers/regulator/lp87565-regulator.c         | 21 +++++-
 include/linux/mfd/lp87565.h                   |  1 +
 4 files changed, 73 insertions(+), 19 deletions(-)

-- 
2.27.0

[RFC 1/4] regulator: lp87565: enable voltage regardless of ENx pin

[Luca Ceresoli]

This driver enables outputs by setting bit EN_BUCKn in the BUCKn_CTRL1
register. However, if bit EN_PIN_CTRLn in the same register is set, the
output is actually enabled only if EN_BUCKn is set AND an enable pin is
active. Since the driver does not touch EN_PIN_CTRLn, the choice is left to
the hardware, which in turn gets this bit from OTP memory, and in absence
of OTP data it uses a default value that is documented in the datasheet for
LP8752x, but not for LP8756x.

Thus the driver doesn't really "know" whether it is actually enabling the
output or not.

In order to make sure activation is always driver-controlled, just clear
the EN_PIN_CTRLn bit. Now all activation solely depend on the EN_BUCKn bit.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

This is an RFC because there is a potential regression on existing
hardware. If there are boards where the enable pins _are_ used to toggle
power on/off in addition to I2C control, then this patch would make these
board ignore the enable pin as if it were always active.

Whether setups with ENx pins are used or not is hard to say. A Linux system
that needs to set a register _and_ to move a GPIO in order to enable a
regulatore output seems to me a bit weird, but there might be sound use
cases that I ignore.

I suspect the only solution that allows to configure the EN_PIN_CTRLn bits
correctly in all the possible hardware setups would be to tell in device
tree / board info whether each enable pin is connected or not (which is a
hardware _fact_) and which ENx pin should control which regulator output
(which is a policy). But it would make this simple driver considerably more
complex.

Any suggestion about the correct way to handle this situation would be
greatly appreciated.
---
 drivers/regulator/lp87565-regulator.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/lp87565-regulator.c b/drivers/regulator/lp87565-regulator.c
index 5d525dacf959..fbed6bc80c1a 100644
--- a/drivers/regulator/lp87565-regulator.c
+++ b/drivers/regulator/lp87565-regulator.c
@@ -11,8 +11,8 @@
 
 #include <linux/mfd/lp87565.h>
 
-#define LP87565_REGULATOR(_name, _id, _of, _ops, _n, _vr, _vm, _er, _em, \
-			 _delay, _lr, _cr)				\
+#define LP87565_REGULATOR(_name, _id, _of, _ops, _n, _vr, _vm,		\
+			  _er, _em, _ev, _delay, _lr, _cr)		\
 	[_id] = {							\
 		.desc = {						\
 			.name			= _name,		\
@@ -28,6 +28,7 @@
 			.vsel_mask		= _vm,			\
 			.enable_reg		= _er,			\
 			.enable_mask		= _em,			\
+			.enable_val		= _ev,			\
 			.ramp_delay		= _delay,		\
 			.linear_ranges		= _lr,			\
 			.n_linear_ranges	= ARRAY_SIZE(_lr),	\
@@ -121,38 +122,54 @@ static const struct lp87565_regulator regulators[] = {
 	LP87565_REGULATOR("BUCK0", LP87565_BUCK_0, "buck0", lp87565_buck_ops,
 			  256, LP87565_REG_BUCK0_VOUT, LP87565_BUCK_VSET,
 			  LP87565_REG_BUCK0_CTRL_1,
+			  LP87565_BUCK_CTRL_1_EN |
+			  LP87565_BUCK_CTRL_1_EN_PIN_CTRL,
 			  LP87565_BUCK_CTRL_1_EN, 3230,
 			  buck0_1_2_3_ranges, LP87565_REG_BUCK0_CTRL_2),
 	LP87565_REGULATOR("BUCK1", LP87565_BUCK_1, "buck1", lp87565_buck_ops,
 			  256, LP87565_REG_BUCK1_VOUT, LP87565_BUCK_VSET,
 			  LP87565_REG_BUCK1_CTRL_1,
+			  LP87565_BUCK_CTRL_1_EN |
+			  LP87565_BUCK_CTRL_1_EN_PIN_CTRL,
 			  LP87565_BUCK_CTRL_1_EN, 3230,
 			  buck0_1_2_3_ranges, LP87565_REG_BUCK1_CTRL_2),
 	LP87565_REGULATOR("BUCK2", LP87565_BUCK_2, "buck2", lp87565_buck_ops,
 			  256, LP87565_REG_BUCK2_VOUT, LP87565_BUCK_VSET,
 			  LP87565_REG_BUCK2_CTRL_1,
+			  LP87565_BUCK_CTRL_1_EN |
+			  LP87565_BUCK_CTRL_1_EN_PIN_CTRL,
 			  LP87565_BUCK_CTRL_1_EN, 3230,
 			  buck0_1_2_3_ranges, LP87565_REG_BUCK2_CTRL_2),
 	LP87565_REGULATOR("BUCK3", LP87565_BUCK_3, "buck3", lp87565_buck_ops,
 			  256, LP87565_REG_BUCK3_VOUT, LP87565_BUCK_VSET,
 			  LP87565_REG_BUCK3_CTRL_1,
+			  LP87565_BUCK_CTRL_1_EN |
+			  LP87565_BUCK_CTRL_1_EN_PIN_CTRL,
 			  LP87565_BUCK_CTRL_1_EN, 3230,
 			  buck0_1_2_3_ranges, LP87565_REG_BUCK3_CTRL_2),
 	LP87565_REGULATOR("BUCK10", LP87565_BUCK_10, "buck10", lp87565_buck_ops,
 			  256, LP87565_REG_BUCK0_VOUT, LP87565_BUCK_VSET,
 			  LP87565_REG_BUCK0_CTRL_1,
 			  LP87565_BUCK_CTRL_1_EN |
+			  LP87565_BUCK_CTRL_1_EN_PIN_CTRL |
+			  LP87565_BUCK_CTRL_1_FPWM_MP_0_2,
+			  LP87565_BUCK_CTRL_1_EN |
 			  LP87565_BUCK_CTRL_1_FPWM_MP_0_2, 3230,
 			  buck0_1_2_3_ranges, LP87565_REG_BUCK0_CTRL_2),
 	LP87565_REGULATOR("BUCK23", LP87565_BUCK_23, "buck23", lp87565_buck_ops,
 			  256, LP87565_REG_BUCK2_VOUT, LP87565_BUCK_VSET,
 			  LP87565_REG_BUCK2_CTRL_1,
+			  LP87565_BUCK_CTRL_1_EN |
+			  LP87565_BUCK_CTRL_1_EN_PIN_CTRL,
 			  LP87565_BUCK_CTRL_1_EN, 3230,
 			  buck0_1_2_3_ranges, LP87565_REG_BUCK2_CTRL_2),
 	LP87565_REGULATOR("BUCK3210", LP87565_BUCK_3210, "buck3210",
 			  lp87565_buck_ops, 256, LP87565_REG_BUCK0_VOUT,
 			  LP87565_BUCK_VSET, LP87565_REG_BUCK0_CTRL_1,
 			  LP87565_BUCK_CTRL_1_EN |
+			  LP87565_BUCK_CTRL_1_EN_PIN_CTRL |
+			  LP87565_BUCK_CTRL_1_FPWM_MP_0_2,
+			  LP87565_BUCK_CTRL_1_EN |
 			  LP87565_BUCK_CTRL_1_FPWM_MP_0_2, 3230,
 			  buck0_1_2_3_ranges, LP87565_REG_BUCK0_CTRL_2),
 };
-- 
2.27.0

[RFC 2/4] regulator: lp87565: dt: remove duplicated section

[Luca Ceresoli]

The "Required properties:" section is copied verbatim for each of the two
supported chips. In preparation to add a new chip variant make it a common
section and keep the two examples to differentiate between the two chips.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 .../devicetree/bindings/mfd/lp87565.txt       | 21 ++++---------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/lp87565.txt b/Documentation/devicetree/bindings/mfd/lp87565.txt
index 41671e0dc26b..b75ae23a1ef3 100644
--- a/Documentation/devicetree/bindings/mfd/lp87565.txt
+++ b/Documentation/devicetree/bindings/mfd/lp87565.txt
@@ -1,7 +1,7 @@
 TI LP87565 PMIC MFD driver
 
 Required properties:
-  - compatible:	"ti,lp87565", "ti,lp87565-q1"
+  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1"
   - reg:		I2C slave address.
   - gpio-controller:	Marks the device node as a GPIO Controller.
   - #gpio-cells:	Should be two.  The first cell is the pin number and
@@ -10,7 +10,8 @@ Required properties:
   - xxx-in-supply:	Phandle to parent supply node of each regulator
 			populated under regulators node. xxx should match
 			the supply_name populated in driver.
-Example:
+
+Example for the TI LP87565-Q1 PMIC (dual 2-phase output configuration):
 
 lp87565_pmic: pmic@60 {
 	compatible = "ti,lp87565-q1";
@@ -42,21 +43,7 @@ lp87565_pmic: pmic@60 {
 	};
 };
 
-TI LP87561 PMIC:
-
-This is a single output 4-phase regulator configuration
-
-Required properties:
-  - compatible:	"ti,lp87561-q1"
-  - reg:		I2C slave address.
-  - gpio-controller:	Marks the device node as a GPIO Controller.
-  - #gpio-cells:	Should be two.  The first cell is the pin number and
-			the second cell is used to specify flags.
-			See ../gpio/gpio.txt for more information.
-  - xxx-in-supply:	Phandle to parent supply node of each regulator
-			populated under regulators node. xxx should match
-			the supply_name populated in driver.
-Example:
+Example for the TI LP87561 PMIC (single 4-phase output configuration):
 
 lp87561_pmic: pmic@62 {
 	compatible = "ti,lp87561-q1";
-- 
2.27.0

[RFC 3/4] regulator: lp87565: dt: add LP87524-Q1 variant

[Luca Ceresoli]

Add the LP87524-Q1 to the lp87565 bindings document along with an example.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 .../devicetree/bindings/mfd/lp87565.txt       | 47 ++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/lp87565.txt b/Documentation/devicetree/bindings/mfd/lp87565.txt
index b75ae23a1ef3..839eac6b75c2 100644
--- a/Documentation/devicetree/bindings/mfd/lp87565.txt
+++ b/Documentation/devicetree/bindings/mfd/lp87565.txt
@@ -1,7 +1,8 @@
 TI LP87565 PMIC MFD driver
 
 Required properties:
-  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1"
+  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1",
+			"ti,lp87524-q1"
   - reg:		I2C slave address.
   - gpio-controller:	Marks the device node as a GPIO Controller.
   - #gpio-cells:	Should be two.  The first cell is the pin number and
@@ -64,3 +65,47 @@ lp87561_pmic: pmic@62 {
 		};
 	};
 };
+
+Example for the TI LP87524-Q1 PMIC (four 1-phase output configuration):
+
+lp87524_pmic: pmic@60 {
+	compatible = "ti,lp87524-q1";
+	reg = <0x60>;
+	gpio-controller;
+	#gpio-cells = <2>;
+
+	buck0-in-supply = <&vdd_5v0>;
+	buck1-in-supply = <&vdd_5v0>;
+	buck2-in-supply = <&vdd_5v0>;
+	buck3-in-supply = <&vdd_5v0>;
+
+	regulators {
+		buck0_reg: buck0 {
+			regulator-name = "buck0";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			regulator-always-on;
+		};
+
+		buck1_reg: buck1 {
+			regulator-name = "buck1";
+			regulator-min-microvolt = <1350000>;
+			regulator-max-microvolt = <1350000>;
+			regulator-always-on;
+		};
+
+		buck2_reg: buck2 {
+			regulator-name = "buck2";
+			regulator-min-microvolt = <950000>;
+			regulator-max-microvolt = <950000>;
+			regulator-always-on;
+		};
+
+		buck3_reg: buck3 {
+			regulator-name = "buck3";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-always-on;
+		};
+	};
+};
-- 
2.27.0

[RFC 4/4] regulator: lp87565: add LP87524-Q1 variant

[Luca Ceresoli]

Add support for the LP87524B/J/P-Q1 Four 4-MHz Buck Converter. This is a
variant of the LP87565 having 4 single-phase outputs and up to 10 A of
total output current.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/mfd/lp87565.c       | 4 ++++
 include/linux/mfd/lp87565.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/mfd/lp87565.c b/drivers/mfd/lp87565.c
index 4a5c8ade4ae0..cc1072927f6d 100644
--- a/drivers/mfd/lp87565.c
+++ b/drivers/mfd/lp87565.c
@@ -26,6 +26,10 @@ static const struct mfd_cell lp87565_cells[] = {
 
 static const struct of_device_id of_lp87565_match_table[] = {
 	{ .compatible = "ti,lp87565", },
+	{
+		.compatible = "ti,lp87524-q1",
+		.data = (void *)LP87565_DEVICE_TYPE_LP87524_Q1,
+	},
 	{
 		.compatible = "ti,lp87565-q1",
 		.data = (void *)LP87565_DEVICE_TYPE_LP87565_Q1,
diff --git a/include/linux/mfd/lp87565.h b/include/linux/mfd/lp87565.h
index ce965354bbad..ad240f2d0d3f 100644
--- a/include/linux/mfd/lp87565.h
+++ b/include/linux/mfd/lp87565.h
@@ -14,6 +14,7 @@
 
 enum lp87565_device_type {
 	LP87565_DEVICE_TYPE_UNKNOWN	= 0,
+	LP87565_DEVICE_TYPE_LP87524_Q1,
 	LP87565_DEVICE_TYPE_LP87561_Q1,
 	LP87565_DEVICE_TYPE_LP87565_Q1,
 };
-- 
2.27.0

Re: [RFC 3/4] regulator: lp87565: dt: add LP87524-Q1 variant

[Lee Jones]

On Wed, 03 Jun 2020, Luca Ceresoli wrote:

> Add the LP87524-Q1 to the lp87565 bindings document along with an example.

Keep the way to describe model numbers consistent.

I suggest LP87565 here (even though the filename is in lower case).

> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> ---
>  .../devicetree/bindings/mfd/lp87565.txt       | 47 ++++++++++++++++++-

This is an MFD patch.

Please change the patch subject to reflect that.

>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/lp87565.txt b/Documentation/devicetree/bindings/mfd/lp87565.txt
> index b75ae23a1ef3..839eac6b75c2 100644
> --- a/Documentation/devicetree/bindings/mfd/lp87565.txt
> +++ b/Documentation/devicetree/bindings/mfd/lp87565.txt
> @@ -1,7 +1,8 @@
>  TI LP87565 PMIC MFD driver
>  
>  Required properties:
> -  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1"
> +  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1",
> +			"ti,lp87524-q1"

Maybe have 2 per line.  It would really help with my OCD!

>    - reg:		I2C slave address.
>    - gpio-controller:	Marks the device node as a GPIO Controller.
>    - #gpio-cells:	Should be two.  The first cell is the pin number and
> @@ -64,3 +65,47 @@ lp87561_pmic: pmic@62 {
>  		};
>  	};
>  };
> +
> +Example for the TI LP87524-Q1 PMIC (four 1-phase output configuration):
> +
> +lp87524_pmic: pmic@60 {
> +	compatible = "ti,lp87524-q1";
> +	reg = <0x60>;
> +	gpio-controller;
> +	#gpio-cells = <2>;
> +
> +	buck0-in-supply = <&vdd_5v0>;
> +	buck1-in-supply = <&vdd_5v0>;
> +	buck2-in-supply = <&vdd_5v0>;
> +	buck3-in-supply = <&vdd_5v0>;
> +
> +	regulators {
> +		buck0_reg: buck0 {
> +			regulator-name = "buck0";
> +			regulator-min-microvolt = <3300000>;
> +			regulator-max-microvolt = <3300000>;
> +			regulator-always-on;
> +		};
> +
> +		buck1_reg: buck1 {
> +			regulator-name = "buck1";
> +			regulator-min-microvolt = <1350000>;
> +			regulator-max-microvolt = <1350000>;
> +			regulator-always-on;
> +		};
> +
> +		buck2_reg: buck2 {
> +			regulator-name = "buck2";
> +			regulator-min-microvolt = <950000>;
> +			regulator-max-microvolt = <950000>;
> +			regulator-always-on;
> +		};
> +
> +		buck3_reg: buck3 {
> +			regulator-name = "buck3";
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +			regulator-always-on;
> +		};
> +	};
> +};

Patch looks okay, but Rob or Mark must review too.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC 4/4] regulator: lp87565: add LP87524-Q1 variant

[Lee Jones]

On Wed, 03 Jun 2020, Luca Ceresoli wrote:

> Add support for the LP87524B/J/P-Q1 Four 4-MHz Buck Converter. This is a
> variant of the LP87565 having 4 single-phase outputs and up to 10 A of
> total output current.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> ---
>  drivers/mfd/lp87565.c       | 4 ++++
>  include/linux/mfd/lp87565.h | 1 +

Again, this is an MFD patch.  Please change the subject line.

>  2 files changed, 5 insertions(+)

Once changed, please re-submit with my:

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC 2/4] regulator: lp87565: dt: remove duplicated section

[Lee Jones]

On Wed, 03 Jun 2020, Luca Ceresoli wrote:

> The "Required properties:" section is copied verbatim for each of the two
> supported chips. In preparation to add a new chip variant make it a common
> section and keep the two examples to differentiate between the two chips.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> ---
>  .../devicetree/bindings/mfd/lp87565.txt       | 21 ++++---------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/lp87565.txt b/Documentation/devicetree/bindings/mfd/lp87565.txt
> index 41671e0dc26b..b75ae23a1ef3 100644
> --- a/Documentation/devicetree/bindings/mfd/lp87565.txt
> +++ b/Documentation/devicetree/bindings/mfd/lp87565.txt
> @@ -1,7 +1,7 @@
>  TI LP87565 PMIC MFD driver
>  
>  Required properties:
> -  - compatible:	"ti,lp87565", "ti,lp87565-q1"
> +  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1"

What happened to your tabbing here?

All the other entries start with a capital letter.  So should this.

>    - reg:		I2C slave address.
>    - gpio-controller:	Marks the device node as a GPIO Controller.
>    - #gpio-cells:	Should be two.  The first cell is the pin number and
> @@ -10,7 +10,8 @@ Required properties:
>    - xxx-in-supply:	Phandle to parent supply node of each regulator
>  			populated under regulators node. xxx should match
>  			the supply_name populated in driver.
> -Example:
> +
> +Example for the TI LP87565-Q1 PMIC (dual 2-phase output configuration):
>  
>  lp87565_pmic: pmic@60 {
>  	compatible = "ti,lp87565-q1";
> @@ -42,21 +43,7 @@ lp87565_pmic: pmic@60 {
>  	};
>  };
>  
> -TI LP87561 PMIC:
> -
> -This is a single output 4-phase regulator configuration
> -
> -Required properties:
> -  - compatible:	"ti,lp87561-q1"
> -  - reg:		I2C slave address.
> -  - gpio-controller:	Marks the device node as a GPIO Controller.
> -  - #gpio-cells:	Should be two.  The first cell is the pin number and
> -			the second cell is used to specify flags.
> -			See ../gpio/gpio.txt for more information.
> -  - xxx-in-supply:	Phandle to parent supply node of each regulator
> -			populated under regulators node. xxx should match
> -			the supply_name populated in driver.
> -Example:
> +Example for the TI LP87561 PMIC (single 4-phase output configuration):
>  
>  lp87561_pmic: pmic@62 {
>  	compatible = "ti,lp87561-q1";

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC 2/4] regulator: lp87565: dt: remove duplicated section

[Lee Jones]

On Thu, 04 Jun 2020, Lee Jones wrote:

> On Wed, 03 Jun 2020, Luca Ceresoli wrote:
> 
> > The "Required properties:" section is copied verbatim for each of the two
> > supported chips. In preparation to add a new chip variant make it a common
> > section and keep the two examples to differentiate between the two chips.
> > 
> > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> > ---
> >  .../devicetree/bindings/mfd/lp87565.txt       | 21 ++++---------------
> >  1 file changed, 4 insertions(+), 17 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/lp87565.txt b/Documentation/devicetree/bindings/mfd/lp87565.txt
> > index 41671e0dc26b..b75ae23a1ef3 100644
> > --- a/Documentation/devicetree/bindings/mfd/lp87565.txt
> > +++ b/Documentation/devicetree/bindings/mfd/lp87565.txt
> > @@ -1,7 +1,7 @@
> >  TI LP87565 PMIC MFD driver
> >  
> >  Required properties:
> > -  - compatible:	"ti,lp87565", "ti,lp87565-q1"
> > +  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1"
> 
> What happened to your tabbing here?
> 
> All the other entries start with a capital letter.  So should this.

This is also an MFD patch.  Please change the subject line.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC 3/4] regulator: lp87565: dt: add LP87524-Q1 variant

[Luca Ceresoli]

Hi Lee,

On 04/06/20 08:01, Lee Jones wrote:
> On Wed, 03 Jun 2020, Luca Ceresoli wrote:
> 
>> Add the LP87524-Q1 to the lp87565 bindings document along with an example.
> 
> Keep the way to describe model numbers consistent.
> 
> I suggest LP87565 here (even though the filename is in lower case).

Ok.

>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>> ---
>>  .../devicetree/bindings/mfd/lp87565.txt       | 47 ++++++++++++++++++-
> 
> This is an MFD patch.
> 
> Please change the patch subject to reflect that.

Done that here and in patch 2 as well, and also changed from "dt:" to a
"dt-bindings:" prefix. This will be in v2:

  dt-bindings: mfd: lp87565: add LP87524-Q1 variant

>>  1 file changed, 46 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/lp87565.txt b/Documentation/devicetree/bindings/mfd/lp87565.txt
>> index b75ae23a1ef3..839eac6b75c2 100644
>> --- a/Documentation/devicetree/bindings/mfd/lp87565.txt
>> +++ b/Documentation/devicetree/bindings/mfd/lp87565.txt
>> @@ -1,7 +1,8 @@
>>  TI LP87565 PMIC MFD driver
>>  
>>  Required properties:
>> -  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1"
>> +  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1",
>> +			"ti,lp87524-q1"
> 
> Maybe have 2 per line.  It would really help with my OCD!

Ok.

-- 
Luca

Re: [RFC 2/4] regulator: lp87565: dt: remove duplicated section

[Luca Ceresoli]

Hi Lee,

thanks for the very prompt review!

On 04/06/20 08:47, Lee Jones wrote:
> On Wed, 03 Jun 2020, Luca Ceresoli wrote:
> 
>> The "Required properties:" section is copied verbatim for each of the two
>> supported chips. In preparation to add a new chip variant make it a common
>> section and keep the two examples to differentiate between the two chips.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>> ---
>>  .../devicetree/bindings/mfd/lp87565.txt       | 21 ++++---------------
>>  1 file changed, 4 insertions(+), 17 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/lp87565.txt b/Documentation/devicetree/bindings/mfd/lp87565.txt
>> index 41671e0dc26b..b75ae23a1ef3 100644
>> --- a/Documentation/devicetree/bindings/mfd/lp87565.txt
>> +++ b/Documentation/devicetree/bindings/mfd/lp87565.txt
>> @@ -1,7 +1,7 @@
>>  TI LP87565 PMIC MFD driver
>>  
>>  Required properties:
>> -  - compatible:	"ti,lp87565", "ti,lp87565-q1"
>> +  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1"
> 
> What happened to your tabbing here?

Looks weird, but I actually fixed it. The patch format is misleading, it
you look at the file you'll see there's a missing tab there before the
patch. Hm, guess I should have mentioned it in the commit message, will
do in v2.

> All the other entries start with a capital letter.  So should this.

Right.

-- 
Luca

Re: [RFC 4/4] regulator: lp87565: add LP87524-Q1 variant

[Luca Ceresoli]

Hi Lee,

On 04/06/20 08:44, Lee Jones wrote:
> On Wed, 03 Jun 2020, Luca Ceresoli wrote:
> 
>> Add support for the LP87524B/J/P-Q1 Four 4-MHz Buck Converter. This is a
>> variant of the LP87565 having 4 single-phase outputs and up to 10 A of
>> total output current.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>> ---
>>  drivers/mfd/lp87565.c       | 4 ++++
>>  include/linux/mfd/lp87565.h | 1 +
> 
> Again, this is an MFD patch.  Please change the subject line.

Right.

>>  2 files changed, 5 insertions(+)
> 
> Once changed, please re-submit with my:
> 
> For my own reference (apply this as-is to your sign-off block):
> 
>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

Good, thank you.
-- 
Luca

Re: [RFC 1/4] regulator: lp87565: enable voltage regardless of ENx pin

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

On Wed, Jun 03, 2020 at 10:03:16PM +0200, Luca Ceresoli wrote:

> I suspect the only solution that allows to configure the EN_PIN_CTRLn bits
> correctly in all the possible hardware setups would be to tell in device
> tree / board info whether each enable pin is connected or not (which is a
> hardware _fact_) and which ENx pin should control which regulator output
> (which is a policy). But it would make this simple driver considerably more
> complex.

> Any suggestion about the correct way to handle this situation would be
> greatly appreciated.

We can tell if we've got a software controlled GPIO connected, if we
have then we should ensure that it continues to take effect.  That
should just be a single register write at startup from the sounds of it.
Otherwise yeah, just ignoring that there's a possibility of a GPIO we
don't know about seems sensible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 1/4] regulator: lp87565: enable voltage regardless of ENx pin

[Luca Ceresoli]

Hi Mark,

On 04/06/20 13:07, Mark Brown wrote:
> On Wed, Jun 03, 2020 at 10:03:16PM +0200, Luca Ceresoli wrote:
> 
>> I suspect the only solution that allows to configure the EN_PIN_CTRLn bits
>> correctly in all the possible hardware setups would be to tell in device
>> tree / board info whether each enable pin is connected or not (which is a
>> hardware _fact_) and which ENx pin should control which regulator output
>> (which is a policy). But it would make this simple driver considerably more
>> complex.
> 
>> Any suggestion about the correct way to handle this situation would be
>> greatly appreciated.
> 
> We can tell if we've got a software controlled GPIO connected, if we
> have then we should ensure that it continues to take effect.

Ideally yes, but it would be made more complex by the chip flexibility:
it's possible to choose which enable pin should drive each output.

For example this configuration is supported by the chip:

- BUCK0 is on if EN_BUCK0 high AND pin EN0 is active
- BUCK1 is on if EN_BUCK1 high AND pin EN0 is active
- BUCK2 is on if EN_BUCK2 high AND pin EN1 is active
- BUCK3 is on if EN_BUCK3 high (no pin used)
- pin EN2 is used as a GPIO (LP875xx acts as an I2C GPIO expander)

So it would be absolutely OK to describe in DT that EN0 and EN1 connect
the SoC to the LP875xx and that EN2 is connected to something else.

But describing in DT the association between enable pins and buck
outputs would be more a configuration than hardware description IMO.

And I'm not even considering the case where the enable pins are be
connected to something else, out of the SoC control, but still meant to
be used to control the buck output.

> That
> should just be a single register write at startup from the sounds of it.

Exactly, each buck output has a register containing the bits involved in
this discussion.

> Otherwise yeah, just ignoring that there's a possibility of a GPIO we
> don't know about seems sensible.

Thanks,
-- 
Luca

Re: [RFC 2/4] regulator: lp87565: dt: remove duplicated section

[Rob Herring]

On Wed, Jun 03, 2020 at 10:03:17PM +0200, Luca Ceresoli wrote:
> The "Required properties:" section is copied verbatim for each of the two
> supported chips. In preparation to add a new chip variant make it a common
> section and keep the two examples to differentiate between the two chips.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> ---
>  .../devicetree/bindings/mfd/lp87565.txt       | 21 ++++---------------
>  1 file changed, 4 insertions(+), 17 deletions(-)

If you want to clean this up, can you convert it to DT schema?

> 
> diff --git a/Documentation/devicetree/bindings/mfd/lp87565.txt b/Documentation/devicetree/bindings/mfd/lp87565.txt
> index 41671e0dc26b..b75ae23a1ef3 100644
> --- a/Documentation/devicetree/bindings/mfd/lp87565.txt
> +++ b/Documentation/devicetree/bindings/mfd/lp87565.txt
> @@ -1,7 +1,7 @@
>  TI LP87565 PMIC MFD driver
>  
>  Required properties:
> -  - compatible:	"ti,lp87565", "ti,lp87565-q1"
> +  - compatible:		one of "ti,lp87565", "ti,lp87565-q1", "ti,lp87561-q1"
>    - reg:		I2C slave address.
>    - gpio-controller:	Marks the device node as a GPIO Controller.
>    - #gpio-cells:	Should be two.  The first cell is the pin number and
> @@ -10,7 +10,8 @@ Required properties:
>    - xxx-in-supply:	Phandle to parent supply node of each regulator
>  			populated under regulators node. xxx should match
>  			the supply_name populated in driver.
> -Example:
> +
> +Example for the TI LP87565-Q1 PMIC (dual 2-phase output configuration):
>  
>  lp87565_pmic: pmic@60 {
>  	compatible = "ti,lp87565-q1";
> @@ -42,21 +43,7 @@ lp87565_pmic: pmic@60 {
>  	};
>  };
>  
> -TI LP87561 PMIC:
> -
> -This is a single output 4-phase regulator configuration
> -
> -Required properties:
> -  - compatible:	"ti,lp87561-q1"
> -  - reg:		I2C slave address.
> -  - gpio-controller:	Marks the device node as a GPIO Controller.
> -  - #gpio-cells:	Should be two.  The first cell is the pin number and
> -			the second cell is used to specify flags.
> -			See ../gpio/gpio.txt for more information.
> -  - xxx-in-supply:	Phandle to parent supply node of each regulator
> -			populated under regulators node. xxx should match
> -			the supply_name populated in driver.
> -Example:
> +Example for the TI LP87561 PMIC (single 4-phase output configuration):
>  
>  lp87561_pmic: pmic@62 {
>  	compatible = "ti,lp87561-q1";
> -- 
> 2.27.0
> 

Re: [RFC 2/4] regulator: lp87565: dt: remove duplicated section

[Luca Ceresoli]

Hi Rob, Keerthy,

On 13/06/20 00:19, Rob Herring wrote:
> On Wed, Jun 03, 2020 at 10:03:17PM +0200, Luca Ceresoli wrote:
>> The "Required properties:" section is copied verbatim for each of the two
>> supported chips. In preparation to add a new chip variant make it a common
>> section and keep the two examples to differentiate between the two chips.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>> ---
>>  .../devicetree/bindings/mfd/lp87565.txt       | 21 ++++---------------
>>  1 file changed, 4 insertions(+), 17 deletions(-)
> 
> If you want to clean this up, can you convert it to DT schema?

Sure, no problem. My only question is who should I set in the
"maintainers" property.

Keerty, as the original author and TI employee, you surely know this
chip series much better than I do. Would you like to be the maintainer
for this binding document? Otherwise I can do it "best effort".

Regards,
-- 
Luca

Re: [RFC 2/4] regulator: lp87565: dt: remove duplicated section

[J, KEERTHY]

On 6/15/2020 1:30 AM, Luca Ceresoli wrote:
> Hi Rob, Keerthy,
> 
> On 13/06/20 00:19, Rob Herring wrote:
>> On Wed, Jun 03, 2020 at 10:03:17PM +0200, Luca Ceresoli wrote:
>>> The "Required properties:" section is copied verbatim for each of the two
>>> supported chips. In preparation to add a new chip variant make it a common
>>> section and keep the two examples to differentiate between the two chips.
>>>
>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>>> ---
>>>   .../devicetree/bindings/mfd/lp87565.txt       | 21 ++++---------------
>>>   1 file changed, 4 insertions(+), 17 deletions(-)
>>
>> If you want to clean this up, can you convert it to DT schema?
> 
> Sure, no problem. My only question is who should I set in the
> "maintainers" property.
> 
> Keerty, as the original author and TI employee, you surely know this
> chip series much better than I do. Would you like to be the maintainer
> for this binding document? Otherwise I can do it "best effort".

Hi Luca,

You can add me as the maintainer.

Regards,
Keerthy
> 
> Regards,
>