Patchwork [1/4] mfd: tps65217: Set PMIC to shutdowm on PWR_EN toggle

login
register
mail settings
Submitter AnilKumar Chimata
Date Nov. 5, 2012, 9:42 a.m.
Message ID <1352108549-9341-2-git-send-email-anilkumar@ti.com>
Download mbox | patch
Permalink /patch/197169/
State New
Headers show

Comments

AnilKumar Chimata - Nov. 5, 2012, 9:42 a.m.
From: Colin Foe-Parker <colin.foeparker@logicpd.com>

Set tps65217 PMIC status to OFF if power enable toggle is
supported. Also adds platform data flag, which should be
passed from board init data.

Signed-off-by: Colin Foe-Parker <colin.foeparker@logicpd.com>
[anilkumar@ti.com: move the additions to tps65217 MFD driver]
Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
 .../devicetree/bindings/regulator/tps65217.txt     |    4 ++++
 drivers/mfd/tps65217.c                             |   12 ++++++++++++
 2 files changed, 16 insertions(+)
Cousson, Benoit - Nov. 5, 2012, 4:59 p.m.
+ Mark

On 11/05/2012 10:42 AM, AnilKumar Ch wrote:
> From: Colin Foe-Parker <colin.foeparker@logicpd.com>
> 
> Set tps65217 PMIC status to OFF if power enable toggle is
> supported. Also adds platform data flag, which should be
> passed from board init data.
> 
> Signed-off-by: Colin Foe-Parker <colin.foeparker@logicpd.com>
> [anilkumar@ti.com: move the additions to tps65217 MFD driver]
> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> ---
>  .../devicetree/bindings/regulator/tps65217.txt     |    4 ++++
>  drivers/mfd/tps65217.c                             |   12 ++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
> index d316fb8..4f05d20 100644
> --- a/Documentation/devicetree/bindings/regulator/tps65217.txt
> +++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
> @@ -11,6 +11,9 @@ Required properties:
>    using the standard binding for regulators found at
>    Documentation/devicetree/bindings/regulator/regulator.txt.
>  
> +Optional properties:
> +- ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.

That sounds like a generic functionality to me. Don't we have some more
generic way to handle that?

If not, that should probably not be a TI only attribute.

It looks like a GPIO like kind of interface at PMIC level.

Regards,
Benoit

