[linux-dvb] Re: refactoring

[Andrew de Quincey <adq_dvb@lidskialf.net>]

On Sunday 10 Oct 2004 12:48, Holger Waechtler wrote:
> Andrew de Quincey wrote:
> >On Saturday 09 Oct 2004 10:10, Gerd Knorr wrote:
> >>>>Even a sane designed library will allocate and release the data
> >>>>structures itself and *not* ask the user of the library to do that.
> >>>>Data structures which are private (to the driver, to library or
> >>>>whatever you prefeare to call it) should be maintained by the owner and
> >>>>not someone else.  Not designing it this way is just insane and asking
> >>>>for trouble, please don't.
> >>>
> >>>You can find examples of exactly this approach all over the kernel.
> >>>Look at i2c_adapter for example - there are several fields defined in
> >>>there that should not be messed with by anything outside the i2c core.
> >>>Nothing is stopping you for mucking about with the list of clients
> >>>directly - but you'd be mad if you did.
> >>
> >>That it exists doesn't mean that it is a good idea.  Also i2c core isn't
> >>exactly a good example, it has various cruft in there for historical
> >>reasons.  Also note that the struct i2c_adapter is allocated + released
> >>by the i2c adapter drivers and _not_ by the i2c core.
> >>
> >>>This is pragmatism - hiding data is good I agree. However, there comes
> >>>a time when it is simply not worth the extra hassle.
> >>
> >>There is no extra hassle.  You have to allocate the data struct for fe
> >>_anyway_.  You'll just move that from the fe driver to the dvb adapter
> >>driver, which is simply the wrong place.
> >>
> >>IMHO the interface should look like that:
> >>
> >>struct dvb_fe {
> >> fe_type_t       type;
> >> struct fe_ops   *ops;
> >> void            *priv;
> >>};
> >>
> >>struct fe_ops {
> >> void (*set_tps)(struct dvb_fe*, struct dvb_frontend_parameters*);
> >> void (*get_tps)(struct dvb_fe*, struct dvb_frontend_parameters*);
> >> void (*get_status)(struct dvb_fe*, struct fe_status*);
> >> /* whatever else is needed, for diseqc, ... */
> >> void (*destroy)(struct dvb_fe*);
> >>};
> >>
> >>The prototypes for initializing the fe driver:
> >>
> >>struct dvb_fe* create_<fename>(/* whatever is needed for the fe in
> >> question */);
> >>
> >>dvb_(un)register_frontend() needs just two args then: the dvb adapter
> >>ptr and a ptr to struct dvb_fe.  Anything inside the dvb code which has
> >>to deal with frontends just has to pass around a struct dvb_fe ptr as
> >>reference for the frontend, all you need to access it is in there.
> >>
> >>Also note that there is absolutely no reference to i2c, if there is some
> >>frontend wich is *not* connected via i2c -- no problem.  The only
> >>reference to i2c will be in the frontend-specific create_<fename>()
> >>function, where the i2c ones will get passed in a ptr to the i2c_adapter
> >>they should attach to.
> >
> >Sounds nifty! I'll have a look into it later. This _is_ an experimental
> > branch after all - suggest something cooler and I'll do it. :)
> >
> >Also, I saw Johannes' comment about the number of parameters - this is
> >certainly the case for the stv0299 - it would have about 15 parameters to
> > the create_XXX() function.
>
> The early proposals in this thread suggested a stv0299_config struct
> containing the card-specific inittable, clock values and addresses of
> the i2c devices. This is static data and can even live in the .bss section.

A couple of the drivers do contain a small amount of internal state - things 
like the stv0299 have to remember the previous frequency tuned to so it can 
do a "finetune". These should not "really" be accessible to things external 
to the frontend. I simply put them in the _state/_cardspec structures to 
avoid having to kmalloc/kfree memory within the frontends. This is because it 
is usually just a couple of bytes needed and I didn't see the point in extra 
complication. We shall see though.

As to whether Gerd's suggestion is going to work or not - I simply don't know 
yet - but I'm willing to investigate it to see, once I've done the transition 
to a function pointer API.