Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 2857E608FF
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Larry Finger <Larry.Finger@lwfinger.net>
Cc:     yhchuang@realtek.com, linux-wireless@vger.kernel.org,
        johannes@sipsolutions.net, pkshih@realtek.com, tehuang@realtek.com,
        sgruszka@redhat.com, briannorris@chromium.org,
        gregkh@linuxfoundation.org
Subject: Re: [PATCH v9 04/14] rtw88: trx files
References: <1555653004-1795-1-git-send-email-yhchuang@realtek.com>
        <1555653004-1795-5-git-send-email-yhchuang@realtek.com>
        <87bm0npsp7.fsf@kamboji.qca.qualcomm.com>
        <04c534eb-af32-adfc-62bb-90d83526af1a@lwfinger.net>
Date:   Wed, 01 May 2019 21:30:10 +0300
In-Reply-To: <04c534eb-af32-adfc-62bb-90d83526af1a@lwfinger.net> (Larry
        Finger's message of "Wed, 1 May 2019 13:18:10 -0500")
Message-ID: <87a7g6ni31.fsf@kamboji.qca.qualcomm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Larry Finger <Larry.Finger@lwfinger.net> writes:

> On 4/30/19 7:45 AM, Kalle Valo wrote:
>> I'm not really fond of these "byte macros" or whatever they should be
>> called, you use these a lot in rtw88 but I have seen the same usage also
>> other drivers. The upstream way of doing this is to create a struct,
>> which also acts as a documentation, and you can pass it around different
>> functions. And the GENMASK()s are defined close the struct.
>>
>> Also you could change these defines to static inline functions, which
>> take the struct as a pointer, and that you get type checking from the
>> compiler. And that way you would get rid of that ugly casting as well.
>
> Kalle,
>
> I have never been a fan of those complicated macros dating back to the
> day that I had to make them endian correct. Without Sparse, I never
> would have made it.
>
> I understand your comment about making them be static inline
> functions, but I am intrigued be the struct method. Is there something
> other than bit field constructions that could accomplish this?

My comment was about handling firmware commands and events as a byte
array, not about bitfields. So that instead of accessing 'index + 1' and
'index + 4' you should create a proper struct for the command and access
it using 'cmd->foo' and 'cmd->bar'. Sure, bitfields you still need to
access using FIELD_GET() or similar but having a struct for commands is
a lot cleaner approach. And most upstream drivers do this: ath10k,
ath6kl, iwlwifi, p54 and whatnot.

Sorry, no time now to explain further now but, if needed, I can provide
a better example tomorrow.

> If not, then this method would be very difficult to implement. My
> basis is an E-mail by Linus that said it was almost impossible to get
> this type of construct to be endian correct. If he thinks it is
> difficult, then I know not to tackle it. :)

Could you please point Linus' email about this? I would like to
understand more, I didn't understand your comment.

-- 
Kalle Valo