[PATCH 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[PATCH 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[Bill Huang]

This patch series add new property into regulator DT for telling whether or not
to hook pmic's power off routine to system call "pm_power_off".

Patch 1 add power off support for Tegra20 boards using TPS6586x
Patch 2 add power off support for Tegra30 boards using TPS65910

Verified on Seaboard (Tegra20) and Cardhu (Tegra30)

Bill Huang (2):
  mfd: dt: tps6586x: Add power off control
  mfd: dt: tps65910: add power off control

 Documentation/devicetree/bindings/mfd/tps65910.txt |    4 +++
 .../devicetree/bindings/regulator/tps6586x.txt     |    6 +++++
 drivers/mfd/tps6586x.c                             |   19 +++++++++++++++++
 drivers/mfd/tps65910.c                             |   22 ++++++++++++++++++++
 include/linux/mfd/tps6586x.h                       |    1 +
 include/linux/mfd/tps65910.h                       |    3 ++
 6 files changed, 55 insertions(+), 0 deletions(-)

-- 
1.7.4.1

[PATCH 1/2] mfd: dt: tps6586x: Add power off control

[Bill Huang]

Add DT property "ti,system-power-controller" telling whether or not this
pmic is in charge of controlling the system power, so the power off
routine can be hooked up to system call "pm_power_off".

Based on the work by:
Dan Willemsen <dwillemsen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Signed-off-by: Bill Huang <bilhuang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/regulator/tps6586x.txt     |    6 ++++++
 drivers/mfd/tps6586x.c                             |   19 +++++++++++++++++++
 include/linux/mfd/tps6586x.h                       |    1 +
 3 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/tps6586x.txt b/Documentation/devicetree/bindings/regulator/tps6586x.txt
index d156e1b..03dfa4e 100644
--- a/Documentation/devicetree/bindings/regulator/tps6586x.txt
+++ b/Documentation/devicetree/bindings/regulator/tps6586x.txt
@@ -18,6 +18,10 @@ Required properties:
 - vinldo678-supply: The input supply for the LDO6, LDO7 and LDO8
 - vinldo9-supply: The input supply for the LDO9
 
+Optional properties:
+- ti,system-power-controller: Telling whether or not this pmic is controlling
+  the system power.
+
 Each regulator is defined using the standard binding for regulators.
 
 Example:
@@ -30,6 +34,8 @@ Example:
 		#gpio-cells = <2>;
 		gpio-controller;
 
+		ti,system-power-controller;
+
 		sm0-supply = <&some_reg>;
 		sm1-supply = <&some_reg>;
 		sm2-supply = <&some_reg>;
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 353c348..93d57df 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -29,6 +29,10 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps6586x.h>
 
+#define TPS6586X_SUPPLYENE	0x14
+#define EXITSLREQ_BIT		BIT(1)
+#define SLEEP_MODE_BIT		BIT(3)
+
 /* interrupt control registers */
 #define TPS6586X_INT_ACK1	0xb5
 #define TPS6586X_INT_ACK2	0xb6
@@ -409,6 +413,7 @@ static struct tps6586x_platform_data *tps6586x_parse_dt(struct i2c_client *clien
 	pdata->subdevs = devs;
 	pdata->gpio_base = -1;
 	pdata->irq_base = -1;
+	pdata->pm_off = of_property_read_bool(np, "ti,system-power-controller");
 
 	return pdata;
 }
@@ -441,6 +446,15 @@ static const struct regmap_config tps6586x_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+static struct device *tps6586x_dev;
+static void tps6586x_power_off(void)
+{
+	if (tps6586x_clr_bits(tps6586x_dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT))
+		return;
+
+	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
+}
+
 static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
@@ -505,6 +519,11 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 		goto err_add_devs;
 	}
 
+	tps6586x_dev = &client->dev;
+
+	if (pdata->pm_off && !pm_power_off)
+		pm_power_off = tps6586x_power_off;
+
 	return 0;
 
 err_add_devs:
diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
index f350fd0..08f109f 100644
--- a/include/linux/mfd/tps6586x.h
+++ b/include/linux/mfd/tps6586x.h
@@ -77,6 +77,7 @@ struct tps6586x_platform_data {
 
 	int gpio_base;
 	int irq_base;
+	bool pm_off;
 };
 
 /*
-- 
1.7.4.1

Re: [PATCH 1/2] mfd: dt: tps6586x: Add power off control

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

On Fri, Aug 17, 2012 at 02:16:28AM -0700, Bill Huang wrote:
[...]
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
[...]
> @@ -505,6 +519,11 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
>  		goto err_add_devs;
>  	}
>  
> +	tps6586x_dev = &client->dev;
> +
> +	if (pdata->pm_off && !pm_power_off)
> +		pm_power_off = tps6586x_power_off;
> +

I think the assignment of tps6586x_dev needs to go inside the if block
as well. Otherwise it might be overwritten by another instance for
systems that actually have more than one tps6586x. Since currently the
driver can't be built as a module it probably makes little sense to
clean this up in .remove(), but it might still be worth adding so it
isn't forgotten if and when somebody tries to convert the driver to a
module.

I should note that I don't like very much how the pm_power_off works.
Maybe this should really be changed to allow passing a context for the
function to work from. Something like:

	pm_power_off = tps6586x_power_off;
	pm_power_off_data = &client->dev;

Where pm_power_off() would receive pm_power_off_data as an argument.

Even that's not very pretty. On the other hand this doesn't really buy
us much because only the storage location of the variable would change
and nothing else. But it would still make the association of the data
clearer.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/2] mfd: dt: tps6586x: Add power off control

[Bill Huang]

nvpublic
> On Fri, Aug 17, 2012 at 02:16:28AM -0700, Bill Huang wrote:
> [...]
> > diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
> [...]
> > @@ -505,6 +519,11 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
> >  		goto err_add_devs;
> >  	}
> >
> > +	tps6586x_dev = &client->dev;
> > +
> > +	if (pdata->pm_off && !pm_power_off)
> > +		pm_power_off = tps6586x_power_off;
> > +
> 
> I think the assignment of tps6586x_dev needs to go inside the if block as well. Otherwise it might be
> overwritten by another instance for systems that actually have more than one tps6586x. Since currently
> the driver can't be built as a module it probably makes little sense to clean this up in .remove(),
> but it might still be worth adding so it isn't forgotten if and when somebody tries to convert the
> driver to a module.
> 
Thanks, good point.

> I should note that I don't like very much how the pm_power_off works.
> Maybe this should really be changed to allow passing a context for the function to work from.
> Something like:
> 
> 	pm_power_off = tps6586x_power_off;
> 	pm_power_off_data = &client->dev;
> 
> Where pm_power_off() would receive pm_power_off_data as an argument.
> 
> Even that's not very pretty. On the other hand this doesn't really buy us much because only the
> storage location of the variable would change and nothing else. But it would still make the
> association of the data clearer.
> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1

Re: [PATCH 1/2] mfd: dt: tps6586x: Add power off control

[Stephen Warren]

On 08/17/2012 03:16 AM, Bill Huang wrote:
> Add DT property "ti,system-power-controller" telling whether or not this
> pmic is in charge of controlling the system power, so the power off
> routine can be hooked up to system call "pm_power_off".
> 
> Based on the work by:
> Dan Willemsen <dwillemsen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Signed-off-by: Bill Huang <bilhuang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Tested-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

Note that this conflicts (only context in tps6586x.txt I think) with
commit b93fffb "regulator: tps6586x: add support for SYS rail" in the
regulator tree. Also note that the patch is based on linux-next, which
contains a mis-merge of tps6586x.txt from the regulator tree. In other
words, expect some slight conflicts when you apply this. Taking it
through the regulator tree would eliminate them.

[PATCH 2/2] mfd: dt: tps65910: add power off control

[Bill Huang]

Add DT property "ti,system-power-controller" telling whether or not this
pmic is in charge of controlling the system power, so the power off
routine can be hooked up to system call "pm_power_off".

Based on the work by:
Dan Willemsen <dwillemsen@nvidia.com>

Signed-off-by: Bill Huang <bilhuang@nvidia.com>
---
 Documentation/devicetree/bindings/mfd/tps65910.txt |    4 +++
 drivers/mfd/tps65910.c                             |   22 ++++++++++++++++++++
 include/linux/mfd/tps65910.h                       |    3 ++
 3 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/tps65910.txt b/Documentation/devicetree/bindings/mfd/tps65910.txt
index db03599..2e33048 100644
--- a/Documentation/devicetree/bindings/mfd/tps65910.txt
+++ b/Documentation/devicetree/bindings/mfd/tps65910.txt
@@ -59,6 +59,8 @@ Optional properties:
   in TPS6591X datasheet)
 - ti,en-gpio-sleep: enable sleep control for gpios
   There should be 9 entries here, one for each gpio.
+- ti,system-power-controller: Telling whether or not this pmic is controlling
+  the system power.
 
 Regulator Optional properties:
 - ti,regulator-ext-sleep-control: enable external sleep
@@ -79,6 +81,8 @@ Example:
 		#interrupt-cells = <2>;
 		interrupt-controller;
 
+		ti,system-power-controller;
+
 		ti,vmbch-threshold = 0;
 		ti,vmbch2-threshold = 0;
 		ti,en-ck32k-xtal;
diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 1c56379..a7925a9 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -198,6 +198,8 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
 
 	board_info->irq = client->irq;
 	board_info->irq_base = -1;
+	board_info->pm_off = of_property_read_bool(np,
+			"ti,system-power-controller");
 
 	return board_info;
 }
@@ -210,6 +212,21 @@ struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
 }
 #endif
 
+static struct i2c_client *tps65910_i2c_client;
+static void tps65910_power_off(void)
+{
+	struct tps65910 *tps65910;
+
+	tps65910 = dev_get_drvdata(&tps65910_i2c_client->dev);
+
+	if (tps65910_reg_set_bits(tps65910, TPS65910_DEVCTRL,
+			DEVCTRL_PWR_OFF_MASK) < 0)
+		return;
+
+	tps65910_reg_clear_bits(tps65910, TPS65910_DEVCTRL,
+			DEVCTRL_DEV_ON_MASK);
+}
+
 static __devinit int tps65910_i2c_probe(struct i2c_client *i2c,
 					const struct i2c_device_id *id)
 {
@@ -267,6 +284,11 @@ static __devinit int tps65910_i2c_probe(struct i2c_client *i2c,
 	tps65910_ck32k_init(tps65910, pmic_plat_data);
 	tps65910_sleepinit(tps65910, pmic_plat_data);
 
+	tps65910_i2c_client = i2c;
+
+	if (pmic_plat_data->pm_off && !pm_power_off)
+		pm_power_off = tps65910_power_off;
+
 	return ret;
 }
 
diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index 9bf8767..ac772b3 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -366,6 +366,8 @@
 
 
 /*Register DEVCTRL  (0x80) register.RegisterDescription */
+#define DEVCTRL_PWR_OFF_MASK				0x80
+#define DEVCTRL_PWR_OFF_SHIFT				7
 #define DEVCTRL_RTC_PWDN_MASK				0x40
 #define DEVCTRL_RTC_PWDN_SHIFT				6
 #define DEVCTRL_CK32K_CTRL_MASK				0x20
@@ -809,6 +811,7 @@ struct tps65910_board {
 	int vmbch2_threshold;
 	bool en_ck32k_xtal;
 	bool en_dev_slp;
+	bool pm_off;
 	struct tps65910_sleep_keepon_data *slp_keepon;
 	bool en_gpio_sleep[TPS6591X_MAX_NUM_GPIO];
 	unsigned long regulator_ext_sleep_control[TPS65910_NUM_REGS];
-- 
1.7.4.1

Re: [PATCH 2/2] mfd: dt: tps65910: add power off control

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On Fri, Aug 17, 2012 at 02:16:29AM -0700, Bill Huang wrote:
> Add DT property "ti,system-power-controller" telling whether or not this
> pmic is in charge of controlling the system power, so the power off
> routine can be hooked up to system call "pm_power_off".
> 
> Based on the work by:
> Dan Willemsen <dwillemsen@nvidia.com>
> 
> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> ---
>  Documentation/devicetree/bindings/mfd/tps65910.txt |    4 +++
>  drivers/mfd/tps65910.c                             |   22 ++++++++++++++++++++
>  include/linux/mfd/tps65910.h                       |    3 ++
>  3 files changed, 29 insertions(+), 0 deletions(-)

The same comments as for the tps6586x driver apply here as well.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] mfd: dt: tps65910: add power off control

[Stephen Warren]

On 08/17/2012 03:16 AM, Bill Huang wrote:
> Add DT property "ti,system-power-controller" telling whether or not this
> pmic is in charge of controlling the system power, so the power off
> routine can be hooked up to system call "pm_power_off".
> 
> Based on the work by:
> Dan Willemsen <dwillemsen@nvidia.com>
> 
> Signed-off-by: Bill Huang <bilhuang@nvidia.com>

Tested-by: Stephen Warren <swarren@wwwdotorg.org>

AFAIK, this one doesn't have any conflicts.