[RFC/PATCH] dvb-core: add template code for i2c binding model

Commit Message

Akihiro TSUKADA Dec. 5, 2014, 10:49 a.m. UTC

From: Akihiro Tsukada <[e-mail]>

Define a standard interface for demod/tuner i2c driver modules.
A module client calls dvb_i2c_attach_{fe,tuner}(),
and a module driver defines struct dvb_i2c_module_param and
calls DEFINE_DVB_I2C_MODULE() macro.

This template provides implicit module requests and ref-counting,
alloc/free's private data structures,
fixes the usage of clientdata of i2c devices,
and defines the platformdata structures for passing around
device specific config/output parameters between drivers and clients.
These kinds of code are common to (almost) all dvb i2c drivers/client,
but they were scattered over adapter modules and demod/tuner modules.

Signed-off-by: Akihiro Tsukada <[e-mail]>
---
 drivers/media/dvb-core/Makefile       |   4 +
 drivers/media/dvb-core/dvb_frontend.h |   1 +
 drivers/media/dvb-core/dvb_i2c.c      | 219 ++++++++++++++++++++++++++++++++++
 drivers/media/dvb-core/dvb_i2c.h      | 110 +++++++++++++++++
 4 files changed, 334 insertions(+)
 create mode 100644 drivers/media/dvb-core/dvb_i2c.c
 create mode 100644 drivers/media/dvb-core/dvb_i2c.h

Comments

Akihiro TSUKADA Dec. 5, 2014, 10:55 a.m. UTC | #1

From: Akihiro Tsukada <[e-mail]>

These patches are example ports of earth-pt3 (adapter),
tc90522 (demod), qm1d1c0042,mxl301rf (tuners) drivers
using the i2c template code in dvb-core.

Akihiro Tsukada (5):
  dvb: qm1d1c0042: use dvb-core i2c binding model template
  dvb: mxl301rf: use dvb-core i2c binding model template
  dvb: tc90522: use dvb-core i2c binding model template
  dvb: tc90522: remove a redundant state variable
  dvb: pci/pt3: use dvb-core i2c binding model template

 drivers/media/dvb-frontends/tc90522.c | 145 ++++++++++++++--------------------
 drivers/media/dvb-frontends/tc90522.h |   8 +-
 drivers/media/pci/pt3/pt3.c           |  89 +++++++--------------
 drivers/media/pci/pt3/pt3.h           |  12 +--
 drivers/media/tuners/mxl301rf.c       |  50 +++---------
 drivers/media/tuners/mxl301rf.h       |   2 +-
 drivers/media/tuners/qm1d1c0042.c     |  61 +++++---------
 drivers/media/tuners/qm1d1c0042.h     |   2 -
 8 files changed, 131 insertions(+), 238 deletions(-)

Akihiro TSUKADA Dec. 26, 2014, 11 a.m. UTC | #2

ping.
we don't need this kind of feature in the dvb-core?
--
akihiro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab Dec. 30, 2014, 1:10 p.m. UTC | #3

Em Fri, 05 Dec 2014 19:49:33 +0900
[e-mail] escreveu:

> From: Akihiro Tsukada <[e-mail]>
> 
> Define a standard interface for demod/tuner i2c driver modules.
> A module client calls dvb_i2c_attach_{fe,tuner}(),
> and a module driver defines struct dvb_i2c_module_param and
> calls DEFINE_DVB_I2C_MODULE() macro.
> 
> This template provides implicit module requests and ref-counting,
> alloc/free's private data structures,
> fixes the usage of clientdata of i2c devices,
> and defines the platformdata structures for passing around
> device specific config/output parameters between drivers and clients.
> These kinds of code are common to (almost) all dvb i2c drivers/client,
> but they were scattered over adapter modules and demod/tuner modules.

The idea behind this patch is good. I didn't have time yet to test it on a
device that I have currently access.

I have a few comments below, mostly regards with naming convention.

By seeing the patches you converted a few drivers to use this, I'm a little
bit concern about the conversion. We need something that won't be hard to
convert to the new model without requiring to re-test everything.

