pda_power: clean up irq, timer, return usage

PATCH In review Jeff Garzik 4 participants 9 messages Jul 14, 2007 – Jul 15, 2007 W1 dallas's 1-wire bus

lore

Share & tools

lore

Jeff Garzik

author

2007-07-14 23:12 UTC #0 lore raw

PATCH 1/1 pda_power: clean up irq, timer, return usage

Clean up pda_power interrupt handling:

Prior to this patch, the driver would pass information it needed to the interrupt handler dev_id pointer, and then prompt forget it ever did so, recreating that same information after a couple passes through the timer-based state machine.

This patch removes the redundant checks by passing the pda_power_supply[] pointer through the state machine. The current code passed 'irq' through the state machine, as an index to recreate the pointer, when we could more simply pass around the pointer itself.

Additionally, bogus "return;" statements were removed.

This patch makes it easier to remove the 'irq' argument in the future, in addition to cleaning up the driver today.

P.S. Where are the MAINTAINERS entries for this driver, and drivers/power in general?

Signed-off-by Jeff Garzik <[email protected]>

drivers/power/pda_power.c +8 ~~-14~~

index 4e1eb04..dacbe50 100644
@@ -97,36 +97,31 @@ static void update_charger(void)
 		dev_dbg(dev, "charger off\n");
 		pdata->set_charge(0);
 	}
-
-	return;
 }
 
-static void supply_timer_func(unsigned long irq)
+static void supply_timer_func(unsigned long power_supply_ptr)
 {
-	if (ac_irq && irq == ac_irq->start)
-		power_supply_changed(&pda_power_supplies[0]);
-	else if (usb_irq && irq == usb_irq->start)
-		power_supply_changed(&pda_power_supplies[1]);
-	return;
+	void *power_supply = (void *) power_supply_ptr;
+
+	power_supply_changed(power_supply);
 }
 
-static void charger_timer_func(unsigned long irq)
+static void charger_timer_func(unsigned long power_supply_ptr)
 {
 	update_charger();
 
 	/* Okay, charger set. Now wait a bit before notifying supplicants,
 	 * charge power should stabilize. */
-	supply_timer.data = irq;
+	supply_timer.data = power_supply_ptr;
 	mod_timer(&supply_timer,
 		  jiffies + msecs_to_jiffies(pdata->wait_for_charger));
-	return;
 }
 
-static irqreturn_t power_changed_isr(int irq, void *unused)
+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 = irq;
+	charger_timer.data = (unsigned long) power_supply;
 	mod_timer(&charger_timer,
 		  jiffies + msecs_to_jiffies(pdata->wait_for_status));
 	return IRQ_HANDLED;

@@ -252,7 +247,6 @@ static int __init pda_power_init(void)
 static void __exit pda_power_exit(void)
 {
 	platform_driver_unregister(&pda_power_pdrv);
-	return;
 }
 
 module_init(pda_power_init);

Anton Vorontsov

2007-07-15 00:20 UTC #1 lore raw

Quoted 2 lines · Jeff Garzik

Clean up pda_power interrupt handling:

Nice, thanks! Just few cosmetic comments.

Quoted 11 lines

Prior to this patch, the driver would pass information it needed to the interrupt handler dev_id pointer, and then prompt forget it ever did so, recreating that same information after a couple passes through the timer-based state machine.
This patch removes the redundant checks by passing the pda_power_supply[] pointer through the state machine. The current code passed 'irq' through the state machine, as an index to recreate the pointer, when we could more simply pass around the pointer itself.
Additionally, bogus "return;" statements were removed.

My preference is to use "return;" statements in functions returning void, even if functions is very small. It's just sugar for my eyes, to really see exit points. . Without returns I've feeling that something is missing there. Yes, my oddity.

Plus, so far CodingStyle does not say anything about non obligatory return statements.

These should be "fixed" too, though:
~/linux-2.6$ grep -h "return;" -A1 -r drivers/ arch/ | grep "^}$" | \
wc -l
1354

Obviously, drivers/ata is almost pure (3). ;-)

