Skip to contentSkip to conversation

DVBSky V3 PCIe card: add some changes to M88DS3103 for supporting the demod of M88RS6000

PATCH Superseded Nibble Max 2 participants 9 messages Oct 27, 2014 – Nov 3, 2014

Subsystems (2)
Media (V4L/DVB) M88DS3103 media driver

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Nibble Max

author
#0 lore raw

PATCH v2 3/3 DVBSky V3 PCIe card: add some changes to M88DS3103 for supporting the demod of M88RS6000

v2:
-make demod mclk selection logic simple.
-merge demod mclk and ts mclk into one call back.

M88RS6000 is the integrated chip, which includes tuner and demod.
Its internal demod is similar with M88DS3103 except some registers definition.
The main different part of this internal demod from others is its clock/pll generation IP block sitting inside the tuner die. So clock/pll functions should be configed through its tuner i2c bus, NOT its demod i2c bus.
The demod of M88RS6000 need the firmware: dvb-demod-m88rs6000.fw
firmware download link: http://www.dvbsky.net/download/linux/dvbsky-firmware.tar.gz

Signed-off-by Nibble Max <[email protected]>
---
 drivers/media/dvb-frontends/m88ds3103.c      | 229 +++++++++++++++++++--------
 drivers/media/dvb-frontends/m88ds3103_priv.h | 181 +++++++++++++++++++++
 2 files changed, 340 insertions(+), 70 deletions(-)
drivers/media/dvb-frontends/m88ds3103.c +159 -70
index 81657e9..a4cfef4 100644
@@ -1,5 +1,5 @@
 /*
- * Montage M88DS3103 demodulator driver
+ * Montage M88DS3103/M88RS6000 demodulator driver
  *
  * Copyright (C) 2013 Antti Palosaari <[email protected]>
  *

@@ -162,7 +162,7 @@ static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv,
 
 	dev_dbg(&priv->i2c->dev, "%s: tab_len=%d\n", __func__, tab_len);
 
-	if (tab_len > 83) {
+	if (tab_len > 86) {
 		ret = -EINVAL;
 		goto err;
 	}

@@ -291,6 +291,30 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
+	/* select M88RS6000 demod main mclk and ts mclk from tuner die. */
+	if (priv->chip_id == M88RS6000_CHIP_ID) {
+		if (c->symbol_rate > 45010000)
+			priv->mclk_khz = 110250;
+		else
+			priv->mclk_khz = 96000;
+		if (c->delivery_system == SYS_DVBS)
+			target_mclk = 96000;
+		else
+			target_mclk = 144000;
+		ret = m88ds3103_wr_reg(priv, 0x06, 0xe0);
+		if (ret)
+			goto err;
+		if (fe->ops.tuner_ops.set_config) {
+			ret = fe->ops.tuner_ops.set_config(fe, &target_mclk);
+			if (ret)
+				goto err;
+		}
+		ret = m88ds3103_wr_reg(priv, 0x06, 0x00);
+		if (ret)
+			goto err;
+		usleep_range(10000, 20000);
+	}
+
 	ret = m88ds3103_wr_reg(priv, 0xb2, 0x01);
 	if (ret)
 		goto err;

@@ -301,36 +325,46 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
 	switch (c->delivery_system) {
 	case SYS_DVBS:
-		len = ARRAY_SIZE(m88ds3103_dvbs_init_reg_vals);
-		init = m88ds3103_dvbs_init_reg_vals;
-		target_mclk = 96000;
+		if (priv->chip_id == M88RS6000_CHIP_ID) {
+			len = ARRAY_SIZE(m88rs6000_dvbs_init_reg_vals);
+			init = m88rs6000_dvbs_init_reg_vals;
+		} else {
+			len = ARRAY_SIZE(m88ds3103_dvbs_init_reg_vals);
+			init = m88ds3103_dvbs_init_reg_vals;
+			target_mclk = 96000;
+		}
 		break;
 	case SYS_DVBS2:
-		len = ARRAY_SIZE(m88ds3103_dvbs2_init_reg_vals);
-		init = m88ds3103_dvbs2_init_reg_vals;
-
-		switch (priv->cfg->ts_mode) {
-		case M88DS3103_TS_SERIAL:
-		case M88DS3103_TS_SERIAL_D7:
-			if (c->symbol_rate < 18000000)
-				target_mclk = 96000;
-			else
-				target_mclk = 144000;
-			break;
-		case M88DS3103_TS_PARALLEL:
-		case M88DS3103_TS_CI:
-			if (c->symbol_rate < 18000000)
-				target_mclk = 96000;
-			else if (c->symbol_rate < 28000000)
-				target_mclk = 144000;
-			else
-				target_mclk = 192000;
-			break;
-		default:
-			dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n",
-					__func__);
-			ret = -EINVAL;
-			goto err;
+		if (priv->chip_id == M88RS6000_CHIP_ID) {
+			len = ARRAY_SIZE(m88rs6000_dvbs2_init_reg_vals);
+			init = m88rs6000_dvbs2_init_reg_vals;
+		} else {
+			len = ARRAY_SIZE(m88ds3103_dvbs2_init_reg_vals);
+			init = m88ds3103_dvbs2_init_reg_vals;
+
+			switch (priv->cfg->ts_mode) {
+			case M88DS3103_TS_SERIAL:
+			case M88DS3103_TS_SERIAL_D7:
+				if (c->symbol_rate < 18000000)
+					target_mclk = 96000;
+				else
+					target_mclk = 144000;
+				break;
+			case M88DS3103_TS_PARALLEL:
+			case M88DS3103_TS_CI:
+				if (c->symbol_rate < 18000000)
+					target_mclk = 96000;
+				else if (c->symbol_rate < 28000000)
+					target_mclk = 144000;
+				else
+					target_mclk = 192000;
+				break;
+			default:
+				dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n",
+						__func__);
+				ret = -EINVAL;
+				goto err;
+			}
 		}
 		break;
 	default:

@@ -347,7 +381,25 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 			goto err;
 	}
 
+	if ((priv->chip_id == M88RS6000_CHIP_ID)
+		&& (c->delivery_system == SYS_DVBS2)
+		&& ((c->symbol_rate / 1000) <= 5000)) {
+		ret = m88ds3103_wr_reg(priv, 0xc0, 0x04);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0x8a, 0x09);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0x8b, 0x22);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0x8c, 0x88);
+		if (ret)
+			goto err;
+	}
+
 	u8tmp1 = 0; /* silence compiler warning */
+	u8tmp2 = 0;
 	switch (priv->cfg->ts_mode) {
 	case M88DS3103_TS_SERIAL:
 		u8tmp1 = 0x00;

@@ -385,6 +437,41 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 			goto err;
 	}
 
+	if (priv->chip_id == M88RS6000_CHIP_ID) {
+		ret = m88ds3103_wr_reg_mask(priv, 0x9d, 0x08, 0x08);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0xf1, 0x01);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg_mask(priv, 0x30, 0x80, 0x80);
+		if (ret)
+			goto err;
+	} else {
+		switch (target_mclk) {
+		case 96000:
+			u8tmp1 = 0x02; /* 0b10 */
+			u8tmp2 = 0x01; /* 0b01 */
+			break;
+		case 144000:
+			u8tmp1 = 0x00; /* 0b00 */
+			u8tmp2 = 0x01; /* 0b01 */
+			break;
+		case 192000:
+			u8tmp1 = 0x03; /* 0b11 */
+			u8tmp2 = 0x00; /* 0b00 */
+			break;
+		}
+
+		ret = m88ds3103_wr_reg_mask(priv, 0x22, u8tmp1 << 6, 0xc0);
+		if (ret)
+			goto err;
+
+		ret = m88ds3103_wr_reg_mask(priv, 0x24, u8tmp2 << 6, 0xc0);
+		if (ret)
+			goto err;
+	}
+
 	if (priv->cfg->ts_clk) {
 		divide_ratio = DIV_ROUND_UP(target_mclk, priv->cfg->ts_clk);
 		u8tmp1 = divide_ratio / 2;

@@ -420,29 +507,6 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
-	switch (target_mclk) {
-	case 96000:
-		u8tmp1 = 0x02; /* 0b10 */
-		u8tmp2 = 0x01; /* 0b01 */
-		break;
-	case 144000:
-		u8tmp1 = 0x00; /* 0b00 */
-		u8tmp2 = 0x01; /* 0b01 */
-		break;
-	case 192000:
-		u8tmp1 = 0x03; /* 0b11 */
-		u8tmp2 = 0x00; /* 0b00 */
-		break;
-	}
-
-	ret = m88ds3103_wr_reg_mask(priv, 0x22, u8tmp1 << 6, 0xc0);
-	if (ret)
-		goto err;
-
-	ret = m88ds3103_wr_reg_mask(priv, 0x24, u8tmp2 << 6, 0xc0);
-	if (ret)
-		goto err;
-
 	if (c->symbol_rate <= 3000000)
 		u8tmp = 0x20;
 	else if (c->symbol_rate <= 10000000)

@@ -466,7 +530,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
-	u16tmp = DIV_ROUND_CLOSEST((c->symbol_rate / 1000) << 15, M88DS3103_MCLK_KHZ / 2);
+	u16tmp = DIV_ROUND_CLOSEST((c->symbol_rate / 1000) << 15, priv->mclk_khz / 2);
 	buf[0] = (u16tmp >> 0) & 0xff;
 	buf[1] = (u16tmp >> 8) & 0xff;
 	ret = m88ds3103_wr_regs(priv, 0x61, buf, 2);

@@ -489,7 +553,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 			(tuner_frequency - c->frequency));
 
 	s32tmp = 0x10000 * (tuner_frequency - c->frequency);
-	s32tmp = DIV_ROUND_CLOSEST(s32tmp, M88DS3103_MCLK_KHZ);
+	s32tmp = DIV_ROUND_CLOSEST(s32tmp, priv->mclk_khz);
 	if (s32tmp < 0)
 		s32tmp += 0x10000;
 

@@ -520,7 +584,7 @@ static int m88ds3103_init(struct dvb_frontend *fe)
 	struct m88ds3103_priv *priv = fe->demodulator_priv;
 	int ret, len, remaining;
 	const struct firmware *fw = NULL;
-	u8 *fw_file = M88DS3103_FIRMWARE;
+	u8 *fw_file;
 	u8 u8tmp;
 
 	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);

