[i2c] [patch 2.6.20-rc1 5/6] remove i2c_adapter.dev from i2c core and adapters
Jean Delvare
khali at linux-fr.org
Wed Dec 20 22:57:49 CET 2006
Hi David,
On Mon, 18 Dec 2006 13:21:21 -0800, David Brownell wrote:
> Some I2C driver model core cleanup, removing:
>
> - Inappropriate "i2c_adapter.dev"; users of that should have used
> (or, for adapters, provided) i2c_adapter.class_dev.dev instead.
>
> - The non-class "i2c-adapter" driver, which should never have been
> needed given the i2c_adapter class.
>
> And corresponding changes to all I2C adapter drivers:
>
> - LOTS of one-line updates to adapter init: don't set the parent of
> a no-longer-existing device, set i2c_adapter.class_dev.dev instead.
>
> - The "i2c-isa" driver needed somewhat larger changes, since it was
> directly mucking about with the stuff that was removed.
>
> This shrinks the data footprint for each I2C adapter by a fair amount,
> and i2c core code (and data) space by a bit. Later, the core can shrink
> more, removing lists and pointers duplicating more driver model functions.
>
> To cope with incomplete driver model conversions in I2C bus drivers, if
> there's no real device associated with the adapter, a warning is issued
> and a "legacy_i2c_adapter" platform device is created. One hopes all
> the I2C bus drivers will be fixed over the next year or so, letting that
> hack be removed.
I'm not happy with this. We're in the process of cleaning the i2c-core,
and now we're adding yet another temporary workaround. When we later
remove this workaround, we'll once again modify the i2c_adapter
structure, and the sysfs representation will change as well. I'd rather
fix the offending drivers right now.
In a number of cases, the missing device is not declared, but it's
there. That's the case for all PCI and USB devices, mostly in
media/video drivers. I have a patch to fix these, attached.
For the rest, these are legacy drivers, I suppose we would need to
convert them to platform drivers. That would be a significant amount of
work... But after investigating a bit, I found that registering a class
device without an associated device should work. Other drivers do that
already (fbcon or lp for example.) So I suggest that we don't implement
that legacy_i2c_adapter hack at all.
>
> Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
>
> ---
> Updated to match feedback from Jean re previous patches.
>
> drivers/i2c/busses/i2c-ali1535.c | 4 -
> drivers/i2c/busses/i2c-ali1563.c | 2
> drivers/i2c/busses/i2c-ali15x3.c | 4 -
> drivers/i2c/busses/i2c-amd756.c | 4 -
> drivers/i2c/busses/i2c-amd8111.c | 4 -
> drivers/i2c/busses/i2c-at91.c | 2
> drivers/i2c/busses/i2c-hydra.c | 2
> drivers/i2c/busses/i2c-i801.c | 4 -
> drivers/i2c/busses/i2c-i810.c | 6 -
> drivers/i2c/busses/i2c-ibm_iic.c | 1
> drivers/i2c/busses/i2c-iop3xx.c | 2
> drivers/i2c/busses/i2c-isa.c | 53 ++++++----------
> drivers/i2c/busses/i2c-ixp2000.c | 2
> drivers/i2c/busses/i2c-ixp4xx.c | 2
> drivers/i2c/busses/i2c-mpc.c | 2
> drivers/i2c/busses/i2c-mv64xxx.c | 1
> drivers/i2c/busses/i2c-nforce2.c | 2
> drivers/i2c/busses/i2c-ocores.c | 2
> drivers/i2c/busses/i2c-omap.c | 2
> drivers/i2c/busses/i2c-piix4.c | 4 -
> drivers/i2c/busses/i2c-pnx.c | 2
> drivers/i2c/busses/i2c-powermac.c | 2
> drivers/i2c/busses/i2c-prosavage.c | 2
> drivers/i2c/busses/i2c-pxa.c | 2
> drivers/i2c/busses/i2c-s3c2410.c | 2
> drivers/i2c/busses/i2c-savage4.c | 4 -
> drivers/i2c/busses/i2c-sis5595.c | 4 -
> drivers/i2c/busses/i2c-sis630.c | 4 -
> drivers/i2c/busses/i2c-sis96x.c | 4 -
> drivers/i2c/busses/i2c-versatile.c | 2
> drivers/i2c/busses/i2c-via.c | 4 -
> drivers/i2c/busses/i2c-viapro.c | 2
> drivers/i2c/busses/i2c-voodoo3.c | 6 -
> drivers/i2c/i2c-core.c | 94 ++++++++++++-----------------
> drivers/media/common/saa7146_i2c.c | 2
> drivers/media/dvb/pluto2/pluto2.c | 2
> drivers/media/video/bt8xx/bttv-i2c.c | 2
> drivers/media/video/cx88/cx88-i2c.c | 2
> drivers/media/video/cx88/cx88-vp3054-i2c.c | 2
> drivers/media/video/em28xx/em28xx-i2c.c | 2
> drivers/media/video/saa7134/saa7134-i2c.c | 2
> drivers/video/aty/radeon_i2c.c | 2
> drivers/video/i810/i810-i2c.c | 2
> drivers/video/intelfb/intelfb_i2c.c | 2
> drivers/video/nvidia/nv_i2c.c | 2
> drivers/video/riva/rivafb-i2c.c | 2
> drivers/video/savage/savagefb-i2c.c | 2
> include/linux/i2c.h | 9 +-
> 48 files changed, 124 insertions(+), 150 deletions(-)
>
> Index: g26/include/linux/i2c.h
> ===================================================================
> --- g26.orig/include/linux/i2c.h 2006-12-18 12:15:26.000000000 -0800
> +++ g26/include/linux/i2c.h 2006-12-18 13:05:40.000000000 -0800
> @@ -37,8 +37,6 @@
>
> /* --- For i2c-isa ---------------------------------------------------- */
>
> -extern void i2c_adapter_dev_release(struct device *dev);
> -extern struct device_driver i2c_adapter_driver;
> extern struct class i2c_adapter_class;
> extern struct bus_type i2c_bus_type;
>
> @@ -222,8 +220,9 @@ struct i2c_adapter {
>
> int timeout;
> int retries;
> - struct device dev; /* the adapter device */
> struct class_device class_dev; /* the class device */
> + struct platform_device *legacy_hack;
> + void *adapdata;
>
> int nr;
> struct list_head clients;
What about dev_released? Shouldn't it go away with dev?
> @@ -237,12 +236,12 @@ struct i2c_adapter {
>
> static inline void *i2c_get_adapdata (struct i2c_adapter *dev)
> {
> - return dev_get_drvdata (&dev->dev);
> + return dev->adapdata;
> }
>
> static inline void i2c_set_adapdata (struct i2c_adapter *dev, void *data)
> {
> - dev_set_drvdata (&dev->dev, data);
> + dev->adapdata = data;
> }
struct class_device has a class_data pointer, isn't it meant exactly
for this usage? If so, we don't need the new adapdata field.
>
> /*flags for the client struct: */
> Index: g26/drivers/i2c/i2c-core.c
> ===================================================================
> --- g26.orig/drivers/i2c/i2c-core.c 2006-12-18 12:48:50.000000000 -0800
> +++ g26/drivers/i2c/i2c-core.c 2006-12-18 13:08:25.000000000 -0800
> @@ -83,36 +83,29 @@ struct bus_type i2c_bus_type = {
> .resume = i2c_bus_resume,
> };
>
> -void i2c_adapter_dev_release(struct device *dev)
> -{
> - struct i2c_adapter *adap = dev_to_i2c_adapter(dev);
> - complete(&adap->dev_released);
> -}
> -
> -struct device_driver i2c_adapter_driver = {
> - .owner = THIS_MODULE,
> - .name = "i2c_adapter",
> - .bus = &i2c_bus_type,
> -};
> -
> static void i2c_adapter_class_dev_release(struct class_device *dev)
> {
> struct i2c_adapter *adap = class_dev_to_i2c_adapter(dev);
> complete(&adap->class_dev_released);
> }
>
> -struct class i2c_adapter_class = {
> - .owner = THIS_MODULE,
> - .name = "i2c-adapter",
> - .release = &i2c_adapter_class_dev_release,
> -};
> -
> -static ssize_t show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
> +static ssize_t show_adapter_name(struct class_device *cdev, char *buf)
> {
> - struct i2c_adapter *adap = dev_to_i2c_adapter(dev);
> + struct i2c_adapter *adap = class_dev_to_i2c_adapter(cdev);
> return sprintf(buf, "%s\n", adap->name);
> }
> -static DEVICE_ATTR(name, S_IRUGO, show_adapter_name, NULL);
> +
> +static struct class_device_attribute adapter_attrs[] = {
> + __ATTR(name, S_IRUGO, show_adapter_name, NULL),
> + { },
> +};
> +
> +struct class i2c_adapter_class = {
> + .name = "i2c-adapter",
> + .owner = THIS_MODULE,
> + .class_dev_attrs = adapter_attrs,
> + .release = &i2c_adapter_class_dev_release,
> +};
I see why this is necessary, but it changes the interface we offer to
user-space, and will break some tools and libraries. For example
libsensors uses this field to present the adapter name to the user, and
will need to be updated to get the class name attribute instead of the
device name attribute.
Unfortunately there's no way around it now. We should have anticipated
the change by creating the class attribute earlier, and by teaching
libsensors to read from it earlier. It's too late now... But we might
have to delay a bit the merging of this patch because of this :(
>
>
> static void i2c_client_release(struct device *dev)
> @@ -172,30 +165,34 @@ int i2c_add_adapter(struct i2c_adapter *
> list_add_tail(&adap->list,&adapters);
> INIT_LIST_HEAD(&adap->clients);
>
> - /* Add the adapter to the driver core.
> - * If the parent pointer is not set up,
> - * we add this adapter to the host bus.
> + /* Caller must have zero-initialized adap->class_dev, then
> + * initialized adap->class_dev.dev as the real device so we
> + * can properly add this adapter to the i2c_adapter class.
> + *
> + * As a **TEMPORARY migration aid** we create a platform
> + * device for drivers that don't yet use the driver model.
> + * Expect this to vanish by January 2008.
If we really do this, then this must be documented in
Documentation/feature-removal-schedule.txt, not just here. But as I
said above, I don't think we need to introduce these fake devices.
> */
> - if (adap->dev.parent == NULL)
> - adap->dev.parent = &platform_bus;
> - sprintf(adap->dev.bus_id, "i2c-%d", adap->nr);
> - adap->dev.driver = &i2c_adapter_driver;
> - adap->dev.release = &i2c_adapter_dev_release;
> - res = device_register(&adap->dev);
> - if (res)
> - goto out_list;
> - res = device_create_file(&adap->dev, &dev_attr_name);
> - if (res)
> - goto out_unregister;
> + if (!dev) {
> + printk(KERN_WARNING "I2C bus '%s' has no device; "
> + "convert it to the driver model\n",
> + adap->name);
> + adap->legacy_hack = platform_device_register_simple(
> + "legacy_i2c_adapter", adap->nr, NULL, 0);
> + if (adap->legacy_hack) {
> + dev = &adap->legacy_hack->dev;
> + adap->class_dev.dev = dev;
> + } else {
> + res = -ENODEV;
> + goto out_unlock;
> + }
> + }
>
> - /* Add this adapter to the i2c_adapter class */
> - memset(&adap->class_dev, 0x00, sizeof(struct class_device));
> - adap->class_dev.dev = &adap->dev;
> + sprintf(adap->class_dev.class_id, "i2c-%d", adap->nr);
> adap->class_dev.class = &i2c_adapter_class;
> - strlcpy(adap->class_dev.class_id, adap->dev.bus_id, BUS_ID_SIZE);
> res = class_device_register(&adap->class_dev);
> if (res)
> - goto out_remove_name;
> + goto out_list;
>
> dev_dbg(dev, "adapter %s [%s] registered\n",
> adap->class_dev.class_id, adap->name);
> @@ -212,14 +209,9 @@ out_unlock:
> mutex_unlock(&core_lists);
> return res;
>
> -out_remove_name:
> - device_remove_file(&adap->dev, &dev_attr_name);
> -out_unregister:
> - init_completion(&adap->dev_released); /* Needed? */
> - device_unregister(&adap->dev);
> - wait_for_completion(&adap->dev_released);
> out_list:
> list_del(&adap->list);
> + platform_device_put(adap->legacy_hack);
> idr_remove(&i2c_adapter_idr, adap->nr);
> goto out_unlock;
> }
> @@ -276,8 +268,6 @@ int i2c_del_adapter(struct i2c_adapter *
> init_completion(&adap->dev_released);
We should no longer need to touch dev_released in this function, right?
> init_completion(&adap->class_dev_released);
> class_device_unregister(&adap->class_dev);
> - device_remove_file(&adap->dev, &dev_attr_name);
> - device_unregister(&adap->dev);
> list_del(&adap->list);
>
> /* wait for sysfs to drop all references */
You forgot to unregister the fake platform device here, so we have a
leak. Not that it really matters, as I still think we don't want this
fake device...
> @@ -426,7 +416,7 @@ int i2c_attach_client(struct i2c_client
>
> client->usage_count = 0;
>
> - client->dev.parent = &client->adapter->dev;
> + client->dev.parent = adapter->class_dev.dev;
> client->dev.driver = &client->driver->driver;
> client->dev.bus = &i2c_bus_type;
> client->dev.release = &i2c_client_release;
> @@ -575,16 +565,12 @@ static int __init i2c_init(void)
> retval = bus_register(&i2c_bus_type);
> if (retval)
> return retval;
> - retval = driver_register(&i2c_adapter_driver);
> - if (retval)
> - return retval;
> return class_register(&i2c_adapter_class);
> }
>
> static void __exit i2c_exit(void)
> {
> class_unregister(&i2c_adapter_class);
> - driver_unregister(&i2c_adapter_driver);
> bus_unregister(&i2c_bus_type);
> }
>
> @@ -1177,8 +1163,6 @@ s32 i2c_smbus_xfer(struct i2c_adapter *
>
>
> /* Next four are needed by i2c-isa */
> -EXPORT_SYMBOL_GPL(i2c_adapter_dev_release);
> -EXPORT_SYMBOL_GPL(i2c_adapter_driver);
> EXPORT_SYMBOL_GPL(i2c_adapter_class);
> EXPORT_SYMBOL_GPL(i2c_bus_type);
s/four/two/ so that the comment matches the code.
>
> Index: g26/drivers/i2c/busses/i2c-isa.c
> ===================================================================
> --- g26.orig/drivers/i2c/busses/i2c-isa.c 2006-12-18 13:04:39.000000000 -0800
> +++ g26/drivers/i2c/busses/i2c-isa.c 2006-12-18 13:05:40.000000000 -0800
> @@ -56,21 +56,24 @@ static struct i2c_adapter isa_adapter =
> .name = "ISA main adapter",
> };
>
> -/* We can't do a thing... */
> -static u32 isa_func(struct i2c_adapter *adapter)
> +static void i2c_isa_dev_release(struct device *dev)
> {
> - return 0;
> + complete(&isa_adapter.dev_released);
So this is where you use i2c_adapter.dev_released... Given that this is
the only place where you will still use it, and i2c-isa is eventually
going away, wouldn't it be better to make it local to the i2c-isa
driver?
BTW, it's still a mystery to me what these operations we do on
dev_released are for, if you could explain that to me...
> }
>
> +static struct platform_device i2c_isa_dev = {
> + .name = "i2c_isa",
> + .id = -1,
> + .dev = {
> + .release = i2c_isa_dev_release,
> + },
> +};
>
> -/* Copied from i2c-core */
> -static ssize_t show_adapter_name(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +/* We can't do a thing... */
> +static u32 isa_func(struct i2c_adapter *adapter)
> {
> - struct i2c_adapter *adap = dev_to_i2c_adapter(dev);
> - return sprintf(buf, "%s\n", adap->name);
> + return 0;
> }
> -static DEVICE_ATTR(name, S_IRUGO, show_adapter_name, NULL);
The diff is much larger than it should, I can't really figure out why
"diff" can't make it better :(
>
>
> /* We implement an interface which resembles i2c_{add,del}_driver,
> @@ -137,44 +140,33 @@ static int __init i2c_isa_init(void)
>
> mutex_init(&isa_adapter.clist_lock);
> INIT_LIST_HEAD(&isa_adapter.clients);
> + init_completion(&isa_adapter.dev_released);
> + init_completion(&isa_adapter.class_dev_released);
Now I'm confused. Here we initialize them at the beginning, while in
i2c-core we initialize them right before deleting the i2c_adapter?
>
> isa_adapter.nr = ANY_I2C_ISA_BUS;
> - isa_adapter.dev.parent = &platform_bus;
> - sprintf(isa_adapter.dev.bus_id, "i2c-%d", isa_adapter.nr);
> - isa_adapter.dev.driver = &i2c_adapter_driver;
> - isa_adapter.dev.release = &i2c_adapter_dev_release;
> - err = device_register(&isa_adapter.dev);
> + sprintf(isa_adapter.class_dev.class_id, "i2c-%d", isa_adapter.nr);
> +
> + err = platform_device_register(&i2c_isa_dev);
> if (err) {
> printk(KERN_ERR "i2c-isa: Failed to register device\n");
> goto exit;
> }
> - err = device_create_file(&isa_adapter.dev, &dev_attr_name);
> - if (err) {
> - printk(KERN_ERR "i2c-isa: Failed to create name file\n");
> - goto exit_unregister;
> - }
>
> /* Add this adapter to the i2c_adapter class */
> - memset(&isa_adapter.class_dev, 0x00, sizeof(struct class_device));
> - isa_adapter.class_dev.dev = &isa_adapter.dev;
> + isa_adapter.class_dev.dev = &i2c_isa_dev.dev;
> isa_adapter.class_dev.class = &i2c_adapter_class;
> - strlcpy(isa_adapter.class_dev.class_id, isa_adapter.dev.bus_id,
> - BUS_ID_SIZE);
> err = class_device_register(&isa_adapter.class_dev);
> if (err) {
> printk(KERN_ERR "i2c-isa: Failed to register class device\n");
> - goto exit_remove_name;
> + goto exit_unregister;
> }
>
> dev_dbg(isa_adapter.class_dev.dev, "%s registered\n", isa_adapter.name);
>
> return 0;
>
> -exit_remove_name:
> - device_remove_file(&isa_adapter.dev, &dev_attr_name);
> exit_unregister:
> - init_completion(&isa_adapter.dev_released); /* Needed? */
> - device_unregister(&isa_adapter.dev);
> + platform_device_unregister(&i2c_isa_dev);
> wait_for_completion(&isa_adapter.dev_released);
> exit:
> return err;
> @@ -203,11 +195,8 @@ static void __exit i2c_isa_exit(void)
>
> /* Clean up the sysfs representation */
> dev_dbg(isa_adapter.class_dev.dev, "Unregistering from sysfs\n");
> - init_completion(&isa_adapter.dev_released);
> - init_completion(&isa_adapter.class_dev_released);
> class_device_unregister(&isa_adapter.class_dev);
> - device_remove_file(&isa_adapter.dev, &dev_attr_name);
> - device_unregister(&isa_adapter.dev);
> + platform_device_unregister(&i2c_isa_dev);
>
> /* Wait for sysfs to drop all references */
> dev_dbg(isa_adapter.class_dev.dev, "Waiting for sysfs completion\n");
I start wondering if we shouldn't kill i2c-isa right now so that we
don't have to modify it (and break user-space compatibility...)
Thanks,
--
Jean Delvare
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: i2c-missing-i2c-adapter-devices.patch
Url: http://lists.lm-sensors.org/pipermail/i2c/attachments/20061220/43767d46/attachment.pl
More information about the i2c
mailing list