The getter method of a field works with the field type, but its setter
expects the type of the register. This leads to an asymmetry in the
From/Into implementations required for a field with a dedicated type.
For instance, a field declared as

    pub struct ControlReg(u32) {
        3:0 mode as u8 ?=> Mode;
        ...
    }

currently requires the following implementations:

    impl TryFrom<u8> for Mode {
      ...
    }

    impl From<Mode> for u32 {
      ...
    }

Change this so the `From<Mode>` now needs to be implemented for `u8`,
i.e. the primitive type of the field. This is more consistent, and will
become a requirement once we start using the TryFrom/Into derive macros
to implement these automatically.

Reported-by: Edwin Peer <epeer@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/falcon.rs      | 38 +++++++++++++++++++++++++-----------
 drivers/gpu/nova-core/regs/macros.rs | 10 +++++-----
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 37e6298195e4..3f505b870601 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -22,11 +22,11 @@
 pub(crate) mod sec2;
 
 // TODO[FPRI]: Replace with `ToPrimitive`.
-macro_rules! impl_from_enum_to_u32 {
+macro_rules! impl_from_enum_to_u8 {
     ($enum_type:ty) => {
-        impl From<$enum_type> for u32 {
+        impl From<$enum_type> for u8 {
             fn from(value: $enum_type) -> Self {
-                value as u32
+                value as u8
             }
         }
     };
@@ -46,7 +46,7 @@ pub(crate) enum FalconCoreRev {
     Rev6 = 6,
     Rev7 = 7,
 }
-impl_from_enum_to_u32!(FalconCoreRev);
+impl_from_enum_to_u8!(FalconCoreRev);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconCoreRev {
@@ -81,7 +81,7 @@ pub(crate) enum FalconCoreRevSubversion {
     Subversion2 = 2,
     Subversion3 = 3,
 }
-impl_from_enum_to_u32!(FalconCoreRevSubversion);
+impl_from_enum_to_u8!(FalconCoreRevSubversion);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconCoreRevSubversion {
@@ -125,7 +125,7 @@ pub(crate) enum FalconSecurityModel {
     /// Also known as High-Secure, Privilege Level 3 or PL3.
     Heavy = 3,
 }
-impl_from_enum_to_u32!(FalconSecurityModel);
+impl_from_enum_to_u8!(FalconSecurityModel);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconSecurityModel {
@@ -157,7 +157,7 @@ pub(crate) enum FalconModSelAlgo {
     #[default]
     Rsa3k = 1,
 }
-impl_from_enum_to_u32!(FalconModSelAlgo);
+impl_from_enum_to_u8!(FalconModSelAlgo);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconModSelAlgo {
@@ -179,7 +179,7 @@ pub(crate) enum DmaTrfCmdSize {
     #[default]
     Size256B = 0x6,
 }
-impl_from_enum_to_u32!(DmaTrfCmdSize);
+impl_from_enum_to_u8!(DmaTrfCmdSize);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for DmaTrfCmdSize {
@@ -202,7 +202,6 @@ pub(crate) enum PeregrineCoreSelect {
     /// RISC-V core is active.
     Riscv = 1,
 }
-impl_from_enum_to_u32!(PeregrineCoreSelect);
 
 impl From<bool> for PeregrineCoreSelect {
     fn from(value: bool) -> Self {
@@ -213,6 +212,15 @@ fn from(value: bool) -> Self {
     }
 }
 
+impl From<PeregrineCoreSelect> for bool {
+    fn from(value: PeregrineCoreSelect) -> Self {
+        match value {
+            PeregrineCoreSelect::Falcon => false,
+            PeregrineCoreSelect::Riscv => true,
+        }
+    }
+}
+
 /// Different types of memory present in a falcon core.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub(crate) enum FalconMem {
@@ -236,7 +244,7 @@ pub(crate) enum FalconFbifTarget {
     /// Non-coherent system memory (System DRAM).
     NoncoherentSysmem = 2,
 }
-impl_from_enum_to_u32!(FalconFbifTarget);
+impl_from_enum_to_u8!(FalconFbifTarget);
 
 // TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconFbifTarget {
@@ -263,7 +271,6 @@ pub(crate) enum FalconFbifMemType {
     /// Physical memory addresses.
     Physical = 1,
 }
-impl_from_enum_to_u32!(FalconFbifMemType);
 
 /// Conversion from a single-bit register field.
 impl From<bool> for FalconFbifMemType {
@@ -275,6 +282,15 @@ fn from(value: bool) -> Self {
     }
 }
 
+impl From<FalconFbifMemType> for bool {
+    fn from(value: FalconFbifMemType) -> Self {
+        match value {
+            FalconFbifMemType::Virtual => false,
+            FalconFbifMemType::Physical => true,
+        }
+    }
+}
+
 /// Type used to represent the `PFALCON` registers address base for a given falcon engine.
 pub(crate) struct PFalconBase(());
 
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 754c14ee7f40..73811a115762 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -482,7 +482,7 @@ impl $name {
         register!(
             @leaf_accessor $name $hi:$lo $field
             { |f| <$into_type>::from(if f != 0 { true } else { false }) }
-            $into_type => $into_type $(, $comment)?;
+            bool $into_type => $into_type $(, $comment)?;
         );
     };
 
@@ -499,7 +499,7 @@ impl $name {
             $(, $comment:literal)?;
     ) => {
         register!(@leaf_accessor $name $hi:$lo $field
-            { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
+            { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
             ::core::result::Result<
                 $try_into_type,
                 <$try_into_type as ::core::convert::TryFrom<$type>>::Error
@@ -513,7 +513,7 @@ impl $name {
             $(, $comment:literal)?;
     ) => {
         register!(@leaf_accessor $name $hi:$lo $field
-            { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
+            { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
     };
 
     // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
@@ -527,7 +527,7 @@ impl $name {
     // Generates the accessor methods for a single field.
     (
         @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
-            { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
+            { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
     ) => {
         ::kernel::macros::paste!(
         const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
@@ -559,7 +559,7 @@ pub(crate) fn $field(self) -> $res_type {
         pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
             const MASK: u32 = $name::[<$field:upper _MASK>];
             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
-            let value = (u32::from(value) << SHIFT) & MASK;
+            let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
             self.0 = (self.0 & !MASK) | value;
 
             self

-- 
2.51.0

On Thu, Oct 09, 2025 at 09:37:08PM +0900, Alexandre Courbot wrote:
> The getter method of a field works with the field type, but its setter
> expects the type of the register. This leads to an asymmetry in the
> From/Into implementations required for a field with a dedicated type.
> For instance, a field declared as
> 
>     pub struct ControlReg(u32) {
>         3:0 mode as u8 ?=> Mode;
>         ...
>     }
> 
> currently requires the following implementations:
> 
>     impl TryFrom<u8> for Mode {
>       ...
>     }
> 
>     impl From<Mode> for u32 {
>       ...
>     }
> 
> Change this so the `From<Mode>` now needs to be implemented for `u8`,
> i.e. the primitive type of the field. This is more consistent, and will
> become a requirement once we start using the TryFrom/Into derive macros
> to implement these automatically.
> 
> Reported-by: Edwin Peer <epeer@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

thanks,

 - Joel

> ---
>  drivers/gpu/nova-core/falcon.rs      | 38 +++++++++++++++++++++++++-----------
>  drivers/gpu/nova-core/regs/macros.rs | 10 +++++-----
>  2 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 37e6298195e4..3f505b870601 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -22,11 +22,11 @@
>  pub(crate) mod sec2;
>  
>  // TODO[FPRI]: Replace with `ToPrimitive`.
> -macro_rules! impl_from_enum_to_u32 {
> +macro_rules! impl_from_enum_to_u8 {
>      ($enum_type:ty) => {
> -        impl From<$enum_type> for u32 {
> +        impl From<$enum_type> for u8 {
>              fn from(value: $enum_type) -> Self {
> -                value as u32
> +                value as u8
>              }
>          }
>      };
> @@ -46,7 +46,7 @@ pub(crate) enum FalconCoreRev {
>      Rev6 = 6,
>      Rev7 = 7,
>  }
> -impl_from_enum_to_u32!(FalconCoreRev);
> +impl_from_enum_to_u8!(FalconCoreRev);
>  
>  // TODO[FPRI]: replace with `FromPrimitive`.
>  impl TryFrom<u8> for FalconCoreRev {
> @@ -81,7 +81,7 @@ pub(crate) enum FalconCoreRevSubversion {
>      Subversion2 = 2,
>      Subversion3 = 3,
>  }
> -impl_from_enum_to_u32!(FalconCoreRevSubversion);
> +impl_from_enum_to_u8!(FalconCoreRevSubversion);
>  
>  // TODO[FPRI]: replace with `FromPrimitive`.
>  impl TryFrom<u8> for FalconCoreRevSubversion {
> @@ -125,7 +125,7 @@ pub(crate) enum FalconSecurityModel {
>      /// Also known as High-Secure, Privilege Level 3 or PL3.
>      Heavy = 3,
>  }
> -impl_from_enum_to_u32!(FalconSecurityModel);
> +impl_from_enum_to_u8!(FalconSecurityModel);
>  
>  // TODO[FPRI]: replace with `FromPrimitive`.
>  impl TryFrom<u8> for FalconSecurityModel {
> @@ -157,7 +157,7 @@ pub(crate) enum FalconModSelAlgo {
>      #[default]
>      Rsa3k = 1,
>  }
> -impl_from_enum_to_u32!(FalconModSelAlgo);
> +impl_from_enum_to_u8!(FalconModSelAlgo);
>  
>  // TODO[FPRI]: replace with `FromPrimitive`.
>  impl TryFrom<u8> for FalconModSelAlgo {
> @@ -179,7 +179,7 @@ pub(crate) enum DmaTrfCmdSize {
>      #[default]
>      Size256B = 0x6,
>  }
> -impl_from_enum_to_u32!(DmaTrfCmdSize);
> +impl_from_enum_to_u8!(DmaTrfCmdSize);
>  
>  // TODO[FPRI]: replace with `FromPrimitive`.
>  impl TryFrom<u8> for DmaTrfCmdSize {
> @@ -202,7 +202,6 @@ pub(crate) enum PeregrineCoreSelect {
>      /// RISC-V core is active.
>      Riscv = 1,
>  }
> -impl_from_enum_to_u32!(PeregrineCoreSelect);
>  
>  impl From<bool> for PeregrineCoreSelect {
>      fn from(value: bool) -> Self {
> @@ -213,6 +212,15 @@ fn from(value: bool) -> Self {
>      }
>  }
>  
> +impl From<PeregrineCoreSelect> for bool {
> +    fn from(value: PeregrineCoreSelect) -> Self {
> +        match value {
> +            PeregrineCoreSelect::Falcon => false,
> +            PeregrineCoreSelect::Riscv => true,
> +        }
> +    }
> +}
> +
>  /// Different types of memory present in a falcon core.
>  #[derive(Debug, Clone, Copy, PartialEq, Eq)]
>  pub(crate) enum FalconMem {
> @@ -236,7 +244,7 @@ pub(crate) enum FalconFbifTarget {
>      /// Non-coherent system memory (System DRAM).
>      NoncoherentSysmem = 2,
>  }
> -impl_from_enum_to_u32!(FalconFbifTarget);
> +impl_from_enum_to_u8!(FalconFbifTarget);
>  
>  // TODO[FPRI]: replace with `FromPrimitive`.
>  impl TryFrom<u8> for FalconFbifTarget {
> @@ -263,7 +271,6 @@ pub(crate) enum FalconFbifMemType {
>      /// Physical memory addresses.
>      Physical = 1,
>  }
> -impl_from_enum_to_u32!(FalconFbifMemType);
>  
>  /// Conversion from a single-bit register field.
>  impl From<bool> for FalconFbifMemType {
> @@ -275,6 +282,15 @@ fn from(value: bool) -> Self {
>      }
>  }
>  
> +impl From<FalconFbifMemType> for bool {
> +    fn from(value: FalconFbifMemType) -> Self {
> +        match value {
> +            FalconFbifMemType::Virtual => false,
> +            FalconFbifMemType::Physical => true,
> +        }
> +    }
> +}
> +
>  /// Type used to represent the `PFALCON` registers address base for a given falcon engine.
>  pub(crate) struct PFalconBase(());
>  
> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
> index 754c14ee7f40..73811a115762 100644
> --- a/drivers/gpu/nova-core/regs/macros.rs
> +++ b/drivers/gpu/nova-core/regs/macros.rs
> @@ -482,7 +482,7 @@ impl $name {
>          register!(
>              @leaf_accessor $name $hi:$lo $field
>              { |f| <$into_type>::from(if f != 0 { true } else { false }) }
> -            $into_type => $into_type $(, $comment)?;
> +            bool $into_type => $into_type $(, $comment)?;
>          );
>      };
>  
> @@ -499,7 +499,7 @@ impl $name {
>              $(, $comment:literal)?;
>      ) => {
>          register!(@leaf_accessor $name $hi:$lo $field
> -            { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
> +            { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
>              ::core::result::Result<
>                  $try_into_type,
>                  <$try_into_type as ::core::convert::TryFrom<$type>>::Error
> @@ -513,7 +513,7 @@ impl $name {
>              $(, $comment:literal)?;
>      ) => {
>          register!(@leaf_accessor $name $hi:$lo $field
> -            { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
> +            { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
>      };
>  
>      // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
> @@ -527,7 +527,7 @@ impl $name {
>      // Generates the accessor methods for a single field.
>      (
>          @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
> -            { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
> +            { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
>      ) => {
>          ::kernel::macros::paste!(
>          const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
> @@ -559,7 +559,7 @@ pub(crate) fn $field(self) -> $res_type {
>          pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
>              const MASK: u32 = $name::[<$field:upper _MASK>];
>              const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> -            let value = (u32::from(value) << SHIFT) & MASK;
> +            let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
>              self.0 = (self.0 & !MASK) | value;
>  
>              self
> 
> -- 
> 2.51.0
>

> On Oct 9, 2025, at 5:37 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> The getter method of a field works with the field type, but its setter
> expects the type of the register. This leads to an asymmetry in the
> From/Into implementations required for a field with a dedicated type.
> For instance, a field declared as
> 
>    pub struct ControlReg(u32) {
>        3:0 mode as u8 ?=> Mode;
>        ...
>    }
> 
> currently requires the following implementations:
> 
>    impl TryFrom<u8> for Mode {
>      ...
>    }
> 
>    impl From<Mode> for u32 {
>      ...
>    }
> 
> Change this so the `From<Mode>` now needs to be implemented for `u8`,
> i.e. the primitive type of the field. This is more consistent, and will
> become a requirement once we start using the TryFrom/Into derive macros
> to implement these automatically.
> 
> Reported-by: Edwin Peer <epeer@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> drivers/gpu/nova-core/falcon.rs      | 38 +++++++++++++++++++++++++-----------
> drivers/gpu/nova-core/regs/macros.rs | 10 +++++-----
> 2 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 37e6298195e4..3f505b870601 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -22,11 +22,11 @@
> pub(crate) mod sec2;
> 
> // TODO[FPRI]: Replace with `ToPrimitive`.
> -macro_rules! impl_from_enum_to_u32 {
> +macro_rules! impl_from_enum_to_u8 {
>     ($enum_type:ty) => {
> -        impl From<$enum_type> for u32 {
> +        impl From<$enum_type> for u8 {
>             fn from(value: $enum_type) -> Self {
> -                value as u32
> +                value as u8
>             }
>         }
>     };
> @@ -46,7 +46,7 @@ pub(crate) enum FalconCoreRev {
>     Rev6 = 6,
>     Rev7 = 7,
> }
> -impl_from_enum_to_u32!(FalconCoreRev);
> +impl_from_enum_to_u8!(FalconCoreRev);
> 
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconCoreRev {
> @@ -81,7 +81,7 @@ pub(crate) enum FalconCoreRevSubversion {
>     Subversion2 = 2,
>     Subversion3 = 3,
> }
> -impl_from_enum_to_u32!(FalconCoreRevSubversion);
> +impl_from_enum_to_u8!(FalconCoreRevSubversion);
> 
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconCoreRevSubversion {
> @@ -125,7 +125,7 @@ pub(crate) enum FalconSecurityModel {
>     /// Also known as High-Secure, Privilege Level 3 or PL3.
>     Heavy = 3,
> }
> -impl_from_enum_to_u32!(FalconSecurityModel);
> +impl_from_enum_to_u8!(FalconSecurityModel);
> 
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconSecurityModel {
> @@ -157,7 +157,7 @@ pub(crate) enum FalconModSelAlgo {
>     #[default]
>     Rsa3k = 1,
> }
> -impl_from_enum_to_u32!(FalconModSelAlgo);
> +impl_from_enum_to_u8!(FalconModSelAlgo);
> 
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconModSelAlgo {
> @@ -179,7 +179,7 @@ pub(crate) enum DmaTrfCmdSize {
>     #[default]
>     Size256B = 0x6,
> }
> -impl_from_enum_to_u32!(DmaTrfCmdSize);
> +impl_from_enum_to_u8!(DmaTrfCmdSize);
> 
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for DmaTrfCmdSize {
> @@ -202,7 +202,6 @@ pub(crate) enum PeregrineCoreSelect {
>     /// RISC-V core is active.
>     Riscv = 1,
> }
> -impl_from_enum_to_u32!(PeregrineCoreSelect);
> 
> impl From<bool> for PeregrineCoreSelect {
>     fn from(value: bool) -> Self {
> @@ -213,6 +212,15 @@ fn from(value: bool) -> Self {
>     }
> }
> 
> +impl From<PeregrineCoreSelect> for bool {
> +    fn from(value: PeregrineCoreSelect) -> Self {
> +        match value {
> +            PeregrineCoreSelect::Falcon => false,
> +            PeregrineCoreSelect::Riscv => true,
> +        }
> +    }
> +}
> +
> /// Different types of memory present in a falcon core.
> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> pub(crate) enum FalconMem {
> @@ -236,7 +244,7 @@ pub(crate) enum FalconFbifTarget {
>     /// Non-coherent system memory (System DRAM).
>     NoncoherentSysmem = 2,
> }
> -impl_from_enum_to_u32!(FalconFbifTarget);
> +impl_from_enum_to_u8!(FalconFbifTarget);
> 
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconFbifTarget {
> @@ -263,7 +271,6 @@ pub(crate) enum FalconFbifMemType {
>     /// Physical memory addresses.
>     Physical = 1,
> }
> -impl_from_enum_to_u32!(FalconFbifMemType);
> 
> /// Conversion from a single-bit register field.
> impl From<bool> for FalconFbifMemType {
> @@ -275,6 +282,15 @@ fn from(value: bool) -> Self {
>     }
> }
> 
> +impl From<FalconFbifMemType> for bool {
> +    fn from(value: FalconFbifMemType) -> Self {
> +        match value {
> +            FalconFbifMemType::Virtual => false,
> +            FalconFbifMemType::Physical => true,
> +        }
> +    }
> +}
> +
> /// Type used to represent the `PFALCON` registers address base for a given falcon engine.
> pub(crate) struct PFalconBase(());
> 
> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs

The examples in the doc comments should also be updated. Otherwise, LGTM.

Reviewed-by: Edwin Peer <epeer@nvidia.com>

Regards,
Edwin Peer

> index 754c14ee7f40..73811a115762 100644
> --- a/drivers/gpu/nova-core/regs/macros.rs
> +++ b/drivers/gpu/nova-core/regs/macros.rs
> @@ -482,7 +482,7 @@ impl $name {
>         register!(
>             @leaf_accessor $name $hi:$lo $field
>             { |f| <$into_type>::from(if f != 0 { true } else { false }) }
> -            $into_type => $into_type $(, $comment)?;
> +            bool $into_type => $into_type $(, $comment)?;
>         );
>     };
> 
> @@ -499,7 +499,7 @@ impl $name {
>             $(, $comment:literal)?;
>     ) => {
>         register!(@leaf_accessor $name $hi:$lo $field
> -            { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
> +            { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
>             ::core::result::Result<
>                 $try_into_type,
>                 <$try_into_type as ::core::convert::TryFrom<$type>>::Error
> @@ -513,7 +513,7 @@ impl $name {
>             $(, $comment:literal)?;
>     ) => {
>         register!(@leaf_accessor $name $hi:$lo $field
> -            { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
> +            { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
>     };
> 
>     // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
> @@ -527,7 +527,7 @@ impl $name {
>     // Generates the accessor methods for a single field.
>     (
>         @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
> -            { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
> +            { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
>     ) => {
>         ::kernel::macros::paste!(
>         const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
> @@ -559,7 +559,7 @@ pub(crate) fn $field(self) -> $res_type {
>         pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
>             const MASK: u32 = $name::[<$field:upper _MASK>];
>             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> -            let value = (u32::from(value) << SHIFT) & MASK;
> +            let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
>             self.0 = (self.0 & !MASK) | value;
> 
>             self
> 
> -- 
> 2.51.0
> 

Hi Alex,

On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
> The getter method of a field works with the field type, but its setter
> expects the type of the register. This leads to an asymmetry in the
> From/Into implementations required for a field with a dedicated type.
> For instance, a field declared as
> 
>     pub struct ControlReg(u32) {
>         3:0 mode as u8 ?=> Mode;
>         ...
>     }
> 
> currently requires the following implementations:
> 
>     impl TryFrom<u8> for Mode {
>       ...
>     }
> 
>     impl From<Mode> for u32 {
>       ...
>     }
> 
> Change this so the `From<Mode>` now needs to be implemented for `u8`,
> i.e. the primitive type of the field. This is more consistent, and will
> become a requirement once we start using the TryFrom/Into derive macros
> to implement these automatically.
> 
> Reported-by: Edwin Peer <epeer@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

As these are incremental improvements, could you please rebase on top of the v6
bitfield series so it does not conflict?

https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6

Thanks.

> On Oct 9, 2025, at 11:17 AM, Joel Fernandes <joelagnelf@nvidia.com> wrote:
> 
> Hi Alex,
> 
>> On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
>> The getter method of a field works with the field type, but its setter
>> expects the type of the register. This leads to an asymmetry in the
>> From/Into implementations required for a field with a dedicated type.
>> For instance, a field declared as
>> 
>>    pub struct ControlReg(u32) {
>>        3:0 mode as u8 ?=> Mode;
>>        ...
>>    }
>> 
>> currently requires the following implementations:
>> 
>>    impl TryFrom<u8> for Mode {
>>      ...
>>    }
>> 
>>    impl From<Mode> for u32 {
>>      ...
>>    }
>> 
>> Change this so the `From<Mode>` now needs to be implemented for `u8`,
>> i.e. the primitive type of the field. This is more consistent, and will
>> become a requirement once we start using the TryFrom/Into derive macros
>> to implement these automatically.
>> 
>> Reported-by: Edwin Peer <epeer@nvidia.com>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> As these are incremental improvements, could you please rebase on top of the v6
> bitfield series so it does not conflict?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6

On second thought, I could just carry this patch on top of my v6 series and avoid too much conflict.

So if it is ok with you, please only carry the last 2 patches of this series whenever applying.

For this patch:
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

I will review the other two patches as well. Thanks.


> 
> Thanks.

On Fri Oct 10, 2025 at 12:41 AM JST, Joel Fernandes wrote:
>
>
>> On Oct 9, 2025, at 11:17 AM, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>> 
>> Hi Alex,
>> 
>>> On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
>>> The getter method of a field works with the field type, but its setter
>>> expects the type of the register. This leads to an asymmetry in the
>>> From/Into implementations required for a field with a dedicated type.
>>> For instance, a field declared as
>>> 
>>>    pub struct ControlReg(u32) {
>>>        3:0 mode as u8 ?=> Mode;
>>>        ...
>>>    }
>>> 
>>> currently requires the following implementations:
>>> 
>>>    impl TryFrom<u8> for Mode {
>>>      ...
>>>    }
>>> 
>>>    impl From<Mode> for u32 {
>>>      ...
>>>    }
>>> 
>>> Change this so the `From<Mode>` now needs to be implemented for `u8`,
>>> i.e. the primitive type of the field. This is more consistent, and will
>>> become a requirement once we start using the TryFrom/Into derive macros
>>> to implement these automatically.
>>> 
>>> Reported-by: Edwin Peer <epeer@nvidia.com>
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> 
>> As these are incremental improvements, could you please rebase on top of the v6
>> bitfield series so it does not conflict?
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6
>
> On second thought, I could just carry this patch on top of my v6 series and avoid too much conflict.
>
> So if it is ok with you, please only carry the last 2 patches of this series whenever applying.
>
> For this patch:
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
>
> I will review the other two patches as well. Thanks.

The idea is for this patch to go *before* your series, to avoid the
asymmetry in the From/Into implementions of bitfields. We could also put
it after, but it would become larger as a result and I think it can be
merge soon after -rc1 is tagged anyway.

On Fri Oct 10, 2025 at 5:24 PM JST, Alexandre Courbot wrote:
> On Fri Oct 10, 2025 at 12:41 AM JST, Joel Fernandes wrote:
>>
>>
>>> On Oct 9, 2025, at 11:17 AM, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>> 
>>> Hi Alex,
>>> 
>>>> On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
>>>> The getter method of a field works with the field type, but its setter
>>>> expects the type of the register. This leads to an asymmetry in the
>>>> From/Into implementations required for a field with a dedicated type.
>>>> For instance, a field declared as
>>>> 
>>>>    pub struct ControlReg(u32) {
>>>>        3:0 mode as u8 ?=> Mode;
>>>>        ...
>>>>    }
>>>> 
>>>> currently requires the following implementations:
>>>> 
>>>>    impl TryFrom<u8> for Mode {
>>>>      ...
>>>>    }
>>>> 
>>>>    impl From<Mode> for u32 {
>>>>      ...
>>>>    }
>>>> 
>>>> Change this so the `From<Mode>` now needs to be implemented for `u8`,
>>>> i.e. the primitive type of the field. This is more consistent, and will
>>>> become a requirement once we start using the TryFrom/Into derive macros
>>>> to implement these automatically.
>>>> 
>>>> Reported-by: Edwin Peer <epeer@nvidia.com>
>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>> 
>>> As these are incremental improvements, could you please rebase on top of the v6
>>> bitfield series so it does not conflict?
>>> 
>>> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6
>>
>> On second thought, I could just carry this patch on top of my v6 series and avoid too much conflict.
>>
>> So if it is ok with you, please only carry the last 2 patches of this series whenever applying.
>>
>> For this patch:
>> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
>>
>> I will review the other two patches as well. Thanks.
>
> The idea is for this patch to go *before* your series, to avoid the
> asymmetry in the From/Into implementions of bitfields. We could also put
> it after, but it would become larger as a result and I think it can be
> merge soon after -rc1 is tagged anyway.

I forgot to mention that this patch is not really part of the series ;
it's just here to allow it to apply on top of drm-rust-next should
anyone want to try it.

On 10/10/2025 4:26 AM, Alexandre Courbot wrote:
> On Fri Oct 10, 2025 at 5:24 PM JST, Alexandre Courbot wrote:
>> On Fri Oct 10, 2025 at 12:41 AM JST, Joel Fernandes wrote:
>>>
>>>
>>>> On Oct 9, 2025, at 11:17 AM, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>>> On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
>>>>> The getter method of a field works with the field type, but its setter
>>>>> expects the type of the register. This leads to an asymmetry in the
>>>>> From/Into implementations required for a field with a dedicated type.
>>>>> For instance, a field declared as
>>>>>
>>>>>    pub struct ControlReg(u32) {
>>>>>        3:0 mode as u8 ?=> Mode;
>>>>>        ...
>>>>>    }
>>>>>
>>>>> currently requires the following implementations:
>>>>>
>>>>>    impl TryFrom<u8> for Mode {
>>>>>      ...
>>>>>    }
>>>>>
>>>>>    impl From<Mode> for u32 {
>>>>>      ...
>>>>>    }
>>>>>
>>>>> Change this so the `From<Mode>` now needs to be implemented for `u8`,
>>>>> i.e. the primitive type of the field. This is more consistent, and will
>>>>> become a requirement once we start using the TryFrom/Into derive macros
>>>>> to implement these automatically.
>>>>>
>>>>> Reported-by: Edwin Peer <epeer@nvidia.com>
>>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>>
>>>> As these are incremental improvements, could you please rebase on top of the v6
>>>> bitfield series so it does not conflict?
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6
>>>
>>> On second thought, I could just carry this patch on top of my v6 series and avoid too much conflict.
>>>
>>> So if it is ok with you, please only carry the last 2 patches of this series whenever applying.
>>>
>>> For this patch:
>>> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
>>>
>>> I will review the other two patches as well. Thanks.
>>
>> The idea is for this patch to go *before* your series, to avoid the
>> asymmetry in the From/Into implementions of bitfields. We could also put
>> it after, but it would become larger as a result and I think it can be
>> merge soon after -rc1 is tagged anyway.
> 
> I forgot to mention that this patch is not really part of the series ;
> it's just here to allow it to apply on top of drm-rust-next should
> anyone want to try it.

These are incremental improvements, I don't see the point in making the
improvements before bitfield extraction within Nova. Very least, we can apply
the bitfield extraction within nova and then make the improvements (within
nova), since the extraction work happened well before (and it was a non-trivial
amount of work), before any of these improvements. Let us work together to make
sure we don't have to throw away old work due to new patches please. :)

thanks,

 - Joel