| Message ID | 1352108549-9341-2-git-send-email-anilkumar@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
+ 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); >
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
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.
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
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 >
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?
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
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.
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
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?
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
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);