[PATCH] i2c: Warn when adapters have no parent

[PATCH] i2c: Warn when adapters have no parent

[Jean Delvare]

Warn more loudly when an i2c-adapter is registered without a parent.
There should be almost no driver left in this case, and it's about
time to fix these.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/i2c-core.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- linux-2.6.30-rc3.orig/drivers/i2c/i2c-core.c	2009-04-25 08:46:12.000000000 +0200
+++ linux-2.6.30-rc3/drivers/i2c/i2c-core.c	2009-04-26 10:10:40.000000000 +0200
@@ -457,8 +457,9 @@ static int i2c_register_adapter(struct i
 	 */
 	if (adap->dev.parent == NULL) {
 		adap->dev.parent = &platform_bus;
-		pr_debug("I2C adapter driver [%s] forgot to specify "
-			 "physical device\n", adap->name);
+		pr_warning("I2C adapter driver [%s] forgot to specify "
+			   "physical device\n", adap->name);
+		pr_warning("Fix it now, it will soon break!\n");
 	}
 
 	/* Set default timeout to 1 second if not already set */


-- 
Jean Delvare

[PATCH] i2c: Do not give adapters a default parent

[Jean Delvare]

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 <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
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
without a parent? Kay?

 drivers/i2c/i2c-core.c |   11 -----------
 1 file changed, 11 deletions(-)

--- linux-2.6.30-rc4.orig/drivers/i2c/i2c-core.c	2009-05-03 09:08:51.000000000 +0200
+++ linux-2.6.30-rc4/drivers/i2c/i2c-core.c	2009-05-04 11:19:59.000000000 +0200
@@ -29,7 +29,6 @@
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/idr.h>
-#include <linux/platform_device.h>
 #include <linux/mutex.h>
 #include <linux/completion.h>
 #include <linux/hardirq.h>
@@ -451,16 +450,6 @@ static int i2c_register_adapter(struct i
 
 	mutex_lock(&core_lock);
 
-	/* Add the adapter to the driver core.
-	 * If the parent pointer is not set up,
-	 * we add this adapter to the host bus.
-	 */
-	if (adap->dev.parent == NULL) {
-		adap->dev.parent = &platform_bus;
-		pr_debug("I2C adapter driver [%s] forgot to specify "
-			 "physical device\n", adap->name);
-	}
-
 	/* Set default timeout to 1 second if not already set */
 	if (adap->timeout == 0)
 		adap->timeout = HZ;

-- 
Jean Delvare

Re: [PATCH] i2c: Do not give adapters a default parent

[Kay Sievers]

On Mon, May 4, 2009 at 12:43, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> 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 <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
> 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.)

Thanks,
Kay

Re: [PATCH] i2c: Do not give adapters a default parent

[Jean Delvare]

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 <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> 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 <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > ---
> > 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

Re: [PATCH] i2c: Do not give adapters a default parent

[Kay Sievers]

On Sat, Jul 4, 2009 at 19:14, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, 4 May 2009 14:40:36 +0200, Kay Sievers wrote:

>> (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?

Looks fine, by just looking at the patch.

> 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?

Yeah, we can add that. I guess others will need that too, if we
convert things from class to bus. How would that look like? Like a
device_add_class_compat_link(*dev, *class)?

> * 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?

I think this is already removed in the current git tree, and all
should use dev_pm_ops, yes.

> * 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.

Yeah, we just need to apply the /sys/devices/virtual logic to bus
devices too, it's currently limited to class devices, because there
was no bus device so far who needed this, but should be an easy
change.

> I would be grateful if you can advise on any of the above points.

If you decide to do it that way, you would need the driver core to be able:
  - to create a link from an otherwise empty "struct class" to an
existing bus-device
  - put bus devices without a parent into the /sys/devices/virtual logic
right? Let me know, I can look into that, if you need that.

Thanks,
Kay

Re: [PATCH] i2c: Do not give adapters a default parent

[Jean Delvare]

Hi Kay,

On Sun, 5 Jul 2009 22:19:46 +0200, Kay Sievers wrote:
> On Sat, Jul 4, 2009 at 19:14, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Mon, 4 May 2009 14:40:36 +0200, Kay Sievers wrote:
> 
> >> (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?
> 
> Looks fine, by just looking at the patch.
> 
> > 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?
> 
> Yeah, we can add that. I guess others will need that too, if we
> convert things from class to bus. How would that look like? Like a
> device_add_class_compat_link(*dev, *class)?

Yes. I'm not just sure what "class" would be exactly... either a "real"
fake class, or a mere string representing the fake class name?

> > * 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?
> 
> I think this is already removed in the current git tree, and all
> should use dev_pm_ops, yes.

In which git tree? In Linus' tree, struct bus_type definitely doesn't
use dev_pm_ops yet.

> > * 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.
> 
> Yeah, we just need to apply the /sys/devices/virtual logic to bus
> devices too, it's currently limited to class devices, because there
> was no bus device so far who needed this, but should be an easy
> change.

It will probably have do be a little different, as it is valid to
register a device without a parent, to have it appear at the root
(actually 1st level) of the device tree. So we'll need a way to
differentiate between this case and the virtual device case.

I admit I am a little surprised that I am the first person asking for
virtual bus devices, especially given how you like to repeat that i2c
was doing things differently from the rest of the world so far and I am
merely changing i2c to fit in the common model.

> > I would be grateful if you can advise on any of the above points.
> 
> If you decide to do it that way, you would need the driver core to be able:
>   - to create a link from an otherwise empty "struct class" to an
> existing bus-device
>   - put bus devices without a parent into the /sys/devices/virtual logic
> right? Let me know, I can look into that, if you need that.

Yes, this is a good summary of my needs. With some room for discussion
on both points:
* Do we need an actually struct class for each fake class, or just a
  class name?
* Do we want to put virtual devices in devices/virtual directly, or do
  we want separate namespaces?
But these are details which can be solved on the way, and I have no
strong opinion about them anyway.

Thanks,
-- 
Jean Delvare

Re: [PATCH] i2c: Do not give adapters a default parent

[Kay Sievers]

On Sun, Jul 5, 2009 at 22:56, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Sun, 5 Jul 2009 22:19:46 +0200, Kay Sievers wrote:

>> > * 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?
>>
>> I think this is already removed in the current git tree, and all
>> should use dev_pm_ops, yes.
>
> In which git tree? In Linus' tree, struct bus_type definitely doesn't
> use dev_pm_ops yet.

But it removed all the other stuff:
  commit 00725787511e20dbd1fdc1fb233606120ae5c8cf
  Author: Magnus Damm <damm-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
  Date:   Thu Jun 4 22:13:25 2009 +0200

  PM: Remove device_type suspend()/resume()

>> > * 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.
>>
>> Yeah, we just need to apply the /sys/devices/virtual logic to bus
>> devices too, it's currently limited to class devices, because there
>> was no bus device so far who needed this, but should be an easy
>> change.
>
> It will probably have do be a little different, as it is valid to
> register a device without a parent, to have it appear at the root
> (actually 1st level) of the device tree. So we'll need a way to
> differentiate between this case and the virtual device case.
>
> I admit I am a little surprised that I am the first person asking for
> virtual bus devices, especially given how you like to repeat that i2c
> was doing things differently from the rest of the world so far and I am
> merely changing i2c to fit in the common model.

Usually bus devices are a child of root device like pci or some
platform stuff. If i2c does not have any parents like this, and is by
itself a root for other stuff, maybe we get /sys/devices/i2c, like we
have for pci, acpi, ccw (IBM s390), and all these device roots.

>> > I would be grateful if you can advise on any of the above points.
>>
>> If you decide to do it that way, you would need the driver core to be able:
>>   - to create a link from an otherwise empty "struct class" to an
>> existing bus-device
>>   - put bus devices without a parent into the /sys/devices/virtual logic
>> right? Let me know, I can look into that, if you need that.
>
> Yes, this is a good summary of my needs. With some room for discussion
> on both points:
> * Do we need an actually struct class for each fake class, or just a
>  class name?

We will need to create a kobject for the compat class directory, we
will not need a "struct class" for it and can just use a simple
pointer to the registered kobject. If we use a string, we would need
to find the registered kobject with every call to create a link there,
not necessarily bad, but an explicitely registered object might be
easier.

> * Do we want to put virtual devices in devices/virtual directly, or do
>  we want separate namespaces?
> But these are details which can be solved on the way, and I have no
> strong opinion about them anyway.

It's basically the question if these objects usually represent real
hardware or not. If the root i2c device just happens to have no other
existing bus as a parent, we might just want:  /sys/devices/i2c/.

Thanks,
Kay

Re: [PATCH] i2c: Do not give adapters a default parent

[Jean Delvare]

Hi Kay,

On Sun, 5 Jul 2009 23:34:01 +0200, Kay Sievers wrote:
> On Sun, Jul 5, 2009 at 22:56, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Sun, 5 Jul 2009 22:19:46 +0200, Kay Sievers wrote:
> >> I think this is already removed in the current git tree, and all
> >> should use dev_pm_ops, yes.
> >
> > In which git tree? In Linus' tree, struct bus_type definitely doesn't
> > use dev_pm_ops yet.
> 
> But it removed all the other stuff:
>   commit 00725787511e20dbd1fdc1fb233606120ae5c8cf
>   Author: Magnus Damm <damm-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
>   Date:   Thu Jun 4 22:13:25 2009 +0200
> 
>   PM: Remove device_type suspend()/resume()

This is correct but only for device_type. For bus_type, I still can see
both suspend/resume and dev_pm_ops. And right now the i2c core is using
the suspend/resume flavor. So my question is: how do I convert from
suspend/resume to dev_pm_ops? I'm fairly sure I am not the first
developer who has to do this, so my hope was that there was either a
document explaining the process, or a commit doing this for another
subsystem, which I could simply stare at and copy from.

> >> Yeah, we just need to apply the /sys/devices/virtual logic to bus
> >> devices too, it's currently limited to class devices, because there
> >> was no bus device so far who needed this, but should be an easy
> >> change.
> >
> > It will probably have do be a little different, as it is valid to
> > register a device without a parent, to have it appear at the root
> > (actually 1st level) of the device tree. So we'll need a way to
> > differentiate between this case and the virtual device case.
> >
> > I admit I am a little surprised that I am the first person asking for
> > virtual bus devices, especially given how you like to repeat that i2c
> > was doing things differently from the rest of the world so far and I am
> > merely changing i2c to fit in the common model.
> 
> Usually bus devices are a child of root device like pci or some
> platform stuff. If i2c does not have any parents like this, and is by
> itself a root for other stuff, maybe we get /sys/devices/i2c, like we
> have for pci, acpi, ccw (IBM s390), and all these device roots.

For physical I2C controller devices, I do expect them to have a parent
(either platform or pci in most cases.) i2c-stub is different, it's a
software-only driver. So I do not expect that we have /sys/devices/i2c,
but I do expect us to have /sys/devices/virtual/i2c or some such.

> >> If you decide to do it that way, you would need the driver core to be able:
> >>   - to create a link from an otherwise empty "struct class" to an
> >> existing bus-device
> >>   - put bus devices without a parent into the /sys/devices/virtual logic
> >> right? Let me know, I can look into that, if you need that.
> >
> > Yes, this is a good summary of my needs. With some room for discussion
> > on both points:
> > * Do we need an actually struct class for each fake class, or just a
> >  class name?
> 
> We will need to create a kobject for the compat class directory, we
> will not need a "struct class" for it and can just use a simple
> pointer to the registered kobject. If we use a string, we would need
> to find the registered kobject with every call to create a link there,
> not necessarily bad, but an explicitely registered object might be
> easier.

Any progress on this? I have just committed the patches to
sensors-detect and libsensors, and the kernel patch is ready to go, but
without the compatibility links it doesn't make any sense to push it
upstream, not even in linux-next: lm-sensors would be broken for almost
all users.

> > * Do we want to put virtual devices in devices/virtual directly, or do
> >  we want separate namespaces?
> > But these are details which can be solved on the way, and I have no
> > strong opinion about them anyway.
> 
> It's basically the question if these objects usually represent real
> hardware or not. If the root i2c device just happens to have no other
> existing bus as a parent, we might just want:  /sys/devices/i2c/.

In the only case I am aware of at the moment, i2c-stub, no it doesn't
represent real hardware.

Note that I don't really care how we handle i2c-stub, BTW. It's really
only a developer tool so we don't have to care too much about backwards
compatibility etc.

-- 
Jean Delvare

Re: [PATCH] i2c: Do not give adapters a default parent

[Kay Sievers]

On Wed, 2009-07-22 at 21:07 +0200, Jean Delvare wrote:
> > > * Do we need an actually struct class for each fake class, or just a
> > >  class name?
> > 
> > We will need to create a kobject for the compat class directory, we
> > will not need a "struct class" for it and can just use a simple
> > pointer to the registered kobject. If we use a string, we would need
> > to find the registered kobject with every call to create a link there,
> > not necessarily bad, but an explicitely registered object might be
> > easier.
> 
> Any progress on this? I have just committed the patches to
> sensors-detect and libsensors, and the kernel patch is ready to go, but
> without the compatibility links it doesn't make any sense to push it
> upstream

Something like this? Please change it as you need. I did only a very
quick test.

The only important part is that the kobject of the class directly is not
exposed, so nobody else can do weird things with it.

Thanks,
Kay


--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -488,6 +488,45 @@ void class_interface_unregister(struct c
 	class_put(parent);
 }
 
+struct class_compat {
+	struct kobject *kobj;
+};
+
+struct class_compat *class_compat_register(const char *name)
+{
+	struct class_compat *cls;
+
+	cls = kmalloc(sizeof(struct class_compat), GFP_KERNEL);
+	if (!cls)
+		return NULL;
+	cls->kobj = kobject_create_and_add(name, &class_kset->kobj);
+	if (!cls->kobj) {
+		kfree(cls);
+		return NULL;
+	}
+	return cls;
+}
+EXPORT_SYMBOL_GPL(class_compat_register);
+
+void class_compat_unregister(struct class_compat *cls)
+{
+	kobject_put(cls->kobj);
+	kfree(cls);
+}
+EXPORT_SYMBOL_GPL(class_compat_unregister);
+
+int class_compat_create_link(struct class_compat *cls, struct device *dev)
+{
+	return sysfs_create_link(cls->kobj, &dev->kobj, dev_name(dev));
+}
+EXPORT_SYMBOL_GPL(class_compat_create_link);
+
+void class_compat_remove_link(struct class_compat *cls, struct device *dev)
+{
+	return sysfs_remove_link(cls->kobj, dev_name(dev));
+}
+EXPORT_SYMBOL_GPL(class_compat_remove_link);
+
 int __init classes_init(void)
 {
 	class_kset = kset_create_and_add("class", NULL, NULL);

--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -223,6 +223,12 @@ extern void class_unregister(struct clas
 	__class_register(class, &__key);	\
 })
 
+struct class_compat;
+struct class_compat *class_compat_register(const char *name);
+void class_compat_unregister(struct class_compat *cls);
+int class_compat_create_link(struct class_compat *cls, struct device *dev);
+void class_compat_remove_link(struct class_compat *cls, struct device *dev);
+
 extern void class_dev_iter_init(struct class_dev_iter *iter,
 				struct class *class,
 				struct device *start,

Re: [PATCH] i2c: Do not give adapters a default parent

[Jean Delvare]

Hi Kay,

On Wed, 22 Jul 2009 23:04:48 +0200, Kay Sievers wrote:
> On Wed, 2009-07-22 at 21:07 +0200, Jean Delvare wrote:
> > Any progress on this? I have just committed the patches to
> > sensors-detect and libsensors, and the kernel patch is ready to go, but
> > without the compatibility links it doesn't make any sense to push it
> > upstream
> 
> Something like this? Please change it as you need. I did only a very
> quick test.
> 
> The only important part is that the kobject of the class directly is not
> exposed, so nobody else can do weird things with it.
> (...)
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -488,6 +488,45 @@ void class_interface_unregister(struct c
>  	class_put(parent);
>  }
>  
> +struct class_compat {
> +	struct kobject *kobj;
> +};
> +
> +struct class_compat *class_compat_register(const char *name)
> +{
> +	struct class_compat *cls;
> +
> +	cls = kmalloc(sizeof(struct class_compat), GFP_KERNEL);
> +	if (!cls)
> +		return NULL;
> +	cls->kobj = kobject_create_and_add(name, &class_kset->kobj);
> +	if (!cls->kobj) {
> +		kfree(cls);
> +		return NULL;
> +	}
> +	return cls;
> +}
> +EXPORT_SYMBOL_GPL(class_compat_register);
> +
> +void class_compat_unregister(struct class_compat *cls)
> +{
> +	kobject_put(cls->kobj);
> +	kfree(cls);
> +}
> +EXPORT_SYMBOL_GPL(class_compat_unregister);
> +
> +int class_compat_create_link(struct class_compat *cls, struct device *dev)
> +{
> +	return sysfs_create_link(cls->kobj, &dev->kobj, dev_name(dev));
> +}
> +EXPORT_SYMBOL_GPL(class_compat_create_link);
> +
> +void class_compat_remove_link(struct class_compat *cls, struct device *dev)
> +{
> +	return sysfs_remove_link(cls->kobj, dev_name(dev));

I don't think you need the "return" here, as sysfs_remove_link returns
void.

> +}
> +EXPORT_SYMBOL_GPL(class_compat_remove_link);
> +
>  int __init classes_init(void)
>  {
>  	class_kset = kset_create_and_add("class", NULL, NULL);
> 
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -223,6 +223,12 @@ extern void class_unregister(struct clas
>  	__class_register(class, &__key);	\
>  })
>  
> +struct class_compat;
> +struct class_compat *class_compat_register(const char *name);
> +void class_compat_unregister(struct class_compat *cls);
> +int class_compat_create_link(struct class_compat *cls, struct device *dev);
> +void class_compat_remove_link(struct class_compat *cls, struct device *dev);
> +
>  extern void class_dev_iter_init(struct class_dev_iter *iter,
>  				struct class *class,
>  				struct device *start,

Other than that (and in practice even with that) your patch works just
fine for me. Thanks! Unfortunately it doesn't provide perfect
compatibility, because sensors-detect likes to query attributes of the
i2c-adapter's parent device (when it is a PCI device) and the lack of
device link (now that i2c adapters are bus devices) prevent it from
doing so. I will have to check how bad it is for older versions of the
script (when I am done moving to my new place, that is.) For the
current version, it prevents sensors-detect from being smart on default
answers, but that's about it, so it isn't a blocker IMHO. And I'm
not really sure if/how this could be addressed anyway. Would it be OK
to add this device link (pointing to "..") temporarily or would that be
too confusing?

libsensors only needs the adapter's name so the compatibility layer
works perfectly there.

Another thing we have to discuss is the compatibility option. For now
I've made it i2c-specific and enabled by default:

config I2C_COMPAT
	boolean "Enable compatibility bits for old user-space"
	default y
	help
	  Say Y here if you intend to run lm-sensors 3.1.1 or older.

But this means a lot of ifdefs in my code (6). With a system-wide
option, we could provide empty stubs I could get rid of them. OTOH, It
is easier to control the lifetime, and change the default value, of a
subsystem specific option. So I'm not too sure what do to. Maybe it
depends on how many subsystems will need the compatibility layer... Do
you have an opinion?

Thanks,
-- 
Jean Delvare

Re: [PATCH] i2c: Do not give adapters a default parent

[Kay Sievers]

On Thu, Jul 23, 2009 at 16:02, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Wed, 22 Jul 2009 23:04:48 +0200, Kay Sievers wrote:

>> +     return sysfs_remove_link(cls->kobj, dev_name(dev));
>
> I don't think you need the "return" here, as sysfs_remove_link returns
> void.

Fixed. Thanks.

> Other than that (and in practice even with that) your patch works just
> fine for me. Thanks! Unfortunately it doesn't provide perfect
> compatibility, [...] to add this device link (pointing to "..") temporarily
> or would that be too confusing?

I think that's ok, if it solves a real problem. The entire idea of _a_
"device" link is pretty flawed, and the reason we ripped all the
"struct class_device" devices out.

> Another thing we have to discuss is the compatibility option. For now
> I've made it i2c-specific and enabled by default:
>
> config I2C_COMPAT
>        boolean "Enable compatibility bits for old user-space"
>        default y
>        help
>          Say Y here if you intend to run lm-sensors 3.1.1 or older.
>
> But this means a lot of ifdefs in my code (6). With a system-wide
> option, we could provide empty stubs I could get rid of them. OTOH, It
> is easier to control the lifetime, and change the default value, of a
> subsystem specific option. So I'm not too sure what do to.

I'm not sure too. I think it's fine to have it per-subsytem, as only
the subsystem knows for how long it is needed, and it can probably be
dropped some day.

> Maybe it
> depends on how many subsystems will need the compatibility layer... Do
> you have an opinion?

I don't know of any other subsytem needing that, I've seen some just
moving things around without taking care about that. :)

Thanks,
Kay

Re: [PATCH] i2c: Do not give adapters a default parent

[Jean Delvare]

Hi Kay,

On Thu, 23 Jul 2009 17:19:22 +0200, Kay Sievers wrote:
> On Thu, Jul 23, 2009 at 16:02, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > I don't think you need the "return" here, as sysfs_remove_link returns
> > void.
> 
> Fixed. Thanks.

What's the merge timeline for this patch?

> > Other than that (and in practice even with that) your patch works just
> > fine for me. Thanks! Unfortunately it doesn't provide perfect
> > compatibility, [...] to add this device link (pointing to "..") temporarily
> > or would that be too confusing?
> 
> I think that's ok, if it solves a real problem. The entire idea of _a_
> "device" link is pretty flawed, and the reason we ripped all the
> "struct class_device" devices out.

OK, I've added the "device" link and now compatibility works perfectly.
I'll post the updated patch series soon, if you want to take a look.

Would it make sense to move the "device" link creation into
class_compat_create_link()? I suspect other users of a compatibility
class may need it as well.

> > Another thing we have to discuss is the compatibility option. For now
> > I've made it i2c-specific and enabled by default:
> >
> > config I2C_COMPAT
> >        boolean "Enable compatibility bits for old user-space"
> >        default y
> >        help
> >          Say Y here if you intend to run lm-sensors 3.1.1 or older.
> >
> > But this means a lot of ifdefs in my code (6). With a system-wide
> > option, we could provide empty stubs I could get rid of them. OTOH, It
> > is easier to control the lifetime, and change the default value, of a
> > subsystem specific option. So I'm not too sure what do to.
> 
> I'm not sure too. I think it's fine to have it per-subsytem, as only
> the subsystem knows for how long it is needed, and it can probably be
> dropped some day.

OK, fine with me.

-- 
Jean Delvare

Re: [PATCH] i2c: Do not give adapters a default parent

[Kay Sievers]

On Tue, Jul 28, 2009 at 08:47, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>
> What's the merge timeline for this patch?

I just did it in the moment you asked, there are no other users so
far. Just submit it with your changes that need it, if Greg is ok with
that?

>> > Other than that (and in practice even with that) your patch works just
>> > fine for me. Thanks! Unfortunately it doesn't provide perfect
>> > compatibility, [...] to add this device link (pointing to "..") temporarily
>> > or would that be too confusing?
>>
>> I think that's ok, if it solves a real problem. The entire idea of _a_
>> "device" link is pretty flawed, and the reason we ripped all the
>> "struct class_device" devices out.
>
> OK, I've added the "device" link and now compatibility works perfectly.
> I'll post the updated patch series soon, if you want to take a look.

Sounds great.

> Would it make sense to move the "device" link creation into
> class_compat_create_link()? I suspect other users of a compatibility
> class may need it as well.

Might make sense, as long as it's not the built-in default.

Sometimes we need to insert devices into the devpath, and then the
"device" link does not point to the direct parent, but the next one.
So it would need to take another device parameter, if we do that, and
also accept NULL, if no such link is really needed.

The "device" link itself is a pretty broken concept, and should be
avoided wherever possible, so it should be as optional as we can do
it. :)

Thanks,
Kay

Re: [PATCH] i2c: Do not give adapters a default parent

[Jean Delvare]

Hi Kay,

On Thu, 30 Jul 2009 11:12:05 -0400, Kay Sievers wrote:
> On Tue, Jul 28, 2009 at 08:47, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> >
> > What's the merge timeline for this patch?
> 
> I just did it in the moment you asked, there are no other users so
> far. Just submit it with your changes that need it, if Greg is ok with
> that?

OK, will do.

> >> > Other than that (and in practice even with that) your patch works just
> >> > fine for me. Thanks! Unfortunately it doesn't provide perfect
> >> > compatibility, [...] to add this device link (pointing to "..") temporarily
> >> > or would that be too confusing?
> >>
> >> I think that's ok, if it solves a real problem. The entire idea of _a_
> >> "device" link is pretty flawed, and the reason we ripped all the
> >> "struct class_device" devices out.
> >
> > OK, I've added the "device" link and now compatibility works perfectly.
> > I'll post the updated patch series soon, if you want to take a look.
> 
> Sounds great.
> 
> > Would it make sense to move the "device" link creation into
> > class_compat_create_link()? I suspect other users of a compatibility
> > class may need it as well.
> 
> Might make sense, as long as it's not the built-in default.
> 
> Sometimes we need to insert devices into the devpath, and then the
> "device" link does not point to the direct parent, but the next one.
> So it would need to take another device parameter, if we do that, and
> also accept NULL, if no such link is really needed.
> 
> The "device" link itself is a pretty broken concept, and should be
> avoided wherever possible, so it should be as optional as we can do
> it. :)

Yes, you are perfectly right. I have come up with the following
implementation, would that be OK with you? If it is, please add your
Signed-off-by.

From: Kay Sievers <kay.sievers-tD+1rO4QERM@public.gmane.org>
Subject: Add support for compatibility classes

When turning class devices into bus devices, we may need to
temporarily add links in sysfs so that user-space applications
are not confused. This is done by adding the following API:

* Functions to register and unregister compatibility classes.
  These appear in sysfs at the same location as regular classes, but
  instead of class devices, they contain links to bus devices.
* Functions to create and delete such links. Additionally, the caller
  can optionally pass a target device to which a "device" link should
  point (typically that would be the device's parent), to fully emulate
  the original class device.

The i2c subsystem will be the first user of this API, as i2c adapters
are being converted from class devices to bus devices.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/base/class.c   |   87 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |    8 ++++
 2 files changed, 95 insertions(+)

--- linux-2.6.31-rc4.orig/drivers/base/class.c	2009-07-30 22:21:31.000000000 +0200
+++ linux-2.6.31-rc4/drivers/base/class.c	2009-07-31 18:45:37.000000000 +0200
@@ -488,6 +488,93 @@ void class_interface_unregister(struct c
 	class_put(parent);
 }
 
+struct class_compat {
+	struct kobject *kobj;
+};
+
+/**
+ * class_compat_register - register a compatibility class
+ * @name: the name of the class
+ *
+ * Compatibility class are meant as a temporary user-space compatibility
+ * workaround when converting a family of class devices to a bus devices.
+ */
+struct class_compat *class_compat_register(const char *name)
+{
+	struct class_compat *cls;
+
+	cls = kmalloc(sizeof(struct class_compat), GFP_KERNEL);
+	if (!cls)
+		return NULL;
+	cls->kobj = kobject_create_and_add(name, &class_kset->kobj);
+	if (!cls->kobj) {
+		kfree(cls);
+		return NULL;
+	}
+	return cls;
+}
+EXPORT_SYMBOL_GPL(class_compat_register);
+
+/**
+ * class_compat_unregister - unregister a compatibility class
+ * @cls: the class to unregister
+ */
+void class_compat_unregister(struct class_compat *cls)
+{
+	kobject_put(cls->kobj);
+	kfree(cls);
+}
+EXPORT_SYMBOL_GPL(class_compat_unregister);
+
+/**
+ * class_compat_create_link - create a compatibility class device link to
+ *			      a bus device
+ * @cls: the compatibility class
+ * @dev: the target bus device
+ * @device_link: an optional device to which a "device" link should be created
+ */
+int class_compat_create_link(struct class_compat *cls, struct device *dev,
+			     struct device *device_link)
+{
+	int error;
+
+	error = sysfs_create_link(cls->kobj, &dev->kobj, dev_name(dev));
+	if (error)
+		return error;
+
+	/*
+	 * Optionally add a "device" link (typically to the parent), as a
+	 * class device would have one and we want to provide as much
+	 * backwards compatibility as possible.
+	 */
+	if (device_link) {
+		error = sysfs_create_link(&dev->kobj, &device_link->kobj,
+					  "device");
+		if (error)
+			sysfs_remove_link(cls->kobj, dev_name(dev));
+	}
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(class_compat_create_link);
+
+/**
+ * class_compat_remove_link - remove a compatibility class device link to
+ *			      a bus device
+ * @cls: the compatibility class
+ * @dev: the target bus device
+ * @device_link: an optional device to which a "device" link was previously
+ * 		 created
+ */
+void class_compat_remove_link(struct class_compat *cls, struct device *dev,
+			      struct device *device_link)
+{
+	if (device_link)
+		sysfs_remove_link(&dev->kobj, "device");
+	sysfs_remove_link(cls->kobj, dev_name(dev));
+}
+EXPORT_SYMBOL_GPL(class_compat_remove_link);
+
 int __init classes_init(void)
 {
 	class_kset = kset_create_and_add("class", NULL, NULL);
--- linux-2.6.31-rc4.orig/include/linux/device.h	2009-07-30 22:21:31.000000000 +0200
+++ linux-2.6.31-rc4/include/linux/device.h	2009-07-31 11:30:25.000000000 +0200
@@ -223,6 +223,14 @@ extern void class_unregister(struct clas
 	__class_register(class, &__key);	\
 })
 
+struct class_compat;
+struct class_compat *class_compat_register(const char *name);
+void class_compat_unregister(struct class_compat *cls);
+int class_compat_create_link(struct class_compat *cls, struct device *dev,
+			     struct device *device_link);
+void class_compat_remove_link(struct class_compat *cls, struct device *dev,
+			      struct device *device_link);
+
 extern void class_dev_iter_init(struct class_dev_iter *iter,
 				struct class *class,
 				struct device *start,


-- 
Jean Delvare

Re: [PATCH] i2c: Do not give adapters a default parent

[Kay Sievers]

Hey Jean,

On Tue, Aug 4, 2009 at 12:55, Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> I have come up with the following
> implementation, would that be OK with you? If it is, please add your
> Signed-off-by.

Looks all good to me. Feel free to add:
  Signed-off-by: Kay Sievers <kay.sievers-tD+1rO4QERM@public.gmane.org>

Thanks,
Kay