Re: [PATCH] Marvell 6440 SAS/SATA driver

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Fri, 2008-01-25 at 09:24 -0800, 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?
> And the one letter difference in the name is just asking for trouble.
> Better to call it "mmio_base" or something a bit longer that won't
> likely collide with other stuff.
> 
> +/* offset for D2H FIS in the Received FIS List Structure */
> +#define SATA_RECEIVED_D2H_FIS(reg_set) \
> +	(mvi->rx_fis + 0x400 + 0x100 * reg_set + 0x40)
> 
> Ditto.
> 
> +	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.
> 
> #define MODE_AUTO_DET_EN   (0xff << 8)    /* enable auto detect on all
> 8 ports */
> 
> 
> Ditto for MODE_SAS_SATA.

Given that we don't have public docs for this driver, having register
bit definitions in the driver, even if they aren't used anywhere can be
incredibly helpful, so please don't remove them.  Even more useful would
be additional comments saying what they actually do ...

James