[i2c] [patch 2.6.20-rc2] migration aids for i2c_adapter.dev removal

[Jean Delvare]

Hi David,

On 12/29/2006, "David Brownell" <david-b at pacbell.net> wrote:

>Hi Jean,
>
>Based on our discussion which culminated in
>
>  http://lists.lm-sensors.org/pipermail/i2c/2006-December/000648.html
>
>see the appended patch, which I understand should be a 2.6.20 candidate.
>You were particularly concerned about the class attribute, but it seemed
>to me the other two changes would make it easier to get adapter driver
>updates integrated sooner.

I agree with one, not the other, see below.

>
>So I think the status of those discussions is summarized as:
>
> - four patches mostly switching diagnostics to i2c_adapter.class_dev.dev
>   ... ready "now" for merge into MM
>
> - your patch initializing i2c_adapter.dev.parent for many adapters
>   ... can merge at any time, though I'd suggest making them init
>   i2c_adapter.class_dev.dev "now" (after merging the appended patch)
>   rather than later;
>
> - my "patch #5/6" -- "5A" (without removal of i2c_adapter.dev from i2c-core)
>   ... as above, can merge at any time after merging the appended patch
>
> - my "patch #5/6" -- "5B" (only the removal of i2c_adapter.dev)
>   ... defer till july-ish
>
> - my "patch #6/6" (adding suspend/resume/shutdown methods)
>   ... you didn't re-review, but should be ok at any time (after reissue
>   to cope with #5B being deferred.  (And actually nice for 2.6.20 too.)
>
>I'm not going to re-send the first four, unless you ask.  But after I send
>this one, I'll send the revised #5A, and later #6; thinking those could go
>into MM at any time after the appended patch.

Yes, good summary, we agree on the plan.

>Flag i2c_adapter.dev for removal after userspace tools get upgraded, and
>include some near-term code migration aids to facilitate this:
>
> - The class device gets the name attribute it should have had.  This
>   was previously (wrongly) associated with the i2c_adapter.dev node.
>   Sysfs tools can start converting right away.

"sysfs-based tools and libraries"?

>
> - Handle adapter drivers that have stopped using i2c_adapter.dev; init
>   uses i2c_adapter.class_dev.dev if i2c_adapter.dev.parent is null.
>   Adapter drivers can start converting right away.
>
> - Issue a warning for legacy adapter drivers that don't provide any
>   physical device node, in either class_dev.dev or dev.parent; so
>   systems with those drivers will know to fix this problem earlier.
>
>This is one of a series of patches to help the I2C stack become a better
>citizen of the Linux Driver Model world.
>
>Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
>
>Index: at91/Documentation/feature-removal-schedule.txt
>===================================================================
>--- at91.orig/Documentation/feature-removal-schedule.txt	2006-12-15 10:08:52.000000000 -0800
>+++ at91/Documentation/feature-removal-schedule.txt	2006-12-28 17:03:18.000000000 -0800
>@@ -226,6 +226,23 @@ Who:	Jean Delvare <khali at linux-fr.org>
>
> ---------------------------
>
>+What:	i2c_adapter.dev
>+	i2c_adapter.list
>+When:	July 2007
>+Why:	Superfluous, given i2c_adapter.class_dev:
>+	  * The "dev" was a stand-in for the physical device node that legacy
>+	    drivers would not have; but now it's almost always present.  Any
>+	    remaining legacy drivers must upgrade (they now trigger warnings).
>+	  * The "list" duplicates class device children.
>+	The delay in removing this is so upgraded lm_sensors and libsensors
>+	can get deployed.  (Removal causes minor changes in the sysfs layout,
>+	notably the location of the adapter type name and parenting the i2c
>+	client hardware directly from their controller.)
>+Who:	Jean Delvare <khali at linux-fr.org>,
>+	David Brownell <dbrownell at users.sourceforge.net>
>+
>+---------------------------
>+
> What:	IPv4 only connection tracking/NAT/helpers
> When:	2.6.22
> Why:	The new layer 3 independant connection tracking replaces the old
>Index: at91/drivers/i2c/i2c-core.c
>===================================================================
>--- at91.orig/drivers/i2c/i2c-core.c	2006-12-28 16:21:07.000000000 -0800
>+++ at91/drivers/i2c/i2c-core.c	2006-12-28 19:56:52.000000000 -0800
>@@ -95,16 +95,32 @@ struct device_driver i2c_adapter_driver
> 	.bus = &i2c_bus_type,
> };
>
>+/* ------------------------------------------------------------------------- */
>+
>+/* I2C bus adapters -- one roots each I2C or SMBUS segment */
>+
> 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);
> }
>
>+static ssize_t i2c_adapter_show_name(struct class_device *cdev, char *buf)
>+{
>+	struct i2c_adapter *adap = class_dev_to_i2c_adapter(cdev);
>+	return sprintf(buf, "%s\n", adap->name);
>+}
>+
>+static struct class_device_attribute i2c_adapter_attrs[] = {
>+	__ATTR(name, S_IRUGO, i2c_adapter_show_name, NULL),
>+	{ },
>+};
>+
> struct class i2c_adapter_class = {
>-	.owner =	THIS_MODULE,
>-	.name =		"i2c-adapter",
>-	.release =	&i2c_adapter_class_dev_release,
>+	.owner =		THIS_MODULE,
>+	.name =			"i2c-adapter",
>+	.class_dev_attrs =	i2c_adapter_attrs,
>+	.release =		&i2c_adapter_class_dev_release,
> };
>
> static ssize_t show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
>@@ -171,12 +187,21 @@ int i2c_add_adapter(struct i2c_adapter *
> 	list_add_tail(&adap->list,&adapters);
> 	INIT_LIST_HEAD(&adap->clients);
>
>+	/* When we get rid of adap->dev, class_dev.dev must always be
>+	 * provided.  Allow that init scheme before it's needed, too.
>+	 */
>+	if (adap->dev.parent == NULL)
>+		adap->dev.parent = adap->class_dev.dev;
>+

This could theoretically have bad side effects, so I'm reluctant to push
this in 2.6.20. Best case, it doesn't actually change anything (no
driver in the kernel tree is currently setting adap->class_dev.dev.)

I'd rather have "this change" in patch #5A, where we change all the
bus drivers to initialize adap->class_dev.dev instead of
adap->dev.parent. By "this change", I don't mean the conditional
above, though, but making adap->class_dev.dev the only possible
initialization source.

> 	/* 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)
>+	if (adap->dev.parent == NULL) {
> 		adap->dev.parent = &platform_bus;
>+		printk(KERN_WARNING "i2c-%d: physical device not specified\n",
>+			adap->nr);
>+	}

I'd like a more explicit message, something that sounds to developers
like "you must fix this driver". No need to go dramatic, we don't
want to frighten the regular user either, but I'm certain that a random
developer wouldn't pay any attention to the message above. It doesn't
even mention the name of the offending device...

> 	sprintf(adap->dev.bus_id, "i2c-%d", adap->nr);
> 	adap->dev.driver = &i2c_adapter_driver;
> 	adap->dev.release = &i2c_adapter_dev_release;

Thanks,
--
Jean Delvare