Hi Trent,
On Fri, 16 Jan 2009 05:34:59 -0800 (PST), Trent Piepho wrote:
> On Fri, 16 Jan 2009, Jean Delvare wrote:
> > Hi Mauro, Trent,
> > On Fri, 16 Jan 2009 00:02:52 -0200, Mauro Carvalho Chehab wrote:
> > > For now, we should finish the Hans approach, since it also helps to stop
> > > using
> > > the legacy i2c methods. After having all drivers using it, we can do some
> > > cleanup at the drivers.
>
> How will this work for drivers like bttv, where the i2c address of the
> tuner chips isn't know for every supported card?
Is this a problem in practice? My understanding was that I2C gates were
relatively recent in the history of V4L devices, so I assumed that for
devices with an I2C gate we would know where the devices are.
> > (...)
> > Well, the scanning order problem is IMHO better resolved by
> > instantiating the devices explicitly instead of letting the i2c
> > subsystem probe for them. Everything is already in place to do this,
> > and a number of drivers are already being converted in this direction.
>
> The problem is that then one needs to know where the i2c devices are, and
> this is not known for all cards. When the bttv driver was converted to use
> the new i2c subsystem a decade ago and used probing as the only way to find
> devices, I said it was a bad idea. But now we have this database of
> cards with no information about what chips and what address.
I understand the problem. This is why even the new-style (standard) i2c
device driver binding still offers two flavors of device detection, to
handle the older devices.
> > > I'm not sure if we should implement it inside v4l core, or at i2c-core.
> >
> > This is an excellent question, I don't know for sure myself.
>
> Why does either need support?
>
> static int cx88_gate_i2c_xfer(struct i2c_adapter *i2c_adap,
> struct i2c_msg *msgs, int num)
> {
> struct cx88_gate_i2c *gate= i2c_get_adapdata(i2c_adap);
> int ret;
>
> cx88_dvb_gate_ctrl(gate->core, 1);
> ret = i2c_transfer(&gate->core->i2c_adap, msgs, num);
> cx88_dvb_gate_ctrl(gate->core, 0);
> return ret;
> }
>
> ...
> static struct i2c_algorithm cx88_gate = {
> .master_xfer = cx88_gate_i2c_xfer,
> ...
> };
> static struct i2c_adapter adap = {
> .algo = &cx88_gate,
> ...
> };
> struct cx88_gate_i2c *gate = kzalloc(sizeof(typeof(*gate)), GFP_KERNEL);
> gate->core = core;
> i2c_set_adapdata(&adap, gate);
> i2c_add_adapter(&adap);
>
> What part needs to go somewhere else?
This is indeed a possible implementation. One could argue though that
it's a bit overkill to instantiate a separate i2c_adapter just for a
gate. There are also caveats you must pay attention to. Two things in
particular that come to my mind:
* You must set the adapter depth properly, otherwise lockdep will
complain.
* Your proposal above, in its simple form, is incompatible with I2C
device detection, because devices which are before the gate would be
double-detected (once on each segment.)
The first point is very easy to handle, the second is a little more
complicated. Basically you should add an address check at the beginning
of cx88_gate_i2c_xfer() to reject all transfers except to the device
you know is behind the gate.
At which point it is no longer clear why you want to have 2
i2c_adapters. You can have just one which opens (and closes) the gate
automatically for the right I2C address and not for the other addresses.
> > of 2 other:
> >
> > * At I2C bus driver level. We can have a pre-transfer handler and a
> > post-transfer handler, which does what needs to be done (like opening
> > and closing a gate) based on the address of the device that is being
> > accessed. I had discussed this approach with Michael Krufky long ago.
>
> This won't work when muxes are used to put multiple i2c devices with the
> same address behind a single master.
Sorry for not being clear, I was only trying to address the gate issue
here, not the (more complex) multiplexing issue. I am not aware of V4L
devices having real I2C muxes?
> It still has the problem of probe order when one wants to scan for chips.
> The i2c core does scanning when a new adapter is created. But, suppose the
> mux is an i2c device on the adapter? In order for the scanner to find
> anything behind the mux, it needs to be detected and working before the bus
> is scanned. But this may not happen. Say one calls:
> i2c_add_adapter(adap); i2c_new_device(adap, &the_mux);
>
> If the client driver for the device behind the mux uses probing, and is
> already loaded when i2c_add_adapter() is called, then it will be probed for
> before the i2c_new_device() call and won't be found because the mux isn't
> working.
You are right, this is a problem.
> If instead the mux driver created a new i2c adapter for the segment(s)
> behind it, which would happen when i2c_new_device() is called, then those
> segments would be probed at the correct time.
This doesn't fully solve the problem, because you have no idea in which
state the mux is initially. If one of its segments is already selected,
then the detection will happen when it shouldn't (the chip drivers
thinks it has found a device on the trunk) and things will break as
soon as the mux driver is in action (the device behind the mux will
"move" to its actual branch.)
This is the reason why muxes must be handled specifically by i2c-core,
and also the reason why this still didn't happen: this is a non-trivial
thing. My impression is that mux chips should be somehow declared as
part of the i2c_adapter, rather than as regular I2C chips, and they
should be initialized _before_ the i2c_adapter is made public.
> > * At the chip driver level, using a custom API. I think this is what is
> > done right now? You lose transparency, but performance may be better:
> > if you have many commands to send to a device behind the gate, you can
> > open the gate, batch them, then close the gate. With the transparent
> > models, the gate is opened and closed again for every I2C transfer. I
> > don't know if this is a big problem in practice, as presumably this
> > only happens once at initialization time?
>
> No, the devices behind the gate are usually used throughout device
> operation. Typically there is only one transfer at a time and so nothing
> to batch together.
>
> But this isn't insolvable. For a real mux, the driver could only switch
> the mux when the selected segment is different from the last one. For a
> gate, one could reset a timer on each transfer that will close the gate
> when it gets triggered. That way when multiple commands arrive close
> enough together the gate can be left open for all of them. This has an
> advantage over manually batching commands in that in many cases the code
> that sends one command does not know that another command will be sent
> immediately afterwards and so they are not batched.
But this has the disadvantage to leave the gate opened for longer than
it really has to, which could have adverse consequences on video
quality. Anyway, I'm leaving this up to you video/media people, as I
don't know enough myself about this to make a sane decision.
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
