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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/16/19 9:20 PM, Harald Geyer wrote:
> marek.vasut@xxxxxxxxx writes:
>> 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.
>> ---
>>  .../bindings/regulator/gpio-regulator.txt     | 23 +++++++++++++------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
>> index 1f496159e2bb..acca13c1eaf3 100644
>> --- 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.
>> +			  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.
>> +- 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
> 
> I think this must be a required property.

I'd say I agree, unless there's some obscure ancient edge case, which I
didn't find.

>> +			  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.
> 
> This is not very clear. Maybe add an example below or explain it better.
> 
> I guess we can't change it now anymore anyway, but it's not clear to
> me, why we have this in the first place: A regulator should only be
> active when it has a consumer or is "always-on".

Be careful here, the regulator-gpio may be instantiated such that it
will select between two voltage states, neither of which is 0V/off.
Consider eMMC vccq supply, which alternates between 1V8 and 3V3, but
does not have a third, 0V/off state (unless you pull the plug).

> IIRC the regulator
> core automatically selects the lowest voltage/current compatible with
> all consumers. It seems the only usecase is to specify an initial
> state that is different from all states in the "states" property, before
> the regulator is turned on for the first time. However it also is not
> an off-state, because it won't be set again on turning the regulator off.

Correct, this is not an off state. If you have a better wording, I am
open to that.

>>  - startup-delay-us	: Startup time in microseconds.
> 
> Not relevant for your patch, but I wonder why this is not in the
> generic regulator binding.

Presumably because it's implemented only be the gpio regulator driver,
however it could be moved into the core (?)

>> -- 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.
> 
> HTH,
> Harald
> 


-- 
Best regards,
Marek Vasut



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux