[PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Antti Palosaari]

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

That commit has just basic binding stuff and driver itself still
needs to be converted more complete later.

Cc: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/tuners/e4000.c            | 73 ++++++++++++++++++++++-----------
 drivers/media/tuners/e4000.h            |  9 +++-
 drivers/media/tuners/e4000_priv.h       |  4 +-
 drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 31 +++++++++-----
 4 files changed, 78 insertions(+), 39 deletions(-)

diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 54e2d8a..f6a5dbd 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -27,7 +27,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
 	u8 buf[1 + len];
 	struct i2c_msg msg[1] = {
 		{
-			.addr = priv->cfg->i2c_addr,
+			.addr = priv->i2c_addr,
 			.flags = 0,
 			.len = sizeof(buf),
 			.buf = buf,
@@ -56,12 +56,12 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
 	u8 buf[len];
 	struct i2c_msg msg[2] = {
 		{
-			.addr = priv->cfg->i2c_addr,
+			.addr = priv->i2c_addr,
 			.flags = 0,
 			.len = 1,
 			.buf = &reg,
 		}, {
-			.addr = priv->cfg->i2c_addr,
+			.addr = priv->i2c_addr,
 			.flags = I2C_M_RD,
 			.len = sizeof(buf),
 			.buf = buf,
@@ -233,8 +233,8 @@ 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;
@@ -358,17 +358,6 @@ static int e4000_get_if_frequency(struct dvb_frontend *fe, u32 *frequency)
 	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",
@@ -376,8 +365,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,
@@ -385,9 +372,12 @@ 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 i2c_adapter *i2c = client->adapter;
 	struct e4000_priv *priv;
 	int ret;
 	u8 chip_id;
@@ -402,7 +392,9 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
 		goto err;
 	}
 
-	priv->cfg = cfg;
+	priv->i2c_addr = cfg->i2c_addr;
+	priv->clock = cfg->clock;
+	priv->i2c_client = client;
 	priv->i2c = i2c;
 
 	/* check if the tuner is there */
@@ -412,8 +404,10 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
 
 	dev_dbg(&priv->i2c->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);
@@ -431,16 +425,45 @@ 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);
 	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);
+
+	dev_dbg(&client->dev, "%s:\n", __func__);
+
+	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..760b206 100644
--- a/drivers/media/tuners/e4000.h
+++ b/drivers/media/tuners/e4000.h
@@ -26,6 +26,11 @@
 
 struct e4000_config {
 	/*
+	 * frontend
+	 */
+	struct dvb_frontend *fe;
+
+	/*
 	 * I2C address
 	 * 0x64, 0x65, 0x66, 0x67
 	 */
@@ -39,10 +44,10 @@ struct e4000_config {
 
 #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);
+		struct i2c_adapter *i2c, 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)
+		struct i2c_adapter *i2c, struct e4000_config *cfg)
 {
 	dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n", __func__);
 	return NULL;
diff --git a/drivers/media/tuners/e4000_priv.h b/drivers/media/tuners/e4000_priv.h
index a385505..7f47068 100644
--- a/drivers/media/tuners/e4000_priv.h
+++ b/drivers/media/tuners/e4000_priv.h
@@ -24,8 +24,10 @@
 #include "e4000.h"
 
 struct e4000_priv {
-	const struct e4000_config *cfg;
 	struct i2c_adapter *i2c;
+	struct i2c_client *i2c_client;
+	u8 i2c_addr;
+	u32 clock;
 };
 
 struct e4000_pll {
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index defc491..573805a 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -843,11 +843,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,
@@ -874,10 +869,14 @@ 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_client *client = 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],
@@ -897,9 +896,20 @@ 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],
+				.i2c_addr = 0x64,
+				.clock = 28800000,
+			};
+
+			strlcpy(info.type, "e4000", I2C_NAME_SIZE);
+			info.addr = 0x64;
+			info.platform_data = &e4000_config;
+
+			request_module("e4000");
+			client = i2c_new_device(&d->i2c_adap, &info);
+		}
 		break;
 	case TUNER_RTL2832_FC2580:
 		fe = dvb_attach(fc2580_attach, adap->fe[0], &d->i2c_adap,
@@ -927,12 +937,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 && (client == NULL || client->driver == NULL)) {
 		ret = -ENODEV;
 		goto err;
 	}
-- 
1.8.3.1

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Mauro Carvalho Chehab]

Em Wed, 16 Oct 2013 01:31:04 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> Initial driver conversion from proprietary DVB tuner model to more
> general I2C driver model.
> 
> That commit has just basic binding stuff and driver itself still
> needs to be converted more complete later.
> 
> Cc: Jean Delvare <khali@linux-fr.org>
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  drivers/media/tuners/e4000.c            | 73 ++++++++++++++++++++++-----------
>  drivers/media/tuners/e4000.h            |  9 +++-
>  drivers/media/tuners/e4000_priv.h       |  4 +-
>  drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 31 +++++++++-----
>  4 files changed, 78 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
> index 54e2d8a..f6a5dbd 100644
> --- a/drivers/media/tuners/e4000.c
> +++ b/drivers/media/tuners/e4000.c
> @@ -27,7 +27,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
>  	u8 buf[1 + len];
>  	struct i2c_msg msg[1] = {
>  		{
> -			.addr = priv->cfg->i2c_addr,
> +			.addr = priv->i2c_addr,
>  			.flags = 0,
>  			.len = sizeof(buf),
>  			.buf = buf,
> @@ -56,12 +56,12 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
>  	u8 buf[len];
>  	struct i2c_msg msg[2] = {
>  		{
> -			.addr = priv->cfg->i2c_addr,
> +			.addr = priv->i2c_addr,
>  			.flags = 0,
>  			.len = 1,
>  			.buf = &reg,
>  		}, {
> -			.addr = priv->cfg->i2c_addr,
> +			.addr = priv->i2c_addr,
>  			.flags = I2C_M_RD,
>  			.len = sizeof(buf),
>  			.buf = buf,
> @@ -233,8 +233,8 @@ 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;
> @@ -358,17 +358,6 @@ static int e4000_get_if_frequency(struct dvb_frontend *fe, u32 *frequency)
>  	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",
> @@ -376,8 +365,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,
> @@ -385,9 +372,12 @@ 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 i2c_adapter *i2c = client->adapter;
>  	struct e4000_priv *priv;
>  	int ret;
>  	u8 chip_id;
> @@ -402,7 +392,9 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>  		goto err;
>  	}
>  
> -	priv->cfg = cfg;
> +	priv->i2c_addr = cfg->i2c_addr;
> +	priv->clock = cfg->clock;
> +	priv->i2c_client = client;
>  	priv->i2c = i2c;
>  
>  	/* check if the tuner is there */
> @@ -412,8 +404,10 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>  
>  	dev_dbg(&priv->i2c->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);
> @@ -431,16 +425,45 @@ 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);
>  	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);
> +
> +	dev_dbg(&client->dev, "%s:\n", __func__);
> +
> +	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..760b206 100644
> --- a/drivers/media/tuners/e4000.h
> +++ b/drivers/media/tuners/e4000.h
> @@ -26,6 +26,11 @@
>  
>  struct e4000_config {
>  	/*
> +	 * frontend
> +	 */
> +	struct dvb_frontend *fe;
> +
> +	/*
>  	 * I2C address
>  	 * 0x64, 0x65, 0x66, 0x67
>  	 */
> @@ -39,10 +44,10 @@ struct e4000_config {
>  
>  #if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)

You can get rid of #if IS_ENABLED, by replacing it by a generic attach
function, using the subdev model. In other words, this is how a tuner
subdev is defined:

	struct v4l2_subdev_tuner_ops {
		int (*s_radio)(struct v4l2_subdev *sd);
		int (*s_frequency)(struct v4l2_subdev *sd, const struct v4l2_frequency *freq);
		int (*g_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency *freq);
		int (*g_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner *vt);
		int (*s_tuner)(struct v4l2_subdev *sd, const struct v4l2_tuner *vt);
		int (*g_modulator)(struct v4l2_subdev *sd, struct v4l2_modulator *vm);
		int (*s_modulator)(struct v4l2_subdev *sd, const struct v4l2_modulator *vm);
		int (*s_type_addr)(struct v4l2_subdev *sd, struct tuner_setup *type);
		int (*s_config)(struct v4l2_subdev *sd, const struct v4l2_priv_tun_config *config);
	};

As the presence of the device is known, there's no need to actually attach
anything, but you need to pass the configuration for the subdevice. This is
done by calling the subdev s_config ops to setup them:

	v4l2_subdev_call(sd, tuner, s_config, vfh, arg);

A new callback will be needed there, in order to put the device into DVB mode,
e. g. passing the DVB specific parameters.

As the subdev callbacks are defined on a generic .h header and they always
exist, if the driver is not compiled, i2c_new_device() won't find the driver,
and the bridge driver won't call s_config() for this I2C client.

That's IMHO, one of the biggest advantages of this conversion: as a bonus,
we get rid of several Kconfig issues. Also, with this change, lsmod will
properly show that the tuner is bound to the bridge driver.

>  extern struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
> -		struct i2c_adapter *i2c, const struct e4000_config *cfg);
> +		struct i2c_adapter *i2c, 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)
> +		struct i2c_adapter *i2c, struct e4000_config *cfg)
>  {
>  	dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n", __func__);
>  	return NULL;
> diff --git a/drivers/media/tuners/e4000_priv.h b/drivers/media/tuners/e4000_priv.h
> index a385505..7f47068 100644
> --- a/drivers/media/tuners/e4000_priv.h
> +++ b/drivers/media/tuners/e4000_priv.h
> @@ -24,8 +24,10 @@
>  #include "e4000.h"
>  
>  struct e4000_priv {
> -	const struct e4000_config *cfg;
>  	struct i2c_adapter *i2c;
> +	struct i2c_client *i2c_client;
> +	u8 i2c_addr;
> +	u32 clock;
>  };
>  
>  struct e4000_pll {
> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> index defc491..573805a 100644
> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> @@ -843,11 +843,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,
> @@ -874,10 +869,14 @@ 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_client *client = 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],
> @@ -897,9 +896,20 @@ 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],
> +				.i2c_addr = 0x64,
> +				.clock = 28800000,
> +			};
> +
> +			strlcpy(info.type, "e4000", I2C_NAME_SIZE);
> +			info.addr = 0x64;
> +			info.platform_data = &e4000_config;
> +
> +			request_module("e4000");
> +			client = i2c_new_device(&d->i2c_adap, &info);
> +		}
>  		break;
>  	case TUNER_RTL2832_FC2580:
>  		fe = dvb_attach(fc2580_attach, adap->fe[0], &d->i2c_adap,
> @@ -927,12 +937,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 && (client == NULL || client->driver == NULL)) {
>  		ret = -ENODEV;
>  		goto err;
>  	}


