From b194ad3eb3861467ee5334d005b54c06280bef65 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?D=C3=A1niel=20Buga?= <bugadani@gmail.com>
Date: Thu, 7 Nov 2024 13:00:54 +0100
Subject: [PATCH 1/4] Implement apply_config, SetConfig

---
 esp-hal/CHANGELOG.md                          |  1 +
 esp-hal/MIGRATING-0.21.md                     | 10 +-
 esp-hal/src/i2c/master/mod.rs                 | 98 ++++++++++++++-----
 examples/src/bin/embassy_i2c.rs               | 14 ++-
 .../embassy_i2c_bmp180_calibration_data.rs    | 14 ++-
 .../src/bin/i2c_bmp180_calibration_data.rs    | 13 ++-
 examples/src/bin/i2c_display.rs               | 14 ++-
 examples/src/bin/lcd_cam_ov2640.rs            |  7 +-
 hil-test/tests/i2c.rs                         |  5 +-
 9 files changed, 135 insertions(+), 41 deletions(-)

diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md
index 3f12da94ae0..3398006021c 100644
--- a/esp-hal/CHANGELOG.md
+++ b/esp-hal/CHANGELOG.md
@@ -36,6 +36,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `embassy_embedded_hal::SetConfig` is now implemented for `{Spi, SpiDma, SpiDmaBus}` (#2448)
 - `slave::Spi::{with_mosi(), with_miso(), with_sclk(), with_cs()}` functions (#2485)
 - I8080: Added `set_8bits_order()` to set the byte order in 8-bit mode (#2487)
+- `embassy_embedded_hal::SetConfig` is now implemented for `spi::master::{Spi, SpiDma, SpiDmaBus}`, `i2c::master::I2c` (#2448, #?)
 
 ### Changed
 
diff --git a/esp-hal/MIGRATING-0.21.md b/esp-hal/MIGRATING-0.21.md
index ca14b32d0e7..bb8281368f2 100644
--- a/esp-hal/MIGRATING-0.21.md
+++ b/esp-hal/MIGRATING-0.21.md
@@ -69,11 +69,17 @@ let spi: Spi<'static, FullDuplexMode, SPI2> = Spi::new_typed(peripherals.SPI2, 1
 
 The I2C master driver and related types have been moved to `esp_hal::i2c::master`.
 
-The `with_timeout` constructors have been removed in favour of `set_timeout` or `with_timeout`.
+The `with_timeout` constructors have been removed. `new` and `new_typed` now take a `Config` struct
+with the available configuration options.
 
 ```diff
 -let i2c = I2c::new_with_timeout(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz(), timeout);
-+let i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz()).with_timeout(timeout);
++let i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, {
++    let mut config = Config::default();
++    config.frequency = 100.kHz();
++    config.timeout = timeout;
++    config
++});
 ```
 
 ## Changes to half-duplex SPI
diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs
index 036e2099d63..24e505c8b90 100644
--- a/esp-hal/src/i2c/master/mod.rs
+++ b/esp-hal/src/i2c/master/mod.rs
@@ -18,7 +18,7 @@
 //!
 //! ```rust, no_run
 #![doc = crate::before_snippet!()]
-//! # use esp_hal::i2c::master::I2c;
+//! # use esp_hal::i2c::master::{Config, I2c};
 //! # use esp_hal::gpio::Io;
 //! let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
 //!
@@ -28,7 +28,7 @@
 //!     peripherals.I2C0,
 //!     io.pins.gpio1,
 //!     io.pins.gpio2,
-//!     100.kHz(),
+//!     Config::default(),
 //! );
 //!
 //! loop {
@@ -46,6 +46,7 @@ use core::{
     task::{Context, Poll},
 };
 
+use embassy_embedded_hal::SetConfig;
 use embassy_sync::waitqueue::AtomicWaker;
 use embedded_hal::i2c::Operation as EhalOperation;
 use fugit::HertzU32;
@@ -106,6 +107,11 @@ pub enum Error {
     InvalidZeroLength,
 }
 
+/// I2C-specific configuration errors
+#[derive(Debug, Clone, Copy, PartialEq)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+pub enum ConfigError {}
+
 #[derive(PartialEq)]
 // This enum is used to keep track of the last/next operation that was/will be
 // performed in an embedded-hal(-async) I2c::transaction. It is used to
@@ -230,12 +236,54 @@ impl From<Ack> for u32 {
     }
 }
 
+/// I2C driver configuration
+#[derive(Debug, Clone, Copy)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+pub struct Config {
+    /// The I2C clock frequency.
+    pub frequency: HertzU32,
+
+    /// I2C SCL timeout period.
+    ///
+    /// When the level of SCL remains unchanged for more than `timeout` bus
+    /// clock cycles, the bus goes to idle state.
+    ///
+    /// The default value is about 10 bus clock cycles.
+    #[doc = ""]
+    #[cfg_attr(
+        not(esp32),
+        doc = "Note that the effective timeout may be longer than the value configured here."
+    )]
+    #[cfg_attr(not(esp32), doc = "Configuring `None` disables timeout control.")]
+    #[cfg_attr(esp32, doc = "Configuring `None` equals to the maximum timeout value.")]
+    // TODO: when supporting interrupts, document that SCL = high also triggers an interrupt.
+    pub timeout: Option<u32>,
+}
+
+impl Default for Config {
+    fn default() -> Self {
+        use fugit::RateExtU32;
+        Config {
+            frequency: 100.kHz(),
+            timeout: Some(10),
+        }
+    }
+}
+
 /// I2C driver
 pub struct I2c<'d, DM: Mode, T = AnyI2c> {
     i2c: PeripheralRef<'d, T>,
     phantom: PhantomData<DM>,
