i2c-dev: add ioctls PROBE and REMOVE

From: Michael Lawnick <[email protected]>

After system startup currently there is no way to add an i2c client to subsystem. This may occur if the client sits on a subsystem that was powerless at the beginning or is plugged in at runtime.
This patch adds ioctl to /dev/i2c-X to initiate a probe of an client and to remove a client that may have gone.
Example:
fd=open("/dev/i2c-1",O_RDWR); ioctl(fd,I2C_PROBE,"lm75,0x49");
close(fd);
will start probing of an lm75 compatible sensor at bus 1, id 0x49 ioctl(fd,I2C_REMOVE,0x49)
removes it again.

---
Add ioctls I2C_PROBE and I2C_REMOVE to /dev/i2c-x

 drivers/i2c/i2c-dev.c   |   50
++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/i2c-dev.h |    2 +
 2 files changed, 51 insertions(+), 1 deletions(-)

index c171988..82cbe7f
@@ -367,9 +367,22 @@ static noinline int i2cdev_ioctl_smbus(struct

i2c_client *client,
return res;
}

+static int i2c_check_addr(struct device *dev, void *addrp) +{
+ struct i2c_client *client = i2c_verify_client(dev);
+ int addr = *(int *)addrp;
+
+ if (client && client->addr == addr)
+ return -EBUSY;
+ return 0;
+}
+
static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
{
- struct i2c_client *client = (struct i2c_client *)file->private_data;
+ struct i2c_client *bus_client, *client = (struct i2c_client
*)file->private_data;
+ struct i2c_board_info bInfo;
+ struct device *dev;
+ char *colon, request[I2C_NAME_SIZE + 5];
unsigned long funcs;

dev_dbg(&client->adapter->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n",
@@ -424,6 +437,41 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
case I2C_TIMEOUT:
client->adapter->timeout = arg;
break;
+ case I2C_PROBE:
+ memset(&bInfo, 0, sizeof(bInfo));
+ copy_from_user(request, (char *)arg, sizeof(request));
+
+ colon = strchr(request, ',');
+ if((!colon) || (colon - request > I2C_NAME_SIZE - 1))
+ return -EINVAL;
+
+ strncpy(bInfo.type, request, colon - request);
+ bInfo.type[I2C_NAME_SIZE - 1] = '\0';
+ bInfo.addr = simple_strtoul(colon + 1, NULL, 0);
+
+ bus_client = i2c_new_device(client->adapter, &bInfo);
+
+ if(!bus_client)
+ return -EIO;
+
+ if(!bus_client->driver)
+ {
+ /* probe failed -> cleanup */
+ i2c_unregister_device(bus_client);
+ return -ENODEV;
+ }
+ break;
+ case I2C_REMOVE:
+ dev = device_find_child(&client->adapter->dev, &arg, i2c_check_addr);
+ if (!dev)
+ return -ENODEV;
+
+ bus_client = i2c_verify_client(dev);
+ if(!bus_client)
+ return -EIO;
+
+ i2c_unregister_device(bus_client);
+ break;
default:
/* NOTE: returning a fault code here could cause trouble
* in buggy userspace code. Some old kernel bugs returned

index 311315b..8b66638
@@ -50,6 +50,8 @@
 #define I2C_PEC		0x0708	/* != 0 to use PEC with SMBus */
 #define I2C_SMBUS	0x0720	/* SMBus transfer */
 
+#define I2C_PROBE   0x0730  /* Probe for client "<name>,<addr>" */
+#define I2C_REMOVE  0x0731  /* Remove client <addr> */
 
 /* This is the structure as used in the I2C_SMBUS ioctl call */
 struct i2c_smbus_ioctl_data {

Hi Jean,

could you give it a look?
If you at least ACK the interface, I could do the patch for the documentation.

Regards,
Michael

Hi Michael,

Come on, which part of "this should be implemented in sysfs" did you not understand? I'm a little tired of you trying to implement it using _all_ other possible ways except the one we agreed on. If you need this feature faster than I can implement it, please stick to the plan and implement it using sysfs. If not, then just stay quiet and wait for it to happen.

Thanks,

Jean Delvare

Jean Delvare said the following:

Huh - don't bite me!

This step I wanted to discuss next:
AFAICS I2C subsystem doesn't implement any sysFs-entry itself. They are all either from client or device-driver system. I not even dared to ask for a comment to this before adding the feature on i2c-dev, a way that does not change the visible interface.
I can't wait too long for implementation on sysFs. I'm ready to do it now, if we agree on the place where to create the entries, naming and interface.
What is your time line?
Naming IMHO should be probe/remove, parameters could be "<client-name>,<id>" / "<id>", just as done in i2c-dev implementation, if created per adapter.
But where to place?

*Kind* regards,
Michael

You didn't look carefully then. From i2c-core.c:

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);
}

static struct device_attribute i2c_adapter_attrs[] = {
	__ATTR(name, S_IRUGO, show_adapter_name, NULL),
	{ },
};

static struct class i2c_adapter_class = {
	.owner			= THIS_MODULE,
	.name			= "i2c-adapter",
	.dev_attrs		= i2c_adapter_attrs,
};

What do you think i2c-dev is, if not a visible interface? If we add a new ioctl to the i2c-dev interface, we have to maintain it forever. That's not to be done lightly just because you can't wait for you actual need to be fulfilled by i2c-core.

The place is very clearly /sys/class/i2c-adapter/i2c-*, next to the "name" attribute I quoted above.

The naming and interface are to be discussed. Could be 2 write-only files, add_device and delete_device, and the most basic syntax I can think of would be:

echo lm75 0x49 > /sys/class/i2c-adapter/i2c-0/add_device echo 0x49 > /sys/class/i2c-adapter/i2c-0/delete_device

add_device would optionally accept an irq value. As for delete_device, I am wondering if we should require the chip name as well, for symmetry with add_device and for safety reasons, or if the above is sufficient.

Getting rid of the legacy binding model has higher priority and is far from being done. I also must take care of Rodolfo's multiplexer support, which a lot of people have been asking for, and which might as well be what you really need. The above interface comes next, I can't give it higher priority as the same can already be achieved today using module parameters for most drivers.

No, we can't use probe/remove as these are used for a completely different meaning in the Linux driver model (and these _are_ misnomers there, no question about that, but we have to live with it.) Not that "probe" would be a good name anyway, as you are instantiating a device, you are not probing for anything.

I'd go with a space instead of comma between parameters, for consistency with the pci subsystem's "new_id" files.

And it's really <address>, NOT <id>, even though we use the address as part of the device id internally.

Answered above, that was the easy part.

Jean Delvare

BTW, if you don't want your problem to be overlooked, I strongly suggest that you create yourself a wiki account on
http://i2c.wiki.kernel.org/, create a page describing the problem and its proposed solution, and link to it from the "Work in progress" section.

Jean Delvare

Jean Delvare said the following:

Ok, done.
<http://i2c.wiki.kernel.org/index.php?title=SysFs_entries_add_device/delete_device>

I made a patch, that uses i2c_new_device. But this won't use the detect method of the client, so its possible to instantiate almost any code on the given address :-(
Shouldn't we use i2c_detect_address() or something similar?

Kind regards,
Michael

Hi Michael,

Sorry for the late answer, I am pretty busy with the 2.6.30 merge window.

Thank you.

No. The whole point of the sysfs entries you are adding is to let the user instantiate I2C devices (at the device driver model level) for physical devices which can _not_ be detected. For devices which can be reliably detected, you would simply implement the .detect() method, set the I2C class, and let i2c-core and the device driver take care of the detection and device creation.

I hear you, "what about hot-plugged devices, they won't be detected"? For one thing, these need to be behind a mux or gate, which we don't support at the moment, so discussing this now doesn't make much sense. But at any rate I don't think that the files you are adding in sysfs are meant to address this particular issue. If we want to be able to re-trigger detection on a given I2C segment (which currently only happens at I2C adapter creation time and when a new driver is registered) this would be done by a separate sysfs entry (say "detect", write 1 to re-trigger all detections on a given segment). But really I'd rather wait for Rodoflo's work to be merged first, depending on the implementation there may or may not be a practical need for this.

Right now I am working hard at getting rid of the few remaining legacy i2c device drivers. Then I can clean up i2c-core and then we can look into Rodolfo's patches. I know it seems to take forever, but actually we are making progress and will finally get there.

Jean Delvare

Hi Jean,

although you are very busy, I hope you can spend a little more time.

Jean Delvare said the following:

I see, that's _your_ motivation. You want a user 'force' method.

...

No, sorry, here your are mistaken.
IMHO you recognize only a part of the problem: Even if separation is done by a mux (what is not strictly necessary), it will be powered by main system and its driver (i.e. adapters) loaded on boot. Still clients may come up when system is already initialized and auto detection on the sub buses has already completed.

Sorry, then you misunderstood me - it *is* this particular issue I need to solve.
Please forget the thread about lm75a (vs. lm75), this was just a stone on my way to a new running kernel. It took it, made a remark and threw it aside. Nothing more. It is not related to the detect problem.

This solution will cause much useless traffic while the system is already running and doing other hard work :-(

AFAICS has Rodolfo's work nothing to do with the problem above, as on adapter creation an auto detection will (should) be triggered.

Michael Lawnick

Hi Michael,

Yes it is. Admittedly, the feature you are asking for is not one I think a lot of people will be interested in, it seems pretty borderline as far as the I2C standard is concerned, therefore I am trying to see what other widely needed feature could be implemented, which would incidentally cover your uncommon need.

[Nitpicking mode] Instantiate, not force. You're really only creating a device, and then letting any interested driver bind to it if it feels like it. Just like platform data can do on embedded platforms, except that it comes from user-space.

But yes, there is no verification (or detection) step involved. The i2c subsystem takes your word, assumes you know what you're doing, and simply does what you asked for.

It is necessary. "Simple" hot-plug is a violation of the I2C standard. You need a way to guarantee that no I2C traffic will be on-going when the new device is connected, and for this you need at least an I2C gate to isolate the bus segment in question from all masters on the bus.

This is where I am unclear. It might make sense to remove (in the device driver model sense of the term) i2c segments which are being modified, and restore them afterwards. That is, I2C segments hanging off a mux would have 3 possible states, active, inactive and disabled. The change from disabled to active or inactive would trigger device detection, just as the mere addition of an i2c adapter currently does.

I don't think this is what Rodoflo did, but this would address your problem as far as I can see.

Much useless traffic? It will cause way less traffic than when adding a new i2c adapter (or bus segment) in the first place. All devices which are already bound will be excluded from this detection round. This should be most of them as I understand it. Most probably, only the newly added device will receive I2C traffic.

Sure, we could extend the syntax so that you can ask for just one address on one bus segment to be probed (maybe even by just one driver). But what's the point? If you already know all this, then the add_device sysfs interface is doing the job. And remember that the device you just connected might not even be reliably detectable at all!

See my point above... The detection could happen at creation time _or_ at activation time. In the latter case Rodoflo's work becomes totally relevant.

Jean Delvare