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

Message ID 20180712110240.3006-2-m.felsch@pengutronix.de
State Changes Requested
Headers show
Series
  • Re-Enable support to disable switch regulators
Related show

Commit Message

Marco Felsch July 12, 2018, 11:02 a.m.
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(+)

Comments

Fabio Estevam July 12, 2018, 3:19 p.m. | #1
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>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown July 12, 2018, 3:31 p.m. | #2
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.
Marco Felsch July 13, 2018, 7:58 a.m. | #3
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.
Marco Felsch July 13, 2018, 8:30 a.m. | #4
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown July 13, 2018, 11:50 a.m. | #5
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.

Patch

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