Messages in this thread | | | From | "Yu, Xiangliang" <> | Subject | RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver | Date | Mon, 11 Jan 2016 07:07:51 +0000 |
| |
> From: Jon Mason <jdmason@kudzu.us> > > On Wed, Jan 6, 2016 at 9:50 PM, Yu, Xiangliang <Xiangliang.Yu@amd.com> > > 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.
The main aim of these macros is to focuses on the name of register (FOO), not the whole macros of register. and I see there are lots of these style in kernel. If your guys still can't accept the style, I can change it. And I'll change ioread/iowrite to readl/writel because it only access mmio space.
| |