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:
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.
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
--
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w
or CLAM xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN
