Re: [PATCH] Marvell 6440 SAS/SATA driver

[Jeff Garzik <jeff@garzik.org>]

Grant Grundler wrote:
> On Jan 25, 2008 8:43 AM, Ke Wei <kewei.mv@gmail.com> 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