Grant Grundler wrote:
On Jan 25, 2008 8:43 AM, Ke Wei <[EMAIL PROTECTED]> wrote:
The attached is Marvell 6440 SAS/SATA driver. I will try to send email
by git-send-email.

I know this isn't part of this patch:
 #define mr32(reg)      readl(regs + MVS_##reg)
 #define mw32(reg,val)  writel((val), regs + MVS_##reg)

But can "regs" be declared a parameter to the macro?

This is a common technique in drivers (especially net drivers), eliminating the redundant-across-the-entire-driver argument passed to each register read or write. The result is infinitely more readable and compact.

        val = mr32(IRQ_STAT);

immediately communicates all the necessary information you need.


+       MODE_AUTO_DET_PORT7 = (1U << 15), /* port0 SAS/SATA autodetect */
+       MODE_AUTO_DET_PORT6 = (1U << 14),
+       MODE_AUTO_DET_PORT5 = (1U << 13),
+       MODE_AUTO_DET_PORT4 = (1U << 12),
+       MODE_AUTO_DET_PORT3 = (1U << 11),
+       MODE_AUTO_DET_PORT2 = (1U << 10),
+       MODE_AUTO_DET_PORT1 = (1U << 9),
+       MODE_AUTO_DET_PORT0 = (1U << 8),

These really aren't needed.

Like James noted, without public docs, we don't want to be removing any hardware definitions.


Have to stop for now...but I'm wonder how this driver ever worked
given the number of HW register bits that were changed (assuming
they were wrong before this patch). And this driver looks _alot_
better than the previous Marvell drivers I've looked at. Please keep
up the good work! :)

Before this patch, the driver did not work. As I do with all other drivers I write, I write the entire driver from the datasheet before testing anything.

        Jeff



-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to