-- 

Cheers,
Mauro

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Michael Krufky]

This kinda makes me a bit nervous.  The patch itself looks OK but the
cascading effects that it will have across the DVB subsystem need to
be discussed.

Is there a discussion about this kind of conversion on the mailing
list somewhere that I've missed?

-Mike

On Tue, Oct 15, 2013 at 7:33 PM, Mauro Carvalho Chehab
<m.chehab@samsung.com> wrote:
> Em Wed, 16 Oct 2013 01:31:04 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> Initial driver conversion from proprietary DVB tuner model to more
>> general I2C driver model.
>>
>> That commit has just basic binding stuff and driver itself still
>> needs to be converted more complete later.
>>
>> Cc: Jean Delvare <khali@linux-fr.org>
>> Signed-off-by: Antti Palosaari <crope@iki.fi>
>> ---
>>  drivers/media/tuners/e4000.c            | 73 ++++++++++++++++++++++-----------
>>  drivers/media/tuners/e4000.h            |  9 +++-
>>  drivers/media/tuners/e4000_priv.h       |  4 +-
>>  drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 31 +++++++++-----
>>  4 files changed, 78 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
>> index 54e2d8a..f6a5dbd 100644
>> --- a/drivers/media/tuners/e4000.c
>> +++ b/drivers/media/tuners/e4000.c
>> @@ -27,7 +27,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
>>       u8 buf[1 + len];
>>       struct i2c_msg msg[1] = {
>>               {
>> -                     .addr = priv->cfg->i2c_addr,
>> +                     .addr = priv->i2c_addr,
>>                       .flags = 0,
>>                       .len = sizeof(buf),
>>                       .buf = buf,
>> @@ -56,12 +56,12 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
>>       u8 buf[len];
>>       struct i2c_msg msg[2] = {
>>               {
>> -                     .addr = priv->cfg->i2c_addr,
>> +                     .addr = priv->i2c_addr,
>>                       .flags = 0,
>>                       .len = 1,
>>                       .buf = &reg,
>>               }, {
>> -                     .addr = priv->cfg->i2c_addr,
>> +                     .addr = priv->i2c_addr,
>>                       .flags = I2C_M_RD,
>>                       .len = sizeof(buf),
>>                       .buf = buf,
>> @@ -233,8 +233,8 @@ 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;
>> @@ -358,17 +358,6 @@ static int e4000_get_if_frequency(struct dvb_frontend *fe, u32 *frequency)
>>       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",
>> @@ -376,8 +365,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,
>> @@ -385,9 +372,12 @@ 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 i2c_adapter *i2c = client->adapter;
>>       struct e4000_priv *priv;
>>       int ret;
>>       u8 chip_id;
>> @@ -402,7 +392,9 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>>               goto err;
>>       }
>>
>> -     priv->cfg = cfg;
>> +     priv->i2c_addr = cfg->i2c_addr;
>> +     priv->clock = cfg->clock;
>> +     priv->i2c_client = client;
>>       priv->i2c = i2c;
>>
>>       /* check if the tuner is there */
>> @@ -412,8 +404,10 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>>
>>       dev_dbg(&priv->i2c->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);
>> @@ -431,16 +425,45 @@ 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);
>>       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);
>> +
>> +     dev_dbg(&client->dev, "%s:\n", __func__);
>> +
>> +     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..760b206 100644
>> --- a/drivers/media/tuners/e4000.h
>> +++ b/drivers/media/tuners/e4000.h
>> @@ -26,6 +26,11 @@
>>
>>  struct e4000_config {
>>       /*
>> +      * frontend
>> +      */
>> +     struct dvb_frontend *fe;
>> +
>> +     /*
>>        * I2C address
>>        * 0x64, 0x65, 0x66, 0x67
>>        */
>> @@ -39,10 +44,10 @@ struct e4000_config {
>>
>>  #if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
>
> You can get rid of #if IS_ENABLED, by replacing it by a generic attach
> function, using the subdev model. In other words, this is how a tuner
> subdev is defined:
>
>         struct v4l2_subdev_tuner_ops {
>                 int (*s_radio)(struct v4l2_subdev *sd);
>                 int (*s_frequency)(struct v4l2_subdev *sd, const struct v4l2_frequency *freq);
>                 int (*g_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency *freq);
>                 int (*g_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner *vt);
>                 int (*s_tuner)(struct v4l2_subdev *sd, const struct v4l2_tuner *vt);
>                 int (*g_modulator)(struct v4l2_subdev *sd, struct v4l2_modulator *vm);
>                 int (*s_modulator)(struct v4l2_subdev *sd, const struct v4l2_modulator *vm);
>                 int (*s_type_addr)(struct v4l2_subdev *sd, struct tuner_setup *type);
>                 int (*s_config)(struct v4l2_subdev *sd, const struct v4l2_priv_tun_config *config);
>         };
>
> As the presence of the device is known, there's no need to actually attach
> anything, but you need to pass the configuration for the subdevice. This is
> done by calling the subdev s_config ops to setup them:
>
>         v4l2_subdev_call(sd, tuner, s_config, vfh, arg);
>
> A new callback will be needed there, in order to put the device into DVB mode,
> e. g. passing the DVB specific parameters.
>
> As the subdev callbacks are defined on a generic .h header and they always
> exist, if the driver is not compiled, i2c_new_device() won't find the driver,
> and the bridge driver won't call s_config() for this I2C client.
>
> That's IMHO, one of the biggest advantages of this conversion: as a bonus,
> we get rid of several Kconfig issues. Also, with this change, lsmod will
> properly show that the tuner is bound to the bridge driver.
>
>>  extern struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>> -             struct i2c_adapter *i2c, const struct e4000_config *cfg);
>> +             struct i2c_adapter *i2c, 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)
>> +             struct i2c_adapter *i2c, struct e4000_config *cfg)
>>  {
>>       dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n", __func__);
>>       return NULL;
>> diff --git a/drivers/media/tuners/e4000_priv.h b/drivers/media/tuners/e4000_priv.h
>> index a385505..7f47068 100644
>> --- a/drivers/media/tuners/e4000_priv.h
>> +++ b/drivers/media/tuners/e4000_priv.h
>> @@ -24,8 +24,10 @@
>>  #include "e4000.h"
>>
>>  struct e4000_priv {
>> -     const struct e4000_config *cfg;
>>       struct i2c_adapter *i2c;
>> +     struct i2c_client *i2c_client;
>> +     u8 i2c_addr;
>> +     u32 clock;
>>  };
>>
>>  struct e4000_pll {
>> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>> index defc491..573805a 100644
>> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>> @@ -843,11 +843,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,
>> @@ -874,10 +869,14 @@ 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_client *client = 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],
>> @@ -897,9 +896,20 @@ 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],
>> +                             .i2c_addr = 0x64,
>> +                             .clock = 28800000,
>> +                     };
>> +
>> +                     strlcpy(info.type, "e4000", I2C_NAME_SIZE);
>> +                     info.addr = 0x64;
>> +                     info.platform_data = &e4000_config;
>> +
>> +                     request_module("e4000");
>> +                     client = i2c_new_device(&d->i2c_adap, &info);
>> +             }
>>               break;
>>       case TUNER_RTL2832_FC2580:
>>               fe = dvb_attach(fc2580_attach, adap->fe[0], &d->i2c_adap,
>> @@ -927,12 +937,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 && (client == NULL || client->driver == NULL)) {
>>               ret = -ENODEV;
>>               goto err;
>>       }
>
>
> --
>
> Cheers,
> Mauro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Mauro Carvalho Chehab]

