On 17/04/2024 09:48, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> 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.
>
Ehh, I don't want to learn one more tool right now.
> [...]
>
>> @@ -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.
>
It's repeated code. Those things are already added up into
cnt_ofdm_fail a few lines above.
> [...]
>
>> @@ -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;
>
No, because they are only two bytes apart in this old chip:
#define REG_HMEBOX_EXT_0 0x0088
#define REG_HMEBOX_EXT_1 0x008A
#define REG_HMEBOX_EXT_2 0x008C
#define REG_HMEBOX_EXT_3 0x008E
>> 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 |
>
> [...]
>
Okay, I will add it.
