Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1739790 > unrolled thread

[PATCH 0/6] [media] TDA8261: Fine-tuning for five function implementations

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] [media] TDA8261: Fine-tuning for five function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-26 13:30 +0200
    [PATCH 1/6] [media] tda8261: Use common error handling code in  tda8261_set_params() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-26 13:30 +0200
      Re: [PATCH 1/6] [media] tda8261: Use common error handling code in  tda8261_set_params() Christoph Böhmwalder <christoph@boehmwalder.at> - 2017-09-26 13:50 +0200
        Re: [PATCH 1/6] [media] tda8261: Use common error handling code in  tda8261_set_params() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-26 19:00 +0200
    [PATCH 2/6] [media] tda8261: Improve a size determination in  tda8261_attach() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-26 13:30 +0200
    [PATCH 6/6] [media] tda8261: Delete an unnecessary variable  initialisation in three functions SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-26 13:40 +0200
    [PATCH 4/6] [media] tda8261: Delete an unnecessary variable  initialisation in tda8261_attach() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-26 13:40 +0200
    [PATCH 3/6] [media] tda8261: Return directly after a failed kzalloc()  in tda8261_attach() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-26 13:40 +0200
    [PATCH 5/6] [media] tda8261: Adjust three function calls together  with a variable assignment SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-26 13:40 +0200

#1739790 — [PATCH 0/6] [media] TDA8261: Fine-tuning for five function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
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

[toc] | [next] | [standalone]

#1739791 — [PATCH 1/6] [media] tda8261: Use common error handling code in tda8261_set_params()

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

#1739804 — Re: [PATCH 1/6] [media] tda8261: Use common error handling code in tda8261_set_params()

[Multipart message — attachments visible in raw view] — view raw

On 26.09.2017 13:27, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

#1740040 — Re: [PATCH 1/6] [media] tda8261: Use common error handling code in tda8261_set_params()

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

[toc] | [prev] | [next] | [standalone]

#1739795 — [PATCH 2/6] [media] tda8261: Improve a size determination in tda8261_attach()

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

#1739796 — [PATCH 6/6] [media] tda8261: Delete an unnecessary variable initialisation in three functions

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

#1739797 — [PATCH 4/6] [media] tda8261: Delete an unnecessary variable initialisation in tda8261_attach()

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

#1739799 — [PATCH 3/6] [media] tda8261: Return directly after a failed kzalloc() in tda8261_attach()

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

#1739801 — [PATCH 5/6] [media] tda8261: Adjust three function calls together with a variable assignment

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web