[linux-dvb] Re: CX88 i2c issue w/ DVB tuners

[Holger Waechtler <holger@qanu.de>]

Ralph Metzler wrote:
> Andrew de Quincey writes:
>  > On Friday 10 Sep 2004 13:10, Ralph Metzler wrote:
>  > > Holger Waechtler writes:
>  > >  > Andrew de Quincey wrote:
>  > >  > > Yeah, that sounds good. So effectively the frontend drivers will cease
>  > >  > > to be kernel i2c devices. However, we'll still use the kernel i2c for
>  > >  > > the adapter's i2c buses if need be, so no one can complain we're
>  > >  > > duplicating stuff again. That makes sense - theres no point really in
>  > >  > > making the frontends generic devices usable by non-DVB code.
>  > >  > >
>  > >  > > How will we prevent things like that tda9887 tuner from taking over
>  > >  > > our addresses on the bus accidentally if our bus is marked as ANALOGUE
>  > >  > > and DIGITAL? We should probably claim addresses on the bus that we use
>  > >  > > - but it all depends on what order things are loaded in.
>  > >  >
>  > >  > I would not claim any addresses, why should we? Even registration to the
>  > >  > kernel i2c API is optional (and only introduces delays and the risk of
>  > >  > accidentally attached devices from other subsystems).
>  > >  >
>  > >  > The generic i2c_read/write() implementations in the card drivers should
>  > >  > allow writes to all addresses. If not we should fix them, we're
>  > >  > maintaining them anyway. We also need to ensure they are correctly
>  > >  > locking on the low level because we don't rely on the i2c layer locking
>  > >  > mechanism anymore (but last time I checked most drivers implemented
>  > >  > correct semaphores, so this should not really be a problem).
>  > >  >
>  > >  > If properly done both systems can existence peacefully in coexistence
>  > >  > and concurrent accesses are allowed (like it was the case for the
>  > >  > dvb_i2c_bus code).
>  > 
>  > Sorry! hit the wrong key before.
>  > 
>  > > And what exactly are the advantages of having these two systems for
>  > > similar things again?
>  > > You will again have a lot of code duplication with only minimal
>  > > differences depending on the card type and necessary fixes
>  > > (like the locking) to make it work with existing structures if
>  > > you happen to need other i2c clients.
>  > 
>  > yeah, I don't like having multiple i2c implementations hitting the same bus. 
>  > Thats why I would prefer to use the kernel i2c implementation where possible, 
>  > since it handles locking and bitbanging (if necessary) for us.
>  > 
>  > > Getting rid of wrong attaches is easily done by introducing a
>  > > client type field as I described before or just by getting an id range
>  > > reserved in i2c-id.h as it is already done for sensors and V4L2 common
>  > > components (is the latter used yet?).
>  > 
>  > We can't have an id per frontend - from what Holger described about the 
>  > binary-only firmwares.
>
> Completely closed drivers could still use their own thin I2C layers or
> other means (see below).
>
> I see how what Holger describes would make things a little bit 
> easier for pure DVB drivers if you want to use existing
> i2c clients you get added complexity.
no. Even the braindead kernel i2c API provides a simple read/write 
interface:

extern int i2c_transfer (struct i2c_adapter *adap, struct i2c_msg msg[], 
int num);

well, it's brain-dead but usable.


> But I can see how one could use such a really thin layer and
> alternatively either put a generic i2c kernel client registration
> layer above it or use it directly with a DVB driver if the latter 
> really does not need anything else.
The thin layer already exists, almost all i2c drivers implement the 
master_transfer function like this:

int i2c_transfer (struct i2c_adapter *adap,
                  struct i2c_msg msg[],
                  int num)
{
	for (i=0; i<num; i++) {
		if (msg[i].flags & I2C_M_RD)
			i2c_read(adap, msg[i].buf, msg[i].len);
		else
			i2c_write(adap, msg[i].buf, msg[i].len);
	}
}


Nothing new needs to get introduced despite two convinience functions 
which reverse the useless API abstraction and access the read/write 
functions directly again:


int card_i2c_write (struct card *card, const u8 *buf, int len)
{
	struct i2c_msg m = { .flags = 0, .buf = buf, .len = len };
	return i2c_transfer(&card->i2c_adap, &m, 1) == 1 ? len : -EIO;
}


int card_i2c_read (struct card *card, const u8 *buf, int len)
{
	struct i2c_msg m = { .flags = I2C_M_RD, .buf = buf, .len = len };
	return i2c_transfer(&card->i2c_adap, &m, 1) == 1 ? len : -EIO;
}


wrapping a wrapper API.


> If the client would have a type of DVB demodulator or would be in the id
> range of DVB demodulators the adapter also could always attach and
> register it as a frontend, even if it does not know this specific one.
> Configuration of specific frontend subtypes could still be handled by
> SET_TYPE calls or even config table upload calls.
>
> You could also add your own secret GET_TYPE calls where the frontend 
> tells the adapter what kind it is if you really do not want one 
> id per frontend for those binary-only drivers.
I really don't get the point _why_ you want to use this API if it was 
not able to suit your needs (even if they're meesing around and trying 
to 'improve' it since years now).

Holger