Re: [PATCH 12/44] mfd: ab8500-sysctrl: Register with kernel poweroff handler

2014-10-09 Thread Catalin Marinas 
On Tue, Oct 07, 2014 at 09:00:48AM +0100, Lee Jones wrote:
 On Mon, 06 Oct 2014, Guenter Roeck wrote:
  --- a/drivers/mfd/ab8500-sysctrl.c
  +++ b/drivers/mfd/ab8500-sysctrl.c
  @@ -6,6 +6,7 @@
 
 [...]
 
  +static int ab8500_power_off(struct notifier_block *this, unsigned long 
  unused1,
  +   void *unused2)
   {
  sigset_t old;
  sigset_t all;
  @@ -34,11 +36,6 @@ static void ab8500_power_off(void)
  struct power_supply *psy;
  int ret;
   
  -   if (sysctrl_dev == NULL) {
  -   pr_err(%s: sysctrl not initialized\n, __func__);
  -   return;
  -   }
 
 Can you explain the purpose of this change please?

I guess it's because the sysctrl_dev is already initialised when
registering the power_off handler, so there isn't a way to call the
above function with a NULL sysctrl_dev. Probably even with the original
code you didn't need this check (after some race fix in
ab8500_sysctrl_remove but races is one of the things Guenter's patches
try to address).

-- 
Catalin
--
To unsubscribe from this list: send the line unsubscribe linux-metag in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/44] mfd: ab8500-sysctrl: Register with kernel poweroff handler

2014-10-09 Thread Guenter Roeck 

On 10/09/2014 03:49 AM, Lee Jones wrote:

On Thu, 09 Oct 2014, Catalin Marinas wrote:


On Tue, Oct 07, 2014 at 09:00:48AM +0100, Lee Jones wrote:

On Mon, 06 Oct 2014, Guenter Roeck wrote:

--- a/drivers/mfd/ab8500-sysctrl.c
+++ b/drivers/mfd/ab8500-sysctrl.c
@@ -6,6 +6,7 @@


[...]


+static int ab8500_power_off(struct notifier_block *this, unsigned long unused1,
+   void *unused2)
  {
sigset_t old;
sigset_t all;
@@ -34,11 +36,6 @@ static void ab8500_power_off(void)
struct power_supply *psy;
int ret;

-   if (sysctrl_dev == NULL) {
-   pr_err(%s: sysctrl not initialized\n, __func__);
-   return;
-   }


Can you explain the purpose of this change please?


I guess it's because the sysctrl_dev is already initialised when
registering the power_off handler, so there isn't a way to call the
above function with a NULL sysctrl_dev. Probably even with the original
code you didn't need this check (after some race fix in
ab8500_sysctrl_remove but races is one of the things Guenter's patches
try to address).


Sounds reasonable, although I think this change should be part of
another patch.


Sure, no problem. I'll split this into two patches.

Since we are at it, any idea what to do with the restart function
in the same file ? It is not used anywhere.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-metag in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/44] mfd: ab8500-sysctrl: Register with kernel poweroff handler

2014-10-09 Thread Lee Jones 
On Thu, 09 Oct 2014, Guenter Roeck wrote:

 On 10/09/2014 03:49 AM, Lee Jones wrote:
 On Thu, 09 Oct 2014, Catalin Marinas wrote:
 
 On Tue, Oct 07, 2014 at 09:00:48AM +0100, Lee Jones wrote:
 On Mon, 06 Oct 2014, Guenter Roeck wrote:
 --- a/drivers/mfd/ab8500-sysctrl.c
 +++ b/drivers/mfd/ab8500-sysctrl.c
 @@ -6,6 +6,7 @@
 
 [...]
 
 +static int ab8500_power_off(struct notifier_block *this, unsigned long 
 unused1,
 + void *unused2)
   {
   sigset_t old;
   sigset_t all;
 @@ -34,11 +36,6 @@ static void ab8500_power_off(void)
   struct power_supply *psy;
   int ret;
 
 - if (sysctrl_dev == NULL) {
 - pr_err(%s: sysctrl not initialized\n, __func__);
 - return;
 - }
 
 Can you explain the purpose of this change please?
 
 I guess it's because the sysctrl_dev is already initialised when
 registering the power_off handler, so there isn't a way to call the
 above function with a NULL sysctrl_dev. Probably even with the original
 code you didn't need this check (after some race fix in
 ab8500_sysctrl_remove but races is one of the things Guenter's patches
 try to address).
 
 Sounds reasonable, although I think this change should be part of
 another patch.
 
 Sure, no problem. I'll split this into two patches.
 
 Since we are at it, any idea what to do with the restart function
 in the same file ? It is not used anywhere.

You can strip it out with Linus Walleij's Ack.  Or I'll be happy to do
it?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-metag in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/44] mfd: ab8500-sysctrl: Register with kernel poweroff handler

