[PATCH] regulator: gpio: Reword the binding document

[PATCH] regulator: gpio: Reword the binding document

[marek.vasut]

From: Marek Vasut <marek.vasut+renesas@gmail.com>

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@gmail.com>
Cc: Harald Geyer <harald@ccbib.org>
Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: linux-renesas-soc@vger.kernel.org
To: devicetree@vger.kernel.org
---
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
+			  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.
 
-- 
2.19.2

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

[Harald Geyer]

marek.vasut@gmail.com writes:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 
> 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@gmail.com>
> Cc: Harald Geyer <harald@ccbib.org>
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: linux-renesas-soc@vger.kernel.org
> To: devicetree@vger.kernel.org
> ---
> 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.

> +			  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". 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.

>  - startup-delay-us	: Startup time in microseconds.

Not relevant for your patch, but I wonder why this is not in the
generic regulator binding.

> -- 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

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

[Marek Vasut]

On 2/16/19 9:20 PM, Harald Geyer wrote:
> marek.vasut@gmail.com writes:
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>
>> 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@gmail.com>
>> Cc: Harald Geyer <harald@ccbib.org>
>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: devicetree@vger.kernel.org
>> ---
>> 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

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

[Harald Geyer]

Marek Vasut writes:
> On 2/16/19 9:20 PM, Harald Geyer wrote:
> > marek.vasut@gmail.com writes:
> >> +			  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).

Yes, if it is always on (or simply has no enable-gpio) then we might
select a voltage while probing the device. My point is: At some point
(probably when the first consumer probes, but I think there are no
guarantees) the core will switch the regulator state. That usually
happens a few milliseconds to seconds after probing the regulator
itself. This seems to be nothing but a source of undefined behaviour.

If continuous operation of the regulator is important, I'd expect
we don't touch whatever the firmware setup instead of by default
setting all gpios to low.

However as we can't change this now, there isn't much point in
discussin this further.

> > 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.

I think it should be clearer that this is an array. (Looking at various
users I doubt everybody was aware of that, but since we have only instances
with one gpio, there is no functional difference between array and bit field.)
Also it should be recommended to set this to match whatever the bootloader
sets up. Maybe something like:

- gpios-states: Array of GPIOs values set during probing the regulator.
		0: LOW, 1: HIGH. If continous operation during boot is
		desired, this needs to match what the firmware or bootloader
		sets up. By default all GPIOs are set to low during probing.

HTH,
Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via CLAM to xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN
or via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w

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

[Marek Vasut]

On 2/17/19 3:26 PM, Harald Geyer wrote:
> Marek Vasut writes:
>> On 2/16/19 9:20 PM, Harald Geyer wrote:
>>> marek.vasut@gmail.com writes:
>>>> +			  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).
> 
> Yes, if it is always on (or simply has no enable-gpio) then we might
> select a voltage while probing the device. My point is: At some point
> (probably when the first consumer probes, but I think there are no
> guarantees) the core will switch the regulator state. That usually
> happens a few milliseconds to seconds after probing the regulator
> itself. This seems to be nothing but a source of undefined behaviour.

Why would the core switch the regulator state when the first consumer
probes ? It is up to the consumer to reconfigure the regulator when/as
it sees fit . The core should only set the default state and that's it.

> If continuous operation of the regulator is important, I'd expect
> we don't touch whatever the firmware setup instead of by default
> setting all gpios to low.
> 
> However as we can't change this now, there isn't much point in
> discussin this further.

The firmware could've left the regulator in non-default state.

>>> 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.
> 
> I think it should be clearer that this is an array. (Looking at various
> users I doubt everybody was aware of that, but since we have only instances
> with one gpio, there is no functional difference between array and bit field.)
> Also it should be recommended to set this to match whatever the bootloader
> sets up. Maybe something like:
> 
> - gpios-states: Array of GPIOs values set during probing the regulator.
> 		0: LOW, 1: HIGH. If continous operation during boot is
> 		desired, this needs to match what the firmware or bootloader
> 		sets up. By default all GPIOs are set to low during probing.

gpio-states has nothing to do with continuous operation, that's what
enable-gpios is for, we should not confuse the readers with it. Also
note that "probing" is OS specific, so we should use "default" instead.
What about this:

