[V2] regulator: gpio: Reword the binding document

Commit Message

Marek Vasut March 4, 2019, 7:40 p.m. UTC

From: Marek Vasut <[e-mail]>

Reword the binding document to make it clear how the propeties work
and which properties affect which other properties.

Signed-off-by: Marek Vasut <[e-mail]>
Cc: Harald Geyer <[e-mail]>
Cc: Kuninori Morimoto <[e-mail]>
Cc: Linus Walleij <[e-mail]>
Cc: Mark Brown <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: [e-mail]
To: [e-mail]
---
V2: - Make "gpios" a mandatory property
    - Reword "gpio-states" property description
    - Change "enable-gpio" to "enable-gpios" to match modern DT rules
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     | 25 +++++++++++++------
 1 file changed, 17 insertions(+), 8 deletions(-)

Comments

Linus Walleij March 4, 2019, 10:20 p.m. UTC | #1

On Mon, Mar 4, 2019 at 8:40 PM <[e-mail]> wrote:

> From: Marek Vasut <[e-mail]>
>
> Reword the binding document to make it clear how the propeties work
> and which properties affect which other properties.
>
> Signed-off-by: Marek Vasut <[e-mail]>
> Cc: Harald Geyer <[e-mail]>
> Cc: Kuninori Morimoto <[e-mail]>
> Cc: Linus Walleij <[e-mail]>
> Cc: Mark Brown <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: [e-mail]
> To: [e-mail]
> ---
> V2: - Make "gpios" a mandatory property
>     - Reword "gpio-states" property description
>     - Change "enable-gpio" to "enable-gpios" to match modern DT rules
> 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.

Excellent work!
Reviewed-by: Linus Walleij <[e-mail]>

Yours,
Linus Walleij

Harald Geyer March 5, 2019, 10:07 a.m. UTC | #2

[e-mail] writes:
> From: Marek Vasut <[e-mail]>
> 
> Reword the binding document to make it clear how the propeties work
> and which properties affect which other properties.
> 
> Signed-off-by: Marek Vasut <[e-mail]>
> Cc: Harald Geyer <[e-mail]>
> Cc: Kuninori Morimoto <[e-mail]>
> Cc: Linus Walleij <[e-mail]>
> Cc: Mark Brown <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: [e-mail]
> To: [e-mail]
> ---
> V2: - Make "gpios" a mandatory property
>     - Reword "gpio-states" property description
>     - Change "enable-gpio" to "enable-gpios" to match modern DT rules
> 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 for the update. I think it addresses all my concerns except for
one:

> +- gpios-states	: State of GPIO pins in "gpios" array that is set until
> +			  changed by the first consumer. 0: LOW, 1: HIGH.
> +			  Default is LOW if nothing else is specified.

I still believe this not true: There is no guarantee that the regulator
core won't change the state of GPIO pins before the first consumer comes
up.

I still think my proposal describes the property more acurately:
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.

Since we had this discussion already in the V1 thread and you clearly don't
agree with me, the maintainers will need to decide. You can add 
Reviewed-by: Harald Geyer <[e-mail]>
once Rob and/or Mark have addressed this issue.

Thanks for your work!
Harald

Marek Vasut March 5, 2019, 10:59 a.m. UTC | #3

On 3/5/19 11:07 AM, Harald Geyer wrote:
> [e-mail] writes:
>> From: Marek Vasut <[e-mail]>
>>
>> Reword the binding document to make it clear how the propeties work
>> and which properties affect which other properties.
>>
>> Signed-off-by: Marek Vasut <[e-mail]>
>> Cc: Harald Geyer <[e-mail]>
>> Cc: Kuninori Morimoto <[e-mail]>
>> Cc: Linus Walleij <[e-mail]>
>> Cc: Mark Brown <[e-mail]>
>> Cc: Rob Herring <[e-mail]>
>> Cc: [e-mail]
>> To: [e-mail]
>> ---
>> V2: - Make "gpios" a mandatory property
>>     - Reword "gpio-states" property description
>>     - Change "enable-gpio" to "enable-gpios" to match modern DT rules
>> 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 for the update. I think it addresses all my concerns except for
> one:
> 
>> +- gpios-states	: State of GPIO pins in "gpios" array that is set until
>> +			  changed by the first consumer. 0: LOW, 1: HIGH.
>> +			  Default is LOW if nothing else is specified.
> 
> I still believe this not true: There is no guarantee that the regulator
> core won't change the state of GPIO pins before the first consumer comes
> up.

