Michael Krufky wrote:
> Manu Abraham wrote:
>> Michael Krufky wrote:
>>
>>> For the past few months, I've been working on refactoring the analog tuner.ko module, such that all hardware-specific code can be separated into dvb_frontend style tuner modules.
>>>
>>> This allows for a single module to be used by both the v4l2 tuner interface via the tuner.ko i2c_client driver, and directly by the dvb subsystem's tuning system.
>>>
>>> This refactoring process has zero impact to the way that v4l and dvb functions.
>>>
>>> I have completed phase one of the refactoring process, and now it is ready for testing and review.
>>>
>>> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-1
>>>
>>> A brief description of the individual changesets follows:
>>>
>>> - tuner: kill i2c_client interface to tuner sub-drivers
>>>
>>> This changeset removes the i2c_client interface between tuner.ko and the tuner sub-drivers.
>>>
>>> The i2c_client interface to tuner.ko, itself, remains the same as it has been -- this is only an internal change that affects the interaction between tuner.ko and the hardware-specific code.
>>>
>>> Some helper functions and macros were added in this changeset, in order to ease the conversion process, without causing headaches or breakage. (see tuner-i2c.c) We can remove these extra structs and helper functions after the refactoring process is complete.
>>>
>>> - hybrid tuner refactoring core changes, phase 1
>>>
>>> This changeset contains the more interesting work, where tuner-core is altered to support attachment of dvb_frontend style tuner modules. An additional method "set_analog_params" was added to struct dvb_tuner_ops, so as to avoid altering the DVB subsystem userspace API headers. This change does not create any dependency of the DVB subsystem on V4L, nor does it create any dependency of the V4L subsystem on DVB.
>>>
>>>
>> It looks fine, althought one aspect i would like to mention:
>>
>>
>> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-1/file/2813710f99ba/linux/drivers/media/dvb/dvb-core/dvb_frontend.h
>>
>> 67 struct analog_parameters {
>> 68 unsigned int frequency;
>> 69 unsigned int mode;
>> 70 unsigned int audmode;
>> 71 __u64 std;
>> 72 };
>>
>> 84 int (*set_analog_params)(struct dvb_frontend *fe, struct
>> analog_parameters *p);
>>
>>
>> Rather than having the analog operations/data structures into
>> dvb_frontend (which is supposed to be purely digital for anyone reading
>> the code), you can move the operations/struct into the hybrid tuner
>> header (below) where the operations are really meant for.
>>
>>
>> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-1/file/2813710f99ba/linux/drivers/media/video/tuner-driver.h
>>
>> 28 #include "dvb_frontend.h"
>>
>>
>> This would allow not to add in dvb_frontend header into the tuner header
>> unnecessarily as well.
>>
>>
>> The whole point being if we keep adding all stuff to dvb_frontend, in
>> the end it ends up with lot of stuff which aren't directly related.
>>
>>
>> Looks fine otherwise.
>>
> Manu,
>
> tuner-driver.h will be entirely deleted at the end of the refactoring
> process. If you notice, inside the new dvb_frontend style tuners
> (post-conversion) these source files no longer include "tuner-driver.h"
> -- All of the contents of tuner-driver.h is for the old tuner.ko
> internal api, which has been replaced by the dvb_frontend internal api.
> This file will be deleted after tda9887 goes through analog if demod
> refactoring. The "struct tuner" declaration will be moved to the top of
> tuner-core.c
You will need to handle the IOCTL calls for analog operations some
place, whatever you remove.
I don't see at any place you are handling the IOCTL's directly in the
drivers. So you will be using callbacks from the place where you
are applying the ioctl's
Just add in a header for the same, where you handle the specific
callbacks for those specific operations
Even if you throw away that header:
you will need to put these ops somewhere in a header, to be used by
individual drivers:
34 struct tuner_operations {
35 void (*set_tv_freq)(struct tuner *t, unsigned int freq);
36 void (*set_radio_freq)(struct tuner *t, unsigned int freq);
37 int (*has_signal)(struct tuner *t);
38 int (*is_stereo)(struct tuner *t);
39 int (*get_afc)(struct tuner *t);
40 void (*tuner_status)(struct tuner *t);
41 void (*standby)(struct tuner *t);
42 void (*release)(struct tuner *t);
43 };
You can put the struct "where this lump of fn. pointers will be" as they
belong to the same "device class"
Having it in one place, makes it easier to read the code as well.
> We cannot move struct analog_parameters out of dvb_frontend.h , since
> the new set_analog_params method must be a part of the dvb_tuner_ops
> structure. This is a very small price to pay, for quite a large gain,
You mean to say it is impossible to do that ? sounds strange for
something that simple.
> without creating any dependencies of the DVB subsystem onto V4L, and
> also without creating any dependencies of the V4L subsystem onto DVB.
>
> The idea of my refactoring work, was to add the analog tuning capability
> to the dvb_frontend style tuner modules, allowing us to have a single
> module that can receive tuning commands for both analog and digital.
Still just one single driver only. Haven't been talking about a driver
split in two for analog or digital at all. One driver per silicon, was
what i stated earlier as well.
> Since we are adding this capability to the dvb_frontend internal API,
> naturally, this must all appear within dvb_frontend.h --
Not necessarily. See attached patch.
> There are
> already dvb_frontend tuner modules in the frontends/ tree whose hardware
> is capable of tuning analog, but have not been able to thus far, due to
> code limitations. The change described above gets us past those
> limitations.
Ok, that's the whole point we had for the xc3028 discussions as well,
the same goes for other discussions that came out of it as well.
> There is no need for this desire to separate this code into separate
> headers. After all, we are dealing with, potentially, a single piece of
> hardware that will have to be able to tune both analog and digital.
The point here is not a single device tuning to analog or digital. DVB
doesn't need to know about analog operations, although the same chip
driver should.
NOTE:
A while later, when you see analog demodulators/decoders also on the same
chip, people will start pushing even more in. Then we will have even more
of issues. Restart the flamewars again (someone writing a driver for a
new chip will do that, provided he has a large ego) that we saw
a while back.
> This is one thing that we are not going to agree on,
Why disagree ?
It is not nice to push everything into the DVB frontend header, just
because that is convenient for the time being. If it were a driver alone, no
one would have cared -- but this affects the entire subsystem.
Also: the analog operation (hybrid feature) is device specific, ie a
private resource that which is not handled by the DVB frontend, hence no
need for DVB frontend to know what it is, but the drivers needs the
same, which is used elsewhere, ie not used within DVB.
The only reason why DVB is aware of the private resource is because it
is behind a DVB specific operation (gated control of the DVB demodulator)
> unless you can show
> a patch on top of my tree that will satisfy your requirements.
> Otherwise, I hope that you can get past this small issue.
I did have a complete driver that does this, a dummy hybrid driver, if
you look back. Posted to the DVB and V4L mailing lists. Any problems
that you see there ?
Have been a bit busy for the promo for the next month beginning, but
still took a bite at it, to provide an idea. The changes apply to
tuner_simple alone, you should disable compilation of the others for not
causing a build failure after the application of the patch. It can be
applied to other devices as well, the same way. Nothing earth shattering
in there.
Also a minor __u* to u* change. You shouldn't use __u64 for internal
kernel interfaces, it should be used only for user API, ie what the
userland interfaces with.
The patch attached depicts the same.
diff -Naurp original/linux/drivers/media/dvb/dvb-core/dvb_frontend.h tuner-refactor-phase-1/linux/drivers/media/dvb/dvb-core/dvb_frontend.h
--- original/linux/drivers/media/dvb/dvb-core/dvb_frontend.h 2007-08-20 10:46:55.000000000 +0400
+++ tuner-refactor-phase-1/linux/drivers/media/dvb/dvb-core/dvb_frontend.h 2007-08-20 13:13:21.000000000 +0400
@@ -64,13 +64,6 @@ struct dvb_tuner_info {
u32 bandwidth_step;
};
-struct analog_parameters {
- unsigned int frequency;
- unsigned int mode;
- unsigned int audmode;
- __u64 std;
-};
-
struct dvb_tuner_ops {
struct dvb_tuner_info info;
@@ -81,7 +74,6 @@ struct dvb_tuner_ops {
/** This is for simple PLLs - set all parameters in one go. */
int (*set_params)(struct dvb_frontend *fe, struct dvb_frontend_parameters *p);
- int (*set_analog_params)(struct dvb_frontend *fe, struct analog_parameters *p);
/** This is support for demods like the mt352 - fills out the supplied buffer with what to write. */
int (*calc_regs)(struct dvb_frontend *fe, struct dvb_frontend_parameters *p, u8 *buf, int buf_len);
@@ -168,6 +160,7 @@ struct dvb_frontend {
void* tuner_priv;
void* frontend_priv;
void* sec_priv;
+ void* hybrid_priv;
};
extern int dvb_register_frontend(struct dvb_adapter* dvb,
diff -Naurp original/linux/drivers/media/video/tuner-core.c tuner-refactor-phase-1/linux/drivers/media/video/tuner-core.c
--- original/linux/drivers/media/video/tuner-core.c 2007-08-20 10:46:56.000000000 +0400
+++ tuner-refactor-phase-1/linux/drivers/media/video/tuner-core.c 2007-08-20 14:15:56.000000000 +0400
@@ -99,6 +99,34 @@ MODULE_DESCRIPTION("device driver for va
MODULE_AUTHOR("Ralph Metzler, Gerd Knorr, Gunther Mayer");
MODULE_LICENSE("GPL");
+
+#if LINUX_VERSION_CODE <= KERNEL_VERSION(2,6,15)
+#define tuner_warn(fmt, arg...) do {\
+ printk(KERN_WARNING "%s %d-%04x: " fmt, t->i2c.driver->name, \
+ i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
+#define tuner_info(fmt, arg...) do {\
+ printk(KERN_INFO "%s %d-%04x: " fmt, t->i2c.driver->name, \
+ i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
+#define tuner_dbg(fmt, arg...) do {\
+ extern int tuner_debug; \
+ if (tuner_debug) \
+ printk(KERN_DEBUG "%s %d-%04x: " fmt, t->i2c.driver->name, \
+ i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
+#else
+#define tuner_warn(fmt, arg...) do {\
+ printk(KERN_WARNING "%s %d-%04x: " fmt, t->i2c.driver->driver.name, \
+ i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
+#define tuner_info(fmt, arg...) do {\
+ printk(KERN_INFO "%s %d-%04x: " fmt, t->i2c.driver->driver.name, \
+ i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
+#define tuner_dbg(fmt, arg...) do {\
+ extern int tuner_debug; \
+ if (tuner_debug) \
+ printk(KERN_DEBUG "%s %d-%04x: " fmt, t->i2c.driver->driver.name, \
+ i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
+#endif
+
+
static struct i2c_driver driver;
static struct i2c_client client_template;
@@ -106,7 +134,8 @@ static struct i2c_client client_template
static void fe_set_freq(struct tuner *t, unsigned int freq)
{
- struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
+ struct dvb_frontend *fe = &t->fe;
+ struct hybrid_ops *hybrid = fe->hybrid_priv;
struct analog_parameters params = {
.frequency = freq,
@@ -115,11 +144,10 @@ static void fe_set_freq(struct tuner *t,
.std = t->std
};
- if (NULL == fe_tuner_ops->set_analog_params) {
- tuner_warn("Tuner frontend module has no way to set freq\n");
- return;
- }
- fe_tuner_ops->set_analog_params(&t->fe, ¶ms);
+ if (hybrid->set_analog_params)
+ hybrid->set_analog_params(&t->fe, ¶ms);
+ else
+ tuner_warn("Missing callback to set analog params\n");
}
static void fe_release(struct tuner *t)
@@ -221,6 +249,9 @@ static void set_type(struct i2c_client *
struct dvb_tuner_ops *fe_tuner_ops = &t->fe.ops.tuner_ops;
unsigned char buffer[4];
+ struct dvb_frontend *fe = &t->fe;
+ struct hybrid_ops *hybrid = fe->hybrid_priv;
+
if (type == UNSET || type == TUNER_ABSENT) {
tuner_dbg ("tuner 0x%02x: Tuner type absent\n",c->addr);
return;
@@ -323,7 +354,7 @@ static void set_type(struct i2c_client *
break;
}
- if (fe_tuner_ops->set_analog_params) {
+ if (hybrid->set_analog_params) {
strlcpy(t->i2c.name, fe_tuner_ops->info.name, sizeof(t->i2c.name));
t->ops.set_tv_freq = fe_set_freq;
diff -Naurp original/linux/drivers/media/video/tuner-driver.h tuner-refactor-phase-1/linux/drivers/media/video/tuner-driver.h
--- original/linux/drivers/media/video/tuner-driver.h 2007-08-20 10:46:56.000000000 +0400
+++ tuner-refactor-phase-1/linux/drivers/media/video/tuner-driver.h 2007-08-20 14:09:03.000000000 +0400
@@ -29,6 +29,13 @@
extern unsigned const int tuner_count;
+struct analog_parameters {
+ unsigned int frequency;
+ unsigned int mode;
+ unsigned int audmode;
+ u64 std;
+};
+
struct tuner;
struct tuner_operations {
@@ -42,6 +49,10 @@ struct tuner_operations {
void (*release)(struct tuner *t);
};
+struct hybrid_ops {
+ int (*set_analog_params)(struct dvb_frontend *fe, struct analog_parameters *p);
+};
+
struct tuner {
/* device */
struct i2c_client i2c;
@@ -76,34 +87,9 @@ extern int tda9887_tuner_init(struct tun
/* ------------------------------------------------------------------------ */
-#if LINUX_VERSION_CODE <= KERNEL_VERSION(2,6,15)
-#define tuner_warn(fmt, arg...) do {\
- printk(KERN_WARNING "%s %d-%04x: " fmt, t->i2c.driver->name, \
- i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
-#define tuner_info(fmt, arg...) do {\
- printk(KERN_INFO "%s %d-%04x: " fmt, t->i2c.driver->name, \
- i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
-#define tuner_dbg(fmt, arg...) do {\
- extern int tuner_debug; \
- if (tuner_debug) \
- printk(KERN_DEBUG "%s %d-%04x: " fmt, t->i2c.driver->name, \
- i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
-#else
-#define tuner_warn(fmt, arg...) do {\
- printk(KERN_WARNING "%s %d-%04x: " fmt, t->i2c.driver->driver.name, \
- i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
-#define tuner_info(fmt, arg...) do {\
- printk(KERN_INFO "%s %d-%04x: " fmt, t->i2c.driver->driver.name, \
- i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
-#define tuner_dbg(fmt, arg...) do {\
- extern int tuner_debug; \
- if (tuner_debug) \
- printk(KERN_DEBUG "%s %d-%04x: " fmt, t->i2c.driver->driver.name, \
- i2c_adapter_id(t->i2c.adapter), t->i2c.addr , ##arg); } while (0)
-#endif
-
#endif /* __TUNER_HW_H__ */
+
/*
* Overrides for Emacs so that we follow Linus's tabbing style.
* ---------------------------------------------------------------------------
diff -Naurp original/linux/drivers/media/video/tuner-simple.c tuner-refactor-phase-1/linux/drivers/media/video/tuner-simple.c
--- original/linux/drivers/media/video/tuner-simple.c 2007-08-20 10:46:56.000000000 +0400
+++ tuner-refactor-phase-1/linux/drivers/media/video/tuner-simple.c 2007-08-20 14:07:21.000000000 +0400
@@ -16,6 +16,7 @@
#endif
#include "tuner-i2c.h"
#include "tuner-simple.h"
+#include "tuner-driver.h"
static int debug = 0;
module_param(debug, int, 0644);
@@ -620,12 +621,15 @@ static struct dvb_tuner_ops simple_tuner
.frequency_step = ,
},
#endif
- .set_analog_params = simple_set_params,
.release = simple_release,
.get_frequency = simple_get_frequency,
.get_status = simple_get_status,
};
+static struct hybrid_ops simple_hybrid_ops = {
+ .set_analog_params = simple_set_params,
+};
+
struct dvb_frontend *simple_tuner_attach(struct dvb_frontend *fe,
struct i2c_adapter * i2c_adap,
u8 i2c_addr, unsigned int type,
@@ -637,6 +641,7 @@ struct dvb_frontend *simple_tuner_attach
if (priv == NULL)
return NULL;
fe->tuner_priv = priv;
+ fe->hybrid_priv = &simple_hybrid_ops;
priv->i2c_props.addr = i2c_addr;
priv->i2c_props.adap = i2c_adap;
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
