Re: [PATCH v2 1/5] rtw88: 8821c: add cck pd settings

[Kalle Valo <kvalo@codeaurora.org>]

<yhchuang@realtek.com> writes:

> From: Tzu-En Huang <tehuang@realtek.com>
>
> CCK PD can reduce the number of false alarm of the CCK rates.
> It dynamically adjusts the power threshold and CS ratio.
> The values are compared to the values of the previous level, if
> the level is changed, set new values of power threshold and CS
> ratio.
>
> Implement rtw_chip_ops::cck_pd_set() for 8821c.
>
> 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
> @@ -102,6 +102,7 @@ static void rtw8821c_phy_set_param(struct rtw_dev *rtwdev)
>  	rtwdev->chip->ch_param[2] = rtw_read32_mask(rtwdev, REG_TXFILTER, MASKDWORD);
>  
>  	rtw_phy_init(rtwdev);
> +	rtwdev->dm_info.cck_pd_default = rtw_read8(rtwdev, REG_CSRATIO) & 0x1f;
>  }
>  
>  static int rtw8821c_mac_init(struct rtw_dev *rtwdev)
> @@ -596,6 +597,29 @@ static void rtw8821c_phy_calibration(struct rtw_dev *rtwdev)
>  	rtw8821c_do_iqk(rtwdev);
>  }
>  
> +static void rtw8821c_phy_cck_pd_set(struct rtw_dev *rtwdev, u8 new_lvl)
> +{
> +	struct rtw_dm_info *dm_info = &rtwdev->dm_info;
> +	u8 pd[CCK_PD_LV_MAX] = {3, 7, 13, 13, 13};
> +
> +	if (dm_info->min_rssi > 60) {
> +		new_lvl = 4;
> +		pd[4] = 0x1d;
> +		goto set_cck_pd;
> +	}
> +
> +	if (dm_info->cck_pd_lv[RTW_CHANNEL_WIDTH_20][RF_PATH_A] == new_lvl)
> +		return;
> +
> +	dm_info->cck_fa_avg = CCK_FA_AVG_RESET;
> +
> +set_cck_pd:
> +	dm_info->cck_pd_lv[RTW_CHANNEL_WIDTH_20][RF_PATH_A] = new_lvl;
> +	rtw_write32_mask(rtwdev, REG_PWRTH, 0x3f0000, pd[new_lvl]);
> +	rtw_write32_mask(rtwdev, REG_PWRTH2, 0x1f0000,
> +			 dm_info->cck_pd_default + new_lvl * 2);
> +}

I'm starting to see the trend of using magic values in rtw88 on the rise
again. Please be careful with this, the source code should not be full
of undocument values. In some special cases (eg calibration data etc)
using undocumented values is ok, but most of cases should have proper
defines for documenting what's happening.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches