[PATCH 1/3] e4000: convert DVB tuner to I2C driver model

[PATCH 1/3] e4000: convert DVB tuner to I2C driver model

[Antti Palosaari]

Driver conversion from proprietary DVB tuner model to more
general I2C driver model.

Cc: Jean Delvare <khali@linux-fr.org>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/tuners/e4000.c            | 115 ++++++++++++++++++++------------
 drivers/media/tuners/e4000.h            |  21 ++----
 drivers/media/tuners/e4000_priv.h       |   5 +-
 drivers/media/usb/dvb-usb-v2/rtl28xxu.c |  41 +++++++++---
 drivers/media/usb/dvb-usb-v2/rtl28xxu.h |   1 +
 5 files changed, 111 insertions(+), 72 deletions(-)

diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 40c1da7..0153169 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -31,7 +31,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
 	u8 buf[MAX_XFER_SIZE];
 	struct i2c_msg msg[1] = {
 		{
-			.addr = priv->cfg->i2c_addr,
+			.addr = priv->client->addr,
 			.flags = 0,
 			.len = 1 + len,
 			.buf = buf,
@@ -39,7 +39,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
 	};
 
 	if (1 + len > sizeof(buf)) {
-		dev_warn(&priv->i2c->dev,
+		dev_warn(&priv->client->dev,
 			 "%s: i2c wr reg=%04x: len=%d is too big!\n",
 			 KBUILD_MODNAME, reg, len);
 		return -EINVAL;
@@ -48,11 +48,11 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
 	buf[0] = reg;
 	memcpy(&buf[1], val, len);
 
-	ret = i2c_transfer(priv->i2c, msg, 1);
+	ret = i2c_transfer(priv->client->adapter, msg, 1);
 	if (ret == 1) {
 		ret = 0;
 	} else {
-		dev_warn(&priv->i2c->dev,
+		dev_warn(&priv->client->dev,
 				"%s: i2c wr failed=%d reg=%02x len=%d\n",
 				KBUILD_MODNAME, ret, reg, len);
 		ret = -EREMOTEIO;
@@ -67,12 +67,12 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
 	u8 buf[MAX_XFER_SIZE];
 	struct i2c_msg msg[2] = {
 		{
-			.addr = priv->cfg->i2c_addr,
+			.addr = priv->client->addr,
 			.flags = 0,
 			.len = 1,
 			.buf = &reg,
 		}, {
-			.addr = priv->cfg->i2c_addr,
+			.addr = priv->client->addr,
 			.flags = I2C_M_RD,
 			.len = len,
 			.buf = buf,
@@ -80,18 +80,18 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
 	};
 
 	if (len > sizeof(buf)) {
-		dev_warn(&priv->i2c->dev,
+		dev_warn(&priv->client->dev,
 			 "%s: i2c rd reg=%04x: len=%d is too big!\n",
 			 KBUILD_MODNAME, reg, len);
 		return -EINVAL;
 	}
 
-	ret = i2c_transfer(priv->i2c, msg, 2);
+	ret = i2c_transfer(priv->client->adapter, msg, 2);
 	if (ret == 2) {
 		memcpy(val, buf, len);
 		ret = 0;
 	} else {
-		dev_warn(&priv->i2c->dev,
+		dev_warn(&priv->client->dev,
 				"%s: i2c rd failed=%d reg=%02x len=%d\n",
 				KBUILD_MODNAME, ret, reg, len);
 		ret = -EREMOTEIO;
@@ -117,7 +117,7 @@ static int e4000_init(struct dvb_frontend *fe)
 	struct e4000_priv *priv = fe->tuner_priv;
 	int ret;
 
-	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);
+	dev_dbg(&priv->client->dev, "%s:\n", __func__);
 
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 1);
@@ -186,7 +186,7 @@ err:
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 0);
 
-	dev_dbg(&priv->i2c->dev, "%s: failed=%d\n", __func__, ret);
+	dev_dbg(&priv->client->dev, "%s: failed=%d\n", __func__, ret);
 	return ret;
 }
 
@@ -195,7 +195,7 @@ static int e4000_sleep(struct dvb_frontend *fe)
 	struct e4000_priv *priv = fe->tuner_priv;
 	int ret;
 
-	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);
+	dev_dbg(&priv->client->dev, "%s:\n", __func__);
 
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 1);
@@ -212,7 +212,7 @@ err:
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 0);
 
-	dev_dbg(&priv->i2c->dev, "%s: failed=%d\n", __func__, ret);
+	dev_dbg(&priv->client->dev, "%s: failed=%d\n", __func__, ret);
 	return ret;
 }
 
@@ -224,7 +224,7 @@ static int e4000_set_params(struct dvb_frontend *fe)
 	unsigned int f_vco;
 	u8 buf[5], i_data[4], q_data[4];
 
-	dev_dbg(&priv->i2c->dev,
+	dev_dbg(&priv->client->dev,
 			"%s: delivery_system=%d frequency=%d bandwidth_hz=%d\n",
 			__func__, c->delivery_system, c->frequency,
 			c->bandwidth_hz);
@@ -253,14 +253,15 @@ static int e4000_set_params(struct dvb_frontend *fe)
 	 * or more.
 	 */
 	f_vco = c->frequency * e4000_pll_lut[i].mul;
-	sigma_delta = div_u64(0x10000ULL * (f_vco % priv->cfg->clock), priv->cfg->clock);
-	buf[0] = f_vco / priv->cfg->clock;
+	sigma_delta = div_u64(0x10000ULL * (f_vco % priv->clock), priv->clock);
+	buf[0] = f_vco / priv->clock;
 	buf[1] = (sigma_delta >> 0) & 0xff;
 	buf[2] = (sigma_delta >> 8) & 0xff;
 	buf[3] = 0x00;
 	buf[4] = e4000_pll_lut[i].div;
 
-	dev_dbg(&priv->i2c->dev, "%s: f_vco=%u pll div=%d sigma_delta=%04x\n",
+	dev_dbg(&priv->client->dev,
+			"%s: f_vco=%u pll div=%d sigma_delta=%04x\n",
 			__func__, f_vco, buf[0], sigma_delta);
 
 	ret = e4000_wr_regs(priv, 0x09, buf, 5);
@@ -369,7 +370,7 @@ err:
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 0);
 
-	dev_dbg(&priv->i2c->dev, "%s: failed=%d\n", __func__, ret);
+	dev_dbg(&priv->client->dev, "%s: failed=%d\n", __func__, ret);
 	return ret;
 }
 
@@ -377,24 +378,13 @@ static int e4000_get_if_frequency(struct dvb_frontend *fe, u32 *frequency)
 {
 	struct e4000_priv *priv = fe->tuner_priv;
 
-	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);
+	dev_dbg(&priv->client->dev, "%s:\n", __func__);
 
 	*frequency = 0; /* Zero-IF */
 
 	return 0;
 }
 
-static int e4000_release(struct dvb_frontend *fe)
-{
-	struct e4000_priv *priv = fe->tuner_priv;
-
-	dev_dbg(&priv->i2c->dev, "%s:\n", __func__);
-
-	kfree(fe->tuner_priv);
-
-	return 0;
-}
-
 static const struct dvb_tuner_ops e4000_tuner_ops = {
 	.info = {
 		.name           = "Elonics E4000",
@@ -402,8 +392,6 @@ static const struct dvb_tuner_ops e4000_tuner_ops = {
 		.frequency_max  = 862000000,
 	},
 
-	.release = e4000_release,
-
 	.init = e4000_init,
 	.sleep = e4000_sleep,
 	.set_params = e4000_set_params,
@@ -411,9 +399,11 @@ static const struct dvb_tuner_ops e4000_tuner_ops = {
 	.get_if_frequency = e4000_get_if_frequency,
 };
 
-struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
-		struct i2c_adapter *i2c, const struct e4000_config *cfg)
+static int e4000_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
 {
+	struct e4000_config *cfg = client->dev.platform_data;
+	struct dvb_frontend *fe = cfg->fe;
 	struct e4000_priv *priv;
 	int ret;
 	u8 chip_id;
@@ -424,29 +414,33 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
 	priv = kzalloc(sizeof(struct e4000_priv), GFP_KERNEL);
 	if (!priv) {
 		ret = -ENOMEM;
-		dev_err(&i2c->dev, "%s: kzalloc() failed\n", KBUILD_MODNAME);
+		dev_err(&client->dev, "%s: kzalloc() failed\n", KBUILD_MODNAME);
 		goto err;
 	}
 
-	priv->cfg = cfg;
-	priv->i2c = i2c;
+	priv->clock = cfg->clock;
+	priv->client = client;
+	priv->fe = cfg->fe;
 
 	/* check if the tuner is there */
 	ret = e4000_rd_reg(priv, 0x02, &chip_id);
 	if (ret < 0)
 		goto err;
 
-	dev_dbg(&priv->i2c->dev, "%s: chip_id=%02x\n", __func__, chip_id);
+	dev_dbg(&priv->client->dev,
+			"%s: chip_id=%02x\n", __func__, chip_id);
 
-	if (chip_id != 0x40)
+	if (chip_id != 0x40) {
+		ret = -ENODEV;
 		goto err;
+	}
 
 	/* put sleep as chip seems to be in normal mode by default */
 	ret = e4000_wr_reg(priv, 0x00, 0x00);
 	if (ret < 0)
 		goto err;
 
-	dev_info(&priv->i2c->dev,
+	dev_info(&priv->client->dev,
 			"%s: Elonics E4000 successfully identified\n",
 			KBUILD_MODNAME);
 
@@ -457,16 +451,49 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 0);
 
-	return fe;
+	i2c_set_clientdata(client, priv);
+
+	return 0;
 err:
 	if (fe->ops.i2c_gate_ctrl)
 		fe->ops.i2c_gate_ctrl(fe, 0);
 
-	dev_dbg(&i2c->dev, "%s: failed=%d\n", __func__, ret);
+	dev_dbg(&client->dev, "%s: failed=%d\n", __func__, ret);
 	kfree(priv);
-	return NULL;
+	return ret;
 }
-EXPORT_SYMBOL(e4000_attach);
+
+static int e4000_remove(struct i2c_client *client)
+{
+	struct e4000_priv *priv = i2c_get_clientdata(client);
+	struct dvb_frontend *fe = priv->fe;
+
+	dev_dbg(&client->dev, "%s:\n", __func__);
+
+	memset(&fe->ops.tuner_ops, 0, sizeof(struct dvb_tuner_ops));
+	fe->tuner_priv = NULL;
+	kfree(priv);
+
+	return 0;
+}
+
+static const struct i2c_device_id e4000_id[] = {
+	{"e4000", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, e4000_id);
+
+static struct i2c_driver e4000_driver = {
+	.driver = {
+		.owner	= THIS_MODULE,
+		.name	= "e4000",
+	},
+	.probe		= e4000_probe,
+	.remove		= e4000_remove,
+	.id_table	= e4000_id,
+};
+
+module_i2c_driver(e4000_driver);
 
 MODULE_DESCRIPTION("Elonics E4000 silicon tuner driver");
 MODULE_AUTHOR("Antti Palosaari <crope@iki.fi>");
diff --git a/drivers/media/tuners/e4000.h b/drivers/media/tuners/e4000.h
index 25ee7c0..e74b8b2 100644
--- a/drivers/media/tuners/e4000.h
+++ b/drivers/media/tuners/e4000.h
@@ -24,12 +24,15 @@
 #include <linux/kconfig.h>
 #include "dvb_frontend.h"
 
+/*
+ * I2C address
+ * 0x64, 0x65, 0x66, 0x67
+ */
 struct e4000_config {
 	/*
-	 * I2C address
-	 * 0x64, 0x65, 0x66, 0x67
+	 * frontend
 	 */
-	u8 i2c_addr;
+	struct dvb_frontend *fe;
 
 	/*
 	 * clock
@@ -37,16 +40,4 @@ struct e4000_config {
 	u32 clock;
 };
 
-#if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
-extern struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
-		struct i2c_adapter *i2c, const struct e4000_config *cfg);
-#else
-static inline struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
-		struct i2c_adapter *i2c, const struct e4000_config *cfg)
-{
-	dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n", __func__);
-	return NULL;
-}
-#endif
-
 #endif
diff --git a/drivers/media/tuners/e4000_priv.h b/drivers/media/tuners/e4000_priv.h
index a385505..8f45a30 100644
--- a/drivers/media/tuners/e4000_priv.h
+++ b/drivers/media/tuners/e4000_priv.h
@@ -24,8 +24,9 @@
 #include "e4000.h"
 
 struct e4000_priv {
-	const struct e4000_config *cfg;
-	struct i2c_adapter *i2c;
+	struct i2c_client *client;
+	u32 clock;
+	struct dvb_frontend *fe;
 };
 
 struct e4000_pll {
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index 77f1fc9..76cf0de 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -857,11 +857,6 @@ err:
 	return ret;
 }
 
-static const struct e4000_config rtl2832u_e4000_config = {
-	.i2c_addr = 0x64,
-	.clock = 28800000,
-};
-
 static const struct fc2580_config rtl2832u_fc2580_config = {
 	.i2c_addr = 0x56,
 	.clock = 16384000,
@@ -895,10 +890,13 @@ static int rtl2832u_tuner_attach(struct dvb_usb_adapter *adap)
 	int ret;
 	struct dvb_usb_device *d = adap_to_d(adap);
 	struct rtl28xxu_priv *priv = d_to_priv(d);
-	struct dvb_frontend *fe;
+	struct dvb_frontend *fe = NULL;
+	struct i2c_board_info info;
 
 	dev_dbg(&d->udev->dev, "%s:\n", __func__);
 
+	memset(&info, 0, sizeof(struct i2c_board_info));
+
 	switch (priv->tuner) {
 	case TUNER_RTL2832_FC0012:
 		fe = dvb_attach(fc0012_attach, adap->fe[0],
@@ -918,9 +916,19 @@ static int rtl2832u_tuner_attach(struct dvb_usb_adapter *adap)
 		adap->fe[0]->ops.read_signal_strength =
 				adap->fe[0]->ops.tuner_ops.get_rf_strength;
 		return 0;
-	case TUNER_RTL2832_E4000:
-		fe = dvb_attach(e4000_attach, adap->fe[0], &d->i2c_adap,
-				&rtl2832u_e4000_config);
+	case TUNER_RTL2832_E4000: {
+			struct e4000_config e4000_config = {
+				.fe = adap->fe[0],
+				.clock = 28800000,
+			};
+
+			strlcpy(info.type, "e4000", I2C_NAME_SIZE);
+			info.addr = 0x64;
+			info.platform_data = &e4000_config;
+
+			request_module("e4000");
+			priv->client = i2c_new_device(&d->i2c_adap, &info);
+		}
 		break;
 	case TUNER_RTL2832_FC2580:
 		fe = dvb_attach(fc2580_attach, adap->fe[0], &d->i2c_adap,
@@ -969,12 +977,11 @@ static int rtl2832u_tuner_attach(struct dvb_usb_adapter *adap)
 				adap->fe[0]->ops.tuner_ops.get_rf_strength;
 		break;
 	default:
-		fe = NULL;
 		dev_err(&d->udev->dev, "%s: unknown tuner=%d\n", KBUILD_MODNAME,
 				priv->tuner);
 	}
 
-	if (fe == NULL) {
+	if (fe == NULL && priv->client == NULL) {
 		ret = -ENODEV;
 		goto err;
 	}
@@ -1019,6 +1026,17 @@ err:
 	return ret;
 }
 
+static void rtl28xxu_exit(struct dvb_usb_device *d)
+{
+	struct rtl28xxu_priv *priv = d->priv;
+
+	dev_dbg(&d->udev->dev, "%s:\n", __func__);
+
+	i2c_release_client(priv->client);
+
+	return;
+}
+
 static int rtl2831u_power_ctrl(struct dvb_usb_device *d, int onoff)
 {
 	int ret;
@@ -1381,6 +1399,7 @@ static const struct dvb_usb_device_properties rtl2832u_props = {
 	.frontend_attach = rtl2832u_frontend_attach,
 	.tuner_attach = rtl2832u_tuner_attach,
 	.init = rtl28xxu_init,
+	.exit = rtl28xxu_exit,
 	.get_rc_config = rtl2832u_get_rc_config,
 
 	.num_adapters = 1,
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
index 2142bcb..367aca1 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
@@ -56,6 +56,7 @@ struct rtl28xxu_priv {
 	char *tuner_name;
 	u8 page; /* integrated demod active register page */
 	bool rc_active;
+	struct i2c_client *client;
 };
 
 enum rtl28xxu_chip_id {
-- 
1.8.5.3

[PATCH 2/3] e4000: add manual gain controls

[Antti Palosaari]

Add gain control for LNA, Mixer and IF. Expose controls via DVB
frontend .set_config callback.

Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/tuners/e4000.c      | 68 +++++++++++++++++++++++++++++++++++++++
 drivers/media/tuners/e4000.h      |  6 ++++
 drivers/media/tuners/e4000_priv.h | 63 ++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+)

diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 0153169..651de11 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -385,6 +385,73 @@ static int e4000_get_if_frequency(struct dvb_frontend *fe, u32 *frequency)
 	return 0;
 }
 
+static int e4000_set_config(struct dvb_frontend *fe, void *priv_cfg)
+{
+	struct e4000_priv *priv = fe->tuner_priv;
+	struct e4000_ctrl *ctrl = priv_cfg;
+	int ret;
+	u8 buf[2];
+	u8 u8tmp;
+	dev_dbg(&priv->client->dev, "%s: lna=%d mixer=%d if=%d\n", __func__,
+			ctrl->lna_gain, ctrl->mixer_gain, ctrl->if_gain);
+
+	if (fe->ops.i2c_gate_ctrl)
+		fe->ops.i2c_gate_ctrl(fe, 1);
+
+	if (ctrl->lna_gain == INT_MIN && ctrl->if_gain == INT_MIN)
+		u8tmp = 0x17;
+	else if (ctrl->lna_gain == INT_MIN)
+		u8tmp = 0x19;
+	else if (ctrl->if_gain == INT_MIN)
+		u8tmp = 0x16;
+	else
+		u8tmp = 0x10;
+
+	ret = e4000_wr_reg(priv, 0x1a, u8tmp);
+	if (ret)
+		goto err;
+
+	if (ctrl->mixer_gain == INT_MIN)
+		u8tmp = 0x15;
+	else
+		u8tmp = 0x14;
+
+	ret = e4000_wr_reg(priv, 0x20, u8tmp);
+	if (ret)
+		goto err;
+
+	if (ctrl->lna_gain != INT_MIN) {
+		ret = e4000_wr_reg(priv, 0x14, ctrl->lna_gain);
+		if (ret)
+			goto err;
+	}
+
+	if (ctrl->mixer_gain != INT_MIN) {
+		ret = e4000_wr_reg(priv, 0x15, ctrl->mixer_gain);
+		if (ret)
+			goto err;
+	}
+
+	if (ctrl->if_gain != INT_MIN) {
+		buf[0] = e4000_if_gain_lut[ctrl->if_gain].reg16_val;
+		buf[1] = e4000_if_gain_lut[ctrl->if_gain].reg17_val;
+		ret = e4000_wr_regs(priv, 0x16, buf, 2);
+		if (ret)
+			goto err;
+	}
+
+	if (fe->ops.i2c_gate_ctrl)
+		fe->ops.i2c_gate_ctrl(fe, 0);
+
+	return 0;
+err:
+	if (fe->ops.i2c_gate_ctrl)
+		fe->ops.i2c_gate_ctrl(fe, 0);
+
+	dev_dbg(&priv->client->dev, "%s: failed=%d\n", __func__, ret);
+	return ret;
+}
+
 static const struct dvb_tuner_ops e4000_tuner_ops = {
 	.info = {
 		.name           = "Elonics E4000",
@@ -395,6 +462,7 @@ static const struct dvb_tuner_ops e4000_tuner_ops = {
 	.init = e4000_init,
 	.sleep = e4000_sleep,
 	.set_params = e4000_set_params,
+	.set_config = e4000_set_config,
 
 	.get_if_frequency = e4000_get_if_frequency,
 };
diff --git a/drivers/media/tuners/e4000.h b/drivers/media/tuners/e4000.h
index e74b8b2..d95c472 100644
--- a/drivers/media/tuners/e4000.h
+++ b/drivers/media/tuners/e4000.h
@@ -40,4 +40,10 @@ struct e4000_config {
 	u32 clock;
 };
 
+struct e4000_ctrl {
+	int lna_gain;
+	int mixer_gain;
+	int if_gain;
+};
+
 #endif
diff --git a/drivers/media/tuners/e4000_priv.h b/drivers/media/tuners/e4000_priv.h
index 8f45a30..a75a383 100644
--- a/drivers/media/tuners/e4000_priv.h
+++ b/drivers/media/tuners/e4000_priv.h
@@ -145,4 +145,67 @@ static const struct e4000_if_filter e4000_if_filter_lut[] = {
 	{ 0xffffffff, 0x00, 0x20 },
 };
 
+struct e4000_if_gain {
+	u8 reg16_val;
+	u8 reg17_val;
+};
+
+static const struct e4000_if_gain e4000_if_gain_lut[] = {
+	{0x00, 0x00},
+	{0x20, 0x00},
+	{0x40, 0x00},
+	{0x02, 0x00},
+	{0x22, 0x00},
+	{0x42, 0x00},
+	{0x04, 0x00},
+	{0x24, 0x00},
+	{0x44, 0x00},
+	{0x01, 0x00},
+	{0x21, 0x00},
+	{0x41, 0x00},
+	{0x03, 0x00},
+	{0x23, 0x00},
+	{0x43, 0x00},
+	{0x05, 0x00},
+	{0x25, 0x00},
+	{0x45, 0x00},
+	{0x07, 0x00},
+	{0x27, 0x00},
+	{0x47, 0x00},
+	{0x0f, 0x00},
+	{0x2f, 0x00},
+	{0x4f, 0x00},
+	{0x17, 0x00},
+	{0x37, 0x00},
+	{0x57, 0x00},
+	{0x1f, 0x00},
+	{0x3f, 0x00},
+	{0x5f, 0x00},
+	{0x1f, 0x01},
+	{0x3f, 0x01},
+	{0x5f, 0x01},
+	{0x1f, 0x02},
+	{0x3f, 0x02},
+	{0x5f, 0x02},
+	{0x1f, 0x03},
+	{0x3f, 0x03},
+	{0x5f, 0x03},
+	{0x1f, 0x04},
+	{0x3f, 0x04},
+	{0x5f, 0x04},
+	{0x1f, 0x0c},
+	{0x3f, 0x0c},
+	{0x5f, 0x0c},
+	{0x1f, 0x14},
+	{0x3f, 0x14},
+	{0x5f, 0x14},
+	{0x1f, 0x1c},
+	{0x3f, 0x1c},
+	{0x5f, 0x1c},
+	{0x1f, 0x24},
+	{0x3f, 0x24},
+	{0x5f, 0x24},
+	{0x7f, 0x24},
+};
+
 #endif
-- 
1.8.5.3

[PATCH 3/3] rtl2832_sdr: expose E4000 gain controls to user space

[Antti Palosaari]

Provide E4000 gain controls to userspace via V4L2 API. LNA, Mixer
and IF gain controls are offered, each one both manual and automode.

Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/staging/media/rtl2832u_sdr/Makefile      |   1 +
 drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c | 115 +++++++++++++++++------
 2 files changed, 88 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/media/rtl2832u_sdr/Makefile b/drivers/staging/media/rtl2832u_sdr/Makefile
index 1009276..7e00a0d 100644
--- a/drivers/staging/media/rtl2832u_sdr/Makefile
+++ b/drivers/staging/media/rtl2832u_sdr/Makefile
@@ -2,4 +2,5 @@ obj-$(CONFIG_DVB_RTL2832_SDR) += rtl2832_sdr.o
 
 ccflags-y += -Idrivers/media/dvb-core
 ccflags-y += -Idrivers/media/dvb-frontends
+ccflags-y += -Idrivers/media/tuners
 ccflags-y += -Idrivers/media/usb/dvb-usb-v2
diff --git a/drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c b/drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c
index fccb16f..ee72233 100644
--- a/drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c
+++ b/drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c
@@ -25,6 +25,7 @@
 #include "dvb_frontend.h"
 #include "rtl2832_sdr.h"
 #include "dvb_usb.h"
+#include "e4000.h"
 
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
@@ -147,9 +148,14 @@ struct rtl2832_sdr_state {
 	u32 pixelformat;
 
 	/* Controls */
-	struct v4l2_ctrl_handler ctrl_handler;
+	struct v4l2_ctrl_handler hdl;
+	struct v4l2_ctrl *lna_gain_auto;
+	struct v4l2_ctrl *lna_gain;
+	struct v4l2_ctrl *mixer_gain_auto;
+	struct v4l2_ctrl *mixer_gain;
+	struct v4l2_ctrl *if_gain_auto;
+	struct v4l2_ctrl *if_gain;
 	struct v4l2_ctrl *ctrl_tuner_bw;
-	struct v4l2_ctrl *ctrl_tuner_gain;
 
 	/* for sample rate calc */
 	unsigned int sample;
@@ -917,10 +923,49 @@ err:
 	return;
 };
 
+static int rtl2832_sdr_set_gain_e4000(struct rtl2832_sdr_state *s)
+{
+	int ret;
+	struct dvb_frontend *fe = s->fe;
+	struct e4000_ctrl ctrl;
+	dev_dbg(&s->udev->dev, "%s: lna=%d mixer=%d if=%d\n", __func__,
+			s->lna_gain->val, s->mixer_gain->val, s->if_gain->val);
+
+	ctrl.lna_gain = s->lna_gain_auto->val ? INT_MIN : s->lna_gain->val;
+	ctrl.mixer_gain = s->mixer_gain_auto->val ? INT_MIN : s->mixer_gain->val;
+	ctrl.if_gain = s->if_gain_auto->val ? INT_MIN : s->if_gain->val;
+
+	if (fe->ops.tuner_ops.set_config) {
+		ret = fe->ops.tuner_ops.set_config(fe, &ctrl);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+err:
+	dev_dbg(&s->udev->dev, "%s: failed %d\n", __func__, ret);
+	return ret;
+};
+
+static int rtl2832_sdr_set_gain(struct rtl2832_sdr_state *s)
+{
+	int ret;
+
+	switch (s->cfg->tuner) {
+	case RTL2832_TUNER_E4000:
+		ret = rtl2832_sdr_set_gain_e4000(s);
+		break;
+	default:
+		ret = 0;
+	}
+	return ret;
+}
+
 static int rtl2832_sdr_set_tuner(struct rtl2832_sdr_state *s)
 {
 	struct dvb_frontend *fe = s->fe;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
+	int ret;
 
 	/*
 	 * tuner RF (Hz)
@@ -932,14 +977,9 @@ static int rtl2832_sdr_set_tuner(struct rtl2832_sdr_state *s)
 	 */
 	unsigned int bandwidth = s->ctrl_tuner_bw->val;
 
-	/*
-	 * gain (dB)
-	 */
-	int gain = s->ctrl_tuner_gain->val;
-
 	dev_dbg(&s->udev->dev,
-			"%s: f_rf=%u bandwidth=%d gain=%d\n",
-			__func__, f_rf, bandwidth, gain);
+			"%s: f_rf=%u bandwidth=%d\n",
+			__func__, f_rf, bandwidth);
 
 	if (f_rf == 0)
 		return 0;
@@ -961,6 +1001,8 @@ static int rtl2832_sdr_set_tuner(struct rtl2832_sdr_state *s)
 	if (fe->ops.tuner_ops.set_params)
 		fe->ops.tuner_ops.set_params(fe);
 
+	ret = rtl2832_sdr_set_gain(s);
+
 	return 0;
 };
 
@@ -1290,7 +1332,7 @@ static int rtl2832_sdr_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct rtl2832_sdr_state *s =
 			container_of(ctrl->handler, struct rtl2832_sdr_state,
-					ctrl_handler);
+					hdl);
 	int ret;
 	dev_dbg(&s->udev->dev,
 			"%s: id=%d name=%s val=%d min=%d max=%d step=%d\n",
@@ -1302,6 +1344,15 @@ static int rtl2832_sdr_s_ctrl(struct v4l2_ctrl *ctrl)
 	case RTL2832_SDR_CID_TUNER_GAIN:
 		ret = rtl2832_sdr_set_tuner(s);
 		break;
+	case  V4L2_CID_LNA_GAIN_AUTO:
+	case  V4L2_CID_LNA_GAIN:
+	case  V4L2_CID_MIXER_GAIN_AUTO:
+	case  V4L2_CID_MIXER_GAIN:
+	case  V4L2_CID_IF_GAIN_AUTO:
+	case  V4L2_CID_IF_GAIN:
+		dev_dbg(&s->udev->dev, "%s: GAIN IOCTL\n", __func__);
+		ret = rtl2832_sdr_set_gain(s);
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -1318,7 +1369,7 @@ static void rtl2832_sdr_video_release(struct v4l2_device *v)
 	struct rtl2832_sdr_state *s =
 			container_of(v, struct rtl2832_sdr_state, v4l2_dev);
 
-	v4l2_ctrl_handler_free(&s->ctrl_handler);
+	v4l2_ctrl_handler_free(&s->hdl);
 	v4l2_device_unregister(&s->v4l2_dev);
 	kfree(s);
 }
@@ -1328,6 +1379,7 @@ struct dvb_frontend *rtl2832_sdr_attach(struct dvb_frontend *fe,
 {
 	int ret;
 	struct rtl2832_sdr_state *s;
+	const struct v4l2_ctrl_ops *ops = &rtl2832_sdr_ctrl_ops;
 	struct dvb_usb_device *d = i2c_get_adapdata(i2c);
 	static const struct v4l2_ctrl_config ctrl_tuner_bw = {
 		.ops    = &rtl2832_sdr_ctrl_ops,
@@ -1339,16 +1391,6 @@ struct dvb_frontend *rtl2832_sdr_attach(struct dvb_frontend *fe,
 		.def    = 0,
 		.step   = 1,
 	};
-	static const struct v4l2_ctrl_config ctrl_tuner_gain = {
-		.ops    = &rtl2832_sdr_ctrl_ops,
-		.id     = RTL2832_SDR_CID_TUNER_GAIN,
-		.type   = V4L2_CTRL_TYPE_INTEGER,
-		.name   = "Tuner Gain",
-		.min    = 0,
-		.max    = 102,
-		.def    = 0,
-		.step   = 1,
-	};
 
 	s = kzalloc(sizeof(struct rtl2832_sdr_state), GFP_KERNEL);
 	if (s == NULL) {
@@ -1386,11 +1428,28 @@ struct dvb_frontend *rtl2832_sdr_attach(struct dvb_frontend *fe,
 	}
 
 	/* Register controls */
-	v4l2_ctrl_handler_init(&s->ctrl_handler, 2);
-	s->ctrl_tuner_bw = v4l2_ctrl_new_custom(&s->ctrl_handler, &ctrl_tuner_bw, NULL);
-	s->ctrl_tuner_gain = v4l2_ctrl_new_custom(&s->ctrl_handler, &ctrl_tuner_gain, NULL);
-	if (s->ctrl_handler.error) {
-		ret = s->ctrl_handler.error;
+	switch (s->cfg->tuner) {
+	case RTL2832_TUNER_E4000:
+		v4l2_ctrl_handler_init(&s->hdl, 7);
+		s->ctrl_tuner_bw = v4l2_ctrl_new_custom(&s->hdl, &ctrl_tuner_bw, NULL);
+		s->lna_gain_auto = v4l2_ctrl_new_std(&s->hdl, ops, V4L2_CID_LNA_GAIN_AUTO, 0, 1, 1, 1);
+		s->lna_gain = v4l2_ctrl_new_std(&s->hdl, ops, V4L2_CID_LNA_GAIN, 0, 15, 1, 10);
+		v4l2_ctrl_auto_cluster(2, &s->lna_gain_auto, 0, false);
+		s->mixer_gain_auto = v4l2_ctrl_new_std(&s->hdl, ops, V4L2_CID_MIXER_GAIN_AUTO, 0, 1, 1, 1);
+		s->mixer_gain = v4l2_ctrl_new_std(&s->hdl, ops, V4L2_CID_MIXER_GAIN, 0, 1, 1, 1);
+		v4l2_ctrl_auto_cluster(2, &s->mixer_gain_auto, 0, false);
+		s->if_gain_auto = v4l2_ctrl_new_std(&s->hdl, ops, V4L2_CID_IF_GAIN_AUTO, 0, 1, 1, 1);
+		s->if_gain = v4l2_ctrl_new_std(&s->hdl, ops, V4L2_CID_IF_GAIN, 0, 54, 1, 0);
+		v4l2_ctrl_auto_cluster(2, &s->if_gain_auto, 0, false);
+		break;
+	default:
+		v4l2_ctrl_handler_init(&s->hdl, 1);
+		s->ctrl_tuner_bw = v4l2_ctrl_new_custom(&s->hdl, &ctrl_tuner_bw, NULL);
+		break;
+	}
+
+	if (s->hdl.error) {
+		ret = s->hdl.error;
 		dev_err(&s->udev->dev, "Could not initialize controls\n");
 		goto err_free_controls;
 	}
@@ -1411,7 +1470,7 @@ struct dvb_frontend *rtl2832_sdr_attach(struct dvb_frontend *fe,
 		goto err_free_controls;
 	}
 
-	s->v4l2_dev.ctrl_handler = &s->ctrl_handler;
+	s->v4l2_dev.ctrl_handler = &s->hdl;
 	s->vdev.v4l2_dev = &s->v4l2_dev;
 	s->vdev.lock = &s->v4l2_lock;
 	s->vdev.vfl_dir = VFL_DIR_RX;
@@ -1436,7 +1495,7 @@ struct dvb_frontend *rtl2832_sdr_attach(struct dvb_frontend *fe,
 err_unregister_v4l2_dev:
 	v4l2_device_unregister(&s->v4l2_dev);
 err_free_controls:
-	v4l2_ctrl_handler_free(&s->ctrl_handler);
+	v4l2_ctrl_handler_free(&s->hdl);
 err_free_mem:
 	kfree(s);
 	return NULL;
-- 
1.8.5.3

Re: [PATCH 1/3] e4000: convert DVB tuner to I2C driver model

[Devin Heitmueller]

On Sun, Jan 26, 2014 at 7:16 PM, Antti Palosaari <crope@iki.fi> wrote:
> Driver conversion from proprietary DVB tuner model to more
> general I2C driver model.

Mike should definitely weigh in on this.  Eliminating the existing
model of using dvb_attach() for tuners is something that needs to be
considered carefully, and this course of action should be agreed on by
the subsystem maintainers before we start converting drivers.  This
could be particularly relevant for hybrid tuners where the driver
instance is instantiated via tuner-core using dvb_attach() for the
analog side.

In the meantime, this change makes this driver work differently than
every other tuner in the tree.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/3] e4000: convert DVB tuner to I2C driver model

[Antti Palosaari]

On 27.01.2014 02:28, Devin Heitmueller wrote:
> On Sun, Jan 26, 2014 at 7:16 PM, Antti Palosaari <crope@iki.fi> wrote:
>> Driver conversion from proprietary DVB tuner model to more
>> general I2C driver model.
>
> Mike should definitely weigh in on this.  Eliminating the existing
> model of using dvb_attach() for tuners is something that needs to be
> considered carefully, and this course of action should be agreed on by
> the subsystem maintainers before we start converting drivers.  This
> could be particularly relevant for hybrid tuners where the driver
> instance is instantiated via tuner-core using dvb_attach() for the
> analog side.
>
> In the meantime, this change makes this driver work differently than
> every other tuner in the tree.

Heh, it is quite stupid to do things otherwise than rest of the kernel 
and also I think it is against i2c documentation. For more we refuse to 
use kernel standard practices the more there will be problems in a long ran.

There is things that are build top of these standard models and if you 
are using some proprietary method, then you are without these services. 
I think it was regmap which I was looking once, but dropped it as it 
requires i2c client.

Also, I already implemented one tuner driver using standard I2C model. 
If there will be problems then those are surely fixable.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 1/3] e4000: convert DVB tuner to I2C driver model

[Michael Krufky]

On Sun, Jan 26, 2014 at 7:39 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 27.01.2014 02:28, Devin Heitmueller wrote:
>>
>> On Sun, Jan 26, 2014 at 7:16 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>
>>> Driver conversion from proprietary DVB tuner model to more
>>> general I2C driver model.
>>
>>
>> Mike should definitely weigh in on this.  Eliminating the existing
>> model of using dvb_attach() for tuners is something that needs to be
>> considered carefully, and this course of action should be agreed on by
>> the subsystem maintainers before we start converting drivers.  This
>> could be particularly relevant for hybrid tuners where the driver
>> instance is instantiated via tuner-core using dvb_attach() for the
>> analog side.
>>
>> In the meantime, this change makes this driver work differently than
>> every other tuner in the tree.
>
>
> Heh, it is quite stupid to do things otherwise than rest of the kernel and
> also I think it is against i2c documentation. For more we refuse to use
> kernel standard practices the more there will be problems in a long ran.
>
> There is things that are build top of these standard models and if you are
> using some proprietary method, then you are without these services. I think
> it was regmap which I was looking once, but dropped it as it requires i2c
> client.
>
> Also, I already implemented one tuner driver using standard I2C model. If
> there will be problems then those are surely fixable.
>
> regards
> Antti

Devin is right-  I should have been cc'd on this.  Please remember to
cc me on any DVB or tuner (both analog and digital) subsystem-level
changes.

What Devin probably doesn't realize, however, is that I have basically
already agreed on this change -- this, overall, will be a very
positive move for the media subsystem.  We just need to do it
correctly, subsystem-wide.

But just because this is a good idea, its no reason to call the
current mechanism 'quite stupid' .. We're all working together here,
let's not belittle the work that others have done in the past.  I'd
rather just look forward and build a better codebase for the future
:-)

Antti submitted similar patches a few months ago - I have to review
his newer series and see if anything has changed.  My goal would be to
commit these patches into a new branch and work on converting the
entire tuner tree to the newer method, only merging to master once all
is done and tested.

-Mike

Re: [PATCH 1/3] e4000: convert DVB tuner to I2C driver model

[Antti Palosaari]

On 27.01.2014 19:45, Michael Krufky wrote:
> On Sun, Jan 26, 2014 at 7:39 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 27.01.2014 02:28, Devin Heitmueller wrote:
>>>
>>> On Sun, Jan 26, 2014 at 7:16 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>
>>>> Driver conversion from proprietary DVB tuner model to more
>>>> general I2C driver model.
>>>
>>>
>>> Mike should definitely weigh in on this.  Eliminating the existing
>>> model of using dvb_attach() for tuners is something that needs to be
>>> considered carefully, and this course of action should be agreed on by
>>> the subsystem maintainers before we start converting drivers.  This
>>> could be particularly relevant for hybrid tuners where the driver
>>> instance is instantiated via tuner-core using dvb_attach() for the
>>> analog side.
>>>
>>> In the meantime, this change makes this driver work differently than
>>> every other tuner in the tree.
>>
>>
>> Heh, it is quite stupid to do things otherwise than rest of the kernel and
>> also I think it is against i2c documentation. For more we refuse to use
>> kernel standard practices the more there will be problems in a long ran.
>>
>> There is things that are build top of these standard models and if you are
>> using some proprietary method, then you are without these services. I think
>> it was regmap which I was looking once, but dropped it as it requires i2c
>> client.
>>
>> Also, I already implemented one tuner driver using standard I2C model. If
>> there will be problems then those are surely fixable.
>>
>> regards
>> Antti
>
> Devin is right-  I should have been cc'd on this.  Please remember to
> cc me on any DVB or tuner (both analog and digital) subsystem-level
> changes.
>
> What Devin probably doesn't realize, however, is that I have basically
> already agreed on this change -- this, overall, will be a very
> positive move for the media subsystem.  We just need to do it
> correctly, subsystem-wide.
>
> But just because this is a good idea, its no reason to call the
> current mechanism 'quite stupid' .. We're all working together here,
> let's not belittle the work that others have done in the past.  I'd
> rather just look forward and build a better codebase for the future
> :-)
>
> Antti submitted similar patches a few months ago - I have to review
> his newer series and see if anything has changed.  My goal would be to
> commit these patches into a new branch and work on converting the
> entire tuner tree to the newer method, only merging to master once all
> is done and tested.

My biggest point was to criticize that general resistance of new design 
models which has been DVB side, not only that simple change but many 
others too. I am pretty sure the many "mistakes" are taken when there 
has not been better alternative available at the time, and later was 
developed proper solution, which is not still taken into use.
I have also feeling that these wrong solutions and design models used 
are one source of problems we have. All the chip I/Os should be modeled 
in a standard manner to make sure it is possible to interconnect 
flexible. GPIOs should be implemented as kernel GPIOs. I2C should be 
implemented as kernel I2C. Clock should be implemented as a kernel 
clock. Chip power-management should be implement as regulator (or what 
ever that is). TS interface also should be modeled and implement in a 
standard manner. Implementing things like that makes it possible to 
interconnect complex hardware without fearing some small change will 
break functionality because of some home-brew solution.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 1/3] e4000: convert DVB tuner to I2C driver model

[Devin Heitmueller]

Hi Antti,

> My biggest point was to criticize that general resistance of new design
> models which has been DVB side, not only that simple change but many others
> too. I am pretty sure the many "mistakes" are taken when there has not been
> better alternative available at the time, and later was developed proper
> solution, which is not still taken into use.

Sometimes the simplest looking changes can introduce all sorts of
regressions.  Just look at the mess that was caused by Mauro's
supposedly trivial "dynamic stack allocation" fixes as a prime
example.

In principle I don't have any objection to adopting common frameworks.
 That said, the changes you've proposed do deviate from how the
framework currently works, and it might have been more constructive to
post an RFC to the mailing list describing your proposed changes to
the framework rather than just submitting a patch for a single tuner.

In this particular case, your approach does give us some advantages in
being able to leverage the I2C framework, but it has costs as well.
Specifically my concerns are as follows:

1.  Removing the abstraction layer that dvb_attach() provided will
make it more difficult to add resource tracking code to handle tuner
locking/sharing.  To solve those problems would actually require us to
later *reintroduce* a layer of abstraction between the bridge and
tuner drivers if this patch were accepted as-is.

Case in point - in the V4L2 layer, they actually went in the opposite
direction - adding the V4L2 subdev framework in order to provide
additional abstraction between the bridge and I2C devices.

2.  Your approach now makes it the responsibility of the caller to
explicitly call request_module(), something that is taken care of
today by dvb_attach().  Right now you can't forget to include the
dependency since it's taken care of for you - with your change the
implementor *can* forget, and the result will be that it will fail
*sometimes* based on whether the module happens to already be loaded.
In theory your approach would give us a bit more flexibility to have
drivers with fewer module dependencies if people are compiling the
kernel from scratch for a single tuner, but that's hardly the common
use case and it significantly increases the risk of new bugs in
failing to specify dependencies.

3.  Your change gives no consideration to analog or hybrid tuners.
This isn't surprising since you've never worked in that area, but if
the model you are proposing is to be applied to all tuners, then we
need to fully understand the effects on tuner-core.ko.

> I have also feeling that these wrong solutions and design models used are
> one source of problems we have. All the chip I/Os should be modeled in a
> standard manner to make sure it is possible to interconnect flexible. GPIOs
> should be implemented as kernel GPIOs. I2C should be implemented as kernel
> I2C. Clock should be implemented as a kernel clock. Chip power-management
> should be implement as regulator (or what ever that is). TS interface also
> should be modeled and implement in a standard manner. Implementing things
> like that makes it possible to interconnect complex hardware without fearing
> some small change will break functionality because of some home-brew
> solution.

Sure.  Modular design practices are a good thing.  Thanks for stating
the obvious.  Did they also teach you about how refactoring can
introduce bugs, especially in instances where there are no unit tests
and you cannot exercise all the possible code paths?  :-)

I am confident that after the above factors are considered/addressed
that some variant of this patch can certainly be incorporated
upstream, and I look forward to seeing the continued improvement of
the codebase while not introducing new regressions.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/3] e4000: convert DVB tuner to I2C driver model

[Mauro Carvalho Chehab]

Em Mon, 27 Jan 2014 12:45:51 -0500
Michael Krufky <mkrufky@linuxtv.org> escreveu:

> Antti submitted similar patches a few months ago - I have to review
> his newer series and see if anything has changed.  My goal would be to
> commit these patches into a new branch and work on converting the
> entire tuner tree to the newer method, only merging to master once all
> is done and tested.

It passed almost two months without such review. I can't see any reason
to delay merging this patch, as the only device using this tuner should
work with the new approach.

Em Tue, 28 Jan 2014 08:41:40 -0500
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> Hi Antti,
> 
> > My biggest point was to criticize that general resistance of new design
> > models which has been DVB side, not only that simple change but many others
> > too. I am pretty sure the many "mistakes" are taken when there has not been
> > better alternative available at the time, and later was developed proper
> > solution, which is not still taken into use.
> 
> Sometimes the simplest looking changes can introduce all sorts of
> regressions.  Just look at the mess that was caused by Mauro's
> supposedly trivial "dynamic stack allocation" fixes as a prime
> example.
> 
> In principle I don't have any objection to adopting common frameworks.
>  That said, the changes you've proposed do deviate from how the
> framework currently works, and it might have been more constructive to
> post an RFC to the mailing list describing your proposed changes to
> the framework rather than just submitting a patch for a single tuner.

As far as I remember, Antti did it. There weren't many comments about
that.

> In this particular case, your approach does give us some advantages in
> being able to leverage the I2C framework, but it has costs as well.
> Specifically my concerns are as follows:
> 
> 1.  Removing the abstraction layer that dvb_attach() provided will
> make it more difficult to add resource tracking code to handle tuner
> locking/sharing.  To solve those problems would actually require us to
> later *reintroduce* a layer of abstraction between the bridge and
> tuner drivers if this patch were accepted as-is.

dvb_attach() is a dirty hack. It doesn't even take into account which
modules are using a DVB module - as can be seen when using lsmod.

All dvb_attach() does is to load a module when a non-existent symbol
is called.

Up to today, we have issues with that, as some Kconfig configurations
cause compilation failure on dibcom drivers (as, there, there are other
symbols other than attach that are needed for the bridge to properly
talk with the demod).

> Case in point - in the V4L2 layer, they actually went in the opposite
> direction - adding the V4L2 subdev framework in order to provide
> additional abstraction between the bridge and I2C devices.

Yes, but the subdev layer requires that drivers will first be a fully
compliant I2C module, in order to work.

We'll likely need a subdev layer on DVB side too in the future, in
order, for example, to solve the binding problems between dib0700
and the frontends, were there are some additional callbacks that are
needed for it to work properly.

> 2.  Your approach now makes it the responsibility of the caller to
> explicitly call request_module(), something that is taken care of
> today by dvb_attach().  Right now you can't forget to include the
> dependency since it's taken care of for you - with your change the
> implementor *can* forget, and the result will be that it will fail
> *sometimes* based on whether the module happens to already be loaded.
> In theory your approach would give us a bit more flexibility to have
> drivers with fewer module dependencies if people are compiling the
> kernel from scratch for a single tuner, but that's hardly the common
> use case and it significantly increases the risk of new bugs in
> failing to specify dependencies.

The same is true for V4L2 or hybrid drivers. The historic number of bugs
of people forgetting to add request_module() there was very small
because this is easily noticed during development phase, as, without
loading the module, the driver won't work properly.

> 3.  Your change gives no consideration to analog or hybrid tuners.
> This isn't surprising since you've never worked in that area, but if
> the model you are proposing is to be applied to all tuners, then we
> need to fully understand the effects on tuner-core.ko.

Good point. The first time we do such kind of conversion to a tuner
used by an hybrid driver, we'll need to address those issues.

> > I have also feeling that these wrong solutions and design models used are
> > one source of problems we have. All the chip I/Os should be modeled in a
> > standard manner to make sure it is possible to interconnect flexible. GPIOs
> > should be implemented as kernel GPIOs. I2C should be implemented as kernel
> > I2C. Clock should be implemented as a kernel clock. Chip power-management
> > should be implement as regulator (or what ever that is). TS interface also
> > should be modeled and implement in a standard manner. Implementing things
> > like that makes it possible to interconnect complex hardware without fearing
> > some small change will break functionality because of some home-brew
> > solution.
> 
> Sure.  Modular design practices are a good thing.  Thanks for stating
> the obvious.  Did they also teach you about how refactoring can
> introduce bugs, especially in instances where there are no unit tests
> and you cannot exercise all the possible code paths?  :-)

The e4000 tuner is used only on DVB and SDR realtek drivers, all
maintained by Antti.

I'm assuming that he tested that both keeps working after the changes.
In any case, if Antti broke something on his drivers, he should fix it.

> I am confident that after the above factors are considered/addressed
> that some variant of this patch can certainly be incorporated
> upstream, and I look forward to seeing the continued improvement of
> the codebase while not introducing new regressions.
> 
> Devin
> 

Regards,
Mauro