[PATCH 0/4] wifi: rtw88: rtw8822c: refactor DAC IQ calibration to s32

[PATCH 0/4] wifi: rtw88: rtw8822c: refactor DAC IQ calibration to s32

[Arsenii Pashchenko]

This series refactors the Digital-to-Analog Converter (DAC) IQ 
calibration routines in the rtw8822c driver. 

The original code heavily relies on unsigned 32-bit math mapped over
a 10-bit circular hardware register scale centered around 0x200. This 
introduces complex nested boundary checks, custom bubble sort rules,
and custom min/max tracking helpers.

By migrating the temporary stack arrays and signatures to signed s32
and utilizing the kernel's sign_extend32() API, the coordination system 
is linearized. This allows the removal of internal custom helpers and 
collapses unsigned wrap-around logic into standard linear operations.

The series is broken down as follows:
Patch 1: Prepares signatures and stack buffers, converting types to s32.
Patch 2: Introduces sign_extend32() and simplifies the validation window.
Patch 3: Replaces custom bubble sort with the kernel's native sort().
Patch 4: Eliminates minmax_cmp, streamlining search and offset loops.

AI Disclosure:
An AI assistant was used for source code analysis and conceptual 
consultation regarding the signed math transformation. All code 
modifications, final implementations, and refactoring steps were 
written, reviewed, and verified manually by the author.

Testing:
- Every patch in the series was verified with scripts/checkpatch.pl 
  and yielded 0 errors and 0 warnings.
- Each patch was sequentially built (bisected and compiled) to ensure 
  there are no compiler warnings, pointer type mismatches, or broken 
  builds at any intermediate step.

Arsenii Pashchenko (4):
  wifi: rtw88: rtw8822c: convert DAC IQ buffers and signatures to s32
  wifi: rtw88: rtw8822c: use sign_extend32 for DAC IQ validation
  wifi: rtw88: rtw8822c: switch to the kernel's sort() library
  wifi: rtw88: rtw8822c: simplify amplitude search and offset via s32

 drivers/net/wireless/realtek/rtw88/rtw8822c.c | 146 ++++++------------
 1 file changed, 43 insertions(+), 103 deletions(-)

-- 
2.55.0

[PATCH 1/4] wifi: rtw88: rtw8822c: convert DAC IQ buffers and signatures to s32

[Arsenii Pashchenko]

Prepare the DAC IQ calibration path for signed math by converting the
temporary stack arrays 'iv' and 'qv' from u32 to s32. Update all
internal function signatures to accept s32 pointers.

To guarantee bisectability, wrap the existing unsigned logic in
temporary casts to u32. This maintains identical mathematical behavior
and avoids any compiler sign-compare warnings.

Signed-off-by: Arsenii Pashchenko <ulijg308@gmail.com>
---
 drivers/net/wireless/realtek/rtw88/rtw8822c.c | 49 +++++++++++--------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
index 244c80264..896e3e7b9 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
@@ -153,9 +153,12 @@ static void rtw8822c_dac_restore_reg(struct rtw_dev *rtwdev,
 	}
 }
 
-static void rtw8822c_rf_minmax_cmp(struct rtw_dev *rtwdev, u32 value,
-				   u32 *min, u32 *max)
+static void rtw8822c_rf_minmax_cmp(struct rtw_dev *rtwdev, s32 value_s32,
+				   s32 *min_s32, s32 *max_s32)
 {
+	u32 value = (u32)value_s32;
+	u32 *min = (u32 *)min_s32;
+	u32 *max = (u32 *)max_s32;
 	if (value >= 0x200) {
 		if (*min >= 0x200) {
 			if (*min > value)
@@ -182,8 +185,10 @@ static void rtw8822c_rf_minmax_cmp(struct rtw_dev *rtwdev, u32 value,
 	}
 }
 
-static void __rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, u32 *v1, u32 *v2)
+static void __rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, s32 *v1_s32, s32 *v2_s32)
 {
+	u32 *v1 = (u32 *)v1_s32;
+	u32 *v2 = (u32 *)v2_s32;
 	if (*v1 >= 0x200 && *v2 >= 0x200) {
 		if (*v1 > *v2)
 			swap(*v1, *v2);
@@ -195,7 +200,7 @@ static void __rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, u32 *v1, u32 *v2)
 	}
 }
 
