Claude review: drm/tyr: move probe resources into registration data

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

Patch Review

**driver.rs — TyrDrmDeviceData emptied and TyrDrmRegistrationData introduced**

The core restructuring is clean. `TyrDrmDeviceData` becomes a unit struct and all resources move to the new `TyrDrmRegistrationData<'bound>`:

```rust
#[pin_data]
pub(crate) struct TyrDrmDeviceData;

#[pin_data]
pub(crate) struct TyrDrmRegistrationData<'bound> {
    pub(crate) pdev: ARef<platform::Device>,
    pub(crate) fw: Arc<Firmware<'bound>>,
    #[pin]
    clks: Mutex<Clocks>,
    #[pin]
    regulators: Mutex<Regulators>,
    pub(crate) iomem: Arc<IoMem<'bound>>,
    pub(crate) gpu_info: GpuInfo,
}
```

The `ForLt` implementation is the standard pattern for the Rust DRM lifetime framework:

```rust
impl ForLt for TyrDrmRegistrationData<'static> {
    type Of<'bound> = TyrDrmRegistrationData<'bound>;
}
```

**driver.rs — probe() restructuring**

The probe sequence now creates the DRM device first (unregistered), then creates auxiliary objects, then builds registration data and registers:

```rust
let dev_data = try_pin_init!(TyrDrmDeviceData {});
let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, dev_data)?;

let mmu = Mmu::new(iomem.as_arc_borrow(), &gpu_info)?;
let firmware = Firmware::new(pdev, iomem.clone(), &unreg_dev, mmu.as_arc_borrow(), &gpu_info)?;
```

**Concern: `Mmu` created but never stored.** The `Mmu` is constructed (which logs the address space slot count) and passed as `ArcBorrow` to `Firmware::new()`, but `Firmware` ignores it (`_mmu` parameter). The `Arc<Mmu>` is then dropped when `mmu` goes out of scope. This means the MMU object exists only transiently for its logging side effect. Consider either:
- Not creating it here at all (defer to when it's actually needed), or
- Storing it in `TyrDrmRegistrationData` if it needs to outlive probe

The current approach creates an object just to drop it, which is confusing even for a stub.

**driver.rs — Safety comment on `new_with_lt`**

```rust
// SAFETY: `reg` is stored in the platform driver data and is not leaked or
// forgotten, so it is dropped before the `'bound` registration data can become
// invalid.
```

The safety argument is correct in substance — the `Registration` is stored in `TyrPlatformDriverData` which is the platform driver data, dropped when the device unbinds. However, it would be slightly more precise to state that the `Registration` is dropped before `'bound` ends because the platform framework guarantees `TyrPlatformDriverData` is dropped during unbind, which happens while the device is still bound.

**driver.rs — IoMem wrapping change**

```rust
-        let iomem = request.iomap_sized::<SZ_2M>()?;
+        let iomem = Arc::new(request.iomap_sized::<SZ_2M>()?, GFP_KERNEL)?;
```

This changes from a direct value (or `Arc::pin_init` in the current tree) to `Arc::new`. Since `IoMem` is now shared between `TyrDrmRegistrationData` and `Firmware`, wrapping in `Arc` is correct. Verify that `IoMem` doesn't require pinning — if the base series' `IoMem<'bound>` is `Unpin`, `Arc::new` is fine; if it needs pinning, this should be `Arc::pin_init`.

**driver.rs — Cosmetic blank line removal**

```rust
 pub(crate) type TyrDrmDevice<Ctx = drm::Registered> = drm::Device<TyrDrmDriver, Ctx>;
-
 pub(crate) struct TyrPlatformDriver;
```

Nit: the blank line between unrelated declarations is conventional and aids readability. Consider keeping it.

**file.rs — dev_query uses registration data**

The changes here are a straightforward consequence of moving `gpu_info` to registration data:

```rust
-        ddev: &TyrDrmDevice,
+        _ddev: &TyrDrmDevice,
         _pdev: &platform::Device<device::Bound>,
-        _reg_data: &(),
+        reg_data: &TyrDrmRegistrationData<'_>,
```

and:

```rust
-                    devquery.size = core::mem::size_of_val(&ddev.gpu_info) as u32;
+                    devquery.size = core::mem::size_of_val(&reg_data.gpu_info) as u32;
```

This is correct. The `gpu_info` accessor properly follows the data to its new home.

**fw.rs — Firmware stub**

The stub is clean and clearly documented. A few observations:

```rust
pub(crate) struct Firmware<'bound> {
    _pdev: ARef<platform::Device>,
    _iomem: Arc<IoMem<'bound>>,
}
```

**Minor: redundant `_pdev` reference.** The `pdev` is already held by `TyrDrmRegistrationData`, and `Firmware` is owned by the registration data via `Arc<Firmware<'bound>>`. The firmware doesn't need its own `ARef<platform::Device>` to keep the device alive — the registration data already does that. Unless future firmware code needs to call device APIs directly (likely), in which case keeping it is forward-looking and fine.

```rust
pub(crate) fn boot(&self) -> Result {
    todo!()
}
```

This will panic at runtime if called. Fine for a stub, but ensure no code path can reach it before the implementation is filled in.

**mmu.rs — MMU stub**

```rust
pub(super) struct Mmu<'bound> {
    _bound: PhantomData<&'bound ()>,
}
```

The struct carries the `'bound` lifetime via `PhantomData` even though it currently holds no `'bound`-lifetime resources. This is correct forward-looking design for when MMU will hold `IoMem<'bound>` or similar.

```rust
let present = AS_PRESENT::from_raw(gpu_info.as_present).present().get();
let slot_count: usize = present.count_ones().try_into()?;
pr_info!("MMU: {} address space slots present", slot_count);
```

**Nit: `pr_info!` vs `dev_info!`.** Kernel driver messages should use `dev_info!` to include the device name for multi-device systems. The `Mmu::new()` would need to accept a device reference to do this. Consider adding `pdev: &platform::Device` or `dev: &Device` as a parameter.

**Nit: `try_into()` is infallible here.** `u32::count_ones()` returns `u32`, and `u32` → `usize` never fails on any Linux-supported architecture (usize ≥ 32 bits). Using `as usize` would be simpler and avoids an unnecessary error path, though `try_into()` is the safer Rust idiom.

**tyr.rs — module declarations**

```rust
+mod fw;
 mod gem;
 mod gpu;
+mod mmu;
```

Straightforward module additions, properly sorted alphabetically.

**Overall: the patch is well-structured and correct for its stated purpose of establishing lifetime scaffolding.** The main actionable feedback is the `Mmu` create-and-drop pattern and the `pr_info!` vs `dev_info!` issue.

---
Generated by Claude Code Patch Reviewer