diff mbox series

[linux-next] ice: use string choice helpers

Message ID 20241027141907.503946-1-prosunofficial@gmail.com
State Under Review
Delegated to: Anthony Nguyen
Headers show
Series [linux-next] ice: use string choice helpers | expand

Commit Message

R Sundar Oct. 27, 2024, 2:19 p.m. UTC
Use string choice helpers for better readability.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@inria.fr>
Closes: https://lore.kernel.org/r/202410121553.SRNFzc2M-lkp@intel.com/
Signed-off-by: R Sundar <prosunofficial@gmail.com>
---

Reported in linux repository.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

cocci warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/intel/ice/ice_ptp_hw.c:396:4-22: opportunity for str_enabled_disabled(dw24 . ts_pll_enable)
   drivers/net/ethernet/intel/ice/ice_ptp_hw.c:474:4-22: opportunity for str_enabled_disabled(dw24 . ts_pll_enable)

vim +396 drivers/net/ethernet/intel/ice/ice_ptp_hw.c


 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

R Sundar Oct. 29, 2024, 4:37 p.m. UTC | #1
On 28/10/24 15:24, Przemek Kitszel wrote:
> On 10/27/24 15:19, R Sundar wrote:
>> Use string choice helpers for better readability.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Julia Lawall <julia.lawall@inria.fr>
>> Closes: https://lore.kernel.org/r/202410121553.SRNFzc2M-lkp@intel.com/
>> Signed-off-by: R Sundar <prosunofficial@gmail.com>
>> ---
> 
> thanks, this indeed covers all "enabled/disabled" cases, so:
> Acked-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
Hi,

Thanks for comments.

> for future submissions for Intel Ethernet drivers please use the
> iwl-next (or iwl-net) target trees.
> 

Sure. Noted.

