| Message ID | 1386965234-26461-1-git-send-email-tony@atomide.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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.
* 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 -- 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
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. 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.
* 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 -- 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
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. > 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.
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; }
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@samsung.com> Cc: Olof Johansson <olof@lixom.net> Signed-off-by: Tony Lindgren <tony@atomide.com> --- .../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(-)