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

[Marek Vasut <marek.vasut@xxxxxxxxx>]

On 2/19/19 4:18 PM, Thierry Reding wrote:
> On Sat, Feb 16, 2019 at 02:46:27PM +0100, marek.vasut@xxxxxxxxx wrote:
>> From: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>> Cc: Jan Kotas <jank@xxxxxxxxxxx>
>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> Cc: Mark Brown <broonie@xxxxxxxxxx>
>> Cc: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
>> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
>> To: linux-gpio@xxxxxxxxxxxxxxx
>> ---
>>  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