[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