gpios-states		: Array of initial states of GPIO pins in "gpios" array.
0: LOW, 1: HIGH. Default is LOW if nothing else is specified.


-- 
Best regards,
Marek Vasut

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

[Harald Geyer]

Marek Vasut writes:
> On 2/17/19 3:26 PM, Harald Geyer wrote:
> > Marek Vasut writes:
> >> On 2/16/19 9:20 PM, Harald Geyer wrote:
> >>> marek.vasut@gmail.com writes:
> >>>> +			  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).
> > 
> > Yes, if it is always on (or simply has no enable-gpio) then we might
> > select a voltage while probing the device. My point is: At some point
> > (probably when the first consumer probes, but I think there are no
> > guarantees) the core will switch the regulator state. That usually
> > happens a few milliseconds to seconds after probing the regulator
> > itself. This seems to be nothing but a source of undefined behaviour.
> 
> Why would the core switch the regulator state when the first consumer
> probes ? It is up to the consumer to reconfigure the regulator when/as
> it sees fit.

AFAIK the core is free to switch the regulator to any state that is
compatible with all consumers. Ie there are no guarantees before the
first consumer requests the regulator. I only mentioned probing the
first consumer, because it is a time where the core is likely to
revisit it's decisions. The root problem is, that this property (as it
is documented now, and also as it is implemented in linux now) doesn't
acutally define behaviour beyond "gpios will be set to this state once
(but at an unkown time and for an unkown duration)".

> The core should only set the default state and that's it.

That's hits exactly the problem I see:
* gpios-states is not a _default_ state (but an initial state)
* the core doesn't set this state (the driver writes it during probe - maybe
  at a time where probing might still fail)
* the core doesn't even know about this state

> > If continuous operation of the regulator is important, I'd expect
> > we don't touch whatever the firmware setup instead of by default
> > setting all gpios to low.
> > 
> > However as we can't change this now, there isn't much point in
> > discussin this further.
> 
> The firmware could've left the regulator in non-default state.

If this is the case, why would we need to set an initial state instead
of just waiting what the first consumer requests. I still don't see
a use case (aside from don't accidentally shut down something important
during probing).

> >>> 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.
> > 
> > I think it should be clearer that this is an array. (Looking at various
> > users I doubt everybody was aware of that, but since we have only instances
> > with one gpio, there is no functional difference between array and bit field.)
> > Also it should be recommended to set this to match whatever the bootloader
> > sets up. Maybe something like:
> > 
> > - gpios-states: Array of GPIOs values set during probing the regulator.
> > 		0: LOW, 1: HIGH. If continous operation during boot is
> > 		desired, this needs to match what the firmware or bootloader
> > 		sets up. By default all GPIOs are set to low during probing.
> 
> gpio-states has nothing to do with continuous operation, that's what
> enable-gpios is for, we should not confuse the readers with it.

I think we have some misunderstanding here, because I don't see how
enable-gpios is relevant. Also maybe I still haven't understood the
usecase, but IMO that indicates, that it should be cleared up in
the document.

> Also
> note that "probing" is OS specific, so we should use "default" instead.
> What about this:

> gpios-states		: Array of initial states of GPIO pins in "gpios" array.
> 0: LOW, 1: HIGH. Default is LOW if nothing else is specified.

"initial" is a bit too unspecific IMO.

I believe this description doesn't define behaviour sufficiently and a
future rewrite (or reimplementation for a different OS) is likely to
interpret it in an incompatible way. I think one could even argue that
completely ignoring this property and just setting up a valid state from
"states" is allowed behaviour. This would likely break existing DTs, where
among other things this regulator is used to set the CPU's voltage.

HTH,
Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via CLAM to xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN
or via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w

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

[Marek Vasut]

