Received-SPF: pass (google.com: domain of linux-wireless+bounces-6462-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <fb3c2dcd-f9ea-434d-beaa-be3bdceb611e@gmail.com>
Date: Wed, 17 Apr 2024 20:29:20 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 6/6] wifi: rtlwifi: Adjust rtl8192d-common for USB
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>
 <2d0d22e1-c4c5-46cb-8d54-63a4a16d22c6@gmail.com>
 <44aae72b9e2c441f9d667d98d0e779f6@realtek.com>
Content-Language: en-US
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <44aae72b9e2c441f9d667d98d0e779f6@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 17/04/2024 10:33, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> 
>> A few of the shared functions need small changes for the USB driver:
>>  - firmware loading
>>  - efuse reading
>>  - rate mask updating
>>  - rf register reading
>>  - initial gain for scanning
>>
>> Also, add a few macros to wifi.h, add some new members to struct
>> rtl_priv, and initialise rtlhal.interfaceindex for USB devices.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>> v5:
>>  - No change.
>>
>> v4:
>>  - Update the commit message.
>>  - Move _rtl92d_pci_lock and _rtl92d_pci_unlock to phy_common.h to
>>    silence a checkpatch warning.
>>  - Move rate_mask declaration higher (sorted by length).
>>  - Add some bit definitions to reg.h.
>>  - Add to struct rtl_priv the things shared by the two USB interfaces
>>    of a dual MAC RTL8192DU:
>>         u32 *curveindex_2g;
>>         u32 *curveindex_5g;
>>         struct mutex *mutex_for_power_on_off;
>>         struct mutex *mutex_for_hw_init;
>>
>> v3:
>>  - Silence sparse warnings about locks in phy_common.c by using helper
>>    functions _rtl92d_pci_lock() and _rtl92d_pci_unlock().
>>
>> v2:
>>  - Delete the macro IS_NORMAL_CHIP. It wasn't actually used.
>>  - Add RTL_USB_DELAY_FACTOR macro.
>>  - Use FEN_CPUEN macro.
>>  - Make params_pci and params_usb arrays static const, and make
>>    rtl_get_hwinfo() take a const pointer.
>>  - Put the rate mask, macid, etc. in a struct in
>>    rtl92de_update_hal_rate_mask().
>>  - Use sizeof(rtlpriv->rate_mask) in memcpy instead of hardcoded 5.
>>  - Rename temp variable to phy_ctrl.
>>  - Define _TXDMA_*_MAP and _HPQ, _LPQ, _PUBQ macros using GENMASK and
>>    remove the leading underscore.
>>  - Delete _NPQ macro because it's not very useful.
> 
> [...]
> 
> 
>> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192d/fw_common.h
>> b/drivers/net/wireless/realtek/rtlwifi/rtl8192d/fw_common.h
>> index 33b8ba363ca2..3d994238985f 100644
>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192d/fw_common.h
>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192d/fw_common.h
>> @@ -25,6 +25,16 @@
>>  #define GET_FIRMWARE_HDR_SUB_VER(__fwhdr)              \
>>         le32_get_bits(*(__le32 *)((__fwhdr) + 4), GENMASK(23, 16))
>>
>> +#define RAID_MASK               GENMASK(31, 28)
>> +#define RATE_MASK_MASK          GENMASK(27, 0)
>> +#define SHORT_GI_MASK           BIT(5)
>> +#define MACID_MASK              GENMASK(4, 0)
>> +
>> +struct rtl92d_rate_mask_h2c {
>> +       __le32 rate_mask_and_raid;
>> +       u8 macid_and_short_gi;
>> +} __packed;
>> +
> 
> You asked me to drop a patch that looks very like this. Is this part correct?
> 

Yes, it is correct in rtl8192d-common. This is what rtl92d_fill_h2c_cmd()
expects. It was not correct in rtl8xxxu because rtl8xxxu_gen1_h2c_cmd()
expects something else.

> [...]
> 
>> @@ -2768,6 +2773,12 @@ struct rtl_priv {
>>          */
>>         bool use_new_trx_flow;
>>
>> +       /* For dual MAC RTL8192DU, things shared by the 2 USB interfaces */
>> +       u32 *curveindex_2g;
>> +       u32 *curveindex_5g;
>> +       struct mutex *mutex_for_power_on_off; /* for power on/off */
>> +       struct mutex *mutex_for_hw_init; /* for hardware init */
> 
> It would be better to introduce these locks along with consumers. Then, I will
> not miss to review them carefully.
> 

Okay, I will remove them from this patch set.

> By the way, I don't review remaining patches of v4 until this new (smaller)
> patchset gets merged. 
> 

That makes sense.