v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Manu Abraham]

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



[-- Attachment #2: 0009-CXD2820r-Query-DVB-frontend-delivery-capabilities.patch --]
[-- Type: text/x-patch, Size: 22369 bytes --]

From 4fdffdcc77228a140c944c20ce2a9f2b6c5b7658 Mon Sep 17 00:00:00 2001
From: Manu Abraham <abraham.manu@gmail.com>
Date: Thu, 24 Nov 2011 20:29:53 +0530
Subject: [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

Override default delivery system information provided by FE_GET_INFO, so
that applications can enumerate delivery systems provided by the frontend.

Signed-off-by: Manu Abraham <abraham.manu@gmail.com>
---
 drivers/media/dvb/frontends/cxd2820r_c.c    |    2 +-
 drivers/media/dvb/frontends/cxd2820r_core.c |  651 +++++++++------------------
 drivers/media/dvb/frontends/cxd2820r_priv.h |    5 +-
 drivers/media/dvb/frontends/cxd2820r_t.c    |    2 +-
 drivers/media/dvb/frontends/cxd2820r_t2.c   |    2 +-
 5 files changed, 221 insertions(+), 441 deletions(-)

diff --git a/drivers/media/dvb/frontends/cxd2820r_c.c b/drivers/media/dvb/frontends/cxd2820r_c.c
index b85f501..01baeae 100644
--- a/drivers/media/dvb/frontends/cxd2820r_c.c
+++ b/drivers/media/dvb/frontends/cxd2820r_c.c
@@ -22,7 +22,7 @@
 #include "cxd2820r_priv.h"
 
 int cxd2820r_set_frontend_c(struct dvb_frontend *fe,
-	struct dvb_frontend_parameters *params)
+			    struct dvb_frontend_parameters *params)
 {
 	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
diff --git a/drivers/media/dvb/frontends/cxd2820r_core.c b/drivers/media/dvb/frontends/cxd2820r_core.c
index 036480f..5b0120a 100644
--- a/drivers/media/dvb/frontends/cxd2820r_core.c
+++ b/drivers/media/dvb/frontends/cxd2820r_core.c
@@ -240,43 +240,6 @@ error:
 	return ret;
 }
 
-/* lock FE */
-static int cxd2820r_lock(struct cxd2820r_priv *priv, int active_fe)
-{
-	int ret = 0;
-	dbg("%s: active_fe=%d", __func__, active_fe);
-
-	mutex_lock(&priv->fe_lock);
-
-	/* -1=NONE, 0=DVB-T/T2, 1=DVB-C */
-	if (priv->active_fe == active_fe)
-		;
-	else if (priv->active_fe == -1)
-		priv->active_fe = active_fe;
-	else
-		ret = -EBUSY;
-
-	mutex_unlock(&priv->fe_lock);
-
-	return ret;
-}
-
-/* unlock FE */
-static void cxd2820r_unlock(struct cxd2820r_priv *priv, int active_fe)
-{
-	dbg("%s: active_fe=%d", __func__, active_fe);
-
-	mutex_lock(&priv->fe_lock);
-
-	/* -1=NONE, 0=DVB-T/T2, 1=DVB-C */
-	if (priv->active_fe == active_fe)
-		priv->active_fe = -1;
-
-	mutex_unlock(&priv->fe_lock);
-
-	return;
-}
-
 /* 64 bit div with round closest, like DIV_ROUND_CLOSEST but 64 bit */
 u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor)
 {
@@ -284,378 +247,230 @@ u32 cxd2820r_div_u64_round_closest(u64 dividend, u32 divisor)
 }
 
 static int cxd2820r_set_frontend(struct dvb_frontend *fe,
-	struct dvb_frontend_parameters *p)
+				 struct dvb_frontend_parameters *p)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
 	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		switch (priv->delivery_system) {
-		case SYS_UNDEFINED:
-			if (c->delivery_system == SYS_DVBT) {
-				/* SLEEP => DVB-T */
-				ret = cxd2820r_set_frontend_t(fe, p);
-			} else {
-				/* SLEEP => DVB-T2 */
-				ret = cxd2820r_set_frontend_t2(fe, p);
-			}
-			break;
-		case SYS_DVBT:
-			if (c->delivery_system == SYS_DVBT) {
-				/* DVB-T => DVB-T */
-				ret = cxd2820r_set_frontend_t(fe, p);
-			} else if (c->delivery_system == SYS_DVBT2) {
-				/* DVB-T => DVB-T2 */
-				ret = cxd2820r_sleep_t(fe);
-				if (ret)
-					break;
-				ret = cxd2820r_set_frontend_t2(fe, p);
-			}
-			break;
-		case SYS_DVBT2:
-			if (c->delivery_system == SYS_DVBT2) {
-				/* DVB-T2 => DVB-T2 */
-				ret = cxd2820r_set_frontend_t2(fe, p);
-			} else if (c->delivery_system == SYS_DVBT) {
-				/* DVB-T2 => DVB-T */
-				ret = cxd2820r_sleep_t2(fe);
-				if (ret)
-					break;
-				ret = cxd2820r_set_frontend_t(fe, p);
-			}
-			break;
-		default:
-			dbg("%s: error state=%d", __func__,
-				priv->delivery_system);
-			ret = -EINVAL;
-		}
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
 
+	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
+	switch (c->delivery_system) {
+	case SYS_DVBT:
+		ret = cxd2820r_init_t(fe);
+		if (ret < 0)
+			goto err;
+		ret = cxd2820r_set_frontend_t(fe, p);
+		if (ret < 0)
+			goto err;
+		break;
+	case SYS_DVBT2:
+		ret = cxd2820r_init_t(fe);
+		if (ret < 0)
+			goto err;
+		ret = cxd2820r_set_frontend_t2(fe, p);
+		if (ret < 0)
+			goto err;
+		break;
+	case SYS_DVBC_ANNEX_AC:
+		ret = cxd2820r_init_c(fe);
+		if (ret < 0)
+			goto err;
 		ret = cxd2820r_set_frontend_c(fe, p);
+		if (ret < 0)
+			goto err;
+		break;
+	default:
+		dbg("%s: error state=%d", __func__, fe->dtv_property_cache.delivery_system);
+		ret = -EINVAL;
+		break;
 	}
