RE: [PATCH 2/9] wifi: rtw88: Debug output for rtw8723x EFUSE

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

> -----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 2/9] wifi: rtw88: Debug output for rtw8723x EFUSE
> 
> Some 8703b chips contain invalid EFUSE data, getting detailed
> information is critical when analyzing issues caused by that.
> 
> Signed-off-by: Fiona Klute <fiona.klute@gmx.de>
> ---
> The TX power table debug output function (rtw8723x_debug_txpwr_limit)
> isn't specific to the chip family. Should I move it to debug.c
> (e.g. as rtw_debug_txpwr_limit_2g)?

I think no need. My another thinking is to add an debugfs entry to read
these stuff out, but if failed to probe we can't get these information. 

> 
>  drivers/net/wireless/realtek/rtw88/rtw8723x.c | 159 ++++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/rtw8723x.h |  11 ++
>  2 files changed, 170 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723x.c
> b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
> index 2b58064547..bca650c6bb 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8723x.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
> @@ -63,6 +63,163 @@ static void __rtw8723x_lck(struct rtw_dev *rtwdev)
>                 rtw_write8(rtwdev, REG_TXPAUSE, 0x00);
>  }
> 
> +#define DBG_EFUSE_VAL(map, name)                        \

I think we should not hide 'rtwdev'

> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, # name "=0x%x\n", \
> +               (map)->name)
> +#define DBG_EFUSE_2BYTE(map, name)                        \
> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, # name "=0x%x%x\n", \

Should format be '%02x%02x' for two bytes?

> +               (map)->name[0], (map)->name[1])
> +
> +static void rtw8723xe_efuse_debug(struct rtw_dev *rtwdev,
> +                                 struct rtw8723x_efuse *map)
> +{
> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, "mac_addr=%pM\n", map->e.mac_addr);
> +       DBG_EFUSE_2BYTE(map, e.vendor_id);
> +       DBG_EFUSE_2BYTE(map, e.device_id);
> +       DBG_EFUSE_2BYTE(map, e.sub_vendor_id);
> +       DBG_EFUSE_2BYTE(map, e.sub_device_id);
> +}
> +
> +static void rtw8723xu_efuse_debug(struct rtw_dev *rtwdev,
> +                                 struct rtw8723x_efuse *map)
> +{
> +       DBG_EFUSE_2BYTE(map, u.vendor_id);
> +       DBG_EFUSE_2BYTE(map, u.product_id);
> +       DBG_EFUSE_VAL(map, u.usb_option);
> +       rtw_dbg(rtwdev, RTW_DBG_EFUSE, "mac_addr=%pM\n", map->u.mac_addr);

Just curious why 'mac_addr' is the first one in rtw8723xe_efuse_debug(), but
the last one here?


> +}
> +

[...]