linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/1] mfd: palmas: Add power off control
@ 2013-07-31  7:17 Bill Huang
  2013-07-31 11:57 ` Nishanth Menon
  0 siblings, 1 reply; 9+ messages in thread
From: Bill Huang @ 2013-07-31  7:17 UTC (permalink / raw)
  To: sameo
  Cc: rob.herring, pawel.moll, mark.rutland, swarren, ian.campbell,
	rob, lee.jones, broonie, nm, j-keerthy, grant.likely, ian,
	devicetree, linux-doc, linux-kernel, Bill Huang,
	Mallikarjun Kasoju

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                               |   33 ++++++++++++++++++--
 include/linux/mfd/palmas.h                         |    1 +
 3 files changed, 37 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..220a34d 100644
--- a/drivers/mfd/palmas.c
+++ b/drivers/mfd/palmas.c
@@ -229,6 +229,32 @@ 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;
+	int ret, slave;
+
+	if (!palmas_dev)
+		return;
+
+	slave = PALMAS_BASE_TO_SLAVE(PALMAS_PMU_CONTROL_BASE);
+	addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_DEV_CTRL);
+
+	ret = regmap_update_bits(
+			palmas_dev->regmap[slave],
+			addr,
+			PALMAS_DEV_CTRL_DEV_ON,
+			0);
+
+	if (ret)
+		pr_err("%s: Unable to write to DEV_CTRL_DEV_ON: %d\n",
+				__func__, ret);
 }
 
 static unsigned int palmas_features = PALMAS_PMIC_FEATURE_SMPS10_BOOST;
@@ -423,10 +449,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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] mfd: palmas: Add power off control
  2013-07-31  7:17 [PATCH v2 1/1] mfd: palmas: Add power off control Bill Huang
@ 2013-07-31 11:57 ` Nishanth Menon
  2013-07-31 17:20   ` Stephen Warren
  2013-08-01 11:08   ` Bill Huang
  0 siblings, 2 replies; 9+ messages in thread
From: Nishanth Menon @ 2013-07-31 11:57 UTC (permalink / raw)
  To: Bill Huang
  Cc: sameo, rob.herring, pawel.moll, mark.rutland, swarren,
	ian.campbell, rob, lee.jones, broonie, j-keerthy, grant.likely,
	ian, devicetree, linux-doc, linux-kernel, Mallikarjun Kasoju

On 07/31/2013 02:17 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                               |   33 ++++++++++++++++++--

Since the specific question on v1 was not answered, I will ask again, 
any reason why it wont fit in drivers/power/reset/ is'nt it the right
place to add this?


>   include/linux/mfd/palmas.h                         |    1 +
>   3 files changed, 37 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..220a34d 100644
> --- a/drivers/mfd/palmas.c
> +++ b/drivers/mfd/palmas.c
> @@ -229,6 +229,32 @@ 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;
> +	int ret, slave;
> +
> +	if (!palmas_dev)
> +		return;
> +

If you notice the reference code I send, atleast on TWL6035/37 variants 
of Palmas, USB IRQ unmask is mandatory for power on with USB cable - 
example usage scenario: extremely low battery, device powered off, plug 
in usb cable to restart charging - you'd like to initiate charging logic 
in bootloader, but that wont work if the device does not do OFF-ON 
transition with usb cable plugged in for vbus.

> +	slave = PALMAS_BASE_TO_SLAVE(PALMAS_PMU_CONTROL_BASE);
> +	addr = PALMAS_BASE_TO_REG(PALMAS_PMU_CONTROL_BASE, PALMAS_DEV_CTRL);
> +
> +	ret = regmap_update_bits(
> +			palmas_dev->regmap[slave],
> +			addr,
> +			PALMAS_DEV_CTRL_DEV_ON,
> +			0);
> +
> +	if (ret)
> +		pr_err("%s: Unable to write to DEV_CTRL_DEV_ON: %d\n",
> +				__func__, ret);
>   }
>
>   static unsigned int palmas_features = PALMAS_PMIC_FEATURE_SMPS10_BOOST;
> @@ -423,10 +449,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;
>


