On Mon, Feb 20, 2012 at 07:22:00PM +0800, Daniel Kurtz wrote:
On Feb 15, 2012 6:48 PM, "Daniel Vetter" daniel@ffwll.ch wrote:
On Thu, Feb 09, 2012 at 12:03:17PM -0800, Benson Leung wrote:
gmbus_xfer with a single message (particularly a single message write) would set Bus Cycle Select to 100b, the Gen Stop cycle, instead of 101b, No Index, Stop cycle. This would not start single message i2c transactions.
Also, gmbus_xfer done: will disable the interface without checking if it is idle. In the case of writes, there will be no wait on status or delay to ensure the write starts and completes before the interface is turned off.
Fixed the former issue by using the same cycle selection as used in the I2C_M_RD for the write case. GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0) Fixed the latter by waiting on GMBUS_ACTIVE to deassert before disable.
Signed-off-by: Benson Leung bleung@chromium.org
Reviewed-by: Daniel Kurtz djkurtz@chromium.org
Can you clarify the commit message a bit and say that the first hunk is just for optics and the issue is only with the write path (because the read path is correct already). Silly me is just to easily confused ;-)
Btw, I've reworked the gmbus -> gpio bit-banging fallback code a bit and if that passes review and all I'll reenable gmbus by default again. See
If the write case is fixed by Benson's patch, is there any known use case that still requires i2c bit banging on these pins? It would be a nice cleanup to remove it completely.
Let's first see how much things blow up when re-enabling gmbus again ;-)
Also, I can think of at least two further potential performance improvements that I was wondering if anybody has yet pursued: (1) Enabling the i915's gmbus interrupt. This would eliminate the need for the (relatively slow) wait_for polling loop. (2) Taking advantage of the i915's "INDEX" cycles to combine writing a (1 or 2 byte) address & reading back an array of bytes into a single transaction.
Afaik no one looked into this, but patches are highly welcome.
Cheers, Daniel