@@ -541,15 +605,6 @@ static int m88ds3103_init(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
-	/* reset */
-	ret = m88ds3103_wr_reg(priv, 0x07, 0x60);
-	if (ret)
-		goto err;
-
-	ret = m88ds3103_wr_reg(priv, 0x07, 0x00);
-	if (ret)
-		goto err;
-
 	/* firmware status */
 	ret = m88ds3103_rd_reg(priv, 0xb9, &u8tmp);
 	if (ret)

@@ -560,10 +615,23 @@ static int m88ds3103_init(struct dvb_frontend *fe)
 	if (u8tmp)
 		goto skip_fw_download;
 
+	/* global reset, global diseqc reset, golbal fec reset */
+	ret = m88ds3103_wr_reg(priv, 0x07, 0xe0);
+	if (ret)
+		goto err;
+
+	ret = m88ds3103_wr_reg(priv, 0x07, 0x00);
+	if (ret)
+		goto err;
+
 	/* cold state - try to download firmware */
 	dev_info(&priv->i2c->dev, "%s: found a '%s' in cold state\n",
 			KBUILD_MODNAME, m88ds3103_ops.info.name);
 
+	if (priv->chip_id == M88RS6000_CHIP_ID)
+		fw_file = M88RS6000_FIRMWARE;
+	else
+		fw_file = M88DS3103_FIRMWARE;
 	/* request the firmware, this will block and timeout */
 	ret = request_firmware(&fw, fw_file, priv->i2c->dev.parent);
 	if (ret) {

@@ -635,13 +703,18 @@ static int m88ds3103_sleep(struct dvb_frontend *fe)
 {
 	struct m88ds3103_priv *priv = fe->demodulator_priv;
 	int ret;
+	u8 u8reg;
 
 	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);
 
 	priv->delivery_system = SYS_UNDEFINED;
 
 	/* TS Hi-Z */
-	ret = m88ds3103_wr_reg_mask(priv, 0x27, 0x00, 0x01);
+	if (priv->chip_id == M88RS6000_CHIP_ID)
+		u8reg = 0x29;
+	else
+		u8reg = 0x27;
+	ret = m88ds3103_wr_reg_mask(priv, u8reg, 0x00, 0x01);
 	if (ret)
 		goto err;
 

@@ -830,7 +903,7 @@ static int m88ds3103_get_frontend(struct dvb_frontend *fe)
 		goto err;
 
 	c->symbol_rate = 1ull * ((buf[1] << 8) | (buf[0] << 0)) *
-			M88DS3103_MCLK_KHZ * 1000 / 0x10000;
+			priv->mclk_khz * 1000 / 0x10000;
 
 	return 0;
 err:

@@ -1310,18 +1383,22 @@ struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg,
 	priv->i2c = i2c;
 	mutex_init(&priv->i2c_mutex);
 
-	ret = m88ds3103_rd_reg(priv, 0x01, &chip_id);
+	/* 0x00: chip id[6:0], 0x01: chip ver[7:0], 0x02: chip ver[15:8] */
+	ret = m88ds3103_rd_reg(priv, 0x00, &chip_id);
 	if (ret)
 		goto err;
 
-	dev_dbg(&priv->i2c->dev, "%s: chip_id=%02x\n", __func__, chip_id);
+	chip_id >>= 1;
+	dev_info(&priv->i2c->dev, "%s: chip_id=%02x\n", __func__, chip_id);
 
 	switch (chip_id) {
-	case 0xd0:
+	case M88RS6000_CHIP_ID:
+	case M88DS3103_CHIP_ID:
 		break;
 	default:
 		goto err;
 	}
+	priv->chip_id = chip_id;
 
 	switch (priv->cfg->clock_out) {
 	case M88DS3103_CLOCK_OUT_DISABLED:

@@ -1337,6 +1414,11 @@ struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg,
 		goto err;
 	}
 
+	/* 0x29 register is defined differently for m88rs6000. */
+	/* set internal tuner address to 0x42 */
+	if (chip_id == M88RS6000_CHIP_ID)
+		u8tmp = 0x00;
+
 	ret = m88ds3103_wr_reg(priv, 0x29, u8tmp);
 	if (ret)
 		goto err;

@@ -1362,8 +1444,14 @@ struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg,
 
 	*tuner_i2c_adapter = priv->i2c_adapter;
 
+	/* set main mclk to default value */
+	priv->mclk_khz = M88DS3103_MCLK_KHZ;
+
 	/* create dvb_frontend */
 	memcpy(&priv->fe.ops, &m88ds3103_ops, sizeof(struct dvb_frontend_ops));
+	if (priv->chip_id == M88RS6000_CHIP_ID)
+		strncpy(priv->fe.ops.info.name,
+			"Montage M88RS6000", sizeof(priv->fe.ops.info.name));
 	priv->fe.demodulator_priv = priv;
 
 	return &priv->fe;

@@ -1423,3 +1511,4 @@ MODULE_AUTHOR("Antti Palosaari <[email protected]>");
 MODULE_DESCRIPTION("Montage M88DS3103 DVB-S/S2 demodulator driver");
 MODULE_LICENSE("GPL");
 MODULE_FIRMWARE(M88DS3103_FIRMWARE);
+MODULE_FIRMWARE(M88RS6000_FIRMWARE);
drivers/media/dvb-frontends/m88ds3103_priv.h +181 -0

198 lines not on this page · raw message

Signature · 1 line
1.9.1
#1 lore raw

Moikka!
This is full review for that driver. Quite a many small changes requested which are easy to fix. One bigger thing I still want do differently is that target_mclk from tuner. I think you could duplicate TS MCLK calculation in a tuner driver as it is only 4 lines - select mclk per delivery system.

regards
Antti

Quoted 29 lines · Nibble Max

v2:

(quoted diff) +0 -2
-make demod mclk selection logic simple.
-merge demod mclk and ts mclk into one call back.

M88RS6000 is the integrated chip, which includes tuner and demod.
Its internal demod is similar with M88DS3103 except some registers definition.
The main different part of this internal demod from others is its clock/pll generation IP block sitting inside the tuner die. So clock/pll functions should be configed through its tuner i2c bus, NOT its demod i2c bus.
The demod of M88RS6000 need the firmware: dvb-demod-m88rs6000.fw
firmware download link: http://www.dvbsky.net/download/linux/dvbsky-firmware.tar.gz

Signed-off-by Nibble Max <[email protected]>
---
  drivers/media/dvb-frontends/m88ds3103.c      | 229 +++++++++++++++++++--------
  drivers/media/dvb-frontends/m88ds3103_priv.h | 181 +++++++++++++++++++++
  2 files changed, 340 insertions(+), 70 deletions(-)
drivers/media/dvb-frontends/m88ds3103.c +1 -1
index 81657e9..a4cfef4 100644
@@ -1,5 +1,5 @@
  /*
- * Montage M88DS3103 demodulator driver
+ * Montage M88DS3103/M88RS6000 demodulator driver
   *
   * Copyright (C) 2013 Antti Palosaari <[email protected]>
   *

drivers/media/dvb-frontends/m88ds3103.c static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv, · line 165

Quoted 5 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -1
@@ -162,7 +162,7 @@ static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv,
 
  	dev_dbg(&priv->i2c->dev, "%s: tab_len=%d\n", __func__, tab_len);
 
-	if (tab_len > 83) {
+	if (tab_len > 86) {

That is not nice, but I could try find better solution and fix it later.

Quoted 21 lines
drivers/media/dvb-frontends/m88ds3103.c +14 -0
  		ret = -EINVAL;
  		goto err;
  	}

@@ -291,6 +291,30 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	if (ret)
  		goto err;
 
+	/* select M88RS6000 demod main mclk and ts mclk from tuner die. */
+	if (priv->chip_id == M88RS6000_CHIP_ID) {
+		if (c->symbol_rate > 45010000)
+			priv->mclk_khz = 110250;
+		else
+			priv->mclk_khz = 96000;
+		if (c->delivery_system == SYS_DVBS)
+			target_mclk = 96000;
+		else
+			target_mclk = 144000;
+		ret = m88ds3103_wr_reg(priv, 0x06, 0xe0);
+		if (ret)
+			goto err;
+		if (fe->ops.tuner_ops.set_config) {

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

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +10 -0
+			ret = fe->ops.tuner_ops.set_config(fe, &target_mclk);
+			if (ret)
+				goto err;
+		}
+		ret = m88ds3103_wr_reg(priv, 0x06, 0x00);
+		if (ret)
+			goto err;
+		usleep_range(10000, 20000);
+	}
+

Clock selection. What this does:
* select mclk_khz
* select target_mclk
* calls set_config() in order to pass target_mclk to tuner driver
* + some strange looking sleep, which is not likely needed

One thing what I don't like this is that you have implemented M88RS6000 things here and M88DS3103 elsewhere. Generally speaking, code must have some logic where same things are done in same place. So I expect to see both M88DS3103 and M88RS6000 target_mclk and mclk_khz selection implemented here or these moved to place where M88DS3103 implementation is.

Also, even set_config() is somehow logically correctly used here, I prefer to duplicate that 4 line long target_mclk selection to tuner driver and get rid of whole set_config(). Even better solution could be to register M88RS6000 as a clock provider using clock framework, but imho it is not worth that simple case.

Quoted 8 lines
drivers/media/dvb-frontends/m88ds3103.c +0 -1
  	ret = m88ds3103_wr_reg(priv, 0xb2, 0x01);
  	if (ret)
  		goto err;

@@ -301,36 +325,46 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
  	switch (c->delivery_system) {
  	case SYS_DVBS:
-		len = ARRAY_SIZE(m88ds3103_dvbs_init_reg_vals);

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

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +8 -2
-		init = m88ds3103_dvbs_init_reg_vals;
-		target_mclk = 96000;
+		if (priv->chip_id == M88RS6000_CHIP_ID) {
+			len = ARRAY_SIZE(m88rs6000_dvbs_init_reg_vals);
+			init = m88rs6000_dvbs_init_reg_vals;
+		} else {
+			len = ARRAY_SIZE(m88ds3103_dvbs_init_reg_vals);
+			init = m88ds3103_dvbs_init_reg_vals;
+			target_mclk = 96000;
+		}

Aah, M88DS3103 target_mclk selection is found from here. That is just what I was expecting to see on same place than M88RS6000 target_mclk selection.

Quoted 47 lines
drivers/media/dvb-frontends/m88ds3103.c +20 -25
  		break;
  	case SYS_DVBS2:
-		len = ARRAY_SIZE(m88ds3103_dvbs2_init_reg_vals);
-		init = m88ds3103_dvbs2_init_reg_vals;
-
-		switch (priv->cfg->ts_mode) {
-		case M88DS3103_TS_SERIAL:
-		case M88DS3103_TS_SERIAL_D7:
-			if (c->symbol_rate < 18000000)
-				target_mclk = 96000;
-			else
-				target_mclk = 144000;
-			break;
-		case M88DS3103_TS_PARALLEL:
-		case M88DS3103_TS_CI:
-			if (c->symbol_rate < 18000000)
-				target_mclk = 96000;
-			else if (c->symbol_rate < 28000000)
-				target_mclk = 144000;
-			else
-				target_mclk = 192000;
-			break;
-		default:
-			dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n",
-					__func__);
-			ret = -EINVAL;
-			goto err;
+		if (priv->chip_id == M88RS6000_CHIP_ID) {
+			len = ARRAY_SIZE(m88rs6000_dvbs2_init_reg_vals);
+			init = m88rs6000_dvbs2_init_reg_vals;
+		} else {
+			len = ARRAY_SIZE(m88ds3103_dvbs2_init_reg_vals);
+			init = m88ds3103_dvbs2_init_reg_vals;
+
+			switch (priv->cfg->ts_mode) {
+			case M88DS3103_TS_SERIAL:
+			case M88DS3103_TS_SERIAL_D7:
+				if (c->symbol_rate < 18000000)
+					target_mclk = 96000;
+				else
+					target_mclk = 144000;
+				break;
+			case M88DS3103_TS_PARALLEL:
+			case M88DS3103_TS_CI:
+				if (c->symbol_rate < 18000000)
+					target_mclk = 96000;
+				else if (c->symbol_rate < 28000000)

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

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +10 -0
+					target_mclk = 144000;
+				else
+					target_mclk = 192000;
+				break;
+			default:
+				dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n",
+						__func__);
+				ret = -EINVAL;
+				goto err;
+			}

You should really move M88RS6000 target_mclk selection here like M88DS3103 already is.

Quoted 14 lines
drivers/media/dvb-frontends/m88ds3103.c +7 -0
  		}
  		break;
  	default:

@@ -347,7 +381,25 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  			goto err;
  	}
 
+	if ((priv->chip_id == M88RS6000_CHIP_ID)
+		&& (c->delivery_system == SYS_DVBS2)
+		&& ((c->symbol_rate / 1000) <= 5000)) {
+		ret = m88ds3103_wr_reg(priv, 0xc0, 0x04);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0x8a, 0x09);

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

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +10 -0
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0x8b, 0x22);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0x8c, 0x88);
+		if (ret)
+			goto err;
+	}
+

Does not look wrong, but I wonder what this does. Some special handling for DVB-S2 low symbol rates. You should be able to write registers 0x8a to 0x8c using register address auto-increment. Please do.

Quoted 9 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -0
  	u8tmp1 = 0; /* silence compiler warning */
+	u8tmp2 = 0;
  	switch (priv->cfg->ts_mode) {
  	case M88DS3103_TS_SERIAL:
  		u8tmp1 = 0x00;

@@ -385,6 +437,41 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  			goto err;
  	}

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

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +10 -0
+	if (priv->chip_id == M88RS6000_CHIP_ID) {
+		ret = m88ds3103_wr_reg_mask(priv, 0x9d, 0x08, 0x08);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0xf1, 0x01);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg_mask(priv, 0x30, 0x80, 0x80);
+		if (ret)
+			goto err;

Maybe you could move these register writes to same place where was that special low symbol rate thing.
Old/existing functionality here to program TS MCLK and that new one you added is something else (TS MCLK is coming from tuner die in a case of M88RS6000).

Quoted 126 lines
drivers/media/dvb-frontends/m88ds3103.c +41 -35
+	} else {
+		switch (target_mclk) {
+		case 96000:
+			u8tmp1 = 0x02; /* 0b10 */
+			u8tmp2 = 0x01; /* 0b01 */
+			break;
+		case 144000:
+			u8tmp1 = 0x00; /* 0b00 */
+			u8tmp2 = 0x01; /* 0b01 */
+			break;
+		case 192000:
+			u8tmp1 = 0x03; /* 0b11 */
+			u8tmp2 = 0x00; /* 0b00 */
+			break;
+		}
+
+		ret = m88ds3103_wr_reg_mask(priv, 0x22, u8tmp1 << 6, 0xc0);
+		if (ret)
+			goto err;
+
+		ret = m88ds3103_wr_reg_mask(priv, 0x24, u8tmp2 << 6, 0xc0);
+		if (ret)
+			goto err;
+	}
+
  	if (priv->cfg->ts_clk) {
  		divide_ratio = DIV_ROUND_UP(target_mclk, priv->cfg->ts_clk);
  		u8tmp1 = divide_ratio / 2;

@@ -420,29 +507,6 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	if (ret)
  		goto err;
 
-	switch (target_mclk) {
-	case 96000:
-		u8tmp1 = 0x02; /* 0b10 */
-		u8tmp2 = 0x01; /* 0b01 */
-		break;
-	case 144000:
-		u8tmp1 = 0x00; /* 0b00 */
-		u8tmp2 = 0x01; /* 0b01 */
-		break;
-	case 192000:
-		u8tmp1 = 0x03; /* 0b11 */
-		u8tmp2 = 0x00; /* 0b00 */
-		break;
-	}
-
-	ret = m88ds3103_wr_reg_mask(priv, 0x22, u8tmp1 << 6, 0xc0);
-	if (ret)
-		goto err;
-
-	ret = m88ds3103_wr_reg_mask(priv, 0x24, u8tmp2 << 6, 0xc0);
-	if (ret)
-		goto err;
-
  	if (c->symbol_rate <= 3000000)
  		u8tmp = 0x20;
  	else if (c->symbol_rate <= 10000000)

@@ -466,7 +530,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	if (ret)
  		goto err;
 
-	u16tmp = DIV_ROUND_CLOSEST((c->symbol_rate / 1000) << 15, M88DS3103_MCLK_KHZ / 2);
+	u16tmp = DIV_ROUND_CLOSEST((c->symbol_rate / 1000) << 15, priv->mclk_khz / 2);
  	buf[0] = (u16tmp >> 0) & 0xff;
  	buf[1] = (u16tmp >> 8) & 0xff;
  	ret = m88ds3103_wr_regs(priv, 0x61, buf, 2);

@@ -489,7 +553,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  			(tuner_frequency - c->frequency));
 
  	s32tmp = 0x10000 * (tuner_frequency - c->frequency);
-	s32tmp = DIV_ROUND_CLOSEST(s32tmp, M88DS3103_MCLK_KHZ);
+	s32tmp = DIV_ROUND_CLOSEST(s32tmp, priv->mclk_khz);
  	if (s32tmp < 0)
  		s32tmp += 0x10000;
 

@@ -520,7 +584,7 @@ static int m88ds3103_init(struct dvb_frontend *fe)
  	struct m88ds3103_priv *priv = fe->demodulator_priv;
  	int ret, len, remaining;
  	const struct firmware *fw = NULL;
-	u8 *fw_file = M88DS3103_FIRMWARE;
+	u8 *fw_file;
  	u8 u8tmp;
 
  	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);

@@ -541,15 +605,6 @@ static int m88ds3103_init(struct dvb_frontend *fe)
  	if (ret)
  		goto err;
 
-	/* reset */
-	ret = m88ds3103_wr_reg(priv, 0x07, 0x60);
-	if (ret)
-		goto err;
-
-	ret = m88ds3103_wr_reg(priv, 0x07, 0x00);
-	if (ret)
-		goto err;
-
  	/* firmware status */
  	ret = m88ds3103_rd_reg(priv, 0xb9, &u8tmp);
  	if (ret)

@@ -560,10 +615,23 @@ static int m88ds3103_init(struct dvb_frontend *fe)
  	if (u8tmp)
  		goto skip_fw_download;
 
+	/* global reset, global diseqc reset, golbal fec reset */
+	ret = m88ds3103_wr_reg(priv, 0x07, 0xe0);
+	if (ret)
+		goto err;
+
+	ret = m88ds3103_wr_reg(priv, 0x07, 0x00);
+	if (ret)
+		goto err;
+
  	/* cold state - try to download firmware */
  	dev_info(&priv->i2c->dev, "%s: found a '%s' in cold state\n",
  			KBUILD_MODNAME, m88ds3103_ops.info.name);
 