-- 
Regards,
Nishanth Menon

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] mfd: palmas: Add power off control
  2013-07-31 11:57 ` Nishanth Menon
@ 2013-07-31 17:20   ` Stephen Warren
  2013-07-31 18:31     ` Nishanth Menon
  2013-08-01 11:08   ` Bill Huang
  1 sibling, 1 reply; 9+ messages in thread
From: Stephen Warren @ 2013-07-31 17:20 UTC (permalink / raw)
  To: Nishanth Menon
  Cc: Bill Huang, sameo, rob.herring, pawel.moll, mark.rutland,
	ian.campbell, rob, lee.jones, broonie, j-keerthy, grant.likely,
	ian, devicetree, linux-doc, linux-kernel, Mallikarjun Kasoju

On 07/31/2013 05:57 AM, Nishanth Menon wrote:
> On 07/31/2013 02:17 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                               |   33
>> ++++++++++++++++++--
> 
> Since the specific question on v1 was not answered, I will ask again,
> any reason why it wont fit in drivers/power/reset/ is'nt it the right
> place to add this?

I think it makes sense to put simple standalone reset drivers into
drivers/power/reset. However, where reset is just one tiny function of a
larger device that already has a driver, it's fine for one driver to
implement multiple features or essentially expose multiple subsystems.

(besides this is system power off not system reset; which is
drivers/power/reset?)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] mfd: palmas: Add power off control
  2013-07-31 17:20   ` Stephen Warren
@ 2013-07-31 18:31     ` Nishanth Menon
  0 siblings, 0 replies; 9+ messages in thread
From: Nishanth Menon @ 2013-07-31 18:31 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Bill Huang, sameo, rob.herring, pawel.moll, mark.rutland,
	ian.campbell, rob, lee.jones, broonie, j-keerthy, grant.likely,
	ian, devicetree, linux-doc, linux-kernel, Mallikarjun Kasoju

On 11:20-20130731, Stephen Warren wrote:
> On 07/31/2013 05:57 AM, Nishanth Menon wrote:
> > On 07/31/2013 02:17 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                               |   33
> >> ++++++++++++++++++--
> > 
> > Since the specific question on v1 was not answered, I will ask again,
> > any reason why it wont fit in drivers/power/reset/ is'nt it the right
> > place to add this?
> 
> I think it makes sense to put simple standalone reset drivers into
> drivers/power/reset. However, where reset is just one tiny function of a
> larger device that already has a driver, it's fine for one driver to
Yes, this will probably increase in logic when we add stuff like USB IRQ
enable.. :(
> implement multiple features or essentially expose multiple subsystems.
I would generally agree to the same, but given we seem to now have
isolated it out to it's own subsystem, /me shrugs.

> 
> (besides this is system power off not system reset; which is
> drivers/power/reset?)

[1] seems to indicate that "or shut it down, by manipulating the main
power supply on the board." which seems to be precisely what we are
doing here, unless, ofcourse, my understanding of Palmas is wrong at
this point..

[1]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/power/reset/Kconfig
-- 
Regards,
Nishanth Menon

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] mfd: palmas: Add power off control
  2013-07-31 11:57 ` Nishanth Menon
  2013-07-31 17:20   ` Stephen Warren
@ 2013-08-01 11:08   ` Bill Huang
  2013-08-01 13:08     ` Nishanth Menon
  1 sibling, 1 reply; 9+ messages in thread
From: Bill Huang @ 2013-08-01 11:08 UTC (permalink / raw)
  To: Nishanth Menon
  Cc: sameo, rob.herring, pawel.moll, mark.rutland, swarren,
	ian.campbell, rob, lee.jones, broonie, j-keerthy, grant.likely,
	ian, devicetree, linux-doc, linux-kernel, Mallikarjun Kasoju

