> On Oct 16, 2025, at 1:48 PM, Yury Norov <[email protected]> wrote: > > On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote: >> Move the bitfield-specific code from the register macro into a new macro >> called bitfield. This will be used to define structs with bitfields, >> similar to C language. > > Can you please fix line length issues before v8? > > $ awk '{print length}' drivers/gpu/nova-core/bitfield.rs | sort -rn | uniq -c > 1 118 > 1 116 > 1 113 > 1 109 > 1 105 > 1 103
That is intentional. I will look again but long lines can be a matter of style and if wrapping effects readability then we do not want to do that. That is why it is a checkpatch warning not an error. We have to look it case by case. > ... > >> Reviewed-by: Elle Rhumsaa <[email protected]> >> Reviewed-by: Alexandre Courbot <[email protected]> >> Reviewed-by: Edwin Peer <[email protected]> >> Signed-off-by: Joel Fernandes <[email protected]> >> --- >> drivers/gpu/nova-core/bitfield.rs | 319 +++++++++++++++++++++++++++ >> drivers/gpu/nova-core/nova_core.rs | 3 + >> drivers/gpu/nova-core/regs/macros.rs | 259 +--------------------- >> 3 files changed, 332 insertions(+), 249 deletions(-) >> create mode 100644 drivers/gpu/nova-core/bitfield.rs > > ... > >> +/// >> +/// bitfield! { >> +/// struct ControlReg { >> +/// 3:0 mode as u8 ?=> Mode; >> +/// 7:7 state as bool => State; >> +/// } >> +/// } > > This notation is really unwelcome this days. It may be OK for a random > macro in some local driver, but doesn't really work for a global basic > data type: > > https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sxgpjrdbfqfwlb3bi13...@mail.gmail.com/ > > I've already shared this link with you, and shared my concern. > > I realize that rust/bitfield derives the GENMASK(hi, lo) notation here, > and GENMASK() derives verilog or hardware specs popular notations. But > software people prefer lo:hi. I'm probably OK if you choose C-style > start:nbits, if you prefer. But let's stop this hi:lo early, please. > > Let me quote Linus from the link above: > > It does "high, low", which is often very unintuitive, and in fact the > very commit that introduced this thing from hell had to convert the > sane "low,high" cases to the other way around. I agree with Linus but I disagree with comparing it with these macros. I agree with Linus it is oddly unreadable when used as function parameters. But that is a different syntax. Over here we are using colons with sufficient whitespace around hi:lo. > > See commit 10ef6b0dffe4 ("bitops: Introduce a more generic BITMASK > macro"), and notice how ALMOST ALL use cases were switched around to > the illogical "high,low" format by that introductory phase. Again this is different syntax so assuming that Linus will not be ok with it is a stretch IMO. The rust macros here have their own syntax and are not function parameters. > > And yes, I understand why that person did it: many datasheets show > bits in a register graphically, and then you see that "high .. low" > thing in a rectangle that describes the register, and that ordering > them makes 100% sense IN THAT CONTEXT. > > But it damn well does not make sense in most other contexts. > > In fact, even in the context of generating mask #defines, it actually > reads oddly, because you end up having things like > > /* Status register (SR) */ > #define I2C_SR_OP GENMASK(1, 0) /* Operation */ > #define I2C_SR_STATUS GENMASK(3, 2) /* controller status */ > #define I2C_SR_CAUSE GENMASK(6, 4) /* Abort cause */ > #define I2C_SR_TYPE GENMASK(8, 7) /* Receive type */ > #define I2C_SR_LENGTH GENMASK(19, 9) /* Transfer length */ Sure this is super odd indeed. But again not apples to apples here. thanks, - Joel > > ... > > Now compare it to what we've got in nova right now: > > register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the > GPU" { > 3:0 minor_revision as u8, "Minor revision of the chip"; > 7:4 major_revision as u8, "Major revision of the chip"; > 8:8 architecture_1 as u8, "MSB of the architecture"; > 23:20 implementation as u8, "Implementation version of the > architecture"; > 28:24 architecture_0 as u8, "Lower bits of the architecture"; > }); > > There's so far 36 thousands of GENMASK()s in the kernel, and only 67 > register!()s. It's a separate topic what to do with the GENMASK() > codebase. But now that you do this massive refactoring for the > register!() macro, let's convert those 67 users to the lo:hi notation. > > As a side note, for GENMASKs, I tried this trick: > > #define GENMASK(a, b) UNSAFE_GENMASK(MIN(a, b), MAX(a, b)) > > It works, but bloats defconfig kernel for another 1K. I don't think it > would add to readability on both C and rust sides. > > Thanks, > Yury
