Re: [PATCH v9 04/14] rtw88: trx files

[Kalle Valo <kvalo@codeaurora.org>]

<yhchuang@realtek.com> writes:

> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> trx files for Realtek 802.11ac wireless network chips
>
> Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>

[...]

> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/rx.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> +/* Copyright(c) 2018-2019  Realtek Corporation
> + */
> +
> +#ifndef __RTW_RX_H_
> +#define __RTW_RX_H_
> +
> +#define GET_RX_DESC_PHYST(rxdesc)                                              \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x00), BIT(26))
> +#define GET_RX_DESC_ICV_ERR(rxdesc)                                            \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x00), BIT(15))
> +#define GET_RX_DESC_CRC32(rxdesc)                                              \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x00), BIT(14))
> +#define GET_RX_DESC_SWDEC(rxdesc)                                              \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x00), BIT(27))
> +#define GET_RX_DESC_C2H(rxdesc)                                                \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x02), BIT(28))
> +#define GET_RX_DESC_PKT_LEN(rxdesc)                                            \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x00), GENMASK(13, 0))
> +#define GET_RX_DESC_DRV_INFO_SIZE(rxdesc)                                      \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x00), GENMASK(19, 16))
> +#define GET_RX_DESC_SHIFT(rxdesc)                                              \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x00), GENMASK(25, 24))
> +#define GET_RX_DESC_RX_RATE(rxdesc)                                            \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x03), GENMASK(6, 0))
> +#define GET_RX_DESC_MACID(rxdesc)                                              \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x01), GENMASK(6, 0))
> +#define GET_RX_DESC_PPDU_CNT(rxdesc)                                           \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x02), GENMASK(30, 29))
> +#define GET_RX_DESC_TSFL(rxdesc)                                               \
> +	le32_get_bits(*((__le32 *)(rxdesc) + 0x05), GENMASK(31, 0))

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 Valo