Hi Kay, Sorry for the late reply...
On Mon, 4 May 2009 14:40:36 +0200, Kay Sievers wrote: > On Mon, May 4, 2009 at 12:43, Jean Delvare <[email protected]> wrote: > > We don't need to give adapters a parent if they don't have one. The > > driver core will put them in the virtual device directory and all will > > be fine. > > > > Signed-off-by: Jean Delvare <[email protected]> > > --- > > Much better than my first attempt. Not sure what will happen if we ever > > turn i2c-adapters into bus devices. I see that the driver core puts > > class devices without a parent in virtual, but what about _bus_ devices > > I'm sure, that "virtual" logic could be done for bus devices too, if > it's needed. I don't see any reason why that wouldn't be fine. > > (The less difference between classes and buses the better. It is wrong > to have two types of subsystems, doing almost the same thing. One > could argue that it could be useful inside the kernel, which it isn't, > I think, but exporting them to userspace was definitely the wrong > thing.) I finally took a stab at this. The resulting patch is below. I have used device_type to differentiate between I2C clients and I2C adapters. Is this what you had in mind? It seems to work reasonably well, with the following issues remaining: * The change breaks at least sensors-detect and libsensors. I can easily modify them so that they work again, but we still have a compatibility issue. Is it possible to have a compatibility option that would add symbolic links from class/i2c-adapter/i2c-* to bus/i2c/devices/i2c-* for a couple years? * Now that i2c-core makes use of device_type, I tried to move the power management handling callbacks there from bus_type, to save a test in each function, however I found that the callback set is different between bus_type and device_type.pm. Why is it so? Is there a document explaining the difference? Is the whole world (including bus_type) eventually moving to dev_pm_ops? * When i2c-adapters were class devices, virtual ones (for example i2c-stub) appeared in sysfs as devices/virtual/i2c-adapter/i2c-*, which made sense and seemed safe. Now that I have turned them into bus devices, virtual ones appear in sysfs as devices/i2c-* directly, which looks dirty and could result in collisions someday. What should be done about this? I wanted to use virtual_device_parent() but it is internal to the driver core at the moment, and doesn't even exist if CONFIG_SYSFS_DEPRECATED=y. I would be grateful if you can advise on any of the above points. Thanks. * * * * * * Introduce two device types for i2c: i2c_client_type and i2c_adapter_type. * Remove i2c-adapter class and use i2c_adapter_type instead. * Many callbacks had to be modified to check whether the device that is passed has the correct type, now that clients and adapters live in the same namespace. --- drivers/i2c/i2c-core.c | 140 ++++++++++++++++++++++++++++++------------------- 1 file changed, 86 insertions(+), 54 deletions(-) --- linux-2.6.31-rc1.orig/drivers/i2c/i2c-core.c 2009-07-04 18:33:04.000000000 +0200 +++ linux-2.6.31-rc1/drivers/i2c/i2c-core.c 2009-07-04 19:06:04.000000000 +0200 @@ -46,6 +46,7 @@ static DEFINE_MUTEX(core_lock); static DEFINE_IDR(i2c_adapter_idr); static LIST_HEAD(userspace_devices); +static struct device_type i2c_client_type; static int i2c_check_addr(struct i2c_adapter *adapter, int addr); static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver); @@ -64,9 +65,13 @@ static const struct i2c_device_id *i2c_m static int i2c_device_match(struct device *dev, struct device_driver *drv) { - struct i2c_client *client = to_i2c_client(dev); - struct i2c_driver *driver = to_i2c_driver(drv); + struct i2c_client *client = i2c_verify_client(dev); + struct i2c_driver *driver; + + if (!client) + return 0; + driver = to_i2c_driver(drv); /* match on an id table if there is one */ if (driver->id_table) return i2c_match_id(driver->id_table, client) != NULL; @@ -94,10 +99,14 @@ static int i2c_device_uevent(struct devi static int i2c_device_probe(struct device *dev) { - struct i2c_client *client = to_i2c_client(dev); - struct i2c_driver *driver = to_i2c_driver(dev->driver); + struct i2c_client *client = i2c_verify_client(dev); + struct i2c_driver *driver; int status; + if (!client) + return 0; + + driver = to_i2c_driver(dev->driver); if (!driver->probe || !driver->id_table) return -ENODEV; client->driver = driver; @@ -114,11 +123,11 @@ static int i2c_device_probe(struct devic static int i2c_device_remove(struct device *dev) { - struct i2c_client *client = to_i2c_client(dev); + struct i2c_client *client = i2c_verify_client(dev); struct i2c_driver *driver; int status; - if (!dev->driver) + if (!client || !dev->driver) return 0; driver = to_i2c_driver(dev->driver); @@ -136,37 +145,40 @@ static int i2c_device_remove(struct devi static void i2c_device_shutdown(struct device *dev) { + struct i2c_client *client = i2c_verify_client(dev); struct i2c_driver *driver; - if (!dev->driver) + if (!client || !dev->driver) return; driver = to_i2c_driver(dev->driver); if (driver->shutdown) - driver->shutdown(to_i2c_client(dev)); + driver->shutdown(client); } static int i2c_device_suspend(struct device *dev, pm_message_t mesg) { + struct i2c_client *client = i2c_verify_client(dev); struct i2c_driver *driver; - if (!dev->driver) + if (!client || !dev->driver) return 0; driver = to_i2c_driver(dev->driver); if (!driver->suspend) return 0; - return driver->suspend(to_i2c_client(dev), mesg); + return driver->suspend(client, mesg); } static int i2c_device_resume(struct device *dev) { + struct i2c_client *client = i2c_verify_client(dev); struct i2c_driver *driver; - if (!dev->driver) + if (!client || !dev->driver) return 0; driver = to_i2c_driver(dev->driver); if (!driver->resume) return 0; - return driver->resume(to_i2c_client(dev)); + return driver->resume(client); } static void i2c_client_dev_release(struct device *dev) @@ -175,10 +187,10 @@ static void i2c_client_dev_release(struc } static ssize_t -show_client_name(struct device *dev, struct device_attribute *attr, char *buf) +show_name(struct device *dev, struct device_attribute *attr, char *buf) { - struct i2c_client *client = to_i2c_client(dev); - return sprintf(buf, "%s\n", client->name); + return sprintf(buf, "%s\n", dev->type == &i2c_client_type ? + to_i2c_client(dev)->name : to_i2c_adapter(dev)->name); } static ssize_t @@ -188,18 +200,28 @@ show_modalias(struct device *dev, struct return sprintf(buf, "%s%s\n", I2C_MODULE_PREFIX, client->name); } -static struct device_attribute i2c_dev_attrs[] = { - __ATTR(name, S_IRUGO, show_client_name, NULL), +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL); +static DEVICE_ATTR(modalias, S_IRUGO, show_modalias, NULL); + +static struct attribute *i2c_dev_attrs[] = { + &dev_attr_name.attr, /* modalias helps coldplug: modprobe $(cat .../modalias) */ - __ATTR(modalias, S_IRUGO, show_modalias, NULL), - { }, + &dev_attr_modalias.attr, + NULL +}; + +static struct attribute_group i2c_dev_attr_group = { + .attrs = i2c_dev_attrs, +}; + +static struct attribute_group *i2c_dev_attr_groups[] = { + &i2c_dev_attr_group, + NULL }; struct bus_type i2c_bus_type = { .name = "i2c", - .dev_attrs = i2c_dev_attrs, .match = i2c_device_match, - .uevent = i2c_device_uevent, .probe = i2c_device_probe, .remove = i2c_device_remove, .shutdown = i2c_device_shutdown, @@ -208,6 +230,12 @@ struct bus_type i2c_bus_type = { }; EXPORT_SYMBOL_GPL(i2c_bus_type); +static struct device_type i2c_client_type = { + .groups = i2c_dev_attr_groups, + .uevent = i2c_device_uevent, + .release = i2c_client_dev_release, +}; + /** * i2c_verify_client - return parameter as i2c_client, or NULL @@ -220,7 +248,7 @@ EXPORT_SYMBOL_GPL(i2c_bus_type); */ struct i2c_client *i2c_verify_client(struct device *dev) { - return (dev->bus == &i2c_bus_type) + return (dev->type == &i2c_client_type) ? to_i2c_client(dev) : NULL; } @@ -273,7 +301,7 @@ i2c_new_device(struct i2c_adapter *adap, client->dev.parent = &client->adapter->dev; client->dev.bus = &i2c_bus_type; - client->dev.release = i2c_client_dev_release; + client->dev.type = &i2c_client_type; dev_set_name(&client->dev, "%d-%04x", i2c_adapter_id(adap), client->addr); @@ -368,13 +396,6 @@ static void i2c_adapter_dev_release(stru complete(&adap->dev_released); } -static ssize_t -show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf) -{ - struct i2c_adapter *adap = to_i2c_adapter(dev); - return sprintf(buf, "%s\n", adap->name); -} - /* * Let users instantiate I2C devices through sysfs. This can be used when * platform initialization code doesn't contain the proper data for @@ -493,17 +514,28 @@ i2c_sysfs_delete_device(struct device *d return res; } -static struct device_attribute i2c_adapter_attrs[] = { - __ATTR(name, S_IRUGO, show_adapter_name, NULL), - __ATTR(new_device, S_IWUSR, NULL, i2c_sysfs_new_device), - __ATTR(delete_device, S_IWUSR, NULL, i2c_sysfs_delete_device), - { }, +static DEVICE_ATTR(new_device, S_IWUSR, NULL, i2c_sysfs_new_device); +static DEVICE_ATTR(delete_device, S_IWUSR, NULL, i2c_sysfs_delete_device); + +static struct attribute *i2c_adapter_attrs[] = { + &dev_attr_name.attr, + &dev_attr_new_device.attr, + &dev_attr_delete_device.attr, + NULL }; -static struct class i2c_adapter_class = { - .owner = THIS_MODULE, - .name = "i2c-adapter", - .dev_attrs = i2c_adapter_attrs, +static struct attribute_group i2c_adapter_attr_group = { + .attrs = i2c_adapter_attrs, +}; + +static struct attribute_group *i2c_adapter_attr_groups[] = { + &i2c_adapter_attr_group, + NULL +}; + +static struct device_type i2c_adapter_type = { + .groups = i2c_adapter_attr_groups, + .release = i2c_adapter_dev_release, }; static void i2c_scan_static_board_info(struct i2c_adapter *adapter) @@ -555,8 +587,8 @@ static int i2c_register_adapter(struct i adap->timeout = HZ; dev_set_name(&adap->dev, "i2c-%d", adap->nr); - adap->dev.release = &i2c_adapter_dev_release; - adap->dev.class = &i2c_adapter_class; + adap->dev.bus = &i2c_bus_type; + adap->dev.type = &i2c_adapter_type; res = device_register(&adap->dev); if (res) goto out_list; @@ -768,9 +800,13 @@ EXPORT_SYMBOL(i2c_del_adapter); static int __attach_adapter(struct device *dev, void *data) { - struct i2c_adapter *adapter = to_i2c_adapter(dev); + struct i2c_adapter *adapter; struct i2c_driver *driver = data; + if (dev->type != &i2c_adapter_type) + return 0; + adapter = to_i2c_adapter(dev); + i2c_detect(adapter, driver); /* Legacy drivers scan i2c busses directly */ @@ -809,8 +845,7 @@ int i2c_register_driver(struct module *o INIT_LIST_HEAD(&driver->clients); /* Walk the adapters that are already present */ mutex_lock(&core_lock); - class_for_each_device(&i2c_adapter_class, NULL, driver, - __attach_adapter); + bus_for_each_dev(&i2c_bus_type, NULL, driver, __attach_adapter); mutex_unlock(&core_lock); return 0; @@ -819,10 +854,14 @@ EXPORT_SYMBOL(i2c_register_driver); static int __detach_adapter(struct device *dev, void *data) { - struct i2c_adapter *adapter = to_i2c_adapter(dev); + struct i2c_adapter *adapter; struct i2c_driver *driver = data; struct i2c_client *client, *_n; + if (dev->type != &i2c_adapter_type) + return 0; + adapter = to_i2c_adapter(dev); + /* 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) { @@ -850,8 +889,7 @@ static int __detach_adapter(struct devic void i2c_del_driver(struct i2c_driver *driver) { mutex_lock(&core_lock); - class_for_each_device(&i2c_adapter_class, NULL, driver, - __detach_adapter); + bus_for_each_dev(&i2c_bus_type, NULL, driver, __detach_adapter); mutex_unlock(&core_lock); driver_unregister(&driver->driver); @@ -940,16 +978,11 @@ static int __init i2c_init(void) retval = bus_register(&i2c_bus_type); if (retval) return retval; - retval = class_register(&i2c_adapter_class); - if (retval) - goto bus_err; retval = i2c_add_driver(&dummy_driver); if (retval) - goto class_err; + goto bus_err; return 0; -class_err: - class_unregister(&i2c_adapter_class); bus_err: bus_unregister(&i2c_bus_type); return retval; @@ -958,7 +991,6 @@ bus_err: static void __exit i2c_exit(void) { i2c_del_driver(&dummy_driver); - class_unregister(&i2c_adapter_class); bus_unregister(&i2c_bus_type); } -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
