I2C: Rewrite i2c_probe

Hi Greg,

i2c_probe was quite complex and slow, so I rewrote it in a more efficient and hopefully clearer way.

Note that this slightly changes the way the module parameters are handled. This shouldn't change anything for the most common cases though.

For one thing, the function now respects the order of the parameters for address probing. It used to always do lower addresses first. The new approach gives the user more control.

For another, ignore addresses don't overrule probe addresses anymore. This could have been restored the way it was at the cost of a few more lines of code, but I don't think it's worth it. Both lists are given as module parameters, so a user would be quite silly to specify the same addresses in both lists. The normal addresses list is the only one that isn't controlled by a module parameter, thus is the only one the user may reasonably want to remove an address from.

Another significant change is the fact that i2c_probe() will no more stop when a detection function returns -ENODEV. Just because a driver found a chip it doesn't support isn't a valid reason to stop all probings for this one driver. This closes the long standing lm_sensors ticket #1807.

http://www2.lm-sensors.nu/~lm78/readticket.cgi?ticket07

I updated the documentation accordingly.

In terms of algorithmic complexity, the new code is way better. If I is the ignore address count, P the probe address count, N the normal address count and F the force address count, the old code was doing 128 * (F + I + P + N) iterations max, while the new code does F + P + ((I+1) * N) iterations max. For the most common case where F, I and P are empty, this is down from 128 * N to N.

 Documentation/i2c/writing-clients |    7 -
 drivers/i2c/i2c-core.c            |  167 ++++++++++++++++++++------------------
 2 files changed, 94 insertions(+), 80 deletions(-)

@@ -662,107 +662,120 @@
  * Will not work for 10-bit addresses!
  * ----------------------------------------------------
  */
