The previous implementation performs check for the band
in each iteration, which is unnecessary and further more
there is a else condition which will never get triggered,
since a check is done to see if the band is either 2G or
5G earlier and the band either be any of those 2. We can
refactor this by assigning a pointer to the appropriate
power offset array based on the band before the loop and
updating this.

Signed-off-by: Mohammed Anees <pvmohammedanees2003@gmail.com>
---
 drivers/net/wireless/realtek/rtw88/phy.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
index 37ef80c9091d..17d61f1d9257 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -1465,15 +1465,14 @@ static void rtw_phy_store_tx_power_by_rate(struct rtw_dev *rtwdev,
 		    rate_num > RTW_RF_PATH_MAX))
 		return;
 
+	s8 (*tx_pwr_by_rate_offset) = (band == PHY_BANK_2G)
+						? hal->tx_pwr_by_rate_offset_2g[rfpath]
+						: hal->tx_pwr_by_rate_offset_5g[rfpath];
+
 	for (i = 0; i < rate_num; i++) {
 		offset = pwr_by_rate[i];
 		rate = rates[i];
-		if (band == PHY_BAND_2G)
-			hal->tx_pwr_by_rate_offset_2g[rfpath][rate] = offset;
-		else if (band == PHY_BAND_5G)
-			hal->tx_pwr_by_rate_offset_5g[rfpath][rate] = offset;
-		else
-			continue;
+		tx_pwr_by_rate_offset[rate] = offset;
 	}
 }
 
-- 
2.47.0

Hi Mohammed,

kernel test robot noticed the following build errors:

[auto build test ERROR on wireless-next/main]
[also build test ERROR on wireless/main linus/master v6.12-rc3 next-20241016]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mohammed-Anees/wifi-rtw88-Refactor-looping-in-rtw_phy_store_tx_power_by_rate/20241016-140811
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link:    https://lore.kernel.org/r/20241016060605.11359-1-pvmohammedanees2003%40gmail.com
patch subject: [PATCH] wifi: rtw88: Refactor looping in rtw_phy_store_tx_power_by_rate
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20241017/202410171143.OnFlgIwK-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410171143.OnFlgIwK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410171143.OnFlgIwK-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/net/wireless/realtek/rtw88/phy.c: In function 'rtw_phy_store_tx_power_by_rate':
>> drivers/net/wireless/realtek/rtw88/phy.c:1468:48: error: 'PHY_BANK_2G' undeclared (first use in this function); did you mean 'PHY_BAND_2G'?
    1468 |         s8 (*tx_pwr_by_rate_offset) = (band == PHY_BANK_2G)
         |                                                ^~~~~~~~~~~
         |                                                PHY_BAND_2G
   drivers/net/wireless/realtek/rtw88/phy.c:1468:48: note: each undeclared identifier is reported only once for each function it appears in

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]


vim +1468 drivers/net/wireless/realtek/rtw88/phy.c

  1447	
  1448	static void rtw_phy_store_tx_power_by_rate(struct rtw_dev *rtwdev,
  1449						   u32 band, u32 rfpath, u32 txnum,
  1450						   u32 regaddr, u32 bitmask, u32 data)
  1451	{
  1452		struct rtw_hal *hal = &rtwdev->hal;
  1453		u8 rate_num = 0;
  1454		u8 rate;
  1455		u8 rates[RTW_RF_PATH_MAX] = {0};
  1456		s8 offset;
  1457		s8 pwr_by_rate[RTW_RF_PATH_MAX] = {0};
  1458		int i;
  1459	
  1460		rtw_phy_get_rate_values_of_txpwr_by_rate(rtwdev, regaddr, bitmask, data,
  1461							 rates, pwr_by_rate, &rate_num);
  1462	
  1463		if (WARN_ON(rfpath >= RTW_RF_PATH_MAX ||
  1464			    (band != PHY_BAND_2G && band != PHY_BAND_5G) ||
  1465			    rate_num > RTW_RF_PATH_MAX))
  1466			return;
  1467	
> 1468		s8 (*tx_pwr_by_rate_offset) = (band == PHY_BANK_2G)
  1469							? hal->tx_pwr_by_rate_offset_2g[rfpath]
  1470							: hal->tx_pwr_by_rate_offset_5g[rfpath];
  1471	
  1472		for (i = 0; i < rate_num; i++) {
  1473			offset = pwr_by_rate[i];
  1474			rate = rates[i];
  1475			tx_pwr_by_rate_offset[rate] = offset;
  1476		}
  1477	}
  1478	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Mohammed Anees <pvmohammedanees2003@gmail.com> wrote:
