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

Michael Krufky mkrufky at linuxtv.org
Mon Aug 20 17:22:41 CEST 2007


Manu Abraham wrote:
> 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
You are getting ahead of things, Manu.  The tuner.ko module will 
continue to remain as the v4l2 / i2c_client interface to the tuning 
functionality.  We do not (yet) have to add these ioctl calls to the 
card-level drivers, as they will continue to interact with the various 
tuner modules via tuner.ko's i2c_client interface.

I do have plans to remove this tuner.ko interface in the future, but 
that is way way in the future, after most of my planned tuner 
refactoring work is complete.  There will be a completely separate RFC 
issued at that time.  When that happens, the card level drivers will 
include dvb_frontend.h, just as the dvb card drivers do, and access the 
tuning methods that way, through the dvb_frontend structure.

I believe that is is very important to deal with one thing at a time -- 
THIS step of the refactoring process deals with separating the 
hardware-specific code from tuner.ko, making those modules usable by 
both the analog and digital tuning systems.

> 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"
No, these pointers are completely internal to tuner.ko, only currently 
in a separate header because tda9887 has not yet been refactored -- this 
will change after the analog IF demod refactoring.

> 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.
Manu, if you consider an hybrid NIM, for example, the LG-H06xF, you find 
the following internals:

tua6034 - tuner pll
lgdt3303 - digital demodulator
tda9887 - analog demodulator

OK, now lets consider the fmd1216me

tua6034 - tuner pll
tda9887 - analog demodulator
in this case, the digital demodulator is found externally.

My point here, is that the "frontend" , in general, includes the tuner 
itself, sometimes the digital demodulator, and sometimes the analog 
demodulator.  I feel that to keep this separate adds an unnecessary 
layer of abstraction, and can only lead to further confusion.  The 
separation is logical, based on the structures within dvb_frontend.h -- 
to have things in separate header files makes things look dirty to me.


Hans Verkuil wrote:
>> Manu Abraham wrote:
>>     
>>> Michael Krufky wrote:
>>> 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.
>>     
>
> Why not just add a forward reference to the analog_parameters struct in
> dvb_frontend.h and have the actual definition somewhere else? As long as
> that struct is not actually used it still compiles fine.
>
> At first glance that certainly seems better than adding a void pointer as
> a replacement for the set_analog_params function pointer.
>
> Or am I missing something here?
I don't like the void pointer either -- those _priv pointers are usually 
used to store device-specific private data, and it seems to me to be 
just a hack to avoid adding anything into dvb_frontend.h at zero gain.

If we really do have to do something like this, then a forward reference 
does seem much more appropriate to me.  Although, all in all, this seems 
to be unnecessary, and only adds extra confusion to what was originally 
quite a simple change.

We must keep in mind that we are only trying to add a way to reuse 
existing code to be able to tune analog -- this is something that must 
be available to _every_ tuner module, and to store it in a separate 
header only leads to the #include of that header into more files, where 
the one appropriate place for this structure *is* within dvb_frontend.h

...or are you suggesting to create a new header file and include that 
one within dvb_frontend.h ??  Again, I dont see a gain from this, unless 
we are simply getting hun up on the presence of these structures inside 
the dvb_frontend.h file.  To include the new header in this same file 
make would leave everything as-is, just move the structures elsewhere to 
satisfy Manu's desire to keep dvb_frontend.h "to be purely digital for 
anyone reading the code".

Manu Abraham wrote:
> Hans Verkuil wrote:
>
>   
>> At first glance that certainly seems better than adding a void pointer as
>> a replacement for the set_analog_params function pointer.
>>     
>
>
> Sometimes elegance also matters. Other than that the convention that we
> have is (from dvb_frontend.h)
>
>       151 	void* demodulator_priv;
>       152 	void* tuner_priv;
>       153 	void* frontend_priv;
>       154 	void* sec_priv;
>
> for private resources. Since the hybrid operation is private to the
> device, just followed the same convention, which additionally brings in
> the advantage of *not* having private resources into the header for dvb
> core.
THIS is the root of our disagreement -- The analog tuning functionality 
is _not_ private to the device -- this is a system-wide addition to the 
dvb_frontend structure, because we are adding analog tuning 
functionality to the dvb_frontend.

I agree that elegance is very important, and that is why I disagree with 
the extra layers of complexity that you are proposing here.

-Mike




More information about the linux-dvb mailing list