Why would it do that ? That would completely invalidate any remaining
useful-ness of this property.

> I still think my proposal describes the property more acurately:
> 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.
> 
> Since we had this discussion already in the V1 thread and you clearly don't
> agree with me, the maintainers will need to decide. You can add 
> Reviewed-by: Harald Geyer <[e-mail]>
> once Rob and/or Mark have addressed this issue.

I think we're just looking at this from two different perspectives and
for whatever reason can't reconcile them.

> Thanks for your work!
> Harald
>

Harald Geyer March 5, 2019, 4:10 p.m. UTC | #4

Marek Vasut writes:
> On 3/5/19 11:07 AM, Harald Geyer wrote:
> > [e-mail] writes:
> >> From: Marek Vasut <[e-mail]>
> >>
> >> Reword the binding document to make it clear how the propeties work
> >> and which properties affect which other properties.
> >>
> >> Signed-off-by: Marek Vasut <[e-mail]>
> >> Cc: Harald Geyer <[e-mail]>
> >> Cc: Kuninori Morimoto <[e-mail]>
> >> Cc: Linus Walleij <[e-mail]>
> >> Cc: Mark Brown <[e-mail]>
> >> Cc: Rob Herring <[e-mail]>
> >> Cc: [e-mail]
> >> To: [e-mail]
> >> ---
> >> V2: - Make "gpios" a mandatory property
> >>     - Reword "gpio-states" property description
> >>     - Change "enable-gpio" to "enable-gpios" to match modern DT rules
> >> 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 for the update. I think it addresses all my concerns except for
> > one:
> > 
> >> +- gpios-states	: State of GPIO pins in "gpios" array that is set until
> >> +			  changed by the first consumer. 0: LOW, 1: HIGH.
> >> +			  Default is LOW if nothing else is specified.
> > 
> > I still believe this not true: There is no guarantee that the regulator
> > core won't change the state of GPIO pins before the first consumer comes
> > up.
> 
> Why would it do that ?

Because the regulator core doesn't know about this driver specific
property at all. And without any constraints placed by consumers, the
core is free to choose any state whatsoever at any point in time.

> That would completely invalidate any remaining
> useful-ness of this property.

