[linux-dvb] Re: refactoring

[Holger Waechtler <holger@qanu.de>]

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.

Holger