> -----Original Message-----
> From: Greg KH <[email protected]>
>
> On Wed, Feb 10, 2021 at 11:54:06AM -0600, Mike Ximing Chen wrote:
> > +
> > +#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?
>
>
> > -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.
> > +/*******************************/
> > +/****** 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.
>
> > 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
