[PATCH 1/1] mfd: palmas: Add power off control

[PATCH 1/1] mfd: palmas: Add power off control

[Bill Huang]

Hook up "pm_power_off" to palmas power off routine if there is DT
property "ti,system-power-controller" defined, so platform which is
powered by this regulator can be powered off properly.

Based on work by:
Mallikarjun Kasoju <mkasoju@nvidia.com>

Signed-off-by: Bill Huang <bilhuang@nvidia.com>
---
 drivers/mfd/palmas.c       |   25 +++++++++++++++++++++++--
 include/linux/mfd/palmas.h |    1 +
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
index bbdbc50..ee8180d 100644
--- a/drivers/mfd/palmas.c
+++ b/drivers/mfd/palmas.c
@@ -283,6 +283,22 @@ static void palmas_dt_to_pdata(struct device_node *node,
 		pdata->power_ctrl = PALMAS_POWER_CTRL_NSLEEP_MASK |
 					PALMAS_POWER_CTRL_ENABLE1_MASK |
 					PALMAS_POWER_CTRL_ENABLE2_MASK;
+
+	pdata->pm_off = of_property_read_bool(node,
+			"ti,system-power-controller");
+}
+
+static struct palmas *palmas_dev;
+static void palmas_power_off(void)
+{
+	unsigned int addr;
+
+	if (!palmas_dev)
+		return;
+
+	addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_DEV_CTRL);
+
+	regmap_update_bits(palmas_dev->regmap[0], addr, 1, 0);
 }
 
 static int palmas_i2c_probe(struct i2c_client *i2c,
@@ -435,10 +451,15 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
 	 */
 	if (node) {
 		ret = of_platform_populate(node, NULL, NULL, &i2c->dev);
-		if (ret < 0)
+		if (ret < 0) {
 			goto err_irq;
-		else
+		} else {
+			if (pdata->pm_off && !pm_power_off) {
+				palmas_dev = palmas;
+				pm_power_off = palmas_power_off;
+			}
 			return ret;
+		}
 	}
 
 	children = kmemdup(palmas_children, sizeof(palmas_children),
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index a4d13d7..a447e54 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -232,6 +232,7 @@ struct palmas_platform_data {
 	 */
 	int mux_from_pdata;
 	u8 pad1, pad2;
+	bool pm_off;
 
 	struct palmas_pmic_platform_data *pmic_pdata;
 	struct palmas_gpadc_platform_data *gpadc_pdata;
-- 
1.7.9.5

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Stephen Warren]

On 03/14/2013 04:58 AM, Bill Huang wrote:
> Hook up "pm_power_off" to palmas power off routine if there is DT
> property "ti,system-power-controller" defined, so platform which is
> powered by this regulator can be powered off properly.

> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c

> +	pdata->pm_off = of_property_read_bool(node,
> +			"ti,system-power-controller");

You would need to add that property to the DT binding documentation for
this device.

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Bill Huang]

On Fri, 2013-03-15 at 13:19 +0800, Stephen Warren wrote:
> On 03/14/2013 04:58 AM, Bill Huang wrote:
> > Hook up "pm_power_off" to palmas power off routine if there is DT
> > property "ti,system-power-controller" defined, so platform which is
> > powered by this regulator can be powered off properly.
> 
> > diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> 
> > +	pdata->pm_off = of_property_read_bool(node,
> > +			"ti,system-power-controller");
> 
> You would need to add that property to the DT binding documentation for
> this device.
Does it work that some time later Laxmain helps to add it when he
submits his pmic bindings for Palmas?

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Stephen Warren]

On 03/14/2013 11:51 PM, Bill Huang wrote:
> On Fri, 2013-03-15 at 13:19 +0800, Stephen Warren wrote:
>> On 03/14/2013 04:58 AM, Bill Huang wrote:
>>> Hook up "pm_power_off" to palmas power off routine if there is DT
>>> property "ti,system-power-controller" defined, so platform which is
>>> powered by this regulator can be powered off properly.
>>
>>> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
>>
>>> +	pdata->pm_off = of_property_read_bool(node,
>>> +			"ti,system-power-controller");
>>
>> You would need to add that property to the DT binding documentation for
>> this device.
>
> Does it work that some time later Laxmain helps to add it when he
> submits his pmic bindings for Palmas?

I'll note that Ian Lartey and Graeme Gregory actually seem to be the
people defining the PMIC bindings for Palmas. See:

http://comments.gmane.org/gmane.linux.documentation/9948

Or is Laxman planning to send some updates to that?

But irrespective of that no, I don't think that influences this at all;
if you introduce driver support for new DT content, then either that DT
content should already be documented in the binding, or this patch
should add it to the documentation.

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Bill Huang]

On Sat, 2013-03-16 at 01:22 +0800, Stephen Warren wrote:
> On 03/14/2013 11:51 PM, Bill Huang wrote:
> > On Fri, 2013-03-15 at 13:19 +0800, Stephen Warren wrote:
> >> On 03/14/2013 04:58 AM, Bill Huang wrote:
> >>> Hook up "pm_power_off" to palmas power off routine if there is DT
> >>> property "ti,system-power-controller" defined, so platform which is
> >>> powered by this regulator can be powered off properly.
> >>
> >>> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> >>
> >>> +	pdata->pm_off = of_property_read_bool(node,
> >>> +			"ti,system-power-controller");
> >>
> >> You would need to add that property to the DT binding documentation for
> >> this device.
> >
> > Does it work that some time later Laxmain helps to add it when he
> > submits his pmic bindings for Palmas?
> 
> I'll note that Ian Lartey and Graeme Gregory actually seem to be the
> people defining the PMIC bindings for Palmas. See:
> 
> http://comments.gmane.org/gmane.linux.documentation/9948
> 
> Or is Laxman planning to send some updates to that?
> 
> But irrespective of that no, I don't think that influences this at all;
> if you introduce driver support for new DT content, then either that DT
> content should already be documented in the binding, or this patch
> should add it to the documentation.

Thanks, here I just need to add a property into the DT bindings but
currently there is no any Palmas binding document exist so that means I
have to hold off the patch.

[PATCH 1/1] mfd: palmas: Add power off control

[Bill Huang]

Hook up "pm_power_off" to palmas power off routine if there is DT
property "ti,system-power-controller" defined, so platform which is
powered by this regulator can be powered off properly.

Based on work by:
Mallikarjun Kasoju <mkasoju@nvidia.com>

Signed-off-by: Bill Huang <bilhuang@nvidia.com>
cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
---
 .../devicetree/bindings/regulator/palmas-pmic.txt  |    5 +++++
 drivers/mfd/palmas.c                               |   23 ++++++++++++++++++--
 include/linux/mfd/palmas.h                         |    1 +
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
index 30b0581..4adca2a 100644
--- a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
+++ b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
@@ -36,6 +36,9 @@ Optional nodes:
 	       ti,smps-range - OTP has the wrong range set for the hardware so override
 	       0 - low range, 1 - high range.
 
+- ti,system-power-controller: Telling whether or not this pmic is controlling
+			      the system power.
+
 Example:
 
 #include <dt-bindings/interrupt-controller/irq.h>
@@ -48,6 +51,8 @@ pmic {
 
 	ti,ldo6-vibrator;
 
+	ti,system-power-controller;
+
 	regulators {
 		smps12_reg : smps12 {
 			regulator-name = "smps12";
diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
index e4d1c70..0662b21 100644
--- a/drivers/mfd/palmas.c
+++ b/drivers/mfd/palmas.c
@@ -229,6 +229,22 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
 					PALMAS_POWER_CTRL_ENABLE2_MASK;
 	if (i2c->irq)
 		palmas_set_pdata_irq_flag(i2c, pdata);
+
+	pdata->pm_off = of_property_read_bool(node,
+			"ti,system-power-controller");
+}
+
+static struct palmas *palmas_dev;
+static void palmas_power_off(void)
+{
+	unsigned int addr;
+
+	if (!palmas_dev)
+		return;
+
+	addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_DEV_CTRL);
+
+	regmap_update_bits(palmas_dev->regmap[0], addr, 1, 0);
 }
 
 static unsigned int palmas_features = PALMAS_PMIC_FEATURE_SMPS10_BOOST;
@@ -423,10 +439,13 @@ no_irq:
 	 */
 	if (node) {
 		ret = of_platform_populate(node, NULL, NULL, &i2c->dev);
-		if (ret < 0)
+		if (ret < 0) {
 			goto err_irq;
-		else
+		} else if (pdata->pm_off && !pm_power_off) {
+			palmas_dev = palmas;
+			pm_power_off = palmas_power_off;
 			return ret;
+		}
 	}
 
 	return ret;
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index 1a8dd7a..061cce0 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -258,6 +258,7 @@ struct palmas_platform_data {
 	 */
 	int mux_from_pdata;
 	u8 pad1, pad2;
+	bool pm_off;
 
 	struct palmas_pmic_platform_data *pmic_pdata;
 	struct palmas_gpadc_platform_data *gpadc_pdata;
-- 
1.7.9.5

[PATCH 1/1] mfd: palmas: Add power off control

[Bill Huang]

Hook up "pm_power_off" to palmas power off routine if there is DT
property "ti,system-power-controller" defined, so platform which is
powered by this regulator can be powered off properly.

Based on work by:
Mallikarjun Kasoju <mkasoju@nvidia.com>

Signed-off-by: Bill Huang <bilhuang@nvidia.com>
cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
---
 .../devicetree/bindings/regulator/palmas-pmic.txt  |    5 +++++
 drivers/mfd/palmas.c                               |   23 ++++++++++++++++++--
 include/linux/mfd/palmas.h                         |    1 +
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
index 30b0581..4adca2a 100644
--- a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
+++ b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
@@ -36,6 +36,9 @@ Optional nodes:
 	       ti,smps-range - OTP has the wrong range set for the hardware so override
 	       0 - low range, 1 - high range.
 
+- ti,system-power-controller: Telling whether or not this pmic is controlling
+			      the system power.
+
 Example:
 
 #include <dt-bindings/interrupt-controller/irq.h>
@@ -48,6 +51,8 @@ pmic {
 
 	ti,ldo6-vibrator;
 
+	ti,system-power-controller;
+
 	regulators {
 		smps12_reg : smps12 {
 			regulator-name = "smps12";
diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
index e4d1c70..0662b21 100644
--- a/drivers/mfd/palmas.c
+++ b/drivers/mfd/palmas.c
@@ -229,6 +229,22 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
 					PALMAS_POWER_CTRL_ENABLE2_MASK;
 	if (i2c->irq)
 		palmas_set_pdata_irq_flag(i2c, pdata);
+
+	pdata->pm_off = of_property_read_bool(node,
+			"ti,system-power-controller");
+}
+
+static struct palmas *palmas_dev;
+static void palmas_power_off(void)
+{
+	unsigned int addr;
+
+	if (!palmas_dev)
+		return;
+
+	addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_DEV_CTRL);
+
+	regmap_update_bits(palmas_dev->regmap[0], addr, 1, 0);
 }
 
 static unsigned int palmas_features = PALMAS_PMIC_FEATURE_SMPS10_BOOST;
@@ -423,10 +439,13 @@ no_irq:
 	 */
 	if (node) {
 		ret = of_platform_populate(node, NULL, NULL, &i2c->dev);
-		if (ret < 0)
+		if (ret < 0) {
 			goto err_irq;
-		else
+		} else if (pdata->pm_off && !pm_power_off) {
+			palmas_dev = palmas;
+			pm_power_off = palmas_power_off;
 			return ret;
+		}
 	}
 
 	return ret;
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index 1a8dd7a..061cce0 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -258,6 +258,7 @@ struct palmas_platform_data {
 	 */
 	int mux_from_pdata;
 	u8 pad1, pad2;
+	bool pm_off;
 
 	struct palmas_pmic_platform_data *pmic_pdata;
 	struct palmas_gpadc_platform_data *gpadc_pdata;
-- 
1.7.9.5

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Nishanth Menon]

On 07/30/2013 07:05 AM, Bill Huang wrote:
> Hook up "pm_power_off" to palmas power off routine if there is DT
> property "ti,system-power-controller" defined, so platform which is
> powered by this regulator can be powered off properly.
>
> Based on work by:
> Mallikarjun Kasoju <mkasoju@nvidia.com>
>
> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
> ---
>   .../devicetree/bindings/regulator/palmas-pmic.txt  |    5 +++++
>   drivers/mfd/palmas.c                               |   23 ++++++++++++++++++--
>   include/linux/mfd/palmas.h                         |    1 +
any reason why it wont fit in drivers/power/reset/ is'nt it the right 
place to add this?
>   3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> index 30b0581..4adca2a 100644
> --- a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> +++ b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> @@ -36,6 +36,9 @@ Optional nodes:
>   	       ti,smps-range - OTP has the wrong range set for the hardware so override
>   	       0 - low range, 1 - high range.
>
> +- ti,system-power-controller: Telling whether or not this pmic is controlling
controller or master?
> +			      the system power.
> +
>   Example:
>
>   #include <dt-bindings/interrupt-controller/irq.h>
> @@ -48,6 +51,8 @@ pmic {
>
>   	ti,ldo6-vibrator;
>
> +	ti,system-power-controller;
> +
>   	regulators {
>   		smps12_reg : smps12 {
>   			regulator-name = "smps12";
> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> index e4d1c70..0662b21 100644
> --- a/drivers/mfd/palmas.c
> +++ b/drivers/mfd/palmas.c
> @@ -229,6 +229,22 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
>   					PALMAS_POWER_CTRL_ENABLE2_MASK;
>   	if (i2c->irq)
>   		palmas_set_pdata_irq_flag(i2c, pdata);
> +
> +	pdata->pm_off = of_property_read_bool(node,
> +			"ti,system-power-controller");
> +}
> +
> +static struct palmas *palmas_dev;
> +static void palmas_power_off(void)
> +{
> +	unsigned int addr;
> +
> +	if (!palmas_dev)
> +		return;
> +
> +	addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_DEV_CTRL);
> +
> +	regmap_update_bits(palmas_dev->regmap[0], addr, 1, 0);
slave = PALMAS_BASE_TO_SLAVE(base);
addr = PALMAS_BASE_TO_REG(base, reg);

r = regmap_update_bits(palmas->regmap[slave], addr, mask, val);
instead?

just for reference, an old implementation I had done is available at [1]


[1] 
http://git.omapzoom.org/?p=kernel/omap.git;a=blob;f=drivers/mfd/palmas-poweroff.c;hb=p-linux-omap-3.4

-- 
Regards,
Nishanth Menon

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Nishanth Menon]

On 07/30/2013 07:05 AM, Bill Huang wrote:
> Hook up "pm_power_off" to palmas power off routine if there is DT
> property "ti,system-power-controller" defined, so platform which is
> powered by this regulator can be powered off properly.
>
> Based on work by:
> Mallikarjun Kasoju <mkasoju@nvidia.com>
>
> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
> ---
>   .../devicetree/bindings/regulator/palmas-pmic.txt  |    5 +++++
>   drivers/mfd/palmas.c                               |   23 ++++++++++++++++++--
>   include/linux/mfd/palmas.h                         |    1 +
any reason why it wont fit in drivers/power/reset/ is'nt it the right 
place to add this?
>   3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> index 30b0581..4adca2a 100644
> --- a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> +++ b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> @@ -36,6 +36,9 @@ Optional nodes:
>   	       ti,smps-range - OTP has the wrong range set for the hardware so override
>   	       0 - low range, 1 - high range.
>
> +- ti,system-power-controller: Telling whether or not this pmic is controlling
controller or master?
> +			      the system power.
> +
>   Example:
>
>   #include <dt-bindings/interrupt-controller/irq.h>
> @@ -48,6 +51,8 @@ pmic {
>
>   	ti,ldo6-vibrator;
>
> +	ti,system-power-controller;
> +
>   	regulators {
>   		smps12_reg : smps12 {
>   			regulator-name = "smps12";
> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> index e4d1c70..0662b21 100644
> --- a/drivers/mfd/palmas.c
> +++ b/drivers/mfd/palmas.c
> @@ -229,6 +229,22 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
>   					PALMAS_POWER_CTRL_ENABLE2_MASK;
>   	if (i2c->irq)
>   		palmas_set_pdata_irq_flag(i2c, pdata);
> +
> +	pdata->pm_off = of_property_read_bool(node,
> +			"ti,system-power-controller");
> +}
> +
> +static struct palmas *palmas_dev;
> +static void palmas_power_off(void)
> +{
> +	unsigned int addr;
> +
> +	if (!palmas_dev)
> +		return;
> +
> +	addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_DEV_CTRL);
> +
> +	regmap_update_bits(palmas_dev->regmap[0], addr, 1, 0);
slave = PALMAS_BASE_TO_SLAVE(base);
addr = PALMAS_BASE_TO_REG(base, reg);

r = regmap_update_bits(palmas->regmap[slave], addr, mask, val);
instead?

just for reference, an old implementation I had done is available at [1]


[1] 
http://git.omapzoom.org/?p=kernel/omap.git;a=blob;f=drivers/mfd/palmas-poweroff.c;hb=p-linux-omap-3.4

-- 
Regards,
Nishanth Menon

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Bill Huang]

On Tue, 2013-07-30 at 21:17 +0800, Nishanth Menon wrote:
> On 07/30/2013 07:05 AM, Bill Huang wrote:
> > Hook up "pm_power_off" to palmas power off routine if there is DT
> > property "ti,system-power-controller" defined, so platform which is
> > powered by this regulator can be powered off properly.
> >
> > Based on work by:
> > Mallikarjun Kasoju <mkasoju@nvidia.com>
> >
> > Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> > cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
> > ---
> >   .../devicetree/bindings/regulator/palmas-pmic.txt  |    5 +++++
> >   drivers/mfd/palmas.c                               |   23 ++++++++++++++++++--
> >   include/linux/mfd/palmas.h                         |    1 +
> any reason why it wont fit in drivers/power/reset/ is'nt it the right 
> place to add this?
> >   3 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> > index 30b0581..4adca2a 100644
> > --- a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> > +++ b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> > @@ -36,6 +36,9 @@ Optional nodes:
> >   	       ti,smps-range - OTP has the wrong range set for the hardware so override
> >   	       0 - low range, 1 - high range.
> >
> > +- ti,system-power-controller: Telling whether or not this pmic is controlling
> controller or master?

We name it controller since it controls the system power of the
platform, I'm not sure will it make more sense to be master? Or does it
makes sense to other pmic which also control the system power (the same
property are already used in pmic TPS6586x and TPS65910).

> > +			      the system power.
> > +
> >   Example:
> >
> >   #include <dt-bindings/interrupt-controller/irq.h>
> > @@ -48,6 +51,8 @@ pmic {
> >
> >   	ti,ldo6-vibrator;
> >
> > +	ti,system-power-controller;
> > +
> >   	regulators {
> >   		smps12_reg : smps12 {
> >   			regulator-name = "smps12";
> > diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> > index e4d1c70..0662b21 100644
> > --- a/drivers/mfd/palmas.c
> > +++ b/drivers/mfd/palmas.c
> > @@ -229,6 +229,22 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
> >   					PALMAS_POWER_CTRL_ENABLE2_MASK;
> >   	if (i2c->irq)
> >   		palmas_set_pdata_irq_flag(i2c, pdata);
> > +
> > +	pdata->pm_off = of_property_read_bool(node,
> > +			"ti,system-power-controller");
> > +}
> > +
> > +static struct palmas *palmas_dev;
> > +static void palmas_power_off(void)
> > +{
> > +	unsigned int addr;
> > +
> > +	if (!palmas_dev)
> > +		return;
> > +
> > +	addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_DEV_CTRL);
> > +
> > +	regmap_update_bits(palmas_dev->regmap[0], addr, 1, 0);
> slave = PALMAS_BASE_TO_SLAVE(base);
> addr = PALMAS_BASE_TO_REG(base, reg);
> 
> r = regmap_update_bits(palmas->regmap[slave], addr, mask, val);
> instead?
> 
> just for reference, an old implementation I had done is available at [1]
> 
Thanks, I'll send out v2.
> 
> [1] 
> http://git.omapzoom.org/?p=kernel/omap.git;a=blob;f=drivers/mfd/palmas-poweroff.c;hb=p-linux-omap-3.4
> 

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Nishanth Menon]

On 07/31/2013 01:22 AM, Bill Huang wrote:
> On Tue, 2013-07-30 at 21:17 +0800, Nishanth Menon wrote:
>> On 07/30/2013 07:05 AM, Bill Huang wrote:
[...]

>>> diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
>>> index 30b0581..4adca2a 100644
>>> --- a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
>>> +++ b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
>>> @@ -36,6 +36,9 @@ Optional nodes:
>>>    	       ti,smps-range - OTP has the wrong range set for the hardware so override
>>>    	       0 - low range, 1 - high range.
>>>
>>> +- ti,system-power-controller: Telling whether or not this pmic is controlling
>> controller or master?
>
> We name it controller since it controls the system power of the
> platform, I'm not sure will it make more sense to be master? Or does it
> makes sense to other pmic which also control the system power (the same
> property are already used in pmic TPS6586x and TPS65910).


No strong feelings either way, especially if there is precedence.

-- 
Regards,
Nishanth Menon

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Stephen Warren]

On 07/31/2013 12:22 AM, Bill Huang wrote:
> On Tue, 2013-07-30 at 21:17 +0800, Nishanth Menon wrote:
>> On 07/30/2013 07:05 AM, Bill Huang wrote:
>>> Hook up "pm_power_off" to palmas power off routine if there is DT
>>> property "ti,system-power-controller" defined, so platform which is
>>> powered by this regulator can be powered off properly.
>>>
>>> Based on work by:
>>> Mallikarjun Kasoju <mkasoju@nvidia.com>
>>>
>>> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
>>> cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
>>> ---
>>>   .../devicetree/bindings/regulator/palmas-pmic.txt  |    5 +++++
>>>   drivers/mfd/palmas.c                               |   23 ++++++++++++++++++--
>>>   include/linux/mfd/palmas.h                         |    1 +
>> any reason why it wont fit in drivers/power/reset/ is'nt it the right 
>> place to add this?
>>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
>>> index 30b0581..4adca2a 100644
>>> --- a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
>>> +++ b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
>>> @@ -36,6 +36,9 @@ Optional nodes:
>>>   	       ti,smps-range - OTP has the wrong range set for the hardware so override
>>>   	       0 - low range, 1 - high range.
>>>
>>> +- ti,system-power-controller: Telling whether or not this pmic is controlling
>> controller or master?
> 
> We name it controller since it controls the system power of the
> platform, I'm not sure will it make more sense to be master? Or does it
> makes sense to other pmic which also control the system power (the same
> property are already used in pmic TPS6586x and TPS65910).

The property name quoted above is probably the best choice since it's
consistent with other properties that do the exact same thing in other
bindings.

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Lee Jones]

On Tue, 30 Jul 2013, Bill Huang wrote:

> Hook up "pm_power_off" to palmas power off routine if there is DT
> property "ti,system-power-controller" defined, so platform which is
> powered by this regulator can be powered off properly.
> 
> Based on work by:
> Mallikarjun Kasoju <mkasoju@nvidia.com>
> 
> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> cc: Mallikarjun Kasoju <mkasoju@nvidia.com>

Please put the 'Cc:' (not 'cc:') above the SoBs, then drop the "Based
on work by:" and replace with:

Signed-off-by: Mallikarjun Kasoju <mkasoju@nvidia.com>
Signed-off-by: Bill Huang <bilhuang@nvidia.com>

This insinuates that the original patch was crated by Mallikarjun.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Bill Huang]

On Wed, 2013-07-31 at 17:56 +0800, Lee Jones wrote:
> On Tue, 30 Jul 2013, Bill Huang wrote:
> 
> > Hook up "pm_power_off" to palmas power off routine if there is DT
> > property "ti,system-power-controller" defined, so platform which is
> > powered by this regulator can be powered off properly.
> > 
> > Based on work by:
> > Mallikarjun Kasoju <mkasoju@nvidia.com>
> > 
> > Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> > cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
> 
> Please put the 'Cc:' (not 'cc:') above the SoBs, then drop the "Based
> on work by:" and replace with:
> 
> Signed-off-by: Mallikarjun Kasoju <mkasoju@nvidia.com>
> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> 
> This insinuates that the original patch was crated by Mallikarjun.
> 
Thanks, will fix in next version.

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Stephen Warren]

On 07/31/2013 03:56 AM, Lee Jones wrote:
> On Tue, 30 Jul 2013, Bill Huang wrote:
> 
>> Hook up "pm_power_off" to palmas power off routine if there is DT
>> property "ti,system-power-controller" defined, so platform which is
>> powered by this regulator can be powered off properly.
>>
>> Based on work by:
>> Mallikarjun Kasoju <mkasoju@nvidia.com>
>>
>> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
>> cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
> 
> Please put the 'Cc:' (not 'cc:') above the SoBs, then drop the "Based
> on work by:" and replace with:
> 
> Signed-off-by: Mallikarjun Kasoju <mkasoju@nvidia.com>
> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> 
> This insinuates that the original patch was crated by Mallikarjun.

That advice may not be correct. Did Mallikarjun actually create *this*
patch? More likely, this patch was based on an equivalent change to some
other PMIC, and Bill just applied the same technique to this other
driver. If Mallikarjun really did write this patch, then the git author
field should also be set to Mallikarjun not Bill.

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Lee Jones]

On Wed, 31 Jul 2013, Stephen Warren wrote:

> On 07/31/2013 03:56 AM, Lee Jones wrote:
> > On Tue, 30 Jul 2013, Bill Huang wrote:
> > 
> >> Hook up "pm_power_off" to palmas power off routine if there is DT
> >> property "ti,system-power-controller" defined, so platform which is
> >> powered by this regulator can be powered off properly.
> >>
> >> Based on work by:
> >> Mallikarjun Kasoju <mkasoju@nvidia.com>
> >>
> >> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> >> cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
> > 
> > Please put the 'Cc:' (not 'cc:') above the SoBs, then drop the "Based
> > on work by:" and replace with:
> > 
> > Signed-off-by: Mallikarjun Kasoju <mkasoju@nvidia.com>
> > Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> > 
> > This insinuates that the original patch was crated by Mallikarjun.
> 
> That advice may not be correct. Did Mallikarjun actually create *this*
> patch? More likely, this patch was based on an equivalent change to some
> other PMIC, and Bill just applied the same technique to this other
> driver.

Yes, I agree with this, and I'm sure there is a place for "Based on
work by:" or "Originally authored by:" tags, but in general, I think
the SoBs can paint a pretty good picture.

For example, if this patch is simply using techniques which already
exist in other drivers, I would personally not mention it in the
commit message. A massive percentage of kernel code has been
influenced by already existing implementations. Not much truly new and
unique kernel code enters the kernel these days.

> If Mallikarjun really did write this patch, then the git author
> field should also be set to Mallikarjun not Bill.

That's not how I'm lead to believe it works. I am under the impression
that if you take an already existing patch and upstream it with little
changes, then you keep the original creator's authorship and apply
your SoB before sending. Whereas if you have make considerable (down
to perception) changes to the patch, then you may adopt authorship. To
credit the efforts of the original author in this case I would advise
to keep the first SoB. Providing they agree with the changes of course.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/1] mfd: palmas: Add power off control

