diff mbox

[v5,18/48] mfd: twl4030-power: Register with kernel power-off handler

Message ID 1415292213-28652-19-git-send-email-linux@roeck-us.net (mailing list archive)
State New, archived
Headers show

Commit Message

Guenter Roeck Nov. 6, 2014, 4:43 p.m. UTC 
Register with kernel power-off handler instead of setting pm_power_off
directly. Register with low priority to reflect that the original code
only sets pm_power_off if it was not already set.

Make twl4030_power_off static as it is only called from the twl4030-power
driver. Drop remove function as it is no longer needed.

Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: linux-omap@vger.kernel.org
Acked-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v5:
- Rebase to v3.18-rc3
v4:
- Do not use notifiers but internal functions and data structures to manage
  the list of power-off handlers. Drop unused parameters from callbacks, and
  make the power-off function type void
v3:
- Replace poweroff in all newly introduced variables and in text
  with power_off or power-off as appropriate
- Replace POWEROFF_PRIORITY_xxx with POWER_OFF_PRIORITY_xxx
v2:
- Use define to specify poweroff handler priority
- Use dev_warn instead of dev_err
- Use devm_register_power_off_handler
- Drop remove function as it is no longer needed.

 drivers/mfd/twl4030-power.c | 25 +++++++++++++++----------
 include/linux/i2c/twl.h     |  1 -
 2 files changed, 15 insertions(+), 11 deletions(-)

Comments

Pavel Machek Nov. 10, 2014, 8:46 a.m. UTC | #1 
Hi!

> @@ -611,7 +611,7 @@ twl4030_power_configure_resources(const struct twl4030_power_data *pdata)
>   * After a successful execution, TWL shuts down the power to the SoC
>   * and all peripherals connected to it.
>   */
> -void twl4030_power_off(void)
> +static void twl4030_power_off(struct power_off_handler_block *this)
>  {
>  	int err;
>  
> @@ -621,6 +621,11 @@ void twl4030_power_off(void)
>  		pr_err("TWL4030 Unable to power off\n");
>  }
>  
> +static struct power_off_handler_block twl4030_power_off_hb = {
> +	.handler = twl4030_power_off,
> +	.priority = POWER_OFF_PRIORITY_LOW,
> +};
> +
>  static bool twl4030_power_use_poweroff(const struct twl4030_power_data *pdata,
>  					struct device_node *node)
>  {
> @@ -839,7 +844,9 @@ static int twl4030_power_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Board has to be wired properly to use this feature */
> -	if (twl4030_power_use_poweroff(pdata, node) && !pm_power_off) {
> +	if (twl4030_power_use_poweroff(pdata, node)) {
> +		int ret;
> +
>  		/* Default for SEQ_OFFSYNC is set, lets ensure this */
>  		err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
>  				      TWL4030_PM_MASTER_CFG_P123_TRANSITION);
> @@ -856,7 +863,11 @@ static int twl4030_power_probe(struct platform_device *pdev)
>  			}
>  		}
>  
> -		pm_power_off = twl4030_power_off;
> +		ret = devm_register_power_off_handler(&pdev->dev,
> +						      &twl4030_power_off_hb);
> +		if (ret)
> +			dev_warn(&pdev->dev,
> +				 "Failed to register power-off handler\n");
>  	}
>  

Could we get rid of the "struct power_off_handler_block" and guarantee
that register_power_off never fails (or print message from the
register_power_off...)? That way, your patch would be an cleanup.

You could then add priorities if they turn out to be really
neccessary, later...
									Pavel
