RE: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
> If you want to create enum->#ENUM structs and > "const char *" lookup functions, please be my guest. > > otherwise, hex is at least a consistent way to display > what should be infrequent output. If I've typed it right: #define tags(x) x(A) x(B) x(C) #define x(t) t, enum {tags(x) tag_count}; #undef x #define x(t) ##t, static const char names[] = { tags(x) }; #undef x David
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
Joe Percheswrites: > On Sat, 2016-09-24 at 14:06 -0500, Larry Finger wrote: >> On 09/24/2016 12:32 PM, Joe Perches wrote: > [] >> o Reindent all the switch/case blocks to a more normal >> kernel style (git diff -w would show no changes here) >> That sounds like busy work to me, but if you want to do it, go ahead. > > It's really just to make the comparison case block reductions > easier to verify for later steps done > >> > o cast, spacing and parenthesis reductions >> > Lots of odd and somewhat unique styles in various >> > drivers, looks like too many individual authors without >> > a style guide / code enforcer using slightly different >> > personalized code. Glancing at the code, it looks to be >> > similar logic, just written in different styles. >> Same comment. > > Same rationale > >> > o Logic changes like >> > from: >> > if (foo) func(..., bar, ...); else func(..., baz, ...); >> > to: >> > func(..., foo ? bar : baz, ...); >> > to make the case statement code blocks more consistent >> > and emit somewhat smaller object code. >> I find if .. else constructs much easier to read than the cond ? : >> form. I would reject any such patches. > > I think object code reduction generally a good thing > but then again, I'm not a maintainer here. I missed this part, but I am with Larry here - 'foo ? bar : boo' are just obfuscating the code and far less clear than if or switch statements. Jes
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
On Sat, 2016-09-24 at 14:06 -0500, Larry Finger wrote: > On 09/24/2016 12:32 PM, Joe Perches wrote: [] > o Reindent all the switch/case blocks to a more normal > kernel style (git diff -w would show no changes here) > That sounds like busy work to me, but if you want to do it, go ahead. It's really just to make the comparison case block reductions easier to verify for later steps done > > o cast, spacing and parenthesis reductions > > Lots of odd and somewhat unique styles in various > > drivers, looks like too many individual authors without > > a style guide / code enforcer using slightly different > > personalized code. Glancing at the code, it looks to be > > similar logic, just written in different styles. > Same comment. Same rationale > > o Logic changes like > > from: > > if (foo) func(..., bar, ...); else func(..., baz, ...); > > to: > > func(..., foo ? bar : baz, ...); > > to make the case statement code blocks more consistent > > and emit somewhat smaller object code. > I find if .. else constructs much easier to read than the cond ? : > form. I would reject any such patches. I think object code reduction generally a good thing but then again, I'm not a maintainer here. > > o Consolidation of equivalent function spanning drivers > > With the style only changes minimized, where possible > > make the drivers use common ops/callback functions. > The is no question that there are similar routines in different drivers. I > would > like to place as much as possible into common routines, but I never seem to > find > the time. There are too many bugs in other things I support to consider these > niceties. Consolidation generally reduces defects and improves ease of updating. >
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
Larry Fingerwrites: > On 09/24/2016 12:32 PM, Joe Perches wrote: >> Is there any value in that or is Jes' work going to make >> doing any or all of this unnecessary and futile? > > That is not yet determined. The only driver that is to be replaced at > this point is rtl8192cu. Jes only has USB I/O for his driver. We are > looking at adding SDIO, and once that is done, PCI should be possible. If someone else wants to address PCI then it could happen quite soon, but at the current schedule I don't see PCI happen in my driver for at least a year, probably more. If you can reduce the size of rtlwifi in the mean time that probably isn't going to upset a lot of people. Jes
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
On 09/24/2016 12:32 PM, Joe Perches wrote: (adding Jes Sorensen to recipients) On Sat, 2016-09-24 at 11:35 -0500, Larry Finger wrote: I have patches that makes HAL_DEF_WOWLAN be a no-op for the rest of the drivers, and one that sets the enum values for that particular statement to hex values. I also looked at the other large enums and decided that they never need the human lookup. Hey Larry. There are many somewhat common realtek wireless drivers. Not to step on your toes, but what do you think of rationalizing the switch/case statements of all the realtek drivers in a few steps: o Reindent all the switch/case blocks to a more normal kernel style (git diff -w would show no changes here) That sounds like busy work to me, but if you want to do it, go ahead. o cast, spacing and parenthesis reductions Lots of odd and somewhat unique styles in various drivers, looks like too many individual authors without a style guide / code enforcer using slightly different personalized code. Glancing at the code, it looks to be similar logic, just written in different styles. Same comment. o Logic changes like from: if (foo) func(..., bar, ...); else func(..., baz, ...); to: func(..., foo ? bar : baz, ...); to make the case statement code blocks more consistent and emit somewhat smaller object code. I find if .. else constructs much easier to read than the cond ? : form. I would reject any such patches. o Consolidation of equivalent function spanning drivers With the style only changes minimized, where possible make the drivers use common ops/callback functions. The is no question that there are similar routines in different drivers. I would like to place as much as possible into common routines, but I never seem to find the time. There are too many bugs in other things I support to consider these niceties. Is there any value in that or is Jes' work going to make doing any or all of this unnecessary and futile? That is not yet determined. The only driver that is to be replaced at this point is rtl8192cu. Jes only has USB I/O for his driver. We are looking at adding SDIO, and once that is done, PCI should be possible. Larry
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
(adding Jes Sorensen to recipients) On Sat, 2016-09-24 at 11:35 -0500, Larry Finger wrote: > I have patches that makes HAL_DEF_WOWLAN be a no-op for the rest of the > drivers, > and one that sets the enum values for that particular statement to hex > values. I > also looked at the other large enums and decided that they never need the > human > lookup. Hey Larry. There are many somewhat common realtek wireless drivers. Not to step on your toes, but what do you think of rationalizing the switch/case statements of all the realtek drivers in a few steps: o Reindent all the switch/case blocks to a more normal kernel style (git diff -w would show no changes here) o cast, spacing and parenthesis reductions Lots of odd and somewhat unique styles in various drivers, looks like too many individual authors without a style guide / code enforcer using slightly different personalized code. Glancing at the code, it looks to be similar logic, just written in different styles. o Logic changes like from: if (foo) func(..., bar, ...); else func(..., baz, ...); to: func(..., foo ? bar : baz, ...); to make the case statement code blocks more consistent and emit somewhat smaller object code. o Consolidation of equivalent function spanning drivers With the style only changes minimized, where possible make the drivers use common ops/callback functions. Is there any value in that or is Jes' work going to make doing any or all of this unnecessary and futile?
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
On 09/24/2016 11:15 AM, Joe Perches wrote: On Sat, 2016-09-24 at 17:55 +0200, Jean Delvare wrote: Would it make sense to explicitly set the enum values, or add them as comments, to make such look-ups easier? If you want to create enum->#ENUM structs and "const char *" lookup functions, please be my guest. otherwise, hex is at least a consistent way to display what should be infrequent output. Displaying those values as hex is OK. As Joe says, they will not be shown very often. I have patches that makes HAL_DEF_WOWLAN be a no-op for the rest of the drivers, and one that sets the enum values for that particular statement to hex values. I also looked at the other large enums and decided that they never need the human lookup. Larry
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
On Sat, 2016-09-24 at 17:55 +0200, Jean Delvare wrote: > Would it make sense to explicitly set the enum values, or add them as > comments, to make such look-ups easier? If you want to create enum->#ENUM structs and "const char *" lookup functions, please be my guest. otherwise, hex is at least a consistent way to display what should be infrequent output.
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
Hi Joe, Larry, On Fri, 23 Sep 2016 12:02:43 -0700, Joe Perches wrote: > On Fri, 2016-09-23 at 13:59 -0500, Larry Finger wrote: > > I'm not familiar with the %#x format. What does it do? > > Outputs SPECIAL prefix, it's the same as "0x%x" > > lib/vsprintf.c: > #define SPECIAL 64 /* prefix hex with "0x", octal with "0" > */ Is hexadecimal actually the best way to display these values? I guess it depends how they are listed in the datasheets (if there's anything like that for these chips?) I found it a bit difficult to look up the meaning of the value. HAL_DEF_WOWLAN is an enum value, the number is not set and there's no comment. I had to count the line numbers, taking blank lines into account... I ended up pasting the whole enum to a random C file and printing the value of HAL_DEF_WOWLAN to make sure it was 92. Would it make sense to explicitly set the enum values, or add them as comments, to make such look-ups easier? -- Jean Delvare SUSE L3 Support
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
On 09/23/2016 01:27 PM, Joe Perches wrote: Help along debugging by showing what switch/case variable is not being processed in these messages. Signed-off-by: Joe PerchesAcked-by: Larry Finger Thanks, Larry
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
On Fri, 2016-09-23 at 13:59 -0500, Larry Finger wrote: > I'm not familiar with the %#x format. What does it do? Outputs SPECIAL prefix, it's the same as "0x%x" lib/vsprintf.c: #define SPECIAL 64 /* prefix hex with "0x", octal with "0" */
Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages
On 09/23/2016 01:27 PM, Joe Perches wrote: Help along debugging by showing what switch/case variable is not being processed in these messages. Signed-off-by: Joe PerchesJoe, You beat me to the patch. No problem as this one looks OK; however, I'm not familiar with the %#x format. What does it do? Larry --- drivers/net/wireless/realtek/rtlwifi/core.c | 3 ++- drivers/net/wireless/realtek/rtlwifi/pci.c | 3 ++- drivers/net/wireless/realtek/rtlwifi/ps.c| 2 +- drivers/net/wireless/realtek/rtlwifi/rtl8188ee/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8188ee/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c | 10 ++ .../wireless/realtek/rtlwifi/rtl8192c/fw_common.c| 4 ++-- .../wireless/realtek/rtlwifi/rtl8192c/phy_common.c | 8 +--- drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c | 7 --- drivers/net/wireless/realtek/rtlwifi/rtl8192ce/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192ce/phy.c | 7 ++- drivers/net/wireless/realtek/rtlwifi/rtl8192cu/hw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192cu/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192cu/phy.c | 7 ++- drivers/net/wireless/realtek/rtlwifi/rtl8192de/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192de/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8192de/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c | 15 +++ drivers/net/wireless/realtek/rtlwifi/rtl8192ee/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192ee/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8192ee/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c | 10 ++ drivers/net/wireless/realtek/rtlwifi/rtl8192se/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8192se/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192se/phy.c | 5 +++-- drivers/net/wireless/realtek/rtlwifi/rtl8723ae/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8723ae/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8723ae/phy.c | 10 ++ drivers/net/wireless/realtek/rtlwifi/rtl8723be/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c | 10 +- drivers/net/wireless/realtek/rtlwifi/rtl8723be/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c | 12 +++- drivers/net/wireless/realtek/rtlwifi/rtl8821ae/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8821ae/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c | 20 ++-- 38 files changed, 128 insertions(+), 123 deletions(-) diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c index 7aee5ebb1..f95760c 100644 --- a/drivers/net/wireless/realtek/rtlwifi/core.c +++ b/drivers/net/wireless/realtek/rtlwifi/core.c @@ -765,7 +765,8 @@ static int rtl_op_config(struct ieee80211_hw *hw, u32 changed) mac->bw_40 = false; mac->bw_80 = false; RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG, -"switch case not processed\n"); +"switch case %#x not processed\n", +channel_type); break; } } diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c index d12586d..0dfa9ea 100644 --- a/drivers/net/wireless/realtek/rtlwifi/pci.c +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c @@ -179,7 +179,8 @@ static void _rtl_pci_update_default_setting(struct ieee80211_hw *hw) break; default: RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG, -"switch case not processed\n"); +"switch case %#x not processed\n", +rtlpci->const_support_pciaspm); break; } diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c b/drivers/net/wireless/realtek/rtlwifi/ps.c index 9a64f9b..18d979a 100644 --- a/drivers/net/wireless/realtek/rtlwifi/ps.c +++ b/drivers/net/wireless/realtek/rtlwifi/ps.c @@ -151,7 +151,7 @@ static bool rtl_ps_set_rf_state(struct ieee80211_hw *hw, default: RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG, -"switch case not processed\n"); +"switch case %#x not processed\n", state_toset); break; } diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/fw.c
[PATCH] realtek: Add switch variable to 'switch case not processed' messages
Help along debugging by showing what switch/case variable is not being processed in these messages. Signed-off-by: Joe Perches--- drivers/net/wireless/realtek/rtlwifi/core.c | 3 ++- drivers/net/wireless/realtek/rtlwifi/pci.c | 3 ++- drivers/net/wireless/realtek/rtlwifi/ps.c| 2 +- drivers/net/wireless/realtek/rtlwifi/rtl8188ee/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8188ee/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8188ee/phy.c | 10 ++ .../wireless/realtek/rtlwifi/rtl8192c/fw_common.c| 4 ++-- .../wireless/realtek/rtlwifi/rtl8192c/phy_common.c | 8 +--- drivers/net/wireless/realtek/rtlwifi/rtl8192ce/hw.c | 7 --- drivers/net/wireless/realtek/rtlwifi/rtl8192ce/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192ce/phy.c | 7 ++- drivers/net/wireless/realtek/rtlwifi/rtl8192cu/hw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192cu/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192cu/phy.c | 7 ++- drivers/net/wireless/realtek/rtlwifi/rtl8192de/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192de/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8192de/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c | 15 +++ drivers/net/wireless/realtek/rtlwifi/rtl8192ee/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192ee/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8192ee/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192ee/phy.c | 10 ++ drivers/net/wireless/realtek/rtlwifi/rtl8192se/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8192se/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8192se/phy.c | 5 +++-- drivers/net/wireless/realtek/rtlwifi/rtl8723ae/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8723ae/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8723ae/phy.c | 10 ++ drivers/net/wireless/realtek/rtlwifi/rtl8723be/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c | 10 +- drivers/net/wireless/realtek/rtlwifi/rtl8723be/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c | 12 +++- drivers/net/wireless/realtek/rtlwifi/rtl8821ae/fw.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 9 + drivers/net/wireless/realtek/rtlwifi/rtl8821ae/led.c | 4 ++-- drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c | 20 ++-- 38 files changed, 128 insertions(+), 123 deletions(-) diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c index 7aee5ebb1..f95760c 100644 --- a/drivers/net/wireless/realtek/rtlwifi/core.c +++ b/drivers/net/wireless/realtek/rtlwifi/core.c @@ -765,7 +765,8 @@ static int rtl_op_config(struct ieee80211_hw *hw, u32 changed) mac->bw_40 = false; mac->bw_80 = false; RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG, -"switch case not processed\n"); +"switch case %#x not processed\n", +channel_type); break; } } diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c index d12586d..0dfa9ea 100644 --- a/drivers/net/wireless/realtek/rtlwifi/pci.c +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c @@ -179,7 +179,8 @@ static void _rtl_pci_update_default_setting(struct ieee80211_hw *hw) break; default: RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG, -"switch case not processed\n"); +"switch case %#x not processed\n", +rtlpci->const_support_pciaspm); break; } diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c b/drivers/net/wireless/realtek/rtlwifi/ps.c index 9a64f9b..18d979a 100644 --- a/drivers/net/wireless/realtek/rtlwifi/ps.c +++ b/drivers/net/wireless/realtek/rtlwifi/ps.c @@ -151,7 +151,7 @@ static bool rtl_ps_set_rf_state(struct ieee80211_hw *hw, default: RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG, -"switch case not processed\n"); +"switch case %#x not processed\n", state_toset); break; } diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/fw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/fw.c index 6291256..5360d53 100644 --- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/fw.c +++