[linux-dvb] [RFC] Hybrid tuner refactoring, phase 1

Manu Abraham abraham.manu at gmail.com
Mon Aug 20 16:33:16 CEST 2007

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.

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: cleanup_mkrufky_refactor.patch
Type: text/x-patch
Size: 8006 bytes
Desc: not available
Url : http://www.linuxtv.org/pipermail/linux-dvb/attachments/20070820/866ec3ad/attachment.bin 

More information about the linux-dvb mailing list