-    frequency: HertzU32,
-    timeout: Option<u32>,
+    config: Config,
+}
+
+impl<T: Instance, DM: Mode> SetConfig for I2c<'_, DM, T> {
+    type Config = Config;
+    type ConfigError = ConfigError;
+
+    fn set_config(&mut self, config: &Self::Config) -> Result<(), Self::ConfigError> {
+        self.apply_config(config)
+    }
 }
 
 impl<T> embedded_hal_02::blocking::i2c::Read for I2c<'_, Blocking, T>
@@ -301,7 +349,7 @@ where
         i2c: impl Peripheral<P = T> + 'd,
         sda: impl Peripheral<P = impl PeripheralOutput> + 'd,
         scl: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        frequency: HertzU32,
+        config: Config,
     ) -> Self {
         crate::into_ref!(i2c);
         crate::into_mapped_ref!(sda, scl);
@@ -309,8 +357,7 @@ where
         let i2c = I2c {
             i2c,
             phantom: PhantomData,
-            frequency,
-            timeout: None,
+            config,
         };
 
         PeripheralClockControl::reset(i2c.info().peripheral);
@@ -318,7 +365,7 @@ where
 
         let i2c = i2c.with_sda(sda).with_scl(scl);
 
-        i2c.info().setup(frequency, None);
+        unwrap!(i2c.info().setup(&i2c.config));
         i2c
     }
 
@@ -330,16 +377,15 @@ where
         PeripheralClockControl::reset(self.info().peripheral);
         PeripheralClockControl::enable(self.info().peripheral);
 
-        self.info().setup(self.frequency, self.timeout);
+        // We know the configuration is valid, we can ignore the result.
+        _ = self.info().setup(&self.config);
     }
 
-    /// Set the I2C timeout.
-    // TODO: explain this function better - what's the unit, what happens on
-    // timeout, and just what exactly is a timeout in this context?
-    pub fn with_timeout(mut self, timeout: Option<u32>) -> Self {
-        self.timeout = timeout;
-        self.info().setup(self.frequency, self.timeout);
-        self
+    /// Applies a new configuration.
+    pub fn apply_config(&mut self, config: &Config) -> Result<(), ConfigError> {
+        self.info().setup(config)?;
+        self.config = *config;
+        Ok(())
     }
 
     fn transaction_impl<'a>(
@@ -442,9 +488,9 @@ impl<'d> I2c<'d, Blocking> {
         i2c: impl Peripheral<P = impl Instance> + 'd,
         sda: impl Peripheral<P = impl PeripheralOutput> + 'd,
         scl: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        frequency: HertzU32,
+        config: Config,
     ) -> Self {
-        Self::new_typed(i2c.map_into(), sda, scl, frequency)
+        Self::new_typed(i2c.map_into(), sda, scl, config)
     }
 }
 
@@ -459,9 +505,9 @@ where
         i2c: impl Peripheral<P = T> + 'd,
         sda: impl Peripheral<P = impl PeripheralOutput> + 'd,
         scl: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        frequency: HertzU32,
+        config: Config,
     ) -> Self {
-        Self::new_internal(i2c, sda, scl, frequency)
+        Self::new_internal(i2c, sda, scl, config)
     }
 
     // TODO: missing interrupt APIs
@@ -473,8 +519,7 @@ where
         I2c {
             i2c: self.i2c,
             phantom: PhantomData,
-            frequency: self.frequency,
-            timeout: self.timeout,
+            config: self.config,
         }
     }
 
@@ -743,8 +788,7 @@ where
         I2c {
             i2c: self.i2c,
             phantom: PhantomData,
-            frequency: self.frequency,
-            timeout: self.timeout,
+            config: self.config,
         }
     }
 