On Wed, 2013-07-31 at 19:57 +0800, Nishanth Menon wrote:
> 
> If you notice the reference code I send, atleast on TWL6035/37 variants 
> of Palmas, USB IRQ unmask is mandatory for power on with USB cable - 
> example usage scenario: extremely low battery, device powered off, plug 
> in usb cable to restart charging - you'd like to initiate charging logic 
> in bootloader, but that wont work if the device does not do OFF-ON 
> transition with usb cable plugged in for vbus.
> 
Why do we need to add Palmas USB_IRQ unmask logic in shutdown? Does that
mean for all platform using Palmas has to unmask USB IRQ (including
those do not power vbus through Palmas)? Can't we just have a simple
shutdown function but have the VBus programming been done in USB driver
or maybe platform driver since it is platform specific control?


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] mfd: palmas: Add power off control
  2013-08-01 11:08   ` Bill Huang
@ 2013-08-01 13:08     ` Nishanth Menon
  2013-08-02  5:45       ` Bill Huang
  0 siblings, 1 reply; 9+ messages in thread
From: Nishanth Menon @ 2013-08-01 13:08 UTC (permalink / raw)
  To: Bill Huang
  Cc: sameo, rob.herring, pawel.moll, mark.rutland, swarren,
	ian.campbell, rob, lee.jones, broonie, j-keerthy, grant.likely,
	ian, devicetree, linux-doc, linux-kernel, Mallikarjun Kasoju

On 04:08-20130801, Bill Huang wrote:
> On Wed, 2013-07-31 at 19:57 +0800, Nishanth Menon wrote:
> > 
> > If you notice the reference code I send, atleast on TWL6035/37 variants 
> > of Palmas, USB IRQ unmask is mandatory for power on with USB cable - 
> > example usage scenario: extremely low battery, device powered off, plug 
> > in usb cable to restart charging - you'd like to initiate charging logic 
> > in bootloader, but that wont work if the device does not do OFF-ON 
> > transition with usb cable plugged in for vbus.
> > 
> Why do we need to add Palmas USB_IRQ unmask logic in shutdown? Does that
> mean for all platform using Palmas has to unmask USB IRQ (including
> those do not power vbus through Palmas)? Can't we just have a simple
> shutdown function but have the VBus programming been done in USB driver
> or maybe platform driver since it is platform specific control?
we dont have a irq cleanup, irq handling is done in palmas-mfd. Further,

Why would USB driver care about vbus supply needs in complete power off
- it is the job of palmas driver? Further, palmas-mfd shutdown
handler(currently missing) if probably cleansup things:

 mfd_remove_devices(palmas->dev);
  palmas_irq_exit(palmas);

shutdown sequence becomes complicated further esp if things are
cleanedup in shutdown (Dummy patch[1]).

 
All I am saying is this: shutdown should allow powerup functionality to
work as well, how we do that is upto us - I personally found it a little
easier to keep the IRQ unmask in shutdown easier to deal with, but other
options might be possible as well.

[1]
diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
index e4d1c70..6998863 100644
--- a/drivers/mfd/palmas.c
+++ b/drivers/mfd/palmas.c
@@ -447,6 +447,11 @@ static int palmas_i2c_remove(struct i2c_client *i2c)
 	return 0;
 }
 
+static void palmas_i2c_shutdown(struct i2c_client *i2c)
+{
+	palmas_i2c_remove(i2c);
+}
+
 static const struct i2c_device_id palmas_i2c_id[] = {
 	{ "palmas", },
 	{ "twl6035", },
@@ -464,6 +469,7 @@ static struct i2c_driver palmas_i2c_driver = {
 	},
 	.probe = palmas_i2c_probe,
 	.remove = palmas_i2c_remove,
+	.shutdown = palmas_i2c_shutdown,
 	.id_table = palmas_i2c_id,
 };
 
-- 
Regards,
Nishanth Menon

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] mfd: palmas: Add power off control
  2013-08-01 13:08     ` Nishanth Menon
