Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Thu, Dec 07, 2017 at 12:17:32AM +0200, Marcus Wolf wrote:
> To be honest, I am a little bit astonished about that question. Don't you do
> a regression test after such a redesign? I would be a little bit afraid to
> offer such redesign without testing.

He probably doesn't have the hardware...  That's fine.  Most patches to
staging are like this.  We have a pretty good track record for rarely
introducing bugs.

This patchset is pretty easy to review with confidence because it's
mostly just renames so there are only a few bits to review by hand.
I've attached the perl script I use for reviewing renames and indent
changes.

regards,
dan carpenter
#!/usr/bin/perl

# This is a tool to help review variable rename patches. The goal is
# to strip out the automatic sed renames and the white space changes
# and leaves the interesting code changes.
#
# Example 1: A patch renames openInfo to open_info:
# cat diff | rename_review.pl openInfo open_info
#
# Example 2: A patch swaps the first two arguments to some_func():
# cat diff | rename_review.pl \
#-e 's/some_func\((.*?),(.*?),/some_func\($2, $1,/'
#
# Example 3: A patch removes the xkcd_ prefix from some but not all the
# variables.  Instead of trying to figure out which variables were renamed
# just remove the prefix from them all:
# cat diff | rename_review.pl -ea 's/xkcd_//g'
#
# Example 4: A patch renames 20 CamelCase variables.  To review this let's
# just ignore all case changes and all '_' chars.
# cat diff | rename_review -ea 'tr/[A-Z]/[a-z]/' -ea 's/_//g'
#
# The other arguments are:
# -nc removes comments
# -ns removes '\' chars if they are at the end of the line.

use strict;
use File::Temp qw/ :mktemp  /;

sub usage() {
print "usage: cat diff | $0 old new old new old new...\n";
print "   or: cat diff | $0 -e 's/old/new/g'\n";
print " -a : auto";
print " -e : execute on old lines\n";
print " -ea: execute on all lines\n";
print " -nc: no comments\n";
print " -nb: no unneeded braces\n";
print " -ns: no slashes at the end of a line\n";
print " -pull: for function pull.  deletes context.\n";
print " -r : NULL, bool";
exit(1);
}
my @subs;
my @strict_subs;
my @cmds;
my $strip_comments;
my $strip_braces;
my $strip_slashes;
my $pull_context;
my $auto;

sub filter($) {
my $line = shift();
my $old = 0;
if ($line =~ /^-/) {
$old = 1;
}
# remove the first char
$line =~ s/^[ +-]//;
if ($strip_comments) {
$line =~ s/\/\*.*?\*\///g;
$line =~ s/\/\/.*//;
}
foreach my $cmd (@cmds) {
if ($old || $cmd->[0] =~ /^-ea$/) {
eval "\$line =~ $cmd->[1]";
}
}
foreach my $sub (@subs) {
if ($old) {
$line =~ s/$sub->[0]/$sub->[1]/g;
}
}
foreach my $sub (@strict_subs) {
if ($old) {
$line =~ s/\b$sub->[0]\b/$sub->[1]/g;
}
}

# remove the newline so we can move curly braces here if we want.
$line =~ s/\n//;
return $line;
}

while (my $param1 = shift()) {
if ($param1 =~ /^-a$/) {
$auto = 1;
next;
}
if ($param1 =~ /^-nc$/) {
$strip_comments = 1;
next;
}
if ($param1 =~ /^-nb$/) {
$strip_braces = 1;
next;
}
if ($param1 =~ /^-ns$/) {
$strip_slashes = 1;
next;
}
if ($param1 =~ /^-pull$/) {
$pull_context = 1;
next;
}
my $param2 = shift();
if ($param2 =~ /^$/) {
usage();
}
if ($param1 =~ /^-e(a|)$/) {
push @cmds, [$param1, $param2];
next;
}
if ($param1 =~ /^-r$/) {
if ($param2 =~ /bool/) {
push @cmds, ["-e", "s/== true//"];
push @cmds, ["-e", "s/true ==//"];
push @cmds, ["-e", "s/([a-zA-Z\-\>\._]+) == false/!\$1/"];
next;
} elsif ($param2 =~ /NULL/) {
push @cmds, ["-e", "s/ != NULL//"];
push @cmds, ["-e", "s/([a-zA-Z\-\>\._0-9]+) == NULL/!\$1/"];
next;
} elsif ($param2 =~ /BIT/) {
push @cmds, ["-e", 's/1[uU]* *<< *(\d+)/BIT($1)/'];
push @cmds, ["-e", 's/\(1 *<< *(\w+)\)/BIT($1)/'];
push @cmds, ["-e", 's/\(BIT\((.*?)\)\)/BIT($1)/'];
next;
}
usage();
}

push @subs, [$param1, $param2];
}

