[PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[marek.vasut]

From: Marek Vasut <marek.vasut+renesas@gmail.com>

Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors")
the GPIO regulator had inverted the polarity of the control GPIO. This
problem manifested itself on systems with DT containing the following
description (snippet from salvator-common.dtsi):

	gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
	gpios-states = <1>;
	states = <3300000 1
		  1800000 0>;

Prior to the aforementioned commit, the gpio-regulator code used
gpio_request_array() to claim the GPIO(s) specified in the "gpios"
DT node, while the commit changed that to devm_gpiod_get_index().

The legacy gpio_request_array() calls gpio_request_one() and then
gpiod_request(), which parses the DT flags of the "gpios" node and
populates the GPIO descriptor flags field accordingly.

The new devm_gpiod_get_index() calls gpiod_get_index(), then
of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL,
and then of_gpio_flags_quirks(). Since commit a603a2b8d86e
("gpio: of: Add special quirk to parse regulator flags"),
of_gpio_flags_quirks() contains a quirk for regulator-gpio
which was never triggered by the legacy gpio_request_array()
code path, but is triggered by devm_gpiod_get_index() code
path.

This quirk checks whether a GPIO is associated with a fixed
or gpio-regulator and if so, checks two additional conditions.
First, whether such GPIO is active-low, and if so, ignores the
active-low flag. Second, whether the regulator DT node does
have an "enable-active-high" property and if the property is
NOT present, sets the GPIO flags as active-low.

The second check triggers a problem, since it is applied to all
GPIOs associated with a gpio-regulator, rather than only on the
"enable" GPIOs, as the old code did. This changes the way the
gpio-regulator interprets the DT description of the control
GPIOs.

The old code using gpio_request_array() explicitly parsed the
"enable-active-high" DT property and only applied it to the
GPIOs described in the "enable-gpios" DT node, and only if
those were present.

This patch fixes the quirk code by only applying the quirk
to "enable-gpios", thus restoring the old behavior.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Jan Kotas <jank@cadence.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: linux-renesas-soc@vger.kernel.org
To: linux-gpio@vger.kernel.org
---
 drivers/gpio/gpiolib-of.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 03ec3ddaba60..1b4c741e0635 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -84,6 +84,7 @@ static void of_gpio_flags_quirks(struct device_node *np,
 	 * Note that active low is the default.
 	 */
 	if (IS_ENABLED(CONFIG_REGULATOR) &&
+	    !strcmp(propname, "enable-gpio") &&
 	    (of_device_is_compatible(np, "regulator-fixed") ||
 	     of_device_is_compatible(np, "reg-fixed-voltage") ||
 	     of_device_is_compatible(np, "regulator-gpio"))) {
-- 
2.19.2

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Wolfram Sang]

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


> +	    !strcmp(propname, "enable-gpio") &&

I haven't looked at the actual issue, but one minor nit I usually have:

I think 'strcmp() == 0' is easier to read since the above is too easy to
be mistaken as "if not equal". I have just been bitten again by this
after a long day, although I should know better... :/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Marek Vasut]

On 2/16/19 3:04 PM, Wolfram Sang wrote:
> 
>> +	    !strcmp(propname, "enable-gpio") &&
> 
> I haven't looked at the actual issue, but one minor nit I usually have:
> 
> I think 'strcmp() == 0' is easier to read since the above is too easy to
> be mistaken as "if not equal". I have just been bitten again by this
> after a long day, although I should know better... :/

Note that I just followed the coding style in the rest of the function,
if we want to update the coding style the gpiolib, that's for separate
patch.

-- 
Best regards,
Marek Vasut

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Wolfram Sang]

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

On Sat, Feb 16, 2019 at 03:06:55PM +0100, Marek Vasut wrote:
> On 2/16/19 3:04 PM, Wolfram Sang wrote:
> > 
> >> +	    !strcmp(propname, "enable-gpio") &&
> > 
> > I haven't looked at the actual issue, but one minor nit I usually have:
> > 
> > I think 'strcmp() == 0' is easier to read since the above is too easy to
> > be mistaken as "if not equal". I have just been bitten again by this
> > after a long day, although I should know better... :/
> 
> Note that I just followed the coding style in the rest of the function,
> if we want to update the coding style the gpiolib, that's for separate
> patch.

Fair enough.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Marek Vasut]

On 2/16/19 3:47 PM, Wolfram Sang wrote:
> On Sat, Feb 16, 2019 at 03:06:55PM +0100, Marek Vasut wrote:
>> On 2/16/19 3:04 PM, Wolfram Sang wrote:
>>>
>>>> +	    !strcmp(propname, "enable-gpio") &&
>>>
>>> I haven't looked at the actual issue, but one minor nit I usually have:
>>>
>>> I think 'strcmp() == 0' is easier to read since the above is too easy to
>>> be mistaken as "if not equal". I have just been bitten again by this
>>> after a long day, although I should know better... :/
>>
>> Note that I just followed the coding style in the rest of the function,
>> if we want to update the coding style the gpiolib, that's for separate
>> patch.
> 
> Fair enough.
> 
Could be something Gustavo can look at, since he seems to be doing a lot
of automated cleanups ?

-- 
Best regards,
Marek Vasut

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Linus Walleij]

On Sat, Feb 16, 2019 at 2:46 PM <marek.vasut@gmail.com> wrote:

> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors")
> the GPIO regulator had inverted the polarity of the control GPIO. This
> problem manifested itself on systems with DT containing the following
> description (snippet from salvator-common.dtsi):
>
>         gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
>         gpios-states = <1>;
>         states = <3300000 1
>                   1800000 0>;
>
> Prior to the aforementioned commit, the gpio-regulator code used
> gpio_request_array() to claim the GPIO(s) specified in the "gpios"
> DT node, while the commit changed that to devm_gpiod_get_index().
>
> The legacy gpio_request_array() calls gpio_request_one() and then
> gpiod_request(), which parses the DT flags of the "gpios" node and
> populates the GPIO descriptor flags field accordingly.
>
> The new devm_gpiod_get_index() calls gpiod_get_index(), then
> of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL,
> and then of_gpio_flags_quirks(). Since commit a603a2b8d86e
> ("gpio: of: Add special quirk to parse regulator flags"),
> of_gpio_flags_quirks() contains a quirk for regulator-gpio
> which was never triggered by the legacy gpio_request_array()
> code path, but is triggered by devm_gpiod_get_index() code
> path.
>
> This quirk checks whether a GPIO is associated with a fixed
> or gpio-regulator and if so, checks two additional conditions.
> First, whether such GPIO is active-low, and if so, ignores the
> active-low flag. Second, whether the regulator DT node does
> have an "enable-active-high" property and if the property is
> NOT present, sets the GPIO flags as active-low.
>
> The second check triggers a problem, since it is applied to all
> GPIOs associated with a gpio-regulator, rather than only on the
> "enable" GPIOs, as the old code did. This changes the way the
> gpio-regulator interprets the DT description of the control
> GPIOs.
>
> The old code using gpio_request_array() explicitly parsed the
> "enable-active-high" DT property and only applied it to the
> GPIOs described in the "enable-gpios" DT node, and only if
> those were present.
>
> This patch fixes the quirk code by only applying the quirk
> to "enable-gpios", thus restoring the old behavior.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Jan Kotas <jank@cadence.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-gpio@vger.kernel.org

