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