support for DVBSky dvb-s2 usb: add some config and set_voltage for m88ds3103

PATCH In review nibble.max 2 participants 5 messages Aug 6, 2014 – Aug 7, 2014

Subsystems (2)

Media (V4L/DVB) M88DS3103 media driver

lore

Share & tools

lore

nibble.max

author

2014-08-06 04:27 UTC #0 lore raw

PATCH 1/4 support for DVBSky dvb-s2 usb: add some config and set_voltage for m88ds3103

Add some config parameters and set_voltage function for m88ds3103.

Signed-off-by Nibble Max <[email protected]>

---
 drivers/media/dvb-frontends/m88ds3103.c | 91 +++++++++++++++++++++++----------
 drivers/media/dvb-frontends/m88ds3103.h | 37 ++++++++++++--
 2 files changed, 96 insertions(+), 32 deletions(-)

drivers/media/dvb-frontends/m88ds3103.c +64 ~~-27~~

index dfe0c2f..df2f89c 100644
@@ -247,8 +247,9 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 	u8 u8tmp, u8tmp1, u8tmp2;
 	u8 buf[2];
 	u16 u16tmp, divide_ratio;
-	u32 tuner_frequency, target_mclk, ts_clk;
+	u32 tuner_frequency, target_mclk;
 	s32 s32tmp;
+	fe_status_t status;
 	dev_dbg(&priv->i2c->dev,
 			"%s: delivery_system=%d modulation=%d frequency=%d symbol_rate=%d inversion=%d pilot=%d rolloff=%d\n",
 			__func__, c->delivery_system,

@@ -316,9 +317,6 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 				target_mclk = 144000;
 			break;
 		case M88DS3103_TS_PARALLEL:
-		case M88DS3103_TS_PARALLEL_12:
-		case M88DS3103_TS_PARALLEL_16:
-		case M88DS3103_TS_PARALLEL_19_2:
 		case M88DS3103_TS_CI:
 			if (c->symbol_rate < 18000000)
 				target_mclk = 96000;

@@ -352,33 +350,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 	switch (priv->cfg->ts_mode) {
 	case M88DS3103_TS_SERIAL:
 		u8tmp1 = 0x00;
-		ts_clk = 0;
-		u8tmp = 0x46;
+		u8tmp = 0x06;
 		break;
 	case M88DS3103_TS_SERIAL_D7:
 		u8tmp1 = 0x20;
-		ts_clk = 0;
-		u8tmp = 0x46;
+		u8tmp = 0x06;
 		break;
 	case M88DS3103_TS_PARALLEL:
-		ts_clk = 24000;
-		u8tmp = 0x42;
-		break;
-	case M88DS3103_TS_PARALLEL_12:
-		ts_clk = 12000;
-		u8tmp = 0x42;
-		break;
-	case M88DS3103_TS_PARALLEL_16:
-		ts_clk = 16000;
-		u8tmp = 0x42;
-		break;
-	case M88DS3103_TS_PARALLEL_19_2:
-		ts_clk = 19200;
-		u8tmp = 0x42;
+		u8tmp = 0x02;
 		break;
 	case M88DS3103_TS_CI:
-		ts_clk = 6000;
-		u8tmp = 0x43;
+		u8tmp = 0x03;
 		break;
 	default:
 		dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n", __func__);

@@ -386,6 +368,9 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 		goto err;
 	}
 
+	if (priv->cfg->ts_clk_pol)
+		u8tmp |= 0x40;
+
 	/* TS mode */
 	ret = m88ds3103_wr_reg(priv, 0xfd, u8tmp);
 	if (ret)

@@ -399,8 +384,8 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 			goto err;
 	}
 
-	if (ts_clk) {
-		divide_ratio = DIV_ROUND_UP(target_mclk, ts_clk);
+	if (priv->cfg->ts_clk) {
+		divide_ratio = DIV_ROUND_UP(target_mclk, priv->cfg->ts_clk);
 		u8tmp1 = divide_ratio / 2;
 		u8tmp2 = DIV_ROUND_UP(divide_ratio, 2);
 	} else {

@@ -411,7 +396,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
 	dev_dbg(&priv->i2c->dev,
 			"%s: target_mclk=%d ts_clk=%d divide_ratio=%d\n",
-			__func__, target_mclk, ts_clk, divide_ratio);
+			__func__, target_mclk, priv->cfg->ts_clk, divide_ratio);
 
 	u8tmp1--;
 	u8tmp2--;

@@ -523,6 +508,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
 	priv->delivery_system = c->delivery_system;
 
+	if (priv->cfg->start_ctrl) {
+		for (len = 0; len < 30 ; len++) {
+			m88ds3103_read_status(fe, &status);
+			if (status & FE_HAS_LOCK) {
+				priv->cfg->start_ctrl(fe);
+				break;
+			}
+			msleep(20);
+		}
+	}
+
 	return 0;
 err:
 	dev_dbg(&priv->i2c->dev, "%s: failed=%d\n", __func__, ret);

@@ -1053,6 +1049,46 @@ err:
 	return ret;
 }
 
+static int m88ds3103_set_voltage(struct dvb_frontend *fe,
+	fe_sec_voltage_t voltage)
+{
+	struct m88ds3103_priv *priv = fe->demodulator_priv;
+	u8 data;
+
+	dev_dbg(&priv->i2c->dev, "%s: pin_ctrl = (%02x)\n",
+			__func__, priv->cfg->pin_ctrl);
+
+	if (priv->cfg->set_voltage)
+		priv->cfg->set_voltage(fe, voltage);
+
+	m88ds3103_rd_reg(priv, 0xa2, &data);
+
+	if (priv->cfg->pin_ctrl & 0x80) { /*If control pin is assigned.*/
+		data &= ~0x03; /* bit0 V/H, bit1 off/on */
+		if (priv->cfg->pin_ctrl & 0x02)
+			data |= 0x02;
+
+		switch (voltage) {
+		case SEC_VOLTAGE_18:
+		     if ((priv->cfg->pin_ctrl & 0x01) == 0)
+			data |= 0x01;
+		     break;
+		case SEC_VOLTAGE_13:
+		     if (priv->cfg->pin_ctrl & 0x01)
+			data |= 0x01;
+		     break;
+		case SEC_VOLTAGE_OFF:
+		     if (priv->cfg->pin_ctrl & 0x02)
+			data &= ~0x02;
+		     else
+			data |= 0x02;
+		     break;
+		}
+	}
+	m88ds3103_wr_reg(priv, 0xa2, data);
+	return 0;
+}
+
 static int m88ds3103_diseqc_send_master_cmd(struct dvb_frontend *fe,
 		struct dvb_diseqc_master_cmd *diseqc_cmd)
 {

@@ -1370,6 +1406,7 @@ static struct dvb_frontend_ops m88ds3103_ops = {
 	.diseqc_send_burst = m88ds3103_diseqc_send_burst,
 
 	.set_tone = m88ds3103_set_tone,
+	.set_voltage = m88ds3103_set_voltage,
 };
 
 MODULE_AUTHOR("Antti Palosaari <[email protected]>");

drivers/media/dvb-frontends/m88ds3103.h +32 -5

index bbb7e3a..db82d89 100644
@@ -47,14 +47,23 @@ struct m88ds3103_config {
 	 */
 #define M88DS3103_TS_SERIAL             0 /* TS output pin D0, normal */
 #define M88DS3103_TS_SERIAL_D7          1 /* TS output pin D7 */
-#define M88DS3103_TS_PARALLEL           2 /* 24 MHz, normal */
-#define M88DS3103_TS_PARALLEL_12        3 /* 12 MHz */
-#define M88DS3103_TS_PARALLEL_16        4 /* 16 MHz */
-#define M88DS3103_TS_PARALLEL_19_2      5 /* 19.2 MHz */
-#define M88DS3103_TS_CI                 6 /* 6 MHz */
+#define M88DS3103_TS_PARALLEL           2 /* TS Parallel mode */
+#define M88DS3103_TS_CI                 3 /* TS CI Mode */
 	u8 ts_mode;
 
 	/*
+	 * TS clk in KHz
+	 * Default: 0.
+	 */
+	u32 ts_clk;
+
+	/*
+	 * TS clk polarity.
+	 * Default: 0. 1-active at falling edge; 0-active at rising edge.
+	 */
+	u8 ts_clk_pol:1;
+
+	/*
 	 * spectrum inversion
 	 * Default: 0
 	 */

@@ -86,6 +95,24 @@ struct m88ds3103_config {
 	 * Default: none, must set
 	 */
 	u8 agc;
+
+	/*
+	 * LNB pin control
+	 *
+	 */
+	u8 pin_ctrl;
+
+	/*
+	 * LNB voltage function
+	 *
+	 */
+	int (*set_voltage)(struct dvb_frontend *fe, fe_sec_voltage_t voltage);
+
+	/*
+	 * start stream function
+	 *
+	 */
+	int (*start_ctrl)(struct dvb_frontend *fe);
 };
 
 /*

Antti Palosaari

2014-08-07 08:18 UTC #1 lore raw

Moikka!

That patch contains (too) many changes:
1) TS clock config option
2) TS clock polarity config option
3) start_ctrl() callback
4) set_voltage implementation
5) set_voltage() callback

Generally I am fine with 1, 2 and 4.

When you do that many different logical changes for existing driver, you should split it is one patch per change.

Rest of the comments are between the code.

Quoted 106 lines · nibble.max

Add some config parameters and set_voltage function for m88ds3103.

Signed-off-by Nibble Max <[email protected]>

---
  drivers/media/dvb-frontends/m88ds3103.c | 91 +++++++++++++++++++++++----------
  drivers/media/dvb-frontends/m88ds3103.h | 37 ++++++++++++--
  2 files changed, 96 insertions(+), 32 deletions(-)

drivers/media/dvb-frontends/m88ds3103.c +13 ~~-27~~

index dfe0c2f..df2f89c 100644
@@ -247,8 +247,9 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	u8 u8tmp, u8tmp1, u8tmp2;
  	u8 buf[2];
  	u16 u16tmp, divide_ratio;
-	u32 tuner_frequency, target_mclk, ts_clk;
+	u32 tuner_frequency, target_mclk;
  	s32 s32tmp;
+	fe_status_t status;
  	dev_dbg(&priv->i2c->dev,
  			"%s: delivery_system=%d modulation=%d frequency=%d symbol_rate=%d inversion=%d pilot=%d rolloff=%d\n",
  			__func__, c->delivery_system,

@@ -316,9 +317,6 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  				target_mclk = 144000;
  			break;
  		case M88DS3103_TS_PARALLEL:
-		case M88DS3103_TS_PARALLEL_12:
-		case M88DS3103_TS_PARALLEL_16:
-		case M88DS3103_TS_PARALLEL_19_2:
  		case M88DS3103_TS_CI:
  			if (c->symbol_rate < 18000000)
  				target_mclk = 96000;

@@ -352,33 +350,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	switch (priv->cfg->ts_mode) {
  	case M88DS3103_TS_SERIAL:
  		u8tmp1 = 0x00;
-		ts_clk = 0;
-		u8tmp = 0x46;
+		u8tmp = 0x06;
  		break;
  	case M88DS3103_TS_SERIAL_D7:
  		u8tmp1 = 0x20;
-		ts_clk = 0;
-		u8tmp = 0x46;
+		u8tmp = 0x06;
  		break;
  	case M88DS3103_TS_PARALLEL:
-		ts_clk = 24000;
-		u8tmp = 0x42;
-		break;
-	case M88DS3103_TS_PARALLEL_12:
-		ts_clk = 12000;
-		u8tmp = 0x42;
-		break;
-	case M88DS3103_TS_PARALLEL_16:
-		ts_clk = 16000;
-		u8tmp = 0x42;
-		break;
-	case M88DS3103_TS_PARALLEL_19_2:
-		ts_clk = 19200;
-		u8tmp = 0x42;
+		u8tmp = 0x02;
  		break;
  	case M88DS3103_TS_CI:
-		ts_clk = 6000;
-		u8tmp = 0x43;
+		u8tmp = 0x03;
  		break;
  	default:
  		dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n", __func__);

@@ -386,6 +368,9 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  		goto err;
  	}
 
+	if (priv->cfg->ts_clk_pol)
+		u8tmp |= 0x40;
+
  	/* TS mode */
  	ret = m88ds3103_wr_reg(priv, 0xfd, u8tmp);
  	if (ret)

@@ -399,8 +384,8 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  			goto err;
  	}
 
-	if (ts_clk) {
-		divide_ratio = DIV_ROUND_UP(target_mclk, ts_clk);
+	if (priv->cfg->ts_clk) {
+		divide_ratio = DIV_ROUND_UP(target_mclk, priv->cfg->ts_clk);
  		u8tmp1 = divide_ratio / 2;
  		u8tmp2 = DIV_ROUND_UP(divide_ratio, 2);
  	} else {

@@ -411,7 +396,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
  	dev_dbg(&priv->i2c->dev,
  			"%s: target_mclk=%d ts_clk=%d divide_ratio=%d\n",
-			__func__, target_mclk, ts_clk, divide_ratio);
+			__func__, target_mclk, priv->cfg->ts_clk, divide_ratio);
 
  	u8tmp1--;
  	u8tmp2--;

@@ -523,6 +508,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
  	priv->delivery_system = c->delivery_system;
 
+	if (priv->cfg->start_ctrl) {

drivers/media/dvb-frontends/m88ds3103.c static int m88ds3103_set_frontend(struct dvb_frontend *fe) · line 521

Quoted 10 lines

drivers/media/dvb-frontends/m88ds3103.c +10 -0

+		for (len = 0; len < 30 ; len++) {
+			m88ds3103_read_status(fe, &status);
+			if (status & FE_HAS_LOCK) {
+				priv->cfg->start_ctrl(fe);
+				break;
+			}
+			msleep(20);
+		}
+	}
+

What is idea of that start_ctrl logic? It looks ugly. Why it is needed? What is wrong with default DVB-core implementation? You should not need to poll demod lock here and then call streaming control callback from USB driver. If you implement .streaming_ctrl() callback to DVB USB driver, it is called automatically for you.

Quoted 8 lines

drivers/media/dvb-frontends/m88ds3103.c +1 -0

  	return 0;
  err:
  	dev_dbg(&priv->i2c->dev, "%s: failed=%d\n", __func__, ret);

@@ -1053,6 +1049,46 @@ err:
  	return ret;
  }
 
+static int m88ds3103_set_voltage(struct dvb_frontend *fe,

drivers/media/dvb-frontends/m88ds3103.c err: · line 1062

Quoted 10 lines

drivers/media/dvb-frontends/m88ds3103.c +10 -0

+	fe_sec_voltage_t voltage)
+{
+	struct m88ds3103_priv *priv = fe->demodulator_priv;
+	u8 data;
+
+	dev_dbg(&priv->i2c->dev, "%s: pin_ctrl = (%02x)\n",
+			__func__, priv->cfg->pin_ctrl);
+
+	if (priv->cfg->set_voltage)
+		priv->cfg->set_voltage(fe, voltage);

Own callback. Why?
If you really need alter some standard demod callback execution path, you should do it overriding callback inside bus interface driver (USB driver in that case) and not taint demod driver with custom callbacks like that. Also, if you really need some weird stuff (something which is not normal looking) you need to add comments why it is needed. See af9015.c driver as a example how to override demod callbacks.

Quoted 19 lines

drivers/media/dvb-frontends/m88ds3103.c +19 -0

+
+	m88ds3103_rd_reg(priv, 0xa2, &data);
+
+	if (priv->cfg->pin_ctrl & 0x80) { /*If control pin is assigned.*/
+		data &= ~0x03; /* bit0 V/H, bit1 off/on */
+		if (priv->cfg->pin_ctrl & 0x02)
+			data |= 0x02;
+
+		switch (voltage) {
+		case SEC_VOLTAGE_18:
+		     if ((priv->cfg->pin_ctrl & 0x01) == 0)
+			data |= 0x01;
+		     break;
+		case SEC_VOLTAGE_13:
+		     if (priv->cfg->pin_ctrl & 0x01)
+			data |= 0x01;
+		     break;
+		case SEC_VOLTAGE_OFF:
+		     if (priv->cfg->pin_ctrl & 0x02)

drivers/media/dvb-frontends/m88ds3103.c err: · line 1091

Quoted 10 lines

drivers/media/dvb-frontends/m88ds3103.c +10 -0

+			data &= ~0x02;
+		     else
+			data |= 0x02;
+		     break;
+		}
+	}
+	m88ds3103_wr_reg(priv, 0xa2, data);
+	return 0;
+}
+

Otherwise that m88ds3103_set_voltage looks fine, but custom callback (priv->cfg->set_voltage) should go away as there is no need for it.

Quoted 59 lines

drivers/media/dvb-frontends/m88ds3103.c +1 -0

  static int m88ds3103_diseqc_send_master_cmd(struct dvb_frontend *fe,
  		struct dvb_diseqc_master_cmd *diseqc_cmd)
  {

@@ -1370,6 +1406,7 @@ static struct dvb_frontend_ops m88ds3103_ops = {
  	.diseqc_send_burst = m88ds3103_diseqc_send_burst,
 
  	.set_tone = m88ds3103_set_tone,
+	.set_voltage = m88ds3103_set_voltage,
  };
 
  MODULE_AUTHOR("Antti Palosaari <[email protected]>");

drivers/media/dvb-frontends/m88ds3103.h +25 -5

index bbb7e3a..db82d89 100644
@@ -47,14 +47,23 @@ struct m88ds3103_config {
  	 */
  #define M88DS3103_TS_SERIAL             0 /* TS output pin D0, normal */
  #define M88DS3103_TS_SERIAL_D7          1 /* TS output pin D7 */
-#define M88DS3103_TS_PARALLEL           2 /* 24 MHz, normal */
-#define M88DS3103_TS_PARALLEL_12        3 /* 12 MHz */
-#define M88DS3103_TS_PARALLEL_16        4 /* 16 MHz */
-#define M88DS3103_TS_PARALLEL_19_2      5 /* 19.2 MHz */
-#define M88DS3103_TS_CI                 6 /* 6 MHz */
+#define M88DS3103_TS_PARALLEL           2 /* TS Parallel mode */
+#define M88DS3103_TS_CI                 3 /* TS CI Mode */
  	u8 ts_mode;
 
  	/*
+	 * TS clk in KHz
+	 * Default: 0.
+	 */
+	u32 ts_clk;
+
+	/*
+	 * TS clk polarity.
+	 * Default: 0. 1-active at falling edge; 0-active at rising edge.
+	 */
+	u8 ts_clk_pol:1;
+
+	/*
  	 * spectrum inversion
  	 * Default: 0
  	 */

@@ -86,6 +95,24 @@ struct m88ds3103_config {
  	 * Default: none, must set
  	 */
  	u8 agc;
+
+	/*
+	 * LNB pin control
+	 *
+	 */
+	u8 pin_ctrl;
+
+	/*
+	 * LNB voltage function
+	 *
+	 */

drivers/media/dvb-frontends/m88ds3103.h struct m88ds3103_config { · line 118

Quoted 10 lines

drivers/media/dvb-frontends/m88ds3103.h +7 -0

+	int (*set_voltage)(struct dvb_frontend *fe, fe_sec_voltage_t voltage);
+
+	/*
+	 * start stream function
+	 *
+	 */
+	int (*start_ctrl)(struct dvb_frontend *fe);
  };
 
  /*

regards
Antti

Signature · 1 line

http://palosaari.fi/

nibble.max

author

2014-08-07 09:31 UTC #2 lore raw

Re: Re: [PATCH 1/4] support for DVBSky dvb-s2 usb: add some config andset_voltage for m88ds3103

In reply to nibble.max

Moikka!
Thanks for your review.

Quoted 132 lines

Moikka!

That patch contains (too) many changes:
1) TS clock config option
2) TS clock polarity config option
3) start_ctrl() callback
4) set_voltage implementation
5) set_voltage() callback

Generally I am fine with 1, 2 and 4.

When you do that many different logical changes for existing driver, you should split it is one patch per change.

Rest of the comments are between the code.

Quoted 114 lines · nibble.max

Add some config parameters and set_voltage function for m88ds3103.

Signed-off-by Nibble Max <[email protected]>

---
  drivers/media/dvb-frontends/m88ds3103.c | 91 +++++++++++++++++++++++----------
  drivers/media/dvb-frontends/m88ds3103.h | 37 ++++++++++++--
  2 files changed, 96 insertions(+), 32 deletions(-)

drivers/media/dvb-frontends/m88ds3103.c +21 ~~-27~~

index dfe0c2f..df2f89c 100644
@@ -247,8 +247,9 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	u8 u8tmp, u8tmp1, u8tmp2;
  	u8 buf[2];
  	u16 u16tmp, divide_ratio;
-	u32 tuner_frequency, target_mclk, ts_clk;
+	u32 tuner_frequency, target_mclk;
  	s32 s32tmp;
+	fe_status_t status;
  	dev_dbg(&priv->i2c->dev,
  			"%s: delivery_system=%d modulation=%d frequency=%d symbol_rate=%d inversion=%d pilot=%d rolloff=%d\n",
  			__func__, c->delivery_system,

@@ -316,9 +317,6 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  				target_mclk = 144000;
  			break;
  		case M88DS3103_TS_PARALLEL:
-		case M88DS3103_TS_PARALLEL_12:
-		case M88DS3103_TS_PARALLEL_16:
-		case M88DS3103_TS_PARALLEL_19_2:
  		case M88DS3103_TS_CI:
  			if (c->symbol_rate < 18000000)
  				target_mclk = 96000;

@@ -352,33 +350,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	switch (priv->cfg->ts_mode) {
  	case M88DS3103_TS_SERIAL:
  		u8tmp1 = 0x00;
-		ts_clk = 0;
-		u8tmp = 0x46;
+		u8tmp = 0x06;
  		break;
  	case M88DS3103_TS_SERIAL_D7:
  		u8tmp1 = 0x20;
-		ts_clk = 0;
-		u8tmp = 0x46;
+		u8tmp = 0x06;
  		break;
  	case M88DS3103_TS_PARALLEL:
-		ts_clk = 24000;
-		u8tmp = 0x42;
-		break;
-	case M88DS3103_TS_PARALLEL_12:
-		ts_clk = 12000;
-		u8tmp = 0x42;
-		break;
-	case M88DS3103_TS_PARALLEL_16:
-		ts_clk = 16000;
-		u8tmp = 0x42;
-		break;
-	case M88DS3103_TS_PARALLEL_19_2:
-		ts_clk = 19200;
-		u8tmp = 0x42;
+		u8tmp = 0x02;
  		break;
  	case M88DS3103_TS_CI:
-		ts_clk = 6000;
-		u8tmp = 0x43;
+		u8tmp = 0x03;
  		break;
  	default:
  		dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n", __func__);

@@ -386,6 +368,9 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  		goto err;
  	}
 
+	if (priv->cfg->ts_clk_pol)
+		u8tmp |= 0x40;
+
  	/* TS mode */
  	ret = m88ds3103_wr_reg(priv, 0xfd, u8tmp);
  	if (ret)

@@ -399,8 +384,8 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  			goto err;
  	}
 
-	if (ts_clk) {
-		divide_ratio = DIV_ROUND_UP(target_mclk, ts_clk);
+	if (priv->cfg->ts_clk) {
+		divide_ratio = DIV_ROUND_UP(target_mclk, priv->cfg->ts_clk);
  		u8tmp1 = divide_ratio / 2;
  		u8tmp2 = DIV_ROUND_UP(divide_ratio, 2);
  	} else {

@@ -411,7 +396,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
  	dev_dbg(&priv->i2c->dev,
  			"%s: target_mclk=%d ts_clk=%d divide_ratio=%d\n",
-			__func__, target_mclk, ts_clk, divide_ratio);
+			__func__, target_mclk, priv->cfg->ts_clk, divide_ratio);
 
  	u8tmp1--;
  	u8tmp2--;

@@ -523,6 +508,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
  	priv->delivery_system = c->delivery_system;
 
+	if (priv->cfg->start_ctrl) {
+		for (len = 0; len < 30 ; len++) {
+			m88ds3103_read_status(fe, &status);
+			if (status & FE_HAS_LOCK) {
+				priv->cfg->start_ctrl(fe);
+				break;
+			}
+			msleep(20);
+		}

Quoted 8 lines

drivers/media/dvb-frontends/m88ds3103.c static int m88ds3103_set_frontend(struct dvb_frontend *fe) · line 521
Quoted 2 lines
drivers/media/dvb-frontends/m88ds3103.c +2 -0
+	}
+
What is idea of that start_ctrl logic? It looks ugly. Why it is needed? What is wrong with default DVB-core implementation? You should not need to poll demod lock here and then call streaming control callback from USB driver. If you implement .streaming_ctrl() callback to DVB USB driver, it is called automatically for you.

It is nothing with streaming_ctrl of DVB USB driver.
It relates with the hardware chip problem.
The m88ds3103 will not output ts clock when it powers up at the first time. It starts to output ts clock as soon as it locks the signal.
But the slave fifo of Cypress usb chip really need this clock to work. If there is no this clock, the salve fifo will stop.
start_ctrl logic is used to restart the salve fifo when the ts clock is valid.

Quoted 21 lines

Quoted 10 lines

drivers/media/dvb-frontends/m88ds3103.c +1 -0

  	return 0;
  err:
  	dev_dbg(&priv->i2c->dev, "%s: failed=%d\n", __func__, ret);

@@ -1053,6 +1049,46 @@ err:
  	return ret;
  }
 
+static int m88ds3103_set_voltage(struct dvb_frontend *fe,

drivers/media/dvb-frontends/m88ds3103.c err: · line 1062

Quoted 10 lines

drivers/media/dvb-frontends/m88ds3103.c +10 -0

+	fe_sec_voltage_t voltage)
+{
+	struct m88ds3103_priv *priv = fe->demodulator_priv;
+	u8 data;
+
+	dev_dbg(&priv->i2c->dev, "%s: pin_ctrl = (%02x)\n",
+			__func__, priv->cfg->pin_ctrl);
+
+	if (priv->cfg->set_voltage)
+		priv->cfg->set_voltage(fe, voltage);

Quoted 7 lines

Own callback. Why?
If you really need alter some standard demod callback execution path, you should do it overriding callback inside bus interface driver (USB driver in that case) and not taint demod driver with custom callbacks like that. Also, if you really need some weird stuff (something which is not normal looking) you need to add comments why it is needed. See af9015.c driver as a example how to override demod callbacks.

In S960 the LNB voltage is not controlled by the usb chip or m88ds3103 totally.
LNB off/on is controlled by gpio of the usb chip, but the H and V voltage is controlled by m88ds3103. That need callback. Yes, it can implement into the usb driver too.

Quoted 109 lines

Quoted 20 lines

drivers/media/dvb-frontends/m88ds3103.c +19 -0

+
+	m88ds3103_rd_reg(priv, 0xa2, &data);
+
+	if (priv->cfg->pin_ctrl & 0x80) { /*If control pin is assigned.*/
+		data &= ~0x03; /* bit0 V/H, bit1 off/on */
+		if (priv->cfg->pin_ctrl & 0x02)
+			data |= 0x02;
+
+		switch (voltage) {
+		case SEC_VOLTAGE_18:
+		     if ((priv->cfg->pin_ctrl & 0x01) == 0)
+			data |= 0x01;
+		     break;
+		case SEC_VOLTAGE_13:
+		     if (priv->cfg->pin_ctrl & 0x01)
+			data |= 0x01;
+		     break;
+		case SEC_VOLTAGE_OFF:
+		     if (priv->cfg->pin_ctrl & 0x02)

drivers/media/dvb-frontends/m88ds3103.c err: · line 1091

Quoted 10 lines

drivers/media/dvb-frontends/m88ds3103.c +10 -0

+			data &= ~0x02;
+		     else
+			data |= 0x02;
+		     break;
+		}
+	}
+	m88ds3103_wr_reg(priv, 0xa2, data);
+	return 0;
+}
+

Otherwise that m88ds3103_set_voltage looks fine, but custom callback (priv->cfg->set_voltage) should go away as there is no need for it.

Quoted 59 lines

drivers/media/dvb-frontends/m88ds3103.c +1 -0

  static int m88ds3103_diseqc_send_master_cmd(struct dvb_frontend *fe,
  		struct dvb_diseqc_master_cmd *diseqc_cmd)
  {

@@ -1370,6 +1406,7 @@ static struct dvb_frontend_ops m88ds3103_ops = {
  	.diseqc_send_burst = m88ds3103_diseqc_send_burst,
 
  	.set_tone = m88ds3103_set_tone,
+	.set_voltage = m88ds3103_set_voltage,
  };
 
  MODULE_AUTHOR("Antti Palosaari <[email protected]>");

drivers/media/dvb-frontends/m88ds3103.h +25 -5

index bbb7e3a..db82d89 100644
@@ -47,14 +47,23 @@ struct m88ds3103_config {
  	 */
  #define M88DS3103_TS_SERIAL             0 /* TS output pin D0, normal */
  #define M88DS3103_TS_SERIAL_D7          1 /* TS output pin D7 */
-#define M88DS3103_TS_PARALLEL           2 /* 24 MHz, normal */
-#define M88DS3103_TS_PARALLEL_12        3 /* 12 MHz */
-#define M88DS3103_TS_PARALLEL_16        4 /* 16 MHz */
-#define M88DS3103_TS_PARALLEL_19_2      5 /* 19.2 MHz */
-#define M88DS3103_TS_CI                 6 /* 6 MHz */
+#define M88DS3103_TS_PARALLEL           2 /* TS Parallel mode */
+#define M88DS3103_TS_CI                 3 /* TS CI Mode */
  	u8 ts_mode;
 
  	/*
+	 * TS clk in KHz
+	 * Default: 0.
+	 */
+	u32 ts_clk;
+
+	/*
+	 * TS clk polarity.
+	 * Default: 0. 1-active at falling edge; 0-active at rising edge.
+	 */
+	u8 ts_clk_pol:1;
+
+	/*
  	 * spectrum inversion
  	 * Default: 0
  	 */

@@ -86,6 +95,24 @@ struct m88ds3103_config {
  	 * Default: none, must set
  	 */
  	u8 agc;
+
+	/*
+	 * LNB pin control
+	 *
+	 */
+	u8 pin_ctrl;
+
+	/*
+	 * LNB voltage function
+	 *
+	 */

drivers/media/dvb-frontends/m88ds3103.h struct m88ds3103_config { · line 118

Quoted 10 lines

drivers/media/dvb-frontends/m88ds3103.h +7 -0

+	int (*set_voltage)(struct dvb_frontend *fe, fe_sec_voltage_t voltage);
+
+	/*
+	 * start stream function
+	 *
+	 */
+	int (*start_ctrl)(struct dvb_frontend *fe);
  };
 
  /*

regards
Antti

Signature · 1 line

http://palosaari.fi/

Antti Palosaari

2014-08-07 10:27 UTC #3 lore raw

Re: [PATCH 1/4] support for DVBSky dvb-s2 usb: add some config andset_voltage for m88ds3103

Moikka!

Quoted 21 lines · nibble.max

Quoted 13 lines
Quoted 13 lines
drivers/media/dvb-frontends/m88ds3103.c +9 -0
@@ -523,6 +508,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
   	priv->delivery_system = c->delivery_system;
 
+	if (priv->cfg->start_ctrl) {
+		for (len = 0; len < 30 ; len++) {
+			m88ds3103_read_status(fe, &status);
+			if (status & FE_HAS_LOCK) {
+				priv->cfg->start_ctrl(fe);
+				break;
+			}
+			msleep(20);
+		}
Quoted 8 lines
Review static int m88ds3103_set_frontend(struct dvb_frontend *fe) · line 521
Quoted 2 lines
(quoted diff) +2 -0
+	}
+
What is idea of that start_ctrl logic? It looks ugly. Why it is needed? What is wrong with default DVB-core implementation? You should not need to poll demod lock here and then call streaming control callback from USB driver. If you implement .streaming_ctrl() callback to DVB USB driver, it is called automatically for you.

Quoted 7 lines

It is nothing with streaming_ctrl of DVB USB driver.
It relates with the hardware chip problem.
The m88ds3103 will not output ts clock when it powers up at the first time. It starts to output ts clock as soon as it locks the signal.
But the slave fifo of Cypress usb chip really need this clock to work. If there is no this clock, the salve fifo will stop.
start_ctrl logic is used to restart the salve fifo when the ts clock is valid.

OK. Then we have to find out some solution... Is there anyone who has a nice idea how to signal USB interface driver when demod gains a lock? Sure USB driver could poll read_status() too, but it does not sound good solution neither.

How about overriding FE .read_status() callback. It is called all the time by DVB-core when frontend is open. Hook .read_status() to USB interface driver, then call original .read_status() (implemented by m88ds3103 driver), and after each call check if status is LOCKED or NOT LOCKED. When status changes from NOT LOCKED to LOCKED call that board specific routine which restarts TS FIFO. No change for demod driver needed at all.

regards
Antto

Signature · 1 line

http://palosaari.fi/

nibble.max

author

2014-08-07 13:30 UTC #4 lore raw

Re: Re: [PATCH 1/4] support for DVBSky dvb-s2 usb: add some config andset_voltagefor m88ds3103

In reply to nibble.max

Quoted 38 lines

Moikka!
Quoted 21 lines · nibble.max
Quoted 13 lines
Quoted 13 lines
drivers/media/dvb-frontends/m88ds3103.c +9 -0
@@ -523,6 +508,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
   	priv->delivery_system = c->delivery_system;
 
+	if (priv->cfg->start_ctrl) {
+		for (len = 0; len < 30 ; len++) {
+			m88ds3103_read_status(fe, &status);
+			if (status & FE_HAS_LOCK) {
+				priv->cfg->start_ctrl(fe);
+				break;
+			}
+			msleep(20);
+		}
Quoted 8 lines
Review static int m88ds3103_set_frontend(struct dvb_frontend *fe) · line 521
Quoted 2 lines
(quoted diff) +2 -0
+	}
+
What is idea of that start_ctrl logic? It looks ugly. Why it is needed? What is wrong with default DVB-core implementation? You should not need to poll demod lock here and then call streaming control callback from USB driver. If you implement .streaming_ctrl() callback to DVB USB driver, it is called automatically for you.
Quoted 7 lines
It is nothing with streaming_ctrl of DVB USB driver.
It relates with the hardware chip problem.
The m88ds3103 will not output ts clock when it powers up at the first time. It starts to output ts clock as soon as it locks the signal.
But the slave fifo of Cypress usb chip really need this clock to work. If there is no this clock, the salve fifo will stop.
start_ctrl logic is used to restart the salve fifo when the ts clock is valid.
OK. Then we have to find out some solution... Is there anyone who has a nice idea how to signal USB interface driver when demod gains a lock? Sure USB driver could poll read_status() too, but it does not sound good solution neither.

Quoted 8 lines

How about overriding FE .read_status() callback. It is called all the time by DVB-core when frontend is open. Hook .read_status() to USB interface driver, then call original .read_status() (implemented by m88ds3103 driver), and after each call check if status is LOCKED or NOT LOCKED. When status changes from NOT LOCKED to LOCKED call that board specific routine which restarts TS FIFO. No change for demod driver needed at all.

It sounds a good idea.
Try to remove start_ctrl() and set_voltage() callback from demod driver and send the patch for m88ds3103 later.

Quoted 5 lines

regards
Antto
Signature · 1 line
http://palosaari.fi/