| Message ID | 1412659726-29957-13-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.walleij@linaro.org> > Cc: Lee Jones <lee.jones@linaro.org> > Cc: Samuel Ortiz <sameo@linux.intel.com> > Signed-off-by: Guenter Roeck <linux@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; > }
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).
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.
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
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?
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
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
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 @@ #include <linux/err.h> #include <linux/module.h> +#include <linux/notifier.h> #include <linux/platform_device.h> #include <linux/pm.h> #include <linux/reboot.h> @@ -23,7 +24,8 @@ static struct device *sysctrl_dev; -static void ab8500_power_off(void) +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; - } - /* * 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; } +static struct notifier_block ab8500_poweroff_nb = { + .notifier_call = ab8500_power_off, + .priority = 64, +}; + /* * Use the AB WD to reset the platform. It will perform a hard * reset instead of a soft reset. Write the reset reason to @@ -185,6 +189,7 @@ static int ab8500_sysctrl_probe(struct platform_device *pdev) struct ab8500 *ab8500 = dev_get_drvdata(pdev->dev.parent); struct ab8500_platform_data *plat; struct ab8500_sysctrl_platform_data *pdata; + int err; plat = dev_get_platdata(pdev->dev.parent); @@ -193,8 +198,9 @@ static int ab8500_sysctrl_probe(struct platform_device *pdev) sysctrl_dev = &pdev->dev; - if (!pm_power_off) - pm_power_off = ab8500_power_off; + err = register_poweroff_handler(&ab8500_poweroff_nb); + if (err) + dev_err(&pdev->dev, "Failed to register poweroff handler\n"); pdata = plat->sysctrl; if (pdata) { @@ -226,11 +232,9 @@ static int ab8500_sysctrl_probe(struct platform_device *pdev) static int ab8500_sysctrl_remove(struct platform_device *pdev) { + unregister_poweroff_handler(&ab8500_poweroff_nb); sysctrl_dev = NULL; - if (pm_power_off == ab8500_power_off) - pm_power_off = NULL; - return 0; }
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.walleij@linaro.org> Cc: Lee Jones <lee.jones@linaro.org> Cc: Samuel Ortiz <sameo@linux.intel.com> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/mfd/ab8500-sysctrl.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)