Claude review: rust: add basic serial device bus abstractions

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

Patch Review

This is the main patch adding `rust/kernel/serdev.rs`, C helpers, Kconfig, and bindings.

**1. Typo in `Parity::Even` doc (line 47 of serdev.rs):**

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

"partiy" should be "parity".

**2. `Timeout::into_jiffies` — silent overflow to infinite wait (lines 66–74):**

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

`Jiffies` is `c_ulong` (u64 on 64-bit). If a caller passes a `NonZero<Jiffies>` value exceeding `isize::MAX`, `try_into()` fails and `unwrap_or_default()` returns 0. Since `serdev_device_write()` in C treats `timeout == 0` as `MAX_SCHEDULE_TIMEOUT`, this silently converts a large-but-finite timeout into an *infinite* timeout. The user explicitly used `Timeout::Jiffies(...)` — they did not ask for `Timeout::Max`. Consider saturating to `isize::MAX` instead:

```rust
Self::Jiffies(value) => value.get().try_into().unwrap_or(isize::MAX),
```

This would cap at the largest representable finite timeout rather than silently going infinite. The same applies to the `Milliseconds` arm.

**3. `Timeout` doc string is misleading (line 53):**

```rust
/// Timeout in Jiffies.
pub enum Timeout {
```

The enum supports Jiffies, Milliseconds, and Max. The doc should say something like "Timeout for serial device operations."

**4. `write_all()` — theoretical i32 truncation of ssize_t (lines 489–504):**

```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(),
        )
    };
    to_result(ret as i32).map(|()| ret.unsigned_abs())
}
```

`serdev_device_write` returns `ssize_t` (i64 on 64-bit). The cast `ret as i32` truncates on 64-bit. If the return value were in the range `[0x80000000, 0xFFFFFFFF]` (2–4 GB), the truncated i32 would be negative and `to_result` would incorrectly treat it as an error code. This is academic for serial devices (serial writes are never that large), but it's a latent correctness issue. The comment "negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`" only covers the error case. A cleaner pattern would be:

```rust
if ret < 0 {
    return to_result(ret as i32).map(|()| 0);
}
Ok(ret as usize)
```

**5. `write()` doc — missing `[` in cross-reference (line 513):**

```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`]` (missing opening backtick-bracket).

**6. `unbind()` doc — incomplete sentence (line 403):**

```rust
/// This callback serves as a place for drivers to perform teardown operations that require a
/// `&Device<Core>` or `&Device<Bound>` reference. For instance.
```

"For instance." is an incomplete sentence. Either provide an example or remove it.

**7. `remove_callback` does not drop PrivateData (lines 223–244):**

```rust
extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
    let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
    let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
    // ... calls T::unbind, but never calls drvdata_obtain to free PrivateData
}
```

This relies on the driver-lifetime framework to drop PrivateData after the remove callback returns. This is correct per the framework design, but worth noting that the driver data (`T::Data`) is dropped via `PinnedDrop` on `PrivateData`, not via `unbind`. The ordering is: `remove_callback` calls `unbind` → framework drops `PrivateData` → `PinnedDrop` drops `T::Data` then closes the device. This is fine.

**8. Probe: receive_buf can fire before T::probe completes (lines 194–210):**

The probe sequence is: set drvdata → set client ops → open device → call `T::probe`. Since `active` starts as `false`, any `receive_buf_callback` firing between `serdev_device_open` and probe completion will see `!*active` and return `length` (discarding data). This is intentional and matches the v11 changelog ("simplify abstraction by removing ability to receive the initial transmission"). Just noting this is a design choice visible to driver authors — data arriving during probe is silently consumed and discarded.

**9. C helpers look correct:**

The three helpers (`serdev_device_driver_unregister`, `serdev_device_put`, `serdev_device_set_client_ops`) wrap inline functions / macros that can't be called directly from Rust bindgen. These are straightforward and correct.

**10. `Device<Bound>` but not `Device<Core>` for serial ops:**

All the serial operations (`set_baudrate`, `set_flow_control`, `write`, etc.) are only available on `Device<Bound>`. However, the `probe` callback receives `Device<Core<'_>>` and `Core` derefs to `Bound`, so the driver can call these methods during probe. This is consistent with the driver-lifetime pattern.

---

---
Generated by Claude Code Patch Reviewer