> -----Original Message-----
> From: Fiona Klute <fiona.klute@xxxxxx>
> Sent: Tuesday, February 6, 2024 3:06 AM
> To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx
> Cc: Kalle Valo <kvalo@xxxxxxxxxx>; Ulf Hansson <ulf.hansson@xxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx; Pavel
> Machek <pavel@xxxxxx>; Ondřej Jirman <megi@xxxxxx>
> Subject: Re: [PATCH 5/9] wifi: rtw88: Add rtw8703b.c
>
>
> I'm afraid I'm not familiar with the details either, but this is how the
> SDIO wifi device for the Pinephone is defined (in
> arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi, as of v6.8-rc2):
>
> &mmc1 {
> pinctrl-names = "default";
> pinctrl-0 = <&mmc1_pins>;
> vmmc-supply = <®_vbat_wifi>;
> vqmmc-supply = <®_dldo4>;
> bus-width = <4>;
> non-removable;
> status = "okay";
>
> rtl8723cs: wifi@1 {
I think rtl8723cs is module name of vendor driver, so will you add rtw88_8723ds?
> reg = <1>;
> };
> };
>
> As far as I understand the "reg = <1>;" line implies that the bootloader
> can provide some setup (like the MAC address). I hope someone with more
> knowledge can correct me or add missing details.
>
> >> drivers/net/wireless/realtek/rtw88/rtw8703b.c | 2106 +++++++++++++++++
> >> 1 file changed, 2106 insertions(+)
> >> create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8703b.c
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.c
> >> b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
> >> new file mode 100644
> >> index 0000000000..ac9b1bf6ea
> >> --- /dev/null
> >> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
> >> @@ -0,0 +1,2106 @@
> >> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> >> +/* Copyright Fiona Klute <fiona.klute@xxxxxx> */
> >> +
> >> +#include <linux/of_net.h>
> >> +#include "main.h"
> >> +#include "coex.h"
> >> +#include "debug.h"
> >> +#include "mac.h"
> >> +#include "phy.h"
> >> +#include "reg.h"
> >> +#include "rx.h"
> >> +#include "rtw8703b.h"
> >> +#include "rtw8703b_tables.h"
> >> +#include "rtw8723x.h"
> >> +
> >> +#define GET_RX_DESC_BW(rxdesc) \
> >> + (le32_get_bits(*((__le32 *)(rxdesc) + 0x04), GENMASK(31, 24)))
> >
> > use struct and le32_get_bits() directly.
>
> Do you mean to create a struct to represent the RX descriptor and then
> use le*_get_bits() on the fields to get the values? I could try, but I'd
> have to replace all the other GET_RX_DESC_* macros defined in rx.h (and
> shared by the other chip drivers), too, or it won't really make a
> difference (more below).
Like this:
88b9d8e6cf9c ("wifi: rtw88: use struct instead of macros to set TX desc")
It needs to modify all across chips.
>
> [...]
>
> >> +
> >> + if (ret != 0)
> >> + return ret;
> >> +
> >> +#ifdef CONFIG_OF
> >> + /* Prefer MAC from DT, if available. On some devices like the
> >> + * Pinephone that might be the only way to get a valid MAC.
> >> + */
> >> + struct device_node *node = rtwdev->dev->of_node;
> >
> > Should move this statement to topmost of this function? no compiler warning?
> >
> > Or, make an individual function to read mac addr from DT?
>
> I can move that to a separate function if you prefer, see below for the
> compiler warning.
Because this is CONFIG_OF chunk, it will look like below if you move declaration upward:
#ifdef CONFIG_OF
struct device_node *node = rtwdev->dev->of_node;
#endif
// other declaration ...
// other code
#ifdef CONFIG_OF
if (node) {
...
}
#endif
It seems like too much #ifdef. With a separate function, it can be single #ifdef.
That is my point.
>
> >> +
> >> + if (node) {
> >> + ret = of_get_mac_address(node, efuse->addr);
> >> + if (ret == 0) {
> >> + rtw_dbg(rtwdev, RTW_DBG_EFUSE,
> >> + "got wifi mac address from DT: %pM\n",
> >> + efuse->addr);
> >> + }
> >> + }
> >> +#endif /* CONFIG_OF */
> >> +
> >> + /* If TX power index table in EFUSE is invalid, fall back to
> >> + * built-in table.
> >> + */
> >> + u8 *pwr = (u8 *)efuse->txpwr_idx_table;
> >> + bool valid = false;
> >
> > I tend to move these declaration to top of this function too, but not sure why
> > compiler also doesn't warn this in my side. Seemingly kernel changes default
> > compiler flags?
>
> Yes, I learned about that while working on this driver. First the move
> to gnu11, and then removing -Wdeclaration-after-statement with
> b5ec6fd286dfa466f64cb0e56ed768092d0342ae in 6.5. The commit message says
> "It will still be recommeneded [sic!] to place declarations at the start
> of a scope where possible, but it will no longer be enforced", so I'll
> move these up.
Thanks for the info.
>
> For the struct device_node pointer I think it makes sense to leave the
> declaration within the #ifdef CONFIG_OF section (unless we restructure
> that into a separate function) because it's unused otherwise.
See my point above. But your point makes sense too.
>
> [...]
>
> >> +static void rtw8703b_query_rx_desc(struct rtw_dev *rtwdev, u8 *rx_desc,
> >> + struct rtw_rx_pkt_stat *pkt_stat,
> >> + struct ieee80211_rx_status *rx_status)
> >> +{
> >> + struct ieee80211_hdr *hdr;
> >> + u32 desc_sz = rtwdev->chip->rx_pkt_desc_sz;
> >> + u8 *phy_status = NULL;
> >> +
> >> + memset(pkt_stat, 0, sizeof(*pkt_stat));
> >> +
> >> + pkt_stat->phy_status = GET_RX_DESC_PHYST(rx_desc);
> >> + pkt_stat->icv_err = GET_RX_DESC_ICV_ERR(rx_desc);
> >> + pkt_stat->crc_err = GET_RX_DESC_CRC32(rx_desc);
> >> + pkt_stat->decrypted = !GET_RX_DESC_SWDEC(rx_desc) &&
> >> + GET_RX_DESC_ENC_TYPE(rx_desc) != RX_DESC_ENC_NONE;
> >> + pkt_stat->is_c2h = GET_RX_DESC_C2H(rx_desc);
> >> + pkt_stat->pkt_len = GET_RX_DESC_PKT_LEN(rx_desc);
> >> + pkt_stat->drv_info_sz = GET_RX_DESC_DRV_INFO_SIZE(rx_desc);
> >> + pkt_stat->shift = GET_RX_DESC_SHIFT(rx_desc);
> >> + pkt_stat->rate = GET_RX_DESC_RX_RATE(rx_desc);
> >> + pkt_stat->cam_id = GET_RX_DESC_MACID(rx_desc);
> >> + pkt_stat->ppdu_cnt = 0;
> >> + pkt_stat->tsf_low = GET_RX_DESC_TSFL(rx_desc);
> >
> > Could you add a separate patch to convert these macros to struct style?
> > It is fine to keep as it was, and do this conversion afterward.
>
> In principle yes, but as I mentioned above I'd basically have to
> reinvent all the definitions from rx.h to make it work, I'm not sure if
> that really simplifies things. If you want to refactor those I think
> it'd be best to do it for all chip drivers together.
Yes, for all chips.
>
> The GET_PHY_STAT_* macros are a different matter. The PHY status
> structure is different between 8703b and the other supported chips, so
> those could be replaced with a struct without duplication. Or at least
> mostly, some elements are bit fields or values with < 8 bits, where I
> think a macro is simpler than a struct with different definitions
> depending on endianess. I am worried about introducing an endianess
> error though, so I'd have to ask for careful review of such a patch.
I think we can keep it as it was for this patchset, and another patches later
to convert this kind of macros. Months ago, Kalle guided the rules how
rtw88/89 can do [1]. For me, I will convert macros to struct once I touch
them.
[1] https://lore.kernel.org/all/87a5zpb71j.fsf_-_@xxxxxxxxxx/
