Re: [PATCH v2 1/4] nova-core: bitstruct: Move bitfield-specific code from register! into new macro

["Alexandre Courbot" <acourbot@nvidia.com>]

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.