@ 2013-08-02  5:45       ` Bill Huang
  2013-08-02 14:39         ` Nishanth Menon
  0 siblings, 1 reply; 9+ messages in thread
From: Bill Huang @ 2013-08-02  5:45 UTC (permalink / raw)
  To: Nishanth Menon
  Cc: sameo, rob.herring, pawel.moll, mark.rutland, swarren,
	ian.campbell, rob, lee.jones, broonie, j-keerthy, grant.likely,
	ian, devicetree, linux-doc, linux-kernel, Mallikarjun Kasoju

On Thu, 2013-08-01 at 21:08 +0800, Nishanth Menon wrote:
> On 04:08-20130801, Bill Huang wrote:
> > On Wed, 2013-07-31 at 19:57 +0800, Nishanth Menon wrote:
> > > 
> > > If you notice the reference code I send, atleast on TWL6035/37 variants 
> > > of Palmas, USB IRQ unmask is mandatory for power on with USB cable - 
> > > example usage scenario: extremely low battery, device powered off, plug 
> > > in usb cable to restart charging - you'd like to initiate charging logic 
> > > in bootloader, but that wont work if the device does not do OFF-ON 
> > > transition with usb cable plugged in for vbus.
> > > 
> > Why do we need to add Palmas USB_IRQ unmask logic in shutdown? Does that
> > mean for all platform using Palmas has to unmask USB IRQ (including
> > those do not power vbus through Palmas)? Can't we just have a simple
> > shutdown function but have the VBus programming been done in USB driver
> > or maybe platform driver since it is platform specific control?
> we dont have a irq cleanup, irq handling is done in palmas-mfd. Further,
> 
> Why would USB driver care about vbus supply needs in complete power off
> - it is the job of palmas driver? Further, palmas-mfd shutdown
> handler(currently missing) if probably cleansup things:
> 
>  mfd_remove_devices(palmas->dev);
>   palmas_irq_exit(palmas);
> 
> shutdown sequence becomes complicated further esp if things are
> cleanedup in shutdown (Dummy patch[1]).
> 
>  
> All I am saying is this: shutdown should allow powerup functionality to
> work as well, how we do that is upto us - I personally found it a little
> easier to keep the IRQ unmask in shutdown easier to deal with, but other
> options might be possible as well.