Patch applied for v5.1, the regulator and GPIO changes should work
fine together in -next and merge nicely in the merge window.

Bonus for a long and descriptive commit message and keeping
me company over the Facebook chat while fixing it up! :D

Yours,
Linus Walleij

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Marek Vasut]

On 2/17/19 10:21 PM, Linus Walleij wrote:
> On Sat, Feb 16, 2019 at 2:46 PM <marek.vasut@gmail.com> wrote:
> 
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors")
>> the GPIO regulator had inverted the polarity of the control GPIO. This
>> problem manifested itself on systems with DT containing the following
>> description (snippet from salvator-common.dtsi):
>>
>>         gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
>>         gpios-states = <1>;
>>         states = <3300000 1
>>                   1800000 0>;
>>
>> Prior to the aforementioned commit, the gpio-regulator code used
>> gpio_request_array() to claim the GPIO(s) specified in the "gpios"
>> DT node, while the commit changed that to devm_gpiod_get_index().
>>
>> The legacy gpio_request_array() calls gpio_request_one() and then
>> gpiod_request(), which parses the DT flags of the "gpios" node and
>> populates the GPIO descriptor flags field accordingly.
>>
>> The new devm_gpiod_get_index() calls gpiod_get_index(), then
>> of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL,
>> and then of_gpio_flags_quirks(). Since commit a603a2b8d86e
>> ("gpio: of: Add special quirk to parse regulator flags"),
>> of_gpio_flags_quirks() contains a quirk for regulator-gpio
>> which was never triggered by the legacy gpio_request_array()
>> code path, but is triggered by devm_gpiod_get_index() code
>> path.
>>
>> This quirk checks whether a GPIO is associated with a fixed
>> or gpio-regulator and if so, checks two additional conditions.
>> First, whether such GPIO is active-low, and if so, ignores the
>> active-low flag. Second, whether the regulator DT node does
>> have an "enable-active-high" property and if the property is
>> NOT present, sets the GPIO flags as active-low.
>>
>> The second check triggers a problem, since it is applied to all
>> GPIOs associated with a gpio-regulator, rather than only on the
>> "enable" GPIOs, as the old code did. This changes the way the
>> gpio-regulator interprets the DT description of the control
>> GPIOs.
>>
>> The old code using gpio_request_array() explicitly parsed the
>> "enable-active-high" DT property and only applied it to the
>> GPIOs described in the "enable-gpios" DT node, and only if
>> those were present.
>>
>> This patch fixes the quirk code by only applying the quirk
>> to "enable-gpios", thus restoring the old behavior.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Jan Kotas <jank@cadence.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-gpio@vger.kernel.org
> 
> Patch applied for v5.1, the regulator and GPIO changes should work
> fine together in -next and merge nicely in the merge window.
> 
> Bonus for a long and descriptive commit message and keeping
> me company over the Facebook chat while fixing it up! :D

:-))

-- 
Best regards,
Marek Vasut

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Thierry Reding]

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

On Sat, Feb 16, 2019 at 02:46:27PM +0100, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors")
> the GPIO regulator had inverted the polarity of the control GPIO. This
> problem manifested itself on systems with DT containing the following
> description (snippet from salvator-common.dtsi):
> 
> 	gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
> 	gpios-states = <1>;
> 	states = <3300000 1
> 		  1800000 0>;
> 
> Prior to the aforementioned commit, the gpio-regulator code used
> gpio_request_array() to claim the GPIO(s) specified in the "gpios"
> DT node, while the commit changed that to devm_gpiod_get_index().
> 
> The legacy gpio_request_array() calls gpio_request_one() and then
> gpiod_request(), which parses the DT flags of the "gpios" node and
> populates the GPIO descriptor flags field accordingly.
> 
> The new devm_gpiod_get_index() calls gpiod_get_index(), then
> of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL,
> and then of_gpio_flags_quirks(). Since commit a603a2b8d86e
> ("gpio: of: Add special quirk to parse regulator flags"),
> of_gpio_flags_quirks() contains a quirk for regulator-gpio
> which was never triggered by the legacy gpio_request_array()
> code path, but is triggered by devm_gpiod_get_index() code
> path.
> 
> This quirk checks whether a GPIO is associated with a fixed
> or gpio-regulator and if so, checks two additional conditions.
> First, whether such GPIO is active-low, and if so, ignores the
> active-low flag. Second, whether the regulator DT node does
> have an "enable-active-high" property and if the property is
> NOT present, sets the GPIO flags as active-low.
> 
> The second check triggers a problem, since it is applied to all
> GPIOs associated with a gpio-regulator, rather than only on the
> "enable" GPIOs, as the old code did. This changes the way the
> gpio-regulator interprets the DT description of the control
> GPIOs.
> 
> The old code using gpio_request_array() explicitly parsed the
> "enable-active-high" DT property and only applied it to the
> GPIOs described in the "enable-gpios" DT node, and only if
> those were present.
> 
> This patch fixes the quirk code by only applying the quirk
> to "enable-gpios", thus restoring the old behavior.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Jan Kotas <jank@cadence.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-gpio@vger.kernel.org
> ---
>  drivers/gpio/gpiolib-of.c | 1 +
>  1 file changed, 1 insertion(+)

This causes a runtime regression on Jetson TX1. The symptoms are that
HDMI monitors are no longer reliably detected as plugged in. The reason
is because the GPIO polarity for the HDMI +5V regulator is now reversed
and therefore the HPD signal, which is usually fed by the +5V signal,
does not reflect the correct value.

Now it's somewhat confusing to me why this wasn't broken before. We have
this in device tree:

		vdd_hdmi: regulator@10 {
			compatible = "regulator-fixed";
			reg = <10>;
			regulator-name = "VDD_HDMI_5V0";
			regulator-min-microvolt = <5000000>;
			regulator-max-microvolt = <5000000>;
			gpio = <&exp1 12 GPIO_ACTIVE_LOW>;
			enable-active-high;
			vin-supply = <&vdd_5v0_sys>;
		};

This is from arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi. The above
is obviously wrong because it specifies GPIO_ACTIVE_LOW and I do see the
warning from the GPIO library about how this is being ignored. However,
the above works fine on today's linux-next with this commit reverted
because the quirks will cause the GPIO_ACTIVE_LOW flag to be ignored.

However, with this commit applied, the quirk will be skipped for the
regulator and cause the GPIO_ACTIVE_LOW flag to be used to invert the
GPIO during enable and disable operations.

Now, this is clearly a bug in the DT and I'm going to send out a patch
to fix that up, but I think we need to revert this patch for now because
there may be other cases out there that are broken. The whole point of
these quirks is also to make sure that existing device trees continue to
work.

