[PATCH 5/5] libata/drivers: Add pata_macio, driver Apple PowerMac/PowerBook IDE controller

Benjamin Herrenschmidt benh at kernel.crashing.org
Fri Dec 4 10:55:41 EST 2009


On Thu, 2009-12-03 at 03:12 -0500, Jeff Garzik wrote:

> Looks fine to me.  Two minor comments, which might perhaps be ignored if 
> that is your taste:
> 
> * prefer enums to #define's, for constants

yeah well ... I lifted those definitions from the old driver and didn't
feel like changing them all :-) I might do a separate patch later to
clean that up, we don't actually use a lot of those anymore since I use
pre-calculated tables, though they are good to keep as documentation.

> * prefer direct function call to "ap->ops->foo_bar()", because 
> ap->ops->foo_bar() is guaranteed to be a constant value known to the 
> driver.  The driver is the entity responsible for the function pointer.
>
> Maybe saves a cycle or two.  Not terribly important, but hey, calling 
> ap->ops->sff_exec_command() from pata_macio_bmdma_setup() is a hot path.

Makes sense. I'm tempted to make that a separate patch tho, since I've
already queued up the existing one and it's just a relatively minor
performance optim. 

Cheers,
Ben.




More information about the Linuxppc-dev mailing list