Guenter Roeck Nov. 10, 2014, 2:09 p.m. UTC | #2 
On 11/10/2014 12:46 AM, Pavel Machek wrote:
> Hi!
>
>> @@ -611,7 +611,7 @@ twl4030_power_configure_resources(const struct twl4030_power_data *pdata)
>>    * After a successful execution, TWL shuts down the power to the SoC
>>    * and all peripherals connected to it.
>>    */
>> -void twl4030_power_off(void)
>> +static void twl4030_power_off(struct power_off_handler_block *this)
>>   {
>>   	int err;
>>
>> @@ -621,6 +621,11 @@ void twl4030_power_off(void)
>>   		pr_err("TWL4030 Unable to power off\n");
>>   }
>>
>> +static struct power_off_handler_block twl4030_power_off_hb = {
>> +	.handler = twl4030_power_off,
>> +	.priority = POWER_OFF_PRIORITY_LOW,
>> +};
>> +
>>   static bool twl4030_power_use_poweroff(const struct twl4030_power_data *pdata,
>>   					struct device_node *node)
>>   {
>> @@ -839,7 +844,9 @@ static int twl4030_power_probe(struct platform_device *pdev)
>>   	}
>>
>>   	/* Board has to be wired properly to use this feature */
>> -	if (twl4030_power_use_poweroff(pdata, node) && !pm_power_off) {
>> +	if (twl4030_power_use_poweroff(pdata, node)) {
>> +		int ret;
>> +
>>   		/* Default for SEQ_OFFSYNC is set, lets ensure this */
>>   		err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
>>   				      TWL4030_PM_MASTER_CFG_P123_TRANSITION);
>> @@ -856,7 +863,11 @@ static int twl4030_power_probe(struct platform_device *pdev)
>>   			}
>>   		}
>>
>> -		pm_power_off = twl4030_power_off;
>> +		ret = devm_register_power_off_handler(&pdev->dev,
>> +						      &twl4030_power_off_hb);
>> +		if (ret)
>> +			dev_warn(&pdev->dev,
>> +				 "Failed to register power-off handler\n");
>>   	}
>>
>
> Could we get rid of the "struct power_off_handler_block" and guarantee
> that register_power_off never fails (or print message from the
> register_power_off...)? That way, your patch would be an cleanup.
>
> You could then add priorities if they turn out to be really
> neccessary, later...

Priorities are necessary. We had _that_ discussion before.
Priorities solve the problem where multiple handlers are installed,
either conditionally or unconditionally. If I take priorities away,
a substantial part of the patch set's value gets lost, and I might
as well drop it.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck Nov. 10, 2014, 2:49 p.m. UTC | #3 
On 11/10/2014 06:09 AM, Guenter Roeck wrote:
> On 11/10/2014 12:46 AM, Pavel Machek wrote:
>> Hi!
>>
>>> @@ -611,7 +611,7 @@ twl4030_power_configure_resources(const struct twl4030_power_data *pdata)
>>>    * After a successful execution, TWL shuts down the power to the SoC
>>>    * and all peripherals connected to it.
>>>    */
>>> -void twl4030_power_off(void)
>>> +static void twl4030_power_off(struct power_off_handler_block *this)
>>>   {
>>>       int err;
>>>
>>> @@ -621,6 +621,11 @@ void twl4030_power_off(void)
>>>           pr_err("TWL4030 Unable to power off\n");
>>>   }
>>>
>>> +static struct power_off_handler_block twl4030_power_off_hb = {
>>> +    .handler = twl4030_power_off,
>>> +    .priority = POWER_OFF_PRIORITY_LOW,
>>> +};
>>> +
>>>   static bool twl4030_power_use_poweroff(const struct twl4030_power_data *pdata,
>>>                       struct device_node *node)
>>>   {
>>> @@ -839,7 +844,9 @@ static int twl4030_power_probe(struct platform_device *pdev)
>>>       }
>>>
>>>       /* Board has to be wired properly to use this feature */
>>> -    if (twl4030_power_use_poweroff(pdata, node) && !pm_power_off) {
>>> +    if (twl4030_power_use_poweroff(pdata, node)) {
>>> +        int ret;
>>> +
>>>           /* Default for SEQ_OFFSYNC is set, lets ensure this */
>>>           err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
>>>                         TWL4030_PM_MASTER_CFG_P123_TRANSITION);
>>> @@ -856,7 +863,11 @@ static int twl4030_power_probe(struct platform_device *pdev)
>>>               }
>>>           }
>>>
>>> -        pm_power_off = twl4030_power_off;
>>> +        ret = devm_register_power_off_handler(&pdev->dev,
>>> +                              &twl4030_power_off_hb);
>>> +        if (ret)
>>> +            dev_warn(&pdev->dev,
>>> +                 "Failed to register power-off handler\n");
>>>       }
>>>
>>
>> Could we get rid of the "struct power_off_handler_block" and guarantee
>> that register_power_off never fails (or print message from the
>> register_power_off...)? That way, your patch would be an cleanup.
>>
>> You could then add priorities if they turn out to be really
>> neccessary, later...
>
> Priorities are necessary. We had _that_ discussion before.
> Priorities solve the problem where multiple handlers are installed,
> either conditionally or unconditionally. If I take priorities away,
> a substantial part of the patch set's value gets lost, and I might
> as well drop it.
>
I have an idea: Instead of dropping the priority, drop
power_off_handler_block and add two parameters to register_power_off_handler
and devm_register_power_off_handler instead: the priority and a context.
At the same time, declare that those two functions must be called
with the memory subsystem initialized (register_power_off_handler_simple
must be used otherwise).