2014-10-09 Thread Guenter Roeck 
On Thu, Oct 09, 2014 at 02:33:55PM +0100, Lee Jones wrote:
 On Thu, 09 Oct 2014, Guenter Roeck wrote:
 
  On 10/09/2014 03:49 AM, Lee Jones wrote:
  On Thu, 09 Oct 2014, Catalin Marinas wrote:
  
  On Tue, Oct 07, 2014 at 09:00:48AM +0100, Lee Jones wrote:
  On Mon, 06 Oct 2014, Guenter Roeck wrote:
  --- a/drivers/mfd/ab8500-sysctrl.c
  +++ b/drivers/mfd/ab8500-sysctrl.c
  @@ -6,6 +6,7 @@
  
  [...]
  
  +static int ab8500_power_off(struct notifier_block *this, unsigned long 
  unused1,
  +   void *unused2)
{
  sigset_t old;
  sigset_t all;
  @@ -34,11 +36,6 @@ static void ab8500_power_off(void)
  struct power_supply *psy;
  int ret;
  
  -   if (sysctrl_dev == NULL) {
  -   pr_err(%s: sysctrl not initialized\n, __func__);
  -   return;
  -   }
  
  Can you explain the purpose of this change please?
  
  I guess it's because the sysctrl_dev is already initialised when
  registering the power_off handler, so there isn't a way to call the
  above function with a NULL sysctrl_dev. Probably even with the original
  code you didn't need this check (after some race fix in
  ab8500_sysctrl_remove but races is one of the things Guenter's patches
  try to address).
  
  Sounds reasonable, although I think this change should be part of
  another patch.
  
  Sure, no problem. I'll split this into two patches.
  
  Since we are at it, any idea what to do with the restart function
  in the same file ? It is not used anywhere.
 
 You can strip it out with Linus Walleij's Ack.  Or I'll be happy to do
 it?
 
I'll strip it out in a 3rd patch.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-metag in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/44] mfd: ab8500-sysctrl: Register with kernel poweroff handler

2014-10-09 Thread Guenter Roeck 
On Thu, Oct 09, 2014 at 11:49:27AM +0100, Lee Jones wrote:
 On Thu, 09 Oct 2014, Catalin Marinas wrote:
 
  On Tue, Oct 07, 2014 at 09:00:48AM +0100, Lee Jones wrote:
   On Mon, 06 Oct 2014, Guenter Roeck wrote:
--- a/drivers/mfd/ab8500-sysctrl.c
+++ b/drivers/mfd/ab8500-sysctrl.c
@@ -6,6 +6,7 @@
   
   [...]
   
+static int ab8500_power_off(struct notifier_block *this, unsigned long 
unused1,
+   void *unused2)
 {
sigset_t old;
sigset_t all;
@@ -34,11 +36,6 @@ static void ab8500_power_off(void)
struct power_supply *psy;
int ret;
 
-   if (sysctrl_dev == NULL) {
-   pr_err(%s: sysctrl not initialized\n, __func__);
-   return;
-   }
   
   Can you explain the purpose of this change please?
  
  I guess it's because the sysctrl_dev is already initialised when
  registering the power_off handler, so there isn't a way to call the
  above function with a NULL sysctrl_dev. Probably even with the original
  code you didn't need this check (after some race fix in
  ab8500_sysctrl_remove but races is one of the things Guenter's patches
  try to address).
 
 Sounds reasonable, although I think this change should be part of
 another patch.
 
Turns out the options are to either drop the check or to use the device
managed function to register the poweroff handler. I decided to keep
the check and use the device managed function.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-metag in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/44] mfd: ab8500-sysctrl: Register with kernel poweroff handler

2014-10-07 Thread Lee Jones 
On Mon, 06 Oct 2014, Guenter Roeck wrote:

 Register with kernel poweroff handler instead of setting pm_power_off
 directly. Register with a low priority value of 64 to reflect that
 the original code only sets pm_power_off if it was not already set.
 
 Cc: Linus Walleij linus.wall...@linaro.org
 Cc: Lee Jones lee.jo...@linaro.org
 Cc: Samuel Ortiz sa...@linux.intel.com
 Signed-off-by: Guenter Roeck li...@roeck-us.net
 ---
  drivers/mfd/ab8500-sysctrl.c | 26 +++---
  1 file changed, 15 insertions(+), 11 deletions(-)
 
 diff --git a/drivers/mfd/ab8500-sysctrl.c b/drivers/mfd/ab8500-sysctrl.c
 index 8e0dae5..677438f 100644
 --- a/drivers/mfd/ab8500-sysctrl.c
 +++ b/drivers/mfd/ab8500-sysctrl.c
 @@ -6,6 +6,7 @@

[...]

 +static int ab8500_power_off(struct notifier_block *this, unsigned long 
 unused1,
 + void *unused2)
  {
   sigset_t old;
   sigset_t all;
 @@ -34,11 +36,6 @@ static void ab8500_power_off(void)
   struct power_supply *psy;
   int ret;
  
 - if (sysctrl_dev == NULL) {
 - pr_err(%s: sysctrl not initialized\n, __func__);
 - return;
 - }

Can you explain the purpose of this change please?

   /*
* If we have a charger connected and we're powering off,
* reboot into charge-only mode.
 @@ -83,8 +80,15 @@ shutdown:
AB8500_STW4500CTRL1_SWRESET4500N);
   (void)sigprocmask(SIG_SETMASK, old, NULL);
   }
 +
 + return NOTIFY_DONE;
  }

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-metag in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html