diff mbox

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

Message ID 20131216230622.GH26293@atomide.com
State Superseded, archived
Headers show

Commit Message

Tony Lindgren Dec. 16, 2013, 11:06 p.m. UTC 
* Mark Brown <broonie@kernel.org> [131216 13:42]:
> On Mon, Dec 16, 2013 at 01:05:13PM -0800, Tony Lindgren wrote:
> > * Mark Brown <broonie@kernel.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@atomide.com>
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@samsung.com>
Cc: Olof Johansson <olof@lixom.net>
Signed-off-by: Tony Lindgren <tony@atomide.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

Comments

Mark Brown Dec. 16, 2013, 11:37 p.m. UTC | #1 
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).
Tony Lindgren Dec. 17, 2013, 3:37 p.m. UTC | #2 
* 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
--
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 Dec. 19, 2013, 1:26 p.m. UTC | #3 
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.
diff mbox

Patch

--- 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