Repository navigation
I2C Passthrough Fixes - #382
Merged
Merged
Conversation
i2c_read picks the register address width based on the value: one byte if it fits, two bytes otherwise. Devices like the CCGX PD controllers always use 16-bit register addresses, even for small values, so add i2c_read_addr16 that always sends two bytes (LSB first). Both share the same implementation, which now takes the already encoded address bytes and logs them instead of the numeric address. Signed-off-by: Daniel Schaefer <dhs@frame.work>
The one byte form was chosen for addr < 0xFF, so 0xFF was sent as two bytes [0xFF, 0x00] and the device saw the wrong register. Signed-off-by: Daniel Schaefer <dhs@frame.work>
There was a problem hiding this comment.
🟡 Changes recommended
The new 16-bit read API can return platform-dependent extra bytes on Windows.
1 open finding
What changed in this PR
Refactors I2C passthrough reads and improves register-address handling.
Changes:
- Corrects
0xFFto use one-byte addressing. - Adds forced two-byte, little-endian register addressing.
| File | Description |
|---|---|
framework_lib/src/chromium_ec/i2c_passthrough.rs |
Adds shared read logic and address-width handling. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
JohnAZoidberg
marked this pull request as draft
October 14, 2026 03:02
The driver's IOCTL_CROSEC_XCMD reply length is sizeof(CROSEC_COMMAND)
plus the EC response, where CROSEC_COMMAND is a 20 byte (5x UINT32)
header followed by a flexible Data[] array:
requiredReplySize = sizeof(CROSEC_COMMAND) + res;
WdfRequestSetInformation(Request, requiredReplySize);
But we sliced cmd.buffer, which already starts after that header, with
the full reply length. So every command returned 20 bytes of leftover
request data after the response. With METHOD_BUFFERED the input and
output buffer are the same, so those bytes are whatever we just sent.
That's where the mysterious +20 in assert_win_len and in
send_command_extra came from. Subtract the header instead, so the
Windows driver returns the same length as portio and cros_ec do, and
drop both workarounds.
It also means a failed I2C read no longer looks like it returned data.
On a Laptop 16 whose dGPU retimer is asleep, the read NAKs and returns
nothing, but `framework_tool --versions` reported
Parade Retimers
dGPU: 11.0.1.0
which is just the first four bytes of our own request buffer
(0x11, 0x00, 0x01, 0x00) read back.
Signed-off-by: Daniel Schaefer <dhs@frame.work>
It only differs from i2c_read_impl in the byte order of the register address, so let it share the implementation. This also drops its unconditional truncation to the requested length. That was a workaround for the Windows driver returning extra bytes, which is fixed now. Worse, it could panic: when the device NAKs, the EC stops before the read message and returns only the 2 byte header, see i2c_command_passthru() in common/i2c_passthru.c: args->response_size = sizeof(*resp) + in_len; Returning the short response as-is is what the callers expect anyway, they check i2c_status before looking at the data. Signed-off-by: Daniel Schaefer <dhs@frame.work>
Both chunked readers loop until they have as many bytes as requested. Before, every read grew the buffer because the response was padded to the requested length, on Windows even past it. Now a read can come back short, so bail out instead of retrying the same chunk forever. The EC only returns a short response when it also sets i2c_status, so the preceding is_successful() check should already catch it. This is just to make sure we can't hang. Signed-off-by: Daniel Schaefer <dhs@frame.work>
get_version() returns Ok(None) for "no retimer on this system", which
makes --versions print nothing at all. It also took that path when the
retimer did not return any data, so a dGPU whose retimer is asleep
looked like a system without one.
Decide it by the I2C status the EC reports instead of by how many bytes
came back. Once the EC has told us there's a compatible dGPU we know the
retimer exists, so a NAK is an error, and the caller already prints
Parade Retimers
Unknown
for that. The length based check only worked before because a failed
read still came back padded to the requested size.
Signed-off-by: Daniel Schaefer <dhs@frame.work>
The EC processes passthru messages in order and stops at the first one
that fails, see i2c_command_passthru() in common/i2c_passthru.c. So on
a read (one write message plus one read message) num_msgs can also be
1, when the register address write is ACKed but the read is NAKed.
The old assert allowed only 0 or all messages and would panic a debug
build in that case. Assert the only real invariant instead: the EC
can't process more messages than we sent. That matches what the kernel
checks in ec_i2c_parse_response(), see
drivers/i2c/busses/i2c-cros-ec-tunnel.c:
/* Other side could send us back fewer messages, but not more */
if (resp->num_msgs > *num)
return -EPROTO;
Signed-off-by: Daniel Schaefer <dhs@frame.work>
The check rejects len > MAX_I2C_CHUNK, so 128 is allowed. Say <=128 and reference the constant instead of hardcoding it. Signed-off-by: Daniel Schaefer <dhs@frame.work>
Signed-off-by: Daniel Schaefer <dhs@frame.work>
Express "where does the data buffer start inside CROSEC_COMMAND" directly instead of deriving it from the struct and buffer sizes. No functional change. Signed-off-by: Daniel Schaefer <dhs@frame.work>
JohnAZoidberg
force-pushed
the
i2c-fixes
branch
from
October 14, 2026 04:02
b162c67 to
5e06c48
Compare
JohnAZoidberg
marked this pull request as ready for review
October 14, 2026 04:04
Member
Author
|
Tested a couple of PD and retimer i2c passthrough commands on windows. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.