diff mbox

[3/5] regulator: dt-bindings: Add regulator-initial-mode support

Message ID 1412775847-15213-4-git-send-email-javier.martinez@collabora.co.uk
State New, archived
Headers show

Commit Message

Javier Martinez Canillas Oct. 8, 2014, 1:44 p.m. UTC
Regulators can run on different operating modes (opmodes). This allows
systems to choose the most efficient opmode for each regulator. The
regulator core defines a set of generic modes so each system can define
the opmode in these generic terms and drivers are responsible to map the
generic modes to the ones supported by each hardware according to their
data-sheet.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 Documentation/devicetree/bindings/regulator/regulator.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Mark Brown Oct. 8, 2014, 2:34 p.m. UTC | #1
On Wed, Oct 08, 2014 at 03:44:05PM +0200, Javier Martinez Canillas wrote:

> +- regulator-initial-mode: initial regulator operating mode. One of following:
> +	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
> +	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
> +	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
> +	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
> +  modes are defined in the dt-bindings/regulator/regulator.h header and can be
> +  used in device tree sources files. If no mode is defined, then the OS will not
> +  manage the operating mode and the HW default values will be used instead.

...and as previously and repeatedly discussed this still gives the user
no way of telling if or how these modes might correspond to what the
chip is capable of doing.  This really needs addressing rather than
ignoring, for example by not trying to define the modes in generic
bindings.
Javier Martinez Canillas Oct. 8, 2014, 4 p.m. UTC | #2
On 10/08/2014 04:34 PM, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 03:44:05PM +0200, Javier Martinez Canillas wrote:
> 
>> +- regulator-initial-mode: initial regulator operating mode. One of following:
>> +	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
>> +	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
>> +	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
>> +	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
>> +  modes are defined in the dt-bindings/regulator/regulator.h header and can be
>> +  used in device tree sources files. If no mode is defined, then the OS will not
>> +  manage the operating mode and the HW default values will be used instead.
> 
> ...and as previously and repeatedly discussed this still gives the user
> no way of telling if or how these modes might correspond to what the
> chip is capable of doing.  This really needs addressing rather than
> ignoring, for example by not trying to define the modes in generic
> bindings.
> 

Drivers could add custom per-device DT properties. That is how it was
solved in the ChromeOS kernel. There is a regulator-op-mode DT property
whose value is just a magic number with the value that has to be written
in the OPMODE field of the control register of each regulator and that
is made on the driver probe.

Another option is to not document the standard modes in the generic
regulator binding but instead on each device DT binding doc. That way
each device binding can define what are the semantics of the standard
modes and how correspond to the real modes in the chip.

That way the regulator-initial-mode could still be generic and parsed
by the core instead of each driver implementing their own custom parsing.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Oct. 9, 2014, 8:45 a.m. UTC | #3
On śro, 2014-10-08 at 15:44 +0200, Javier Martinez Canillas wrote:
> Regulators can run on different operating modes (opmodes). This allows
> systems to choose the most efficient opmode for each regulator. The
> regulator core defines a set of generic modes so each system can define
> the opmode in these generic terms and drivers are responsible to map the
> generic modes to the ones supported by each hardware according to their
> data-sheet.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  Documentation/devicetree/bindings/regulator/regulator.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
> index ccba90b..a9d6767 100644
> --- a/Documentation/devicetree/bindings/regulator/regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
> @@ -23,6 +23,14 @@ Optional properties:
>    state among following defined suspend states:
>    <3>: PM_SUSPEND_MEM - Setup regulator according to regulator-state-mem
>    <4>: PM_SUSPEND_MAX - Setup regulator according to regulator-state-disk
> +- regulator-initial-mode: initial regulator operating mode. One of following:
> +	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
> +	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
> +	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
> +	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
> +  modes are defined in the dt-bindings/regulator/regulator.h header and can be
> +  used in device tree sources files. If no mode is defined, then the OS will not
> +  manage the operating mode and the HW default values will be used instead.
>  - regulator-state-mem sub-root node for Suspend-to-RAM mode
>    : suspend to memory, the device goes to sleep, but all data stored in memory,
>    only some external interrupt can wake the device.

I agree with the need and the idea of generic bindings for operating
modes for regulators. At least for Exynos-based boards the PMICs have
quite similar opmodes.

However the regulator mode from consumer.h (and in above doc) does not
match well with these opmodes. Example is yours patch 4/5:
 - idle ("more efficient mode") maps to "low power mode in suspend",
 - standby ("the most efficient mode") maps to "OFF in suspend".

Actually we are not enable "efficient modes" but we configure how the
regulator will behave when AP says - I'm suspending.


Another issue: is "initial_state" not doing all this already?

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas Oct. 9, 2014, 3:04 p.m. UTC | #4
Hello Krzysztof,

Thanks a lot for your feedback.