On 2/17/19 9:00 PM, Harald Geyer wrote:
> Marek Vasut writes:
>> On 2/17/19 3:26 PM, Harald Geyer wrote:
>>> Marek Vasut writes:
>>>> On 2/16/19 9:20 PM, Harald Geyer wrote:
>>>>> marek.vasut@gmail.com writes:
>>>>>> +			  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).
>>>
>>> Yes, if it is always on (or simply has no enable-gpio) then we might
>>> select a voltage while probing the device. My point is: At some point
>>> (probably when the first consumer probes, but I think there are no
>>> guarantees) the core will switch the regulator state. That usually
>>> happens a few milliseconds to seconds after probing the regulator
>>> itself. This seems to be nothing but a source of undefined behaviour.
>>
>> Why would the core switch the regulator state when the first consumer
>> probes ? It is up to the consumer to reconfigure the regulator when/as
>> it sees fit.
> 
> AFAIK the core is free to switch the regulator to any state that is
> compatible with all consumers. Ie there are no guarantees before the
> first consumer requests the regulator.

That's correct.

> I only mentioned probing the
> first consumer, because it is a time where the core is likely to
> revisit it's decisions. The root problem is, that this property (as it
> is documented now, and also as it is implemented in linux now) doesn't
> acutally define behaviour beyond "gpios will be set to this state once
> (but at an unkown time and for an unkown duration)".

ACK

>> The core should only set the default state and that's it.
> 
> That's hits exactly the problem I see:
> * gpios-states is not a _default_ state (but an initial state)

Right, OK, this we should clarify.

> * the core doesn't set this state (the driver writes it during probe - maybe
>   at a time where probing might still fail)

Presumably the regulator driver, yes ?

> * the core doesn't even know about this state
> 
>>> If continuous operation of the regulator is important, I'd expect
>>> we don't touch whatever the firmware setup instead of by default
>>> setting all gpios to low.
>>>
>>> However as we can't change this now, there isn't much point in
>>> discussin this further.
>>
>> The firmware could've left the regulator in non-default state.
> 
> If this is the case, why would we need to set an initial state instead
> of just waiting what the first consumer requests. I still don't see
> a use case (aside from don't accidentally shut down something important
> during probing).

Possibly to prevent a state which might be harmful until the first
consumer comes up. However this is rather theoretical.

>>>>> 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.
>>>
>>> I think it should be clearer that this is an array. (Looking at various
>>> users I doubt everybody was aware of that, but since we have only instances
>>> with one gpio, there is no functional difference between array and bit field.)
>>> Also it should be recommended to set this to match whatever the bootloader
>>> sets up. Maybe something like:
>>>
>>> - gpios-states: Array of GPIOs values set during probing the regulator.
>>> 		0: LOW, 1: HIGH. If continous operation during boot is
>>> 		desired, this needs to match what the firmware or bootloader
>>> 		sets up. By default all GPIOs are set to low during probing.
>>
>> gpio-states has nothing to do with continuous operation, that's what
>> enable-gpios is for, we should not confuse the readers with it.
> 
> I think we have some misunderstanding here, because I don't see how
> enable-gpios is relevant. Also maybe I still haven't understood the
> usecase, but IMO that indicates, that it should be cleared up in
> the document.

The enable-gpios property is what turns the regulator ON/OFF , the
gpio-states is what selects the output voltage/current . Thus, I think
we should not mention "continuous operation" in the description.

>> Also
>> note that "probing" is OS specific, so we should use "default" instead.
>> What about this:
> 
>> gpios-states		: Array of initial states of GPIO pins in "gpios" array.
>> 0: LOW, 1: HIGH. Default is LOW if nothing else is specified.
> 
> "initial" is a bit too unspecific IMO.

Well, got any better idea ? It's not "default" either.

> I believe this description doesn't define behaviour sufficiently and a
> future rewrite (or reimplementation for a different OS) is likely to
> interpret it in an incompatible way. I think one could even argue that
> completely ignoring this property and just setting up a valid state from
> "states" is allowed behaviour. This would likely break existing DTs, where
> among other things this regulator is used to set the CPU's voltage.

I suspect ignoring this property, when it's present, wouldn't be a good
idea. After all, it adds additional information about the behavior of
the system upon start up.

btw I am rewording it exactly because there was a recent breakage in the
regulator code.

-- 
Best regards,
Marek Vasut

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

[Harald Geyer]

