[PATCH] gpu: nova: add impl_num_enum!() macro to replace FromPrimitive boilerplate

[PATCH] gpu: nova: add impl_num_enum!() macro to replace FromPrimitive boilerplate

[Artem Lytkin]

Add a declarative macro impl_num_enum!() to generate both From<Enum>
for primitive and TryFrom<primitive> for Enum implementations for
repr(primitive) enums. This addresses the TODO[FPRI] markers
throughout the driver.

The macro accepts a list of variants to map, allowing certain variants
to be excluded from the reverse mapping (e.g. dead_code variants like
FalconModSelAlgo::Aes).

Replace the manual TryFrom and impl_from_enum_to_u8!() boilerplate for:
  - FalconCoreRev
  - FalconSecurityModel
  - FalconModSelAlgo
  - DmaTrfCmdSize
  - FalconFbifTarget
  - Architecture

FalconCoreRevSubversion retains its manual TryFrom because it requires
a bitmask operation on the input value. The Chipset enum retains its
existing define_chipset!() macro which already generates TryFrom.

While at it, add the missing repr(u8) attribute to FalconFbifTarget,
which is needed for the enum-to-primitive cast to be well-defined.

Signed-off-by: Artem Lytkin <iprintercanon@gmail.com>
---
 drivers/gpu/nova-core/falcon.rs | 128 ++++++--------------------------
 drivers/gpu/nova-core/gpu.rs    |  21 +-----
 drivers/gpu/nova-core/num.rs    |  34 +++++++++
 3 files changed, 57 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 82c661aef594..5b67cabe02f2 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -23,6 +23,7 @@
     driver::Bar0,
     gpu::Chipset,
     num::{
+        impl_num_enum,
         FromSafeCast,
         IntoSafeCast, //
     },
@@ -34,17 +35,6 @@
 mod hal;
 pub(crate) mod sec2;
 
-// TODO[FPRI]: Replace with `ToPrimitive`.
-macro_rules! impl_from_enum_to_u8 {
-    ($enum_type:ty) => {
-        impl From<$enum_type> for u8 {
-            fn from(value: $enum_type) -> Self {
-                value as u8
-            }
-        }
-    };
-}
-
 /// Revision number of a falcon core, used in the [`crate::regs::NV_PFALCON_FALCON_HWCFG1`]
 /// register.
 #[repr(u8)]
@@ -59,29 +49,7 @@ pub(crate) enum FalconCoreRev {
     Rev6 = 6,
     Rev7 = 7,
 }
-impl_from_enum_to_u8!(FalconCoreRev);
-
-// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for FalconCoreRev {
-    type Error = Error;
-
-    fn try_from(value: u8) -> Result<Self> {
-        use FalconCoreRev::*;
-
-        let rev = match value {
-            1 => Rev1,
-            2 => Rev2,
-            3 => Rev3,
-            4 => Rev4,
-            5 => Rev5,
-            6 => Rev6,
-            7 => Rev7,
-            _ => return Err(EINVAL),
-        };
-
-        Ok(rev)
-    }
-}
+impl_num_enum!(FalconCoreRev: u8 [Rev1, Rev2, Rev3, Rev4, Rev5, Rev6, Rev7] => EINVAL);
 
 /// Revision subversion number of a falcon core, used in the
 /// [`crate::regs::NV_PFALCON_FALCON_HWCFG1`] register.
@@ -94,24 +62,27 @@ pub(crate) enum FalconCoreRevSubversion {
     Subversion2 = 2,
     Subversion3 = 3,
 }
-impl_from_enum_to_u8!(FalconCoreRevSubversion);
 
