[i2c] Feedback on new-style i2c drivers

[David Brownell]

On Wednesday 21 March 2007 5:05 pm, Hans Verkuil wrote:
> Hi David,
> 
> My v4l-dvb tree with the changes I had to make is available here:
> 
> http://linuxtv.org/hg/~hverkuil/ivtv-i2c
> 
> It might be useful to see what I had to change in order to implement 
> this.

A few times I looked and it didn't respond ... finally got in.


> > > 2* Right now, the i2c-core explicitly forbibs to mix legacy and
> > > old-style driver callback functions. What is the rationale behind
> > > this?
> >
> > (I presume you mean legacy and new-style ... legacy == old-style, so
> > they always "mix"!)
> >
> > It's clear how to do new-style drivers; it's also clear how to do
> > legacy drivers.  But it's not at all clear how to do drivers that
> > attempt to mix the two models.  The memory management policy is
> > very different (incompatible!), as are interactions with i2c-core.
> 
> I'm not sure why this would be such a big deal. If you look at the 
> changes to e.g. saa7115 then the old-style attach function now simply 
> allocates the i2c_client and then calls the new probe function (which 
> used to be part of attach). Same with detach/remove. It was quite 
> simple, really.

Hmm, looks like Mercurial really has no clue about how to format code;
the web interface was unusable.  (At least from Konqueror, which I was
using at the time.)  It didn't show leading tabs or spaces sanely, so
of course program code came out unreadable ...

That said:  it seems your drivers were already handing around pointers
to i2c_client structures.  The conversions I'm thinking of are drivers
that have to change from

	struct my_driver_data {
		...
		struct i2c_client	client;
		...
	};

to something else, which is just a lot of changes to a core memory
management model.  You're probably right that having two different
drivers, where the legacy driver is a wrapper around the new-style
one, simplifies things.



> > > My understanding is that we need to differentiate between legacy
> > > and new-style clients, but do we need to differentiate between
> > > drivers?
> >
> > Legacy drivers create and free the clients themselves;
> > new-style drivers use clients created and freed by other code.
> >
> > Legacy drivers use i2c-only models to connect devices and adapters;
> > new-style drivers use models that apply everywhere.
> >
> > I believe that mixing the two models inside one driver would be
> > confusing, and hence error prone.  Plus it would make it harder
> > to eventually remove the legacy model.  On the plus side, it would
> > lend new legitimacy to the religion of the flying spaghetti-code
> > monster, and such proofs always lead to new converts!  ;)
> 
> Again, see the way it is done in the i2c drivers I converted, it really 
> was no problem having both old and new style in one driver.

But, that's using *two* different drivers.   Sharing common code, yes.
The "saa7115" driver is different from "sa7115_legacy" ...


> > > Each v4l adapter needs a number of different drivers, which are
> > > often shared with other adapter types,
> >
> > I don't quite follow that comment ... V4L boards B1 and B2 use
> > adapters A1, A2, A3, but share drivers D4, D5, D6, so they'd all
> > need to update together?
> >
> > If so, that's a bit awkward, but no more so than for example the
> > AT91 platform updates I sent.  More like the OMAP ones I didn't
> > yet refresh.  It's just an issue of needing to update more code;
> > but it can still be done one driver at a time.  (Adapters first,
> > at least where numbered adapters are needed.  That shouldn't be
> > an issue with V4L.)
> 
> The problem is that if you want to convert a i2c driver like saa7115 to 
> the new style (without keeping the old style API), then you will also 
> need to update all the v4l PCI/USB drivers that use that i2c driver. 
> Unfortunately each PCI/USB driver is maintained by different people, 
> and especially for older drivers this conversion isn't easy.
> 
> In practice you want to be able to convert each PCI/USB driver in turn, 
> which means that for a certain i2c driver you will have a period where 
> some drivers call it new-style and some call it old-style. Anything 
> else will lead to a maintenance nightmare in my opinion.