+	if (priv->chip_id == M88RS6000_CHIP_ID)
+		fw_file = M88RS6000_FIRMWARE;
+	else
+		fw_file = M88DS3103_FIRMWARE;
  	/* request the firmware, this will block and timeout */
  	ret = request_firmware(&fw, fw_file, priv->i2c->dev.parent);
  	if (ret) {

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

Quoted 4 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -0
@@ -635,13 +703,18 @@ static int m88ds3103_sleep(struct dvb_frontend *fe)
  {
  	struct m88ds3103_priv *priv = fe->demodulator_priv;
  	int ret;
+	u8 u8reg;

Rename it to u8tmp, just to keep it consistent with other funtions in that driver.

Quoted 51 lines
drivers/media/dvb-frontends/m88ds3103.c +13 -5
  	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);
 
  	priv->delivery_system = SYS_UNDEFINED;
 
  	/* TS Hi-Z */
-	ret = m88ds3103_wr_reg_mask(priv, 0x27, 0x00, 0x01);
+	if (priv->chip_id == M88RS6000_CHIP_ID)
+		u8reg = 0x29;
+	else
+		u8reg = 0x27;
+	ret = m88ds3103_wr_reg_mask(priv, u8reg, 0x00, 0x01);
  	if (ret)
  		goto err;
 

@@ -830,7 +903,7 @@ static int m88ds3103_get_frontend(struct dvb_frontend *fe)
  		goto err;
 
  	c->symbol_rate = 1ull * ((buf[1] << 8) | (buf[0] << 0)) *
-			M88DS3103_MCLK_KHZ * 1000 / 0x10000;
+			priv->mclk_khz * 1000 / 0x10000;
 
  	return 0;
  err:

@@ -1310,18 +1383,22 @@ struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg,
  	priv->i2c = i2c;
  	mutex_init(&priv->i2c_mutex);
 
-	ret = m88ds3103_rd_reg(priv, 0x01, &chip_id);
+	/* 0x00: chip id[6:0], 0x01: chip ver[7:0], 0x02: chip ver[15:8] */
+	ret = m88ds3103_rd_reg(priv, 0x00, &chip_id);
  	if (ret)
  		goto err;
 
-	dev_dbg(&priv->i2c->dev, "%s: chip_id=%02x\n", __func__, chip_id);
+	chip_id >>= 1;
+	dev_info(&priv->i2c->dev, "%s: chip_id=%02x\n", __func__, chip_id);
 
  	switch (chip_id) {
-	case 0xd0:
+	case M88RS6000_CHIP_ID:
+	case M88DS3103_CHIP_ID:
  		break;
  	default:
  		goto err;
  	}
+	priv->chip_id = chip_id;
 
  	switch (priv->cfg->clock_out) {
  	case M88DS3103_CLOCK_OUT_DISABLED:

drivers/media/dvb-frontends/m88ds3103.c struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg, · line 1421

Quoted 8 lines
drivers/media/dvb-frontends/m88ds3103.c +5 -0
@@ -1337,6 +1414,11 @@ struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg,
  		goto err;
  	}
 
+	/* 0x29 register is defined differently for m88rs6000. */
+	/* set internal tuner address to 0x42 */
+	if (chip_id == M88RS6000_CHIP_ID)
+		u8tmp = 0x00;
+

So that is I2C address used for tuner. Does that register set tuner I2C address? Or is it just needed by demod for some reason?

Bridge driver uses
+ tuner_info.addr = 0x21;
which is 0x42, but in a standard 7 bit notation. So maybe comment should say I2C address is 0x21.

drivers/media/dvb-frontends/m88ds3103.c struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg, · line 1449

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +3 -0
  	ret = m88ds3103_wr_reg(priv, 0x29, u8tmp);
  	if (ret)
  		goto err;

@@ -1362,8 +1444,14 @@ struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg,
 
  	*tuner_i2c_adapter = priv->i2c_adapter;
 
+	/* set main mclk to default value */
+	priv->mclk_khz = M88DS3103_MCLK_KHZ;
+

That should go to same place where you select M88RS6000 mclk_khz.

Quoted 219 lines
drivers/media/dvb-frontends/m88ds3103.c +4 -0
  	/* create dvb_frontend */
  	memcpy(&priv->fe.ops, &m88ds3103_ops, sizeof(struct dvb_frontend_ops));
+	if (priv->chip_id == M88RS6000_CHIP_ID)
+		strncpy(priv->fe.ops.info.name,
+			"Montage M88RS6000", sizeof(priv->fe.ops.info.name));
  	priv->fe.demodulator_priv = priv;
 
  	return &priv->fe;

@@ -1423,3 +1511,4 @@ MODULE_AUTHOR("Antti Palosaari <[email protected]>");
  MODULE_DESCRIPTION("Montage M88DS3103 DVB-S/S2 demodulator driver");
  MODULE_LICENSE("GPL");
  MODULE_FIRMWARE(M88DS3103_FIRMWARE);
+MODULE_FIRMWARE(M88RS6000_FIRMWARE);
drivers/media/dvb-frontends/m88ds3103_priv.h +181 -0
index 9169fdd..a2c0958 100644
@@ -25,7 +25,10 @@
  #include <linux/math64.h>
 
  #define M88DS3103_FIRMWARE "dvb-demod-m88ds3103.fw"
+#define M88RS6000_FIRMWARE "dvb-demod-m88rs6000.fw"
  #define M88DS3103_MCLK_KHZ 96000
+#define M88RS6000_CHIP_ID 0x74
+#define M88DS3103_CHIP_ID 0x70
 
  struct m88ds3103_priv {
  	struct i2c_adapter *i2c;

@@ -38,6 +41,10 @@ struct m88ds3103_priv {
  	u32 ber;
  	bool warm; /* FW running */
  	struct i2c_adapter *i2c_adapter;
+	/* auto detect chip id to do different config */
+	u8 chip_id;
+	/* main mclk is calculated for M88RS6000 dynamically */
+	u32 mclk_khz;
  };
 
  struct m88ds3103_reg_val {

@@ -214,4 +221,178 @@ static const struct m88ds3103_reg_val m88ds3103_dvbs2_init_reg_vals[] = {
  	{0xb8, 0x00},
  };
 
+static const struct m88ds3103_reg_val m88rs6000_dvbs_init_reg_vals[] = {
+	{0x23, 0x07},
+	{0x08, 0x03},
+	{0x0c, 0x02},
+	{0x20, 0x00},
+	{0x21, 0x54},
+	{0x25, 0x82},
+	{0x27, 0x31},
+	{0x30, 0x08},
+	{0x31, 0x40},
+	{0x32, 0x32},
+	{0x33, 0x35},
+	{0x35, 0xff},
+	{0x3a, 0x00},
+	{0x37, 0x10},
+	{0x38, 0x10},
+	{0x39, 0x02},
+	{0x42, 0x60},
+	{0x4a, 0x80},
+	{0x4b, 0x04},
+	{0x4d, 0x91},
+	{0x5d, 0xc8},
+	{0x50, 0x36},
+	{0x51, 0x36},
+	{0x52, 0x36},
+	{0x53, 0x36},
+	{0x63, 0x0f},
+	{0x64, 0x30},
+	{0x65, 0x40},
+	{0x68, 0x26},
+	{0x69, 0x4c},
+	{0x70, 0x20},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x40},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x60},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x80},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0xa0},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x1f},
+	{0x76, 0x38},
+	{0x77, 0xa6},
+	{0x78, 0x0c},
+	{0x79, 0x80},
+	{0x7f, 0x14},
+	{0x7c, 0x00},
+	{0xae, 0x82},
+	{0x80, 0x64},
+	{0x81, 0x66},
+	{0x82, 0x44},
+	{0x85, 0x04},
+	{0xcd, 0xf4},
+	{0x90, 0x33},
+	{0xa0, 0x44},
+	{0xbe, 0x00},
+	{0xc0, 0x08},
+	{0xc3, 0x10},
+	{0xc4, 0x08},
+	{0xc5, 0xf0},
+	{0xc6, 0xff},
+	{0xc7, 0x00},
+	{0xc8, 0x1a},
+	{0xc9, 0x80},
+	{0xe0, 0xf8},
+	{0xe6, 0x8b},
+	{0xd0, 0x40},
+	{0xf8, 0x20},
+	{0xfa, 0x0f},
+	{0x00, 0x00},
+	{0xbd, 0x01},
+	{0xb8, 0x00},
+	{0x29, 0x11},
+};
+
+static const struct m88ds3103_reg_val m88rs6000_dvbs2_init_reg_vals[] = {
+	{0x23, 0x07},
+	{0x08, 0x07},
+	{0x0c, 0x02},
+	{0x20, 0x00},
+	{0x21, 0x54},
+	{0x25, 0x82},
+	{0x27, 0x31},
+	{0x30, 0x08},
+	{0x32, 0x32},
+	{0x33, 0x35},
+	{0x35, 0xff},
+	{0x3a, 0x00},
+	{0x37, 0x10},
+	{0x38, 0x10},
+	{0x39, 0x02},
+	{0x42, 0x60},
+	{0x4a, 0x80},
+	{0x4b, 0x04},
+	{0x4d, 0x91},
+	{0x5d, 0xc8},
+	{0x50, 0x36},
+	{0x51, 0x36},
+	{0x52, 0x36},
+	{0x53, 0x36},
+	{0x63, 0x0f},
+	{0x64, 0x10},
+	{0x65, 0x20},
+	{0x68, 0x46},
+	{0x69, 0xcd},
+	{0x70, 0x20},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x40},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x60},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x80},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0xa0},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x1f},
+	{0x76, 0x38},
+	{0x77, 0xa6},
+	{0x78, 0x0c},
+	{0x79, 0x80},
+	{0x7f, 0x14},
+	{0x85, 0x08},
+	{0xcd, 0xf4},
+	{0x90, 0x33},
+	{0x86, 0x00},
+	{0x87, 0x0f},
+	{0x89, 0x00},
+	{0x8b, 0x44},
+	{0x8c, 0x66},
+	{0x9d, 0xc1},
+	{0x8a, 0x10},
+	{0xad, 0x40},
+	{0xa0, 0x44},
+	{0xbe, 0x00},
+	{0xc0, 0x08},
+	{0xc1, 0x10},
+	{0xc2, 0x08},
+	{0xc3, 0x10},
+	{0xc4, 0x08},
+	{0xc5, 0xf0},
+	{0xc6, 0xff},
+	{0xc7, 0x00},
+	{0xc8, 0x1a},
+	{0xc9, 0x80},
+	{0xca, 0x23},
+	{0xcb, 0x24},
+	{0xcc, 0xf4},
+	{0xce, 0x74},
+	{0x00, 0x00},
+	{0xbd, 0x01},
+	{0xb8, 0x00},
+	{0x29, 0x01},
+};
  #endif
Signature · 1 line
http://palosaari.fi/
author
#2 lore raw

Re: Re: [PATCH v2 3/3] DVBSky V3 PCIe card: add some changes to M88DS3103for supporting the demod of M88RS6000

In reply to Nibble Max

Hello,
Thanks for review.

Quoted 41 lines · Antti Palosaari

Moikka!
This is full review for that driver. Quite a many small changes requested which are easy to fix. One bigger thing I still want do differently is that target_mclk from tuner. I think you could duplicate TS MCLK calculation in a tuner driver as it is only 4 lines - select mclk per delivery system.

regards
Antti

Quoted 30 lines · Nibble Max

v2:

(quoted diff) +0 -2
-make demod mclk selection logic simple.
-merge demod mclk and ts mclk into one call back.

M88RS6000 is the integrated chip, which includes tuner and demod.
Its internal demod is similar with M88DS3103 except some registers definition.
The main different part of this internal demod from others is its clock/pll generation IP block sitting inside the tuner die. So clock/pll functions should be configed through its tuner i2c bus, NOT its demod i2c bus.
The demod of M88RS6000 need the firmware: dvb-demod-m88rs6000.fw
firmware download link: http://www.dvbsky.net/download/linux/dvbsky-firmware.tar.gz

Signed-off-by Nibble Max <[email protected]>
---
  drivers/media/dvb-frontends/m88ds3103.c      | 229 +++++++++++++++++++--------
  drivers/media/dvb-frontends/m88ds3103_priv.h | 181 +++++++++++++++++++++
  2 files changed, 340 insertions(+), 70 deletions(-)
drivers/media/dvb-frontends/m88ds3103.c +1 -1
index 81657e9..a4cfef4 100644
@@ -1,5 +1,5 @@
  /*
- * Montage M88DS3103 demodulator driver
+ * Montage M88DS3103/M88RS6000 demodulator driver
   *
   * Copyright (C) 2013 Antti Palosaari <[email protected]>
   *

@@ -162,7 +162,7 @@ static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv,
Quoted 6 lines

drivers/media/dvb-frontends/m88ds3103.c static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv, · line 165

Quoted 4 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -1
  	dev_dbg(&priv->i2c->dev, "%s: tab_len=%d\n", __func__, tab_len);
 
-	if (tab_len > 83) {
+	if (tab_len > 86) {

That is not nice, but I could try find better solution and fix it later.

What is the reason to check this parameter?
How about remove this check?

Quoted 31 lines
Quoted 31 lines
drivers/media/dvb-frontends/m88ds3103.c +22 -0
  		ret = -EINVAL;
  		goto err;
  	}

@@ -291,6 +291,30 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	if (ret)
  		goto err;
 
+	/* select M88RS6000 demod main mclk and ts mclk from tuner die. */
+	if (priv->chip_id == M88RS6000_CHIP_ID) {
+		if (c->symbol_rate > 45010000)
+			priv->mclk_khz = 110250;
+		else
+			priv->mclk_khz = 96000;
+		if (c->delivery_system == SYS_DVBS)
+			target_mclk = 96000;
+		else
+			target_mclk = 144000;
+		ret = m88ds3103_wr_reg(priv, 0x06, 0xe0);
+		if (ret)
+			goto err;
+		if (fe->ops.tuner_ops.set_config) {
+			ret = fe->ops.tuner_ops.set_config(fe, &target_mclk);
+			if (ret)
+				goto err;
+		}
+		ret = m88ds3103_wr_reg(priv, 0x06, 0x00);
+		if (ret)
+			goto err;
+		usleep_range(10000, 20000);
Quoted 8 lines

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

Quoted 2 lines
drivers/media/dvb-frontends/m88ds3103.c +2 -0
+	}
+

Clock selection. What this does:
* select mclk_khz
* select target_mclk
* calls set_config() in order to pass target_mclk to tuner driver
* + some strange looking sleep, which is not likely needed

The clock of M88RS6000's demod comes from tuner dies.
So the first thing is turning on the demod main clock from tuner die after the demod reset. Without this clock, the following register's content will fail to update. Before changing the demod main clock, it should close clock path.
After changing the demod main clock, it open clock path and wait the clock to stable.

Quoted 7 lines

One thing what I don't like this is that you have implemented M88RS6000 things here and M88DS3103 elsewhere. Generally speaking, code must have some logic where same things are done in same place. So I expect to see both M88DS3103 and M88RS6000 target_mclk and mclk_khz selection implemented here or these moved to place where M88DS3103 implementation is.

I will move M88DS3103 implementation to here.

Quoted 5 lines

Also, even set_config() is somehow logically correctly used here, I prefer to duplicate that 4 line long target_mclk selection to tuner driver and get rid of whole set_config(). Even better solution could be to register M88RS6000 as a clock provider using clock framework, but imho it is not worth that simple case.

Do you suggest to set demod main clock and ts clock in tuner's set_params call back?

Quoted 331 lines
Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +0 -1
  	ret = m88ds3103_wr_reg(priv, 0xb2, 0x01);
  	if (ret)
  		goto err;

@@ -301,36 +325,46 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
 
  	switch (c->delivery_system) {
  	case SYS_DVBS:
-		len = ARRAY_SIZE(m88ds3103_dvbs_init_reg_vals);

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

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +8 -2
-		init = m88ds3103_dvbs_init_reg_vals;
-		target_mclk = 96000;
+		if (priv->chip_id == M88RS6000_CHIP_ID) {
+			len = ARRAY_SIZE(m88rs6000_dvbs_init_reg_vals);
+			init = m88rs6000_dvbs_init_reg_vals;
+		} else {
+			len = ARRAY_SIZE(m88ds3103_dvbs_init_reg_vals);
+			init = m88ds3103_dvbs_init_reg_vals;
+			target_mclk = 96000;
+		}

Aah, M88DS3103 target_mclk selection is found from here. That is just what I was expecting to see on same place than M88RS6000 target_mclk selection.

Quoted 47 lines
drivers/media/dvb-frontends/m88ds3103.c +20 -25
  		break;
  	case SYS_DVBS2:
-		len = ARRAY_SIZE(m88ds3103_dvbs2_init_reg_vals);
-		init = m88ds3103_dvbs2_init_reg_vals;
-
-		switch (priv->cfg->ts_mode) {
-		case M88DS3103_TS_SERIAL:
-		case M88DS3103_TS_SERIAL_D7:
-			if (c->symbol_rate < 18000000)
-				target_mclk = 96000;
-			else
-				target_mclk = 144000;
-			break;
-		case M88DS3103_TS_PARALLEL:
-		case M88DS3103_TS_CI:
-			if (c->symbol_rate < 18000000)
-				target_mclk = 96000;
-			else if (c->symbol_rate < 28000000)
-				target_mclk = 144000;
-			else
-				target_mclk = 192000;
-			break;
-		default:
-			dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n",
-					__func__);
-			ret = -EINVAL;
-			goto err;
+		if (priv->chip_id == M88RS6000_CHIP_ID) {
+			len = ARRAY_SIZE(m88rs6000_dvbs2_init_reg_vals);
+			init = m88rs6000_dvbs2_init_reg_vals;
+		} else {
+			len = ARRAY_SIZE(m88ds3103_dvbs2_init_reg_vals);
+			init = m88ds3103_dvbs2_init_reg_vals;
+
+			switch (priv->cfg->ts_mode) {
+			case M88DS3103_TS_SERIAL:
+			case M88DS3103_TS_SERIAL_D7:
+				if (c->symbol_rate < 18000000)
+					target_mclk = 96000;
+				else
+					target_mclk = 144000;
+				break;
+			case M88DS3103_TS_PARALLEL:
+			case M88DS3103_TS_CI:
+				if (c->symbol_rate < 18000000)
+					target_mclk = 96000;
+				else if (c->symbol_rate < 28000000)

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

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +10 -0
+					target_mclk = 144000;
+				else
+					target_mclk = 192000;
+				break;
+			default:
+				dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n",
+						__func__);
+				ret = -EINVAL;
+				goto err;
+			}

You should really move M88RS6000 target_mclk selection here like M88DS3103 already is.

Quoted 14 lines
drivers/media/dvb-frontends/m88ds3103.c +7 -0
  		}
  		break;
  	default:

@@ -347,7 +381,25 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  			goto err;
  	}
 
+	if ((priv->chip_id == M88RS6000_CHIP_ID)
+		&& (c->delivery_system == SYS_DVBS2)
+		&& ((c->symbol_rate / 1000) <= 5000)) {
+		ret = m88ds3103_wr_reg(priv, 0xc0, 0x04);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0x8a, 0x09);

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

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +10 -0
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0x8b, 0x22);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0x8c, 0x88);
+		if (ret)
+			goto err;
+	}
+

Does not look wrong, but I wonder what this does. Some special handling for DVB-S2 low symbol rates. You should be able to write registers 0x8a to 0x8c using register address auto-increment. Please do.

Quoted 9 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -0
  	u8tmp1 = 0; /* silence compiler warning */
+	u8tmp2 = 0;
  	switch (priv->cfg->ts_mode) {
  	case M88DS3103_TS_SERIAL:
  		u8tmp1 = 0x00;

@@ -385,6 +437,41 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  			goto err;
  	}

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

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +10 -0
+	if (priv->chip_id == M88RS6000_CHIP_ID) {
+		ret = m88ds3103_wr_reg_mask(priv, 0x9d, 0x08, 0x08);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg(priv, 0xf1, 0x01);
+		if (ret)
+			goto err;
+		ret = m88ds3103_wr_reg_mask(priv, 0x30, 0x80, 0x80);
+		if (ret)
+			goto err;

Maybe you could move these register writes to same place where was that special low symbol rate thing.
Old/existing functionality here to program TS MCLK and that new one you added is something else (TS MCLK is coming from tuner die in a case of M88RS6000).

Quoted 126 lines
drivers/media/dvb-frontends/m88ds3103.c +41 -35
+	} else {
+		switch (target_mclk) {
+		case 96000:
+			u8tmp1 = 0x02; /* 0b10 */
+			u8tmp2 = 0x01; /* 0b01 */
+			break;
+		case 144000:
+			u8tmp1 = 0x00; /* 0b00 */
+			u8tmp2 = 0x01; /* 0b01 */
+			break;
+		case 192000:
+			u8tmp1 = 0x03; /* 0b11 */
+			u8tmp2 = 0x00; /* 0b00 */
+			break;
+		}
+
+		ret = m88ds3103_wr_reg_mask(priv, 0x22, u8tmp1 << 6, 0xc0);
+		if (ret)
+			goto err;
+
+		ret = m88ds3103_wr_reg_mask(priv, 0x24, u8tmp2 << 6, 0xc0);
+		if (ret)
+			goto err;
+	}
+
  	if (priv->cfg->ts_clk) {
  		divide_ratio = DIV_ROUND_UP(target_mclk, priv->cfg->ts_clk);
  		u8tmp1 = divide_ratio / 2;

@@ -420,29 +507,6 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	if (ret)
  		goto err;
 
-	switch (target_mclk) {
-	case 96000:
-		u8tmp1 = 0x02; /* 0b10 */
-		u8tmp2 = 0x01; /* 0b01 */
-		break;
-	case 144000:
-		u8tmp1 = 0x00; /* 0b00 */
-		u8tmp2 = 0x01; /* 0b01 */
-		break;
-	case 192000:
-		u8tmp1 = 0x03; /* 0b11 */
-		u8tmp2 = 0x00; /* 0b00 */
-		break;
-	}
-
-	ret = m88ds3103_wr_reg_mask(priv, 0x22, u8tmp1 << 6, 0xc0);
-	if (ret)
-		goto err;
-
-	ret = m88ds3103_wr_reg_mask(priv, 0x24, u8tmp2 << 6, 0xc0);
-	if (ret)
-		goto err;
-
  	if (c->symbol_rate <= 3000000)
  		u8tmp = 0x20;
  	else if (c->symbol_rate <= 10000000)

@@ -466,7 +530,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  	if (ret)
  		goto err;
 
-	u16tmp = DIV_ROUND_CLOSEST((c->symbol_rate / 1000) << 15, M88DS3103_MCLK_KHZ / 2);
+	u16tmp = DIV_ROUND_CLOSEST((c->symbol_rate / 1000) << 15, priv->mclk_khz / 2);
  	buf[0] = (u16tmp >> 0) & 0xff;
  	buf[1] = (u16tmp >> 8) & 0xff;
  	ret = m88ds3103_wr_regs(priv, 0x61, buf, 2);

@@ -489,7 +553,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend *fe)
  			(tuner_frequency - c->frequency));
 
  	s32tmp = 0x10000 * (tuner_frequency - c->frequency);
-	s32tmp = DIV_ROUND_CLOSEST(s32tmp, M88DS3103_MCLK_KHZ);
+	s32tmp = DIV_ROUND_CLOSEST(s32tmp, priv->mclk_khz);
  	if (s32tmp < 0)
  		s32tmp += 0x10000;
 

@@ -520,7 +584,7 @@ static int m88ds3103_init(struct dvb_frontend *fe)
  	struct m88ds3103_priv *priv = fe->demodulator_priv;
  	int ret, len, remaining;
  	const struct firmware *fw = NULL;
-	u8 *fw_file = M88DS3103_FIRMWARE;
+	u8 *fw_file;
  	u8 u8tmp;
 
  	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);

@@ -541,15 +605,6 @@ static int m88ds3103_init(struct dvb_frontend *fe)
  	if (ret)
  		goto err;
 
-	/* reset */
-	ret = m88ds3103_wr_reg(priv, 0x07, 0x60);
-	if (ret)
-		goto err;
-
-	ret = m88ds3103_wr_reg(priv, 0x07, 0x00);
-	if (ret)
-		goto err;
-
  	/* firmware status */
  	ret = m88ds3103_rd_reg(priv, 0xb9, &u8tmp);
  	if (ret)

@@ -560,10 +615,23 @@ static int m88ds3103_init(struct dvb_frontend *fe)
  	if (u8tmp)
  		goto skip_fw_download;
 
+	/* global reset, global diseqc reset, golbal fec reset */
+	ret = m88ds3103_wr_reg(priv, 0x07, 0xe0);
+	if (ret)
+		goto err;
+
+	ret = m88ds3103_wr_reg(priv, 0x07, 0x00);
+	if (ret)
+		goto err;
+
  	/* cold state - try to download firmware */
  	dev_info(&priv->i2c->dev, "%s: found a '%s' in cold state\n",
  			KBUILD_MODNAME, m88ds3103_ops.info.name);
 
+	if (priv->chip_id == M88RS6000_CHIP_ID)
+		fw_file = M88RS6000_FIRMWARE;
+	else
+		fw_file = M88DS3103_FIRMWARE;
  	/* request the firmware, this will block and timeout */
  	ret = request_firmware(&fw, fw_file, priv->i2c->dev.parent);
  	if (ret) {

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

Quoted 4 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -0
@@ -635,13 +703,18 @@ static int m88ds3103_sleep(struct dvb_frontend *fe)
  {
  	struct m88ds3103_priv *priv = fe->demodulator_priv;
  	int ret;
+	u8 u8reg;

Rename it to u8tmp, just to keep it consistent with other funtions in that driver.

Quoted 54 lines
drivers/media/dvb-frontends/m88ds3103.c +13 -5
  	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);
 
  	priv->delivery_system = SYS_UNDEFINED;
 
  	/* TS Hi-Z */
-	ret = m88ds3103_wr_reg_mask(priv, 0x27, 0x00, 0x01);
+	if (priv->chip_id == M88RS6000_CHIP_ID)
+		u8reg = 0x29;
+	else
+		u8reg = 0x27;
+	ret = m88ds3103_wr_reg_mask(priv, u8reg, 0x00, 0x01);
  	if (ret)
  		goto err;
 

@@ -830,7 +903,7 @@ static int m88ds3103_get_frontend(struct dvb_frontend *fe)
  		goto err;
 
  	c->symbol_rate = 1ull * ((buf[1] << 8) | (buf[0] << 0)) *
-			M88DS3103_MCLK_KHZ * 1000 / 0x10000;
+			priv->mclk_khz * 1000 / 0x10000;
 
  	return 0;
  err:

@@ -1310,18 +1383,22 @@ struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg,
  	priv->i2c = i2c;
  	mutex_init(&priv->i2c_mutex);
 
-	ret = m88ds3103_rd_reg(priv, 0x01, &chip_id);
+	/* 0x00: chip id[6:0], 0x01: chip ver[7:0], 0x02: chip ver[15:8] */
+	ret = m88ds3103_rd_reg(priv, 0x00, &chip_id);
  	if (ret)
  		goto err;
 
-	dev_dbg(&priv->i2c->dev, "%s: chip_id=%02x\n", __func__, chip_id);
+	chip_id >>= 1;
+	dev_info(&priv->i2c->dev, "%s: chip_id=%02x\n", __func__, chip_id);
 
  	switch (chip_id) {
-	case 0xd0:
+	case M88RS6000_CHIP_ID:
+	case M88DS3103_CHIP_ID:
  		break;
  	default:
  		goto err;
  	}
+	priv->chip_id = chip_id;
 
  	switch (priv->cfg->clock_out) {
  	case M88DS3103_CLOCK_OUT_DISABLED:

@@ -1337,6 +1414,11 @@ struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg,
  		goto err;
  	}
Quoted 8 lines

drivers/media/dvb-frontends/m88ds3103.c struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg, · line 1421

Quoted 5 lines
drivers/media/dvb-frontends/m88ds3103.c +5 -0
+	/* 0x29 register is defined differently for m88rs6000. */
+	/* set internal tuner address to 0x42 */
+	if (chip_id == M88RS6000_CHIP_ID)
+		u8tmp = 0x00;
+

So that is I2C address used for tuner. Does that register set tuner I2C address? Or is it just needed by demod for some reason?

The tuner I2C address is configured by this demod register.

Quoted 231 lines

Bridge driver uses

(quoted diff) +1 -0
+	tuner_info.addr = 0x21;

which is 0x42, but in a standard 7 bit notation. So maybe comment should say I2C address is 0x21.

drivers/media/dvb-frontends/m88ds3103.c struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg, · line 1449

Quoted 10 lines
drivers/media/dvb-frontends/m88ds3103.c +3 -0
  	ret = m88ds3103_wr_reg(priv, 0x29, u8tmp);
  	if (ret)
  		goto err;

@@ -1362,8 +1444,14 @@ struct dvb_frontend *m88ds3103_attach(const struct m88ds3103_config *cfg,
 
  	*tuner_i2c_adapter = priv->i2c_adapter;
 
+	/* set main mclk to default value */
+	priv->mclk_khz = M88DS3103_MCLK_KHZ;
+

That should go to same place where you select M88RS6000 mclk_khz.

Quoted 211 lines
drivers/media/dvb-frontends/m88ds3103.c +4 -0
  	/* create dvb_frontend */
  	memcpy(&priv->fe.ops, &m88ds3103_ops, sizeof(struct dvb_frontend_ops));
+	if (priv->chip_id == M88RS6000_CHIP_ID)
+		strncpy(priv->fe.ops.info.name,
+			"Montage M88RS6000", sizeof(priv->fe.ops.info.name));
  	priv->fe.demodulator_priv = priv;
 
  	return &priv->fe;

@@ -1423,3 +1511,4 @@ MODULE_AUTHOR("Antti Palosaari <[email protected]>");
  MODULE_DESCRIPTION("Montage M88DS3103 DVB-S/S2 demodulator driver");
  MODULE_LICENSE("GPL");
  MODULE_FIRMWARE(M88DS3103_FIRMWARE);
+MODULE_FIRMWARE(M88RS6000_FIRMWARE);
drivers/media/dvb-frontends/m88ds3103_priv.h +175 -0
index 9169fdd..a2c0958 100644
@@ -25,7 +25,10 @@
  #include <linux/math64.h>
 
  #define M88DS3103_FIRMWARE "dvb-demod-m88ds3103.fw"
+#define M88RS6000_FIRMWARE "dvb-demod-m88rs6000.fw"
  #define M88DS3103_MCLK_KHZ 96000
+#define M88RS6000_CHIP_ID 0x74
+#define M88DS3103_CHIP_ID 0x70
 
  struct m88ds3103_priv {
  	struct i2c_adapter *i2c;

@@ -38,6 +41,10 @@ struct m88ds3103_priv {
  	u32 ber;
  	bool warm; /* FW running */
  	struct i2c_adapter *i2c_adapter;
+	/* auto detect chip id to do different config */
+	u8 chip_id;
+	/* main mclk is calculated for M88RS6000 dynamically */
+	u32 mclk_khz;
  };
 
  struct m88ds3103_reg_val {

@@ -214,4 +221,178 @@ static const struct m88ds3103_reg_val m88ds3103_dvbs2_init_reg_vals[] = {
  	{0xb8, 0x00},
  };
 
+static const struct m88ds3103_reg_val m88rs6000_dvbs_init_reg_vals[] = {
+	{0x23, 0x07},
+	{0x08, 0x03},
+	{0x0c, 0x02},
+	{0x20, 0x00},
+	{0x21, 0x54},
+	{0x25, 0x82},
+	{0x27, 0x31},
+	{0x30, 0x08},
+	{0x31, 0x40},
+	{0x32, 0x32},
+	{0x33, 0x35},
+	{0x35, 0xff},
+	{0x3a, 0x00},
+	{0x37, 0x10},
+	{0x38, 0x10},
+	{0x39, 0x02},
+	{0x42, 0x60},
+	{0x4a, 0x80},
+	{0x4b, 0x04},
+	{0x4d, 0x91},
+	{0x5d, 0xc8},
+	{0x50, 0x36},
+	{0x51, 0x36},
+	{0x52, 0x36},
+	{0x53, 0x36},
+	{0x63, 0x0f},
+	{0x64, 0x30},
+	{0x65, 0x40},
+	{0x68, 0x26},
+	{0x69, 0x4c},
+	{0x70, 0x20},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x40},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x60},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x80},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0xa0},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x1f},
+	{0x76, 0x38},
+	{0x77, 0xa6},
+	{0x78, 0x0c},
+	{0x79, 0x80},
+	{0x7f, 0x14},
+	{0x7c, 0x00},
+	{0xae, 0x82},
+	{0x80, 0x64},
+	{0x81, 0x66},
+	{0x82, 0x44},
+	{0x85, 0x04},
+	{0xcd, 0xf4},
+	{0x90, 0x33},
+	{0xa0, 0x44},
+	{0xbe, 0x00},
+	{0xc0, 0x08},
+	{0xc3, 0x10},
+	{0xc4, 0x08},
+	{0xc5, 0xf0},
+	{0xc6, 0xff},
+	{0xc7, 0x00},
+	{0xc8, 0x1a},
+	{0xc9, 0x80},
+	{0xe0, 0xf8},
+	{0xe6, 0x8b},
+	{0xd0, 0x40},
+	{0xf8, 0x20},
+	{0xfa, 0x0f},
+	{0x00, 0x00},
+	{0xbd, 0x01},
+	{0xb8, 0x00},
+	{0x29, 0x11},
+};
+
+static const struct m88ds3103_reg_val m88rs6000_dvbs2_init_reg_vals[] = {
+	{0x23, 0x07},
+	{0x08, 0x07},
+	{0x0c, 0x02},
+	{0x20, 0x00},
+	{0x21, 0x54},
+	{0x25, 0x82},
+	{0x27, 0x31},
+	{0x30, 0x08},
+	{0x32, 0x32},
+	{0x33, 0x35},
+	{0x35, 0xff},
+	{0x3a, 0x00},
+	{0x37, 0x10},
+	{0x38, 0x10},
+	{0x39, 0x02},
+	{0x42, 0x60},
+	{0x4a, 0x80},
+	{0x4b, 0x04},
+	{0x4d, 0x91},
+	{0x5d, 0xc8},
+	{0x50, 0x36},
+	{0x51, 0x36},
+	{0x52, 0x36},
+	{0x53, 0x36},
+	{0x63, 0x0f},
+	{0x64, 0x10},
+	{0x65, 0x20},
+	{0x68, 0x46},
+	{0x69, 0xcd},
+	{0x70, 0x20},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x40},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x60},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x80},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0xa0},
+	{0x71, 0x70},
+	{0x72, 0x04},
+	{0x73, 0x00},
+	{0x70, 0x1f},
+	{0x76, 0x38},
+	{0x77, 0xa6},
+	{0x78, 0x0c},
+	{0x79, 0x80},
+	{0x7f, 0x14},
+	{0x85, 0x08},
+	{0xcd, 0xf4},
+	{0x90, 0x33},
+	{0x86, 0x00},
+	{0x87, 0x0f},
+	{0x89, 0x00},
+	{0x8b, 0x44},
+	{0x8c, 0x66},
+	{0x9d, 0xc1},
+	{0x8a, 0x10},
+	{0xad, 0x40},
+	{0xa0, 0x44},
+	{0xbe, 0x00},
+	{0xc0, 0x08},
+	{0xc1, 0x10},
+	{0xc2, 0x08},
+	{0xc3, 0x10},
+	{0xc4, 0x08},
+	{0xc5, 0xf0},
+	{0xc6, 0xff},
+	{0xc7, 0x00},
+	{0xc8, 0x1a},
+	{0xc9, 0x80},
+	{0xca, 0x23},
+	{0xcb, 0x24},
+	{0xcc, 0xf4},
Quoted 11 lines
Quoted 8 lines
drivers/media/dvb-frontends/m88ds3103_priv.h +6 -0
+	{0xce, 0x74},
+	{0x00, 0x00},
+	{0xbd, 0x01},
+	{0xb8, 0x00},
+	{0x29, 0x01},
+};
  #endif
Signature · 1 line
http://palosaari.fi/

BR,
Max

#3 lore raw

Re: [PATCH v2 3/3] DVBSky V3 PCIe card: add some changes to M88DS3103for supporting the demod of M88RS6000

Quoted 7 lines · Nibble Max
Quoted 4 lines

drivers/media/dvb-frontends/m88ds3103.c static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv, · line 165

Quoted 2 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -1
-	if (tab_len > 83) {
+	if (tab_len > 86) {

That is not nice, but I could try find better solution and fix it later.

What is the reason to check this parameter?
How about remove this check?

It is just to check you will not overwrite buffer accidentally. Buffer is 83 bytes long, which should be also increased...
The correct solution is somehow calculate max possible tab size on compile time. It should be possible as init tabs are static const tables. Use some macro to calculate max value and use it - not plain numbers.

Something like that
#define BUF_SIZE MAX(m88ds3103_tab_dvbs, m88ds3103_tab_dvbs2, m88rs6000_tab_dvbs, m88rs6000_tab_dvbs2)

Quoted 22 lines
Quoted 5 lines

Clock selection. What this does:
* select mclk_khz
* select target_mclk
* calls set_config() in order to pass target_mclk to tuner driver
* + some strange looking sleep, which is not likely needed

The clock of M88RS6000's demod comes from tuner dies.
So the first thing is turning on the demod main clock from tuner die after the demod reset. Without this clock, the following register's content will fail to update. Before changing the demod main clock, it should close clock path.
After changing the demod main clock, it open clock path and wait the clock to stable.

Quoted 7 lines

One thing what I don't like this is that you have implemented M88RS6000 things here and M88DS3103 elsewhere. Generally speaking, code must have some logic where same things are done in same place. So I expect to see both M88DS3103 and M88RS6000 target_mclk and mclk_khz selection implemented here or these moved to place where M88DS3103 implementation is.

I will move M88DS3103 implementation to here.

Quoted 7 lines
Quoted 5 lines

Also, even set_config() is somehow logically correctly used here, I prefer to duplicate that 4 line long target_mclk selection to tuner driver and get rid of whole set_config(). Even better solution could be to register M88RS6000 as a clock provider using clock framework, but imho it is not worth that simple case.

Do you suggest to set demod main clock and ts clock in tuner's set_params call back?

Yes, and you did it already on that latest patch, thanks. It is not logically correct, but a bit hackish solution, but I think it is best in that special case in order to keep things simple here.

One thing with that new patch I would like to check is this 10us delay after clock path is enabled. You enable clock just before mcu is stopped and demod is configured during mcu is on freeze. 10us is almost nothing and it sounds like having no need in a situation you stop even mcu. It is about one I2C command which will took longer than 10us. Hard to see why you need wait 10us to settle clock in such case. What happens if you don't wait? I assume nothing, it works just as it should as stable clocks are needed a long after that, when mcu is take out of reset.

regards
Antti

Signature · 1 line
http://palosaari.fi/
author
#4 lore raw

Re: Re: [PATCH v2 3/3] DVBSky V3 PCIe card: add some changes to M88DS3103forsupporting the demod of M88RS6000

In reply to Nibble Max

Hello Antti,

Quoted 60 lines · Antti Palosaari
Quoted 7 lines · Nibble Max
Quoted 4 lines

drivers/media/dvb-frontends/m88ds3103.c static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv, · line 165

Quoted 2 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -1
-	if (tab_len > 83) {
+	if (tab_len > 86) {

That is not nice, but I could try find better solution and fix it later.

What is the reason to check this parameter?
How about remove this check?

It is just to check you will not overwrite buffer accidentally. Buffer is 83 bytes long, which should be also increased...
The correct solution is somehow calculate max possible tab size on compile time. It should be possible as init tabs are static const tables. Use some macro to calculate max value and use it - not plain numbers.

Something like that
#define BUF_SIZE MAX(m88ds3103_tab_dvbs, m88ds3103_tab_dvbs2,
m88rs6000_tab_dvbs, m88rs6000_tab_dvbs2)

Quoted 22 lines
Quoted 5 lines

Clock selection. What this does:
* select mclk_khz
* select target_mclk
* calls set_config() in order to pass target_mclk to tuner driver
* + some strange looking sleep, which is not likely needed

The clock of M88RS6000's demod comes from tuner dies.
So the first thing is turning on the demod main clock from tuner die after the demod reset. Without this clock, the following register's content will fail to update. Before changing the demod main clock, it should close clock path.
After changing the demod main clock, it open clock path and wait the clock to stable.

Quoted 7 lines

One thing what I don't like this is that you have implemented M88RS6000 things here and M88DS3103 elsewhere. Generally speaking, code must have some logic where same things are done in same place. So I expect to see both M88DS3103 and M88RS6000 target_mclk and mclk_khz selection implemented here or these moved to place where M88DS3103 implementation is.

I will move M88DS3103 implementation to here.

Quoted 7 lines
Quoted 5 lines

Also, even set_config() is somehow logically correctly used here, I prefer to duplicate that 4 line long target_mclk selection to tuner driver and get rid of whole set_config(). Even better solution could be to register M88RS6000 as a clock provider using clock framework, but imho it is not worth that simple case.

Do you suggest to set demod main clock and ts clock in tuner's set_params call back?

Yes, and you did it already on that latest patch, thanks. It is not logically correct, but a bit hackish solution, but I think it is best in that special case in order to keep things simple here.

Quoted 9 lines

One thing with that new patch I would like to check is this 10us delay after clock path is enabled. You enable clock just before mcu is stopped and demod is configured during mcu is on freeze. 10us is almost nothing and it sounds like having no need in a situation you stop even mcu. It is about one I2C command which will took longer than 10us. Hard to see why you need wait 10us to settle clock in such case. What happens if you don't wait? I assume nothing, it works just as it should as stable clocks are needed a long after that, when mcu is take out of reset.

usleep_range(10000, 20000);
This delay time at least is 10ms, not 10us.

Quoted 5 lines

regards
Antti

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

BR,
Max

#5 lore raw

Re: [PATCH v2 3/3] DVBSky V3 PCIe card: add some changes to M88DS3103forsupporting the demod of M88RS6000

Quoted 73 lines · Nibble Max

Hello Antti,

Quoted 60 lines · Antti Palosaari
Quoted 7 lines · Nibble Max
Quoted 4 lines

drivers/media/dvb-frontends/m88ds3103.c static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv, · line 165

Quoted 2 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -1
-	if (tab_len > 83) {
+	if (tab_len > 86) {

That is not nice, but I could try find better solution and fix it later.

What is the reason to check this parameter?
How about remove this check?

It is just to check you will not overwrite buffer accidentally. Buffer is 83 bytes long, which should be also increased...
The correct solution is somehow calculate max possible tab size on compile time. It should be possible as init tabs are static const tables. Use some macro to calculate max value and use it - not plain numbers.

Something like that
#define BUF_SIZE MAX(m88ds3103_tab_dvbs, m88ds3103_tab_dvbs2,
m88rs6000_tab_dvbs, m88rs6000_tab_dvbs2)

Quoted 22 lines
Quoted 5 lines

Clock selection. What this does:
* select mclk_khz
* select target_mclk
* calls set_config() in order to pass target_mclk to tuner driver
* + some strange looking sleep, which is not likely needed

The clock of M88RS6000's demod comes from tuner dies.
So the first thing is turning on the demod main clock from tuner die after the demod reset. Without this clock, the following register's content will fail to update. Before changing the demod main clock, it should close clock path.
After changing the demod main clock, it open clock path and wait the clock to stable.

Quoted 7 lines

One thing what I don't like this is that you have implemented M88RS6000 things here and M88DS3103 elsewhere. Generally speaking, code must have some logic where same things are done in same place. So I expect to see both M88DS3103 and M88RS6000 target_mclk and mclk_khz selection implemented here or these moved to place where M88DS3103 implementation is.

I will move M88DS3103 implementation to here.

Quoted 7 lines
Quoted 5 lines

Also, even set_config() is somehow logically correctly used here, I prefer to duplicate that 4 line long target_mclk selection to tuner driver and get rid of whole set_config(). Even better solution could be to register M88RS6000 as a clock provider using clock framework, but imho it is not worth that simple case.

Do you suggest to set demod main clock and ts clock in tuner's set_params call back?

Yes, and you did it already on that latest patch, thanks. It is not logically correct, but a bit hackish solution, but I think it is best in that special case in order to keep things simple here.

Quoted 9 lines

One thing with that new patch I would like to check is this 10us delay after clock path is enabled. You enable clock just before mcu is stopped and demod is configured during mcu is on freeze. 10us is almost nothing and it sounds like having no need in a situation you stop even mcu. It is about one I2C command which will took longer than 10us. Hard to see why you need wait 10us to settle clock in such case. What happens if you don't wait? I assume nothing, it works just as it should as stable clocks are needed a long after that, when mcu is take out of reset.

Quoted 2 lines

usleep_range(10000, 20000);
This delay time at least is 10ms, not 10us.

ah, yes, you were correct. 10ms is indeed a much larger, it is about century on a digital logic signal path where frequency is around 100MHz. 100MHz clock means clock cycle is 10ns, 10ms is 10000000ns => 1,000,000 - one million clock cycles. Whilst I don't know how chip designed in a logic level, I have still done some digital logic designing myself and this sounds long. If you don't enable clock path, what is next command which will fail? Probably it will not even fail, but never lock to signal as demod core is not clocked at all.

regards
Antti

Signature · 1 line
http://palosaari.fi/
#6 lore raw

Re: [PATCH v2 3/3] DVBSky V3 PCIe card: add some changes to M88DS3103forsupporting the demod of M88RS6000

Quoted 89 lines · Antti Palosaari
Quoted 83 lines · Nibble Max

Hello Antti,

Quoted 70 lines · Antti Palosaari
Quoted 8 lines · Nibble Max
Quoted 5 lines

drivers/media/dvb-frontends/m88ds3103.c static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv, · line 165

Quoted 2 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -1
-    if (tab_len > 83) {
+    if (tab_len > 86) {

That is not nice, but I could try find better solution and fix it later.

What is the reason to check this parameter?
How about remove this check?

It is just to check you will not overwrite buffer accidentally. Buffer is 83 bytes long, which should be also increased...
The correct solution is somehow calculate max possible tab size on compile time. It should be possible as init tabs are static const tables. Use some macro to calculate max value and use it - not plain numbers.

Something like that
#define BUF_SIZE MAX(m88ds3103_tab_dvbs, m88ds3103_tab_dvbs2,
m88rs6000_tab_dvbs, m88rs6000_tab_dvbs2)

Quoted 36 lines
Quoted 5 lines

Clock selection. What this does:
* select mclk_khz
* select target_mclk
* calls set_config() in order to pass target_mclk to tuner driver
* + some strange looking sleep, which is not likely needed

The clock of M88RS6000's demod comes from tuner dies.
So the first thing is turning on the demod main clock from tuner die after the demod reset.
Without this clock, the following register's content will fail to update.
Before changing the demod main clock, it should close clock path. After changing the demod main clock, it open clock path and wait the clock to stable.

Quoted 11 lines

One thing what I don't like this is that you have implemented M88RS6000
things here and M88DS3103 elsewhere. Generally speaking, code must have
some logic where same things are done in same place. So I expect to see
both M88DS3103 and M88RS6000 target_mclk and mclk_khz selection implemented here or these moved to place where M88DS3103 implementation is.

I will move M88DS3103 implementation to here.

Quoted 6 lines

Also, even set_config() is somehow logically correctly used here, I prefer to duplicate that 4 line long target_mclk selection to tuner driver and get rid of whole set_config(). Even better solution could be
to register M88RS6000 as a clock provider using clock framework, but imho it is not worth that simple case.

Quoted 2 lines

Do you suggest to set demod main clock and ts clock in tuner's set_params call back?

Yes, and you did it already on that latest patch, thanks. It is not logically correct, but a bit hackish solution, but I think it is best in that special case in order to keep things simple here.

Quoted 9 lines

One thing with that new patch I would like to check is this 10us delay after clock path is enabled. You enable clock just before mcu is stopped and demod is configured during mcu is on freeze. 10us is almost nothing and it sounds like having no need in a situation you stop even mcu. It is about one I2C command which will took longer than 10us. Hard to see why you need wait 10us to settle clock in such case. What happens if you don't wait? I assume nothing, it works just as it should as stable clocks are needed a long after that, when mcu is take out of reset.

Quoted 2 lines

usleep_range(10000, 20000);
This delay time at least is 10ms, not 10us.

Quoted 9 lines

ah, yes, you were correct. 10ms is indeed a much larger, it is about century on a digital logic signal path where frequency is around 100MHz. 100MHz clock means clock cycle is 10ns, 10ms is 10000000ns => 1,000,000
- one million clock cycles. Whilst I don't know how chip designed in a logic level, I have still done some digital logic designing myself and this sounds long.
If you don't enable clock path, what is next command which will fail? Probably it will not even fail, but never lock to signal as demod core is not clocked at all.

But if you want keep that delay then keep it. For my eyes it sounds weird to wait that long after clock path is enabled. I have feeling it is clock which is needed much later, but I may be wrong and I cannot even make any tests without hardware. It is also possible that master clock is needed in order to perform all the next commands.

regards
Antti

Signature · 1 line
http://palosaari.fi/
author
#7 lore raw

Re: Re: [PATCH v2 3/3] DVBSky V3 PCIe card: add some changes to M88DS3103forsupportingthe demod of M88RS6000

In reply to Antti Palosaari

Hello Antti,

Quoted 102 lines · Antti Palosaari
Quoted 89 lines · Antti Palosaari
Quoted 83 lines · Nibble Max

Hello Antti,

Quoted 70 lines · Antti Palosaari
Quoted 8 lines · Nibble Max
Quoted 5 lines

drivers/media/dvb-frontends/m88ds3103.c static int m88ds3103_wr_reg_val_tab(struct m88ds3103_priv *priv, · line 165

Quoted 2 lines
drivers/media/dvb-frontends/m88ds3103.c +1 -1
-    if (tab_len > 83) {
+    if (tab_len > 86) {

That is not nice, but I could try find better solution and fix it later.

What is the reason to check this parameter?
How about remove this check?

It is just to check you will not overwrite buffer accidentally. Buffer is 83 bytes long, which should be also increased...
The correct solution is somehow calculate max possible tab size on compile time. It should be possible as init tabs are static const tables. Use some macro to calculate max value and use it - not plain numbers.

Something like that
#define BUF_SIZE MAX(m88ds3103_tab_dvbs, m88ds3103_tab_dvbs2,
m88rs6000_tab_dvbs, m88rs6000_tab_dvbs2)

Quoted 36 lines
Quoted 5 lines

Clock selection. What this does:
* select mclk_khz
* select target_mclk
* calls set_config() in order to pass target_mclk to tuner driver
* + some strange looking sleep, which is not likely needed

The clock of M88RS6000's demod comes from tuner dies.
So the first thing is turning on the demod main clock from tuner die after the demod reset.
Without this clock, the following register's content will fail to update.
Before changing the demod main clock, it should close clock path. After changing the demod main clock, it open clock path and wait the clock to stable.

Quoted 11 lines

One thing what I don't like this is that you have implemented M88RS6000
things here and M88DS3103 elsewhere. Generally speaking, code must have
some logic where same things are done in same place. So I expect to see
both M88DS3103 and M88RS6000 target_mclk and mclk_khz selection implemented here or these moved to place where M88DS3103 implementation is.

I will move M88DS3103 implementation to here.

Quoted 6 lines

Also, even set_config() is somehow logically correctly used here, I prefer to duplicate that 4 line long target_mclk selection to tuner driver and get rid of whole set_config(). Even better solution could be
to register M88RS6000 as a clock provider using clock framework, but imho it is not worth that simple case.

Quoted 2 lines

Do you suggest to set demod main clock and ts clock in tuner's set_params call back?

Yes, and you did it already on that latest patch, thanks. It is not logically correct, but a bit hackish solution, but I think it is best in that special case in order to keep things simple here.

Quoted 9 lines

One thing with that new patch I would like to check is this 10us delay after clock path is enabled. You enable clock just before mcu is stopped and demod is configured during mcu is on freeze. 10us is almost nothing and it sounds like having no need in a situation you stop even mcu. It is about one I2C command which will took longer than 10us. Hard to see why you need wait 10us to settle clock in such case. What happens if you don't wait? I assume nothing, it works just as it should as stable clocks are needed a long after that, when mcu is take out of reset.

Quoted 2 lines

usleep_range(10000, 20000);
This delay time at least is 10ms, not 10us.

Quoted 9 lines

ah, yes, you were correct. 10ms is indeed a much larger, it is about century on a digital logic signal path where frequency is around 100MHz. 100MHz clock means clock cycle is 10ns, 10ms is 10000000ns => 1,000,000
- one million clock cycles. Whilst I don't know how chip designed in a logic level, I have still done some digital logic designing myself and this sounds long.
If you don't enable clock path, what is next command which will fail? Probably it will not even fail, but never lock to signal as demod core is not clocked at all.

Quoted 6 lines

But if you want keep that delay then keep it. For my eyes it sounds weird to wait that long after clock path is enabled. I have feeling it is clock which is needed much later, but I may be wrong and I cannot even make any tests without hardware. It is also possible that master clock is needed in order to perform all the next commands.

If don't enable clock path, the next I2C commands looks ok, no error message. But the demod can not lock to the signal any more.
I am not the chip designer so I do not know the exact detail information of this internal logic.

Quoted 5 lines

regards
Antti

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

BR,
Max