| Message ID | 20140206092800.GB31780@elgon.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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, ®) < 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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
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 -- 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
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 -- 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
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, ®) < 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++; } }
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> -- 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