stv090x: remove indent levels
Subsystems (2)
PATCH 1/1 stv090x: remove indent levels
1) We can flip the "if (!lock)" check to "if (lock) return lock;" and then remove a big chunk of indenting.
2) There is a redundant "if (!lock)" which we can remove since we already know that lock is zero. This removes another indent level.
drivers/media/dvb-frontends/stv090x.c +79 -80
@@ -2146,7 +2146,7 @@ static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd)u32 reg;s32 car_step, steps, cur_step, dir, freq, timeout_lock; -int lock =0 ; +int lock;if (state->srate >=10000000 ) timeout_lock = timeout_dmd /3 ;
@@ -2154,97 +2154,96 @@ static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd) timeout_lock = timeout_dmd /2 ; lock =stv090x_get_dmdlock (state, timeout_lock);/* cold start wait */ -if (!lock) { -if (state->srate >=10000000 ) { -if (stv090x_chk_tmg (state)) { -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) - goto err; - lock =stv090x_get_dmdlock (state, timeout_dmd); - }else { - lock =0 ; - } +if (lock) +return lock; + +if (state->srate >=10000000 ) { +if (stv090x_chk_tmg (state)) { +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) + goto err; + lock =stv090x_get_dmdlock (state, timeout_dmd); }else { -if (state->srate <=4000000 ) - car_step =1000 ; -else if (state->srate <=7000000 ) - car_step =2000 ; -else if (state->srate <=10000000 ) - car_step =3000 ; + lock =0 ; + } + }else { +if (state->srate <=4000000 ) + car_step =1000 ; +else if (state->srate <=7000000 ) + car_step =2000 ; +else if (state->srate <=10000000 ) + car_step =3000 ; +else + car_step =5000 ; + + steps = (state->search_range /1000 ) / car_step; + steps /=2 ; + steps =2 * (steps +1 ); +if (steps <0 ) + steps =2 ; +else if (steps >12 ) + steps =12 ; + + cur_step =1 ; + dir =1 ; + + freq = state->frequency; + state->tuner_bw =stv090x_car_width (state->srate, state->rolloff) + state->srate; +while ((cur_step <= steps) && (!lock)) { +if (dir >0 ) + freq += cur_step * car_step;else - car_step =5000 ; - - steps = (state->search_range /1000 ) / car_step; - steps /=2 ; - steps =2 * (steps +1 ); -if (steps <0 ) - steps =2 ; -else if (steps >12 ) - steps =12 ; - - cur_step =1 ; - dir =1 ; - -if (!lock) { - freq = state->frequency; - state->tuner_bw =stv090x_car_width (state->srate, state->rolloff) + state->srate; -while ((cur_step <= steps) && (!lock)) { -if (dir >0 ) - freq += cur_step * car_step; -else - freq -= cur_step * car_step; - -/* Setup tuner */ -if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) - goto err; + freq -= cur_step * car_step; -if (state->config->tuner_set_frequency) { -if (state->config->tuner_set_frequency (fe, freq) <0 ) - goto err_gateoff; - } +/* Setup tuner */ +if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) + goto err; -if (state->config->tuner_set_bandwidth) { -if (state->config->tuner_set_bandwidth (fe, state->tuner_bw) <0 ) - goto err_gateoff; - } +if (state->config->tuner_set_frequency) { +if (state->config->tuner_set_frequency (fe, freq) <0 ) + goto err_gateoff; + } -if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) - goto err; +if (state->config->tuner_set_bandwidth) { +if (state->config->tuner_set_bandwidth (fe, state->tuner_bw) <0 ) + goto err_gateoff; + } -msleep (50 ); +if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) + goto err; -if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) - goto err; +msleep (50 ); -if (state->config->tuner_get_status) { -if (state->config->tuner_get_status (fe, ®) <0 ) - goto err_gateoff; - } +if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) + goto err; -if (reg) -dprintk (FE_DEBUG ,1 ,"Tuner phase locked" ); -else -dprintk (FE_DEBUG ,1 ,"Tuner unlocked" ); +if (state->config->tuner_get_status) { +if (state->config->tuner_get_status (fe, ®) <0 ) + goto err_gateoff; + } -if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) - goto err; +if (reg) +dprintk (FE_DEBUG ,1 ,"Tuner phase locked" ); +else +dprintk (FE_DEBUG ,1 ,"Tuner unlocked" ); -STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1c ); -if (STV090x_WRITE_DEMOD (state,CFRINIT1 ,0x00 ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,CFRINIT0 ,0x00 ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) - goto err; - lock =stv090x_get_dmdlock (state, (timeout_dmd /3 )); +if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) + goto err; - dir *=-1 ; - cur_step++; - } - } +STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1c ); +if (STV090x_WRITE_DEMOD (state,CFRINIT1 ,0x00 ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,CFRINIT0 ,0x00 ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) + goto err; + lock =stv090x_get_dmdlock (state, (timeout_dmd /3 )); + + dir *=-1 ; + cur_step++; } }
Hi Dan,
Quoted 4 lines · Dan Carpenter
1) We can flip the "if (!lock)" check to "if (lock) return lock;" and then remove a big chunk of indenting.
2) There is a redundant "if (!lock)" which we can remove since we already know that lock is zero. This removes another indent level.
The stv090x driver is a mature, but slightly complex driver supporting quite some
different configurations. Is it that some bug you are trying to fix in there ? I wouldn't prefer unnecessary code churn in such a driver for something as simple
as gain in an indentation level.
Thanks,
Manu
Quoted 9 lines · Manu Abraham
Hi Dan,
Quoted 4 lines · Dan Carpenter
1) We can flip the "if (!lock)" check to "if (lock) return lock;" and then remove a big chunk of indenting.
2) There is a redundant "if (!lock)" which we can remove since we already know that lock is zero. This removes another indent level.
Quoted 6 lines
The stv090x driver is a mature, but slightly complex driver supporting quite some
different configurations. Is it that some bug you are trying to fix in there ? I wouldn't prefer unnecessary code churn in such a driver for something as simple
as gain in an indentation level.
I thought the cleanup was jusitification enough, but the real reason I wrote this patch is that testing:
if (!lock) {
if (!lock) {sets off a static checker warning. That kind of code is puzzling and if we don't clean it up then it wastes a lot of reviewer time.
Also when you're reviewing these patches please consider that the original code might be buggy and not simply messy. Perhaps something other than "if (!lock) {" was intended? When I review static checker warnings I am looking for bugs and I don't even list cleanup patches like this one in my status reports to my employer. Fixing these is just something I do which saves time in the long run.
Btw, I help maintain staging so I review these kinds of patches all the
time. I use a script to review these kinds of changes. It strips out
the whitespace changes and leaves the interesting bits of the patch. I have attached it.
cat email.patch | rename_rev.pl
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 " -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";
exit(1);
}my @subs;
my @cmds;
my $strip_comments;
my $strip_braces;
my $strip_slashes;
sub filter($) {
my $_ = shift();
my $old = 0;
if ($_ =~ /^-/) {
$old = 1;
}
# remove the first char
s/^[ +-]//;
if ($strip_comments) {
s/\/\*.*?\*\///g;
s/\/\/.*//;
}
foreach my $cmd (@cmds) {
if ($old || $cmd->[0] =~ /^-ea$/) {
eval $cmd->[1];
}
}
foreach my $sub (@subs) {
if ($old) {
s/$sub->[0]/$sub->[1]/g;
}
}
return $_;
}while (my $param1 = shift()) {
if ($param1 =~ /^-nc$/) {
$strip_comments = 1;
next;
}
if ($param1 =~ /^-nb$/) {
$strip_braces = 1;
next;
}
if ($param1 =~ /^-ns$/) {
$strip_slashes = 1;
next;
}
my $param2 = shift();
if ($param2 =~ /^$/) {
usage();
}
if ($param1 =~ /^-e(a|)$/) {
push @cmds, [$param1, $param2];
next;
}
push @subs, [$param1, $param2];
}my ($oldfh, $oldfile) = mkstemp("/tmp/oldXXXXX");
my ($newfh, $newfile) = mkstemp("/tmp/newXXXXX");
while (<>) {
if ($_ =~ /^(---|\+\+\+)/) {
next;
}
my $output = filter($_);
if ($_ =~ /^-/) {
print $oldfh $output;
next;
}
if ($_ =~ /^\+/) {
print $newfh $output;
next;
}
print $oldfh $output;
print $newfh $output;
}my $hunk;
my $old_txt;
my $new_txt;
open diff, "diff -uw $oldfile $newfile |";
while (<diff>) {
if ($_ =~ /^(---|\+\+\+)/) {
next;
}
if ($_ =~ /^@/) {
if ($strip_comments) {
$old_txt =~ s/\/\*.*?\*\///g;
$new_txt =~ s/\/\*.*?\*\///g;
}
if ($strip_braces) {
$old_txt =~ s/{([^;{]*?);}/$1;/g;
$new_txt =~ s/{([^;{]*?);}/$1;/g;
# this is a hack because i don't know how to replace nested
# unneeded curly braces.
$old_txt =~ s/{([^;{]*?);}/$1;/g;
$new_txt =~ s/{([^;{]*?);}/$1;/g;
}
if ($old_txt ne $new_txt) {
print $hunk;
print $_;
}
$hunk = "";
$old_txt = "";
$new_txt = "";
next;
}
$hunk = $hunk . $_;
if ($strip_slashes) {
s/\\$//;
}
if ($_ =~ /^-/) {
s/-//;
s/[ \t\n]//g;
$old_txt = $old_txt . $_;
next;
}
if ($_ =~ /^\+/) {
s/\+//;
s/[ \t\n]//g;
$new_txt = $new_txt . $_;
next;
}
if ($_ =~ /^ /) {
s/^ //;
s/[ \t\n]//g;
$old_txt = $old_txt . $_;
$new_txt = $new_txt . $_;
}
}
if ($old_txt ne $new_txt) {
if ($strip_comments) {
$old_txt =~ s/\/\*.*?\*\///g;
$new_txt =~ s/\/\*.*?\*\///g;
}
if ($strip_braces) {
$old_txt =~ s/{([^;{]*?);}/$1;/g;
$new_txt =~ s/{([^;{]*?);}/$1;/g;
$old_txt =~ s/{([^;{]*?);}/$1;/g;
$new_txt =~ s/{([^;{]*?);}/$1;/g;
}
print $hunk;
}unlink($oldfile);
unlink($newfile);
Quoted 23 lines · Dan Carpenter
Quoted 9 lines · Manu Abraham
Hi Dan,
Quoted 4 lines · Dan Carpenter
1) We can flip the "if (!lock)" check to "if (lock) return lock;" and then remove a big chunk of indenting.
2) There is a redundant "if (!lock)" which we can remove since we already know that lock is zero. This removes another indent level.Quoted 6 lines
The stv090x driver is a mature, but slightly complex driver supporting quite some
different configurations. Is it that some bug you are trying to fix in there ? I wouldn't prefer unnecessary code churn in such a driver for something as simple
as gain in an indentation level.I thought the cleanup was jusitification enough, but the real reason I wrote this patch is that testing:
if (!lock) { if (!lock) {
Quoted 7 lines
sets off a static checker warning. That kind of code is puzzling and if we don't clean it up then it wastes a lot of reviewer time.
Also when you're reviewing these patches please consider that the original code might be buggy and not simply messy. Perhaps something other than "if (!lock) {" was intended?
I can't seem to find the possible bug in there..
The code:
lock = fn();
if (!lock) {
if (condition1) {
lock = fn()
} else {
if (!lock) {
}
}
}looks harmless to me, AFAICS. Also, please do note that, if the function coldlock exits due to some reason for not finding valid symbols, stv090x_search is again fired up from the kernel frontend thread. It is easy to make such cleanup patches and cause breakages, but a lot time consuming to fix such issues. My 2c.
Best Regards,
Manu
Quoted 46 lines · Manu Abraham
Quoted 23 lines · Dan Carpenter
Quoted 9 lines · Manu Abraham
Hi Dan,
Quoted 4 lines · Dan Carpenter
1) We can flip the "if (!lock)" check to "if (lock) return lock;" and then remove a big chunk of indenting.
2) There is a redundant "if (!lock)" which we can remove since we already know that lock is zero. This removes another indent level.Quoted 6 lines
The stv090x driver is a mature, but slightly complex driver supporting quite some
different configurations. Is it that some bug you are trying to fix in there ? I wouldn't prefer unnecessary code churn in such a driver for something as simple
as gain in an indentation level.I thought the cleanup was jusitification enough, but the real reason I wrote this patch is that testing:
if (!lock) { if (!lock) {Quoted 7 lines
sets off a static checker warning. That kind of code is puzzling and if we don't clean it up then it wastes a lot of reviewer time.
Also when you're reviewing these patches please consider that the original code might be buggy and not simply messy. Perhaps something other than "if (!lock) {" was intended?
I can't seem to find the possible bug in there..
The code:
lock =
fn();if (!lock) { if (condition1) { lock = fn() } else { if (!lock) { } } }
Quoted 1 line
looks harmless to me, AFAICS.
Yes. I thought so too. It's just a messy, but not broken. Let's just fix the static checker warning so that we don't have to keep reviewing it every several months.
Quoted 3 lines
Also, please do note that, if the
function coldlock exits due to some reason for not finding valid symbols,stv090x_searchis again fired up from the kernel frontend thread.
This sentence really scares me. Are you trying to say that the second check on lock is valid for certain use cases? That is not possibly true because it is a stack variable so (ignoring memory corruption) it can't be modified from outside the code.
Hm...
The code actually looks like this:
lock = fn();
if (!lock) {
if (condition1) {
lock = fn()
} else {
if (!lock) {
while ((cur_step <= steps) && (!lock)) {
lock = stv090x_get_dmdlock(state, (timeout_dmd / 3));
}
}
}
}Are you sure it's not buggy? Maybe the if statement should be moved inside the while () condition?
Quoted 2 lines
It is easy to make such cleanup patches and cause breakages, but a lot time consuming to fix such issues. My 2c.
Greg K-H and I review more of these cleanup patches than any other kernel maintainers so I'm aware of the challenges. If you want to write a smaller patch to fix the static checker warning then I will review it for you. If you do that then please give me a:
regards,
dan carpenter
Quoted 53 lines · Dan Carpenter
Quoted 46 lines · Manu Abraham
Quoted 23 lines · Dan Carpenter
Quoted 9 lines · Manu Abraham
Hi Dan,
Quoted 4 lines · Dan Carpenter
1) We can flip the "if (!lock)" check to "if (lock) return lock;" and then remove a big chunk of indenting.
2) There is a redundant "if (!lock)" which we can remove since we already know that lock is zero. This removes another indent level.Quoted 6 lines
The stv090x driver is a mature, but slightly complex driver supporting quite some
different configurations. Is it that some bug you are trying to fix in there ? I wouldn't prefer unnecessary code churn in such a driver for something as simple
as gain in an indentation level.I thought the cleanup was jusitification enough, but the real reason I wrote this patch is that testing:
if (!lock) { if (!lock) {Quoted 7 lines
sets off a static checker warning. That kind of code is puzzling and if we don't clean it up then it wastes a lot of reviewer time.
Also when you're reviewing these patches please consider that the original code might be buggy and not simply messy. Perhaps something other than "if (!lock) {" was intended?
I can't seem to find the possible bug in there..
The code:
lock =
fn();if (!lock) { if (condition1) { lock = fn() } else { if (!lock) { } } }Quoted 1 line
looks harmless to me, AFAICS.
Yes. I thought so too. It's just a messy, but not broken. Let's just fix the static checker warning so that we don't have to keep reviewing it every several months.
Quoted 8 lines
Quoted 3 lines
Also, please do note that, if the
function coldlock exits due to some reason for not finding valid symbols,stv090x_searchis again fired up from the kernel frontend thread.This sentence really scares me. Are you trying to say that the second check on lock is valid for certain use cases? That is not possibly true because it is a stack variable so (ignoring memory corruption) it can't be modified from outside the code.
No, the demodulator registers are Read-modify-Write. ie, if a read didn't occur, possibly registers won't be updated as when required. This might cause the demodulator and the driver to be in 2 different states. It's actually a state machine in there. ie, the demodulator might be in a warm state, while the driver might assume a cold state or vice versa. Possibly, this would result in longer, lock acquisition periods. Since it is doing a blind symbol rate search, a possibility of a faulty lock also exists, if the initial state is screwed up. Hence, hesitant to flip the logic involved.
Quoted 21 lines
Hm...
The code actually looks like this:
lock =
fn();if (!lock) { if (condition1) { lock = fn() } else { if (!lock) { while ((cur_step <= steps) && (!lock)) { lock = stv090x_get_dmdlock(state, (timeout_dmd / 3)); } } } }Are you sure it's not buggy? Maybe the if statement should be moved inside the while () condition?
Quoted 8 lines
Quoted 2 lines
It is easy to make such cleanup patches and cause breakages, but a lot time consuming to fix such issues. My 2c.
Greg K-H and I review more of these cleanup patches than any other kernel maintainers so I'm aware of the challenges. If you want to write a smaller patch to fix the static checker warning then I will review it for you. If you do that then please give me a:
Reported-by Dan Carpenter <[email protected]>
Ok, will have a look at it later, the second lock test might be superfluous, which will fix your static checker as well.
Best Regards,
Manu
Guys, what Manu is saying is purest nonsense. The "lock" variable is a stack variable, it's not a "demodulator Read-modify-Write register". The implications of changing "if (!lock)" to "if (lock)" are simple and obvious.
He's not reviewing patches, he's just NAKing them. It's not helpful.
regards,
dan carpenter
Quoted 4 lines · Dan Carpenter
Guys, what Manu is saying is purest nonsense. The "lock" variable is a stack variable, it's not a "demodulator Read-modify-Write register". The implications of changing "if (!lock)" to "if (lock)" are simple and obvious.
Sorry, you mistook. By demodulator Read-modify-Write register, I do really mean a register on the demodulator. If you do miss a read when flipping a logic, it does indeed make a large difference.
Quoted 3 lines
He's not reviewing patches, he's just NAKing them. It's not helpful.
Uh !?
I said "Ok, will have a look at it later, the second lock test might be superfluous,
which will fix your static checker as well."
Where's the NAK in there ?
Just said that, I prefer a simplified version, rather than that logic flip.
Regards,
Manu
Hi Dan,
This can be improved even more:
Quoted 35 lines · Dan Carpenter
1) We can flip the "if (!lock)" check to "if (lock) return lock;" and then remove a big chunk of indenting.
2) There is a redundant "if (!lock)" which we can remove since we already know that lock is zero. This removes another indent level.Signed-off-by Dan Carpenter <[email protected]>drivers/media/dvb-frontends/stv090x.c +1
-12@@ -2146,7 +2146,7 @@ static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd)u32 reg;s32 car_step, steps, cur_step, dir, freq, timeout_lock; -int lock =0 ; +int lock;if (state->srate >=10000000 ) timeout_lock = timeout_dmd /3 ;@@ -2154,97 +2154,96 @@ static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd) timeout_lock = timeout_dmd /2 ; lock =stv090x_get_dmdlock (state, timeout_lock);/* cold start wait */ -if (!lock) { -if (state->srate >=10000000 ) { -if (stv090x_chk_tmg (state)) { -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) - goto err; - lock =stv090x_get_dmdlock (state, timeout_dmd); - }else { - lock =0 ; - }
drivers/media/dvb-frontends/stv090x.c static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd) · line 2166
Quoted 10 lines
drivers/media/dvb-frontends/stv090x.c +10
-0+if (lock) +return lock; + +if (state->srate >=10000000 ) { +if (stv090x_chk_tmg (state)) { +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) + goto err; + lock =stv090x_get_dmdlock (state, timeout_dmd);
You can just return here...
drivers/media/dvb-frontends/stv090x.c static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd) · line 2168
Quoted 8 lines
drivers/media/dvb-frontends/stv090x.c +1
-6}else { -if (state->srate <=4000000 ) - car_step =1000 ; -else if (state->srate <=7000000 ) - car_step =2000 ; -else if (state->srate <=10000000 ) - car_step =3000 ; + lock =0 ;
and here. That way everything inside 'else' can be move one indent to the left as well.
Regards,
Hans
Quoted 148 lines
drivers/media/dvb-frontends/stv090x.c +67
-63+ } + }else { +if (state->srate <=4000000 ) + car_step =1000 ; +else if (state->srate <=7000000 ) + car_step =2000 ; +else if (state->srate <=10000000 ) + car_step =3000 ; +else + car_step =5000 ; + + steps = (state->search_range /1000 ) / car_step; + steps /=2 ; + steps =2 * (steps +1 ); +if (steps <0 ) + steps =2 ; +else if (steps >12 ) + steps =12 ; + + cur_step =1 ; + dir =1 ; + + freq = state->frequency; + state->tuner_bw =stv090x_car_width (state->srate, state->rolloff) + state->srate; +while ((cur_step <= steps) && (!lock)) { +if (dir >0 ) + freq += cur_step * car_step;else - car_step =5000 ; - - steps = (state->search_range /1000 ) / car_step; - steps /=2 ; - steps =2 * (steps +1 ); -if (steps <0 ) - steps =2 ; -else if (steps >12 ) - steps =12 ; - - cur_step =1 ; - dir =1 ; - -if (!lock) { - freq = state->frequency; - state->tuner_bw =stv090x_car_width (state->srate, state->rolloff) + state->srate; -while ((cur_step <= steps) && (!lock)) { -if (dir >0 ) - freq += cur_step * car_step; -else - freq -= cur_step * car_step; - -/* Setup tuner */ -if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) - goto err; + freq -= cur_step * car_step; -if (state->config->tuner_set_frequency) { -if (state->config->tuner_set_frequency (fe, freq) <0 ) - goto err_gateoff; - } +/* Setup tuner */ +if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) + goto err; -if (state->config->tuner_set_bandwidth) { -if (state->config->tuner_set_bandwidth (fe, state->tuner_bw) <0 ) - goto err_gateoff; - } +if (state->config->tuner_set_frequency) { +if (state->config->tuner_set_frequency (fe, freq) <0 ) + goto err_gateoff; + } -if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) - goto err; +if (state->config->tuner_set_bandwidth) { +if (state->config->tuner_set_bandwidth (fe, state->tuner_bw) <0 ) + goto err_gateoff; + } -msleep (50 ); +if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) + goto err; -if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) - goto err; +msleep (50 ); -if (state->config->tuner_get_status) { -if (state->config->tuner_get_status (fe, ®) <0 ) - goto err_gateoff; - } +if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) + goto err; -if (reg) -dprintk (FE_DEBUG ,1 ,"Tuner phase locked" ); -else -dprintk (FE_DEBUG ,1 ,"Tuner unlocked" ); +if (state->config->tuner_get_status) { +if (state->config->tuner_get_status (fe, ®) <0 ) + goto err_gateoff; + } -if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) - goto err; +if (reg) +dprintk (FE_DEBUG ,1 ,"Tuner phase locked" ); +else +dprintk (FE_DEBUG ,1 ,"Tuner unlocked" ); -STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1c ); -if (STV090x_WRITE_DEMOD (state,CFRINIT1 ,0x00 ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,CFRINIT0 ,0x00 ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) - goto err; - lock =stv090x_get_dmdlock (state, (timeout_dmd /3 )); +if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) + goto err; - dir *=-1 ; - cur_step++; - } - } +STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1c ); +if (STV090x_WRITE_DEMOD (state,CFRINIT1 ,0x00 ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,CFRINIT0 ,0x00 ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) + goto err; + lock =stv090x_get_dmdlock (state, (timeout_dmd /3 )); + + dir *=-1 ; + cur_step++; } } --To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines · Hans Verkuil
Hi Dan,
This can be improved even more:
Sure. Thanks. I will send v2 tomorrow.
regards,
dan carpenter
PATCH v2 1/1 stv090x: remove indent levels in stv090x_get_coldlock()
In reply to Hans VerkuilThis code is needlessly complicated and checkpatch.pl complains that we go over the 80 characters per line limit.
If we flip the "if (!lock) {" test to "if (lock) return;" then we can remove an indent level from the rest of the function.
We can add two returns in the "if (state->srate >= 10000000) {" condition and move the else statement back an additional indent level.
There is another "if (!lock) {" check which can be removed since we have already checked "lock" and know it is zero at this point. This second check on "lock" is also a problem because it sets off a static checker warning. I have reviewed this code for some time to see if something else was intended, but have concluded that it was simply an oversight and should be removed. Removing this duplicative check gains us an third indent level.
---
v2: add the returns in the "if (state->srate >= 10000000) {" condition.
drivers/media/dvb-frontends/stv090x.c +80 -82
@@ -2146,7 +2146,7 @@ static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd)u32 reg;s32 car_step, steps, cur_step, dir, freq, timeout_lock; -int lock =0 ; +int lock;if (state->srate >=10000000 ) timeout_lock = timeout_dmd /3 ;
@@ -2154,98 +2154,96 @@ static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd) timeout_lock = timeout_dmd /2 ; lock =stv090x_get_dmdlock (state, timeout_lock);/* cold start wait */ -if (!lock) { -if (state->srate >=10000000 ) { -if (stv090x_chk_tmg (state)) { -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) - goto err; - lock =stv090x_get_dmdlock (state, timeout_dmd); - }else { - lock =0 ; - } - }else { -if (state->srate <=4000000 ) - car_step =1000 ; -else if (state->srate <=7000000 ) - car_step =2000 ; -else if (state->srate <=10000000 ) - car_step =3000 ; -else - car_step =5000 ; - - steps = (state->search_range /1000 ) / car_step; - steps /=2 ; - steps =2 * (steps +1 ); -if (steps <0 ) - steps =2 ; -else if (steps >12 ) - steps =12 ; - - cur_step =1 ; - dir =1 ; - -if (!lock) { - freq = state->frequency; - state->tuner_bw =stv090x_car_width (state->srate, state->rolloff) + state->srate; -while ((cur_step <= steps) && (!lock)) { -if (dir >0 ) - freq += cur_step * car_step; -else - freq -= cur_step * car_step; - -/* Setup tuner */ -if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) - goto err; +if (lock) +return lock; -if (state->config->tuner_set_frequency) { -if (state->config->tuner_set_frequency (fe, freq) <0 ) - goto err_gateoff; - } +if (state->srate >=10000000 ) { +if (stv090x_chk_tmg (state)) { +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) + goto err; +return stv090x_get_dmdlock (state, timeout_dmd); + } +return 0 ; + } -if (state->config->tuner_set_bandwidth) { -if (state->config->tuner_set_bandwidth (fe, state->tuner_bw) <0 ) - goto err_gateoff; - } +if (state->srate <=4000000 ) + car_step =1000 ; +else if (state->srate <=7000000 ) + car_step =2000 ; +else if (state->srate <=10000000 ) + car_step =3000 ; +else + car_step =5000 ; -if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) - goto err; + steps = (state->search_range /1000 ) / car_step; + steps /=2 ; + steps =2 * (steps +1 ); +if (steps <0 ) + steps =2 ; +else if (steps >12 ) + steps =12 ; -msleep (50 ); + cur_step =1 ; + dir =1 ; -if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) - goto err; + freq = state->frequency; + state->tuner_bw =stv090x_car_width (state->srate, state->rolloff) + state->srate; +while ((cur_step <= steps) && (!lock)) { +if (dir >0 ) + freq += cur_step * car_step; +else + freq -= cur_step * car_step; -if (state->config->tuner_get_status) { -if (state->config->tuner_get_status (fe, ®) <0 ) - goto err_gateoff; - } +/* Setup tuner */ +if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) + goto err; -if (reg) -dprintk (FE_DEBUG ,1 ,"Tuner phase locked" ); -else -dprintk (FE_DEBUG ,1 ,"Tuner unlocked" ); +if (state->config->tuner_set_frequency) { +if (state->config->tuner_set_frequency (fe, freq) <0 ) + goto err_gateoff; + } -if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) - goto err; +if (state->config->tuner_set_bandwidth) { +if (state->config->tuner_set_bandwidth (fe, state->tuner_bw) <0 ) + goto err_gateoff; + } -STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1c ); -if (STV090x_WRITE_DEMOD (state,CFRINIT1 ,0x00 ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,CFRINIT0 ,0x00 ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) - goto err; - lock =stv090x_get_dmdlock (state, (timeout_dmd /3 )); +if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) + goto err; - dir *=-1 ; - cur_step++; - } - } +msleep (50 ); + +if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) + goto err; + +if (state->config->tuner_get_status) { +if (state->config->tuner_get_status (fe, ®) <0 ) + goto err_gateoff; } + +if (reg) +dprintk (FE_DEBUG ,1 ,"Tuner phase locked" ); +else +dprintk (FE_DEBUG ,1 ,"Tuner unlocked" ); + +if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) + goto err; + +STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1c ); +if (STV090x_WRITE_DEMOD (state,CFRINIT1 ,0x00 ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,CFRINIT0 ,0x00 ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) + goto err; + lock =stv090x_get_dmdlock (state, (timeout_dmd /3 )); + + dir *=-1 ; + cur_step++; }return lock;
Re: [patch v2] [media] stv090x: remove indent levels in stv090x_get_coldlock()
Am 21.02.2014 09:50, schrieb Dan Carpenter:
Quoted 102 lines
This code is needlessly complicated and checkpatch.pl complains that we go over the 80 characters per line limit.
If we flip the "if (!lock) {" test to "if (lock) return;" then we can remove an indent level from the rest of the function.
We can add two returns in the "if (
state->srate>= 10000000) {" condition and move the else statement back an additional indent level.There is another "if (!lock) {" check which can be removed since we have already checked "lock" and know it is zero at this point. This second check on "lock" is also a problem because it sets off a static checker warning. I have reviewed this code for some time to see if something else was intended, but have concluded that it was simply an oversight and should be removed. Removing this duplicative check gains us an third indent level.
Signed-off-by Dan Carpenter <[email protected]>---
v2: add the returns in the "if (state->srate>= 10000000) {" condition.drivers/media/dvb-frontends/stv090x.c +13
-51@@ -2146,7 +2146,7 @@ static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd)u32 reg;s32 car_step, steps, cur_step, dir, freq, timeout_lock; -int lock =0 ; +int lock;if (state->srate >=10000000 ) timeout_lock = timeout_dmd /3 ;@@ -2154,98 +2154,96 @@ static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd) timeout_lock = timeout_dmd /2 ; lock =stv090x_get_dmdlock (state, timeout_lock);/* cold start wait */ -if (!lock) { -if (state->srate >=10000000 ) { -if (stv090x_chk_tmg (state)) { -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) - goto err; - lock =stv090x_get_dmdlock (state, timeout_dmd); - }else { - lock =0 ; - } - }else { -if (state->srate <=4000000 ) - car_step =1000 ; -else if (state->srate <=7000000 ) - car_step =2000 ; -else if (state->srate <=10000000 ) - car_step =3000 ; -else - car_step =5000 ; - - steps = (state->search_range /1000 ) / car_step; - steps /=2 ; - steps =2 * (steps +1 ); -if (steps <0 ) - steps =2 ; -else if (steps >12 ) - steps =12 ; - - cur_step =1 ; - dir =1 ; - -if (!lock) { - freq = state->frequency; - state->tuner_bw =stv090x_car_width (state->srate, state->rolloff) + state->srate; -while ((cur_step <= steps) && (!lock)) { -if (dir >0 ) - freq += cur_step * car_step; -else - freq -= cur_step * car_step; - -/* Setup tuner */ -if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) - goto err; +if (lock) +return lock; -if (state->config->tuner_set_frequency) { -if (state->config->tuner_set_frequency (fe, freq) <0 ) - goto err_gateoff; - } +if (state->srate >=10000000 ) { +if (stv090x_chk_tmg (state)) { +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) + goto err; +return stv090x_get_dmdlock (state, timeout_dmd); + } +return 0 ; + } -if (state->config->tuner_set_bandwidth) { -if (state->config->tuner_set_bandwidth (fe, state->tuner_bw) <0 )
drivers/media/dvb-frontends/stv090x.c static int stv090x_get_coldlock(struct stv090x_state *state, s32 timeout_dmd) · line 2178
Quoted 10 lines
drivers/media/dvb-frontends/stv090x.c +8
-2- goto err_gateoff; - } +if (state->srate <=4000000 ) + car_step =1000 ; +else if (state->srate <=7000000 ) + car_step =2000 ; +else if (state->srate <=10000000 ) + car_step =3000 ; +else + car_step =5000 ;
hi dan, just a hint
when you encounter large numbers like this: please format it in a way that it is easy to to see if a number is missing like:
car_step = 5000; if (state->srate <= 4000000) car_step = 1000; else if (state->srate <= 7000000) car_step = 2000; else (state->srate <= 10000000) car_step = 3000;
an other hint from the department of silly steps: do not add 0 in front or you will searching for the bug a long time :)
just my 2 cents,
re,
wh
Quoted 105 lines
drivers/media/dvb-frontends/stv090x.c +59
-30-if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) - goto err; + steps = (state->search_range /1000 ) / car_step; + steps /=2 ; + steps =2 * (steps +1 ); +if (steps <0 ) + steps =2 ; +else if (steps >12 ) + steps =12 ; -msleep (50 ); + cur_step =1 ; + dir =1 ; -if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) - goto err; + freq = state->frequency; + state->tuner_bw =stv090x_car_width (state->srate, state->rolloff) + state->srate; +while ((cur_step <= steps) && (!lock)) { +if (dir >0 ) + freq += cur_step * car_step; +else + freq -= cur_step * car_step; -if (state->config->tuner_get_status) { -if (state->config->tuner_get_status (fe, ®) <0 ) - goto err_gateoff; - } +/* Setup tuner */ +if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) + goto err; -if (reg) -dprintk (FE_DEBUG ,1 ,"Tuner phase locked" ); -else -dprintk (FE_DEBUG ,1 ,"Tuner unlocked" ); +if (state->config->tuner_set_frequency) { +if (state->config->tuner_set_frequency (fe, freq) <0 ) + goto err_gateoff; + } -if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) - goto err; +if (state->config->tuner_set_bandwidth) { +if (state->config->tuner_set_bandwidth (fe, state->tuner_bw) <0 ) + goto err_gateoff; + } -STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1c ); -if (STV090x_WRITE_DEMOD (state,CFRINIT1 ,0x00 ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,CFRINIT0 ,0x00 ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) - goto err; -if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) - goto err; - lock =stv090x_get_dmdlock (state, (timeout_dmd /3 )); +if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) + goto err; - dir *=-1 ; - cur_step++; - } - } +msleep (50 ); + +if (stv090x_i2c_gate_ctrl (state,1 ) <0 ) + goto err; + +if (state->config->tuner_get_status) { +if (state->config->tuner_get_status (fe, ®) <0 ) + goto err_gateoff; } + +if (reg) +dprintk (FE_DEBUG ,1 ,"Tuner phase locked" ); +else +dprintk (FE_DEBUG ,1 ,"Tuner unlocked" ); + +if (stv090x_i2c_gate_ctrl (state,0 ) <0 ) + goto err; + +STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1c ); +if (STV090x_WRITE_DEMOD (state,CFRINIT1 ,0x00 ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,CFRINIT0 ,0x00 ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x1f ) <0 ) + goto err; +if (STV090x_WRITE_DEMOD (state,DMDISTATE ,0x15 ) <0 ) + goto err; + lock =stv090x_get_dmdlock (state, (timeout_dmd /3 )); + + dir *=-1 ; + cur_step++; }return lock; --To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html