-static void rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, u32 *iv, u32 *qv)
+static void rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, s32 *iv, s32 *qv)
 {
 	u32 i, j;
 
@@ -207,8 +212,9 @@ 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 void rtw8822c_dac_iq_offset(struct rtw_dev *rtwdev, s32 *vec_s32, u32 *val)
 {
+	u32 *vec = (u32 *)vec_s32;
 	u32 p, m, t, i;
 
 	m = 0;
@@ -271,8 +277,9 @@ 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_s32)
 {
+	u32 value = (u32)value_s32;
 	bool ret = true;
 
 	if ((value >= 0x200 && (0x400 - value) > 0x64) ||
@@ -284,7 +291,7 @@ static bool rtw8822c_dac_iq_check(struct rtw_dev *rtwdev, u32 value)
 	return ret;
 }
 
-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 +299,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_mask(rtwdev, 0x2dbc, 0x3fffff);
-		iv[i] = (temp & 0x3ff000) >> 12;
-		qv[i] = temp & 0x3ff;
+		iv[i] = (s32)((temp & 0x3ff000) >> 12);
+		qv[i] = (s32)(temp & 0x3ff);
 
 		if (rtw8822c_dac_iq_check(rtwdev, iv[i]) &&
 		    rtw8822c_dac_iq_check(rtwdev, qv[i]))
@@ -302,7 +309,7 @@ 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;
@@ -311,13 +318,13 @@ static void rtw8822c_dac_cal_iq_search(struct rtw_dev *rtwdev,
 	int i, cnt = 0;
 
 	do {
-		i_min = iv[0];
-		i_max = iv[0];
-		q_min = qv[0];
-		q_max = qv[0];
+		i_min = (u32)iv[0];
+		i_max = (u32)iv[0];
+		q_min = (u32)qv[0];
+		q_max = (u32)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);
+			rtw8822c_rf_minmax_cmp(rtwdev, iv[i], (s32 *)&i_min, (s32 *)&i_max);
+			rtw8822c_rf_minmax_cmp(rtwdev, qv[i], (s32 *)&q_min, (s32 *)&q_max);
 		}
 
 		if (i_max < 0x200 && i_min < 0x200)
@@ -345,11 +352,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] = (temp & 0x3ff000) >> 12;
-			qv[0] = temp & 0x3ff;
+			iv[0] = (s32)((temp & 0x3ff000) >> 12);
+			qv[0] = (s32)(temp & 0x3ff);
 			temp = rtw_read32_mask(rtwdev, 0x2dbc, 0x3fffff);
-			iv[DACK_SN_8822C - 1] = (temp & 0x3ff000) >> 12;
-			qv[DACK_SN_8822C - 1] = temp & 0x3ff;
+			iv[DACK_SN_8822C - 1] = (s32)((temp & 0x3ff000) >> 12);
+			qv[DACK_SN_8822C - 1] = (s32)(temp & 0x3ff);
 		} else {
 			break;
 		}
@@ -362,7 +369,7 @@ static void rtw8822c_dac_cal_iq_search(struct rtw_dev *rtwdev,
 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

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

[Arsenii Pashchenko]

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.

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@gmail.com>
---
 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>
 #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) {
 		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);
 
 		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);
 			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);
 		} else {
 			break;
 		}
-- 
2.55.0

[PATCH 3/4] wifi: rtw88: rtw8822c: switch to the kernel's sort() library

[Arsenii Pashchenko]

Replace the custom, nested bubble-sort logic in rtw8822c_dac_iq_sort()
and __rtw8822c_dac_iq_sort() with a call to the kernel's optimized
heapsort implementation.

Implement a concise, localized s32 comparator. Since the previous
patch already guarantees that input values are properly sign-extended
into signed arrays, the custom circular index sorting rules completely
disappear, trimming down lines of code.

Signed-off-by: Arsenii Pashchenko <ulijg308@gmail.com>
---
 drivers/net/wireless/realtek/rtw88/rtw8822c.c | 28 ++++++-------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
index d06986d09..642ac2e2b 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
@@ -4,6 +4,7 @@
 
 #include <linux/module.h>
 #include <linux/bitops.h>
+#include <linux/sort.h>
 #include "main.h"
 #include "coex.h"
 #include "fw.h"
@@ -186,31 +187,18 @@ static void rtw8822c_rf_minmax_cmp(struct rtw_dev *rtwdev, s32 value_s32,
 	}
 }
 
