I2C codegen dedupe (#2569)

jamesmunns · web-flow · commit 11f1f5a580bd · 2026-07-10T14:36:26.000Z
Closes #2552 This is the most straightforward de-duplication approach, essentially "pushing down" the knowledge of whether a device requires a rail or not. This change *matches* the current behavior, and that of the system prior to #2538, and replaces the pre-existing by-rail-name accessor with the newer by-rail-name accessor (giving it the pre-existing accessor's nicer name). It *feels* like this change might not go far enough yet: for all of the devices we are creating, it is probably statically knowable whether that device SHOULD or SHOULD NOT have a rail index. For example, `Bmr491` and `Tps546B24A` do not, and previously had a "dead" rail parameter that I've removed. For correctness' sake, it feels like we should ideally fail out at compile time if a device SHOULD have multiple rails, and doesn't (according to the app toml), OR if a device SHOULDN'T have multiple rails, but does (according to the app toml).
diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs
@@ -1386,31 +1386,14 @@ impl ConfigGenerator {
             for (rail, (device, index)) in &all {
                 let raw_bank = index.unwrap_or(0);
 
-                // ----
-                // First accessor, returns `(I2cDevice, u8)`
-
-                // if we update this code to be more clever than just
-                // to-lowercase'ing the rail names, you might need to go update
-                // the mapping in `control-plane-agent`!
-                write!(
-                    &mut self.output,
-                    r##"
-        #[allow(dead_code)]
-        pub fn {}(task: TaskId) -> (I2cDevice, u8) {{"##,
-                    rail.to_lowercase(),
-                )?;
-
-                let out = self.generate_device(device, 16);
-                writeln!(&mut self.output, "({out}, {raw_bank})\n        }}")?;
-
                 // ---
-                // Second accessor, returns `(I2cDevice, Option<u8>)`
+                // Accessor, returns `(I2cDevice, Option<u8>)`
 
                 write!(
                     &mut self.output,
                     r##"
         #[allow(dead_code)]
-        pub fn {}_with_opt_page_idx(task: TaskId)"##,
+        pub fn {}(task: TaskId)"##,
                     rail.to_lowercase(),
                 )?;
                 write!(&mut self.output, " -> (I2cDevice, Option<u8>) {{")?;
diff --git a/drv/cosmo-seq-server/src/main.rs b/drv/cosmo-seq-server/src/main.rs
@@ -195,7 +195,7 @@ fn main() -> ! {
         use drv_i2c_devices::bmr491::{Bmr491, ExternalInputVoltageProtection};
 
         let dev = i2c_config::devices::bmr491_u80(I2C.get_task_id());
-        let driver = Bmr491::new(&dev, 0);
+        let driver = Bmr491::new(&dev);
 
         // Cosmo provides external undervoltage protection that kicks in at a
         // lower voltage than we'd like to tolerate, so, request additional
diff --git a/drv/cosmo-seq-server/src/vcore.rs b/drv/cosmo-seq-server/src/vcore.rs
@@ -190,10 +190,12 @@ const VCORE_NSAMPLES: usize = 25;
 
 impl VCore {
     pub fn new(i2c: TaskId) -> Self {
-        let (device, rail) = i2c_config::pmbus::vddcr_cpu0_a0(i2c);
+        let (device, opt_rail) = i2c_config::pmbus::vddcr_cpu0_a0(i2c);
+        let rail = opt_rail.unwrap_or(0);
         let vddcr_cpu0 = Raa229620A::new(&device, rail);
 
-        let (device, rail) = i2c_config::pmbus::vddcr_cpu1_a0(i2c);
+        let (device, opt_rail) = i2c_config::pmbus::vddcr_cpu1_a0(i2c);
+        let rail = opt_rail.unwrap_or(0);
         let vddcr_cpu1 = Raa229620A::new(&device, rail);
 
         Self {
diff --git a/drv/gimlet-seq-server/src/main.rs b/drv/gimlet-seq-server/src/main.rs
@@ -462,7 +462,7 @@ impl<S: SpiServer + Clone> ServerImpl<S> {
             };
 
             let dev = i2c_config::devices::bmr491_u431(I2C.get_task_id());
-            let driver = Bmr491::new(&dev, 0);
+            let driver = Bmr491::new(&dev);
 
             // Gimlet provides external undervoltage protection that is better
             // than what we'd get from the 491, so we rely on that.
@@ -528,7 +528,9 @@ impl<S: SpiServer + Clone> ServerImpl<S> {
         // Turn on the chassis LED once we reach A2
         sys.gpio_set(CHASSIS_LED);
 
-        let (device, rail) = i2c_config::pmbus::vdd_vcore(I2C.get_task_id());
+        let (device, opt_rail) =
+            i2c_config::pmbus::vdd_vcore(I2C.get_task_id());
+        let rail = opt_rail.unwrap_or(0);
 
         let mut server = Self {
             state: PowerState::A2,
@@ -1578,10 +1580,12 @@ cfg_if::cfg_if! {
             use drv_i2c_devices::raa229618::Raa229618;
             let i2c = I2C.get_task_id();
 
-            let (device, rail) = i2c_config::pmbus::vdd_vcore(i2c);
+            let (device, opt_rail) = i2c_config::pmbus::vdd_vcore(i2c);
+            let rail = opt_rail.unwrap_or(0);
             let mut vdd_vcore = Raa229618::new(&device, rail);
 
-            let (device, rail) = i2c_config::pmbus::vddcr_soc(i2c);
+            let (device, opt_rail) = i2c_config::pmbus::vddcr_soc(i2c);
+            let rail = opt_rail.unwrap_or(0);
             let mut vddcr_soc = Raa229618::new(&device, rail);
 
             retry_i2c_txn(I2cTxn::VCoreOff, || vdd_vcore.turn_off())?;
@@ -1593,10 +1597,12 @@ cfg_if::cfg_if! {
             use drv_i2c_devices::raa229618::Raa229618;
             let i2c = I2C.get_task_id();
 
-            let (device, rail) = i2c_config::pmbus::vdd_vcore(i2c);
+            let (device, opt_rail) = i2c_config::pmbus::vdd_vcore(i2c);
+            let rail = opt_rail.unwrap_or(0);
             let mut vdd_vcore = Raa229618::new(&device, rail);
 
-            let (device, rail) = i2c_config::pmbus::vddcr_soc(i2c);
+            let (device, opt_rail) = i2c_config::pmbus::vddcr_soc(i2c);
+            let rail = opt_rail.unwrap_or(0);
             let mut vddcr_soc = Raa229618::new(&device, rail);
 
             retry_i2c_txn(I2cTxn::VCoreOn, || vdd_vcore.turn_on())?;
@@ -1620,8 +1626,8 @@ cfg_if::cfg_if! {
 
             let i2c = I2C.get_task_id();
 
-            let (device, rail) = i2c_config::pmbus::v3p3_sys_a0(i2c);
-            let v3p3_sys_a0 = Tps546B24A::new(&device, rail);
+            let (device, _rail) = i2c_config::pmbus::v3p3_sys_a0(i2c);
+            let v3p3_sys_a0 = Tps546B24A::new(&device);
 
             ringbuf_entry!(
                 Trace::V3P3SysA0VOut(v3p3_sys_a0.read_vout().unwrap_lite())
diff --git a/drv/i2c-devices/src/bmr491.rs b/drv/i2c-devices/src/bmr491.rs
@@ -133,7 +133,7 @@ pub struct MitigationFailure {
 }
 
 impl Bmr491 {
-    pub fn new(device: &I2cDevice, _rail: u8) -> Self {
+    pub fn new(device: &I2cDevice) -> Self {
         Bmr491 {
             device: *device,
             mode: Cell::new(None),
diff --git a/drv/i2c-devices/src/tps546b24a.rs b/drv/i2c-devices/src/tps546b24a.rs
@@ -55,7 +55,7 @@ impl From<Error> for ResponseCode {
 }
 
 impl Tps546B24A {
-    pub fn new(device: &I2cDevice, _rail: u8) -> Self {
+    pub fn new(device: &I2cDevice) -> Self {
         Tps546B24A {
             device: *device,
             mode: Cell::new(None),
diff --git a/drv/psc-seq-server/src/bsp/observer_a.rs b/drv/psc-seq-server/src/bsp/observer_a.rs
@@ -3,9 +3,7 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 use super::{PSU_COUNT, i2c_config, notifications};
-use drv_i2c_api::I2cDevice;
 use drv_stm32xx_sys_api as sys_api;
-use userlib::TaskId;
 
 pub use drv_i2c_devices::mwocp6x::Mwocp67 as Mwocp6x;
 
@@ -49,8 +47,11 @@ pub const PSU_PWR_OK_NOTIF: [u32; PSU_COUNT] = [
     notifications::PSU_PWR_OK_5_MASK,
 ];
 
+/// Type returned by generated pmbus rail functions
+pub type SummonFn = fn(userlib::TaskId) -> (drv_i2c_api::I2cDevice, Option<u8>);
+
 /// In order to get the PMBus devices by PSU index, we need a little lookup table.
-pub const PSU_PMBUS_DEVS: [fn(TaskId) -> (I2cDevice, u8); PSU_COUNT] = [
+pub const PSU_PMBUS_DEVS: [SummonFn; PSU_COUNT] = [
     i2c_config::pmbus::v50_main_psu0,
     i2c_config::pmbus::v50_main_psu1,
     i2c_config::pmbus::v50_main_psu2,
diff --git a/drv/psc-seq-server/src/bsp/psc_bc.rs b/drv/psc-seq-server/src/bsp/psc_bc.rs
@@ -3,9 +3,7 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 use super::{PSU_COUNT, i2c_config, notifications};
-use drv_i2c_api::I2cDevice;
 use drv_stm32xx_sys_api as sys_api;
-use userlib::TaskId;
 
 pub use drv_i2c_devices::mwocp6x::Mwocp68 as Mwocp6x;
 
@@ -51,8 +49,11 @@ pub const PSU_PWR_OK_NOTIF: [u32; PSU_COUNT] = [
     notifications::PSU_PWR_OK_6_MASK,
 ];
 
+/// Type returned by generated pmbus rail functions
+pub type SummonFn = fn(userlib::TaskId) -> (drv_i2c_api::I2cDevice, Option<u8>);
+
 /// In order to get the PMBus devices by PSU index, we need a little lookup table.
-pub const PSU_PMBUS_DEVS: [fn(TaskId) -> (I2cDevice, u8); PSU_COUNT] = [
+pub const PSU_PMBUS_DEVS: [SummonFn; PSU_COUNT] = [
     i2c_config::pmbus::v54_psu0,
     i2c_config::pmbus::v54_psu1,
     i2c_config::pmbus::v54_psu2,
diff --git a/drv/psc-seq-server/src/main.rs b/drv/psc-seq-server/src/main.rs
@@ -509,7 +509,8 @@ fn main() -> ! {
         let dev = {
             let i2c = I2C.get_task_id();
             let make_dev = bsp::PSU_PMBUS_DEVS[i];
-            let (dev, rail) = make_dev(i2c);
+            let (dev, opt_rail) = make_dev(i2c);
+            let rail = opt_rail.unwrap_or(0);
             bsp::Mwocp6x::new(&dev, rail)
         };
         let slot = PSU_SLOTS[i];
diff --git a/drv/sidecar-seq-server/src/main.rs b/drv/sidecar-seq-server/src/main.rs
@@ -906,7 +906,7 @@ fn main() -> ! {
         use drv_i2c_devices::bmr491::{Bmr491, ExternalInputVoltageProtection};
 
         let dev = i2c_config::devices::bmr491_u12(I2C.get_task_id());
-        let driver = Bmr491::new(&dev, 0);
+        let driver = Bmr491::new(&dev);
 
         // Sidecar provides external undervoltage protection that is better than
         // what we'd get from the 491, so we rely on that.
diff --git a/drv/sidecar-seq-server/src/tofino.rs b/drv/sidecar-seq-server/src/tofino.rs
@@ -18,7 +18,9 @@ pub(crate) struct Tofino {
 
 impl Tofino {
     pub fn new(i2c_task: userlib::TaskId) -> Self {
-        let (i2c_device, rail) = i2c_config::pmbus::v0p8_tf2_vdd_core(i2c_task);
+        let (i2c_device, opt_rail) =
+            i2c_config::pmbus::v0p8_tf2_vdd_core(i2c_task);
+        let rail = opt_rail.unwrap_or(0);
         let vddcore = Raa229618::new(&i2c_device, rail);
         Self {
             policy: TofinoSequencerPolicy::Disabled,
diff --git a/task/control-plane-agent/build.rs b/task/control-plane-agent/build.rs
@@ -111,7 +111,7 @@ fn do_pmbus() -> Result<()> {
         // build_i2c *also* only to-lowercases the rail names to make functions
         write!(
             file,
-            "summon_fn: crate::i2c_config::pmbus::{}_with_opt_page_idx, ",
+            "summon_fn: crate::i2c_config::pmbus::{}, ",
             rail.to_lowercase()
         )?;
         write!(file, "status_bits: Capabilities(0x{:08x}) ", caps.0)?;
@@ -184,7 +184,7 @@ fn context_create_file(path: &Path) -> Result<File> {
 
 /// Look at the `pmbus` crate metadata to see if a specific command is "Illegal"
 /// and set the capability bit if not.
-macro_rules! check_if_marked_as_illegal_by_pmbus_metadata {
+macro_rules! set_if_pmbus_read_illegal {
     ($out:ident, $module:ident, $cmd:ident) => {{
         use drv_i2c_types::pmbus_status::Capabilities;
         use pmbus::{Command, Operation};
@@ -204,54 +204,16 @@ macro_rules! generator {
     ($name:literal, $module:ident) => {
         ($name, || {
             let mut out = 0u32;
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out,
-                $module,
-                STATUS_WORD
-            );
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out,
-                $module,
-                STATUS_VOUT
-            );
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out,
-                $module,
-                STATUS_IOUT
-            );
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out,
-                $module,
-                STATUS_TEMPERATURE
-            );
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out, $module, STATUS_CML
-            );
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out,
-                $module,
-                STATUS_OTHER
-            );
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out,
-                $module,
-                STATUS_INPUT
-            );
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out,
-                $module,
-                STATUS_MFR_SPECIFIC
-            );
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out,
-                $module,
-                STATUS_FANS_1_2
-            );
-            check_if_marked_as_illegal_by_pmbus_metadata!(
-                out,
-                $module,
-                STATUS_FANS_3_4
-            );
+            set_if_pmbus_read_illegal!(out, $module, STATUS_WORD);
+            set_if_pmbus_read_illegal!(out, $module, STATUS_VOUT);
+            set_if_pmbus_read_illegal!(out, $module, STATUS_IOUT);
+            set_if_pmbus_read_illegal!(out, $module, STATUS_TEMPERATURE);
+            set_if_pmbus_read_illegal!(out, $module, STATUS_CML);
+            set_if_pmbus_read_illegal!(out, $module, STATUS_OTHER);
+            set_if_pmbus_read_illegal!(out, $module, STATUS_INPUT);
+            set_if_pmbus_read_illegal!(out, $module, STATUS_MFR_SPECIFIC);
+            set_if_pmbus_read_illegal!(out, $module, STATUS_FANS_1_2);
+            set_if_pmbus_read_illegal!(out, $module, STATUS_FANS_3_4);
             Capabilities(out)
         })
     };
diff --git a/task/power/src/bsp/cosmo_ab.rs b/task/power/src/bsp/cosmo_ab.rs
@@ -344,7 +344,9 @@ impl State {
         .iter()
         .enumerate()
         {
-            let (dev, rail) = (builder)(i2c_task);
+            let (dev, opt_rail) = (builder)(i2c_task);
+            // If there is no rail index, then there must be a single rail
+            let rail = opt_rail.unwrap_or(0);
             let m = Max5970::new(&dev, rail, Ohms(0.005), true);
             if let Err(err) = m.set_dac_fast(0x99) {
                 ringbuf_entry!(Trace::Max5970ConfigFailed { u2_index: i, err });
diff --git a/task/power/src/main.rs b/task/power/src/main.rs

Original file line number	Diff line number	Diff line change
`@@ -133,7 +133,7 @@ pub struct MitigationFailure {`
`133`	`133`	`}`
`134`	`134`
`135`	`135`	`impl Bmr491 {`
`136`		`- pub fn new(device: &I2cDevice, _rail: u8) -> Self {`
	`136`	`+ pub fn new(device: &I2cDevice) -> Self {`
`137`	`137`	`Bmr491 {`
`138`	`138`	`device: *device,`
`139`	`139`	`mode: Cell::new(None),`
Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,7 @@ impl From<Error> for ResponseCode {`
`55`	`55`	`}`
`56`	`56`
`57`	`57`	`impl Tps546B24A {`
`58`		`- pub fn new(device: &I2cDevice, _rail: u8) -> Self {`
	`58`	`+ pub fn new(device: &I2cDevice) -> Self {`
`59`	`59`	`Tps546B24A {`
`60`	`60`	`device: *device,`
`61`	`61`	`mode: Cell::new(None),`