-
+err:
 	return ret;
 }
-
 static int cxd2820r_read_status(struct dvb_frontend *fe, fe_status_t *status)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		switch (fe->dtv_property_cache.delivery_system) {
-		case SYS_DVBT:
-			ret = cxd2820r_read_status_t(fe, status);
-			break;
-		case SYS_DVBT2:
-			ret = cxd2820r_read_status_t2(fe, status);
-			break;
-		default:
-			ret = -EINVAL;
-		}
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
 
+	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
+	switch (fe->dtv_property_cache.delivery_system) {
+	case SYS_DVBT:
+		ret = cxd2820r_read_status_t(fe, status);
+		break;
+	case SYS_DVBT2:
+		ret = cxd2820r_read_status_t2(fe, status);
+		break;
+	case SYS_DVBC_ANNEX_AC:
 		ret = cxd2820r_read_status_c(fe, status);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
-
 	return ret;
 }
 
 static int cxd2820r_get_frontend(struct dvb_frontend *fe,
-	struct dvb_frontend_parameters *p)
+				 struct dvb_frontend_parameters *p)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		switch (fe->dtv_property_cache.delivery_system) {
-		case SYS_DVBT:
-			ret = cxd2820r_get_frontend_t(fe, p);
-			break;
-		case SYS_DVBT2:
-			ret = cxd2820r_get_frontend_t2(fe, p);
-			break;
-		default:
-			ret = -EINVAL;
-		}
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
 
+	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
+	switch (fe->dtv_property_cache.delivery_system) {
+	case SYS_DVBT:
+		ret = cxd2820r_get_frontend_t(fe, p);
+		break;
+	case SYS_DVBT2:
+		ret = cxd2820r_get_frontend_t2(fe, p);
+		break;
+	case SYS_DVBC_ANNEX_AC:
 		ret = cxd2820r_get_frontend_c(fe, p);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
-
 	return ret;
 }
 
 static int cxd2820r_read_ber(struct dvb_frontend *fe, u32 *ber)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		switch (fe->dtv_property_cache.delivery_system) {
-		case SYS_DVBT:
-			ret = cxd2820r_read_ber_t(fe, ber);
-			break;
-		case SYS_DVBT2:
-			ret = cxd2820r_read_ber_t2(fe, ber);
-			break;
-		default:
-			ret = -EINVAL;
-		}
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
 
+	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
+	switch (fe->dtv_property_cache.delivery_system) {
+	case SYS_DVBT:
+		ret = cxd2820r_read_ber_t(fe, ber);
+		break;
+	case SYS_DVBT2:
+		ret = cxd2820r_read_ber_t2(fe, ber);
+		break;
+	case SYS_DVBC_ANNEX_AC:
 		ret = cxd2820r_read_ber_c(fe, ber);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
-
 	return ret;
 }
 
 static int cxd2820r_read_signal_strength(struct dvb_frontend *fe, u16 *strength)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		switch (fe->dtv_property_cache.delivery_system) {
-		case SYS_DVBT:
-			ret = cxd2820r_read_signal_strength_t(fe, strength);
-			break;
-		case SYS_DVBT2:
-			ret = cxd2820r_read_signal_strength_t2(fe, strength);
-			break;
-		default:
-			ret = -EINVAL;
-		}
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
 
+	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
+	switch (fe->dtv_property_cache.delivery_system) {
+	case SYS_DVBT:
+		ret = cxd2820r_read_signal_strength_t(fe, strength);
+		break;
+	case SYS_DVBT2:
+		ret = cxd2820r_read_signal_strength_t2(fe, strength);
+		break;
+	case SYS_DVBC_ANNEX_AC:
 		ret = cxd2820r_read_signal_strength_c(fe, strength);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
-
 	return ret;
 }
 
 static int cxd2820r_read_snr(struct dvb_frontend *fe, u16 *snr)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		switch (fe->dtv_property_cache.delivery_system) {
-		case SYS_DVBT:
-			ret = cxd2820r_read_snr_t(fe, snr);
-			break;
-		case SYS_DVBT2:
-			ret = cxd2820r_read_snr_t2(fe, snr);
-			break;
-		default:
-			ret = -EINVAL;
-		}
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
 
+	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
+	switch (fe->dtv_property_cache.delivery_system) {
+	case SYS_DVBT:
+		ret = cxd2820r_read_snr_t(fe, snr);
+		break;
+	case SYS_DVBT2:
+		ret = cxd2820r_read_snr_t2(fe, snr);
+		break;
+	case SYS_DVBC_ANNEX_AC:
 		ret = cxd2820r_read_snr_c(fe, snr);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
-
 	return ret;
 }
 
 static int cxd2820r_read_ucblocks(struct dvb_frontend *fe, u32 *ucblocks)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		switch (fe->dtv_property_cache.delivery_system) {
-		case SYS_DVBT:
-			ret = cxd2820r_read_ucblocks_t(fe, ucblocks);
-			break;
-		case SYS_DVBT2:
-			ret = cxd2820r_read_ucblocks_t2(fe, ucblocks);
-			break;
-		default:
-			ret = -EINVAL;
-		}
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
 
+	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
+	switch (fe->dtv_property_cache.delivery_system) {
+	case SYS_DVBT:
+		ret = cxd2820r_read_ucblocks_t(fe, ucblocks);
+		break;
+	case SYS_DVBT2:
+		ret = cxd2820r_read_ucblocks_t2(fe, ucblocks);
+		break;
+	case SYS_DVBC_ANNEX_AC:
 		ret = cxd2820r_read_ucblocks_c(fe, ucblocks);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
-
 	return ret;
 }
 
 static int cxd2820r_init(struct dvb_frontend *fe)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
