[i2c] Feedback on new-style i2c drivers

[David Brownell]

On Thursday 22 March 2007 1:58 am, Jean Delvare wrote:
> Hi Hans, David,
> 
> On Thu, 22 Mar 2007 01:05:49 +0100, Hans Verkuil wrote:
> > On Wednesday 21 March 2007 22:20, David Brownell wrote:
> > > 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?
> > 
> > I'm not sure whether this will be typical, but it was the easiest way to 
> > implement it in ivtv, which is how I noticed this pointer problem.
> 
> I expect it to be typical. When using i2c_register_board_info(), the
> i2c_board_info data is allocated by i2c-core and will stay available
> pretty much forever, because the bus the information relates to can be
> (re-)added at any time. But for all the other use cases such as the TV
> adapters, struct i2c_board_info is merely a transport shell. You fill
> it, give it to i2c_new_device(), then you have no reason to keep it
> around - you don't use it ever again. So the stack is the natural
> place, I guess.

OK, I'm persuaded.  This issue has been resolved in the current versions
of those patches.  Perhaps either of you (Jean, Hans) could update the
kerneldoc for that function to capture this information?


> Hopefully we can continue shrinking struct i2c_client with later
> cleanups.

Let's hope.  ;)



> > 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.
> 
> Let's not mix two things here. Having support for both the old style
> and the new style in the same C file is not a problem. This is what you
> did, and it works. What David objects to is having support for both
> style in the same i2c_driver. And I tend to agree - ultimately we want
> to get rid of the legacy model, and while I think that we _could_
> support both styles with the same i2c_driver, it would make it harder
> achieve that goal.

So:  support both in one "module", no problem -- we all agree.
But: support both in one "i2c_driver" -- big problem, we don't want it.

And as a transition strategy, it's OK to have both a legacy driver and
a new-style driver in the same module/c-file ... but we want the legacy
stuff to go away.




> > This is the code snippet from ivtv-i2c.c:
> > 
> >         client = i2c_new_device(&itv->i2c_adap, &info);
> >         if (client->driver == NULL) {

Nitpick:  if (client != NULL && client->driver == NULL)

> >                 i2c_unregister_device(client);
> >                 client = NULL;
> >         }
> 
> That construct is a bit surprising. Can you explain the situation in
> greater details? I wonder if there is a cleaner way to achieve the same.

My interpretation:  the new-style driver it might use was already
registered, so we know it will be probed.  But the probe() won't
succeed if the device isn't actually there -- it checks -- so that
code snippet guarantees that client is non-null only iff the relevant
driver found the right chip at that address.


> Here is the documentation update I have come up with, comments welcome:
> 
> ---
>  Documentation/i2c/writing-clients |   35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> --- linux-2.6.21-rc4.orig/Documentation/i2c/writing-clients	2007-03-22 08:51:41.000000000 +0100
> +++ linux-2.6.21-rc4/Documentation/i2c/writing-clients	2007-03-22 09:54:01.000000000 +0100
> @@ -200,6 +200,41 @@ device typing support in the hardware.  
>  match, so hotplug/coldplug mechanisms will modprobe the driver.
>  
>  
> +Device Creation (Standard driver model)
> +---------------------------------------
> +
> +If you know for a fact that an I2C device is connected to a given I2C bus,
> +you can instantiate that device by simply filling an i2c_board_info
> +structure with the device address and driver name, and calling

And platform data if needed, etc ...

> +i2c_new_device().  This will create the device, then the driver core will
> +take care of finding the right driver and will call its probe() method.
> +If a driver supports different device types, you can specify the type you
> +want using the type field.
> +
> +Sometimes you know that a device is connected to a given I2C bus, but you
> +don't know the exact address it uses.  This happens on TV adapters for
> +example, where the same driver supports dozens of slightly different
> +models, and I2C device addresses change from one model to the next.  In
> +that case, you can use the i2c_new_probed_device() variant, which is
> +similar to i2c_new_device(), except that it takes an additional list of
> +possible I2C addresses to probe.  A device is created for the first
> +responsive address in the list.

Minor issue there if there are multiple such devices ... 

> +
> +In both cases, the I2C device creation typically happens in the I2C bus
> +driver. You may want to save the returned i2c_client reference for later
> +use.

Device creation happens in the I2C core ... not the bus adapter/driver.

> +
> +
> +Device Deletion (Standard driver model)
> +---------------------------------------
> +
> +Each I2C device which has been created using i2c_new_device() or
> +i2c_new_probed_device() can be unregistered by calling
> +i2c_unregister_device().  If you don't call it explicitly, it will be
> +called automatically before the underlying I2C bus itself is removed, as a
> +device can't survive its parent in the device driver model.
> +
> +
>  Legacy Driver Binding Model
>  ---------------------------

That seems OK, other than not being in kerneldoc format.  ;)

- Dave


> > > > 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.
> 
> Yes.
> 
> -- 
> Jean Delvare
>