The bitfield-specific into new macro. This will be used to define
structs with bitfields, similar to C language.

Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 drivers/gpu/nova-core/bitstruct.rs   | 271 +++++++++++++++++++++++++++
 drivers/gpu/nova-core/nova_core.rs   |   3 +
 drivers/gpu/nova-core/regs/macros.rs | 247 +-----------------------
 3 files changed, 282 insertions(+), 239 deletions(-)
 create mode 100644 drivers/gpu/nova-core/bitstruct.rs

diff --git a/drivers/gpu/nova-core/bitstruct.rs b/drivers/gpu/nova-core/bitstruct.rs
new file mode 100644
index 000000000000..1dd9edab7d07
--- /dev/null
+++ b/drivers/gpu/nova-core/bitstruct.rs
@@ -0,0 +1,271 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// bitstruct.rs — Bitfield library for Rust structures
+//
+// A library that provides support for defining bit fields in Rust
+// structures. Also used from things that need bitfields like register macro.
+///
+/// # Syntax
+///
+/// ```rust
+/// bitstruct! {
+///     struct ControlReg {
+///         3:0       mode        as u8 ?=> Mode;
+///         7:4       state       as u8 => State;
+///     }
+/// }
+/// ```
+///
+/// This generates a struct with:
+/// - Field accessors: `mode()`, `state()`, etc.
+/// - Field setters: `set_mode()`, `set_state()`, etc. (supports builder pattern).
+/// - Debug and Default implementations
+///
+/// The field setters can be used with the builder pattern, example:
+/// ControlReg::default().set_mode(mode).set_state(state);
+///
+/// Fields are defined as follows:
+///
+/// - `as <type>` simply returns the field value casted to <type>, typically `u32`, `u16`, `u8` or
+///   `bool`. Note that `bool` fields must have a range of 1 bit.
+/// - `as <type> => <into_type>` calls `<into_type>`'s `From::<<type>>` implementation and returns
+///   the result.
+/// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s `TryFrom::<<type>>` implementation
+///   and returns the result. This is useful with fields for which not all values are valid.
+macro_rules! bitstruct {
+    // Main entry point - defines the bitfield struct with fields
+    (struct $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
+        bitstruct!(@core $name $(, $comment)? { $($fields)* });
+    };
+
+    // All rules below are helpers.
+
+    // Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
+    // `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
+    (@core $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
+        $(
+        #[doc=$comment]
+        )?
+        #[repr(transparent)]
+        #[derive(Clone, Copy)]
+        pub(crate) struct $name(u32);
+
+        impl ::core::ops::BitOr for $name {
+            type Output = Self;
+
+            fn bitor(self, rhs: Self) -> Self::Output {
+                Self(self.0 | rhs.0)
+            }
+        }
+
+        impl ::core::convert::From<$name> for u32 {
+            fn from(val: $name) -> u32 {
+                val.0
+            }
+        }
+
+        bitstruct!(@fields_dispatcher $name { $($fields)* });
+    };
+
+    // Captures the fields and passes them to all the implementers that require field information.
+    //
+    // Used to simplify the matching rules for implementers, so they don't need to match the entire
+    // complex fields rule even though they only make use of part of it.
+    (@fields_dispatcher $name:ident {
+        $($hi:tt:$lo:tt $field:ident as $type:tt
+            $(?=> $try_into_type:ty)?
+            $(=> $into_type:ty)?
+            $(, $comment:literal)?
+        ;
+        )*
+    }
+    ) => {
+        bitstruct!(@field_accessors $name {
+            $(
+                $hi:$lo $field as $type
+                $(?=> $try_into_type)?
+                $(=> $into_type)?
+                $(, $comment)?
+            ;
+            )*
+        });
+        bitstruct!(@debug $name { $($field;)* });
+        bitstruct!(@default $name { $($field;)* });
+    };
+
+    // Defines all the field getter/setter methods for `$name`.
+    (
+        @field_accessors $name:ident {
+        $($hi:tt:$lo:tt $field:ident as $type:tt
+            $(?=> $try_into_type:ty)?
+            $(=> $into_type:ty)?
+            $(, $comment:literal)?
+        ;
+        )*
+        }
+    ) => {
+        $(
+            bitstruct!(@check_field_bounds $hi:$lo $field as $type);
+        )*
+
+        #[allow(dead_code)]
+        impl $name {
+            $(
+            bitstruct!(@field_accessor $name $hi:$lo $field as $type
+                $(?=> $try_into_type)?
+                $(=> $into_type)?
+                $(, $comment)?
+                ;
+            );
+            )*
+        }
+    };
+
+    // Boolean fields must have `$hi == $lo`.
+    (@check_field_bounds $hi:tt:$lo:tt $field:ident as bool) => {
+        #[allow(clippy::eq_op)]
+        const _: () = {
+            ::kernel::build_assert!(
+                $hi == $lo,
+                concat!("boolean field `", stringify!($field), "` covers more than one bit")
+            );
+        };
+    };
+
+    // Non-boolean fields must have `$hi >= $lo`.
+    (@check_field_bounds $hi:tt:$lo:tt $field:ident as $type:tt) => {
+        #[allow(clippy::eq_op)]
+        const _: () = {
+            ::kernel::build_assert!(
+                $hi >= $lo,
+                concat!("field `", stringify!($field), "`'s MSB is smaller than its LSB")
+            );
+        };
+    };
+
+    // Catches fields defined as `bool` and convert them into a boolean value.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
+            $(, $comment:literal)?;
+    ) => {
+        bitstruct!(
+            @leaf_accessor $name $hi:$lo $field
+            { |f| <$into_type>::from(if f != 0 { true } else { false }) }
+            $into_type => $into_type $(, $comment)?;
+        );
+    };
+
+    // Shortcut for fields defined as `bool` without the `=>` syntax.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
+    ) => {
+        bitstruct!(@field_accessor $name $hi:$lo $field as bool => bool $(, $comment)?;);
+    };
+
+    // Catches the `?=>` syntax for non-boolean fields.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
+            $(, $comment:literal)?;
+    ) => {
+        bitstruct!(@leaf_accessor $name $hi:$lo $field
+            { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
+            ::core::result::Result<
+                $try_into_type,
+                <$try_into_type as ::core::convert::TryFrom<$type>>::Error
+            >
+            $(, $comment)?;);
+    };
+
+    // Catches the `=>` syntax for non-boolean fields.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
+            $(, $comment:literal)?;
+    ) => {
+        bitstruct!(@leaf_accessor $name $hi:$lo $field
+            { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
+    };
+
+    // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
+    (
+        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt
+            $(, $comment:literal)?;
+    ) => {
+        bitstruct!(@field_accessor $name $hi:$lo $field as $type => $type $(, $comment)?;);
+    };
+
+    // 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)?;
+    ) => {
+        ::kernel::macros::paste!(
+        const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
+        const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
+        const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
+        );
+
+        $(
+        #[doc="Returns the value of this field:"]
+        #[doc=$comment]
+        )?
+        #[inline(always)]
+        pub(crate) fn $field(self) -> $res_type {
+            ::kernel::macros::paste!(
+            const MASK: u32 = $name::[<$field:upper _MASK>];
+            const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
+            );
+            let field = ((self.0 & MASK) >> SHIFT);
+
+            $process(field)
+        }
+
+        ::kernel::macros::paste!(
+        $(
+        #[doc="Sets the value of this field:"]
+        #[doc=$comment]
+        )?
+        #[inline(always)]
+        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;
+            self.0 = (self.0 & !MASK) | value;
+
+            self
+        }
+        );
+    };
+
+    // Generates the `Debug` implementation for `$name`.
+    (@debug $name:ident { $($field:ident;)* }) => {
+        impl ::core::fmt::Debug for $name {
+            fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
+                f.debug_struct(stringify!($name))
+                    .field("<raw>", &format_args!("{:#x}", &self.0))
+                $(
+                    .field(stringify!($field), &self.$field())
+                )*
+                    .finish()
+            }
+        }
+    };
+
+    // Generates the `Default` implementation for `$name`.
+    (@default $name:ident { $($field:ident;)* }) => {
+        /// Returns a value for the bitstruct where all fields are set to their default value.
+        impl ::core::default::Default for $name {
+            fn default() -> Self {
+                #[allow(unused_mut)]
+                let mut value = Self(Default::default());
+
+                ::kernel::macros::paste!(
+                $(
+                value.[<set_ $field>](Default::default());
+                )*
+                );
+
+                value
+            }
+        }
+    };
+}
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index cb2bbb30cba1..b218a2d42573 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -2,6 +2,9 @@
 
 //! Nova Core GPU Driver
 