I can understand that, yes.  I was looking from a slightly different
perspective:  converting an entire embedded arch to "new style".  That's
significantly more manageable, even in cases like OMAP (where there's
a boatload of stuff that can't yet go upstream).

 
> > > so it is impossible to convert each
> > > driver individually, and it's not realistic to hope to convert all
> > > the drivers at once. So the upgrade path isn't clear.
> >
> > It's probably simplest to update one driver and its users at a time.
> >
> > I guess I don't see what would be "impossible" there.  All the
> > relevant code is in-tree after all -- right?
> 
> Again, the new-style requires that you know on which i2c addresses 
> devices are. For some v4l drivers you have the information, but for 
> others you don't. In the latter case the conversion is much harder.

OK ... but I saw Jean's patch help out in that area.  In general,
I agree that we won't get completely away from the need to poke the
hardware to see what responds.  I'm content to let other folk chase
those issues.  (At least, I will be unless/until I run into such a
need myself!)

 
> > > What I proposed to Hans is to define two i2c_driver structures in
> > > every device driver, one implementing the legacy model, one
> > > implementing the new-style model, and each adapter driver uses the
> > > one it needs. While it appears to work, it is probably not ideal as
> > > far as memory footprint and performance are concerned. Would it be
> > > possible to allow one driver to support both styles?
> >
> > The memory foot print cost would be parallel probe and remove code,
> > an extra i2c_client structure (for legacy driver modes), and
> > different driver structs.  But the legacy stuff could then just be
> > removed later; the core of the driver could be shared.
> >
> > The delta between that and a "one driver, both styles" case would be
> > just the driver struct ... and whatever extra complexity goes into
> > the i2c core to support this model mixing.  That doesn't seem very
> > worthwhile to me.
> 
> Personally I do not have a problem with having two driver structures.

As a transitional model, I don't; and from what you've said, that sounds
like it'll be a useful strategy.  Long term, I think we'd rather just get
rid of all "legacy" driver structs.  (Maybe starting in 2.6.25 or so we
will be wrapping up that removal.  Just a guesstimate.)
 

> > > 3* Hans pointed out a lack of documentation about i2c_new_device()
> > > and i2c_unregister_device(). For example it's not obvious when
> > > i2c_unregister_device() needs to be called.
> >
> > I don't see the issue with i2c_new_device() ...
> 
> Just a simple code snippet on how to use it would be sufficient.

Feel free to update the kerneldoc.  Not, of course, that the kerneldoc
gets collected yet into a PDF anywhere ... ;)


> > As for i2c_unregister_device(), it's exported mostly for
> > completeness; I'm not sure any normal driver would need to call it,
> > since it's called automatically when the parent adapter is
> > unregistered and since I2C doesn't really hotplug.
> 
> Well, it is used in ivtv: you try to add a particular i2c device and if 
> it is not found, then the newly created i2c device has to be 
> unregistered again. There are several cards that can be differentiated 
> only by the i2c devices that they have, so there is no alternative I 
> think.
> 
> This is the code snippet from ivtv-i2c.c:
> 
>         client = i2c_new_device(&itv->i2c_adap, &info);
>         if (client->driver == NULL) {
>                 i2c_unregister_device(client);
>                 client = NULL;
>         }

Fine by me!  I'm glad that I made a point of that "completeness";
without that, I think you'd have had problems ... ;)

 
> > > The good news is that, these minor implementation details left
> > > apart, Hans thinks that this new i2c model will work fine for v4l
> > > drivers :)
> >
> > Glad to hear it!  So -- this stuff goes into 2.6.22 early-ish then?
> 
> I'm all for it: it solves a truck-load of problems for me at least.

And is now in MM, so at least build issues will get resolved, and it'l
be easy to roll conversion patches into 2.6.22.git in the merge window.

Glad to know this helps more than just the specific embedded issues I
was looking at.  DVB is a slightly different kind of problem, so I'm
not surprised to see you turn up a few things that need more tweaking.

- Dave

 
> Regards,
> 
> 	Hans
>