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

-----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()
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?
 

quoted

-static void
-dlb_pf_unmap_pci_bar_space(struct dlb *dlb, struct pci_dev *pdev)
+static void dlb_pf_unmap_pci_bar_space(struct dlb *dlb, struct pci_dev *pdev)

Why reformat code here, and not do it right the first time around?

Sorry,  this should not happen. Will fix it.

quoted

+/*******************************/
+/****** Driver management ******/
+/*******************************/
+
+static int dlb_pf_init_driver_state(struct dlb *dlb)
+{
+	mutex_init(&dlb->resource_mutex);
+
+	return 0;

If this can not fail, why is this not just void?

Sure, will change it void.

quoted

diff --git a/drivers/misc/dlb/dlb_resource.h b/drivers/misc/dlb/dlb_resource.h
new file mode 100644
index 000000000000..2229813d9c45
--- /dev/null
+++ b/drivers/misc/dlb/dlb_resource.h

Why do you have lots of little .h files and not just one simple .h file
for the driver?  That makes it much easier to maintain over time, right?

I combined a couple of header files in this version.  dlb_regs.h is pretty big (3640 lines), and is
generated by SW. I will merge other .h files into one.

Thanks
Mike