Also, checking for only the enable-gpio property is wrong in this case.
enable-gpio is only specified for regulator-gpio. The standard GPIO for
regulator-fixed is "gpio", so the quirks must be applied to that
property in order to ensure backwards-compatibility.

> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 03ec3ddaba60..1b4c741e0635 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -84,6 +84,7 @@ static void of_gpio_flags_quirks(struct device_node *np,
>  	 * Note that active low is the default.
>  	 */
>  	if (IS_ENABLED(CONFIG_REGULATOR) &&
> +	    !strcmp(propname, "enable-gpio") &&
>  	    (of_device_is_compatible(np, "regulator-fixed") ||
>  	     of_device_is_compatible(np, "reg-fixed-voltage") ||
>  	     of_device_is_compatible(np, "regulator-gpio"))) {

I think this would need to be something like the below to reflect what
you were trying to achieve, while keeping backwards compatibility:

--- >8 ---
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 1b4c741e0635..bddfc6102a50 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -84,10 +84,10 @@ static void of_gpio_flags_quirks(struct device_node *np,
 	 * Note that active low is the default.
 	 */
 	if (IS_ENABLED(CONFIG_REGULATOR) &&
-	    !strcmp(propname, "enable-gpio") &&
 	    (of_device_is_compatible(np, "regulator-fixed") ||
 	     of_device_is_compatible(np, "reg-fixed-voltage") ||
-	     of_device_is_compatible(np, "regulator-gpio"))) {
+	     (of_device_is_compatible(np, "regulator-gpio") &&
+	      strcmp(propname, "enable-gpio") == 0))) {
 		/*
 		 * The regulator GPIO handles are specified such that the
 		 * presence or absence of "enable-active-high" solely controls
--- >8 ---

That applied on top of today's linux-next fixes the regression for me
without a need to modify the device tree. Feel free to integrate that
into your patch.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Marek Vasut]

On 2/19/19 4:18 PM, Thierry Reding wrote:
> On Sat, Feb 16, 2019 at 02:46:27PM +0100, marek.vasut@gmail.com wrote:
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors")
>> the GPIO regulator had inverted the polarity of the control GPIO. This
>> problem manifested itself on systems with DT containing the following
>> description (snippet from salvator-common.dtsi):
>>
>> 	gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
>> 	gpios-states = <1>;
>> 	states = <3300000 1
>> 		  1800000 0>;
>>
>> Prior to the aforementioned commit, the gpio-regulator code used
>> gpio_request_array() to claim the GPIO(s) specified in the "gpios"
>> DT node, while the commit changed that to devm_gpiod_get_index().
>>
>> The legacy gpio_request_array() calls gpio_request_one() and then
>> gpiod_request(), which parses the DT flags of the "gpios" node and
>> populates the GPIO descriptor flags field accordingly.
>>
>> The new devm_gpiod_get_index() calls gpiod_get_index(), then
>> of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL,
>> and then of_gpio_flags_quirks(). Since commit a603a2b8d86e
>> ("gpio: of: Add special quirk to parse regulator flags"),
>> of_gpio_flags_quirks() contains a quirk for regulator-gpio
>> which was never triggered by the legacy gpio_request_array()
>> code path, but is triggered by devm_gpiod_get_index() code
>> path.
>>
>> This quirk checks whether a GPIO is associated with a fixed
>> or gpio-regulator and if so, checks two additional conditions.
>> First, whether such GPIO is active-low, and if so, ignores the
>> active-low flag. Second, whether the regulator DT node does
>> have an "enable-active-high" property and if the property is
>> NOT present, sets the GPIO flags as active-low.
>>
>> The second check triggers a problem, since it is applied to all
>> GPIOs associated with a gpio-regulator, rather than only on the
>> "enable" GPIOs, as the old code did. This changes the way the
>> gpio-regulator interprets the DT description of the control
>> GPIOs.
>>
>> The old code using gpio_request_array() explicitly parsed the
>> "enable-active-high" DT property and only applied it to the
>> GPIOs described in the "enable-gpios" DT node, and only if
>> those were present.
>>
>> This patch fixes the quirk code by only applying the quirk
>> to "enable-gpios", thus restoring the old behavior.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Jan Kotas <jank@cadence.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-gpio@vger.kernel.org
>> ---
>>  drivers/gpio/gpiolib-of.c | 1 +
>>  1 file changed, 1 insertion(+)
> 
> This causes a runtime regression on Jetson TX1. The symptoms are that
> HDMI monitors are no longer reliably detected as plugged in. The reason
> is because the GPIO polarity for the HDMI +5V regulator is now reversed
> and therefore the HPD signal, which is usually fed by the +5V signal,
> does not reflect the correct value.
> 
> Now it's somewhat confusing to me why this wasn't broken before. We have
> this in device tree:
> 
> 		vdd_hdmi: regulator@10 {
> 			compatible = "regulator-fixed";
> 			reg = <10>;
> 			regulator-name = "VDD_HDMI_5V0";
> 			regulator-min-microvolt = <5000000>;
> 			regulator-max-microvolt = <5000000>;
> 			gpio = <&exp1 12 GPIO_ACTIVE_LOW>;
> 			enable-active-high;
> 			vin-supply = <&vdd_5v0_sys>;
> 		};
> 
> This is from arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi. The above
> is obviously wrong because it specifies GPIO_ACTIVE_LOW and I do see the
> warning from the GPIO library about how this is being ignored. However,
> the above works fine on today's linux-next with this commit reverted
> because the quirks will cause the GPIO_ACTIVE_LOW flag to be ignored.
> 
> However, with this commit applied, the quirk will be skipped for the
> regulator and cause the GPIO_ACTIVE_LOW flag to be used to invert the
> GPIO during enable and disable operations.
> 
> Now, this is clearly a bug in the DT and I'm going to send out a patch
> to fix that up, but I think we need to revert this patch for now because
> there may be other cases out there that are broken.

This patch fixes a breakage on R-Car Gen3 platforms, so reverting this
patch will break those platforms. However, those platforms do not have a
buggy DT, unlike the Jetson.

> The whole point of
> these quirks is also to make sure that existing device trees continue to
> work.
> 
> Also, checking for only the enable-gpio property is wrong in this case.
> enable-gpio is only specified for regulator-gpio. The standard GPIO for
> regulator-fixed is "gpio", so the quirks must be applied to that
> property in order to ensure backwards-compatibility.
> 
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> index 03ec3ddaba60..1b4c741e0635 100644
>> --- a/drivers/gpio/gpiolib-of.c
>> +++ b/drivers/gpio/gpiolib-of.c
>> @@ -84,6 +84,7 @@ static void of_gpio_flags_quirks(struct device_node *np,
>>  	 * Note that active low is the default.
>>  	 */
>>  	if (IS_ENABLED(CONFIG_REGULATOR) &&
>> +	    !strcmp(propname, "enable-gpio") &&
>>  	    (of_device_is_compatible(np, "regulator-fixed") ||
>>  	     of_device_is_compatible(np, "reg-fixed-voltage") ||
>>  	     of_device_is_compatible(np, "regulator-gpio"))) {
> 
> I think this would need to be something like the below to reflect what
> you were trying to achieve, while keeping backwards compatibility:
> 
> --- >8 ---
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 1b4c741e0635..bddfc6102a50 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -84,10 +84,10 @@ static void of_gpio_flags_quirks(struct device_node *np,
>  	 * Note that active low is the default.
>  	 */
>  	if (IS_ENABLED(CONFIG_REGULATOR) &&
> -	    !strcmp(propname, "enable-gpio") &&
>  	    (of_device_is_compatible(np, "regulator-fixed") ||
>  	     of_device_is_compatible(np, "reg-fixed-voltage") ||
> -	     of_device_is_compatible(np, "regulator-gpio"))) {
> +	     (of_device_is_compatible(np, "regulator-gpio") &&
> +	      strcmp(propname, "enable-gpio") == 0))) {
>  		/*
>  		 * The regulator GPIO handles are specified such that the
>  		 * presence or absence of "enable-active-high" solely controls
> --- >8 ---
> 
> That applied on top of today's linux-next fixes the regression for me
> without a need to modify the device tree. Feel free to integrate that
> into your patch.

This one works on R-Car Gen3 too, so if we can add that on top of this
patch, fine by me . Can you send a formal patch ?

-- 
Best regards,
Marek Vasut

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Thierry Reding]

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

On Tue, Feb 19, 2019 at 04:25:59PM +0100, Marek Vasut wrote:
> On 2/19/19 4:18 PM, Thierry Reding wrote:
> > On Sat, Feb 16, 2019 at 02:46:27PM +0100, marek.vasut@gmail.com wrote:
> >> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>
> >> Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors")
> >> the GPIO regulator had inverted the polarity of the control GPIO. This
> >> problem manifested itself on systems with DT containing the following
> >> description (snippet from salvator-common.dtsi):
> >>
> >> 	gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
> >> 	gpios-states = <1>;
> >> 	states = <3300000 1
> >> 		  1800000 0>;
> >>
> >> Prior to the aforementioned commit, the gpio-regulator code used
> >> gpio_request_array() to claim the GPIO(s) specified in the "gpios"
> >> DT node, while the commit changed that to devm_gpiod_get_index().
> >>
> >> The legacy gpio_request_array() calls gpio_request_one() and then
> >> gpiod_request(), which parses the DT flags of the "gpios" node and
> >> populates the GPIO descriptor flags field accordingly.
> >>
> >> The new devm_gpiod_get_index() calls gpiod_get_index(), then
> >> of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL,
> >> and then of_gpio_flags_quirks(). Since commit a603a2b8d86e
> >> ("gpio: of: Add special quirk to parse regulator flags"),
> >> of_gpio_flags_quirks() contains a quirk for regulator-gpio
> >> which was never triggered by the legacy gpio_request_array()
> >> code path, but is triggered by devm_gpiod_get_index() code
> >> path.
> >>
> >> This quirk checks whether a GPIO is associated with a fixed
> >> or gpio-regulator and if so, checks two additional conditions.
> >> First, whether such GPIO is active-low, and if so, ignores the
> >> active-low flag. Second, whether the regulator DT node does
> >> have an "enable-active-high" property and if the property is
> >> NOT present, sets the GPIO flags as active-low.
> >>
> >> The second check triggers a problem, since it is applied to all
> >> GPIOs associated with a gpio-regulator, rather than only on the
> >> "enable" GPIOs, as the old code did. This changes the way the
> >> gpio-regulator interprets the DT description of the control
> >> GPIOs.
> >>
> >> The old code using gpio_request_array() explicitly parsed the
> >> "enable-active-high" DT property and only applied it to the
> >> GPIOs described in the "enable-gpios" DT node, and only if
> >> those were present.
> >>
> >> This patch fixes the quirk code by only applying the quirk
> >> to "enable-gpios", thus restoring the old behavior.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >> Cc: Jan Kotas <jank@cadence.com>
> >> Cc: Linus Walleij <linus.walleij@linaro.org>
> >> Cc: Mark Brown <broonie@kernel.org>
> >> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> To: linux-gpio@vger.kernel.org
> >> ---
> >>  drivers/gpio/gpiolib-of.c | 1 +
> >>  1 file changed, 1 insertion(+)
> > 
> > This causes a runtime regression on Jetson TX1. The symptoms are that
> > HDMI monitors are no longer reliably detected as plugged in. The reason
> > is because the GPIO polarity for the HDMI +5V regulator is now reversed
> > and therefore the HPD signal, which is usually fed by the +5V signal,
> > does not reflect the correct value.
> > 
> > Now it's somewhat confusing to me why this wasn't broken before. We have
> > this in device tree:
> > 
> > 		vdd_hdmi: regulator@10 {
> > 			compatible = "regulator-fixed";
> > 			reg = <10>;
> > 			regulator-name = "VDD_HDMI_5V0";
> > 			regulator-min-microvolt = <5000000>;
> > 			regulator-max-microvolt = <5000000>;
> > 			gpio = <&exp1 12 GPIO_ACTIVE_LOW>;
> > 			enable-active-high;
> > 			vin-supply = <&vdd_5v0_sys>;
> > 		};
> > 
> > This is from arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi. The above
> > is obviously wrong because it specifies GPIO_ACTIVE_LOW and I do see the
> > warning from the GPIO library about how this is being ignored. However,
> > the above works fine on today's linux-next with this commit reverted
> > because the quirks will cause the GPIO_ACTIVE_LOW flag to be ignored.
> > 
> > However, with this commit applied, the quirk will be skipped for the
> > regulator and cause the GPIO_ACTIVE_LOW flag to be used to invert the
> > GPIO during enable and disable operations.
> > 
> > Now, this is clearly a bug in the DT and I'm going to send out a patch
> > to fix that up, but I think we need to revert this patch for now because
> > there may be other cases out there that are broken.
> 
> This patch fixes a breakage on R-Car Gen3 platforms, so reverting this
> patch will break those platforms. However, those platforms do not have a
> buggy DT, unlike the Jetson.

Erm... that's not how we do things. You can't break one platform just to
make another work. By all means let's fix breakage on R-Car Gen3
platforms, but let's do it in a way that keeps everyone else happy as
well.

> > The whole point of
> > these quirks is also to make sure that existing device trees continue to
> > work.
> > 
> > Also, checking for only the enable-gpio property is wrong in this case.
> > enable-gpio is only specified for regulator-gpio. The standard GPIO for
> > regulator-fixed is "gpio", so the quirks must be applied to that
> > property in order to ensure backwards-compatibility.
> > 
> >> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >> index 03ec3ddaba60..1b4c741e0635 100644
> >> --- a/drivers/gpio/gpiolib-of.c
> >> +++ b/drivers/gpio/gpiolib-of.c
> >> @@ -84,6 +84,7 @@ static void of_gpio_flags_quirks(struct device_node *np,
> >>  	 * Note that active low is the default.
> >>  	 */
> >>  	if (IS_ENABLED(CONFIG_REGULATOR) &&
> >> +	    !strcmp(propname, "enable-gpio") &&
> >>  	    (of_device_is_compatible(np, "regulator-fixed") ||
> >>  	     of_device_is_compatible(np, "reg-fixed-voltage") ||
> >>  	     of_device_is_compatible(np, "regulator-gpio"))) {
> > 
> > I think this would need to be something like the below to reflect what
> > you were trying to achieve, while keeping backwards compatibility:
> > 
> > --- >8 ---
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 1b4c741e0635..bddfc6102a50 100644
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -84,10 +84,10 @@ static void of_gpio_flags_quirks(struct device_node *np,
> >  	 * Note that active low is the default.
> >  	 */
> >  	if (IS_ENABLED(CONFIG_REGULATOR) &&
> > -	    !strcmp(propname, "enable-gpio") &&
> >  	    (of_device_is_compatible(np, "regulator-fixed") ||
> >  	     of_device_is_compatible(np, "reg-fixed-voltage") ||
> > -	     of_device_is_compatible(np, "regulator-gpio"))) {
> > +	     (of_device_is_compatible(np, "regulator-gpio") &&
> > +	      strcmp(propname, "enable-gpio") == 0))) {
> >  		/*
> >  		 * The regulator GPIO handles are specified such that the
> >  		 * presence or absence of "enable-active-high" solely controls
> > --- >8 ---
> > 
> > That applied on top of today's linux-next fixes the regression for me
> > without a need to modify the device tree. Feel free to integrate that
> > into your patch.
> 
> This one works on R-Car Gen3 too, so if we can add that on top of this
> patch, fine by me . Can you send a formal patch ?

Linus,

can you squash this into Marek's original commit? I'd rather not make
that two patches because that would potentially cause bisectability
issues.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Marek Vasut]

On 2/19/19 5:08 PM, Thierry Reding wrote:
> On Tue, Feb 19, 2019 at 04:25:59PM +0100, Marek Vasut wrote:
>> On 2/19/19 4:18 PM, Thierry Reding wrote:
>>> On Sat, Feb 16, 2019 at 02:46:27PM +0100, marek.vasut@gmail.com wrote:
>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>
>>>> Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors")
>>>> the GPIO regulator had inverted the polarity of the control GPIO. This
>>>> problem manifested itself on systems with DT containing the following
>>>> description (snippet from salvator-common.dtsi):
>>>>
>>>> 	gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
>>>> 	gpios-states = <1>;
>>>> 	states = <3300000 1
>>>> 		  1800000 0>;
>>>>
>>>> Prior to the aforementioned commit, the gpio-regulator code used
>>>> gpio_request_array() to claim the GPIO(s) specified in the "gpios"
>>>> DT node, while the commit changed that to devm_gpiod_get_index().
>>>>
>>>> The legacy gpio_request_array() calls gpio_request_one() and then
>>>> gpiod_request(), which parses the DT flags of the "gpios" node and
>>>> populates the GPIO descriptor flags field accordingly.
>>>>
>>>> The new devm_gpiod_get_index() calls gpiod_get_index(), then
>>>> of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL,
>>>> and then of_gpio_flags_quirks(). Since commit a603a2b8d86e
>>>> ("gpio: of: Add special quirk to parse regulator flags"),
>>>> of_gpio_flags_quirks() contains a quirk for regulator-gpio
>>>> which was never triggered by the legacy gpio_request_array()
>>>> code path, but is triggered by devm_gpiod_get_index() code
>>>> path.
>>>>
>>>> This quirk checks whether a GPIO is associated with a fixed
>>>> or gpio-regulator and if so, checks two additional conditions.
>>>> First, whether such GPIO is active-low, and if so, ignores the
>>>> active-low flag. Second, whether the regulator DT node does
>>>> have an "enable-active-high" property and if the property is
>>>> NOT present, sets the GPIO flags as active-low.
>>>>
>>>> The second check triggers a problem, since it is applied to all
>>>> GPIOs associated with a gpio-regulator, rather than only on the
>>>> "enable" GPIOs, as the old code did. This changes the way the
>>>> gpio-regulator interprets the DT description of the control
>>>> GPIOs.
>>>>
>>>> The old code using gpio_request_array() explicitly parsed the
>>>> "enable-active-high" DT property and only applied it to the
>>>> GPIOs described in the "enable-gpios" DT node, and only if
>>>> those were present.
>>>>
>>>> This patch fixes the quirk code by only applying the quirk
>>>> to "enable-gpios", thus restoring the old behavior.
>>>>
>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> Cc: Jan Kotas <jank@cadence.com>
>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>> Cc: Mark Brown <broonie@kernel.org>
>>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>> To: linux-gpio@vger.kernel.org
>>>> ---
>>>>  drivers/gpio/gpiolib-of.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>
>>> This causes a runtime regression on Jetson TX1. The symptoms are that
>>> HDMI monitors are no longer reliably detected as plugged in. The reason
>>> is because the GPIO polarity for the HDMI +5V regulator is now reversed
>>> and therefore the HPD signal, which is usually fed by the +5V signal,
>>> does not reflect the correct value.
>>>
>>> Now it's somewhat confusing to me why this wasn't broken before. We have
>>> this in device tree:
>>>
>>> 		vdd_hdmi: regulator@10 {
>>> 			compatible = "regulator-fixed";
>>> 			reg = <10>;
>>> 			regulator-name = "VDD_HDMI_5V0";
>>> 			regulator-min-microvolt = <5000000>;
>>> 			regulator-max-microvolt = <5000000>;
>>> 			gpio = <&exp1 12 GPIO_ACTIVE_LOW>;
>>> 			enable-active-high;
>>> 			vin-supply = <&vdd_5v0_sys>;
>>> 		};
>>>
>>> This is from arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi. The above
>>> is obviously wrong because it specifies GPIO_ACTIVE_LOW and I do see the
>>> warning from the GPIO library about how this is being ignored. However,
>>> the above works fine on today's linux-next with this commit reverted
>>> because the quirks will cause the GPIO_ACTIVE_LOW flag to be ignored.
>>>
>>> However, with this commit applied, the quirk will be skipped for the
>>> regulator and cause the GPIO_ACTIVE_LOW flag to be used to invert the
>>> GPIO during enable and disable operations.
>>>
>>> Now, this is clearly a bug in the DT and I'm going to send out a patch
>>> to fix that up, but I think we need to revert this patch for now because
>>> there may be other cases out there that are broken.
>>
>> This patch fixes a breakage on R-Car Gen3 platforms, so reverting this
>> patch will break those platforms. However, those platforms do not have a
>> buggy DT, unlike the Jetson.
> 
> Erm... that's not how we do things. You can't break one platform just to
> make another work. By all means let's fix breakage on R-Car Gen3
> platforms, but let's do it in a way that keeps everyone else happy as
> well.

Erm... that's not what I suggested. It's just that this regulator rework
( d6cd33ad7102 ) changed the behavior of the gpio-regulator because
there are a lot of obscure, inobvious and undocumented edge-cases in the
gpio-regulator code.

If we were to revert this patch, we'd have to revert the d6cd33ad7102
too to fix the Gen3. But since d6cd33ad7102 is a nice cleanup and it
makes sense, I'd rather if we fixed the situation, restored the DT
handling to the way it was before and moved forward.

Handling broken DTs only adds to the complexity, but I think this cannot
be helped, since those DTs can be stored in some ROM.

btw if you can, also look at
[PATCH] regulator: gpio: Reword the binding document
the binding document could use a once-over.

>>> The whole point of
>>> these quirks is also to make sure that existing device trees continue to
>>> work.
>>>
>>> Also, checking for only the enable-gpio property is wrong in this case.
>>> enable-gpio is only specified for regulator-gpio. The standard GPIO for
>>> regulator-fixed is "gpio", so the quirks must be applied to that
>>> property in order to ensure backwards-compatibility.
>>>
>>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>>>> index 03ec3ddaba60..1b4c741e0635 100644
>>>> --- a/drivers/gpio/gpiolib-of.c
>>>> +++ b/drivers/gpio/gpiolib-of.c
>>>> @@ -84,6 +84,7 @@ static void of_gpio_flags_quirks(struct device_node *np,
>>>>  	 * Note that active low is the default.
>>>>  	 */
>>>>  	if (IS_ENABLED(CONFIG_REGULATOR) &&
>>>> +	    !strcmp(propname, "enable-gpio") &&
>>>>  	    (of_device_is_compatible(np, "regulator-fixed") ||
>>>>  	     of_device_is_compatible(np, "reg-fixed-voltage") ||
>>>>  	     of_device_is_compatible(np, "regulator-gpio"))) {
>>>
>>> I think this would need to be something like the below to reflect what
>>> you were trying to achieve, while keeping backwards compatibility:
>>>
>>> --- >8 ---
>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>>> index 1b4c741e0635..bddfc6102a50 100644
>>> --- a/drivers/gpio/gpiolib-of.c
>>> +++ b/drivers/gpio/gpiolib-of.c
>>> @@ -84,10 +84,10 @@ static void of_gpio_flags_quirks(struct device_node *np,
>>>  	 * Note that active low is the default.
>>>  	 */
>>>  	if (IS_ENABLED(CONFIG_REGULATOR) &&
>>> -	    !strcmp(propname, "enable-gpio") &&
>>>  	    (of_device_is_compatible(np, "regulator-fixed") ||
>>>  	     of_device_is_compatible(np, "reg-fixed-voltage") ||
>>> -	     of_device_is_compatible(np, "regulator-gpio"))) {
>>> +	     (of_device_is_compatible(np, "regulator-gpio") &&
>>> +	      strcmp(propname, "enable-gpio") == 0))) {
>>>  		/*
>>>  		 * The regulator GPIO handles are specified such that the
>>>  		 * presence or absence of "enable-active-high" solely controls
>>> --- >8 ---
>>>
>>> That applied on top of today's linux-next fixes the regression for me
>>> without a need to modify the device tree. Feel free to integrate that
>>> into your patch.
>>
>> This one works on R-Car Gen3 too, so if we can add that on top of this
>> patch, fine by me . Can you send a formal patch ?
> 
> Linus,
> 
> can you squash this into Marek's original commit? I'd rather not make
> that two patches because that would potentially cause bisectability
> issues.

I am happy with this solution too, sure.

-- 
Best regards,
Marek Vasut

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Thierry Reding]

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

On Tue, Feb 19, 2019 at 05:30:35PM +0100, Marek Vasut wrote:
> On 2/19/19 5:08 PM, Thierry Reding wrote:
> > On Tue, Feb 19, 2019 at 04:25:59PM +0100, Marek Vasut wrote:
> >> On 2/19/19 4:18 PM, Thierry Reding wrote:
> >>> On Sat, Feb 16, 2019 at 02:46:27PM +0100, marek.vasut@gmail.com wrote:
> >>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>
> >>>> Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors")
> >>>> the GPIO regulator had inverted the polarity of the control GPIO. This
> >>>> problem manifested itself on systems with DT containing the following
> >>>> description (snippet from salvator-common.dtsi):
> >>>>
> >>>> 	gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
> >>>> 	gpios-states = <1>;
> >>>> 	states = <3300000 1
> >>>> 		  1800000 0>;
> >>>>
> >>>> Prior to the aforementioned commit, the gpio-regulator code used
> >>>> gpio_request_array() to claim the GPIO(s) specified in the "gpios"
> >>>> DT node, while the commit changed that to devm_gpiod_get_index().
> >>>>
> >>>> The legacy gpio_request_array() calls gpio_request_one() and then
> >>>> gpiod_request(), which parses the DT flags of the "gpios" node and
> >>>> populates the GPIO descriptor flags field accordingly.
> >>>>
> >>>> The new devm_gpiod_get_index() calls gpiod_get_index(), then
> >>>> of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL,
> >>>> and then of_gpio_flags_quirks(). Since commit a603a2b8d86e
> >>>> ("gpio: of: Add special quirk to parse regulator flags"),
> >>>> of_gpio_flags_quirks() contains a quirk for regulator-gpio
> >>>> which was never triggered by the legacy gpio_request_array()
> >>>> code path, but is triggered by devm_gpiod_get_index() code
> >>>> path.
> >>>>
> >>>> This quirk checks whether a GPIO is associated with a fixed
> >>>> or gpio-regulator and if so, checks two additional conditions.
> >>>> First, whether such GPIO is active-low, and if so, ignores the
> >>>> active-low flag. Second, whether the regulator DT node does
> >>>> have an "enable-active-high" property and if the property is
> >>>> NOT present, sets the GPIO flags as active-low.
> >>>>
> >>>> The second check triggers a problem, since it is applied to all
> >>>> GPIOs associated with a gpio-regulator, rather than only on the
> >>>> "enable" GPIOs, as the old code did. This changes the way the
> >>>> gpio-regulator interprets the DT description of the control
> >>>> GPIOs.
> >>>>
> >>>> The old code using gpio_request_array() explicitly parsed the
> >>>> "enable-active-high" DT property and only applied it to the
> >>>> GPIOs described in the "enable-gpios" DT node, and only if
> >>>> those were present.
> >>>>
> >>>> This patch fixes the quirk code by only applying the quirk
> >>>> to "enable-gpios", thus restoring the old behavior.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >>>> Cc: Jan Kotas <jank@cadence.com>
> >>>> Cc: Linus Walleij <linus.walleij@linaro.org>
> >>>> Cc: Mark Brown <broonie@kernel.org>
> >>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>>> Cc: linux-renesas-soc@vger.kernel.org
> >>>> To: linux-gpio@vger.kernel.org
> >>>> ---
> >>>>  drivers/gpio/gpiolib-of.c | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>
> >>> This causes a runtime regression on Jetson TX1. The symptoms are that
> >>> HDMI monitors are no longer reliably detected as plugged in. The reason
> >>> is because the GPIO polarity for the HDMI +5V regulator is now reversed
> >>> and therefore the HPD signal, which is usually fed by the +5V signal,
> >>> does not reflect the correct value.
> >>>
> >>> Now it's somewhat confusing to me why this wasn't broken before. We have
> >>> this in device tree:
> >>>
> >>> 		vdd_hdmi: regulator@10 {
> >>> 			compatible = "regulator-fixed";
> >>> 			reg = <10>;
> >>> 			regulator-name = "VDD_HDMI_5V0";
> >>> 			regulator-min-microvolt = <5000000>;
> >>> 			regulator-max-microvolt = <5000000>;
> >>> 			gpio = <&exp1 12 GPIO_ACTIVE_LOW>;
> >>> 			enable-active-high;
> >>> 			vin-supply = <&vdd_5v0_sys>;
> >>> 		};
> >>>
> >>> This is from arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi. The above
> >>> is obviously wrong because it specifies GPIO_ACTIVE_LOW and I do see the
> >>> warning from the GPIO library about how this is being ignored. However,
> >>> the above works fine on today's linux-next with this commit reverted
> >>> because the quirks will cause the GPIO_ACTIVE_LOW flag to be ignored.
> >>>
> >>> However, with this commit applied, the quirk will be skipped for the
> >>> regulator and cause the GPIO_ACTIVE_LOW flag to be used to invert the
> >>> GPIO during enable and disable operations.
> >>>
> >>> Now, this is clearly a bug in the DT and I'm going to send out a patch
> >>> to fix that up, but I think we need to revert this patch for now because
> >>> there may be other cases out there that are broken.
> >>
> >> This patch fixes a breakage on R-Car Gen3 platforms, so reverting this
> >> patch will break those platforms. However, those platforms do not have a
> >> buggy DT, unlike the Jetson.
> > 
> > Erm... that's not how we do things. You can't break one platform just to
> > make another work. By all means let's fix breakage on R-Car Gen3
> > platforms, but let's do it in a way that keeps everyone else happy as
> > well.
> 
> Erm... that's not what I suggested. It's just that this regulator rework
> ( d6cd33ad7102 ) changed the behavior of the gpio-regulator because
> there are a lot of obscure, inobvious and undocumented edge-cases in the
> gpio-regulator code.

Sorry if I misinterpreted what you were saying.

> If we were to revert this patch, we'd have to revert the d6cd33ad7102
> too to fix the Gen3. But since d6cd33ad7102 is a nice cleanup and it
> makes sense, I'd rather if we fixed the situation, restored the DT
> handling to the way it was before and moved forward.

Okay, sounds like we have the same goal, and from what you said earlier
the proposed fixup on top of your patch should fix this for everyone.
Let's hope there aren't any more nasty corner cases.

> Handling broken DTs only adds to the complexity, but I think this cannot
> be helped, since those DTs can be stored in some ROM.

FWIW, the Jetson TX1 DT isn't technically broken because it used to work
fine. It's just that it relied on the quirks for correctness. So we are
in the lucky situation that the DT is not in a ROM and we can easily
update it, but I agree, others may be in a different situation, so let's
fix this so that things are back to normal for everyone. That doesn't
mean that we shouldn't fix DTs when we can, so I'll still send out that
DT patch for Jetson TX1.

> btw if you can, also look at
> [PATCH] regulator: gpio: Reword the binding document
> the binding document could use a once-over.

Okay, I'll take a look.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Marek Vasut]

On 2/19/19 6:05 PM, Thierry Reding wrote:
> On Tue, Feb 19, 2019 at 05:30:35PM +0100, Marek Vasut wrote:
>> On 2/19/19 5:08 PM, Thierry Reding wrote:
>>> On Tue, Feb 19, 2019 at 04:25:59PM +0100, Marek Vasut wrote:
>>>> On 2/19/19 4:18 PM, Thierry Reding wrote:
>>>>> On Sat, Feb 16, 2019 at 02:46:27PM +0100, marek.vasut@gmail.com wrote:
>>>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>
>>>>>> Since commit d6cd33ad7102 ("regulator: gpio: Convert to use descriptors")
>>>>>> the GPIO regulator had inverted the polarity of the control GPIO. This
>>>>>> problem manifested itself on systems with DT containing the following
>>>>>> description (snippet from salvator-common.dtsi):
>>>>>>
>>>>>> 	gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>;
>>>>>> 	gpios-states = <1>;
>>>>>> 	states = <3300000 1
>>>>>> 		  1800000 0>;
>>>>>>
>>>>>> Prior to the aforementioned commit, the gpio-regulator code used
>>>>>> gpio_request_array() to claim the GPIO(s) specified in the "gpios"
>>>>>> DT node, while the commit changed that to devm_gpiod_get_index().
>>>>>>
>>>>>> The legacy gpio_request_array() calls gpio_request_one() and then
>>>>>> gpiod_request(), which parses the DT flags of the "gpios" node and
>>>>>> populates the GPIO descriptor flags field accordingly.
>>>>>>
>>>>>> The new devm_gpiod_get_index() calls gpiod_get_index(), then
>>>>>> of_find_gpio(), of_get_named_gpiod_flags() with flags != NULL,
>>>>>> and then of_gpio_flags_quirks(). Since commit a603a2b8d86e
>>>>>> ("gpio: of: Add special quirk to parse regulator flags"),
>>>>>> of_gpio_flags_quirks() contains a quirk for regulator-gpio
>>>>>> which was never triggered by the legacy gpio_request_array()
>>>>>> code path, but is triggered by devm_gpiod_get_index() code
>>>>>> path.
>>>>>>
>>>>>> This quirk checks whether a GPIO is associated with a fixed
>>>>>> or gpio-regulator and if so, checks two additional conditions.
>>>>>> First, whether such GPIO is active-low, and if so, ignores the
>>>>>> active-low flag. Second, whether the regulator DT node does
>>>>>> have an "enable-active-high" property and if the property is
>>>>>> NOT present, sets the GPIO flags as active-low.
>>>>>>
>>>>>> The second check triggers a problem, since it is applied to all
>>>>>> GPIOs associated with a gpio-regulator, rather than only on the
>>>>>> "enable" GPIOs, as the old code did. This changes the way the
>>>>>> gpio-regulator interprets the DT description of the control
>>>>>> GPIOs.
>>>>>>
>>>>>> The old code using gpio_request_array() explicitly parsed the
>>>>>> "enable-active-high" DT property and only applied it to the
>>>>>> GPIOs described in the "enable-gpios" DT node, and only if
>>>>>> those were present.
>>>>>>
>>>>>> This patch fixes the quirk code by only applying the quirk
>>>>>> to "enable-gpios", thus restoring the old behavior.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>> Cc: Jan Kotas <jank@cadence.com>
>>>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>>>> Cc: Mark Brown <broonie@kernel.org>
>>>>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>> To: linux-gpio@vger.kernel.org
>>>>>> ---
>>>>>>  drivers/gpio/gpiolib-of.c | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> This causes a runtime regression on Jetson TX1. The symptoms are that
>>>>> HDMI monitors are no longer reliably detected as plugged in. The reason
>>>>> is because the GPIO polarity for the HDMI +5V regulator is now reversed
>>>>> and therefore the HPD signal, which is usually fed by the +5V signal,
>>>>> does not reflect the correct value.
>>>>>
>>>>> Now it's somewhat confusing to me why this wasn't broken before. We have
>>>>> this in device tree:
>>>>>
>>>>> 		vdd_hdmi: regulator@10 {
>>>>> 			compatible = "regulator-fixed";
>>>>> 			reg = <10>;
>>>>> 			regulator-name = "VDD_HDMI_5V0";
>>>>> 			regulator-min-microvolt = <5000000>;
>>>>> 			regulator-max-microvolt = <5000000>;
>>>>> 			gpio = <&exp1 12 GPIO_ACTIVE_LOW>;
>>>>> 			enable-active-high;
>>>>> 			vin-supply = <&vdd_5v0_sys>;
>>>>> 		};
>>>>>
>>>>> This is from arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi. The above
>>>>> is obviously wrong because it specifies GPIO_ACTIVE_LOW and I do see the
>>>>> warning from the GPIO library about how this is being ignored. However,
>>>>> the above works fine on today's linux-next with this commit reverted
>>>>> because the quirks will cause the GPIO_ACTIVE_LOW flag to be ignored.
>>>>>
>>>>> However, with this commit applied, the quirk will be skipped for the
>>>>> regulator and cause the GPIO_ACTIVE_LOW flag to be used to invert the
>>>>> GPIO during enable and disable operations.
>>>>>
>>>>> Now, this is clearly a bug in the DT and I'm going to send out a patch
>>>>> to fix that up, but I think we need to revert this patch for now because
>>>>> there may be other cases out there that are broken.
>>>>
>>>> This patch fixes a breakage on R-Car Gen3 platforms, so reverting this
>>>> patch will break those platforms. However, those platforms do not have a
>>>> buggy DT, unlike the Jetson.
>>>
>>> Erm... that's not how we do things. You can't break one platform just to
>>> make another work. By all means let's fix breakage on R-Car Gen3
>>> platforms, but let's do it in a way that keeps everyone else happy as
>>> well.
>>
>> Erm... that's not what I suggested. It's just that this regulator rework
>> ( d6cd33ad7102 ) changed the behavior of the gpio-regulator because
>> there are a lot of obscure, inobvious and undocumented edge-cases in the
>> gpio-regulator code.
> 
> Sorry if I misinterpreted what you were saying.

Sorry, my English just sucks ;-/

>> If we were to revert this patch, we'd have to revert the d6cd33ad7102
>> too to fix the Gen3. But since d6cd33ad7102 is a nice cleanup and it
>> makes sense, I'd rather if we fixed the situation, restored the DT
>> handling to the way it was before and moved forward.
> 
> Okay, sounds like we have the same goal, and from what you said earlier
> the proposed fixup on top of your patch should fix this for everyone.
> Let's hope there aren't any more nasty corner cases.

Sounds good!

>> Handling broken DTs only adds to the complexity, but I think this cannot
>> be helped, since those DTs can be stored in some ROM.
> 
> FWIW, the Jetson TX1 DT isn't technically broken because it used to work
> fine. It's just that it relied on the quirks for correctness. So we are
> in the lucky situation that the DT is not in a ROM and we can easily
> update it, but I agree, others may be in a different situation, so let's
> fix this so that things are back to normal for everyone. That doesn't
> mean that we shouldn't fix DTs when we can, so I'll still send out that
> DT patch for Jetson TX1.

Thanks!

>> btw if you can, also look at
>> [PATCH] regulator: gpio: Reword the binding document
>> the binding document could use a once-over.
> 
> Okay, I'll take a look.

:)

-- 
Best regards,
Marek Vasut

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Linus Walleij]

On Tue, Feb 19, 2019 at 5:30 PM Marek Vasut <marek.vasut@gmail.com> wrote:

> Handling broken DTs only adds to the complexity, but I think this cannot
> be helped, since those DTs can be stored in some ROM.

I agree, but if the broken DT needs to be supported going forward
I would like it to be quite explicit, so that the code down in
gpiolib-of.c does this:

/*
 * Comment about this machine
 */
if (of_machine_is_compatible("nvidia,problematic-board") {
   /* Quirk for this specific DT for this machine*/
}

This makes the standard behavior clear and avoids the risk
of applying hairy machine-specific quirks to any other machine,
but even more importantly it makes it easy for people reading
the code to see what is going on and why.

Yours,
Linus Walleij

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Thierry Reding]

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

On Wed, Feb 20, 2019 at 10:01:50AM +0100, Linus Walleij wrote:
> On Tue, Feb 19, 2019 at 5:30 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> 
> > Handling broken DTs only adds to the complexity, but I think this cannot
> > be helped, since those DTs can be stored in some ROM.
> 
> I agree, but if the broken DT needs to be supported going forward
> I would like it to be quite explicit, so that the code down in
> gpiolib-of.c does this:
> 
> /*
>  * Comment about this machine
>  */
> if (of_machine_is_compatible("nvidia,problematic-board") {
>    /* Quirk for this specific DT for this machine*/
> }
> 
> This makes the standard behavior clear and avoids the risk
> of applying hairy machine-specific quirks to any other machine,
> but even more importantly it makes it easy for people reading
> the code to see what is going on and why.

At the risk of repeating myself: the device tree bindings for regulator-
fixed are very clear: any flags in the specifier are ignored and the
polarity of the GPIO is determined exclusively by the enable-active-high
property.

This means that, contrary to what I said earlier, the Jetson TX1 device
tree is not actually buggy. Is it confusing and inconsistent? Yes it is.
But it was never wrong according to the bindings.

The commit that you merged broke the ABI set forth in the bindings by
not applying the quirks necessary to adhere to the bindings for these
fixed regulators. It was supposed to only avoid the quirks for GPIO
regulators, which are an entirely different thing.

Given the above, I don't think adding the quirk for a specific machine
is acceptable. There could be any number of device trees that rely on
the literal interpretation of the device tree bindings and they would
all remain broken if we added a quirk only for Jetson TX1.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gpio: of: Apply regulator-gpio quirk only to enable-gpios

[Linus Walleij]

On Tue, Feb 19, 2019 at 5:08 PM Thierry Reding <thierry.reding@gmail.com> wrote:

> Linus,
>
> can you squash this into Marek's original commit? I'd rather not make
> that two patches because that would potentially cause bisectability
> issues.

Sadly no, new development is merged on top.
Bisection breaks sometimes, it's just how it is.

(OK if you tell me off very sternly about how this destroys
everything for you then I will have to revert all development
and re-pull pull requests etc but I'd rather not.)

Could you send a patch fixing the patch, preferably along
the lines of
if (of_machine_is_compatible("nvidia,problematic-board"))
as I outlined?

Yours,
Linus Walleij