Larry Finger <Larry.Finger@xxxxxxxxxxxx> writes:
> The driver uses complicated macros to set parts of word 0 of the TX and RX
> descriptors. These are changed into inline routines.
>
> Signed-off-by: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
> ---
>
> Kalle,
>
> Based on your comment on how much you dislike those "byte macros", I have
> converted a few of them from rtl8821ae into static inline functions.
>
> Is this what you had in mind, and do you consider these changes to
> improve the code?
[...]
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
> @@ -14,25 +14,67 @@
> #define USB_HWDESC_HEADER_LEN 40
> #define CRCLENGTH 4
>
> -#define SET_TX_DESC_PKT_SIZE(__pdesc, __val) \
> - SET_BITS_TO_LE_4BYTE(__pdesc, 0, 16, __val)
> -#define SET_TX_DESC_OFFSET(__pdesc, __val) \
> - SET_BITS_TO_LE_4BYTE(__pdesc, 16, 8, __val)
> -#define SET_TX_DESC_BMC(__pdesc, __val) \
> - SET_BITS_TO_LE_4BYTE(__pdesc, 24, 1, __val)
> -#define SET_TX_DESC_HTC(__pdesc, __val) \
> - SET_BITS_TO_LE_4BYTE(__pdesc, 25, 1, __val)
> -#define SET_TX_DESC_LAST_SEG(__pdesc, __val) \
> - SET_BITS_TO_LE_4BYTE(__pdesc, 26, 1, __val)
> -#define SET_TX_DESC_FIRST_SEG(__pdesc, __val) \
> - SET_BITS_TO_LE_4BYTE(__pdesc, 27, 1, __val)
> -#define SET_TX_DESC_LINIP(__pdesc, __val) \
> - SET_BITS_TO_LE_4BYTE(__pdesc, 28, 1, __val)
> -#define SET_TX_DESC_OWN(__pdesc, __val) \
> - SET_BITS_TO_LE_4BYTE(__pdesc, 31, 1, __val)
> -
> -#define GET_TX_DESC_OWN(__pdesc) \
> - LE_BITS_TO_4BYTE(__pdesc, 31, 1)
> +/* Set packet size (16 bits) in TX descriptor word 0 */
> +static inline void set_tx_desc_pkt_size(__le32 *__pdesc, u16 __val)
> +{
> + *__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(0, 15)) |
> + __val);
> +}
This was not exactly what I had mind. My point was that the firmware
command or event should be handled as a complete structure, not parsed
one (or four) bytes at a time. To show what I mean here's a random
example from iwlwifi:
struct iwl_alive_resp {
u8 ucode_minor;
u8 ucode_major;
__le16 reserved1;
u8 sw_rev[8];
u8 ver_type;
u8 ver_subtype; /* not "9" for runtime alive */
__le16 reserved2;
__le32 log_event_table_ptr; /* SRAM address for event log */
__le32 error_event_table_ptr; /* SRAM address for error log */
__le32 timestamp;
__le32 is_valid;
} __packed;
This a nice, clean and robust way both use AND document an event coming
from the firmware. And the driver can pass around 'struct iwl_alive_res
*resp' pointer to other functions to make it easy to understand what
event the buffer contains. Compared to rtlwifi where there's just 'u8
buf[]' and nobody has any clue what it contains, and need to spend at
least five minutes figuring that out everytime they are looking at a
function.
But to be honest I'm not sure if it's worth trying to cleanup rtlwifi,
it really would need a full rewrite to become a clean driver. IMHO much
better to put more effort on rtl8xxxxu and rtw88, which both already are
clean drivers.
--
Kalle Valo