Em Wed, 16 Oct 2013 11:54:57 -0400
Michael Krufky <mkrufky@linuxtv.org> escreveu:

> This kinda makes me a bit nervous.  The patch itself looks OK but the
> cascading effects that it will have across the DVB subsystem need to
> be discussed.
> 
> Is there a discussion about this kind of conversion on the mailing
> list somewhere that I've missed?

There were several discussions on IRC about that.

Yeah, those changes require some care, but, on the other hand, they should
solve a series of issues at the DVB side and provide a clearer subsystem
integration.

Regards,
Mauro.

> 
> -Mike
> 
> On Tue, Oct 15, 2013 at 7:33 PM, Mauro Carvalho Chehab
> <m.chehab@samsung.com> wrote:
> > Em Wed, 16 Oct 2013 01:31:04 +0300
> > Antti Palosaari <crope@iki.fi> escreveu:
> >
> >> Initial driver conversion from proprietary DVB tuner model to more
> >> general I2C driver model.
> >>
> >> That commit has just basic binding stuff and driver itself still
> >> needs to be converted more complete later.
> >>
> >> Cc: Jean Delvare <khali@linux-fr.org>
> >> Signed-off-by: Antti Palosaari <crope@iki.fi>
> >> ---
> >>  drivers/media/tuners/e4000.c            | 73 ++++++++++++++++++++++-----------
> >>  drivers/media/tuners/e4000.h            |  9 +++-
> >>  drivers/media/tuners/e4000_priv.h       |  4 +-
> >>  drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 31 +++++++++-----
> >>  4 files changed, 78 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
> >> index 54e2d8a..f6a5dbd 100644
> >> --- a/drivers/media/tuners/e4000.c
> >> +++ b/drivers/media/tuners/e4000.c
> >> @@ -27,7 +27,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
> >>       u8 buf[1 + len];
> >>       struct i2c_msg msg[1] = {
> >>               {
> >> -                     .addr = priv->cfg->i2c_addr,
> >> +                     .addr = priv->i2c_addr,
> >>                       .flags = 0,
> >>                       .len = sizeof(buf),
> >>                       .buf = buf,
> >> @@ -56,12 +56,12 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
> >>       u8 buf[len];
> >>       struct i2c_msg msg[2] = {
> >>               {
> >> -                     .addr = priv->cfg->i2c_addr,
> >> +                     .addr = priv->i2c_addr,
> >>                       .flags = 0,
> >>                       .len = 1,
> >>                       .buf = &reg,
> >>               }, {
> >> -                     .addr = priv->cfg->i2c_addr,
> >> +                     .addr = priv->i2c_addr,
> >>                       .flags = I2C_M_RD,
> >>                       .len = sizeof(buf),
> >>                       .buf = buf,
> >> @@ -233,8 +233,8 @@ 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;
> >> @@ -358,17 +358,6 @@ static int e4000_get_if_frequency(struct dvb_frontend *fe, u32 *frequency)
> >>       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",
> >> @@ -376,8 +365,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,
> >> @@ -385,9 +372,12 @@ 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 i2c_adapter *i2c = client->adapter;
> >>       struct e4000_priv *priv;
> >>       int ret;
> >>       u8 chip_id;
> >> @@ -402,7 +392,9 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
> >>               goto err;
> >>       }
> >>
> >> -     priv->cfg = cfg;
> >> +     priv->i2c_addr = cfg->i2c_addr;
> >> +     priv->clock = cfg->clock;
> >> +     priv->i2c_client = client;
> >>       priv->i2c = i2c;
> >>
> >>       /* check if the tuner is there */
> >> @@ -412,8 +404,10 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
> >>
> >>       dev_dbg(&priv->i2c->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);
> >> @@ -431,16 +425,45 @@ 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);
> >>       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);
> >> +
> >> +     dev_dbg(&client->dev, "%s:\n", __func__);
> >> +
> >> +     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..760b206 100644
> >> --- a/drivers/media/tuners/e4000.h
> >> +++ b/drivers/media/tuners/e4000.h
> >> @@ -26,6 +26,11 @@
> >>
> >>  struct e4000_config {
> >>       /*
> >> +      * frontend
> >> +      */
> >> +     struct dvb_frontend *fe;
> >> +
> >> +     /*
> >>        * I2C address
> >>        * 0x64, 0x65, 0x66, 0x67
> >>        */
> >> @@ -39,10 +44,10 @@ struct e4000_config {
> >>
> >>  #if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
> >
> > You can get rid of #if IS_ENABLED, by replacing it by a generic attach
> > function, using the subdev model. In other words, this is how a tuner
> > subdev is defined:
> >
> >         struct v4l2_subdev_tuner_ops {
> >                 int (*s_radio)(struct v4l2_subdev *sd);
> >                 int (*s_frequency)(struct v4l2_subdev *sd, const struct v4l2_frequency *freq);
> >                 int (*g_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency *freq);
> >                 int (*g_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner *vt);
> >                 int (*s_tuner)(struct v4l2_subdev *sd, const struct v4l2_tuner *vt);
> >                 int (*g_modulator)(struct v4l2_subdev *sd, struct v4l2_modulator *vm);
> >                 int (*s_modulator)(struct v4l2_subdev *sd, const struct v4l2_modulator *vm);
> >                 int (*s_type_addr)(struct v4l2_subdev *sd, struct tuner_setup *type);
> >                 int (*s_config)(struct v4l2_subdev *sd, const struct v4l2_priv_tun_config *config);
> >         };
> >
> > As the presence of the device is known, there's no need to actually attach
> > anything, but you need to pass the configuration for the subdevice. This is
> > done by calling the subdev s_config ops to setup them:
> >
> >         v4l2_subdev_call(sd, tuner, s_config, vfh, arg);
> >
> > A new callback will be needed there, in order to put the device into DVB mode,
> > e. g. passing the DVB specific parameters.
> >
> > As the subdev callbacks are defined on a generic .h header and they always
> > exist, if the driver is not compiled, i2c_new_device() won't find the driver,
> > and the bridge driver won't call s_config() for this I2C client.
> >
> > That's IMHO, one of the biggest advantages of this conversion: as a bonus,
> > we get rid of several Kconfig issues. Also, with this change, lsmod will
> > properly show that the tuner is bound to the bridge driver.
> >
> >>  extern struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
> >> -             struct i2c_adapter *i2c, const struct e4000_config *cfg);
> >> +             struct i2c_adapter *i2c, 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)
> >> +             struct i2c_adapter *i2c, struct e4000_config *cfg)
> >>  {
> >>       dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n", __func__);
> >>       return NULL;
> >> diff --git a/drivers/media/tuners/e4000_priv.h b/drivers/media/tuners/e4000_priv.h
> >> index a385505..7f47068 100644
> >> --- a/drivers/media/tuners/e4000_priv.h
> >> +++ b/drivers/media/tuners/e4000_priv.h
> >> @@ -24,8 +24,10 @@
> >>  #include "e4000.h"
> >>
> >>  struct e4000_priv {
> >> -     const struct e4000_config *cfg;
> >>       struct i2c_adapter *i2c;
> >> +     struct i2c_client *i2c_client;
> >> +     u8 i2c_addr;
> >> +     u32 clock;
> >>  };
> >>
> >>  struct e4000_pll {
> >> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> >> index defc491..573805a 100644
> >> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> >> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
> >> @@ -843,11 +843,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,
> >> @@ -874,10 +869,14 @@ 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_client *client = 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],
> >> @@ -897,9 +896,20 @@ 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],
> >> +                             .i2c_addr = 0x64,
> >> +                             .clock = 28800000,
> >> +                     };
> >> +
> >> +                     strlcpy(info.type, "e4000", I2C_NAME_SIZE);
> >> +                     info.addr = 0x64;
> >> +                     info.platform_data = &e4000_config;
> >> +
> >> +                     request_module("e4000");
> >> +                     client = i2c_new_device(&d->i2c_adap, &info);
> >> +             }
> >>               break;
> >>       case TUNER_RTL2832_FC2580:
> >>               fe = dvb_attach(fc2580_attach, adap->fe[0], &d->i2c_adap,
> >> @@ -927,12 +937,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 && (client == NULL || client->driver == NULL)) {
> >>               ret = -ENODEV;
> >>               goto err;
> >>       }
> >
> >
> > --
> >
> > Cheers,
> > Mauro
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Cheers,
Mauro

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Antti Palosaari]

