On Jan 22, 2008 7:58 PM, Jeff Garzik <jeff@xxxxxxxxxx> wrote:
> > +#define READ_PORT_CONFIG_DATA(i) \
> > + ((i > 3)?mr32(P4_CFG_DATA + (i - 4) * 8):mr32(P0_CFG_DATA + i * 8))
> > +#define WRITE_PORT_CONFIG_DATA(i,tmp) \
> > + {if (i > 3)mw32(P4_CFG_DATA + (i - 4) * 8, tmp); \
> > + else \
> > + mw32(P0_CFG_DATA + i * 8, tmp); }
> > +#define WRITE_PORT_CONFIG_ADDR(i,tmp) \
> > + {if (i > 3)mw32(P4_CFG_ADDR + (i - 4) * 8, tmp); \
> > + else \
> > + mw32(P0_CFG_ADDR + i * 8, tmp); }
> > +
> > +#define READ_PORT_PHY_CONTROL(i) \
> > + ((i > 3)?mr32(P4_SER_CTLSTAT + (i - 4) * 4):mr32(P0_SER_CTLSTAT+i * 4))
> > +#define WRITE_PORT_PHY_CONTROL(i,tmp) \
> > + {if (i > 3)mw32(P4_SER_CTLSTAT + (i - 4) * 4, tmp); \
> > + else \
> > + mw32(P0_SER_CTLSTAT + i * 4, tmp); }
> > +
> > +#define READ_PORT_VSR_DATA(i) \
> > + ((i > 3)?mr32(P4_VSR_DATA + (i - 4) * 8):mr32(P0_VSR_DATA+i*8))
> > +#define WRITE_PORT_VSR_DATA(i,tmp) \
> > + {if (i > 3)mw32(P4_VSR_DATA + (i - 4) * 8, tmp); \
> > + else \
> > + mw32(P0_VSR_DATA + i*8, tmp); }
> > +#define WRITE_PORT_VSR_ADDR(i,tmp) \
> > + {if (i > 3)mw32(P4_VSR_ADDR + (i - 4) * 8, tmp); \
> > + else \
> > + mw32(P0_VSR_ADDR + i * 8, tmp); }
> > +
> > +#define READ_PORT_IRQ_STAT(i) \
> > + ((i > 3)?mr32(P4_INT_STAT + (i - 4) * 8):mr32(P0_INT_STAT + i * 8))
> > +#define WRITE_PORT_IRQ_STAT(i,tmp) \
> > + {if (i > 3)mw32(P4_INT_STAT + (i-4) * 8, tmp); \
> > + else \
> > + mw32(P0_INT_STAT + i * 8, tmp); }
> > +#define READ_PORT_IRQ_MASK(i) \
> > + ((i > 3)?mr32(P4_INT_MASK + (i-4) * 8):mr32(P0_INT_MASK+i*8))
> > +#define WRITE_PORT_IRQ_MASK(i,tmp) \
> > + {if (i > 3)mw32(P4_INT_MASK + (i-4) * 8, tmp); \
> > + else \
> > + mw32(P0_INT_MASK + i * 8, tmp); }
>
>
> make these macros readable, by breaking each C statement into a separate
> line
I'd prefer these all be written as static functions. Let the compiler do
the type checking instead of assuming we've got it right. There is no
performance difference AFAIK.
>
>
>
>
> > @@ -260,13 +368,33 @@ enum hw_register_bits {
> > PHYEV_RDY_CH = (1U << 0), /* phy ready changed state */
> >
> > /* MVS_PCS */
> > + PCS_EN_SATA_REG = (16), /* Enable SATA Register Set*/
> > + PCS_EN_PORT_XMT_START = (12), /* Enable Port Transmit*/
> > + PCS_EN_PORT_XMT_START2 = (8), /* For 6480*/
> > PCS_SATA_RETRY = (1U << 8), /* retry ctl FIS on R_ERR */
> > PCS_RSP_RX_EN = (1U << 7), /* raw response rx */
> > PCS_SELF_CLEAR = (1U << 5), /* self-clearing int mode */
> > PCS_FIS_RX_EN = (1U << 4), /* FIS rx enable */
What's the difference between PCS_FIS_RX_EN and PCS_EN_SATA_REG?
Grouping them together implies they define bits/commands for the same register.
The three new entries collide with the existing definitions and it's
not obvious what
the intended usage is.
Later, jeff pointed out drivers should be using dev_dbg() instead of splattering
"MVS_PRINTK" alll over the place. I'd like to strongly encourage that.
hth,
grant
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
