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: >>>>>>>>> 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 I don't like it, but I added it there and sent V3, let's see what others think. -- Best regards, Marek Vasut
