Hi Nathan, > Virtually every i2c client driver uses exactly the same code, so > there's little point in requiring them all to implement this callback. > The first patch in this series adds two new fields to the i2c_driver > structure, `address_data' and `detect_client', and if they are set by > the driver, the i2c core will automatically call i2c_probe using those > fields as the second and third argument. This sounds very good to me. I have been wanting to do that some times ago, but for some reason didn't. Most probably I was waiting to be done with the various other i2c-core cleanups that were needed before (and this just happened.) > If the `class' field of the i2c_driver structure is set, it will be > compared with the adapter class first. I'm not sure I agree with the "if" clause, more about that below. > Patches 2 and 3 add these fields to the i2c_driver initializer in the > i2c hwmon and misc i2c chip drivers and remove the corresponding > attach_adapter callbacks. Looks good to me too. > The second improvement (which is really the point of this patch set) > is to add the functions i2c_probe_device and i2c_remove_device for > directly creating and destroying i2c clients on a particular adapter: > > int i2c_probe_device(struct i2c_adapter *adapter, int driver_id, > int addr, int kind); > int i2c_remove_device(struct i2c_adapter *adapter, int driver_id, > int addr); > > These functions make the i2c subsystem usable for special-purpose i2c > buses where probing isn't possible, either because probing is known to > be dangerous for devices that are present on the bus, or because the > i2c adapter lacks quick writes and/or error reporting. > > The final patch adds a new i2c adapter flag to indicate that the > adapter should never be probed. I think I understand the point of i2c_probe_device(). However, it would help if you could additionally show how this is going to help the media/video drivers. Currently, all these drivers use the traditional probing mecanism, and have to jam "foreign" probes, right? I would hope that these two patches will make it possible to improve this. Can you provide a few examples of use? We need to figure out how good this new interface/mechanism is, and this can only be done with concrete examples. I do not understand the point of i2c_remove_device() though. You shouldn't have to search for the clients you have been registering explicitely before. Instead, you should keep a reference to them (which suggests that maybe i2c_remove_device should return an i2c_client* instead of an int.) With that reference, you should be able to unregister the client directly, which is much more efficient that walking all the list of clients for that adapter. I am although a bit surprised that you are passing the adapter as an i2c_adapter* and the driver as an id. Why not passing the driver as a i2c_driver*? Or both the adapter and the driver as ids? This is a truly na?ve question, I just don't know. This is where concrete examples really help. I am not totally convinced by the reintroduction of the i2c_adapter flags. I hope we can do without it. One possibility would be to have an additional class of client, say I2C_CLASS_MISC. This would cover all the chip drivers which do not have a well-defined class, so that every client would *have to* define a class (we could enforce that at core level - I think this was the planned ultimate goal of .class when it was first introduced.) Adapters that do not want to be probed would then simply *not* define a class, so the class test would always fail for them, and no client could attach through the "automatic" probe mechanism. If I2C_CLASS_MISC doesn't please you, then we can simply keep the idea that i2c_adapters that do not want to be probed do not define a class. This alone could be a condition to skip any probing to that adapters, we just have to make an explicit test on i2c_adapter.id not being 0 before going on with generic probing. Another possibility, in case you really don't like neither the first one nor its variant, would be to keep two separate lists of i2c adapters, one with those which want probing, and one with those which don't. As it seems that these are two very different types of adapters, on which we will use completely different probing functions, this seems to make sense, and should speed up the processing on both sides. I admit I don't know if doing so would have implications at the i2c-core level. Maybe Greg can comment on this idea? I have additional comments on the code itself, which I will post as replies to each patch. Thanks, -- Jean Delvare
