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;
Subject: Re: [PATCH v9 04/14] rtw88: trx files
To:     Kalle Valo <kvalo@codeaurora.org>
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
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>
 <87a7g6ni31.fsf@kamboji.qca.qualcomm.com>
From:   Larry Finger <Larry.Finger@lwfinger.net>
Message-ID: <7a03880d-0fe9-0fdb-7df9-18560ac000c2@lwfinger.net>
Date:   Wed, 1 May 2019 15:09:39 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <87a7g6ni31.fsf@kamboji.qca.qualcomm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 5/1/19 1:30 PM, Kalle Valo wrote:
> 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.
> 

Do not worry about giving a further answer. I'll read those other drivers and 
figure out what they are doing.

The email exchange I saw was at https://yarchive.net/comp/linux/bitfields.html.

Larry