[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BLUPR12MB0420FA3986F9C5090CA5C547EBF60@BLUPR12MB0420.namprd12.prod.outlook.com>
Date: Fri, 8 Jan 2016 03:14:13 +0000
From: "Yu, Xiangliang" <Xiangliang.Yu@....com>
To: Jon Mason <jdmason@...zu.us>
CC: Dave Jiang <dave.jiang@...el.com>,
"Hubbe, Allen" <Allen.Hubbe@....com>,
"linux-ntb@...glegroups.com" <linux-ntb@...glegroups.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
SPG_Linux_Kernel <SPG_Linux_Kernel@....com>
Subject: RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver
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.
Powered by blists - more mailing lists