On 15/01/2020 21:31, Jason Gunthorpe wrote:
> 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
I'll take a look, thanks.
