[Q] i2c_new_device vs i2c_del_driver

[Q] i2c_new_device vs i2c_del_driver

[Guennadi Liakhovetski]

Hi,

I have a question regarding dynimically created i2c devices. If I have a 
card with an i2c device on it. Once I load a driver for that card, it uses 
its knowledge of the hardware and registers a new i2c device using 
i2c_new_device. Then at some point a driver for that i2c device is loaded, 
it calls i2c_add_driver, then its probe() method is called with the 
dynamically created device. So far so good. Now I unload the i2c driver, 
it calls i2c_del_driver(), and then, IIUC, __detach_adapter will be called 
for all devices attached to this driver, including our device from above, 
then i2c_unregister_device will be called for it, a comment to which says

"reverse effect of i2c_new_device()"

So, our newly created device is gone, and loading the i2c driver again 
will not find it any more?... This doesn't seem to be the case, so, I 
think, the comment is wrong and has to be fixed.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [Q] i2c_new_device vs i2c_del_driver

[Jean Delvare]

Hi Guennadi,

On Tue, 17 Mar 2009 18:24:49 +0100 (CET), Guennadi Liakhovetski wrote:
> I have a question regarding dynimically created i2c devices. If I have a 
> card with an i2c device on it. Once I load a driver for that card, it uses 
> its knowledge of the hardware and registers a new i2c device using 
> i2c_new_device. Then at some point a driver for that i2c device is loaded, 
> it calls i2c_add_driver, then its probe() method is called with the 
> dynamically created device. So far so good. Now I unload the i2c driver, 
> it calls i2c_del_driver(), and then, IIUC, __detach_adapter will be called 
> for all devices attached to this driver,

Actually,  __detach_adapter will be called for all *adapters* on the
system. Then in turn __detach_adapter does a number of actions on
devices present on said adapters.

> including our device from above, 

No. Your driver is a new-style one (otherwise you couldn't use
i2c_new_device) which means that __detach_adapter returns relatively
quickly. The only part of __detach_adapter which is run is the one
which removes auto-instantiated devices. This isn't the case of your
device (you instantiated it explicitly) so __detach_adapter is a no-op
for you.

> then i2c_unregister_device will be called for it, a comment to which says
> 
> "reverse effect of i2c_new_device()"
> 
> So, our newly created device is gone, and loading the i2c driver again 
> will not find it any more?... This doesn't seem to be the case, so, I 
> think, the comment is wrong and has to be fixed.

The comment is correct. What isn't is your understanding of the
(admittedly complex and often confusing) i2c-core code. But I hope to
solve it soon, by killing the legacy binding model altogether.

In the meantime, if you have more questions, I will be pleased to
answer them.

-- 
Jean Delvare

Re: [Q] i2c_new_device vs i2c_del_driver

[Guennadi Liakhovetski]

On Tue, 17 Mar 2009, Jean Delvare wrote:

> Hi Guennadi,
> 
> On Tue, 17 Mar 2009 18:24:49 +0100 (CET), Guennadi Liakhovetski wrote:
> > I have a question regarding dynimically created i2c devices. If I have a 
> > card with an i2c device on it. Once I load a driver for that card, it uses 
> > its knowledge of the hardware and registers a new i2c device using 
> > i2c_new_device. Then at some point a driver for that i2c device is loaded, 
> > it calls i2c_add_driver, then its probe() method is called with the 
> > dynamically created device. So far so good. Now I unload the i2c driver, 
> > it calls i2c_del_driver(), and then, IIUC, __detach_adapter will be called 
> > for all devices attached to this driver,
> 
> Actually,  __detach_adapter will be called for all *adapters* on the
> system. Then in turn __detach_adapter does a number of actions on
> devices present on said adapters.
> 
> > including our device from above, 
> 
> No. Your driver is a new-style one (otherwise you couldn't use
> i2c_new_device) which means that __detach_adapter returns relatively
> quickly. The only part of __detach_adapter which is run is the one
> which removes auto-instantiated devices. This isn't the case of your
> device (you instantiated it explicitly) so __detach_adapter is a no-op
> for you.

Aha, so, the driver->clients list will not contain my device, and it won't 
get unregistered. Ic.

> > then i2c_unregister_device will be called for it, a comment to which says
> > 
> > "reverse effect of i2c_new_device()"
> > 
> > So, our newly created device is gone, and loading the i2c driver again 
> > will not find it any more?... This doesn't seem to be the case, so, I 
> > think, the comment is wrong and has to be fixed.
> 
> The comment is correct. What isn't is your understanding of the
> (admittedly complex and often confusing) i2c-core code. But I hope to
> solve it soon, by killing the legacy binding model altogether.
> 
> In the meantime, if you have more questions, I will be pleased to
> answer them.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [Q] i2c_new_device vs i2c_del_driver

[Jean Delvare]

