RE: [PATCH rtw-next v6 2/3] wifi: rtw88: rtw8822c: Convert DAC IQ calibration path to signed math

Arsenii Pashchenko <[email protected]> wrote:
> Convert the temporary stack arrays 'iv' and 'qv' from u32 to s32.
> Update all internal function signatures to accept s32 pointers and
> handle signed values natively.
> 
> Use sign_extend32() combined with FIELD_GET() to properly interpret
> the 10-bit hardware values as signed integers. 

Forgot to update commit message as FIELD_GET_SIGNED()...

> This allows flattening
> the complex nested unsigned boundary conditions in
> rtw8822c_dac_iq_check() into a simple amplitude boundary check. Ensure
> rtw8822c_dac_iq_offset() properly maps the signed average back to a
> 10-bit unsigned format expected by the hardware registers.
> 
> Signed-off-by: Arsenii Pashchenko <[email protected]>
> ---

[...]

> @@ -207,30 +172,16 @@ static void rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, u32 *iv, u32 *qv)
>         }
>  }
> 
> -static void rtw8822c_dac_iq_offset(struct rtw_dev *rtwdev, u32 *vec, u32 *val)
> +static u32 rtw8822c_dac_iq_offset(struct rtw_dev *rtwdev, s32 *vec)
>  {
> -       u32 p, m, t, i;
> +       s32 sum = 0;
> 
> -       m = 0;
> -       p = 0;
> -       for (i = 10; i < DACK_SN_8822C - 10; i++) {
> -               if (vec[i] > 0x200)
> -                       m = (0x400 - vec[i]) + m;
> -               else
> -                       p = vec[i] + p;
> -       }
> +       for (u32 i = 10; i < DACK_SN_8822C - 10; i++)
> +               sum += vec[i];
> 
> -       if (p > m) {
> -               t = p - m;
> -               t = t / (DACK_SN_8822C - 20);
> -       } else {
> -               t = m - p;
> -               t = t / (DACK_SN_8822C - 20);
> -               if (t != 0x0)
> -                       t = 0x400 - t;
> -       }
> +       s32 avg = sum / (DACK_SN_8822C - 20);

No declaration in the middle.

> 
> -       *val = t;
> +       return avg >= 0 ? (u32)avg : (u32)(0x400 + avg);

Maybe, we can give an example to understand easier, like

  avg = -1, return 0x3FF. (signed 10 bits value)


>  }
> 
>  static u32 rtw8822c_get_path_write_addr(u8 path)
> @@ -271,20 +222,18 @@ static u32 rtw8822c_get_path_read_addr(u8 path)
>         return base_addr;
>  }
> 
> -static bool rtw8822c_dac_iq_check(struct rtw_dev *rtwdev, u32 value)
> +static bool rtw8822c_dac_iq_check(struct rtw_dev *rtwdev, s32 value)
>  {
> -       bool ret = true;
> 
> -       if ((value >= 0x200 && (0x400 - value) > 0x64) ||
> -           (value < 0x200 && value > 0x64)) {
> -               ret = false;
> +       if (value > 100 || value < -100) {
>                 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, u32 *iv, u32 *qv)
> +static void rtw8822c_dac_cal_iq_sample(struct rtw_dev *rtwdev, s32 *iv, s32 *qv)
>  {
>         u32 temp;
>         int i = 0, cnt = 0;
> @@ -292,8 +241,8 @@ static void rtw8822c_dac_cal_iq_sample(struct rtw_dev *rtwdev, u32 *iv, u32 *qv)
>         while (i < DACK_SN_8822C && cnt < 10000) {
>                 cnt++;
>                 temp = rtw_read32(rtwdev, 0x2dbc);
> -               iv[i] = FIELD_GET(RTW8822C_DAC_IV_MASK, temp);
> -               qv[i] = FIELD_GET(RTW8822C_DAC_QV_MASK, temp);
> +               iv[i] = FIELD_GET_SIGNED(RTW8822C_DAC_IV_MASK, temp);
> +               qv[i] = FIELD_GET_SIGNED(RTW8822C_DAC_QV_MASK, temp);
> 
>                 if (rtw8822c_dac_iq_check(rtwdev, iv[i]) &&
>                     rtw8822c_dac_iq_check(rtwdev, qv[i]))
> @@ -302,67 +251,63 @@ static void rtw8822c_dac_cal_iq_sample(struct rtw_dev *rtwdev, u32 *iv, u32 *qv)
>  }
> 
>  static void rtw8822c_dac_cal_iq_search(struct rtw_dev *rtwdev,
> -                                      u32 *iv, u32 *qv,
> +                                      s32 *iv, s32 *qv,
>                                        u32 *i_value, u32 *q_value)
>  {
> -       u32 i_max = 0, q_max = 0, i_min = 0, q_min = 0;
> +       s32 i_max = 0, q_max = 0, i_min = 0, q_min = 0;
>         u32 i_delta, q_delta;
>         u32 temp;
> -       int i, cnt = 0;
> +       u32 i, cnt = 0;

Why did you need changing 'int' to 'u32'?

> 
>         do {
>                 i_min = iv[0];
>                 i_max = iv[0];
>                 q_min = qv[0];
>                 q_max = qv[0];
> -               for (i = 0; i < DACK_SN_8822C; i++) {
> -                       rtw8822c_rf_minmax_cmp(rtwdev, iv[i], &i_min, &i_max);
> -                       rtw8822c_rf_minmax_cmp(rtwdev, qv[i], &q_min, &q_max);
> +               for (i = 1; i < DACK_SN_8822C; i++) {
> +                       if (iv[i] < i_min)
> +                               i_min = iv[i];
> +                       if (iv[i] > i_max)
> +                               i_max = iv[i];
> +                       if (qv[i] < q_min)
> +                               q_min = qv[i];
> +                       if (qv[i] > q_max)
> +                               q_max = qv[i];
>                 }
> 
> -               if (i_max < 0x200 && i_min < 0x200)
> -                       i_delta = i_max - i_min;
> -               else if (i_max >= 0x200 && i_min >= 0x200)
> -                       i_delta = i_max - i_min;
> -               else
> -                       i_delta = i_max + (0x400 - i_min);
> +               i_delta = i_max - i_min;
> +               q_delta = q_max - q_min;
> 
> -               if (q_max < 0x200 && q_min < 0x200)
> -                       q_delta = q_max - q_min;
> -               else if (q_max >= 0x200 && q_min >= 0x200)
> -                       q_delta = q_max - q_min;
> -               else
> -                       q_delta = q_max + (0x400 - q_min);
> 
>                 rtw_dbg(rtwdev, RTW_DBG_RFK,
> -                       "[DACK] i: min=0x%08x, max=0x%08x, delta=0x%08x\n",
> +                       "[DACK] i: min=%d, max=%d, delta=%d\n",
>                         i_min, i_max, i_delta);
>                 rtw_dbg(rtwdev, RTW_DBG_RFK,
> -                       "[DACK] q: min=0x%08x, max=0x%08x, delta=0x%08x\n",
> +                       "[DACK] q: min=%d, max=%d, delta=%d",
>                         q_min, q_max, q_delta);
> 
>                 rtw8822c_dac_iq_sort(rtwdev, iv, qv);
> 
>                 if (i_delta > 5 || q_delta > 5) {
>                         temp = rtw_read32(rtwdev, 0x2dbc);
> -                       iv[0] = FIELD_GET(RTW8822C_DAC_IV_MASK, temp);
> -                       qv[0] = FIELD_GET(RTW8822C_DAC_QV_MASK, temp);
> +                       iv[0] = FIELD_GET_SIGNED(RTW8822C_DAC_IV_MASK, temp);
> +                       qv[0] = FIELD_GET_SIGNED(RTW8822C_DAC_QV_MASK, temp);
>                         temp = rtw_read32(rtwdev, 0x2dbc);
> -                       iv[DACK_SN_8822C - 1] = FIELD_GET(RTW8822C_DAC_IV_MASK, temp);
> -                       qv[DACK_SN_8822C - 1] = FIELD_GET(RTW8822C_DAC_QV_MASK, temp);
> +                       iv[DACK_SN_8822C - 1] = FIELD_GET_SIGNED(RTW8822C_DAC_IV_MASK, temp);
> +                       qv[DACK_SN_8822C - 1] = FIELD_GET_SIGNED(RTW8822C_DAC_QV_MASK, temp);
>                 } else {
>                         break;
>                 }
>         } while (cnt++ < 100);
> 
> -       rtw8822c_dac_iq_offset(rtwdev, iv, i_value);
> -       rtw8822c_dac_iq_offset(rtwdev, qv, q_value);
> +       *i_value = rtw8822c_dac_iq_offset(rtwdev, iv);
> +       *q_value = rtw8822c_dac_iq_offset(rtwdev, qv);
>  }
> 
>  static void rtw8822c_dac_cal_rf_mode(struct rtw_dev *rtwdev,
>                                      u32 *i_value, u32 *q_value)
>  {
> -       u32 iv[DACK_SN_8822C], qv[DACK_SN_8822C];
> +       s32 iv[DACK_SN_8822C], qv[DACK_SN_8822C];
>         u32 rf_a, rf_b;
> 
>         rf_a = rtw_read_rf(rtwdev, RF_PATH_A, 0x0, RFREG_MASK);
> --
> 2.55.0