On 09/27/2014 02:49 PM, Francois Romieu wrote:

Larry Finger [off-list ref] :
[...]

quoted

diff --git a/drivers/net/wireless/rtlwifi/core.c b/drivers/net/wireless/rtlwifi/core.c
index 40738e3..d30f416 100644
--- a/drivers/net/wireless/rtlwifi/core.c
+++ b/drivers/net/wireless/rtlwifi/core.c

@@ -28,6 +28,7 @@
  #include "cam.h"
  #include "base.h"
  #include "ps.h"
+#include "pwrseqcmd.h"

  #include "btcoexist/rtl_btc.h"
  #include <linux/firmware.h>

@@ -1700,3 +1701,100 @@ const struct ieee80211_ops rtl_ops = {
  };
  EXPORT_SYMBOL_GPL(rtl_ops);

+/*	Description:
+ *		This routine deals with the Power Configuration CMD
+ *		 parsing for RTL8723/RTL8188E Series IC.
+ *	Assumption:
+ *		We should follow specific format that was released from HW SD.

"released" is a bit misleading if it does not mean "publicly released".

quoted

+ */
+bool rtl_hal_pwrseqcmdparsing(struct rtl_priv *rtlpriv, u8 cut_version,
+			      u8 faversion, u8 interface_type,
+			      struct wlan_pwr_cfg pwrcfgcmd[])
+{
+	struct wlan_pwr_cfg cfg_cmd = {0};
+	bool polling_bit = false;
+	u32 ary_idx = 0;

	u32 i;

quoted

+	u8 value = 0;
+	u32 offset = 0;

Useless init.

quoted

+	u32 polling_count = 0;

Wrong scope.

quoted

+	u32 max_polling_cnt = 5000;

Inverted xmas tree for declaration.

I do not understand what you mean here.

quoted

+
+	do {

	while (1) {

quoted

+		cfg_cmd = pwrcfgcmd[ary_idx];
+		RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
+			 "rtl_hal_pwrseqcmdparsing(): offset(%#x),cut_msk(%#x), famsk(%#x), interface_msk(%#x), base(%#x), cmd(%#x), msk(%#x), value(%#x)\n",

                                                                 ^^^ missing space

quoted

+			 GET_PWR_CFG_OFFSET(cfg_cmd),
+					    GET_PWR_CFG_CUT_MASK(cfg_cmd),
+			 GET_PWR_CFG_FAB_MASK(cfg_cmd),
+					      GET_PWR_CFG_INTF_MASK(cfg_cmd),
+			 GET_PWR_CFG_BASE(cfg_cmd), GET_PWR_CFG_CMD(cfg_cmd),
+			 GET_PWR_CFG_MASK(cfg_cmd), GET_PWR_CFG_VALUE(cfg_cmd));
+
+		if ((GET_PWR_CFG_FAB_MASK(cfg_cmd)&faversion) &&
+		    (GET_PWR_CFG_CUT_MASK(cfg_cmd)&cut_version) &&

                                                  ^^^ missing spaces

quoted

+		    (GET_PWR_CFG_INTF_MASK(cfg_cmd)&interface_type)) {
+			switch (GET_PWR_CFG_CMD(cfg_cmd)) {
+			case PWR_CMD_READ:
+				RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
+					"rtl_hal_pwrseqcmdparsing(): PWR_CMD_READ\n");

It should line up after the parenthesis.

quoted

+				break;
+			case PWR_CMD_WRITE:
+				RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
+					"rtl_hal_pwrseqcmdparsing(): PWR_CMD_WRITE\n");
+				offset = GET_PWR_CFG_OFFSET(cfg_cmd);
+
+				/*Read the value from system register*/
+				value = rtl_read_byte(rtlpriv, offset);
+				value &= (~(GET_PWR_CFG_MASK(cfg_cmd)));

Excess parenthsis.

quoted

+				value |= (GET_PWR_CFG_VALUE(cfg_cmd) &
+					  GET_PWR_CFG_MASK(cfg_cmd));

Excess parenthsis.

quoted

+
+				/*Write the value back to sytem register*/
+				rtl_write_byte(rtlpriv, offset, value);
+				break;
+			case PWR_CMD_POLLING:
+				RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
+					"rtl_hal_pwrseqcmdparsing(): PWR_CMD_POLLING\n");

It should line up after the parenthesis.

quoted

+				polling_bit = false;
+				offset = GET_PWR_CFG_OFFSET(cfg_cmd);
+
+				do {
+					value = rtl_read_byte(rtlpriv, offset);
+
+					value &= GET_PWR_CFG_MASK(cfg_cmd);
+					if (value ==
+					    (GET_PWR_CFG_VALUE(cfg_cmd) &
+					     GET_PWR_CFG_MASK(cfg_cmd)))
+						polling_bit = true;
+					else
+						udelay(10);
+
+					if (polling_count++ > max_polling_cnt)
+						return false;
+				} while (!polling_bit);

Simple "for" loop in disguise.

An helper function may help.

Imnsho the whole do { more, more, more } while (1) concept is broken.

[...]

quoted

diff --git a/drivers/net/wireless/rtlwifi/rtl8188ee/def.h b/drivers/net/wireless/rtlwifi/rtl8188ee/def.h
index c764fff..d9ea9d0 100644
--- a/drivers/net/wireless/rtlwifi/rtl8188ee/def.h
+++ b/drivers/net/wireless/rtlwifi/rtl8188ee/def.h

[...]

quoted

@@ -225,44 +218,37 @@ enum power_polocy_config {
  };

  enum interface_select_pci {
-	INTF_SEL1_MINICARD,
-	INTF_SEL0_PCIE,
-	INTF_SEL2_RSV,
-	INTF_SEL3_RSV,
+	INTF_SEL1_MINICARD = 0,
+	INTF_SEL0_PCIE = 1,
+	INTF_SEL2_RSV = 2,
+	INTF_SEL3_RSV = 3,

Please use tabs to line things up.

[...]

quoted

+	HAL_FW_C2H_CMD_READ_MACREG = 0,
+	HAL_FW_C2H_CMD_READ_BBREG = 1,
+	HAL_FW_C2H_CMD_READ_RFREG = 2,
+	HAL_FW_C2H_CMD_READ_EEPROM = 3,
+	HAL_FW_C2H_CMD_READ_EFUSE = 4,

See above.

[...]

quoted

diff --git a/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c b/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c
index f8daa61..2aa34d9 100644
--- a/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c
+++ b/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c

[...]

quoted

-			rtl_set_bbreg(hw, ROFDM0_XATXIQIMBAL, MASKDWORD,
-				      value32);
+			rtl_set_bbreg(hw, ROFDM0_XATXIQIMBALANCE,
+				      MASKDWORD, value32);

It lacks of consistency, see the *NICE* marker below.

quoted

  			value32 = (ele_c & 0x000003C0) >> 6;
-			rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS, value32);
+			rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS,
+				      value32);