On 16.10.2013 18:54, Michael Krufky wrote:
> This kinda makes me a bit nervous.  The patch itself looks OK but the
> cascading effects that it will have across the DVB subsystem need to
> be discussed.

Basically, the only effect is that you must use i2c_new_device() or 
i2c_new_probed_device() instead of old proprietary dvb_attach(). Convert 
old proprietary method with Kernel standard one.

But thats not all. It was only first step. My final goal is to study, 
test and finally make radio tuners as a own "standalone" I2C drivers. 
This means that I will remove DVB frontend dependency next from the 
given tuner driver. And before you ask why: I need some more 
sophisticated access to tuner in order to meet SDR needs. Also I don't 
see any reason why those tuners should be a part of DVB API or part of 
V4L2 API (or even both in case of "hybrid" model). Actual reasons are 
likely payload from the history, but as of today view it is stupid design :]

> Is there a discussion about this kind of conversion on the mailing
> list somewhere that I've missed?

There has been many times around the years. i2c_new_probed_device() 
function [1] seems to be added for somehow for out needs too...

[1] http://lists.lm-sensors.org/pipermail/i2c/2007-March/000990.html


regards
Antti

>
> -Mike
>
> On Tue, Oct 15, 2013 at 7:33 PM, Mauro Carvalho Chehab
> <m.chehab@samsung.com> wrote:
>> Em Wed, 16 Oct 2013 01:31:04 +0300
>> Antti Palosaari <crope@iki.fi> escreveu:
>>
>>> Initial driver conversion from proprietary DVB tuner model to more
>>> general I2C driver model.
>>>
>>> That commit has just basic binding stuff and driver itself still
>>> needs to be converted more complete later.
>>>
>>> Cc: Jean Delvare <khali@linux-fr.org>
>>> Signed-off-by: Antti Palosaari <crope@iki.fi>
>>> ---
>>>   drivers/media/tuners/e4000.c            | 73 ++++++++++++++++++++++-----------
>>>   drivers/media/tuners/e4000.h            |  9 +++-
>>>   drivers/media/tuners/e4000_priv.h       |  4 +-
>>>   drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 31 +++++++++-----
>>>   4 files changed, 78 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
>>> index 54e2d8a..f6a5dbd 100644
>>> --- a/drivers/media/tuners/e4000.c
>>> +++ b/drivers/media/tuners/e4000.c
>>> @@ -27,7 +27,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
>>>        u8 buf[1 + len];
>>>        struct i2c_msg msg[1] = {
>>>                {
>>> -                     .addr = priv->cfg->i2c_addr,
>>> +                     .addr = priv->i2c_addr,
>>>                        .flags = 0,
>>>                        .len = sizeof(buf),
>>>                        .buf = buf,
>>> @@ -56,12 +56,12 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
>>>        u8 buf[len];
>>>        struct i2c_msg msg[2] = {
>>>                {
>>> -                     .addr = priv->cfg->i2c_addr,
>>> +                     .addr = priv->i2c_addr,
>>>                        .flags = 0,
>>>                        .len = 1,
>>>                        .buf = &reg,
>>>                }, {
>>> -                     .addr = priv->cfg->i2c_addr,
>>> +                     .addr = priv->i2c_addr,
>>>                        .flags = I2C_M_RD,
>>>                        .len = sizeof(buf),
>>>                        .buf = buf,
>>> @@ -233,8 +233,8 @@ 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;
>>> @@ -358,17 +358,6 @@ static int e4000_get_if_frequency(struct dvb_frontend *fe, u32 *frequency)
>>>        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",
>>> @@ -376,8 +365,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,
>>> @@ -385,9 +372,12 @@ 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 i2c_adapter *i2c = client->adapter;
>>>        struct e4000_priv *priv;
>>>        int ret;
>>>        u8 chip_id;
>>> @@ -402,7 +392,9 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>>>                goto err;
>>>        }
>>>
>>> -     priv->cfg = cfg;
>>> +     priv->i2c_addr = cfg->i2c_addr;
>>> +     priv->clock = cfg->clock;
>>> +     priv->i2c_client = client;
>>>        priv->i2c = i2c;
>>>
>>>        /* check if the tuner is there */
>>> @@ -412,8 +404,10 @@ struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>>>
>>>        dev_dbg(&priv->i2c->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);
>>> @@ -431,16 +425,45 @@ 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);
>>>        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);
>>> +
>>> +     dev_dbg(&client->dev, "%s:\n", __func__);
>>> +
>>> +     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..760b206 100644
>>> --- a/drivers/media/tuners/e4000.h
>>> +++ b/drivers/media/tuners/e4000.h
>>> @@ -26,6 +26,11 @@
>>>
>>>   struct e4000_config {
>>>        /*
>>> +      * frontend
>>> +      */
>>> +     struct dvb_frontend *fe;
>>> +
>>> +     /*
>>>         * I2C address
>>>         * 0x64, 0x65, 0x66, 0x67
>>>         */
>>> @@ -39,10 +44,10 @@ struct e4000_config {
>>>
>>>   #if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
>>
>> You can get rid of #if IS_ENABLED, by replacing it by a generic attach
>> function, using the subdev model. In other words, this is how a tuner
>> subdev is defined:
>>
>>          struct v4l2_subdev_tuner_ops {
>>                  int (*s_radio)(struct v4l2_subdev *sd);
>>                  int (*s_frequency)(struct v4l2_subdev *sd, const struct v4l2_frequency *freq);
>>                  int (*g_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency *freq);
>>                  int (*g_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner *vt);
>>                  int (*s_tuner)(struct v4l2_subdev *sd, const struct v4l2_tuner *vt);
>>                  int (*g_modulator)(struct v4l2_subdev *sd, struct v4l2_modulator *vm);
>>                  int (*s_modulator)(struct v4l2_subdev *sd, const struct v4l2_modulator *vm);
>>                  int (*s_type_addr)(struct v4l2_subdev *sd, struct tuner_setup *type);
>>                  int (*s_config)(struct v4l2_subdev *sd, const struct v4l2_priv_tun_config *config);
>>          };
>>
>> As the presence of the device is known, there's no need to actually attach
>> anything, but you need to pass the configuration for the subdevice. This is
>> done by calling the subdev s_config ops to setup them:
>>
>>          v4l2_subdev_call(sd, tuner, s_config, vfh, arg);
>>
>> A new callback will be needed there, in order to put the device into DVB mode,
>> e. g. passing the DVB specific parameters.
>>
>> As the subdev callbacks are defined on a generic .h header and they always
>> exist, if the driver is not compiled, i2c_new_device() won't find the driver,
>> and the bridge driver won't call s_config() for this I2C client.
>>
>> That's IMHO, one of the biggest advantages of this conversion: as a bonus,
>> we get rid of several Kconfig issues. Also, with this change, lsmod will
>> properly show that the tuner is bound to the bridge driver.
>>
>>>   extern struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>>> -             struct i2c_adapter *i2c, const struct e4000_config *cfg);
>>> +             struct i2c_adapter *i2c, 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)
>>> +             struct i2c_adapter *i2c, struct e4000_config *cfg)
>>>   {
>>>        dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n", __func__);
>>>        return NULL;
>>> diff --git a/drivers/media/tuners/e4000_priv.h b/drivers/media/tuners/e4000_priv.h
>>> index a385505..7f47068 100644
>>> --- a/drivers/media/tuners/e4000_priv.h
>>> +++ b/drivers/media/tuners/e4000_priv.h
>>> @@ -24,8 +24,10 @@
>>>   #include "e4000.h"
>>>
>>>   struct e4000_priv {
>>> -     const struct e4000_config *cfg;
>>>        struct i2c_adapter *i2c;
>>> +     struct i2c_client *i2c_client;
>>> +     u8 i2c_addr;
>>> +     u32 clock;
>>>   };
>>>
>>>   struct e4000_pll {
>>> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>> index defc491..573805a 100644
>>> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>> @@ -843,11 +843,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,
>>> @@ -874,10 +869,14 @@ 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_client *client = 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],
>>> @@ -897,9 +896,20 @@ 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],
>>> +                             .i2c_addr = 0x64,
>>> +                             .clock = 28800000,
>>> +                     };
>>> +
>>> +                     strlcpy(info.type, "e4000", I2C_NAME_SIZE);
>>> +                     info.addr = 0x64;
>>> +                     info.platform_data = &e4000_config;
>>> +
>>> +                     request_module("e4000");
>>> +                     client = i2c_new_device(&d->i2c_adap, &info);
>>> +             }
>>>                break;
>>>        case TUNER_RTL2832_FC2580:
>>>                fe = dvb_attach(fc2580_attach, adap->fe[0], &d->i2c_adap,
>>> @@ -927,12 +937,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 && (client == NULL || client->driver == NULL)) {
>>>                ret = -ENODEV;
>>>                goto err;
>>>        }
>>
>>
>> --
>>
>> Cheers,
>> Mauro
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
http://palosaari.fi/

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Michael Krufky]

