RE: [PATCH v10 03/20] dlb: add resource and device initialization

-----Original Message-----
From: Greg KH <gregkh@linuxfoundation.org>
Sent: Wednesday, March 10, 2021 3:13 AM
To: Chen, Mike Ximing <redacted>
Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
arnd@arndb.de; Williams, Dan J [off-list ref]; pierre-
louis.bossart@linux.intel.com; Gage Eads [off-list ref]
Subject: Re: [PATCH v10 03/20] dlb: add resource and device initialization

On Wed, Mar 10, 2021 at 01:33:24AM +0000, Chen, Mike Ximing wrote:

quoted

quoted

-----Original Message-----
From: Greg KH <gregkh@linuxfoundation.org>

On Wed, Feb 10, 2021 at 11:54:06AM -0600, Mike Ximing Chen wrote:

quoted

+
+#include "dlb_bitmap.h"
+
+#define BITS_SET(x, val, mask)	(x = ((x) & ~(mask))     \
+				 | (((val) << (mask##_LOC)) & (mask)))
+#define BITS_GET(x, mask)       (((x) & (mask)) >> (mask##_LOC))

Why not use the built-in kernel functions for this?  Why are you
creating your own?

FIELD_GET(_mask, _val) and FIELD_PREP(_mask, _val) in include/linux/bitfield.h
are similar to our BITS_GET() and BITS_SET().  However in our case, mask##_LOC
is a known constant defined in dlb_regs.h,  so we don't need to use
_buildin_ffs(mask) to calculate the location of mask as FIELD_GET() and

FIELD_PREP()

quoted

do.  We can still use FIELD_GET and FIELD_PREP, but our macros are a little more
efficient. Would it be OK to keep them?

No, please use the kernel-wide proper functions, there's no need for
single tiny driver to be "special" in this regard.  If somehow the
in-kernel functions are not sufficient, it's always better to fix them
up than to go your own way here.

OK. I will use FIELD_GET() and FIELD_PREP() macros in the next revision.

Thanks
Mike