i2c-mux-gpio platform device ID issue

i2c-mux-gpio platform device ID issue

[Jean Delvare]

Hi Peter,

I am anticipating an issue with driver i2c-mux-gpio. Right now there
are no user in the kernel tree (surprisingly...), but I'm about to add
the first one (for SMBus multiplexing on an x86 motherboard) and I
expect more will follow.

Right now I am allocating the multiplexer device with:

	platform_device_alloc("i2c-mux-gpio", -1);

This is OK as long as I am the only user. But if the same system needs
another instance of "i2c-mux-gpio", the second attempt will fail,
because both devices will have the same name and this isn't allowed by
the driver core. I could pass an instance ID at allocation time,
however it would have to be different for my driver and the other
driver, and there's no way to guarantee this for two random drivers.

Do you have any idea how we can solve this problem?

My only idea so far is to change the semantics of pdev->id == -1 from
"there's only one device" to "I don't care about the device ID".
Platform devices created with pdev->id == -1 would get an arbitrary ID
value, using an IDA-style mechanism. The IDA would be stored at the
platform driver level. It has the drawback of adding some burden to all
platform drivers, but I'm not sure how much yet, it might be just
acceptable.

If a per-driver IDA isn't acceptable, then we can go for a global IDA,
but then we can't re-use pdev->id == -1 for it. We'd have to introduce
pdev->id == -2 for drivers which want a dynamically allocated unique
ID. Actually this might be a better approach, as it's less intrusive.

Opinions?

-- 
Jean Delvare

Re: i2c-mux-gpio platform device ID issue

[Jean Delvare]

On Wed, 25 Jul 2012 14:44:09 +0200, Jean Delvare wrote:
> If a per-driver IDA isn't acceptable, then we can go for a global IDA,
> but then we can't re-use pdev->id == -1 for it. We'd have to introduce
> pdev->id == -2 for drivers which want a dynamically allocated unique
> ID. Actually this might be a better approach, as it's less intrusive.

A possible implementation of this would be:

From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Subject: platform: Add support for automatic device IDs

Right now we have support for explicit platform device IDs, as well as
ID-less platform devices when a given device type can only have one
instance. However there are cases where multiple instances of a device
type can exist, and their IDs aren't (and can't be) known in advance
and do not matter. In that case we need automatic device IDs to avoid
device name collisions.

I am using magic ID value -4 for this (I left -2 and -3 free in case
we ever need a couple of other magic values.) The automatically
allocated device IDs are global (to avoid an additional per-driver
cost) and are stored internally as negative numbers, starting with -4.
This is required so that the IDs can be freed later. Externally the
positive value is used.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
If anyone has a problem with the -4 or using negative device IDs
internally, it would be possible to avoid that by adding a boolean
attribute to every platform device to record whether the ID needs to
be freed. This would cost some memory.

The code assumes that a given platform driver doesn't mix devices
with explicit IDs and devices with automatic IDs. This seems
reasonable to me. If not, we'll need separate namespaces for both
types.

 drivers/base/platform.c |   30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

--- linux-3.5.orig/drivers/base/platform.c	2012-07-21 22:58:29.000000000 +0200
+++ linux-3.5/drivers/base/platform.c	2012-07-25 16:37:03.707518815 +0200
@@ -20,9 +20,12 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
+#include <linux/idr.h>
 
 #include "base.h"
 
+static DEFINE_IDA(platform_ida);
+
 #define to_platform_driver(drv)	(container_of((drv), struct platform_driver, \
 				 driver))
 
