[PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Tony Lindgren]

We can start moving regulators to using the standard gpios property instead
of various custom GPIO related properties. The reason for doing this is that
at least regulator-fixed can currently cause silent bugs if "gpios" property
is used instead of "gpio" property.

Fix the issue by trying to use "gpios" where possible in the drivers that
can already use it, and if that fails, then try to use the deprecated custom
property for getting the GPIO.

Cc: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/regulator/fixed-regulator.txt     |  8 ++++++--
 Documentation/devicetree/bindings/regulator/lp872x.txt    |  9 ++++++---
 .../devicetree/bindings/regulator/max8997-regulator.txt   | 15 ++++++++++-----
 Documentation/devicetree/bindings/regulator/tps65090.txt  |  6 +++++-
 drivers/regulator/fixed.c                                 |  4 +++-
 drivers/regulator/lp872x.c                                |  4 +++-
 drivers/regulator/max8997.c                               |  4 +++-
 drivers/regulator/tps65090-regulator.c                    |  7 +++++--
 8 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
index 4fae41d..cabdb77 100644
--- a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -4,7 +4,7 @@ Required properties:
 - compatible: Must be "regulator-fixed";
 
 Optional properties:
-- gpio: gpio to use for enable control
+- gpios: gpio to use for enable control
 - startup-delay-us: startup time in microseconds
 - enable-active-high: Polarity of GPIO is Active high
 If this property is missing, the default assumed is Active low.
@@ -12,6 +12,10 @@ If this property is missing, the default assumed is Active low.
   If this property is missing then default assumption is false.
 -vin-supply: Input supply name.
 
+Deprecated properties:
+- gpio: Use the standard gpios property listed above instead of
+  this.
+
 Any property defined as part of the core regulator
 binding, defined in regulator.txt, can also be used.
 However a fixed voltage regulator is expected to have the
@@ -25,7 +29,7 @@ Example:
 		regulator-name = "fixed-supply";
 		regulator-min-microvolt = <1800000>;
 		regulator-max-microvolt = <1800000>;
-		gpio = <&gpio1 16 0>;
+		gpios = <&gpio1 16 0>;
 		startup-delay-us = <70000>;
 		enable-active-high;
 		regulator-boot-on;
diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt b/Documentation/devicetree/bindings/regulator/lp872x.txt
index 7818318..12a4268 100644
--- a/Documentation/devicetree/bindings/regulator/lp872x.txt
+++ b/Documentation/devicetree/bindings/regulator/lp872x.txt
@@ -25,7 +25,7 @@ Optional properties:
     For more details, please see the datasheet.
 
   - ti,update-config: define it when LP872X_GENERAL_CFG register should be set
-  - ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
+  - gpios: GPIO specifier for external DVS pin control of LP872x devices.
   - ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2.
   - ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH.
 
@@ -35,6 +35,9 @@ Optional properties:
     For more details, please see the following binding document.
     (Documentation/devicetree/bindings/regulator/regulator.txt)
 
+Deprecated properties:
+  - ti,dvs-gpio: Use the standard gpios property listed above instead of this.
+
 Datasheet
   - LP8720: http://www.ti.com/lit/ds/symlink/lp8720.pdf
   - LP8725: http://www.ti.com/lit/ds/symlink/lp8725.pdf
