[linux-dvb] Re: Proposal for new frontend architecture]

[Gerd Knorr <kraxel@bytesex.org>]

  Hi,

Oops, did hit reply instead of group-reply :-/
resending ...

  Gerd

----- Forwarded message from Gerd Knorr <kraxel@bytesex.org> -----

Date: Fri, 24 Sep 2004 12:11:11 +0200
From: Gerd Knorr <kraxel@bytesex.org>
Subject: Re: [linux-dvb] Re: Proposal for new frontend architecture
To: Andrew de Quincey <adq_dvb@lidskialf.net>
Content-Type: text/plain; charset=us-ascii
In-Reply-To: <200409231846.00425.adq_dvb@lidskialf.net>
User-Agent: Mutt/1.5.6i

On Thu, Sep 23, 2004 at 06:46:00PM +0100, Andrew de Quincey wrote:
> On Thursday 23 Sep 2004 18:03, Michael Hunold wrote:
> >
> > I was just about to finish my proof-of-concept on the stv0299 driver.
> > Grrr... ;-)

Faster ;)

> > int i2c_connect_client(struct i2c_adapter *adap, int id, void
> > *interface, struct i2c_client **client)

> Looks interesting - I agree it would be better not to have an explicit 
> dependency. The only niggly things I can think of are:
> 
> (a) It uses IDs to identify i2c clients - I believe there were reasons to 
> avoid this for proprietary drivers - they'd need to have an i2c ID defined in 
> the OSS kernel, which isn't right.

I think I'd prefeare strcmp on ->name as well.

Not sure all the ID stuff was a good idea in the first place.  We have
both name and id in all the structs which is sort of redundant.  ->name
is nicer as it is human readable: you can printk it, put it into sysfs
and so on.  The places where ->id is used are not performance critical,
so using ->name instead and drop ->id should be ok.  But thats another
story ...

> (b) It isn't typesafe.

Exactly.  Thats the main issue I see with the approach, we have *again*
a void ptr where we'll pass random stuff, and I suspect the guys on lkml
will still not like that very much.

I see your point in not wanting to have a symbol dependendy, I don't see
a path to do that without loosing type checking through.  Especially the
initialization is critical as the arguments needed for that are very
hardware specific.  Or is there any chance to come up with a set of
initialization parameters which we don't have to touch for each and
every new frontend?  So we can have a init function common to all dvb
frontends?

I also don't like the i2c_connect_client().  I think I'd prefeare to
have just this in i2c-core:

	struct i2c_driver* i2c_driver_get(char *name);
	i2c_driver_put(struct i2c_driver *drv);

i2c_driver_get() would just do the lookup, also care about module ref
counting and anything else which is needed (maybe even request_module()
?).  connecting/disconnection is left to the user (i.e. dvb adapter
driver in our case), which means we could have our own connect function,
maybe this way:

	struct dvb_fe_i2c_driver {
		struct i2c_driver i2c;
		struct i2c_client* fe_connect(i2c_adapter,
					      dvb_adapter,
					      struct fe_init_params);
		fe_disconnect(struct i2c_client *c);
	}

init:

	request_module("cx22702")
	i2c_driver *drv = i2c_driver_get("cx22702");
	dvb_fe_i2c_driver *fe = container_of(drv, struct dvb_fe_i2c_driver, i2c);
	i2c_client *cl = fe->fe_connect(...);

exit:

	fe->de_disconnect(cl);
	i2c_driver_put(drv);

Comments?

  Gerd

-- 
return -ENOSIG;