[PATCH 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[PATCH 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[Laurent Pinchart]

Hello,

While working on regulators, GPIOs and DT I noticed that many of our DT source
files incorrectly describe fixed regulators. The common error patterns are

- Usage of the undefined (and never parsed) enable-active-low property
- Usage of the enable-active-high property without specifying an enable GPIO
- Typos in the enabl GPIO property name (gpios instead of gpio)
- Mismatch between the enable-active-high property (or the lack thereof) and
  the enable GPIO flags

This patch series fixes those issues in all the DT sources after locating the
errors using the following script.

------------------------------------------------------------------------------
#!/bin/sh

echo $1
cat $1 | awk '
BEGIN {
	open_drain = 0;
	active_high = 0;
	gpio = 0;
	flags = 0;
}

match($0, /([a-zA-Z0-9@_-]*) {/, ary) {
	name = ary[1];
}

/compatible.*"regulator-fixed"/ {
	found = 1;
}

/enable-active-high/ {
	active_high = 1;
}

/gpio-open-drain/ {
	open_drain = 1;
}

match($0, /gpio += <.* ([^ ]*)>/, ary) {
	gpio = 1;
	flags = ary[1];
	if (flags == 0)
		flags = "GPIO_ACTIVE_HIGH";
}

/}/ {
	if (found) {
		if (gpio) {
			print "\t" name ": active high " active_high " " flags " open drain " open_drain;
			if ((active_high && flags == "GPIO_ACTIVE_LOW") ||
			    (!active_high && flags == "GPIO_ACTIVE_HIGH"))
				print "WARNING: enable-active-high and flags do not match"
		} else {
			if (active_high)
				print "WARNING: active high without GPIO"
			if (open_drain)
				print "WARNING: open drain without GPIO"
		}
	}

	gpio = 0;
	found = 0;
	active_high = 0;
	open_drain = 0;
	flags = 0;
}
'
------------------------------------------------------------------------------

All patches except for the ones touching omap3-beagle-xm and omap3-overo-base
are untested as I lack test hardware.

As there's no dependency between the patches touching different source files
the appropriate maintainers could take their share of the patches in their
tree. Alternatively I could send a single pull request after collecting all
acks but that might be more complex.

Cc: devicetree@vger.kernel.org
Cc: linux-omap@vger.kernel.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-tegra@vger.kernel.org
Cc: linux-gpio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Benoit Cousson <bcousson@baylibre.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>

Laurent Pinchart (37):
  ARM: dts: am437x-gp-evm: Remove unneeded regulator property
  ARM: dts: am43xx-epos-evm: Remove unneeded regulator property
  ARM: mvebu: Armada 388 GP: Remove unneeded regulator property
  ARM: imx6sx-sdb: Fix typo in regulator enable GPIO property
  ARM: dts: s5pv210-aquila: Fix typo in regulator enable GPIO property
  ARM: dts: s5pv210-goni: Fix typo in regulator enable GPIO property
  ARM: dts: omap3-evm: Remove invalid enable-active-low regulator
    property
  ARM: dts: omap3-sb-t35: Remove invalid enable-active-low regulator
    property
  ARM: dts: omap3-tao3530: Remove invalid enable-active-low regulator
    property
  ARM: dts: imx6qdl-tx6: Fix regulator enable GPIO polarity
  ARM: dts: dove-cm-a510: Fix regulator enable GPIO polarity
  ARM: dts: dove-sbc-a510: Fix regulator enable GPIO polarity
  ARM: dts: exynos5250-arndale: Fix regulator enable GPIO polarity
  ARM: dts: imx23-evk: Fix regulator enable GPIO polarity
  ARM: dts: imx23-stmp378x_devb: Fix regulator enable GPIO polarity
  ARM: dts: imx25-pdk: Fix regulator enable GPIO polarity
  ARM: dts: imx28-cfa10036: Fix regulator enable GPIO polarity
  ARM: dts: imx28-evk: Fix regulator enable GPIO polarity
  ARM: dts: imx28-m28cu3: Fix regulator enable GPIO polarity
  ARM: dts: imx28-m28evk: Fix regulator enable GPIO polarity
  ARM: dts: imx28-sps1: Fix regulator enable GPIO polarity
  ARM: dts: imx28-tx28: Fix regulator enable GPIO polarity
  ARM: dts: imx53-m53evk: Fix regulator enable GPIO polarity
  ARM: dts: imx53-mba53: Fix regulator enable GPIO polarity
  ARM: dts: imx53-tx53: Fix regulator enable GPIO polarity
  ARM: dts: imx6q-dmo-edmqmx6: Fix regulator enable GPIO polarity
  ARM: dts: kirkwood-blackarmor-nas220: Fix regulator enable GPIO
    polarity
  ARM: dts: omap4-duovero: Fix regulator enable GPIO polarity
  ARM: dts: kirkwood-nsa3x0-common: Fix regulator enable GPIO polarity
  ARM: dts: omap3-beagle-xm: Fix regulator enable GPIO polarity
  ARM: dts: omap3-beagle: Fix regulator enable GPIO polarity
  ARM: dts: omap3-overo-base: Fix regulator enable GPIO polarity
  ARM: dts: omap3-tao3530: Fix regulator enable GPIO polarity
  ARM: dts: tegra20-harmony: Fix regulator enable GPIO polarity
  ARM: dts: tegra20-iris-512: Fix regulator enable GPIO polarity
  ARM: dts: tegra20-seaboard: Fix regulator enable GPIO polarity
  ARM: dts: tegra20-ventana: Fix regulator enable GPIO polarity

 arch/arm/boot/dts/am437x-gp-evm.dts              | 1 -
 arch/arm/boot/dts/am43x-epos-evm.dts             | 1 -
 arch/arm/boot/dts/armada-388-gp.dts              | 2 --
 arch/arm/boot/dts/dove-cm-a510.dtsi              | 2 +-
 arch/arm/boot/dts/dove-sbc-a510.dts              | 4 ++--
 arch/arm/boot/dts/exynos5250-arndale.dts         | 2 +-
 arch/arm/boot/dts/imx23-evk.dts                  | 3 ++-
 arch/arm/boot/dts/imx23-stmp378x_devb.dts        | 3 ++-
 arch/arm/boot/dts/imx25-pdk.dts                  | 2 +-
 arch/arm/boot/dts/imx28-cfa10036.dts             | 2 +-
 arch/arm/boot/dts/imx28-evk.dts                  | 4 ++--
 arch/arm/boot/dts/imx28-m28cu3.dts               | 4 ++--
 arch/arm/boot/dts/imx28-m28evk.dts               | 6 +++---
 arch/arm/boot/dts/imx28-sps1.dts                 | 2 +-
 arch/arm/boot/dts/imx28-tx28.dts                 | 2 +-
 arch/arm/boot/dts/imx53-m53evk.dts               | 2 +-
 arch/arm/boot/dts/imx53-mba53.dts                | 2 +-
 arch/arm/boot/dts/imx53-tx53.dtsi                | 2 +-
 arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts          | 2 +-
 arch/arm/boot/dts/imx6qdl-tx6.dtsi               | 3 +--
 arch/arm/boot/dts/imx6sx-sdb.dtsi                | 2 +-
 arch/arm/boot/dts/kirkwood-blackarmor-nas220.dts | 4 ++--
 arch/arm/boot/dts/kirkwood-nsa3x0-common.dtsi    | 2 +-
 arch/arm/boot/dts/omap3-beagle-xm.dts            | 2 +-
 arch/arm/boot/dts/omap3-beagle.dts               | 2 +-
 arch/arm/boot/dts/omap3-evm-common.dtsi          | 1 -
 arch/arm/boot/dts/omap3-overo-base.dtsi          | 4 ++--
 arch/arm/boot/dts/omap3-sb-t35.dtsi              | 1 -
 arch/arm/boot/dts/omap3-tao3530.dtsi             | 3 +--
 arch/arm/boot/dts/omap4-duovero.dtsi             | 2 +-
 arch/arm/boot/dts/s5pv210-aquila.dts             | 2 +-
 arch/arm/boot/dts/s5pv210-goni.dts               | 4 ++--
 arch/arm/boot/dts/tegra20-harmony.dts            | 2 +-
 arch/arm/boot/dts/tegra20-iris-512.dts           | 2 +-
 arch/arm/boot/dts/tegra20-seaboard.dts           | 2 +-
 arch/arm/boot/dts/tegra20-ventana.dts            | 2 +-
 36 files changed, 41 insertions(+), 47 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 34/37] ARM: dts: tegra20-harmony: Fix regulator enable GPIO polarity

[Laurent Pinchart]

The enable GPIO is active low, but is flagged as active high in the gpio
property. As the gpio property flags are currently unused by the driver
this doesn't cause any issue for now, but will break later if the driver
starts making use of the flags. Fix it.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-harmony.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

diff --git a/arch/arm/boot/dts/tegra20-harmony.dts b/arch/arm/boot/dts/tegra20-harmony.dts
index b926a07b9443..d18801fe6730 100644
--- a/arch/arm/boot/dts/tegra20-harmony.dts
+++ b/arch/arm/boot/dts/tegra20-harmony.dts
@@ -688,7 +688,7 @@
 			regulator-name = "vdd_1v5";
 			regulator-min-microvolt = <1500000>;
 			regulator-max-microvolt = <1500000>;
-			gpio = <&pmic 0 GPIO_ACTIVE_HIGH>;
+			gpio = <&pmic 0 GPIO_ACTIVE_LOW>;
 		};
 
 		regulator@2 {
-- 
2.4.9

[PATCH 35/37] ARM: dts: tegra20-iris-512: Fix regulator enable GPIO polarity

[Laurent Pinchart]

The enable GPIO is active low, but is flagged as active high in the gpio
property. As the gpio property flags are currently unused by the driver
this doesn't cause any issue for now, but will break later if the driver
starts making use of the flags. Fix it.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-iris-512.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

diff --git a/arch/arm/boot/dts/tegra20-iris-512.dts b/arch/arm/boot/dts/tegra20-iris-512.dts
index 1dd7d7bfdfcc..9b04a367575c 100644
--- a/arch/arm/boot/dts/tegra20-iris-512.dts
+++ b/arch/arm/boot/dts/tegra20-iris-512.dts
@@ -85,7 +85,7 @@
 			regulator-max-microvolt = <5000000>;
 			regulator-boot-on;
 			regulator-always-on;
-			gpio = <&gpio TEGRA_GPIO(W, 2) GPIO_ACTIVE_HIGH>;
+			gpio = <&gpio TEGRA_GPIO(W, 2) GPIO_ACTIVE_LOW>;
 		};
 
 		vcc_sd_reg: regulator@1 {
-- 
2.4.9

[PATCH 36/37] ARM: dts: tegra20-seaboard: Fix regulator enable GPIO polarity

[Laurent Pinchart]

The enable GPIO is active low, but is flagged as active high in the gpio
property. As the gpio property flags are currently unused by the driver
this doesn't cause any issue for now, but will break later if the driver
starts making use of the flags. Fix it.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-seaboard.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts b/arch/arm/boot/dts/tegra20-seaboard.dts
index aea8994b35f2..1d790cd59c21 100644
--- a/arch/arm/boot/dts/tegra20-seaboard.dts
+++ b/arch/arm/boot/dts/tegra20-seaboard.dts
@@ -850,7 +850,7 @@
 			regulator-name = "vdd_1v5";
 			regulator-min-microvolt = <1500000>;
 			regulator-max-microvolt = <1500000>;
-			gpio = <&pmic 0 GPIO_ACTIVE_HIGH>;
+			gpio = <&pmic 0 GPIO_ACTIVE_LOW>;
 		};
 
 		regulator@2 {
-- 
2.4.9

[PATCH 37/37] ARM: dts: tegra20-ventana: Fix regulator enable GPIO polarity

[Laurent Pinchart]

The enable GPIO is active low, but is flagged as active high in the gpio
property. As the gpio property flags are currently unused by the driver
this doesn't cause any issue for now, but will break later if the driver
starts making use of the flags. Fix it.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index 04c58e9ca490..ba1fc1487c69 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -635,7 +635,7 @@
 			regulator-name = "vdd_1v5";
 			regulator-min-microvolt = <1500000>;
 			regulator-max-microvolt = <1500000>;
-			gpio = <&pmic 0 GPIO_ACTIVE_HIGH>;
+			gpio = <&pmic 0 GPIO_ACTIVE_LOW>;
 		};
 
 		regulator@2 {
-- 
2.4.9

Re: [PATCH 37/37] ARM: dts: tegra20-ventana: Fix regulator enable GPIO polarity

[Stephen Warren]

On 10/12/2015 03:13 PM, Laurent Pinchart wrote:
> The enable GPIO is active low,

It'd be good to mention a justification for that statement in the 
patches, since the cover letter isn't going to be checked in.

> but is flagged as active high in the gpio
> property. As the gpio property flags are currently unused by the driver
> this doesn't cause any issue for now, but will break later if the driver
> starts making use of the flags. Fix it.

IIRC the history here was that for some bizarre reason not all GPIO 
bindings contained an active-high/low flag and there was resistance to 
extending them in a backwards compatible way. So the regulator binding 
needed the separate property to represent this. For bindings that did 
have the flag, we had to set the GPIO flag to active-high, so that if 
anything started honoring the GPIO flags (e.g. I thikn the gpiod API 
does today, but the legacy GPIO API doesn't), we wouldn't apply both 
"active low indicators", and end up driving an active-high signal, and 
breaking things.

So while this change is logically correct when read in isolation (and 
for Harmony, Seaboard, and Ventana I verified that these regulators do 
use an active-low GPIO), I worry that making it makes mistakes likely 
later. How would we mitigate that?

Re: [PATCH 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [151012 14:17]:
> Hello,
> 
> While working on regulators, GPIOs and DT I noticed that many of our DT source
> files incorrectly describe fixed regulators. The common error patterns are
> 
> - Usage of the undefined (and never parsed) enable-active-low property
> - Usage of the enable-active-high property without specifying an enable GPIO
> - Typos in the enabl GPIO property name (gpios instead of gpio)
> - Mismatch between the enable-active-high property (or the lack thereof) and
>   the enable GPIO flags
> 
> This patch series fixes those issues in all the DT sources after locating the
> errors using the following script.
> 
> ------------------------------------------------------------------------------
> #!/bin/sh
> 
> echo $1
> cat $1 | awk '
> BEGIN {
> 	open_drain = 0;
> 	active_high = 0;
> 	gpio = 0;
> 	flags = 0;
> }
> 
> match($0, /([a-zA-Z0-9@_-]*) {/, ary) {
> 	name = ary[1];
> }
> 
> /compatible.*"regulator-fixed"/ {
> 	found = 1;
> }
> 
> /enable-active-high/ {
> 	active_high = 1;
> }
> 
> /gpio-open-drain/ {
> 	open_drain = 1;
> }
> 
> match($0, /gpio += <.* ([^ ]*)>/, ary) {
> 	gpio = 1;
> 	flags = ary[1];
> 	if (flags == 0)
> 		flags = "GPIO_ACTIVE_HIGH";
> }
> 
> /}/ {
> 	if (found) {
> 		if (gpio) {
> 			print "\t" name ": active high " active_high " " flags " open drain " open_drain;
> 			if ((active_high && flags == "GPIO_ACTIVE_LOW") ||
> 			    (!active_high && flags == "GPIO_ACTIVE_HIGH"))
> 				print "WARNING: enable-active-high and flags do not match"
> 		} else {
> 			if (active_high)
> 				print "WARNING: active high without GPIO"
> 			if (open_drain)
> 				print "WARNING: open drain without GPIO"
> 		}
> 	}
> 
> 	gpio = 0;
> 	found = 0;
> 	active_high = 0;
> 	open_drain = 0;
> 	flags = 0;
> }
> '
> ------------------------------------------------------------------------------
> 
> All patches except for the ones touching omap3-beagle-xm and omap3-overo-base
> are untested as I lack test hardware.
> 
> As there's no dependency between the patches touching different source files
> the appropriate maintainers could take their share of the patches in their
> tree. Alternatively I could send a single pull request after collecting all
> acks but that might be more complex.

Nice clean-up. For omaps, there's an earlier patch posted by
Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org> as "[PATCH] ARM: dts: Use
defined GPIO constants in flags cell for OMAP2+ boards". Can you guys do some
cross checking and let me know which combination I should appluy for omaps?

Regards,

Tony
--
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 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[Javier Martinez Canillas]

Hello Tony,

On 10/12/2015 11:46 PM, Tony Lindgren wrote:
> * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [151012 14:17]:
>> Hello,
>>
>> While working on regulators, GPIOs and DT I noticed that many of our DT source
>> files incorrectly describe fixed regulators. The common error patterns are
>>
>> - Usage of the undefined (and never parsed) enable-active-low property
>> - Usage of the enable-active-high property without specifying an enable GPIO
>> - Typos in the enabl GPIO property name (gpios instead of gpio)
>> - Mismatch between the enable-active-high property (or the lack thereof) and
>>   the enable GPIO flags
>>
>> This patch series fixes those issues in all the DT sources after locating the
>> errors using the following script.
>>
>> ------------------------------------------------------------------------------
>> #!/bin/sh
>>
>> echo $1
>> cat $1 | awk '
>> BEGIN {
>> 	open_drain = 0;
>> 	active_high = 0;
>> 	gpio = 0;
>> 	flags = 0;
>> }
>>
>> match($0, /([a-zA-Z0-9@_-]*) {/, ary) {
>> 	name = ary[1];
>> }
>>
>> /compatible.*"regulator-fixed"/ {
>> 	found = 1;
>> }
>>
>> /enable-active-high/ {
>> 	active_high = 1;
>> }
>>
>> /gpio-open-drain/ {
>> 	open_drain = 1;
>> }
>>
>> match($0, /gpio += <.* ([^ ]*)>/, ary) {
>> 	gpio = 1;
>> 	flags = ary[1];
>> 	if (flags == 0)
>> 		flags = "GPIO_ACTIVE_HIGH";
>> }
>>
>> /}/ {
>> 	if (found) {
>> 		if (gpio) {
>> 			print "\t" name ": active high " active_high " " flags " open drain " open_drain;
>> 			if ((active_high && flags == "GPIO_ACTIVE_LOW") ||
>> 			    (!active_high && flags == "GPIO_ACTIVE_HIGH"))
>> 				print "WARNING: enable-active-high and flags do not match"
>> 		} else {
>> 			if (active_high)
>> 				print "WARNING: active high without GPIO"
>> 			if (open_drain)
>> 				print "WARNING: open drain without GPIO"
>> 		}
>> 	}
>>
>> 	gpio = 0;
>> 	found = 0;
>> 	active_high = 0;
>> 	open_drain = 0;
>> 	flags = 0;
>> }
>> '
>> ------------------------------------------------------------------------------
>>
>> All patches except for the ones touching omap3-beagle-xm and omap3-overo-base
>> are untested as I lack test hardware.
>>
>> As there's no dependency between the patches touching different source files
>> the appropriate maintainers could take their share of the patches in their
>> tree. Alternatively I could send a single pull request after collecting all
>> acks but that might be more complex.
> 
> Nice clean-up. For omaps, there's an earlier patch posted by
> Javier Martinez Canillas <javier@osg.samsung.com> as "[PATCH] ARM: dts: Use
> defined GPIO constants in flags cell for OMAP2+ boards". Can you guys do some
> cross checking and let me know which combination I should appluy for omaps?
>

Since Laurent's changes for OMAP are part of a bigger series and my patch
was only for OMAP, probably makes sense for you to pick his patches and I
can re-spin mine on top of that.

BTW, I posted as a single patch since the changes were trivial but maybe
that made handling these conflicts harder and I should split the changes
instead, since I'll resend anyways.

What do you prefer? a patch per SoC family (i.e: OMAP{2,3,4,5}) or patch
per board DTS?
 
> Regards,
> 
> Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[Laurent Pinchart]

Hi Javier,

On Tuesday 13 October 2015 00:19:20 Javier Martinez Canillas wrote:
> On 10/12/2015 11:46 PM, Tony Lindgren wrote:
> > * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [151012 14:17]:
> >> Hello,
> >> 
> >> While working on regulators, GPIOs and DT I noticed that many of our DT
> >> source files incorrectly describe fixed regulators. The common error
> >> patterns are
> >> 
> >> - Usage of the undefined (and never parsed) enable-active-low property
> >> - Usage of the enable-active-high property without specifying an enable
> >>   GPIO
> >> - Typos in the enabl GPIO property name (gpios instead of gpio)
> >> - Mismatch between the enable-active-high property (or the lack thereof)
> >>   and the enable GPIO flags
> >> 
> >> This patch series fixes those issues in all the DT sources after locating
> >> the errors using the following script.
> >> 
> >> -------------------------------------------------------------------------
> >> !/bin/sh
> >> 
> >> echo $1
> >> cat $1 | awk '
> >> BEGIN {
> >> 	open_drain = 0;
> >> 	active_high = 0;
> >> 	gpio = 0;
> >> 	flags = 0;
> >> }
> >> 
> >> match($0, /([a-zA-Z0-9@_-]*) {/, ary) {
> >> 	name = ary[1];
> >> }
> >> 
> >> /compatible.*"regulator-fixed"/ {
> >> 	found = 1;
> >> }
> >> 
> >> /enable-active-high/ {
> >> 	active_high = 1;
> >> }
> >> 
> >> /gpio-open-drain/ {
> >> 	open_drain = 1;
> >> }
> >> 
> >> match($0, /gpio += <.* ([^ ]*)>/, ary) {
> >> 	gpio = 1;
> >> 	flags = ary[1];
> >> 	if (flags == 0)
> >> 		flags = "GPIO_ACTIVE_HIGH";
> >> }
> >> 
> >> /}/ {
> >> 	if (found) {
> >> 		if (gpio) {
> >> 			print "\t" name ": active high " active_high " " flags " open 
drain "
> >> 			open_drain;
> >> 			if ((active_high && flags == "GPIO_ACTIVE_LOW") ||
> >> 			    (!active_high && flags == "GPIO_ACTIVE_HIGH"))
> >> 				print "WARNING: enable-active-high and flags do not 
match"
> >> 		} else {
> >> 			if (active_high)
> >> 				print "WARNING: active high without GPIO"
> >> 			if (open_drain)
> >> 				print "WARNING: open drain without GPIO"
> >> 		}
> >> 	}
> >> 	
> >> 	gpio = 0;
> >> 	found = 0;
> >> 	active_high = 0;
> >> 	open_drain = 0;
> >> 	flags = 0;
> >> }
> >> '
> >> -------------------------------------------------------------------------
> >> 
> >> All patches except for the ones touching omap3-beagle-xm and
> >> omap3-overo-base are untested as I lack test hardware.
> >> 
> >> As there's no dependency between the patches touching different source
> >> files the appropriate maintainers could take their share of the patches
> >> in their tree. Alternatively I could send a single pull request after
> >> collecting all acks but that might be more complex.
> > 
> > Nice clean-up. For omaps, there's an earlier patch posted by
> > Javier Martinez Canillas <javier@osg.samsung.com> as "[PATCH] ARM: dts:
> > Use defined GPIO constants in flags cell for OMAP2+ boards". Can you guys
> > do some cross checking and let me know which combination I should appluy
> > for omaps?
>
> Since Laurent's changes for OMAP are part of a bigger series and my patch
> was only for OMAP, probably makes sense for you to pick his patches and I
> can re-spin mine on top of that.
> 
> BTW, I posted as a single patch since the changes were trivial but maybe
> that made handling these conflicts harder and I should split the changes
> instead, since I'll resend anyways.
> 
> What do you prefer? a patch per SoC family (i.e: OMAP{2,3,4,5}) or patch
> per board DTS?

My series will likely miss the next merge window as more discussion is needed. 
I'll thus respin the patches on top of yours, please proceed without caring 
about this.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [151012 15:26]:
> Hi Javier,
> 
> On Tuesday 13 October 2015 00:19:20 Javier Martinez Canillas wrote:
> > On 10/12/2015 11:46 PM, Tony Lindgren wrote:
> > > * Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> [151012 14:17]:
> > >> Hello,
> > >> 
> > >> While working on regulators, GPIOs and DT I noticed that many of our DT
> > >> source files incorrectly describe fixed regulators. The common error
> > >> patterns are
> > >> 
> > >> - Usage of the undefined (and never parsed) enable-active-low property
> > >> - Usage of the enable-active-high property without specifying an enable
> > >>   GPIO
> > >> - Typos in the enabl GPIO property name (gpios instead of gpio)
> > >> - Mismatch between the enable-active-high property (or the lack thereof)
> > >>   and the enable GPIO flags
> > >> 
> > >> This patch series fixes those issues in all the DT sources after locating
> > >> the errors using the following script.
> > >> 
> > >> -------------------------------------------------------------------------
> > >> !/bin/sh
> > >> 
> > >> echo $1
> > >> cat $1 | awk '
> > >> BEGIN {
> > >> 	open_drain = 0;
> > >> 	active_high = 0;
> > >> 	gpio = 0;
> > >> 	flags = 0;
> > >> }
> > >> 
> > >> match($0, /([a-zA-Z0-9@_-]*) {/, ary) {
> > >> 	name = ary[1];
> > >> }
> > >> 
> > >> /compatible.*"regulator-fixed"/ {
> > >> 	found = 1;
> > >> }
> > >> 
> > >> /enable-active-high/ {
> > >> 	active_high = 1;
> > >> }
> > >> 
> > >> /gpio-open-drain/ {
> > >> 	open_drain = 1;
> > >> }
> > >> 
> > >> match($0, /gpio += <.* ([^ ]*)>/, ary) {
> > >> 	gpio = 1;
> > >> 	flags = ary[1];
> > >> 	if (flags == 0)
> > >> 		flags = "GPIO_ACTIVE_HIGH";
> > >> }
> > >> 
> > >> /}/ {
> > >> 	if (found) {
> > >> 		if (gpio) {
> > >> 			print "\t" name ": active high " active_high " " flags " open 
> drain "
> > >> 			open_drain;
> > >> 			if ((active_high && flags == "GPIO_ACTIVE_LOW") ||
> > >> 			    (!active_high && flags == "GPIO_ACTIVE_HIGH"))
> > >> 				print "WARNING: enable-active-high and flags do not 
> match"
> > >> 		} else {
> > >> 			if (active_high)
> > >> 				print "WARNING: active high without GPIO"
> > >> 			if (open_drain)
> > >> 				print "WARNING: open drain without GPIO"
> > >> 		}
> > >> 	}
> > >> 	
> > >> 	gpio = 0;
> > >> 	found = 0;
> > >> 	active_high = 0;
> > >> 	open_drain = 0;
> > >> 	flags = 0;
> > >> }
> > >> '
> > >> -------------------------------------------------------------------------
> > >> 
> > >> All patches except for the ones touching omap3-beagle-xm and
> > >> omap3-overo-base are untested as I lack test hardware.
> > >> 
> > >> As there's no dependency between the patches touching different source
> > >> files the appropriate maintainers could take their share of the patches
> > >> in their tree. Alternatively I could send a single pull request after
> > >> collecting all acks but that might be more complex.
> > > 
> > > Nice clean-up. For omaps, there's an earlier patch posted by
> > > Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org> as "[PATCH] ARM: dts:
> > > Use defined GPIO constants in flags cell for OMAP2+ boards". Can you guys
> > > do some cross checking and let me know which combination I should appluy
> > > for omaps?
> >
> > Since Laurent's changes for OMAP are part of a bigger series and my patch
> > was only for OMAP, probably makes sense for you to pick his patches and I
> > can re-spin mine on top of that.
> > 
> > BTW, I posted as a single patch since the changes were trivial but maybe
> > that made handling these conflicts harder and I should split the changes
> > instead, since I'll resend anyways.
> > 
> > What do you prefer? a patch per SoC family (i.e: OMAP{2,3,4,5}) or patch
> > per board DTS?
> 
> My series will likely miss the next merge window as more discussion is needed. 
> I'll thus respin the patches on top of yours, please proceed without caring 
> about this.

OK applying Javier's patch into omap-for-v4.4/dt then.

Regards,

Tony
--
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 37/37] ARM: dts: tegra20-ventana: Fix regulator enable GPIO polarity

[Laurent Pinchart]

Hi Stephen,

On Monday 12 October 2015 15:34:43 Stephen Warren wrote:
> On 10/12/2015 03:13 PM, Laurent Pinchart wrote:
> > The enable GPIO is active low,
> 
> It'd be good to mention a justification for that statement in the
> patches, since the cover letter isn't going to be checked in.
> 
> > but is flagged as active high in the gpio
> > property. As the gpio property flags are currently unused by the driver
> > this doesn't cause any issue for now, but will break later if the driver
> > starts making use of the flags. Fix it.
> 
> IIRC the history here was that for some bizarre reason not all GPIO
> bindings contained an active-high/low flag and there was resistance to
> extending them in a backwards compatible way. So the regulator binding
> needed the separate property to represent this. For bindings that did
> have the flag, we had to set the GPIO flag to active-high, so that if
> anything started honoring the GPIO flags (e.g. I thikn the gpiod API
> does today, but the legacy GPIO API doesn't), we wouldn't apply both
> "active low indicators", and end up driving an active-high signal, and
> breaking things.
> 
> So while this change is logically correct when read in isolation (and
> for Harmony, Seaboard, and Ventana I verified that these regulators do
> use an active-low GPIO), I worry that making it makes mistakes likely
> later. How would we mitigate that?

That's a very good point. Is the resistance to move to the standard GPIO 
active low/high flags still present, or is it now only history ? In other 
words, could we aim for using GPIO flags as the primary method to specify 
polarities, and fall back to the custom properties for backward compatibility 
(and possibly for GPIO controllers that don't support the flags) ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[Sascha Hauer]

On Tue, Oct 13, 2015 at 12:12:29AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> While working on regulators, GPIOs and DT I noticed that many of our DT source
> files incorrectly describe fixed regulators. The common error patterns are
> 
> - Usage of the undefined (and never parsed) enable-active-low property
> - Usage of the enable-active-high property without specifying an enable GPIO
> - Typos in the enabl GPIO property name (gpios instead of gpio)
> - Mismatch between the enable-active-high property (or the lack thereof) and
>   the enable GPIO flags
> 
> This patch series fixes those issues in all the DT sources after locating the
> errors using the following script.

Nice. For the i.MX boards:

Reviewed-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[Shawn Guo]

On Tue, Oct 13, 2015 at 12:12:29AM +0300, Laurent Pinchart wrote:
> Laurent Pinchart (37):
...
>   ARM: imx6sx-sdb: Fix typo in regulator enable GPIO property
...
>   ARM: dts: imx6qdl-tx6: Fix regulator enable GPIO polarity
...
>   ARM: dts: imx23-evk: Fix regulator enable GPIO polarity
>   ARM: dts: imx23-stmp378x_devb: Fix regulator enable GPIO polarity
>   ARM: dts: imx25-pdk: Fix regulator enable GPIO polarity
>   ARM: dts: imx28-cfa10036: Fix regulator enable GPIO polarity
>   ARM: dts: imx28-evk: Fix regulator enable GPIO polarity
>   ARM: dts: imx28-m28cu3: Fix regulator enable GPIO polarity
>   ARM: dts: imx28-m28evk: Fix regulator enable GPIO polarity
>   ARM: dts: imx28-sps1: Fix regulator enable GPIO polarity
>   ARM: dts: imx28-tx28: Fix regulator enable GPIO polarity
>   ARM: dts: imx53-m53evk: Fix regulator enable GPIO polarity
>   ARM: dts: imx53-mba53: Fix regulator enable GPIO polarity
>   ARM: dts: imx53-tx53: Fix regulator enable GPIO polarity
>   ARM: dts: imx6q-dmo-edmqmx6: Fix regulator enable GPIO polarity

Applied these 15 patches, thanks.

Re: [PATCH 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[Laurent Pinchart]

Hi Shawn,

On Tuesday 13 October 2015 22:09:46 Shawn Guo wrote:
> On Tue, Oct 13, 2015 at 12:12:29AM +0300, Laurent Pinchart wrote:
> > Laurent Pinchart (37):
> ...
> 
> >   ARM: imx6sx-sdb: Fix typo in regulator enable GPIO property
> 
> ...
> 
> >   ARM: dts: imx6qdl-tx6: Fix regulator enable GPIO polarity
> 
> ...
> 
> >   ARM: dts: imx23-evk: Fix regulator enable GPIO polarity
> >   ARM: dts: imx23-stmp378x_devb: Fix regulator enable GPIO polarity
> >   ARM: dts: imx25-pdk: Fix regulator enable GPIO polarity
> >   ARM: dts: imx28-cfa10036: Fix regulator enable GPIO polarity
> >   ARM: dts: imx28-evk: Fix regulator enable GPIO polarity
> >   ARM: dts: imx28-m28cu3: Fix regulator enable GPIO polarity
> >   ARM: dts: imx28-m28evk: Fix regulator enable GPIO polarity
> >   ARM: dts: imx28-sps1: Fix regulator enable GPIO polarity
> >   ARM: dts: imx28-tx28: Fix regulator enable GPIO polarity
> >   ARM: dts: imx53-m53evk: Fix regulator enable GPIO polarity
> >   ARM: dts: imx53-mba53: Fix regulator enable GPIO polarity
> >   ARM: dts: imx53-tx53: Fix regulator enable GPIO polarity
> >   ARM: dts: imx6q-dmo-edmqmx6: Fix regulator enable GPIO polarity
> 
> Applied these 15 patches, thanks.

There's ongoing discussions regarding whether this is the right thing to do. 
Please see http://www.spinics.net/lists/arm-kernel/msg451724.html. It's thus a 
bit early to apply the patches at this point I'm afraid.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/37] ARM: dts: Fix fixed regulators enable GPIO polarity

[Shawn Guo]

On Tue, Oct 13, 2015 at 05:17:24PM +0300, Laurent Pinchart wrote:
> Hi Shawn,
> 
> On Tuesday 13 October 2015 22:09:46 Shawn Guo wrote:
> > On Tue, Oct 13, 2015 at 12:12:29AM +0300, Laurent Pinchart wrote:
> > > Laurent Pinchart (37):
> > ...
> > 
> > >   ARM: imx6sx-sdb: Fix typo in regulator enable GPIO property
> > 
> > ...
> > 
> > >   ARM: dts: imx6qdl-tx6: Fix regulator enable GPIO polarity
> > 
> > ...
> > 
> > >   ARM: dts: imx23-evk: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx23-stmp378x_devb: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx25-pdk: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx28-cfa10036: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx28-evk: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx28-m28cu3: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx28-m28evk: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx28-sps1: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx28-tx28: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx53-m53evk: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx53-mba53: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx53-tx53: Fix regulator enable GPIO polarity
> > >   ARM: dts: imx6q-dmo-edmqmx6: Fix regulator enable GPIO polarity
> > 
> > Applied these 15 patches, thanks.
> 
> There's ongoing discussions regarding whether this is the right thing to do. 
> Please see http://www.spinics.net/lists/arm-kernel/msg451724.html. It's thus a 
> bit early to apply the patches at this point I'm afraid.

Okay, dropped them except the first one which fixes a typo for
imx6sx-sdb.

Shawn

Re: [PATCH 37/37] ARM: dts: tegra20-ventana: Fix regulator enable GPIO polarity

[Stephen Warren]

On 10/12/2015 04:24 PM, Laurent Pinchart wrote:
> Hi Stephen,
>
> On Monday 12 October 2015 15:34:43 Stephen Warren wrote:
>> On 10/12/2015 03:13 PM, Laurent Pinchart wrote:
>>> The enable GPIO is active low,
>>
>> It'd be good to mention a justification for that statement in the
>> patches, since the cover letter isn't going to be checked in.
>>
>>> but is flagged as active high in the gpio
>>> property. As the gpio property flags are currently unused by the driver
>>> this doesn't cause any issue for now, but will break later if the driver
>>> starts making use of the flags. Fix it.
>>
>> IIRC the history here was that for some bizarre reason not all GPIO
>> bindings contained an active-high/low flag and there was resistance to
>> extending them in a backwards compatible way. So the regulator binding
>> needed the separate property to represent this. For bindings that did
>> have the flag, we had to set the GPIO flag to active-high, so that if
>> anything started honoring the GPIO flags (e.g. I thikn the gpiod API
>> does today, but the legacy GPIO API doesn't), we wouldn't apply both
>> "active low indicators", and end up driving an active-high signal, and
>> breaking things.
>>
>> So while this change is logically correct when read in isolation (and
>> for Harmony, Seaboard, and Ventana I verified that these regulators do
>> use an active-low GPIO), I worry that making it makes mistakes likely
>> later. How would we mitigate that?
>
> That's a very good point. Is the resistance to move to the standard GPIO
> active low/high flags still present, or is it now only history ?

This was a few years back, so I don't remember the details; it might 
have been as simple as "some bindings don't already have GPIO flags, and 
I'd rather get GPIO regulators implemented first before thinking about 
fixing that" or it could have been "some bindings don't already have 
GPIO flags, and there's ${some reason} why it's not possible to solve 
that in a backwards-compatible fashion" (recalling that DT bindings must 
evolve in a backwards-compatible fashion since they're an ABI). 
Unfortunately, you'd have to read through the mailing list posts related 
to the patches that defined the GPIO regulator bindings or added the 
nodes to DT.

> In other
> words, could we aim for using GPIO flags as the primary method to specify
> polarities, and fall back to the custom properties for backward compatibility
> (and possibly for GPIO controllers that don't support the flags) ?

I don't think we can switch to using GPIO flags, without changing the 
compatible values for the relevant DT nodes.

For one, we'd need some way of actively marking the nodes to say whether 
they are written to expect that the GPIO flags or the other properties 
be used. It's not possible in all cases to determine this automatically. 
For example, if enable-active-high it's fairly clear we should honor 
this flag, yet if it's missing does that mean the GPIO is active-low or 
simply that the node was written to expect that the GPIO flags be used 
instead?

Also, old DTs must work with new kernels (and preferably also, new DTs 
must work with old kernels). If the GPIO flags are wrong in current DTs, 
then we can't use them. Of course, there's an argument that the 
backwards-compatibility constraint doesn't apply to buggy DTs, just to 
correctly written DTs. However, if we deliberately chose to make all 
regulator GPIO flags ACTIVE_HIGH, then the current DTs aren't buggy.