@@ -1324,7 +1368,7 @@ impl Info {
 
     /// Configures the I2C peripheral with the specified frequency, clocks, and
     /// optional timeout.
-    fn setup(&self, frequency: HertzU32, timeout: Option<u32>) {
+    fn setup(&self, config: &Config) -> Result<(), ConfigError> {
         self.register_block().ctr().write(|w| {
             // Set I2C controller to master mode
             w.ms_mode().set_bit();
@@ -1358,12 +1402,14 @@ impl Info {
                 let clock = clocks.xtal_clock.convert();
             }
         }
-        self.set_frequency(clock, frequency, timeout);
+        self.set_frequency(clock, config.frequency, config.timeout);
 
         self.update_config();
 
         // Reset entire peripheral (also resets fifo)
         self.reset();
+
+        Ok(())
     }
 
     /// Resets the I2C controller (FIFO + FSM + command list)
diff --git a/examples/src/bin/embassy_i2c.rs b/examples/src/bin/embassy_i2c.rs
index 0f757be363e..c3118707c19 100644
--- a/examples/src/bin/embassy_i2c.rs
+++ b/examples/src/bin/embassy_i2c.rs
@@ -19,7 +19,12 @@
 use embassy_executor::Spawner;
 use embassy_time::{Duration, Timer};
 use esp_backtrace as _;
-use esp_hal::{gpio::Io, i2c::master::I2c, prelude::*, timer::timg::TimerGroup};
+use esp_hal::{
+    gpio::Io,
+    i2c::master::{Config, I2c},
+    prelude::*,
+    timer::timg::TimerGroup,
+};
 use lis3dh_async::{Lis3dh, Range, SlaveAddr};
 
 #[esp_hal_embassy::main]
@@ -31,7 +36,12 @@ async fn main(_spawner: Spawner) {
 
     let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
 
-    let i2c0 = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 400.kHz()).into_async();
+    let i2c0 = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, {
+        let mut config = Config::default();
+        config.frequency = 400.kHz();
+        config
+    })
+    .into_async();
 
     let mut lis3dh = Lis3dh::new_i2c(i2c0, SlaveAddr::Alternate).await.unwrap();
     lis3dh.set_range(Range::G8).await.unwrap();
diff --git a/examples/src/bin/embassy_i2c_bmp180_calibration_data.rs b/examples/src/bin/embassy_i2c_bmp180_calibration_data.rs
index 14c1a3f52b8..cbe5677bf23 100644
--- a/examples/src/bin/embassy_i2c_bmp180_calibration_data.rs
+++ b/examples/src/bin/embassy_i2c_bmp180_calibration_data.rs
@@ -20,7 +20,12 @@
 use embassy_executor::Spawner;
 use embassy_time::{Duration, Timer};
 use esp_backtrace as _;
-use esp_hal::{gpio::Io, i2c::master::I2c, prelude::*, timer::timg::TimerGroup};
+use esp_hal::{
+    gpio::Io,
+    i2c::master::{Config, I2c},
+    prelude::*,
+    timer::timg::TimerGroup,
+};
 
 #[esp_hal_embassy::main]
 async fn main(_spawner: Spawner) {
@@ -31,7 +36,12 @@ async fn main(_spawner: Spawner) {
 
     let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
 
-    let mut i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 400.kHz()).into_async();
+    let mut i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, {
+        let mut config = Config::default();
+        config.frequency = 400.kHz();
+        config
+    })
+    .into_async();
 
     loop {
         let mut data = [0u8; 22];
diff --git a/examples/src/bin/i2c_bmp180_calibration_data.rs b/examples/src/bin/i2c_bmp180_calibration_data.rs
index 88fc52fa0aa..5ffe3a2e03c 100644
--- a/examples/src/bin/i2c_bmp180_calibration_data.rs
+++ b/examples/src/bin/i2c_bmp180_calibration_data.rs
@@ -12,7 +12,11 @@
 #![no_main]
 
 use esp_backtrace as _;
-use esp_hal::{gpio::Io, i2c::master::I2c, prelude::*};
+use esp_hal::{
+    gpio::Io,
+    i2c::master::{Config, I2c},
+    prelude::*,
+};
 use esp_println::println;
 
 #[entry]
@@ -23,7 +27,12 @@ fn main() -> ! {
 
     // Create a new peripheral object with the described wiring and standard
     // I2C clock speed:
-    let mut i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz());
+    let mut i2c = I2c::new(
+        peripherals.I2C0,
+        io.pins.gpio4,
+        io.pins.gpio5,
+        Config::default(),
+    );
 
     loop {
         let mut data = [0u8; 22];
diff --git a/examples/src/bin/i2c_display.rs b/examples/src/bin/i2c_display.rs
index bb1270dc468..25836cb4fd6 100644
--- a/examples/src/bin/i2c_display.rs
+++ b/examples/src/bin/i2c_display.rs
@@ -22,7 +22,12 @@ use embedded_graphics::{
     text::{Alignment, Text},
 };
 use esp_backtrace as _;
-use esp_hal::{delay::Delay, gpio::Io, i2c::master::I2c, prelude::*};
+use esp_hal::{
+    delay::Delay,
+    gpio::Io,
+    i2c::master::{Config, I2c},
+    prelude::*,
+};
 use ssd1306::{prelude::*, I2CDisplayInterface, Ssd1306};
 
 #[entry]
@@ -34,7 +39,12 @@ fn main() -> ! {
 
     // Create a new peripheral object with the described wiring
     // and standard I2C clock speed
-    let i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz());
+    let i2c = I2c::new(
+        peripherals.I2C0,
+        io.pins.gpio4,
+        io.pins.gpio5,
+        Config::default(),
+    );
 
     // Initialize display
     let interface = I2CDisplayInterface::new(i2c);
diff --git a/examples/src/bin/lcd_cam_ov2640.rs b/examples/src/bin/lcd_cam_ov2640.rs
index 03290740bcd..d57023b36e9 100644
--- a/examples/src/bin/lcd_cam_ov2640.rs
+++ b/examples/src/bin/lcd_cam_ov2640.rs
@@ -29,7 +29,10 @@ use esp_hal::{
     dma::{Dma, DmaPriority},
     dma_rx_stream_buffer,
     gpio::Io,
-    i2c::{self, master::I2c},
+    i2c::{
+        self,
+        master::{Config, I2c},
+    },
     lcd_cam::{
         cam::{Camera, RxEightBits},
         LcdCam,
@@ -79,7 +82,7 @@ fn main() -> ! {
 
     delay.delay_millis(500u32);
 
-    let i2c = I2c::new(peripherals.I2C0, cam_siod, cam_sioc, 100u32.kHz());
+    let i2c = I2c::new(peripherals.I2C0, cam_siod, cam_sioc, Config::default());
 
     let mut sccb = Sccb::new(i2c);
 
diff --git a/hil-test/tests/i2c.rs b/hil-test/tests/i2c.rs
index 2106897f071..b0afb9fc786 100644
--- a/hil-test/tests/i2c.rs
+++ b/hil-test/tests/i2c.rs
@@ -7,8 +7,7 @@
 
 use esp_hal::{
     gpio::Io,
-    i2c::master::{I2c, Operation},
-    prelude::*,
+    i2c::master::{Config, I2c, Operation},
     Async,
     Blocking,
 };
@@ -40,7 +39,7 @@ mod tests {
 
         // Create a new peripheral object with the described wiring and standard
         // I2C clock speed:
-        let i2c = I2c::new(peripherals.I2C0, sda, scl, 100.kHz());
+        let i2c = I2c::new(peripherals.I2C0, sda, scl, Config::default());
 
         Context { i2c }
     }

From 0a4e714242ae9e137355bd5e4284030d96cca6e9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?D=C3=A1niel=20Buga?= <bugadani@gmail.com>
Date: Thu, 7 Nov 2024 13:08:02 +0100
Subject: [PATCH 2/4] Remove pins from constructors

---
 esp-hal/CHANGELOG.md                          |  4 +-
 esp-hal/MIGRATING-0.21.md                     | 21 ++++--
 esp-hal/src/i2c/master/mod.rs                 | 66 +++++++------------
 examples/src/bin/embassy_i2c.rs               |  4 +-
 .../embassy_i2c_bmp180_calibration_data.rs    |  4 +-
 .../src/bin/i2c_bmp180_calibration_data.rs    |  9 +--
 examples/src/bin/i2c_display.rs               |  9 +--
 examples/src/bin/lcd_cam_ov2640.rs            |  4 +-
 hil-test/tests/i2c.rs                         |  4 +-
 9 files changed, 59 insertions(+), 66 deletions(-)

diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md
index 3398006021c..e6a52bcf6c6 100644
--- a/esp-hal/CHANGELOG.md
+++ b/esp-hal/CHANGELOG.md
@@ -36,7 +36,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `embassy_embedded_hal::SetConfig` is now implemented for `{Spi, SpiDma, SpiDmaBus}` (#2448)
 - `slave::Spi::{with_mosi(), with_miso(), with_sclk(), with_cs()}` functions (#2485)
 - I8080: Added `set_8bits_order()` to set the byte order in 8-bit mode (#2487)
-- `embassy_embedded_hal::SetConfig` is now implemented for `spi::master::{Spi, SpiDma, SpiDmaBus}`, `i2c::master::I2c` (#2448, #?)
+- `embassy_embedded_hal::SetConfig` is now implemented for `spi::master::{Spi, SpiDma, SpiDmaBus}`, `i2c::master::I2c` (#2448, #2477)
+- `I2c::{apply_config(), with_sda(), with_scl()}` (#2477)
 
 ### Changed
 
@@ -76,6 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The `i2s::{I2sWrite, I2sWriteDma, I2sRead, I2sReadDma, I2sWriteDmaAsync, I2sReadDmaAsync}` traits have been removed. (#2316)
 - The `ledc::ChannelHW` trait is no longer generic. (#2387)
 - The `I2c::new_with_timeout` constructors have been removed (#2361)
+- `I2c::new()` no longer takes `frequency` and pins as parameters. (#?)
 - The `spi::master::HalfDuplexReadWrite` trait has been removed. (#2373)
 - The `Spi::with_pins` methods have been removed. (#2373)
 - The `Spi::new_half_duplex` constructor have been removed. (#2373)
diff --git a/esp-hal/MIGRATING-0.21.md b/esp-hal/MIGRATING-0.21.md
index bb8281368f2..33b34419cc8 100644
--- a/esp-hal/MIGRATING-0.21.md
+++ b/esp-hal/MIGRATING-0.21.md
@@ -72,14 +72,23 @@ The I2C master driver and related types have been moved to `esp_hal::i2c::master
 The `with_timeout` constructors have been removed. `new` and `new_typed` now take a `Config` struct
 with the available configuration options.
 
+The constructors no longer take pins. Use `with_sda` and `with_scl` instead.
+
 ```diff
+-use esp_hal::i2c::I2c;
++use esp_hal::i2c::{Config, I2c};
 -let i2c = I2c::new_with_timeout(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz(), timeout);
-+let i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, {
-+    let mut config = Config::default();
-+    config.frequency = 100.kHz();
-+    config.timeout = timeout;
-+    config
-+});
++I2c::new_with_config(
++    peripherals.I2C0,
++    {
++        let mut config = Config::default();
++        config.frequency = 100.kHz();
++        config.timeout = timeout;
++        config
++    },
++)
++.with_sda(io.pins.gpio4)
++.with_scl(io.pins.gpio5);
 ```
 
 ## Changes to half-duplex SPI
diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs
index 24e505c8b90..4891dba9e7c 100644
--- a/esp-hal/src/i2c/master/mod.rs
+++ b/esp-hal/src/i2c/master/mod.rs
@@ -26,10 +26,10 @@
 //! // and standard I2C clock speed.
 //! let mut i2c = I2c::new(
 //!     peripherals.I2C0,
-//!     io.pins.gpio1,
-//!     io.pins.gpio2,
 //!     Config::default(),
-//! );
+//! )
+//! .with_sda(io.pins.gpio1)
+//! .with_scl(io.pins.gpio2);
 //!
 //! loop {
 //!     let mut data = [0u8; 22];
@@ -345,30 +345,6 @@ impl<'d, T, DM: Mode> I2c<'d, DM, T>
 where
     T: Instance,
 {
-    fn new_internal(
-        i2c: impl Peripheral<P = T> + 'd,
-        sda: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        scl: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        config: Config,
-    ) -> Self {
-        crate::into_ref!(i2c);
-        crate::into_mapped_ref!(sda, scl);
-
-        let i2c = I2c {
-            i2c,
-            phantom: PhantomData,
-            config,
-        };
-
-        PeripheralClockControl::reset(i2c.info().peripheral);
-        PeripheralClockControl::enable(i2c.info().peripheral);
-
-        let i2c = i2c.with_sda(sda).with_scl(scl);
-
-        unwrap!(i2c.info().setup(&i2c.config));
-        i2c
-    }
-
     fn info(&self) -> &Info {
         self.i2c.info()
     }
@@ -445,14 +421,16 @@ where
         Ok(())
     }
 
-    fn with_sda(self, sda: impl Peripheral<P = impl PeripheralOutput> + 'd) -> Self {
+    /// Connect a pin to the I2C SDA signal.
+    pub fn with_sda(self, sda: impl Peripheral<P = impl PeripheralOutput> + 'd) -> Self {
         let info = self.info();
         let input = info.sda_input;
         let output = info.sda_output;
         self.with_pin(sda, input, output)
     }
 
-    fn with_scl(self, scl: impl Peripheral<P = impl PeripheralOutput> + 'd) -> Self {
+    /// Connect a pin to the I2C SCL signal.
+    pub fn with_scl(self, scl: impl Peripheral<P = impl PeripheralOutput> + 'd) -> Self {
         let info = self.info();
         let input = info.scl_input;
         let output = info.scl_output;
@@ -484,13 +462,8 @@ impl<'d> I2c<'d, Blocking> {
     /// Create a new I2C instance
     /// This will enable the peripheral but the peripheral won't get
     /// automatically disabled when this gets dropped.
-    pub fn new(
-        i2c: impl Peripheral<P = impl Instance> + 'd,
-        sda: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        scl: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        config: Config,
-    ) -> Self {
-        Self::new_typed(i2c.map_into(), sda, scl, config)
+    pub fn new(i2c: impl Peripheral<P = impl Instance> + 'd, config: Config) -> Self {
+        Self::new_typed(i2c.map_into(), config)
     }
 }
 
@@ -501,13 +474,20 @@ where
     /// Create a new I2C instance
     /// This will enable the peripheral but the peripheral won't get
     /// automatically disabled when this gets dropped.
-    pub fn new_typed(
-        i2c: impl Peripheral<P = T> + 'd,
-        sda: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        scl: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        config: Config,
-    ) -> Self {
-        Self::new_internal(i2c, sda, scl, config)
+    pub fn new_typed(i2c: impl Peripheral<P = T> + 'd, config: Config) -> Self {
+        crate::into_ref!(i2c);
+
+        let i2c = I2c {
+            i2c,
+            phantom: PhantomData,
+            config,
+        };
+
+        PeripheralClockControl::reset(i2c.info().peripheral);
+        PeripheralClockControl::enable(i2c.info().peripheral);
+
+        unwrap!(i2c.info().setup(&i2c.config));
+        i2c
     }
 
     // TODO: missing interrupt APIs
diff --git a/examples/src/bin/embassy_i2c.rs b/examples/src/bin/embassy_i2c.rs
index c3118707c19..64c046695a7 100644
--- a/examples/src/bin/embassy_i2c.rs
+++ b/examples/src/bin/embassy_i2c.rs
@@ -36,11 +36,13 @@ async fn main(_spawner: Spawner) {
 
     let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
 
-    let i2c0 = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, {
+    let i2c0 = I2c::new(peripherals.I2C0, {
         let mut config = Config::default();
         config.frequency = 400.kHz();
         config
     })
+    .with_sda(io.pins.gpio4)
+    .with_scl(io.pins.gpio5)
     .into_async();
 
     let mut lis3dh = Lis3dh::new_i2c(i2c0, SlaveAddr::Alternate).await.unwrap();
diff --git a/examples/src/bin/embassy_i2c_bmp180_calibration_data.rs b/examples/src/bin/embassy_i2c_bmp180_calibration_data.rs
index cbe5677bf23..204f9e61f38 100644
--- a/examples/src/bin/embassy_i2c_bmp180_calibration_data.rs
+++ b/examples/src/bin/embassy_i2c_bmp180_calibration_data.rs
@@ -36,11 +36,13 @@ async fn main(_spawner: Spawner) {
 
     let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
 
-    let mut i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, {
+    let mut i2c = I2c::new(peripherals.I2C0, {
         let mut config = Config::default();
         config.frequency = 400.kHz();
         config
     })
+    .with_sda(io.pins.gpio4)
+    .with_scl(io.pins.gpio5)
     .into_async();
 
     loop {
diff --git a/examples/src/bin/i2c_bmp180_calibration_data.rs b/examples/src/bin/i2c_bmp180_calibration_data.rs
index 5ffe3a2e03c..7d75aa73adc 100644
--- a/examples/src/bin/i2c_bmp180_calibration_data.rs
+++ b/examples/src/bin/i2c_bmp180_calibration_data.rs
@@ -27,12 +27,9 @@ fn main() -> ! {
 
     // Create a new peripheral object with the described wiring and standard
     // I2C clock speed:
-    let mut i2c = I2c::new(
-        peripherals.I2C0,
-        io.pins.gpio4,
-        io.pins.gpio5,
-        Config::default(),
-    );
+    let mut i2c = I2c::new(peripherals.I2C0, Config::default())
+        .with_sda(io.pins.gpio4)
+        .with_scl(io.pins.gpio5);
 
     loop {
         let mut data = [0u8; 22];
diff --git a/examples/src/bin/i2c_display.rs b/examples/src/bin/i2c_display.rs
index 25836cb4fd6..cf17fa0a02e 100644
--- a/examples/src/bin/i2c_display.rs
+++ b/examples/src/bin/i2c_display.rs
@@ -39,12 +39,9 @@ fn main() -> ! {
 
     // Create a new peripheral object with the described wiring
     // and standard I2C clock speed
-    let i2c = I2c::new(
-        peripherals.I2C0,
-        io.pins.gpio4,
-        io.pins.gpio5,
-        Config::default(),
-    );
+    let i2c = I2c::new(peripherals.I2C0, Config::default())
+        .with_sda(io.pins.gpio4)
+        .with_scl(io.pins.gpio5);
 
     // Initialize display
     let interface = I2CDisplayInterface::new(i2c);
diff --git a/examples/src/bin/lcd_cam_ov2640.rs b/examples/src/bin/lcd_cam_ov2640.rs
index d57023b36e9..6b9198f97bf 100644
--- a/examples/src/bin/lcd_cam_ov2640.rs
+++ b/examples/src/bin/lcd_cam_ov2640.rs
@@ -82,7 +82,9 @@ fn main() -> ! {
 
     delay.delay_millis(500u32);
 
-    let i2c = I2c::new(peripherals.I2C0, cam_siod, cam_sioc, Config::default());
+    let i2c = I2c::new(peripherals.I2C0, Config::default())
+        .with_sda(cam_siod)
+        .with_scl(cam_sioc);
 
     let mut sccb = Sccb::new(i2c);
 
diff --git a/hil-test/tests/i2c.rs b/hil-test/tests/i2c.rs
index b0afb9fc786..09718abf151 100644
--- a/hil-test/tests/i2c.rs
+++ b/hil-test/tests/i2c.rs
@@ -39,7 +39,9 @@ mod tests {
 
         // Create a new peripheral object with the described wiring and standard
         // I2C clock speed:
-        let i2c = I2c::new(peripherals.I2C0, sda, scl, Config::default());
+        let i2c = I2c::new(peripherals.I2C0, Config::default())
+            .with_sda(sda)
+            .with_scl(scl);
 
         Context { i2c }
     }

From 8dba74a89755110f50df651165ef48836126cf92 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?D=C3=A1niel=20Buga?= <bugadani@gmail.com>
Date: Fri, 8 Nov 2024 12:38:04 +0100
Subject: [PATCH 3/4] Implement timeout changes, document them

---
 esp-hal/CHANGELOG.md          |  2 ++
 esp-hal/MIGRATING-0.21.md     | 14 +++++++++++
 esp-hal/src/i2c/master/mod.rs | 45 +++++++++++++----------------------
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md
index e6a52bcf6c6..297763bf4b3 100644
--- a/esp-hal/CHANGELOG.md
+++ b/esp-hal/CHANGELOG.md
@@ -61,6 +61,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The I2S driver has been moved to `i2s::master` (#2472)
 - `slave::Spi` constructors no longer take pins (#2485)
 - The `I2c` master driver has been moved from `esp_hal::i2c` to `esp_hal::i2c::master`. (#2476)
+- The `I2c` master driver has been moved to `esp_hal::i2c::master`. (#2476)
+- `I2c` SCL timeout is now defined in bus clock cycles. (#2477)
 
 ### Fixed
 
diff --git a/esp-hal/MIGRATING-0.21.md b/esp-hal/MIGRATING-0.21.md
index 33b34419cc8..57460bd8037 100644
--- a/esp-hal/MIGRATING-0.21.md
+++ b/esp-hal/MIGRATING-0.21.md
@@ -72,6 +72,10 @@ The I2C master driver and related types have been moved to `esp_hal::i2c::master
 The `with_timeout` constructors have been removed. `new` and `new_typed` now take a `Config` struct
 with the available configuration options.
 
+- The default configuration is now:
+  - bus frequency: 100 kHz
+  - timeout: about 10 bus clock cycles
+
 The constructors no longer take pins. Use `with_sda` and `with_scl` instead.
 
 ```diff
@@ -91,6 +95,16 @@ The constructors no longer take pins. Use `with_sda` and `with_scl` instead.
 +.with_scl(io.pins.gpio5);
 ```
 
+### The calculation of I2C timeout has changed
+
+Previously, I2C timeouts were counted in increments of I2C peripheral clock cycles. This meant that
+the configure value meant different lengths of time depending on the device. With this update, the
+I2C configuration now expects the timeout value in number of bus clock cycles, which is consistent
+between devices.
+
+ESP32 and ESP32-S2 use an exact number of clock cycles for its timeout. Other MCUs, however, use
+the `2^timeout` value internally, and the HAL rounds up the timeout to the next appropriate value.
+
 ## Changes to half-duplex SPI
 
 The `HalfDuplexMode` and `FullDuplexMode` type parameters have been removed from SPI master and slave
diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs
index 4891dba9e7c..01135959dbe 100644
--- a/esp-hal/src/i2c/master/mod.rs
+++ b/esp-hal/src/i2c/master/mod.rs
@@ -1240,8 +1240,7 @@ fn configure_clock(
     scl_stop_setup_time: u32,
     scl_start_hold_time: u32,
     scl_stop_hold_time: u32,
-    time_out_value: u32,
-    time_out_en: bool,
+    timeout: Option<u32>,
 ) {
     unsafe {
         // divider
@@ -1291,19 +1290,15 @@ fn configure_clock(
         // timeout mechanism
         cfg_if::cfg_if! {
             if #[cfg(esp32)] {
-                // timeout
                 register_block
                     .to()
-                    .write(|w| w.time_out().bits(time_out_value));
+                    .write(|w| w.time_out().bits(unwrap!(timeout)));
             } else {
-                // timeout
-                // FIXME: Enable timout for other chips!
-                #[allow(clippy::useless_conversion)]
                 register_block
                     .to()
-                    .write(|w| w.time_out_en().bit(time_out_en)
+                    .write(|w| w.time_out_en().bit(timeout.is_some())
                     .time_out_value()
-                    .bits(time_out_value.try_into().unwrap())
+                    .bits(timeout.unwrap_or(1) as _)
                 );
             }
         }
@@ -1437,8 +1432,9 @@ impl Info {
         let sda_sample = scl_high / 2;
         let setup = half_cycle;
         let hold = half_cycle;
-        // default we set the timeout value to 10 bus cycles
-        let time_out_value = timeout.unwrap_or(half_cycle * 20);
+        let timeout = timeout.map_or(Some(0xF_FFFF), |to_bus| {
+            Some((to_bus * 2 * half_cycle).min(0xF_FFFF))
+        });
 
         // SCL period. According to the TRM, we should always subtract 1 to SCL low
         // period
@@ -1491,8 +1487,7 @@ impl Info {
             scl_stop_setup_time,
             scl_start_hold_time,
             scl_stop_hold_time,
-            time_out_value,
-            true,
+            timeout,
         );
     }
 
@@ -1515,8 +1510,6 @@ impl Info {
         let sda_sample = half_cycle / 2 - 1;
         let setup = half_cycle;
         let hold = half_cycle;
-        // default we set the timeout value to 10 bus cycles
-        let time_out_value = timeout.unwrap_or(half_cycle * 20);
 
         // scl period
         let scl_low_period = scl_low - 1;
@@ -1531,7 +1524,6 @@ impl Info {
         // hold
         let scl_start_hold_time = hold - 1;
         let scl_stop_hold_time = hold;
-        let time_out_en = true;
 
         configure_clock(
             self.register_block(),
@@ -1545,8 +1537,7 @@ impl Info {
             scl_stop_setup_time,
             scl_start_hold_time,
             scl_stop_hold_time,
-            time_out_value,
-            time_out_en,
+            timeout.map(|to_bus| (to_bus * 2 * half_cycle).min(0xFF_FFFF)),
         );
     }
 
@@ -1578,14 +1569,6 @@ impl Info {
         let setup = half_cycle;
         let hold = half_cycle;
 
-        let time_out_value = if let Some(timeout) = timeout {
-            timeout
-        } else {
-            // default we set the timeout value to about 10 bus cycles
-            // log(20*half_cycle)/log(2) = log(half_cycle)/log(2) +  log(20)/log(2)
-            (4 * 8 - (5 * half_cycle).leading_zeros()) + 2
-        };
-
         // According to the Technical Reference Manual, the following timings must be
         // subtracted by 1. However, according to the practical measurement and
         // some hardware behaviour, if wait_high_period and scl_high minus one.
@@ -1605,7 +1588,6 @@ impl Info {
         // hold
         let scl_start_hold_time = hold - 1;
         let scl_stop_hold_time = hold - 1;
-        let time_out_en = true;
 
         configure_clock(
             self.register_block(),
@@ -1619,8 +1601,13 @@ impl Info {
             scl_stop_setup_time,
             scl_start_hold_time,
             scl_stop_hold_time,
-            time_out_value,
-            time_out_en,
+            timeout.map(|to_bus| {
+                let to_peri = (to_bus * 2 * half_cycle).max(1);
+                let log2 = to_peri.ilog2();
+                // Round up so that we don't shorten timeouts.
+                let raw = if to_peri != 1 << log2 { log2 + 1 } else { log2 };
+                raw.min(0x1F)
+            }),
         );
     }
 

From d793250a8167c6d4355cfab919767b0cf677081e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?D=C3=A1niel=20Buga?= <bugadani@gmail.com>
Date: Fri, 8 Nov 2024 13:56:04 +0100
Subject: [PATCH 4/4] Fix up changelog

---
 esp-hal/CHANGELOG.md | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md
index 297763bf4b3..4de005f9d94 100644
--- a/esp-hal/CHANGELOG.md
+++ b/esp-hal/CHANGELOG.md
@@ -33,10 +33,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `{Uart, UartRx, UartTx}` now implement `embassy_embedded_hal::SetConfig` (#2449)
 - GPIO ETM tasks and events now accept `InputSignal` and `OutputSignal` (#2427)
 - `spi::master::Config` and `{Spi, SpiDma, SpiDmaBus}::apply_config` (#2448)
-- `embassy_embedded_hal::SetConfig` is now implemented for `{Spi, SpiDma, SpiDmaBus}` (#2448)
+- `embassy_embedded_hal::SetConfig` is now implemented for `spi::master::{Spi, SpiDma, SpiDmaBus}`, `i2c::master::I2c` (#2448, #2477)
 - `slave::Spi::{with_mosi(), with_miso(), with_sclk(), with_cs()}` functions (#2485)
 - I8080: Added `set_8bits_order()` to set the byte order in 8-bit mode (#2487)
-- `embassy_embedded_hal::SetConfig` is now implemented for `spi::master::{Spi, SpiDma, SpiDmaBus}`, `i2c::master::I2c` (#2448, #2477)
 - `I2c::{apply_config(), with_sda(), with_scl()}` (#2477)
 
 ### Changed
@@ -61,7 +60,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The I2S driver has been moved to `i2s::master` (#2472)
 - `slave::Spi` constructors no longer take pins (#2485)
 - The `I2c` master driver has been moved from `esp_hal::i2c` to `esp_hal::i2c::master`. (#2476)
-- The `I2c` master driver has been moved to `esp_hal::i2c::master`. (#2476)
 - `I2c` SCL timeout is now defined in bus clock cycles. (#2477)
 
 ### Fixed
@@ -79,7 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The `i2s::{I2sWrite, I2sWriteDma, I2sRead, I2sReadDma, I2sWriteDmaAsync, I2sReadDmaAsync}` traits have been removed. (#2316)
 - The `ledc::ChannelHW` trait is no longer generic. (#2387)
 - The `I2c::new_with_timeout` constructors have been removed (#2361)
-- `I2c::new()` no longer takes `frequency` and pins as parameters. (#?)
+- `I2c::new()` no longer takes `frequency` and pins as parameters. (#2477)
 - The `spi::master::HalfDuplexReadWrite` trait has been removed. (#2373)
 - The `Spi::with_pins` methods have been removed. (#2373)
 - The `Spi::new_half_duplex` constructor have been removed. (#2373)