[PATCH 0/5] Clean-ups to the cx22702 frontend driver

[PATCH 0/5] Clean-ups to the cx22702 frontend driver

[Jean Delvare]

Hi all, hi Steven,

I wrote these 5 patches cleaning up the cx22702 frontend driver a long
time ago, I've been using them for months, so it's probably about time
that I post them for review and, hopefully, upstream inclusion.

Note that I unfortunately do not have access to the CX22702 datasheet,
so my changes are based solely on analysis of the original code, and
testing with a real chip (on my WinFast DTV1000-T.)

[PATCH 1/5] cx22702: Clean up register access functions
[PATCH 2/5] cx22702: Drop useless initializations to 0
[PATCH 3/5] cx22702: Avoid duplicating code in branches
[PATCH 4/5] cx22702: Some things never change
[PATCH 5/5] cx22702: Simplify cx22702_set_tps()

Reviews/comments welcome, of course.

-- 
Jean Delvare

[PATCH 1/5] cx22702: Clean up register access functions

[Jean Delvare]

* Avoid temporary variables.
* Optimize success paths.
* Make error messages consistently verbose.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Steven Toth <stoth@kernellabs.com>
---
 drivers/media/dvb/frontends/cx22702.c |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

--- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c	2009-10-16 09:47:14.000000000 +0200
+++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c	2009-10-16 09:47:45.000000000 +0200
@@ -92,33 +92,36 @@ static int cx22702_writereg(struct cx227
 
 	ret = i2c_transfer(state->i2c, &msg, 1);
 
-	if (ret != 1)
+	if (ret != 1) {
 		printk(KERN_ERR
 			"%s: error (reg == 0x%02x, val == 0x%02x, ret == %i)\n",
 			__func__, reg, data, ret);
+		return -1;
+	}
 
-	return (ret != 1) ? -1 : 0;
+	return 0;
 }
 
 static u8 cx22702_readreg(struct cx22702_state *state, u8 reg)
 {
 	int ret;
-	u8 b0[] = { reg };
-	u8 b1[] = { 0 };
+	u8 data;
 
 	struct i2c_msg msg[] = {
 		{ .addr = state->config->demod_address, .flags = 0,
-			.buf = b0, .len = 1 },
+			.buf = &reg, .len = 1 },
 		{ .addr = state->config->demod_address, .flags = I2C_M_RD,
-			.buf = b1, .len = 1 } };
+			.buf = &data, .len = 1 } };
 
 	ret = i2c_transfer(state->i2c, msg, 2);
 
-	if (ret != 2)
-		printk(KERN_ERR "%s: readreg error (ret == %i)\n",
-			__func__, ret);
+	if (ret != 2) {
+		printk(KERN_ERR "%s: error (reg == 0x%02x, ret == %i)\n",
+			__func__, reg, ret);
+		return 0;
+	}
 
-	return b1[0];
+	return data;
 }
 
 static int cx22702_set_inversion(struct cx22702_state *state, int inversion)

-- 
Jean Delvare

Re: [PATCH 1/5] cx22702: Clean up register access functions

[Mauro Carvalho Chehab]

