Received-SPF: pass (google.com: domain of linux-wireless+bounces-8795-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <6ea2dd8b-fe9e-4434-b502-5badfe547c1f@gmail.com>
Date: Tue, 11 Jun 2024 16:33:19 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] wifi: rtw88: usb: Further limit the TX aggregation
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: Sascha Hauer <sha@pengutronix.de>
References: <0996d2d0-e7b8-4e43-ba12-63074ba9df1b@gmail.com>
 <dfb019b6-4285-4381-86d2-d85409abee4b@gmail.com>
 <3fedc4d56ac3414e8e500961b9850aa1@realtek.com>
Content-Language: en-US
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <3fedc4d56ac3414e8e500961b9850aa1@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 11/06/2024 06:26, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> @@ -2065,25 +2066,26 @@ const struct rtw_chip_info rtw8703b_hw_spec = {
>>         .bt_rssi_type = COEX_BTRSSI_RATIO,
>>         .ant_isolation = 15,
>>         .rssi_tolerance = 2,
>> -       .bt_rssi_step = bt_rssi_step_8703b,
>> -       .wl_rssi_step = wl_rssi_step_8703b,
>>         /* sant -> shared antenna, nsant -> non-shared antenna
>>          * Not sure if 8703b versions with non-shard antenna even exist.
>>          */
>>         .table_sant_num = ARRAY_SIZE(table_sant_8703b),
>> -       .table_sant = table_sant_8703b,
> 
> Not sure why you move array and array size apart. 
> No need to churn this as well as other similar stuffs. 
> 

I thought you wanted the members initialised in the same order
they appear in struct rtw_chip_info in main.h. Maybe I misunderstood.

> This patch can only change things required by what subject mention. Align these
> fields by another patch, or do it later. Both are fine to me. 
> 

Ah, it should be done in a different patch, okay.

I'm still not sure what you meant here:

>> diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
>> index 49894331f7b4..49a3fd4fb7dc 100644
>> --- a/drivers/net/wireless/realtek/rtw88/main.h
>> +++ b/drivers/net/wireless/realtek/rtw88/main.h
>> @@ -1197,6 +1197,8 @@ struct rtw_chip_info {
>>         u16 fw_fifo_addr[RTW_FW_FIFO_MAX];
>>         const struct rtw_fwcd_segs *fwcd_segs;
>>
>> +       u8 usb_tx_agg_desc_num;
>> +
>
> Please keep order of field and instance declaration, like rtw8703b_hw_spec.

and here:

>>  #define REG_DWBCN0_CTRL                0x0208
>>  #define BIT_BCN_VALID          BIT(16)
>>  #define REG_TXDMA_OFFSET_CHK   0x020C
>> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.c
>> b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
>> index 8919f9e11f03..222608de33cd 100644
>> --- a/drivers/net/wireless/realtek/rtw88/rtw8703b.c
>> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
>> @@ -2013,6 +2013,7 @@ const struct rtw_chip_info rtw8703b_hw_spec = {
>>         .tx_stbc = false,
>>         .max_power_index = 0x3f,
>>         .ampdu_density = IEEE80211_HT_MPDU_DENSITY_16,
>> +       .usb_tx_agg_desc_num = 1, /* Not sure if this chip has USB interface */
>
> The position to declare this field is very different from others. 
> 
> Not sure when we messed up the order, but please don't feel free to add one,
> keep the order.