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