On 12/04/2024 11:22, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> 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@xxxxxxxxx>
>
> [...]
>
>> 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.