Either way, I prefer to leave alone these "return;"s, until CodingStyle permits them.

Quoted 5 lines

This patch makes it easier to remove the 'irq' argument in the future, in addition to cleaning up the driver today.
P.S. Where are the MAINTAINERS entries for this driver, and drivers/power in general?

I'll send patch shortly.

drivers/power/pda_power.c static void update_charger(void) · line 104

Quoted 1 line

drivers/power/pda_power.c +1 -0
+	void *power_supply = (void *) power_supply_ptr;

I guess common practice is not to put space here.

drivers/power/pda_power.c static void update_charger(void) · line 124

Quoted 1 line

drivers/power/pda_power.c +1 -0
+	charger_timer.data = (unsigned long) power_supply;

And here.

I'll correct this, and will apply to battery2-2.6.git.

Thanks!

Signature · 4 lines

Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2

Jeff Garzik

author

2007-07-15 00:29 UTC #2 lore raw

Quoted 30 lines · Anton Vorontsov

Quoted 1 line · Jeff Garzik
Clean up pda_power interrupt handling:
Nice, thanks! Just few cosmetic comments.
Quoted 11 lines
Prior to this patch, the driver would pass information it needed to the interrupt handler dev_id pointer, and then prompt forget it ever did so, recreating that same information after a couple passes through the timer-based state machine.
This patch removes the redundant checks by passing the pda_power_supply[] pointer through the state machine. The current code passed 'irq' through the state machine, as an index to recreate the pointer, when we could more simply pass around the pointer itself.
Additionally, bogus "return;" statements were removed.
My preference is to use "return;" statements in functions returning void, even if functions is very small. It's just sugar for my eyes, to really see exit points. . Without returns I've feeling that something is missing there. Yes, my oddity.
Plus, so far CodingStyle does not say anything about non obligatory return statements.
These should be "fixed" too, though:
~/linux-2.6$ grep -h "return;" -A1 -r drivers/ arch/ | grep "^}$" | \
wc -l
1354

Quoted 4 lines

Obviously, drivers/ata is almost pure (3). ;-)
Either way, I prefer to leave alone these "return;"s, until CodingStyle permits them.

CodingStyle is not the end-all of rulebooks. See repeated messages by Linus, me, and others on the subject. CodingStyle intentionally does not list a rule for every possible C code incarnation.

It's the general style to avoid these return statements in new code. It makes the page longer, meaning less code to review. It tends to trip up people making bombing runs through drivers, and searching for return statements in their favorite editor.

If you consider how many void functions there are in the kernel, 1354 is the obvious minority, being far smaller than "millions." Please follow what -millions- of void functions already do in the kernel.

Quoted 11 lines

Quoted 5 lines
This patch makes it easier to remove the 'irq' argument in the future, in addition to cleaning up the driver today.
P.S. Where are the MAINTAINERS entries for this driver, and drivers/power in general?
I'll send patch shortly.
drivers/power/pda_power.c static void update_charger(void) · line 104
Quoted 1 line
drivers/power/pda_power.c +1 -0
+	void *power_supply = (void *) power_supply_ptr;
                                     ^
I guess common practice is not to put space here.

incorrect. a space goes there, as I put it.

Quoted 3 lines

drivers/power/pda_power.c static void update_charger(void) · line 124
Quoted 1 line
drivers/power/pda_power.c +1 -0
+	charger_timer.data = (unsigned long) power_supply;
And here.

ditto.

Anton Vorontsov

2007-07-15 01:47 UTC #3 lore raw

PATCH 1/1 pda_power: clean up irq, timer

Clean up pda_power interrupt handling:

This patch makes it easier to remove the 'irq' argument in the future, in addition to cleaning up the driver today.

Signed-off-by Jeff Garzik <[email protected]>

---
 drivers/power/pda_power.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

drivers/power/pda_power.c +8 -9

index 4e1eb04..8304506 100644
@@ -101,32 +101,31 @@ static void update_charger(void)
 	return;
 }
 
