Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 15AEA605A0
From:   Kalle Valo <kvalo@codeaurora.org>
To:     <yhchuang@realtek.com>
Cc:     <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH 07/11] rtw88: 8822c: use more accurate ofdm fa counting
References: <1560321917-17751-1-git-send-email-yhchuang@realtek.com>
        <1560321917-17751-8-git-send-email-yhchuang@realtek.com>
Date:   Wed, 12 Jun 2019 10:42:06 +0300
In-Reply-To: <1560321917-17751-8-git-send-email-yhchuang@realtek.com>
        (yhchuang@realtek.com's message of "Wed, 12 Jun 2019 14:45:13 +0800")
Message-ID: <875zpbgshd.fsf@kamboji.qca.qualcomm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

<yhchuang@realtek.com> writes:

> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> 8822c used to count OFDM FA count by subtracting tx count from FA count.
> But it need to substract more counters to be accurate.
>
> However, we can count it by adding up all of the FA counters we want.
> And it is simpler to add than list all of the components to substract.
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>

[...]

> --- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> @@ -1491,13 +1491,29 @@ static void rtw8822c_false_alarm_statistics(struct rtw_dev *rtwdev)
>  	u32 cck_enable;
>  	u32 cck_fa_cnt;
>  	u32 ofdm_fa_cnt;
> -	u32 ofdm_tx_counter;
> +	u32 ofdm_fa_cnt1, ofdm_fa_cnt2, ofdm_fa_cnt3, ofdm_fa_cnt4, ofdm_fa_cnt5;
> +	u16 parity_fail, rate_illegal, crc8_fail, mcs_fail, sb_search_fail,
> +	    fast_fsync, crc8_fail_vhta, mcs_fail_vht;
>  
>  	cck_enable = rtw_read32(rtwdev, REG_ENCCK) & BIT_CCK_BLK_EN;
>  	cck_fa_cnt = rtw_read16(rtwdev, REG_CCK_FACNT);
> -	ofdm_fa_cnt = rtw_read16(rtwdev, REG_OFDM_FACNT);
> -	ofdm_tx_counter = rtw_read16(rtwdev, REG_OFDM_TXCNT);
> -	ofdm_fa_cnt -= ofdm_tx_counter;
> +
> +	ofdm_fa_cnt1 = rtw_read32(rtwdev, REG_OFDM_FACNT1);
> +	ofdm_fa_cnt2 = rtw_read32(rtwdev, REG_OFDM_FACNT2);
> +	ofdm_fa_cnt3 = rtw_read32(rtwdev, REG_OFDM_FACNT3);
> +	ofdm_fa_cnt4 = rtw_read32(rtwdev, REG_OFDM_FACNT4);
> +	ofdm_fa_cnt5 = rtw_read32(rtwdev, REG_OFDM_FACNT5);
> +
> +	parity_fail = (ofdm_fa_cnt1 & 0xffff0000) >> 16;
> +	rate_illegal = (ofdm_fa_cnt2 & 0xffff);
> +	crc8_fail = (ofdm_fa_cnt2 & 0xffff0000) >> 16;
> +	crc8_fail_vhta = (ofdm_fa_cnt3 & 0xffff);
> +	mcs_fail = (ofdm_fa_cnt4 & 0xffff);
> +	mcs_fail_vht = (ofdm_fa_cnt4 & 0xffff0000) >> 16;
> +	fast_fsync = (ofdm_fa_cnt5 & 0xffff);
> +	sb_search_fail = (ofdm_fa_cnt5 & 0xffff0000) >> 16;

No magic numbers, please. IMO the cleanest way is to use create defines
using GENMASK() and FIELD_GET(), that way the values are proper
documented. An example (but not compile tested so can be buggy!):

#define OFDM_FA_CNT1_PARITY_FAIL GENMASK(31, 16)

parity_fail = FIELD_GET(OFDM_FA_CNT1_PARITY_FAIL, ofdm_fa_cnt1);

-- 
Kalle Valo