[i2c] Feedback on new-style i2c drivers

[David Brownell]

On Monday 19 March 2007 6:05 am, Jean Delvare wrote:
> Hi David,
> 
> Hans Verkuil has been trying to convert some v4l drivers to the new i2c
> infrastructure yesterday, and provided useful feedback. I'd like to
> discuss it, then we can see how to round the rough edges.
> 
> 1* i2c_client.driver_name is currently a pointer, while
> i2c_board_info.driver_name is an array of chars. In v4l drivers,
> typically the i2c_board_info structure will be on the stack,

That's surprising; why?

> this 
> causes i2c_new_device() to create an i2c_client those driver_name
> points to a string on the stack... no good.
> 
> It would seem to make sense to either make i2c_client.driver_name and
> i2c_board_info.driver_name both pointers, or both arrays of chars, but
> mixing both is not convenient. What do you think?

Both as arrays, I guess.  We only eliminated that 32+ bytes of
space in i2c_client so we could use it for something else, right?
Adding 20 bytes for the driver name is still a net shrink.  :)


> 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.


> 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!  ;)


> 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.)


> 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?


> 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.

 
> 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() ... 

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.

 
> 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?

- Dave