+#[macro_use]
+mod bitstruct;
+
 mod dma;
 mod driver;
 mod falcon;
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 754c14ee7f40..3fb6852dff04 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -284,25 +284,25 @@ pub(crate) trait RegisterBase<T> {
 macro_rules! register {
     // Creates a register at a fixed offset of the MMIO space.
     ($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitstruct!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_fixed $name @ $offset);
     };
 
     // Creates an alias register of fixed offset register `alias` with its own fields.
     ($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } ) => {
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitstruct!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_fixed $name @ $alias::OFFSET);
     };
 
     // Creates a register at a relative offset from a base address provider.
     ($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? { $($fields:tt)* } ) => {
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitstruct!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $offset ]);
     };
 
     // Creates an alias register of relative offset register `alias` with its own fields.
     ($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { $($fields:tt)* }) => {
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitstruct!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET ]);
     };
 
@@ -313,7 +313,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitstruct!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_array $name @ $offset [ $size ; $stride ]);
     };
 
@@ -334,7 +334,7 @@ macro_rules! register {
             $(, $comment:literal)? { $($fields:tt)* }
     ) => {
         static_assert!(::core::mem::size_of::<u32>() <= $stride);
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitstruct!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
     };
 
@@ -356,7 +356,7 @@ macro_rules! register {
         }
     ) => {
         static_assert!($idx < $alias::SIZE);
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitstruct!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
     };
 