On 10/09/2014 10:45 AM, Krzysztof Kozlowski wrote:
> On śro, 2014-10-08 at 15:44 +0200, Javier Martinez Canillas wrote:
>> --- a/Documentation/devicetree/bindings/regulator/regulator.txt
>> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
>> @@ -23,6 +23,14 @@ Optional properties:
>>    state among following defined suspend states:
>>    <3>: PM_SUSPEND_MEM - Setup regulator according to regulator-state-mem
>>    <4>: PM_SUSPEND_MAX - Setup regulator according to regulator-state-disk
>> +- regulator-initial-mode: initial regulator operating mode. One of following:
>> +	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
>> +	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
>> +	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
>> +	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
>> +  modes are defined in the dt-bindings/regulator/regulator.h header and can be
>> +  used in device tree sources files. If no mode is defined, then the OS will not
>> +  manage the operating mode and the HW default values will be used instead.
>>  - regulator-state-mem sub-root node for Suspend-to-RAM mode
>>    : suspend to memory, the device goes to sleep, but all data stored in memory,
>>    only some external interrupt can wake the device.
> 
> I agree with the need and the idea of generic bindings for operating
> modes for regulators. At least for Exynos-based boards the PMICs have
> quite similar opmodes.
> 
> However the regulator mode from consumer.h (and in above doc) does not
> match well with these opmodes. Example is yours patch 4/5:
>  - idle ("more efficient mode") maps to "low power mode in suspend",
>  - standby ("the most efficient mode") maps to "OFF in suspend".
> 
> Actually we are not enable "efficient modes" but we configure how the
> regulator will behave when AP says - I'm suspending.
>

Agree, Mark also pointed out that there is a difference between changing
how the regulator will behave on runtime vs changing how the regulator
will behave during system suspend. AFAIU from his explanation, the modes
defined in consumer.h only applies to the former and conceptually there
should be a difference between those two cases even when the Maxim PMIC
seems to mix it both in the data-sheet and by using the same field.

I answered Mark on patch #5 with a suggestion on how to handle this in
a generic way to avoid each driver having their own custom DT binding
and duplicating the parsing code.

Could you please answer there with your thoughts?
 
> 
> Another issue: is "initial_state" not doing all this already?
> 

One of the options I indeed evaluated was if using initial_state could be
possible. But currently the .set_suspend_mode function handler is only
called for the STANDBY, MEM and MAX suspend modes. While is true that this
could be extended to also support PM_SUSPEND_ON, I don't think that it
makes sense conceptually. Since is not the same to set a mode to be used
at runtime than setting a mode to be used during suspend.

AFAIU that is the reason why there are separate .set_suspend_mode and
.set_mode operations and that's why there are both an .initial_state
and an .initial_mode.

> Best regards,
> Krzysztof
>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Oct. 9, 2014, 7:01 p.m. UTC | #5
On Thu, Oct 09, 2014 at 05:04:35PM +0200, Javier Martinez Canillas wrote:

> Agree, Mark also pointed out that there is a difference between changing
> how the regulator will behave on runtime vs changing how the regulator
> will behave during system suspend. AFAIU from his explanation, the modes
> defined in consumer.h only applies to the former and conceptually there
> should be a difference between those two cases even when the Maxim PMIC
> seems to mix it both in the data-sheet and by using the same field.

No, that's not accurate at all - you're still not getting the concepts
of modes or suspend handling in the regulator API.  I really think you
need to take a step back and try to understand what's currently there
before trying to make changes here.  We've got a set of operations we
can use to change the regulator configuration, if you look at the
existing driver interface you'll see that these are matched with
equivalent operations for setting the behaviour when in suspend
(including a set_suspend_mode() operation).  

Like I keep saying abstractions are really important to making sure the
code is maintainable.
Javier Martinez Canillas Oct. 9, 2014, 9:56 p.m. UTC | #6
On 10/09/2014 09:01 PM, Mark Brown wrote:
> On Thu, Oct 09, 2014 at 05:04:35PM +0200, Javier Martinez Canillas wrote:
> 
>> Agree, Mark also pointed out that there is a difference between changing
>> how the regulator will behave on runtime vs changing how the regulator
>> will behave during system suspend. AFAIU from his explanation, the modes
>> defined in consumer.h only applies to the former and conceptually there
>> should be a difference between those two cases even when the Maxim PMIC
>> seems to mix it both in the data-sheet and by using the same field.
> 
> No, that's not accurate at all - you're still not getting the concepts
> of modes or suspend handling in the regulator API.  I really think you
> need to take a step back and try to understand what's currently there
> before trying to make changes here.  We've got a set of operations we
> can use to change the regulator configuration, if you look at the
> existing driver interface you'll see that these are matched with
> equivalent operations for setting the behaviour when in suspend
> (including a set_suspend_mode() operation).  
> 

I tried to say that there is a difference between the need to change
within the kernel a regulator configuration (e.g: change opmode from
normal to low power) when the system is going to enter in suspend state
vs setting a register at probe time that will force the PMIC to change
the regulator to low power mode on suspend without kernel intervention.

> Like I keep saying abstractions are really important to making sure the
> code is maintainable.
> 

Agree, I'll try to come up with a more sensible solution by using the
existing abstractions from the regulator framework.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index ccba90b..a9d6767 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -23,6 +23,14 @@  Optional properties:
   state among following defined suspend states:
   <3>: PM_SUSPEND_MEM - Setup regulator according to regulator-state-mem
   <4>: PM_SUSPEND_MAX - Setup regulator according to regulator-state-disk
+- regulator-initial-mode: initial regulator operating mode. One of following:
+	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
+	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
+	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
+	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
+  modes are defined in the dt-bindings/regulator/regulator.h header and can be
+  used in device tree sources files. If no mode is defined, then the OS will not
+  manage the operating mode and the HW default values will be used instead.
 - regulator-state-mem sub-root node for Suspend-to-RAM mode
   : suspend to memory, the device goes to sleep, but all data stored in memory,
   only some external interrupt can wake the device.