Marek Vasut writes:
> On 2/17/19 9:00 PM, Harald Geyer wrote:
> > Marek Vasut writes:
> >> On 2/17/19 3:26 PM, Harald Geyer wrote:
> >>> Marek Vasut writes:
> >>>> On 2/16/19 9:20 PM, Harald Geyer wrote:
> >>>>> marek.vasut@gmail.com writes:
> >>>>>> +			  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).
> >>>
> >>> Yes, if it is always on (or simply has no enable-gpio) then we might
> >>> select a voltage while probing the device. My point is: At some point
> >>> (probably when the first consumer probes, but I think there are no
> >>> guarantees) the core will switch the regulator state. That usually
> >>> happens a few milliseconds to seconds after probing the regulator
> >>> itself. This seems to be nothing but a source of undefined behaviour.
> >>
> >> Why would the core switch the regulator state when the first consumer
> >> probes ? It is up to the consumer to reconfigure the regulator when/as
> >> it sees fit.
> > 
> > AFAIK the core is free to switch the regulator to any state that is
> > compatible with all consumers. Ie there are no guarantees before the
> > first consumer requests the regulator.
> 
> That's correct.
> 
> > I only mentioned probing the
> > first consumer, because it is a time where the core is likely to
> > revisit it's decisions. The root problem is, that this property (as it
> > is documented now, and also as it is implemented in linux now) doesn't
> > actually define behaviour beyond "gpios will be set to this state once
> > (but at an unkown time and for an unkown duration)".
> 
> ACK
> 
> >> The core should only set the default state and that's it.
> > 
> > That's hits exactly the problem I see:
> > * gpios-states is not a _default_ state (but an initial state)
> 
> Right, OK, this we should clarify.
> 
> > * the core doesn't set this state (the driver writes it during probe - maybe
> >   at a time where probing might still fail)
> 
> Presumably the regulator driver, yes ?

Yes, the  regulator-gpio driver.

> > * the core doesn't even know about this state
> > 
> >>> If continuous operation of the regulator is important, I'd expect
> >>> we don't touch whatever the firmware setup instead of by default
> >>> setting all gpios to low.
> >>>
> >>> However as we can't change this now, there isn't much point in
> >>> discussin this further.
> >>
> >> The firmware could've left the regulator in non-default state.
> > 
> > If this is the case, why would we need to set an initial state instead
> > of just waiting what the first consumer requests. I still don't see
> > a use case (aside from don't accidentally shut down something important
> > during probing).
> 
> Possibly to prevent a state which might be harmful until the first
> consumer comes up. However this is rather theoretical.
> 
> >>>>> 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.
> >>>
> >>> I think it should be clearer that this is an array. (Looking at various
> >>> users I doubt everybody was aware of that, but since we have only instances
> >>> with one gpio, there is no functional difference between array and bit field.)
> >>> Also it should be recommended to set this to match whatever the bootloader
> >>> sets up. Maybe something like:
> >>>
> >>> - gpios-states: Array of GPIOs values set during probing the regulator.
> >>> 		0: LOW, 1: HIGH. If continuous operation during boot is
> >>> 		desired, this needs to match what the firmware or bootloader
> >>> 		sets up. By default all GPIOs are set to low during probing.
> >>
> >> gpios-states has nothing to do with continuous operation, that's what
> >> enable-gpios is for, we should not confuse the readers with it.
> > 
> > I think we have some misunderstanding here, because I don't see how
> > enable-gpios is relevant. Also maybe I still haven't understood the
> > usecase, but IMO that indicates, that it should be cleared up in
> > the document.
> 
> The enable-gpios property is what turns the regulator ON/OFF , the
> gpio-states is what selects the output voltage/current . Thus, I think
> we should not mention "continuous operation" in the description.

Okay, I see where you are coming from. Yes, gpios-states is only part
of what is required for continuous operation during boot. Still it seems
to be the only realistic usecase, we have come up so far. (Also in
existing DTs it seems to be used mostly with "boot-on" regulators.)
Maybe we can word it better then I did above, but I think mentioning
this helps a lot to make it clearer how an implementation must behave
if this property is present.

> >> Also
> >> note that "probing" is OS specific, so we should use "default" instead.
> >> What about this:
> > 
> >> gpios-states		: Array of initial states of GPIO pins in "gpios" array.
> >> 0: LOW, 1: HIGH. Default is LOW if nothing else is specified.
> > 
> > "initial" is a bit too unspecific IMO.
> 
> Well, got any better idea ? It's not "default" either.

