[PATCH 1/4] Modifications to the driver mb86a20s

[PATCH 1/4] Modifications to the driver mb86a20s

[Manoel PN]

[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]


Hi to all,

I added some modifications to the driver mb86a20s and would appreciate your comments.

>
> File: drivers/media/dvb/frontends/mb86a20s.c
>
> -static int debug = 1;
> +static int debug = 0;
> module_param(debug, int, 0644);
> MODULE_PARM_DESC(debug, "Activates frontend debugging (default:0)");
>
 
How is in the description by default debug is off.

>
> -#define rc(args...)  do {
> +#define printk_rc(args...)  do {
>

For clarity, only rc is somewhat vague.

>
> +static int mb86a20s_i2c_gate_ctrl(struct dvb_frontend *fe, int enable) 
> 

Adds the i2c_gate_ctrl to mb86a20s driver.


The mb86a20s has an i2c bus which controls the flow of data to the tuner. When enabled, the data stream flowing normally through the i2c bus, when disabled the data stream to the tuner is cut and the i2c bus between mb86a20s and the tuner goes to tri-state. The data flow between the mb86a20s and its controller (CPU, USB), is not affected.

In hybrid systems with analog and digital TV, the i2c bus control can be done in the analog demodulator.

>
> -    if (fe->ops.i2c_gate_ctrl)
>-        fe->ops.i2c_gate_ctrl(fe, 0);
>     val = mb86a20s_readreg(state, 0x0a) & 0xf;
> -    if (fe->ops.i2c_gate_ctrl)
> -        fe->ops.i2c_gate_ctrl(fe, 1);
>

The i2c_gate_ctrl controls the i2c bus of the tuner so does not need to enable it or disable it here.


>
> +    for (i = 0; i < 20; i++) {
> +        if (mb86a20s_readreg(state, 0x0a) >= 8) break;
> +        msleep(100);
> +    }
>

Waits for the stabilization of the demodulator.

>
> +static int mb86a20s_get_algo(struct dvb_frontend *fe)
> +{
> +    return DVBFE_ALGO_HW;
> +}
>

Because the mb86a20s_tune function was implemented.

Thanks, best regards,

Manoel.


Signed-off-by: Manoel Pinheiro <pinusdtv@hotmail.com>



 		 	   		  

[-- Attachment #2: i2c_gate_ctrl.patch --]
[-- Type: application/octet-stream, Size: 4350 bytes --]

diff --git a/drivers/media/dvb/frontends/mb86a20s.c b/drivers/media/dvb/frontends/mb86a20s.c
index 0f867a5..fc8c930 100644
--- a/drivers/media/dvb/frontends/mb86a20s.c
+++ b/drivers/media/dvb/frontends/mb86a20s.c
@@ -22,11 +22,11 @@
 #include "dvb_frontend.h"
 #include "mb86a20s.h"
 
-static int debug = 1;
+static int debug = 0;
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Activates frontend debugging (default:0)");
 
-#define rc(args...)  do {						\
+#define printk_rc(args...)  do {						\
 	printk(KERN_ERR  "mb86a20s: " args);				\
 } while (0)
 
