Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Manivannan Sadhasivam' <manivannan.sadhasivam@linaro.org>
CC:     "mhi@lists.linux.dev" <mhi@lists.linux.dev>,
        "quic_hemantk@quicinc.com" <quic_hemantk@quicinc.com>,
        "quic_bbhatt@quicinc.com" <quic_bbhatt@quicinc.com>,
        "quic_jhugo@quicinc.com" <quic_jhugo@quicinc.com>,
        "vinod.koul@linaro.org" <vinod.koul@linaro.org>,
        "bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
        "dmitry.baryshkov@linaro.org" <dmitry.baryshkov@linaro.org>,
        "quic_vbadigan@quicinc.com" <quic_vbadigan@quicinc.com>,
        "quic_cang@quicinc.com" <quic_cang@quicinc.com>,
        "quic_skananth@quicinc.com" <quic_skananth@quicinc.com>,
        "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "elder@linaro.org" <elder@linaro.org>
Subject: RE: [PATCH v4 05/27] bus: mhi: Use bitfield operations for handling
 DWORDs of ring elements
Thread-Topic: [PATCH v4 05/27] bus: mhi: Use bitfield operations for handling
 DWORDs of ring elements
Thread-Index: AQHYLKD5KBrT0mkrcEuEmuVBLjzK3Kyo+w/ggAAO0ACAAAu88A==
Date:   Mon, 28 Feb 2022 15:40:56 +0000
Message-ID: <8433702975794b5389563393bf7bc405@AcuMS.aculab.com>
References: <20220228124344.77359-1-manivannan.sadhasivam@linaro.org>
 <20220228124344.77359-6-manivannan.sadhasivam@linaro.org>
 <90b1d3954b8c4157a4045db82b562271@AcuMS.aculab.com>
 <20220228144336.GB12451@workstation>
In-Reply-To: <20220228144336.GB12451@workstation>
Accept-Language: en-GB, en-US
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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)