RE: [PATCH v5 5/6] wifi: rtlwifi: Clean up rtl8192d-common a bit

[Ping-Ke Shih <pkshih@realtek.com>]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:

> Improve readability:
>  * add empty lines
>  * use abs_diff in rtl92d_dm_txpower_tracking_callback_thermalmeter
>  * roll up repeated statements into a for loop in
>    rtl92d_dm_txpower_tracking_callback_thermalmeter
>  * shorten lines by replacing many instances of "rtlpriv->dm" with "dm"
>    pointer in rtl92d_dm_txpower_tracking_callback_thermalmeter
>  * sort some declarations by length
>  * refactor _rtl92d_get_txpower_writeval_by_regulatory a little
> 
> Delete unused structs tag_dynamic_init_gain_operation_type_definition
> and swat.
> 
> Simplify rtl92d_fill_h2c_cmd a little and delete a pointless wrapper
> function.
> 

[...]

> @@ -137,16 +135,18 @@ static void rtl92d_dm_rxgain_tracking_thermalmeter(struct ieee80211_hw *hw)
>                 0x05, 0x04, 0x04, 0x03, 0x02
>         };
>         struct rtl_priv *rtlpriv = rtl_priv(hw);
> +       int i, idx;
>         u32 u4tmp;
> -       int i;
> 
> -       u4tmp = (index_mapping[(rtlpriv->efuse.eeprom_thermalmeter -
> -                               rtlpriv->dm.thermalvalue_rxgain)]) << 12;
> +       idx = rtlpriv->efuse.eeprom_thermalmeter - rtlpriv->dm.thermalvalue_rxgain;
> +       u4tmp = index_mapping[idx] << 12;
> +
>         rtl_dbg(rtlpriv, COMP_POWER_TRACKING, DBG_LOUD,
>                 "===> Rx Gain %x\n", u4tmp);
> +
>         for (i = RF90_PATH_A; i < rtlpriv->phy.num_total_rfpath; i++)
>                 rtl_set_rfreg(hw, i, 0x3C, RFREG_OFFSET_MASK,
> -                             (rtlpriv->phy.reg_rf3c[i] & (~(0xF000))) | u4tmp);
> +                             (rtlpriv->phy.reg_rf3c[i] & ~0xF000) | u4tmp);

There are similar instances like 'tmp1byte &= ~(BIT(0));'
Maybe, you can use coccinelle (spatch) as helper to convert all of them.

[...]

> @@ -713,22 +696,17 @@ void rtl92d_dm_false_alarm_counter_statistics(struct ieee80211_hw *hw)
>                 falsealm_cnt->cnt_cck_fail = 0;
>         }
> 
> -       /* reset false alarm counter registers */
> -       falsealm_cnt->cnt_all = falsealm_cnt->cnt_fast_fsync_fail +
> -                               falsealm_cnt->cnt_sb_search_fail +
> -                               falsealm_cnt->cnt_parity_fail +
> -                               falsealm_cnt->cnt_rate_illegal +
> -                               falsealm_cnt->cnt_crc8_fail +
> -                               falsealm_cnt->cnt_mcs_fail +
> +       falsealm_cnt->cnt_all = falsealm_cnt->cnt_ofdm_fail +
>                                 falsealm_cnt->cnt_cck_fail;

Not sure why you did this change.

[...]