[Stephen Warren]

On 08/01/2013 02:47 AM, Lee Jones wrote:
> On Wed, 31 Jul 2013, Stephen Warren wrote:
> 
>> On 07/31/2013 03:56 AM, Lee Jones wrote:
>>> On Tue, 30 Jul 2013, Bill Huang wrote:
>>>
>>>> Hook up "pm_power_off" to palmas power off routine if there is DT
>>>> property "ti,system-power-controller" defined, so platform which is
>>>> powered by this regulator can be powered off properly.
>>>>
>>>> Based on work by:
>>>> Mallikarjun Kasoju <mkasoju@nvidia.com>
>>>>
>>>> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
>>>> cc: Mallikarjun Kasoju <mkasoju@nvidia.com>
>>>
>>> Please put the 'Cc:' (not 'cc:') above the SoBs, then drop the "Based
>>> on work by:" and replace with:
>>>
>>> Signed-off-by: Mallikarjun Kasoju <mkasoju@nvidia.com>
>>> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
>>>
>>> This insinuates that the original patch was crated by Mallikarjun.
>>
>> That advice may not be correct. Did Mallikarjun actually create *this*
>> patch? More likely, this patch was based on an equivalent change to some
>> other PMIC, and Bill just applied the same technique to this other
>> driver.
> 
> Yes, I agree with this, and I'm sure there is a place for "Based on
> work by:" or "Originally authored by:" tags, but in general, I think
> the SoBs can paint a pretty good picture.
> 
> For example, if this patch is simply using techniques which already
> exist in other drivers, I would personally not mention it in the
> commit message. A massive percentage of kernel code has been
> influenced by already existing implementations. Not much truly new and
> unique kernel code enters the kernel these days.
> 
>> If Mallikarjun really did write this patch, then the git author
>> field should also be set to Mallikarjun not Bill.
> 
> That's not how I'm lead to believe it works. I am under the impression
> that if you take an already existing patch and upstream it with little
> changes, then you keep the original creator's authorship and apply
> your SoB before sending.

Yes.

> Whereas if you have make considerable (down
> to perception) changes to the patch, then you may adopt authorship. To
> credit the efforts of the original author in this case I would advise
> to keep the first SoB. Providing they agree with the changes of course.

I'm not sure about this. I've certainly made significant changes then
still been asked to maintain the original authorship. There is plenty of
opportunity to list the changes you made in the free-form commit
description, or for smaller changes, to inline the list into the final
tag paragraph.

Either way, this situation is quite unlikely to apply to this simple patch.

But anyway, Bill knows the exact history of the patch and can presumably
choose the correct approach.