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

Hans Verkuil hverkuil at xs4all.nl
Mon Aug 20 17:42:08 CEST 2007


On Monday 20 August 2007 17:22:41 Michael Krufky wrote:
> 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".

I was perfectly content with your original version, but if some people 
object to the presence of the analog_parameters struct in this header 
then a forward reference is an acceptable alternative.

> 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.

I agree: void pointers assume that the type is unknown (differs for each 
driver). That is not the case here, so the correct type should be used 
in the interest of type-safety.

Regards,

	Hans



More information about the linux-dvb mailing list