RE: [PATCH 4/9] wifi: rtw88: Add rtw8703b.h

[Ping-Ke Shih <pkshih@realtek.com>]

> -----Original Message-----
> From: Fiona Klute <fiona.klute@gmx.de>
> Sent: Tuesday, February 6, 2024 9:09 AM
> To: Ping-Ke Shih <pkshih@realtek.com>; linux-wireless@vger.kernel.org
> Cc: Kalle Valo <kvalo@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; Pavel
> Machek <pavel@ucw.cz>; Ondřej Jirman <megi@xff.cz>
> Subject: Re: [PATCH 4/9] wifi: rtw88: Add rtw8703b.h
> 
> Am 05.02.24 um 03:24 schrieb Ping-Ke Shih:
> >> -----Original Message-----
> >> From: Fiona Klute <fiona.klute@gmx.de>
> >> Sent: Friday, February 2, 2024 8:11 PM
> >> To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com>
> >> Cc: Kalle Valo <kvalo@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org;
> Pavel
> >> Machek <pavel@ucw.cz>; Ondřej Jirman <megi@xff.cz>; Fiona Klute <fiona.klute@gmx.de>
> >> Subject: [PATCH 4/9] wifi: rtw88: Add rtw8703b.h
> >>
> >> This is the main header for the new rtw88_8703b chip driver.
> >>
> >> Signed-off-by: Fiona Klute <fiona.klute@gmx.de>
> >> ---
> >>   drivers/net/wireless/realtek/rtw88/rtw8703b.h | 62 +++++++++++++++++++
> >>   1 file changed, 62 insertions(+)
> >>   create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8703b.h
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.h
> >> b/drivers/net/wireless/realtek/rtw88/rtw8703b.h
> >> new file mode 100644
> >> index 0000000000..f5ff23f2ee
> >> --- /dev/null
> >> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.h
> >> @@ -0,0 +1,62 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> >> +/* Copyright Fiona Klute <fiona.klute@gmx.de> */
> >> +
> >> +#ifndef __RTW8703B_H__
> >> +#define __RTW8703B_H__
> >> +
> >> +extern const struct rtw_chip_info rtw8703b_hw_spec;
> >> +
> >> +/* phy status parsing */
> >> +#define GET_PHY_STAT_AGC_GAIN_A(phy_stat)                                   \
> >> +       (le32_get_bits(*((__le32 *)(phy_stat) + 0x00), GENMASK(6, 0)))
> >
> > We are planning to use struct and le32_get_bits() directly, so don't introduce
> > this old style anymore. An example is
> >
> > struct rtw8703b_phy_stat {
> >       __le32 w0;
> >       __le32 w1;
> >       ...
> > };
> >
> > #define RTW8703B_PHY_STAT_W0_AGC_GAIN_A GENMASK(6, 0)
> >
> > val_s8 = le32_get_bits(stat->w0, RTW8703B_PHY_STAT_W0_AGC_GAIN_A);
> 
> Sorry, of all your mails this one got stuck in the spam filter. This
> answers my question about what you meant by struct style, I hadn't
> thought of using __le types. I assume you'd still want to use
> appropriately sized types/arrays for elements that are multiples of one
> byte?

Not sure "multiples of one byte" means. I guess you want to use something like
u8 or __le16 for the elements that aren't required bit access, right? 
I'd say it is hard to define single one rule for all cases. 

Example 1-1 (fake): 
struct rtw8703b_phy_stat {
	u8 mac_id;
	u8 rssi;
	u8 evm;
	u8 evm_2;
	...
} __packed;

Example 1-2 (fake): 
struct rtw8703b_phy_stat {
	__le32 w0;
	...
} __packed;

#define PHY_STAT_W0_MACID GENMASK(7, 0)
#define PHY_STAT_W0_RSSI GENMASK(15, 8)
#define PHY_STAT_W0_EVM GENMASK(23, 16)
#define PHY_STAT_W0_EVM_2 GENMASK(31,24)

It is clear that example 1-1 is better than 1-2. 

Example 2-1 (fake): 
struct rtw8703b_phy_stat {
	u8 mac_id;
	u8 rssi;
	__le16 phy_st;		// evm: 7, evm_2: 7, rsvd :2
	...
} __packed;

#define PHY_ST_EVM GENMASK(6, 0)
#define PHY_ST_EVM_2 GENMASK(13, 7)


Example 2-2 (fake): 
struct rtw8703b_phy_stat {
	__le32 w0;
	...
} __packed;

#define PHY_STAT_W0_MACID GENMASK(7, 0)
#define PHY_STAT_W0_RSSI GENMASK(15, 8)
#define PHY_STAT_W0_EVM GENMASK(22, 16)
#define PHY_STAT_W0_EVM_2 GENMASK(29, 23)


Compare 2-1 and 2-2, it would be hard to say which one is better, because 2-1
mixes u8 and bit field. This is just a simple example, fields of real struct
are much more, so normally I use 1-2 and 2-2 styles.