From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755566AbcAHPj6 (ORCPT ); Fri, 8 Jan 2016 10:39:58 -0500 Received: from mailuogwhop.emc.com ([168.159.213.141]:56884 "EHLO mailuogwhop.emc.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755324AbcAHPj4 convert rfc822-to-8bit (ORCPT ); Fri, 8 Jan 2016 10:39:56 -0500 X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd01.lss.emc.com u08Fdgni002944 X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd01.lss.emc.com u08Fdgni002944 From: "Allen Hubbe" To: "'Jon Mason'" , "'Yu, Xiangliang'" Cc: "'Dave Jiang'" , , "'linux-kernel'" , "'SPG_Linux_Kernel'" References: <1450878175-9300-1-git-send-email-Xiangliang.Yu@amd.com> In-Reply-To: Subject: RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver Date: Fri, 8 Jan 2016 10:39:24 -0500 Message-ID: <000d01d14a2a$c0879d50$4196d7f0$@emc.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT X-Mailer: Microsoft Outlook 14.0 Thread-Index: AQHRPV4hWwkZvz/peU2fNkpVXDjzSZ7vAFyAgADFIoCAAl5igP//sDQg Content-Language: en-us X-RSA-Classifications: Source Code, public X-Sentrion-Hostname: mailuogwprd01.lss.emc.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Jon Mason > On Wed, Jan 6, 2016 at 9:50 PM, Yu, Xiangliang > 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