@@ -355,7 +355,7 @@ static int mb86a20s_i2c_readreg(struct mb86a20s_state *state,
 	rc = i2c_transfer(state->i2c, msg, 2);
 
 	if (rc != 2) {
-		rc("%s: reg=0x%x (error=%d)\n", __func__, reg, rc);
+		printk_rc("%s: reg=0x%x (error=%d)\n", __func__, reg, rc);
 		return rc;
 	}
 
@@ -370,6 +370,26 @@ static int mb86a20s_i2c_readreg(struct mb86a20s_state *state,
 	mb86a20s_i2c_writeregdata(state, state->config->demod_address, \
 	regdata, ARRAY_SIZE(regdata))
 
+/*
+ * The mb86a20s has an i2c bus which controls the flow of data to the tuner.
+ * When enabled, the data stream flowing normally through the i2c bus, when
+ * disabled the data stream to the tuner is cut and the i2c bus between
+ * mb86a20s and the tuner goes to tri-state.
+ * The data flow between the mb86a20s and its controller (CPU, USB), is not affected.
+ * In hybrid systems with analog and digital TV, the i2c bus control can be done
+ * in the analog demodulator.
+ */
+static int mb86a20s_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
+{
+	struct mb86a20s_state *state = (struct mb86a20s_state *)fe->demodulator_priv;
+
+	/* Enable/Disable I2C bus for tuner control */
+	if (enable)
+		return mb86a20s_writereg(state, 0xfe, 0);
+	else
+		return mb86a20s_writereg(state, 0xfe, 1);
+}
+
 static int mb86a20s_initfe(struct dvb_frontend *fe)
 {
 	struct mb86a20s_state *state = fe->demodulator_priv;
@@ -459,11 +479,7 @@ static int mb86a20s_read_status(struct dvb_frontend *fe, fe_status_t *status)
 	dprintk("\n");
 	*status = 0;
 
-	if (fe->ops.i2c_gate_ctrl)
-		fe->ops.i2c_gate_ctrl(fe, 0);
 	val = mb86a20s_readreg(state, 0x0a) & 0xf;
-	if (fe->ops.i2c_gate_ctrl)
-		fe->ops.i2c_gate_ctrl(fe, 1);
 
 	if (val >= 2)
 		*status |= FE_HAS_SIGNAL;
@@ -489,14 +505,18 @@ static int mb86a20s_set_frontend(struct dvb_frontend *fe,
 	struct dvb_frontend_parameters *p)
 {
 	struct mb86a20s_state *state = fe->demodulator_priv;
-	int rc;
+	int i, rc;
 
 	dprintk("\n");
 
-	if (fe->ops.i2c_gate_ctrl)
-		fe->ops.i2c_gate_ctrl(fe, 1);
-	dprintk("Calling tuner set parameters\n");
-	fe->ops.tuner_ops.set_params(fe, p);
+	if (fe->ops.tuner_ops.set_params) {
+		dprintk("Calling tuner set parameters\n");
+		fe->ops.tuner_ops.set_params(fe, p);
+		/* disable I2C bus tuner control */
+		if (fe->ops.i2c_gate_ctrl)
+			fe->ops.i2c_gate_ctrl(fe, 0);
+		msleep(100);
+	}
 
 	/*
 	 * Make it more reliable: if, for some reason, the initial
@@ -511,13 +531,16 @@ static int mb86a20s_set_frontend(struct dvb_frontend *fe,
 	if (state->need_init)
 		mb86a20s_initfe(fe);
 
-	if (fe->ops.i2c_gate_ctrl)
-		fe->ops.i2c_gate_ctrl(fe, 0);
 	rc = mb86a20s_writeregdata(state, mb86a20s_reset_reception);
-	if (fe->ops.i2c_gate_ctrl)
-		fe->ops.i2c_gate_ctrl(fe, 1);
+	if (rc < 0)
+		return rc;
 
-	return rc;
+	for (i = 0; i < 20; i++) {
+		if (mb86a20s_readreg(state, 0x0a) >= 8) break;
+		msleep(100);
+	}
+
+	return 0;
 }
 
 static int mb86a20s_get_frontend(struct dvb_frontend *fe,
@@ -553,6 +576,11 @@ static int mb86a20s_tune(struct dvb_frontend *fe,
 	return rc;
 }
 
+static int mb86a20s_get_algo(struct dvb_frontend *fe)
+{
+	return DVBFE_ALGO_HW;
+}
+
 static void mb86a20s_release(struct dvb_frontend *fe)
 {
 	struct mb86a20s_state *state = fe->demodulator_priv;
@@ -575,7 +603,7 @@ struct dvb_frontend *mb86a20s_attach(const struct mb86a20s_config *config,
 
 	dprintk("\n");
 	if (state == NULL) {
-		rc("Unable to kzalloc\n");
+		printk_rc("Unable to kzalloc\n");
 		goto error;
 	}
 
@@ -631,6 +659,8 @@ static struct dvb_frontend_ops mb86a20s_ops = {
 	.get_frontend = mb86a20s_get_frontend,
 	.read_status = mb86a20s_read_status,
 	.read_signal_strength = mb86a20s_read_signal_strength,
+	.i2c_gate_ctrl = mb86a20s_i2c_gate_ctrl,
+	.get_frontend_algo = mb86a20s_get_algo,
 	.tune = mb86a20s_tune,
 };
 

Re: [PATCH 1/4] Modifications to the driver mb86a20s

[Mauro Carvalho Chehab]

Em 13-05-2011 04:02, Manoel PN escreveu:
> 
> Hi to all,
> 
> I added some modifications to the driver mb86a20s and would appreciate your comments.
> 
>>
>> File: drivers/media/dvb/frontends/mb86a20s.c
>>
>> -static int debug = 1;
>> +static int debug = 0;
>> module_param(debug, int, 0644);
>> MODULE_PARM_DESC(debug, "Activates frontend debugging (default:0)");
>>
>  
> How is in the description by default debug is off.

Breaking it into smaller patches it the proper way to get it upstreamed. However, 
each logical change should be sent into its your own patch.

For example, in this case, you should be sending a one-line patch just fixing the
debug parameter, with a description like:

[PATCH 1/xx] Disable debug by default

The mb86a20s is producing debug information by default. This is not
the expected behavior. Also, the parameter description tells that the
default should be 0. So, fix it.

Signed-off-by: ...

> 
>>
>> -#define rc(args...)  do {
>> +#define printk_rc(args...)  do {
>>
> 
> For clarity, only rc is somewhat vague.
> 

The same as above applies here.

The first two changes are OK for me, and I would have applied them if you
were sending one logic change by patch.

>>
>> +static int mb86a20s_i2c_gate_ctrl(struct dvb_frontend *fe, int enable) 
>>
> 
> Adds the i2c_gate_ctrl to mb86a20s driver.
> 
> 
> The mb86a20s has an i2c bus which controls the flow of data to the tuner. When enabled, the data stream flowing normally through the i2c bus, when disabled the data stream to the tuner is cut and the i2c bus between mb86a20s and the tuner goes to tri-state. The data flow between the mb86a20s and its controller (CPU, USB), is not affected.
> 
> In hybrid systems with analog and digital TV, the i2c bus control can be done in the analog demodulator.
> 
>>
>> -    if (fe->ops.i2c_gate_ctrl)
>> -        fe->ops.i2c_gate_ctrl(fe, 0);
>>     val = mb86a20s_readreg(state, 0x0a) & 0xf;
>> -    if (fe->ops.i2c_gate_ctrl)
>> -        fe->ops.i2c_gate_ctrl(fe, 1);
>>
> 
> The i2c_gate_ctrl controls the i2c bus of the tuner so does not need to enable it or disable it here.
>

Those two changes are moving the responsibility of handling the i2c gate
control to happen outside the demod. If applied as-is, it will break the
boards already supported, causing a regression. One of the rules for a patch
to be applied is that no regressions are accepted. So, if you need to change
it, for whatever reason, you should take a look at the existing cases and
fix the logic (or try to) at the existing drivers.
 
> 
>>
>> +    for (i = 0; i < 20; i++) {
>> +        if (mb86a20s_readreg(state, 0x0a) >= 8) break;
>> +        msleep(100);
>> +    }
>>
> 
> Waits for the stabilization of the demodulator.

Ok, makes sense to me.

Again, this change should be separate. One of the reasons why we need
separate logical changes is that a patch from the series may cause some
regressions. By having one logical change per patch, fixing the regression
is as simple as reverting one git patch. However, if you mix two different
things at the same patch, reverting it will be very painful.
> 
>>
>> +static int mb86a20s_get_algo(struct dvb_frontend *fe)
>> +{
>> +    return DVBFE_ALGO_HW;
>> +}
>>
> 
> Because the mb86a20s_tune function was implemented.

Ok. Same as above: please break mb86a20s_tune() implementation into
a separate patch.

> 
> Thanks, best regards,
> 
> Manoel.
> 
> 
> Signed-off-by: Manoel Pinheiro <pinusdtv@hotmail.com>
> 
> 
> 
>  		 	   		  =