-// TODO[FPRI]: replace with `FromPrimitive`.
+impl From<FalconCoreRevSubversion> for u8 {
+    fn from(val: FalconCoreRevSubversion) -> u8 {
+        val as u8
+    }
+}
+
+// Manual TryFrom is required here due to the bitmask operation on the input value.
 impl TryFrom<u8> for FalconCoreRevSubversion {
     type Error = Error;
 
     fn try_from(value: u8) -> Result<Self> {
         use FalconCoreRevSubversion::*;
 
-        let sub_version = match value & 0b11 {
-            0 => Subversion0,
-            1 => Subversion1,
-            2 => Subversion2,
-            3 => Subversion3,
-            _ => return Err(EINVAL),
-        };
-
-        Ok(sub_version)
+        match value & 0b11 {
+            0 => Ok(Subversion0),
+            1 => Ok(Subversion1),
+            2 => Ok(Subversion2),
+            3 => Ok(Subversion3),
+            _ => Err(EINVAL),
+        }
     }
 }
 
@@ -138,25 +109,7 @@ pub(crate) enum FalconSecurityModel {
     /// Also known as High-Secure, Privilege Level 3 or PL3.
     Heavy = 3,
 }
-impl_from_enum_to_u8!(FalconSecurityModel);
-
-// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for FalconSecurityModel {
-    type Error = Error;
-
-    fn try_from(value: u8) -> Result<Self> {
-        use FalconSecurityModel::*;
-
-        let sec_model = match value {
-            0 => None,
-            2 => Light,
-            3 => Heavy,
-            _ => return Err(EINVAL),
-        };
-
-        Ok(sec_model)
-    }
-}
+impl_num_enum!(FalconSecurityModel: u8 [None, Light, Heavy] => EINVAL);
 
 /// Signing algorithm for a given firmware, used in the [`crate::regs::NV_PFALCON2_FALCON_MOD_SEL`]
 /// register. It is passed to the Falcon Boot ROM (BROM) as a parameter.
@@ -170,19 +123,7 @@ pub(crate) enum FalconModSelAlgo {
     #[default]
     Rsa3k = 1,
 }
-impl_from_enum_to_u8!(FalconModSelAlgo);
-
-// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for FalconModSelAlgo {
-    type Error = Error;
-
-    fn try_from(value: u8) -> Result<Self> {
-        match value {
-            1 => Ok(FalconModSelAlgo::Rsa3k),
-            _ => Err(EINVAL),
-        }
-    }
-}
+impl_num_enum!(FalconModSelAlgo: u8 [Rsa3k] => EINVAL);
 
 /// Valid values for the `size` field of the [`crate::regs::NV_PFALCON_FALCON_DMATRFCMD`] register.
 #[repr(u8)]
@@ -192,19 +133,7 @@ pub(crate) enum DmaTrfCmdSize {
     #[default]
     Size256B = 0x6,
 }
-impl_from_enum_to_u8!(DmaTrfCmdSize);
-
-// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for DmaTrfCmdSize {
-    type Error = Error;
-
-    fn try_from(value: u8) -> Result<Self> {
-        match value {
-            0x6 => Ok(Self::Size256B),
-            _ => Err(EINVAL),
-        }
-    }
-}
+impl_num_enum!(DmaTrfCmdSize: u8 [Size256B] => EINVAL);
 
 /// Currently active core on a dual falcon/riscv (Peregrine) controller.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
@@ -247,6 +176,7 @@ pub(crate) enum FalconMem {
 /// This determines the memory type for external memory access during a DMA transfer, which is
 /// performed by the Falcon's Framebuffer DMA (FBDMA) engine. See falcon.rst for more details.
 #[derive(Debug, Clone, Default)]
+#[repr(u8)]
 pub(crate) enum FalconFbifTarget {
     /// VRAM.
     #[default]
@@ -257,23 +187,7 @@ pub(crate) enum FalconFbifTarget {
     /// Non-coherent system memory (System DRAM).
     NoncoherentSysmem = 2,
 }
-impl_from_enum_to_u8!(FalconFbifTarget);
-
-// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for FalconFbifTarget {
-    type Error = Error;
-
-    fn try_from(value: u8) -> Result<Self> {
-        let res = match value {
-            0 => Self::LocalFb,
-            1 => Self::CoherentSysmem,
-            2 => Self::NoncoherentSysmem,
-            _ => return Err(EINVAL),
-        };
-
-        Ok(res)
-    }
-}
+impl_num_enum!(FalconFbifTarget: u8 [LocalFb, CoherentSysmem, NoncoherentSysmem] => EINVAL);
 
 /// Type of memory addresses to use.
 #[derive(Debug, Clone, Default)]
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 629c9d2dc994..045bd35f2d8f 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -19,6 +19,7 @@
     fb::SysmemFlush,
     gfw,
     gsp::Gsp,
+    num::impl_num_enum,
     regs,
 };
 