> 
> The previous implementation performs check for the band
> in each iteration, which is unnecessary and further more
> there is a else condition which will never get triggered,

I feel compilers can optimize the check for the band, and we can just remove
the else condition. Or 
   if (2ghz)
      foo_2g();
   else
      foo_5g();


> since a check is done to see if the band is either 2G or
> 5G earlier and the band either be any of those 2. We can
> refactor this by assigning a pointer to the appropriate
> power offset array based on the band before the loop and
> updating this.
> 
> Signed-off-by: Mohammed Anees <pvmohammedanees2003@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtw88/phy.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
> index 37ef80c9091d..17d61f1d9257 100644
> --- a/drivers/net/wireless/realtek/rtw88/phy.c
> +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> @@ -1465,15 +1465,14 @@ static void rtw_phy_store_tx_power_by_rate(struct rtw_dev *rtwdev,
>                     rate_num > RTW_RF_PATH_MAX))
>                 return;
> 
> +       s8 (*tx_pwr_by_rate_offset) = (band == PHY_BANK_2G)
> +                                               ? hal->tx_pwr_by_rate_offset_2g[rfpath]
> +                                               : hal->tx_pwr_by_rate_offset_5g[rfpath];
> +

Though -Wdeclaration-after-statement was dropped, still recommend to place
declarations at the beginning of function.

The operands ? and : should place at the end of statement. 

x = y ?
    z0 :
    z1;


>         for (i = 0; i < rate_num; i++) {
>                 offset = pwr_by_rate[i];
>                 rate = rates[i];
> -               if (band == PHY_BAND_2G)
> -                       hal->tx_pwr_by_rate_offset_2g[rfpath][rate] = offset;
> -               else if (band == PHY_BAND_5G)
> -                       hal->tx_pwr_by_rate_offset_5g[rfpath][rate] = offset;
> -               else
> -                       continue;
> +               tx_pwr_by_rate_offset[rate] = offset;
>         }
>  }
> 
> --
> 2.47.0

Oops, I sent over the wrong patch with typo,
I'll make sure to fix that in the next version.

> I feel compilers can optimize the check for the band, and we can just remove
> the else condition. Or
>   if (2ghz)
>      foo_2g();
>   else
>      foo_5g();

I do agree with that but I feel, it would be
better to make it independent of compiler
optimization, thoughts?

Let me know what you think is better, that is 
whether letting it be if - else, or using a
pointer.

thanks

Mohammed Anees <pvmohammedanees2003@gmail.com> wrote:
> Oops, I sent over the wrong patch with typo,
> I'll make sure to fix that in the next version.
> 
> > I feel compilers can optimize the check for the band, and we can just remove
> > the else condition. Or
> >   if (2ghz)
> >      foo_2g();
> >   else
> >      foo_5g();
> 
> I do agree with that but I feel, it would be
> better to make it independent of compiler
> optimization, thoughts?
> 
> Let me know what you think is better, that is
> whether letting it be if - else, or using a
> pointer.

Using a pointer looks delicate and optimized. The if-else code is simple to
read. Since this chunk is small, I don't bias one of them. If I must choose
one, I vote if-else.

Thank you for your feedback! I appreciate your perspective on this,
I'll stick to if-else then and remove the unnecessary else and 
send over the patch.

Thanks!