@@ -273,10 +276,27 @@ int platform_device_add(struct platform_
 
 	pdev->dev.bus = &platform_bus_type;
 
-	if (pdev->id != -1)
+	switch (pdev->id) {
+	default:
 		dev_set_name(&pdev->dev, "%s.%d", pdev->name,  pdev->id);
-	else
+		break;
+	case -1:
 		dev_set_name(&pdev->dev, "%s", pdev->name);
+		break;
+	case -4:
+		/*
+		 * Automatically allocated device ID. Stored as a negative
+		 * number so that we remember it must be freed. We use the
+		 * opposite in the device name so that the user still sees a
+		 * positive device ID.
+		 */
+		i = ida_simple_get(&platform_ida, 4, 0, GFP_KERNEL);
+		if (i < 0)
+			return i;
+		pdev->id = -i;
+		dev_set_name(&pdev->dev, "%s.%d", pdev->name, -pdev->id);
+		break;
+	}
 
 	for (i = 0; i < pdev->num_resources; i++) {
 		struct resource *p, *r = &pdev->resource[i];
@@ -309,6 +329,9 @@ int platform_device_add(struct platform_
 		return ret;
 
  failed:
+	if (pdev->id <= -4)
+		ida_simple_remove(&platform_ida, -pdev->id);
+
 	while (--i >= 0) {
 		struct resource *r = &pdev->resource[i];
 		unsigned long type = resource_type(r);
@@ -336,6 +359,9 @@ void platform_device_del(struct platform
 	if (pdev) {
 		device_del(&pdev->dev);
 
+		if (pdev->id <= -4)
+			ida_simple_remove(&platform_ida, -pdev->id);
+
 		for (i = 0; i < pdev->num_resources; i++) {
 			struct resource *r = &pdev->resource[i];
 			unsigned long type = resource_type(r);

-- 
Jean Delvare

Re: i2c-mux-gpio platform device ID issue

[Mark Brown]

On Wed, Jul 25, 2012 at 04:39:13PM +0200, Jean Delvare wrote:

> I am using magic ID value -4 for this (I left -2 and -3 free in case
> we ever need a couple of other magic values.) The automatically

We could always add the new magic values starting at -3 instead...

Re: i2c-mux-gpio platform device ID issue

[Jean Delvare]

Hi Mark,

On Wed, 25 Jul 2012 20:32:43 +0100, Mark Brown wrote:
> On Wed, Jul 25, 2012 at 04:39:13PM +0200, Jean Delvare wrote:
> 
> > I am using magic ID value -4 for this (I left -2 and -3 free in case
> > we ever need a couple of other magic values.) The automatically
> 
> We could always add the new magic values starting at -3 instead...

No, because my approach consumes _all_ negative values below the magic
value. First mux will get -4, second will get -5 etc.

-- 
Jean Delvare

Re: i2c-mux-gpio platform device ID issue

[Mark Brown]

On Wed, Jul 25, 2012 at 11:37:27PM +0200, Jean Delvare wrote:
> On Wed, 25 Jul 2012 20:32:43 +0100, Mark Brown wrote:
> > On Wed, Jul 25, 2012 at 04:39:13PM +0200, Jean Delvare wrote:

> > > I am using magic ID value -4 for this (I left -2 and -3 free in case
> > > we ever need a couple of other magic values.) The automatically

> > We could always add the new magic values starting at -3 instead...

> No, because my approach consumes _all_ negative values below the magic
> value. First mux will get -4, second will get -5 etc.

Oh, these are the values you're assigning not the value you use to
request this behaviour?  I see, that makes more sense.

Re: i2c-mux-gpio platform device ID issue

[Jean Delvare]

Hi again Peter and Mark,

On Wed, 25 Jul 2012 14:44:09 +0200, Jean Delvare wrote:
> If a per-driver IDA isn't acceptable, then we can go for a global IDA,
> but then we can't re-use pdev->id == -1 for it. We'd have to introduce
> pdev->id == -2 for drivers which want a dynamically allocated unique
> ID. Actually this might be a better approach, as it's less intrusive.

I slept on it and came up with what I think is an easier and more
elegant solution. We could simply agree on using GPIO pin numbers as
platform device IDs, as two i2c-gpio-mux instances can't control the
same GPIO pin.

* * * * *

From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Subject: i2c-mux-gpio: Document what device ID to use

We need a convention for i2c-mux-gpio platform device IDs so that they
do not collide.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Peter Korsgaard <peter.korsgaard-ob4gmnvZ1/cAvxtiuMwx3w@public.gmane.org>
---
 Documentation/i2c/muxes/i2c-mux-gpio |    7 +++++++
 1 file changed, 7 insertions(+)

--- linux-3.5.orig/Documentation/i2c/muxes/i2c-mux-gpio	2012-07-21 22:58:29.000000000 +0200
+++ linux-3.5/Documentation/i2c/muxes/i2c-mux-gpio	2012-07-26 08:41:23.291014194 +0200
@@ -63,3 +63,10 @@ static struct platform_device myboard_i2
 		.platform_data	= &myboard_i2cmux_data,
 	},
 };
+
+Device Registration
+-------------------
+
+When registering your i2c-gpio-mux device, you should pass the number
+of any GPIO pin it uses as the device ID. This guarantees that every
+instance has a different ID.


-- 
Jean Delvare

Re: i2c-mux-gpio platform device ID issue

[Jean Delvare]

Yeah, replying to myself once again...

On Thu, 26 Jul 2012 09:28:14 +0200, Jean Delvare wrote:
> I slept on it and came up with what I think is an easier and more
> elegant solution. We could simply agree on using GPIO pin numbers as
> platform device IDs, as two i2c-gpio-mux instances can't control the
> same GPIO pin.

Unfortunately this simple and elegant solution is not compatible with a
feature I want to add to the i2c-mux-gpio driver (deferred probing.)
I'll post that other patch later today. Unless someone sees a solution
I missed, I'm afraid we'll have to choose which one of these two
patches we want to take in, and discard the other one.

-- 
Jean Delvare