Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 28 May 2020 18:19:56 +0200
From:   Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To:     yhchuang@realtek.com
Cc:     kvalo@codeaurora.org, linux-wireless@vger.kernel.org,
        tehuang@realtek.com
Subject: Re: [PATCH 6/7] rtw88: 8821c: add false alarm statistics
Message-ID: <20200528161956.kuqont7heof5ebsn@linutronix.de>
References: <20200520052335.22466-1-yhchuang@realtek.com>
 <20200520052335.22466-7-yhchuang@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8BIT
In-Reply-To: <20200520052335.22466-7-yhchuang@realtek.com>
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 2020-05-20 13:23:34 [+0800], yhchuang@realtek.com wrote:
> From: Tzu-En Huang <tehuang@realtek.com>
> 
> This ops is used to do statistics of false alarm periodically,
> and then fine tune RX initial gain to adaptive different
> circumstance.
> Implement check false alarm statistics for 8821c.

I would use rtw_chip_ops::false_alarm_statistics() or so. Very nice.

> Signed-off-by: Tzu-En Huang <tehuang@realtek.com>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> ---
…
> --- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> @@ -507,6 +507,54 @@ static void rtw8821c_set_tx_power_index(struct rtw_dev *rtwdev)
>  	}
>  }
>  
> +static void rtw8821c_false_alarm_statistics(struct rtw_dev *rtwdev)
> +{
> +	struct rtw_dm_info *dm_info = &rtwdev->dm_info;
> +	u32 cck_enable;
> +	u32 cck_fa_cnt;
> +	u32 ofdm_fa_cnt;
> +	u32 crc32_cnt;
> +	u32 cca32_cnt;
> +
> +	cck_enable = rtw_read32(rtwdev, REG_RXPSEL) & BIT(28);
> +	cck_fa_cnt = rtw_read16(rtwdev, REG_FA_CCK);
> +	ofdm_fa_cnt = rtw_read16(rtwdev, REG_FA_OFDM);
> +
> +	dm_info->cck_fa_cnt = cck_fa_cnt;
> +	dm_info->ofdm_fa_cnt = ofdm_fa_cnt;

> +	dm_info->total_fa_cnt = ofdm_fa_cnt;
> +	dm_info->total_fa_cnt += cck_enable ? cck_fa_cnt : 0;

Since ofdm_fa_cnt isn't used otherwise you could do

	if (cck_enable)
		ofdm_fa_cnt += cck_fa_cnt;
	dm_info->total_fa_cnt = ofdm_fa_cnt;

> +	crc32_cnt = rtw_read32(rtwdev, REG_CRC_CCK);
> +	dm_info->cck_ok_cnt = FIELD_GET(GENMASK(15, 0), crc32_cnt);
> +	dm_info->cck_err_cnt = FIELD_GET(GENMASK(31, 16), crc32_cnt);

a space please

> +	crc32_cnt = rtw_read32(rtwdev, REG_CRC_OFDM);
> +	dm_info->ofdm_ok_cnt = FIELD_GET(GENMASK(15, 0), crc32_cnt);
> +	dm_info->ofdm_err_cnt = FIELD_GET(GENMASK(31, 16), crc32_cnt);

a space please
> +	crc32_cnt = rtw_read32(rtwdev, REG_CRC_HT);
> +	dm_info->ht_ok_cnt = FIELD_GET(GENMASK(15, 0), crc32_cnt);
> +	dm_info->ht_err_cnt = FIELD_GET(GENMASK(31, 16), crc32_cnt);

a space please
> +	crc32_cnt = rtw_read32(rtwdev, REG_CRC_VHT);
> +	dm_info->vht_ok_cnt = FIELD_GET(GENMASK(15, 0), crc32_cnt);
> +	dm_info->vht_err_cnt = FIELD_GET(GENMASK(31, 16), crc32_cnt);
> +
> +	cca32_cnt = rtw_read32(rtwdev, REG_CCA_OFDM);
> +	dm_info->ofdm_cca_cnt = FIELD_GET(GENMASK(31, 16), cca32_cnt);
> +	dm_info->total_cca_cnt = dm_info->ofdm_cca_cnt;
> +	if (cck_enable) {
> +		cca32_cnt = rtw_read32(rtwdev, 0xfcc);

would a register for 0xfcc look good?
			
> +		dm_info->cck_cca_cnt = FIELD_GET(GENMASK(15, 0), cca32_cnt);
> +		dm_info->total_cca_cnt += dm_info->cck_cca_cnt;
> +	}
> +
> +	rtw_write32_set(rtwdev, REG_FAS, BIT(17));
> +	rtw_write32_clr(rtwdev, REG_FAS, BIT(17));
> +	rtw_write32_clr(rtwdev, REG_RXDESC, BIT(15));
> +	rtw_write32_set(rtwdev, REG_RXDESC, BIT(15));
> +	rtw_write32_set(rtwdev, 0xb58, BIT(0));
> +	rtw_write32_clr(rtwdev, 0xb58, BIT(0));
> +}
> +
>  static struct rtw_pwr_seq_cmd trans_carddis_to_cardemu_8821c[] = {
>  	{0x0086,
>  	 RTW_PWR_CUT_ALL_MSK,

Sebastian