-/* Return: kind (>= 0) if force found, -1 if not found */
-static inline int i2c_probe_forces(struct i2c_adapter *adapter, int addr,
-				   unsigned short **forces)
-{
-	unsigned short kind;
-	int j, adap_id = i2c_adapter_id(adapter);
-
-	for (kind = 0; forces[kind]; kind++) {
-		for (j = 0; forces[kind][j] != I2C_CLIENT_END; j += 2) {
-			if ((forces[kind][j] = adap_id ||
-			     forces[kind][j] = ANY_I2C_BUS)
-			 && forces[kind][j + 1] = addr) {
-				dev_dbg(&adapter->dev, "found force parameter, "
-					"addr 0x%02x, kind %u\n", addr, kind);
-				return kind;
-			}
-		}
+static int i2c_probe_address(struct i2c_adapter *adapter, int addr, int kind,
+			     int (*found_proc) (struct i2c_adapter *, int, int))
+{
+	int err;
+
+	/* Make sure the address is valid */
+	if (addr < 0x03 || addr > 0x77) {
+		dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n",
+			 addr);
+		return -EINVAL;
 	}
 
-	return -1;
+	/* Skip if already in use */
+	if (i2c_check_addr(adapter, addr))
+		return 0;
+
+	/* Make sure there is something at this address, unless forced */
+	if (kind < 0
+	 && i2c_smbus_xfer(adapter, addr, 0, 0, 0, I2C_SMBUS_QUICK, NULL) < 0)
+		return 0;
+
+	/* Finally call the custom detection function */
+	err = found_proc(adapter, addr, kind);
+
+	/* -ENODEV can be returned if there is a chip at the given address
+	   but it isn't supported by this chip driver. We catch it here as
+	   this isn't an error. */
+	return (err = -ENODEV) ? 0 : err;
 }
 
 int i2c_probe(struct i2c_adapter *adapter,
 	      struct i2c_client_address_data *address_data,
 	      int (*found_proc) (struct i2c_adapter *, int, int))
 {
-	int addr,i,found,err;
+	int i, err;
 	int adap_id = i2c_adapter_id(adapter);
 
 	/* Forget it if we can't probe using SMBUS_QUICK */
 	if (! i2c_check_functionality(adapter,I2C_FUNC_SMBUS_QUICK))
 		return -1;
 
-	for (addr = 0x00; addr <= 0x7f; addr++) {
-
-		/* Skip if already in use */
-		if (i2c_check_addr(adapter,addr))
-			continue;
-
-		/* If it is in one of the force entries, we don't do any detection
-		   at all */
-		found = 0;
-
-		if (address_data->forces) {
-			int kind = i2c_probe_forces(adapter, addr,
-						    address_data->forces);
-			if (kind >= 0) { /* force found */
-				if ((err = found_proc(adapter, addr, kind)))
-					return err;
-				continue;
-			}
-		}
-
-		/* If this address is in one of the ignores, we can forget about
-		   it right now */
-		for (i = 0;
-		     !found && (address_data->ignore[i] != I2C_CLIENT_END);
-		     i += 2) {
-			if (((adap_id = address_data->ignore[i]) || 
-			    ((address_data->ignore[i] = ANY_I2C_BUS))) &&
-			    (addr = address_data->ignore[i+1])) {
-				dev_dbg(&adapter->dev, "found ignore parameter for adapter %d, "
-					"addr %04x\n", adap_id ,addr);
-				found = 1;
+	/* Force entries are done first, and are not affected by ignore
+	   entries */
+	if (address_data->forces) {
+		unsigned short **forces = address_data->forces;
+		int kind;
+
+		for (kind = 0; forces[kind]; kind++) {
+			for (i = 0; forces[kind][i] != I2C_CLIENT_END;
+			     i += 2) {
+				if (forces[kind][i] = adap_id
+				 || forces[kind][i] = ANY_I2C_BUS) {
+					dev_dbg(&adapter->dev, "found force "
+						"parameter for adapter %d, "
+						"addr 0x%02x, kind %d\n",
+						adap_id, forces[kind][i + 1],
+						kind);
+					err = i2c_probe_address(adapter,
+						forces[kind][i + 1],
+						kind, found_proc);
+					if (err)
+						return err;
+				}
 			}
 		}
-		if (found) 
-			continue;
+	}
 
-		/* Now, we will do a detection, but only if it is in the normal or 
-		   probe entries */  
-		for (i = 0;
-		     !found && (address_data->normal_i2c[i] != I2C_CLIENT_END);
-		     i += 1) {
-			if (addr = address_data->normal_i2c[i]) {
-				found = 1;
-				dev_dbg(&adapter->dev, "found normal i2c entry for adapter %d, "
-					"addr %02x\n", adap_id, addr);
-			}
+	/* Probe entries are done second, and are not affected by ignore
+	   entries either */
+	for (i = 0; address_data->probe[i] != I2C_CLIENT_END; i += 2) {
+		if (address_data->probe[i] = adap_id
+		 || address_data->probe[i] = ANY_I2C_BUS) {
+			dev_dbg(&adapter->dev, "found probe parameter for "
+				"adapter %d, addr 0x%02x\n", adap_id,
+				address_data->probe[i + 1]);
+			err = i2c_probe_address(adapter,
+						address_data->probe[i + 1],
+						-1, found_proc);
+			if (err)
+				return err;
 		}
+	}
 
-		for (i = 0;
-		     !found && (address_data->probe[i] != I2C_CLIENT_END);
-		     i += 2) {
-			if (((adap_id = address_data->probe[i]) ||
-			    ((address_data->probe[i] = ANY_I2C_BUS))) &&
-			    (addr = address_data->probe[i+1])) {
-				found = 1;
-				dev_dbg(&adapter->dev, "found probe parameter for adapter %d, "
-					"addr %04x\n", adap_id,addr);
+	/* Normal entries are done last, unless shadowed by an ignore entry */
+	for (i = 0; address_data->normal_i2c[i] != I2C_CLIENT_END; i += 1) {
+		int j, ignore;
+
+		ignore = 0;
+		for (j = 0; address_data->ignore[j] != I2C_CLIENT_END;
+		     j += 2) {
+			if ((address_data->ignore[j] = adap_id ||
+			     address_data->ignore[j] = ANY_I2C_BUS)
+			 && address_data->ignore[j + 1]
+			    = address_data->normal_i2c[i]) {
+				dev_dbg(&adapter->dev, "found ignore "
+					"parameter for adapter %d, "
+					"addr 0x%02x\n", adap_id,
+					address_data->ignore[j + 1]);
 			}
+			ignore = 1;
+			break;
 		}
-		if (!found) 
+		if (ignore)
 			continue;
 
-		/* OK, so we really should examine this address. First check
-		   whether there is some client here at all! */
-		if (i2c_smbus_xfer(adapter,addr,0,0,0,I2C_SMBUS_QUICK,NULL) >= 0)
-			if ((err = found_proc(adapter,addr,-1)))
-				return err;
+		dev_dbg(&adapter->dev, "found normal entry for adapter %d, "
+			"addr 0x%02x\n", adap_id,
+			address_data->normal_i2c[i]);
+		err = i2c_probe_address(adapter, address_data->normal_i2c[i],
+					-1, found_proc);
+		if (err)
+			return err;
 	}
+
 	return 0;
 }
 

@@ -241,9 +241,10 @@
 parts are between /* SENSORS ONLY START */ and /* SENSORS ONLY END */
 markers. 
 
-This function should only return an error (any value != 0) if there is
-some reason why no more detection should be done anymore. If the
-detection just fails for this address, return 0.
+Returning an error different from -ENODEV in a detect function will cause
+the detection to stop: other addresses and adapters won't be scanned.
+This should only be done on fatal or internal errors, such as a memory
+shortage or i2c_attach_client failing.
 
 For now, you can ignore the `flags' parameter. It is there for future use.
 

Jean Delvare

Hi Jean,

Good thing you brought this up. This is a site of a problem I see. I snipped all but a particular line in source you are posting so you can see.

As you can see in the below code, the call to i2c_smbus_xfer() has I2C_SMBUS_QUICK in the 2nd to last parameter.
Looking at the definition of i2c_smbus_xfer(), we see that the parameters are:
extern s32 i2c_smbus_xfer (struct i2c_adapter * adapter, u16 addr,
unsigned short flags,
char read_write, u8 command, int size, union i2c_smbus_data * data);

As the code below shows, the 3rd to last should be I2C_SMBUS_QUICK. Is this an accurate assumption? If not, can you please explain this? I have seen this problem pretty much everywhere i2c_smbus_xfer() and i2c_smbus_xfer_emulated() are called. If this is a semantic bug, I will be glad to patch this today. You can also see in the removed block that you did type it as it was before.

What is the verdict?

-David van Hoose

;SNIP through i2c_probe_addresses
+ /* Make sure there is something at this address, unless forced */
+ if (kind < 0
+ && i2c_smbus_xfer(adapter, addr, 0, 0, 0, I2C_SMBUS_QUICK, NULL) < 0)
+ return 0;
+
+ /* Finally call the custom detection function */
+ err = found_proc(adapter, addr, kind);

;SNIP several more lines to i2c_probe

- /* OK, so we really should examine this address. First check
- whether there is some client here at all! */
- if (i2c_smbus_xfer(adapter,addr,0,0,0,I2C_SMBUS_QUICK,NULL) >= 0)
- if ((err = found_proc(adapter,addr,-1)))
- return err;
+ dev_dbg(&adapter->dev, "found normal entry for adapter %d, "
+ "addr 0x%02x\n", adap_id,
+ address_data->normal_i2c[i]);
+ err = i2c_probe_address(adapter, address_data->normal_i2c[i],
+ -1, found_proc);
+ if (err)
+ return err;

;SNIP end

Hi David,

No, it's not. The code looks correct to me.

"size" refers to the type of SMBus transaction that is being performed. These are refered to as "protocols" in the SMBus 2.0 specification (page 26, 5.3):

"There are eleven possible command protocols for any given device. A slave device may use any or all of the eleven protocols to communicate. The protocols are Quick Command, Send Byte, Receive Byte, Write Byte, Write Word, Read Byte, Read Word, Process Call, Block Read, Block Write and Block Write-Block Read Process Call."

I think that the "size" term comes from the fact that the seven fundamental "protocols" have an increasing data size: quick command has 0 data byte, send/receive byte have 1, write/read byte have 2 and write/read word have 3. I agree that this is somewhat of a misnommer, but it ain't that bad when you know what it is all about, and I wouldn't like "protocol" much better.

"command" refers to the first data byte being transfered after the address+write byte. If I remember correctly, all "protocols" except Quick Command and Receive Byte start with a command byte. So please note that the "0" value for the command parameter in our example doesn't mean anything, it's not used at all. From the SMBus 2.0, same page and section as above:

"A typical SMBus device will have a set of commands by which data can be read and written. All commands are 8 bits (1 byte) long. Command arguments and return values can vary in length."

This is a semantic issue, but I wouldn't call it a bug. I don't think the "command" and "size" names are as bad as you claim. Especially "command" is taken from the SMBus specification itself. I see no immediate need to change the parameter names, as it would possibly confuse people more than help them. You are the first one to complain about these names since I started working on i2c/lm_sensors, two years ago.

Thanks,

Jean Delvare

Hi David,

What hardcoded integer constants are you talking about?

Jean Delvare

Hi Jean,

From places like below (a snippet from your patch). You use 0 instead
of preprocessor tokens. It is also this way in various other places in the i2c core code.

// Snippet of i2c_probe_addresses
+ /* Make sure there is something at this address, unless forced */
+ if (kind < 0
+ && i2c_smbus_xfer(adapter, addr, 0, 0, 0, I2C_SMBUS_QUICK, NULL) < 0)
+ return 0;
+
+ /* Finally call the custom detection function */
+ err = found_proc(adapter, addr, kind);
// Snippet end

IMHO it would make the code a bit less obfuscated.
I can get a patch done by tonight against 2.6.12.4 unless you have another source I should try.

Thanks,
David van Hoose

Hi David,

As far as I can see only one "0" (the middle one) has a replacement preprocessor token (I2C_SMBUS_WRITE), right? I don't think we have preprocessor token for "no flags" nor "command doesn't matter", and I don't think we should introduce them, as 0 is just as natural in this case.

I think that I2C_SMBUS_WRITE is not used here on purpose, because there is not such thing as a quick read command in the SMBus protocol. The quick command is always a write, sending a single bit of data to the target chip. This is a *very* weird choice from Intel if you want my opinion, but that's the way SMBus was designed. I'd expect that you will find 0 or 1 instead of I2C_SMBUS_WRITE or I2C_SMBUS_READ, respectively, only when I2C_SMBUS_QUICK is used. If this is the case, that's not something we want to change, as it makes some sense with regards to the SMBus protocol.

Now, if there are non-quick commands not using I2C_SMBUS_WRITE/READ when they should, please point them out to us.

Thanks,

Jean Delvare

Hi Jean,

Sounds like a good point to me. I'll submit patches for any code that could semantically use the defines.
Why or why should we not use enums instead of ints? Personally, I find enums are better for type checking, and I find that "command none" or "flags none" is more natural as it is very explicit. It is really a matter of convention though as it doesn't affect the running any. However, it is possible that it could uncover and possibly prevent obscure typo related bugs.
Thoughts?

Regards,
David