Re: [PATCH 01/19 v3] regulator: fixed: Convert to use GPIO descriptor only

[Linus Walleij]

On Fri, Jun 1, 2018 at 11:35 AM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> On Mon, May 14, 2018 at 10:06:22AM +0200, Linus Walleij wrote:

>> As we augmented the regulator core to accept a GPIO descriptor instead
>> of a GPIO number, we can augment the fixed GPIO regulator to look up
>> and pass that descriptor directly from device tree or board GPIO
>> descriptor look up tables.
(...)
> This causes an HDMI display regression on Jetson TK1. From what I can
> tell, the problem is that we now get a double-inversion for low-active
> GPIOs. For example, we have this in the Jetson TK1 device tree:
>
>                 vdd_hdmi_pll: regulator@11 {
>                         compatible = "regulator-fixed";
>                         reg = <11>;
>                         regulator-name = "+1.05V_RUN_AVDD_HDMI_PLL";
>                         regulator-min-microvolt = <1050000>;
>                         regulator-max-microvolt = <1050000>;
>                         gpio = <&gpio TEGRA_GPIO(H, 7) GPIO_ACTIVE_LOW>;
>                         vin-supply = <&vdd_1v05_run>;
>                 };
>
> We've got GPIO_ACTIVE_LOW for the regulator's enable GPIO and since we
> don't have enable-active-high, the regulator core will treat the GPIO as
> low active. The presence of the GPIO_ACTIVE_LOW flag will cause the GPIO
> polarity to be inversed, transparently in gpiolib, and the lack of the
> enable-active-high property causes the GPIO polarity to inversed as
> well, so we effectively end up with a high-active enable GPIO for one
> which should really be low-active.

I dug into this. It turns out there are some layers of mess....

The DT binding for "regulator-fixed" specifies that enable-active-high
should be set for polarity inversion. For historical reasons, I guess,
we screwed up. The example in the binding uses that.

I interpreted the binding such that also specifying GPIO_ACTIVE_LOW
was essentially illegal, so I patched gpiolib in commit
a603a2b8d86ee93ee2107da8ca75fd854fd4ff32
"gpio: of: Add special quirk to parse regulator flags"
so it looks like that:

+       if (IS_ENABLED(CONFIG_REGULATOR) &&
+           (of_device_is_compatible(np, "reg-fixed-voltage") ||
+            of_device_is_compatible(np, "regulator-gpio"))) {
+               /*
+                * The regulator GPIO handles are specified such that the
+                * presence or absence of "enable-active-high" solely controls
+                * the polarity of the GPIO line. Any phandle flags must
+                * be actively ignored.
+                */
+               if (*flags & OF_GPIO_ACTIVE_LOW) {
+                       pr_warn("%s GPIO handle specifies active low -
ignored\n",
+                               of_node_full_name(np));
+                       *flags &= ~OF_GPIO_ACTIVE_LOW;
+               }
+               if (!of_property_read_bool(np, "enable-active-high"))
+                       *flags |= OF_GPIO_ACTIVE_LOW;
+       }

This means no matter if you specify active low, the GPIO
will be treated as active low, as this is the default behaviour.
We will warn if things are overspecified as active low on the
flag as well.

The second clause (notice nagation!) specifies that
if enable-acive-high flag is NOT specified, we will be active
low.

Sadly this only handled the undocumented fixed
regulator binding "reg-fixed-voltage". So I need to fix it
for "regulator-fixed" as well, and then it "should work".

I will follow up with a patch.

Yours,
Linus Walleij