[PATCH v2 0/2] Re-Enable support to disable switch regulators

[PATCH v2 0/2] Re-Enable support to disable switch regulators

[Marco Felsch]

Hi,

Anson had added the support to disable the switched regulators, but
there were regressions [1] with old dtb's, so the commit was reverted [2].
At all, the support to disable the switch regulators seems to me to be a
good feature. But we have to add a special dt-property to avoid
regressions with older kernels. The property allows the user to decide if
the switch regulators should be disabled or not.

Since the revert patch is on Marks regulator repo, my patches are based
on his repo too. Each commit has a changelog, so I ommit it here. I
tested it on a custom board.

Used Kernel:
Repo:
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
Branch:
regulator/for-4.19

Regards,
Marco

[1] https://patchwork.kernel.org/patch/10490381/
[2] https://patchwork.kernel.org/patch/10500333/


Marco Felsch (3):
  dt-bindings: pfuze100: add optional disable switch-regulators binding
  regulator: pfuze100: add support to en-/disable switch regulators

 .../bindings/regulator/pfuze100.txt           | 11 ++++++
 drivers/regulator/core.c                      |  2 ++
 drivers/regulator/pfuze100-regulator.c        | 36 +++++++++++++++++++
 3 files changed, 49 insertions(+)

-- 
2.18.0

[PATCH v2 1/3] dt-bindings: pfuze100: add optional disable switch-regulators binding

[Marco Felsch]

This binding is used to keep the backward compatibility with the current
dtb's [1]. The binding informs the driver that the unused switch regulators
can be disabled.
If it is not specified, the driver doesn't disable the switch regulators.

[1] https://patchwork.kernel.org/patch/10490381/

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

---
Changes in V2:
  - add more information about the binding
  - rename binding and add vendor prefix

 .../devicetree/bindings/regulator/pfuze100.txt        | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/pfuze100.txt b/Documentation/devicetree/bindings/regulator/pfuze100.txt
index 672c939045ff..2c46b8d368db 100644
--- a/Documentation/devicetree/bindings/regulator/pfuze100.txt
+++ b/Documentation/devicetree/bindings/regulator/pfuze100.txt
@@ -4,6 +4,17 @@ Required properties:
 - compatible: "fsl,pfuze100", "fsl,pfuze200", "fsl,pfuze3000", "fsl,pfuze3001"
 - reg: I2C slave address
 
+Optional properties:
+- fsl,pfuze-support-disable: Boolean, if present disable all unused switch
+  regulators to save power consumption. Attention, till 4.18 these regulators
+  were always on without specifying "regulator-always-on". So be sure to mark
+  import regulators as "regulator-always-on" (e.g. DDR ref, DDR supply). If
+  not present, the switched regualtors are always on and can't be disabled.
+  This binding is a workaround to keep backward compatibility with old dtb's
+  which rely on the fact that the switched regulators are always on and don't
+  mark them explicit as "regulator-always-on". On new dtbs this property should
+  always be present.
+
 Required child node:
 - regulators: This is the list of child nodes that specify the regulator
   initialization data for defined regulators. Please refer to below doc
-- 
2.18.0

[PATCH v2 2/3] regulator: pfuze100: add support to en-/disable switch regulators

[Marco Felsch]

Add enable/disable support for switch regulators on pfuze100.