On Wed, Oct 16, 2013 at 12:43 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 16.10.2013 18:54, Michael Krufky wrote:
>>
>> This kinda makes me a bit nervous.  The patch itself looks OK but the
>> cascading effects that it will have across the DVB subsystem need to
>> be discussed.
>
>
> Basically, the only effect is that you must use i2c_new_device() or
> i2c_new_probed_device() instead of old proprietary dvb_attach(). Convert old
> proprietary method with Kernel standard one.
>
> But thats not all. It was only first step. My final goal is to study, test
> and finally make radio tuners as a own "standalone" I2C drivers. This means
> that I will remove DVB frontend dependency next from the given tuner driver.
> And before you ask why: I need some more sophisticated access to tuner in
> order to meet SDR needs. Also I don't see any reason why those tuners should
> be a part of DVB API or part of V4L2 API (or even both in case of "hybrid"
> model). Actual reasons are likely payload from the history, but as of today
> view it is stupid design :]
>
>
>> Is there a discussion about this kind of conversion on the mailing
>> list somewhere that I've missed?
>
>
> There has been many times around the years. i2c_new_probed_device() function
> [1] seems to be added for somehow for out needs too...
>
> [1] http://lists.lm-sensors.org/pipermail/i2c/2007-March/000990.html
>
>
> regards
> Antti
>
>
>>
>> -Mike
>>
>> On Tue, Oct 15, 2013 at 7:33 PM, Mauro Carvalho Chehab
>> <m.chehab@samsung.com> wrote:
>>>
>>> Em Wed, 16 Oct 2013 01:31:04 +0300
>>> Antti Palosaari <crope@iki.fi> escreveu:
>>>
>>>> Initial driver conversion from proprietary DVB tuner model to more
>>>> general I2C driver model.
>>>>
>>>> That commit has just basic binding stuff and driver itself still
>>>> needs to be converted more complete later.
>>>>
>>>> Cc: Jean Delvare <khali@linux-fr.org>
>>>> Signed-off-by: Antti Palosaari <crope@iki.fi>
>>>> ---
>>>>   drivers/media/tuners/e4000.c            | 73
>>>> ++++++++++++++++++++++-----------
>>>>   drivers/media/tuners/e4000.h            |  9 +++-
>>>>   drivers/media/tuners/e4000_priv.h       |  4 +-
>>>>   drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 31 +++++++++-----
>>>>   4 files changed, 78 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
>>>> index 54e2d8a..f6a5dbd 100644
>>>> --- a/drivers/media/tuners/e4000.c
>>>> +++ b/drivers/media/tuners/e4000.c
>>>> @@ -27,7 +27,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8
>>>> reg, u8 *val, int len)
>>>>        u8 buf[1 + len];
>>>>        struct i2c_msg msg[1] = {
>>>>                {
>>>> -                     .addr = priv->cfg->i2c_addr,
>>>> +                     .addr = priv->i2c_addr,
>>>>                        .flags = 0,
>>>>                        .len = sizeof(buf),
>>>>                        .buf = buf,
>>>> @@ -56,12 +56,12 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8
>>>> reg, u8 *val, int len)
>>>>        u8 buf[len];
>>>>        struct i2c_msg msg[2] = {
>>>>                {
>>>> -                     .addr = priv->cfg->i2c_addr,
>>>> +                     .addr = priv->i2c_addr,
>>>>                        .flags = 0,
>>>>                        .len = 1,
>>>>                        .buf = &reg,
>>>>                }, {
>>>> -                     .addr = priv->cfg->i2c_addr,
>>>> +                     .addr = priv->i2c_addr,
>>>>                        .flags = I2C_M_RD,
>>>>                        .len = sizeof(buf),
>>>>                        .buf = buf,
>>>> @@ -233,8 +233,8 @@ 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;
>>>> @@ -358,17 +358,6 @@ static int e4000_get_if_frequency(struct
>>>> dvb_frontend *fe, u32 *frequency)
>>>>        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",
>>>> @@ -376,8 +365,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,
>>>> @@ -385,9 +372,12 @@ 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 i2c_adapter *i2c = client->adapter;
>>>>        struct e4000_priv *priv;
>>>>        int ret;
>>>>        u8 chip_id;
>>>> @@ -402,7 +392,9 @@ struct dvb_frontend *e4000_attach(struct
>>>> dvb_frontend *fe,
>>>>                goto err;
>>>>        }
>>>>
>>>> -     priv->cfg = cfg;
>>>> +     priv->i2c_addr = cfg->i2c_addr;
>>>> +     priv->clock = cfg->clock;
>>>> +     priv->i2c_client = client;
>>>>        priv->i2c = i2c;
>>>>
>>>>        /* check if the tuner is there */
>>>> @@ -412,8 +404,10 @@ struct dvb_frontend *e4000_attach(struct
>>>> dvb_frontend *fe,
>>>>
>>>>        dev_dbg(&priv->i2c->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);
>>>> @@ -431,16 +425,45 @@ 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);
>>>>        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);
>>>> +
>>>> +     dev_dbg(&client->dev, "%s:\n", __func__);
>>>> +
>>>> +     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..760b206 100644
>>>> --- a/drivers/media/tuners/e4000.h
>>>> +++ b/drivers/media/tuners/e4000.h
>>>> @@ -26,6 +26,11 @@
>>>>
>>>>   struct e4000_config {
>>>>        /*
>>>> +      * frontend
>>>> +      */
>>>> +     struct dvb_frontend *fe;
>>>> +
>>>> +     /*
>>>>         * I2C address
>>>>         * 0x64, 0x65, 0x66, 0x67
>>>>         */
>>>> @@ -39,10 +44,10 @@ struct e4000_config {
>>>>
>>>>   #if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
>>>
>>>
>>> You can get rid of #if IS_ENABLED, by replacing it by a generic attach
>>> function, using the subdev model. In other words, this is how a tuner
>>> subdev is defined:
>>>
>>>          struct v4l2_subdev_tuner_ops {
>>>                  int (*s_radio)(struct v4l2_subdev *sd);
>>>                  int (*s_frequency)(struct v4l2_subdev *sd, const struct
>>> v4l2_frequency *freq);
>>>                  int (*g_frequency)(struct v4l2_subdev *sd, struct
>>> v4l2_frequency *freq);
>>>                  int (*g_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner
>>> *vt);
>>>                  int (*s_tuner)(struct v4l2_subdev *sd, const struct
>>> v4l2_tuner *vt);
>>>                  int (*g_modulator)(struct v4l2_subdev *sd, struct
>>> v4l2_modulator *vm);
>>>                  int (*s_modulator)(struct v4l2_subdev *sd, const struct
>>> v4l2_modulator *vm);
>>>                  int (*s_type_addr)(struct v4l2_subdev *sd, struct
>>> tuner_setup *type);
>>>                  int (*s_config)(struct v4l2_subdev *sd, const struct
>>> v4l2_priv_tun_config *config);
>>>          };
>>>
>>> As the presence of the device is known, there's no need to actually
>>> attach
>>> anything, but you need to pass the configuration for the subdevice. This
>>> is
>>> done by calling the subdev s_config ops to setup them:
>>>
>>>          v4l2_subdev_call(sd, tuner, s_config, vfh, arg);
>>>
>>> A new callback will be needed there, in order to put the device into DVB
>>> mode,
>>> e. g. passing the DVB specific parameters.
>>>
>>> As the subdev callbacks are defined on a generic .h header and they
>>> always
>>> exist, if the driver is not compiled, i2c_new_device() won't find the
>>> driver,
>>> and the bridge driver won't call s_config() for this I2C client.
>>>
>>> That's IMHO, one of the biggest advantages of this conversion: as a
>>> bonus,
>>> we get rid of several Kconfig issues. Also, with this change, lsmod will
>>> properly show that the tuner is bound to the bridge driver.
>>>
>>>>   extern struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>>>> -             struct i2c_adapter *i2c, const struct e4000_config *cfg);
>>>> +             struct i2c_adapter *i2c, 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)
>>>> +             struct i2c_adapter *i2c, struct e4000_config *cfg)
>>>>   {
>>>>        dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n",
>>>> __func__);
>>>>        return NULL;
>>>> diff --git a/drivers/media/tuners/e4000_priv.h
>>>> b/drivers/media/tuners/e4000_priv.h
>>>> index a385505..7f47068 100644
>>>> --- a/drivers/media/tuners/e4000_priv.h
>>>> +++ b/drivers/media/tuners/e4000_priv.h
>>>> @@ -24,8 +24,10 @@
>>>>   #include "e4000.h"
>>>>
>>>>   struct e4000_priv {
>>>> -     const struct e4000_config *cfg;
>>>>        struct i2c_adapter *i2c;
>>>> +     struct i2c_client *i2c_client;
>>>> +     u8 i2c_addr;
>>>> +     u32 clock;
>>>>   };
>>>>
>>>>   struct e4000_pll {
>>>> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>>> b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>>> index defc491..573805a 100644
>>>> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>>> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>>> @@ -843,11 +843,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,
>>>> @@ -874,10 +869,14 @@ 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_client *client = 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],
>>>> @@ -897,9 +896,20 @@ 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],
>>>> +                             .i2c_addr = 0x64,
>>>> +                             .clock = 28800000,
>>>> +                     };
>>>> +
>>>> +                     strlcpy(info.type, "e4000", I2C_NAME_SIZE);
>>>> +                     info.addr = 0x64;
>>>> +                     info.platform_data = &e4000_config;
>>>> +
>>>> +                     request_module("e4000");
>>>> +                     client = i2c_new_device(&d->i2c_adap, &info);
>>>> +             }
>>>>                break;
>>>>        case TUNER_RTL2832_FC2580:
>>>>                fe = dvb_attach(fc2580_attach, adap->fe[0], &d->i2c_adap,
>>>> @@ -927,12 +937,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 && (client == NULL || client->driver == NULL)) {
>>>>                ret = -ENODEV;
>>>>                goto err;
>>>>        }
>>>
>>>
>>>
>>> --
>>>
>>> Cheers,
>>> Mauro
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> http://palosaari.fi/



