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

[PATCH 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 in real or if it is a 'simulated' disabling. By 'simulated'
I mean that the regulator-fw think it is disabled but the switch keeps
on in real.

Since the revert patch is on Marks regulator repo, my patches are based
on his repo too.

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 (2):
  dt-bindings: pfuze100: add optional pfuze-disable-sw binding
  regulator: pfuze100: add support to en-/disable switch regulators

 .../bindings/regulator/pfuze100.txt           |  8 ++++++
 drivers/regulator/pfuze100-regulator.c        | 26 +++++++++++++++++++
 2 files changed, 34 insertions(+)

-- 
2.18.0

[PATCH 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw 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>
---
 Documentation/devicetree/bindings/regulator/pfuze100.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/pfuze100.txt b/Documentation/devicetree/bindings/regulator/pfuze100.txt
index 672c939045ff..1f17ab7c7123 100644
--- a/Documentation/devicetree/bindings/regulator/pfuze100.txt
+++ b/Documentation/devicetree/bindings/regulator/pfuze100.txt
@@ -4,6 +4,14 @@ Required properties:
 - compatible: "fsl,pfuze100", "fsl,pfuze200", "fsl,pfuze3000", "fsl,pfuze3001"
 - reg: I2C slave address
 
+Optional properties:
+- pfuze-disable-sw: Disable all unused switch regulators to save power
+  consumption. Attention, some platforms are using the switch regulators as DDR
+  ref or supply voltage. Mark these regulators as "regulator-always-on" to skip
+  disabling these regulators. If not specified, the driver simualtes the
+  disabling. This means the state of the regulator is set to 'disabled' but the
+  driver don't disable the regulator.
+
 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 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw binding

[Fabio Estevam]

On Thu, Jul 12, 2018 at 8:02 AM, Marco Felsch <m.felsch@pengutronix.de> wrote:

> +Optional properties:
> +- pfuze-disable-sw: Disable all unused switch regulators to save power
> +  consumption. Attention, some platforms are using the switch regulators as DDR
> +  ref or supply voltage. Mark these regulators as "regulator-always-on" to skip
> +  disabling these regulators. If not specified, the driver simualtes the

s/simualtes/simulates/

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Re: [PATCH 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw binding

[Marco Felsch]

Hi Fabio,

On 18-07-12 12:19, Fabio Estevam wrote:
> On Thu, Jul 12, 2018 at 8:02 AM, Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > +Optional properties:
> > +- pfuze-disable-sw: Disable all unused switch regulators to save power
> > +  consumption. Attention, some platforms are using the switch regulators as DDR
> > +  ref or supply voltage. Mark these regulators as "regulator-always-on" to skip
> > +  disabling these regulators. If not specified, the driver simualtes the
> 
> s/simualtes/simulates/
> 
> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Thanks for the feedback, I'll fix it in v2.

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

Re: [PATCH 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw binding

[Mark Brown]

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

On Thu, Jul 12, 2018 at 01:02:39PM +0200, Marco Felsch wrote:

> +Optional properties:
> +- pfuze-disable-sw: Disable all unused switch regulators to save power
> +  consumption. Attention, some platforms are using the switch regulators as DDR
> +  ref or supply voltage. Mark these regulators as "regulator-always-on" to skip
> +  disabling these regulators. If not specified, the driver simualtes the
> +  disabling. This means the state of the regulator is set to 'disabled' but the
> +  driver don't disable the regulator.

This is a bit of a confused way of specifying things that depends on the
Linux implementation, and the property sounds like a double negative
too.  I'd say something like "pfuze-support-disable" and then explicitly
say that this is a workaround for backwards compatibility.

I'd also recommend changing the implementation patch to just register a
different version of the desc and ops that just doesn't have the disable
operation so that the framework knows what's going on.  While the
current implementation works now there's the possibility that at some
point in the future we might start relying on the disable actually
having taken effect somehow and will get confused.  There's some
existing drivers that optimize their resume paths if they know power
wasn't removed.

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

Re: [PATCH 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw binding

[Marco Felsch]

Hi Mark,

On 18-07-12 16:31, Mark Brown wrote:
> On Thu, Jul 12, 2018 at 01:02:39PM +0200, Marco Felsch wrote:
> 
> > +Optional properties:
> > +- pfuze-disable-sw: Disable all unused switch regulators to save power
> > +  consumption. Attention, some platforms are using the switch regulators as DDR
> > +  ref or supply voltage. Mark these regulators as "regulator-always-on" to skip
> > +  disabling these regulators. If not specified, the driver simualtes the
> > +  disabling. This means the state of the regulator is set to 'disabled' but the
> > +  driver don't disable the regulator.
> 
> This is a bit of a confused way of specifying things that depends on the
> Linux implementation, and the property sounds like a double negative
> too.  I'd say something like "pfuze-support-disable" and then explicitly
> say that this is a workaround for backwards compatibility.

I can't find the double negative. Anyway your binding sounds better. So
I will use yours. Should we add a vendor prefix too to be clear? I will
also add some more informations to mark it as workaround.

> I'd also recommend changing the implementation patch to just register a
> different version of the desc and ops that just doesn't have the disable
> operation so that the framework knows what's going on.  While the
> current implementation works now there's the possibility that at some
> point in the future we might start relying on the disable actually
> having taken effect somehow and will get confused.  There's some
> existing drivers that optimize their resume paths if they know power
> wasn't removed.

Okay I will change that too. I didn't know that there are drivers with
optimized resume paths.

Thanks for your feedback.

Regards,
Marco

Re: [PATCH 1/2] dt-bindings: pfuze100: add optional pfuze-disable-sw binding

[Mark Brown]

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

On Fri, Jul 13, 2018 at 10:30:39AM +0200, Marco Felsch wrote:
> On 18-07-12 16:31, Mark Brown wrote:
> > On Thu, Jul 12, 2018 at 01:02:39PM +0200, Marco Felsch wrote:

> > > +Optional properties:
> > > +- pfuze-disable-sw: Disable all unused switch regulators to save power

> > This is a bit of a confused way of specifying things that depends on the
> > Linux implementation, and the property sounds like a double negative
> > too.  I'd say something like "pfuze-support-disable" and then explicitly
> > say that this is a workaround for backwards compatibility.

> I can't find the double negative. Anyway your binding sounds better. So
> I will use yours. Should we add a vendor prefix too to be clear? I will
> also add some more informations to mark it as workaround.

The property doesn't disable the use of switch regulators, it enables
their disabling.  A vendor prefix is probably required but I can't
entirely follow the DT rules there, it certainly shouldn't hurt anyway.

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

[PATCH 2/2] 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
"pfuze-disable-sw" in its device tree to keep backward compatibility with
current dtb's [1]. Otherwise the regulators are marked as 'disabled', but
the driver keeps them on. So it's more like a simulated disable.

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

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

---
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 | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
index cde6eda1d283..2f5f3eb43e42 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
@@ -54,6 +56,7 @@ struct pfuze_regulator {
 
 struct pfuze_chip {
 	int	chip_id;
+	int     flags;
 	struct regmap *regmap;
 	struct device *dev;
 	struct pfuze_regulator regulator_descs[PFUZE100_MAX_REGULATOR];
@@ -106,6 +109,18 @@ static const struct of_device_id pfuze_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, pfuze_dt_ids);
 
+static int pfuze100_disable(struct regulator_dev *rdev)
+{
+	struct pfuze_chip *pfuze100 = rdev_get_drvdata(rdev);
+
+	/*
+	 * Only disable the regulator if the user really wants it, else simulate
+	 * a disabled regulator but leave the regulator on.
+	 */
+	return (pfuze100->flags & PFUZE_FLAG_DISABLE_SW) ?
+		regulator_disable_regmap(rdev) : 0;
+}
+
 static int pfuze100_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 {
 	struct pfuze_chip *pfuze100 = rdev_get_drvdata(rdev);
@@ -163,6 +178,9 @@ static const struct regulator_ops pfuze100_fixed_regulator_ops = {
 };
 
 static const struct regulator_ops pfuze100_sw_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = pfuze100_disable,
+	.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,
@@ -209,6 +227,11 @@ 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_val = 0x8,	\
+			.disable_val = 0x0,	\
+			.enable_mask = 0xf,	\
+			.enable_time = 500,	\
 		},	\
 		.stby_reg = (base) + PFUZE100_STANDBY_OFFSET,	\
 		.stby_mask = 0x3f,	\
@@ -471,6 +494,9 @@ static int pfuze_parse_regulators_dt(struct pfuze_chip *chip)
 	if (!np)
 		return -EINVAL;
 
+	if (of_property_read_bool(np, "pfuze-disable-sw"))
+		chip->flags |= PFUZE_FLAG_DISABLE_SW;
+
 	parent = of_get_child_by_name(np, "regulators");
 	if (!parent) {
 		dev_err(dev, "regulators node not found\n");
-- 
2.18.0

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

[Fabio Estevam]

Hi Marco,

On Thu, Jul 12, 2018 at 8:02 AM, Marco Felsch <m.felsch@pengutronix.de> wrote:
> 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
> "pfuze-disable-sw" in its device tree to keep backward compatibility with
> current dtb's [1]. Otherwise the regulators are marked as 'disabled', but
> the driver keeps them on. So it's more like a simulated disable.
>
> [1] https://patchwork.kernel.org/patch/10490381/
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>