-static void supply_timer_func(unsigned long irq)
+static void supply_timer_func(unsigned long power_supply_ptr)
 {
-	if (ac_irq && irq == ac_irq->start)
-		power_supply_changed(&pda_power_supplies[0]);
-	else if (usb_irq && irq == usb_irq->start)
-		power_supply_changed(&pda_power_supplies[1]);
+	void *power_supply = (void *) power_supply_ptr;
+
+	power_supply_changed(power_supply);
 	return;
 }
 
-static void charger_timer_func(unsigned long irq)
+static void charger_timer_func(unsigned long power_supply_ptr)
 {
 	update_charger();
 
 	/* Okay, charger set. Now wait a bit before notifying supplicants,
 	 * charge power should stabilize. */
-	supply_timer.data = irq;
+	supply_timer.data = power_supply_ptr;
 	mod_timer(&supply_timer,
 		  jiffies + msecs_to_jiffies(pdata->wait_for_charger));
 	return;
 }
 
-static irqreturn_t power_changed_isr(int irq, void *unused)
+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 = irq;
+	charger_timer.data = (unsigned long) power_supply;
 	mod_timer(&charger_timer,
 		  jiffies + msecs_to_jiffies(pdata->wait_for_status));
 	return IRQ_HANDLED;

Signature · 1 line

1.5.1.1-dirty

Anton Vorontsov

2007-07-15 01:49 UTC #4 lore raw

PATCH 1/1 Power supply class and drivers: remove non obligatory return statements

In reply to Jeff Garzik

Per Jeff Garzik request.

Signed-off-by Jeff Garzik <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/power/apm_power.c          |    4 ----
 drivers/power/ds2760_battery.c     |    7 -------
 drivers/power/olpc_battery.c       |    1 -
 drivers/power/pda_power.c          |    5 -----
 drivers/power/pmu_battery.c        |    2 --
 drivers/power/power_supply_core.c  |    6 ------
 drivers/power/power_supply_leds.c  |    8 --------
 drivers/power/power_supply_sysfs.c |    2 --
 drivers/w1/slaves/w1_ds2760.c      |    4 ----
 9 files changed, 0 insertions(+), 39 deletions(-)

drivers/power/apm_power.c +0 -4

index 042bd95..39a90a6 100644
@@ -48,8 +48,6 @@ static void find_main_battery(void)
 	}
 	if (!main_battery)
 		main_battery = batm;
-
-	return;
 }
 
 static int calculate_time(int status)

@@ -218,7 +216,6 @@ static void apm_battery_apm_get_power_status(struct apm_power_info *info)
 	}
 
 	up(&power_supply_class->sem);
-	return;
 }
 
 static int __init apm_battery_init(void)

@@ -232,7 +229,6 @@ static int __init apm_battery_init(void)
 static void __exit apm_battery_exit(void)
 {
 	apm_get_power_status = NULL;
-	return;
 }
 
 module_init(apm_battery_init);

drivers/power/ds2760_battery.c +0 -7

index 00e1ea6..be7021e 100644
@@ -254,8 +254,6 @@ static void ds2760_battery_update_status(struct ds2760_device_info *di)
 
 	if (di->charge_status != old_charge_status)
 		power_supply_changed(&di->bat);
-
-	return;
 }
 
 static void ds2760_battery_work(struct work_struct *work)

@@ -268,8 +266,6 @@ static void ds2760_battery_work(struct work_struct *work)
 
 	ds2760_battery_update_status(di);
 	queue_delayed_work(di->monitor_wqueue, &di->monitor_work, interval);
-
-	return;
 }
 
 #define to_ds2760_device_info(x) container_of((x), struct ds2760_device_info, \

@@ -283,8 +279,6 @@ static void ds2760_battery_external_power_changed(struct power_supply *psy)
 
 	cancel_delayed_work(&di->monitor_work);
 	queue_delayed_work(di->monitor_wqueue, &di->monitor_work, HZ/10);