Ugh, apologies for my previous toppost(again) ... I wish gmail had an
option for replies below quoted text, instead of the default.  (If
someone knows a way - please let me know ;-) )

I only ask because I'm used to seeing an RFC proposal on the mailing
list for this type of change - I can't spend all day on irc like I had
in the past.

Anyway, cool... I have been wanting to do something like this for
years, too... but not exactly this way.  Meanwhile, it does indeed
seem like i2c_new_probed_device() will achieve what we're looking for,
so perhaps this will indeed be a good solution.

Are we sure that this leaves total control of the driver attachment to
the driver itself?  I mean, no worries of the wrong i2c tuner driver
trying to attach?  No worries about automatic probing that could
damage the hardware?  It seems OK to me but somebody needs to ask
those questions ;-)

-Mike

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Michael Krufky]

On Wed, Oct 16, 2013 at 1:01 PM, Michael Krufky <mkrufky@linuxtv.org> wrote:
> On Wed, Oct 16, 2013 at 12:43 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 16.10.2013 18:54, Michael Krufky wrote:
>>>
>>> This kinda makes me a bit nervous.  The patch itself looks OK but the
>>> cascading effects that it will have across the DVB subsystem need to
>>> be discussed.
>>
>>
>> Basically, the only effect is that you must use i2c_new_device() or
>> i2c_new_probed_device() instead of old proprietary dvb_attach(). Convert old
>> proprietary method with Kernel standard one.
>>
>> But thats not all. It was only first step. My final goal is to study, test
>> and finally make radio tuners as a own "standalone" I2C drivers. This means
>> that I will remove DVB frontend dependency next from the given tuner driver.
>> And before you ask why: I need some more sophisticated access to tuner in
>> order to meet SDR needs. Also I don't see any reason why those tuners should
>> be a part of DVB API or part of V4L2 API (or even both in case of "hybrid"
>> model). Actual reasons are likely payload from the history, but as of today
>> view it is stupid design :]
>>
>>
>>> Is there a discussion about this kind of conversion on the mailing
>>> list somewhere that I've missed?
>>
>>
>> There has been many times around the years. i2c_new_probed_device() function
>> [1] seems to be added for somehow for out needs too...
>>
>> [1] http://lists.lm-sensors.org/pipermail/i2c/2007-March/000990.html
>>
>>
>> regards
>> Antti
>>
>>
>>>
>>> -Mike
>>>
>>> On Tue, Oct 15, 2013 at 7:33 PM, Mauro Carvalho Chehab
>>> <m.chehab@samsung.com> wrote:
>>>>
>>>> Em Wed, 16 Oct 2013 01:31:04 +0300
>>>> Antti Palosaari <crope@iki.fi> escreveu:
>>>>
>>>>> Initial driver conversion from proprietary DVB tuner model to more
>>>>> general I2C driver model.
>>>>>
>>>>> That commit has just basic binding stuff and driver itself still
>>>>> needs to be converted more complete later.
>>>>>
>>>>> Cc: Jean Delvare <khali@linux-fr.org>
>>>>> Signed-off-by: Antti Palosaari <crope@iki.fi>
>>>>> ---
>>>>>   drivers/media/tuners/e4000.c            | 73
>>>>> ++++++++++++++++++++++-----------
>>>>>   drivers/media/tuners/e4000.h            |  9 +++-
>>>>>   drivers/media/tuners/e4000_priv.h       |  4 +-
>>>>>   drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 31 +++++++++-----
>>>>>   4 files changed, 78 insertions(+), 39 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
>>>>> index 54e2d8a..f6a5dbd 100644
>>>>> --- a/drivers/media/tuners/e4000.c
>>>>> +++ b/drivers/media/tuners/e4000.c
>>>>> @@ -27,7 +27,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8
>>>>> reg, u8 *val, int len)
>>>>>        u8 buf[1 + len];
>>>>>        struct i2c_msg msg[1] = {
>>>>>                {
>>>>> -                     .addr = priv->cfg->i2c_addr,
>>>>> +                     .addr = priv->i2c_addr,
>>>>>                        .flags = 0,
>>>>>                        .len = sizeof(buf),
>>>>>                        .buf = buf,
>>>>> @@ -56,12 +56,12 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8
>>>>> reg, u8 *val, int len)
>>>>>        u8 buf[len];
>>>>>        struct i2c_msg msg[2] = {
>>>>>                {
>>>>> -                     .addr = priv->cfg->i2c_addr,
>>>>> +                     .addr = priv->i2c_addr,
>>>>>                        .flags = 0,
>>>>>                        .len = 1,
>>>>>                        .buf = &reg,
>>>>>                }, {
>>>>> -                     .addr = priv->cfg->i2c_addr,
>>>>> +                     .addr = priv->i2c_addr,
>>>>>                        .flags = I2C_M_RD,
>>>>>                        .len = sizeof(buf),
>>>>>                        .buf = buf,
>>>>> @@ -233,8 +233,8 @@ 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;
>>>>> @@ -358,17 +358,6 @@ static int e4000_get_if_frequency(struct
>>>>> dvb_frontend *fe, u32 *frequency)
>>>>>        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",
>>>>> @@ -376,8 +365,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,
>>>>> @@ -385,9 +372,12 @@ 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 i2c_adapter *i2c = client->adapter;
>>>>>        struct e4000_priv *priv;
>>>>>        int ret;
>>>>>        u8 chip_id;
>>>>> @@ -402,7 +392,9 @@ struct dvb_frontend *e4000_attach(struct
>>>>> dvb_frontend *fe,
>>>>>                goto err;
>>>>>        }
>>>>>
>>>>> -     priv->cfg = cfg;
>>>>> +     priv->i2c_addr = cfg->i2c_addr;
>>>>> +     priv->clock = cfg->clock;
>>>>> +     priv->i2c_client = client;
>>>>>        priv->i2c = i2c;
>>>>>
>>>>>        /* check if the tuner is there */
>>>>> @@ -412,8 +404,10 @@ struct dvb_frontend *e4000_attach(struct
>>>>> dvb_frontend *fe,
>>>>>
>>>>>        dev_dbg(&priv->i2c->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);
>>>>> @@ -431,16 +425,45 @@ 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);
>>>>>        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);
>>>>> +
>>>>> +     dev_dbg(&client->dev, "%s:\n", __func__);
>>>>> +
>>>>> +     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..760b206 100644
>>>>> --- a/drivers/media/tuners/e4000.h
>>>>> +++ b/drivers/media/tuners/e4000.h
>>>>> @@ -26,6 +26,11 @@
>>>>>
>>>>>   struct e4000_config {
>>>>>        /*
>>>>> +      * frontend
>>>>> +      */
>>>>> +     struct dvb_frontend *fe;
>>>>> +
>>>>> +     /*
>>>>>         * I2C address
>>>>>         * 0x64, 0x65, 0x66, 0x67
>>>>>         */
>>>>> @@ -39,10 +44,10 @@ struct e4000_config {
>>>>>
>>>>>   #if IS_ENABLED(CONFIG_MEDIA_TUNER_E4000)
>>>>
>>>>
>>>> You can get rid of #if IS_ENABLED, by replacing it by a generic attach
>>>> function, using the subdev model. In other words, this is how a tuner
>>>> subdev is defined:
>>>>
>>>>          struct v4l2_subdev_tuner_ops {
>>>>                  int (*s_radio)(struct v4l2_subdev *sd);
>>>>                  int (*s_frequency)(struct v4l2_subdev *sd, const struct
>>>> v4l2_frequency *freq);
>>>>                  int (*g_frequency)(struct v4l2_subdev *sd, struct
>>>> v4l2_frequency *freq);
>>>>                  int (*g_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner
>>>> *vt);
>>>>                  int (*s_tuner)(struct v4l2_subdev *sd, const struct
>>>> v4l2_tuner *vt);
>>>>                  int (*g_modulator)(struct v4l2_subdev *sd, struct
>>>> v4l2_modulator *vm);
>>>>                  int (*s_modulator)(struct v4l2_subdev *sd, const struct
>>>> v4l2_modulator *vm);
>>>>                  int (*s_type_addr)(struct v4l2_subdev *sd, struct
>>>> tuner_setup *type);
>>>>                  int (*s_config)(struct v4l2_subdev *sd, const struct
>>>> v4l2_priv_tun_config *config);
>>>>          };
>>>>
>>>> As the presence of the device is known, there's no need to actually
>>>> attach
>>>> anything, but you need to pass the configuration for the subdevice. This
>>>> is
>>>> done by calling the subdev s_config ops to setup them:
>>>>
>>>>          v4l2_subdev_call(sd, tuner, s_config, vfh, arg);
>>>>
>>>> A new callback will be needed there, in order to put the device into DVB
>>>> mode,
>>>> e. g. passing the DVB specific parameters.
>>>>
>>>> As the subdev callbacks are defined on a generic .h header and they
>>>> always
>>>> exist, if the driver is not compiled, i2c_new_device() won't find the
>>>> driver,
>>>> and the bridge driver won't call s_config() for this I2C client.
>>>>
>>>> That's IMHO, one of the biggest advantages of this conversion: as a
>>>> bonus,
>>>> we get rid of several Kconfig issues. Also, with this change, lsmod will
>>>> properly show that the tuner is bound to the bridge driver.
>>>>
>>>>>   extern struct dvb_frontend *e4000_attach(struct dvb_frontend *fe,
>>>>> -             struct i2c_adapter *i2c, const struct e4000_config *cfg);
>>>>> +             struct i2c_adapter *i2c, 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)
>>>>> +             struct i2c_adapter *i2c, struct e4000_config *cfg)
>>>>>   {
>>>>>        dev_warn(&i2c->dev, "%s: driver disabled by Kconfig\n",
>>>>> __func__);
>>>>>        return NULL;
>>>>> diff --git a/drivers/media/tuners/e4000_priv.h
>>>>> b/drivers/media/tuners/e4000_priv.h
>>>>> index a385505..7f47068 100644
>>>>> --- a/drivers/media/tuners/e4000_priv.h
>>>>> +++ b/drivers/media/tuners/e4000_priv.h
>>>>> @@ -24,8 +24,10 @@
>>>>>   #include "e4000.h"
>>>>>
>>>>>   struct e4000_priv {
>>>>> -     const struct e4000_config *cfg;
>>>>>        struct i2c_adapter *i2c;
>>>>> +     struct i2c_client *i2c_client;
>>>>> +     u8 i2c_addr;
>>>>> +     u32 clock;
>>>>>   };
>>>>>
>>>>>   struct e4000_pll {
>>>>> diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>>>> b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>>>> index defc491..573805a 100644
>>>>> --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>>>> +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
>>>>> @@ -843,11 +843,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,
>>>>> @@ -874,10 +869,14 @@ 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_client *client = 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],
>>>>> @@ -897,9 +896,20 @@ 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],
>>>>> +                             .i2c_addr = 0x64,
>>>>> +                             .clock = 28800000,
>>>>> +                     };
>>>>> +
>>>>> +                     strlcpy(info.type, "e4000", I2C_NAME_SIZE);
>>>>> +                     info.addr = 0x64;
>>>>> +                     info.platform_data = &e4000_config;
>>>>> +
>>>>> +                     request_module("e4000");
>>>>> +                     client = i2c_new_device(&d->i2c_adap, &info);
>>>>> +             }
>>>>>                break;
>>>>>        case TUNER_RTL2832_FC2580:
>>>>>                fe = dvb_attach(fc2580_attach, adap->fe[0], &d->i2c_adap,
>>>>> @@ -927,12 +937,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 && (client == NULL || client->driver == NULL)) {
>>>>>                ret = -ENODEV;
>>>>>                goto err;
>>>>>        }
>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Cheers,
>>>> Mauro
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> http://palosaari.fi/
>
>
>
> Ugh, apologies for my previous toppost(again) ... I wish gmail had an
> option for replies below quoted text, instead of the default.  (If
> someone knows a way - please let me know ;-) )
>
> I only ask because I'm used to seeing an RFC proposal on the mailing
> list for this type of change - I can't spend all day on irc like I had
> in the past.
>
> Anyway, cool... I have been wanting to do something like this for
> years, too... but not exactly this way.  Meanwhile, it does indeed
> seem like i2c_new_probed_device() will achieve what we're looking for,
> so perhaps this will indeed be a good solution.
>
> Are we sure that this leaves total control of the driver attachment to
> the driver itself?  I mean, no worries of the wrong i2c tuner driver
> trying to attach?  No worries about automatic probing that could
> damage the hardware?  It seems OK to me but somebody needs to ask
> those questions ;-)
>
> -Mike