Anyway, my plan is to take a deeper look on it next week and convert one
or two drivers to use the new I2C binding approach you're proposing.
> 
> Signed-off-by: Akihiro Tsukada <[e-mail]>
> ---
>  drivers/media/dvb-core/Makefile       |   4 +
>  drivers/media/dvb-core/dvb_frontend.h |   1 +
>  drivers/media/dvb-core/dvb_i2c.c      | 219 ++++++++++++++++++++++++++++++++++
>  drivers/media/dvb-core/dvb_i2c.h      | 110 +++++++++++++++++
>  4 files changed, 334 insertions(+)
>  create mode 100644 drivers/media/dvb-core/dvb_i2c.c
>  create mode 100644 drivers/media/dvb-core/dvb_i2c.h
> 
> diff --git a/drivers/media/dvb-core/Makefile b/drivers/media/dvb-core/Makefile
> index 8f22bcd..271648d 100644
> --- a/drivers/media/dvb-core/Makefile
> +++ b/drivers/media/dvb-core/Makefile
> @@ -8,4 +8,8 @@ dvb-core-objs := dvbdev.o dmxdev.o dvb_demux.o dvb_filter.o 	\
>  		 dvb_ca_en50221.o dvb_frontend.o 		\
>  		 $(dvb-net-y) dvb_ringbuffer.o dvb_math.o
>  
> +ifneq ($(CONFIG_I2C)$(CONFIG_I2C_MODULE),)
> +dvb-core-objs += dvb_i2c.o
> +endif
> +
>  obj-$(CONFIG_DVB_CORE) += dvb-core.o
> diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h
> index 816269e..41aae1b 100644
> --- a/drivers/media/dvb-core/dvb_frontend.h
> +++ b/drivers/media/dvb-core/dvb_frontend.h
> @@ -415,6 +415,7 @@ struct dtv_frontend_properties {
>  struct dvb_frontend {
>  	struct dvb_frontend_ops ops;
>  	struct dvb_adapter *dvb;
> +	struct i2c_client *fe_cl;

fe_cl doesn't seem to be a good name for something that should be
accessed by all drivers that use it. fe_i2c_client is likely a
better name for it.

>  	void *demodulator_priv;
>  	void *tuner_priv;
>  	void *frontend_priv;
> diff --git a/drivers/media/dvb-core/dvb_i2c.c b/drivers/media/dvb-core/dvb_i2c.c
> new file mode 100644
> index 0000000..4ea4e5e
> --- /dev/null
> +++ b/drivers/media/dvb-core/dvb_i2c.c
> @@ -0,0 +1,219 @@
> +/*
> + * dvb_i2c.c
> + *
> + * Copyright 2014 Akihiro Tsukada <tskd08 AT gmail DOT com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "dvb_i2c.h"
> +
> +static struct i2c_client *
> +dvb_i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info *info,
> +		   const unsigned short *probe_addrs)
> +{
> +	struct i2c_client *cl;
> +
> +	request_module(I2C_MODULE_PREFIX "%s", info->type);
> +	/* Create the i2c client */
> +	if (info->addr == 0 && probe_addrs)
> +		cl = i2c_new_probed_device(adap, info, probe_addrs, NULL);
> +	else
> +		cl = i2c_new_device(adap, info);
> +	if (!cl || !cl->dev.driver)
> +		return NULL;
> +	return cl;

The best would be to also register the device with the media controller,
if CONFIG_MEDIA_CONTROLLER is defined, just like v4l2_i2c_subdev_init()
does.

I would also try to use similar names for the function calls to the ones
that the v4l subsystem uses for subdevices.

> +}
> +
> +struct dvb_frontend *
> +dvb_i2c_attach_fe(struct i2c_adapter *adap, const struct i2c_board_info *info,
> +		  const void *cfg, void **out)
> +{
> +	struct i2c_client *cl;
> +	struct i2c_board_info bi;

Better to call this as "tmp_info", as this seems to be just a temp var.

> +	struct dvb_i2c_dev_config dcfg;

I would, instead, call this as "cfg", and rename the parameter as
cfg_template.

> +
> +	dcfg.priv_cfg = cfg;
> +	dcfg.out = out;
> +	bi = *info;
> +	bi.platform_data = &dcfg;
> +
> +	cl = dvb_i2c_new_device(adap, &bi, NULL);
> +	if (!cl)
> +		return NULL;
> +	return i2c_get_clientdata(cl);
> +}
> +EXPORT_SYMBOL(dvb_i2c_attach_fe);
> +
> +struct i2c_client *
> +dvb_i2c_attach_tuner(struct i2c_adapter *adap,
> +		     const struct i2c_board_info *info,
> +		     struct dvb_frontend *fe,
> +		     const void *cfg, void **out)
> +{
> +	struct i2c_board_info bi;
> +	struct dvb_i2c_tuner_config tcfg;

Same as above with regards to name: please rename:
	- bi -> tmp_info
	- tcfg -> cfg

> +
> +	tcfg.fe = fe;
> +	tcfg.devcfg.priv_cfg = cfg;
> +	tcfg.devcfg.out = out;
> +	bi = *info;
> +	bi.platform_data = &tcfg;
> +
> +	return dvb_i2c_new_device(adap, &bi, NULL);
> +}
> +EXPORT_SYMBOL(dvb_i2c_attach_tuner);
> +
> +
> +static int
> +probe_tuner(struct i2c_client *client, const struct i2c_device_id *id,
> +	    const struct dvb_i2c_module_param *param,
> +	    struct module *this_module)
> +{
> +	struct dvb_frontend *fe;
> +	struct dvb_i2c_tuner_config *tcfg;

Same here with regards to naming convention.

> +	int ret;
> +
> +	if (!try_module_get(this_module))
> +		return -ENODEV;
> +
> +	tcfg = client->dev.platform_data;
> +	fe = tcfg->fe;
> +	i2c_set_clientdata(client, fe);
> +
> +	if (param->priv_size > 0) {
> +		fe->tuner_priv = kzalloc(param->priv_size, GFP_KERNEL);
> +		if (!fe->tuner_priv) {
> +			ret = -ENOMEM;
> +			goto err_mem;
> +		}
> +	}
> +
> +	if (param->ops.tuner_ops)
> +		memcpy(&fe->ops.tuner_ops, param->ops.tuner_ops,
> +			sizeof(fe->ops.tuner_ops));
> +
> +	ret = 0;
> +	if (param->priv_probe)
> +		ret = param->priv_probe(client, id);
> +	if (ret != 0) {
> +		dev_info(&client->dev, "driver private probe failed.\n");
> +		goto err_priv;
> +	}
> +	return 0;
> +
> +err_priv:
> +	kfree(fe->tuner_priv);
> +err_mem:
> +	fe->tuner_priv = NULL;
> +	module_put(this_module);
> +	return ret;
> +}
> +
> +static int remove_tuner(struct i2c_client *client,
> +			const struct dvb_i2c_module_param *param,
> +			struct module *this_module)
> +{
> +	struct dvb_frontend *fe;
> +
> +	if (param->priv_remove)
> +		param->priv_remove(client);
> +	fe = i2c_get_clientdata(client);
> +	kfree(fe->tuner_priv);
> +	fe->tuner_priv = NULL;
> +	module_put(this_module);
> +	return 0;
> +}
> +
> +
> +static int probe_fe(struct i2c_client *client, const struct i2c_device_id *id,
> +		    const struct dvb_i2c_module_param *param,
> +		    struct module *this_module)
> +{
> +	struct dvb_frontend *fe;
> +	int ret;
> +
> +	if (!try_module_get(this_module))
> +		return -ENODEV;
> +
> +	fe = kzalloc(sizeof(*fe), GFP_KERNEL);
> +	if (!fe) {
> +		ret = -ENOMEM;
> +		goto err_fe_kfree;
> +	}
> +	i2c_set_clientdata(client, fe);
> +	fe->fe_cl = client;
> +
> +	if (param->priv_size > 0) {
> +		fe->demodulator_priv = kzalloc(param->priv_size, GFP_KERNEL);
> +		if (!fe->demodulator_priv) {
> +			ret = -ENOMEM;
> +			goto err_fe_kfree;
> +		}
> +	}
> +
> +	if (param->ops.fe_ops)
> +		memcpy(&fe->ops, param->ops.fe_ops, sizeof(fe->ops));
> +
> +	ret = 0;
> +	if (param->priv_probe)
> +		ret = param->priv_probe(client, id);
> +	if (ret != 0) {
> +		dev_info(&client->dev, "driver private probe failed.\n");
> +		goto err_priv_kfree;
> +	}
> +	return 0;
> +
> +err_priv_kfree:
> +	kfree(fe->demodulator_priv);
> +err_fe_kfree:
> +	kfree(fe);
> +	module_put(this_module);
> +	return ret;
> +}
> +
> +static int remove_fe(struct i2c_client *client,
> +		     const struct dvb_i2c_module_param *param,
> +		     struct module *this_module)
> +{
> +	struct dvb_frontend *fe;
> +
> +	if (param->priv_remove)
> +		param->priv_remove(client);
> +	fe = i2c_get_clientdata(client);
> +	kfree(fe->demodulator_priv);
> +	kfree(fe);
> +	module_put(this_module);
> +	return 0;
> +}
> +
> +
> +int dvb_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id,
> +		  const struct dvb_i2c_module_param *param,
> +		  struct module *this_module)
> +{
> +	return param->is_tuner ? probe_tuner(client, id, param, this_module) :
> +				 probe_fe(client, id, param, this_module);
> +}
> +EXPORT_SYMBOL(dvb_i2c_probe);
> +
> +int dvb_i2c_remove(struct i2c_client *client,
> +		   const struct dvb_i2c_module_param *param,
> +		   struct module *this_module)
> +{
> +	return param->is_tuner ? remove_tuner(client, param, this_module) :
> +				 remove_fe(client, param, this_module);
> +}
> +EXPORT_SYMBOL(dvb_i2c_remove);
> diff --git a/drivers/media/dvb-core/dvb_i2c.h b/drivers/media/dvb-core/dvb_i2c.h
> new file mode 100644
> index 0000000..2bf409d
> --- /dev/null
> +++ b/drivers/media/dvb-core/dvb_i2c.h
> @@ -0,0 +1,110 @@
> +/*
> + * dvb_i2c.h
> + *
> + * Copyright 2014 Akihiro Tsukada <tskd08 AT gmail DOT com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +/* template code for i2c driver modules */
> +
> +#ifndef _DVB_I2C_H_
> +#define _DVB_I2C_H_
> +
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include "dvb_frontend.h"
> +
> +/* interface for module clients */
> +
> +struct dvb_frontend *dvb_i2c_attach_fe(struct i2c_adapter *adap,
> +				       const struct i2c_board_info *info,
> +				       const void *cfg, void **out);
> +
> +struct i2c_client *dvb_i2c_attach_tuner(struct i2c_adapter *adap,
> +					const struct i2c_board_info *info,
> +					struct dvb_frontend *fe,
> +					const void *cfg, void **out);
> +
> +/* interface for module drivers */
> +
> +/* data structures that are set to i2c_client.dev.platform_data */
> +
> +struct dvb_i2c_dev_config {
> +	const void *priv_cfg;
> +	/* @out [OUT] pointer to per-device data returned from driver */
> +	void **out;
> +};
> +
> +struct dvb_i2c_tuner_config {
> +	struct dvb_frontend *fe;
> +	struct dvb_i2c_dev_config devcfg;
> +};
> +
> +typedef int (*dvb_i2c_probe_func)(struct i2c_client *client,
> +				  const struct i2c_device_id *id);
> +typedef int (*dvb_i2c_remove_func)(struct i2c_client *client);
> +
> +struct dvb_i2c_module_param {
> +	union {
> +		const struct dvb_frontend_ops *fe_ops;
> +		const struct dvb_tuner_ops *tuner_ops;
> +	} ops;
> +	/* driver private probe/remove functions */
> +	dvb_i2c_probe_func  priv_probe;
> +	dvb_i2c_remove_func priv_remove;
> +
> +	u32 priv_size; /* sizeof(device private data) */
> +	bool is_tuner;
> +};
> +
> +
> +int dvb_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id,
> +		  const struct dvb_i2c_module_param *param,
> +		  struct module *this_module);
> +
> +int dvb_i2c_remove(struct i2c_client *client,
> +		   const struct dvb_i2c_module_param *param,
> +		   struct module *this_module);
> +
> +
> +#define DEFINE_DVB_I2C_MODULE(dname, idtab, param) \
> +static int _##dname##_probe(struct i2c_client *client,\
> +				const struct i2c_device_id *id)\
> +{\
> +	return dvb_i2c_probe(client, id, &(param), THIS_MODULE);\
> +} \
> +\
> +static int _##dname##_remove(struct i2c_client *client)\
> +{\
> +	return dvb_i2c_remove(client, &(param), THIS_MODULE);\
> +} \
> +\
> +MODULE_DEVICE_TABLE(i2c, idtab);\
> +\
> +static struct i2c_driver dname##_driver = {\
> +	.driver = {\
> +		.name = #dname,\
> +	},\
> +	.probe    = _##dname##_probe,\
> +	.remove   = _##dname##_remove,\
> +	.id_table = idtab,\
> +};\
> +\
> +module_i2c_driver(dname##_driver)
> +
> +#endif /* _DVB_I2C_H */

Akihiro TSUKADA Jan. 5, 2015, 11:59 a.m. UTC | #4

Hi, thank you for the comment.
I understood the naming conventions you mentioned,
and I'll update them in the next version.

>> diff --git a/drivers/media/dvb-core/dvb_i2c.c b/drivers/media/dvb-core/dvb_i2c.c
>> new file mode 100644
>> index 0000000..4ea4e5e
>> --- /dev/null
>> +++ b/drivers/media/dvb-core/dvb_i2c.c
....
>> +static struct i2c_client *
>> +dvb_i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info *info,
>> +		   const unsigned short *probe_addrs)
>> +{
>> +	struct i2c_client *cl;
>> +
>> +	request_module(I2C_MODULE_PREFIX "%s", info->type);
>> +	/* Create the i2c client */
>> +	if (info->addr == 0 && probe_addrs)
>> +		cl = i2c_new_probed_device(adap, info, probe_addrs, NULL);
>> +	else
>> +		cl = i2c_new_device(adap, info);
>> +	if (!cl || !cl->dev.driver)
>> +		return NULL;
>> +	return cl;
> 
> The best would be to also register the device with the media controller,
> if CONFIG_MEDIA_CONTROLLER is defined, just like v4l2_i2c_subdev_init()
> does.

I'll comment to your patch on this.

> I would also try to use similar names for the function calls to the ones
> that the v4l subsystem uses for subdevices.

So the name should be dvb_i2c_new_subdev()?
I am a bit worried that it would be rather confusing because
this func is different from v4l2_i2c_new_subdev() in that
it does not return "struct dvb_frontend *", requires info.platform_data parameter,
and is a static/internal function.

One more thing that I'm wondering about is whether we should add fe->demod_i2c_bus
to support tuner devices on a (dedicated) i2c bus hosted on a demod device.
I guess that this structure is pretty common (to reduce noise),
and this removes i2c_adapter from "out" structure and
confines the usage of "out" structure to just device-specific ones.

--
akihiro 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Antti Palosaari Jan. 13, 2015, 6:54 p.m. UTC | #5

On 12/05/2014 12:49 PM, [e-mail] wrote:
> From: Akihiro Tsukada <[e-mail]>
>
> Define a standard interface for demod/tuner i2c driver modules.
> A module client calls dvb_i2c_attach_{fe,tuner}(),
> and a module driver defines struct dvb_i2c_module_param and
> calls DEFINE_DVB_I2C_MODULE() macro.
>
> This template provides implicit module requests and ref-counting,
> alloc/free's private data structures,
> fixes the usage of clientdata of i2c devices,
> and defines the platformdata structures for passing around
> device specific config/output parameters between drivers and clients.
> These kinds of code are common to (almost) all dvb i2c drivers/client,
> but they were scattered over adapter modules and demod/tuner modules.
>
> Signed-off-by: Akihiro Tsukada <[e-mail]>
> ---
>   drivers/media/dvb-core/Makefile       |   4 +
>   drivers/media/dvb-core/dvb_frontend.h |   1 +
>   drivers/media/dvb-core/dvb_i2c.c      | 219 ++++++++++++++++++++++++++++++++++
>   drivers/media/dvb-core/dvb_i2c.h      | 110 +++++++++++++++++
>   4 files changed, 334 insertions(+)
>   create mode 100644 drivers/media/dvb-core/dvb_i2c.c
>   create mode 100644 drivers/media/dvb-core/dvb_i2c.h
>
> diff --git a/drivers/media/dvb-core/Makefile b/drivers/media/dvb-core/Makefile
> index 8f22bcd..271648d 100644
> --- a/drivers/media/dvb-core/Makefile
> +++ b/drivers/media/dvb-core/Makefile
> @@ -8,4 +8,8 @@ dvb-core-objs := dvbdev.o dmxdev.o dvb_demux.o dvb_filter.o 	\
>   		 dvb_ca_en50221.o dvb_frontend.o 		\
>   		 $(dvb-net-y) dvb_ringbuffer.o dvb_math.o
>
> +ifneq ($(CONFIG_I2C)$(CONFIG_I2C_MODULE),)
> +dvb-core-objs += dvb_i2c.o
> +endif
> +
>   obj-$(CONFIG_DVB_CORE) += dvb-core.o
> diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h
> index 816269e..41aae1b 100644
> --- a/drivers/media/dvb-core/dvb_frontend.h
> +++ b/drivers/media/dvb-core/dvb_frontend.h
> @@ -415,6 +415,7 @@ struct dtv_frontend_properties {
>   struct dvb_frontend {
>   	struct dvb_frontend_ops ops;
>   	struct dvb_adapter *dvb;
> +	struct i2c_client *fe_cl;

IMHO that is ugly as hell. You should not add any hardware/driver things 
to DVB frontend, even more bad this adds I2C dependency to DVB frontend, 
how about some other busses... DVB frontend is *logical* entity 
representing the DVB device to system. All hardware specific things 
belongs to drivers - not the frontend at all.


>   	void *demodulator_priv;
>   	void *tuner_priv;
>   	void *frontend_priv;

regards
Antti

Akihiro TSUKADA Jan. 15, 2015, 11:20 a.m. UTC | #6

Moikka,
thank you for the comment.

On 2015?01?14? 03:54, Antti Palosaari wrote:
>> --- a/drivers/media/dvb-core/dvb_frontend.h
>> +++ b/drivers/media/dvb-core/dvb_frontend.h
>> @@ -415,6 +415,7 @@ struct dtv_frontend_properties {
>>   struct dvb_frontend {
>>       struct dvb_frontend_ops ops;
>>       struct dvb_adapter *dvb;
>> +    struct i2c_client *fe_cl;
> 
> IMHO that is ugly as hell. You should not add any hardware/driver things
> to DVB frontend, even more bad this adds I2C dependency to DVB frontend,
> how about some other busses... DVB frontend is *logical* entity
> representing the DVB device to system. All hardware specific things
> belongs to drivers - not the frontend at all.

Currently, the i2c_client pointer is distributed/copied among
each demod drivers plus adapter drivers,
so I thought it would be better to unify them into one place,
but since there seems to be no good (common) place to store it,
I'll consider your advice and remove this fe_cl member.

Regards,
akihiro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/media/dvb-core/Makefile b/drivers/media/dvb-core/Makefile
index 8f22bcd..271648d 100644
--- a/drivers/media/dvb-core/Makefile
+++ b/drivers/media/dvb-core/Makefile
@@ -8,4 +8,8 @@  dvb-core-objs := dvbdev.o dmxdev.o dvb_demux.o dvb_filter.o 	\
 		 dvb_ca_en50221.o dvb_frontend.o 		\
 		 $(dvb-net-y) dvb_ringbuffer.o dvb_math.o
 
+ifneq ($(CONFIG_I2C)$(CONFIG_I2C_MODULE),)
+dvb-core-objs += dvb_i2c.o
+endif
+
 obj-$(CONFIG_DVB_CORE) += dvb-core.o
diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h
index 816269e..41aae1b 100644
--- a/drivers/media/dvb-core/dvb_frontend.h
+++ b/drivers/media/dvb-core/dvb_frontend.h
@@ -415,6 +415,7 @@  struct dtv_frontend_properties {
 struct dvb_frontend {
 	struct dvb_frontend_ops ops;
 	struct dvb_adapter *dvb;
+	struct i2c_client *fe_cl;
 	void *demodulator_priv;
 	void *tuner_priv;
 	void *frontend_priv;
diff --git a/drivers/media/dvb-core/dvb_i2c.c b/drivers/media/dvb-core/dvb_i2c.c
new file mode 100644
index 0000000..4ea4e5e
--- /dev/null
+++ b/drivers/media/dvb-core/dvb_i2c.c
@@ -0,0 +1,219 @@ 
+/*
+ * dvb_i2c.c
+ *
+ * Copyright 2014 Akihiro Tsukada <tskd08 AT gmail DOT com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "dvb_i2c.h"
+
+static struct i2c_client *
+dvb_i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info *info,
+		   const unsigned short *probe_addrs)
+{
+	struct i2c_client *cl;
+
+	request_module(I2C_MODULE_PREFIX "%s", info->type);
+	/* Create the i2c client */
+	if (info->addr == 0 && probe_addrs)
+		cl = i2c_new_probed_device(adap, info, probe_addrs, NULL);
+	else
+		cl = i2c_new_device(adap, info);
+	if (!cl || !cl->dev.driver)
+		return NULL;
+	return cl;
+}
+
+struct dvb_frontend *
+dvb_i2c_attach_fe(struct i2c_adapter *adap, const struct i2c_board_info *info,
+		  const void *cfg, void **out)
+{
+	struct i2c_client *cl;
+	struct i2c_board_info bi;
+	struct dvb_i2c_dev_config dcfg;
+
+	dcfg.priv_cfg = cfg;
+	dcfg.out = out;
+	bi = *info;
+	bi.platform_data = &dcfg;
+
+	cl = dvb_i2c_new_device(adap, &bi, NULL);
+	if (!cl)
+		return NULL;
+	return i2c_get_clientdata(cl);
+}
+EXPORT_SYMBOL(dvb_i2c_attach_fe);
+
+struct i2c_client *
+dvb_i2c_attach_tuner(struct i2c_adapter *adap,
+		     const struct i2c_board_info *info,
+		     struct dvb_frontend *fe,
+		     const void *cfg, void **out)
+{
+	struct i2c_board_info bi;
+	struct dvb_i2c_tuner_config tcfg;
+
+	tcfg.fe = fe;
+	tcfg.devcfg.priv_cfg = cfg;
+	tcfg.devcfg.out = out;
+	bi = *info;
+	bi.platform_data = &tcfg;
+
+	return dvb_i2c_new_device(adap, &bi, NULL);
+}
+EXPORT_SYMBOL(dvb_i2c_attach_tuner);
+
+
+static int
+probe_tuner(struct i2c_client *client, const struct i2c_device_id *id,
+	    const struct dvb_i2c_module_param *param,
+	    struct module *this_module)
+{
+	struct dvb_frontend *fe;
+	struct dvb_i2c_tuner_config *tcfg;
+	int ret;
+
+	if (!try_module_get(this_module))
+		return -ENODEV;
+
+	tcfg = client->dev.platform_data;
+	fe = tcfg->fe;
+	i2c_set_clientdata(client, fe);
+
+	if (param->priv_size > 0) {
+		fe->tuner_priv = kzalloc(param->priv_size, GFP_KERNEL);
+		if (!fe->tuner_priv) {
+			ret = -ENOMEM;
+			goto err_mem;
+		}
+	}
+
+	if (param->ops.tuner_ops)
+		memcpy(&fe->ops.tuner_ops, param->ops.tuner_ops,
+			sizeof(fe->ops.tuner_ops));
+
+	ret = 0;
+	if (param->priv_probe)
+		ret = param->priv_probe(client, id);
+	if (ret != 0) {
+		dev_info(&client->dev, "driver private probe failed.\n");
+		goto err_priv;
+	}
+	return 0;
+
+err_priv:
+	kfree(fe->tuner_priv);
+err_mem:
+	fe->tuner_priv = NULL;
+	module_put(this_module);
+	return ret;
+}
+
+static int remove_tuner(struct i2c_client *client,
+			const struct dvb_i2c_module_param *param,
+			struct module *this_module)
+{
+	struct dvb_frontend *fe;
+
+	if (param->priv_remove)
+		param->priv_remove(client);
+	fe = i2c_get_clientdata(client);
+	kfree(fe->tuner_priv);
+	fe->tuner_priv = NULL;
+	module_put(this_module);
+	return 0;
+}
+
+
+static int probe_fe(struct i2c_client *client, const struct i2c_device_id *id,
+		    const struct dvb_i2c_module_param *param,
+		    struct module *this_module)
+{
+	struct dvb_frontend *fe;
+	int ret;
+
+	if (!try_module_get(this_module))
+		return -ENODEV;
+
+	fe = kzalloc(sizeof(*fe), GFP_KERNEL);
+	if (!fe) {
+		ret = -ENOMEM;
+		goto err_fe_kfree;
+	}
+	i2c_set_clientdata(client, fe);
+	fe->fe_cl = client;
+
+	if (param->priv_size > 0) {
+		fe->demodulator_priv = kzalloc(param->priv_size, GFP_KERNEL);
+		if (!fe->demodulator_priv) {
+			ret = -ENOMEM;
+			goto err_fe_kfree;
+		}
+	}
+
+	if (param->ops.fe_ops)
+		memcpy(&fe->ops, param->ops.fe_ops, sizeof(fe->ops));
+
+	ret = 0;
+	if (param->priv_probe)
+		ret = param->priv_probe(client, id);
+	if (ret != 0) {
+		dev_info(&client->dev, "driver private probe failed.\n");
+		goto err_priv_kfree;
+	}
+	return 0;
+
+err_priv_kfree:
+	kfree(fe->demodulator_priv);
+err_fe_kfree:
+	kfree(fe);
+	module_put(this_module);
+	return ret;
+}
+
+static int remove_fe(struct i2c_client *client,
+		     const struct dvb_i2c_module_param *param,
+		     struct module *this_module)
+{
+	struct dvb_frontend *fe;
+
+	if (param->priv_remove)
+		param->priv_remove(client);
+	fe = i2c_get_clientdata(client);
+	kfree(fe->demodulator_priv);
+	kfree(fe);
+	module_put(this_module);
+	return 0;
+}
+
+
+int dvb_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id,
+		  const struct dvb_i2c_module_param *param,
+		  struct module *this_module)
+{
+	return param->is_tuner ? probe_tuner(client, id, param, this_module) :
+				 probe_fe(client, id, param, this_module);
+}
+EXPORT_SYMBOL(dvb_i2c_probe);
+
+int dvb_i2c_remove(struct i2c_client *client,
+		   const struct dvb_i2c_module_param *param,
+		   struct module *this_module)
+{
+	return param->is_tuner ? remove_tuner(client, param, this_module) :
+				 remove_fe(client, param, this_module);
+}
+EXPORT_SYMBOL(dvb_i2c_remove);
diff --git a/drivers/media/dvb-core/dvb_i2c.h b/drivers/media/dvb-core/dvb_i2c.h
new file mode 100644
index 0000000..2bf409d
--- /dev/null
+++ b/drivers/media/dvb-core/dvb_i2c.h
@@ -0,0 +1,110 @@ 
+/*
+ * dvb_i2c.h
+ *
+ * Copyright 2014 Akihiro Tsukada <tskd08 AT gmail DOT com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+/* template code for i2c driver modules */
+
+#ifndef _DVB_I2C_H_
+#define _DVB_I2C_H_
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "dvb_frontend.h"
+
+/* interface for module clients */
+
+struct dvb_frontend *dvb_i2c_attach_fe(struct i2c_adapter *adap,
+				       const struct i2c_board_info *info,
+				       const void *cfg, void **out);
+
+struct i2c_client *dvb_i2c_attach_tuner(struct i2c_adapter *adap,
+					const struct i2c_board_info *info,
+					struct dvb_frontend *fe,
+					const void *cfg, void **out);
+
+/* interface for module drivers */
+
+/* data structures that are set to i2c_client.dev.platform_data */
+
+struct dvb_i2c_dev_config {
+	const void *priv_cfg;
+	/* @out [OUT] pointer to per-device data returned from driver */
+	void **out;
+};
+
+struct dvb_i2c_tuner_config {
+	struct dvb_frontend *fe;
+	struct dvb_i2c_dev_config devcfg;
+};
+
+typedef int (*dvb_i2c_probe_func)(struct i2c_client *client,
+				  const struct i2c_device_id *id);
+typedef int (*dvb_i2c_remove_func)(struct i2c_client *client);
+
+struct dvb_i2c_module_param {
+	union {
+		const struct dvb_frontend_ops *fe_ops;
+		const struct dvb_tuner_ops *tuner_ops;
+	} ops;
+	/* driver private probe/remove functions */
+	dvb_i2c_probe_func  priv_probe;
+	dvb_i2c_remove_func priv_remove;
+
+	u32 priv_size; /* sizeof(device private data) */
+	bool is_tuner;
+};
+
+
+int dvb_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id,
+		  const struct dvb_i2c_module_param *param,
+		  struct module *this_module);
+
+int dvb_i2c_remove(struct i2c_client *client,
+		   const struct dvb_i2c_module_param *param,
+		   struct module *this_module);
+
+
+#define DEFINE_DVB_I2C_MODULE(dname, idtab, param) \
+static int _##dname##_probe(struct i2c_client *client,\
+				const struct i2c_device_id *id)\
+{\
+	return dvb_i2c_probe(client, id, &(param), THIS_MODULE);\
+} \
+\
+static int _##dname##_remove(struct i2c_client *client)\
+{\
+	return dvb_i2c_remove(client, &(param), THIS_MODULE);\
+} \
+\
+MODULE_DEVICE_TABLE(i2c, idtab);\
+\
+static struct i2c_driver dname##_driver = {\
+	.driver = {\
+		.name = #dname,\
+	},\
+	.probe    = _##dname##_probe,\
+	.remove   = _##dname##_remove,\
+	.id_table = idtab,\
+};\
+\
+module_i2c_driver(dname##_driver)
+
+#endif /* _DVB_I2C_H */