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

[Allen Hubbe]

From: Jon Mason <jdmason@xxxxxxxx>
> On Wed, Jan 6, 2016 at 9:50 PM, Yu, Xiangliang <Xiangliang.Yu@xxxxxxx>
> wrote:
> >> > +#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.

I don't like AMD_##r##_OFFSET in these macros.  It hides the use of a globally named constant like AMD_FOO_OFFSET, since one would read only FOO in the code.  It makes cross referencing difficult, since the reader needs to know FOO is really AMD_FOO_OFFSET.  This would defeat automatic cross referencing like cscope and lxr.

#define AMD_FOO_OFFSET 0xc0ff33
vs
NTB_READ_OFFSET(dev->foo_base, FOO, offset_in_foo)
// Where is FOO defined?

This macro would have at least been better written without ##; so cross referencing would still work.

NTB_READ_OFFSET(dev->foo_base, FOO, offset_in_foo)
vs
NTB_READ_OFFSET(dev->foo_base, AMD_FOO_OFFSET, offset_in_foo)
// AMD_FOO_OFFSET is 0xcoff33 (obviously)

But without ##, the macro is just the addition of its parameters.  Change the commas to addition, and the macro to ioread, and you'll see there is no benefit for having this macro any more.

NTB_READ_OFFSET(dev->foo_base, AMD_FOO_OFFSET, offset_in_foo)
vs
ioread32(dev->foo_base + AMD_FOO_OFFSET + offset_in_foo)

I second Jon's opinion.  Please make the change.  This would be better as simply ioread/write in the code.

Allen