Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490394 > unrolled thread

Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] realtek: Add switch variable to 'switch case not  processed' messages Joe Perches <joe@perches.com> - 2016-09-23 21:10 +0200
    Re: [PATCH] realtek: Add switch variable to 'switch case not  processed' messages Jean Delvare <jdelvare@suse.de> - 2016-09-24 18:00 +0200
      Re: [PATCH] realtek: Add switch variable to 'switch case not  processed' messages Joe Perches <joe@perches.com> - 2016-09-24 18:20 +0200
        Re: [PATCH] realtek: Add switch variable to 'switch case not  processed' messages Larry Finger <Larry.Finger@lwfinger.net> - 2016-09-24 18:40 +0200
          Re: [PATCH] realtek: Add switch variable to 'switch case not  processed' messages Joe Perches <joe@perches.com> - 2016-09-24 19:40 +0200
            Re: [PATCH] realtek: Add switch variable to 'switch case not  processed' messages Larry Finger <Larry.Finger@lwfinger.net> - 2016-09-24 21:10 +0200
              Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages Jes Sorensen <Jes.Sorensen@redhat.com> - 2016-09-24 22:10 +0200
              Re: [PATCH] realtek: Add switch variable to 'switch case not  processed' messages Joe Perches <joe@perches.com> - 2016-09-24 22:30 +0200
                Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages Jes Sorensen <Jes.Sorensen@redhat.com> - 2016-09-24 22:40 +0200
        RE: [PATCH] realtek: Add switch variable to 'switch case not  processed' messages David Laight <David.Laight@ACULAB.COM> - 2016-09-26 11:40 +0200

#1490394 — 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" */

[toc] | [next] | [standalone]

#1490669

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

[toc] | [prev] | [next] | [standalone]

#1490673

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.

[toc] | [prev] | [next] | [standalone]

#1490678

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

[toc] | [prev] | [next] | [standalone]

#1490695

(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?

[toc] | [prev] | [next] | [standalone]

#1490723

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 ? xxxx : yyyy 
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

[toc] | [prev] | [next] | [standalone]

#1490742 — Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

Larry Finger <Larry.Finger@lwfinger.net> writes:
> 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

[toc] | [prev] | [next] | [standalone]

#1490746

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 ? xxxx : yyyy 
> form. I would reject any such patches.

<shrug> 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.
> 

[toc] | [prev] | [next] | [standalone]

#1490748 — Re: [PATCH] realtek: Add switch variable to 'switch case not processed' messages

Joe Perches <joe@perches.com> writes:
> 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 ? xxxx : yyyy 
>> form. I would reject any such patches.
>
> <shrug> 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

[toc] | [prev] | [next] | [standalone]

#1491183

> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web