Claude review: gpu: nova: add impl_num_enum!() macro to replace FromPrimitive boilerplate

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

Patch Review

**Macro design — if-chain vs match:**

The generated `TryFrom` uses an `if`-chain:

```rust
$(if val == <$enum_type>::$variant as $prim_type {
    return Ok(<$enum_type>::$variant);
})*
Err($err)
```

This is a pragmatic choice since Rust match arm patterns can't contain arbitrary expressions like `Self::Variant as u8`. The if-chain is functionally equivalent and the compiler will optimize it similarly. Acceptable.

**Missing `#[repr(u8)]` on `FalconFbifTarget` — good catch:**

```rust
+#[repr(u8)]
 pub(crate) enum FalconFbifTarget {
```

The original code was already using `impl_from_enum_to_u8!` which did `value as u8` on this enum without an explicit `#[repr(u8)]`. While Rust does define discriminant values for field-less enums, adding the explicit repr is correct and makes the contract clear. This is a legitimate fix bundled into the patch. Consider mentioning it more prominently — perhaps as a separate `Fixes:` or at least calling it out in the subject as a pre-requisite fix rather than "while at it."

**Hardcoded `kernel::error::Error` in the macro:**

```rust
type Error = kernel::error::Error;

fn try_from(val: $prim_type) -> core::result::Result<Self, Self::Error> {
```

The macro hardcodes the full path `kernel::error::Error` and `core::result::Result`. This works, but the existing code used `Error` and `Result` via the prelude (e.g., `type Error = Error;` and `fn try_from(value: u8) -> Result<Self>`). Using fully-qualified paths in macro-generated code is actually more robust and avoids depending on what's in scope at the call site, so this is fine — just noting the style difference.

**`FalconCoreRevSubversion` — manual `From` impl added back:**

```rust
+impl From<FalconCoreRevSubversion> for u8 {
+    fn from(val: FalconCoreRevSubversion) -> u8 {
+        val as u8
+    }
+}
```

Since this enum can't use `impl_num_enum!` (due to the bitmask in `TryFrom`), it needs its own `From` impl. This is correct, but it re-introduces the exact one-off boilerplate that `impl_from_enum_to_u8!` was providing. Consider whether the macro could support a "From-only" mode (e.g., `impl_num_enum!(FalconCoreRevSubversion: u8)` without the variant list) to avoid this manual impl, or alternatively keep `impl_from_enum_to_u8!` around for just this one case. Not a blocker, but worth considering.

**Minor style nit in `FalconCoreRevSubversion` TryFrom refactor:**

The patch also refactors the `TryFrom` for `FalconCoreRevSubversion` from:
```rust
-        let sub_version = match value & 0b11 {
-            0 => Subversion0,
-            ...
-            _ => return Err(EINVAL),
-        };
-        Ok(sub_version)
```
to:
```rust
+        match value & 0b11 {
+            0 => Ok(Subversion0),
+            ...
+            _ => Err(EINVAL),
+        }
```

This is a minor style cleanup that's unrelated to the macro work. It's fine but is an unrelated change that could be split out per the "one logical change per patch" convention.

**Nit — the `FalconCoreRevSubversion` bitmask `_ =>` arm is unreachable:**

After `value & 0b11`, the result is always 0–3, and all four values are covered. The `_ => Err(EINVAL)` arm is dead code. The original code had the same issue, so this isn't introduced by the patch, but it could be cleaned up with an `unreachable!()` or simply removing it if the compiler can prove exhaustiveness (it can't for computed values, so keeping the `_` arm is fine — just noting it).

**Overall:** Clean, well-motivated patch that reduces ~100 lines of boilerplate. The macro is straightforward and correctly handles the edge cases (excluded variants, different error types per enum). The `#[repr(u8)]` addition to `FalconFbifTarget` is a worthwhile correctness fix. Minor suggestions above are non-blocking.

---
Generated by Claude Code Patch Reviewer