?

quoted

  			value32 = ((iqk_result_x * ele_d) >> 7) & 0x01;
-			rtl_set_bbreg(hw, ROFDM0_ECCATHRES, BIT(24), value32);
+			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD, BIT(24),
+				      value32);
  			break;
  		case RF90_PATH_B:
  			value32 = (ele_d << 22)|((ele_c & 0x3F)<<16) | ele_a;
-			rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBAL,
-				      MASKDWORD, value32);
+			rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBALANCE, MASKDWORD,
+				      value32);

*NICE*

[...]

quoted

@@ -210,16 +209,20 @@ static void rtl88e_set_iqk_matrix(struct ieee80211_hw *hw,
  	} else {
  		switch (rfpath) {
  		case RF90_PATH_A:
-			rtl_set_bbreg(hw, ROFDM0_XATXIQIMBAL, MASKDWORD,
-				      ofdmswing_table[ofdm_index]);
-			rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS, 0x00);
-			rtl_set_bbreg(hw, ROFDM0_ECCATHRES, BIT(24), 0x00);
+			rtl_set_bbreg(hw, ROFDM0_XATXIQIMBALANCE,
+				      MASKDWORD, ofdmswing_table[ofdm_index]);
+			rtl_set_bbreg(hw, ROFDM0_XCTXAFE,
+				      MASKH4BITS, 0x00);

			rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS, 0x00);

