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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

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.