@@ -365,241 +365,10 @@ macro_rules! register {
     // to avoid it being interpreted in place of the relative register array alias rule.
     ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
         static_assert!($idx < $alias::SIZE);
-        register!(@core $name $(, $comment)? { $($fields)* } );
+        bitstruct!(struct $name $(, $comment)? { $($fields)* } );
         register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
     };
 
-    // All rules below are helpers.
-
-    // Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
-    // `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
-    (@core $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
-        $(
-        #[doc=$comment]
-        )?
-        #[repr(transparent)]
-        #[derive(Clone, Copy)]
-        pub(crate) struct $name(u32);
-
-        impl ::core::ops::BitOr for $name {
-            type Output = Self;
-
-            fn bitor(self, rhs: Self) -> Self::Output {
-                Self(self.0 | rhs.0)
-            }
-        }
-
-        impl ::core::convert::From<$name> for u32 {
-            fn from(reg: $name) -> u32 {
-                reg.0
-            }
-        }
-
-        register!(@fields_dispatcher $name { $($fields)* });
-    };
-
-    // Captures the fields and passes them to all the implementers that require field information.
-    //
-    // Used to simplify the matching rules for implementers, so they don't need to match the entire
-    // complex fields rule even though they only make use of part of it.
-    (@fields_dispatcher $name:ident {
-        $($hi:tt:$lo:tt $field:ident as $type:tt
-            $(?=> $try_into_type:ty)?
-            $(=> $into_type:ty)?
-            $(, $comment:literal)?
-        ;
-        )*
-    }
-    ) => {
-        register!(@field_accessors $name {
-            $(
-                $hi:$lo $field as $type
-                $(?=> $try_into_type)?
-                $(=> $into_type)?
-                $(, $comment)?
-            ;
-            )*
-        });
-        register!(@debug $name { $($field;)* });
-        register!(@default $name { $($field;)* });
-    };
-
-    // Defines all the field getter/methods methods for `$name`.
-    (
-        @field_accessors $name:ident {
-        $($hi:tt:$lo:tt $field:ident as $type:tt
-            $(?=> $try_into_type:ty)?
-            $(=> $into_type:ty)?
-            $(, $comment:literal)?
-        ;
-        )*
-        }
-    ) => {
-        $(
-            register!(@check_field_bounds $hi:$lo $field as $type);
-        )*
-
-        #[allow(dead_code)]
-        impl $name {
-            $(
-            register!(@field_accessor $name $hi:$lo $field as $type
-                $(?=> $try_into_type)?
-                $(=> $into_type)?
-                $(, $comment)?
-                ;
-            );
-            )*
-        }
-    };
-
-    // Boolean fields must have `$hi == $lo`.
-    (@check_field_bounds $hi:tt:$lo:tt $field:ident as bool) => {
-        #[allow(clippy::eq_op)]
-        const _: () = {
-            ::kernel::build_assert!(
-                $hi == $lo,
-                concat!("boolean field `", stringify!($field), "` covers more than one bit")
-            );
-        };
-    };
-
-    // Non-boolean fields must have `$hi >= $lo`.
-    (@check_field_bounds $hi:tt:$lo:tt $field:ident as $type:tt) => {
-        #[allow(clippy::eq_op)]
-        const _: () = {
-            ::kernel::build_assert!(
-                $hi >= $lo,
-                concat!("field `", stringify!($field), "`'s MSB is smaller than its LSB")
-            );
-        };
-    };
-
-    // Catches fields defined as `bool` and convert them into a boolean value.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
-            $(, $comment:literal)?;
-    ) => {
-        register!(
-            @leaf_accessor $name $hi:$lo $field
-            { |f| <$into_type>::from(if f != 0 { true } else { false }) }
-            $into_type => $into_type $(, $comment)?;
-        );
-    };
-
-    // Shortcut for fields defined as `bool` without the `=>` syntax.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
-    ) => {
-        register!(@field_accessor $name $hi:$lo $field as bool => bool $(, $comment)?;);
-    };
-
-    // Catches the `?=>` syntax for non-boolean fields.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
-            $(, $comment:literal)?;
-    ) => {
-        register!(@leaf_accessor $name $hi:$lo $field
-            { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
-            ::core::result::Result<
-                $try_into_type,
-                <$try_into_type as ::core::convert::TryFrom<$type>>::Error
-            >
-            $(, $comment)?;);
-    };
-
-    // Catches the `=>` syntax for non-boolean fields.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
-            $(, $comment:literal)?;
-    ) => {
-        register!(@leaf_accessor $name $hi:$lo $field
-            { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
-    };
-
-    // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
-    (
-        @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt
-            $(, $comment:literal)?;
-    ) => {
-        register!(@field_accessor $name $hi:$lo $field as $type => $type $(, $comment)?;);
-    };
-
-    // 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)?;
-    ) => {
-        ::kernel::macros::paste!(
-        const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
-        const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
-        const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
-        );
-
-        $(
-        #[doc="Returns the value of this field:"]
-        #[doc=$comment]
-        )?
-        #[inline(always)]
-        pub(crate) fn $field(self) -> $res_type {
-            ::kernel::macros::paste!(
-            const MASK: u32 = $name::[<$field:upper _MASK>];
-            const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
-            );
-            let field = ((self.0 & MASK) >> SHIFT);
-
-            $process(field)
-        }
-
-        ::kernel::macros::paste!(
-        $(
-        #[doc="Sets the value of this field:"]
-        #[doc=$comment]
-        )?
-        #[inline(always)]
-        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;
-            self.0 = (self.0 & !MASK) | value;
-
-            self
-        }
-        );
-    };
-
-    // Generates the `Debug` implementation for `$name`.
-    (@debug $name:ident { $($field:ident;)* }) => {
-        impl ::core::fmt::Debug for $name {
-            fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
-                f.debug_struct(stringify!($name))
-                    .field("<raw>", &format_args!("{:#x}", &self.0))
-                $(
-                    .field(stringify!($field), &self.$field())
-                )*
-                    .finish()
-            }
-        }
-    };
-
-    // Generates the `Default` implementation for `$name`.
-    (@default $name:ident { $($field:ident;)* }) => {
-        /// Returns a value for the register where all fields are set to their default value.
-        impl ::core::default::Default for $name {
-            fn default() -> Self {
-                #[allow(unused_mut)]
-                let mut value = Self(Default::default());
-
-                ::kernel::macros::paste!(
-                $(
-                value.[<set_ $field>](Default::default());
-                )*
-                );
-
-                value
-            }
-        }
-    };
-
     // Generates the IO accessors for a fixed offset register.
     (@io_fixed $name:ident @ $offset:expr) => {
         #[allow(dead_code)]
-- 
2.34.1

On Thu Sep 4, 2025 at 6:54 AM JST, Joel Fernandes wrote:
> The bitfield-specific into new macro. This will be used to define
> structs with bitfields, similar to C language.
>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
>  drivers/gpu/nova-core/bitstruct.rs   | 271 +++++++++++++++++++++++++++
>  drivers/gpu/nova-core/nova_core.rs   |   3 +
>  drivers/gpu/nova-core/regs/macros.rs | 247 +-----------------------
>  3 files changed, 282 insertions(+), 239 deletions(-)
>  create mode 100644 drivers/gpu/nova-core/bitstruct.rs
>
> diff --git a/drivers/gpu/nova-core/bitstruct.rs b/drivers/gpu/nova-core/bitstruct.rs
> new file mode 100644
> index 000000000000..1dd9edab7d07
> --- /dev/null
> +++ b/drivers/gpu/nova-core/bitstruct.rs
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// bitstruct.rs — Bitfield library for Rust structures
> +//
> +// A library that provides support for defining bit fields in Rust
> +// structures. Also used from things that need bitfields like register macro.
> +///
> +/// # Syntax
> +///
> +/// ```rust
> +/// bitstruct! {
> +///     struct ControlReg {

The `struct` naming here looks a bit confusing to me - as of this patch,
this is a u32, right? And eventually these types will be limited to primitive types,
so why not just `ControlReg: u32 {` ?

> +///         3:0       mode        as u8 ?=> Mode;
> +///         7:4       state       as u8 => State;
> +///     }
> +/// }
> +/// ```

As this will move to the kernel crate, it is particularly important to
make sure that this example can compile and run - so please provide
simple definitions for `Mode` and `State` to make sure the kunit tests
will pass after patch 4 (in the current state I'm pretty sure they won't).

> +///
> +/// This generates a struct with:
> +/// - Field accessors: `mode()`, `state()`, etc.
> +/// - Field setters: `set_mode()`, `set_state()`, etc. (supports builder pattern).
> +/// - Debug and Default implementations
> +///
> +/// The field setters can be used with the builder pattern, example:
> +/// ControlReg::default().set_mode(mode).set_state(state);
> +///
> +/// Fields are defined as follows:
> +///
> +/// - `as <type>` simply returns the field value casted to <type>, typically `u32`, `u16`, `u8` or
> +///   `bool`. Note that `bool` fields must have a range of 1 bit.
> +/// - `as <type> => <into_type>` calls `<into_type>`'s `From::<<type>>` implementation and returns
> +///   the result.
> +/// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s `TryFrom::<<type>>` implementation
> +///   and returns the result. This is useful with fields for which not all values are valid.

Can you remove the related documentation from `register!` and replace it
with a sentence like "Please look at the [`bitstruct`] macro for the
complete syntax of fields definitions"? This will ensure we don't have
to update the documentation twice if/when the syntax gets updated.

The rest of the patch is a perfect move (with a few renames) of the
internal rules from one macro to the other, which makes it really easy
to review. Thanks for doing this!

Hi Alex,

On 9/7/2025 11:12 PM, Alexandre Courbot wrote:
> On Thu Sep 4, 2025 at 6:54 AM JST, Joel Fernandes wrote:
>> The bitfield-specific into new macro. This will be used to define
>> structs with bitfields, similar to C language.
>>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/bitstruct.rs   | 271 +++++++++++++++++++++++++++
>>  drivers/gpu/nova-core/nova_core.rs   |   3 +
>>  drivers/gpu/nova-core/regs/macros.rs | 247 +-----------------------
>>  3 files changed, 282 insertions(+), 239 deletions(-)
>>  create mode 100644 drivers/gpu/nova-core/bitstruct.rs
>>
>> diff --git a/drivers/gpu/nova-core/bitstruct.rs b/drivers/gpu/nova-core/bitstruct.rs
>> new file mode 100644
>> index 000000000000..1dd9edab7d07
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/bitstruct.rs
>> @@ -0,0 +1,271 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// bitstruct.rs — Bitfield library for Rust structures
>> +//
>> +// A library that provides support for defining bit fields in Rust
>> +// structures. Also used from things that need bitfields like register macro.
>> +///
>> +/// # Syntax
>> +///
>> +/// ```rust
>> +/// bitstruct! {
>> +///     struct ControlReg {
> 
> The `struct` naming here looks a bit confusing to me - as of this patch,
> this is a u32, right? And eventually these types will be limited to primitive types,
> so why not just `ControlReg: u32 {` ?

This is done in a later patch. This patch is only code movement, in later patch
we add precisely the syntax you're describing when we add storage types, and
update the register! macro. In this patch bitstruct is only u32.

> 
>> +///         3:0       mode        as u8 ?=> Mode;
>> +///         7:4       state       as u8 => State;
>> +///     }
>> +/// }
>> +/// ```
> 
> As this will move to the kernel crate, it is particularly important to
> make sure that this example can compile and run - so please provide
> simple definitions for `Mode` and `State` to make sure the kunit tests
> will pass after patch 4 (in the current state I'm pretty sure they won't).

Good catch. This will blow up the example though. I will change it to no_run
like the register! macro did if that's Ok.

> 
>> +///
>> +/// This generates a struct with:
>> +/// - Field accessors: `mode()`, `state()`, etc.
>> +/// - Field setters: `set_mode()`, `set_state()`, etc. (supports builder pattern).
>> +/// - Debug and Default implementations
>> +///
>> +/// The field setters can be used with the builder pattern, example:
>> +/// ControlReg::default().set_mode(mode).set_state(state);
>> +///
>> +/// Fields are defined as follows:
>> +///
>> +/// - `as <type>` simply returns the field value casted to <type>, typically `u32`, `u16`, `u8` or
>> +///   `bool`. Note that `bool` fields must have a range of 1 bit.
>> +/// - `as <type> => <into_type>` calls `<into_type>`'s `From::<<type>>` implementation and returns
>> +///   the result.
>> +/// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s `TryFrom::<<type>>` implementation
>> +///   and returns the result. This is useful with fields for which not all values are valid.
> 
> Can you remove the related documentation from `register!` and replace it
> with a sentence like "Please look at the [`bitstruct`] macro for the
> complete syntax of fields definitions"? This will ensure we don't have
> to update the documentation twice if/when the syntax gets updated.

Sure!

> 
> The rest of the patch is a perfect move (with a few renames) of the
> internal rules from one macro to the other, which makes it really easy
> to review. Thanks for doing this!

My pleasure, thanks for the suggestion!

 - Joel

On Tue Sep 9, 2025 at 2:16 AM JST, Joel Fernandes wrote:
> Hi Alex,
>
> On 9/7/2025 11:12 PM, Alexandre Courbot wrote:
>> On Thu Sep 4, 2025 at 6:54 AM JST, Joel Fernandes wrote:
>>> The bitfield-specific into new macro. This will be used to define
>>> structs with bitfields, similar to C language.
>>>
>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>>> ---
>>>  drivers/gpu/nova-core/bitstruct.rs   | 271 +++++++++++++++++++++++++++
>>>  drivers/gpu/nova-core/nova_core.rs   |   3 +
>>>  drivers/gpu/nova-core/regs/macros.rs | 247 +-----------------------
>>>  3 files changed, 282 insertions(+), 239 deletions(-)
>>>  create mode 100644 drivers/gpu/nova-core/bitstruct.rs
>>>
>>> diff --git a/drivers/gpu/nova-core/bitstruct.rs b/drivers/gpu/nova-core/bitstruct.rs
>>> new file mode 100644
>>> index 000000000000..1dd9edab7d07
>>> --- /dev/null
>>> +++ b/drivers/gpu/nova-core/bitstruct.rs
>>> @@ -0,0 +1,271 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +//
>>> +// bitstruct.rs — Bitfield library for Rust structures
>>> +//
>>> +// A library that provides support for defining bit fields in Rust
>>> +// structures. Also used from things that need bitfields like register macro.
>>> +///
>>> +/// # Syntax
>>> +///
>>> +/// ```rust
>>> +/// bitstruct! {
>>> +///     struct ControlReg {
>> 
>> The `struct` naming here looks a bit confusing to me - as of this patch,
>> this is a u32, right? And eventually these types will be limited to primitive types,
>> so why not just `ControlReg: u32 {` ?
>
> This is done in a later patch. This patch is only code movement, in later patch
> we add precisely the syntax you're describing when we add storage types, and
> update the register! macro. In this patch bitstruct is only u32.

My point was, is the `struct` keyword needed at all? Isn't it a bit
confusing since these types are technically not Rust structs?

I agree the `: u32` can be introduced later, the original `register!`
macro did not specify any type information so there is indeed no reason
to add it in this patch.

>
>> 
>>> +///         3:0       mode        as u8 ?=> Mode;
>>> +///         7:4       state       as u8 => State;
>>> +///     }
>>> +/// }
>>> +/// ```
>> 
>> As this will move to the kernel crate, it is particularly important to
>> make sure that this example can compile and run - so please provide
>> simple definitions for `Mode` and `State` to make sure the kunit tests
>> will pass after patch 4 (in the current state I'm pretty sure they won't).
>
> Good catch. This will blow up the example though. I will change it to no_run
> like the register! macro did if that's Ok.

If you reduce `State` to 1 bit and change its type to `bool`, and limit
`Mode` to two or three variants, the example should remain short. I
think it is valuable to provide a complete working example here as the
syntax is not obvious at first sight.

On Tue, Sep 09, 2025 at 11:37:15AM +0900, Alexandre Courbot wrote:
> On Tue Sep 9, 2025 at 2:16 AM JST, Joel Fernandes wrote:
> > Hi Alex,
> >
> > On 9/7/2025 11:12 PM, Alexandre Courbot wrote:
> >> On Thu Sep 4, 2025 at 6:54 AM JST, Joel Fernandes wrote:
> >>> The bitfield-specific into new macro. This will be used to define
> >>> structs with bitfields, similar to C language.
> >>>
> >>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> >>> ---
> >>>  drivers/gpu/nova-core/bitstruct.rs   | 271 +++++++++++++++++++++++++++
> >>>  drivers/gpu/nova-core/nova_core.rs   |   3 +
> >>>  drivers/gpu/nova-core/regs/macros.rs | 247 +-----------------------
> >>>  3 files changed, 282 insertions(+), 239 deletions(-)
> >>>  create mode 100644 drivers/gpu/nova-core/bitstruct.rs
> >>>
> >>> diff --git a/drivers/gpu/nova-core/bitstruct.rs b/drivers/gpu/nova-core/bitstruct.rs
> >>> new file mode 100644
> >>> index 000000000000..1dd9edab7d07
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/nova-core/bitstruct.rs
> >>> @@ -0,0 +1,271 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +//
> >>> +// bitstruct.rs — Bitfield library for Rust structures
> >>> +//
> >>> +// A library that provides support for defining bit fields in Rust
> >>> +// structures. Also used from things that need bitfields like register macro.
> >>> +///
> >>> +/// # Syntax
> >>> +///
> >>> +/// ```rust
> >>> +/// bitstruct! {
> >>> +///     struct ControlReg {
> >> 
> >> The `struct` naming here looks a bit confusing to me - as of this patch,
> >> this is a u32, right? And eventually these types will be limited to primitive types,
> >> so why not just `ControlReg: u32 {` ?
> >
> > This is done in a later patch. This patch is only code movement, in later patch
> > we add precisely the syntax you're describing when we add storage types, and
> > update the register! macro. In this patch bitstruct is only u32.
> 
> My point was, is the `struct` keyword needed at all? Isn't it a bit
> confusing since these types are technically not Rust structs?

Now that bitstruct has changed to bitfield, I would really insist on leaving
'struct' in there.

So it will look like this:

//! bitfield! {
//!     struct ControlReg {
//!         3:0       mode        as u8 ?=> Mode;
//!         7         state       as bool => State;
//!     }
//! }

Sounds reasonable?

> I agree the `: u32` can be introduced later, the original `register!`
> macro did not specify any type information so there is indeed no reason
> to add it in this patch.

Yep.

> >
> >> 
> >>> +///         3:0       mode        as u8 ?=> Mode;
> >>> +///         7:4       state       as u8 => State;
> >>> +///     }
> >>> +/// }
> >>> +/// ```
> >> 
> >> As this will move to the kernel crate, it is particularly important to
> >> make sure that this example can compile and run - so please provide
> >> simple definitions for `Mode` and `State` to make sure the kunit tests
> >> will pass after patch 4 (in the current state I'm pretty sure they won't).
> >
> > Good catch. This will blow up the example though. I will change it to no_run
> > like the register! macro did if that's Ok.
> 
> If you reduce `State` to 1 bit and change its type to `bool`, and limit
> `Mode` to two or three variants, the example should remain short. I
> think it is valuable to provide a complete working example here as the
> syntax is not obvious at first sight.

Ok, so it will look like this, still about 40 lines more, but that works for me.

@@ -7,11 +7,54 @@
 //!
 //! # Syntax
 //!
-//! ```no_run
+//! ```rust
+//! #[derive(Debug, Clone, Copy)]
+//! enum Mode {
+//!     Low = 0,
+//!     High = 1,
+//!     Auto = 2,
+//! }
+//!
+//! impl TryFrom<u8> for Mode {
+//!     type Error = u8;
+//!     fn try_from(value: u8) -> Result<Self, Self::Error> {
+//!         match value {
+//!             0 => Ok(Mode::Low),
+//!             1 => Ok(Mode::High),
+//!             2 => Ok(Mode::Auto),
+//!             _ => Err(value),
+//!         }
+//!     }
+//! }
+//!
+//! impl From<Mode> for u32 {
+//!     fn from(mode: Mode) -> u32 {
+//!         mode as u32
+//!     }
+//! }
+//!
+//! #[derive(Debug, Clone, Copy)]
+//! enum State {
+//!     Inactive = 0,
+//!     Active = 1,
+//! }
+//!
+//! impl From<bool> for State {
+//!     fn from(value: bool) -> Self {
+//!         if value { State::Active } else { State::Inactive }
+//!     }
+//! }
+//!
+//! impl From<State> for u32 {
+//!     fn from(state: State) -> u32 {
+//!         state as u32
+//!     }
+//! }
+//!
 //! bitfield! {
 //!     struct ControlReg {
 //!         3:0       mode        as u8 ?=> Mode;
-//!         7:4       state       as u8 => State;
+//!         7         state       as bool => State;
 //!     }
 //! }
 //! ```

 thanks,

  - Joel

On Wed Sep 10, 2025 at 3:55 AM JST, Joel Fernandes wrote:
> On Tue, Sep 09, 2025 at 11:37:15AM +0900, Alexandre Courbot wrote:
>> On Tue Sep 9, 2025 at 2:16 AM JST, Joel Fernandes wrote:
>> > Hi Alex,
>> >
>> > On 9/7/2025 11:12 PM, Alexandre Courbot wrote:
>> >> On Thu Sep 4, 2025 at 6:54 AM JST, Joel Fernandes wrote:
>> >>> The bitfield-specific into new macro. This will be used to define
>> >>> structs with bitfields, similar to C language.
>> >>>
>> >>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> >>> ---
>> >>>  drivers/gpu/nova-core/bitstruct.rs   | 271 +++++++++++++++++++++++++++
>> >>>  drivers/gpu/nova-core/nova_core.rs   |   3 +
>> >>>  drivers/gpu/nova-core/regs/macros.rs | 247 +-----------------------
>> >>>  3 files changed, 282 insertions(+), 239 deletions(-)
>> >>>  create mode 100644 drivers/gpu/nova-core/bitstruct.rs
>> >>>
>> >>> diff --git a/drivers/gpu/nova-core/bitstruct.rs b/drivers/gpu/nova-core/bitstruct.rs
>> >>> new file mode 100644
>> >>> index 000000000000..1dd9edab7d07
>> >>> --- /dev/null
>> >>> +++ b/drivers/gpu/nova-core/bitstruct.rs
>> >>> @@ -0,0 +1,271 @@
>> >>> +// SPDX-License-Identifier: GPL-2.0
>> >>> +//
>> >>> +// bitstruct.rs — Bitfield library for Rust structures
>> >>> +//
>> >>> +// A library that provides support for defining bit fields in Rust
>> >>> +// structures. Also used from things that need bitfields like register macro.
>> >>> +///
>> >>> +/// # Syntax
>> >>> +///
>> >>> +/// ```rust
>> >>> +/// bitstruct! {
>> >>> +///     struct ControlReg {
>> >> 
>> >> The `struct` naming here looks a bit confusing to me - as of this patch,
>> >> this is a u32, right? And eventually these types will be limited to primitive types,
>> >> so why not just `ControlReg: u32 {` ?
>> >
>> > This is done in a later patch. This patch is only code movement, in later patch
>> > we add precisely the syntax you're describing when we add storage types, and
>> > update the register! macro. In this patch bitstruct is only u32.
>> 
>> My point was, is the `struct` keyword needed at all? Isn't it a bit
>> confusing since these types are technically not Rust structs?
>
> Now that bitstruct has changed to bitfield, I would really insist on leaving
> 'struct' in there.
>
> So it will look like this:
>
> //! bitfield! {
> //!     struct ControlReg {
> //!         3:0       mode        as u8 ?=> Mode;
> //!         7         state       as bool => State;
> //!     }
> //! }
>
> Sounds reasonable?

I was about to write "but it not a struct", and then I remembered that
the body of the macro does this:

        pub(crate) struct $name(u32);

... so there goes my argument. :') Just one more thing below.

>
>> I agree the `: u32` can be introduced later, the original `register!`
>> macro did not specify any type information so there is indeed no reason
>> to add it in this patch.
>
> Yep.

When you introduce the types, can you change the syntax from `: u32` to
`(u32)`? That way the declaration becomes

    bitfield! {
        struct ControlReg(u32) {
            ...
        }
    }

... which at least looks like a valid declaration for a Rust struct
that wraps a primitive type. Same for registers, if possible.

On 9/10/2025 9:25 AM, Alexandre Courbot wrote:
>>> I agree the `: u32` can be introduced later, the original `register!`
>>> macro did not specify any type information so there is indeed no reason
>>> to add it in this patch.
>>>> Yep.
>> When you introduce the types, can you change the syntax from `: u32` to
> `(u32)`? That way the declaration becomes
> 
>     bitfield! {
>         struct ControlReg(u32) {
>             ...
>         }
>     }
> 
> ... which at least looks like a valid declaration for a Rust struct
> that wraps a primitive type. Same for registers, if possible.

Sure, I agree with your suggestion and will make the changes.

thanks,

 - Joel