Need help to fix some issues with the linux driver "i2c-gpio"

Quoted 1 line

Quoted 1 line

Bill Gatliff <bgat-uPd5UNENI//[email protected]> 30.11.2010 17:44 >>>

** High Priority **

we have to use the linux driver "i2c-gpio" because the "i2c-at91" is marked as "BROKEN" and for our application it can as well not be used.

Here a brief description of the application:

AT91SAM9261 based embedded system running kernel 2.6.25.4, with Atmel and our own BSP patches. This system uses both SPI interfaces, one USART (for console), MMC, Sound on SPI and SSC, digital poti for contrast control and the an chip Frambuffer for a monochrome LCD (QVGA).

On the TWI interface are attached:

    the AT24C04 SMB EEPROM,   (@ 0x50)
    two LM84 Temperature sensors  (@ 0x18, 0x19)
    and the Infrared temperature sensor MLX90614 manfactured by

MELEXIS. (@ 0x5A)
Note: The LM84 sensors are not yet operated by the linux kernel.

Now the description of the issue we have with the I2C subsystem:

1. the EEPROM is working fine with "i2c-at91" and the "i2c-gpio" modules

2. for IR-Sensor MLX90614, a hwmon class linux driver was implemented by Linutronix on our demand. This driver works fine but delivers sporadic the error message "i2c-adapter i2c-0: sendbytes: NAK bailout." (this message is thrown by the "i2c-algo-bit" driver), or invalid temperature values ( near 0xFFFF). The invalid temperature values and as well the error message appear as reponse on bus timeout situations which are not correctly handled by the linux driver. This we find out using a I2C analyzer. In our opinion these issues come while the i2c communication is disturbed by other tasks and/or interrupt service routines (ISR) which extend the SMB clock over the permitted timeouts, leaving the IR-Sensor in an undefined or erroneous state.

The address mentioned in the driver source "Haavard Skinnemoen <[email protected]>" invalid (unknown)

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> 30.11.2010 18:21 >>>

Quoted 1 line

** High Priority **

There is no such thing here. People on this list reply to help

on their free time. This is best effort, with zero guarantee.

Quoted 1 line

we have to use the linux driver "i2c-gpio" because the "i2c-at91"

Quoted 1 line

marked as "BROKEN" and for our application it can as well not be

Quoted 4 lines

Here a brief description of the application:

AT91SAM9261 based embedded system running kernel 2.6.25.4, with

Quoted 1 line

and our own BSP patches. This system uses both SPI interfaces, one

Quoted 1 line

(for console), MMC, Sound on SPI and SSC, digital poti for

Quoted 9 lines

control and the an chip Frambuffer for a monochrome LCD (QVGA).

On the TWI interface are attached:

    the AT24C04 SMB EEPROM,   (@ 0x50)
    two LM84 Temperature sensors  (@ 0x18, 0x19)
    and the Infrared temperature sensor MLX90614 manfactured by

MELEXIS. (@ 0x5A)
Note: The LM84 sensors are not yet operated by the linux kernel.

Quoted 4 lines

Now the description of the issue we have with the I2C subsystem:

1. the EEPROM is working fine with "i2c-at91" and the "i2c-gpio" modules

So at least the i2c-gpio driver isn't totally broken on your

Quoted 1 line

2. for IR-Sensor MLX90614, a hwmon class linux driver was

Quoted 2 lines

by Linutronix on our demand. This driver works fine but delivers sporadic the error message "i2c-adapter i2c-0: sendbytes: NAK

Quoted 2 lines

(this message is thrown by the "i2c-algo-bit" driver), or invalid temperature values ( near 0xFFFF). The invalid temperature values

Quoted 1 line

well the error message appear as reponse on bus timeout situations

Quoted 1 line

are not correctly handled by the linux driver. This we find out

Quoted 3 lines

I2C analyzer. In our opinion these issues come while the i2c communication is disturbed by other tasks and/or interrupt service routines (ISR) which extend the SMB clock over the permitted

Quoted 1 line

leaving the IR-Sensor in an undefined or erroneous state.

It's difficult to answer here without seeing the source code of the MLX90614 driver. What I can say is that values "near 0xFFFF" look

uncaught (negative) error codes carelessly cast to u16. So you

ensure that your driver properly deals with errors returned by the

layer (i2c_transfer and i2c_smbus_*). And if such errors happen, you should print them so that you see what exactly is going on and when.

If the EEPROM works fine, it may depend on the transaction types. I can't comment further because you didn't tell us which driver you

using (eeprom or at24). But it would be interesting to see which transactions fail, and if there is a pattern.

For your investigation, you may be interested in backporting (the i2c-algo-bit-part of) the following patch:

If you hit unexpected timeout conditions, you may want to try again after backporting this fix:

More generally, you may want to change the timeout value and see if

helps - although the default of 100 ms seem reasonable to me.

Note that i2c-algo-bit is CPU-driven. It doesn't sleep, so it

be preempted by regular code, but nothing can be done against interrupts. I see that the MLX90614 has a very short timeout when