@@ -50,10 +53,10 @@ lp8720@7d {
 	ti,update-config;
 
 	/*
-	 * The dvs-gpio depends on the processor environment.
+	 * The gpios depends on the processor environment.
 	 * For example, following GPIO specifier means GPIO134 in OMAP4.
 	 */
-	ti,dvs-gpio = <&gpio5 6 0>;
+	gpios = <&gpio5 6 0>;
 	ti,dvs-vsel = /bits/ 8 <1>;		/* SEL_V2 */
 	ti,dvs-state = /bits/ 8 <1>;		/* DVS_HIGH */
 
diff --git a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
index 5c186a7..460eac8 100644
--- a/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/max8997-regulator.txt
@@ -37,6 +37,7 @@ Optional properties:
 - interrupts: Interrupt specifiers for two interrupt sources.
   - First interrupt specifier is for 'irq1' interrupt.
   - Second interrupt specifier is for 'alert' interrupt.
+- gpios: The gpio dvs to use for 'buck1', 'buck2' and 'buck5'.
 - max8997,pmic-buck1-uses-gpio-dvs: 'buck1' can be controlled by gpio dvs.
 - max8997,pmic-buck2-uses-gpio-dvs: 'buck2' can be controlled by gpio dvs.
 - max8997,pmic-buck5-uses-gpio-dvs: 'buck5' can be controlled by gpio dvs.
@@ -52,8 +53,12 @@ Additional properties required if either of the optional properties are used:
   property should be between 0 and 7. If not specified or if out of range, the
   default value of this property is set to 0.
 
-- max8997,pmic-buck125-dvs-gpios: GPIO specifiers for three host gpio's used
-  for dvs. The format of the gpio specifier depends in the gpio controller.
+- gpios: GPIO specifiers for three host gpio's used for dvs. The format of
+  the gpio specifier depends in the gpio controller.
+
+Deprecated properties:
+- max8997,pmic-buck125-dvs-gpios: Use the standard gpios property listed above
+  instead of this.
 
 Regulators: The regulators of max8997 that have to be instantiated should be
 included in a sub-node named 'regulators'. Regulator nodes included in this
@@ -102,9 +107,9 @@ Example:
 		max8997,pmic-ignore-gpiodvs-side-effect;
 		max8997,pmic-buck125-default-dvs-idx = <0>;
 
-		max8997,pmic-buck125-dvs-gpios = <&gpx0 0 1 0 0>, /* SET1 */
-						 <&gpx0 1 1 0 0>, /* SET2 */
-						 <&gpx0 2 1 0 0>; /* SET3 */
+		gpios = <&gpx0 0 1 0 0>, /* SET1 */
+			<&gpx0 1 1 0 0>, /* SET2 */
+			<&gpx0 2 1 0 0>; /* SET3 */
 
 		max8997,pmic-buck1-dvs-voltage = <1350000>, <1300000>,
 						 <1250000>, <1200000>,
diff --git a/Documentation/devicetree/bindings/regulator/tps65090.txt b/Documentation/devicetree/bindings/regulator/tps65090.txt
index 313a60b..05870a8 100644
--- a/Documentation/devicetree/bindings/regulator/tps65090.txt
+++ b/Documentation/devicetree/bindings/regulator/tps65090.txt
@@ -16,12 +16,16 @@ Required properties:
 Optional properties:
 - ti,enable-ext-control: This is applicable for DCDC1, DCDC2 and DCDC3.
   If DCDCs are externally controlled then this property should be there.
-- "dcdc-ext-control-gpios: This is applicable for DCDC1, DCDC2 and DCDC3.
+- gpios: This is applicable for DCDC1, DCDC2 and DCDC3.
   If DCDCs are externally controlled and if it is from GPIO then GPIO
   number should be provided. If it is externally controlled and no GPIO
   entry then driver will just configure this rails as external control
   and will not provide any enable/disable APIs.
 
+Deprecated properties:
+- dcdc-ext-control-gpios: Use the standard gpios property listed above
+  instead of this.
+
 Each regulator is defined using the standard binding for regulators.
 
 Example:
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 5ea64b9..652adf7 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -77,7 +77,9 @@ of_get_fixed_voltage_config(struct device *dev)
 	if (init_data->constraints.boot_on)
 		config->enabled_at_boot = true;
 
-	config->gpio = of_get_named_gpio(np, "gpio", 0);
+	config->gpio = of_get_gpio(np, 0);
+	if (!gpio_is_valid(config->gpio))
+		config->gpio = of_get_named_gpio(np, "gpio", 0);
 	/*
 	 * of_get_named_gpio() currently returns ENODEV rather than
 	 * EPROBE_DEFER. This code attempts to be compatible with both
diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 2e4734f..29192f2 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -863,7 +863,9 @@ static struct lp872x_platform_data
 	if (!pdata->dvs)
 		goto out;
 
-	pdata->dvs->gpio = of_get_named_gpio(np, "ti,dvs-gpio", 0);
+	pdata->dvs->gpio = of_get_gpio(np, 0);
+	if (!gpio_is_valid(pdata->dvs->gpio))
+		pdata->dvs->gpio = of_get_named_gpio(np, "ti,dvs-gpio", 0);
 	of_property_read_u8(np, "ti,dvs-vsel", (u8 *)&pdata->dvs->vsel);
 	of_property_read_u8(np, "ti,dvs-state", &dvs_state);
 	pdata->dvs->init_state = dvs_state ? DVS_HIGH : DVS_LOW;
diff --git a/drivers/regulator/max8997.c b/drivers/regulator/max8997.c
index 2d618fc..0703066 100644
--- a/drivers/regulator/max8997.c
+++ b/drivers/regulator/max8997.c
@@ -899,7 +899,9 @@ static int max8997_pmic_dt_parse_dvs_gpio(struct platform_device *pdev,
 	int i, gpio;
 
 	for (i = 0; i < 3; i++) {
-		gpio = of_get_named_gpio(pmic_np,
+		gpio = of_get_gpio(pmic_np, i);
+		if (!gpio_is_valid(gpio))
+			gpio = of_get_named_gpio(pmic_np,
 					"max8997,pmic-buck125-dvs-gpios", i);
 		if (!gpio_is_valid(gpio)) {
 			dev_err(&pdev->dev, "invalid gpio[%d]: %d\n", i, gpio);
diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
index 676f755..e118ba4 100644
--- a/drivers/regulator/tps65090-regulator.c
+++ b/drivers/regulator/tps65090-regulator.c
@@ -208,9 +208,12 @@ static struct tps65090_platform_data *tps65090_parse_dt_reg_data(
 		rpdata->enable_ext_control = of_property_read_bool(
 					tps65090_matches[idx].of_node,
 					"ti,enable-ext-control");
-		if (rpdata->enable_ext_control)
-			rpdata->gpio = of_get_named_gpio(np,
+		if (rpdata->enable_ext_control) {
+			rpdata->gpio = of_get_gpio(np, 0);
+			if (!gpio_is_valid(rpdata->gpio))
+				rpdata->gpio = of_get_named_gpio(np,
 					"dcdc-ext-control-gpios", 0);
+		}
 
 		tps65090_pdata->reg_pdata[idx] = rpdata;
 	}
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Mark Brown]

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

On Fri, Dec 13, 2013 at 12:07:14PM -0800, Tony Lindgren wrote:
> We can start moving regulators to using the standard gpios property instead
> of various custom GPIO related properties. The reason for doing this is that
> at least regulator-fixed can currently cause silent bugs if "gpios" property
> is used instead of "gpio" property.

If the issue is typos then I'm not convinced that for singular GPIOs
it's going to be helpful, either way is prone to typos.  If the problem
is error reporting then that seems like a more important thing to fix.

> Fix the issue by trying to use "gpios" where possible in the drivers that
> can already use it, and if that fails, then try to use the deprecated custom
> property for getting the GPIO.

Please split stuff like this up per driver rather than just sending a
jumbo patch for the subsystem, it makes it much easier to review
especially when they're not all doing exactly the same thing.

> -  - ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
> +  - gpios: GPIO specifier for external DVS pin control of LP872x devices.

This is definitely a step backwards, it's changing from a named property
which does a specific thing to a property with no useful semantics in
the name.  That would be a step backwards for both legibility and
extensibility, if support for any other GPIO controlled features is
added then adding them into the sam property is going to be unpleasant
and lead to the usability failures so typical of the older device tree
bindings.  The the binding document says to use a name prefix, not to
call everything just "gpios".

To be honest I'm also struggling to summon up the enthusiasm for the
churn in the bindings, especially without going through and updating all
the boards (and all the other GPIO properties in various DTs).  It seems
like there's stuff missing in the helpers here, if we really wanted to
force the properties to have -gpios on the end of their names then we
should've had that being added by the helpers.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [131216 10:37]:
> On Fri, Dec 13, 2013 at 12:07:14PM -0800, Tony Lindgren wrote:
> > We can start moving regulators to using the standard gpios property instead
> > of various custom GPIO related properties. The reason for doing this is that
> > at least regulator-fixed can currently cause silent bugs if "gpios" property
> > is used instead of "gpio" property.
> 
> If the issue is typos then I'm not convinced that for singular GPIOs
> it's going to be helpful, either way is prone to typos.  If the problem
> is error reporting then that seems like a more important thing to fix.

Are you serious? A typo here in the binding leads to silent errors where
nothing happens with the GPIO. That's just totally messed up considering
we use "gpios" instead of "gpio" everywhere else. So both "gpio" and
"gpios" should be parsed for sure.
 
> > Fix the issue by trying to use "gpios" where possible in the drivers that
> > can already use it, and if that fails, then try to use the deprecated custom
> > property for getting the GPIO.
> 
> Please split stuff like this up per driver rather than just sending a
> jumbo patch for the subsystem, it makes it much easier to review
> especially when they're not all doing exactly the same thing.

OK I'll just do patch for the fixed regulator.
 
> > -  - ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices.
> > +  - gpios: GPIO specifier for external DVS pin control of LP872x devices.
> 
> This is definitely a step backwards, it's changing from a named property
> which does a specific thing to a property with no useful semantics in
> the name.  That would be a step backwards for both legibility and
> extensibility, if support for any other GPIO controlled features is
> added then adding them into the sam property is going to be unpleasant
> and lead to the usability failures so typical of the older device tree
> bindings.  The the binding document says to use a name prefix, not to
> call everything just "gpios".

OK let's drop that change and just fix the fixed regulator.
 
> To be honest I'm also struggling to summon up the enthusiasm for the
> churn in the bindings, especially without going through and updating all
> the boards (and all the other GPIO properties in various DTs).  It seems
> like there's stuff missing in the helpers here, if we really wanted to
> force the properties to have -gpios on the end of their names then we
> should've had that being added by the helpers.

Sounds like exposing an infinite number of random *-gpio and *-gpios
bindings is a topic for another discussion. Anyways it's already totally
out of control so what do I care.

Regards,

Tony

Re: [PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Mark Brown]

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

On Mon, Dec 16, 2013 at 11:40:23AM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [131216 10:37]:

> > If the issue is typos then I'm not convinced that for singular GPIOs
> > it's going to be helpful, either way is prone to typos.  If the problem
> > is error reporting then that seems like a more important thing to fix.

> Are you serious? A typo here in the binding leads to silent errors where
> nothing happens with the GPIO. That's just totally messed up considering
> we use "gpios" instead of "gpio" everywhere else. So both "gpio" and
> "gpios" should be parsed for sure.

This is the first time anyone's mentioned this so it probably isn't that
serious an issue and bear in mind that the patch was also handling all
the named GPIO specifiers too.

In any case, the thing is that there's a difference between parsing both
and deprecation - deprecation implies an intention to remove the old one
which would just reintroduce the problem the other way around since
people are likely to drop or forget the plural, use old DTs and so on.
Adding a gpios property in parallel with plain gpio is fine and what I
was mostly suggesting.

> > To be honest I'm also struggling to summon up the enthusiasm for the
> > churn in the bindings, especially without going through and updating all
> > the boards (and all the other GPIO properties in various DTs).  It seems
> > like there's stuff missing in the helpers here, if we really wanted to
> > force the properties to have -gpios on the end of their names then we
> > should've had that being added by the helpers.

> Sounds like exposing an infinite number of random *-gpio and *-gpios
> bindings is a topic for another discussion. Anyways it's already totally
> out of control so what do I care.

Exactly, I think it's way more trouble than it's worth to try to change
for named single element lists.  The standard property makes more sense.

The root of the issue is that the GPIO binding originally said that
everything should use a single gpios property for everything but that's
got usability issues.  The attempt to require a -gpios suffix was a
later addition but it was just put into the binding with no real effort
to propagate it through integration with the helpers or whatever.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [131216 12:12]:
> On Mon, Dec 16, 2013 at 11:40:23AM -0800, Tony Lindgren wrote:
> > * Mark Brown <broonie@kernel.org> [131216 10:37]:
> 
> > > If the issue is typos then I'm not convinced that for singular GPIOs
> > > it's going to be helpful, either way is prone to typos.  If the problem
> > > is error reporting then that seems like a more important thing to fix.
> 
> > Are you serious? A typo here in the binding leads to silent errors where
> > nothing happens with the GPIO. That's just totally messed up considering
> > we use "gpios" instead of "gpio" everywhere else. So both "gpio" and
> > "gpios" should be parsed for sure.
> 
> This is the first time anyone's mentioned this so it probably isn't that
> serious an issue and bear in mind that the patch was also handling all
> the named GPIO specifiers too.

I've certainly debugged this same exact issue twice already and that
probably means that same issue has been debugged tens or hundreds
of times by other people.

For the other random named GPIO properties, I don't frankly care, that's
really not my problem.

Personally I don't see any value for a regulator describing the names of
the GPIOs in the binding, it's really up to the driver to make sense of
them. Especially if there are one or more similar GPIOs. We're not
naming interrupts either.

> In any case, the thing is that there's a difference between parsing both
> and deprecation - deprecation implies an intention to remove the old one
> which would just reintroduce the problem the other way around since
> people are likely to drop or forget the plural, use old DTs and so on.
> Adding a gpios property in parallel with plain gpio is fine and what I
> was mostly suggesting.

Deprecation _may_ imply that it will get removed, but not always.
Naturally we're going to have to keep the old bindings in place since
they were merged. I can changed that to obsoleted if that's any better.
 
> > > To be honest I'm also struggling to summon up the enthusiasm for the
> > > churn in the bindings, especially without going through and updating all
> > > the boards (and all the other GPIO properties in various DTs).  It seems
> > > like there's stuff missing in the helpers here, if we really wanted to
> > > force the properties to have -gpios on the end of their names then we
> > > should've had that being added by the helpers.
> 
> > Sounds like exposing an infinite number of random *-gpio and *-gpios
> > bindings is a topic for another discussion. Anyways it's already totally
> > out of control so what do I care.
> 
> Exactly, I think it's way more trouble than it's worth to try to change
> for named single element lists.  The standard property makes more sense.

I don't think there should be any named GPIOs. If we want names, then
the GPIO usage should be possible to group quite easily rather than create
a new property for everything. Something like "enable-gpio" comes to mind.
 
> The root of the issue is that the GPIO binding originally said that
> everything should use a single gpios property for everything but that's
> got usability issues.  The attempt to require a -gpios suffix was a
> later addition but it was just put into the binding with no real effort
> to propagate it through integration with the helpers or whatever.

There may still be a case for naming of some of the GPIOs, but usually
the order of GPIOs should be enough for the driver to know the meaning
of the GPIO.

Regards,

Tony

Re: [PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Mark Brown]

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

On Mon, Dec 16, 2013 at 01:05:13PM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [131216 12:12]:

> > This is the first time anyone's mentioned this so it probably isn't that
> > serious an issue and bear in mind that the patch was also handling all
> > the named GPIO specifiers too.

> I've certainly debugged this same exact issue twice already and that
> probably means that same issue has been debugged tens or hundreds
> of times by other people.

That surprises me to be honest, but then the fact that the convention
was even defined surprises me.  Like I say you're the first person to
mention it; I suspect you might write more DTs than most.

> Personally I don't see any value for a regulator describing the names of
> the GPIOs in the binding, it's really up to the driver to make sense of
> them. Especially if there are one or more similar GPIOs. We're not

Devices like PMICs frequently have a *lot* of possible pin functions
some of which can get mapped onto GPIOs (in either direction), many of
which are going to be fixed by system design and generally all muxed
onto a much smaller set of physical pins.  If you try to specify those
through an unnamed gpios property you end up with a very large array
(say 30 elements, more wouldn't be too surprising) most of which will be
empty.  That is not at all usable, it's error prone to write and very
hard to read even with the preprocessor support that only got added
quite recently.

> naming interrupts either.

There's typically a much more limited set of interrupts a device can
have (and many of the more optional ones end up getting expressed via
the GPIO binding since you also need to read the state to use them) so
they don't run into the issue so much.

> > In any case, the thing is that there's a difference between parsing both
> > and deprecation - deprecation implies an intention to remove the old one
> > which would just reintroduce the problem the other way around since
> > people are likely to drop or forget the plural, use old DTs and so on.
> > Adding a gpios property in parallel with plain gpio is fine and what I
> > was mostly suggesting.

> Deprecation _may_ imply that it will get removed, but not always.
> Naturally we're going to have to keep the old bindings in place since
> they were merged. I can changed that to obsoleted if that's any better.

Yes, that's better.

> > Exactly, I think it's way more trouble than it's worth to try to change
> > for named single element lists.  The standard property makes more sense.

> I don't think there should be any named GPIOs. If we want names, then
> the GPIO usage should be possible to group quite easily rather than create
> a new property for everything. Something like "enable-gpio" comes to mind.

I don't understand the difference between your suggestion and named
GPIOs.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Tony Lindgren]

* Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [131216 13:42]:
> On Mon, Dec 16, 2013 at 01:05:13PM -0800, Tony Lindgren wrote:
> > * Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [131216 12:12]:
> 
> > > This is the first time anyone's mentioned this so it probably isn't that
> > > serious an issue and bear in mind that the patch was also handling all
> > > the named GPIO specifiers too.
> 
> > I've certainly debugged this same exact issue twice already and that
> > probably means that same issue has been debugged tens or hundreds
> > of times by other people.
> 
> That surprises me to be honest, but then the fact that the convention
> was even defined surprises me.  Like I say you're the first person to
> mention it; I suspect you might write more DTs than most.

OK a limited patch for fixed regulator appended below to deal with the
above issue only.
 
> > Personally I don't see any value for a regulator describing the names of
> > the GPIOs in the binding, it's really up to the driver to make sense of
> > them. Especially if there are one or more similar GPIOs. We're not
> 
> Devices like PMICs frequently have a *lot* of possible pin functions
> some of which can get mapped onto GPIOs (in either direction), many of
> which are going to be fixed by system design and generally all muxed
> onto a much smaller set of physical pins.  If you try to specify those
> through an unnamed gpios property you end up with a very large array
> (say 30 elements, more wouldn't be too surprising) most of which will be
> empty.  That is not at all usable, it's error prone to write and very
> hard to read even with the preprocessor support that only got added
> quite recently.

That's why PMICs usually show up as GPIO controllers. And if a regulator
needs to use those GPIOs, it should most likely just use the standard
"gpios" property.
 
> > naming interrupts either.
> 
> There's typically a much more limited set of interrupts a device can
> have (and many of the more optional ones end up getting expressed via
> the GPIO binding since you also need to read the state to use them) so
> they don't run into the issue so much.

Hmm to me it seems that pretty much all of these should just use "gpios":

$ git grep -B2 -A1 of_get_named_gpio drivers/regulator
 
> > > In any case, the thing is that there's a difference between parsing both
> > > and deprecation - deprecation implies an intention to remove the old one
> > > which would just reintroduce the problem the other way around since
> > > people are likely to drop or forget the plural, use old DTs and so on.
> > > Adding a gpios property in parallel with plain gpio is fine and what I
> > > was mostly suggesting.
> 
> > Deprecation _may_ imply that it will get removed, but not always.
> > Naturally we're going to have to keep the old bindings in place since
> > they were merged. I can changed that to obsoleted if that's any better.
> 
> Yes, that's better.
> 
> > > Exactly, I think it's way more trouble than it's worth to try to change
> > > for named single element lists.  The standard property makes more sense.
> 
> > I don't think there should be any named GPIOs. If we want names, then
> > the GPIO usage should be possible to group quite easily rather than create
> > a new property for everything. Something like "enable-gpio" comes to mind.
> 
> I don't understand the difference between your suggestion and named
> GPIOs.

What I'm trying to say is let's not let drivers invent their random
*-gpio[s] property as those essentially creates new kernel ABIs that
we're stuck with.

Instead, let's try to use standard properties where possible like
"gpios" and "enable-gpios", "cs-gpios" and so on.

Regards,

Tony

8< ------------------------
From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Date: Thu, 12 Dec 2013 16:21:52 -0800
Subject: [PATCH] regulator: Start using standard gpios property for fixed regulator

The fixed regulator has been using a custom "gpio" property instead of
standard "gpios" property which is confusing and can leave into silent bugs
with typos where the GPIO value is not changed for the regulator.

Fix the issue by trying to use "gpios" where possible in the drivers that
can already use it, and if that fails, then try to use the legacy custom
GPIO property.

Note that we cannot remove the existing legacy property, and don't need
to churn the .dts files for it. But we can start using the standard
"gpios" property for the new entries and update the existing entries
where suitable with other clean-up.

Cc: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

--- a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -4,7 +4,7 @@ Required properties:
 - compatible: Must be "regulator-fixed";
 
 Optional properties:
-- gpio: gpio to use for enable control
+- gpios: gpio to use for enable control
 - startup-delay-us: startup time in microseconds
 - enable-active-high: Polarity of GPIO is Active high
 If this property is missing, the default assumed is Active low.
@@ -12,6 +12,10 @@ If this property is missing, the default assumed is Active low.
   If this property is missing then default assumption is false.
 -vin-supply: Input supply name.
 
+Obsoleted properties:
+- gpio: Use the standard gpios property listed above instead of
+  this.
+
 Any property defined as part of the core regulator
 binding, defined in regulator.txt, can also be used.
 However a fixed voltage regulator is expected to have the
@@ -25,7 +29,7 @@ Example:
 		regulator-name = "fixed-supply";
 		regulator-min-microvolt = <1800000>;
 		regulator-max-microvolt = <1800000>;
-		gpio = <&gpio1 16 0>;
+		gpios = <&gpio1 16 0>;
 		startup-delay-us = <70000>;
 		enable-active-high;
 		regulator-boot-on;
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -77,7 +77,9 @@ of_get_fixed_voltage_config(struct device *dev)
 	if (init_data->constraints.boot_on)
 		config->enabled_at_boot = true;
 
-	config->gpio = of_get_named_gpio(np, "gpio", 0);
+	config->gpio = of_get_gpio(np, 0);
+	if (!gpio_is_valid(config->gpio))
+		config->gpio = of_get_named_gpio(np, "gpio", 0);
 	/*
 	 * of_get_named_gpio() currently returns ENODEV rather than
 	 * EPROBE_DEFER. This code attempts to be compatible with both
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Mark Brown]

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

On Mon, Dec 16, 2013 at 03:06:22PM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [131216 13:42]:
> > On Mon, Dec 16, 2013 at 01:05:13PM -0800, Tony Lindgren wrote:

> > > Personally I don't see any value for a regulator describing the names of
> > > the GPIOs in the binding, it's really up to the driver to make sense of
> > > them. Especially if there are one or more similar GPIOs. We're not

> > Devices like PMICs frequently have a *lot* of possible pin functions
> > some of which can get mapped onto GPIOs (in either direction), many of
> > which are going to be fixed by system design and generally all muxed
> > onto a much smaller set of physical pins.  If you try to specify those

> That's why PMICs usually show up as GPIO controllers. And if a regulator
> needs to use those GPIOs, it should most likely just use the standard
> "gpios" property.

No, that's a different thing - the PMIC will typically be able to use
some pins as GPIOs so most expose a GPIO controller.  The functions that
are an issue here are things like voltage selection, voltage transition
completion status, sleep mode, enable control or whatever that may need
to be tied to the SoC for interaction (usually not just limited to the
regulator bit either).  A lot of these things get done either to bypass
register I/O or because they are used as part of power up/down
sequencing and need to be done by hardware.  

If there's any overlap it's with pinctrl though you still need to map
the connected functions to any software controllable GPIOs they're
connected to.

> > > I don't think there should be any named GPIOs. If we want names, then
> > > the GPIO usage should be possible to group quite easily rather than create
> > > a new property for everything. Something like "enable-gpio" comes to mind.

> > I don't understand the difference between your suggestion and named
> > GPIOs.

> What I'm trying to say is let's not let drivers invent their random
> *-gpio[s] property as those essentially creates new kernel ABIs that
> we're stuck with.

> Instead, let's try to use standard properties where possible like
> "gpios" and "enable-gpios", "cs-gpios" and so on.

Oh, OK.  Yes, standardisation of the names has benefits though for some
of the features (especially voltage selection) the implementation gets
rather chip specific and there are also advantages in having the DT
binding correspond to the chip documentation.

Things that really are very standard probably ought to be being done by
the core anyway (like we've done with all the factoring out of standard
voltage map and regmap operations).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [131216 15:39]:
> On Mon, Dec 16, 2013 at 03:06:22PM -0800, Tony Lindgren wrote:
> > * Mark Brown <broonie@kernel.org> [131216 13:42]:
> > > On Mon, Dec 16, 2013 at 01:05:13PM -0800, Tony Lindgren wrote:
> 
> > > > Personally I don't see any value for a regulator describing the names of
> > > > the GPIOs in the binding, it's really up to the driver to make sense of
> > > > them. Especially if there are one or more similar GPIOs. We're not
> 
> > > Devices like PMICs frequently have a *lot* of possible pin functions
> > > some of which can get mapped onto GPIOs (in either direction), many of
> > > which are going to be fixed by system design and generally all muxed
> > > onto a much smaller set of physical pins.  If you try to specify those
> 
> > That's why PMICs usually show up as GPIO controllers. And if a regulator
> > needs to use those GPIOs, it should most likely just use the standard
> > "gpios" property.
> 
> No, that's a different thing - the PMIC will typically be able to use
> some pins as GPIOs so most expose a GPIO controller.  The functions that
> are an issue here are things like voltage selection, voltage transition
> completion status, sleep mode, enable control or whatever that may need
> to be tied to the SoC for interaction (usually not just limited to the
> regulator bit either).  A lot of these things get done either to bypass
> register I/O or because they are used as part of power up/down
> sequencing and need to be done by hardware.  
> 
> If there's any overlap it's with pinctrl though you still need to map
> the connected functions to any software controllable GPIOs they're
> connected to.

OK. Maybe the best way to deal with that is to have the driver specific
regmap (gpiomap? :) configuration describe that? And then the driver
GPIO configuration is picked up just based on the compatible flags and
the gpios property?
 
> > > > I don't think there should be any named GPIOs. If we want names, then
> > > > the GPIO usage should be possible to group quite easily rather than create
> > > > a new property for everything. Something like "enable-gpio" comes to mind.
> 
> > > I don't understand the difference between your suggestion and named
> > > GPIOs.
> 
> > What I'm trying to say is let's not let drivers invent their random
> > *-gpio[s] property as those essentially creates new kernel ABIs that
> > we're stuck with.
> 
> > Instead, let's try to use standard properties where possible like
> > "gpios" and "enable-gpios", "cs-gpios" and so on.
> 
> Oh, OK.  Yes, standardisation of the names has benefits though for some
> of the features (especially voltage selection) the implementation gets
> rather chip specific and there are also advantages in having the DT
> binding correspond to the chip documentation.
> 
> Things that really are very standard probably ought to be being done by
> the core anyway (like we've done with all the factoring out of standard
> voltage map and regmap operations).

Agreed. And a lot of that can be configured automatically based on the
compatible property.

Regards,

Tony

Re: [PATCH] regulator: Start using standard gpios property and deprecate some custom properties

[Mark Brown]

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

On Tue, Dec 17, 2013 at 07:37:27AM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [131216 15:39]:

> > No, that's a different thing - the PMIC will typically be able to use
> > some pins as GPIOs so most expose a GPIO controller.  The functions that
> > are an issue here are things like voltage selection, voltage transition
> > completion status, sleep mode, enable control or whatever that may need
> > to be tied to the SoC for interaction (usually not just limited to the

> OK. Maybe the best way to deal with that is to have the driver specific
> regmap (gpiomap? :) configuration describe that? And then the driver
> GPIO configuration is picked up just based on the compatible flags and
> the gpios property?

Possibly.  I'd need to see the resulting code and DTs - I'm not 100%
visualising what's meant here.

> > Oh, OK.  Yes, standardisation of the names has benefits though for some
> > of the features (especially voltage selection) the implementation gets
> > rather chip specific and there are also advantages in having the DT
> > binding correspond to the chip documentation.

> > Things that really are very standard probably ought to be being done by
> > the core anyway (like we've done with all the factoring out of standard
> > voltage map and regmap operations).

> Agreed. And a lot of that can be configured automatically based on the
> compatible property.

Hopefully not even the compatible property - we ought to just be able to
pick standard names for some of the standard functions and just support
them without any effort from drivers at all.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]