diff mbox

[v2,11/14] regulator: s2mps11: Add opmode for S2MPS14 regulators

Message ID 1392282847-25444-12-git-send-email-k.kozlowski@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Krzysztof Kozlowski Feb. 13, 2014, 9:14 a.m. UTC
S2MPS11/S2MPS14 regulators support different modes of operation:
 - Always off;
 - On/Off controlled by pin/GPIO (PWREN/LDOEN/EMMCEN);
 - Always on;
This is very similar to S5M8767 regulator driver which also supports
opmodes (although S5M8767 have also low-power mode).

This patch adds parsing the operation mode from DTS by reading a
"op_mode" property from regulator child node.

The op_mode is then used for enabling the S2MPS14 regulators.
On S2MPS11 the DTS "op_mode" property is parsed but not used for
enabling, as this was not tested.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
---
 drivers/regulator/s2mps11.c         |   97 ++++++++++++++++++++++++++++++++++-
 include/linux/mfd/samsung/s2mps14.h |   19 +++++++
 2 files changed, 115 insertions(+), 1 deletion(-)

Comments

Yadwinder Singh Brar Feb. 13, 2014, 12:16 p.m. UTC | #1
Hi,

On Thu, Feb 13, 2014 at 2:44 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> S2MPS11/S2MPS14 regulators support different modes of operation:
>  - Always off;
>  - On/Off controlled by pin/GPIO (PWREN/LDOEN/EMMCEN);
>  - Always on;
> This is very similar to S5M8767 regulator driver which also supports
> opmodes (although S5M8767 have also low-power mode).
>
> This patch adds parsing the operation mode from DTS by reading a
> "op_mode" property from regulator child node.
>

First thing since "op_mode" is not generic property, I think it should
be appended with some driver specific prefix.

But IMHO its quite generic property used and required by many other
PMICs(almost all used by Samsung).
I would like to use this opportunity to discuss about adding it as
generic regulator constraint(as initial_mode)
by providing a default mapping of generic Regulator operating
modes(kernel specific) to operating modes supported by hardware in
regulator driver itself.

Regards,
Yadwinder
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Feb. 13, 2014, 12:43 p.m. UTC | #2
> S2MPS11/S2MPS14 regulators support different modes of operation:
>  - Always off;
>  - On/Off controlled by pin/GPIO (PWREN/LDOEN/EMMCEN);
>  - Always on;
> This is very similar to S5M8767 regulator driver which also supports
> opmodes (although S5M8767 have also low-power mode).
> 
> This patch adds parsing the operation mode from DTS by reading a
> "op_mode" property from regulator child node.
> 
> The op_mode is then used for enabling the S2MPS14 regulators.
> On S2MPS11 the DTS "op_mode" property is parsed but not used for
> enabling, as this was not tested.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> ---
>  drivers/regulator/s2mps11.c         |   97 ++++++++++++++++++++++++++++++++++-
>  include/linux/mfd/samsung/s2mps14.h |   19 +++++++
>  2 files changed, 115 insertions(+), 1 deletion(-)

<snip>