Yes, it's difficult - which is why I cheated above and described what the
linux implementation does instead of describing it in general terms.
If the semantics of this property are too unclear to unambiguously
describe in an implementation indepenent way, then that's a point in
favor of marking this property deprecated and let whoever actually
needs it come up with something better.

> > I believe this description doesn't define behaviour sufficiently and a
> > future rewrite (or reimplementation for a different OS) is likely to
> > interpret it in an incompatible way. I think one could even argue that
> > completely ignoring this property and just setting up a valid state from
> > "states" is allowed behaviour. This would likely break existing DTs, where
> > among other things this regulator is used to set the CPU's voltage.
> 
> I suspect ignoring this property, when it's present, wouldn't be a good
> idea. After all, it adds additional information about the behavior of
> the system upon start up.

Yes, it wouldn't be good, because it likely causes breakage. No, I don't
think it adds much additional information, because it is too unclear.

> btw I am rewording it exactly because there was a recent breakage in the
> regulator code.

Yeah, thanks for that. Your patch is an improvement and shouldn't be
held back because it doesn't address the other problems discussed above.

thanks,
Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via CLAM to xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN
or via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w

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

[Marek Vasut]

On 2/18/19 11:04 AM, Harald Geyer wrote:

[...]

>>>>>>> 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.
>>>>>
>>>>> I think it should be clearer that this is an array. (Looking at various
>>>>> users I doubt everybody was aware of that, but since we have only instances
>>>>> with one gpio, there is no functional difference between array and bit field.)
>>>>> Also it should be recommended to set this to match whatever the bootloader
>>>>> sets up. Maybe something like:
>>>>>
>>>>> - gpios-states: Array of GPIOs values set during probing the regulator.
>>>>> 		0: LOW, 1: HIGH. If continuous operation during boot is
>>>>> 		desired, this needs to match what the firmware or bootloader
>>>>> 		sets up. By default all GPIOs are set to low during probing.
>>>>
>>>> gpios-states has nothing to do with continuous operation, that's what
>>>> enable-gpios is for, we should not confuse the readers with it.
>>>
>>> I think we have some misunderstanding here, because I don't see how
>>> enable-gpios is relevant. Also maybe I still haven't understood the
>>> usecase, but IMO that indicates, that it should be cleared up in
>>> the document.
>>
>> The enable-gpios property is what turns the regulator ON/OFF , the
>> gpio-states is what selects the output voltage/current . Thus, I think
>> we should not mention "continuous operation" in the description.
> 
> Okay, I see where you are coming from. Yes, gpios-states is only part
> of what is required for continuous operation during boot. Still it seems
> to be the only realistic usecase, we have come up so far. (Also in
> existing DTs it seems to be used mostly with "boot-on" regulators.)

Consider eMMC bus voltage switch, which only selects between 3V3 and 1V8
. It has nothing to do with continuous operation, it just selects one
voltage level or the other.

So what about

gpios-states : Initial state of GPIO pins in "gpios" array, set on
system start and retained until consumer changes the state. 0: LOW, 1:
HIGH. Default is LOW if nothing else is specified.

> Maybe we can word it better then I did above, but I think mentioning
> this helps a lot to make it clearer how an implementation must behave
> if this property is present.
> 
>>>> Also
>>>> note that "probing" is OS specific, so we should use "default" instead.
>>>> What about this:
>>>
>>>> gpios-states		: Array of initial states of GPIO pins in "gpios" array.
>>>> 0: LOW, 1: HIGH. Default is LOW if nothing else is specified.
>>>
>>> "initial" is a bit too unspecific IMO.
>>
>> Well, got any better idea ? It's not "default" either.
> 
> Yes, it's difficult - which is why I cheated above and described what the
> linux implementation does instead of describing it in general terms.
> If the semantics of this property are too unclear to unambiguously
> describe in an implementation indepenent way, then that's a point in
> favor of marking this property deprecated and let whoever actually
> needs it come up with something better.

We still need to support it though, due to older DTs, so we should make
it clear what it does.

