[lm-sensors] [PATCH 2.6] I2C: Rewrite i2c_probe

[lm-sensors] [PATCH 2.6] I2C: Rewrite i2c_probe

[Jean Delvare]

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?ticket\x1807

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.

Signed-off-by: Jean Delvare <khali@linux-fr.org>

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

--- linux-2.6.13-rc4.orig/drivers/i2c/i2c-core.c	2005-07-31 17:37:44.000000000 +0200
+++ linux-2.6.13-rc4/drivers/i2c/i2c-core.c	2005-07-31 20:55:44.000000000 +0200
@@ -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;
 }
 
--- linux-2.6.13-rc4.orig/Documentation/i2c/writing-clients	2005-07-31 17:37:44.000000000 +0200
+++ linux-2.6.13-rc4/Documentation/i2c/writing-clients	2005-07-31 20:55:44.000000000 +0200
@@ -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

[lm-sensors] [PATCH 2.6] I2C: Rewrite i2c_probe

[David van Hoose]

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

[lm-sensors] [PATCH 2.6] I2C: Rewrite i2c_probe

[Jean Delvare]

Hi David,

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

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

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

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

> If this is a semantic bug, I will be glad to patch this today.

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

[lm-sensors] [PATCH 2.6] I2C: Rewrite i2c_probe

[Jean Delvare]

Hi David,

> Would it be appreciated if I went through the code and changed the 
> hardcoded integer constants to actual preprocessor tokens?

What hardcoded integer constants are you talking about?

-- 
Jean Delvare

[lm-sensors] [PATCH 2.6] I2C: Rewrite i2c_probe

[David van Hoose]

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


Jean Delvare wrote:
> Hi David,
> 
> 
>>Would it be appreciated if I went through the code and changed the 
>>hardcoded integer constants to actual preprocessor tokens?
> 
> 
> What hardcoded integer constants are you talking about?
> 

[lm-sensors] [PATCH 2.6] I2C: Rewrite i2c_probe

[Jean Delvare]

Hi David,

> 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;
> // Snippet end

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

[lm-sensors] [PATCH 2.6] I2C: Rewrite i2c_probe

[David van Hoose]

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

Jean Delvare wrote:
> Hi David,
> 
> 
>>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;
>>// Snippet end
> 
> 
> 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,