| Message ID | 1412775847-15213-4-git-send-email-javier.martinez@collabora.co.uk |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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.
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
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
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
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.
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 --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.
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(+)