YIKES!!  i2c_new_probed_device() does indeed probe the hardware --
this is unacceptable, as such an action can damage the ic.

Is there some additional information that I'm missing that lets this
perform an attach without probe?

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Jean Delvare]

Hi Michael,

On Wed, 16 Oct 2013 13:04:42 -0400, Michael Krufky wrote:
> YIKES!!  i2c_new_probed_device() does indeed probe the hardware --
> this is unacceptable, as such an action can damage the ic.
> 
> Is there some additional information that I'm missing that lets this
> perform an attach without probe?

Oh, i2c_new_probed_device() probes the device, what a surprise! :D

Try, I don't know, i2c_new_device() maybe if you don't want the
probe? ;)

-- 
Jean Delvare

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Michael Krufky]

On Wed, Oct 16, 2013 at 1:09 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Michael,
>
> On Wed, 16 Oct 2013 13:04:42 -0400, Michael Krufky wrote:
>> YIKES!!  i2c_new_probed_device() does indeed probe the hardware --
>> this is unacceptable, as such an action can damage the ic.
>>
>> Is there some additional information that I'm missing that lets this
>> perform an attach without probe?
>
> Oh, i2c_new_probed_device() probes the device, what a surprise! :D
>
> Try, I don't know, i2c_new_device() maybe if you don't want the
> probe? ;)
>
> --
> Jean Delvare

OK, so to confirm that I follow correctly, one can use
i2c_new_device() to attach the sub-driver without probing, and the
line that ensures that the correct sub-driver gets attached is
"strlcpy(info.type, "e4000", I2C_NAME_SIZE);"  ??

