Received-SPF: pass (google.com: domain of linux-wireless+bounces-6461-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <ad6057a1-9d3b-4b2c-ac47-5df688f5bed5@gmail.com>
Date: Wed, 17 Apr 2024 20:13:51 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 5/6] wifi: rtlwifi: Clean up rtl8192d-common a bit
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: Larry Finger <Larry.Finger@lwfinger.net>,
 Stefan Lippers-Hollmann <s.l-h@gmx.de>,
 Christian Hewitt <chewitt@libreelec.tv>
References: <561cc9e4-3ddc-40cd-a076-2c14c44eea87@gmail.com>
 <4e47c56c-4b7f-4ae6-84b9-05f0cac97dac@gmail.com>
 <72bd1a7172154d4e9a79678e5151cdf3@realtek.com>
Content-Language: en-US
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <72bd1a7172154d4e9a79678e5151cdf3@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 17/04/2024 09:48, Ping-Ke Shih wrote:
> 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.
> 

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.