-static void __rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, s32 *v1_s32, s32 *v2_s32)
+static int rtw8822c_dac_iq_cmp_s32(const void *a, const void *b)
 {
-	u32 *v1 = (u32 *)v1_s32;
-	u32 *v2 = (u32 *)v2_s32;
-	if (*v1 >= 0x200 && *v2 >= 0x200) {
-		if (*v1 > *v2)
-			swap(*v1, *v2);
-	} else if (*v1 < 0x200 && *v2 < 0x200) {
-		if (*v1 > *v2)
-			swap(*v1, *v2);
-	} else if (*v1 < 0x200 && *v2 >= 0x200) {
-		swap(*v1, *v2);
-	}
+	s32 va = *(const s32 *)a;
+	s32 vb = *(const s32 *)b;
+
+	return (va > vb) - (va < vb);
 }
 
 static void rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, s32 *iv, s32 *qv)
 {
-	u32 i, j;
-
-	for (i = 0; i < DACK_SN_8822C - 1; i++) {
-		for (j = 0; j < (DACK_SN_8822C - 1 - i) ; j++) {
-			__rtw8822c_dac_iq_sort(rtwdev, &iv[j], &iv[j + 1]);
-			__rtw8822c_dac_iq_sort(rtwdev, &qv[j], &qv[j + 1]);
-		}
-	}
+	sort(iv, DACK_SN_8822C, sizeof(s32), rtw8822c_dac_iq_cmp_s32, NULL);
+	sort(qv, DACK_SN_8822C, sizeof(s32), rtw8822c_dac_iq_cmp_s32, NULL);
 }
 
 static void rtw8822c_dac_iq_offset(struct rtw_dev *rtwdev, s32 *vec_s32, u32 *val)
-- 
2.55.0

[PATCH 4/4] wifi: rtw88: rtw8822c: simplify amplitude search and offset via s32

[Arsenii Pashchenko]

Complete the signed data transition by removing the complex
rtw8822c_rf_minmax_cmp() helper. On a true linear s32 scale, finding
the delta amplitude simplifies to a direct difference (max - min)
without any nested circular wrap-around conditions.

Refactor rtw8822c_dac_iq_offset() to directly calculate the trimmed
arithmetic mean, casting back to the hardware register's offset
orientation only at the final assignment using a ternary operator.

Signed-off-by: Arsenii Pashchenko <ulijg308@gmail.com>
---
 drivers/net/wireless/realtek/rtw88/rtw8822c.c | 103 +++++-------------
 1 file changed, 25 insertions(+), 78 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
index 642ac2e2b..6fcdfc64c 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
@@ -155,38 +155,6 @@ static void rtw8822c_dac_restore_reg(struct rtw_dev *rtwdev,
 	}
 }
 
-static void rtw8822c_rf_minmax_cmp(struct rtw_dev *rtwdev, s32 value_s32,
-				   s32 *min_s32, s32 *max_s32)
-{
-	u32 value = (u32)value_s32;
-	u32 *min = (u32 *)min_s32;
-	u32 *max = (u32 *)max_s32;
-	if (value >= 0x200) {
-		if (*min >= 0x200) {
-			if (*min > value)
-				*min = value;
-		} else {
-			*min = value;
-		}
-		if (*max >= 0x200) {
-			if (*max < value)
-				*max = value;
-		}
-	} else {
-		if (*min < 0x200) {
-			if (*min > value)
-				*min = value;
-		}
-
-		if (*max  >= 0x200) {
-			*max = value;
-		} else {
-			if (*max < value)
-				*max = value;
-		}
-	}
-}
-
 static int rtw8822c_dac_iq_cmp_s32(const void *a, const void *b)
 {
 	s32 va = *(const s32 *)a;
@@ -201,31 +169,16 @@ static void rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, s32 *iv, s32 *qv)
 	sort(qv, DACK_SN_8822C, sizeof(s32), rtw8822c_dac_iq_cmp_s32, NULL);
 }
 
-static void rtw8822c_dac_iq_offset(struct rtw_dev *rtwdev, s32 *vec_s32, u32 *val)
+static s32 rtw8822c_dac_iq_offset(struct rtw_dev *rtwdev, s32 *vec)
 {
-	u32 *vec = (u32 *)vec_s32;
-	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);
 