-	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	priv->delivery_system = SYS_UNDEFINED;
-	/* delivery system is unknown at that (init) phase */
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		ret = cxd2820r_init_t(fe);
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
-
-		ret = cxd2820r_init_c(fe);
-	}
-
-	return ret;
+	return 0;
 }
 
 static int cxd2820r_sleep(struct dvb_frontend *fe)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		switch (fe->dtv_property_cache.delivery_system) {
-		case SYS_DVBT:
-			ret = cxd2820r_sleep_t(fe);
-			break;
-		case SYS_DVBT2:
-			ret = cxd2820r_sleep_t2(fe);
-			break;
-		default:
-			ret = -EINVAL;
-		}
-
-		cxd2820r_unlock(priv, 0);
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
 
+	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
+	switch (fe->dtv_property_cache.delivery_system) {
+	case SYS_DVBT:
+		ret = cxd2820r_sleep_t(fe);
+		break;
+	case SYS_DVBT2:
+		ret = cxd2820r_sleep_t2(fe);
+		break;
+	case SYS_DVBC_ANNEX_AC:
 		ret = cxd2820r_sleep_c(fe);
-
-		cxd2820r_unlock(priv, 1);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
-
 	return ret;
 }
 
 static int cxd2820r_get_tune_settings(struct dvb_frontend *fe,
-	struct dvb_frontend_tune_settings *s)
+				      struct dvb_frontend_tune_settings *s)
 {
-	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	int ret;
-	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
-
-	if (fe->ops.info.type == FE_OFDM) {
-		/* DVB-T/T2 */
-		ret = cxd2820r_lock(priv, 0);
-		if (ret)
-			return ret;
-
-		switch (fe->dtv_property_cache.delivery_system) {
-		case SYS_DVBT:
-			ret = cxd2820r_get_tune_settings_t(fe, s);
-			break;
-		case SYS_DVBT2:
-			ret = cxd2820r_get_tune_settings_t2(fe, s);
-			break;
-		default:
-			ret = -EINVAL;
-		}
-	} else {
-		/* DVB-C */
-		ret = cxd2820r_lock(priv, 1);
-		if (ret)
-			return ret;
 
+	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
+	switch (fe->dtv_property_cache.delivery_system) {
+	case SYS_DVBT:
+		ret = cxd2820r_get_tune_settings_t(fe, s);
+		break;
+	case SYS_DVBT2:
+		ret = cxd2820r_get_tune_settings_t2(fe, s);
+		break;
+	case SYS_DVBC_ANNEX_AC:
 		ret = cxd2820r_get_tune_settings_c(fe, s);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
 	}
-
 	return ret;
 }
 
 static enum dvbfe_search cxd2820r_search(struct dvb_frontend *fe,
-	struct dvb_frontend_parameters *p)
+					 struct dvb_frontend_parameters *p)
 {
 	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
@@ -664,7 +479,7 @@ static enum dvbfe_search cxd2820r_search(struct dvb_frontend *fe,
 	dbg("%s: delsys=%d", __func__, fe->dtv_property_cache.delivery_system);
 
 	/* switch between DVB-T and DVB-T2 when tune fails */
-	if (priv->last_tune_failed) {
+	if (priv->last_tune_failed && (priv->delivery_system != SYS_DVBC_ANNEX_AC)) {
 		if (priv->delivery_system == SYS_DVBT)
 			c->delivery_system = SYS_DVBT2;
 		else
@@ -727,9 +542,7 @@ static void cxd2820r_release(struct dvb_frontend *fe)
 	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	dbg("%s", __func__);
 
-	if (fe->ops.info.type == FE_OFDM)
-		kfree(priv);
-
+	kfree(priv);
 	return;
 }
 
@@ -742,128 +555,98 @@ static int cxd2820r_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
 	return cxd2820r_wr_reg_mask(priv, 0xdb, enable ? 1 : 0, 0x1);
 }
 
-static const struct dvb_frontend_ops cxd2820r_ops[2];
-
-struct dvb_frontend *cxd2820r_attach(const struct cxd2820r_config *cfg,
-	struct i2c_adapter *i2c, struct dvb_frontend *fe)
+static int cxd2820r_get_property(struct dvb_frontend *fe, struct dtv_property *p)
 {
-	int ret;
-	struct cxd2820r_priv *priv = NULL;
-	u8 tmp;
+	dbg("%s()\n", __func__);
+
+	switch (p->cmd) {
+	case DTV_ENUM_DELSYS:
+		p->u.buffer.data[0] = SYS_DVBT;
+		p->u.buffer.data[1] = SYS_DVBT2;
+		p->u.buffer.data[2] = SYS_DVBC_ANNEX_AC;
+		p->u.buffer.len = 3;
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
 
-	if (fe == NULL) {
-		/* FE0 */
-		/* allocate memory for the internal priv */
-		priv = kzalloc(sizeof(struct cxd2820r_priv), GFP_KERNEL);
-		if (priv == NULL)
-			goto error;
+static const struct dvb_frontend_ops cxd2820r_ops = {
+	/* default: DVB-T/T2 */
+	.info = {
+		.name = "Sony CXD2820R (DVB-T/T2)",
+		.type = FE_OFDM,
+
+		.caps =	FE_CAN_FEC_1_2			|
+			FE_CAN_FEC_2_3			|
+			FE_CAN_FEC_3_4			|
+			FE_CAN_FEC_5_6			|
+			FE_CAN_FEC_7_8			|
+			FE_CAN_FEC_AUTO			|
+			FE_CAN_QPSK			|
+			FE_CAN_QAM_16			|
+			FE_CAN_QAM_64			|
+			FE_CAN_QAM_256			|
+			FE_CAN_QAM_AUTO			|
+			FE_CAN_TRANSMISSION_MODE_AUTO	|
+			FE_CAN_GUARD_INTERVAL_AUTO	|
+			FE_CAN_HIERARCHY_AUTO		|
+			FE_CAN_MUTE_TS			|
+			FE_CAN_2G_MODULATION
+		},
 
-		/* setup the priv */
-		priv->i2c = i2c;
-		memcpy(&priv->cfg, cfg, sizeof(struct cxd2820r_config));
-		mutex_init(&priv->fe_lock);
+	.release		= cxd2820r_release,
+	.init			= cxd2820r_init,
+	.sleep			= cxd2820r_sleep,
 
-		priv->active_fe = -1; /* NONE */
+	.get_tune_settings	= cxd2820r_get_tune_settings,
+	.i2c_gate_ctrl		= cxd2820r_i2c_gate_ctrl,
 
-		/* check if the demod is there */
-		priv->bank[0] = priv->bank[1] = 0xff;
-		ret = cxd2820r_rd_reg(priv, 0x000fd, &tmp);
-		dbg("%s: chip id=%02x", __func__, tmp);
-		if (ret || tmp != 0xe1)
-			goto error;
+	.get_frontend		= cxd2820r_get_frontend,
 
-		/* create frontends */
-		memcpy(&priv->fe[0].ops, &cxd2820r_ops[0],
-			sizeof(struct dvb_frontend_ops));
-		memcpy(&priv->fe[1].ops, &cxd2820r_ops[1],
-			sizeof(struct dvb_frontend_ops));
+	.get_frontend_algo	= cxd2820r_get_frontend_algo,
+	.search			= cxd2820r_search,
 
-		priv->fe[0].demodulator_priv = priv;
-		priv->fe[1].demodulator_priv = priv;
+	.read_status		= cxd2820r_read_status,
+	.read_snr		= cxd2820r_read_snr,
+	.read_ber		= cxd2820r_read_ber,
+	.read_ucblocks		= cxd2820r_read_ucblocks,
+	.read_signal_strength	= cxd2820r_read_signal_strength,
 
-		return &priv->fe[0];
+	.get_property		= cxd2820r_get_property,
+};
 
-	} else {
-		/* FE1: FE0 given as pointer, just return FE1 we have
-		 * already created */
-		priv = fe->demodulator_priv;
-		return &priv->fe[1];
-	}
+struct dvb_frontend *cxd2820r_attach(const struct cxd2820r_config *cfg,
+				     struct i2c_adapter *i2c,
+				     struct dvb_frontend *fe)
+{
+	struct cxd2820r_priv *priv = NULL;
+	int ret;
+	u8 tmp;
+
+	priv = kzalloc(sizeof (struct cxd2820r_priv), GFP_KERNEL);
+	if (!priv)
+		goto error;
 
+	priv->i2c = i2c;
+	memcpy(&priv->cfg, cfg, sizeof (struct cxd2820r_config));
+
+	priv->bank[0] = priv->bank[1] = 0xff;
+	ret = cxd2820r_rd_reg(priv, 0x000fd, &tmp);
+	dbg("%s: chip id=%02x", __func__, tmp);
+	if (ret || tmp != 0xe1)
+		goto error;
+
+	memcpy(&priv->fe.ops, &cxd2820r_ops, sizeof (struct dvb_frontend_ops));
+	priv->fe.demodulator_priv = priv;
+	return &priv->fe;
 error:
 	kfree(priv);
 	return NULL;
 }
 EXPORT_SYMBOL(cxd2820r_attach);
 
-static const struct dvb_frontend_ops cxd2820r_ops[2] = {
-	{
-		/* DVB-T/T2 */
-		.info = {
-			.name = "Sony CXD2820R (DVB-T/T2)",
-			.type = FE_OFDM,
-			.caps =
-				FE_CAN_FEC_1_2 | FE_CAN_FEC_2_3 |
-				FE_CAN_FEC_3_4 | FE_CAN_FEC_5_6 |
-				FE_CAN_FEC_7_8 | FE_CAN_FEC_AUTO |
-				FE_CAN_QPSK | FE_CAN_QAM_16 |
-				FE_CAN_QAM_64 | FE_CAN_QAM_256 |
-				FE_CAN_QAM_AUTO |
-				FE_CAN_TRANSMISSION_MODE_AUTO |
-				FE_CAN_GUARD_INTERVAL_AUTO |
-				FE_CAN_HIERARCHY_AUTO |
-				FE_CAN_MUTE_TS |
-				FE_CAN_2G_MODULATION
-		},
-
-		.release = cxd2820r_release,
-		.init = cxd2820r_init,
-		.sleep = cxd2820r_sleep,
-
-		.get_tune_settings = cxd2820r_get_tune_settings,
-		.i2c_gate_ctrl = cxd2820r_i2c_gate_ctrl,
-
-		.get_frontend = cxd2820r_get_frontend,
-
-		.get_frontend_algo = cxd2820r_get_frontend_algo,
-		.search = cxd2820r_search,
-
-		.read_status = cxd2820r_read_status,
-		.read_snr = cxd2820r_read_snr,
-		.read_ber = cxd2820r_read_ber,
-		.read_ucblocks = cxd2820r_read_ucblocks,
-		.read_signal_strength = cxd2820r_read_signal_strength,
-	},
-	{
-		/* DVB-C */
-		.info = {
-			.name = "Sony CXD2820R (DVB-C)",
-			.type = FE_QAM,
-			.caps =
-				FE_CAN_QAM_16 | FE_CAN_QAM_32 | FE_CAN_QAM_64 |
-				FE_CAN_QAM_128 | FE_CAN_QAM_256 |
-				FE_CAN_FEC_AUTO
-		},
-
-		.release = cxd2820r_release,
-		.init = cxd2820r_init,
-		.sleep = cxd2820r_sleep,
-
-		.get_tune_settings = cxd2820r_get_tune_settings,
-		.i2c_gate_ctrl = cxd2820r_i2c_gate_ctrl,
-
-		.set_frontend = cxd2820r_set_frontend,
-		.get_frontend = cxd2820r_get_frontend,
-
-		.read_status = cxd2820r_read_status,
-		.read_snr = cxd2820r_read_snr,
-		.read_ber = cxd2820r_read_ber,
-		.read_ucblocks = cxd2820r_read_ucblocks,
-		.read_signal_strength = cxd2820r_read_signal_strength,
-	},
-};
-
-
 MODULE_AUTHOR("Antti Palosaari <crope@iki.fi>");
 MODULE_DESCRIPTION("Sony CXD2820R demodulator driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/media/dvb/frontends/cxd2820r_priv.h b/drivers/media/dvb/frontends/cxd2820r_priv.h
index 9553913..94dcf7f 100644
--- a/drivers/media/dvb/frontends/cxd2820r_priv.h
+++ b/drivers/media/dvb/frontends/cxd2820r_priv.h
@@ -48,12 +48,9 @@ struct reg_val_mask {
 
 struct cxd2820r_priv {
 	struct i2c_adapter *i2c;
-	struct dvb_frontend fe[2];
+	struct dvb_frontend fe;
 	struct cxd2820r_config cfg;
 
-	struct mutex fe_lock; /* FE lock */
-	int active_fe:2; /* FE lock, -1=NONE, 0=DVB-T/T2, 1=DVB-C */
-
 	bool ber_running;
 
 	u8 bank[2];
diff --git a/drivers/media/dvb/frontends/cxd2820r_t.c b/drivers/media/dvb/frontends/cxd2820r_t.c
index a04f9c8..c153f0a 100644
--- a/drivers/media/dvb/frontends/cxd2820r_t.c
+++ b/drivers/media/dvb/frontends/cxd2820r_t.c
@@ -22,7 +22,7 @@
 #include "cxd2820r_priv.h"
 
 int cxd2820r_set_frontend_t(struct dvb_frontend *fe,
-	struct dvb_frontend_parameters *p)
+			    struct dvb_frontend_parameters *p)
 {
 	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
diff --git a/drivers/media/dvb/frontends/cxd2820r_t2.c b/drivers/media/dvb/frontends/cxd2820r_t2.c
index 6548588..583b5d2 100644
--- a/drivers/media/dvb/frontends/cxd2820r_t2.c
+++ b/drivers/media/dvb/frontends/cxd2820r_t2.c
@@ -22,7 +22,7 @@
 #include "cxd2820r_priv.h"
 
 int cxd2820r_set_frontend_t2(struct dvb_frontend *fe,
-	struct dvb_frontend_parameters *params)
+			     struct dvb_frontend_parameters *params)
 {
 	struct cxd2820r_priv *priv = fe->demodulator_priv;
 	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-- 
1.7.1

Re: v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Antti Palosaari]

Hello Manu,
That patch looks now much acceptable than the older for my eyes, since 
you removed that .set_state() (change from .set_params() to 
.set_state()) I criticized. Thanks!


On 12/10/2011 06:44 AM, Manu Abraham wrote:
>   static int cxd2820r_set_frontend(struct dvb_frontend *fe,
[...]
> +	switch (c->delivery_system) {
> +	case SYS_DVBT:
> +		ret = cxd2820r_init_t(fe);

> +		ret = cxd2820r_set_frontend_t(fe, p);


Anyhow, I don't now like idea you have put .init() calls to 
.set_frontend(). Could you move .init() happen in .init() callback as it 
was earlier?


regards
Antti
-- 
http://palosaari.fi/

Re: v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Mauro Carvalho Chehab]

On 10-12-2011 09:47, Antti Palosaari wrote:
> Hello Manu,
> That patch looks now much acceptable than the older for my eyes, since you removed that .set_state() (change from .set_params() to .set_state()) I criticized. Thanks!
>
>
> On 12/10/2011 06:44 AM, Manu Abraham wrote:
>> static int cxd2820r_set_frontend(struct dvb_frontend *fe,
> [...]
>> + switch (c->delivery_system) {
>> + case SYS_DVBT:
>> + ret = cxd2820r_init_t(fe);
>
>> + ret = cxd2820r_set_frontend_t(fe, p);
>
>
> Anyhow, I don't now like idea you have put .init() calls to .set_frontend(). Could you move .init() happen in .init() callback as it was earlier?

Changing .init() to happen at set_frontend() actually makes sense for me ;)

Why didn't you like this change?

Regards,
Mauro

Re: v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Antti Palosaari]

On 12/10/2011 02:47 PM, Mauro Carvalho Chehab wrote:
> On 10-12-2011 09:47, Antti Palosaari wrote:
>> Hello Manu,
>> That patch looks now much acceptable than the older for my eyes, since
>> you removed that .set_state() (change from .set_params() to
>> .set_state()) I criticized. Thanks!
>>
>>
>> On 12/10/2011 06:44 AM, Manu Abraham wrote:
>>> static int cxd2820r_set_frontend(struct dvb_frontend *fe,
>> [...]
>>> + switch (c->delivery_system) {
>>> + case SYS_DVBT:
>>> + ret = cxd2820r_init_t(fe);
>>
>>> + ret = cxd2820r_set_frontend_t(fe, p);
>>
>>
>> Anyhow, I don't now like idea you have put .init() calls to
>> .set_frontend(). Could you move .init() happen in .init() callback as
>> it was earlier?
>
> Changing .init() to happen at set_frontend() actually makes sense for me ;)
>
> Why didn't you like this change?

Because there is already callback named .init() just for that as I 
guess. .init() differs from .set_frontend() as init is called only once 
when device is opened whilst .set_frontend() is called every time when 
tuning request is done.

Anyhow, it is up to decision remove .init() from the frontends callbacks 
as we can live without.

regards
Antti

-- 
http://palosaari.fi/

Re: v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Mauro Carvalho Chehab]

On 10-12-2011 11:09, Antti Palosaari wrote:
> On 12/10/2011 02:47 PM, Mauro Carvalho Chehab wrote:
>> On 10-12-2011 09:47, Antti Palosaari wrote:
>>> Hello Manu,
>>> That patch looks now much acceptable than the older for my eyes, since
>>> you removed that .set_state() (change from .set_params() to
>>> .set_state()) I criticized. Thanks!
>>>
>>>
>>> On 12/10/2011 06:44 AM, Manu Abraham wrote:
>>>> static int cxd2820r_set_frontend(struct dvb_frontend *fe,
>>> [...]
>>>> + switch (c->delivery_system) {
>>>> + case SYS_DVBT:
>>>> + ret = cxd2820r_init_t(fe);
>>>
>>>> + ret = cxd2820r_set_frontend_t(fe, p);
>>>
>>>
>>> Anyhow, I don't now like idea you have put .init() calls to
>>> .set_frontend(). Could you move .init() happen in .init() callback as
>>> it was earlier?
>>
>> Changing .init() to happen at set_frontend() actually makes sense for me ;)
>>
>> Why didn't you like this change?
>
> Because there is already callback named .init() just for that as I guess. .init() differs from .set_frontend() as init is called only once when device is opened whilst .set_frontend() is called every time when tuning request is done.
>
> Anyhow, it is up to decision remove .init() from the frontends callbacks as we can live without.

IMO, it makes sense to not do any init before set_frontend on most devices.
An obvious exception would be if is there any device where the capabilities need
to be discovered at runtime.

Regards,
Mauro

Re: v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Manu Abraham]

On Sat, Dec 10, 2011 at 5:17 PM, Antti Palosaari <crope@iki.fi> wrote:
> Hello Manu,
> That patch looks now much acceptable than the older for my eyes, since you
> removed that .set_state() (change from .set_params() to .set_state()) I
> criticized. Thanks!
>


:-)

>
> On 12/10/2011 06:44 AM, Manu Abraham wrote:
>>
>>  static int cxd2820r_set_frontend(struct dvb_frontend *fe,
>
> [...]
>>
>> +       switch (c->delivery_system) {
>> +       case SYS_DVBT:
>> +               ret = cxd2820r_init_t(fe);
>
>
>> +               ret = cxd2820r_set_frontend_t(fe, p);
>
>
>
> Anyhow, I don't now like idea you have put .init() calls to .set_frontend().
> Could you move .init() happen in .init() callback as it was earlier?

This was there in the earlier patch as well. Maybe you have a
new issue now ? ;-)

ok.

The argument what you make doesn't hold well, Why ?

int cxd2820r_init_t(struct dvb_frontend *fe)
{
	ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
}


int cxd2820r_init_c(struct dvb_frontend *fe)
{
	ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
}


Now, you might like to point that, the Base I2C address location
is different comparing DVB-T/DVBT2 to DVB-C

So, If you have the init as in earlier with a common init, then you
will likely init the wrong device at .init(), as init is called open().
So, this might result in an additional register write, which could
be avoided altogether.  One register access is not definitely
something to brag about, but is definitely a small incremental
difference. Other than that this register write doesn't do anything
more than an ADC_START. So starting the ADC at init doesn't
make sense. But does so when you want to select the right ADC.
So definitely, this change is an improvement. Also, you can
compare the time taken for the device to tune now. It is quite
a lot faster compared to without this patch. So you or any other
user should be happy. :-)


I don't think that in any way, the init should be used at init as
you say, which sounds pretty much incorrect.

Re: v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Andreas Oberritter]

On 12.12.2011 05:28, Manu Abraham wrote:
> On Sat, Dec 10, 2011 at 5:17 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 12/10/2011 06:44 AM, Manu Abraham wrote:
>>>
>>>  static int cxd2820r_set_frontend(struct dvb_frontend *fe,
>>
>> [...]
>>>
>>> +       switch (c->delivery_system) {
>>> +       case SYS_DVBT:
>>> +               ret = cxd2820r_init_t(fe);
>>
>>
>>> +               ret = cxd2820r_set_frontend_t(fe, p);
>>
>>
>>
>> Anyhow, I don't now like idea you have put .init() calls to .set_frontend().
>> Could you move .init() happen in .init() callback as it was earlier?
> 
> This was there in the earlier patch as well. Maybe you have a
> new issue now ? ;-)
> 
> ok.
> 
> The argument what you make doesn't hold well, Why ?
> 
> int cxd2820r_init_t(struct dvb_frontend *fe)
> {
> 	ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
> }
> 
> 
> int cxd2820r_init_c(struct dvb_frontend *fe)
> {
> 	ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
> }
> 
> 
> Now, you might like to point that, the Base I2C address location
> is different comparing DVB-T/DVBT2 to DVB-C
> 
> So, If you have the init as in earlier with a common init, then you
> will likely init the wrong device at .init(), as init is called open().
> So, this might result in an additional register write, which could
> be avoided altogether.  One register access is not definitely
> something to brag about, but is definitely a small incremental
> difference. Other than that this register write doesn't do anything
> more than an ADC_START. So starting the ADC at init doesn't
> make sense. But does so when you want to select the right ADC.
> So definitely, this change is an improvement. Also, you can
> compare the time taken for the device to tune now. It is quite
> a lot faster compared to without this patch. So you or any other
> user should be happy. :-)
> 
> 
> I don't think that in any way, the init should be used at init as
> you say, which sounds pretty much incorrect.

Maybe the function names should be modified to avoid confusion with the
init driver callback.

Regards,
Andreas

Re: v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Manu Abraham]

On Mon, Dec 12, 2011 at 6:13 PM, Andreas Oberritter <obi@linuxtv.org> wrote:
> On 12.12.2011 05:28, Manu Abraham wrote:
>> On Sat, Dec 10, 2011 at 5:17 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> On 12/10/2011 06:44 AM, Manu Abraham wrote:
>>>>
>>>>  static int cxd2820r_set_frontend(struct dvb_frontend *fe,
>>>
>>> [...]
>>>>
>>>> +       switch (c->delivery_system) {
>>>> +       case SYS_DVBT:
>>>> +               ret = cxd2820r_init_t(fe);
>>>
>>>
>>>> +               ret = cxd2820r_set_frontend_t(fe, p);
>>>
>>>
>>>
>>> Anyhow, I don't now like idea you have put .init() calls to .set_frontend().
>>> Could you move .init() happen in .init() callback as it was earlier?
>>
>> This was there in the earlier patch as well. Maybe you have a
>> new issue now ? ;-)
>>
>> ok.
>>
>> The argument what you make doesn't hold well, Why ?
>>
>> int cxd2820r_init_t(struct dvb_frontend *fe)
>> {
>>       ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
>> }
>>
>>
>> int cxd2820r_init_c(struct dvb_frontend *fe)
>> {
>>       ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
>> }
>>
>>
>> Now, you might like to point that, the Base I2C address location
>> is different comparing DVB-T/DVBT2 to DVB-C
>>
>> So, If you have the init as in earlier with a common init, then you
>> will likely init the wrong device at .init(), as init is called open().
>> So, this might result in an additional register write, which could
>> be avoided altogether.  One register access is not definitely
>> something to brag about, but is definitely a small incremental
>> difference. Other than that this register write doesn't do anything
>> more than an ADC_START. So starting the ADC at init doesn't
>> make sense. But does so when you want to select the right ADC.
>> So definitely, this change is an improvement. Also, you can
>> compare the time taken for the device to tune now. It is quite
>> a lot faster compared to without this patch. So you or any other
>> user should be happy. :-)
>>
>>
>> I don't think that in any way, the init should be used at init as
>> you say, which sounds pretty much incorrect.
>
> Maybe the function names should be modified to avoid confusion with the
> init driver callback.


On another tangential thought, Is it really worth to wrap that single
register write with another function name ?

instead of the current usage; ie,

ret = cxd2820r_wr_reg(priv, 0x00085, 0x07); /* Start ADC */

within set_frontend()

in set_frontend(), another thing that's wrapped up similarly is
the set_frontend() within the search() callback, which causes
another set of confusions within the driver.


Regards,
Manu

Re: v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Antti Palosaari]

On 12/12/2011 02:55 PM, Manu Abraham wrote:
> On Mon, Dec 12, 2011 at 6:13 PM, Andreas Oberritter<obi@linuxtv.org>  wrote:
>> On 12.12.2011 05:28, Manu Abraham wrote:
>>> On Sat, Dec 10, 2011 at 5:17 PM, Antti Palosaari<crope@iki.fi>  wrote:
>>>> On 12/10/2011 06:44 AM, Manu Abraham wrote:
>>>>>
>>>>>   static int cxd2820r_set_frontend(struct dvb_frontend *fe,
>>>>
>>>> [...]
>>>>>
>>>>> +       switch (c->delivery_system) {
>>>>> +       case SYS_DVBT:
>>>>> +               ret = cxd2820r_init_t(fe);
>>>>
>>>>
>>>>> +               ret = cxd2820r_set_frontend_t(fe, p);
>>>>
>>>>
>>>>
>>>> Anyhow, I don't now like idea you have put .init() calls to .set_frontend().
>>>> Could you move .init() happen in .init() callback as it was earlier?
>>>
>>> This was there in the earlier patch as well. Maybe you have a
>>> new issue now ? ;-)

You mean I didn't mentioned it when you send first version? Sorry, I 
didn't looked it very carefully since I first meet stuff that was not 
related whole thing, I mean there was that code changing from 
.set_params() to .set_state(). And I stopped reading rest of the patch.


>>>
>>> ok.
>>>
>>> The argument what you make doesn't hold well, Why ?
>>>
>>> int cxd2820r_init_t(struct dvb_frontend *fe)
>>> {
>>>        ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
>>> }
>>>
>>>
>>> int cxd2820r_init_c(struct dvb_frontend *fe)
>>> {
>>>        ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
>>> }
>>>
>>>
>>> Now, you might like to point that, the Base I2C address location
>>> is different comparing DVB-T/DVBT2 to DVB-C
>>>
>>> So, If you have the init as in earlier with a common init, then you
>>> will likely init the wrong device at .init(), as init is called open().
>>> So, this might result in an additional register write, which could
>>> be avoided altogether.  One register access is not definitely
>>> something to brag about, but is definitely a small incremental
>>> difference. Other than that this register write doesn't do anything
>>> more than an ADC_START. So starting the ADC at init doesn't
>>> make sense. But does so when you want to select the right ADC.
>>> So definitely, this change is an improvement. Also, you can
>>> compare the time taken for the device to tune now. It is quite
>>> a lot faster compared to without this patch. So you or any other
>>> user should be happy. :-)
>>>
>>>
>>> I don't think that in any way, the init should be used at init as
>>> you say, which sounds pretty much incorrect.
>>
>> Maybe the function names should be modified to avoid confusion with the
>> init driver callback.
>
>
> On another tangential thought, Is it really worth to wrap that single
> register write with another function name ?
>
> instead of the current usage; ie,
>
> ret = cxd2820r_wr_reg(priv, 0x00085, 0x07); /* Start ADC */
>
> within set_frontend()
>
> in set_frontend(), another thing that's wrapped up similarly is
> the set_frontend() within the search() callback, which causes
> another set of confusions within the driver.

Actually there was was a lot more code first but because I ran problems 
selsys needed for T/T2 init was not known at the time .init() was called 
I moved those set_frontend. I left that in a hope I can later fix 
properly adding more stuff back to init.

That is not functionality issue, it is issue about naming callbacks and 
what is functionality of each callback.
As for these days it have been in my understanding initialization stuff 
are done in .init() and leave as less as possible code to 
.set_frontend(). Leaving set_frontend() handle only tuning requests and 
reconfigure IF control etc. And if you look most demod drivers there is 
rather similar logic used.

So I would like to ask what is meaning of:
.attach()
* create FE
* no HW init
* as less as possible HW I/O, mainly reading chip ID and nothing more

.init()
* do nothing here?
* download firmware?

.set_frontend()
* program tuner
* init demod?
* tune demod
* download firmware?

.sleep()
* put device sleep mode
* powersave


After all it is just fine for me apply that patch, but I would like to 
get clear idea what is meaning of every single callback we have. And if 
we really end up .init() is not needed and all should be put to 
.set_frontend() when possible it means I have to change all my demod 
drivers and maybe tuner drivers too.

regards
Antti


-- 
http://palosaari.fi/

Re: v4 [PATCH 09/10] CXD2820r: Query DVB frontend delivery capabilities

[Manu Abraham]

On Mon, Dec 12, 2011 at 7:11 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 12/12/2011 02:55 PM, Manu Abraham wrote:
>>
>> On Mon, Dec 12, 2011 at 6:13 PM, Andreas Oberritter<obi@linuxtv.org>
>>  wrote:
>>>
>>> On 12.12.2011 05:28, Manu Abraham wrote:
>>>>
>>>> On Sat, Dec 10, 2011 at 5:17 PM, Antti Palosaari<crope@iki.fi>  wrote:
>>>>>
>>>>> On 12/10/2011 06:44 AM, Manu Abraham wrote:
>>>>>>
>>>>>>
>>>>>>  static int cxd2820r_set_frontend(struct dvb_frontend *fe,
>>>>>
>>>>>
>>>>> [...]
>>>>>>
>>>>>>
>>>>>> +       switch (c->delivery_system) {
>>>>>> +       case SYS_DVBT:
>>>>>> +               ret = cxd2820r_init_t(fe);
>>>>>
>>>>>
>>>>>
>>>>>> +               ret = cxd2820r_set_frontend_t(fe, p);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Anyhow, I don't now like idea you have put .init() calls to
>>>>> .set_frontend().
>>>>> Could you move .init() happen in .init() callback as it was earlier?
>>>>
>>>>
>>>> This was there in the earlier patch as well. Maybe you have a
>>>> new issue now ? ;-)
>
>
> You mean I didn't mentioned it when you send first version? Sorry, I didn't
> looked it very carefully since I first meet stuff that was not related whole
> thing, I mean there was that code changing from .set_params() to
> .set_state(). And I stopped reading rest of the patch.
>
>
>
>>>>
>>>> ok.
>>>>
>>>> The argument what you make doesn't hold well, Why ?
>>>>
>>>> int cxd2820r_init_t(struct dvb_frontend *fe)
>>>> {
>>>>       ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
>>>> }
>>>>
>>>>
>>>> int cxd2820r_init_c(struct dvb_frontend *fe)
>>>> {
>>>>       ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
>>>> }
>>>>
>>>>
>>>> Now, you might like to point that, the Base I2C address location
>>>> is different comparing DVB-T/DVBT2 to DVB-C
>>>>
>>>> So, If you have the init as in earlier with a common init, then you
>>>> will likely init the wrong device at .init(), as init is called open().
>>>> So, this might result in an additional register write, which could
>>>> be avoided altogether.  One register access is not definitely
>>>> something to brag about, but is definitely a small incremental
>>>> difference. Other than that this register write doesn't do anything
>>>> more than an ADC_START. So starting the ADC at init doesn't
>>>> make sense. But does so when you want to select the right ADC.
>>>> So definitely, this change is an improvement. Also, you can
>>>> compare the time taken for the device to tune now. It is quite
>>>> a lot faster compared to without this patch. So you or any other
>>>> user should be happy. :-)
>>>>
>>>>
>>>> I don't think that in any way, the init should be used at init as
>>>> you say, which sounds pretty much incorrect.
>>>
>>>
>>> Maybe the function names should be modified to avoid confusion with the
>>> init driver callback.
>>
>>
>>
>> On another tangential thought, Is it really worth to wrap that single
>> register write with another function name ?
>>
>> instead of the current usage; ie,
>>
>> ret = cxd2820r_wr_reg(priv, 0x00085, 0x07); /* Start ADC */
>>
>> within set_frontend()
>>
>> in set_frontend(), another thing that's wrapped up similarly is
>> the set_frontend() within the search() callback, which causes
>> another set of confusions within the driver.
>
>
> Actually there was was a lot more code first but because I ran problems
> selsys needed for T/T2 init was not known at the time .init() was called I
> moved those set_frontend. I left that in a hope I can later fix properly
> adding more stuff back to init.
>
> That is not functionality issue, it is issue about naming callbacks and what
> is functionality of each callback.
> As for these days it have been in my understanding initialization stuff are
> done in .init() and leave as less as possible code to .set_frontend().
> Leaving set_frontend() handle only tuning requests and reconfigure IF
> control etc. And if you look most demod drivers there is rather similar
> logic used.
>
> So I would like to ask what is meaning of:
> .attach()
> * create FE
> * no HW init
> * as less as possible HW I/O, mainly reading chip ID and nothing more

Generally it should be simply create a DVB frontend data structure,
if it finds a valid device.

In some clunky cases, the demodulator clock would be from the tuner,
in which case the tuner has to be attached prior to the demod; and
then later on read device details, once clocks are setup.


>
> .init()
> * do nothing here?
> * download firmware?
>

init should simply initialize the device in the lowest power
mode, where it is ready to do a tune. (in some cases tuner also
might need an init. In such cases, the tuner_ops.init can be just
called).

> .set_frontend()
> * program tuner
> * init demod?
> * tune demod
> * download firmware?

Ideally set_frontend should just setup the frontend as a whole
(tuner, demod, or maybe more devices) and return status.

In some clunky cases, there could be cases where each tune
needs a firmware download. Maybe this download can be
optimized in some paths. This depends on the device.

>
> .sleep()
> * put device sleep mode
> * powersave

Yep.


>
> After all it is just fine for me apply that patch, but I would like to get
> clear idea what is meaning of every single callback we have. And if we
> really end up .init() is not needed and all should be put to .set_frontend()
> when possible it means I have to change all my demod drivers and maybe tuner
> drivers too.

If the init callback is something simple, that which do not take
much of time for operation completion and that it is dependent
on a delivery system, it then makes sense to have such
operations within set_frontend. If those operations take long
and is common to all supported delivery systems, it might make
more sense to have them within .init()

Regards,
Manu