Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1298554 > unrolled thread
| Started by | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| First post | 2015-12-28 10:20 +0100 |
| Last post | 2015-12-28 15:50 +0100 |
| Articles | 10 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH] [media] tuners: One check less in m88rs6000t_get_rf_strength() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-28 10:20 +0100
Re: [PATCH] [media] tuners: One check less in m88rs6000t_get_rf_strength() after error detection Julia Lawall <julia.lawall@lip6.fr> - 2015-12-28 10:30 +0100
Re: [media] tuners: One check less in m88rs6000t_get_rf_strength() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-28 11:40 +0100
Re: [media] tuners: One check less in m88rs6000t_get_rf_strength() after error detection Julia Lawall <julia.lawall@lip6.fr> - 2015-12-28 11:40 +0100
[PATCH 0/2] [media] m88rs6000t: Fine-tuning for some function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-28 15:40 +0100
[PATCH 1/2] [media] m88rs6000t: Better exception handling in five functions SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-28 15:40 +0100
Re: [PATCH 1/2] [media] m88rs6000t: Better exception handling in five functions Julia Lawall <julia.lawall@lip6.fr> - 2015-12-28 15:50 +0100
Re: [media] m88rs6000t: Better exception handling in five functions SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-28 16:10 +0100
Re: [media] m88rs6000t: Better exception handling in five functions Julia Lawall <julia.lawall@lip6.fr> - 2015-12-28 16:20 +0100
[PATCH 2/2] [media] tuners: Refactoring for m88rs6000t_sleep() SF Markus Elfring <elfring@users.sourceforge.net> - 2015-12-28 15:50 +0100
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2015-12-28 10:20 +0100 |
| Subject | [PATCH] [media] tuners: One check less in m88rs6000t_get_rf_strength() after error detection |
| Message-ID | <qKC89-8ir-5@gated-at.bofh.it> |
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 10:10:34 +0100
This issue was detected by using the Coccinelle software.
Move the jump label directly before the desired log statement
so that the variable "ret" will not be checked once more
after it was determined that a function call failed.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/media/tuners/m88rs6000t.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
index 504bfbc..b45594e 100644
--- a/drivers/media/tuners/m88rs6000t.c
+++ b/drivers/media/tuners/m88rs6000t.c
@@ -510,27 +510,27 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
ret = regmap_read(dev->regmap, 0x5A, &val);
if (ret)
- goto err;
+ goto report_failure;
RF_GC = val & 0x0f;
ret = regmap_read(dev->regmap, 0x5F, &val);
if (ret)
- goto err;
+ goto report_failure;
IF_GC = val & 0x0f;
ret = regmap_read(dev->regmap, 0x3F, &val);
if (ret)
- goto err;
+ goto report_failure;
TIA_GC = (val >> 4) & 0x07;
ret = regmap_read(dev->regmap, 0x77, &val);
if (ret)
- goto err;
+ goto report_failure;
BB_GC = (val >> 4) & 0x0f;
ret = regmap_read(dev->regmap, 0x76, &val);
if (ret)
- goto err;
+ goto report_failure;
PGA2_GC = val & 0x3f;
PGA2_cri = PGA2_GC >> 2;
PGA2_crf = PGA2_GC & 0x03;
@@ -562,9 +562,11 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
/* scale value to 0x0000-0xffff */
gain = clamp_val(gain, 1000U, 10500U);
*strength = (10500 - gain) * 0xffff / (10500 - 1000);
-err:
- if (ret)
+
+ if (ret) {
+report_failure:
dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+ }
return ret;
}
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2015-12-28 10:30 +0100 |
| Subject | Re: [PATCH] [media] tuners: One check less in m88rs6000t_get_rf_strength() after error detection |
| Message-ID | <qKChP-8lQ-3@gated-at.bofh.it> |
| In reply to | #1298554 |
On Mon, 28 Dec 2015, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 28 Dec 2015 10:10:34 +0100
>
> This issue was detected by using the Coccinelle software.
>
> Move the jump label directly before the desired log statement
> so that the variable "ret" will not be checked once more
> after it was determined that a function call failed.
Why not avoid both unnecessary ifs and the enormous ugliness of a label
inside an if by making two returns: a return 0 for success and a dev_dbg
and return ret for failure?
julia
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/media/tuners/m88rs6000t.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
> index 504bfbc..b45594e 100644
> --- a/drivers/media/tuners/m88rs6000t.c
> +++ b/drivers/media/tuners/m88rs6000t.c
> @@ -510,27 +510,27 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
>
> ret = regmap_read(dev->regmap, 0x5A, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> RF_GC = val & 0x0f;
>
> ret = regmap_read(dev->regmap, 0x5F, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> IF_GC = val & 0x0f;
>
> ret = regmap_read(dev->regmap, 0x3F, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> TIA_GC = (val >> 4) & 0x07;
>
> ret = regmap_read(dev->regmap, 0x77, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> BB_GC = (val >> 4) & 0x0f;
>
> ret = regmap_read(dev->regmap, 0x76, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> PGA2_GC = val & 0x3f;
> PGA2_cri = PGA2_GC >> 2;
> PGA2_crf = PGA2_GC & 0x03;
> @@ -562,9 +562,11 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
> /* scale value to 0x0000-0xffff */
> gain = clamp_val(gain, 1000U, 10500U);
> *strength = (10500 - gain) * 0xffff / (10500 - 1000);
> -err:
> - if (ret)
> +
> + if (ret) {
> +report_failure:
> dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> + }
> return ret;
> }
>
> --
> 2.6.3
>
> --
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2015-12-28 11:40 +0100 |
| Subject | Re: [media] tuners: One check less in m88rs6000t_get_rf_strength() after error detection |
| Message-ID | <qKDnz-AY-1@gated-at.bofh.it> |
| In reply to | #1298556 |
>> Move the jump label directly before the desired log statement >> so that the variable "ret" will not be checked once more >> after it was determined that a function call failed. > > Why not avoid both unnecessary ifs I would find such a fine-tuning also nice in principle at more source code places. > and the enormous ugliness of a label inside an if by making two returns: > a return 0 for success and a dev_dbg and return ret for failure? How should your suggestion finally work when the desired execution success can be determined for such functions only after several other calls succeeded? Is consistent checking of failure predicates usually required? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2015-12-28 11:40 +0100 |
| Subject | Re: [media] tuners: One check less in m88rs6000t_get_rf_strength() after error detection |
| Message-ID | <qKDnA-AY-5@gated-at.bofh.it> |
| In reply to | #1298602 |
On Mon, 28 Dec 2015, SF Markus Elfring wrote: > >> Move the jump label directly before the desired log statement > >> so that the variable "ret" will not be checked once more > >> after it was determined that a function call failed. > > > > Why not avoid both unnecessary ifs > > I would find such a fine-tuning also nice in principle at more source code places. > > > > and the enormous ugliness of a label inside an if by making two returns: > > a return 0 for success and a dev_dbg and return ret for failure? > > How should your suggestion finally work when the desired execution success > can be determined for such functions only after several other calls succeeded? Not idea what this means, but immediate return 0 followed by various code for reacting to an error is very common, so it looks like it should be possible here. julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2015-12-28 15:40 +0100 |
| Subject | [PATCH 0/2] [media] m88rs6000t: Fine-tuning for some function implementations |
| Message-ID | <qKH7Q-3lo-1@gated-at.bofh.it> |
| In reply to | #1298603 |
From: Markus Elfring <elfring@users.sourceforge.net> Date: Mon, 28 Dec 2015 15:32:20 +0100 A few update suggestions were taken into account from static source code analysis. Markus Elfring (2): Better exception handling in five functions Refactoring for m88rs6000t_sleep() drivers/media/tuners/m88rs6000t.c | 165 +++++++++++++++++++------------------- 1 file changed, 83 insertions(+), 82 deletions(-) -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2015-12-28 15:40 +0100 |
| Subject | [PATCH 1/2] [media] m88rs6000t: Better exception handling in five functions |
| Message-ID | <qKH7Q-3lo-7@gated-at.bofh.it> |
| In reply to | #1298658 |
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 15:10:30 +0100
This issue was detected by using the Coccinelle software.
Move the jump label directly before the desired log statement
so that the variable "ret" will not be checked once more
after a function call.
Use the identifier "report_failure" instead of "err".
Suggested-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/media/tuners/m88rs6000t.c | 154 +++++++++++++++++++-------------------
1 file changed, 78 insertions(+), 76 deletions(-)
diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
index 504bfbc..7e59a9f 100644
--- a/drivers/media/tuners/m88rs6000t.c
+++ b/drivers/media/tuners/m88rs6000t.c
@@ -44,7 +44,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
/* select demod main mclk */
ret = regmap_read(dev->regmap, 0x15, &utmp);
if (ret)
- goto err;
+ goto report_failure;
reg15 = utmp;
if (c->symbol_rate > 45010000) {
reg11 = 0x0E;
@@ -106,7 +106,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
ret = regmap_read(dev->regmap, 0x1D, &utmp);
if (ret)
- goto err;
+ goto report_failure;
reg1D = utmp;
reg1D &= ~0x03;
reg1D |= N - 1;
@@ -116,42 +116,42 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
/* program and recalibrate demod PLL */
ret = regmap_write(dev->regmap, 0x05, 0x40);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x11, 0x08);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x15, reg15);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x16, reg16);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x1D, reg1D);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x1E, reg1E);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x1F, reg1F);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x17, 0xc1);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x17, 0x81);
if (ret)
- goto err;
+ goto report_failure;
usleep_range(5000, 50000);
ret = regmap_write(dev->regmap, 0x05, 0x00);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x11, reg11);
if (ret)
- goto err;
+ goto report_failure;
usleep_range(5000, 50000);
-err:
- if (ret)
- dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+ return 0;
+report_failure:
+ dev_dbg(&dev->client->dev, "failed=%d\n", ret);
return ret;
}
@@ -169,13 +169,13 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
ret = regmap_write(dev->regmap, 0x36, (refDiv - 8));
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x31, 0x00);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x2c, 0x02);
if (ret)
- goto err;
+ goto report_failure;
if (tuner_freq_MHz >= 1550) {
ucLoDiv1 = 2;
@@ -227,105 +227,105 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
reg27 = (((ulNDiv1 >> 8) & 0x0F) + ucLomod1) & 0x7F;
ret = regmap_write(dev->regmap, 0x27, reg27);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x28, (u8)(ulNDiv1 & 0xFF));
if (ret)
- goto err;
+ goto report_failure;
reg29 = (((ulNDiv2 >> 8) & 0x0F) + ucLomod2) & 0x7f;
ret = regmap_write(dev->regmap, 0x29, reg29);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x2a, (u8)(ulNDiv2 & 0xFF));
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x2F, 0xf5);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x30, 0x05);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x08, 0x1f);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x08, 0x3f);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x09, 0x20);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x09, 0x00);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x3e, 0x11);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x08, 0x2f);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x08, 0x3f);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x09, 0x10);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x09, 0x00);
if (ret)
- goto err;
+ goto report_failure;
usleep_range(2000, 50000);
ret = regmap_read(dev->regmap, 0x42, &utmp);
if (ret)
- goto err;
+ goto report_failure;
reg42 = utmp;
ret = regmap_write(dev->regmap, 0x3e, 0x10);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x08, 0x2f);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x08, 0x3f);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x09, 0x10);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x09, 0x00);
if (ret)
- goto err;
+ goto report_failure;
usleep_range(2000, 50000);
ret = regmap_read(dev->regmap, 0x42, &utmp);
if (ret)
- goto err;
+ goto report_failure;
reg42buf = utmp;
if (reg42buf < reg42) {
ret = regmap_write(dev->regmap, 0x3e, 0x11);
if (ret)
- goto err;
+ goto report_failure;
}
usleep_range(5000, 50000);
ret = regmap_read(dev->regmap, 0x2d, &utmp);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x2d, utmp);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_read(dev->regmap, 0x2e, &utmp);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x2e, utmp);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_read(dev->regmap, 0x27, &utmp);
if (ret)
- goto err;
+ goto report_failure;
reg27 = utmp & 0x70;
ret = regmap_read(dev->regmap, 0x83, &utmp);
if (ret)
- goto err;
+ goto report_failure;
if (reg27 == (utmp & 0x70)) {
ucLoDiv = ucLoDiv1;
ulNDiv = ulNDiv1;
@@ -340,7 +340,7 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
refDiv = 18;
ret = regmap_write(dev->regmap, 0x36, (refDiv - 8));
if (ret)
- goto err;
+ goto report_failure;
ulNDiv = ((tuner_freq_MHz * ucLoDiv * 1000) * refDiv
/ fcry_KHz - 1024) / 2;
}
@@ -349,16 +349,16 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
+ ((ulNDiv >> 8) & 0x0F)) & 0xFF;
ret = regmap_write(dev->regmap, 0x27, reg27);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x28, (u8)(ulNDiv & 0xFF));
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x29, 0x80);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x31, 0x03);
if (ret)
- goto err;
+ goto report_failure;
if (ucLoDiv == 3)
utmp = 0xCE;
@@ -366,15 +366,15 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
utmp = 0x8A;
ret = regmap_write(dev->regmap, 0x3b, utmp);
if (ret)
- goto err;
+ goto report_failure;
dev->frequency_khz = fcry_KHz * (ulNDiv * 2 + 1024) / refDiv / ucLoDiv;
dev_dbg(&dev->client->dev,
"actual tune frequency=%d\n", dev->frequency_khz);
-err:
- if (ret)
- dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+ return 0;
+report_failure:
+ dev_dbg(&dev->client->dev, "failed=%d\n", ret);
return ret;
}
@@ -413,21 +413,23 @@ static int m88rs6000t_set_params(struct dvb_frontend *fe)
freq_MHz = (realFreq + 500) / 1000;
ret = m88rs6000t_set_pll_freq(dev, freq_MHz);
if (ret)
- goto err;
+ goto report_failure;
ret = m88rs6000t_set_bb(dev, c->symbol_rate / 1000, lpf_offset_KHz);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x00, 0x01);
if (ret)
- goto err;
+ goto report_failure;
ret = regmap_write(dev->regmap, 0x00, 0x00);
if (ret)
- goto err;
+ goto report_failure;
/* set demod mlck */
ret = m88rs6000t_set_demod_mclk(fe);
-err:
if (ret)
- dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+ goto report_failure;
+ return 0;
+report_failure:
+ dev_dbg(&dev->client->dev, "failed=%d\n", ret);
return ret;
}
@@ -440,16 +442,16 @@ static int m88rs6000t_init(struct dvb_frontend *fe)
ret = regmap_update_bits(dev->regmap, 0x11, 0x08, 0x08);
if (ret)
- goto err;
+ goto report_failure;
usleep_range(5000, 50000);
ret = regmap_update_bits(dev->regmap, 0x10, 0x01, 0x01);
if (ret)
- goto err;
+ goto report_failure;
usleep_range(10000, 50000);
ret = regmap_write(dev->regmap, 0x07, 0x7d);
-err:
- if (ret)
- dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+ return 0;
+report_failure:
+ dev_dbg(&dev->client->dev, "failed=%d\n", ret);
return ret;
}
@@ -510,27 +512,27 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
ret = regmap_read(dev->regmap, 0x5A, &val);
if (ret)
- goto err;
+ goto report_failure;
RF_GC = val & 0x0f;
ret = regmap_read(dev->regmap, 0x5F, &val);
if (ret)
- goto err;
+ goto report_failure;
IF_GC = val & 0x0f;
ret = regmap_read(dev->regmap, 0x3F, &val);
if (ret)
- goto err;
+ goto report_failure;
TIA_GC = (val >> 4) & 0x07;
ret = regmap_read(dev->regmap, 0x77, &val);
if (ret)
- goto err;
+ goto report_failure;
BB_GC = (val >> 4) & 0x0f;
ret = regmap_read(dev->regmap, 0x76, &val);
if (ret)
- goto err;
+ goto report_failure;
PGA2_GC = val & 0x3f;
PGA2_cri = PGA2_GC >> 2;
PGA2_crf = PGA2_GC & 0x03;
@@ -562,9 +564,9 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
/* scale value to 0x0000-0xffff */
gain = clamp_val(gain, 1000U, 10500U);
*strength = (10500 - gain) * 0xffff / (10500 - 1000);
-err:
- if (ret)
- dev_dbg(&dev->client->dev, "failed=%d\n", ret);
+ return 0;
+report_failure:
+ dev_dbg(&dev->client->dev, "failed=%d\n", ret);
return ret;
}
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2015-12-28 15:50 +0100 |
| Subject | Re: [PATCH 1/2] [media] m88rs6000t: Better exception handling in five functions |
| Message-ID | <qKHhv-3pQ-11@gated-at.bofh.it> |
| In reply to | #1298659 |
On Mon, 28 Dec 2015, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 28 Dec 2015 15:10:30 +0100
>
> This issue was detected by using the Coccinelle software.
>
> Move the jump label directly before the desired log statement
> so that the variable "ret" will not be checked once more
> after a function call.
This commit message fits with the previous change.
It could be nice to put a blank line before the error handling code. See
what is done elsewhere in the file.
julia
>
> Suggested-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/media/tuners/m88rs6000t.c | 154 +++++++++++++++++++-------------------
> 1 file changed, 78 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
> index 504bfbc..7e59a9f 100644
> --- a/drivers/media/tuners/m88rs6000t.c
> +++ b/drivers/media/tuners/m88rs6000t.c
> @@ -44,7 +44,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
> /* select demod main mclk */
> ret = regmap_read(dev->regmap, 0x15, &utmp);
> if (ret)
> - goto err;
> + goto report_failure;
> reg15 = utmp;
> if (c->symbol_rate > 45010000) {
> reg11 = 0x0E;
> @@ -106,7 +106,7 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
>
> ret = regmap_read(dev->regmap, 0x1D, &utmp);
> if (ret)
> - goto err;
> + goto report_failure;
> reg1D = utmp;
> reg1D &= ~0x03;
> reg1D |= N - 1;
> @@ -116,42 +116,42 @@ static int m88rs6000t_set_demod_mclk(struct dvb_frontend *fe)
> /* program and recalibrate demod PLL */
> ret = regmap_write(dev->regmap, 0x05, 0x40);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x11, 0x08);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x15, reg15);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x16, reg16);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x1D, reg1D);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x1E, reg1E);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x1F, reg1F);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x17, 0xc1);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x17, 0x81);
> if (ret)
> - goto err;
> + goto report_failure;
> usleep_range(5000, 50000);
> ret = regmap_write(dev->regmap, 0x05, 0x00);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x11, reg11);
> if (ret)
> - goto err;
> + goto report_failure;
> usleep_range(5000, 50000);
> -err:
> - if (ret)
> - dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> + return 0;
> +report_failure:
> + dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> return ret;
> }
>
> @@ -169,13 +169,13 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
>
> ret = regmap_write(dev->regmap, 0x36, (refDiv - 8));
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x31, 0x00);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x2c, 0x02);
> if (ret)
> - goto err;
> + goto report_failure;
>
> if (tuner_freq_MHz >= 1550) {
> ucLoDiv1 = 2;
> @@ -227,105 +227,105 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
> reg27 = (((ulNDiv1 >> 8) & 0x0F) + ucLomod1) & 0x7F;
> ret = regmap_write(dev->regmap, 0x27, reg27);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x28, (u8)(ulNDiv1 & 0xFF));
> if (ret)
> - goto err;
> + goto report_failure;
> reg29 = (((ulNDiv2 >> 8) & 0x0F) + ucLomod2) & 0x7f;
> ret = regmap_write(dev->regmap, 0x29, reg29);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x2a, (u8)(ulNDiv2 & 0xFF));
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x2F, 0xf5);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x30, 0x05);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x08, 0x1f);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x08, 0x3f);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x09, 0x20);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x09, 0x00);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x3e, 0x11);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x08, 0x2f);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x08, 0x3f);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x09, 0x10);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x09, 0x00);
> if (ret)
> - goto err;
> + goto report_failure;
> usleep_range(2000, 50000);
>
> ret = regmap_read(dev->regmap, 0x42, &utmp);
> if (ret)
> - goto err;
> + goto report_failure;
> reg42 = utmp;
>
> ret = regmap_write(dev->regmap, 0x3e, 0x10);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x08, 0x2f);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x08, 0x3f);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x09, 0x10);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x09, 0x00);
> if (ret)
> - goto err;
> + goto report_failure;
> usleep_range(2000, 50000);
>
> ret = regmap_read(dev->regmap, 0x42, &utmp);
> if (ret)
> - goto err;
> + goto report_failure;
> reg42buf = utmp;
> if (reg42buf < reg42) {
> ret = regmap_write(dev->regmap, 0x3e, 0x11);
> if (ret)
> - goto err;
> + goto report_failure;
> }
> usleep_range(5000, 50000);
>
> ret = regmap_read(dev->regmap, 0x2d, &utmp);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x2d, utmp);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_read(dev->regmap, 0x2e, &utmp);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x2e, utmp);
> if (ret)
> - goto err;
> + goto report_failure;
>
> ret = regmap_read(dev->regmap, 0x27, &utmp);
> if (ret)
> - goto err;
> + goto report_failure;
> reg27 = utmp & 0x70;
> ret = regmap_read(dev->regmap, 0x83, &utmp);
> if (ret)
> - goto err;
> + goto report_failure;
> if (reg27 == (utmp & 0x70)) {
> ucLoDiv = ucLoDiv1;
> ulNDiv = ulNDiv1;
> @@ -340,7 +340,7 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
> refDiv = 18;
> ret = regmap_write(dev->regmap, 0x36, (refDiv - 8));
> if (ret)
> - goto err;
> + goto report_failure;
> ulNDiv = ((tuner_freq_MHz * ucLoDiv * 1000) * refDiv
> / fcry_KHz - 1024) / 2;
> }
> @@ -349,16 +349,16 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
> + ((ulNDiv >> 8) & 0x0F)) & 0xFF;
> ret = regmap_write(dev->regmap, 0x27, reg27);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x28, (u8)(ulNDiv & 0xFF));
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x29, 0x80);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x31, 0x03);
> if (ret)
> - goto err;
> + goto report_failure;
>
> if (ucLoDiv == 3)
> utmp = 0xCE;
> @@ -366,15 +366,15 @@ static int m88rs6000t_set_pll_freq(struct m88rs6000t_dev *dev,
> utmp = 0x8A;
> ret = regmap_write(dev->regmap, 0x3b, utmp);
> if (ret)
> - goto err;
> + goto report_failure;
>
> dev->frequency_khz = fcry_KHz * (ulNDiv * 2 + 1024) / refDiv / ucLoDiv;
>
> dev_dbg(&dev->client->dev,
> "actual tune frequency=%d\n", dev->frequency_khz);
> -err:
> - if (ret)
> - dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> + return 0;
> +report_failure:
> + dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> return ret;
> }
>
> @@ -413,21 +413,23 @@ static int m88rs6000t_set_params(struct dvb_frontend *fe)
> freq_MHz = (realFreq + 500) / 1000;
> ret = m88rs6000t_set_pll_freq(dev, freq_MHz);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = m88rs6000t_set_bb(dev, c->symbol_rate / 1000, lpf_offset_KHz);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x00, 0x01);
> if (ret)
> - goto err;
> + goto report_failure;
> ret = regmap_write(dev->regmap, 0x00, 0x00);
> if (ret)
> - goto err;
> + goto report_failure;
> /* set demod mlck */
> ret = m88rs6000t_set_demod_mclk(fe);
> -err:
> if (ret)
> - dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> + goto report_failure;
> + return 0;
> +report_failure:
> + dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> return ret;
> }
>
> @@ -440,16 +442,16 @@ static int m88rs6000t_init(struct dvb_frontend *fe)
>
> ret = regmap_update_bits(dev->regmap, 0x11, 0x08, 0x08);
> if (ret)
> - goto err;
> + goto report_failure;
> usleep_range(5000, 50000);
> ret = regmap_update_bits(dev->regmap, 0x10, 0x01, 0x01);
> if (ret)
> - goto err;
> + goto report_failure;
> usleep_range(10000, 50000);
> ret = regmap_write(dev->regmap, 0x07, 0x7d);
> -err:
> - if (ret)
> - dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> + return 0;
> +report_failure:
> + dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> return ret;
> }
>
> @@ -510,27 +512,27 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
>
> ret = regmap_read(dev->regmap, 0x5A, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> RF_GC = val & 0x0f;
>
> ret = regmap_read(dev->regmap, 0x5F, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> IF_GC = val & 0x0f;
>
> ret = regmap_read(dev->regmap, 0x3F, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> TIA_GC = (val >> 4) & 0x07;
>
> ret = regmap_read(dev->regmap, 0x77, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> BB_GC = (val >> 4) & 0x0f;
>
> ret = regmap_read(dev->regmap, 0x76, &val);
> if (ret)
> - goto err;
> + goto report_failure;
> PGA2_GC = val & 0x3f;
> PGA2_cri = PGA2_GC >> 2;
> PGA2_crf = PGA2_GC & 0x03;
> @@ -562,9 +564,9 @@ static int m88rs6000t_get_rf_strength(struct dvb_frontend *fe, u16 *strength)
> /* scale value to 0x0000-0xffff */
> gain = clamp_val(gain, 1000U, 10500U);
> *strength = (10500 - gain) * 0xffff / (10500 - 1000);
> -err:
> - if (ret)
> - dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> + return 0;
> +report_failure:
> + dev_dbg(&dev->client->dev, "failed=%d\n", ret);
> return ret;
> }
>
> --
> 2.6.3
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2015-12-28 16:10 +0100 |
| Subject | Re: [media] m88rs6000t: Better exception handling in five functions |
| Message-ID | <qKHAS-3Nt-15@gated-at.bofh.it> |
| In reply to | #1298663 |
>> Move the jump label directly before the desired log statement >> so that the variable "ret" will not be checked once more >> after a function call. > > This commit message fits with the previous change. Do you prefer an other wording? > It could be nice to put a blank line before the error handling code. Is it really a coding style requirement to insert another blank line between the suggested placement of the statement "return 0;" and the jump label? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2015-12-28 16:20 +0100 |
| Subject | Re: [media] m88rs6000t: Better exception handling in five functions |
| Message-ID | <qKHKx-3R8-1@gated-at.bofh.it> |
| In reply to | #1298674 |
On Mon, 28 Dec 2015, SF Markus Elfring wrote: > >> Move the jump label directly before the desired log statement > >> so that the variable "ret" will not be checked once more > >> after a function call. > > > > This commit message fits with the previous change. > > Do you prefer an other wording? Something like "Split the return into success and error cases, to avoid the need for testing before logging." The concept of the return being duplicated didn't come across in your message. > > It could be nice to put a blank line before the error handling code. > > Is it really a coding style requirement to insert another blank line between > the suggested placement of the statement "return 0;" and the jump label? I don't think it is a requirement. But some files do it, and if other functions in this file do it, then it would be nice to do the same. julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2015-12-28 15:50 +0100 |
| Subject | [PATCH 2/2] [media] tuners: Refactoring for m88rs6000t_sleep() |
| Message-ID | <qKHhw-3pQ-31@gated-at.bofh.it> |
| In reply to | #1298658 |
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 28 Dec 2015 15:20:45 +0100
This issue was detected by using the Coccinelle software.
1. Let us return directly if a call of the regmap_write() function failed.
2. Delete the jump label "err" then.
3. Return zero as a constant at the end.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/media/tuners/m88rs6000t.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/media/tuners/m88rs6000t.c b/drivers/media/tuners/m88rs6000t.c
index 7e59a9f..8d10798 100644
--- a/drivers/media/tuners/m88rs6000t.c
+++ b/drivers/media/tuners/m88rs6000t.c
@@ -463,13 +463,12 @@ static int m88rs6000t_sleep(struct dvb_frontend *fe)
dev_dbg(&dev->client->dev, "%s:\n", __func__);
ret = regmap_write(dev->regmap, 0x07, 0x6d);
- if (ret)
- goto err;
- usleep_range(5000, 10000);
-err:
- if (ret)
+ if (ret) {
dev_dbg(&dev->client->dev, "failed=%d\n", ret);
- return ret;
+ return ret;
+ }
+ usleep_range(5000, 10000);
+ return 0;
}
static int m88rs6000t_get_frequency(struct dvb_frontend *fe, u32 *frequency)
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web