Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1304637

Re: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver

Show all headers | View raw

On Wed, Jan 6, 2016 at 9:50 PM, Yu, Xiangliang <Xiangliang.Yu@amd.com> wrote:
>> > +
>> > +#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.

I disagree.  It is an unnecessary layer and can add to confusion.
Please make the change.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver "Yu, Xiangliang" <Xiangliang.Yu@amd.com> - 2016-01-07 04:00 +0100
  Re: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver Jon Mason <jdmason@kudzu.us> - 2016-01-08 16:10 +0100
    RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver "Allen Hubbe" <Allen.Hubbe@emc.com> - 2016-01-08 16:50 +0100
      RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver "Yu, Xiangliang" <Xiangliang.Yu@amd.com> - 2016-01-11 08:30 +0100

csiph-web