[patch] [media] stv090x: remove indent levels

[patch] [media] stv090x: remove indent levels

[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 <dan.carpenter@oracle.com>

diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
index 23e872f84742..76ee559577dd 100644
--- a/drivers/media/dvb-frontends/stv090x.c
+++ b/drivers/media/dvb-frontends/stv090x.c
@@ -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, &reg) < 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, &reg) < 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++;
 		}
 	}
 

[patch] [media] stv090x: remove indent levels

[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 <dan.carpenter@oracle.com>

diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
index 23e872f84742..76ee559577dd 100644
--- a/drivers/media/dvb-frontends/stv090x.c
+++ b/drivers/media/dvb-frontends/stv090x.c
@@ -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, &reg) < 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, &reg) < 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++;
 		}
 	}
 

Re: [patch] [media] stv090x: remove indent levels

[Manu Abraham]

Hi Dan,

On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 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

Re: [patch] [media] stv090x: remove indent levels

[Manu Abraham]

Hi Dan,

On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 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

Re: [patch] [media] stv090x: remove indent levels

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

On Tue, Feb 18, 2014 at 09:25:36AM +0530, Manu Abraham wrote:
> Hi Dan,
> 
> On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > 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.

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


[-- Attachment #2: rename_rev.pl --]
[-- Type: text/x-perl, Size: 4221 bytes --]

#!/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);

Re: [patch] [media] stv090x: remove indent levels

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

On Tue, Feb 18, 2014 at 09:25:36AM +0530, Manu Abraham wrote:
> Hi Dan,
> 
> On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > 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.

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


[-- Attachment #2: rename_rev.pl --]
[-- Type: text/x-perl, Size: 4221 bytes --]

#!/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);

Re: [patch] [media] stv090x: remove indent levels

[Manu Abraham]

On Tue, Feb 18, 2014 at 2:26 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Feb 18, 2014 at 09:25:36AM +0530, Manu Abraham wrote:
>> Hi Dan,
>>
>> On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > 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.
>
> 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?
>

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

Re: [patch] [media] stv090x: remove indent levels

[Manu Abraham]

On Tue, Feb 18, 2014 at 2:26 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Feb 18, 2014 at 09:25:36AM +0530, Manu Abraham wrote:
>> Hi Dan,
>>
>> On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > 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.
>
> 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?
>

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

Re: [patch] [media] stv090x: remove indent levels

[Dan Carpenter]

On Wed, Feb 19, 2014 at 10:52:32AM +0530, Manu Abraham wrote:
> On Tue, Feb 18, 2014 at 2:26 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Feb 18, 2014 at 09:25:36AM +0530, Manu Abraham wrote:
> >> Hi Dan,
> >>
> >> On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> > 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.
> >
> > 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?
> >
> 
> 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.

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.

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

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?

> 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 <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [patch] [media] stv090x: remove indent levels

[Dan Carpenter]

On Wed, Feb 19, 2014 at 10:52:32AM +0530, Manu Abraham wrote:
> On Tue, Feb 18, 2014 at 2:26 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Feb 18, 2014 at 09:25:36AM +0530, Manu Abraham wrote:
> >> Hi Dan,
> >>
> >> On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> > 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.
> >
> > 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?
> >
> 
> 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.

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.

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

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?

> 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 <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [patch] [media] stv090x: remove indent levels

[Manu Abraham]

On Wed, Feb 19, 2014 at 1:14 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Feb 19, 2014 at 10:52:32AM +0530, Manu Abraham wrote:
>> On Tue, Feb 18, 2014 at 2:26 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Tue, Feb 18, 2014 at 09:25:36AM +0530, Manu Abraham wrote:
>> >> Hi Dan,
>> >>
>> >> On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> >> > 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.
>> >
>> > 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?
>> >
>>
>> 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.
>
> 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.
>
>> 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.
>
> 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.

>
> 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?
>
>> 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 <dan.carpenter@oracle.com>

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

Re: [patch] [media] stv090x: remove indent levels

[Manu Abraham]

On Wed, Feb 19, 2014 at 1:14 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Feb 19, 2014 at 10:52:32AM +0530, Manu Abraham wrote:
>> On Tue, Feb 18, 2014 at 2:26 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Tue, Feb 18, 2014 at 09:25:36AM +0530, Manu Abraham wrote:
>> >> Hi Dan,
>> >>
>> >> On Thu, Feb 6, 2014 at 2:58 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> >> > 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.
>> >
>> > 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?
>> >
>>
>> 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.
>
> 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.
>
>> 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.
>
> 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.

>
> 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?
>
>> 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 <dan.carpenter@oracle.com>

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

Re: [patch] [media] stv090x: remove indent levels

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

He's not reviewing patches, he's just NAKing them.  It's not helpful.

regards,
dan carpenter

Re: [patch] [media] stv090x: remove indent levels

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

He's not reviewing patches, he's just NAKing them.  It's not helpful.

regards,
dan carpenter

Re: [patch] [media] stv090x: remove indent levels

[Manu Abraham]

On Thu, Feb 20, 2014 at 2:55 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 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.


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

Re: [patch] [media] stv090x: remove indent levels

[Manu Abraham]

On Thu, Feb 20, 2014 at 2:55 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 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.


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

Re: [patch] [media] stv090x: remove indent levels

[Hans Verkuil]

Hi Dan,

This can be improved even more:

On 02/06/14 10:28, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
> index 23e872f84742..76ee559577dd 100644
> --- a/drivers/media/dvb-frontends/stv090x.c
> +++ b/drivers/media/dvb-frontends/stv090x.c
> @@ -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);

You can just return here...

>  		} 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