I'm not sure if I understand your comments completely (maybe due to I'm
not familiar with the mechanism of unmasking USB IRQ in Palmas driver)
but doing cleanup in each driver shutdown handler makes sense to me, if
those clean up can be done in shutdown then we can make power off
function as simple as possible and being part of Palmas mfd driver?
> 
> [1]
> diff --git a/drivers/mfd/palmas.c b/drivers/mfd/palmas.c
> index e4d1c70..6998863 100644
> --- a/drivers/mfd/palmas.c
> +++ b/drivers/mfd/palmas.c
> @@ -447,6 +447,11 @@ static int palmas_i2c_remove(struct i2c_client *i2c)
>  	return 0;
>  }
>  
> +static void palmas_i2c_shutdown(struct i2c_client *i2c)
> +{
> +	palmas_i2c_remove(i2c);
> +}
> +
>  static const struct i2c_device_id palmas_i2c_id[] = {
>  	{ "palmas", },
>  	{ "twl6035", },
> @@ -464,6 +469,7 @@ static struct i2c_driver palmas_i2c_driver = {
>  	},
>  	.probe = palmas_i2c_probe,
>  	.remove = palmas_i2c_remove,
> +	.shutdown = palmas_i2c_shutdown,
>  	.id_table = palmas_i2c_id,
>  };
>  



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] mfd: palmas: Add power off control
  2013-08-02  5:45       ` Bill Huang
@ 2013-08-02 14:39         ` Nishanth Menon
  2013-08-05 10:32           ` Bill Huang
  0 siblings, 1 reply; 9+ messages in thread
From: Nishanth Menon @ 2013-08-02 14:39 UTC (permalink / raw)
  To: Bill Huang
  Cc: sameo, rob.herring, pawel.moll, mark.rutland, swarren,
	ian.campbell, rob, lee.jones, broonie, j-keerthy, grant.likely,
	ian, devicetree, linux-doc, linux-kernel, Mallikarjun Kasoju

On 08/02/2013 12:45 AM, Bill Huang wrote:
> On Thu, 2013-08-01 at 21:08 +0800, Nishanth Menon wrote:
>> On 04:08-20130801, Bill Huang wrote:
>>> On Wed, 2013-07-31 at 19:57 +0800, Nishanth Menon wrote:
>>>>
>>>> If you notice the reference code I send, atleast on TWL6035/37 variants
>>>> of Palmas, USB IRQ unmask is mandatory for power on with USB cable -
>>>> example usage scenario: extremely low battery, device powered off, plug
>>>> in usb cable to restart charging - you'd like to initiate charging logic
>>>> in bootloader, but that wont work if the device does not do OFF-ON
>>>> transition with usb cable plugged in for vbus.
>>>>
>>> Why do we need to add Palmas USB_IRQ unmask logic in shutdown? Does that
>>> mean for all platform using Palmas has to unmask USB IRQ (including
>>> those do not power vbus through Palmas)? Can't we just have a simple
>>> shutdown function but have the VBus programming been done in USB driver
>>> or maybe platform driver since it is platform specific control?
>> we dont have a irq cleanup, irq handling is done in palmas-mfd. Further,
>>
>> Why would USB driver care about vbus supply needs in complete power off
>> - it is the job of palmas driver? Further, palmas-mfd shutdown
>> handler(currently missing) if probably cleansup things:
>>
>>   mfd_remove_devices(palmas->dev);
>>    palmas_irq_exit(palmas);
>>
>> shutdown sequence becomes complicated further esp if things are
>> cleanedup in shutdown (Dummy patch[1]).
>>
>>
>> All I am saying is this: shutdown should allow powerup functionality to
>> work as well, how we do that is upto us - I personally found it a little
>> easier to keep the IRQ unmask in shutdown easier to deal with, but other
>> options might be possible as well.
>
> I'm not sure if I understand your comments completely (maybe due to I'm
> not familiar with the mechanism of unmasking USB IRQ in Palmas driver)

This is IMHO an weird configuration I saw on TWL6035/6037 Palmas device 
- so I suspect should be the case in probably other palmas devices as 
well. I hit power off, and I can start up the device again by supplying 
vbus -(usecase was, at that point in time, battery charging)

anyways, I was working with busybox and no usb driver even enabled, but 
I am told by the twl6035/7 support folks that due to design USB IRQ 
needs to be unmasked at shutdown/poweroff to allow OFF->ON transition 
sequence to start inside Palmas when vbus is supplied.


> but doing cleanup in each driver shutdown handler makes sense to me, if
> those clean up can be done in shutdown then we can make power off
> function as simple as possible and being part of Palmas mfd driver?

not really, mfd shutdown does not guarantee rest of the drivers' 
shutdown functions are safely called, pm_power_off unfortunately is the 
only "official point" where we can safely and cleanly shutdown the system.

-- 
Regards,
Nishanth Menon

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] mfd: palmas: Add power off control
  2013-08-02 14:39         ` Nishanth Menon
