Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Kalle Valo <kvalo@kernel.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v2 04/16] rtw89: 8852c: rfk: add TSSI
References: <20220429071809.32104-1-pkshih@realtek.com>
        <20220429071809.32104-5-pkshih@realtek.com>
Date:   Mon, 02 May 2022 09:39:07 +0300
In-Reply-To: <20220429071809.32104-5-pkshih@realtek.com> (Ping-Ke Shih's
        message of "Fri, 29 Apr 2022 15:17:57 +0800")
Message-ID: <871qxccub8.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Ping-Ke Shih <pkshih@realtek.com> writes:

> TSSI is transmitter signal strength indication, which is a close-loop
> hardware circuit to feedback actual transmitting power as a reference for
> next transmission.
>
> When we setup channel to connect an AP, it does full calibration. When
> switching bands or channels, it needs to reset hardware status to prevent
> use wrong feedback of previous transmission.
>
> To do TX power compensation reflecting current temperature, it loads tables
> of compensation values into registers according to channel and band group.
>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

> +static void _tssi_set_tmeter_tbl(struct rtw89_dev *rtwdev, enum rtw89_phy_idx phy,
> +				 enum rtw89_rf_path path)
> +{
> +#define __get_val(ptr, idx)				\
> +({							\
> +	s8 *__ptr = (ptr);				\
> +	u8 __idx = (idx), __i, __v;			\
> +	u32 __val = 0;					\
> +	for (__i = 0; __i < 4; __i++) {			\
> +		__v = (__ptr[__idx + __i]);		\
> +		__val |= (__v << (8 * __i));		\
> +	}						\
> +	__val;						\
> +})

The convention is to use upper case for macros. And maybe __get_val() is
too generic name, RTW89_GET_VAL() or something like that?

> +static void _tssi_set_efuse_to_de(struct rtw89_dev *rtwdev,
> +				  enum rtw89_phy_idx phy)
> +{
> +#define __DE_MASK 0x003ff000
> +	struct rtw89_tssi_info *tssi_info = &rtwdev->tssi;
> +	static const u32 r_cck_long[RF_PATH_NUM_8852C] = {0x5858, 0x7858};
> +	static const u32 r_cck_short[RF_PATH_NUM_8852C] = {0x5860, 0x7860};
> +	static const u32 r_mcs_20m[RF_PATH_NUM_8852C] = {0x5838, 0x7838};
> +	static const u32 r_mcs_40m[RF_PATH_NUM_8852C] = {0x5840, 0x7840};
> +	static const u32 r_mcs_80m[RF_PATH_NUM_8852C] = {0x5848, 0x7848};
> +	static const u32 r_mcs_80m_80m[RF_PATH_NUM_8852C] = {0x5850, 0x7850};
> +	static const u32 r_mcs_5m[RF_PATH_NUM_8852C] = {0x5828, 0x7828};
> +	static const u32 r_mcs_10m[RF_PATH_NUM_8852C] = {0x5830, 0x7830};
> +	u8 ch = rtwdev->hal.current_channel;
> +	u8 gidx;
> +	s8 ofdm_de;
> +	s8 trim_de;
> +	s32 val;
> +	u32 i, path = RF_PATH_A, path_max = RF_PATH_NUM_8852C;
> +
> +	rtw89_debug(rtwdev, RTW89_DBG_TSSI, "[TSSI][TRIM]: phy=%d ch=%d\n",
> +		    phy, ch);
> +
> +	if (rtwdev->dbcc_en) {
> +		if (phy == RTW89_PHY_0) {
> +			path = RF_PATH_A;
> +			path_max = RF_PATH_B;
> +		} else if (phy == RTW89_PHY_1) {
> +			path = RF_PATH_B;
> +			path_max = RF_PATH_NUM_8852C;
> +		}
> +	}
> +
> +	for (i = path; i < path_max; i++) {
> +		gidx = _tssi_get_cck_group(rtwdev, ch);
> +		trim_de = _tssi_get_ofdm_trim_de(rtwdev, phy, i);
> +		val = tssi_info->tssi_cck[i][gidx] + trim_de;
> +
> +		rtw89_debug(rtwdev, RTW89_DBG_TSSI,
> +			    "[TSSI][TRIM]: path=%d cck[%d]=0x%x trim=0x%x\n",
> +			    i, gidx, tssi_info->tssi_cck[i][gidx], trim_de);
> +
> +		rtw89_phy_write32_mask(rtwdev, r_cck_long[i], __DE_MASK, val);
> +		rtw89_phy_write32_mask(rtwdev, r_cck_short[i], __DE_MASK, val);
> +
> +		rtw89_debug(rtwdev, RTW89_DBG_TSSI,
> +			    "[TSSI] Set TSSI CCK DE 0x%x[21:12]=0x%x\n",
> +			    r_cck_long[i],
> +			    rtw89_phy_read32_mask(rtwdev, r_cck_long[i],
> +						  __DE_MASK));
> +
> +		ofdm_de = _tssi_get_ofdm_de(rtwdev, phy, i);
> +		trim_de = _tssi_get_ofdm_trim_de(rtwdev, phy, i);
> +		val = ofdm_de + trim_de;
> +
> +		rtw89_debug(rtwdev, RTW89_DBG_TSSI,
> +			    "[TSSI][TRIM]: path=%d mcs=0x%x trim=0x%x\n",
> +			    i, ofdm_de, trim_de);
> +
> +		rtw89_phy_write32_mask(rtwdev, r_mcs_20m[i], __DE_MASK, val);
> +		rtw89_phy_write32_mask(rtwdev, r_mcs_40m[i], __DE_MASK, val);
> +		rtw89_phy_write32_mask(rtwdev, r_mcs_80m[i], __DE_MASK, val);
> +		rtw89_phy_write32_mask(rtwdev, r_mcs_80m_80m[i], __DE_MASK, val);
> +		rtw89_phy_write32_mask(rtwdev, r_mcs_5m[i], __DE_MASK, val);
> +		rtw89_phy_write32_mask(rtwdev, r_mcs_10m[i], __DE_MASK, val);
> +
> +		rtw89_debug(rtwdev, RTW89_DBG_TSSI,
> +			    "[TSSI] Set TSSI MCS DE 0x%x[21:12]=0x%x\n",
> +			    r_mcs_20m[i],
> +			    rtw89_phy_read32_mask(rtwdev, r_mcs_20m[i],
> +						  __DE_MASK));
> +	}
> +#undef __DE_MASK
> +}

That __DE_MASK is odd and even more so when undeffing it. I would create
a proper name for it and also group all such register and mask
definitions to the beginning of the .c file.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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