All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kalle Valo <kvalo@codeaurora.org>
To: Ping-Ke Shih <pkshih@realtek.com>
Cc: <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v6 02/24] rtw89: add BT coexistence files
Date: Fri, 01 Oct 2021 18:26:37 +0300	[thread overview]
Message-ID: <87sfxkix5e.fsf@codeaurora.org> (raw)
In-Reply-To: <20210820043538.12424-3-pkshih@realtek.com> (Ping-Ke Shih's message of "Fri, 20 Aug 2021 12:35:16 +0800")

Ping-Ke Shih <pkshih@realtek.com> writes:

> BT coexistence uses TDMA-based mechanism to coordinate with WiFi and BT.
> Now, we implement basic coexistence features for wide use cases, such as
> HID and A2DP. More will be supported later.
>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

A small tip for future drivers, try to remove all the optional features
from the driver as much possible and keep only the absolutely needed
features to get ping working. For example this file was pain to review
and I suspect coex support could have been submitted separately.

But don't change anything for this driver, this comment is for rtw90 :)

> +struct rtw89_btc_fbtc_cysta_cpu {
> +	u8 fver;
> +	u8 rsvd;
> +	u16 cycles;
> +	u16 cycles_a2dp[CXT_FLCTRL_MAX];
> +	u16 a2dpept;
> +	u16 a2dpeptto;
> +	u16 tavg_cycle[CXT_MAX];
> +	u16 tmax_cycle[CXT_MAX];
> +	u16 tmaxdiff_cycle[CXT_MAX];
> +	u16 tavg_a2dp[CXT_FLCTRL_MAX];
> +	u16 tmax_a2dp[CXT_FLCTRL_MAX];
> +	u16 tavg_a2dpept;
> +	u16 tmax_a2dpept;
> +	u16 tavg_lk;
> +	u16 tmax_lk;
> +	u32 slot_cnt[CXST_MAX];
> +	u32 bcn_cnt[CXBCN_MAX];
> +	u32 leakrx_cnt;
> +	u32 collision_cnt;
> +	u32 skip_cnt;
> +	u32 exception;
> +	u32 except_cnt;
> +#if (FCXCYSTA_VER > 1)
> +	u16 tslot_cycle[BTC_CYCLE_SLOT_MAX];
> +#endif
> +};

Please remove the if check, in all cases:

coex.c.789:#if (FCXCYSTA_VER > 1)
coex.c.829:#if (FCXCYSTA_VER > 1)
core.h.1456:#if (FCXCYSTA_VER > 1)

> +	memcpy((void *)pfinfo, (void *)rpt_content, pcinfo->req_len);

Please remove the casts.

> +	memcpy((void *)ptr, chip->mon_reg, n * ulen);

Here too.

> +#define rtw89_btc_wl_rx_gain(rtwdev, level)  do {} while (0)

What's the use of this? Please remove.

> +static void _set_bt_tx_power(struct rtw89_dev *rtwdev, u8 level)
> +{
> +	struct rtw89_btc *btc = &rtwdev->btc;
> +	struct rtw89_btc_bt_info *bt = &btc->cx.bt;
> +	u8 buf = 0;

Nitpicking, but no need to initialise buf here.

> +	switch (bw) {
> +	case RTW89_CHANNEL_WIDTH_20:
> +#ifdef BTC_NON_SHARED_ANT_FREERUN
> +		bw = 48;
> +#else
> +		bw = 20 + chip->afh_guard_ch * 2;
> +#endif

No ifdefs like this. Please remove.

> +#define _set_bt_slot_req(rtwdev) do {} while (0)

I can't see why this is needed, please remove.

> +#define _get_wl_nhm_dbm(rtwdev) do {} while (0)

This too.

> +	/* ======= parse raw info low-Byte2 ======= */
> +	/* ======= parse raw info low-Byte3 ======= */
> +	/* ======= parse raw info high-Byte0 ======= */
> +	/* ======= parse raw info high-Byte1 ======= */

No need to use '=' characters in comments.

> +#define _update_bt_psd(rtwdev, buf, len) do {} while (0)
> +#define _update_offload_runinfo(rtwdev, buf, len) do {} while (0)

And more of these, remove.

> +void rtw89_btc_c2h_handle(struct rtw89_dev *rtwdev, struct sk_buff *skb,
> +			  u32 len, u8 class, u8 func)
> +{
> +	/* The below is just sample code. Don't use magic number in your release
> +	 * version.
> +	 */
> +	struct rtw89_btc *btc = &rtwdev->btc;
> +	struct rtw89_btc_btf_fwinfo *pfwinfo = &btc->fwinfo;
> +
> +	/* note: 'len' includes header, so 'buf' length is 'len - 8' */
> +	u8 *buf = &skb->data[8];	/* size of C2H header is 8 */
> +
> +	rtw89_debug(rtwdev, RTW89_DBG_BTC,
> +		    "[BTC], %s(): C2H BT len:%d class:%d fun:%d\n",
> +		    __func__, len, class, func);
> +
> +	if (class != 0x12)
> +		return;

A magic number, a proper define would document better what's happening.

> +
> +	switch (func) {
> +	case BTF_EVNT_RPT:
> +	case BTF_EVNT_BUF_OVERFLOW:
> +		pfwinfo->event[func]++;
> +		/* Don't need rtw89_leave_ps_mode() */
> +		btc_fw_event(rtwdev, func, buf, len);
> +		break;
> +	case BTF_EVNT_BT_INFO:
> +		rtw89_debug(rtwdev, RTW89_DBG_BTC,
> +			    "[BTC], handle C2H BT INFO with data %8ph\n", buf);
> +		btc->cx.cnt_bt[BTC_BCNT_INFOUPDATE]++;
> +		rtw89_leave_ps_mode(rtwdev);
> +		_update_bt_info(rtwdev, buf, len);
> +		break;
> +	case BTF_EVNT_BT_SCBD:
> +		rtw89_debug(rtwdev, RTW89_DBG_BTC,
> +			    "[BTC], handle C2H BT SCBD with data %8ph\n", buf);
> +		btc->cx.cnt_bt[BTC_BCNT_SCBDUPDATE]++;
> +		rtw89_leave_ps_mode(rtwdev);
> +		_update_bt_scbd(rtwdev, false);
> +		break;
> +	case BTF_EVNT_BT_PSD:
> +		_update_bt_psd(rtwdev, buf, len);
> +		break;
> +	case BTF_EVNT_BT_REG:
> +		btc->dbg.rb_done = true;
> +		btc->dbg.rb_val = ((buf[3] << 24) | (buf[2] << 16) |
> +				   (buf[1] << 8) | (buf[0]));

So are just changing endian or what? le_to_cpu() etc?

> +#define _get_bt_polt_cnt(rtwdev, phy, polt_cnt) do {} while (0)

And again, remove.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

  reply	other threads:[~2021-10-01 15:26 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20  4:35 [PATCH v6 00/24] rtw89: add Realtek 802.11ax driver Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 01/24] rtw89: add CAM files Ping-Ke Shih
2021-10-01 14:46   ` Kalle Valo
2021-08-20  4:35 ` [PATCH v6 02/24] rtw89: add BT coexistence files Ping-Ke Shih
2021-10-01 15:26   ` Kalle Valo [this message]
2021-10-01 17:40     ` Small driver submissions and long feedback cycles Brian Norris
2021-08-20  4:35 ` [PATCH v6 03/24] rtw89: add core and trx files Ping-Ke Shih
2021-10-01 16:26   ` Kalle Valo
2021-10-05  7:16     ` Pkshih
2021-10-05  7:46       ` Kalle Valo
2021-10-05  8:42         ` Arnd Bergmann
2021-10-05  9:32           ` Pkshih
2021-10-05  9:59             ` Arnd Bergmann
2021-10-06  1:35               ` Pkshih
2021-10-06  7:32                 ` Arnd Bergmann
2021-10-06  8:19                   ` Pkshih
2021-08-20  4:35 ` [PATCH v6 04/24] rtw89: add debug files Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 05/24] rtw89: add efuse files Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 06/24] rtw89: add files to download and communicate with firmware Ping-Ke Shih
2021-10-01 15:55   ` Kalle Valo
2021-08-20  4:35 ` [PATCH v6 07/24] rtw89: add MAC files Ping-Ke Shih
2021-10-01 16:13   ` Kalle Valo
2021-08-20  4:35 ` [PATCH v6 08/24] rtw89: implement mac80211 ops Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 09/24] rtw89: add pci files Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 10/24] rtw89: add phy files Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 11/24] rtw89: define register names Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 12/24] rtw89: add regulatory support Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 13/24] rtw89: 8852a: add 8852a specific files Ping-Ke Shih
2021-10-01 16:20   ` Kalle Valo
2021-08-20  4:35 ` [PATCH v6 14/24] rtw89: 8852a: add 8852a RFK files Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 15/24] rtw89: 8852a: add 8852a RFK tables Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 16/24] rtw89: 8852a: add 8852a tables (1 of 5) Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 17/24] rtw89: 8852a: add 8852a tables (2 " Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 18/24] rtw89: 8852a: add 8852a tables (3 " Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 19/24] rtw89: 8852a: add 8852a tables (4 " Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 20/24] rtw89: 8852a: add 8852a tables (5 " Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 21/24] rtw89: add ser to recover error reported by firmware Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 22/24] rtw89: add PS files Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 23/24] rtw89: add SAR files Ping-Ke Shih
2021-08-20  4:35 ` [PATCH v6 24/24] rtw89: add Kconfig and Makefile Ping-Ke Shih
2021-08-22  3:43   ` kernel test robot
2021-08-23  1:37     ` Pkshih
2021-10-01 15:57   ` Kalle Valo
2021-10-01 16:34 ` [PATCH v6 00/24] rtw89: add Realtek 802.11ax driver Kalle Valo
2021-10-01 16:42   ` Larry Finger
2021-10-01 16:46     ` Kalle Valo
2021-10-01 17:18       ` Larry Finger
2021-10-05  5:46         ` Kalle Valo
2021-10-04  6:46   ` Pkshih
2021-10-05  5:52     ` Kalle Valo
2021-10-06  0:10       ` Brian Norris
2021-10-08  4:14         ` Pkshih
2021-10-08  4:11       ` Pkshih
2021-10-09  8:28         ` Kalle Valo
2021-10-12  1:53           ` Pkshih

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87sfxkix5e.fsf@codeaurora.org \
    --to=kvalo@codeaurora.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=pkshih@realtek.com \
    --subject='Re: [PATCH v6 02/24] rtw89: add BT coexistence files' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.