On 2/17/19 3:26 PM, Harald Geyer wrote: > Marek Vasut writes: >> On 2/16/19 9:20 PM, Harald Geyer wrote: >>> marek.vasut@xxxxxxxxx writes: >>>> + regulator voltage/current listed in "states". >>>> +- gpios-states : Initial state of GPIO pins in "gpios" array. >>>> + 0: LOW, 1: HIGH. >>>> + Default is LOW if nothing else is specified. >>> >>> This is not very clear. Maybe add an example below or explain it better. >>> >>> I guess we can't change it now anymore anyway, but it's not clear to >>> me, why we have this in the first place: A regulator should only be >>> active when it has a consumer or is "always-on". >> >> Be careful here, the regulator-gpio may be instantiated such that it >> will select between two voltage states, neither of which is 0V/off. >> Consider eMMC vccq supply, which alternates between 1V8 and 3V3, but >> does not have a third, 0V/off state (unless you pull the plug). > > Yes, if it is always on (or simply has no enable-gpio) then we might > select a voltage while probing the device. My point is: At some point > (probably when the first consumer probes, but I think there are no > guarantees) the core will switch the regulator state. That usually > happens a few milliseconds to seconds after probing the regulator > itself. This seems to be nothing but a source of undefined behaviour. Why would the core switch the regulator state when the first consumer probes ? It is up to the consumer to reconfigure the regulator when/as it sees fit . The core should only set the default state and that's it. > If continuous operation of the regulator is important, I'd expect > we don't touch whatever the firmware setup instead of by default > setting all gpios to low. > > However as we can't change this now, there isn't much point in > discussin this further. The firmware could've left the regulator in non-default state. >>> 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 continous 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. gpio-states has nothing to do with continuous operation, that's what enable-gpios is for, we should not confuse the readers with it. 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. -- Best regards, Marek Vasut