-	*val = t;
+	return avg >= 0 ? avg : 0x400 + avg;
 }
 
 static u32 rtw8822c_get_path_write_addr(u8 path)
@@ -280,7 +233,7 @@ static bool rtw8822c_dac_iq_check(struct rtw_dev *rtwdev, s32 value)
 static void rtw8822c_dac_cal_iq_sample(struct rtw_dev *rtwdev, s32 *iv, s32 *qv)
 {
 	u32 temp;
-	int i = 0, cnt = 0;
+	u32 i = 0, cnt = 0;
 
 	while (i < DACK_SN_8822C && cnt < 10000) {
 		cnt++;
@@ -298,34 +251,28 @@ static void rtw8822c_dac_cal_iq_search(struct rtw_dev *rtwdev,
 				       s32 *iv, s32 *qv,
 				       u32 *i_value, u32 *q_value)
 {
-	u32 i_max = 0, q_max = 0, i_min = 0, q_min = 0;
-	u32 i_delta, q_delta;
+	s32 i_max = 0, q_max = 0, i_min = 0, q_min = 0;
 	u32 temp;
-	int i, cnt = 0;
+	u32 i, cnt = 0;
 
 	do {
-		i_min = (u32)iv[0];
-		i_max = (u32)iv[0];
-		q_min = (u32)qv[0];
-		q_max = (u32)qv[0];
+		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], (s32 *)&i_min, (s32 *)&i_max);
-			rtw8822c_rf_minmax_cmp(rtwdev, qv[i], (s32 *)&q_min, (s32 *)&q_max);
+			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);
-
-		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);
+		u32 i_delta = (u32)(i_max - i_min);
+		u32 q_delta = (u32)(q_max - q_min);
 
 		rtw_dbg(rtwdev, RTW_DBG_RFK,
 			"[DACK] i: min=0x%08x, max=0x%08x, delta=0x%08x\n",
@@ -348,8 +295,8 @@ static void rtw8822c_dac_cal_iq_search(struct rtw_dev *rtwdev,
 		}
 	} 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,
-- 
2.55.0

RE: [PATCH 1/4] wifi: rtw88: rtw8822c: convert DAC IQ buffers and signatures to s32

[Ping-Ke Shih]