> +
>    The valid names for regulators are:
>    tps65217: dcdc1, dcdc2, dcdc3, ldo1, ldo2, ldo3 and ldo4
>  
> @@ -20,6 +23,7 @@ Example:
>  
>  	tps: tps@24 {
>  		compatible = "ti,tps65217";
> +		ti,pmic-shutdown-controller;
>  
>  		regulators {
>  			dcdc1_reg: dcdc1 {
> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index 3fb32e6..c7f17d8 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -160,6 +160,7 @@ static int __devinit tps65217_probe(struct i2c_client *client,
>  	unsigned int version;
>  	unsigned int chip_id = ids->driver_data;
>  	const struct of_device_id *match;
> +	bool status_off = false;
>  	int ret;
>  
>  	if (client->dev.of_node) {
> @@ -170,6 +171,8 @@ static int __devinit tps65217_probe(struct i2c_client *client,
>  			return -EINVAL;
>  		}
>  		chip_id = (unsigned int)match->data;
> +		status_off = of_property_read_bool(client->dev.of_node,
> +					"ti,pmic-shutdown-controller");
>  	}
>  
>  	if (!chip_id) {
> @@ -207,6 +210,15 @@ static int __devinit tps65217_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	/* Set the PMIC to shutdown on PWR_EN toggle */
> +	if (status_off) {
> +		ret = tps65217_set_bits(tps, TPS65217_REG_STATUS,
> +				TPS65217_STATUS_OFF, TPS65217_STATUS_OFF,
> +				TPS65217_PROTECT_NONE);
> +		if (ret)
> +			dev_warn(tps->dev, "unable to set the status OFF\n");
> +	}
> +
>  	dev_info(tps->dev, "TPS65217 ID %#x version 1.%d\n",
>  			(version & TPS65217_CHIPID_CHIP_MASK) >> 4,
>  			version & TPS65217_CHIPID_REV_MASK);
>
AnilKumar Chimata - Nov. 6, 2012, 5:13 a.m.
On Mon, Nov 05, 2012 at 22:29:36, Cousson, Benoit wrote:
> + Mark
> 
> On 11/05/2012 10:42 AM, AnilKumar Ch wrote:
> > From: Colin Foe-Parker <colin.foeparker@logicpd.com>
> > 
> > Set tps65217 PMIC status to OFF if power enable toggle is
> > supported. Also adds platform data flag, which should be
> > passed from board init data.
> > 
> > Signed-off-by: Colin Foe-Parker <colin.foeparker@logicpd.com>
> > [anilkumar@ti.com: move the additions to tps65217 MFD driver]
> > Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> > ---
> >  .../devicetree/bindings/regulator/tps65217.txt     |    4 ++++
> >  drivers/mfd/tps65217.c                             |   12 ++++++++++++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
> > index d316fb8..4f05d20 100644
> > --- a/Documentation/devicetree/bindings/regulator/tps65217.txt
> > +++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
> > @@ -11,6 +11,9 @@ Required properties:
> >    using the standard binding for regulators found at
> >    Documentation/devicetree/bindings/regulator/regulator.txt.
> >  
> > +Optional properties:
> > +- ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
> 
> That sounds like a generic functionality to me. Don't we have some more
> generic way to handle that?

But STATUS_OFF should be set only if Board supports it, otherwise
this change doesn't make sense.

> 
> If not, that should probably not be a TI only attribute.
> 
> It looks like a GPIO like kind of interface at PMIC level.

I agree this should be a generic parameter, but in some regulators this STATUS OFF
control might not be available. This is in my mind while name this parameter.

Thanks
AnilKumar
Mark Brown - Nov. 14, 2012, 2:23 a.m.
On Mon, Nov 05, 2012 at 05:59:36PM +0100, Benoit Cousson wrote:
> On 11/05/2012 10:42 AM, AnilKumar Ch wrote:

> > +Optional properties:
> > +- ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.

> That sounds like a generic functionality to me. Don't we have some more
> generic way to handle that?

> If not, that should probably not be a TI only attribute.

It's pretty unusual to have this configurable as a single thing rather
than as part of flexible power sequencing or something that's just fixed
in silicon.
AnilKumar Chimata - Nov. 14, 2012, 5:10 a.m.
On Wed, Nov 14, 2012 at 07:53:42, Mark Brown wrote:
> On Mon, Nov 05, 2012 at 05:59:36PM +0100, Benoit Cousson wrote:
> > On 11/05/2012 10:42 AM, AnilKumar Ch wrote:
> 
> > > +Optional properties:
> > > +- ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
> 
> > That sounds like a generic functionality to me. Don't we have some more
> > generic way to handle that?
> 
> > If not, that should probably not be a TI only attribute.
> 
> It's pretty unusual to have this configurable as a single thing rather
> than as part of flexible power sequencing or something that's just fixed
> in silicon.
> 

"[PATCH 2/4] rtc: OMAP: Add system pm_power_off to rtc driver" thread
have the details of how PMIC is connected to RTC module of SoC.

As part of the power_off sequence we have
1. To write STATUS_OFF in TPS65217 PMIC. If we do so then PMIC will
go to shutdown if PWR_EN is pulled-down. (This patch doing this)
2. To pull down the PWR_EN signal we have to set PMIC_PWR_EN in RTC
module and trigger ALARM2 event. (This piece of code in 2/4 patch).

Thanks
AnilKumar
AnilKumar Chimata - Nov. 14, 2012, 6:11 a.m.
On Wed, Nov 14, 2012 at 10:40:18, AnilKumar, Chimata wrote:
> On Wed, Nov 14, 2012 at 07:53:42, Mark Brown wrote:
> > On Mon, Nov 05, 2012 at 05:59:36PM +0100, Benoit Cousson wrote:
> > > On 11/05/2012 10:42 AM, AnilKumar Ch wrote:
> > 
> > > > +Optional properties:
> > > > +- ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
> > 
> > > That sounds like a generic functionality to me. Don't we have some more
> > > generic way to handle that?
> > 
> > > If not, that should probably not be a TI only attribute.
> > 
> > It's pretty unusual to have this configurable as a single thing rather
> > than as part of flexible power sequencing or something that's just fixed
> > in silicon.
> >

Mark,

From these two threads we can infer that this is handled in power_off
sequence only. And this is feature of PMIC to go to shutdown mode nothing
to be fixed in silicon. PWR_EN line can be connected to any of these like
PRCM control or GPIO or some other instead of RTC(in this case).

Thanks
AnilKumar
 
> 
> "[PATCH 2/4] rtc: OMAP: Add system pm_power_off to rtc driver" thread
> have the details of how PMIC is connected to RTC module of SoC.
> 
> As part of the power_off sequence we have
> 1. To write STATUS_OFF in TPS65217 PMIC. If we do so then PMIC will
> go to shutdown if PWR_EN is pulled-down. (This patch doing this)
> 2. To pull down the PWR_EN signal we have to set PMIC_PWR_EN in RTC
> module and trigger ALARM2 event. (This piece of code in 2/4 patch).
> 
> Thanks
> AnilKumar
>
Mark Brown - Nov. 14, 2012, 6:21 a.m.
On Wed, Nov 14, 2012 at 06:11:45AM +0000, AnilKumar, Chimata wrote:

> From these two threads we can infer that this is handled in power_off
> sequence only. And this is feature of PMIC to go to shutdown mode nothing
> to be fixed in silicon. PWR_EN line can be connected to any of these like
> PRCM control or GPIO or some other instead of RTC(in this case).

OK, but why are you telling me this?  What are you looking for me to do
with this information?
AnilKumar Chimata - Nov. 14, 2012, 6:49 a.m.
On Wed, Nov 14, 2012 at 11:51:19, Mark Brown wrote:
> On Wed, Nov 14, 2012 at 06:11:45AM +0000, AnilKumar, Chimata wrote:
> 
> > From these two threads we can infer that this is handled in power_off
> > sequence only. And this is feature of PMIC to go to shutdown mode nothing
> > to be fixed in silicon. PWR_EN line can be connected to any of these like
> > PRCM control or GPIO or some other instead of RTC(in this case).
> 
> OK, but why are you telling me this?  What are you looking for me to do
> with this information?
> 

Mark,

Earlier you have a comment on this thread, I am adding my comments
on top of it. Sorry if I am in wrong direction.

Thanks
AnilKumar
Mark Brown - Nov. 14, 2012, 7 a.m.
On Wed, Nov 14, 2012 at 06:49:58AM +0000, AnilKumar, Chimata wrote:

> Earlier you have a comment on this thread, I am adding my comments
> on top of it. Sorry if I am in wrong direction.

Ah, I see.  I was just commenting because Benoit was asking if this
should be supported with a standard framework feature - I'm not
convinced that it should right now as there's not any clear patterns in
hardware behaviour.  I've no specific interest in this system.
Cousson, Benoit - Nov. 14, 2012, 10:08 a.m.
Hi Mark,

On 11/14/2012 08:00 AM, Mark Brown wrote:
> On Wed, Nov 14, 2012 at 06:49:58AM +0000, AnilKumar, Chimata wrote:
> 
>> Earlier you have a comment on this thread, I am adding my comments
>> on top of it. Sorry if I am in wrong direction.
> 
> Ah, I see.  I was just commenting because Benoit was asking if this
> should be supported with a standard framework feature - I'm not
> convinced that it should right now as there's not any clear patterns in
> hardware behaviour.  I've no specific interest in this system.

I was wondering that, because exposing a pin to control the whole PMIC
low power mode seems to be something that should be generic enough to be
handled by the regulator framework.

In the current situation we do have a pwr_en pin that can be controlled
by a GPIO or whatever signal from the SoC.
That's very similar, at PMIC level, to the fixedregulator that allow a
GPIO binding to enable it.

Don't you think that should deserve a support in the fmwk?

Regards,
Benoit
Mark Brown - Nov. 14, 2012, 10:24 a.m.
On Wed, Nov 14, 2012 at 11:08:49AM +0100, Benoit Cousson wrote:

> I was wondering that, because exposing a pin to control the whole PMIC
> low power mode seems to be something that should be generic enough to be
> handled by the regulator framework.

Having something that's controlled by software is really not at all
generic - suspending a PMIC from a GPIO is generally tied in very
closely with the CPU power sequencing which means it's typically some
combination of very hard coded things that we can't control or part of
much wider control of sequencing.

> In the current situation we do have a pwr_en pin that can be controlled
> by a GPIO or whatever signal from the SoC.
> That's very similar, at PMIC level, to the fixedregulator that allow a
> GPIO binding to enable it.

> Don't you think that should deserve a support in the fmwk?

I'm not seeing a coherent description of a feature here - what exactly
are you proposing that we do?  When and how would this GPIO be set for
example?
AnilKumar Chimata - Nov. 16, 2012, 6:16 a.m.
On Wed, Nov 14, 2012 at 15:54:53, Mark Brown wrote:
> On Wed, Nov 14, 2012 at 11:08:49AM +0100, Benoit Cousson wrote:
> 
> > I was wondering that, because exposing a pin to control the whole PMIC
> > low power mode seems to be something that should be generic enough to be
> > handled by the regulator framework.
> 
> Having something that's controlled by software is really not at all
> generic - suspending a PMIC from a GPIO is generally tied in very
> closely with the CPU power sequencing which means it's typically some
> combination of very hard coded things that we can't control or part of
> much wider control of sequencing.
> 
> > In the current situation we do have a pwr_en pin that can be controlled
> > by a GPIO or whatever signal from the SoC.
> > That's very similar, at PMIC level, to the fixedregulator that allow a
> > GPIO binding to enable it.
> 
> > Don't you think that should deserve a support in the fmwk?
> 
> I'm not seeing a coherent description of a feature here - what exactly
> are you proposing that we do?  When and how would this GPIO be set for
> example?

It would be better if these patches are going in like this till the
framework exists. We can change/move this portion once the framework
is defined for this kind.

Thanks
AnilKumar

Patch

diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
index d316fb8..4f05d20 100644
--- a/Documentation/devicetree/bindings/regulator/tps65217.txt
+++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
@@ -11,6 +11,9 @@  Required properties:
   using the standard binding for regulators found at
   Documentation/devicetree/bindings/regulator/regulator.txt.
 
+Optional properties:
+- ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
+
   The valid names for regulators are:
   tps65217: dcdc1, dcdc2, dcdc3, ldo1, ldo2, ldo3 and ldo4
 
@@ -20,6 +23,7 @@  Example:
 
 	tps: tps@24 {
 		compatible = "ti,tps65217";
+		ti,pmic-shutdown-controller;
 
 		regulators {
 			dcdc1_reg: dcdc1 {
diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
index 3fb32e6..c7f17d8 100644
--- a/drivers/mfd/tps65217.c
+++ b/drivers/mfd/tps65217.c
@@ -160,6 +160,7 @@  static int __devinit tps65217_probe(struct i2c_client *client,
 	unsigned int version;
 	unsigned int chip_id = ids->driver_data;
 	const struct of_device_id *match;
+	bool status_off = false;
 	int ret;
 
 	if (client->dev.of_node) {
@@ -170,6 +171,8 @@  static int __devinit tps65217_probe(struct i2c_client *client,
 			return -EINVAL;
 		}
 		chip_id = (unsigned int)match->data;
+		status_off = of_property_read_bool(client->dev.of_node,
+					"ti,pmic-shutdown-controller");
 	}
 
 	if (!chip_id) {
@@ -207,6 +210,15 @@  static int __devinit tps65217_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	/* Set the PMIC to shutdown on PWR_EN toggle */
+	if (status_off) {
+		ret = tps65217_set_bits(tps, TPS65217_REG_STATUS,
+				TPS65217_STATUS_OFF, TPS65217_STATUS_OFF,
+				TPS65217_PROTECT_NONE);
+		if (ret)
+			dev_warn(tps->dev, "unable to set the status OFF\n");
+	}
+
 	dev_info(tps->dev, "TPS65217 ID %#x version 1.%d\n",
 			(version & TPS65217_CHIPID_CHIP_MASK) >> 4,
 			version & TPS65217_CHIPID_REV_MASK);