my ($oldfh, $oldfile) = mkstemp("/tmp/oldX");
my ($newfh, $newfile) = mkstemp("/tmp/newX");

my @input = ;

# auto works on the observation that the - line comes before the + line when we
# rename variables.  Take the first - line.  Find the first + line.  Find the
# one word difference.  Test that the old word never occurs in the new text.
if ($auto) {
my %c_keywords = (  auto => 1,
break => 1,
case => 1,
char => 1,
const => 1,
continue => 1,
default => 1,
  

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Thu, Dec 07, 2017 at 12:17:32AM +0200, Marcus Wolf wrote:
> To be honest, I am a little bit astonished about that question. Don't you do
> a regression test after such a redesign? I would be a little bit afraid to
> offer such redesign without testing.

He probably doesn't have the hardware...  That's fine.  Most patches to
staging are like this.  We have a pretty good track record for rarely
introducing bugs.

This patchset is pretty easy to review with confidence because it's
mostly just renames so there are only a few bits to review by hand.
I've attached the perl script I use for reviewing renames and indent
changes.

regards,
dan carpenter
#!/usr/bin/perl

# This is a tool to help review variable rename patches. The goal is
# to strip out the automatic sed renames and the white space changes
# and leaves the interesting code changes.
#
# Example 1: A patch renames openInfo to open_info:
# cat diff | rename_review.pl openInfo open_info
#
# Example 2: A patch swaps the first two arguments to some_func():
# cat diff | rename_review.pl \
#-e 's/some_func\((.*?),(.*?),/some_func\($2, $1,/'
#
# Example 3: A patch removes the xkcd_ prefix from some but not all the
# variables.  Instead of trying to figure out which variables were renamed
# just remove the prefix from them all:
# cat diff | rename_review.pl -ea 's/xkcd_//g'
#
# Example 4: A patch renames 20 CamelCase variables.  To review this let's
# just ignore all case changes and all '_' chars.
# cat diff | rename_review -ea 'tr/[A-Z]/[a-z]/' -ea 's/_//g'
#
# The other arguments are:
# -nc removes comments
# -ns removes '\' chars if they are at the end of the line.

use strict;
use File::Temp qw/ :mktemp  /;

sub usage() {
print "usage: cat diff | $0 old new old new old new...\n";
print "   or: cat diff | $0 -e 's/old/new/g'\n";
print " -a : auto";
print " -e : execute on old lines\n";
print " -ea: execute on all lines\n";
print " -nc: no comments\n";
print " -nb: no unneeded braces\n";
print " -ns: no slashes at the end of a line\n";
print " -pull: for function pull.  deletes context.\n";
print " -r : NULL, bool";
exit(1);
}
my @subs;
my @strict_subs;
my @cmds;
my $strip_comments;
my $strip_braces;
my $strip_slashes;
my $pull_context;
my $auto;

sub filter($) {
my $line = shift();
my $old = 0;
if ($line =~ /^-/) {
$old = 1;
}
# remove the first char
$line =~ s/^[ +-]//;
if ($strip_comments) {
$line =~ s/\/\*.*?\*\///g;
$line =~ s/\/\/.*//;
}
foreach my $cmd (@cmds) {
if ($old || $cmd->[0] =~ /^-ea$/) {
eval "\$line =~ $cmd->[1]";
}
}
foreach my $sub (@subs) {
if ($old) {
$line =~ s/$sub->[0]/$sub->[1]/g;
}
}
foreach my $sub (@strict_subs) {
if ($old) {
$line =~ s/\b$sub->[0]\b/$sub->[1]/g;
}
}

# remove the newline so we can move curly braces here if we want.
$line =~ s/\n//;
return $line;
}

while (my $param1 = shift()) {
if ($param1 =~ /^-a$/) {
$auto = 1;
next;
}
if ($param1 =~ /^-nc$/) {
$strip_comments = 1;
next;
}
if ($param1 =~ /^-nb$/) {
$strip_braces = 1;
next;
}
if ($param1 =~ /^-ns$/) {
$strip_slashes = 1;
next;
}
if ($param1 =~ /^-pull$/) {
$pull_context = 1;
next;
}
my $param2 = shift();
if ($param2 =~ /^$/) {
usage();
}
if ($param1 =~ /^-e(a|)$/) {
push @cmds, [$param1, $param2];
next;
}
if ($param1 =~ /^-r$/) {
if ($param2 =~ /bool/) {
push @cmds, ["-e", "s/== true//"];
push @cmds, ["-e", "s/true ==//"];
push @cmds, ["-e", "s/([a-zA-Z\-\>\._]+) == false/!\$1/"];
next;
} elsif ($param2 =~ /NULL/) {
push @cmds, ["-e", "s/ != NULL//"];
push @cmds, ["-e", "s/([a-zA-Z\-\>\._0-9]+) == NULL/!\$1/"];
next;
} elsif ($param2 =~ /BIT/) {
push @cmds, ["-e", 's/1[uU]* *<< *(\d+)/BIT($1)/'];
push @cmds, ["-e", 's/\(1 *<< *(\w+)\)/BIT($1)/'];
push @cmds, ["-e", 's/\(BIT\((.*?)\)\)/BIT($1)/'];
next;
}
usage();
}

push @subs, [$param1, $param2];
}

my ($oldfh, $oldfile) = mkstemp("/tmp/oldX");
my ($newfh, $newfile) = mkstemp("/tmp/newX");

my @input = ;

# auto works on the observation that the - line comes before the + line when we
# rename variables.  Take the first - line.  Find the first + line.  Find the
# one word difference.  Test that the old word never occurs in the new text.
if ($auto) {
my %c_keywords = (  auto => 1,
break => 1,
case => 1,
char => 1,
const => 1,
continue => 1,
default => 1,
  

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 21:57 schrieb Simon Sandström:

On Wed, Dec 06, 2017 at 01:44:14PM +0300, Dan Carpenter wrote:

On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:



Am 06.12.2017 um 12:23 schrieb Dan Carpenter:



Wow...  This was the one patch I thought was going to sink this
patchset...


I don't get that. What do you mean?


Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.


All enums are for user space (or inteded to be used in userspace in future).
Didn't introduce enums for internal use.


So what I'm asking is if we do this change, does it break any userspace
programs which are used *right now*.  In other words will programs stop
compiling because we've renamed an enum?

regards,
dan carpenter



Hi,

I'm not sure about this so I have to ask: wouldn't the header need to be
in include/uapi/ for userspace to use them? Or is it "allowed" for
userspace programs to use this internal header? Or are we afraid to
break userspace programs that keeps a copy of pi433_if.h?


Regards,
Simon



Hi Simon,

in principle, I think you are right. With my first patch, offering the 
driver I did it that way, but Greg asked me to migrate everything 
(including docu) into staging/pi433. I think, that's related to being in 
staging.
At the moment, I copy pi433_if.h and rf69_enum.h to my userspace 
program, in order to be able to compile it.


To be honest, I am a little bit astonished about that question. Don't 
you do a regression test after such a redesign? I would be a little bit 
afraid to offer such redesign without testing.



Regards,
Marcus

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 21:57 schrieb Simon Sandström:

On Wed, Dec 06, 2017 at 01:44:14PM +0300, Dan Carpenter wrote:

On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:



Am 06.12.2017 um 12:23 schrieb Dan Carpenter:



Wow...  This was the one patch I thought was going to sink this
patchset...


I don't get that. What do you mean?


Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.


All enums are for user space (or inteded to be used in userspace in future).
Didn't introduce enums for internal use.


So what I'm asking is if we do this change, does it break any userspace
programs which are used *right now*.  In other words will programs stop
compiling because we've renamed an enum?

regards,
dan carpenter



Hi,

I'm not sure about this so I have to ask: wouldn't the header need to be
in include/uapi/ for userspace to use them? Or is it "allowed" for
userspace programs to use this internal header? Or are we afraid to
break userspace programs that keeps a copy of pi433_if.h?


Regards,
Simon



Hi Simon,

in principle, I think you are right. With my first patch, offering the 
driver I did it that way, but Greg asked me to migrate everything 
(including docu) into staging/pi433. I think, that's related to being in 
staging.
At the moment, I copy pi433_if.h and rf69_enum.h to my userspace 
program, in order to be able to compile it.


To be honest, I am a little bit astonished about that question. Don't 
you do a regression test after such a redesign? I would be a little bit 
afraid to offer such redesign without testing.



Regards,
Marcus

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Simon Sandström]

On Wed, Dec 06, 2017 at 01:44:14PM +0300, Dan Carpenter wrote:
> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
> > 
> > 
> > Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
> > > 
> > > 
> > > Wow...  This was the one patch I thought was going to sink this
> > > patchset...
> > 
> > I don't get that. What do you mean?
> > 
> > > Isn't enum optionOnOff part of the userspace headers?
> > > 
> > > I thought we weren't allowed to change that.
> > 
> > All enums are for user space (or inteded to be used in userspace in future).
> > Didn't introduce enums for internal use.
> 
> So what I'm asking is if we do this change, does it break any userspace
> programs which are used *right now*.  In other words will programs stop
> compiling because we've renamed an enum?
> 
> regards,
> dan carpenter
> 

Hi,

I'm not sure about this so I have to ask: wouldn't the header need to be
in include/uapi/ for userspace to use them? Or is it "allowed" for
userspace programs to use this internal header? Or are we afraid to
break userspace programs that keeps a copy of pi433_if.h?


Regards,
Simon

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Simon Sandström]

On Wed, Dec 06, 2017 at 01:44:14PM +0300, Dan Carpenter wrote:
> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
> > 
> > 
> > Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
> > > 
> > > 
> > > Wow...  This was the one patch I thought was going to sink this
> > > patchset...
> > 
> > I don't get that. What do you mean?
> > 
> > > Isn't enum optionOnOff part of the userspace headers?
> > > 
> > > I thought we weren't allowed to change that.
> > 
> > All enums are for user space (or inteded to be used in userspace in future).
> > Didn't introduce enums for internal use.
> 
> So what I'm asking is if we do this change, does it break any userspace
> programs which are used *right now*.  In other words will programs stop
> compiling because we've renamed an enum?
> 
> regards,
> dan carpenter
> 

Hi,

I'm not sure about this so I have to ask: wouldn't the header need to be
in include/uapi/ for userspace to use them? Or is it "allowed" for
userspace programs to use this internal header? Or are we afraid to
break userspace programs that keeps a copy of pi433_if.h?


Regards,
Simon

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Sorry, for sending HTML as well - I am writing from my phone...

Yes, you will break something, when renaming.

Since order isn't changed, most probably old programs will still compile with 
old enum.h.

If we want to move from optionOnOff to bool, we will also break old progs.

Since driver is new (mainline for just something like 2 monthes) and stil under 
devel, I think we should "risk" it.

Gruß,

Marcus 

> Am 06.12.2017 um 11:44 schrieb Dan Carpenter :
> 
>> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
>> 
>> 
>>> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
>>> On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
> diff --git a/drivers/staging/pi433/rf69_enum.h 
> b/drivers/staging/pi433/rf69_enum.h
> index babe597e2ec6..5247e9269de9 100644
> --- a/drivers/staging/pi433/rf69_enum.h
> +++ b/drivers/staging/pi433/rf69_enum.h
> @@ -18,9 +18,9 @@
>   #ifndef RF69_ENUM_H
>   #define RF69_ENUM_H
> -enum optionOnOff {
> -optionOff,
> -optionOn
> +enum option_on_off {
> +OPTION_OFF,
> +OPTION_ON
>   };
>   enum mode {
> 
 
 Hi Simon,
 
 nice work.
 
 Thank you very much for all the style fixes :-)
 
>>> 
>>> 
>>> Wow...  This was the one patch I thought was going to sink this
>>> patchset...
>> 
>> I don't get that. What do you mean?
>> 
>>> Isn't enum optionOnOff part of the userspace headers?
>>> 
>>> I thought we weren't allowed to change that.
>> 
>> All enums are for user space (or inteded to be used in userspace in future).
>> Didn't introduce enums for internal use.
> 
> So what I'm asking is if we do this change, does it break any userspace
> programs which are used *right now*.  In other words will programs stop
> compiling because we've renamed an enum?
> 
> regards,
> dan carpenter

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Sorry, for sending HTML as well - I am writing from my phone...

Yes, you will break something, when renaming.

Since order isn't changed, most probably old programs will still compile with 
old enum.h.

If we want to move from optionOnOff to bool, we will also break old progs.

Since driver is new (mainline for just something like 2 monthes) and stil under 
devel, I think we should "risk" it.

Gruß,

Marcus 

> Am 06.12.2017 um 11:44 schrieb Dan Carpenter :
> 
>> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
>> 
>> 
>>> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
>>> On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
> diff --git a/drivers/staging/pi433/rf69_enum.h 
> b/drivers/staging/pi433/rf69_enum.h
> index babe597e2ec6..5247e9269de9 100644
> --- a/drivers/staging/pi433/rf69_enum.h
> +++ b/drivers/staging/pi433/rf69_enum.h
> @@ -18,9 +18,9 @@
>   #ifndef RF69_ENUM_H
>   #define RF69_ENUM_H
> -enum optionOnOff {
> -optionOff,
> -optionOn
> +enum option_on_off {
> +OPTION_OFF,
> +OPTION_ON
>   };
>   enum mode {
> 
 
 Hi Simon,
 
 nice work.
 
 Thank you very much for all the style fixes :-)
 
>>> 
>>> 
>>> Wow...  This was the one patch I thought was going to sink this
>>> patchset...
>> 
>> I don't get that. What do you mean?
>> 
>>> Isn't enum optionOnOff part of the userspace headers?
>>> 
>>> I thought we weren't allowed to change that.
>> 
>> All enums are for user space (or inteded to be used in userspace in future).
>> Didn't introduce enums for internal use.
> 
> So what I'm asking is if we do this change, does it break any userspace
> programs which are used *right now*.  In other words will programs stop
> compiling because we've renamed an enum?
> 
> regards,
> dan carpenter

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
> 
> 
> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
> > On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
> > > > diff --git a/drivers/staging/pi433/rf69_enum.h 
> > > > b/drivers/staging/pi433/rf69_enum.h
> > > > index babe597e2ec6..5247e9269de9 100644
> > > > --- a/drivers/staging/pi433/rf69_enum.h
> > > > +++ b/drivers/staging/pi433/rf69_enum.h
> > > > @@ -18,9 +18,9 @@
> > > >#ifndef RF69_ENUM_H
> > > >#define RF69_ENUM_H
> > > > -enum optionOnOff {
> > > > -   optionOff,
> > > > -   optionOn
> > > > +enum option_on_off {
> > > > +   OPTION_OFF,
> > > > +   OPTION_ON
> > > >};
> > > >enum mode {
> > > > 
> > > 
> > > Hi Simon,
> > > 
> > > nice work.
> > > 
> > > Thank you very much for all the style fixes :-)
> > > 
> > 
> > 
> > Wow...  This was the one patch I thought was going to sink this
> > patchset...
> 
> I don't get that. What do you mean?
> 
> > Isn't enum optionOnOff part of the userspace headers?
> > 
> > I thought we weren't allowed to change that.
> 
> All enums are for user space (or inteded to be used in userspace in future).
> Didn't introduce enums for internal use.

So what I'm asking is if we do this change, does it break any userspace
programs which are used *right now*.  In other words will programs stop
compiling because we've renamed an enum?

regards,
dan carpenter

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
> 
> 
> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
> > On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
> > > > diff --git a/drivers/staging/pi433/rf69_enum.h 
> > > > b/drivers/staging/pi433/rf69_enum.h
> > > > index babe597e2ec6..5247e9269de9 100644
> > > > --- a/drivers/staging/pi433/rf69_enum.h
> > > > +++ b/drivers/staging/pi433/rf69_enum.h
> > > > @@ -18,9 +18,9 @@
> > > >#ifndef RF69_ENUM_H
> > > >#define RF69_ENUM_H
> > > > -enum optionOnOff {
> > > > -   optionOff,
> > > > -   optionOn
> > > > +enum option_on_off {
> > > > +   OPTION_OFF,
> > > > +   OPTION_ON
> > > >};
> > > >enum mode {
> > > > 
> > > 
> > > Hi Simon,
> > > 
> > > nice work.
> > > 
> > > Thank you very much for all the style fixes :-)
> > > 
> > 
> > 
> > Wow...  This was the one patch I thought was going to sink this
> > patchset...
> 
> I don't get that. What do you mean?
> 
> > Isn't enum optionOnOff part of the userspace headers?
> > 
> > I thought we weren't allowed to change that.
> 
> All enums are for user space (or inteded to be used in userspace in future).
> Didn't introduce enums for internal use.

So what I'm asking is if we do this change, does it break any userspace
programs which are used *right now*.  In other words will programs stop
compiling because we've renamed an enum?

regards,
dan carpenter

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 12:23 schrieb Dan Carpenter:

On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:

diff --git a/drivers/staging/pi433/rf69_enum.h 
b/drivers/staging/pi433/rf69_enum.h
index babe597e2ec6..5247e9269de9 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -18,9 +18,9 @@
   #ifndef RF69_ENUM_H
   #define RF69_ENUM_H
-enum optionOnOff {
-   optionOff,
-   optionOn
+enum option_on_off {
+   OPTION_OFF,
+   OPTION_ON
   };
   enum mode {



Hi Simon,

nice work.

Thank you very much for all the style fixes :-)




Wow...  This was the one patch I thought was going to sink this
patchset... 


I don't get that. What do you mean?


Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.


All enums are for user space (or inteded to be used in userspace in 
future). Didn't introduce enums for internal use.


Reagrds,
Marcus

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 12:23 schrieb Dan Carpenter:

On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:

diff --git a/drivers/staging/pi433/rf69_enum.h 
b/drivers/staging/pi433/rf69_enum.h
index babe597e2ec6..5247e9269de9 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -18,9 +18,9 @@
   #ifndef RF69_ENUM_H
   #define RF69_ENUM_H
-enum optionOnOff {
-   optionOff,
-   optionOn
+enum option_on_off {
+   OPTION_OFF,
+   OPTION_ON
   };
   enum mode {



Hi Simon,

nice work.

Thank you very much for all the style fixes :-)




Wow...  This was the one patch I thought was going to sink this
patchset... 


I don't get that. What do you mean?


Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.


All enums are for user space (or inteded to be used in userspace in 
future). Didn't introduce enums for internal use.


Reagrds,
Marcus

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
> > diff --git a/drivers/staging/pi433/rf69_enum.h 
> > b/drivers/staging/pi433/rf69_enum.h
> > index babe597e2ec6..5247e9269de9 100644
> > --- a/drivers/staging/pi433/rf69_enum.h
> > +++ b/drivers/staging/pi433/rf69_enum.h
> > @@ -18,9 +18,9 @@
> >   #ifndef RF69_ENUM_H
> >   #define RF69_ENUM_H
> > -enum optionOnOff {
> > -   optionOff,
> > -   optionOn
> > +enum option_on_off {
> > +   OPTION_OFF,
> > +   OPTION_ON
> >   };
> >   enum mode {
> > 
> 
> Hi Simon,
> 
> nice work.
> 
> Thank you very much for all the style fixes :-)
> 


Wow...  This was the one patch I thought was going to sink this
patchset...  Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.

regards,
dan carpenter

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Dan Carpenter]

On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
> > diff --git a/drivers/staging/pi433/rf69_enum.h 
> > b/drivers/staging/pi433/rf69_enum.h
> > index babe597e2ec6..5247e9269de9 100644
> > --- a/drivers/staging/pi433/rf69_enum.h
> > +++ b/drivers/staging/pi433/rf69_enum.h
> > @@ -18,9 +18,9 @@
> >   #ifndef RF69_ENUM_H
> >   #define RF69_ENUM_H
> > -enum optionOnOff {
> > -   optionOff,
> > -   optionOn
> > +enum option_on_off {
> > +   OPTION_OFF,
> > +   OPTION_ON
> >   };
> >   enum mode {
> > 
> 
> Hi Simon,
> 
> nice work.
> 
> Thank you very much for all the style fixes :-)
> 


Wow...  This was the one patch I thought was going to sink this
patchset...  Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.

regards,
dan carpenter

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 00:08 schrieb Simon Sandström:

Renames the enum optionOnOff and its values optionOn, optionOff to enum
option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
"Avoid CamelCase: , , ".

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c  | 34 ++---
  drivers/staging/pi433/pi433_if.h  | 16 +++---
  drivers/staging/pi433/rf69.c  | 45 ++-
  drivers/staging/pi433/rf69.h  | 15 -
  drivers/staging/pi433/rf69_enum.h |  6 +++---
  5 files changed, 63 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b8efe6a74a2a..4f6830f369e9 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
/* packet config */
/* enable */
SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
afterSyncInterrupt));
}
@@ -206,7 +206,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
}
-   if (rx_cfg->enable_length_byte == optionOn) {
+   if (rx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -220,14 +220,14 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
  
  	/* lengths */

SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
-   if (rx_cfg->enable_length_byte == optionOn)
+   if (rx_cfg->enable_length_byte == OPTION_ON)
{
SET_CHECKED(rf69_set_payload_length(dev->spi, 0xff));
}
else if (rx_cfg->fixed_message_length != 0)
{
payload_length = rx_cfg->fixed_message_length;
-   if (rx_cfg->enable_length_byte  == optionOn) payload_length++;
+   if (rx_cfg->enable_length_byte  == OPTION_ON) payload_length++;
if (rx_cfg->enable_address_filtering != filteringOff) 
payload_length++;
SET_CHECKED(rf69_set_payload_length(dev->spi, payload_length));
}
@@ -237,7 +237,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
}
  
  	/* values */

-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_sync_values(dev->spi, 
rx_cfg->sync_pattern));
}
@@ -264,7 +264,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_tx_start_condition(dev->spi, 
tx_cfg->tx_start_condition));
  
  	/* packet format enable */

-   if (tx_cfg->enable_preamble == optionOn)
+   if (tx_cfg->enable_preamble == OPTION_ON)
{
SET_CHECKED(rf69_set_preamble_length(dev->spi, 
tx_cfg->preamble_length));
}
@@ -273,7 +273,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_preamble_length(dev->spi, 0));
}
SET_CHECKED(rf69_set_sync_enable  (dev->spi, tx_cfg->enable_sync));
-   if (tx_cfg->enable_length_byte == optionOn) {
+   if (tx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -285,7 +285,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_crc_enable   (dev->spi, tx_cfg->enable_crc));
  
  	/* configure sync, if enabled */

-   if (tx_cfg->enable_sync == optionOn) {
+   if (tx_cfg->enable_sync == OPTION_ON) {
SET_CHECKED(rf69_set_sync_size(dev->spi, tx_cfg->sync_length));
SET_CHECKED(rf69_set_sync_values(dev->spi, 
tx_cfg->sync_pattern));
}
@@ -400,7 +400,7 @@ pi433_receive(void *data)
}
  
  	/* length byte enabled? */

-   if (dev->rx_cfg.enable_length_byte == optionOn)
+   if (dev->rx_cfg.enable_length_byte == OPTION_ON)
{
retval = wait_event_interruptible(dev->fifo_wait_queue,
  dev->free_in_fifo < 
FIFO_SIZE);
@@ -525,11 +525,11 @@ pi433_tx_thread(void *data)
size = tx_cfg.fixed_message_length;
  
  		/* increase size, if len byte is requested */

-   if (tx_cfg.enable_length_byte == optionOn)
+   if (tx_cfg.enable_length_byte == OPTION_ON)
size++;
  
  		/* increase size, if adr byte is requested */

-  

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 00:08 schrieb Simon Sandström:

Renames the enum optionOnOff and its values optionOn, optionOff to enum
option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
"Avoid CamelCase: , , ".

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c  | 34 ++---
  drivers/staging/pi433/pi433_if.h  | 16 +++---
  drivers/staging/pi433/rf69.c  | 45 ++-
  drivers/staging/pi433/rf69.h  | 15 -
  drivers/staging/pi433/rf69_enum.h |  6 +++---
  5 files changed, 63 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b8efe6a74a2a..4f6830f369e9 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
/* packet config */
/* enable */
SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
afterSyncInterrupt));
}
@@ -206,7 +206,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
}
-   if (rx_cfg->enable_length_byte == optionOn) {
+   if (rx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -220,14 +220,14 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
  
  	/* lengths */

SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
-   if (rx_cfg->enable_length_byte == optionOn)
+   if (rx_cfg->enable_length_byte == OPTION_ON)
{
SET_CHECKED(rf69_set_payload_length(dev->spi, 0xff));
}
else if (rx_cfg->fixed_message_length != 0)
{
payload_length = rx_cfg->fixed_message_length;
-   if (rx_cfg->enable_length_byte  == optionOn) payload_length++;
+   if (rx_cfg->enable_length_byte  == OPTION_ON) payload_length++;
if (rx_cfg->enable_address_filtering != filteringOff) 
payload_length++;
SET_CHECKED(rf69_set_payload_length(dev->spi, payload_length));
}
@@ -237,7 +237,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
}
  
  	/* values */

-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_sync_values(dev->spi, 
rx_cfg->sync_pattern));
}
@@ -264,7 +264,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_tx_start_condition(dev->spi, 
tx_cfg->tx_start_condition));
  
  	/* packet format enable */