@@ -135,25 +136,7 @@ pub(crate) enum Architecture {
     Ada = 0x19,
 }
 
-impl TryFrom<u8> for Architecture {
-    type Error = Error;
-
-    fn try_from(value: u8) -> Result<Self> {
-        match value {
-            0x16 => Ok(Self::Turing),
-            0x17 => Ok(Self::Ampere),
-            0x19 => Ok(Self::Ada),
-            _ => Err(ENODEV),
-        }
-    }
-}
-
-impl From<Architecture> for u8 {
-    fn from(value: Architecture) -> Self {
-        // CAST: `Architecture` is `repr(u8)`, so this cast is always lossless.
-        value as u8
-    }
-}
+impl_num_enum!(Architecture: u8 [Turing, Ampere, Ada] => ENODEV);
 
 pub(crate) struct Revision {
     major: u8,
diff --git a/drivers/gpu/nova-core/num.rs b/drivers/gpu/nova-core/num.rs
index c952a834e662..d5db97c3de0f 100644
--- a/drivers/gpu/nova-core/num.rs
+++ b/drivers/gpu/nova-core/num.rs
@@ -215,3 +215,37 @@ pub(crate) const fn [<$from _into_ $into>]<const N: $from>() -> $into {
 impl_const_into!(u64 => { u8, u16, u32 });
 impl_const_into!(u32 => { u8, u16 });
 impl_const_into!(u16 => { u8 });
+
+/// Implements [`TryFrom`] and [`From`] conversions between a `#[repr(primitive)]` enum and its
+/// underlying primitive type.
+///
+/// The [`TryFrom`] conversion returns the specified error for unrecognized values. The [`From`]
+/// conversion casts the enum variant to the primitive type.
+///
+/// Only the variants listed in the macro invocation are recognized by [`TryFrom`]. This allows
+/// excluding certain variants (e.g., dead code) from the reverse mapping.
+///
+/// The enum **must** have a `#[repr($prim_type)]` attribute. Without it, the `as` cast in the
+/// generated [`From`] impl is not guaranteed to be correct.
+macro_rules! impl_num_enum {
+    ($enum_type:ty : $prim_type:ty [$($variant:ident),+ $(,)?] => $err:expr) => {
+        impl From<$enum_type> for $prim_type {
+            fn from(val: $enum_type) -> $prim_type {
+                val as $prim_type
+            }
+        }
+
+        impl TryFrom<$prim_type> for $enum_type {
+            type Error = kernel::error::Error;
+
+            fn try_from(val: $prim_type) -> core::result::Result<Self, Self::Error> {
+                $(if val == <$enum_type>::$variant as $prim_type {
+                    return Ok(<$enum_type>::$variant);
+                })*
+                Err($err)
+            }
+        }
+    };
+}
+
+pub(crate) use impl_num_enum;
-- 
2.43.0

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

[Claude Code Review Bot]

Overall Series Review

Subject: gpu: nova: add impl_num_enum!() macro to replace FromPrimitive boilerplate
Author: Artem Lytkin <iprintercanon@gmail.com>
Patches: 1
Reviewed: 2026-03-05T13:19:42.473891

---

This is a single-patch series that introduces a declarative macro `impl_num_enum!()` to replace repetitive `From<Enum> for u8` and `TryFrom<u8> for Enum` boilerplate across the nova-core driver. The motivation is sound — it addresses explicit `TODO[FPRI]` markers in the codebase and substantially reduces boilerplate. The patch is well-scoped and the commit message is clear about what is and isn't changed, and why.

The patch is generally good. There are a few points worth discussing, but no correctness bugs.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

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