Claude review: rust: add basic serial device bus abstractions

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This is the main patch (571 lines). Several observations:

**(1) Typo in Parity doc comment:**

```rust
+    /// Even partiy.
+    Even = bindings::serdev_parity_SERDEV_PARITY_EVEN,
```

Should be `Even parity.`

**(2) `Timeout::into_jiffies` silent overflow to "wait forever":**

```rust
+    fn into_jiffies(self) -> isize {
+        match self {
+            Self::Jiffies(value) => value.get().try_into().unwrap_or_default(),
+            Self::Milliseconds(value) => {
+                msecs_to_jiffies(value.get()).try_into().unwrap_or_default()
+            }
+            Self::Max => 0,
+        }
+    }
```

The `unwrap_or_default()` on overflow returns 0, which the C `serdev_device_write` converts to `MAX_SCHEDULE_TIMEOUT` (wait forever). A caller specifying a large but finite timeout would silently get an infinite wait. This is worth noting in the doc or choosing a different fallback (e.g., `isize::MAX`).

The `Self::Max => 0` mapping relies on `serdev_device_write`'s internal `if (timeout == 0) timeout = MAX_SCHEDULE_TIMEOUT;` behavior. This is correct but fragile — it couples to an implementation detail of `serdev_device_write` rather than passing `MAX_SCHEDULE_TIMEOUT` directly. If this function is ever used for other serdev calls that don't do the 0-to-MAX conversion, it would break silently.

**(3) Potential truncation in `write_all`:**

```rust
+    pub fn write_all(&self, data: &[u8], timeout: Timeout) -> Result<usize> {
+        let ret = unsafe {
+            bindings::serdev_device_write(
+                self.as_raw(),
+                data.as_ptr(),
+                data.len(),
+                timeout.into_jiffies(),
+            )
+        };
+        // CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
+        // which always fit into a `i32`.
+        to_result(ret as i32).map(|()| ret.unsigned_abs())
+    }
```

`serdev_device_write` returns `ssize_t` (isize in Rust). The cast `ret as i32` is safe for negative error codes as the comment states, but on 64-bit systems, a successful return value larger than `i32::MAX` (>2GB) would be truncated to a negative i32, causing `to_result` to return a spurious error. While this is extremely unlikely for serial writes, the same pattern appears in `write()` (where `write_buf` returns `int`, so there's no issue there). A more defensive approach might be to check `ret < 0` directly instead of casting through `to_result`.

**(4) Missing bracket in doc comment for `write`:**

```rust
+    /// Note that any accepted data has only been buffered by the controller. Use
+    /// [ Device::wait_until_sent`] to make sure the controller write buffer has actually been
```

Should be `[`Device::wait_until_sent`]` (no space after `[`).

**(5) `write_all` naming may confuse Rust developers:**

In `std::io::Write`, `write_all` guarantees all bytes are written or returns an error. Here, `write_all` can return fewer bytes than `data.len()` (if interrupted). The doc is clear about this, but the name may mislead readers used to the standard library convention. The C function being wrapped is `serdev_device_write` (vs `serdev_device_write_buf`), so alternative names like `write_timeout` might be less surprising.

**(6) Probe/receive synchronization — correct:**

The `PrivateData` with `Completion` + `UnsafeCell<bool>` for synchronizing probe completion with receive_buf is correctly structured:

```rust
+            // SAFETY: We have exclusive access to `private_data.error`.
+            unsafe { *private_data.error.get() = result.is_err() };
+            private_data.probe_complete.complete_all();
```

The `complete_all()` provides the necessary memory barrier so that `wait_for_completion()` in `receive_buf_callback` sees the error flag. The ordering in `probe_callback` is also correct: `rust_private_data` is set before `set_client_ops` and `devm_serdev_device_open`, ensuring the pointer is valid when `receive_buf` can first be called.

**(7) OPS struct wires `write_wakeup` to C function directly:**

```rust
+    const OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
+        receive_buf: if <F::Of<'static> as Driver<'static>>::HAS_RECEIVE {
+            Some(Self::receive_buf_callback)
+        } else {
+            None
+        },
+        write_wakeup: Some(bindings::serdev_device_write_wakeup),
+    };
```

Using `bindings::serdev_device_write_wakeup` directly as the function pointer is fine since it has the right signature. The `receive_buf` is conditionally set based on whether the driver implements `receive`, which is a nice use of vtable detection.

**(8) `Device` Send/Sync impls:**

```rust
+unsafe impl Send for Device {}
+unsafe impl Sync for Device {}
+unsafe impl Sync for Device<device::Bound> {}
```

The `Send` impl only covers `Device` (i.e., `Device<Normal>`). Missing `Send` for `Device<device::Bound>` and other contexts. However, `Device<Bound>` is typically only used by reference within a driver callback, so it may not need `Send`. This is likely intentional but could use a comment.

---

---
Generated by Claude Code Patch Reviewer