With this change, the registration functions can still fail due to memory
allocation errors, but we can get rid of the data structure and simplify
the calling code while retaining functionality. I'll explore that for v7.

Guenter

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

Patch

diff --git a/drivers/mfd/twl4030-power.c b/drivers/mfd/twl4030-power.c
index cf92a6d..88fd33c 100644
--- a/drivers/mfd/twl4030-power.c
+++ b/drivers/mfd/twl4030-power.c
@@ -25,9 +25,9 @@ 
  */
 
 #include <linux/module.h>
-#include <linux/pm.h>
 #include <linux/i2c/twl.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 
@@ -611,7 +611,7 @@  twl4030_power_configure_resources(const struct twl4030_power_data *pdata)
  * After a successful execution, TWL shuts down the power to the SoC
  * and all peripherals connected to it.
  */
-void twl4030_power_off(void)
+static void twl4030_power_off(struct power_off_handler_block *this)
 {
 	int err;
 
@@ -621,6 +621,11 @@  void twl4030_power_off(void)
 		pr_err("TWL4030 Unable to power off\n");
 }
 
+static struct power_off_handler_block twl4030_power_off_hb = {
+	.handler = twl4030_power_off,
+	.priority = POWER_OFF_PRIORITY_LOW,
+};
+
 static bool twl4030_power_use_poweroff(const struct twl4030_power_data *pdata,
 					struct device_node *node)
 {
@@ -839,7 +844,9 @@  static int twl4030_power_probe(struct platform_device *pdev)
 	}
 
 	/* Board has to be wired properly to use this feature */
-	if (twl4030_power_use_poweroff(pdata, node) && !pm_power_off) {
+	if (twl4030_power_use_poweroff(pdata, node)) {
+		int ret;
+
 		/* Default for SEQ_OFFSYNC is set, lets ensure this */
 		err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &val,
 				      TWL4030_PM_MASTER_CFG_P123_TRANSITION);
@@ -856,7 +863,11 @@  static int twl4030_power_probe(struct platform_device *pdev)
 			}
 		}
 
-		pm_power_off = twl4030_power_off;
+		ret = devm_register_power_off_handler(&pdev->dev,
+						      &twl4030_power_off_hb);
+		if (ret)
+			dev_warn(&pdev->dev,
+				 "Failed to register power-off handler\n");
 	}
 
 relock:
@@ -870,11 +881,6 @@  relock:
 	return err;
 }
 
-static int twl4030_power_remove(struct platform_device *pdev)
-{
-	return 0;
-}
-
 static struct platform_driver twl4030_power_driver = {
 	.driver = {
 		.name	= "twl4030_power",
@@ -882,7 +888,6 @@  static struct platform_driver twl4030_power_driver = {
 		.of_match_table = of_match_ptr(twl4030_power_of_match),
 	},
 	.probe		= twl4030_power_probe,
-	.remove		= twl4030_power_remove,
 };
 
 module_platform_driver(twl4030_power_driver);
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 8cfb50f..f8544f1 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -680,7 +680,6 @@  struct twl4030_power_data {
 };
 
 extern int twl4030_remove_script(u8 flags);
-extern void twl4030_power_off(void);
 
 struct twl4030_codec_data {
 	unsigned int digimic_delay; /* in ms */