pda-power: only register available psu
PATCH 1/1 pda-power: only register available psu
Currently pda-power adds both ac and usb power supply units. This patch fixes it so that psu are added only if they are enabled.
drivers/power/pda_power.c +19 -18
@@ -168,21 +168,14 @@ static int pda_power_probe(struct platform_device *pdev) pda_power_supplies[1 ].num_supplicants = pdata->num_supplicants; } - ret =power_supply_register (&pdev->dev, &pda_power_supplies[0 ]); -if (ret) { -dev_err (dev,"failed to register %s power supply\n" , - pda_power_supplies[0 ].name); - goto supply0_failed; - } - - ret =power_supply_register (&pdev->dev, &pda_power_supplies[1 ]); -if (ret) { -dev_err (dev,"failed to register %s power supply\n" , - pda_power_supplies[1 ].name); - goto supply1_failed; - } -if (ac_irq) { + ret =power_supply_register (&pdev->dev, &pda_power_supplies[0 ]); +if (ret) { +dev_err (dev,"failed to register %s power supply\n" , + pda_power_supplies[0 ].name); + goto ac_failed; + } + ret =request_irq (ac_irq->start, power_changed_isr,get_irq_flags (ac_irq), ac_irq->name, &pda_power_supplies[0 ]);
@@ -193,6 +186,13 @@ static int pda_power_probe(struct platform_device *pdev) }if (usb_irq) { + ret =power_supply_register (&pdev->dev, &pda_power_supplies[1 ]); +if (ret) { +dev_err (dev,"failed to register %s power supply\n" , + pda_power_supplies[1 ].name); + goto usb_failed; + } + ret =request_irq (usb_irq->start, power_changed_isr,get_irq_flags (usb_irq), usb_irq->name, &pda_power_supplies[1 ]);
@@ -205,13 +205,14 @@ static int pda_power_probe(struct platform_device *pdev) goto success; usb_irq_failed: +power_supply_unregister (&pda_power_supplies[1 ]); +usb_failed:if (ac_irq)free_irq (ac_irq->start, &pda_power_supplies[0 ]); ac_irq_failed: -power_supply_unregister (&pda_power_supplies[1 ]); -supply1_failed: -power_supply_unregister (&pda_power_supplies[0 ]); -supply0_failed: +if (ac_irq) +power_supply_unregister (&pda_power_supplies[0 ]); +ac_failed: noirqs: wrongid: success:
Signature · 2 lines
With best wishes Dmitry
Hi Dmitry,
Quoted 2 lines · Dmitry Baryshkov
Currently pda-power adds both ac and usb power supply units. This patch fixes it so that psu are added only if they are enabled.
Thanks for the patch, this should be fixed of course. A comment though...
Quoted 6 lines
Signed-off-by Dmitry Baryshkov <[email protected]>drivers/power/pda_power.c +0
-0
[...]
drivers/power/pda_power.c
Quoted 2 lines
drivers/power/pda_power.c +1
-0if (ac_irq) { + ret =power_supply_register (&pdev->dev, &pda_power_supplies[0 ]);
I don't think we should check for IRQs when determining which one of power supplies to register. Better use is_{ac,usb}_online callbacks, this will not produce an obstacle to implement polling -- when irqs aren't mandatory. I'll send my two pending patches to show the idea.
For this particular issue, I think something like that should work. If it works for you, I'll commit that version, preserving your authorship, of course.
---
drivers/power/pda_power.c +44 -36
@@ -168,66 +168,74 @@ static int pda_power_probe(struct platform_device *pdev) pda_power_supplies[1 ].num_supplicants = pdata->num_supplicants; } - ret =power_supply_register (&pdev->dev, &pda_power_supplies[0 ]); -if (ret) { -dev_err (dev,"failed to register %s power supply\n" , - pda_power_supplies[0 ].name); - goto supply0_failed; - } +if (pdata->is_ac_online) { + ret =power_supply_register (&pdev->dev, &pda_power_supplies[0 ]); +if (ret) { +dev_err (dev,"failed to register %s power supply\n" , + pda_power_supplies[0 ].name); + goto ac_supply_failed; + } - ret =power_supply_register (&pdev->dev, &pda_power_supplies[1 ]); -if (ret) { -dev_err (dev,"failed to register %s power supply\n" , - pda_power_supplies[1 ].name); - goto supply1_failed; +if (ac_irq) { + ret =request_irq (ac_irq->start, power_changed_isr, +get_irq_flags (ac_irq), ac_irq->name, + &pda_power_supplies[0 ]); +if (ret) { +dev_err (dev,"request ac irq failed\n" ); + goto ac_irq_failed; + } + } } -if (ac_irq) { - ret =request_irq (ac_irq->start, power_changed_isr, -get_irq_flags (ac_irq), ac_irq->name, - &pda_power_supplies[0 ]); +if (pdata->is_usb_online) { + ret =power_supply_register (&pdev->dev, &pda_power_supplies[1 ]);if (ret) { -dev_err (dev,"request ac irq failed\n" ); - goto ac_irq_failed; +dev_err (dev,"failed to register %s power supply\n" , + pda_power_supplies[1 ].name); + goto usb_supply_failed; } - } -if (usb_irq) { - ret =request_irq (usb_irq->start, power_changed_isr, -get_irq_flags (usb_irq), usb_irq->name, - &pda_power_supplies[1 ]); -if (ret) { -dev_err (dev,"request usb irq failed\n" ); - goto usb_irq_failed; +if (usb_irq) { + ret =request_irq (usb_irq->start, power_changed_isr, +get_irq_flags (usb_irq), + usb_irq->name, + &pda_power_supplies[1 ]); +if (ret) { +dev_err (dev,"request usb irq failed\n" ); + goto usb_irq_failed; + } } } - goto success; +return 0 ; usb_irq_failed: -if (ac_irq) +if (pdata->is_usb_online) +power_supply_unregister (&pda_power_supplies[1 ]); +usb_supply_failed: +if (pdata->is_ac_online && ac_irq)free_irq (ac_irq->start, &pda_power_supplies[0 ]); ac_irq_failed: -power_supply_unregister (&pda_power_supplies[1 ]); -supply1_failed: -power_supply_unregister (&pda_power_supplies[0 ]); -supply0_failed: +if (pdata->is_ac_online) +power_supply_unregister (&pda_power_supplies[0 ]); +ac_supply_failed: noirqs: wrongid: -success:return ret; }static int pda_power_remove (struct platform_device *pdev) { -if (usb_irq) +if (pdata->is_usb_online && usb_irq)free_irq (usb_irq->start, &pda_power_supplies[1 ]); -if (ac_irq) +if (pdata->is_ac_online && ac_irq)free_irq (ac_irq->start, &pda_power_supplies[0 ]);del_timer_sync (&charger_timer);del_timer_sync (&supply_timer); -power_supply_unregister (&pda_power_supplies[1 ]); -power_supply_unregister (&pda_power_supplies[0 ]); +if (pdata->is_usb_online) +power_supply_unregister (&pda_power_supplies[1 ]); +if (pdata->is_ac_online) +power_supply_unregister (&pda_power_supplies[0 ]);return 0 ; }
PATCH 1/2 pda_power: various cleanups
- handle spurious interrupts correctly;
- get rid of pda_power_supplies array, use two variables instead;
- factor out psy_changed() function, it will be used for polling.
---
Untested, just to show the idea.
drivers/power/pda_power.c | 152 ++++++++++++++++++++++++++++----------------- 1 files changed, 94 insertions(+), 58 deletions(-)
drivers/power/pda_power.c +94 -58
@@ -33,6 +33,16 @@ static struct resource *ac_irq, *usb_irq;static struct timer_list charger_timer;static struct timer_list supply_timer; +enum { +PDA_PSY_OFFLINE =0 , +PDA_PSY_ONLINE =1 , +PDA_PSY_TO_CHANGE , +}; +static int new_ac_status =-1 ; +static int new_usb_status =-1 ; +static int ac_status =-1 ; +static int usb_status =-1 ; +static int pda_power_get_property (struct power_supply *psy,enum power_supply_property psp,union power_supply_propval *val)
@@ -61,36 +71,43 @@ static char *pda_power_supplied_to[] = { "backup-battery", }; -static struct power_supply pda_power_supplies[] = { - { - .name ="ac" , - .type =POWER_SUPPLY_TYPE_MAINS , - .supplied_to = pda_power_supplied_to, - .num_supplicants =ARRAY_SIZE (pda_power_supplied_to), - .properties = pda_power_props, - .num_properties =ARRAY_SIZE (pda_power_props), - .get_property = pda_power_get_property, - }, - { - .name ="usb" , - .type =POWER_SUPPLY_TYPE_USB , - .supplied_to = pda_power_supplied_to, - .num_supplicants =ARRAY_SIZE (pda_power_supplied_to), - .properties = pda_power_props, - .num_properties =ARRAY_SIZE (pda_power_props), - .get_property = pda_power_get_property, - }, +static struct power_supply pda_psy_ac = { + .name ="ac" , + .type =POWER_SUPPLY_TYPE_MAINS , + .supplied_to = pda_power_supplied_to, + .num_supplicants =ARRAY_SIZE (pda_power_supplied_to), + .properties = pda_power_props, + .num_properties =ARRAY_SIZE (pda_power_props), }; +static struct power_supply pda_psy_usb = { + .name ="usb" , + .type =POWER_SUPPLY_TYPE_USB , + .supplied_to = pda_power_supplied_to, + .num_supplicants =ARRAY_SIZE (pda_power_supplied_to), + .properties = pda_power_props, + .num_properties =ARRAY_SIZE (pda_power_props), + .get_property = pda_power_get_property, +}; + +static void update_status (void ) +{ +if (pdata->is_ac_online) + new_ac_status = !!pdata->is_ac_online (); + +if (pdata->is_usb_online) + new_usb_status = !!pdata->is_usb_online (); +} +static void update_charger (void ) {if (!pdata->set_charge)return ; -if (pdata->is_ac_online && pdata->is_ac_online ()) { +if (new_ac_status >0 ) {dev_dbg (dev,"charger on (AC)\n" ); pdata->set_charge (PDA_POWER_CHARGE_AC ); - }else if (pdata->is_usb_online && pdata->is_usb_online ()) { + }else if (new_usb_status >0 ) {dev_dbg (dev,"charger on (USB)\n" ); pdata->set_charge (PDA_POWER_CHARGE_USB ); }else {
@@ -99,31 +116,53 @@ static void update_charger(void) } } -static void supply_timer_func (unsigned long power_supply_ptr) +static void psy_changed (void ) { -void *power_supply = (void *)power_supply_ptr; +update_charger (); -power_supply_changed (power_supply); +/* +* Okay, charger set. Now wait a bit before notifying supplicants, +* charge power should stabilize. +*/ +mod_timer (&supply_timer, + jiffies +msecs_to_jiffies (pdata->wait_for_charger)); } -static void charger_timer_func (unsigned long power_supply_ptr) +static void supply_timer_func (unsigned long unused) { -update_charger (); +if (ac_status ==PDA_PSY_TO_CHANGE ) { + ac_status = new_ac_status; +power_supply_changed (&pda_psy_ac); + } -/* Okay, charger set. Now wait a bit before notifying supplicants, -* charge power should stabilize. */ - supply_timer.data = power_supply_ptr; -mod_timer (&supply_timer, - jiffies +msecs_to_jiffies (pdata->wait_for_charger)); +if (usb_status ==PDA_PSY_TO_CHANGE ) { + usb_status = new_usb_status; +power_supply_changed (&pda_psy_usb); + } +} + +static void charger_timer_func (unsigned long unused) +{ +update_status (); +psy_changed (); }static irqreturn_t power_changed_isr (int irq,void *power_supply) { -/* Wait a bit before reading ac/usb line status and setting charger, -* because ac/usb status readings may lag from irq. */ - charger_timer.data = (unsigned long )power_supply; +if (power_supply == &pda_psy_ac) + ac_status =PDA_PSY_TO_CHANGE ; +else if (power_supply == &pda_psy_usb) + usb_status =PDA_PSY_TO_CHANGE ; +else +return IRQ_NONE ; + +/* +* Wait a bit before reading ac/usb line status and setting charger, +* because ac/usb status readings may lag from irq. +*/ mod_timer (&charger_timer, jiffies +msecs_to_jiffies (pdata->wait_for_status)); +return IRQ_HANDLED ; }
@@ -142,6 +181,7 @@ static int pda_power_probe(struct platform_device *pdev) pdata = pdev->dev.platform_data; +update_status ();update_charger ();if (!pdata->wait_for_status)
@@ -155,31 +195,26 @@ static int pda_power_probe(struct platform_device *pdev) ac_irq =platform_get_resource_byname (pdev,IORESOURCE_IRQ ,"ac" ); usb_irq =platform_get_resource_byname (pdev,IORESOURCE_IRQ ,"usb" ); -if (!ac_irq && !usb_irq) { -dev_err (dev,"no ac/usb irq specified\n" ); - ret = -ENODEV ; - goto noirqs; - }if (pdata->supplied_to) { - pda_power_supplies[0 ].supplied_to = pdata->supplied_to; - pda_power_supplies[1 ].supplied_to = pdata->supplied_to; - pda_power_supplies[0 ].num_supplicants = pdata->num_supplicants; - pda_power_supplies[1 ].num_supplicants = pdata->num_supplicants; + pda_psy_ac.supplied_to = pdata->supplied_to; + pda_psy_ac.num_supplicants = pdata->num_supplicants; + pda_psy_usb.supplied_to = pdata->supplied_to; + pda_psy_usb.num_supplicants = pdata->num_supplicants; }if (pdata->is_ac_online) { - ret =power_supply_register (&pdev->dev, &pda_power_supplies[0 ]); + ret =power_supply_register (&pdev->dev, &pda_psy_ac);if (ret) {dev_err (dev,"failed to register %s power supply\n" , - pda_power_supplies[0 ].name); + pda_psy_ac.name); goto ac_supply_failed; }if (ac_irq) { ret =request_irq (ac_irq->start, power_changed_isr,get_irq_flags (ac_irq), ac_irq->name, - &pda_power_supplies[0 ]); + &pda_psy_ac);if (ret) {dev_err (dev,"request ac irq failed\n" ); goto ac_irq_failed;
@@ -188,18 +223,17 @@ static int pda_power_probe(struct platform_device *pdev) }if (pdata->is_usb_online) { - ret =power_supply_register (&pdev->dev, &pda_power_supplies[1 ]); + ret =power_supply_register (&pdev->dev, &pda_psy_usb);if (ret) {dev_err (dev,"failed to register %s power supply\n" , - pda_power_supplies[1 ].name); + pda_psy_usb.name); goto usb_supply_failed; }if (usb_irq) { ret =request_irq (usb_irq->start, power_changed_isr,get_irq_flags (usb_irq), - usb_irq->name, - &pda_power_supplies[1 ]); + usb_irq->name, &pda_psy_usb);if (ret) {dev_err (dev,"request usb irq failed\n" ); goto usb_irq_failed;
@@ -211,15 +245,14 @@ static int pda_power_probe(struct platform_device *pdev) usb_irq_failed:if (pdata->is_usb_online) -power_supply_unregister (&pda_power_supplies[1 ]); +power_supply_unregister (&pda_psy_usb); usb_supply_failed:if (pdata->is_ac_online && ac_irq) -free_irq (ac_irq->start, &pda_power_supplies[0 ]); +free_irq (ac_irq->start, &pda_psy_ac); ac_irq_failed:if (pdata->is_ac_online) -power_supply_unregister (&pda_power_supplies[0 ]); +power_supply_unregister (&pda_psy_ac); ac_supply_failed: -noirqs: wrongid:return ret; }
@@ -227,15 +260,18 @@ wrongid:static int pda_power_remove (struct platform_device *pdev) {if (pdata->is_usb_online && usb_irq) -free_irq (usb_irq->start, &pda_power_supplies[1 ]); +free_irq (usb_irq->start, &pda_psy_usb);if (pdata->is_ac_online && ac_irq) -free_irq (ac_irq->start, &pda_power_supplies[0 ]); +free_irq (ac_irq->start, &pda_psy_ac); +del_timer_sync (&charger_timer);del_timer_sync (&supply_timer); +if (pdata->is_usb_online) -power_supply_unregister (&pda_power_supplies[1 ]); +power_supply_unregister (&pda_psy_usb);if (pdata->is_ac_online) -power_supply_unregister (&pda_power_supplies[0 ]); +power_supply_unregister (&pda_psy_ac); +return 0 ; }
Signature · 1 line
1.5.2.4
PATCH 2/2 pda_power: implement polling
In reply to Anton Vorontsov---
Untested, just to show the idea.
drivers/power/pda_power.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/linux/pda_power.h | 1 + 2 files changed, 41 insertions(+), 0 deletions(-)
drivers/power/pda_power.c +40 -0
@@ -32,6 +32,8 @@ static struct pda_power_pdata *pdata;static struct resource *ac_irq, *usb_irq;static struct timer_list charger_timer;static struct timer_list supply_timer; +static struct timer_list polling_timer; +static int polling;enum {PDA_PSY_OFFLINE =0 ,
@@ -166,6 +168,28 @@ static irqreturn_t power_changed_isr(int irq, void *power_supply)return IRQ_HANDLED ; } +static void polling_timer_func (unsigned long unused) +{ +int changed =0 ; + +dev_vdbg (dev,"polling...\n" ); + +update_status (); + +if (!ac_irq && new_ac_status != ac_status) { + ac_status =PDA_PSY_TO_CHANGE ; + changed =1 ; + } + +if (!usb_irq && new_usb_status != usb_status) { + usb_status =PDA_PSY_TO_CHANGE ; + changed =1 ; + } + +if (changed) +psy_changed (); +} +static int pda_power_probe (struct platform_device *pdev) {int ret =0 ;
@@ -190,6 +214,9 @@ static int pda_power_probe(struct platform_device *pdev)if (!pdata->wait_for_charger) pdata->wait_for_charger =500 ; +if (!pdata->polling_interval) + pdata->polling_interval =2000 ; +setup_timer (&charger_timer, charger_timer_func,0 );setup_timer (&supply_timer, supply_timer_func,0 );
@@ -219,6 +246,8 @@ static int pda_power_probe(struct platform_device *pdev)dev_err (dev,"request ac irq failed\n" ); goto ac_irq_failed; } + }else { + polling =1 ; } }
@@ -238,9 +267,18 @@ static int pda_power_probe(struct platform_device *pdev)dev_err (dev,"request usb irq failed\n" ); goto usb_irq_failed; } + }else { + polling =1 ; } } +if (polling) { +dev_dbg (dev,"will poll for status\n" ); +setup_timer (&polling_timer, polling_timer_func,0 ); +mod_timer (&polling_timer, + jiffies +msecs_to_jiffies (pdata->polling_interval)); + } +return 0 ; usb_irq_failed:
@@ -264,6 +302,8 @@ static int pda_power_remove(struct platform_device *pdev)if (pdata->is_ac_online && ac_irq)free_irq (ac_irq->start, &pda_psy_ac); +if (polling) +del_timer_sync (&polling_timer);del_timer_sync (&charger_timer);del_timer_sync (&supply_timer);
include/linux/pda_power.h +1 -0
@@ -26,6 +26,7 @@ struct pda_power_pdata {unsigned int wait_for_status;/* msecs, default is 500 */ unsigned int wait_for_charger;/* msecs, default is 500 */ + unsigned int polling_interval; /* msecs, default is 2000 */ };#endif /* __PDA_POWER_H__ */
Signature · 1 line
1.5.2.4
Hi, Anton,
2008/1/4, Anton Vorontsov <[email protected]>:
Quoted 25 lines
Hi Dmitry,
Quoted 2 lines · Dmitry Baryshkov
Currently pda-power adds both ac and usb power supply units. This patch fixes it so that psu are added only if they are enabled.
Thanks for the patch, this should be fixed of course. A comment though...
Quoted 6 lines
Signed-off-by Dmitry Baryshkov <[email protected]>drivers/power/pda_power.c +0
-0[...]
Quoted 2 lines
if (ac_irq) {+ ret =
power_supply_register(&pdev->dev, &pda_power_supplies[0]);I don't think we should check for IRQs when determining which one of power supplies to register. Better use is_{ac,usb}_online callbacks, this will not produce an obstacle to implement polling -- when irqs aren't mandatory. I'll send my two pending patches to show the idea.
Quoted 3 lines
For this particular issue, I think something like that should work. If it works for you, I'll commit that version, preserving your authorship, of course.
Yes, it works. Thank you very much!
Signature · 2 lines
With best wishes Dmitry