[linux-dvb] Re: refactoring

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

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.