Re: [PATCH] regulator: gpio: Reword the binding document

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/17/19 9:00 PM, Harald Geyer wrote:
> Marek Vasut writes:
>> 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.
> 
> AFAIK the core is free to switch the regulator to any state that is
> compatible with all consumers. Ie there are no guarantees before the
> first consumer requests the regulator.

That's correct.

> I only mentioned probing the
> first consumer, because it is a time where the core is likely to
> revisit it's decisions. The root problem is, that this property (as it
> is documented now, and also as it is implemented in linux now) doesn't
> acutally define behaviour beyond "gpios will be set to this state once
> (but at an unkown time and for an unkown duration)".

ACK

>> The core should only set the default state and that's it.
> 
> That's hits exactly the problem I see:
> * gpios-states is not a _default_ state (but an initial state)

Right, OK, this we should clarify.

> * the core doesn't set this state (the driver writes it during probe - maybe
>   at a time where probing might still fail)

Presumably the regulator driver, yes ?

> * the core doesn't even know about this state
> 
>>> 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.
> 
> If this is the case, why would we need to set an initial state instead
> of just waiting what the first consumer requests. I still don't see
> a use case (aside from don't accidentally shut down something important
> during probing).

Possibly to prevent a state which might be harmful until the first
consumer comes up. However this is rather theoretical.

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

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

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

btw I am rewording it exactly because there was a recent breakage in the
regulator code.

-- 
Best regards,
Marek Vasut



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux