[PATCH 0/4] use V4L2 control framework for DVB tuner

[PATCH 0/4] use V4L2 control framework for DVB tuner

[Antti Palosaari]

Mauro blamed a little my earlier solution where I provided runtime
tuner gain controls using DVB .set_config() callback. So here it is,
new solution which uses V4L2 control framework instead.

Hehe, implementation didn't look bad at all IMHO. It was a pretty
simple, but sounds strange as tuner sits on DVB API.

Antti

Antti Palosaari (4):
  rtl28xxu: attach SDR module later
  e4000: implement controls via v4l2 control framework
  rtl2832_sdr: use E4000 tuner controls via V4L framework
  e4000: remove .set_config() which was for controls

 drivers/media/tuners/e4000.c                     | 104 +++++++++++++++++++----
 drivers/media/tuners/e4000.h                     |  18 ++--
 drivers/media/tuners/e4000_priv.h                |  12 +++
 drivers/media/usb/dvb-usb-v2/rtl28xxu.c          |  21 ++++-
 drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c |  92 ++++++++------------
 5 files changed, 164 insertions(+), 83 deletions(-)

-- 
1.8.5.3

[PATCH 1/4] rtl28xxu: attach SDR module later

[Antti Palosaari]

SDR module was attached between demod and tuner. Change it happen
after tuner attached. We are going to implement V4L controls for
tuner drivers and those controls are loaded during SDR attach. Due to
that (tuner controls), tuner driver must be loaded before SDR module.

Also as we are here, limit SDR module loading only for those tuners
we support.

Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index 76cf0de..73348bf 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -777,10 +777,6 @@ static int rtl2832u_frontend_attach(struct dvb_usb_adapter *adap)
 	/* set fe callback */
 	adap->fe[0]->callback = rtl2832u_frontend_callback;
 
-	/* attach SDR */
-	dvb_attach(rtl2832_sdr_attach, adap->fe[0], &d->i2c_adap,
-			rtl2832_config);
-
 	return 0;
 err:
 	dev_dbg(&d->udev->dev, "%s: failed=%d\n", __func__, ret);
@@ -906,6 +902,10 @@ static int rtl2832u_tuner_attach(struct dvb_usb_adapter *adap)
 		 * that to the tuner driver */
 		adap->fe[0]->ops.read_signal_strength =
 				adap->fe[0]->ops.tuner_ops.get_rf_strength;
+
+		/* attach SDR */
+		dvb_attach(rtl2832_sdr_attach, adap->fe[0], &d->i2c_adap,
+				&rtl28xxu_rtl2832_fc0012_config);
 		return 0;
 		break;
 	case TUNER_RTL2832_FC0013:
@@ -915,6 +915,10 @@ static int rtl2832u_tuner_attach(struct dvb_usb_adapter *adap)
 		/* fc0013 also supports signal strength reading */
 		adap->fe[0]->ops.read_signal_strength =
 				adap->fe[0]->ops.tuner_ops.get_rf_strength;
+
+		/* attach SDR */
+		dvb_attach(rtl2832_sdr_attach, adap->fe[0], &d->i2c_adap,
+				&rtl28xxu_rtl2832_fc0013_config);
 		return 0;
 	case TUNER_RTL2832_E4000: {
 			struct e4000_config e4000_config = {
@@ -928,6 +932,11 @@ static int rtl2832u_tuner_attach(struct dvb_usb_adapter *adap)
 
 			request_module("e4000");
 			priv->client = i2c_new_device(&d->i2c_adap, &info);
+
+			/* attach SDR */
+			dvb_attach(rtl2832_sdr_attach, adap->fe[0],
+					&d->i2c_adap,
+					&rtl28xxu_rtl2832_e4000_config);
 		}
 		break;
 	case TUNER_RTL2832_FC2580:
@@ -954,6 +963,10 @@ static int rtl2832u_tuner_attach(struct dvb_usb_adapter *adap)
 		/* Use tuner to get the signal strength */
 		adap->fe[0]->ops.read_signal_strength =
 				adap->fe[0]->ops.tuner_ops.get_rf_strength;
+
+		/* attach SDR */
+		dvb_attach(rtl2832_sdr_attach, adap->fe[0], &d->i2c_adap,
+				&rtl28xxu_rtl2832_r820t_config);
 		break;
 	case TUNER_RTL2832_R828D:
 		/* power off mn88472 demod on GPIO0 */
-- 
1.8.5.3

[PATCH 2/4] e4000: implement controls via v4l2 control framework

[Antti Palosaari]

Implement gain and bandwidth controls using v4l2 control framework.
Pointer to control handler is provided by exported symbol.

Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/tuners/e4000.c      | 142 +++++++++++++++++++++++++++++++++++++-
 drivers/media/tuners/e4000.h      |  14 ++++
 drivers/media/tuners/e4000_priv.h |  12 ++++
 3 files changed, 167 insertions(+), 1 deletion(-)

diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 9187190..11d31b0 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -448,6 +448,110 @@ err:
 	return ret;
 }
 