> @@ -228,35 +238,25 @@ static void _rtl92d_fill_h2c_command(struct ieee80211_hw *hw,
>                         break;
>                 }
>         }
> +
>         while (!bwrite_success) {
>                 wait_writeh2c_limmit--;
>                 if (wait_writeh2c_limmit == 0) {
>                         pr_err("Write H2C fail because no trigger for FW INT!\n");
>                         break;
>                 }
> +
>                 boxnum = rtlhal->last_hmeboxnum;
>                 switch (boxnum) {
> -               case 0:
> -                       box_reg = REG_HMEBOX_0;
> -                       box_extreg = REG_HMEBOX_EXT_0;
> -                       break;
> -               case 1:
> -                       box_reg = REG_HMEBOX_1;
> -                       box_extreg = REG_HMEBOX_EXT_1;
> -                       break;
> -               case 2:
> -                       box_reg = REG_HMEBOX_2;
> -                       box_extreg = REG_HMEBOX_EXT_2;
> -                       break;
> -               case 3:
> -                       box_reg = REG_HMEBOX_3;
> -                       box_extreg = REG_HMEBOX_EXT_3;
> +               case 0 ... 3:
> +                       box_reg = REG_HMEBOX_0 + boxnum * 4;
> +                       box_extreg = REG_HMEBOX_EXT_0 + boxnum * 2;

Should be "* 4" as well?

box_extreg = REG_HMEBOX_EXT_0 + boxnum * 4;

>                         break;
>                 default:
> -                       pr_err("switch case %#x not processed\n",
> -                              boxnum);
> +                       pr_err("switch case %#x not processed\n", boxnum);
>                         break;
>                 }
> +
>                 isfw_read = _rtl92d_check_fw_read_last_h2c(hw, boxnum);
>                 while (!isfw_read) {
>                         wait_h2c_limmit--;
> @@ -266,78 +266,63 @@ static void _rtl92d_fill_h2c_command(struct ieee80211_hw *hw,
>                                         boxnum);
>                                 break;
>                         }
> +
>                         udelay(10);
> +
>                         isfw_read = _rtl92d_check_fw_read_last_h2c(hw, boxnum);
>                         u1b_tmp = rtl_read_byte(rtlpriv, 0x1BF);
>                         rtl_dbg(rtlpriv, COMP_CMD, DBG_LOUD,
>                                 "Waiting for FW read clear HMEBox(%d)!!! 0x1BF = %2x\n",
>                                 boxnum, u1b_tmp);
>                 }
> +
>                 if (!isfw_read) {
>                         rtl_dbg(rtlpriv, COMP_CMD, DBG_LOUD,
>                                 "Write H2C register BOX[%d] fail!!!!! Fw do not read.\n",
>                                 boxnum);
>                         break;
>                 }
> +
>                 memset(boxcontent, 0, sizeof(boxcontent));
>                 memset(boxextcontent, 0, sizeof(boxextcontent));
>                 boxcontent[0] = element_id;
> +
>                 rtl_dbg(rtlpriv, COMP_CMD, DBG_LOUD,
>                         "Write element_id box_reg(%4x) = %2x\n",
>                         box_reg, element_id);
> +
>                 switch (cmd_len) {
> -               case 1:
> -                       boxcontent[0] &= ~(BIT(7));
> -                       memcpy(boxcontent + 1, cmdbuffer + buf_index, 1);
> -                       for (idx = 0; idx < 4; idx++)
> -                               rtl_write_byte(rtlpriv, box_reg + idx,
> -                                              boxcontent[idx]);
> -                       break;
> -               case 2:
> -                       boxcontent[0] &= ~(BIT(7));
> -                       memcpy(boxcontent + 1, cmdbuffer + buf_index, 2);
> -                       for (idx = 0; idx < 4; idx++)
> -                               rtl_write_byte(rtlpriv, box_reg + idx,
> -                                              boxcontent[idx]);
> -                       break;
> -               case 3:
> -                       boxcontent[0] &= ~(BIT(7));
> -                       memcpy(boxcontent + 1, cmdbuffer + buf_index, 3);
> -                       for (idx = 0; idx < 4; idx++)
> -                               rtl_write_byte(rtlpriv, box_reg + idx,
> -                                              boxcontent[idx]);
> -                       break;
> -               case 4:
> -                       boxcontent[0] |= (BIT(7));
> -                       memcpy(boxextcontent, cmdbuffer + buf_index, 2);
> -                       memcpy(boxcontent + 1, cmdbuffer + buf_index + 2, 2);
> -                       for (idx = 0; idx < 2; idx++)
> -                               rtl_write_byte(rtlpriv, box_extreg + idx,
> -                                              boxextcontent[idx]);
> +               case 1 ... 3:
> +                       boxcontent[0] &= ~BIT(7);
> +                       memcpy(boxcontent + 1, cmdbuffer, cmd_len);
> +
>                         for (idx = 0; idx < 4; idx++)
>                                 rtl_write_byte(rtlpriv, box_reg + idx,
>                                                boxcontent[idx]);
>                         break;
> -               case 5:
> -                       boxcontent[0] |= (BIT(7));
> -                       memcpy(boxextcontent, cmdbuffer + buf_index, 2);
> -                       memcpy(boxcontent + 1, cmdbuffer + buf_index + 2, 3);
> +               case 4 ... 5:
> +                       boxcontent[0] |= BIT(7);
> +                       memcpy(boxextcontent, cmdbuffer, 2);
> +                       memcpy(boxcontent + 1, cmdbuffer + 2, cmd_len - 2);

After reading these statements of "case 4...5", I eventually know how BIT(7) works.
With BIT(7) additional two command buffers are introduced, and argument[0...1] are
put into the new buffers. Maybe, we can add a comment to help people to understand
this part.

case 1...3
  BOX:      | ID | A0 | A1 | A2 |
  BOX_EXT:  --- N/A ------

case 4...5
  * ID ext = ID | BIT(7)
  BOX:      | ID ext | A2 | A3 | A4 |
  BOX_EXT:  | A0     | A1 |

[...]