* [RFC PATCH] pm: prevent suspend until power supply events are processed
@ 2013-08-02 20:38 Zoran Markovic
2013-08-22 17:10 ` Zoran Markovic
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Zoran Markovic @ 2013-08-02 20:38 UTC (permalink / raw)
To: linux-kernel
Cc: Zoran Markovic, Anton Vorontsov, David Woodhouse, Arve Hjonnevag,
Todd Poynor, John Stultz
This patch, originally authored by Arve Hjonnevag and Todd Poynor,
prevents the system from entering suspend mode until the power
supply plug, unplug, or any other change of state event is fully
processed. This guarantees that the screen lights up and displays
the battery charging state. The implementation uses the power
supply wakeup_source object.
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Arve Hjonnevag <arve@android.com>
Cc: Todd Poynor <toddpoynor@google.com>
Cc: John Stultz <john.stultz@linaro.org>
Signed-off-by: Zoran Markovic <zoran.markovic@linaro.org>
---
drivers/power/power_supply_core.c | 37 +++++++++++++++++++++++++++++++------
include/linux/power_supply.h | 2 ++
2 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 3b2d5df..e68d598 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -67,23 +67,41 @@ static int __power_supply_changed_work(struct device *dev, void *data)
static void power_supply_changed_work(struct work_struct *work)
{
+ unsigned long flags;
struct power_supply *psy = container_of(work, struct power_supply,
changed_work);
dev_dbg(psy->dev, "%s\n", __func__);
- class_for_each_device(power_supply_class, NULL, psy,
- __power_supply_changed_work);
-
- power_supply_update_leds(psy);
-
- kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
+ spin_lock_irqsave(&psy->changed_lock, flags);
+ if (psy->changed) {
+ psy->changed = false;
+ spin_unlock_irqrestore(&psy->changed_lock, flags);
+ class_for_each_device(power_supply_class, NULL, psy,
+ __power_supply_changed_work);
+ power_supply_update_leds(psy);
+ kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
+ spin_lock_irqsave(&psy->changed_lock, flags);
+ }
+ /* dependent power supplies (e.g. battery) may have changed
+ * state as a result of this event, so poll again and hold
+ * the wakeup_source until all events are processed.
+ */
+ if (!psy->changed)
+ pm_relax(psy->dev);
+ spin_unlock_irqrestore(&psy->changed_lock, flags);
}
void power_supply_changed(struct power_supply *psy)
{
+ unsigned long flags;
+
dev_dbg(psy->dev, "%s\n", __func__);
+ spin_lock_irqsave(&psy->changed_lock, flags);
+ psy->changed = true;
+ pm_stay_awake(psy->dev);
+ spin_unlock_irqrestore(&psy->changed_lock, flags);
schedule_work(&psy->changed_work);
}
EXPORT_SYMBOL_GPL(power_supply_changed);
@@ -500,6 +518,11 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
goto check_supplies_failed;
}
+ spin_lock_init(&psy->changed_lock);
+ rc = device_init_wakeup(dev, true);
+ if (rc)
+ goto wakeup_init_failed;
+
rc = kobject_set_name(&dev->kobj, "%s", psy->name);
if (rc)
goto kobject_set_name_failed;
@@ -529,6 +552,7 @@ create_triggers_failed:
register_cooler_failed:
psy_unregister_thermal(psy);
register_thermal_failed:
+wakeup_init_failed:
device_del(dev);
kobject_set_name_failed:
device_add_failed:
@@ -546,6 +570,7 @@ void power_supply_unregister(struct power_supply *psy)
power_supply_remove_triggers(psy);
psy_unregister_cooler(psy);
psy_unregister_thermal(psy);
+ device_init_wakeup(psy->dev, false);
device_unregister(psy->dev);
}
EXPORT_SYMBOL_GPL(power_supply_unregister);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 804b906..253d412 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -194,6 +194,8 @@ struct power_supply {
/* private */
struct device *dev;
struct work_struct changed_work;
+ spinlock_t changed_lock;
+ bool changed;
#ifdef CONFIG_THERMAL
struct thermal_zone_device *tzd;
struct thermal_cooling_device *tcd;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
2013-08-02 20:38 [RFC PATCH] pm: prevent suspend until power supply events are processed Zoran Markovic
@ 2013-08-22 17:10 ` Zoran Markovic
2013-08-28 1:43 ` Anton Vorontsov
2014-09-03 4:39 ` Viresh Kumar
2 siblings, 0 replies; 10+ messages in thread
From: Zoran Markovic @ 2013-08-22 17:10 UTC (permalink / raw)
To: lkml
Cc: Zoran Markovic, Anton Vorontsov, David Woodhouse, Arve Hjonnevag,
Todd Poynor, John Stultz
Any opinions on this patch?
Regards, Zoran
On 2 August 2013 13:38, Zoran Markovic <zoran.markovic@linaro.org> wrote:
> This patch, originally authored by Arve Hjonnevag and Todd Poynor,
> prevents the system from entering suspend mode until the power
> supply plug, unplug, or any other change of state event is fully
> processed. This guarantees that the screen lights up and displays
> the battery charging state. The implementation uses the power
> supply wakeup_source object.
>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Arve Hjonnevag <arve@android.com>
> Cc: Todd Poynor <toddpoynor@google.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Zoran Markovic <zoran.markovic@linaro.org>
> ---
> drivers/power/power_supply_core.c | 37 +++++++++++++++++++++++++++++++------
> include/linux/power_supply.h | 2 ++
> 2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 3b2d5df..e68d598 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -67,23 +67,41 @@ static int __power_supply_changed_work(struct device *dev, void *data)
>
> static void power_supply_changed_work(struct work_struct *work)
> {
> + unsigned long flags;
> struct power_supply *psy = container_of(work, struct power_supply,
> changed_work);
>
> dev_dbg(psy->dev, "%s\n", __func__);
>
> - class_for_each_device(power_supply_class, NULL, psy,
> - __power_supply_changed_work);
> -
> - power_supply_update_leds(psy);
> -
> - kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(&psy->changed_lock, flags);
> + if (psy->changed) {
> + psy->changed = false;
> + spin_unlock_irqrestore(&psy->changed_lock, flags);
> + class_for_each_device(power_supply_class, NULL, psy,
> + __power_supply_changed_work);
> + power_supply_update_leds(psy);
> + kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(&psy->changed_lock, flags);
> + }
> + /* dependent power supplies (e.g. battery) may have changed
> + * state as a result of this event, so poll again and hold
> + * the wakeup_source until all events are processed.
> + */
> + if (!psy->changed)
> + pm_relax(psy->dev);
> + spin_unlock_irqrestore(&psy->changed_lock, flags);
> }
>
> void power_supply_changed(struct power_supply *psy)
> {
> + unsigned long flags;
> +
> dev_dbg(psy->dev, "%s\n", __func__);
>
> + spin_lock_irqsave(&psy->changed_lock, flags);
> + psy->changed = true;
> + pm_stay_awake(psy->dev);
> + spin_unlock_irqrestore(&psy->changed_lock, flags);
> schedule_work(&psy->changed_work);
> }
> EXPORT_SYMBOL_GPL(power_supply_changed);
> @@ -500,6 +518,11 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
> goto check_supplies_failed;
> }
>
> + spin_lock_init(&psy->changed_lock);
> + rc = device_init_wakeup(dev, true);
> + if (rc)
> + goto wakeup_init_failed;
> +
> rc = kobject_set_name(&dev->kobj, "%s", psy->name);
> if (rc)
> goto kobject_set_name_failed;
> @@ -529,6 +552,7 @@ create_triggers_failed:
> register_cooler_failed:
> psy_unregister_thermal(psy);
> register_thermal_failed:
> +wakeup_init_failed:
> device_del(dev);
> kobject_set_name_failed:
> device_add_failed:
> @@ -546,6 +570,7 @@ void power_supply_unregister(struct power_supply *psy)
> power_supply_remove_triggers(psy);
> psy_unregister_cooler(psy);
> psy_unregister_thermal(psy);
> + device_init_wakeup(psy->dev, false);
> device_unregister(psy->dev);
> }
> EXPORT_SYMBOL_GPL(power_supply_unregister);
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 804b906..253d412 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -194,6 +194,8 @@ struct power_supply {
> /* private */
> struct device *dev;
> struct work_struct changed_work;
> + spinlock_t changed_lock;
> + bool changed;
> #ifdef CONFIG_THERMAL
> struct thermal_zone_device *tzd;
> struct thermal_cooling_device *tcd;
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
2013-08-02 20:38 [RFC PATCH] pm: prevent suspend until power supply events are processed Zoran Markovic
2013-08-22 17:10 ` Zoran Markovic
@ 2013-08-28 1:43 ` Anton Vorontsov
2014-09-03 4:39 ` Viresh Kumar
2 siblings, 0 replies; 10+ messages in thread
From: Anton Vorontsov @ 2013-08-28 1:43 UTC (permalink / raw)
To: Zoran Markovic
Cc: linux-kernel, David Woodhouse, Arve Hjonnevag, Todd Poynor, John Stultz
On Fri, Aug 02, 2013 at 01:38:02PM -0700, Zoran Markovic wrote:
> This patch, originally authored by Arve Hjonnevag and Todd Poynor,
> prevents the system from entering suspend mode until the power
> supply plug, unplug, or any other change of state event is fully
> processed. This guarantees that the screen lights up and displays
> the battery charging state. The implementation uses the power
> supply wakeup_source object.
>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Arve Hjonnevag <arve@android.com>
> Cc: Todd Poynor <toddpoynor@google.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Zoran Markovic <zoran.markovic@linaro.org>
> ---
...
> + kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(&psy->changed_lock, flags);
> + }
> + /* dependent power supplies (e.g. battery) may have changed
Multi-line comments style issue...
> + * state as a result of this event, so poll again and hold
> + * the wakeup_source until all events are processed.
> + */
> + if (!psy->changed)
> + pm_relax(psy->dev);
...
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 804b906..253d412 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -194,6 +194,8 @@ struct power_supply {
> /* private */
> struct device *dev;
> struct work_struct changed_work;
> + spinlock_t changed_lock;
#include <linux/spinlock.h> is needed.
I fixed it up and applied the patch, thanks a lot!
Anton
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
2013-08-02 20:38 [RFC PATCH] pm: prevent suspend until power supply events are processed Zoran Markovic
2013-08-22 17:10 ` Zoran Markovic
2013-08-28 1:43 ` Anton Vorontsov
@ 2014-09-03 4:39 ` Viresh Kumar
2014-09-03 5:05 ` Viresh Kumar
2 siblings, 1 reply; 10+ messages in thread
From: Viresh Kumar @ 2014-09-03 4:39 UTC (permalink / raw)
To: Anton Vorontsov
Cc: linux-kernel, David Woodhouse, Arve Hjonnevag, Todd Poynor,
John Stultz, Zoran Markovic
Don't have Zoran's new email address, but probably other might have
answers to my queries.
I have just started with the power-supply framework a day or two back
and so my understanding might not be good enough :)
On Sat, Aug 3, 2013 at 2:08 AM, Zoran Markovic
<zoran.markovic@linaro.org> wrote:
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 3b2d5df..e68d598 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -67,23 +67,41 @@ static int __power_supply_changed_work(struct device *dev, void *data)
>
> static void power_supply_changed_work(struct work_struct *work)
> {
> + unsigned long flags;
> struct power_supply *psy = container_of(work, struct power_supply,
> changed_work);
>
> dev_dbg(psy->dev, "%s\n", __func__);
>
> - class_for_each_device(power_supply_class, NULL, psy,
> - __power_supply_changed_work);
> -
> - power_supply_update_leds(psy);
> -
> - kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(&psy->changed_lock, flags);
> + if (psy->changed) {
Can this be false here? We have reached here as the work was
scheduled after setting it to true..
Maybe a WARN_ON(psy->changed) is more sensible here ?
> + psy->changed = false;
> + spin_unlock_irqrestore(&psy->changed_lock, flags);
> + class_for_each_device(power_supply_class, NULL, psy,
> + __power_supply_changed_work);
> + power_supply_update_leds(psy);
> + kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(&psy->changed_lock, flags);
> + }
> + /* dependent power supplies (e.g. battery) may have changed
> + * state as a result of this event, so poll again and hold
> + * the wakeup_source until all events are processed.
> + */
> + if (!psy->changed)
> + pm_relax(psy->dev);
I got a bit confused here. Does the above comment say this:
The supplies dependent on 'psy' may change states and that *may*
change the state of 'psy' again? And so psy->changed is set to true
again?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
2014-09-03 4:39 ` Viresh Kumar
@ 2014-09-03 5:05 ` Viresh Kumar
[not found] ` <CA+B8BG5Hf8=ki0JDeRR13Mo7QRFDmsDuEpW8ozepvCp5KsY8ZA@mail.gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Viresh Kumar @ 2014-09-03 5:05 UTC (permalink / raw)
To: Anton Vorontsov, zrn.markovic
Cc: linux-kernel, David Woodhouse, Arve Hjonnevag, Todd Poynor, John Stultz
On Wed, Sep 3, 2014 at 10:09 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Don't have Zoran's new email address, but probably other might have
> answers to my queries.
Got Zoran's email id finally :)
> I have just started with the power-supply framework a day or two back
> and so my understanding might not be good enough :)
>
> On Sat, Aug 3, 2013 at 2:08 AM, Zoran Markovic
> <zoran.markovic@linaro.org> wrote:
>> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
>> index 3b2d5df..e68d598 100644
>> --- a/drivers/power/power_supply_core.c
>> +++ b/drivers/power/power_supply_core.c
>> @@ -67,23 +67,41 @@ static int __power_supply_changed_work(struct device *dev, void *data)
>>
>> static void power_supply_changed_work(struct work_struct *work)
>> {
>> + unsigned long flags;
>> struct power_supply *psy = container_of(work, struct power_supply,
>> changed_work);
>>
>> dev_dbg(psy->dev, "%s\n", __func__);
>>
>> - class_for_each_device(power_supply_class, NULL, psy,
>> - __power_supply_changed_work);
>> -
>> - power_supply_update_leds(psy);
>> -
>> - kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
>> + spin_lock_irqsave(&psy->changed_lock, flags);
>> + if (psy->changed) {
>
> Can this be false here? We have reached here as the work was
> scheduled after setting it to true..
>
> Maybe a WARN_ON(psy->changed) is more sensible here ?
>
>> + psy->changed = false;
>> + spin_unlock_irqrestore(&psy->changed_lock, flags);
>> + class_for_each_device(power_supply_class, NULL, psy,
>> + __power_supply_changed_work);
>> + power_supply_update_leds(psy);
>> + kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
>> + spin_lock_irqsave(&psy->changed_lock, flags);
>> + }
>> + /* dependent power supplies (e.g. battery) may have changed
>> + * state as a result of this event, so poll again and hold
>> + * the wakeup_source until all events are processed.
>> + */
>> + if (!psy->changed)
>> + pm_relax(psy->dev);
>
> I got a bit confused here. Does the above comment say this:
>
> The supplies dependent on 'psy' may change states and that *may*
> change the state of 'psy' again? And so psy->changed is set to true
> again?
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-09-09 21:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-02 20:38 [RFC PATCH] pm: prevent suspend until power supply events are processed Zoran Markovic
2013-08-22 17:10 ` Zoran Markovic
2013-08-28 1:43 ` Anton Vorontsov
2014-09-03 4:39 ` Viresh Kumar
2014-09-03 5:05 ` Viresh Kumar
[not found] ` <CA+B8BG5Hf8=ki0JDeRR13Mo7QRFDmsDuEpW8ozepvCp5KsY8ZA@mail.gmail.com>
2014-09-04 4:51 ` Viresh Kumar
2014-09-04 4:53 ` Viresh Kumar
2014-09-04 16:37 ` Tc, Jenny
2014-09-09 21:53 ` Todd Poynor
2014-09-09 21:55 ` Todd Poynor
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.