diff mbox

[v4,2/7] regulator: dt-bindings: Document the ena-gpios property

Message ID 1417087253-12306-3-git-send-email-k.kozlowski@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Krzysztof Kozlowski Nov. 27, 2014, 11:20 a.m. UTC
Document new properties for regulators (ena-gpios and
ena-gpio-open-drain) for enabling control over GPIO.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 Documentation/devicetree/bindings/regulator/regulator.txt | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Mark Brown Nov. 27, 2014, 6:30 p.m. UTC | #1
On Thu, Nov 27, 2014 at 12:20:48PM +0100, Krzysztof Kozlowski wrote:

> +- ena-gpios: GPIO to use for enable control. Actual implementation depends
> +  on regulator driver. The bindings documentation for given driver describes
> +  which regulator actually supports it.
> +- ena-gpio-open-drain: GPIO is open drain type.

I'm relly not a big fan of adding a fixed name property here with no
override capability, it means that the naming won't reflect the specific
regulator design so closely and in practice for many of the PMICs the
GPIO control can do rather more than just control enables and supports
reprogramming.  The latter case where we've got a signal which can
sometimes be simply and enable but sometimes more makes it especially
worrying to have the property always be there, it's something that might
work in some configurations but could easily be broken if we try to
exploit more advanced functionality (things also triggering other
configuration changes at the same time).

Factoring out the code is good but it seems better to have it be
something which drivers can control, for example by having them
explicitly specify a property name to use or perhaps a flag to enable
the default name.

We also need an invert option.
Krzysztof Kozlowski Nov. 28, 2014, 9:09 a.m. UTC | #2
On czw, 2014-11-27 at 18:30 +0000, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 12:20:48PM +0100, Krzysztof Kozlowski wrote:
> 
> > +- ena-gpios: GPIO to use for enable control. Actual implementation depends
> > +  on regulator driver. The bindings documentation for given driver describes
> > +  which regulator actually supports it.
> > +- ena-gpio-open-drain: GPIO is open drain type.
> 
> I'm relly not a big fan of adding a fixed name property here with no
> override capability, it means that the naming won't reflect the specific
> regulator design so closely and in practice for many of the PMICs the
> GPIO control can do rather more than just control enables and supports
> reprogramming.  The latter case where we've got a signal which can
> sometimes be simply and enable but sometimes more makes it especially
> worrying to have the property always be there, it's something that might
> work in some configurations but could easily be broken if we try to
> exploit more advanced functionality (things also triggering other
> configuration changes at the same time).

I understand your concerns here however I didn't want to overengineer
this. Is the same GPIO (on more complex PMICs) used in different
contexts? Like enable control and something more in the same time?

For example the S5M8767 uses different GPIOs for:
1. enable control - one GPIO per regulator,
2. voltage selection (DVS) - 3 GPIOs total,
so there is no benefit in merging this into one common regulator code.

> Factoring out the code is good but it seems better to have it be
> something which drivers can control, for example by having them
> explicitly specify a property name to use or perhaps a flag to enable
> the default name.

Something like:
 struct regulator_desc desc {
 	.name		= "LDO1
 	.of_match	= of_match_ptr("LDO1"),
 	.regulators_node = of_match_ptr("voltage-regulators"),
 	.ops		= &max77686_ldo_ops,
+	.of_ena_gpio	= of_match_ptr("ena-gpios"),
 ...
 }
?


> We also need an invert option.

This is parsed from gpio specifier in DTS: the flags from
include/dt-bindings/gpio/gpio.h.

Thank you for feedback,
Krzysztof
Mark Brown Nov. 28, 2014, 11:21 a.m. UTC | #3
On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:

> I understand your concerns here however I didn't want to overengineer
> this. Is the same GPIO (on more complex PMICs) used in different
> contexts? Like enable control and something more in the same time?

Yes, and it's often reprogrammable at runtime.

> Something like:
>  struct regulator_desc desc {
>  	.name		= "LDO1
>  	.of_match	= of_match_ptr("LDO1"),
>  	.regulators_node = of_match_ptr("voltage-regulators"),
>  	.ops		= &max77686_ldo_ops,
> +	.of_ena_gpio	= of_match_ptr("ena-gpios"),
>  ...
>  }

Yes, and note that this also means existing bindings can use the core
code.
Krzysztof Kozlowski Nov. 28, 2014, 11:54 a.m. UTC | #4
On pi?, 2014-11-28 at 11:21 +0000, Mark Brown wrote:
> On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:
> 
> > I understand your concerns here however I didn't want to overengineer
> > this. Is the same GPIO (on more complex PMICs) used in different
> > contexts? Like enable control and something more in the same time?
> 
> Yes, and it's often reprogrammable at runtime.

I have doubts if generalized code could support such configuration...

> 
> > Something like:
> >  struct regulator_desc desc {
> >  	.name		= "LDO1
> >  	.of_match	= of_match_ptr("LDO1"),
> >  	.regulators_node = of_match_ptr("voltage-regulators"),
> >  	.ops		= &max77686_ldo_ops,
> > +	.of_ena_gpio	= of_match_ptr("ena-gpios"),
> >  ...
> >  }
> 
> Yes, and note that this also means existing bindings can use the core
> code.

Thanks for idea,
Krzysztof
Mark Brown Nov. 28, 2014, 2:29 p.m. UTC | #5
On Fri, Nov 28, 2014 at 12:54:27PM +0100, Krzysztof Kozlowski wrote:
> On pi?, 2014-11-28 at 11:21 +0000, Mark Brown wrote:
> > On Fri, Nov 28, 2014 at 10:09:44AM +0100, Krzysztof Kozlowski wrote:

> > > I understand your concerns here however I didn't want to overengineer
> > > this. Is the same GPIO (on more complex PMICs) used in different
> > > contexts? Like enable control and something more in the same time?

> > Yes, and it's often reprogrammable at runtime.

> I have doubts if generalized code could support such configuration...

That's pretty much my point.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index 4e7ed762b3bb..dad57b3badc7 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -30,6 +30,10 @@  Optional properties:
 	- regulator-off-in-suspend: regulator should be off in suspend state.
 	- regulator-suspend-microvolt: regulator should be set to this voltage
 	  in suspend.
+- ena-gpios: GPIO to use for enable control. Actual implementation depends
+  on regulator driver. The bindings documentation for given driver describes
+  which regulator actually supports it.
+- ena-gpio-open-drain: GPIO is open drain type.
 
 Deprecated properties:
 - regulator-compatible: If a regulator chip contains multiple