is high (45 to 55 us), so receiving an interrupt in this state could indeed be an issue. You may want to try disabling interrupts before raising SCL (except at the end of the transaction, of course) in i2c-algo-bit.

And, as Bill already underlined, you have to ensure that you're

the bus at the right frequency. The MLX90614 is an SMBus compliant device so it wants a clock between 10 and 100 kHz. This means a

value between 5 and 50.

Quoted 2 lines

The address mentioned in the driver source "Haavard Skinnemoen <[email protected]>" invalid (unknown)

I presume Haavard left Atmel meanwhile. Not much we can do about

except removing his address from the source tree (I will do.)

Quoted 8 lines

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> 30.11.2010 18:21 >>>

It's difficult to answer here without seeing the source code of the MLX90614 driver. What I can say is that values "near 0xFFFF" look like uncaught (negative) error codes carelessly cast to u16. So you should ensure that your driver properly deals with errors returned by the i2c layer (i2c_transfer and i2c_smbus_*). And if such errors happen, you should print them so that you see what exactly is going on and when.

I can provide the sources for the MLX90614 driver, but I think it is not a good ideea to attach it to this E-Mail.

Quoted 4 lines

If the EEPROM works fine, it may depend on the transaction types. I can't comment further because you didn't tell us which driver you were using (eeprom or at24). But it would be interesting to see which transactions fail, and if there is a pattern.

I access the eeprom as generic i2c device (file pointer to i2c-0) without usage of any specific driver.

Quoted 7 lines

Note that i2c-algo-bit is CPU-driven. It doesn't sleep, so it shouldn't be preempted by regular code, but nothing can be done against interrupts. I see that the MLX90614 has a very short timeout when SCL is high (45 to 55 us), so receiving an interrupt in this state could indeed be an issue. You may want to try disabling interrupts before raising SCL (except at the end of the transaction, of course) in i2c-algo-bit.

Please give me an example how the disable interrupts as you suggest.

In the i2c-algo-bits there is used the "bit_dbg" makro to print some debug messages to ksys. Removing the makros wich where placed on the main execution line (not on for error messages) helps to get a better
behavoir: SCL stretching occure on better reproducable communication times.

Quoted 4 lines

And, as Bill already underlined, you have to ensure that you're running the bus at the right frequency. The MLX90614 is an SMBus compliant device so it wants a clock between 10 and 100 kHz. This means a udelay value between 5 and 50.

I checked the clock speed, It can be changed in the specified limits for MLX90614 with similar results in the data output.
Using an I2C analyzer I was able to see that SCL stretching occures on unpredictable times. If these SCL stretching is on specific point in the communication process or match the one of the SMbus timeout conditions (2 different timeout values) unpredictable data is output, with out any error from the I2C subsystem. Only the "i2c-adapter i2c-0: sendbytes:
NAK bailout." message is correctly thrown.

I can provide screeshots which ilustrate the behavoir. How can I make these sceenshots available for you?

Quoted 3 lines

I presume Haavard left Atmel meanwhile. Not much we can do about that, except removing his address from the source tree (I will do.)

the last entry @vger.kernel.org was in 2007

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> schrieb am Donnerstag, 2.

Hi Matthias,

Quoted 2 lines · Matthias Zacharias

Quoted 2 lines

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> 30.11.2010 18:21 >>>

It's difficult to answer here without seeing the source code of

Quoted 2 lines

Quoted 2 lines

MLX90614 driver. What I can say is that values "near 0xFFFF" look like uncaught (negative) error codes carelessly cast to u16. So

Quoted 1 line

Quoted 1 line

should ensure that your driver properly deals with errors

Quoted 1 line

Quoted 1 line

by the i2c layer (i2c_transfer and i2c_smbus_*). And if such

Quoted 1 line

Quoted 1 line

happen, you should print them so that you see what exactly is

Quoted 3 lines

Quoted 1 line

on and when.

I can provide the sources for the MLX90614 driver, but I think it

Quoted 1 line

not a good ideea to attach it to this E-Mail.

If the source code is publicly visible somewhere, you can point us

that location. If not, maybe this is the right time to publish it ;)

you can send it to me privately.

BTW, it would be very interesting to see if you can get a MLX90614

to work with this driver on another system with a different I2C or SMBus master.

Quoted 2 lines

Quoted 2 lines

If the EEPROM works fine, it may depend on the transaction types.

Quoted 6 lines

Quoted 3 lines

can't comment further because you didn't tell us which driver you were using (eeprom or at24). But it would be interesting to see which transactions fail, and if there is a pattern.

I access the eeprom as generic i2c device (file pointer to i2c-0) without usage of any specific driver.

Which system calls and transaction types are you using?

Did you try accessing the MLX90614 the same way?

Quoted 10 lines

Quoted 8 lines

Note that i2c-algo-bit is CPU-driven. It doesn't sleep, so it shouldn't be preempted by regular code, but nothing can be done against interrupts. I see that the MLX90614 has a very short timeout when SCL is high (45 to 55 us), so receiving an interrupt in this state could indeed be an issue. You may want to try disabling interrupts before raising SCL (except at the end of the transaction, of course) in i2c-algo-bit.

Please give me an example how the disable interrupts as you

