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. -- Kalle Valo