Arsenii Pashchenko <ulijg308@gmail.com> wrote:
> Prepare the DAC IQ calibration path for signed math by converting the
> temporary stack arrays 'iv' and 'qv' from u32 to s32. Update all
> internal function signatures to accept s32 pointers.
> 
> To guarantee bisectability, wrap the existing unsigned logic in
> temporary casts to u32. This maintains identical mathematical behavior
> and avoids any compiler sign-compare warnings.
> 
> Signed-off-by: Arsenii Pashchenko <ulijg308@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtw88/rtw8822c.c | 49 +++++++++++--------
>  1 file changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> index 244c80264..896e3e7b9 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> @@ -153,9 +153,12 @@ static void rtw8822c_dac_restore_reg(struct rtw_dev *rtwdev,
>         }
>  }
> 
> -static void rtw8822c_rf_minmax_cmp(struct rtw_dev *rtwdev, u32 value,
> -                                  u32 *min, u32 *max)
> +static void rtw8822c_rf_minmax_cmp(struct rtw_dev *rtwdev, s32 value_s32,
> +                                  s32 *min_s32, s32 *max_s32)
>  {
> +       u32 value = (u32)value_s32;
> +       u32 *min = (u32 *)min_s32;
> +       u32 *max = (u32 *)max_s32;

blank line

Also, it is possible to avoid all casting from s32 to u32, even just a
transition patch? I guess compiler can silently ignore this, and it'd
be much easier to review. Otherwise, I need to check if you remove
these casting clearly. 

>         if (value >= 0x200) {
>                 if (*min >= 0x200) {
>                         if (*min > value)
> @@ -182,8 +185,10 @@ static void rtw8822c_rf_minmax_cmp(struct rtw_dev *rtwdev, u32 value,
>         }
>  }
> 
> -static void __rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, u32 *v1, u32 *v2)
> +static void __rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, s32 *v1_s32, s32 *v2_s32)
>  {
> +       u32 *v1 = (u32 *)v1_s32;
> +       u32 *v2 = (u32 *)v2_s32;

blank line

>         if (*v1 >= 0x200 && *v2 >= 0x200) {
>                 if (*v1 > *v2)
>                         swap(*v1, *v2);

[...]

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

[Ping-Ke Shih]

Arsenii Pashchenko <ulijg308@gmail.com> 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@gmail.com>
> ---
>  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

RE: [PATCH 3/4] wifi: rtw88: rtw8822c: switch to the kernel's sort() library

[Ping-Ke Shih]

Arsenii Pashchenko <ulijg308@gmail.com> wrote:
> Replace the custom, nested bubble-sort logic in rtw8822c_dac_iq_sort()
> and __rtw8822c_dac_iq_sort() with a call to the kernel's optimized
> heapsort implementation.
> 
> Implement a concise, localized s32 comparator. Since the previous
> patch already guarantees that input values are properly sign-extended
> into signed arrays, the custom circular index sorting rules completely
> disappear, trimming down lines of code.
> 
> Signed-off-by: Arsenii Pashchenko <ulijg308@gmail.com>

I think this patch is correct, but please use real hardware to verify please.
The calibration is very important to WiFi performance, and hard to find
a slight degrade (e.g. throughput degrade from 500Mbps to 480Mbps). 

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

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

[Ping-Ke Shih]

> >                         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.
> 

Consider the change of sign_extend32() again. The value is not the same
as before. So, if people run this intermediate patch, the result will be
wrong. Please think how to rearrange your changes.

RE: [PATCH 4/4] wifi: rtw88: rtw8822c: simplify amplitude search and offset via s32

[Ping-Ke Shih]

Arsenii Pashchenko <ulijg308@gmail.com> wrote:
> Complete the signed data transition by removing the complex
> rtw8822c_rf_minmax_cmp() helper. On a true linear s32 scale, finding
> the delta amplitude simplifies to a direct difference (max - min)
> without any nested circular wrap-around conditions.
> 
> Refactor rtw8822c_dac_iq_offset() to directly calculate the trimmed
> arithmetic mean, casting back to the hardware register's offset
> orientation only at the final assignment using a ternary operator.
> 
> Signed-off-by: Arsenii Pashchenko <ulijg308@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtw88/rtw8822c.c | 103 +++++-------------
>  1 file changed, 25 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> index 642ac2e2b..6fcdfc64c 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
> @@ -155,38 +155,6 @@ static void rtw8822c_dac_restore_reg(struct rtw_dev *rtwdev,
>         }
>  }
> 
> -static void rtw8822c_rf_minmax_cmp(struct rtw_dev *rtwdev, s32 value_s32,
> -                                  s32 *min_s32, s32 *max_s32)
> -{
> -       u32 value = (u32)value_s32;
> -       u32 *min = (u32 *)min_s32;
> -       u32 *max = (u32 *)max_s32;
> -       if (value >= 0x200) {
> -               if (*min >= 0x200) {
> -                       if (*min > value)
> -                               *min = value;
> -               } else {
> -                       *min = value;
> -               }
> -               if (*max >= 0x200) {
> -                       if (*max < value)
> -                               *max = value;
> -               }
> -       } else {
> -               if (*min < 0x200) {
> -                       if (*min > value)
> -                               *min = value;
> -               }
> -
> -               if (*max  >= 0x200) {
> -                       *max = value;
> -               } else {
> -                       if (*max < value)
> -                               *max = value;
> -               }
> -       }
> -}
> -
>  static int rtw8822c_dac_iq_cmp_s32(const void *a, const void *b)
>  {
>         s32 va = *(const s32 *)a;
> @@ -201,31 +169,16 @@ static void rtw8822c_dac_iq_sort(struct rtw_dev *rtwdev, s32 *iv, s32 *qv)
>         sort(qv, DACK_SN_8822C, sizeof(s32), rtw8822c_dac_iq_cmp_s32, NULL);
>  }
> 
> -static void rtw8822c_dac_iq_offset(struct rtw_dev *rtwdev, s32 *vec_s32, u32 *val)
> +static s32 rtw8822c_dac_iq_offset(struct rtw_dev *rtwdev, s32 *vec)
>  {
> -       u32 *vec = (u32 *)vec_s32;
> -       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 ? avg : 0x400 + avg;

Return type is 's32', but here doesn't do sign_extension()?

>  }
> 
>  static u32 rtw8822c_get_path_write_addr(u8 path)