From: Markus Elfring <[email protected]>
Date: Tue, 26 Sep 2017 13:20:12 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (6):
Use common error handling code in tda8261_set_params()
Improve a size determination in tda8261_attach()
Return directly after a failed kzalloc() in tda8261_attach()
Delete an unnecessary variable initialisation in tda8261_attach()
Adjust three function calls together with a variable assignment
Delete an unnecessary variable initialisation in three functions
drivers/media/dvb-frontends/tda8261.c | 47 +++++++++++++++++++----------------
1 file changed, 25 insertions(+), 22 deletions(-)
--
2.14.1
From: Markus Elfring <[email protected]>
Date: Tue, 26 Sep 2017 11:01:44 +0200
* Add a jump target so that a bit of exception handling can be better
reused at the end of this function.
This issue was detected by using the Coccinelle software.
* The script "checkpatch.pl" pointed information out like the following.
ERROR: do not use assignment in if condition
Thus fix an affected source code place.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/media/dvb-frontends/tda8261.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/media/dvb-frontends/tda8261.c b/drivers/media/dvb-frontends/tda8261.c
index 4eb294f330bc..5a8a9b6b8107 100644
--- a/drivers/media/dvb-frontends/tda8261.c
+++ b/drivers/media/dvb-frontends/tda8261.c
@@ -129,18 +129,18 @@ static int tda8261_set_params(struct dvb_frontend *fe)
/* Set params */
err = tda8261_write(state, buf);
- if (err < 0) {
- pr_err("%s: I/O Error\n", __func__);
- return err;
- }
+ err = tda8261_get_status(fe, &status);
+ if (err < 0)
+ goto report_failure;
+
/* sleep for some time */
pr_debug("%s: Waiting to Phase LOCK\n", __func__);
msleep(20);
/* check status */
- if ((err = tda8261_get_status(fe, &status)) < 0) {
- pr_err("%s: I/O Error\n", __func__);
- return err;
- }
+ err = tda8261_get_status(fe, &status);
+ if (err < 0)
+ goto report_failure;
+
if (status == 1) {
pr_debug("%s: Tuner Phase locked: status=%d\n", __func__,
status);
@@ -150,6 +150,10 @@ static int tda8261_set_params(struct dvb_frontend *fe)
}
return 0;
+
+report_failure:
+ pr_err("%s: I/O Error\n", __func__);
+ return err;
}
static void tda8261_release(struct dvb_frontend *fe)
--
2.14.1
From: Markus Elfring <[email protected]>
Date: Tue, 26 Sep 2017 12:06:19 +0200
* The script "checkpatch.pl" pointed information out like the following.
ERROR: do not use assignment in if condition
Thus fix an affected source code place.
* Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/media/dvb-frontends/tda8261.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/dvb-frontends/tda8261.c b/drivers/media/dvb-frontends/tda8261.c
index 5a8a9b6b8107..5269a170c84e 100644
--- a/drivers/media/dvb-frontends/tda8261.c
+++ b/drivers/media/dvb-frontends/tda8261.c
@@ -185,7 +185,8 @@ struct dvb_frontend *tda8261_attach(struct dvb_frontend *fe,
{
struct tda8261_state *state = NULL;
- if ((state = kzalloc(sizeof (struct tda8261_state), GFP_KERNEL)) == NULL)
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (!state)
goto exit;
state->config = config;
--
2.14.1
From: Markus Elfring <[email protected]>
Date: Tue, 26 Sep 2017 12:20:33 +0200
* Return directly after a call of the function "kzalloc" failed
at the beginning.
* Delete a call of the function "kfree" and the jump target "exit"
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/media/dvb-frontends/tda8261.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/media/dvb-frontends/tda8261.c b/drivers/media/dvb-frontends/tda8261.c
index 5269a170c84e..e3b4183d00c2 100644
--- a/drivers/media/dvb-frontends/tda8261.c
+++ b/drivers/media/dvb-frontends/tda8261.c
@@ -187,7 +187,7 @@ struct dvb_frontend *tda8261_attach(struct dvb_frontend *fe,
state = kzalloc(sizeof(*state), GFP_KERNEL);
if (!state)
- goto exit;
+ return NULL;
state->config = config;
state->i2c = i2c;
@@ -200,10 +200,6 @@ struct dvb_frontend *tda8261_attach(struct dvb_frontend *fe,
pr_info("%s: Attaching TDA8261 8PSK/QPSK tuner\n", __func__);
return fe;
-
-exit:
- kfree(state);
- return NULL;
}
EXPORT_SYMBOL(tda8261_attach);
--
2.14.1
From: Markus Elfring <[email protected]>
Date: Tue, 26 Sep 2017 12:24:57 +0200
The local variable "state" is reassigned by a statement at the beginning.
Thus omit the explicit initialisation.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/media/dvb-frontends/tda8261.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/dvb-frontends/tda8261.c b/drivers/media/dvb-frontends/tda8261.c
index e3b4183d00c2..492d8c03a5fa 100644
--- a/drivers/media/dvb-frontends/tda8261.c
+++ b/drivers/media/dvb-frontends/tda8261.c
@@ -183,7 +183,7 @@ struct dvb_frontend *tda8261_attach(struct dvb_frontend *fe,
const struct tda8261_config *config,
struct i2c_adapter *i2c)
{
- struct tda8261_state *state = NULL;
+ struct tda8261_state *state;
state = kzalloc(sizeof(*state), GFP_KERNEL);
if (!state)
--
2.14.1
From: Markus Elfring <[email protected]>
Date: Tue, 26 Sep 2017 12:52:24 +0200
The script "checkpatch.pl" pointed information out like the following.
ERROR: do not use assignment in if condition
Thus fix the affected source code places.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/media/dvb-frontends/tda8261.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/media/dvb-frontends/tda8261.c b/drivers/media/dvb-frontends/tda8261.c
index 492d8c03a5fa..20185ee8f253 100644
--- a/drivers/media/dvb-frontends/tda8261.c
+++ b/drivers/media/dvb-frontends/tda8261.c
@@ -42,7 +42,8 @@ static int tda8261_read(struct tda8261_state *state, u8 *buf)
int err = 0;
struct i2c_msg msg = { .addr = config->addr, .flags = I2C_M_RD,.buf = buf, .len = 1 };
- if ((err = i2c_transfer(state->i2c, &msg, 1)) != 1)
+ err = i2c_transfer(state->i2c, &msg, 1);
+ if (err != 1)
pr_err("%s: read error, err=%d\n", __func__, err);
return err;
@@ -54,7 +55,8 @@ static int tda8261_write(struct tda8261_state *state, u8 *buf)
int err = 0;
struct i2c_msg msg = { .addr = config->addr, .flags = 0, .buf = buf, .len = 4 };
- if ((err = i2c_transfer(state->i2c, &msg, 1)) != 1)
+ err = i2c_transfer(state->i2c, &msg, 1);
+ if (err != 1)
pr_err("%s: write error, err=%d\n", __func__, err);
return err;
@@ -67,8 +69,8 @@ static int tda8261_get_status(struct dvb_frontend *fe, u32 *status)
int err = 0;
*status = 0;
-
- if ((err = tda8261_read(state, &result)) < 0) {
+ err = tda8261_read(state, &result);
+ if (err < 0) {
pr_err("%s: I/O Error\n", __func__);
return err;
}
--
2.14.1
From: Markus Elfring <[email protected]>
Date: Tue, 26 Sep 2017 12:55:16 +0200
The local variable "err" is reassigned by a statement at the beginning.
Thus omit the explicit initialisation in these functions.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/media/dvb-frontends/tda8261.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/dvb-frontends/tda8261.c b/drivers/media/dvb-frontends/tda8261.c
index 20185ee8f253..33144a34e337 100644
--- a/drivers/media/dvb-frontends/tda8261.c
+++ b/drivers/media/dvb-frontends/tda8261.c
@@ -39,7 +39,7 @@ struct tda8261_state {
static int tda8261_read(struct tda8261_state *state, u8 *buf)
{
const struct tda8261_config *config = state->config;
- int err = 0;
+ int err;
struct i2c_msg msg = { .addr = config->addr, .flags = I2C_M_RD,.buf = buf, .len = 1 };
err = i2c_transfer(state->i2c, &msg, 1);
@@ -52,7 +52,7 @@ static int tda8261_read(struct tda8261_state *state, u8 *buf)
static int tda8261_write(struct tda8261_state *state, u8 *buf)
{
const struct tda8261_config *config = state->config;
- int err = 0;
+ int err;
struct i2c_msg msg = { .addr = config->addr, .flags = 0, .buf = buf, .len = 4 };
err = i2c_transfer(state->i2c, &msg, 1);
@@ -66,7 +66,7 @@ static int tda8261_get_status(struct dvb_frontend *fe, u32 *status)
{
struct tda8261_state *state = fe->tuner_priv;
u8 result = 0;
- int err = 0;
+ int err;
*status = 0;
err = tda8261_read(state, &result);
--
2.14.1
On 26.09.2017 13:27, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Tue, 26 Sep 2017 11:01:44 +0200
>
> * Add a jump target so that a bit of exception handling can be better
> reused at the end of this function.
>
> This issue was detected by using the Coccinelle software.
>
> * The script "checkpatch.pl" pointed information out like the following.
>
> ERROR: do not use assignment in if condition
>
> Thus fix an affected source code place.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/media/dvb-frontends/tda8261.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/tda8261.c b/drivers/media/dvb-frontends/tda8261.c
> index 4eb294f330bc..5a8a9b6b8107 100644
> --- a/drivers/media/dvb-frontends/tda8261.c
> +++ b/drivers/media/dvb-frontends/tda8261.c
> @@ -129,18 +129,18 @@ static int tda8261_set_params(struct dvb_frontend *fe)
>
> /* Set params */
> err = tda8261_write(state, buf);
> - if (err < 0) {
> - pr_err("%s: I/O Error\n", __func__);
> - return err;
> - }
> + err = tda8261_get_status(fe, &status);
> + if (err < 0)
> + goto report_failure;
> +
Is this change really correct? Doesn't it query the status once more
often than before?
> /* sleep for some time */
> pr_debug("%s: Waiting to Phase LOCK\n", __func__);
> msleep(20);
> /* check status */
> - if ((err = tda8261_get_status(fe, &status)) < 0) {
> - pr_err("%s: I/O Error\n", __func__);
> - return err;
> - }
> + err = tda8261_get_status(fe, &status);
> + if (err < 0)
> + goto report_failure;
> +
> if (status == 1) {
> pr_debug("%s: Tuner Phase locked: status=%d\n", __func__,
> status);
> @@ -150,6 +150,10 @@ static int tda8261_set_params(struct dvb_frontend *fe)
> }
>
> return 0;
> +
> +report_failure:
> + pr_err("%s: I/O Error\n", __func__);
> + return err;
> }
>
> static void tda8261_release(struct dvb_frontend *fe)
>
--
Regards,
Christoph
>> @@ -129,18 +129,18 @@ static int tda8261_set_params(struct dvb_frontend *fe)
>>
>> /* Set params */
>> err = tda8261_write(state, buf);
>> - if (err < 0) {
>> - pr_err("%s: I/O Error\n", __func__);
>> - return err;
>> - }
>> + err = tda8261_get_status(fe, &status);
>> + if (err < 0)
>> + goto report_failure;
>> +
>
> Is this change really correct? Doesn't it query the status once more
> often than before?
Thanks for your inquiry.
Unfortunately, I made a copy mistake at this source code place.
When should I send a corrected suggestion for this update step
in the patch series?
Regards,
Markus