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
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
