On 3/5/19 5:10 PM, Harald Geyer wrote:
> Marek Vasut writes:
>> On 3/5/19 11:07 AM, Harald Geyer wrote:
>>> marek.vasut@xxxxxxxxx writes:
>>>> From: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
>>>>
>>>> Reword the binding document to make it clear how the propeties work
>>>> and which properties affect which other properties.
>>>>
>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
>>>> Cc: Harald Geyer <harald@xxxxxxxxx>
>>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
>>>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
>>>> Cc: Mark Brown <broonie@xxxxxxxxxx>
>>>> Cc: Rob Herring <robh@xxxxxxxxxx>
>>>> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
>>>> To: devicetree@xxxxxxxxxxxxxxx
>>>> ---
>>>> 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 <harald@xxxxxxxxx>
>>> 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
>
--
Best regards,
Marek Vasut
