Messages in this thread | | | From | "Yu, Xiangliang" <> | Subject | RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver | Date | Fri, 8 Jan 2016 03:14:13 +0000 |
| |
Hi Jon,
I find there are lots of macros to hide ioread/iowrite in kernel, I think it is reasonable. What is your concern?
> > > + > > > +#define PCI_DEVICE_ID_AMD_NTB 0x145B > > > > This looks like a tab and not a space > > I'll update it. > > > > > > +#define AMD_LINK_HB_TIMEOUT msecs_to_jiffies(1000) > > > +#define AMD_LINK_STATUS_OFFSET 0x68 #define > NTB_LIN_STA_ACTIVE_BIT > > > +0x00000002 #define NTB_LNK_STA_SPEED_MASK 0x000F0000 #define > > > +NTB_LNK_STA_WIDTH_MASK 0x03F00000 #define > NTB_LNK_STA_ACTIVE(x) > > > +(!!((x) & NTB_LIN_STA_ACTIVE_BIT)) > > > +#define NTB_LNK_STA_SPEED(x) (((x) & > NTB_LNK_STA_SPEED_MASK) >> > > 16) > > > +#define NTB_LNK_STA_WIDTH(x) (((x) & > > NTB_LNK_STA_WIDTH_MASK) >> 20) > > > + > > > +#ifndef ioread64 > > > +#ifdef readq > > > +#define ioread64 readq > > > +#else > > > +#define ioread64 _ioread64 > > > +static inline u64 _ioread64(void __iomem *mmio) { > > > + u64 low, high; > > > + > > > + low = ioread32(mmio); > > > + high = ioread32(mmio + sizeof(u32)); > > > + return low | (high << 32); > > > +} > > > +#endif > > > +#endif > > > + > > > +#ifndef iowrite64 > > > +#ifdef writeq > > > +#define iowrite64 writeq > > > +#else > > > +#define iowrite64 _iowrite64 > > > +static inline void _iowrite64(u64 val, void __iomem *mmio) { > > > + iowrite32(val, mmio); > > > + iowrite32(val >> 32, mmio + sizeof(u32)); } #endif #endif > > > > > How about put ioread64/iowrite64 macro into ntb.h file? So low level driver > can avoid to define these macro. > > > > +#define NTB_READ_REG(base, r) (ioread32(base + AMD_ ## r ## > > _OFFSET)) > > > +#define NTB_WRITE_REG(base, val, r) (iowrite32(val, base + \ > > > + AMD_ ## r ## _OFFSET)) > > > +#define NTB_READ_OFFSET(base, r, of) (ioread32(base + of + \ > > > + AMD_ ## r ## _OFFSET)) > > > +#define NTB_WRITE_OFFSET(base, val, r, of) (iowrite32(val, base + \ > > > + of + AMD_ ## r ## > > > +_OFFSET)) > > > > Please do not use marcos to hide ioread/iowrite. Call iorwad/iowrite > directly. > > I don't see any wrong to hide ioread/iowrite, and I think the macros can > make code readable and easy to maintain.
| |