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