@ 2013-08-05 10:32           ` Bill Huang
  0 siblings, 0 replies; 9+ messages in thread
From: Bill Huang @ 2013-08-05 10:32 UTC (permalink / raw)
  To: Nishanth Menon
  Cc: sameo, rob.herring, pawel.moll, mark.rutland, swarren,
	ian.campbell, rob, lee.jones, broonie, j-keerthy, grant.likely,
	ian, devicetree, linux-doc, linux-kernel, Mallikarjun Kasoju

On Fri, 2013-08-02 at 22:39 +0800, Nishanth Menon wrote:
> On 08/02/2013 12:45 AM, Bill Huang wrote:
> > On Thu, 2013-08-01 at 21:08 +0800, Nishanth Menon wrote:
> >> On 04:08-20130801, Bill Huang wrote:
> >>> On Wed, 2013-07-31 at 19:57 +0800, Nishanth Menon wrote:
> >>>>
> >>>> If you notice the reference code I send, atleast on TWL6035/37 variants
> >>>> of Palmas, USB IRQ unmask is mandatory for power on with USB cable -
> >>>> example usage scenario: extremely low battery, device powered off, plug
> >>>> in usb cable to restart charging - you'd like to initiate charging logic
> >>>> in bootloader, but that wont work if the device does not do OFF-ON
> >>>> transition with usb cable plugged in for vbus.
> >>>>
> >>> Why do we need to add Palmas USB_IRQ unmask logic in shutdown? Does that
> >>> mean for all platform using Palmas has to unmask USB IRQ (including
> >>> those do not power vbus through Palmas)? Can't we just have a simple
> >>> shutdown function but have the VBus programming been done in USB driver
> >>> or maybe platform driver since it is platform specific control?
> >> we dont have a irq cleanup, irq handling is done in palmas-mfd. Further,
> >>
> >> Why would USB driver care about vbus supply needs in complete power off
> >> - it is the job of palmas driver? Further, palmas-mfd shutdown
> >> handler(currently missing) if probably cleansup things:
> >>
> >>   mfd_remove_devices(palmas->dev);
> >>    palmas_irq_exit(palmas);
> >>
> >> shutdown sequence becomes complicated further esp if things are
> >> cleanedup in shutdown (Dummy patch[1]).
> >>
> >>
> >> All I am saying is this: shutdown should allow powerup functionality to
> >> work as well, how we do that is upto us - I personally found it a little
> >> easier to keep the IRQ unmask in shutdown easier to deal with, but other
> >> options might be possible as well.
> >
> > I'm not sure if I understand your comments completely (maybe due to I'm
> > not familiar with the mechanism of unmasking USB IRQ in Palmas driver)
> 
> This is IMHO an weird configuration I saw on TWL6035/6037 Palmas device 
> - so I suspect should be the case in probably other palmas devices as 
> well. I hit power off, and I can start up the device again by supplying 
> vbus -(usecase was, at that point in time, battery charging)
> 
> anyways, I was working with busybox and no usb driver even enabled, but 
> I am told by the twl6035/7 support folks that due to design USB IRQ 
> needs to be unmasked at shutdown/poweroff to allow OFF->ON transition 
> sequence to start inside Palmas when vbus is supplied.
> 
> 
> > but doing cleanup in each driver shutdown handler makes sense to me, if
> > those clean up can be done in shutdown then we can make power off
> > function as simple as possible and being part of Palmas mfd driver?
> 
> not really, mfd shutdown does not guarantee rest of the drivers' 
> shutdown functions are safely called, pm_power_off unfortunately is the 
> only "official point" where we can safely and cleanly shutdown the system.
> 
It looks to me whether or not adding those extra control in a generic
Palmas driver is still in doubt, will you send your patch for this in
the near future? If not, any objection on making it as a basic and
simple power off control (that said, any further processing which has to
be in power off routine can be added on top of that, or can be moved to
be under drivers/reset if needed) using the existing mechanism?



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2013-08-05 10:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-31  7:17 [PATCH v2 1/1] mfd: palmas: Add power off control Bill Huang
2013-07-31 11:57 ` Nishanth Menon
2013-07-31 17:20   ` Stephen Warren
2013-07-31 18:31     ` Nishanth Menon
2013-08-01 11:08   ` Bill Huang
2013-08-01 13:08     ` Nishanth Menon
2013-08-02  5:45       ` Bill Huang
2013-08-02 14:39         ` Nishanth Menon
2013-08-05 10:32           ` Bill Huang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).