Err. I thought there was a kernel function for that, but apparently

was wrong. Well this could be done in asm but I don't think you want

try that.

Maybe you can do something with spin_lock_irqsave() and spin_unlock_irqrestore() around the critical sections, I'm not sure.

Quoted 1 line

In the i2c-algo-bits there is used the "bit_dbg" makro to print

Quoted 1 line

debug messages to ksys. Removing the makros wich where placed on

Quoted 1 line

main execution line (not on for error messages) helps to get a

Quoted 1 line

behavoir: SCL stretching occure on better reproducable

Quoted 1 line

times.

Err, are you by any chance running a kernel built with CONFIG_I2C_DEBUG_ALGO=y, and have set i2c_debug to 2 or more? This could explain your problems, at least in part. i2c-algo-bit is quite verbose in the kernel logs when debugging level is set to 2 or more, and writing the log to the disk is adding a lot of latency to the system.

Unfortunately this is one case where enabling debugging to better understand what is going on also affects what is going on. As you

an external bus analyzer, it's better to not enable debugging in i2c-algo-bit, or at least not beyond level 1.

When CONFIG_I2C_DEBUG_ALGO isn't set, bit_dbg() is a no-op so it certainly can't affect you in any way.

Quoted 7 lines

Quoted 5 lines

And, as Bill already underlined, you have to ensure that you're running the bus at the right frequency. The MLX90614 is an SMBus compliant device so it wants a clock between 10 and 100 kHz. This means a udelay value between 5 and 50.

I checked the clock speed, It can be changed in the specified

Quoted 2 lines

for MLX90614 with similar results in the data output. Using an I2C analyzer I was able to see that SCL stretching occures

Quoted 1 line

unpredictable times. If these SCL stretching is on specific point in

Quoted 1 line

communication process or match the one of the SMbus timeout

Quoted 1 line

(2 different timeout values) unpredictable data is output, with out

Quoted 1 line

error from the I2C subsystem. Only the "i2c-adapter i2c-0:

Quoted 1 line

NAK bailout." message is correctly thrown.

Question is whether the SCL line is stretched by the master or by

slave. Both are allowed to do it to a certain point.

Does stretching happen on SCL high, or SCL low, or both?

If the stretching is done by the master, then you may search for interrupt sources on your system. Maybe you have a misbehaving

or device on your system which causes repeated and long

Making these interruptions shorter (e.g. by moving the real work to

workqueue) would then help.

Quoted 2 lines

I can provide screeshots which ilustrate the behavoir. How can I

Quoted 1 line

these sceenshots available for you?

Either put it on some publicly available web server if you have one at hand; or send them to me by mail privately.

Quoted 5 lines

Quoted 4 lines

I presume Haavard left Atmel meanwhile. Not much we can do about that, except removing his address from the source tree (I will do.)

the last entry @vger.kernel.org was in 2007

Not true. He posted until September 2010:
http://marc.info/?l=linux-kernel&m=128498842805216&w=2

Said post confirms that he left Atmel, BTW. Too bad he did not

or update his address in the tree at that time.

Quoted 7 lines · Matthias Zacharias

Quoted 2 lines

I can provide the sources for the MLX90614 driver, but I think it is not a good ideea to attach it to this E-Mail.

If the source code is publicly visible somewhere, you can point us to that location. If not, maybe this is the right time to publish it ;) Or you can send it to me privately.

Where is a good place to publish the MLX90614 driver?

Quoted 3 lines

BTW, it would be very interesting to see if you can get a MLX90614 chip to work with this driver on another system with a different I2C or SMBus master.

Any suggestion how to do that? I don't know how to get other hardware which has an accessible SMBus to connect the MLX90614 directly.

Quoted 5 lines

Quoted 3 lines

(...)
I access the eeprom as generic i2c device (file pointer to i2c-0) without usage of any specific driver.

Which system calls and transaction types are you using?

The MLX90614 uses now only the i2c_smbus_xfer(..) transaction to read from the SMBus. But I think the MLX90614 driver must be extended to be able to write configuration data (Word) on SMBus device as well.

Quoted 1 line

Did you try accessing the MLX90614 the same way?

The MLX90614 is accessed as a hwmon device using the new device model

Quoted 7 lines

(...)
Err. I thought there was a kernel function for that, but apparently I was wrong. Well this could be done in asm but I don't think you want to try that.

Maybe you can do something with spin_lock_irqsave() and spin_unlock_irqrestore() around the critical sections, I'm not sure.

I have no experience with spin_lock's, don't know how to use them. If you have a good example please point me to it.

Quoted 4 lines

(...)
When CONFIG_I2C_DEBUG_ALGO isn't set, bit_dbg() is a no-op so it certainly can't affect you in any way.

I don't know how the compiler do handle the bit_dbg() makro, but I can see the difference if the bit_dbg() call where included in the code or not, also if CONFIG_I2C_DEBUG_ALGO was not set.

Quoted 2 lines

Question is whether the SCL line is stretched by the master or by the slave. Both are allowed to do it to a certain point.

in the board settings I configure the SCL signal as output only to avoid that SCL streching can be done by the SMBus slaves.

