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 <joelagn...@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