> There are also other cases that we could cover ON/OFF etc
> 
>>
>> Reported in linux repository.
>>
>> tree:   
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>>
>> cocci warnings: (new ones prefixed by >>)
>>>> drivers/net/ethernet/intel/ice/ice_ptp_hw.c:396:4-22: opportunity 
>>>> for str_enabled_disabled(dw24 . ts_pll_enable)
>>     drivers/net/ethernet/intel/ice/ice_ptp_hw.c:474:4-22: opportunity 
>> for str_enabled_disabled(dw24 . ts_pll_enable)
>>
>> vim +396 drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>>
>>
>>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c 
>> b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> index da88c6ccfaeb..d8d3395e49c3 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>> @@ -393,7 +393,7 @@ static int ice_cfg_cgu_pll_e82x(struct ice_hw *hw,
>>       /* Log the current clock configuration */
>>       ice_debug(hw, ICE_DBG_PTP, "Current CGU configuration -- %s, 
>> clk_src %s, clk_freq %s, PLL %s\n",
>> -          dw24.ts_pll_enable ? "enabled" : "disabled",
>> +          str_enabled_disabled(dw24.ts_pll_enable),
>>             ice_clk_src_str(dw24.time_ref_sel),
>>             ice_clk_freq_str(dw9.time_ref_freq_sel),
>>             bwm_lf.plllock_true_lock_cri ? "locked" : "unlocked");
> 
> perhaps locked/unlocked could be added into string_choices.h
> 

Sure, Can I add locked/unlocked changes in linux-next repository and use 
suggested-by Tag?


>> @@ -471,7 +471,7 @@ static int ice_cfg_cgu_pll_e82x(struct ice_hw *hw,
>>       /* Log the current clock configuration */
>>       ice_debug(hw, ICE_DBG_PTP, "New CGU configuration -- %s, clk_src 
>> %s, clk_freq %s, PLL %s\n",
>> -          dw24.ts_pll_enable ? "enabled" : "disabled",
>> +          str_enabled_disabled(dw24.ts_pll_enable),
>>             ice_clk_src_str(dw24.time_ref_sel),
>>             ice_clk_freq_str(dw9.time_ref_freq_sel),
>>             bwm_lf.plllock_true_lock_cri ? "locked" : "unlocked");
>> @@ -548,7 +548,7 @@ static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw,
>>       /* Log the current clock configuration */
>>       ice_debug(hw, ICE_DBG_PTP, "Current CGU configuration -- %s, 
>> clk_src %s, clk_freq %s, PLL %s\n",
>> -          dw24.ts_pll_enable ? "enabled" : "disabled",
>> +          str_enabled_disabled(dw24.ts_pll_enable),
>>             ice_clk_src_str(dw23.time_ref_sel),
>>             ice_clk_freq_str(dw9.time_ref_freq_sel),
>>             ro_lock.plllock_true_lock_cri ? "locked" : "unlocked");
>> @@ -653,7 +653,7 @@ static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw,
>>       /* Log the current clock configuration */
>>       ice_debug(hw, ICE_DBG_PTP, "New CGU configuration -- %s, clk_src 
>> %s, clk_freq %s, PLL %s\n",
>> -          dw24.ts_pll_enable ? "enabled" : "disabled",
>> +          str_enabled_disabled(dw24.ts_pll_enable),
>>             ice_clk_src_str(dw23.time_ref_sel),
>>             ice_clk_freq_str(dw9.time_ref_freq_sel),
>>             ro_lock.plllock_true_lock_cri ? "locked" : "unlocked");
>
Przemek Kitszel Oct. 30, 2024, 7:19 a.m. UTC | #2
On 10/29/24 17:37, R Sundar wrote:
> On 28/10/24 15:24, Przemek Kitszel wrote:
>> On 10/27/24 15:19, R Sundar wrote:
>>> Use string choice helpers for better readability.

>>>             bwm_lf.plllock_true_lock_cri ? "locked" : "unlocked");
>>
>> perhaps locked/unlocked could be added into string_choices.h
>>
> 
> Sure, Can I add locked/unlocked changes in linux-next repository and use 
> suggested-by Tag?

sure, that's way to go
but please first check if there are any other users (despite this
driver)

> 
> 
>>> @@ -471,7 +471,7 @@ static int ice_cfg_cgu_pll_e82x(struct ice_hw *hw,
>>>       /* Log the current clock configuration */
>>>       ice_debug(hw, ICE_DBG_PTP, "New CGU configuration -- %s, 
>>> clk_src %s, clk_freq %s, PLL %s\n",
>>> -          dw24.ts_pll_enable ? "enabled" : "disabled",
>>> +          str_enabled_disabled(dw24.ts_pll_enable),
>>>             ice_clk_src_str(dw24.time_ref_sel),
>>>             ice_clk_freq_str(dw9.time_ref_freq_sel),
>>>             bwm_lf.plllock_true_lock_cri ? "locked" : "unlocked");
>>> @@ -548,7 +548,7 @@ static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw,
>>>       /* Log the current clock configuration */
>>>       ice_debug(hw, ICE_DBG_PTP, "Current CGU configuration -- %s, 
>>> clk_src %s, clk_freq %s, PLL %s\n",
>>> -          dw24.ts_pll_enable ? "enabled" : "disabled",
>>> +          str_enabled_disabled(dw24.ts_pll_enable),
>>>             ice_clk_src_str(dw23.time_ref_sel),
>>>             ice_clk_freq_str(dw9.time_ref_freq_sel),
>>>             ro_lock.plllock_true_lock_cri ? "locked" : "unlocked");
>>> @@ -653,7 +653,7 @@ static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw,
>>>       /* Log the current clock configuration */
>>>       ice_debug(hw, ICE_DBG_PTP, "New CGU configuration -- %s, 
>>> clk_src %s, clk_freq %s, PLL %s\n",
>>> -          dw24.ts_pll_enable ? "enabled" : "disabled",
>>> +          str_enabled_disabled(dw24.ts_pll_enable),
>>>             ice_clk_src_str(dw23.time_ref_sel),
>>>             ice_clk_freq_str(dw9.time_ref_freq_sel),
>>>             ro_lock.plllock_true_lock_cri ? "locked" : "unlocked");
>>
>
Julia Lawall Oct. 30, 2024, 10:08 a.m. UTC | #3
On Mon, 28 Oct 2024, Przemek Kitszel wrote:

> On 10/27/24 15:19, R Sundar wrote:
> > Use string choice helpers for better readability.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Julia Lawall <julia.lawall@inria.fr>
> > Closes: https://lore.kernel.org/r/202410121553.SRNFzc2M-lkp@intel.com/
> > Signed-off-by: R Sundar <prosunofficial@gmail.com>
> > ---
>
> thanks, this indeed covers all "enabled/disabled" cases, so:
> Acked-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>
> for future submissions for Intel Ethernet drivers please use the
> iwl-next (or iwl-net) target trees.
>
> There are also other cases that we could cover ON/OFF etc

I counted the number of occurrences of various cases.  Here are the
results for at least 5 occurrences.  I converted everything to lowercase
and put the two strings in alphabetical order.

julia

" " "\n   ": 5
" (full)" "": 5
" (last)" "": 5
" csc" "": 5
" recoverable" "": 5
"" ".5": 5
"" "1": 5
"" ":" systemlow: 5
"" "\"": 5
"" "_backup": 5
"" "auto-": 5
"" "non": 5
"" "t": 5
"" # x " ": 5
"->" "<-": 5
"070701" "070702": 5
"2.4g" "5g": 5
"2g" dpk->bp[path][kidx].band == 1 ? "5g" : "6g": 5
"80m" dpk->bp[path][kidx].bw == rtw89_channel_width_40 ? "40m" : "20m": 5
"aborted" "completed": 5
"active" "disabled": 5
"anode" "sectors": 5
"assert" "deassert": 5
"attach" "detach": 5
"basic rate" "edr rate": 5
"bulk" "isoc": 5
"client" "server": 5
"closed" "open": 5
"correctable" "uncorrectable": 5
"dedicated" "shared": 5
"fcp" "nvme": 5
"fixed" "roll": 5
"full duplex" "half duplex": 5
"full" "high": 5
"gsi" "smi": 5
"hit" "not hit": 5
"ht20" "ht40": 5
"init" "rt": 5
"ips off" "ips on": 5
"lps off" "lps on": 5
"mc" "uc": 5
"migration" "recovery": 5
"none" "tx": 5
"off!!" "on!!": 5
"pause" "resume": 5
"rf_1t1r" "rf_2t2r": 5
"running" "stopped": 5
"set" "unset": 5
"veb" "vepa": 5
kern_emerg kern_info: 5
" " "\n": 6
" -- kernel too old?" "": 6
" and " "": 6
" flr" "": 6
" iaa" "": 6
" int" "": 6
" pcd" "": 6
" periodic" "": 6
" x4" "": 6
"" ")": 6
"" "*not* ": 6
"" ", ibss": 6
"" ".": 6
"" ":": 6
"" "_flip": 6
"" "amps, ": 6
"" "i": 6
"" "no": 6
"" "pkgpwrl1, ": 6
"" "pkgpwrl2, ": 6
"" "prochot, ": 6
"" "thermstatus, ": 6
"" "vr-therm, ": 6
"(not available)" "(success)": 6
"1" "2": 6
"20m" "40m": 6
"32" "64": 6
"???" "dma write": 6
"active/passive" "passive only": 6
"analog" "digital": 6
"aura" "pool": 6
"beacon" "probe response": 6
"c-vlan" "vlan": 6
"cancelled" "initiated": 6
"capture" "playback": 6
"cc1" "cc2": 6
"decoder" "encoder": 6
"downlink" "uplink": 6
"exmode" "prmode": 6
"found" "lost": 6
"gdt" "ldt": 6
"get" "set": 6
"group" "user": 6
"host" "peripheral": 6
"ipv4" "ipv6": 6
"is" "not": 6
"kill" "on": 6
"ns" "us": 6
"reading" "writing": 6
"recv" "send": 6
" ..." "": 7
" overflow" "": 7
" suspend" "": 7
"" ", nowayout": 7
"" "...": 7
"" "c": 7
"" (#x " "): 7
"" column_sep: 7
"active" "idle": 7
"add" "del": 7
"add" "remove": 7
"autodetected" "insmod option": 7
"capture" "output": 7
"ce" "ue": 7
"dual" "single": 7
"enter" "exit": 7
"fail:" "pass:": 7
"gate" "ungate": 7
"intx" "msi": 7
"private" "shared": 7
"read error" "write error": 7
"read from" "write to": 7
"ro" "rw": 7
kern_debug kern_err: 7
" async" "": 8
" fatal" "": 8
" sof" "": 8
" x16" "": 8
"" "a": 8
"" "b": 8
"10" "100": 8
"40m" "80m": 8
"active" "passive": 8
"bypass" "ram b": 8
"connected" "disconnected": 8
"dcs" "generic": 8
"done" "failed": 8
"dst" "src": 8
"entering" "leaving": 8
"failed" "ok": 8
"failed" "pass": 8
"fast" "slow": 8
"hard" "soft": 8
"invalid" "valid": 8
"kernel" "user": 8
"local" "remote": 8
"primary" "secondary": 8
"ram" "unknown": 8
"restart" "start": 8
"runtime" "system": 8
" err" "": 9
"" "-": 9
"" "/s": 9
"" ": ": 9
"" "[": 9
"" "]": 9
"" "d": 9
"2.4" "5.2": 9
"absent" "present": 9
"fail" "pass": 9
"locked" "unlocked": 9
"offline" "online": 9
"r" "w": 9
"struct" "union": 9
"failed" "success": 53
"" "un": 55
"down" "up": 56
"high" "low": 57
"" "s": 65
"full" "half": 84
"" ",": 86
"not supported" "supported": 91
"" "not ": 96
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index da88c6ccfaeb..d8d3395e49c3 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -393,7 +393,7 @@  static int ice_cfg_cgu_pll_e82x(struct ice_hw *hw,
 
 	/* Log the current clock configuration */
 	ice_debug(hw, ICE_DBG_PTP, "Current CGU configuration -- %s, clk_src %s, clk_freq %s, PLL %s\n",
-		  dw24.ts_pll_enable ? "enabled" : "disabled",
+		  str_enabled_disabled(dw24.ts_pll_enable),
 		  ice_clk_src_str(dw24.time_ref_sel),
 		  ice_clk_freq_str(dw9.time_ref_freq_sel),
 		  bwm_lf.plllock_true_lock_cri ? "locked" : "unlocked");
@@ -471,7 +471,7 @@  static int ice_cfg_cgu_pll_e82x(struct ice_hw *hw,
 
 	/* Log the current clock configuration */
 	ice_debug(hw, ICE_DBG_PTP, "New CGU configuration -- %s, clk_src %s, clk_freq %s, PLL %s\n",
-		  dw24.ts_pll_enable ? "enabled" : "disabled",
+		  str_enabled_disabled(dw24.ts_pll_enable),
 		  ice_clk_src_str(dw24.time_ref_sel),
 		  ice_clk_freq_str(dw9.time_ref_freq_sel),
 		  bwm_lf.plllock_true_lock_cri ? "locked" : "unlocked");
@@ -548,7 +548,7 @@  static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw,
 
 	/* Log the current clock configuration */
 	ice_debug(hw, ICE_DBG_PTP, "Current CGU configuration -- %s, clk_src %s, clk_freq %s, PLL %s\n",
-		  dw24.ts_pll_enable ? "enabled" : "disabled",
+		  str_enabled_disabled(dw24.ts_pll_enable),
 		  ice_clk_src_str(dw23.time_ref_sel),
 		  ice_clk_freq_str(dw9.time_ref_freq_sel),
 		  ro_lock.plllock_true_lock_cri ? "locked" : "unlocked");
@@ -653,7 +653,7 @@  static int ice_cfg_cgu_pll_e825c(struct ice_hw *hw,
 
 	/* Log the current clock configuration */
 	ice_debug(hw, ICE_DBG_PTP, "New CGU configuration -- %s, clk_src %s, clk_freq %s, PLL %s\n",
-		  dw24.ts_pll_enable ? "enabled" : "disabled",
+		  str_enabled_disabled(dw24.ts_pll_enable),
 		  ice_clk_src_str(dw23.time_ref_sel),
 		  ice_clk_freq_str(dw9.time_ref_freq_sel),
 		  ro_lock.plllock_true_lock_cri ? "locked" : "unlocked");