We're matching based on a string?  I think that's kinda yucky, but if
that's what we're doing in i2c nowadays then I'm OK with it.

If not, what prevents the wrong sub-driver from attaching to a device?
 ...or conversely, how does the right sub-driver know which device to
attach to?

Again, if I'm asking "stupid questions" just point me to the documentation.

-Mike

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Antti Palosaari]

On 16.10.2013 20:19, Michael Krufky wrote:
> On Wed, Oct 16, 2013 at 1:09 PM, Jean Delvare <khali@linux-fr.org> wrote:
>> Hi Michael,
>>
>> On Wed, 16 Oct 2013 13:04:42 -0400, Michael Krufky wrote:
>>> YIKES!!  i2c_new_probed_device() does indeed probe the hardware --
>>> this is unacceptable, as such an action can damage the ic.
>>>
>>> Is there some additional information that I'm missing that lets this
>>> perform an attach without probe?
>>
>> Oh, i2c_new_probed_device() probes the device, what a surprise! :D
>>
>> Try, I don't know, i2c_new_device() maybe if you don't want the
>> probe? ;)
>>
>> --
>> Jean Delvare
>
> OK, so to confirm that I follow correctly, one can use
> i2c_new_device() to attach the sub-driver without probing, and the
> line that ensures that the correct sub-driver gets attached is
> "strlcpy(info.type, "e4000", I2C_NAME_SIZE);"  ??
>
> We're matching based on a string?  I think that's kinda yucky, but if
> that's what we're doing in i2c nowadays then I'm OK with it.
>
> If not, what prevents the wrong sub-driver from attaching to a device?
>   ...or conversely, how does the right sub-driver know which device to
> attach to?

Yes, it is that string. Driver has that string as a ID table entry. Then 
you issue i2c_new_device() call with string and it attachs driver when 
strings match.

> Again, if I'm asking "stupid questions" just point me to the documentation.
>
> -Mike
>

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Michael Krufky]

On Wed, Oct 16, 2013 at 1:22 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 16.10.2013 20:19, Michael Krufky wrote:
>>
>> On Wed, Oct 16, 2013 at 1:09 PM, Jean Delvare <khali@linux-fr.org> wrote:
>>>
>>> Hi Michael,
>>>
>>> On Wed, 16 Oct 2013 13:04:42 -0400, Michael Krufky wrote:
>>>>
>>>> YIKES!!  i2c_new_probed_device() does indeed probe the hardware --
>>>> this is unacceptable, as such an action can damage the ic.
>>>>
>>>> Is there some additional information that I'm missing that lets this
>>>> perform an attach without probe?
>>>
>>>
>>> Oh, i2c_new_probed_device() probes the device, what a surprise! :D
>>>
>>> Try, I don't know, i2c_new_device() maybe if you don't want the
>>> probe? ;)
>>>
>>> --
>>> Jean Delvare
>>
>>
>> OK, so to confirm that I follow correctly, one can use
>> i2c_new_device() to attach the sub-driver without probing, and the
>> line that ensures that the correct sub-driver gets attached is
>> "strlcpy(info.type, "e4000", I2C_NAME_SIZE);"  ??
>>
>> We're matching based on a string?  I think that's kinda yucky, but if
>> that's what we're doing in i2c nowadays then I'm OK with it.
>>
>> If not, what prevents the wrong sub-driver from attaching to a device?
>>   ...or conversely, how does the right sub-driver know which device to
>> attach to?
>
>
> Yes, it is that string. Driver has that string as a ID table entry. Then you
> issue i2c_new_device() call with string and it attachs driver when strings
> match.
>
>
>> Again, if I'm asking "stupid questions" just point me to the
>> documentation.
>>
>> -Mike
>>
>
> regards
> Antti


OK, I get it and it does seem OK.  I'm just curious what kind of
impact this refactoring would have over something like the
b2c2-flexcop-fe driver, who does not know which ic's to attach based
on device ids, but it does probe a few frontend combinations one after
another, in an order that the driver authors knew was safe.  I'd
imaging that we'd write some helper abstraction function to switch out
the info.type string as each driver gets probed?  I think that can get
quite ugly, but I know that the general population thinks dvb_attach()
is even uglier, so maybe this could be the right path...

Wanna take a crack at b2c2-flexcop-fe?

-Mike

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Antti Palosaari]

On 16.10.2013 20:33, Michael Krufky wrote:
> On Wed, Oct 16, 2013 at 1:22 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 16.10.2013 20:19, Michael Krufky wrote:
>>>
>>> On Wed, Oct 16, 2013 at 1:09 PM, Jean Delvare <khali@linux-fr.org> wrote:
>>>>
>>>> Hi Michael,
>>>>
>>>> On Wed, 16 Oct 2013 13:04:42 -0400, Michael Krufky wrote:
>>>>>
>>>>> YIKES!!  i2c_new_probed_device() does indeed probe the hardware --
>>>>> this is unacceptable, as such an action can damage the ic.
>>>>>
>>>>> Is there some additional information that I'm missing that lets this
>>>>> perform an attach without probe?
>>>>
>>>>
>>>> Oh, i2c_new_probed_device() probes the device, what a surprise! :D
>>>>
>>>> Try, I don't know, i2c_new_device() maybe if you don't want the
>>>> probe? ;)
>>>>
>>>> --
>>>> Jean Delvare
>>>
>>>
>>> OK, so to confirm that I follow correctly, one can use
>>> i2c_new_device() to attach the sub-driver without probing, and the
>>> line that ensures that the correct sub-driver gets attached is
>>> "strlcpy(info.type, "e4000", I2C_NAME_SIZE);"  ??
>>>
>>> We're matching based on a string?  I think that's kinda yucky, but if
>>> that's what we're doing in i2c nowadays then I'm OK with it.
>>>
>>> If not, what prevents the wrong sub-driver from attaching to a device?
>>>    ...or conversely, how does the right sub-driver know which device to
>>> attach to?
>>
>>
>> Yes, it is that string. Driver has that string as a ID table entry. Then you
>> issue i2c_new_device() call with string and it attachs driver when strings
>> match.
>>
>>
>>> Again, if I'm asking "stupid questions" just point me to the
>>> documentation.
>>>
>>> -Mike
>>>
>>
>> regards
>> Antti
>
>
> OK, I get it and it does seem OK.  I'm just curious what kind of
> impact this refactoring would have over something like the
> b2c2-flexcop-fe driver, who does not know which ic's to attach based
> on device ids, but it does probe a few frontend combinations one after
> another, in an order that the driver authors knew was safe.  I'd
> imaging that we'd write some helper abstraction function to switch out
> the info.type string as each driver gets probed?  I think that can get
> quite ugly, but I know that the general population thinks dvb_attach()
> is even uglier, so maybe this could be the right path...
>
> Wanna take a crack at b2c2-flexcop-fe?
>
> -Mike
>

heh, look the rtl28xxu driver in question, it does same. It probes maybe 
total 10 tuners - checking their device ID too. Probing plain I2C device 
address and making devision from that is simply dead idea. "Standard" 
I2C address range for these silicon tuners are 0x60-0x64 => need to read 
chip ID in order to detect => i2c_new_probed_device() is quite useless.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH REVIEW] e4000: convert DVB tuner to I2C driver model

[Jean Delvare]

On Wed, 16 Oct 2013 20:45:39 +0300, Antti Palosaari wrote:
> On 16.10.2013 20:33, Michael Krufky wrote:
> > OK, I get it and it does seem OK.  I'm just curious what kind of
> > impact this refactoring would have over something like the
> > b2c2-flexcop-fe driver, who does not know which ic's to attach based
> > on device ids, but it does probe a few frontend combinations one after
> > another, in an order that the driver authors knew was safe.  I'd
> > imaging that we'd write some helper abstraction function to switch out
> > the info.type string as each driver gets probed?  I think that can get
> > quite ugly, but I know that the general population thinks dvb_attach()
> > is even uglier, so maybe this could be the right path...
> >
> > Wanna take a crack at b2c2-flexcop-fe?
> 
> heh, look the rtl28xxu driver in question, it does same. It probes maybe 
> total 10 tuners - checking their device ID too. Probing plain I2C device 
> address and making devision from that is simply dead idea. "Standard" 
> I2C address range for these silicon tuners are 0x60-0x64 => need to read 
> chip ID in order to detect => i2c_new_probed_device() is quite useless.

Please remember you can pass a custom probe function to
i2c_new_probed_device(). That probe function can read whatever chip ID
register exists to decide if the probe should be successful or not.

-- 
Jean Delvare