> +		}
> +	} 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, &reg) < 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, &reg) < 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch] [media] stv090x: remove indent levels

[Hans Verkuil]

Hi Dan,

This can be improved even more:

On 02/06/14 10:28, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
> index 23e872f84742..76ee559577dd 100644
> --- a/drivers/media/dvb-frontends/stv090x.c
> +++ b/drivers/media/dvb-frontends/stv090x.c
> @@ -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);

You can just return here...

>  		} 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

> +		}
> +	} 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, &reg) < 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, &reg) < 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch] [media] stv090x: remove indent levels

[Dan Carpenter]

On Thu, Feb 20, 2014 at 11:24:21AM +0100, Hans Verkuil wrote:
> Hi Dan,
> 
> This can be improved even more:
> 

Sure.  Thanks.  I will send v2 tomorrow.

regards,
dan carpenter

Re: [patch] [media] stv090x: remove indent levels

[Dan Carpenter]

On Thu, Feb 20, 2014 at 11:24:21AM +0100, Hans Verkuil wrote:
> Hi Dan,
> 
> This can be improved even more:
> 

Sure.  Thanks.  I will send v2 tomorrow.

regards,
dan carpenter

[patch v2] [media] stv090x: remove indent levels in stv090x_get_coldlock()

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
v2: add the returns in the "if (state->srate >= 10000000) {" condition.

diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
index 23e872f84742..93f4979ea6e9 100644
--- a/drivers/media/dvb-frontends/stv090x.c
+++ b/drivers/media/dvb-frontends/stv090x.c
@@ -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, &reg) < 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, &reg) < 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;

[patch v2] [media] stv090x: remove indent levels in stv090x_get_coldlock()

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
v2: add the returns in the "if (state->srate >= 10000000) {" condition.

diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
index 23e872f84742..93f4979ea6e9 100644
--- a/drivers/media/dvb-frontends/stv090x.c
+++ b/drivers/media/dvb-frontends/stv090x.c
@@ -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, &reg) < 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, &reg) < 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()

[walter harms]

Am 21.02.2014 09:50, schrieb Dan Carpenter:
> 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 <dan.carpenter@oracle.com>
> ---
> v2: add the returns in the "if (state->srate >= 10000000) {" condition.
> 
> diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
> index 23e872f84742..93f4979ea6e9 100644
> --- a/drivers/media/dvb-frontends/stv090x.c
> +++ b/drivers/media/dvb-frontends/stv090x.c
> @@ -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;


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




>  
> -					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, &reg) < 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, &reg) < 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch v2] [media] stv090x: remove indent levels in stv090x_get_coldlock()

[walter harms]

Am 21.02.2014 09:50, schrieb Dan Carpenter:
> 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 <dan.carpenter@oracle.com>
> ---
> v2: add the returns in the "if (state->srate >= 10000000) {" condition.
> 
> diff --git a/drivers/media/dvb-frontends/stv090x.c b/drivers/media/dvb-frontends/stv090x.c
> index 23e872f84742..93f4979ea6e9 100644
> --- a/drivers/media/dvb-frontends/stv090x.c
> +++ b/drivers/media/dvb-frontends/stv090x.c
> @@ -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;


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




>  
> -					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, &reg) < 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, &reg) < 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>