From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from h1.radempa.de ([176.9.142.194]:44598 "EHLO mail.cosmopool.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726028AbfCGJNA (ORCPT ); Thu, 7 Mar 2019 04:13:00 -0500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 07 Mar 2019 10:12:52 +0100 From: Harald Geyer Subject: Re: [PATCH V2] regulator: gpio: Reword the binding document In-Reply-To: <96fe2cd4-c680-31c9-98f2-31a400e6c44b@gmail.com> References: <20190304194023.10926-1-marek.vasut@gmail.com> <834f8576-73a4-be0f-e322-91601bed5388@gmail.com> <4d1cd905-c322-78e5-cb9c-dadba01b0324@gmail.com> <89743562-bb2a-de3f-a57b-331f43bf8982@gmail.com> <96fe2cd4-c680-31c9-98f2-31a400e6c44b@gmail.com> Message-ID: Sender: devicetree-owner@vger.kernel.org To: Marek Vasut Cc: devicetree@vger.kernel.org, Marek Vasut , Kuninori Morimoto , Linus Walleij , Mark Brown , Rob Herring , linux-renesas-soc@vger.kernel.org List-ID: 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@gmail.com writes: >>>>>>>>> From: Marek Vasut >>>>>>>>> >>>>>>>>> Reword the binding document to make it clear how the >>>>>>>>> propeties work >>>>>>>>> and which properties affect which other properties. >>>>>>>>> >>>>>>>>> Signed-off-by: Marek Vasut >>>>>>>>> Cc: Harald Geyer >>>>>>>>> Cc: Kuninori Morimoto >>>>>>>>> Cc: Linus Walleij >>>>>>>>> Cc: Mark Brown >>>>>>>>> Cc: Rob Herring >>>>>>>>> Cc: linux-renesas-soc@vger.kernel.org >>>>>>>>> To: devicetree@vger.kernel.org >>>>>>>>> --- >>>>>>>>> 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