Received-SPF: pass (google.com: domain of linux-wireless+bounces-6291-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <601c4f3c-8069-48ea-aae8-b1e814eba283@gmail.com>
Date: Sun, 14 Apr 2024 21:46:56 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
Subject: Re: [PATCH v4 03/14] wifi: rtlwifi: Move code from rtl8192de to
 rtl8192d-common
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: <91d932b3-5c72-4416-920e-f2bf4fc9b039@gmail.com>
 <6e18882f-3da3-4c17-8bce-4ebffe022713@gmail.com>
 <c2956068d98142e3b527f1faa94ce2aa@realtek.com>
Content-Language: en-US
In-Reply-To: <c2956068d98142e3b527f1faa94ce2aa@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 12/04/2024 11:22, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> 
>>
>> Create the new module rtl8192d-common and move some code into it from
>> rtl8192de. Now the rtl8192de driver (PCI) and the new rtl8192du driver
>> (USB) can share some of the code.
>>
>> This is mostly the code that required little effort to make it
>> shareable. There are a few more functions which they could share, with
>> more changes.
>>
>> Add phy_iq_calibrate member to struct rtl_hal_ops to allow moving the
>> TX power tracking code from dm.c.
>>
>> The other changes in this patch are adjusting whitespace, renaming some
>> functions, making some arrays const, and making checkpatch.pl less
>> unhappy.
>>
>> rtl8192de is compile-tested only. rtl8192d-common is tested with the
>> new rtl8192du driver.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> 
> [...]
> 
>> diff --git a/drivers/net/wireless/realtek/rtlwifi/Kconfig
>> b/drivers/net/wireless/realtek/rtlwifi/Kconfig
>> index 9f6a4e35543c..2319eaa8845a 100644
>> --- a/drivers/net/wireless/realtek/rtlwifi/Kconfig
>> +++ b/drivers/net/wireless/realtek/rtlwifi/Kconfig
>> @@ -37,6 +37,7 @@ config RTL8192SE
>>  config RTL8192DE
>>         tristate "Realtek RTL8192DE/RTL8188DE PCIe Wireless Network Adapter"
>>         depends on PCI
>> +       select RTL8192D_COMMON
>>         select RTLWIFI
>>         select RTLWIFI_PCI
>>         help
>> @@ -142,6 +143,11 @@ config RTL8192C_COMMON
>>         depends on RTL8192CE || RTL8192CU
>>         default y
>>
>> +config RTL8192D_COMMON
>> +       tristate
>> +       depends on RTL8192DE
>> +       default y
>> +
> 
> Existing RTL8723_COMMON also uses both 'depends on' and 'select', which are
> mutual reference, so I think choosing only one of them would be better.
> 
>>  config RTL8723_COMMON
>>         tristate
>>         depends on RTL8723AE || RTL8723BE
> 

I'm not sure about this. Isn't there a good reason why the
"common" drivers do it this way?

[...]
 
>> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192d/trx_common.h
>> b/drivers/net/wireless/realtek/rtlwifi/rtl8192d/trx_common.h
>> new file mode 100644
>> index 000000000000..467e285280dc
>> --- /dev/null
>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192d/trx_common.h
> 
> [...]
> 
>> +struct rx_fwinfo_92d {
>> +       u8 gain_trsw[4];
>> +       u8 pwdb_all;
>> +       u8 cfosho[4];
>> +       u8 cfotail[4];
>> +       s8 rxevm[2];
>> +       s8 rxsnr[4];
>> +       u8 pdsnr[2];
>> +       u8 csi_current[2];
>> +       u8 csi_target[2];
>> +       u8 sigevm;
>> +       u8 max_ex_pwr;
>> +       u8 ex_intf_flag:1;
>> +       u8 sgi_en:1;
>> +       u8 rxsc:2;
>> +       u8 reserve:4;
>> +} __packed;
>> +
>> +struct rx_desc_92d {
>> +       u32 length:14;
>> +       u32 crc32:1;
>> +       u32 icverror:1;
>> +       u32 drv_infosize:4;
>> +       u32 security:3;
>> +       u32 qos:1;
>> +       u32 shift:2;
>> +       u32 phystatus:1;
>> +       u32 swdec:1;
>> +       u32 lastseg:1;
>> +       u32 firstseg:1;
>> +       u32 eor:1;
>> +       u32 own:1;
>> +
>> +       u32 macid:5;
>> +       u32 tid:4;
>> +       u32 hwrsvd:5;
>> +       u32 paggr:1;
>> +       u32 faggr:1;
>> +       u32 a1_fit:4;
>> +       u32 a2_fit:4;
>> +       u32 pam:1;
>> +       u32 pwr:1;
>> +       u32 moredata:1;
>> +       u32 morefrag:1;
>> +       u32 type:2;
>> +       u32 mc:1;
>> +       u32 bc:1;
>> +
>> +       u32 seq:12;
>> +       u32 frag:4;
>> +       u32 nextpktlen:14;
>> +       u32 nextind:1;
>> +       u32 rsvd:1;
>> +
>> +       u32 rxmcs:6;
>> +       u32 rxht:1;
>> +       u32 amsdu:1;
>> +       u32 splcp:1;
>> +       u32 bandwidth:1;
>> +       u32 htc:1;
>> +       u32 tcpchk_rpt:1;
>> +       u32 ipcchk_rpt:1;
>> +       u32 tcpchk_valid:1;
>> +       u32 hwpcerr:1;
>> +       u32 hwpcind:1;
>> +       u32 iv0:16;
>> +
>> +       u32 iv1;
>> +
>> +       u32 tsfl;
>> +
>> +       u32 bufferaddress;
>> +       u32 bufferaddress64;
>> +
>> +} __packed;
> 
> These are not compatible to big-endian machine. 

rx_desc_92d is almost unused, so I deleted it and used the
get_rx_desc_* functions instead. rx_fwinfo_92d is easier to fix.

> 
> [...]
> 
> After going through this patch again, I really want you (or someone) can help
> to refine this driver. The major problem is readability including
>  - should declare in reverse X'mas tree
>  - large function
>  - no proper blank lines
>  - unreadable indent to fit 80 characters limit of checkpatch
>  - ...
> 
> These problems are existing for a long time, and now may be a good time to
> adjust some of them we are touching.
> 
> A way to adjust them is to add patches to convert above items I list 
> one by one. I can ignore flaws (not introduce by this patch though) in this
> patch, and review coming patches (but still hope in the same patchset) that
> fix these flaws. Since this patchset contains 14 patches already, maybe we
> should have first patchset to adjust codes you will touch, and then add 8192DU
> by second patchset. 
> 
> Ping-Ke 
> 

Okay, I did all the things you mentioned, and a bit more.