-
-	return;
 }
 
 static int ds2760_battery_get_property(struct power_supply *psy,

@@ -457,7 +451,6 @@ static int __init ds2760_battery_init(void)
 static void __exit ds2760_battery_exit(void)
 {
 	platform_driver_unregister(&ds2760_battery_driver);
-	return;
 }
 
 module_init(ds2760_battery_init);

drivers/power/olpc_battery.c +0 -1

index 878684d..c998e68 100644
@@ -341,7 +341,6 @@ static void __exit olpc_bat_exit(void)
 	power_supply_unregister(&olpc_bat);
 	power_supply_unregister(&olpc_ac);
 	platform_device_unregister(bat_pdev);
-	return;
 }
 
 module_init(olpc_bat_init);

drivers/power/pda_power.c +0 -5

index 8304506..dacbe50 100644
@@ -97,8 +97,6 @@ static void update_charger(void)
 		dev_dbg(dev, "charger off\n");
 		pdata->set_charge(0);
 	}
-
-	return;
 }
 
 static void supply_timer_func(unsigned long power_supply_ptr)

@@ -106,7 +104,6 @@ static void supply_timer_func(unsigned long power_supply_ptr)
 	void *power_supply = (void *) power_supply_ptr;
 
 	power_supply_changed(power_supply);
-	return;
 }
 
 static void charger_timer_func(unsigned long power_supply_ptr)

@@ -118,7 +115,6 @@ static void charger_timer_func(unsigned long power_supply_ptr)
 	supply_timer.data = power_supply_ptr;
 	mod_timer(&supply_timer,
 		  jiffies + msecs_to_jiffies(pdata->wait_for_charger));
-	return;
 }
 
 static irqreturn_t power_changed_isr(int irq, void *power_supply)

@@ -251,7 +247,6 @@ static int __init pda_power_init(void)
 static void __exit pda_power_exit(void)
 {
 	platform_driver_unregister(&pda_power_pdrv);
-	return;
 }
 
 module_init(pda_power_init);

drivers/power/pmu_battery.c +0 -2

index 2fea4af..60a8cf3 100644
@@ -203,8 +203,6 @@ static void __exit pmu_bat_exit(void)
 	}
 	power_supply_unregister(&pmu_ac);
 	platform_device_unregister(bat_pdev);
-
-	return;
 }
 
 module_init(pmu_bat_init);

drivers/power/power_supply_core.c +0 -6

index e87ea51..a63b75c 100644
@@ -46,8 +46,6 @@ static void power_supply_changed_work(struct work_struct *work)
 	power_supply_update_leds(psy);
 
 	kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
-
-	return;
 }
 
 void power_supply_changed(struct power_supply *psy)

@@ -55,8 +53,6 @@ void power_supply_changed(struct power_supply *psy)
 	dev_dbg(psy->dev, "%s\n", __FUNCTION__);
 
 	schedule_work(&psy->changed_work);
-
-	return;
 }
 
 int power_supply_am_i_supplied(struct power_supply *psy)

@@ -129,7 +125,6 @@ void power_supply_unregister(struct power_supply *psy)
 	power_supply_remove_triggers(psy);
 	power_supply_remove_attrs(psy);
 	device_unregister(psy->dev);
-	return;
 }
 
 static int __init power_supply_class_init(void)

@@ -147,7 +142,6 @@ static int __init power_supply_class_init(void)
 static void __exit power_supply_class_exit(void)
 {
 	class_destroy(power_supply_class);
-	return;
 }
 
 EXPORT_SYMBOL_GPL(power_supply_changed);

drivers/power/power_supply_leds.c +0 -8

index 7232490..7f8f359 100644
@@ -40,8 +40,6 @@ static void power_supply_update_bat_leds(struct power_supply *psy)
 		led_trigger_event(psy->full_trig, LED_OFF);
 		break;
 	}
-
-	return;
 }
 
 static int power_supply_create_bat_triggers(struct power_supply *psy)

@@ -97,7 +95,6 @@ static void power_supply_remove_bat_triggers(struct power_supply *psy)
 	kfree(psy->full_trig_name);
 	kfree(psy->charging_trig_name);
 	kfree(psy->charging_full_trig_name);