Based on commit 5fe156f1cab4 ("regulator: pfuze100: add enable/disable for
switch") which is reverted due to boot regressions by commit 464a5686e6c9
("regulator: Revert "regulator: pfuze100: add enable/disable for switch"").
Disabling the switch regulators will only be done if the user specifies
"fsl,pfuze-support-disable" in its device tree to keep backward
compatibility with current dtb's [1].

[1] https://patchwork.kernel.org/patch/10490381/

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

---
Changes in v2:
  - Don't trick the framework, use a seperate ops struct
    to register the correct callbacks.
  - Set the desc en/disable_val and enable_time only if it necessary
  - Change the dt property binding

Changes since https://patchwork.kernel.org/patch/10405723/
  - Use DT property to keep backward compatibility
  - Use the default register val 0x8 as enable_val

 drivers/regulator/pfuze100-regulator.c | 36 ++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
index cde6eda1d283..b70d16d5a2ff 100644
--- a/drivers/regulator/pfuze100-regulator.c
+++ b/drivers/regulator/pfuze100-regulator.c
@@ -17,6 +17,8 @@
 #include <linux/slab.h>
 #include <linux/regmap.h>
 
+#define PFUZE_FLAG_DISABLE_SW	BIT(1)
+
 #define PFUZE_NUMREGS		128
 #define PFUZE100_VOL_OFFSET	0
 #define PFUZE100_STANDBY_OFFSET	1
@@ -50,10 +52,12 @@ struct pfuze_regulator {
 	struct regulator_desc desc;
 	unsigned char stby_reg;
 	unsigned char stby_mask;
+	bool sw_reg;
 };
 
 struct pfuze_chip {
 	int	chip_id;
+	int     flags;
 	struct regmap *regmap;
 	struct device *dev;
 	struct pfuze_regulator regulator_descs[PFUZE100_MAX_REGULATOR];
@@ -170,6 +174,17 @@ static const struct regulator_ops pfuze100_sw_regulator_ops = {
 	.set_ramp_delay = pfuze100_set_ramp_delay,
 };
 
+static const struct regulator_ops pfuze100_sw_disable_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.list_voltage = regulator_list_voltage_linear,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_time_sel = regulator_set_voltage_time_sel,
+	.set_ramp_delay = pfuze100_set_ramp_delay,
+};
+
 static const struct regulator_ops pfuze100_swb_regulator_ops = {
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -209,9 +224,12 @@ static const struct regulator_ops pfuze100_swb_regulator_ops = {
 			.uV_step = (step),	\
 			.vsel_reg = (base) + PFUZE100_VOL_OFFSET,	\
 			.vsel_mask = 0x3f,	\
+			.enable_reg = (base) + PFUZE100_MODE_OFFSET,	\
+			.enable_mask = 0xf,	\
 		},	\
 		.stby_reg = (base) + PFUZE100_STANDBY_OFFSET,	\
 		.stby_mask = 0x3f,	\
+		.sw_reg = true,		\
 	}
 
 #define PFUZE100_SWB_REG(_chip, _name, base, mask, voltages)	\
@@ -471,6 +489,9 @@ static int pfuze_parse_regulators_dt(struct pfuze_chip *chip)
 	if (!np)
 		return -EINVAL;
 
+	if (of_property_read_bool(np, "fsl,pfuze-support-disable"))
+		chip->flags |= PFUZE_FLAG_DISABLE_SW;
+
 	parent = of_get_child_by_name(np, "regulators");
 	if (!parent) {
 		dev_err(dev, "regulators node not found\n");
@@ -703,6 +724,21 @@ static int pfuze100_regulator_probe(struct i2c_client *client,
 			}
 		}
 
+		/*
+		 * Allow SW regulators to turn off. Checking it trough a flag is
+		 * a workaround to keep the backward compatibility with existing
+		 * old dtb's which may relay on the fact that we didn't disable
+		 * the switched regulator till yet.
+		 */
+		if (pfuze_chip->flags & PFUZE_FLAG_DISABLE_SW) {
+			if (pfuze_chip->regulator_descs[i].sw_reg) {
+				desc->ops = &pfuze100_sw_disable_regulator_ops;
+				desc->enable_val = 0x8;
+				desc->disable_val = 0x0;
+				desc->enable_time = 500;
+			}
+		}
+
 		config.dev = &client->dev;
 		config.init_data = init_data;
 		config.driver_data = pfuze_chip;
-- 
2.18.0

Re: [PATCH v2 1/3] dt-bindings: pfuze100: add optional disable switch-regulators binding

[Mark Brown]

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

On Fri, Jul 13, 2018 at 02:50:01PM +0200, Marco Felsch wrote:

> +Optional properties:
> +- fsl,pfuze-support-disable: Boolean, if present disable all unused switch
> +  regulators to save power consumption. Attention, till 4.18 these regulators

The property name sounds like it affects all the regulators but really
it's just the sw ones - how about adding -sw at the end?  Bit of a
bikeshed but it does end up in an ABI.

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

Re: [PATCH v2 1/3] dt-bindings: pfuze100: add optional disable switch-regulators binding

[Marco Felsch]

Hi Mark,

thanks for the review.

On 18-07-13 16:03, Mark Brown wrote:
> On Fri, Jul 13, 2018 at 02:50:01PM +0200, Marco Felsch wrote:
> 
> > +Optional properties:
> > +- fsl,pfuze-support-disable: Boolean, if present disable all unused switch
> > +  regulators to save power consumption. Attention, till 4.18 these regulators
> 
> The property name sounds like it affects all the regulators but really
> it's just the sw ones - how about adding -sw at the end?  Bit of a
> bikeshed but it does end up in an ABI.

You're right, it sounds better. So the new binding will be
fsl,pfuze-support-disable-sw. Are there any other comments about the
implementation?

Regards,
Marco

Re: [PATCH v2 1/3] dt-bindings: pfuze100: add optional disable switch-regulators binding

[Rob Herring]

On Fri, Jul 13, 2018 at 02:50:01PM +0200, Marco Felsch wrote:
> This binding is used to keep the backward compatibility with the current
> dtb's [1]. The binding informs the driver that the unused switch regulators
> can be disabled.
> If it is not specified, the driver doesn't disable the switch regulators.
> 
> [1] https://patchwork.kernel.org/patch/10490381/
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> ---
> Changes in V2:
>   - add more information about the binding
>   - rename binding and add vendor prefix
> 
>  .../devicetree/bindings/regulator/pfuze100.txt        | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/pfuze100.txt b/Documentation/devicetree/bindings/regulator/pfuze100.txt
> index 672c939045ff..2c46b8d368db 100644
> --- a/Documentation/devicetree/bindings/regulator/pfuze100.txt
> +++ b/Documentation/devicetree/bindings/regulator/pfuze100.txt
> @@ -4,6 +4,17 @@ Required properties:
>  - compatible: "fsl,pfuze100", "fsl,pfuze200", "fsl,pfuze3000", "fsl,pfuze3001"
>  - reg: I2C slave address
>  
> +Optional properties:
> +- fsl,pfuze-support-disable: Boolean, if present disable all unused switch
> +  regulators to save power consumption. Attention, till 4.18 these regulators

You shouldn't have kernel version info in bindings.

> +  were always on without specifying "regulator-always-on". So be sure to mark
> +  import regulators as "regulator-always-on" (e.g. DDR ref, DDR supply). If

s/import/important/

> +  not present, the switched regualtors are always on and can't be disabled.
> +  This binding is a workaround to keep backward compatibility with old dtb's
> +  which rely on the fact that the switched regulators are always on and don't
> +  mark them explicit as "regulator-always-on". On new dtbs this property should
> +  always be present.
> +
>  Required child node:
>  - regulators: This is the list of child nodes that specify the regulator
>    initialization data for defined regulators. Please refer to below doc
> -- 
> 2.18.0
> 

Re: [PATCH v2 1/3] dt-bindings: pfuze100: add optional disable switch-regulators binding

[Mark Brown]

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

On Mon, Jul 16, 2018 at 09:25:16AM +0200, Marco Felsch wrote:

> You're right, it sounds better. So the new binding will be
> fsl,pfuze-support-disable-sw. Are there any other comments about the
> implementation?

Not AFAIR.

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

Re: [PATCH v2 1/3] dt-bindings: pfuze100: add optional disable switch-regulators binding

[Marco Felsch]

Hi Rob,

thanks for the review. I will integrate it in a v3.

Regards,
Marco

On 18-07-16 11:55, Rob Herring wrote:
> On Fri, Jul 13, 2018 at 02:50:01PM +0200, Marco Felsch wrote:
> > This binding is used to keep the backward compatibility with the current
> > dtb's [1]. The binding informs the driver that the unused switch regulators
> > can be disabled.
> > If it is not specified, the driver doesn't disable the switch regulators.
> > 
> > [1] https://patchwork.kernel.org/patch/10490381/
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > 
> > ---
> > Changes in V2:
> >   - add more information about the binding
> >   - rename binding and add vendor prefix
> > 
> >  .../devicetree/bindings/regulator/pfuze100.txt        | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/regulator/pfuze100.txt b/Documentation/devicetree/bindings/regulator/pfuze100.txt
> > index 672c939045ff..2c46b8d368db 100644
> > --- a/Documentation/devicetree/bindings/regulator/pfuze100.txt
> > +++ b/Documentation/devicetree/bindings/regulator/pfuze100.txt
> > @@ -4,6 +4,17 @@ Required properties:
> >  - compatible: "fsl,pfuze100", "fsl,pfuze200", "fsl,pfuze3000", "fsl,pfuze3001"
> >  - reg: I2C slave address
> >  
> > +Optional properties:
> > +- fsl,pfuze-support-disable: Boolean, if present disable all unused switch
> > +  regulators to save power consumption. Attention, till 4.18 these regulators
> 
> You shouldn't have kernel version info in bindings.
> 
> > +  were always on without specifying "regulator-always-on". So be sure to mark
> > +  import regulators as "regulator-always-on" (e.g. DDR ref, DDR supply). If
> 
> s/import/important/
> 
> > +  not present, the switched regualtors are always on and can't be disabled.
> > +  This binding is a workaround to keep backward compatibility with old dtb's
> > +  which rely on the fact that the switched regulators are always on and don't
> > +  mark them explicit as "regulator-always-on". On new dtbs this property should
> > +  always be present.
> > +
> >  Required child node:
> >  - regulators: This is the list of child nodes that specify the regulator
> >    initialization data for defined regulators. Please refer to below doc
> > -- 
> > 2.18.0
> > 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5082 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |