On 2/18/19 11:04 AM, Harald Geyer wrote: [...] >>>>>>> IIRC the regulator >>>>>>> core automatically selects the lowest voltage/current compatible with >>>>>>> all consumers. It seems the only usecase is to specify an initial >>>>>>> state that is different from all states in the "states" property, before >>>>>>> the regulator is turned on for the first time. However it also is not >>>>>>> an off-state, because it won't be set again on turning the regulator off. >>>>>> >>>>>> Correct, this is not an off state. If you have a better wording, I am >>>>>> open to that. >>>>> >>>>> I think it should be clearer that this is an array. (Looking at various >>>>> users I doubt everybody was aware of that, but since we have only instances >>>>> with one gpio, there is no functional difference between array and bit field.) >>>>> Also it should be recommended to set this to match whatever the bootloader >>>>> sets up. Maybe something like: >>>>> >>>>> - gpios-states: Array of GPIOs values set during probing the regulator. >>>>> 0: LOW, 1: HIGH. If continuous operation during boot is >>>>> desired, this needs to match what the firmware or bootloader >>>>> sets up. By default all GPIOs are set to low during probing. >>>> >>>> gpios-states has nothing to do with continuous operation, that's what >>>> enable-gpios is for, we should not confuse the readers with it. >>> >>> I think we have some misunderstanding here, because I don't see how >>> enable-gpios is relevant. Also maybe I still haven't understood the >>> usecase, but IMO that indicates, that it should be cleared up in >>> the document. >> >> The enable-gpios property is what turns the regulator ON/OFF , the >> gpio-states is what selects the output voltage/current . Thus, I think >> we should not mention "continuous operation" in the description. > > Okay, I see where you are coming from. Yes, gpios-states is only part > of what is required for continuous operation during boot. Still it seems > to be the only realistic usecase, we have come up so far. (Also in > existing DTs it seems to be used mostly with "boot-on" regulators.) Consider eMMC bus voltage switch, which only selects between 3V3 and 1V8 . It has nothing to do with continuous operation, it just selects one voltage level or the other. So what about gpios-states : Initial state of GPIO pins in "gpios" array, set on system start and retained until consumer changes the state. 0: LOW, 1: HIGH. Default is LOW if nothing else is specified. > Maybe we can word it better then I did above, but I think mentioning > this helps a lot to make it clearer how an implementation must behave > if this property is present. > >>>> Also >>>> note that "probing" is OS specific, so we should use "default" instead. >>>> What about this: >>> >>>> gpios-states : Array of initial states of GPIO pins in "gpios" array. >>>> 0: LOW, 1: HIGH. Default is LOW if nothing else is specified. >>> >>> "initial" is a bit too unspecific IMO. >> >> Well, got any better idea ? It's not "default" either. > > Yes, it's difficult - which is why I cheated above and described what the > linux implementation does instead of describing it in general terms. > If the semantics of this property are too unclear to unambiguously > describe in an implementation indepenent way, then that's a point in > favor of marking this property deprecated and let whoever actually > needs it come up with something better. We still need to support it though, due to older DTs, so we should make it clear what it does. >>> I believe this description doesn't define behaviour sufficiently and a >>> future rewrite (or reimplementation for a different OS) is likely to >>> interpret it in an incompatible way. I think one could even argue that >>> completely ignoring this property and just setting up a valid state from >>> "states" is allowed behaviour. This would likely break existing DTs, where >>> among other things this regulator is used to set the CPU's voltage. >> >> I suspect ignoring this property, when it's present, wouldn't be a good >> idea. After all, it adds additional information about the behavior of >> the system upon start up. > > Yes, it wouldn't be good, because it likely causes breakage. No, I don't > think it adds much additional information, because it is too unclear. > >> btw I am rewording it exactly because there was a recent breakage in the >> regulator code. > > Yeah, thanks for that. Your patch is an improvement and shouldn't be > held back because it doesn't address the other problems discussed above. I am hoping maybe someone who's actually native english speaker can help out with the fine wording. -- Best regards, Marek Vasut