Em 10-09-2010 10:27, Jean Delvare escreveu:
> * Avoid temporary variables.
> * Optimize success paths.
> * Make error messages consistently verbose.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Steven Toth <stoth@kernellabs.com>
> ---
>  drivers/media/dvb/frontends/cx22702.c |   23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> --- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c	2009-10-16 09:47:14.000000000 +0200
> +++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c	2009-10-16 09:47:45.000000000 +0200
> @@ -92,33 +92,36 @@ static int cx22702_writereg(struct cx227
>  
>  	ret = i2c_transfer(state->i2c, &msg, 1);
>  
> -	if (ret != 1)
> +	if (ret != 1) {

As a suggestion, if you want to optimize success paths, you should use unlikely() for error
checks. This tells gcc to optimize the code to avoid cache cleanups for the likely condition:

	if (unlikely(ret != 1)) {

>  		printk(KERN_ERR
>  			"%s: error (reg == 0x%02x, val == 0x%02x, ret == %i)\n",
>  			__func__, reg, data, ret);
> +		return -1;
> +	}
>  
> -	return (ret != 1) ? -1 : 0;
> +	return 0;
>  }
>  
>  static u8 cx22702_readreg(struct cx22702_state *state, u8 reg)
>  {
>  	int ret;
> -	u8 b0[] = { reg };
> -	u8 b1[] = { 0 };
> +	u8 data;
>  
>  	struct i2c_msg msg[] = {
>  		{ .addr = state->config->demod_address, .flags = 0,
> -			.buf = b0, .len = 1 },
> +			.buf = &reg, .len = 1 },
>  		{ .addr = state->config->demod_address, .flags = I2C_M_RD,
> -			.buf = b1, .len = 1 } };
> +			.buf = &data, .len = 1 } };
>  
>  	ret = i2c_transfer(state->i2c, msg, 2);
>  
> -	if (ret != 2)
> -		printk(KERN_ERR "%s: readreg error (ret == %i)\n",
> -			__func__, ret);
> +	if (ret != 2) {

Same comment applies here.

> +		printk(KERN_ERR "%s: error (reg == 0x%02x, ret == %i)\n",
> +			__func__, reg, ret);
> +		return 0;
> +	}
>  
> -	return b1[0];
> +	return data;
>  }
>  
>  static int cx22702_set_inversion(struct cx22702_state *state, int inversion)
> 

[PATCH 1/5 v2] cx22702: Clean up register access functions

[Jean Delvare]

Hi Mauro,

On Fri, 10 Sep 2010 12:13:32 -0300, Mauro Carvalho Chehab wrote:
> Em 10-09-2010 10:27, Jean Delvare escreveu:
> > * Avoid temporary variables.
> > * Optimize success paths.
> > * Make error messages consistently verbose.
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Steven Toth <stoth@kernellabs.com>
> > ---
> >  drivers/media/dvb/frontends/cx22702.c |   23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > --- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c	2009-10-16 09:47:14.000000000 +0200
> > +++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c	2009-10-16 09:47:45.000000000 +0200
> > @@ -92,33 +92,36 @@ static int cx22702_writereg(struct cx227
> >  
> >  	ret = i2c_transfer(state->i2c, &msg, 1);
> >  
> > -	if (ret != 1)
> > +	if (ret != 1) {
> 
> As a suggestion, if you want to optimize success paths, you should use unlikely() for error
> checks. This tells gcc to optimize the code to avoid cache cleanups for the likely condition:
> 
> 	if (unlikely(ret != 1)) {

Good point. Updated patch follows:

* * * * *

* Avoid temporary variables.
* Optimize success paths.
* Make error messages consistently verbose.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Steven Toth <stoth@kernellabs.com>
---
Changes in v2:
* Use unlikely() to help gcc optimize the success paths.

 drivers/media/dvb/frontends/cx22702.c |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

--- linux-2.6.35.orig/drivers/media/dvb/frontends/cx22702.c	2010-09-10 14:22:31.000000000 +0200
+++ linux-2.6.35/drivers/media/dvb/frontends/cx22702.c	2010-09-10 17:39:58.000000000 +0200
@@ -92,33 +92,36 @@ static int cx22702_writereg(struct cx227
 
 	ret = i2c_transfer(state->i2c, &msg, 1);
 
-	if (ret != 1)
+	if (unlikely(ret != 1)) {
 		printk(KERN_ERR
 			"%s: error (reg == 0x%02x, val == 0x%02x, ret == %i)\n",
 			__func__, reg, data, ret);
+		return -1;
+	}
 
-	return (ret != 1) ? -1 : 0;
+	return 0;
 }
 
 static u8 cx22702_readreg(struct cx22702_state *state, u8 reg)
 {
 	int ret;
-	u8 b0[] = { reg };
-	u8 b1[] = { 0 };
+	u8 data;
 
 	struct i2c_msg msg[] = {
 		{ .addr = state->config->demod_address, .flags = 0,
-			.buf = b0, .len = 1 },
+			.buf = &reg, .len = 1 },
 		{ .addr = state->config->demod_address, .flags = I2C_M_RD,
-			.buf = b1, .len = 1 } };
+			.buf = &data, .len = 1 } };
 
 	ret = i2c_transfer(state->i2c, msg, 2);
 
-	if (ret != 2)
-		printk(KERN_ERR "%s: readreg error (ret == %i)\n",
-			__func__, ret);
+	if (unlikely(ret != 2)) {
+		printk(KERN_ERR "%s: error (reg == 0x%02x, ret == %i)\n",
+			__func__, reg, ret);
+		return 0;
+	}
 
-	return b1[0];
+	return data;
 }
 
 static int cx22702_set_inversion(struct cx22702_state *state, int inversion)




