The builder-pattern setters (self -> Self) enabled method chaining like:

    reg.set_foo(x).set_sec(y).write(bar);

This made separate operations appear as a single expression, obscuring
that each setter is a distinct mutation. These setters are infallible,
so the chaining provides no error-propagation benefit—it just obscures
what are simple, independent assignments.

Change the bitfield!() macro to generate `&mut self` setters, so each
operation is a distinct statement:

    reg.set_foo(x);
    reg.set_sec(y);
    reg.write(bar);

Update all call sites and change update() closures to take `&mut Self`.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/bitfield.rs         |  4 +-
 drivers/gpu/nova-core/falcon.rs           | 98 ++++++++++++-----------
 drivers/gpu/nova-core/falcon/gsp.rs       |  6 +-
 drivers/gpu/nova-core/falcon/hal/ga102.rs | 33 ++++----
 drivers/gpu/nova-core/fb/hal/ga100.rs     | 21 ++---
 drivers/gpu/nova-core/fb/hal/tu102.rs     |  6 +-
 drivers/gpu/nova-core/gsp/cmdq.rs         |  6 +-
 drivers/gpu/nova-core/gsp/fw.rs           |  7 +-
 drivers/gpu/nova-core/regs/macros.rs      | 41 ++++++----
 9 files changed, 120 insertions(+), 102 deletions(-)

diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
index 16e143658c51..be152d1e08e0 100644
--- a/drivers/gpu/nova-core/bitfield.rs
+++ b/drivers/gpu/nova-core/bitfield.rs
@@ -284,13 +284,11 @@ impl $name {
         #[doc=$comment]
         )?
         #[inline(always)]
-        $vis fn [<set_ $field>](mut self, value: $to_type) -> Self {
+        $vis fn [<set_ $field>](&mut self, value: $to_type) {
             const MASK: $storage = $name::[<$field:upper _MASK>];
             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
             let value = ($storage::from($prim_type::from(value)) << SHIFT) & MASK;
             self.0 = (self.0 & !MASK) | value;
-
-            self
         }
         );
     };
diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 82c661aef594..66fd37c73a3a 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -413,12 +413,12 @@ fn reset_eng(&self, bar: &Bar0) -> Result {
             Delta::from_micros(150),
         );
 
-        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| v.set_reset(true));
+        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| { v.set_reset(true); });
 
         // TIMEOUT: falcon engine should not take more than 10us to reset.
         fsleep(Delta::from_micros(10));
 
-        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| v.set_reset(false));
+        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| { v.set_reset(false); });
 
         self.reset_wait_mem_scrubbing(bar)?;
 
@@ -431,9 +431,9 @@ pub(crate) fn reset(&self, bar: &Bar0) -> Result {
         self.hal.select_core(self, bar)?;
         self.reset_wait_mem_scrubbing(bar)?;
 
-        regs::NV_PFALCON_FALCON_RM::default()
-            .set_value(regs::NV_PMC_BOOT_0::read(bar).into())
-            .write(bar, &E::ID);
+        let mut reg = regs::NV_PFALCON_FALCON_RM::default();
+        reg.set_value(regs::NV_PMC_BOOT_0::read(bar).into());
+        reg.write(bar, &E::ID);
 
         Ok(())
     }
@@ -495,30 +495,32 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
 
         // Set up the base source DMA address.
 
