Hi Chris,
Thanks for this! Some suggestions below, although let me know if any
don't make sense.
On Fri, May 3, 2019 at 3:22 PM Chris Chiu <chiu@xxxxxxxxxxxx> wrote:
>
> Add wireless mode, signal strength level, and rate table index
> to tell the firmware that we need to adjust the tx rate bitmap
> accordingly.
> ---
> .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 45 +++++++++++++++++++
> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 45 ++++++++++++++++++-
> 2 files changed, 89 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index 8828baf26e7b..771f58aa7cae 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -1195,6 +1195,50 @@ struct rtl8723bu_c2h {
>
> struct rtl8xxxu_fileops;
>
> +/*mlme related.*/
> +enum wireless_mode {
> + WIRELESS_MODE_UNKNOWN = 0,
> + //Sub-Element
Run these patches through checkpatch.pl, it'll have some suggestions
to bring the coding style in line, for example not using // style
comments.
> + WIRELESS_MODE_B = BIT(0), // tx: cck only , rx: cck only, hw: cck
> + WIRELESS_MODE_G = BIT(1), // tx: ofdm only, rx: ofdm & cck, hw: cck & ofdm
> + WIRELESS_MODE_A = BIT(2), // tx: ofdm only, rx: ofdm only, hw: ofdm only
> + WIRELESS_MODE_N_24G = BIT(3), // tx: MCS only, rx: MCS & cck, hw: MCS & cck
> + WIRELESS_MODE_N_5G = BIT(4), // tx: MCS only, rx: MCS & ofdm, hw: ofdm only
> + WIRELESS_AUTO = BIT(5),
> + WIRELESS_MODE_AC = BIT(6),
> + WIRELESS_MODE_MAX = (WIRELESS_MODE_B|WIRELESS_MODE_G|WIRELESS_MODE_A|WIRELESS_MODE_N_24G|WIRELESS_MODE_N_5G|WIRELESS_MODE_AC),
> +};
> +
> +/* from rtlwifi/wifi.h */
> +enum ratr_table_mode_new {
> + RATEID_IDX_BGN_40M_2SS = 0,
> + RATEID_IDX_BGN_40M_1SS = 1,
> + RATEID_IDX_BGN_20M_2SS_BN = 2,
> + RATEID_IDX_BGN_20M_1SS_BN = 3,
> + RATEID_IDX_GN_N2SS = 4,
> + RATEID_IDX_GN_N1SS = 5,
> + RATEID_IDX_BG = 6,
> + RATEID_IDX_G = 7,
> + RATEID_IDX_B = 8,
> + RATEID_IDX_VHT_2SS = 9,
> + RATEID_IDX_VHT_1SS = 10,
> + RATEID_IDX_MIX1 = 11,
> + RATEID_IDX_MIX2 = 12,
> + RATEID_IDX_VHT_3SS = 13,
> + RATEID_IDX_BGN_3SS = 14,
> +};
> +
> +#define RTL8XXXU_RATR_STA_INIT 0
> +#define RTL8XXXU_RATR_STA_HIGH 1
> +#define RTL8XXXU_RATR_STA_MID 2
> +#define RTL8XXXU_RATR_STA_LOW 3
> +
> +struct rtl8xxxu_rate_adaptive {
> + u16 wireless_mode;
> + u8 ratr_index;
> + u8 rssi_level; /* INIT, HIGH, MIDDLE, LOW */
> +} __packed;
It would be better/cleaner to avoid storing data in per-device
structures if at all possible.
For wireless_mode, I think you should just call
rtl8xxxu_wireless_mode() every time from rtl8723b_refresh_rate_mask().
The work done there is simple (i.e. it's not expensive to call) and
then we avoid having to store data (which might become stale etc).
For ratr_index, I believe you can just make it a parameter passed to
rtl8xxxu_gen2_update_rate_mask which is the only consumer of this
variable. The two callsites (rtl8xxxu_bss_info_changed and
rtl8723b_refresh_rate_mask) already know which value they want to be
used.
rssi_level is the one that you probably do want to store, since I see
the logic in patch 2 - if the rssi level hasn't changed then you don't
need to set the rate mask again, and that's a good idea since it
reduces bus traffic. You could move this into rtl8xxxu_priv rather
than having its own separate structure.
After making those changes it might even make sense to collapse this
all into a single patch; you can judge!
