[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*);
> };

Aha, we're starting to converge here. This is _very_ similar to the structure 
I prototyped for the function pointers a few days ago:

struct dvb_frontend_api {
        struct dvb_frontend_info info;
        struct dvb_adapter *dvb;
        void* vstate;

        int (*sleep)(void* vstate);
 .... other stuff
};

Before anyone gets too excited, this was an experimental structure. I think a 
better solution would be something like:

struct dvb_fe {
 struct fe_api  *api;
 struct dvb_adapter *dvb;
 void            *priv;
};

struct fe_api {
 struct dvb_frontend_info info;

 void (*attach)(....)
 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*);
};

fe_api then allows the card driver to override the functions as necessary, and 
also to tailor the dvb_frontend_info structure to be suited to its exact 
hardware.

Adding dvb_adapter to dvb_fe is purely for convenience - its not really 
necessary if people don't like it.