[linux-dvb] Re: refactoring

[Gerd Knorr <kraxel@bytesex.org>]

On Sun, Oct 10, 2004 at 08:40:46PM +0100, Andrew de Quincey wrote:
> So, prototyping this for at76c651:
> 
> at76c651.h:
> 
> struct at76c651_config
> {
>         /* the i2c adapter to use */
>         struct i2c_client* i2c;

No, struct i2c_client isn't config data and is private to the frontend
driver, that belongs into _state.  Just the i2c address(es) should be in
_config.  Not sure about i2c_adapter, probably that should be passed as
separate argument.  So you have only the really static stuff in _config
(i.e. it could live in read-only mapped memory and be shared between
multiple instances of the frontend).

>         /* PLL maintenance */
>         int (*pll_init)(struct dvb_frontend* fe);
>         int (*pll_set)(struct dvb_frontend* fe, struct 
> dvb_frontend_parameters* params);

Looks ok.  Not sure whenever sharing the pll code for multiple frontends
will work that way through.  Is that important in practice btw?  i.e. do
we actually have frontends with same pll but different demod?

> extern int at76c651_attach(struct dvb_frontend* fe, struct at76c651_config* 
> config);

I'd use this
  struct dvb_frontend* at76c651_attach(struct i2c_adapter *adap,
                                       struct at76c651_config *conf);

> struct at76c651_state {
>         struct at76c651_config* config;
> 
>         /* revision of the chip */
>         u8 revision;
> 
>         /* last QAM value set */
>         u8 qam;
> };

struct i2c_client should be here as well as mentioned above.  Otherwise
it looks fine to me.

> The *_state is the private data to the demod. I can't decide whether it should 
> be a pointer to the config, or whether it should be an instance of it, and 
> just memcpy whatever is passed in to the *_attach() function. 

I think just storing the pointer is fine.  That needs to be clearly
defined through.  Depending on how we handle that the dvb adapter must
make sure it stays valid for the whole livetime of the frontend instance
or not.  If it is possible to have just a static data block for _config
it is trivial to make sure the pointer stays valid through, so I think
we should take that route ;)

  Gerd

-- 
return -ENOSIG;