-   if (tx_cfg->enable_preamble == optionOn)
+   if (tx_cfg->enable_preamble == OPTION_ON)
{
SET_CHECKED(rf69_set_preamble_length(dev->spi, 
tx_cfg->preamble_length));
}
@@ -273,7 +273,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_preamble_length(dev->spi, 0));
}
SET_CHECKED(rf69_set_sync_enable  (dev->spi, tx_cfg->enable_sync));
-   if (tx_cfg->enable_length_byte == optionOn) {
+   if (tx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -285,7 +285,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_crc_enable   (dev->spi, tx_cfg->enable_crc));
  
  	/* configure sync, if enabled */

-   if (tx_cfg->enable_sync == optionOn) {
+   if (tx_cfg->enable_sync == OPTION_ON) {
SET_CHECKED(rf69_set_sync_size(dev->spi, tx_cfg->sync_length));
SET_CHECKED(rf69_set_sync_values(dev->spi, 
tx_cfg->sync_pattern));
}
@@ -400,7 +400,7 @@ pi433_receive(void *data)
}
  
  	/* length byte enabled? */

-   if (dev->rx_cfg.enable_length_byte == optionOn)
+   if (dev->rx_cfg.enable_length_byte == OPTION_ON)
{
retval = wait_event_interruptible(dev->fifo_wait_queue,
  dev->free_in_fifo < 
FIFO_SIZE);
@@ -525,11 +525,11 @@ pi433_tx_thread(void *data)
size = tx_cfg.fixed_message_length;
  
  		/* increase size, if len byte is requested */

-   if (tx_cfg.enable_length_byte == optionOn)
+   if (tx_cfg.enable_length_byte == OPTION_ON)
size++;
  
  		/* increase size, if adr byte is requested */

-   if