On Tue, Jan 14, 2020 at 10:57:01AM +0200, Gal Pressman wrote:
> diff --git a/drivers/infiniband/hw/efa/efa_common_defs.h b/drivers/infiniband/hw/efa/efa_common_defs.h
> index c559ec08898e..845ea5ca9388 100644
> +++ b/drivers/infiniband/hw/efa/efa_common_defs.h
> @@ -9,6 +9,12 @@
> #define EFA_COMMON_SPEC_VERSION_MAJOR 2
> #define EFA_COMMON_SPEC_VERSION_MINOR 0
>
> +#define EFA_GET(ptr, type) \
> + ((*(ptr) & type##_MASK) >> type##_SHIFT)
> +
> +#define EFA_SET(ptr, type, value) \
> + ({ *(ptr) |= ((value) << type##_SHIFT) & type##_MASK; })
> +
Why not just GENMASK properly? You don't need MASK and SHIFT, it is
supposed to be written like:
#define EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN GENMASK(8,7)
*ptr |= FIELD_PREP(val, EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN)
FIELD_PREP automatically deduces the correct shift.
And it would be much nicer if this had some type safety.
You should review the stuff Leon is prepping here:
https://github.com/jgunthorpe/linux/commit/453e85ed7aa46db22d8be16f9b0c88b17b8968af
Which is basically doing the same sorts of things, but with better
type safety and no need for the various structs
Jason