Yes, I believe this property is mostly useless. That's what I want to
get across with my wording proposal. The remaining usecase, that I can see,
is when the GPIOs have been setup by the bootloader and we don't want
to reset them to low during probing (which some OSes might be capable
of, but linux currently doesn't). Also a state of all GPIOs low might
be invalid (not in the "states" property), so we shouldn't set all GPIOs
to low during probing in that case.

HTH,
Harald

> > I still think my proposal describes the property more acurately:
> > 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.
> > 
> > Since we had this discussion already in the V1 thread and you clearly don't
> > agree with me, the maintainers will need to decide. You can add 
> > Reviewed-by: Harald Geyer <[e-mail]>
> > once Rob and/or Mark have addressed this issue.
> 
> I think we're just looking at this from two different perspectives and
> for whatever reason can't reconcile them.
> 
> > Thanks for your work!
> > Harald
> > 
> 
> 
> -- 
> Best regards,
> Marek Vasut

Marek Vasut March 5, 2019, 7:01 p.m. UTC | #5

On 3/5/19 5:10 PM, Harald Geyer wrote:
> Marek Vasut writes:
>> On 3/5/19 11:07 AM, Harald Geyer wrote:
>>> [e-mail] writes:
>>>> From: Marek Vasut <[e-mail]>
>>>>
>>>> Reword the binding document to make it clear how the propeties work
>>>> and which properties affect which other properties.
>>>>
>>>> Signed-off-by: Marek Vasut <[e-mail]>
>>>> Cc: Harald Geyer <[e-mail]>
>>>> Cc: Kuninori Morimoto <[e-mail]>
>>>> Cc: Linus Walleij <[e-mail]>
>>>> Cc: Mark Brown <[e-mail]>
>>>> Cc: Rob Herring <[e-mail]>
>>>> Cc: [e-mail]
>>>> To: [e-mail]
>>>> ---
>>>> V2: - Make "gpios" a mandatory property
>>>>     - Reword "gpio-states" property description
>>>>     - Change "enable-gpio" to "enable-gpios" to match modern DT rules
>>>> 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 for the update. I think it addresses all my concerns except for
>>> one:
>>>
>>>> +- gpios-states	: State of GPIO pins in "gpios" array that is set until
>>>> +			  changed by the first consumer. 0: LOW, 1: HIGH.
>>>> +			  Default is LOW if nothing else is specified.
>>>
>>> I still believe this not true: There is no guarantee that the regulator
>>> core won't change the state of GPIO pins before the first consumer comes
>>> up.
>>
>> Why would it do that ?
> 
> Because the regulator core doesn't know about this driver specific
> property at all. And without any constraints placed by consumers, the
> core is free to choose any state whatsoever at any point in time.

But git grep seems to disagree, see drivers/regulator/gpio-regulator.c:
                    ret = of_property_read_u32_index(np, "gpios-states", i,

The core sets the pins to such a value until the consumer takes over.

>> That would completely invalidate any remaining
>> useful-ness of this property.
> 
> Yes, I believe this property is mostly useless. That's what I want to
> get across with my wording proposal. The remaining usecase, that I can see,
> is when the GPIOs have been setup by the bootloader and we don't want
> to reset them to low during probing (which some OSes might be capable
> of, but linux currently doesn't). Also a state of all GPIOs low might
> be invalid (not in the "states" property), so we shouldn't set all GPIOs
> to low during probing in that case.

I presume the bootloader might even add this property to DT based on the
state in which it leaves the GPIOs in.

> HTH,
> Harald
> 
>>> I still think my proposal describes the property more acurately:
>>> 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.
>>>
>>> Since we had this discussion already in the V1 thread and you clearly don't
>>> agree with me, the maintainers will need to decide. You can add 
>>> Reviewed-by: Harald Geyer <[e-mail]>
>>> once Rob and/or Mark have addressed this issue.
>>
>> I think we're just looking at this from two different perspectives and
>> for whatever reason can't reconcile them.
>>
>>> Thanks for your work!
>>> Harald
>>>
>>
>>
>> -- 
>> Best regards,
>> Marek Vasut
>

Harald Geyer March 5, 2019, 9:36 p.m. UTC | #6

Marek Vasut writes:
> On 3/5/19 5:10 PM, Harald Geyer wrote:
> > Marek Vasut writes:
> >> On 3/5/19 11:07 AM, Harald Geyer wrote:
> >>> [e-mail] writes:
> >>>> From: Marek Vasut <[e-mail]>
> >>>>
> >>>> Reword the binding document to make it clear how the propeties work
> >>>> and which properties affect which other properties.
> >>>>
> >>>> Signed-off-by: Marek Vasut <[e-mail]>
> >>>> Cc: Harald Geyer <[e-mail]>
> >>>> Cc: Kuninori Morimoto <[e-mail]>
> >>>> Cc: Linus Walleij <[e-mail]>
> >>>> Cc: Mark Brown <[e-mail]>
> >>>> Cc: Rob Herring <[e-mail]>
> >>>> Cc: [e-mail]
> >>>> To: [e-mail]
> >>>> ---
> >>>> V2: - Make "gpios" a mandatory property
> >>>>     - Reword "gpio-states" property description
> >>>>     - Change "enable-gpio" to "enable-gpios" to match modern DT rules
> >>>> 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 for the update. I think it addresses all my concerns except for
> >>> one:
> >>>
> >>>> +- gpios-states	: State of GPIO pins in "gpios" array that is set until
> >>>> +			  changed by the first consumer. 0: LOW, 1: HIGH.
> >>>> +			  Default is LOW if nothing else is specified.
> >>>
> >>> I still believe this not true: There is no guarantee that the regulator
> >>> core won't change the state of GPIO pins before the first consumer comes
> >>> up.
> >>
> >> Why would it do that ?
> > 
> > Because the regulator core doesn't know about this driver specific
> > property at all. And without any constraints placed by consumers, the
> > core is free to choose any state whatsoever at any point in time.
> 
> But git grep seems to disagree, see drivers/regulator/gpio-regulator.c:
>                     ret = of_property_read_u32_index(np, "gpios-states", i,
> 
> The core sets the pins to such a value until the consumer takes over.

I think we have a misunderstanding of terminology. When I write "regulator
core", I mean the driver independent regulator code. The line you quote
above is part of the gpio-regulator driver and thus not part of what
I call the "regulator core".

AFAICS the data from the property is only stored in a driver specific
data structure (and not used at all outside of probe) but never passed
to what I call the regulator core.

Why do you believe there is a guarantee that the value set during
probeing is preserved until a consumer takes over?

Harald

Marek Vasut March 5, 2019, 10:23 p.m. UTC | #7

On 3/5/19 10:36 PM, Harald Geyer wrote:
> Marek Vasut writes:
>> On 3/5/19 5:10 PM, Harald Geyer wrote:
>>> Marek Vasut writes:
>>>> On 3/5/19 11:07 AM, Harald Geyer wrote:
>>>>> [e-mail] writes:
>>>>>> From: Marek Vasut <[e-mail]>
>>>>>>
>>>>>> Reword the binding document to make it clear how the propeties work
>>>>>> and which properties affect which other properties.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <[e-mail]>
>>>>>> Cc: Harald Geyer <[e-mail]>
>>>>>> Cc: Kuninori Morimoto <[e-mail]>
>>>>>> Cc: Linus Walleij <[e-mail]>
>>>>>> Cc: Mark Brown <[e-mail]>
>>>>>> Cc: Rob Herring <[e-mail]>
>>>>>> Cc: [e-mail]
>>>>>> To: [e-mail]
>>>>>> ---
>>>>>> V2: - Make "gpios" a mandatory property
>>>>>>     - Reword "gpio-states" property description
>>>>>>     - Change "enable-gpio" to "enable-gpios" to match modern DT rules
>>>>>> 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 for the update. I think it addresses all my concerns except for
>>>>> one:
>>>>>
>>>>>> +- gpios-states	: State of GPIO pins in "gpios" array that is set until
>>>>>> +			  changed by the first consumer. 0: LOW, 1: HIGH.
>>>>>> +			  Default is LOW if nothing else is specified.
>>>>>
>>>>> I still believe this not true: There is no guarantee that the regulator
>>>>> core won't change the state of GPIO pins before the first consumer comes
>>>>> up.
>>>>
>>>> Why would it do that ?
>>>
>>> Because the regulator core doesn't know about this driver specific
>>> property at all. And without any constraints placed by consumers, the
>>> core is free to choose any state whatsoever at any point in time.
>>
>> But git grep seems to disagree, see drivers/regulator/gpio-regulator.c:
>>                     ret = of_property_read_u32_index(np, "gpios-states", i,
>>
>> The core sets the pins to such a value until the consumer takes over.
> 
> I think we have a misunderstanding of terminology. When I write "regulator
> core", I mean the driver independent regulator code. The line you quote
> above is part of the gpio-regulator driver and thus not part of what
> I call the "regulator core".
> 
> AFAICS the data from the property is only stored in a driver specific
> data structure (and not used at all outside of probe) but never passed
> to what I call the regulator core.
> 
> Why do you believe there is a guarantee that the value set during
> probeing is preserved until a consumer takes over?

It is the only sensible behavior and the behavior I see people expect
from this property. I presume it solidified in this sort of semi-defined
state, so we're stuck with assuming it behaves this way to maintain
compatibility.

Harald Geyer March 6, 2019, 8:17 a.m. UTC | #8

Marek Vasut writes:
> On 3/5/19 10:36 PM, Harald Geyer wrote:
> > Marek Vasut writes:
> >> On 3/5/19 5:10 PM, Harald Geyer wrote:
> >>> Marek Vasut writes:
> >>>> On 3/5/19 11:07 AM, Harald Geyer wrote:
> >>>>> [e-mail] writes:
> >>>>>> From: Marek Vasut <[e-mail]>
> >>>>>>
> >>>>>> Reword the binding document to make it clear how the propeties work
> >>>>>> and which properties affect which other properties.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <[e-mail]>
> >>>>>> Cc: Harald Geyer <[e-mail]>
> >>>>>> Cc: Kuninori Morimoto <[e-mail]>
> >>>>>> Cc: Linus Walleij <[e-mail]>
> >>>>>> Cc: Mark Brown <[e-mail]>
> >>>>>> Cc: Rob Herring <[e-mail]>
> >>>>>> Cc: [e-mail]
> >>>>>> To: [e-mail]
> >>>>>> ---
> >>>>>> V2: - Make "gpios" a mandatory property
> >>>>>>     - Reword "gpio-states" property description
> >>>>>>     - Change "enable-gpio" to "enable-gpios" to match modern DT rules
> >>>>>> 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 for the update. I think it addresses all my concerns except for
> >>>>> one:
> >>>>>
> >>>>>> +- gpios-states	: State of GPIO pins in "gpios" array that is set until
> >>>>>> +			  changed by the first consumer. 0: LOW, 1: HIGH.
> >>>>>> +			  Default is LOW if nothing else is specified.
> >>>>>
> >>>>> I still believe this not true: There is no guarantee that the regulator
> >>>>> core won't change the state of GPIO pins before the first consumer comes
> >>>>> up.
> >>>>
> >>>> Why would it do that ?
> >>>
> >>> Because the regulator core doesn't know about this driver specific
> >>> property at all. And without any constraints placed by consumers, the
> >>> core is free to choose any state whatsoever at any point in time.
> >>
> >> But git grep seems to disagree, see drivers/regulator/gpio-regulator.c:
> >>                     ret = of_property_read_u32_index(np, "gpios-states", i,
> >>
> >> The core sets the pins to such a value until the consumer takes over.
> > 
> > I think we have a misunderstanding of terminology. When I write "regulator
> > core", I mean the driver independent regulator code. The line you quote
> > above is part of the gpio-regulator driver and thus not part of what
> > I call the "regulator core".
> > 
> > AFAICS the data from the property is only stored in a driver specific
> > data structure (and not used at all outside of probe) but never passed
> > to what I call the regulator core.
> > 
> > Why do you believe there is a guarantee that the value set during
> > probeing is preserved until a consumer takes over?
> 
> It is the only sensible behavior and the behavior I see people expect
> from this property. I presume it solidified in this sort of semi-defined
> state, so we're stuck with assuming it behaves this way to maintain
> compatibility.

Maybe the behaviour you want would be more sensible, but AFAIK it just
isn't true in general (it might work that way by chance in many cases).
If people expect this behaviour, it is a misunderstanding of the old wording.
I'd prefer we don't have to add a quirk to the regulator subsystem to
cater for a misunderstanding.

I think, if you really want to go forward with making this behaviour
officially maintained, then we should first add the code to linux and
only then add the promise to the binding document. This isn't the scope
of this patch, so I guess we would need to keep the ambiguous wording as
it is for now. I believe it is more important for a binding document
to be correct than to be sensible.

However I don't think we actually need to go to such extremes: In linux
we currently have (arm/boot/dts and arm64/boot/dts) 38 uses of this
property in 29 DTs. All the examples, that I studied in some detail,
seem to either don't need this property at all or have a usecase that is
supported by my proposed wording. I don't expect any problems if we just
document the status quo clearly.

Harald

Marek Vasut March 6, 2019, 9:56 p.m. UTC | #9

On 3/6/19 9:17 AM, Harald Geyer wrote:
> Marek Vasut writes:
>> On 3/5/19 10:36 PM, Harald Geyer wrote:
>>> Marek Vasut writes:
>>>> On 3/5/19 5:10 PM, Harald Geyer wrote:
>>>>> Marek Vasut writes:
>>>>>> On 3/5/19 11:07 AM, Harald Geyer wrote:
>>>>>>> [e-mail] writes:
>>>>>>>> From: Marek Vasut <[e-mail]>
>>>>>>>>
>>>>>>>> Reword the binding document to make it clear how the propeties work
>>>>>>>> and which properties affect which other properties.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <[e-mail]>
>>>>>>>> Cc: Harald Geyer <[e-mail]>
>>>>>>>> Cc: Kuninori Morimoto <[e-mail]>
>>>>>>>> Cc: Linus Walleij <[e-mail]>
>>>>>>>> Cc: Mark Brown <[e-mail]>
>>>>>>>> Cc: Rob Herring <[e-mail]>
>>>>>>>> Cc: [e-mail]
>>>>>>>> To: [e-mail]
>>>>>>>> ---
>>>>>>>> V2: - Make "gpios" a mandatory property
>>>>>>>>     - Reword "gpio-states" property description
>>>>>>>>     - Change "enable-gpio" to "enable-gpios" to match modern DT rules
>>>>>>>> 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 for the update. I think it addresses all my concerns except for
>>>>>>> one:
>>>>>>>
>>>>>>>> +- gpios-states	: State of GPIO pins in "gpios" array that is set until
>>>>>>>> +			  changed by the first consumer. 0: LOW, 1: HIGH.
>>>>>>>> +			  Default is LOW if nothing else is specified.
>>>>>>>
>>>>>>> I still believe this not true: There is no guarantee that the regulator
>>>>>>> core won't change the state of GPIO pins before the first consumer comes
>>>>>>> up.
>>>>>>
>>>>>> Why would it do that ?
>>>>>
>>>>> Because the regulator core doesn't know about this driver specific
>>>>> property at all. And without any constraints placed by consumers, the
>>>>> core is free to choose any state whatsoever at any point in time.
>>>>
>>>> But git grep seems to disagree, see drivers/regulator/gpio-regulator.c:
>>>>                     ret = of_property_read_u32_index(np, "gpios-states", i,
>>>>
>>>> The core sets the pins to such a value until the consumer takes over.
>>>
>>> I think we have a misunderstanding of terminology. When I write "regulator
>>> core", I mean the driver independent regulator code. The line you quote
>>> above is part of the gpio-regulator driver and thus not part of what
>>> I call the "regulator core".
>>>
>>> AFAICS the data from the property is only stored in a driver specific
>>> data structure (and not used at all outside of probe) but never passed
>>> to what I call the regulator core.
>>>
>>> Why do you believe there is a guarantee that the value set during
>>> probeing is preserved until a consumer takes over?
>>
>> It is the only sensible behavior and the behavior I see people expect
>> from this property. I presume it solidified in this sort of semi-defined
>> state, so we're stuck with assuming it behaves this way to maintain
>> compatibility.
> 
> Maybe the behaviour you want would be more sensible, but AFAIK it just
> isn't true in general (it might work that way by chance in many cases).
> If people expect this behaviour, it is a misunderstanding of the old wording.
> I'd prefer we don't have to add a quirk to the regulator subsystem to
> cater for a misunderstanding.
> 
> I think, if you really want to go forward with making this behaviour
> officially maintained, then we should first add the code to linux and
> only then add the promise to the binding document. This isn't the scope
> of this patch, so I guess we would need to keep the ambiguous wording as
> it is for now. I believe it is more important for a binding document
> to be correct than to be sensible.
> 
> However I don't think we actually need to go to such extremes: In linux
> we currently have (arm/boot/dts and arm64/boot/dts) 38 uses of this
> property in 29 DTs. All the examples, that I studied in some detail,
> seem to either don't need this property at all or have a usecase that is
> supported by my proposed wording. I don't expect any problems if we just
> document the status quo clearly.

In that case, provide a suggestion how to document this property better?

Harald Geyer March 7, 2019, 9:12 a.m. UTC | #10

On 06.03.2019 22:56, Marek Vasut wrote:
> On 3/6/19 9:17 AM, Harald Geyer wrote:
>> Marek Vasut writes:
>>> On 3/5/19 10:36 PM, Harald Geyer wrote:
>>>> Marek Vasut writes:
>>>>> On 3/5/19 5:10 PM, Harald Geyer wrote:
>>>>>> Marek Vasut writes:
>>>>>>> On 3/5/19 11:07 AM, Harald Geyer wrote:
>>>>>>>> [e-mail] writes:
>>>>>>>>> From: Marek Vasut <[e-mail]>
>>>>>>>>>
>>>>>>>>> Reword the binding document to make it clear how the 
>>>>>>>>> propeties work
>>>>>>>>> and which properties affect which other properties.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marek Vasut <[e-mail]>
>>>>>>>>> Cc: Harald Geyer <[e-mail]>
>>>>>>>>> Cc: Kuninori Morimoto <[e-mail]>
>>>>>>>>> Cc: Linus Walleij <[e-mail]>
>>>>>>>>> Cc: Mark Brown <[e-mail]>
>>>>>>>>> Cc: Rob Herring <[e-mail]>
>>>>>>>>> Cc: [e-mail]
>>>>>>>>> To: [e-mail]
>>>>>>>>> ---
>>>>>>>>> V2: - Make "gpios" a mandatory property
>>>>>>>>>     - Reword "gpio-states" property description
>>>>>>>>>     - Change "enable-gpio" to "enable-gpios" to match modern 
>>>>>>>>> DT rules
>>>>>>>>> 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 for the update. I think it addresses all my concerns 
>>>>>>>> except for
>>>>>>>> one:
>>>>>>>>
>>>>>>>>> +- gpios-states	: State of GPIO pins in "gpios" array that is 
>>>>>>>>> set until
>>>>>>>>> +			  changed by the first consumer. 0: LOW, 1: HIGH.
>>>>>>>>> +			  Default is LOW if nothing else is specified.
>>>>>>>>
>>>>>>>> I still believe this not true: There is no guarantee that the 
>>>>>>>> regulator
>>>>>>>> core won't change the state of GPIO pins before the first 
>>>>>>>> consumer comes
>>>>>>>> up.
>>>>>>>
>>>>>>> Why would it do that ?
>>>>>>
>>>>>> Because the regulator core doesn't know about this driver 
>>>>>> specific
>>>>>> property at all. And without any constraints placed by 
>>>>>> consumers, the
>>>>>> core is free to choose any state whatsoever at any point in 
>>>>>> time.
>>>>>
>>>>> But git grep seems to disagree, see 
>>>>> drivers/regulator/gpio-regulator.c:
>>>>>                     ret = of_property_read_u32_index(np, 
>>>>> "gpios-states", i,
>>>>>
>>>>> The core sets the pins to such a value until the consumer takes 
>>>>> over.
>>>>
>>>> I think we have a misunderstanding of terminology. When I write 
>>>> "regulator
>>>> core", I mean the driver independent regulator code. The line you 
>>>> quote
>>>> above is part of the gpio-regulator driver and thus not part of 
>>>> what
>>>> I call the "regulator core".
>>>>
>>>> AFAICS the data from the property is only stored in a driver 
>>>> specific
>>>> data structure (and not used at all outside of probe) but never 
>>>> passed
>>>> to what I call the regulator core.
>>>>
>>>> Why do you believe there is a guarantee that the value set during
>>>> probeing is preserved until a consumer takes over?
>>>
>>> It is the only sensible behavior and the behavior I see people 
>>> expect
>>> from this property. I presume it solidified in this sort of 
>>> semi-defined
>>> state, so we're stuck with assuming it behaves this way to maintain
>>> compatibility.
>>
>> Maybe the behaviour you want would be more sensible, but AFAIK it 
>> just
>> isn't true in general (it might work that way by chance in many 
>> cases).
>> If people expect this behaviour, it is a misunderstanding of the old 
>> wording.
>> I'd prefer we don't have to add a quirk to the regulator subsystem 
>> to
>> cater for a misunderstanding.
>>
>> I think, if you really want to go forward with making this behaviour
>> officially maintained, then we should first add the code to linux 
>> and
>> only then add the promise to the binding document. This isn't the 
>> scope
>> of this patch, so I guess we would need to keep the ambiguous 
>> wording as
>> it is for now. I believe it is more important for a binding document
>> to be correct than to be sensible.
>>
>> However I don't think we actually need to go to such extremes: In 
>> linux
>> we currently have (arm/boot/dts and arm64/boot/dts) 38 uses of this
>> property in 29 DTs. All the examples, that I studied in some detail,
>> seem to either don't need this property at all or have a usecase 
>> that is
>> supported by my proposed wording. I don't expect any problems if we 
>> just
>> document the status quo clearly.
>
> In that case, provide a suggestion how to document this property 
> better?

I did: https://www.spinics.net/lists/devicetree/msg275050.html

HTH,
Harald

Marek Vasut March 16, 2019, 8:26 p.m. UTC | #11

On 3/7/19 10:12 AM, Harald Geyer wrote:
> On 06.03.2019 22:56, Marek Vasut wrote:
>> On 3/6/19 9:17 AM, Harald Geyer wrote:
>>> Marek Vasut writes:
>>>> On 3/5/19 10:36 PM, Harald Geyer wrote:
>>>>> Marek Vasut writes:
>>>>>> On 3/5/19 5:10 PM, Harald Geyer wrote:
>>>>>>> Marek Vasut writes:
>>>>>>>> On 3/5/19 11:07 AM, Harald Geyer wrote:
>>>>>>>>> [e-mail] writes:
>>>>>>>>>> From: Marek Vasut <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> Reword the binding document to make it clear how the propeties
>>>>>>>>>> work
>>>>>>>>>> and which properties affect which other properties.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Marek Vasut <[e-mail]>
>>>>>>>>>> Cc: Harald Geyer <[e-mail]>
>>>>>>>>>> Cc: Kuninori Morimoto <[e-mail]>
>>>>>>>>>> Cc: Linus Walleij <[e-mail]>
>>>>>>>>>> Cc: Mark Brown <[e-mail]>
>>>>>>>>>> Cc: Rob Herring <[e-mail]>
>>>>>>>>>> Cc: [e-mail]
>>>>>>>>>> To: [e-mail]
>>>>>>>>>> ---
>>>>>>>>>> V2: - Make "gpios" a mandatory property
>>>>>>>>>>     - Reword "gpio-states" property description
>>>>>>>>>>     - Change "enable-gpio" to "enable-gpios" to match modern
>>>>>>>>>> DT rules
>>>>>>>>>> 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 for the update. I think it addresses all my concerns
>>>>>>>>> except for
>>>>>>>>> one:
>>>>>>>>>
>>>>>>>>>> +- gpios-states    : State of GPIO pins in "gpios" array that
>>>>>>>>>> is set until
>>>>>>>>>> +              changed by the first consumer. 0: LOW, 1: HIGH.
>>>>>>>>>> +              Default is LOW if nothing else is specified.
>>>>>>>>>
>>>>>>>>> I still believe this not true: There is no guarantee that the
>>>>>>>>> regulator
>>>>>>>>> core won't change the state of GPIO pins before the first
>>>>>>>>> consumer comes
>>>>>>>>> up.
>>>>>>>>
>>>>>>>> Why would it do that ?
>>>>>>>
>>>>>>> Because the regulator core doesn't know about this driver specific
>>>>>>> property at all. And without any constraints placed by consumers,
>>>>>>> the
>>>>>>> core is free to choose any state whatsoever at any point in time.
>>>>>>
>>>>>> But git grep seems to disagree, see
>>>>>> drivers/regulator/gpio-regulator.c:
>>>>>>                     ret = of_property_read_u32_index(np,
>>>>>> "gpios-states", i,
>>>>>>
>>>>>> The core sets the pins to such a value until the consumer takes over.
>>>>>
>>>>> I think we have a misunderstanding of terminology. When I write
>>>>> "regulator
>>>>> core", I mean the driver independent regulator code. The line you
>>>>> quote
>>>>> above is part of the gpio-regulator driver and thus not part of what
>>>>> I call the "regulator core".
>>>>>
>>>>> AFAICS the data from the property is only stored in a driver specific
>>>>> data structure (and not used at all outside of probe) but never passed
>>>>> to what I call the regulator core.
>>>>>
>>>>> Why do you believe there is a guarantee that the value set during
>>>>> probeing is preserved until a consumer takes over?
>>>>
>>>> It is the only sensible behavior and the behavior I see people expect
>>>> from this property. I presume it solidified in this sort of
>>>> semi-defined
>>>> state, so we're stuck with assuming it behaves this way to maintain
>>>> compatibility.
>>>
>>> Maybe the behaviour you want would be more sensible, but AFAIK it just
>>> isn't true in general (it might work that way by chance in many cases).
>>> If people expect this behaviour, it is a misunderstanding of the old
>>> wording.
>>> I'd prefer we don't have to add a quirk to the regulator subsystem to
>>> cater for a misunderstanding.
>>>
>>> I think, if you really want to go forward with making this behaviour
>>> officially maintained, then we should first add the code to linux and
>>> only then add the promise to the binding document. This isn't the scope
>>> of this patch, so I guess we would need to keep the ambiguous wording as
>>> it is for now. I believe it is more important for a binding document
>>> to be correct than to be sensible.
>>>
>>> However I don't think we actually need to go to such extremes: In linux
>>> we currently have (arm/boot/dts and arm64/boot/dts) 38 uses of this
>>> property in 29 DTs. All the examples, that I studied in some detail,
>>> seem to either don't need this property at all or have a usecase that is
>>> supported by my proposed wording. I don't expect any problems if we just
>>> document the status quo clearly.
>>
>> In that case, provide a suggestion how to document this property better?
> 
> I did: https://www.spinics.net/lists/devicetree/msg275050.html

I don't like it, but I added it there and sent V3, let's see what others
think.

Patch

diff --git a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
index 1f496159e2bb..d7f8c1b17db6 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
+- gpios			: Array of one or more GPIO pins used to select the
+			  regulator voltage/current listed in "states".
+- 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-gpios		: 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-states		: State of GPIO pins in "gpios" array that is set until
+			  changed by the first consumer. 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.
 
@@ -30,7 +39,7 @@  Example:
 		regulator-max-microvolt = <2600000>;
 		regulator-boot-on;
 
-		enable-gpio = <&gpio0 23 0x4>;
+		enable-gpios = <&gpio0 23 0x4>;
 		gpios = <&gpio0 24 0x4
 			 &gpio0 25 0x4>;
 		states = <1800000 0x3