>>> I believe this description doesn't define behaviour sufficiently and a
>>> future rewrite (or reimplementation for a different OS) is likely to
>>> interpret it in an incompatible way. I think one could even argue that
>>> completely ignoring this property and just setting up a valid state from
>>> "states" is allowed behaviour. This would likely break existing DTs, where
>>> among other things this regulator is used to set the CPU's voltage.
>>
>> I suspect ignoring this property, when it's present, wouldn't be a good
>> idea. After all, it adds additional information about the behavior of
>> the system upon start up.
> 
> Yes, it wouldn't be good, because it likely causes breakage. No, I don't
> think it adds much additional information, because it is too unclear.
> 
>> btw I am rewording it exactly because there was a recent breakage in the
>> regulator code.
> 
> Yeah, thanks for that. Your patch is an improvement and shouldn't be
> held back because it doesn't address the other problems discussed above.

I am hoping maybe someone who's actually native english speaker can help
out with the fine wording.

-- 
Best regards,
Marek Vasut

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

[Harald Geyer]

Marek Vasut writes:
> So what about
> 
> gpios-states : Initial state of GPIO pins in "gpios" array, set on
> system start and retained until consumer changes the state. 0: LOW, 1:
> HIGH. Default is LOW if nothing else is specified.

No. "retained until consumer changes the state" is promising more then
is guaranteed.

>From the explanations provided by Mark it is clear that this property
is an artifact of the implementation in linux. I think we should document
is as such. How about:

gpios-states : On operating systems, that don't support reading back gpio
	       values in output mode (most notably linux), this array
	       provides the state of GPIO pins set when requesting them
	       from the gpio controller. Systems, that are capable of
	       preserving state when requesting the lines, are free to
	       ignore this property. 0: LOW, 1: HIGH. Default is LOW if
	       nothing else is specified.

HTH,
Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via CLAM to xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN
or via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w

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

[Marek Vasut]

On 2/18/19 11:18 PM, Harald Geyer wrote:
> Marek Vasut writes:
>> So what about
>>
>> gpios-states : Initial state of GPIO pins in "gpios" array, set on
>> system start and retained until consumer changes the state. 0: LOW, 1:
>> HIGH. Default is LOW if nothing else is specified.
> 
> No. "retained until consumer changes the state" is promising more then
> is guaranteed.
> 
> From the explanations provided by Mark it is clear that this property
> is an artifact of the implementation in linux. I think we should document
> is as such. How about:
> 
> gpios-states : On operating systems, that don't support reading back gpio
> 	       values in output mode (most notably linux), this array
> 	       provides the state of GPIO pins set when requesting them
> 	       from the gpio controller.

That's good.

> Systems, that are capable of
> 	       preserving state when requesting the lines, are free to
> 	       ignore this property.

Are they ? I think there are systems which depend on preconfiguring the
GPIO according to this property.

> 0: LOW, 1: HIGH. Default is LOW if
> 	       nothing else is specified.
> 
> HTH,
> Harald
> 


-- 
Best regards,
Marek Vasut

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

[Harald Geyer]

Marek Vasut writes:
> On 2/18/19 11:18 PM, Harald Geyer wrote:
> > From the explanations provided by Mark it is clear that this property
> > is an artifact of the implementation in linux. I think we should document
> > is as such. How about:
> > 
> > gpios-states : On operating systems, that don't support reading back gpio
> > 	       values in output mode (most notably linux), this array
> > 	       provides the state of GPIO pins set when requesting them
> > 	       from the gpio controller.
> 
> That's good.
> 
> > Systems, that are capable of
> > 	       preserving state when requesting the lines, are free to
> > 	       ignore this property.
> 
> Are they ?

I think so. Also this seems to be what Mark wrote yesterday:

| With the GPIO API as it stands it is unfortunately not possible to
| preserve the state, if the API were fixed we'd preserve state.

> I think there are systems which depend on preconfiguring the
> GPIO according to this property.

These systems need to preconfigure the GPIOs in firmware anyway, so
they should be fine so long as the driver preserves state.

Since the original wording doesn't give any guarantees, I think the
new wording doesn't change anything. It just makes it clearer, that
there are no guarantees and that some drivers will happily overwrite
state when this property is absent.

Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via CLAM to xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN
or via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w

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

[Marek Vasut]

