[linux-dvb] Re: refactoring

[Gerd Knorr <kraxel@bytesex.org>]

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

  Gerd

-- 
return -ENOSIG;