On Tue, Oct 28, 2025 at 11:18 AM chenmiao <[email protected]> wrote:
> During the implementation process, we found that the current two paradigms in
> Rust — bindings and impl — are extremely complex and lack comprehensive
> documentation. There is no clear explanation as to why Bus and Device models
> need to be implemented using different approaches.
I don't think they need to be implemented using different approaches.
The difference between the two is that:
- the currently implemented devices do not expose any bus, they stay
on a bus. This means the bus is never a child of the device
- the currently implemented buses are all in C code, whereas there are
devices implemented in Rust.
I agree that the Rust-to-C bridge code is complex, but it does have
documentation, much more so than the C versions in fact. If there are
specific aspects of the documentation that you would like to see
improved, you can help by explaining what problems and sources of
confusion you encountered.
> +/// A safe wrapper around [`bindings::I2CBus`].
> +#[repr(transparent)]
> +#[derive(Debug, common::Wrapper)]
> +pub struct I2CBus(Opaque<bindings::I2CBus>);
> +
> +unsafe impl Send for I2CBus {}
> +unsafe impl Sync for I2CBus {}
> +
> +unsafe impl ObjectType for I2CBus {
> + type Class = BusClass;
> + const TYPE_NAME: &'static CStr =
> + unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_I2C_BUS)
> };
> +}
> +
> +qom_isa!(I2CBus: BusState, Object);
> +
> +// TODO: add virtual methods
> +pub trait I2CBusImpl: DeviceImpl + IsA<I2CBus> {}
> +/// Trait for methods of [`I2CBus`] and its subclasses.
> +pub trait I2CBusMethods: ObjectDeref
> +where
> + Self::Target: IsA<I2CBus>
There are no virtual methods, and therefore I2CBus does not have
subclasses. Therefore you don't need these traits and you can
implement the functions directly on I2CBus.
> +{
> + /// # Safety
> + ///
> + /// Initialize an I2C bus
> + fn init_bus(&self, parent: &DeviceState, name: &str) -> *mut
> bindings::I2CBus {
> + assert!(bql::is_locked());
> + unsafe { bindings::i2c_init_bus(parent.as_mut_ptr(),
> name.as_ptr().cast()) }
> + }
This should return an Owned<I2CBus>.
> + /// Sets the I2C bus master.
> + ///
> + /// # Safety
> + ///
> + /// This function is unsafe because:
> + /// - `bh` must be a valid pointer to a `QEMUBH`.
> + /// - The caller must ensure that `self` is in a valid state.
> + /// - The caller must guarantee no data races occur during execution.
> + ///
> + /// TODO: `bindings:QEMUBH` should be wrapped by Opaque<>.
> + unsafe fn set_master(&self, bh: *mut bindings::QEMUBH) {
> + assert!(bql::is_locked());
> + unsafe { bindings::i2c_bus_master(self.upcast().as_mut_ptr(), bh) }
> + }
I agree with Zhao, I would leave out this completely. You can add a
TODO ("i2c_bus_master missing until QEMUBH is wrapped").
> + /// # Safety
> + ///
> + /// Release an I2C bus
> + fn release(&self) {
> + assert!(bql::is_locked());
> + unsafe { bindings::i2c_bus_release(self.upcast().as_mut_ptr()) }
> + }
Same for this, which is the counterpart of i2c_bus_master. In Rust, in
fact, release() should be done with an RAII guard returned by
set_master.
> +unsafe impl ObjectType for I2CSlave {
> + type Class = I2CSlaveClass;
> + const TYPE_NAME: &'static CStr =
> + unsafe {
> CStr::from_bytes_with_nul_unchecked(bindings::TYPE_I2C_SLAVE) };
> +}
> +
> +qom_isa!(I2CSlave: DeviceState, Object);
> +
> +// TODO: add virtual methods
> +pub trait I2CSlaveImpl: DeviceImpl + IsA<I2CSlave> {
> + /// Master to slave. Returns non-zero for a NAK, 0 for success.
> + const SEND: Option<fn(&Self, data: u8) -> Result<bool>> = None;
Make it return an "enum I2CResult { Ack, Nack };" instead of a Result.
Likewise other function pointers here do not need a Result<>.
> + /// Master to slave (asynchronous). Receiving slave must call
> `i2c_ack()`.
> + const SEND_ASYNC: Option<fn(&Self, data: u8)> = None;
This requires the bottom half machinery. Leave it out.
> +/// Trait for methods of [`I2CSlave`] and its subclasses.
> +pub trait I2CSlaveMethods: ObjectDeref
> +where
> + Self::Target: IsA<I2CSlave>,
> +{
> + /// Create an I2C slave device on the heap.
> + ///
> + /// # Arguments
> + /// * `name` - a device type name
> + /// * `addr` - I2C address of the slave when put on a bus
> + ///
> + /// This only initializes the device state structure and allows
> + /// properties to be set. Type `name` must exist. The device still
> + /// needs to be realized.
> + fn init_new(name: &str, addr: u8) -> Owned<I2CSlave> {
> + assert!(bql::is_locked());
> + unsafe {
> + let slave = bindings::i2c_slave_new(name.as_ptr().cast(), addr);
> + Owned::from(I2CSlave::from_raw(slave))
> + }
> + }
> +
> + /// Create and realize an I2C slave device on the heap.
> + ///
> + /// # Arguments
> + /// * `bus` - I2C bus to put it on
> + /// * `name` - I2C slave device type name
> + /// * `addr` - I2C address of the slave when put on a bus
> + ///
> + /// Create the device state structure, initialize it, put it on the
> + /// specified `bus`, and drop the reference to it (the device is
> realized).
> + fn create_simple(&self, bus: &I2CBus, name: &str, addr: u8) ->
> Owned<I2CSlave> {
> + assert!(bql::is_locked());
> + unsafe {
> + let slave =
> + bindings::i2c_slave_create_simple(bus.as_mut_ptr(),
> name.as_ptr().cast(), addr);
> + Owned::from(I2CSlave::from_raw(slave))
> + }
> + }
> +
> + /// Set the I2C bus address of a slave device
> + ///
> + /// # Arguments
> + /// * `address` - I2C address of the slave when put on a bus
> + fn set_address(&self, address: u8) {
> + assert!(bql::is_locked());
> + unsafe { bindings::i2c_slave_set_address(self.upcast().as_mut_ptr(),
> address) }
> + }
These three are used by boards, which we don't model. We can keep the
code simple by leaving them off (in addition, init_new and
create_simple would be *class* methods, as visible from the fact that
they don't use self at all).
> + /// Get the I2C bus address of a slave device
> + fn get_address(&self) -> u8 {
> + assert!(bql::is_locked());
> + // SAFETY: the BQL ensures that no one else writes to the I2CSlave
> structure,
> + // and the I2CSlave must be initialized to get an IsA<I2CSlave>.
> + let slave = unsafe { *self.upcast().as_ptr() };
> + slave.address
> + }
> +}
> +
> +impl<R: ObjectDeref> I2CSlaveMethods for R where R::Target: IsA<I2CSlave> {}
> +
> +/// Enum representing I2C events
> +#[repr(i32)]
> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
> +pub enum I2CEvent {
> + StartRecv = 0,
> + StartSend = 1,
> + StartSendAsync = 2,
> + Finish = 3,
> + Nack = 4,
> +}
Make it "= bindings::I2C_START_RECV" etc. You can then use
#[derive(common::TryInto)] instead of implementing by hand the From
traits.
Paolo