On 2/19/19 11:10 AM, Harald Geyer wrote:
> Marek Vasut writes:
>> On 2/18/19 11:18 PM, Harald Geyer wrote:
>>> From the explanations provided by Mark it is clear that this property
>>> is an artifact of the implementation in linux. I think we should document
>>> is as such. How about:
>>>
>>> gpios-states : On operating systems, that don't support reading back gpio
>>> 	       values in output mode (most notably linux), this array
>>> 	       provides the state of GPIO pins set when requesting them
>>> 	       from the gpio controller.
>>
>> That's good.
>>
>>> Systems, that are capable of
>>> 	       preserving state when requesting the lines, are free to
>>> 	       ignore this property.
>>
>> Are they ?
> 
> I think so. Also this seems to be what Mark wrote yesterday:
> 
> | With the GPIO API as it stands it is unfortunately not possible to
> | preserve the state, if the API were fixed we'd preserve state.
> 
>> I think there are systems which depend on preconfiguring the
>> GPIO according to this property.
> 
> These systems need to preconfigure the GPIOs in firmware anyway, so
> they should be fine so long as the driver preserves state.
> 
> Since the original wording doesn't give any guarantees, I think the
> new wording doesn't change anything. It just makes it clearer, that
> there are no guarantees and that some drivers will happily overwrite
> state when this property is absent.

OK, so how can we move forward with this ? We discussed a lot, but I
don't know what we should do about the patch.

-- 
Best regards,
Marek Vasut

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

[Harald Geyer]

Marek Vasut writes:
> On 2/19/19 11:10 AM, Harald Geyer wrote:
> > Marek Vasut writes:
> >> On 2/18/19 11:18 PM, Harald Geyer wrote:
> >>> From the explanations provided by Mark it is clear that this property
> >>> is an artifact of the implementation in linux. I think we should document
> >>> is as such. How about:
> >>>
> >>> gpios-states : On operating systems, that don't support reading back gpio
> >>> 	       values in output mode (most notably linux), this array
> >>> 	       provides the state of GPIO pins set when requesting them
> >>> 	       from the gpio controller.
> >>
> >> That's good.
> >>
> >>> Systems, that are capable of
> >>> 	       preserving state when requesting the lines, are free to
> >>> 	       ignore this property.
> >>
> >> Are they ?
> > 
> > I think so. Also this seems to be what Mark wrote yesterday:
> > 
> > | With the GPIO API as it stands it is unfortunately not possible to
> > | preserve the state, if the API were fixed we'd preserve state.
> > 
> >> I think there are systems which depend on preconfiguring the
> >> GPIO according to this property.
> > 
> > These systems need to preconfigure the GPIOs in firmware anyway, so
> > they should be fine so long as the driver preserves state.
> > 
> > Since the original wording doesn't give any guarantees, I think the
> > new wording doesn't change anything. It just makes it clearer, that
> > there are no guarantees and that some drivers will happily overwrite
> > state when this property is absent.
> 
> OK, so how can we move forward with this ? We discussed a lot, but I
> don't know what we should do about the patch.

Yes, we discussed a lot. I guess most people lost track of where we
stand. I'd suggest you send a V2 of the patch, picking up all the proposed
changes. If you feel the part about `gpios-states' property is too
controversial, then maybe split it in two patches: The first containing
the non-controversial changes and the second improving `gpios-states'
description, so that maintainers can ACK them independently.

best regards,
Harald

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

[Marek Vasut]