quoted

+			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD,
+				      BIT(24), 0x00);

			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD, BIT(24), 0x00);

quoted

  			break;
  		case RF90_PATH_B:
-			rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBAL, MASKDWORD,
-				      ofdmswing_table[ofdm_index]);
-			rtl_set_bbreg(hw, ROFDM0_XDTXAFE, MASKH4BITS, 0x00);
-			rtl_set_bbreg(hw, ROFDM0_ECCATHRES, BIT(28), 0x00);
+			rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBALANCE,
+				      MASKDWORD, ofdmswing_table[ofdm_index]);
+			rtl_set_bbreg(hw, ROFDM0_XDTXAFE,
+				      MASKH4BITS, 0x00);

			rtl_set_bbreg(hw, ROFDM0_XDTXAFE, MASKH4BITS, 0x00);

quoted

+			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD,
+				      BIT(28), 0x00);

			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD, BIT(28), 0x00);

[...]

quoted

@@ -263,46 +266,75 @@ void rtl88e_dm_txpower_track_adjust(struct ieee80211_hw *hw,
  			 (pwr_val << 24);
  }

-
-static void rtl88e_chk_tx_track(struct ieee80211_hw *hw,
-				enum pwr_track_control_method method,
-				u8 rfpath, u8 index)
+static void dm_tx_pwr_track_set_pwr(struct ieee80211_hw *hw,
+				    enum pwr_track_control_method method,
+				    u8 rfpath, u8 channel_mapped_index)
  {
  	struct rtl_priv *rtlpriv = rtl_priv(hw);
-	struct rtl_phy *rtlphy = &(rtlpriv->phy);
+	struct rtl_phy *rtlphy = &rtlpriv->phy;
  	struct rtl_dm *rtldm = rtl_dm(rtl_priv(hw));
-	int jj = rtldm->swing_idx_cck;
-	int i;

  	if (method == TXAGC) {
-		if (rtldm->swing_flag_ofdm == true ||
-		    rtldm->swing_flag_cck == true) {
-			u8 chan = rtlphy->current_channel;
-			rtl88e_phy_set_txpower_level(hw, chan);
+		if (rtldm->swing_flag_ofdm ||
+		    rtldm->swing_flag_cck) {

Single line.

quoted

+			rtl88e_phy_set_txpower_level(hw,
+						     rtlphy->current_channel);
  			rtldm->swing_flag_ofdm = false;
  			rtldm->swing_flag_cck = false;
  		}
  	} else if (method == BBSWING) {
  		if (!rtldm->cck_inch14) {
-			for (i = 0; i < 8; i++)
-				rtl_write_byte(rtlpriv, 0xa22 + i,
-					       cck_tbl_ch1_13[jj][i]);
+			rtl_write_byte(rtlpriv, 0xa22,
+				       cck_tbl_ch1_13[rtldm->swing_idx_cck][0]);
+			rtl_write_byte(rtlpriv, 0xa23,
+				       cck_tbl_ch1_13[rtldm->swing_idx_cck][1]);
+			rtl_write_byte(rtlpriv, 0xa24,
+				       cck_tbl_ch1_13[rtldm->swing_idx_cck][2]);
+			rtl_write_byte(rtlpriv, 0xa25,
+				       cck_tbl_ch1_13[rtldm->swing_idx_cck][3]);
+			rtl_write_byte(rtlpriv, 0xa26,
+				       cck_tbl_ch1_13[rtldm->swing_idx_cck][4]);
+			rtl_write_byte(rtlpriv, 0xa27,
+				       cck_tbl_ch1_13[rtldm->swing_idx_cck][5]);
+			rtl_write_byte(rtlpriv, 0xa28,
+				       cck_tbl_ch1_13[rtldm->swing_idx_cck][6]);
+			rtl_write_byte(rtlpriv, 0xa29,
+				       cck_tbl_ch1_13[rtldm->swing_idx_cck][7]);

?

I hope someone got the "-R" flag of patch wrong.

quoted

  		} else {
-			for (i = 0; i < 8; i++)
-				rtl_write_byte(rtlpriv, 0xa22 + i,
-					       cck_tbl_ch14[jj][i]);
+			rtl_write_byte(rtlpriv, 0xa22,
+				       cck_tbl_ch14[rtldm->swing_idx_cck][0]);
+			rtl_write_byte(rtlpriv, 0xa23,
+				       cck_tbl_ch14[rtldm->swing_idx_cck][1]);
+			rtl_write_byte(rtlpriv, 0xa24,
+				       cck_tbl_ch14[rtldm->swing_idx_cck][2]);
+			rtl_write_byte(rtlpriv, 0xa25,
+				       cck_tbl_ch14[rtldm->swing_idx_cck][3]);
+			rtl_write_byte(rtlpriv, 0xa26,
+				       cck_tbl_ch14[rtldm->swing_idx_cck][4]);
+			rtl_write_byte(rtlpriv, 0xa27,
+				       cck_tbl_ch14[rtldm->swing_idx_cck][5]);
+			rtl_write_byte(rtlpriv, 0xa28,
+				       cck_tbl_ch14[rtldm->swing_idx_cck][6]);
+			rtl_write_byte(rtlpriv, 0xa29,
+				       cck_tbl_ch14[rtldm->swing_idx_cck][7]);

Sic.

quoted

  		}

  		if (rfpath == RF90_PATH_A) {
-			long x = rtlphy->iqk_matrix[index].value[0][0];
-			long y = rtlphy->iqk_matrix[index].value[0][1];
-			u8 indx = rtldm->swing_idx_ofdm[rfpath];
-			rtl88e_set_iqk_matrix(hw, indx, rfpath, x, y);
+			rtl88e_set_iqk_matrix(hw, rtldm->swing_idx_ofdm[rfpath],
+					      rfpath, rtlphy->iqk_matrix
+					      [channel_mapped_index].
+					      value[0][0],
+					      rtlphy->iqk_matrix
+					      [channel_mapped_index].
+					      value[0][1]);
  		} else if (rfpath == RF90_PATH_B) {
-			u8 indx = rtldm->swing_idx_ofdm[rfpath];
-			long x = rtlphy->iqk_matrix[indx].value[0][4];
-			long y = rtlphy->iqk_matrix[indx].value[0][5];
-			rtl88e_set_iqk_matrix(hw, indx, rfpath, x, y);
+			rtl88e_set_iqk_matrix(hw, rtldm->swing_idx_ofdm[rfpath],
+					      rfpath, rtlphy->iqk_matrix
+					      [channel_mapped_index].
+					      value[0][4],
+					      rtlphy->iqk_matrix
+					      [channel_mapped_index].
+					      value[0][5]);

:o(

I stop here. I don't enjoy playing the bad cop and this patch is feeding crap
into the kernel.

Thanks for the review.

As I explained in V1 of these patches, I am trying to merge the kernel and 
Realtek internal code repositories in the hope that Realtek will keep the two of 
them in sync. In addition, I am hoping that they will take over maintaining 
these codes in the future.

If I insist on an absolute minimum of change in the kernel code, then the churn 
of the Realtek repo is increased, and I run the risk of failing in the process 
of merging the two repos. In my estimation, that would be a bigger disaster than 
getting some code into the kernel that is not as pretty as could be. In 
addition, I am not sure how much longer I want to volunteer full time to support 
these codes.

Larry