On 2/19/19 4:18 PM, Thierry Reding wrote:
> On Sat, Feb 16, 2019 at 02:46:27PM +0100, marek.va...@gmail.com wrote:
>> From: Marek Vasut <marek.vasut+rene...@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+rene...@gmail.com>
>> Cc: Geert Uytterhoeven <geert+rene...@glider.be>
>> Cc: Jan Kotas <j...@cadence.com>
>> Cc: Linus Walleij <linus.wall...@linaro.org>
>> Cc: Mark Brown <broo...@kernel.org>
>> Cc: Wolfram Sang <wsa+rene...@sang-engineering.com>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-g...@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

Reply via email to