-- 
Jean Delvare

[PATCH 2/5] cx22702: Drop useless initializations to 0

[Jean Delvare]

These variables are either unconditionally set right afterward, or
already set to 0 by kzalloc.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Steven Toth <stoth@kernellabs.com>
---
 drivers/media/dvb/frontends/cx22702.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

--- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c	2009-10-17 14:01:34.000000000 +0200
+++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c	2009-10-17 14:23:48.000000000 +0200
@@ -508,7 +508,7 @@ static int cx22702_read_signal_strength(
 {
 	struct cx22702_state *state = fe->demodulator_priv;
 
-	u16 rs_ber = 0;
+	u16 rs_ber;
 	rs_ber = cx22702_readreg(state, 0x23);
 	*signal_strength = (rs_ber << 8) | rs_ber;
 
@@ -519,7 +519,7 @@ static int cx22702_read_snr(struct dvb_f
 {
 	struct cx22702_state *state = fe->demodulator_priv;
 
-	u16 rs_ber = 0;
+	u16 rs_ber;
 	if (cx22702_readreg(state, 0xE4) & 0x02) {
 		/* Realtime statistics */
 		rs_ber = (cx22702_readreg(state, 0xDE) & 0x7F) << 7
@@ -590,7 +590,6 @@ struct dvb_frontend *cx22702_attach(cons
 	/* setup the state */
 	state->config = config;
 	state->i2c = i2c;
-	state->prevUCBlocks = 0;
 
 	/* check if the demod is there */
 	if (cx22702_readreg(state, 0x1f) != 0x3)

-- 
Jean Delvare

[PATCH 3/5] cx22702: Avoid duplicating code in branches

[Jean Delvare]

Calling the same functions in if/else or switch/case branches is
inefficient. Refactor the code for a smaller binary and increased
readability.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Steven Toth <stoth@kernellabs.com>
---
 drivers/media/dvb/frontends/cx22702.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

--- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c	2009-10-24 16:12:18.000000000 +0200
+++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c	2009-10-24 16:27:04.000000000 +0200
@@ -128,19 +128,20 @@ static int cx22702_set_inversion(struct
 {
 	u8 val;
 
+	val = cx22702_readreg(state, 0x0C);
 	switch (inversion) {
 	case INVERSION_AUTO:
 		return -EOPNOTSUPP;
 	case INVERSION_ON:
-		val = cx22702_readreg(state, 0x0C);
-		return cx22702_writereg(state, 0x0C, val | 0x01);
+		val |= 0x01;
+		break;
 	case INVERSION_OFF:
-		val = cx22702_readreg(state, 0x0C);
-		return cx22702_writereg(state, 0x0C, val & 0xfe);
+		val &= 0xfe;
+		break;
 	default:
 		return -EINVAL;
 	}
-
+	return cx22702_writereg(state, 0x0C, val);
 }
 
 /* Retrieve the demod settings */
@@ -247,13 +248,15 @@ static int cx22702_get_tps(struct cx2270
 static int cx22702_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
 {
 	struct cx22702_state *state = fe->demodulator_priv;
+	u8 val;
+
 	dprintk("%s(%d)\n", __func__, enable);
+	val = cx22702_readreg(state, 0x0D);
 	if (enable)
-		return cx22702_writereg(state, 0x0D,
-			cx22702_readreg(state, 0x0D) & 0xfe);
+		val &= 0xfe;
 	else
-		return cx22702_writereg(state, 0x0D,
-			cx22702_readreg(state, 0x0D) | 1);
+		val |= 0x01;
+	return cx22702_writereg(state, 0x0D, val);
 }
 
 /* Talk to the demod, set the FEC, GUARD, QAM settings etc */
@@ -273,23 +276,21 @@ static int cx22702_set_tps(struct dvb_fr
 	cx22702_set_inversion(state, p->inversion);
 
 	/* set bandwidth */
+	val = cx22702_readreg(state, 0x0C) & 0xcf;
 	switch (p->u.ofdm.bandwidth) {
 	case BANDWIDTH_6_MHZ:
-		cx22702_writereg(state, 0x0C,
-			(cx22702_readreg(state, 0x0C) & 0xcf) | 0x20);
+		val |= 0x20;
 		break;
 	case BANDWIDTH_7_MHZ:
-		cx22702_writereg(state, 0x0C,
-			(cx22702_readreg(state, 0x0C) & 0xcf) | 0x10);
+		val |= 0x10;
 		break;
 	case BANDWIDTH_8_MHZ:
-		cx22702_writereg(state, 0x0C,
-			cx22702_readreg(state, 0x0C) & 0xcf);
 		break;
 	default:
 		dprintk("%s: invalid bandwidth\n", __func__);
 		return -EINVAL;
 	}
+	cx22702_writereg(state, 0x0C, val);
 
 	p->u.ofdm.code_rate_LP = FEC_AUTO; /* temp hack as manual not working */
 

-- 
Jean Delvare

[PATCH 4/5] cx22702: Some things never change

[Jean Delvare]

The init sequence never changes so it can be marked const. Likewise,
cx22702_ops is a template and can thus be made read-only.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Steven Toth <stoth@kernellabs.com>
---
 drivers/media/dvb/frontends/cx22702.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c	2009-10-17 14:49:50.000000000 +0200
+++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c	2009-10-18 11:44:09.000000000 +0200
@@ -54,7 +54,7 @@ MODULE_PARM_DESC(debug, "Enable verbose
 #define dprintk	if (debug) printk
 
 /* Register values to initialise the demod */
-static u8 init_tab[] = {
+static const u8 init_tab[] = {
 	0x00, 0x00, /* Stop aquisition */
 	0x0B, 0x06,
 	0x09, 0x01,
@@ -576,7 +576,7 @@ static void cx22702_release(struct dvb_f
 	kfree(state);
 }
 
-static struct dvb_frontend_ops cx22702_ops;
+static const struct dvb_frontend_ops cx22702_ops;
 
 struct dvb_frontend *cx22702_attach(const struct cx22702_config *config,
 	struct i2c_adapter *i2c)
@@ -608,7 +608,7 @@ error:
 }
 EXPORT_SYMBOL(cx22702_attach);
 
-static struct dvb_frontend_ops cx22702_ops = {
+static const struct dvb_frontend_ops cx22702_ops = {
 
 	.info = {
 		.name			= "Conexant CX22702 DVB-T",

-- 
Jean Delvare

[PATCH 5/5] cx22702: Simplify cx22702_set_tps()

[Jean Delvare]

Code in function cx22702_set_tps() can be slightly simplified.
Apparently gcc was smart enough to optimize it anyway, but it can't
hurt to make the code more readable.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Steven Toth <stoth@kernellabs.com>
---
 drivers/media/dvb/frontends/cx22702.c |   58 ++++++++++++++-------------------
 1 file changed, 26 insertions(+), 32 deletions(-)

--- linux-2.6.32-rc5.orig/drivers/media/dvb/frontends/cx22702.c	2009-10-17 14:50:42.000000000 +0200
+++ linux-2.6.32-rc5/drivers/media/dvb/frontends/cx22702.c	2009-10-17 16:12:39.000000000 +0200
@@ -316,33 +316,31 @@ static int cx22702_set_tps(struct dvb_fr
 	}
 
 	/* manually programmed values */
-	val = 0;
-	switch (p->u.ofdm.constellation) {
+	switch (p->u.ofdm.constellation) {		/* mask 0x18 */
 	case QPSK:
-		val = (val & 0xe7);
+		val = 0x00;
 		break;
 	case QAM_16:
-		val = (val & 0xe7) | 0x08;
+		val = 0x08;
 		break;
 	case QAM_64:
-		val = (val & 0xe7) | 0x10;
+		val = 0x10;
 		break;
 	default:
 		dprintk("%s: invalid constellation\n", __func__);
 		return -EINVAL;
 	}
-	switch (p->u.ofdm.hierarchy_information) {
+	switch (p->u.ofdm.hierarchy_information) {	/* mask 0x07 */
 	case HIERARCHY_NONE:
-		val = (val & 0xf8);
 		break;
 	case HIERARCHY_1:
-		val = (val & 0xf8) | 1;
+		val |= 0x01;
 		break;
 	case HIERARCHY_2:
-		val = (val & 0xf8) | 2;
+		val |= 0x02;
 		break;
 	case HIERARCHY_4:
-		val = (val & 0xf8) | 3;
+		val |= 0x03;
 		break;
 	default:
 		dprintk("%s: invalid hierarchy\n", __func__);
@@ -350,44 +348,42 @@ static int cx22702_set_tps(struct dvb_fr
 	}
 	cx22702_writereg(state, 0x06, val);
 
-	val = 0;
-	switch (p->u.ofdm.code_rate_HP) {
+	switch (p->u.ofdm.code_rate_HP) {		/* mask 0x38 */
 	case FEC_NONE:
 	case FEC_1_2:
-		val = (val & 0xc7);
+		val = 0x00;
 		break;
 	case FEC_2_3:
-		val = (val & 0xc7) | 0x08;
+		val = 0x08;
 		break;
 	case FEC_3_4:
-		val = (val & 0xc7) | 0x10;
+		val = 0x10;
 		break;
 	case FEC_5_6:
-		val = (val & 0xc7) | 0x18;
+		val = 0x18;
 		break;
 	case FEC_7_8:
-		val = (val & 0xc7) | 0x20;
+		val = 0x20;
 		break;
 	default:
 		dprintk("%s: invalid code_rate_HP\n", __func__);
 		return -EINVAL;
 	}
-	switch (p->u.ofdm.code_rate_LP) {
+	switch (p->u.ofdm.code_rate_LP) {		/* mask 0x07 */
 	case FEC_NONE:
 	case FEC_1_2:
-		val = (val & 0xf8);
 		break;
 	case FEC_2_3:
-		val = (val & 0xf8) | 1;
+		val |= 0x01;
 		break;
 	case FEC_3_4:
-		val = (val & 0xf8) | 2;
+		val |= 0x02;
 		break;
 	case FEC_5_6:
-		val = (val & 0xf8) | 3;
+		val |= 0x03;
 		break;
 	case FEC_7_8:
-		val = (val & 0xf8) | 4;
+		val |= 0x04;
 		break;
 	default:
 		dprintk("%s: invalid code_rate_LP\n", __func__);
@@ -395,30 +391,28 @@ static int cx22702_set_tps(struct dvb_fr
 	}
 	cx22702_writereg(state, 0x07, val);
 
-	val = 0;
-	switch (p->u.ofdm.guard_interval) {
+	switch (p->u.ofdm.guard_interval) {		/* mask 0x0c */
 	case GUARD_INTERVAL_1_32:
-		val = (val & 0xf3);
+		val = 0x00;
 		break;
 	case GUARD_INTERVAL_1_16:
-		val = (val & 0xf3) | 0x04;
+		val = 0x04;
 		break;
 	case GUARD_INTERVAL_1_8:
-		val = (val & 0xf3) | 0x08;
+		val = 0x08;
 		break;
 	case GUARD_INTERVAL_1_4:
-		val = (val & 0xf3) | 0x0c;
+		val = 0x0c;
 		break;
 	default:
 		dprintk("%s: invalid guard_interval\n", __func__);
 		return -EINVAL;
 	}
-	switch (p->u.ofdm.transmission_mode) {
+	switch (p->u.ofdm.transmission_mode) {		/* mask 0x03 */
 	case TRANSMISSION_MODE_2K:
-		val = (val & 0xfc);
 		break;
 	case TRANSMISSION_MODE_8K:
-		val = (val & 0xfc) | 1;
+		val |= 0x1;
 		break;
 	default:
 		dprintk("%s: invalid transmission_mode\n", __func__);

-- 
Jean Delvare