On Tue, 17 Mar 2009 18:50:50 +0100 (CET), Guennadi Liakhovetski wrote:
> On Tue, 17 Mar 2009, Jean Delvare wrote:
> 
> > Hi Guennadi,
> > 
> > On Tue, 17 Mar 2009 18:24:49 +0100 (CET), Guennadi Liakhovetski wrote:
> > > I have a question regarding dynimically created i2c devices. If I have a 
> > > card with an i2c device on it. Once I load a driver for that card, it uses 
> > > its knowledge of the hardware and registers a new i2c device using 
> > > i2c_new_device. Then at some point a driver for that i2c device is loaded, 
> > > it calls i2c_add_driver, then its probe() method is called with the 
> > > dynamically created device. So far so good. Now I unload the i2c driver, 
> > > it calls i2c_del_driver(), and then, IIUC, __detach_adapter will be called 
> > > for all devices attached to this driver,
> > 
> > Actually,  __detach_adapter will be called for all *adapters* on the
> > system. Then in turn __detach_adapter does a number of actions on
> > devices present on said adapters.
> > 
> > > including our device from above, 
> > 
> > No. Your driver is a new-style one (otherwise you couldn't use
> > i2c_new_device) which means that __detach_adapter returns relatively
> > quickly. The only part of __detach_adapter which is run is the one
> > which removes auto-instantiated devices. This isn't the case of your
> > device (you instantiated it explicitly) so __detach_adapter is a no-op
> > for you.
> 
> Aha, so, the driver->clients list will not contain my device, and it won't 
> get unregistered. Ic.

Correct. Maybe I should have found a better name, like
driver->detected_clients, to make it clearer. Want me to change it
know? It should be straightforward.

-- 
Jean Delvare

Re: [Q] i2c_new_device vs i2c_del_driver

[Guennadi Liakhovetski]

On Tue, 17 Mar 2009, Jean Delvare wrote:

> On Tue, 17 Mar 2009 18:50:50 +0100 (CET), Guennadi Liakhovetski wrote:
> > 
> > Aha, so, the driver->clients list will not contain my device, and it won't 
> > get unregistered. Ic.
> 
> Correct. Maybe I should have found a better name, like
> driver->detected_clients, to make it clearer. Want me to change it
> know? It should be straightforward.

Well, now I know the difference already, but I am not sure this would have 
helped me before you've explained it:-) So, don't bother because of me. 
Maybe adding a couple of words to Documentation/i2c/writing-clients 
explaining the different function i2c_del_driver() performs on different 
devices. Or a comment in the source - even better.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [Q] i2c_new_device vs i2c_del_driver

[Jean Delvare]

Hi Guennadi,

On Tue, 17 Mar 2009 19:29:07 +0100 (CET), Guennadi Liakhovetski wrote:
> On Tue, 17 Mar 2009, Jean Delvare wrote:
> 
> > On Tue, 17 Mar 2009 18:50:50 +0100 (CET), Guennadi Liakhovetski wrote:
> > > 
> > > Aha, so, the driver->clients list will not contain my device, and it won't 
> > > get unregistered. Ic.
> > 
> > Correct. Maybe I should have found a better name, like
> > driver->detected_clients, to make it clearer. Want me to change it
> > know? It should be straightforward.
> 
> Well, now I know the difference already, but I am not sure this would have 
> helped me before you've explained it:-) So, don't bother because of me. 
> Maybe adding a couple of words to Documentation/i2c/writing-clients 
> explaining the different function i2c_del_driver() performs on different 
> devices.

Well, writing-clients is really only about writing driver code, I don't
think it is the right document to describe the i2c-core internals nor
the device lifetime.

I have recently written a new document explaining how devices can be
instantiated:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-how-to-instantiate-devices.patch
This document explains the conditions of destruction of the devices
depending on how they were created. I hope this answers your question.
If not, the document can of course be clarified.

> Or a comment in the source - even better.

For the piece of code which confused you, yes, I agree a short comment
would help. What about:

---
 drivers/i2c/i2c-core.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- linux-2.6.29-rc8.orig/drivers/i2c/i2c-core.c	2009-03-20 09:45:43.000000000 +0100
+++ linux-2.6.29-rc8/drivers/i2c/i2c-core.c	2009-03-20 17:21:29.000000000 +0100
@@ -581,7 +581,8 @@ static int i2c_do_del_adapter(struct dev
 	struct i2c_client *client, *_n;
 	int res;
 
-	/* Remove the devices we created ourselves */
+	/* Remove the devices we created ourselves as the result of hardware
+	 * probing (using a driver's detect method) */
 	list_for_each_entry_safe(client, _n, &driver->clients, detected) {
 		if (client->adapter == adapter) {
 			dev_dbg(&adapter->dev, "Removing %s at 0x%x\n",
@@ -749,6 +750,8 @@ static int __detach_adapter(struct devic
 	struct i2c_driver *driver = data;
 	struct i2c_client *client, *_n;
 
+	/* Remove the devices we created ourselves as the result of hardware
+	 * probing (using a driver's detect method) */
 	list_for_each_entry_safe(client, _n, &driver->clients, detected) {
 		dev_dbg(&adapter->dev, "Removing %s at 0x%x\n",
 			client->name, client->addr);


-- 
Jean Delvare