-        regs::NV_PFALCON_FALCON_DMATRFBASE::default()
-            // CAST: `as u32` is used on purpose since we do want to strip the upper bits, which
-            // will be written to `NV_PFALCON_FALCON_DMATRFBASE1`.
-            .set_base((dma_start >> 8) as u32)
-            .write(bar, &E::ID);
-        regs::NV_PFALCON_FALCON_DMATRFBASE1::default()
-            // CAST: `as u16` is used on purpose since the remaining bits are guaranteed to fit
-            // within a `u16`.
-            .set_base((dma_start >> 40) as u16)
-            .write(bar, &E::ID);
-
-        let cmd = regs::NV_PFALCON_FALCON_DMATRFCMD::default()
-            .set_size(DmaTrfCmdSize::Size256B)
-            .set_imem(target_mem == FalconMem::Imem)
-            .set_sec(if sec { 1 } else { 0 });
+        // CAST: `as u32` is used on purpose since we do want to strip the upper bits, which
+        // will be written to `NV_PFALCON_FALCON_DMATRFBASE1`.
+        let mut reg = regs::NV_PFALCON_FALCON_DMATRFBASE::default();
+        reg.set_base((dma_start >> 8) as u32);
+        reg.write(bar, &E::ID);
+
+        // CAST: `as u16` is used on purpose since the remaining bits are guaranteed to fit
+        // within a `u16`.
+        let mut reg = regs::NV_PFALCON_FALCON_DMATRFBASE1::default();
+        reg.set_base((dma_start >> 40) as u16);
+        reg.write(bar, &E::ID);
+
+        let mut cmd = regs::NV_PFALCON_FALCON_DMATRFCMD::default();
+        cmd.set_size(DmaTrfCmdSize::Size256B);
+        cmd.set_imem(target_mem == FalconMem::Imem);
+        cmd.set_sec(if sec { 1 } else { 0 });
 
         for pos in (0..num_transfers).map(|i| i * DMA_LEN) {
             // Perform a transfer of size `DMA_LEN`.
-            regs::NV_PFALCON_FALCON_DMATRFMOFFS::default()
-                .set_offs(load_offsets.dst_start + pos)
-                .write(bar, &E::ID);
-            regs::NV_PFALCON_FALCON_DMATRFFBOFFS::default()
-                .set_offs(src_start + pos)
-                .write(bar, &E::ID);
+            let mut reg = regs::NV_PFALCON_FALCON_DMATRFMOFFS::default();
+            reg.set_offs(load_offsets.dst_start + pos);
+            reg.write(bar, &E::ID);
+
+            let mut reg = regs::NV_PFALCON_FALCON_DMATRFFBOFFS::default();
+            reg.set_offs(src_start + pos);
+            reg.write(bar, &E::ID);
             cmd.write(bar, &E::ID);
 
             // Wait for the transfer to complete.
@@ -539,8 +541,8 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
     pub(crate) fn dma_load<F: FalconFirmware<Target = E>>(&self, bar: &Bar0, fw: &F) -> Result {
         self.dma_reset(bar);
         regs::NV_PFALCON_FBIF_TRANSCFG::update(bar, &E::ID, 0, |v| {
-            v.set_target(FalconFbifTarget::CoherentSysmem)
-                .set_mem_type(FalconFbifMemType::Physical)
+            v.set_target(FalconFbifTarget::CoherentSysmem);
+            v.set_mem_type(FalconFbifMemType::Physical);
         });
 
         self.dma_wr(bar, fw, FalconMem::Imem, fw.imem_load_params(), true)?;
@@ -549,9 +551,9 @@ pub(crate) fn dma_load<F: FalconFirmware<Target = E>>(&self, bar: &Bar0, fw: &F)
         self.hal.program_brom(self, bar, &fw.brom_params())?;
 
         // Set `BootVec` to start of non-secure code.
-        regs::NV_PFALCON_FALCON_BOOTVEC::default()
-            .set_value(fw.boot_addr())
-            .write(bar, &E::ID);
+        let mut reg = regs::NV_PFALCON_FALCON_BOOTVEC::default();
+        reg.set_value(fw.boot_addr());
+        reg.write(bar, &E::ID);
 
         Ok(())
     }
@@ -572,12 +574,16 @@ pub(crate) fn wait_till_halted(&self, bar: &Bar0) -> Result<()> {
     /// Start the falcon CPU.
     pub(crate) fn start(&self, bar: &Bar0) -> Result<()> {
         match regs::NV_PFALCON_FALCON_CPUCTL::read(bar, &E::ID).alias_en() {
-            true => regs::NV_PFALCON_FALCON_CPUCTL_ALIAS::default()
-                .set_startcpu(true)
-                .write(bar, &E::ID),
-            false => regs::NV_PFALCON_FALCON_CPUCTL::default()
-                .set_startcpu(true)
-                .write(bar, &E::ID),
+            true => {
+                let mut reg = regs::NV_PFALCON_FALCON_CPUCTL_ALIAS::default();
+                reg.set_startcpu(true);
+                reg.write(bar, &E::ID);
+            }
+            false => {
+                let mut reg = regs::NV_PFALCON_FALCON_CPUCTL::default();
+                reg.set_startcpu(true);
+                reg.write(bar, &E::ID);
+            }
         }
 
         Ok(())
@@ -586,15 +592,15 @@ pub(crate) fn start(&self, bar: &Bar0) -> Result<()> {
     /// Writes values to the mailbox registers if provided.
     pub(crate) fn write_mailboxes(&self, bar: &Bar0, mbox0: Option<u32>, mbox1: Option<u32>) {
         if let Some(mbox0) = mbox0 {
-            regs::NV_PFALCON_FALCON_MAILBOX0::default()
-                .set_value(mbox0)
-                .write(bar, &E::ID);
+            let mut reg = regs::NV_PFALCON_FALCON_MAILBOX0::default();
+            reg.set_value(mbox0);
+            reg.write(bar, &E::ID);
         }
 
         if let Some(mbox1) = mbox1 {
-            regs::NV_PFALCON_FALCON_MAILBOX1::default()
-                .set_value(mbox1)
-                .write(bar, &E::ID);
+            let mut reg = regs::NV_PFALCON_FALCON_MAILBOX1::default();
+            reg.set_value(mbox1);
+            reg.write(bar, &E::ID);
         }
     }
 
@@ -657,8 +663,8 @@ pub(crate) fn is_riscv_active(&self, bar: &Bar0) -> bool {
 
     /// Write the application version to the OS register.
     pub(crate) fn write_os_version(&self, bar: &Bar0, app_version: u32) {
-        regs::NV_PFALCON_FALCON_OS::default()
-            .set_value(app_version)
-            .write(bar, &E::ID);
+        let mut reg = regs::NV_PFALCON_FALCON_OS::default();
+        reg.set_value(app_version);
+        reg.write(bar, &E::ID);
     }
 }
diff --git a/drivers/gpu/nova-core/falcon/gsp.rs b/drivers/gpu/nova-core/falcon/gsp.rs
index 67edef3636c1..ce76c75cfdc6 100644
--- a/drivers/gpu/nova-core/falcon/gsp.rs
+++ b/drivers/gpu/nova-core/falcon/gsp.rs
@@ -39,9 +39,9 @@ impl Falcon<Gsp> {
     /// Clears the SWGEN0 bit in the Falcon's IRQ status clear register to
     /// allow GSP to signal CPU for processing new messages in message queue.
     pub(crate) fn clear_swgen0_intr(&self, bar: &Bar0) {
-        regs::NV_PFALCON_FALCON_IRQSCLR::default()
-            .set_swgen0(true)
-            .write(bar, &Gsp::ID);
+        let mut reg = regs::NV_PFALCON_FALCON_IRQSCLR::default();
+        reg.set_swgen0(true);
+        reg.write(bar, &Gsp::ID);
     }
 
     /// Checks if GSP reload/resume has completed during the boot process.
diff --git a/drivers/gpu/nova-core/falcon/hal/ga102.rs b/drivers/gpu/nova-core/falcon/hal/ga102.rs
index 69a7a95cac16..4fb94c727b65 100644
--- a/drivers/gpu/nova-core/falcon/hal/ga102.rs
+++ b/drivers/gpu/nova-core/falcon/hal/ga102.rs
@@ -26,9 +26,9 @@
 fn select_core_ga102<E: FalconEngine>(bar: &Bar0) -> Result {
     let bcr_ctrl = regs::NV_PRISCV_RISCV_BCR_CTRL::read(bar, &E::ID);
     if bcr_ctrl.core_select() != PeregrineCoreSelect::Falcon {
-        regs::NV_PRISCV_RISCV_BCR_CTRL::default()
-            .set_core_select(PeregrineCoreSelect::Falcon)
-            .write(bar, &E::ID);
+        let mut reg = regs::NV_PRISCV_RISCV_BCR_CTRL::default();
+        reg.set_core_select(PeregrineCoreSelect::Falcon);
+        reg.write(bar, &E::ID);
 
         // TIMEOUT: falcon core should take less than 10ms to report being enabled.
         read_poll_timeout(
@@ -75,18 +75,21 @@ fn signature_reg_fuse_version_ga102(
 }
 
 fn program_brom_ga102<E: FalconEngine>(bar: &Bar0, params: &FalconBromParams) -> Result {
-    regs::NV_PFALCON2_FALCON_BROM_PARAADDR::default()
-        .set_value(params.pkc_data_offset)
-        .write(bar, &E::ID, 0);
-    regs::NV_PFALCON2_FALCON_BROM_ENGIDMASK::default()
-        .set_value(u32::from(params.engine_id_mask))
-        .write(bar, &E::ID);
-    regs::NV_PFALCON2_FALCON_BROM_CURR_UCODE_ID::default()
-        .set_ucode_id(params.ucode_id)
-        .write(bar, &E::ID);
-    regs::NV_PFALCON2_FALCON_MOD_SEL::default()
-        .set_algo(FalconModSelAlgo::Rsa3k)
-        .write(bar, &E::ID);
+    let mut reg = regs::NV_PFALCON2_FALCON_BROM_PARAADDR::default();
+    reg.set_value(params.pkc_data_offset);
+    reg.write(bar, &E::ID, 0);
+
+    let mut reg = regs::NV_PFALCON2_FALCON_BROM_ENGIDMASK::default();
+    reg.set_value(u32::from(params.engine_id_mask));
+    reg.write(bar, &E::ID);
+
+    let mut reg = regs::NV_PFALCON2_FALCON_BROM_CURR_UCODE_ID::default();
+    reg.set_ucode_id(params.ucode_id);
+    reg.write(bar, &E::ID);
+
+    let mut reg = regs::NV_PFALCON2_FALCON_MOD_SEL::default();
+    reg.set_algo(FalconModSelAlgo::Rsa3k);
+    reg.write(bar, &E::ID);
 
     Ok(())
 }
diff --git a/drivers/gpu/nova-core/fb/hal/ga100.rs b/drivers/gpu/nova-core/fb/hal/ga100.rs
index e0acc41aa7cd..027f2f59614f 100644
--- a/drivers/gpu/nova-core/fb/hal/ga100.rs
+++ b/drivers/gpu/nova-core/fb/hal/ga100.rs
@@ -19,16 +19,17 @@ pub(super) fn read_sysmem_flush_page_ga100(bar: &Bar0) -> u64 {
 }
 
 pub(super) fn write_sysmem_flush_page_ga100(bar: &Bar0, addr: u64) {
-    regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI::default()
-        // CAST: `as u32` is used on purpose since the remaining bits are guaranteed to fit within
-        // a `u32`.
-        .set_adr_63_40((addr >> FLUSH_SYSMEM_ADDR_SHIFT_HI) as u32)
-        .write(bar);
-    regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::default()
-        // CAST: `as u32` is used on purpose since we want to strip the upper bits that have been
-        // written to `NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI`.
-        .set_adr_39_08((addr >> FLUSH_SYSMEM_ADDR_SHIFT) as u32)
-        .write(bar);
+    // CAST: `as u32` is used on purpose since the remaining bits are guaranteed to fit within
+    // a `u32`.
+    let mut reg = regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI::default();
+    reg.set_adr_63_40((addr >> FLUSH_SYSMEM_ADDR_SHIFT_HI) as u32);
+    reg.write(bar);
+
+    // CAST: `as u32` is used on purpose since we want to strip the upper bits that have been
+    // written to `NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI`.
+    let mut reg = regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::default();
+    reg.set_adr_39_08((addr >> FLUSH_SYSMEM_ADDR_SHIFT) as u32);
+    reg.write(bar);
 }
 
 pub(super) fn display_enabled_ga100(bar: &Bar0) -> bool {
diff --git a/drivers/gpu/nova-core/fb/hal/tu102.rs b/drivers/gpu/nova-core/fb/hal/tu102.rs
index eec984f4e816..994a173dc6f4 100644
--- a/drivers/gpu/nova-core/fb/hal/tu102.rs
+++ b/drivers/gpu/nova-core/fb/hal/tu102.rs
@@ -21,9 +21,9 @@ pub(super) fn write_sysmem_flush_page_gm107(bar: &Bar0, addr: u64) -> Result {
     u32::try_from(addr >> FLUSH_SYSMEM_ADDR_SHIFT)
         .map_err(|_| EINVAL)
         .map(|addr| {
-            regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::default()
-                .set_adr_39_08(addr)
-                .write(bar)
+            let mut reg = regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::default();
+            reg.set_adr_39_08(addr);
+            reg.write(bar);
         })
 }
 
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 6f946d14868a..358c97b96e9a 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -475,9 +475,9 @@ fn calculate_checksum<T: Iterator<Item = u8>>(it: T) -> u32 {
 
     /// Notifies the GSP that we have updated the command queue pointers.
     fn notify_gsp(bar: &Bar0) {
-        regs::NV_PGSP_QUEUE_HEAD::default()
-            .set_address(0)
-            .write(bar);
+        let mut reg = regs::NV_PGSP_QUEUE_HEAD::default();
+        reg.set_address(0);
+        reg.write(bar);
     }
 
     /// Sends `command` to the GSP.
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index abffd6beec65..2436933ac8cd 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -772,9 +772,10 @@ impl MsgHeaderVersion {
     const MINOR_TOT: u8 = 0;
 
     fn new() -> Self {
-        Self::default()
-            .set_major(Self::MAJOR_TOT)
-            .set_minor(Self::MINOR_TOT)
+        let mut v = Self::default();
+        v.set_major(Self::MAJOR_TOT);
+        v.set_minor(Self::MINOR_TOT);
+        v
     }
 }
 
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index fd1a815fa57d..6ab20e960399 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -49,10 +49,15 @@ pub(crate) trait RegisterBase<T> {
 /// let chipset = boot0.chipset()?;
 ///
 /// // Update some fields and write the value back.
-/// boot0.set_major_revision(3).set_minor_revision(10).write(&bar);
+/// boot0.set_major_revision(3);
+/// boot0.set_minor_revision(10);
+/// boot0.write(&bar);
 ///
 /// // Or, just read and update the register in a single step:
-/// BOOT_0::update(&bar, |r| r.set_major_revision(3).set_minor_revision(10));
+/// BOOT_0::update(&bar, |r| {
+///     r.set_major_revision(3);
+///     r.set_minor_revision(10);
+/// });
 /// ```
 ///
 /// The documentation strings are optional. If present, they will be added to the type's
@@ -388,12 +393,13 @@ pub(crate) fn write<const SIZE: usize, T>(self, io: &T) where
             #[inline(always)]
             pub(crate) fn update<const SIZE: usize, T, F>(
                 io: &T,
-                f: F,
+                mut f: F,
             ) where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                F: ::core::ops::FnOnce(Self) -> Self,
+                F: ::core::ops::FnMut(&mut Self),
             {
-                let reg = f(Self::read(io));
+                let mut reg = Self::read(io);
+                f(&mut reg);
                 reg.write(io);
             }
         }
@@ -452,13 +458,14 @@ pub(crate) fn write<const SIZE: usize, T, B>(
             pub(crate) fn update<const SIZE: usize, T, B, F>(
                 io: &T,
                 base: &B,
-                f: F,
+                mut f: F,
             ) where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
                 B: crate::regs::macros::RegisterBase<$base>,
-                F: ::core::ops::FnOnce(Self) -> Self,
+                F: ::core::ops::FnMut(&mut Self),
             {
-                let reg = f(Self::read(io, base));
+                let mut reg = Self::read(io, base);
+                f(&mut reg);
                 reg.write(io, base);
             }
         }
@@ -510,12 +517,13 @@ pub(crate) fn write<const SIZE: usize, T>(
             pub(crate) fn update<const SIZE: usize, T, F>(
                 io: &T,
                 idx: usize,
-                f: F,
+                mut f: F,
             ) where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                F: ::core::ops::FnOnce(Self) -> Self,
+                F: ::core::ops::FnMut(&mut Self),
             {
-                let reg = f(Self::read(io, idx));
+                let mut reg = Self::read(io, idx);
+                f(&mut reg);
                 reg.write(io, idx);
             }
 
@@ -568,7 +576,7 @@ pub(crate) fn try_update<const SIZE: usize, T, F>(
                 f: F,
             ) -> ::kernel::error::Result where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
-                F: ::core::ops::FnOnce(Self) -> Self,
+                F: ::core::ops::FnMut(&mut Self),
             {
                 if idx < Self::SIZE {
                     Ok(Self::update(io, idx, f))
@@ -640,13 +648,14 @@ pub(crate) fn update<const SIZE: usize, T, B, F>(
                 io: &T,
                 base: &B,
                 idx: usize,
-                f: F,
+                mut f: F,
             ) where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
                 B: crate::regs::macros::RegisterBase<$base>,
-                F: ::core::ops::FnOnce(Self) -> Self,
+                F: ::core::ops::FnMut(&mut Self),
             {
-                let reg = f(Self::read(io, base, idx));
+                let mut reg = Self::read(io, base, idx);
+                f(&mut reg);
                 reg.write(io, base, idx);
             }
 
@@ -708,7 +717,7 @@ pub(crate) fn try_update<const SIZE: usize, T, B, F>(
             ) -> ::kernel::error::Result where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
                 B: crate::regs::macros::RegisterBase<$base>,
-                F: ::core::ops::FnOnce(Self) -> Self,
+                F: ::core::ops::FnMut(&mut Self),
             {
                 if idx < Self::SIZE {
                     Ok(Self::update(io, base, idx, f))

base-commit: 7acc70476f14661149774ab88d3fe23d83ba4249
-- 
2.52.0

Hi John,

kernel test robot noticed the following build errors:

[auto build test ERROR on 7acc70476f14661149774ab88d3fe23d83ba4249]

url:    https://github.com/intel-lab-lkp/linux/commits/John-Hubbard/gpu-nova-core-bitfield-use-mut-self-setters-instead-of-builder-pattern/20260101-054823
base:   7acc70476f14661149774ab88d3fe23d83ba4249
patch link:    https://lore.kernel.org/r/20251231214727.448776-1-jhubbard%40nvidia.com
patch subject: [PATCH] gpu: nova-core: bitfield: use &mut self setters instead of builder pattern
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20260101/202601010949.aupX8V1f-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260101/202601010949.aupX8V1f-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601010949.aupX8V1f-lkp@intel.com/

All errors (new ones prefixed by >>):

   PATH=/opt/cross/clang-20/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
   INFO PATH=/opt/cross/rustc-1.88.0-bindgen-0.72.1/cargo/bin:/opt/cross/clang-20/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
   /usr/bin/timeout -k 100 12h /usr/bin/make KCFLAGS= -fno-crash-diagnostics -Wno-error=return-type -Wreturn-type -funsigned-char -Wundef W=1 --keep-going LLVM=1 -j32 -C source O=/kbuild/obj/consumer/x86_64-rhel-9.4-rust ARCH=x86_64 SHELL=/bin/bash rustfmtcheck
   make: Entering directory '/kbuild/src/consumer'
   make[1]: Entering directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust'
>> Diff in drivers/gpu/nova-core/falcon.rs:413:
                Delta::from_micros(150),
            );
    
   -        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| { v.set_reset(true); });
   +        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| {
   +            v.set_reset(true);
   +        });
    
            // TIMEOUT: falcon engine should not take more than 10us to reset.
            fsleep(Delta::from_micros(10));
   Diff in drivers/gpu/nova-core/falcon.rs:420:
    
   -        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| { v.set_reset(false); });
   +        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| {
   +            v.set_reset(false);
   +        });
    
            self.reset_wait_mem_scrubbing(bar)?;
    
>> Diff in drivers/gpu/nova-core/falcon.rs:413:
                Delta::from_micros(150),
            );
    
   -        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| { v.set_reset(true); });
   +        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| {
   +            v.set_reset(true);
   +        });
    
            // TIMEOUT: falcon engine should not take more than 10us to reset.
            fsleep(Delta::from_micros(10));
   Diff in drivers/gpu/nova-core/falcon.rs:420:
    
   -        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| { v.set_reset(false); });
   +        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| {
   +            v.set_reset(false);
   +        });
    
            self.reset_wait_mem_scrubbing(bar)?;
    
   make[2]: *** [Makefile:1871: rustfmt] Error 123
   make[2]: Target 'rustfmtcheck' not remade because of errors.
   make[1]: Leaving directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust'
   make[1]: *** [Makefile:248: __sub-make] Error 2
   make[1]: Target 'rustfmtcheck' not remade because of errors.
   make: *** [Makefile:248: __sub-make] Error 2
   make: Target 'rustfmtcheck' not remade because of errors.
   make: Leaving directory '/kbuild/src/consumer'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Wed, 2025-12-31 at 13:47 -0800, John Hubbard wrote:
> The builder-pattern setters (self -> Self) enabled method chaining like:
> 
>     reg.set_foo(x).set_sec(y).write(bar);
> 
> This made separate operations appear as a single expression, obscuring
> that each setter is a distinct mutation. 

So you're concerned about the fact that the compiler is not merging the set_foo(x) and the
set_sec(y) into a single read-modify-write?

> These setters are infallible,
> so the chaining provides no error-propagation benefit—it just obscures
> what are simple, independent assignments.
> 
> Change the bitfield!() macro to generate `&mut self` setters, so each
> operation is a distinct statement:
> 
>     reg.set_foo(x);
>     reg.set_sec(y);
>     reg.write(bar);

Are you sure about this?  It just seems like you're throwing out a neat little feature of Rust and
replacing it with something that's very C-like.  This breaks compatible with all users of the regs
macros.  Seems really disruptive for what seems to me like a cosmetic change.

On 12/31/25 2:33 PM, Timur Tabi wrote:
> On Wed, 2025-12-31 at 13:47 -0800, John Hubbard wrote:
>> The builder-pattern setters (self -> Self) enabled method chaining like:
>>
>>     reg.set_foo(x).set_sec(y).write(bar);
>>
>> This made separate operations appear as a single expression, obscuring
>> that each setter is a distinct mutation. 
> 
> So you're concerned about the fact that the compiler is not merging the set_foo(x) and the
> set_sec(y) into a single read-modify-write?

No, I don't care about that aspect.

> 
>> These setters are infallible,
>> so the chaining provides no error-propagation benefit—it just obscures
>> what are simple, independent assignments.
>>
>> Change the bitfield!() macro to generate `&mut self` setters, so each
>> operation is a distinct statement:
>>
>>     reg.set_foo(x);
>>     reg.set_sec(y);
>>     reg.write(bar);
> 
> Are you sure about this?  It just seems like you're throwing out a neat little feature of Rust and
> replacing it with something that's very C-like.  This breaks compatible with all users of the regs
> macros.  Seems really disruptive for what seems to me like a cosmetic change.
> 

It's only a neat feature if it *does* something. In this case, it *looks*
like a neat Rust feature, but under the covers it really is just handing
around copies unnecessarily, when really, it *is* doing the C-like thing
in the end.

I object to the fake Rust-ness that's being done here. It's like putting
hubcabs on a car.

thanks,
-- 
John Hubbard

> On Dec 31, 2025, at 5:47 PM, John Hubbard <jhubbard@nvidia.com> wrote:
> 
> On 12/31/25 2:33 PM, Timur Tabi wrote:
>>> On Wed, 2025-12-31 at 13:47 -0800, John Hubbard wrote:
>>> The builder-pattern setters (self -> Self) enabled method chaining like:
>>> 
>>>     reg.set_foo(x).set_sec(y).write(bar);
>>> 
>>> This made separate operations appear as a single expression, obscuring
>>> that each setter is a distinct mutation. 
>> 
>> So you're concerned about the fact that the compiler is not merging the set_foo(x) and the
>> set_sec(y) into a single read-modify-write?
> 
> No, I don't care about that aspect.
> 
>> 
>>> These setters are infallible,
>>> so the chaining provides no error-propagation benefit—it just obscures
>>> what are simple, independent assignments.
>>> 
>>> Change the bitfield!() macro to generate `&mut self` setters, so each
>>> operation is a distinct statement:
>>> 
>>>     reg.set_foo(x);
>>>     reg.set_sec(y);
>>>     reg.write(bar);
>> 
>> Are you sure about this?  It just seems like you're throwing out a neat little feature of Rust and
>> replacing it with something that's very C-like.  This breaks compatible with all users of the regs
>> macros.  Seems really disruptive for what seems to me like a cosmetic change.
>> 
> 
> It's only a neat feature if it *does* something. In this case, it *looks*
> like a neat Rust feature, but under the covers it really is just handing
> around copies unnecessarily, when really, it *is* doing the C-like thing
> in the end.
> 
> I object to the fake Rust-ness that's being done here. It's like putting
> hubcabs on a car.

But IMO there is only one operation here, the IO write. The setter is just mutations. Builder pattern chaining is idiomatic to Rust.

I would not call it fake Rustness since it is Rustness in the Rust language. Afair, similar arguments were made before and conclusion was, well, this is Rust.

Regarding the copies, I am intrigued - have you verified that the code generation actually results in copies? I would be surprised if the compiler did not optimize.

 - Joel





> 
> thanks,
> --
> John Hubbard
> 

On 12/31/25 4:15 PM, Joel Fernandes wrote:
>> On Dec 31, 2025, at 5:47 PM, John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 12/31/25 2:33 PM, Timur Tabi wrote:
>>>> On Wed, 2025-12-31 at 13:47 -0800, John Hubbard wrote:
>>>> The builder-pattern setters (self -> Self) enabled method chaining like:
>>>>
>>>>     reg.set_foo(x).set_sec(y).write(bar);
>>>>
>>>> This made separate operations appear as a single expression, obscuring
>>>> that each setter is a distinct mutation. 
>>>
>>> So you're concerned about the fact that the compiler is not merging the set_foo(x) and the
>>> set_sec(y) into a single read-modify-write?
>>
>> No, I don't care about that aspect.
>>
>>>
>>>> These setters are infallible,
>>>> so the chaining provides no error-propagation benefit—it just obscures
>>>> what are simple, independent assignments.
>>>>
>>>> Change the bitfield!() macro to generate `&mut self` setters, so each
>>>> operation is a distinct statement:
>>>>
>>>>     reg.set_foo(x);
>>>>     reg.set_sec(y);
>>>>     reg.write(bar);
>>>
>>> Are you sure about this?  It just seems like you're throwing out a neat little feature of Rust and
>>> replacing it with something that's very C-like.  This breaks compatible with all users of the regs
>>> macros.  Seems really disruptive for what seems to me like a cosmetic change.
>>>
>>
>> It's only a neat feature if it *does* something. In this case, it *looks*
>> like a neat Rust feature, but under the covers it really is just handing
>> around copies unnecessarily, when really, it *is* doing the C-like thing
>> in the end.
>>
>> I object to the fake Rust-ness that's being done here. It's like putting
>> hubcabs on a car.
> 
> But IMO there is only one operation here, the IO write. The setter is just mutations. Builder pattern chaining is idiomatic to Rust.
> 
> I would not call it fake Rustness since it is Rustness in the Rust language. Afair, similar arguments were made before and conclusion was, well, this is Rust.

There is nothing about doing sequential .set_foo() calls that goes against
Rust idioms.

But this really is fake chaining, because there are no Results involved.
It's not buying us anything except a bit of indirection and cognitive
load for the reader.

> 
> Regarding the copies, I am intrigued - have you verified that the code generation actually results in copies? I would be surprised if the compiler did not optimize.


No no, I just mean conceptually using Copy instead of a mutable Self.

thanks,
-- 
John Hubbard

On 12/31/2025 7:46 PM, John Hubbard wrote:
> On 12/31/25 4:15 PM, Joel Fernandes wrote:
>>> On Dec 31, 2025, at 5:47 PM, John Hubbard <jhubbard@nvidia.com> wrote:
>>>
>>> On 12/31/25 2:33 PM, Timur Tabi wrote:
>>>>> On Wed, 2025-12-31 at 13:47 -0800, John Hubbard wrote:
>>>>> The builder-pattern setters (self -> Self) enabled method chaining like:
>>>>>
>>>>>     reg.set_foo(x).set_sec(y).write(bar);
>>>>>
>>>>> This made separate operations appear as a single expression, obscuring
>>>>> that each setter is a distinct mutation. 
>>>>
>>>> So you're concerned about the fact that the compiler is not merging the set_foo(x) and the
>>>> set_sec(y) into a single read-modify-write?
>>>
>>> No, I don't care about that aspect.
>>>
>>>>
>>>>> These setters are infallible,
>>>>> so the chaining provides no error-propagation benefit—it just obscures
>>>>> what are simple, independent assignments.
>>>>>
>>>>> Change the bitfield!() macro to generate `&mut self` setters, so each
>>>>> operation is a distinct statement:
>>>>>
>>>>>     reg.set_foo(x);
>>>>>     reg.set_sec(y);
>>>>>     reg.write(bar);
>>>>
>>>> Are you sure about this?  It just seems like you're throwing out a neat little feature of Rust and
>>>> replacing it with something that's very C-like.  This breaks compatible with all users of the regs
>>>> macros.  Seems really disruptive for what seems to me like a cosmetic change.
>>>>
>>>
>>> It's only a neat feature if it *does* something. In this case, it *looks*
>>> like a neat Rust feature, but under the covers it really is just handing
>>> around copies unnecessarily, when really, it *is* doing the C-like thing
>>> in the end.
>>>
>>> I object to the fake Rust-ness that's being done here. It's like putting
>>> hubcabs on a car.
>>
>> But IMO there is only one operation here, the IO write. The setter is just mutations. Builder pattern chaining is idiomatic to Rust.
>>
>> I would not call it fake Rustness since it is Rustness in the Rust language. Afair, similar arguments were made before and conclusion was, well, this is Rust.
> 
> There is nothing about doing sequential .set_foo() calls that goes against
> Rust idioms.

Huh, I just meant we should "Ok" with and inclined to be using Rust idioms even
though they may seem unfamiliar at first.

> 
> But this really is fake chaining, because there are no Results involved.
> It's not buying us anything except a bit of indirection and cognitive
> load for the reader.

Chaining is not really only about error propagation. Builder pattern can be used
for other cases too, like passing a setter chained expression to a function
argument for better clarity, I was planning to do that for the sequencer for
instance since there's a lot of parameters passed to it.

But in this case, I am actually absolutely opposed against this, it makes the
API hard to use because now how do you differentiate between an IO function call
versus something that just mutates memory? Is set() an IO or write()?

     reg.set_foo(x);   // no IO
     reg.set_sec(y);
     reg.write(bar);   // IO.

So no thank you, I quite dislike it. :)

Instead with chaining, we can just rely on the last part of the chain concluding
in a write() with the intermediaries just mutating memory.

Further, your suggestion could also make it easier to introduce bugs?

     reg.set_foo(x);
     reg.write(bar);
     reg.set_sec(y);

While this is also possible to mess up with the register macro, it is much
harder to do with chaining since there is no opportunity to interleave a write()
incorrectly.

>> Regarding the copies, I am intrigued - have you verified that the code generation actually results in copies? I would be surprised if the compiler did not optimize.
> 
> 
> No no, I just mean conceptually using Copy instead of a mutable Self.
> 

Conceptually, but the compiler would not do any copies through the setter chain
was my point though. Note that all setters are #[inline(always)], the compiler
should optimize chained calls to simple register bit manipulation. If that is
not happening, we should definitely look more into it.

thanks,

 - Joel

On 12/31/25 6:42 PM, Joel Fernandes wrote:
> On 12/31/2025 7:46 PM, John Hubbard wrote:
>> On 12/31/25 4:15 PM, Joel Fernandes wrote:
>>>> On Dec 31, 2025, at 5:47 PM, John Hubbard <jhubbard@nvidia.com> wrote:
>>>>
>>>> On 12/31/25 2:33 PM, Timur Tabi wrote:
>>>>>> On Wed, 2025-12-31 at 13:47 -0800, John Hubbard wrote:
>>>>>> The builder-pattern setters (self -> Self) enabled method chaining like:
>>>>>>
>>>>>>      reg.set_foo(x).set_sec(y).write(bar);
>>>>>>
>>>>>> This made separate operations appear as a single expression, obscuring
>>>>>> that each setter is a distinct mutation.
>>>>>
>>>>> So you're concerned about the fact that the compiler is not merging the set_foo(x) and the
>>>>> set_sec(y) into a single read-modify-write?
>>>>
>>>> No, I don't care about that aspect.
>>>>
>>>>>
>>>>>> These setters are infallible,
>>>>>> so the chaining provides no error-propagation benefit—it just obscures
>>>>>> what are simple, independent assignments.
>>>>>>
>>>>>> Change the bitfield!() macro to generate `&mut self` setters, so each
>>>>>> operation is a distinct statement:
>>>>>>
>>>>>>      reg.set_foo(x);
>>>>>>      reg.set_sec(y);
>>>>>>      reg.write(bar);
>>>>>
>>>>> Are you sure about this?  It just seems like you're throwing out a neat little feature of Rust and
>>>>> replacing it with something that's very C-like.  This breaks compatible with all users of the regs
>>>>> macros.  Seems really disruptive for what seems to me like a cosmetic change.
>>>>>
>>>>
>>>> It's only a neat feature if it *does* something. In this case, it *looks*
>>>> like a neat Rust feature, but under the covers it really is just handing
>>>> around copies unnecessarily, when really, it *is* doing the C-like thing
>>>> in the end.
>>>>
>>>> I object to the fake Rust-ness that's being done here. It's like putting
>>>> hubcabs on a car.
>>>
>>> But IMO there is only one operation here, the IO write. The setter is just mutations. Builder pattern chaining is idiomatic to Rust.
>>>
>>> I would not call it fake Rustness since it is Rustness in the Rust language. Afair, similar arguments were made before and conclusion was, well, this is Rust.
>>
>> There is nothing about doing sequential .set_foo() calls that goes against
>> Rust idioms.
> 
> Huh, I just meant we should "Ok" with and inclined to be using Rust idioms even
> though they may seem unfamiliar at first.
> 
>>
>> But this really is fake chaining, because there are no Results involved.
>> It's not buying us anything except a bit of indirection and cognitive
>> load for the reader.
> 
> Chaining is not really only about error propagation. Builder pattern can be used
> for other cases too, like passing a setter chained expression to a function
> argument for better clarity, I was planning to do that for the sequencer for
> instance since there's a lot of parameters passed to it.

Let's see if that has any use for this.

So far, though, in the code base that we have today, there is absolutely
zero benefit. The diffs here prove it.


> 
> But in this case, I am actually absolutely opposed against this, it makes the
> API hard to use because now how do you differentiate between an IO function call
> versus something that just mutates memory? Is set() an IO or write()?

That's a completely separate, pre-existing issue with the API.

> 
>       reg.set_foo(x);   // no IO
>       reg.set_sec(y);
>       reg.write(bar);   // IO.
> 
> So no thank you, I quite dislike it. :)
> 
> Instead with chaining, we can just rely on the last part of the chain concluding
> in a write() with the intermediaries just mutating memory.

Same as above, just a more happy-happy chaining interface, but the same
function calls must be made in the same order.

The main benefit of chaining is really in the Result's that allow
an early return from the whole thing. If one is not using that, the
benefits drop sharply off.

> 
> Further, your suggestion could also make it easier to introduce bugs?
> 
>       reg.set_foo(x);
>       reg.write(bar);
>       reg.set_sec(y);
> 
> While this is also possible to mess up with the register macro, it is much
> harder to do with chaining since there is no opportunity to interleave a write()
> incorrectly.
> 
>>> Regarding the copies, I am intrigued - have you verified that the code generation actually results in copies? I would be surprised if the compiler did not optimize.
>>
>>
>> No no, I just mean conceptually using Copy instead of a mutable Self.
>>
> 
> Conceptually, but the compiler would not do any copies through the setter chain
> was my point though. Note that all setters are #[inline(always)], the compiler

No argument about the actual copies. My point here is about the API and
how we use it and understand it.


thanks,
-- 
John Hubbard

Hi John,

On 12/31/2025 9:52 PM, John Hubbard wrote:
[..]
>>> But this really is fake chaining, because there are no Results involved.
>>> It's not buying us anything except a bit of indirection and cognitive
>>> load for the reader.
>>
>> Chaining is not really only about error propagation. Builder pattern can be used
>> for other cases too, like passing a setter chained expression to a function
>> argument for better clarity, I was planning to do that for the sequencer for
>> instance since there's a lot of parameters passed to it.
> 
> Let's see if that has any use for this.
> 
> So far, though, in the code base that we have today, there is absolutely
> zero benefit. The diffs here prove it.
> 

From your patch diff, I see the lines of code increased. But that's not even the
main issue I have with it (though IMO the chaining is more readable..).

>> But in this case, I am actually absolutely opposed against this, it makes the
>> API hard to use because now how do you differentiate between an IO function call
>> versus something that just mutates memory? Is set() an IO or write()?
> 
> That's a completely separate, pre-existing issue with the API.

Nope. With chaining we clearly know that the final operation is a write().

For instance, you cannot do:
  reg.set_foo()
     .write()
     .set_bar()

That wont compile. You cannot intermingle write() with set_XX() because write()
doesn't return anything that can be chained with. The builder pattern is typically:
  obj.set_something()
     .set_something()
     .do_some_action()

The 'set' can also be 'with' from what I've seen, whatever. The point is the
last thing is the action. IMO very readable and simple. I know that the write()
will be what ends up doing the I/O. It is one entity that culminates in the write().

> 
>>
>>       reg.set_foo(x);   // no IO
>>       reg.set_sec(y);
>>       reg.write(bar);   // IO.
>>
>> So no thank you, I quite dislike it. :)
>>
>> Instead with chaining, we can just rely on the last part of the chain concluding
>> in a write() with the intermediaries just mutating memory.
> 
> Same as above, just a more happy-happy chaining interface, but the same
> function calls must be made in the same order.

No, you cannot place write() anywhere except at the end of the chain - the type
system enforces this since write() returns ().

thanks,

 - Joel

On Wed, 31 Dec 2025 23:05:21 -0500
Joel Fernandes <joelagnelf@nvidia.com> wrote:

> Hi John,
> 
> On 12/31/2025 9:52 PM, John Hubbard wrote:
> [..]
> >>> But this really is fake chaining, because there are no Results involved.
> >>> It's not buying us anything except a bit of indirection and cognitive
> >>> load for the reader.  
> >>
> >> Chaining is not really only about error propagation. Builder pattern can be used
> >> for other cases too, like passing a setter chained expression to a function
> >> argument for better clarity, I was planning to do that for the sequencer for
> >> instance since there's a lot of parameters passed to it.  
> > 
> > Let's see if that has any use for this.
> > 
> > So far, though, in the code base that we have today, there is absolutely
> > zero benefit. The diffs here prove it.
> >   
> 
> From your patch diff, I see the lines of code increased. But that's not even the
> main issue I have with it (though IMO the chaining is more readable..).
> 
> >> But in this case, I am actually absolutely opposed against this, it makes the
> >> API hard to use because now how do you differentiate between an IO function call
> >> versus something that just mutates memory? Is set() an IO or write()?  
> > 
> > That's a completely separate, pre-existing issue with the API.  
> 
> Nope. With chaining we clearly know that the final operation is a write().
> 
> For instance, you cannot do:
>   reg.set_foo()
>      .write()
>      .set_bar()
> 
> That wont compile. You cannot intermingle write() with set_XX() because write()
> doesn't return anything that can be chained with. The builder pattern is typically:
>   obj.set_something()
>      .set_something()
>      .do_some_action()
> 
> The 'set' can also be 'with' from what I've seen, whatever. The point is the
> last thing is the action. IMO very readable and simple. I know that the write()
> will be what ends up doing the I/O. It is one entity that culminates in the write().
> 
> >   
> >>
> >>       reg.set_foo(x);   // no IO
> >>       reg.set_sec(y);
> >>       reg.write(bar);   // IO.
> >>
> >> So no thank you, I quite dislike it. :)
> >>
> >> Instead with chaining, we can just rely on the last part of the chain concluding
> >> in a write() with the intermediaries just mutating memory.  
> > 
> > Same as above, just a more happy-happy chaining interface, but the same
> > function calls must be made in the same order.  
> 
> No, you cannot place write() anywhere except at the end of the chain - the type
> system enforces this since write() returns ().

One thing that probably should be added though is `#[must_use]`
annotations on these set functions; this would ensure that if someone
writes

	reg.set_foo();

the compiler would complain that the return value is not used.

Best,
Gary

> On Dec 31, 2025, at 11:29 PM, Gary Guo <gary@garyguo.net> wrote:
[..]
>> 
>>> 
>>>> 
>>>>       reg.set_foo(x);   // no IO
>>>>       reg.set_sec(y);
>>>>       reg.write(bar);   // IO.
>>>> 
>>>> So no thank you, I quite dislike it. :)
>>>> 
>>>> Instead with chaining, we can just rely on the last part of the chain concluding
>>>> in a write() with the intermediaries just mutating memory.  
>>> 
>>> Same as above, just a more happy-happy chaining interface, but the same
>>> function calls must be made in the same order.  
>> 
>> No, you cannot place write() anywhere except at the end of the chain - the type
>> system enforces this since write() returns ().
> 
> One thing that probably should be added though is `#[must_use]`
> annotations on these set functions; this would ensure that if someone
> writes
> 
>    reg.set_foo();
> 
> the compiler would complain that the return value is not used.

True! I thought we already did that, thanks Gary for pointing this out.

I will add to my Todo list to get to it unless someone beats me to it ;-)

 - Joel



> 
> Best,
> Gary
> 
>