> -----Original Message-----
> From: Fiona Klute <fiona.klute@xxxxxx>
> Sent: Tuesday, February 6, 2024 9:09 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 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@xxxxxx>
> >> Sent: Friday, February 2, 2024 8:11 PM
> >> To: linux-wireless@xxxxxxxxxxxxxxx; Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> >> Cc: Kalle Valo <kvalo@xxxxxxxxxx>; Ulf Hansson <ulf.hansson@xxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx;
> Pavel
> >> Machek <pavel@xxxxxx>; Ondřej Jirman <megi@xxxxxx>; Fiona Klute <fiona.klute@xxxxxx>
> >> 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@xxxxxx>
> >> ---
> >> 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@xxxxxx> */
> >> +
> >> +#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.
