RE: [PATCH 2/4] wifi: rtw88: rtw8822c: use sign_extend32 for DAC IQ validation

[Ping-Ke Shih]

Arsenii Pashchenko <ulijg308@xxxxxxxxx> wrote:
> Leverage the kernel's native sign_extend32() macro to properly
> interpret the 10-bit raw values from hardware registers as signed
> 32-bit (s10) integers covering the [-512, 511] range.

New API FIELD_GET_SIGNED() is introduced. I think this is suitable
to this case.

> 
> This allows us to completely rewrite rtw8822c_dac_iq_check() to use
> a simple, clean amplitude boundary check instead of complex, nested
> unsigned boundary conditions. Drop the temporary u32 casts inside
> the check and sampling routines.
> 
> Signed-off-by: Arsenii Pashchenko <ulijg308@xxxxxxxxx>
> ---
>  drivers/net/wireless/realtek/rtw88/rtw8822c.c | 24 +++++++++----------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> index 896e3e7b9..d06986d09 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> @@ -3,6 +3,7 @@
>   */
> 
>  #include <linux/module.h>
> +#include <linux/bitops.h>

I guess you need this because of sign_extend32(). Have you tried to build
without this? I feel it is existing in include chain already.

>  #include "main.h"
>  #include "coex.h"
>  #include "fw.h"
> @@ -277,18 +278,15 @@ static u32 rtw8822c_get_path_read_addr(u8 path)
>         return base_addr;
>  }
> 
> -static bool rtw8822c_dac_iq_check(struct rtw_dev *rtwdev, s32 value_s32)
> +static bool rtw8822c_dac_iq_check(struct rtw_dev *rtwdev, s32 value)
>  {
> -       u32 value = (u32)value_s32;
> -       bool ret = true;
> 
> -       if ((value >= 0x200 && (0x400 - value) > 0x64) ||
> -           (value < 0x200 && value > 0x64)) {
> -               ret = false;
> +       if (value > 100 || value < -100) {

The change is identical?

>                 rtw_dbg(rtwdev, RTW_DBG_RFK, "[DACK] Error overflow\n");
> +               return false;
>         }
> 
> -       return ret;
> +       return true;
>  }
> 
>  static void rtw8822c_dac_cal_iq_sample(struct rtw_dev *rtwdev, s32 *iv, s32 *qv)
> @@ -299,8 +297,8 @@ static void rtw8822c_dac_cal_iq_sample(struct rtw_dev *rtwdev, s32 *iv, s32 *qv)
>         while (i < DACK_SN_8822C && cnt < 10000) {
>                 cnt++;
>                 temp = rtw_read32_mask(rtwdev, 0x2dbc, 0x3fffff);
> -               iv[i] = (s32)((temp & 0x3ff000) >> 12);
> -               qv[i] = (s32)(temp & 0x3ff);
> +               iv[i] = sign_extend32((temp & 0x3ff000) >> 12, 9);
> +               qv[i] = sign_extend32(temp & 0x3ff, 9);

FIELD_GET_SIGNED()

> 
>                 if (rtw8822c_dac_iq_check(rtwdev, iv[i]) &&
>                     rtw8822c_dac_iq_check(rtwdev, qv[i]))
> @@ -352,11 +350,11 @@ static void rtw8822c_dac_cal_iq_search(struct rtw_dev *rtwdev,
> 
>                 if (i_delta > 5 || q_delta > 5) {
>                         temp = rtw_read32_mask(rtwdev, 0x2dbc, 0x3fffff);
> -                       iv[0] = (s32)((temp & 0x3ff000) >> 12);
> -                       qv[0] = (s32)(temp & 0x3ff);
> +                       iv[0] = sign_extend32((temp & 0x3ff000) >> 12, 9);
> +                       qv[0] = sign_extend32(temp & 0x3ff, 9);

FIELD_GET_SIGNED()

>                         temp = rtw_read32_mask(rtwdev, 0x2dbc, 0x3fffff);
> -                       iv[DACK_SN_8822C - 1] = (s32)((temp & 0x3ff000) >> 12);
> -                       qv[DACK_SN_8822C - 1] = (s32)(temp & 0x3ff);
> +                       iv[DACK_SN_8822C - 1] = sign_extend32((temp & 0x3ff000) >> 12, 9);
> +                       qv[DACK_SN_8822C - 1] = sign_extend32(temp & 0x3ff, 9);

FIELD_GET_SIGNED()

Please use real hardware to verify the values.

>                 } else {
>                         break;
>                 }
> --
> 2.55.0