+static int e4000_set_gain(struct dvb_frontend *fe)
+{
+	struct e4000_priv *priv = fe->tuner_priv;
+	int ret;
+	u8 buf[2];
+	u8 u8tmp;
+	dev_dbg(&priv->client->dev, "%s: lna=%d mixer=%d if=%d\n", __func__,
+			priv->lna_gain->val, priv->mixer_gain->val,
+			priv->if_gain->val);
+
+	if (fe->ops.i2c_gate_ctrl)
+		fe->ops.i2c_gate_ctrl(fe, 1);
+
+	if (priv->lna_gain_auto->val && priv->if_gain_auto->val)
+		u8tmp = 0x17;
+	else if (priv->lna_gain_auto->val)
+		u8tmp = 0x19;
+	else if (priv->if_gain_auto->val)
+		u8tmp = 0x16;
+	else
+		u8tmp = 0x10;
+
+	ret = e4000_wr_reg(priv, 0x1a, u8tmp);
+	if (ret)
+		goto err;
+
+	if (priv->mixer_gain_auto->val)
+		u8tmp = 0x15;
+	else
+		u8tmp = 0x14;
+
+	ret = e4000_wr_reg(priv, 0x20, u8tmp);
+	if (ret)
+		goto err;
+
+	if (priv->lna_gain_auto->val == false) {
+		ret = e4000_wr_reg(priv, 0x14, priv->lna_gain->val);
+		if (ret)
+			goto err;
+	}
+
+	if (priv->mixer_gain_auto->val == false) {
+		ret = e4000_wr_reg(priv, 0x15, priv->mixer_gain->val);
+		if (ret)
+			goto err;
+	}
+
+	if (priv->if_gain_auto->val == false) {
+		buf[0] = e4000_if_gain_lut[priv->if_gain->val].reg16_val;
+		buf[1] = e4000_if_gain_lut[priv->if_gain->val].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 int e4000_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct e4000_priv *priv =
+			container_of(ctrl->handler, struct e4000_priv, hdl);
+	struct dvb_frontend *fe = priv->fe;
+	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
+	int ret;
+	dev_dbg(&priv->client->dev,
+			"%s: id=%d name=%s val=%d min=%d max=%d step=%d\n",
+			__func__, ctrl->id, ctrl->name, ctrl->val,
+			ctrl->minimum, ctrl->maximum, ctrl->step);
+
+	switch (ctrl->id) {
+	case V4L2_CID_BANDWIDTH_AUTO:
+	case V4L2_CID_BANDWIDTH:
+		c->bandwidth_hz = priv->bandwidth->val;
+		ret = e4000_set_params(priv->fe);
+		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:
+		ret = e4000_set_gain(priv->fe);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops e4000_ctrl_ops = {
+	.s_ctrl = e4000_s_ctrl,
+};
+
 static const struct dvb_tuner_ops e4000_tuner_ops = {
 	.info = {
 		.name           = "Elonics E4000",
@@ -463,6 +567,13 @@ static const struct dvb_tuner_ops e4000_tuner_ops = {
 	.get_if_frequency = e4000_get_if_frequency,
 };
 
+struct v4l2_ctrl_handler *e4000_get_ctrl_handler(struct dvb_frontend *fe)
+{
+	struct e4000_priv *priv = fe->tuner_priv;
+	return &priv->hdl;
+}
+EXPORT_SYMBOL(e4000_get_ctrl_handler);
+
 static int e4000_probe(struct i2c_client *client,
 		const struct i2c_device_id *id)
 {
@@ -504,6 +615,35 @@ static int e4000_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto err;
 
+	/* Register controls */
+	v4l2_ctrl_handler_init(&priv->hdl, 8);
+	priv->bandwidth_auto = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
+			V4L2_CID_BANDWIDTH_AUTO, 0, 1, 1, 1);
+	priv->bandwidth = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
+			V4L2_CID_BANDWIDTH, 4300000, 11000000, 100000, 4300000);
+	v4l2_ctrl_auto_cluster(2, &priv->bandwidth_auto, 0, false);
+	priv->lna_gain_auto = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
+			V4L2_CID_LNA_GAIN_AUTO, 0, 1, 1, 1);
+	priv->lna_gain = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
+			V4L2_CID_LNA_GAIN, 0, 15, 1, 10);
+	v4l2_ctrl_auto_cluster(2, &priv->lna_gain_auto, 0, false);
+	priv->mixer_gain_auto = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
+			V4L2_CID_MIXER_GAIN_AUTO, 0, 1, 1, 1);
+	priv->mixer_gain = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
+			V4L2_CID_MIXER_GAIN, 0, 1, 1, 1);
+	v4l2_ctrl_auto_cluster(2, &priv->mixer_gain_auto, 0, false);
+	priv->if_gain_auto = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
+			V4L2_CID_IF_GAIN_AUTO, 0, 1, 1, 1);
+	priv->if_gain = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
+			V4L2_CID_IF_GAIN, 0, 54, 1, 0);
+	v4l2_ctrl_auto_cluster(2, &priv->if_gain_auto, 0, false);
+	if (priv->hdl.error) {
+		ret = priv->hdl.error;
+		dev_err(&priv->client->dev, "Could not initialize controls\n");
+		v4l2_ctrl_handler_free(&priv->hdl);
+		goto err;
+	}
+
 	dev_info(&priv->client->dev,
 			"%s: Elonics E4000 successfully identified\n",
 			KBUILD_MODNAME);
@@ -533,7 +673,7 @@ static int e4000_remove(struct i2c_client *client)
 	struct dvb_frontend *fe = priv->fe;
 
 	dev_dbg(&client->dev, "%s:\n", __func__);
-
+	v4l2_ctrl_handler_free(&priv->hdl);
 	memset(&fe->ops.tuner_ops, 0, sizeof(struct dvb_tuner_ops));
 	fe->tuner_priv = NULL;
 	kfree(priv);
diff --git a/drivers/media/tuners/e4000.h b/drivers/media/tuners/e4000.h
index d95c472..d86de6d 100644
--- a/drivers/media/tuners/e4000.h
+++ b/drivers/media/tuners/e4000.h
@@ -46,4 +46,18 @@ struct e4000_ctrl {
 	int if_gain;
 };
 
+#if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
+extern struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
+		struct dvb_frontend *fe
+);
+#else
+static inline struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
+		struct dvb_frontend *fe
+)
+{
+	pr_warn("%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 a75a383..8cc27b3 100644
--- a/drivers/media/tuners/e4000_priv.h
+++ b/drivers/media/tuners/e4000_priv.h
@@ -22,11 +22,23 @@
 #define E4000_PRIV_H
 
 #include "e4000.h"
+#include <media/v4l2-ctrls.h>
 
 struct e4000_priv {
 	struct i2c_client *client;
 	u32 clock;
 	struct dvb_frontend *fe;
+
+	/* Controls */
+	struct v4l2_ctrl_handler hdl;
+	struct v4l2_ctrl *bandwidth_auto;
+	struct v4l2_ctrl *bandwidth;
+	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 e4000_pll {
-- 
1.8.5.3

[PATCH 3/4] rtl2832_sdr: use E4000 tuner controls via V4L framework

[Antti Palosaari]

Use V4L2 control framework for E4000 tuner as it provides controls
that way now.

Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c | 92 +++++++++---------------
 1 file changed, 34 insertions(+), 58 deletions(-)

diff --git a/drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c b/drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c
index 1dfe653..c26c084 100644
--- a/drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c
+++ b/drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c
@@ -922,30 +922,6 @@ 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_r820t(struct rtl2832_sdr_state *s)
 {
 	int ret;
@@ -975,9 +951,6 @@ 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;
 	case RTL2832_TUNER_R820T:
 		ret = rtl2832_sdr_set_gain_r820t(s);
 		break;
@@ -991,35 +964,33 @@ 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;
+	struct v4l2_ctrl *bandwidth_auto;
+	struct v4l2_ctrl *bandwidth;
 	int ret;
 
 	/*
 	 * tuner RF (Hz)
 	 */
-	unsigned int f_rf = s->f_tuner;
+	if (s->f_tuner == 0)
+		return 0;
 
 	/*
 	 * bandwidth (Hz)
 	 */
-	unsigned int bandwidth;
-
-	/* filters */
-	if (s->bandwidth_auto->val)
-		bandwidth = s->f_adc;
-	else
-		bandwidth = s->bandwidth->val;
-
-	s->bandwidth->val = bandwidth;
-
-	dev_dbg(&s->udev->dev, "%s: f_rf=%u bandwidth=%d\n",
-			__func__, f_rf, bandwidth);
+	bandwidth_auto = v4l2_ctrl_find(&s->hdl, V4L2_CID_BANDWIDTH_AUTO);
+	bandwidth = v4l2_ctrl_find(&s->hdl, V4L2_CID_BANDWIDTH);
+	if (v4l2_ctrl_g_ctrl(bandwidth_auto)) {
+		c->bandwidth_hz = s->f_adc;
+		v4l2_ctrl_s_ctrl(bandwidth, s->f_adc);
+	} else {
+		c->bandwidth_hz = v4l2_ctrl_g_ctrl(bandwidth);
+	}
 
-	c->bandwidth_hz = bandwidth;
-	c->frequency = f_rf;
+	c->frequency = s->f_tuner;
 	c->delivery_system = SYS_DVBT;
 
-	if (f_rf == 0)
-		return 0;
+	dev_dbg(&s->udev->dev, "%s: frequency=%u bandwidth=%d\n",
+			__func__, c->frequency, c->bandwidth_hz);
 
 	if (!test_bit(POWER_ON, &s->flags))
 		return 0;
@@ -1362,6 +1333,8 @@ static int rtl2832_sdr_s_ctrl(struct v4l2_ctrl *ctrl)
 	struct rtl2832_sdr_state *s =
 			container_of(ctrl->handler, struct rtl2832_sdr_state,
 					hdl);
+	struct dvb_frontend *fe = s->fe;
+	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
 	int ret;
 	dev_dbg(&s->udev->dev,
 			"%s: id=%d name=%s val=%d min=%d max=%d step=%d\n",
@@ -1371,7 +1344,18 @@ static int rtl2832_sdr_s_ctrl(struct v4l2_ctrl *ctrl)
 	switch (ctrl->id) {
 	case V4L2_CID_BANDWIDTH_AUTO:
 	case V4L2_CID_BANDWIDTH:
-		ret = rtl2832_sdr_set_tuner(s);
+		if (s->bandwidth_auto->val)
+			s->bandwidth->val = s->f_adc;
+
+		c->bandwidth_hz = s->bandwidth->val;
+
+		if (!test_bit(POWER_ON, &s->flags))
+			return 0;
+
+		if (fe->ops.tuner_ops.set_params)
+			ret = fe->ops.tuner_ops.set_params(fe);
+		else
+			ret = 0;
 		break;
 	case  V4L2_CID_LNA_GAIN_AUTO:
 	case  V4L2_CID_LNA_GAIN:
@@ -1410,6 +1394,7 @@ struct dvb_frontend *rtl2832_sdr_attach(struct dvb_frontend *fe,
 	struct rtl2832_sdr_state *s;
 	const struct v4l2_ctrl_ops *ops = &rtl2832_sdr_ctrl_ops;
 	struct dvb_usb_device *d = i2c_get_adapdata(i2c);
+	struct v4l2_ctrl_handler *hdl;
 
 	s = kzalloc(sizeof(struct rtl2832_sdr_state), GFP_KERNEL);
 	if (s == NULL) {
@@ -1449,19 +1434,10 @@ struct dvb_frontend *rtl2832_sdr_attach(struct dvb_frontend *fe,
 	/* Register controls */
 	switch (s->cfg->tuner) {
 	case RTL2832_TUNER_E4000:
-		v4l2_ctrl_handler_init(&s->hdl, 8);
-		s->bandwidth_auto = v4l2_ctrl_new_std(&s->hdl, ops, V4L2_CID_BANDWIDTH_AUTO, 0, 1, 1, 1);
-		s->bandwidth = v4l2_ctrl_new_std(&s->hdl, ops, V4L2_CID_BANDWIDTH, 4300000, 11000000, 100000, 4300000);
-		v4l2_ctrl_auto_cluster(2, &s->bandwidth_auto, 0, false);
-		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);
+		hdl = e4000_get_ctrl_handler(fe);
+		v4l2_ctrl_handler_init(&s->hdl, 0);
+		if (hdl)
+			v4l2_ctrl_add_handler(&s->hdl, hdl, NULL);
 		break;
 	case RTL2832_TUNER_R820T:
 		v4l2_ctrl_handler_init(&s->hdl, 8);
-- 
1.8.5.3

[PATCH 4/4] e4000: remove .set_config() which was for controls

[Antti Palosaari]

That custom DVB callback is not needed anymore for setting gain
controls as those are now implemented using V4L2 control framework.

That change was proposed by Mauro.

Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/tuners/e4000.c | 68 --------------------------------------------
 drivers/media/tuners/e4000.h |  6 ----
 2 files changed, 74 deletions(-)

diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 11d31b0..d5ae341 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -381,73 +381,6 @@ 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 int e4000_set_gain(struct dvb_frontend *fe)
 {
 	struct e4000_priv *priv = fe->tuner_priv;
@@ -562,7 +495,6 @@ 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 d86de6d..989f2ea 100644
--- a/drivers/media/tuners/e4000.h
+++ b/drivers/media/tuners/e4000.h
@@ -40,12 +40,6 @@ struct e4000_config {
 	u32 clock;
 };
 
-struct e4000_ctrl {
-	int lna_gain;
-	int mixer_gain;
-	int if_gain;
-};
-
 #if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
 extern struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
 		struct dvb_frontend *fe
-- 
1.8.5.3

Re: [PATCH 2/4] e4000: implement controls via v4l2 control framework

[Hans Verkuil]

On 02/04/2014 02:39 AM, Antti Palosaari wrote:
> Implement gain and bandwidth controls using v4l2 control framework.
> Pointer to control handler is provided by exported symbol.
> 
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  drivers/media/tuners/e4000.c      | 142 +++++++++++++++++++++++++++++++++++++-
>  drivers/media/tuners/e4000.h      |  14 ++++
>  drivers/media/tuners/e4000_priv.h |  12 ++++
>  3 files changed, 167 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
> index 9187190..11d31b0 100644
> --- a/drivers/media/tuners/e4000.c
> +++ b/drivers/media/tuners/e4000.c
> @@ -448,6 +448,110 @@ err:
>  	return ret;
>  }
>  
> +static int e4000_set_gain(struct dvb_frontend *fe)
> +{
> +	struct e4000_priv *priv = fe->tuner_priv;
> +	int ret;
> +	u8 buf[2];
> +	u8 u8tmp;
> +	dev_dbg(&priv->client->dev, "%s: lna=%d mixer=%d if=%d\n", __func__,
> +			priv->lna_gain->val, priv->mixer_gain->val,
> +			priv->if_gain->val);
> +
> +	if (fe->ops.i2c_gate_ctrl)
> +		fe->ops.i2c_gate_ctrl(fe, 1);
> +
> +	if (priv->lna_gain_auto->val && priv->if_gain_auto->val)
> +		u8tmp = 0x17;
> +	else if (priv->lna_gain_auto->val)
> +		u8tmp = 0x19;
> +	else if (priv->if_gain_auto->val)
> +		u8tmp = 0x16;
> +	else
> +		u8tmp = 0x10;
> +
> +	ret = e4000_wr_reg(priv, 0x1a, u8tmp);
> +	if (ret)
> +		goto err;
> +
> +	if (priv->mixer_gain_auto->val)
> +		u8tmp = 0x15;
> +	else
> +		u8tmp = 0x14;
> +
> +	ret = e4000_wr_reg(priv, 0x20, u8tmp);
> +	if (ret)
> +		goto err;
> +
> +	if (priv->lna_gain_auto->val == false) {
> +		ret = e4000_wr_reg(priv, 0x14, priv->lna_gain->val);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	if (priv->mixer_gain_auto->val == false) {
> +		ret = e4000_wr_reg(priv, 0x15, priv->mixer_gain->val);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	if (priv->if_gain_auto->val == false) {
> +		buf[0] = e4000_if_gain_lut[priv->if_gain->val].reg16_val;
> +		buf[1] = e4000_if_gain_lut[priv->if_gain->val].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 int e4000_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct e4000_priv *priv =
> +			container_of(ctrl->handler, struct e4000_priv, hdl);
> +	struct dvb_frontend *fe = priv->fe;
> +	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
> +	int ret;
> +	dev_dbg(&priv->client->dev,
> +			"%s: id=%d name=%s val=%d min=%d max=%d step=%d\n",
> +			__func__, ctrl->id, ctrl->name, ctrl->val,
> +			ctrl->minimum, ctrl->maximum, ctrl->step);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_BANDWIDTH_AUTO:
> +	case V4L2_CID_BANDWIDTH:
> +		c->bandwidth_hz = priv->bandwidth->val;
> +		ret = e4000_set_params(priv->fe);
> +		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:
> +		ret = e4000_set_gain(priv->fe);

That won't work. You need to handle each gain cluster separately. The control
framework processes the controls one cluster at a time and takes a lock on the
master control before calling s_ctrl. The ctrl->val field is only valid inside
s_ctrl for the controls in the cluster, not for other controls. For other
controls only the ctrl->cur.val field is valid.

Regards,

	Hans

> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops e4000_ctrl_ops = {
> +	.s_ctrl = e4000_s_ctrl,
> +};
> +
>  static const struct dvb_tuner_ops e4000_tuner_ops = {
>  	.info = {
>  		.name           = "Elonics E4000",
> @@ -463,6 +567,13 @@ static const struct dvb_tuner_ops e4000_tuner_ops = {
>  	.get_if_frequency = e4000_get_if_frequency,
>  };
>  
> +struct v4l2_ctrl_handler *e4000_get_ctrl_handler(struct dvb_frontend *fe)
> +{
> +	struct e4000_priv *priv = fe->tuner_priv;
> +	return &priv->hdl;
> +}
> +EXPORT_SYMBOL(e4000_get_ctrl_handler);
> +
>  static int e4000_probe(struct i2c_client *client,
>  		const struct i2c_device_id *id)
>  {
> @@ -504,6 +615,35 @@ static int e4000_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		goto err;
>  
> +	/* Register controls */
> +	v4l2_ctrl_handler_init(&priv->hdl, 8);
> +	priv->bandwidth_auto = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
> +			V4L2_CID_BANDWIDTH_AUTO, 0, 1, 1, 1);
> +	priv->bandwidth = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
> +			V4L2_CID_BANDWIDTH, 4300000, 11000000, 100000, 4300000);
> +	v4l2_ctrl_auto_cluster(2, &priv->bandwidth_auto, 0, false);
> +	priv->lna_gain_auto = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
> +			V4L2_CID_LNA_GAIN_AUTO, 0, 1, 1, 1);
> +	priv->lna_gain = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
> +			V4L2_CID_LNA_GAIN, 0, 15, 1, 10);
> +	v4l2_ctrl_auto_cluster(2, &priv->lna_gain_auto, 0, false);
> +	priv->mixer_gain_auto = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
> +			V4L2_CID_MIXER_GAIN_AUTO, 0, 1, 1, 1);
> +	priv->mixer_gain = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
> +			V4L2_CID_MIXER_GAIN, 0, 1, 1, 1);
> +	v4l2_ctrl_auto_cluster(2, &priv->mixer_gain_auto, 0, false);
> +	priv->if_gain_auto = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
> +			V4L2_CID_IF_GAIN_AUTO, 0, 1, 1, 1);
> +	priv->if_gain = v4l2_ctrl_new_std(&priv->hdl, &e4000_ctrl_ops,
> +			V4L2_CID_IF_GAIN, 0, 54, 1, 0);
> +	v4l2_ctrl_auto_cluster(2, &priv->if_gain_auto, 0, false);
> +	if (priv->hdl.error) {
> +		ret = priv->hdl.error;
> +		dev_err(&priv->client->dev, "Could not initialize controls\n");
> +		v4l2_ctrl_handler_free(&priv->hdl);
> +		goto err;
> +	}
> +
>  	dev_info(&priv->client->dev,
>  			"%s: Elonics E4000 successfully identified\n",
>  			KBUILD_MODNAME);
> @@ -533,7 +673,7 @@ static int e4000_remove(struct i2c_client *client)
>  	struct dvb_frontend *fe = priv->fe;
>  
>  	dev_dbg(&client->dev, "%s:\n", __func__);
> -
> +	v4l2_ctrl_handler_free(&priv->hdl);
>  	memset(&fe->ops.tuner_ops, 0, sizeof(struct dvb_tuner_ops));
>  	fe->tuner_priv = NULL;
>  	kfree(priv);
> diff --git a/drivers/media/tuners/e4000.h b/drivers/media/tuners/e4000.h
> index d95c472..d86de6d 100644
> --- a/drivers/media/tuners/e4000.h
> +++ b/drivers/media/tuners/e4000.h
> @@ -46,4 +46,18 @@ struct e4000_ctrl {
>  	int if_gain;
>  };
>  
> +#if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
> +extern struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
> +		struct dvb_frontend *fe
> +);
> +#else
> +static inline struct v4l2_ctrl_handler *e4000_get_ctrl_handler(
> +		struct dvb_frontend *fe
> +)
> +{
> +	pr_warn("%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 a75a383..8cc27b3 100644
> --- a/drivers/media/tuners/e4000_priv.h
> +++ b/drivers/media/tuners/e4000_priv.h
> @@ -22,11 +22,23 @@
>  #define E4000_PRIV_H
>  
>  #include "e4000.h"
> +#include <media/v4l2-ctrls.h>
>  
>  struct e4000_priv {
>  	struct i2c_client *client;
>  	u32 clock;
>  	struct dvb_frontend *fe;
> +
> +	/* Controls */
> +	struct v4l2_ctrl_handler hdl;
> +	struct v4l2_ctrl *bandwidth_auto;
> +	struct v4l2_ctrl *bandwidth;
> +	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 e4000_pll {
> 

Re: [PATCH 2/4] e4000: implement controls via v4l2 control framework

[Antti Palosaari]

Moi Hans

On 04.02.2014 20:43, Hans Verkuil wrote:
> On 02/04/2014 02:39 AM, Antti Palosaari wrote:
>> Implement gain and bandwidth controls using v4l2 control framework.
>> Pointer to control handler is provided by exported symbol.
>>
>> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
>> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>> Signed-off-by: Antti Palosaari <crope@iki.fi>
>> ---
>>   drivers/media/tuners/e4000.c      | 142 +++++++++++++++++++++++++++++++++++++-
>>   drivers/media/tuners/e4000.h      |  14 ++++
>>   drivers/media/tuners/e4000_priv.h |  12 ++++
>>   3 files changed, 167 insertions(+), 1 deletion(-)

>> +static int e4000_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct e4000_priv *priv =
>> +			container_of(ctrl->handler, struct e4000_priv, hdl);
>> +	struct dvb_frontend *fe = priv->fe;
>> +	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
>> +	int ret;
>> +	dev_dbg(&priv->client->dev,
>> +			"%s: id=%d name=%s val=%d min=%d max=%d step=%d\n",
>> +			__func__, ctrl->id, ctrl->name, ctrl->val,
>> +			ctrl->minimum, ctrl->maximum, ctrl->step);
>> +
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_BANDWIDTH_AUTO:
>> +	case V4L2_CID_BANDWIDTH:
>> +		c->bandwidth_hz = priv->bandwidth->val;
>> +		ret = e4000_set_params(priv->fe);
>> +		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:
>> +		ret = e4000_set_gain(priv->fe);
>
> That won't work. You need to handle each gain cluster separately. The control
> framework processes the controls one cluster at a time and takes a lock on the
> master control before calling s_ctrl. The ctrl->val field is only valid inside
> s_ctrl for the controls in the cluster, not for other controls. For other
> controls only the ctrl->cur.val field is valid.

hmm, actually it worked fine on my tests - but I think see your point. 
It likely woks as my app sets one control per call, but if you try to 
set multiple controls then it go out of sync I think.

I am going to split that gain function to three pieces then.

regards
Antti

-- 
http://palosaari.fi/