Quoted 1 line

Does stretching happen on SCL high, or SCL low, or both?

The streching occures on both levels at un predictable times.

Quoted 5 lines

If the stretching is done by the master, then you may search for interrupt sources on your system. Maybe you have a misbehaving driver or device on your system which causes repeated and long interruptions. Making these interruptions shorter (e.g. by moving the real work to a workqueue) would then help.

I have no ideea how to find a missbehaving driver, if any?

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> schrieb am Montag, 6. Dezember

Hi Matthias,

Quoted 2 lines · Matthias Zacharias

Quoted 2 lines · Matthias Zacharias

Quoted 1 line

I can provide the sources for the MLX90614 driver, but I think

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

is not a good ideea to attach it to this E-Mail.

If the source code is publicly visible somewhere, you can point

Quoted 4 lines

Quoted 2 lines

to that location. If not, maybe this is the right time to publish it ;) Or you can send it to me privately.

Where is a good place to publish the MLX90614 driver?

The lm-sensors list would be a good place, as indicated in section "HARDWARE MONITORING" of file MAINTAINERS in the kernel source tree.

you intend to get this driver in the upstream kernel tree, you'll

to post it there sooner or later anyway (even though the device is

of a temperature sensor than an actual hardware monitoring chip.)

Quoted 1 line

Quoted 1 line

BTW, it would be very interesting to see if you can get a

Quoted 4 lines

Quoted 2 lines

chip to work with this driver on another system with a different I2C or SMBus master.

Hi, jean
Any suggestion how to do that? I don't know how to get other

Quoted 1 line

which has an accessible SMBus to connect the MLX90614 directly.

Some boards have SMBus pins accessible to connect external devices. Otherwise you need a soldering iron and basic electronics skills and equipment.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

