From: 'Manivannan Sadhasivam'
> Sent: 28 February 2022 14:44
>
> On Mon, Feb 28, 2022 at 02:00:07PM +0000, David Laight wrote:
> > From: Manivannan Sadhasivam
> > > Sent: 28 February 2022 12:43
> > >
> > > Instead of using the hardcoded bits in DWORD definitions, let's use the
> > > bitfield operations to make it more clear how the DWORDs are structured.
> >
> > That all makes it as clear as mud.
>
> It depends on how you see it ;)
>
> For instance,
>
> #define MHI_TRE_GET_CMD_TYPE(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 16) & 0xFF)
>
> vs
>
> #define MHI_TRE_GET_CMD_TYPE(tre) (FIELD_GET(GENMASK(23, 16), (MHI_TRE_GET_DWORD(tre, 1))))
>
> The later one makes it more obvious that the "type" field resides between bit 23
> and 16. Plus it avoids the extra masking.
No, (x >> 16) & 0xff is obviously bits 23 to 16.
I can guess or try to remember what FIELD_GET() and GENMASK() do
but it is really hard work.
Both lines are actually too long to read - especially given the
number of times they are repeated with very minor changes.
I actually wonder if you shouldn't just have a struct like:
struct mhi_cmd {
__le64 address;
__le16 len;
u8 state;
u8 vid;
__le16 xxx; /* I can't see what this is */
u8 chid;
u8 cmd;
};
although you might need the odd anonymous union/struct
to get the overlays in.
Even using something like:
#define MAKE_WORD0(len, state, vid) (htole16(len) | state << 16 | vid << 16)
would make for easier reading.
Oh yes, there are some 64bit fields here.
So a 'word' is 64 bits, so a 'double word' would be 128 bits!
WTF is a DWORD anyway????
Are you going to start using DWORD_PTR as well ?????
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
