Claude review: rust: add basic serial device bus abstractions

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

Patch Review

**Typo in `Parity` enum doc comment:**

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

Should be "Even parity." (typo: "partiy" → "parity").

**`Timeout::into_jiffies` returns `isize` but C functions take `long`:**

The `into_jiffies` method returns `isize`, which matches Rust's `c_long` on LP64 platforms. However, the `unwrap_or_default()` fallback for the `Jiffies` and `Milliseconds` variants returns 0 on overflow, which silently maps to `MAX_SCHEDULE_TIMEOUT` (wait forever). This is unlikely in practice but surprising — a caller providing a specific timeout would get an infinite wait if the conversion overflowed. Worth a comment or documenting the behavior.

**`write()` return type is `Result<u32>` but `write_buf` returns `int`:**

```rust
pub fn write(&self, data: &[u8]) -> Result<u32> {
    let ret =
        unsafe { bindings::serdev_device_write_buf(self.as_raw(), data.as_ptr(), data.len()) };
    to_result(ret as i32).map(|()| ret.unsigned_abs())
}
```

The C function `serdev_device_write_buf()` returns `int`. The `ret` here would be `c_int` (i32). Calling `ret.unsigned_abs()` on a `c_int` returns `u32`, which is fine. But `to_result(ret as i32)` followed by `ret.unsigned_abs()` — on success, `ret` could be 0 (zero bytes written), and `to_result(0)` returns `Ok(())`, so `unsigned_abs()` would return 0. That's correct. Looks sound.

**`write_all()` return type cast comment says "negative return values":**

```rust
// 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`. On success it returns a positive `size_t` value cast to `ssize_t`. The `ret as i32` cast is safe for error codes, but for large successful writes (> 2^31 bytes), truncating `ssize_t` to `i32` could wrap to negative and be incorrectly treated as an error. In practice, serial writes are small, but this is technically unsound for very large writes. Since serdev buffer sizes are realistically bounded well below `i32::MAX`, this is acceptable, but a defensive comment would be worthwhile.

**`Device<Bound>` methods but `probe` uses `Device<Core<'_>>`:**

`set_baudrate`, `set_flow_control`, `set_parity`, `write_all`, `write`, `write_flush`, `wait_until_sent` are all only available on `Device<Bound>`. But `probe` receives `Device<Core<'_>>`, and `Core` derefs to `Bound`, so the sample can call `sdev.set_baudrate(...)` through auto-deref. This is correct and follows the device context pattern.

**`receive_buf_callback` holding mutex across `T::receive`:**

```rust
let active = private_data.active.lock();
if !*active {
    return length;
}
// ...
T::receive(sdev, data_pinned, buf)
```

The lock on `active` is held for the entire duration of `T::receive`. This means `receive_buf` and `PinnedDrop` are mutually exclusive, which is the intended safety property. However, this means `T::receive` runs under a mutex — the C documentation says `receive_buf` "may sleep", so using a `Mutex` (which is sleepable in the kernel) is correct here. Good design.

**When `!*active`, returning `length` (consuming all data):**

```rust
if !*active {
    return length;
}
```

This silently discards all received data when the driver has been deactivated. This is reasonable — there's no meaningful consumer for the data anymore — but worth noting that the C serdev core will consider this data "consumed."

**Missing `[ ]` bracket in doc comment:**

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

The opening bracket is separated from the backtick: `[ Device::wait_until_sent`]` should be `[`Device::wait_until_sent`]`. This will produce a broken rustdoc link.

**`AlwaysRefCounted` `dec_ref` implementation:**

```rust
unsafe fn dec_ref(obj: NonNull<Self>) {
    unsafe { bindings::serdev_device_put(obj.cast().as_ptr()) }
}
```

This casts `NonNull<Device>` to `NonNull<bindings::serdev_device>` via `.cast()`. Since `Device` is `#[repr(transparent)]` wrapping `Opaque<bindings::serdev_device>`, the cast is valid.

**`Sync` for `Device<Bound>` but not for other contexts explicitly:**

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

`Device` (i.e., `Device<Normal>`) and `Device<Bound>` both get `Sync`. `Device<Core<'_>>` and `Device<CoreInternal<'_>>` do not have explicit `Sync` impls. Since `Core` and `CoreInternal` are only used during probe/remove (single-threaded contexts), this seems intentional and correct.

**`probe_callback` acquires and dismisses `ScopeGuard` for cleanup:**

The probe error path is well structured:
1. `set_drvdata` installs `PrivateData`
2. `ScopeGuard` is created to clean up via `drvdata_obtain` if probe fails
3. If `T::probe` (the `__pinned_init` call) succeeds, `dismiss()` prevents cleanup
4. `*active = true` is set only on success

This correctly handles the memory leak fix mentioned in the v11 changelog.

---
Generated by Claude Code Patch Reviewer