(...)
I access the eeprom as generic i2c device (file pointer to

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

without usage of any specific driver.

Which system calls and transaction types are you using?

The MLX90614 uses now only the i2c_smbus_xfer(..) transaction to

Quoted 1 line

from the SMBus. But I think the MLX90614 driver must be extended to

Quoted 1 line

able to write configuration data (Word) on SMBus device as well.

My question was related to your access to the EEPROM access, not the MLX90614. Besides, i2c_smbus_xfer() is a generic function for all

transaction types, so you didn't completely answer my question.

SMBus transaction types are you asking i2c_smbus_xfer() for?

Quoted 3 lines

Quoted 1 line

Did you try accessing the MLX90614 the same way?

The MLX90614 is accessed as a hwmon device using the new device

I know that. My question was, did you try an alternative way? For example you could use the i2cget command line tool (part of

for user-space access. It would be interesting to see if it makes a difference.

Quoted 2 lines

Quoted 2 lines

(...)
Err. I thought there was a kernel function for that, but

Quoted 1 line

Quoted 1 line

I was wrong. Well this could be done in asm but I don't think you

Quoted 4 lines

Quoted 4 lines

to try that.

Maybe you can do something with spin_lock_irqsave() and spin_unlock_irqrestore() around the critical sections, I'm not

Quoted 2 lines

I have no experience with spin_lock's, don't know how to use them.

Quoted 1 line

you have a good example please point me to it.

I don't have any experience with spin locks either (at least none of the driver I wrote ever used one) but with over 4000 calls to this function throughout the kernel source tree, it shouldn't be

to find an example you can follow.

Quoted 5 lines

Quoted 4 lines

(...)
When CONFIG_I2C_DEBUG_ALGO isn't set, bit_dbg() is a no-op so it certainly can't affect you in any way.

I don't know how the compiler do handle the bit_dbg() makro, but I

Quoted 1 line

see the difference if the bit_dbg() call where included in the code

Quoted 1 line

not, also if CONFIG_I2C_DEBUG_ALGO was not set.

This is highl

bit_dbg is defined with:

#ifdef DEBUG

#define bit_dbg(level, dev, format, args...) \
	do { \
		if (i2c_debug >= level) \
			dev_dbg(dev, format, ##args); \
	} while (0)
#else
#define bit_dbg(level, dev, format, args...) \
	do {} while (0)

#endif /* DEBUG */

And DEBUG is only set if CONFIG_I2C_DEBUG_ALGO=y. So bit_dbg() is a no-op when CONFIG_I2C_DEBUG_ALGO isn't set.

As a matter of fact, I just tried removing all calls to bit_dbg()

i2c-algo-bit.o is unchanged, as I expected. So either you messed up your testing, or your compiler is seriously broken.

Quoted 5 lines

Quoted 2 lines

Question is whether the SCL line is stretched by the master or by the slave. Both are allowed to do it to a certain point.

in the board settings I configure the SCL signal as output only to avoid that SCL streching can be done by the SMBus slaves.

Oh my! You shot yourself in the foot!

How do you expect _slaves_ to change their behavior just because you said your I2C master to not pay attention to clock stretching? If slaves want to stretch the clock line, they will. The MLX90614 datasheet isn't mentioning clock stretching, but that doesn't mean

device is not doing exactly that.

You really want a bidirectional SCL on the master side, otherwise reliability cannot be guaranteed. In fact, i2c-algo-bit with no SCL

callback is not I2C-compliant. It's just a hack because sometimes we have no other way, but in real systems you really don't want to do that. Maybe I should make i2c-algo-bit complain when SCL read

is missing, to make it more obvious.

So please fix this, set a proper callback for SCL read. This alone might fix your problem.

Quoted 3 lines

Quoted 1 line

Does stretching happen on SCL high, or SCL low, or both?

The streching occures on both levels at un predictable times.

This would be on the master side then. Slaves can stretch the clock line by holding it down, but they can't prevent it from going down

the master pulls it low, so they can't stretch the high part of the clock cycle.

Quoted 6 lines

Quoted 5 lines

If the stretching is done by the master, then you may search for interrupt sources on your system. Maybe you have a misbehaving driver or device on your system which causes repeated and long interruptions. Making these interruptions shorter (e.g. by moving the real work to a workqueue) would then help.

Quoted 1 line

I have no ideea how to find a missbehaving driver, if any?

This isn't trivial. You can try unloading each driver sequentially

see if things get better when one driver is missing. You can also

in /proc/interrupts if any value is increasing particularly fast, or

you can find a correlation between SCL problems and a particular interrupt.

Some kernel debug options might be helpful here, such as CONFIG_DETECT_SOFTLOCKUP and CONFIG_LATENCYTOP, and maybe others.

not too familiar with this.

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> schrieb am Samstag, 11. Dezember

Hi Matthias,

Quoted 3 lines · Matthias Zacharias

Hi Jean,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> schrieb am Montag, 6.

Quoted 1 line

2010 um 11:25 in Nachricht

Quoted 3 lines

Quoted 3 lines

I know that. My question was, did you try an alternative way? For example you could use the i2cget command line tool (part of i2c-tools) for user-space access. It would be interesting to see

Quoted 3 lines

Quoted 1 line

it makes a difference.

I don't get the i2c-tools compiled for my armv5l platform. So can't

Quoted 1 line

to work with these tools. Do you know how to get the i2c-tools

Quoted 1 line

on an ARM9 microcontroller.

How do you expect me to help you here? You did not provide any hint

what is failing. I don't have any ARM platform at hand.

Quoted 1 line

I think it's a problem of setting the right configuration.

"Setting the right configuration" is awfully vague. What do you

If you have trouble building i2c-tools, please start a new

thread for that with all the details and I'll look into it.

Quoted 2 lines

Quoted 2 lines

You really want a bidirectional SCL on the master side, otherwise reliability cannot be guaranteed. In fact, i2c-algo-bit with no

Quoted 2 lines

Quoted 2 lines

read callback is not I2C-compliant. It's just a hack because sometimes we have no other way, but in real systems you really

Quoted 1 line

Quoted 1 line

want to do that. Maybe I should make i2c-algo-bit complain when

Quoted 3 lines

Quoted 3 lines

read callback is missing, to make it more obvious.

So please fix this, set a proper callback for SCL read. This

Quoted 3 lines

Quoted 1 line

might fix your problem.

I remove the SCL Output only setting and as open drain line.

Good.

Quoted 2 lines

Then I take some screenshots with the I2C analyzer. using series resistors on the SMBus I could visualize which of the devices

Quoted 1 line

to the SMBus throws the SCL or the SDA. The higher logic-Low-level

Quoted 1 line

thrown by the SMBus slave and the lower from the SMBus-master.

My electronics knowledge is fairly limited, I didn't even know this

possible. So I(ll trust you ;)

Quoted 2 lines

I drop these screenshots on my Dropbox:
http://www.dropbox.com/gallery/16457261/1/I2C_2_MLX90614?h=8e2a46

Interesting. I see that you included both traces from the MLX90614

traces from the EEPROM, and both exhibit the problem. This, and the

that the clock line can be stuck both high and low, clearly points

the master.

If EEPROM access still works fine, this is probably because the

is more tolerant to clock stalls.

If I read the screenshots correctly, the problem only happens when

stalls high, not low, and only for the MLX90614, right?

This would match the timeouts documented in the MLX90614 datasheet:
only 50 µs when SCL is high. This matches the SMBus specification,

while I2C doesn't have any such timeout.

Quoted 2 lines

Where are the these callback functions located? In i2c-algo-bits, i2c-core, or i2c-gpio?

Your code (user-space or kernel driver) calls into i2c-core, which calls i2c-algo-bit. The functions in i2c-algo-bit ultimately call

i2c-gpio, which forwards the requests directly to the low-level GPIO code.

Such timing problems can't be caused by i2c-core, which is way too

level. i2c-gpio is only a middle man between the software

logic (i2c-algo-bit) and the physical level (gpio), so I doubt

can be wrong there. If something is actually wrong in i2c-gpio, it would be in the initial setup (i2c_gpio_probe), not the callbacks. I definitely hope that you already double-checked that the setup data

i2c-gpio is correct (including pdata->sda_is_open_drain and pdata->scl_is_op

So the suspects are i2c-algo-bit and the low-level GPIO code.

For i2c-algo-bit, as I already said, the known issue is that the bus

software-driven, and the code driving the bus can be preempted by an interrupt. I already suggested that you should check what interrupts are happening on your system during the I2C transfers.

The low-level GPIO code is suspect too. i2c-algo-bit assumes that setting SDA or SCL is very fast, and reading them too. If any of the callbacks takes a significant amount of time, then the timing assumptions made by i2c-algo-bit will tear apart. Unfortunately I am

familiar with GPIOs, and I have no idea how to accurately measure

long the GPIO callbacks last.

Actually, what I see on your analyzer screenshots could be caused by either of these two problems.

Quoted 3 lines

(...)
When printk output is called and as well in the debug output functions, the SCL signal is strechted until the output function

Quoted 1 line

finished to send data to the console on ttyS0.

This is correct. But in your case, the problem happens even without printk being ever called, right? So that's not the problem.

Here is a patch I came up with, which I would like you to try. It

interrupts while SCL is high. Please use your I2C analyzer again

this patch applied. What I expect is that you will still see

clocks, but only when SCL is low. The timeout of the MLX90614 is

higher when SCL is low, so I hope that the clock stretching will be properly handled this time and the MLX90614 will work fine.

---
 drivers/i2c/algos/i2c-algo-bit.c |   17 +++++++++++++++++
 include/linux/i2c-algo-bit.h     |    3 +++
 2 files changed, 20 insertions(+)

---

+0100
+++ linux-2.6.27/drivers/i2c/algos/i2c-algo-bit.c 2010-12-11

(quoted diff) +7 -0

@@ -164,15 +165,18 @@ static int i2c_outb(struct i2c_adapter *
 	int sb;
 	int ack;
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+	unsigned long flags;
 
 	/* assert: scl is low */
 	for (i = 7; i >= 0; i--) {
 		sb = (c >> i) & 1;
 		setsda(adap, sb);
 		udelay((adap->udelay + 1) / 2);
+		spin_lock_irqsave(&adap->lockscl, flags);
 		if (sclhi(adap) < 0) { /* timed out */
 			bit_dbg(1, &i2c_adap->dev, "i2c_outb: 0x%02x, "
 				"timeout at bit #%d\n", (int)c, i);
+			spin_unlock_irqrestore(&adap->lockscl, flags);
 			return -ETIMEDOUT;
 		}
 		/* FIXME do arbitration here:

@@ -182,11 +186,14 @@ static int i2c_outb(struct i2c_adapter *
 		 * the whole (combined) message and *NOT* issue STOP.
 		 */
 		scllo(adap);
+		spin_unlock_irqrestore(&adap->lockscl, flags);
 	}
 	sdahi(adap);
+	spin_lock_irqsave(&adap->lockscl, flags);
 	if (sclhi(adap) < 0) { /* timeout */
 		bit_dbg(1, &i2c_adap->dev, "i2c_outb: 0x%02x, "
 			"timeout at ack\n", (int)c);
+		spin_unlock_irqrestore(&adap->lockscl, flags);
 		return -ETIMEDOUT;
 	}
 

@@ -198,6 +205,7 @@ static int i2c_outb(struct i2c_adapter *
 		ack ? "A" : "NA");
 
 	scllo(adap);
+	spin_unlock_irqrestore(&adap->lockscl, flags);
 	return ack;
 	/* assert: scl is low (sda undef) */
 }

@@ -210,19 +218,23 @@ static int i2c_inb(struct i2c_adapter *i
 	int i;

(quoted diff) +1 -0

 			return -ETIMEDOUT;
 		}
 		indata *= 2;
 		if (getsda(adap))
 			indata |= 0x01;
 		setscl(adap, 0);
+		spin_unlock_irqrestore(&adap->lockscl, flags);
 		udelay(i == 7 ? adap->udelay / 2 : adap->udelay);
 	}
 	/* assert: scl is low */

@@ -385,16 +397,20 @@ static int sendbytes(struct i2c_adapter 
 static int acknak(struct i2c_adapter *i2c_adap, int is_ack)
 {

(quoted diff) +2 -0

+		spin_unlock_irqrestore(&adap->lockscl, flags);
 		return -ETIMEDOUT;
 	}
 	scllo(adap);
+	spin_unlock_irqrestore(&adap->lockscl, flags);
 	return 0;
 }
 

+0200
+++ linux-2.6.27/include/linux/i2c-algo-bit.h 2010-12-11

Thanks for your patch. It is working very well.
But one problem is left:

    see "101214_1058_MLX_I2C_patched_0001.bmp" in my dropbox

(http://www.dropbox.com/gallery/16457261/1/I2C_2_MLX90614?h=8e2a46)

If between the read command (0x5a.0x6) and the answer (0x5a <3x data_bytes) the timeout condition: SCL = High for 43µs match, the MLX90614 go thru reset and returns an invalid value (0xFFFF).

Where can I place the call for "spin_lock_irqsave" and "spin_unlock_irqrestore" to block this timeout situation.

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> schrieb am Dienstag, 14. Dezember

Hi Matthias,

Quoted 5 lines · Matthias Zacharias

Thanks for your patch. It is working very well.
But one problem is left:

    see "101214_1058_MLX_I2C_patched_0001.bmp" in my dropbox

(http://www.dropbox.com/gallery/16457261/1/I2C_2_MLX90614?h=8e2a46)

Quoted 6 lines

If between the read command (0x5a.0x6) and the answer (0x5a <3x data_bytes) the timeout condition: SCL = High for 43µs match, the MLX90614 go thru reset and returns an invalid value (0xFFFF).

Where can I place the call for "spin_lock_irqsave" and "spin_unlock_irqrestore" to block this timeout situation.

Ah, I forgot the repeated start condition. The following updated

fixes this. I have also changed the code to no longer call scllo()

the spinlock is held. That way we can release the spinlock (and thus enable interrupts again) faster, which lowers the system latency.

So please give a try to this new patch, and report.

* * * * *

Candidate fix for SCL getting stretched when high resulting in slave timeouts.

Signed-off-by Jean Delvare <[email protected]>

---
 drivers/i2c/algos/i2c-algo-bit.c |   33

 include/linux/i2c-algo-bit.h     |    3 +++
 2 files changed, 32 insertions(+), 4 deletions(-)

---

+0100
+++ linux-2.6.27/drivers/i2c/algos/i2c-algo-bit.c 2010-12-14

drivers/i2c/algos/i2c-algo-bit.c +9 -1

@@ -131,12 +132,17 @@ static void i2c_start(struct i2c_algo_bi
 
 static void i2c_repstart(struct i2c_algo_bit_data *adap)
 {
+	unsigned long flags;
+
 	/* assert: scl is low */
 	sdahi(adap);
+	spin_lock_irqsave(&adap->lockscl, flags);
 	sclhi(adap);
 	setsda(adap, 0);
 	udelay(adap->udelay);
-	scllo(adap);
+	setscl(adap, 0);
+	spin_unlock_irqrestore(&adap->lockscl, flags);
+	udelay(adap->udelay / 2);
 }
 
 

@@ -164,15 +170,18 @@ static int i2c_outb(struct i2c_adapter *
 	int sb;
 	int ack;
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+	unsigned long flags;
 
 	/* assert: scl is low */
 	for (i = 7; i >= 0; i--) {
 		sb = (c >> i) & 1;
 		setsda(adap, sb);
 		udelay((adap->udelay + 1) / 2);
+		spin_lock_irqsave(&adap->lockscl, flags);
 		if (sclhi(adap) < 0) { /* timed out */
 			bit_dbg(1, &i2c_adap->dev, "i2c_outb: 0x%02x, "
 				"timeout at bit #%d\n", (int)c, i);
+			spin_unlock_irqrestore(&adap->lockscl, flags);
 			return -ETIMEDOUT;
 		}
 		/* FIXME do arbitration here:

drivers/i2c/algos/i2c-algo-bit.c +6 -1

+		udelay(adap->udelay / 2);
 	}
 	sdahi(adap);
+	spin_lock_irqsave(&adap->lockscl, flags);
 	if (sclhi(adap) < 0) { /* timeout */
 		bit_dbg(1, &i2c_adap->dev, "i2c_outb: 0x%02x, "
 			"timeout at ack\n", (int)c);
+		spin_unlock_irqrestore(&adap->lockscl, flags);
 		return -ETIMEDOUT;
 	}
 

@@ -197,7 +210,9 @@ static int i2c_outb(struct i2c_adapter *
 	bit_dbg(2, &i2c_adap->dev, "i2c_outb: 0x%02x %s\n", (int)c,
 		ack ? "A" : "NA");
 
-	scllo(adap);
+	setscl(adap, 0);
+	spin_unlock_irqrestore(&adap->lockscl, flags);
+	udelay(adap->udelay / 2);
 	return ack;
 	/* assert: scl is low (sda undef) */
 }

@@ -210,19 +225,23 @@ static int i2c_inb(struct i2c_adapter *i
 	int i;

drivers/i2c/algos/i2c-algo-bit.c +2 -0

 				"#%d\n", 7 - i);
+			spin_unlock_irqrestore(&adap->lockscl, flags);
 			return -ETIMEDOUT;
 		}
 		indata *= 2;
 		if (getsda(adap))
 			indata |= 0x01;
 		setscl(adap, 0);
+		spin_unlock_irqrestore(&adap->lockscl, flags);
 		udelay(i == 7 ? adap->udelay / 2 : adap->udelay);
 	}
 	/* assert: scl is low */

@@ -385,16 +404,21 @@ static int sendbytes(struct i2c_adapter 
 static int acknak(struct i2c_adapter *i2c_adap, int is_ack)
 {

drivers/i2c/algos/i2c-algo-bit.c +4 -1

+		spin_unlock_irqrestore(&adap->lockscl, flags);
 		return -ETIMEDOUT;
 	}
-	scllo(adap);
+	setscl(adap, 0);
+	spin_unlock_irqrestore(&adap->lockscl, flags);
+	udelay(adap->udelay / 2);
 	return 0;
 }
 

+0100
+++ linux-2.6.27/include/linux/i2c-algo-bit.h 2010-12-14

Thank you for the new patch.
I tried it and it works fine for our system with i2c_debug < 2.

For i2c_debug >=2 the debug outputs leads to timeout condition when address should be acknowledged.

I'll try to publish the "mlx90614" driver after doing some optimizations. Are you interested to review this driver before, because it is the first driver we try to publish?

Do plan to include your patch on the "i2c-algo-bit" into the lastest kernel?

I had applied the patch and done my tests on the 2.6.25.4 kernel, with a bunch of other patches needed for my system.

Thanks a lot for your fast and good support for fixing this issue we had.

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> schrieb am Mittwoch, 15. Dezember

Hallo Matthias,

Quoted 2 lines · Matthias Zacharias

Thank you for the new patch.
I tried it and it works fine for our system with i2c_debug < 2.

Great, thanks for the feedback.

Quoted 2 lines

For i2c_debug >=2 the debug outputs leads to timeout condition when address should be acknowledged.

Hmm, calling printk() with the spinlock held wasn't the smartest

to do, I guess. Updated patch attached, I've made sure to always release the spinlock before calling bit_dbg(), hopefully it should

your last issue.

Quoted 2 lines

I'll try to publish the "mlx90614" driver after doing some optimizations. Are you interested to review this driver before,

Quoted 1 line

it is the first driver we try to publish?

Just publish it as is, and I'll review it publicly. It will

take a number of round trips to get it right, if this is your first contribution, but that's OK.

FYI, I have asked Melexis for an MLX90614 evaluation board, they

sent something to me, so as soon as I receive it I should be able to test your driver.

Quoted 1 line

Do plan to include your patch on the "i2c-algo-bit" into the

Quoted 1 line

kernel?

Yes. Please test the new version of the patch, and if it works OK

you, I'll schedule it for merge in kernel 2.6.38. For 2.6.37 it's

late, the patch is quite intrusive and i2c-algo-bit is used by many many drivers including very popular ones, so I can't merge it that

in the release cycle.

Quoted 1 line

I had applied the patch and done my tests on the 2.6.25.4 kernel,

Quoted 3 lines

a bunch of other patches needed for my system.

Thanks a lot for your fast and good support for fixing this issue

Quoted 1 line

had.

You're welcome.

With the new patch the driver is working fine, also with i2c_debug = 3.

See the screenshot in my dropbox
(http://www.dropbox.com/gallery/16457261/1/I2C_2_MLX90614?h=8e2a46)

Quoted 5 lines

(...)
Just publish it as is, and I'll review it publicly. It will certainly take a number of round trips to get it right, if this is your first contribution, but that's OK.

This MLX90614 driver has a reduced functionality. It was tailored for our application which use only the temperature read function. Also the SMBus address is fix defined in the source code, and no checkes if the factory preprogramming is correct are done. The option to manage the configuration of the MLX90614 as described in the datasheets is not yet implemented, because we don't need it.

Quoted 4 lines

(...)
FYI, I have asked Melexis for an MLX90614 evaluation board, they have sent something to me, so as soon as I receive it I should be able to test your driver.

The evalutionboard (EVB) shiped by Melexis uses his own microcontroller on that board and communicate to the PC via USB. In our system the MLX90614 sensor is connected directly to the port pins of the microcontroller.

Quoted 1 line

Quoted 1 line

Jean Delvare <[email protected]> schrieb am Donnerstag, 16.

Quoted 1 line

With the new patch the driver is working fine, also with i2c_debug

Quoted 4 lines

3.

See the screenshot in my dropbox
(http://www.dropbox.com/gallery/16457261/1/I2C_2_MLX90614?h=8e2a46)

Looks good, great. I'll send the patch for review and comments to

the linux-i2c list and LKML later today.

Do you want me to list you as a tester in the patch header? If I do that, I'll have to include your name and e-mail address, so I need

know if this is OK with you.

Quoted 3 lines

Quoted 3 lines

(...)
Just publish it as is, and I'll review it publicly. It will certainly take a number of round trips to get it right, if this

Quoted 4 lines

Quoted 2 lines

your first contribution, but that's OK.

This MLX90614 driver has a reduced functionality. It was tailored

Quoted 4 lines

our application which use only the temperature read function. Also the SMBus address is fix defined in the source code, and no checkes if the factory preprogramming is correct are done. The option to manage the configuration of the MLX90614 as described

Quoted 1 line

the datasheets is not yet implemented, because we don't need it.

This is totally OK. Others can add functionalities to the driver

if needed.

Quoted 6 lines

Quoted 4 lines

(...)
FYI, I have asked Melexis for an MLX90614 evaluation board, they have sent something to me, so as soon as I receive it I should be able to test your driver.

The evalutionboard (EVB) shiped by Melexis uses his own

Quoted 3 lines

on that board and communicate to the PC via USB. In our system the MLX90614 sensor is connected directly to the port pins of the microcontroller.

I know that. I don't plan to actually use the micro-controller on

evaluation board. There are I2C pins on the board, and I'll connect

external I2C master [1] to them.

[1] http://www.harbaum.org/till/i2c_tiny_usb/index.shtml