On 3/3/19 5:07 PM, Harald Geyer wrote:
> Marek Vasut writes:
>> On 2/19/19 11:10 AM, Harald Geyer wrote:
>>> Marek Vasut writes:
>>>> On 2/18/19 11:18 PM, Harald Geyer wrote:
>>>>> From the explanations provided by Mark it is clear that this property
>>>>> is an artifact of the implementation in linux. I think we should document
>>>>> is as such. How about:
>>>>>
>>>>> gpios-states : On operating systems, that don't support reading back gpio
>>>>> 	       values in output mode (most notably linux), this array
>>>>> 	       provides the state of GPIO pins set when requesting them
>>>>> 	       from the gpio controller.
>>>>
>>>> That's good.
>>>>
>>>>> Systems, that are capable of
>>>>> 	       preserving state when requesting the lines, are free to
>>>>> 	       ignore this property.
>>>>
>>>> Are they ?
>>>
>>> I think so. Also this seems to be what Mark wrote yesterday:
>>>
>>> | With the GPIO API as it stands it is unfortunately not possible to
>>> | preserve the state, if the API were fixed we'd preserve state.
>>>
>>>> I think there are systems which depend on preconfiguring the
>>>> GPIO according to this property.
>>>
>>> These systems need to preconfigure the GPIOs in firmware anyway, so
>>> they should be fine so long as the driver preserves state.
>>>
>>> Since the original wording doesn't give any guarantees, I think the
>>> new wording doesn't change anything. It just makes it clearer, that
>>> there are no guarantees and that some drivers will happily overwrite
>>> state when this property is absent.
>>
>> OK, so how can we move forward with this ? We discussed a lot, but I
>> don't know what we should do about the patch.
> 
> Yes, we discussed a lot. I guess most people lost track of where we
> stand. I'd suggest you send a V2 of the patch, picking up all the proposed
> changes. If you feel the part about `gpios-states' property is too
> controversial, then maybe split it in two patches: The first containing
> the non-controversial changes and the second improving `gpios-states'
> description, so that maintainers can ACK them independently.

Well what are the proposed changes ? I don't think there was any
agreement on them.

-- 
Best regards,
Marek Vasut

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Sun, Feb 17, 2019 at 10:57:26PM +0100, Marek Vasut wrote:
> On 2/17/19 9:00 PM, Harald Geyer wrote:

> > If this is the case, why would we need to set an initial state instead
> > of just waiting what the first consumer requests. I still don't see
> > a use case (aside from don't accidentally shut down something important
> > during probing).

> Possibly to prevent a state which might be harmful until the first
> consumer comes up. However this is rather theoretical.

This is the GPIO API restrictions again - there was a big push at one
point to try to ensure that we set the output state when requesting
output GPIOs, and a lot of the time requesting will set a state anyway
when it puts things into output mode so it may as wel be one we know
about.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

On Sun, Feb 17, 2019 at 03:26:39PM +0100, Harald Geyer wrote:

> If continuous operation of the regulator is important, I'd expect
> we don't touch whatever the firmware setup instead of by default
> setting all gpios to low.

> However as we can't change this now, there isn't much point in
> discussin this further.

The issue here is that the GPIO API does not allow us to read back the
state of output GPIOs (even though for almost all hardware this is
supported by the underlying hardware).  With the GPIO API as it stands
it is unfortunately not possible to preserve the state, if the API were
fixed we'd preserve state.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

On Sat, Feb 16, 2019 at 10:37:37PM +0100, Marek Vasut wrote:
> On 2/16/19 9:20 PM, Harald Geyer wrote:
> >> From: Marek Vasut <marek.vasut+renesas@gmail.com>

> >>  - 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 (?)

The core can do the ramping but for most regulators it should know any
ramp rate restrictions just from knowing which regulator is being used
(and possibly some device specific configuration like the values of some
passives), there should be no requirement to manually configure the
required delay in the DT.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Geert Uytterhoeven]

Hi Marek,

On Sun, Feb 17, 2019 at 8:00 AM <marek.vasut@gmail.com> wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> 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@gmail.com>
> Cc: Harald Geyer <harald@ccbib.org>
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: linux-renesas-soc@vger.kernel.org
> To: devicetree@vger.kernel.org
> ---
> 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) ...

> +                         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>)?
When the flags cell is present, there's indeed opportunity for confusion
(and breakage), combined with the presence/lack of *active-* below...

> +- 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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marek Vasut]

On 2/18/19 10:30 AM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Sun, Feb 17, 2019 at 8:00 AM <marek.vasut@gmail.com> wrote:
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>> 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@gmail.com>
>> Cc: Harald Geyer <harald@ccbib.org>
>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: devicetree@vger.kernel.org
>> ---
>> 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

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

[Geert Uytterhoeven]

Hi Marek,

On Mon, Feb 18, 2019 at 8:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 2/18/19 10:30 AM, Geert Uytterhoeven wrote:
> > On Sun, Feb 17, 2019 at 8:00 AM <marek.vasut@gmail.com> wrote:
> >> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> 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@gmail.com>

> >> --- 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 ?

of_find_gpio() supports both anyway, through gpio_suffixes[].

> >> +                         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  */

Yeah, the code is the binding doc...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds