Remove code that is depending on DBG_RX_SIGNAL_DISPLAY_RAW_DATA cflag
since it's not compiling and there is no reference for it's usage;

Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
---
The previous version suggested to fix the tiny error over the cflag usage.
Greg pointed at [1] that is not common to build using cflags. Since there
is no reference about this debug usage flag, I'm removing all the code depending on it.
Tks and regards.

[1] https://lore.kernel.org/lkml/20241125225308.8702-1-rodrigo.gobbi.7@gmail.com/t/#med5299468bc43fa89d18892d6d869a93d6138475
---
Changelog
v3: remove code as suggested;
v2: https://lore.kernel.org/lkml/20241125225308.8702-1-rodrigo.gobbi.7@gmail.com/t/#mf22f30a9c689bd793988d7e7a58c0b119206116c
v1: https://lore.kernel.org/linux-staging/2024112500-authentic-primarily-b5da@gregkh/T/#mea4fba3775a1015f345dfe322854c55db0cddf43
---
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c |  1 -
 drivers/staging/rtl8723bs/hal/hal_com.c       | 55 -------------------
 .../staging/rtl8723bs/hal/rtl8723b_rxdesc.c   |  4 --
 drivers/staging/rtl8723bs/include/hal_com.h   |  5 --
 drivers/staging/rtl8723bs/include/rtw_recv.h  | 18 ------
 5 files changed, 83 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 317f3db19397..952ce6dd5af9 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -4959,7 +4959,6 @@ void _linked_info_dump(struct adapter *padapter)
 					rtw_hal_get_def_var(padapter, HW_DEF_RA_INFO_DUMP, &i);
 			}
 		}
-		rtw_hal_set_def_var(padapter, HAL_DEF_DBG_RX_INFO_DUMP, NULL);
 	}
 }
 
diff --git a/drivers/staging/rtl8723bs/hal/hal_com.c b/drivers/staging/rtl8723bs/hal/hal_com.c
index 95fb38283c58..b41ec89932af 100644
--- a/drivers/staging/rtl8723bs/hal/hal_com.c
+++ b/drivers/staging/rtl8723bs/hal/hal_com.c
@@ -682,14 +682,6 @@ u8 SetHalDefVar(
 	u8 bResult = _SUCCESS;
 
 	switch (variable) {
-	case HAL_DEF_DBG_RX_INFO_DUMP:
-
-		if (odm->bLinked) {
-			#ifdef DBG_RX_SIGNAL_DISPLAY_RAW_DATA
-			rtw_dump_raw_rssi_info(adapter);
-			#endif
-		}
-		break;
 	case HW_DEF_ODM_DBG_FLAG:
 		ODM_CmnInfoUpdate(odm, ODM_CMNINFO_DBG_COMP, *((u64 *)value));
 		break;
@@ -879,53 +871,6 @@ void rtw_hal_check_rxfifo_full(struct adapter *adapter)
 	}
 }
 
-#ifdef DBG_RX_SIGNAL_DISPLAY_RAW_DATA
-void rtw_dump_raw_rssi_info(struct adapter *padapter)
-{
-	u8 isCCKrate, rf_path;
-	struct hal_com_data *pHalData =  GET_HAL_DATA(padapter);
-	struct rx_raw_rssi *psample_pkt_rssi = &padapter->recvpriv.raw_rssi_info;
-
-	isCCKrate = psample_pkt_rssi->data_rate <= DESC_RATE11M;
-
-	if (isCCKrate)
-		psample_pkt_rssi->mimo_signal_strength[0] = psample_pkt_rssi->pwdball;
-
-	for (rf_path = 0; rf_path < pHalData->NumTotalRFPath; rf_path++) {
-		if (!isCCKrate) {
-			netdev_dbg(padapter->pnetdev, ", rx_ofdm_pwr:%d(dBm), rx_ofdm_snr:%d(dB)\n",
-				   psample_pkt_rssi->ofdm_pwr[rf_path],
-				   psample_pkt_rssi->ofdm_snr[rf_path]);
-		}
-	}
-}
-
-void rtw_store_phy_info(struct adapter *padapter, union recv_frame *prframe)
-{
-	u8 isCCKrate, rf_path;
-	struct hal_com_data *pHalData =  GET_HAL_DATA(padapter);
-	struct rx_pkt_attrib *pattrib = &prframe->u.hdr.attrib;
-
-	struct odm_phy_info *pPhyInfo  = (PODM_PHY_INFO_T)(&pattrib->phy_info);
-	struct rx_raw_rssi *psample_pkt_rssi = &padapter->recvpriv.raw_rssi_info;
-
-	psample_pkt_rssi->data_rate = pattrib->data_rate;
-	isCCKrate = pattrib->data_rate <= DESC_RATE11M;
-
-	psample_pkt_rssi->pwdball = pPhyInfo->rx_pwd_ba11;
-	psample_pkt_rssi->pwr_all = pPhyInfo->recv_signal_power;
-
-	for (rf_path = 0; rf_path < pHalData->NumTotalRFPath; rf_path++) {
-		psample_pkt_rssi->mimo_signal_strength[rf_path] = pPhyInfo->rx_mimo_signal_strength[rf_path];
-		psample_pkt_rssi->mimo_signal_quality[rf_path] = pPhyInfo->rx_mimo_signal_quality[rf_path];
-		if (!isCCKrate) {
-			psample_pkt_rssi->ofdm_pwr[rf_path] = pPhyInfo->RxPwr[rf_path];
-			psample_pkt_rssi->ofdm_snr[rf_path] = pPhyInfo->RxSNR[rf_path];
-		}
-	}
-}
-#endif
-
 static u32 Array_kfreemap[] = {
 	0xf8, 0xe,
 	0xf6, 0xc,
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_rxdesc.c b/drivers/staging/rtl8723bs/hal/rtl8723b_rxdesc.c
index 717faebf8aca..db3d7d72bffa 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_rxdesc.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_rxdesc.c
@@ -67,8 +67,4 @@ void rtl8723b_process_phy_info(struct adapter *padapter, void *prframe)
 	/*  Check EVM */
 	/*  */
 	process_link_qual(padapter,  precvframe);
-	#ifdef DBG_RX_SIGNAL_DISPLAY_RAW_DATA
-	rtw_store_phy_info(padapter, prframe);
-	#endif
-
 }
diff --git a/drivers/staging/rtl8723bs/include/hal_com.h b/drivers/staging/rtl8723bs/include/hal_com.h
index 4db93484725f..258a74076dd9 100644
--- a/drivers/staging/rtl8723bs/include/hal_com.h
+++ b/drivers/staging/rtl8723bs/include/hal_com.h
@@ -149,11 +149,6 @@ bool eqNByte(u8 *str1, u8 *str2, u32 num);
 
 bool GetU1ByteIntegerFromStringInDecimal(char *str, u8 *in);
 
-#ifdef DBG_RX_SIGNAL_DISPLAY_RAW_DATA
-void rtw_store_phy_info(struct adapter *padapter, union recv_frame *prframe);
-void rtw_dump_raw_rssi_info(struct adapter *padapter);
-#endif
-
 #define		HWSET_MAX_SIZE			512
 
 void rtw_bb_rf_gain_offset(struct adapter *padapter);
diff --git a/drivers/staging/rtl8723bs/include/rtw_recv.h b/drivers/staging/rtl8723bs/include/rtw_recv.h
index 18dd1464e0c2..aa9f9d5ecd01 100644
--- a/drivers/staging/rtl8723bs/include/rtw_recv.h
+++ b/drivers/staging/rtl8723bs/include/rtw_recv.h
@@ -89,21 +89,6 @@ struct phy_info {
 	u8 btCoexPwrAdjust;
 };
 
-#ifdef DBG_RX_SIGNAL_DISPLAY_RAW_DATA
-struct rx_raw_rssi {
-	u8 data_rate;
-	u8 pwdball;
-	s8 pwr_all;
-
-	u8 mimo_signal_strength[4];/*  in 0~100 index */
-	u8 mimo_signal_quality[4];
-
-	s8 ofdm_pwr[4];
-	u8 ofdm_snr[4];
-
-};
-#endif
-
 struct rx_pkt_attrib	{
 	u16 pkt_len;
 	u8 physt;
@@ -221,9 +206,6 @@ struct recv_priv {
 	u8 signal_strength;
 	u8 signal_qual;
 	s8 rssi;	/* translate_percentage_to_dbm(ptarget_wlan->network.PhyInfo.SignalStrength); */
-	#ifdef DBG_RX_SIGNAL_DISPLAY_RAW_DATA
-	struct rx_raw_rssi raw_rssi_info;
-	#endif
 	/* s8 rxpwdb; */
 	s16 noise;
 	/* int RxSNRdB[2]; */
-- 
2.47.0

On Fri, Dec 06, 2024 at 07:53:15PM -0300, Rodrigo Gobbi wrote:
> Remove code that is depending on DBG_RX_SIGNAL_DISPLAY_RAW_DATA cflag
> since it's not compiling and there is no reference for it's usage;
> 
> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
> ---
> The previous version suggested to fix the tiny error over the cflag usage.
> Greg pointed at [1] that is not common to build using cflags. Since there
> is no reference about this debug usage flag, I'm removing all the code depending on it.
> Tks and regards.
> 
> [1] https://lore.kernel.org/lkml/20241125225308.8702-1-rodrigo.gobbi.7@gmail.com/t/#med5299468bc43fa89d18892d6d869a93d6138475
> ---
> Changelog
> v3: remove code as suggested;
> v2: https://lore.kernel.org/lkml/20241125225308.8702-1-rodrigo.gobbi.7@gmail.com/t/#mf22f30a9c689bd793988d7e7a58c0b119206116c
> v1: https://lore.kernel.org/linux-staging/2024112500-authentic-primarily-b5da@gregkh/T/#mea4fba3775a1015f345dfe322854c55db0cddf43
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c |  1 -
>  drivers/staging/rtl8723bs/hal/hal_com.c       | 55 -------------------
>  .../staging/rtl8723bs/hal/rtl8723b_rxdesc.c   |  4 --
>  drivers/staging/rtl8723bs/include/hal_com.h   |  5 --
>  drivers/staging/rtl8723bs/include/rtw_recv.h  | 18 ------
>  5 files changed, 83 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 317f3db19397..952ce6dd5af9 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -4959,7 +4959,6 @@ void _linked_info_dump(struct adapter *padapter)
>  					rtw_hal_get_def_var(padapter, HW_DEF_RA_INFO_DUMP, &i);
>  			}
>  		}
> -		rtw_hal_set_def_var(padapter, HAL_DEF_DBG_RX_INFO_DUMP, NULL);

When I'm reading this commit I think, "How is this related to removing the
ifdef?"  The commit message should answer any kind of reasonable questions
a review is probably going to ask but it doesn't give any information on
this.  I figured it out by reviewing the code, but I shouldn't *have* to.
It should be in the commit message.

This commit would easier to review if it were broken up in a different
way.

[patch 1] just delete DBG_RX_SIGNAL_DISPLAY_RAW_DATA ifdefed code
[patch 2] delete HAL_DEF_DBG_RX_INFO_DUMP and related code
[patch 3] delete SetHalDefVar() and all the callers

>  	}
>  }
>  
> diff --git a/drivers/staging/rtl8723bs/hal/hal_com.c b/drivers/staging/rtl8723bs/hal/hal_com.c
> index 95fb38283c58..b41ec89932af 100644
> --- a/drivers/staging/rtl8723bs/hal/hal_com.c
> +++ b/drivers/staging/rtl8723bs/hal/hal_com.c
> @@ -682,14 +682,6 @@ u8 SetHalDefVar(
>  	u8 bResult = _SUCCESS;
>  
>  	switch (variable) {
> -	case HAL_DEF_DBG_RX_INFO_DUMP:
> -
> -		if (odm->bLinked) {
> -			#ifdef DBG_RX_SIGNAL_DISPLAY_RAW_DATA
> -			rtw_dump_raw_rssi_info(adapter);
> -			#endif
> -		}
> -		break;

So just leave this like:

	case HAL_DEF_DBG_RX_INFO_DUMP:
		break;

Otherwise I have to wonder, are there any other callers which pass
HAL_DEF_DBG_RX_INFO_DUMP?  Because if there are, then now they go
to the default case and that triggers a bug.

patch 2 would delete all HAL_DEF_DBG_RX_INFO_DUMP related code.
That is an easy patch to review.  There are probably other unused
values in the enum.

patch 3 would delete SetHalDefVar() and all related code.  Again
pretty easy to review because any missed callers would trigger a
build error.

regards,
dan carpenter

> The commit message should answer any kind of reasonable questions
> a review is probably going to ask but it doesn't give any information on
> this.

I'll try to improve this for future patches, tks for pointing that out.

> This commit would easier to review if it were broken up in a different
> way.

Ok, in that way, a patch series is the best approach here, right? 
Despite the changes in the series being dependent on each other in this case.
Tks and regards.

On Mon, Dec 09, 2024 at 06:33:36PM -0300, Rodrigo Gobbi wrote:
> > The commit message should answer any kind of reasonable questions
> > a review is probably going to ask but it doesn't give any information on
> > this.
> 
> I'll try to improve this for future patches, tks for pointing that out.
> 
> > This commit would easier to review if it were broken up in a different
> > way.
> 
> Ok, in that way, a patch series is the best approach here, right?

I'm not your boss, right?  And I can't tell you what to do and what I'm
suggesting is more work.  If you just want to send patch 1 from the
series instead of all three patches, that's fine.  No one is going to
be annoyed by that.

In patch 1, you would only delete code which is obviously surrounded
by #ifdef DBG_RX_SIGNAL_DISPLAY_RAW_DATA so the case statement would
look like:

	case HAL_DEF_DBG_RX_INFO_DUMP:
		break;

It's basically the same as what you wrote but slightly smaller and
easier to review.  But yeah, we're not going to merge this as-is.

> Despite the changes in the series being dependent on each other in this case.

Yep.  Patchsets are generally dependent on each other.  The rule is that
each patch has to make sense on its own.  You can't break the build.  But
they work on top of each other and they're applied in order.

regards,
dan carpenter