RE: [PATCH v4 05/27] bus: mhi: Use bitfield operations for handling DWORDs of ring elements

[David Laight <David.Laight@ACULAB.COM>]

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)