-	return;
 }
 
 /* Generated power specific LEDs triggers. */

@@ -115,8 +112,6 @@ static void power_supply_update_gen_leds(struct power_supply *psy)
 		led_trigger_event(psy->online_trig, LED_FULL);
 	else
 		led_trigger_event(psy->online_trig, LED_OFF);
-
-	return;
 }
 
 static int power_supply_create_gen_triggers(struct power_supply *psy)

@@ -145,7 +140,6 @@ static void power_supply_remove_gen_triggers(struct power_supply *psy)
 {
 	led_trigger_unregister_simple(psy->online_trig);
 	kfree(psy->online_trig_name);
-	return;
 }
 
 /* Choice what triggers to create&update. */

@@ -156,7 +150,6 @@ void power_supply_update_leds(struct power_supply *psy)
 		power_supply_update_bat_leds(psy);
 	else
 		power_supply_update_gen_leds(psy);
-	return;
 }
 
 int power_supply_create_triggers(struct power_supply *psy)

@@ -172,5 +165,4 @@ void power_supply_remove_triggers(struct power_supply *psy)
 		power_supply_remove_bat_triggers(psy);
 	else
 		power_supply_remove_gen_triggers(psy);
-	return;
 }

drivers/power/power_supply_sysfs.c +0 -2

index c07d425..c7c4574 100644
@@ -176,8 +176,6 @@ void power_supply_remove_attrs(struct power_supply *psy)
 	for (i = 0; i < psy->num_properties; i++)
 		device_remove_file(psy->dev,
 			    &power_supply_attrs[psy->properties[i]]);
-
-	return;
 }
 
 static char *kstruprdup(const char *str, gfp_t gfp)

drivers/w1/slaves/w1_ds2760.c +0 -4

index 88a37fb..15e7f9a 100644
@@ -121,8 +121,6 @@ static void release_bat_id(int id)
 	mutex_lock(&bat_idr_lock);
 	idr_remove(&bat_idr, id);
 	mutex_unlock(&bat_idr_lock);
-
-	return;
 }
 
 static int w1_ds2760_add_slave(struct w1_slave *sl)

@@ -174,8 +172,6 @@ static void w1_ds2760_remove_slave(struct w1_slave *sl)
 	platform_device_unregister(pdev);
 	release_bat_id(id);
 	sysfs_remove_bin_file(&sl->dev.kobj, &w1_ds2760_bin_attr);
-
-	return;
 }
 
 static struct w1_family_ops w1_ds2760_fops = {

Signature · 1 line

1.5.1.1-dirty

Jeff Garzik

author

2007-07-15 02:09 UTC #5 lore raw

Re: [PATCH] Power supply class and drivers: remove non obligatory return statements

Quoted 4 lines · Anton Vorontsov

Per Jeff Garzik request.
Signed-off-by Jeff Garzik <[email protected]>
Signed-off-by Anton Vorontsov <[email protected]>

Quoted 11 lines

---
 drivers/power/apm_power.c          |    4 ----
 drivers/power/ds2760_battery.c     |    7 -------
 drivers/power/olpc_battery.c       |    1 -
 drivers/power/pda_power.c          |    5 -----
 drivers/power/pmu_battery.c        |    2 --
 drivers/power/power_supply_core.c  |    6 ------
 drivers/power/power_supply_leds.c  |    8 --------
 drivers/power/power_supply_sysfs.c |    2 --
 drivers/w1/slaves/w1_ds2760.c      |    4 ----
 9 files changed, 0 insertions(+), 39 deletions(-)

ACK, thanks

Andrew Morton

2007-07-15 04:50 UTC #6 lore raw

In reply to Jeff Garzik

Quoted 31 lines · On Sat, 14 Jul 2007 20:29:07 -0400 Jeff Garzik <[email protected]> wrote:

Quoted 30 lines · Anton Vorontsov
Quoted 1 line · Jeff Garzik
Clean up pda_power interrupt handling:
Nice, thanks! Just few cosmetic comments.
Quoted 11 lines
Prior to this patch, the driver would pass information it needed to the interrupt handler dev_id pointer, and then prompt forget it ever did so, recreating that same information after a couple passes through the timer-based state machine.
This patch removes the redundant checks by passing the pda_power_supply[] pointer through the state machine. The current code passed 'irq' through the state machine, as an index to recreate the pointer, when we could more simply pass around the pointer itself.
Additionally, bogus "return;" statements were removed.
My preference is to use "return;" statements in functions returning void, even if functions is very small. It's just sugar for my eyes, to really see exit points. . Without returns I've feeling that something is missing there. Yes, my oddity.
Plus, so far CodingStyle does not say anything about non obligatory return statements.
These should be "fixed" too, though:
~/linux-2.6$ grep -h "return;" -A1 -r drivers/ arch/ | grep "^}$" | \
wc -l
1354

Quoted 8 lines

Quoted 4 lines
Obviously, drivers/ata is almost pure (3). ;-)
Either way, I prefer to leave alone these "return;"s, until CodingStyle permits them.
CodingStyle is not the end-all of rulebooks. See repeated messages by Linus, me, and others on the subject. CodingStyle intentionally does not list a rule for every possible C code incarnation.