> +++ b/include/linux/mfd/samsung/s2mps14.h
> @@ -149,4 +149,23 @@ enum s2mps14_regulators {
>  #define S2MPS14_LDO_N_VOLTAGES		(S2MPS14_LDO_VSEL_MASK + 1)
>  #define S2MPS14_BUCK_N_VOLTAGES		(S2MPS14_BUCK_VSEL_MASK + 1)
>  
> +#define S2MPS14_ENCTRL_SHIFT		6
> +#define S2MPS14_ENCTRL_MASK		(0x3 << S2MPS14_ENCTRL_SHIFT)
> +
> +/*
> + * Values of regulator operation modes match device tree bindings.
> + */
> +enum s2mps14_regulator_opmode {
> +	S2MPS14_REGULATOR_OPMODE_OFF		= 0,
> +	S2MPS14_REGULATOR_OPMODE_ON		= 1,
> +	/*
> +	 * Reserved for compatibility with S5M8767 where this
> +	 * is a low power mode.
> +	 */
> +	S2MPS14_REGULATOR_OPMODE_RESERVED	= 2,
> +	S2MPS14_REGULATOR_OPMODE_SUSPEND	= 3,

You don't need to number these like this. If you want to force the
numbering to start at '0' initialise the top value, then the rest
should be sequential.

> +	S2MPS14_REGULATOR_OPMODE_MAX,
> +};
> +
>  #endif /*  __LINUX_MFD_S2MPS14_H */
Krzysztof Kozlowski Feb. 13, 2014, 1 p.m. UTC | #3
On Thu, 2014-02-13 at 12:43 +0000, Lee Jones wrote:
> > S2MPS11/S2MPS14 regulators support different modes of operation:
> >  - Always off;
> >  - On/Off controlled by pin/GPIO (PWREN/LDOEN/EMMCEN);
> >  - Always on;
> > This is very similar to S5M8767 regulator driver which also supports
> > opmodes (although S5M8767 have also low-power mode).
> > 
> > This patch adds parsing the operation mode from DTS by reading a
> > "op_mode" property from regulator child node.
> > 
> > The op_mode is then used for enabling the S2MPS14 regulators.
> > On S2MPS11 the DTS "op_mode" property is parsed but not used for
> > enabling, as this was not tested.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Liam Girdwood <lgirdwood@gmail.com>
> > ---
> >  drivers/regulator/s2mps11.c         |   97 ++++++++++++++++++++++++++++++++++-
> >  include/linux/mfd/samsung/s2mps14.h |   19 +++++++
> >  2 files changed, 115 insertions(+), 1 deletion(-)
> 
> <snip>
> 
> > +++ b/include/linux/mfd/samsung/s2mps14.h
> > @@ -149,4 +149,23 @@ enum s2mps14_regulators {
> >  #define S2MPS14_LDO_N_VOLTAGES		(S2MPS14_LDO_VSEL_MASK + 1)
> >  #define S2MPS14_BUCK_N_VOLTAGES		(S2MPS14_BUCK_VSEL_MASK + 1)
> >  
> > +#define S2MPS14_ENCTRL_SHIFT		6
> > +#define S2MPS14_ENCTRL_MASK		(0x3 << S2MPS14_ENCTRL_SHIFT)
> > +
> > +/*
> > + * Values of regulator operation modes match device tree bindings.
> > + */
> > +enum s2mps14_regulator_opmode {
> > +	S2MPS14_REGULATOR_OPMODE_OFF		= 0,
> > +	S2MPS14_REGULATOR_OPMODE_ON		= 1,
> > +	/*
> > +	 * Reserved for compatibility with S5M8767 where this
> > +	 * is a low power mode.
> > +	 */
> > +	S2MPS14_REGULATOR_OPMODE_RESERVED	= 2,
> > +	S2MPS14_REGULATOR_OPMODE_SUSPEND	= 3,
> 
> You don't need to number these like this. If you want to force the
> numbering to start at '0' initialise the top value, then the rest
> should be sequential.

Yes, I know. I wanted to emphasize the relationship to opmode entries in
DTS (to prevent adding new enum value somewhere between them). The code
somehow self-documents that the enum should only be extended, not
modified.

Best regards,
Krzysztof

> 
> > +	S2MPS14_REGULATOR_OPMODE_MAX,
> > +};
> > +
> >  #endif /*  __LINUX_MFD_S2MPS14_H */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 13, 2014, 7:28 p.m. UTC | #4
On Thu, Feb 13, 2014 at 10:14:04AM +0100, Krzysztof Kozlowski wrote:
> S2MPS11/S2MPS14 regulators support different modes of operation:
>  - Always off;
>  - On/Off controlled by pin/GPIO (PWREN/LDOEN/EMMCEN);
>  - Always on;
> This is very similar to S5M8767 regulator driver which also supports
> opmodes (although S5M8767 have also low-power mode).

What is the difference between always off/on and the normal enable
control?  This sounds like a fairly standard register control that can
be overridden by a GPIO.  The usual way of specifying that is by keying
off the presence of the GPIO.
Krzysztof Kozlowski Feb. 14, 2014, 8:15 a.m. UTC | #5
On Thu, 2014-02-13 at 19:28 +0000, Mark Brown wrote:
> On Thu, Feb 13, 2014 at 10:14:04AM +0100, Krzysztof Kozlowski wrote:
> > S2MPS11/S2MPS14 regulators support different modes of operation:
> >  - Always off;
> >  - On/Off controlled by pin/GPIO (PWREN/LDOEN/EMMCEN);
> >  - Always on;
> > This is very similar to S5M8767 regulator driver which also supports
> > opmodes (although S5M8767 have also low-power mode).
> 
> What is the difference between always off/on and the normal enable
> control?  This sounds like a fairly standard register control that can
> be overridden by a GPIO.  The usual way of specifying that is by keying
> off the presence of the GPIO.

My initial idea was to do this similarly to the S5M8767 regulator (where
there is also 4th mode: low power). The presence of GPIO in DTS can
simplify the bindings but on the other hand it wouldn't be compatible
with S5M8767 regulator driver. This may complicate the merge of these
drivers.

What is your opinion on this - should I abandon the "op_mode" idea and
use presence of GPIO?

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Feb. 14, 2014, 1:05 p.m. UTC | #6
On Thu, 2014-02-13 at 17:46 +0530, Yadwinder Singh Brar wrote:
> Hi,
> 
> On Thu, Feb 13, 2014 at 2:44 PM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
> > S2MPS11/S2MPS14 regulators support different modes of operation:
> >  - Always off;
> >  - On/Off controlled by pin/GPIO (PWREN/LDOEN/EMMCEN);
> >  - Always on;
> > This is very similar to S5M8767 regulator driver which also supports
> > opmodes (although S5M8767 have also low-power mode).
> >
> > This patch adds parsing the operation mode from DTS by reading a
> > "op_mode" property from regulator child node.
> >
> 
> First thing since "op_mode" is not generic property, I think it should
> be appended with some driver specific prefix.
> 
> But IMHO its quite generic property used and required by many other
> PMICs(almost all used by Samsung).
> I would like to use this opportunity to discuss about adding it as
> generic regulator constraint(as initial_mode)
> by providing a default mapping of generic Regulator operating
> modes(kernel specific) to operating modes supported by hardware in
> regulator driver itself.
> 
> Regards,
> Yadwinder

Hi,

I was thinking about this. This relates also to ideas pointed by Mark:
 - Maybe s2mps11 and s5m8767 regulator drivers could be merged into one;
 - The external control should be determined by presence of attribute
with gpios.

The S5M8767 has following operation modes (except on/off):
 - external control by GPIO;
 - On/Off controlled by PWREN;
 - low-power mode;
 - low-power mode controlled by PWREN;
Although not all are present for each regulator.

The S2MPS14 is easier:
 - external control by GPIO;
 - On/Off controlled by PWREN;

A generic solution for operating mode of regulators (not only s2mps11
and s5m8767) could cover all of these above or just a subset, for
example regulator bindings could look like:
 - regulator-mode-suspend; /* PWR controls: on/off or low-power mode */
 - regulator-mode-low-power; /* Low power mode */


What do you think?

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 14, 2014, 8:59 p.m. UTC | #7
On Fri, Feb 14, 2014 at 09:15:12AM +0100, Krzysztof Kozlowski wrote:

> My initial idea was to do this similarly to the S5M8767 regulator (where
> there is also 4th mode: low power). The presence of GPIO in DTS can
> simplify the bindings but on the other hand it wouldn't be compatible
> with S5M8767 regulator driver. This may complicate the merge of these
> drivers.

They can always use separate ops.

> What is your opinion on this - should I abandon the "op_mode" idea and
> use presence of GPIO?

Yes, that's better - especially since the framework has support for
enable GPIOs.  It can do things like handle cases where the hardware has
tied the enables for several regulators together so they all need to be
enabled and disabled as one.
Mark Brown Feb. 14, 2014, 9:05 p.m. UTC | #8
On Fri, Feb 14, 2014 at 02:05:56PM +0100, Krzysztof Kozlowski wrote:
> On Thu, 2014-02-13 at 17:46 +0530, Yadwinder Singh Brar wrote:

>  - low-power mode;
>  - low-power mode controlled by PWREN;
> Although not all are present for each regulator.

What exactly is low power mode and how does it interact with the enable?
The name suggests it's a more efficient mode for use with low current
drain, is that right?

> A generic solution for operating mode of regulators (not only s2mps11
> and s5m8767) could cover all of these above or just a subset, for
> example regulator bindings could look like:
>  - regulator-mode-suspend; /* PWR controls: on/off or low-power mode */
>  - regulator-mode-low-power; /* Low power mode */

Those properties have awfully generic names and part of what I was
wondering above is if the low power mode maps onto the idle or standby
modes that the API defines?  We don't cover those in the bindings yet
since they are unfortunately fuzzy but perhaps we need to do so.
Krzysztof Kozlowski Feb. 17, 2014, 8:07 a.m. UTC | #9
On Fri, 2014-02-14 at 21:05 +0000, Mark Brown wrote:
> On Fri, Feb 14, 2014 at 02:05:56PM +0100, Krzysztof Kozlowski wrote:
> > On Thu, 2014-02-13 at 17:46 +0530, Yadwinder Singh Brar wrote:
> 
> >  - low-power mode;
> >  - low-power mode controlled by PWREN;
> > Although not all are present for each regulator.
> 
> What exactly is low power mode and how does it interact with the enable?
> The name suggests it's a more efficient mode for use with low current
> drain, is that right?

Yes, it is (i.e. ground current on battery load typically reduced from
8mA to 1.5mA). In this mode for Buck5 and LDO-s the regulator is enabled
but output current is limited to 5 mA.

> 
> > A generic solution for operating mode of regulators (not only s2mps11
> > and s5m8767) could cover all of these above or just a subset, for
> > example regulator bindings could look like:
> >  - regulator-mode-suspend; /* PWR controls: on/off or low-power mode */
> >  - regulator-mode-low-power; /* Low power mode */
> 
> Those properties have awfully generic names and part of what I was
> wondering above is if the low power mode maps onto the idle or standby
> modes that the API defines?  We don't cover those in the bindings yet
> since they are unfortunately fuzzy but perhaps we need to do so.

The low power maps to REGULATOR_MODE_IDLE or REGULATOR_MODE_STANDBY
(depending on the understanding of "more efficient" and "most efficient"
for light loads). However the suspend mode of S5M8767/S2MPS14 is more
like automatic regulator disable by SoC.

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Feb. 17, 2014, 8:09 a.m. UTC | #10
On Fri, 2014-02-14 at 20:59 +0000, Mark Brown wrote:
> On Fri, Feb 14, 2014 at 09:15:12AM +0100, Krzysztof Kozlowski wrote:
> 
> > My initial idea was to do this similarly to the S5M8767 regulator (where
> > there is also 4th mode: low power). The presence of GPIO in DTS can
> > simplify the bindings but on the other hand it wouldn't be compatible
> > with S5M8767 regulator driver. This may complicate the merge of these
> > drivers.
> 
> They can always use separate ops.
> 
> > What is your opinion on this - should I abandon the "op_mode" idea and
> > use presence of GPIO?
> 
> Yes, that's better - especially since the framework has support for
> enable GPIOs.  It can do things like handle cases where the hardware has
> tied the enables for several regulators together so they all need to be
> enabled and disabled as one.

OK. I'll rewrite this patch and send later when S2MPS14 will be
accepted.

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 18, 2014, 12:35 a.m. UTC | #11
On Mon, Feb 17, 2014 at 09:07:34AM +0100, Krzysztof Kozlowski wrote:

> The low power maps to REGULATOR_MODE_IDLE or REGULATOR_MODE_STANDBY
> (depending on the understanding of "more efficient" and "most efficient"
> for light loads). However the suspend mode of S5M8767/S2MPS14 is more
> like automatic regulator disable by SoC.

I don't understand the above?  Are you saying that suspend mode actually
turns off the regulator or something else?  If it's a separate setting
for suspend mode then it should be using the core suspend mode stuff.
Krzysztof Kozlowski Feb. 18, 2014, 8:12 a.m. UTC | #12
On Tue, 2014-02-18 at 09:35 +0900, Mark Brown wrote:
> On Mon, Feb 17, 2014 at 09:07:34AM +0100, Krzysztof Kozlowski wrote:
> 
> > The low power maps to REGULATOR_MODE_IDLE or REGULATOR_MODE_STANDBY
> > (depending on the understanding of "more efficient" and "most efficient"
> > for light loads). However the suspend mode of S5M8767/S2MPS14 is more
> > like automatic regulator disable by SoC.
> 
> I don't understand the above?  Are you saying that suspend mode actually
> turns off the regulator or something else? If it's a separate setting
> for suspend mode then it should be using the core suspend mode stuff.

No, it is similar to external control (by GPIO) except that regulator is
controlled by PWREN pin. The PMIC's PWREN is not a GPIO, but instead it
is directly connected to AP (for Exynos 4212: XPWRRGTON). In AP's normal
mode the XPWRRGTON/PWREN is high. In sleep mode *AP* sets it low.

Best regards,
Krzysztof



--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 19, 2014, 4:08 a.m. UTC | #13
On Tue, Feb 18, 2014 at 09:12:09AM +0100, Krzysztof Kozlowski wrote:
> On Tue, 2014-02-18 at 09:35 +0900, Mark Brown wrote:

> > I don't understand the above?  Are you saying that suspend mode actually
> > turns off the regulator or something else? If it's a separate setting
> > for suspend mode then it should be using the core suspend mode stuff.

> No, it is similar to external control (by GPIO) except that regulator is
> controlled by PWREN pin. The PMIC's PWREN is not a GPIO, but instead it
> is directly connected to AP (for Exynos 4212: XPWRRGTON). In AP's normal
> mode the XPWRRGTON/PWREN is high. In sleep mode *AP* sets it low.

How is that different to suspend mode then?
Krzysztof Kozlowski Feb. 19, 2014, 10:09 a.m. UTC | #14
On Wed, 2014-02-19 at 13:08 +0900, Mark Brown wrote:
> On Tue, Feb 18, 2014 at 09:12:09AM +0100, Krzysztof Kozlowski wrote:
> > On Tue, 2014-02-18 at 09:35 +0900, Mark Brown wrote:
> 
> > > I don't understand the above?  Are you saying that suspend mode actually
> > > turns off the regulator or something else? If it's a separate setting
> > > for suspend mode then it should be using the core suspend mode stuff.
> 
> > No, it is similar to external control (by GPIO) except that regulator is
> > controlled by PWREN pin. The PMIC's PWREN is not a GPIO, but instead it
> > is directly connected to AP (for Exynos 4212: XPWRRGTON). In AP's normal
> > mode the XPWRRGTON/PWREN is high. In sleep mode *AP* sets it low.
> 
> How is that different to suspend mode then?

Now I see... there is no difference. It seems that the whole idea of
opmode can be replaced with suspend modes and regulator modes.

I can't only find a way to set this from DTS. There are no bindings for
regulation_constraints->state_{disk,mem,standby}.

Should the driver set manually after obtaining init_data from DTS?

Best regards,
Krzysztof



--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 19, 2014, 12:16 p.m. UTC | #15
On Wed, Feb 19, 2014 at 11:09:40AM +0100, Krzysztof Kozlowski wrote:

> I can't only find a way to set this from DTS. There are no bindings for
> regulation_constraints->state_{disk,mem,standby}.

> Should the driver set manually after obtaining init_data from DTS?

Someone should work out a suitably abstract way of defining what these
things mean and create bindings - that's not happened yet.  It's not
clear that what's in the code at the minute (which is a very direct
mapping onto Linux stuff) are general things that would apply well to
other OSs and there's not been much demand for this in general,
especially given the tendency for system designs to become more dynamic
and use case driven (or for these things to not be configurable by
software at all making the whole thing moot).

Something that defined a single suspend mode configuration suitable for
all modes would be a bit easier I think, and/or a good work through of
how things are part of the hardware rather than software configuration.
Krzysztof Kozlowski Feb. 19, 2014, 2:19 p.m. UTC | #16
On Wed, 2014-02-19 at 13:08 +0900, Mark Brown wrote:
> On Tue, Feb 18, 2014 at 09:12:09AM +0100, Krzysztof Kozlowski wrote:
> > On Tue, 2014-02-18 at 09:35 +0900, Mark Brown wrote:
> 
> > > I don't understand the above?  Are you saying that suspend mode actually
> > > turns off the regulator or something else? If it's a separate setting
> > > for suspend mode then it should be using the core suspend mode stuff.
> 
> > No, it is similar to external control (by GPIO) except that regulator is
> > controlled by PWREN pin. The PMIC's PWREN is not a GPIO, but instead it
> > is directly connected to AP (for Exynos 4212: XPWRRGTON). In AP's normal
> > mode the XPWRRGTON/PWREN is high. In sleep mode *AP* sets it low.
> 
> How is that different to suspend mode then?

I found two differences: performance (no need to send I2C commands) and
possible issues during resume.

Example: regulator which should be disabled during suspend to memory and
enabled for normal system operation.

As I understand the suspend mode (correct me if I'm wrong), the
regulator core during suspend to mem:
1. Calls suspend_set_state().
2. rstate->disabled is true so the ops->set_suspend_disable() is called.
3. The ops->set_suspend_disable() function (implemented by the driver)
disables the regulator (e.g. through I2C commands /regmap/).
4. During resume the regulator is enabled normal way (ops->enable(), I2C
again).

Possible problems:
A. What happens if some driver using this regulator resumes earlier then
regulator_suspend_finish()?
B. What happens if resuming regulator requires some other driver to be
resumed earlier (e.g. I2C bus)? If regulator resumes before I2C bus then
calling ops->enable() would fail.


The S5M8767 suspend mode (controlled by PWREN pin) works differently:
1. To enable regulator set regulator mode to "On/Off controller by
PWREN". During normal mode it will be enabled.
2. During suspend the regulator won't be suspended or disabled by
regulator core but instead the SoC will set PWREN to low and PMIC will
disable the regulator. No need to send I2C commands.
3. During resume the PWREN will be set to high and PMIC will enable the
regulator before resuming other drivers. No need to send I2C commands as
well.


Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 19, 2014, 3:07 p.m. UTC | #17
On Wed, Feb 19, 2014 at 03:19:00PM +0100, Krzysztof Kozlowski wrote:

> As I understand the suspend mode (correct me if I'm wrong), the
> regulator core during suspend to mem:
> 1. Calls suspend_set_state().
> 2. rstate->disabled is true so the ops->set_suspend_disable() is called.
> 3. The ops->set_suspend_disable() function (implemented by the driver)
> disables the regulator (e.g. through I2C commands /regmap/).
> 4. During resume the regulator is enabled normal way (ops->enable(), I2C
> again).

No, set_suspend_disable() should *not* disable the regulator, it should
configure what the regulator will do when the hardware enters suspend
mode.  If we were just disabling the regulator there would be no point
in having a special operation, we could just use the normal operation.

> Possible problems:
> A. What happens if some driver using this regulator resumes earlier then
> regulator_suspend_finish()?

Nothing, if the system is not in suspend mode then configuring suspend
mode will have no impact.

> B. What happens if resuming regulator requires some other driver to be
> resumed earlier (e.g. I2C bus)? If regulator resumes before I2C bus then
> calling ops->enable() would fail.

It is expected that as part of exiting suspend mode the hardware will
revert to normal operational mode.
diff mbox

Patch

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index f56ac6f776ae..4a203ef9a605 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -34,6 +34,7 @@ 
 struct s2mps11_info {
 	struct regulator_dev **rdev;
 	unsigned int rdev_num;
+	struct sec_opmode_data *opmode;
 
 	int ramp_delay2;
 	int ramp_delay34;
@@ -43,6 +44,48 @@  struct s2mps11_info {
 	int ramp_delay9;
 };
 
+/* LDO_EN/BUCK_EN register values for enabling/disabling regulator */
+static unsigned int s2mps14_opmode_reg[4] = {
+	[S2MPS14_REGULATOR_OPMODE_OFF]		= 0x0,
+	[S2MPS14_REGULATOR_OPMODE_ON]		= 0x3,
+	[S2MPS14_REGULATOR_OPMODE_RESERVED]	= 0x2,
+	[S2MPS14_REGULATOR_OPMODE_SUSPEND]	= 0x1,
+};
+
+static int s2mps14_get_opmode(struct regulator_dev *rdev)
+{
+	int i, reg_id = rdev_get_id(rdev);
+	int mode = -EINVAL;
+	struct s2mps11_info *s2mps11 = rdev_get_drvdata(rdev);
+
+	for (i = 0; i < s2mps11->rdev_num; i++) {
+		if (s2mps11->opmode[i].id == reg_id) {
+			mode = s2mps11->opmode[i].mode;
+			break;
+		}
+	}
+
+	if (mode == -EINVAL) {
+		dev_warn(rdev_get_dev(rdev),
+				"No op_mode in the driver for regulator %s\n",
+				rdev->desc->name);
+		return mode;
+	}
+
+	return s2mps14_opmode_reg[mode] << S2MPS14_ENCTRL_SHIFT;
+}
+
+static int s2mps14_reg_enable(struct regulator_dev *rdev)
+{
+	int enable_ctrl = s2mps14_get_opmode(rdev);
+
+	if (enable_ctrl < 0)
+		return enable_ctrl;
+
+	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+			S2MPS14_ENCTRL_MASK, enable_ctrl);
+}
+
 static int get_ramp_delay(int ramp_delay)
 {
 	unsigned char cnt = 0;
@@ -405,7 +448,7 @@  static struct regulator_ops s2mps14_reg_ops = {
 	.list_voltage		= regulator_list_voltage_linear,
 	.map_voltage		= regulator_map_voltage_linear,
 	.is_enabled		= regulator_is_enabled_regmap,
-	.enable			= regulator_enable_regmap,
+	.enable			= s2mps14_reg_enable,
 	.disable		= regulator_disable_regmap,
 	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
@@ -520,6 +563,53 @@  static const struct regulator_desc s2mps14_regulators[] __initconst = {
 	regulator_desc_s2mps14_buck1235(5),
 };
 
+static inline void s2mps11_dt_read_opmode(struct platform_device *pdev,
+		struct device_node *np, unsigned int *mode)
+{
+	if (of_property_read_u32(np, "op_mode",	mode)) {
+		dev_warn(&pdev->dev, "no op_mode property property at %s\n",
+				np->full_name);
+		*mode = S2MPS14_REGULATOR_OPMODE_ON;
+	} else if (*mode >= S2MPS14_REGULATOR_OPMODE_MAX ||
+			*mode == S2MPS14_REGULATOR_OPMODE_RESERVED) {
+		dev_warn(&pdev->dev, "wrong op_mode value at %s\n",
+				np->full_name);
+		*mode = S2MPS14_REGULATOR_OPMODE_ON;
+	}
+	/* else: 'mode' was read from DTS and it is valid */
+}
+
+/*
+ * Returns allocated array with opmodes for regulators. The opmodes are read
+ * from DTS.
+ */
+static struct sec_opmode_data *
+s2mps11_pmic_dt_parse_opmode(struct platform_device *pdev,
+		unsigned int rdev_num, struct of_regulator_match *rdata,
+		const struct regulator_desc *regulators)
+{
+	struct sec_opmode_data *rmode;
+	int i;
+
+	rmode = devm_kzalloc(&pdev->dev, sizeof(*rmode)*rdev_num, GFP_KERNEL);
+	if (!rmode) {
+		dev_err(&pdev->dev,
+			"could not allocate memory for regulator mode\n");
+		return NULL;
+	}
+
+	for (i = 0; i < rdev_num; i++) {
+		/*
+		 * The index of rdata and regulators is the same, but this
+		 * may not be equal to ID of regulator.
+		 */
+		rmode[i].id = regulators[i].id;
+		s2mps11_dt_read_opmode(pdev, rdata[i].of_node, &rmode[i].mode);
+	}
+
+	return rmode;
+}
+
 /*
  * Allocates memory under 'regulators' pointer and copies there array
  * of regulator_desc for given device.
@@ -609,9 +699,14 @@  static int s2mps11_pmic_probe(struct platform_device *pdev)
 	}
 
 	of_regulator_match(&pdev->dev, reg_np, rdata, rdev_num);
+	pdata->opmode = s2mps11_pmic_dt_parse_opmode(pdev, rdev_num, rdata,
+				regulators);
+	if (!pdata->opmode)
+		return -ENOMEM;
 
 common_reg:
 	platform_set_drvdata(pdev, s2mps11);
+	s2mps11->opmode = pdata->opmode;
 	s2mps11->rdev_num = rdev_num;
 
 	config.dev = &pdev->dev;
diff --git a/include/linux/mfd/samsung/s2mps14.h b/include/linux/mfd/samsung/s2mps14.h
index ec1e0857ddde..2d36f75a6301 100644
--- a/include/linux/mfd/samsung/s2mps14.h
+++ b/include/linux/mfd/samsung/s2mps14.h
@@ -149,4 +149,23 @@  enum s2mps14_regulators {
 #define S2MPS14_LDO_N_VOLTAGES		(S2MPS14_LDO_VSEL_MASK + 1)
 #define S2MPS14_BUCK_N_VOLTAGES		(S2MPS14_BUCK_VSEL_MASK + 1)
 
+#define S2MPS14_ENCTRL_SHIFT		6
+#define S2MPS14_ENCTRL_MASK		(0x3 << S2MPS14_ENCTRL_SHIFT)
+
+/*
+ * Values of regulator operation modes match device tree bindings.
+ */
+enum s2mps14_regulator_opmode {
+	S2MPS14_REGULATOR_OPMODE_OFF		= 0,
+	S2MPS14_REGULATOR_OPMODE_ON		= 1,
+	/*
+	 * Reserved for compatibility with S5M8767 where this
+	 * is a low power mode.
+	 */
+	S2MPS14_REGULATOR_OPMODE_RESERVED	= 2,
+	S2MPS14_REGULATOR_OPMODE_SUSPEND	= 3,
+
+	S2MPS14_REGULATOR_OPMODE_MAX,
+};
+
 #endif /*  __LINUX_MFD_S2MPS14_H */