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

[Marek Vasut <marek.vasut@xxxxxxxxx>]

On 2/17/19 10:21 PM, Linus Walleij wrote:
> On Sat, Feb 16, 2019 at 2:46 PM <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
> 
> 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