Yup, the `return' is a waste of space.

Quoted 8 lines

Quoted 5 lines
I'll send patch shortly.
drivers/power/pda_power.c static void update_charger(void) · line 104
Quoted 1 line
drivers/power/pda_power.c +1 -0
+	void *power_supply = (void *) power_supply_ptr;
                                     ^
I guess common practice is not to put space here.
incorrect. a space goes there, as I put it.

Nope, the space is a waste of space.

Anton Vorontsov

2007-07-15 01:07 UTC #7 lore raw

PATCH 1/1 MAINTAINERS: Add maintainers for power supply subsystem and drivers (was [PATCH] pda_power: clean up irq, timer, return usage)

In reply to Jeff Garzik

Quoted 2 lines · Jeff Garzik

P.S. Where are the MAINTAINERS entries for this driver, and drivers/power in general?

Collecting acks. David?

From: Anton Vorontsov <[email protected]>
Subject: [PATCH] MAINTAINERS: Add maintainers for power supply subsystem and drivers

Signed-off-by Anton Vorontsov <[email protected]>

---
 MAINTAINERS |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

MAINTAINERS +11 -0

index 845fbf4..3ccefac 100644
@@ -2863,6 +2863,17 @@ M:	[email protected]
 L:	[email protected]
 S:	Supported
 
+POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
+P:	Anton Vorontsov
+M:	[email protected]
+P:	David Woodhouse
+M:	[email protected]
+L:	[email protected]
+L:	[email protected]
+T:	git git.infradead.org/~cbou/battery2-2.6.git
+T:	git git.infradead.org/~dwmw2/battery-2.6.git
+S:	Maintained
+
 POWERPC 4xx EMAC DRIVER
 P:	Eugene Surovegin
 M:	[email protected]

Signature · 1 line

1.5.1.1-dirty

David Woodhouse

2007-07-15 07:55 UTC #8 lore raw

Re: [PATCH] MAINTAINERS: Add maintainers for power supply subsystem and drivers (was [PATCH] pda_power: clean up irq, timer, return usage)

Quoted 10 lines · Anton Vorontsov

Quoted 2 lines · Jeff Garzik
P.S. Where are the MAINTAINERS entries for this driver, and drivers/power in general?
Collecting acks. David?
From: Anton Vorontsov <[email protected]>
Subject: [PATCH] MAINTAINERS: Add maintainers for power supply subsystem and drivers
Signed-off-by Anton Vorontsov <[email protected]>

Looks good to me; except...

MAINTAINERS M: [email protected] · line 2874

Quoted 2 lines

MAINTAINERS +2 -0

+T:	git git.infradead.org/~cbou/battery2-2.6.git
+T:	git git.infradead.org/~dwmw2/battery-2.6.git

Let's just use a single tree. I created
git://git.infradead.org/battery-2.6.git which is group-writable. (it's in /srv/git on bombadil)

Signature · 1 line

dwmw2