Re: [PATCH] pda_power: clean up irq, timer, return usage

[Jeff Garzik <jeff@garzik.org>]

Anton Vorontsov wrote:
> On Sat, Jul 14, 2007 at 07:12:04PM -0400, Jeff Garzik wrote:
>> Clean up pda_power interrupt handling:
> 
> Nice, thanks! Just few cosmetic comments.
> 
>> 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.

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.


>> 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.
> 
>> +	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.


>> +	charger_timer.data = (unsigned long) power_supply;
> 
> And here.

ditto.