Re: [PATCH] regulator: gpio: Reword the binding document

[Marek Vasut <marek.vasut@xxxxxxxxx>]

On 2/18/19 10:30 AM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Sun, Feb 17, 2019 at 8:00 AM <marek.vasut@xxxxxxxxx> wrote:
>> From: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
>> Reword the binding document to make it clear how the propeties work
>> and which properties affect which other properties.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
>> Cc: Harald Geyer <harald@xxxxxxxxx>
>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> Cc: Mark Brown <broonie@xxxxxxxxxx>
>> Cc: Rob Herring <robh@xxxxxxxxxx>
>> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
>> To: devicetree@xxxxxxxxxxxxxxx
>> ---
>> Note: The recent gpio-regulator rework caused breakage. While the
>>       changes in the gpio-regulator code were according to the DT
>>       binding document, they stopped working with older DTs. Make
>>       the binding document clearer to prevent such breakage in the
>>       future.
> 
> Thanks, adding more worms to the can, below...
> 
>> --- a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
>> +++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
>> @@ -4,16 +4,25 @@ Required properties:
>>  - compatible           : Must be "regulator-gpio".
>>  - regulator-name       : Defined in regulator.txt as optional, but required
>>                           here.
>> -- states               : Selection of available voltages and GPIO configs.
>> -                          if there are no states, then use a fixed regulator
>> +- states               : Selection of available voltages/currents provided by
>> +                         this regulator and matching GPIO configurations to
>> +                         achieve them. If there are no states in the "states"
>> +                         array, use a fixed regulator instead.
>>
>>  Optional properties:
>> -- enable-gpio          : GPIO to use to enable/disable the regulator.
>> -- gpios                        : GPIO group used to control voltage.
>> -- gpios-states         : gpios pin's initial states array. 0: LOW, 1: HIGH.
>> -                         defualt is LOW if nothing is specified.
>> +- enable-gpio          : GPIO used to enable/disable the regulator.
> 
> According to modern GPIO rules, that should be "enable-gpios" (plural) ...

Isn't that changing the ABI spec then ?

>> +                         Warning, the GPIO phandle flags are ignored and the
>> +                         GPIO polarity is controlled solely by the presence
>> +                         of "enable-active-high" DT property. This is due to
>> +                         compatibility with old DTs.
> 
> Wasn't the purpose of the various *active-* flags to accommodate GPIOs
> lacking a flags cell (i.e. typically #gpio-cells = <1>)?

Yes

> When the flags cell is present, there's indeed opportunity for confusion
> (and breakage), combined with the presence/lack of *active-* below...

The code is rather clear that the flags cell is ignored.

drivers/gpio/gpiolib-of.c:
 91 /*
 92  * The regulator GPIO handles are specified such that the
 93  * presence or absence of "enable-active-high" solely controls
 94  * the polarity of the GPIO line. Any phandle flags must
 95  * be actively ignored.
 96  */

>> +- enable-active-high   : Polarity of "enable-gpio" GPIO is active HIGH.
>> +                         Default is active LOW.
>> +- gpios                        : Array of one or more GPIO pins used to select the
>> +                         regulator voltage/current listed in "states".
>> +- gpios-states         : Initial state of GPIO pins in "gpios" array.
>> +                         0: LOW, 1: HIGH.
>> +                         Default is LOW if nothing else is specified.
>>  - startup-delay-us     : Startup time in microseconds.
>> -- enable-active-high   : Polarity of GPIO is active high (default is low).
>>  - regulator-type       : Specifies what is being regulated, must be either
>>                           "voltage" or "current", defaults to voltage.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 


-- 
Best regards,
Marek Vasut