Re: [RFC PATCH v1 1/4] rust: add AMBA bus abstractions

["Danilo Krummrich" <dakr@kernel.org>]

On Sun Jan 4, 2026 at 7:06 AM CET, Ke Sun wrote:
> Add Rust abstractions for the ARM AMBA bus, including:
> - Device type wrapper for amba_device
> - DeviceId for device matching
> - TryFrom implementation for converting device::Device to amba::Device
> - IRQ and I/O resource management methods

I don't see any Driver trait or Adapter implementation implementing to register
and probe an AMBA driver.

Since I had a look at your RTC abstractions, I can see the reason why, i.e. you
baked that part into the RTC abstractions, but this is not how the device /
driver model works. I will comment about this in the RTC patch. But for this
one, please implement thing analogous to platform, PCI, etc.

> +impl DeviceId {
> +    /// Creates a new device ID from an AMBA device ID and mask.
> +    ///
> +    /// A driver binds to a device when `(hardware_device_id & mask) == id`.
> +    #[inline(always)]
> +    pub const fn new(id: u32, mask: u32) -> Self {
> +        // SAFETY: FFI type is valid to be zero-initialized.
> +        let mut amba: bindings::amba_id = unsafe { core::mem::zeroed() };

Please use pin_init::zeroed() instead.

> +        amba.id = id;
> +        amba.mask = mask;
> +        amba.data = core::ptr::null_mut();
> +
> +        Self(amba)
> +    }
> +
> +    /// Creates a new device ID with driver-specific data.
> +    #[inline(always)]
> +    pub const fn new_with_data(id: u32, mask: u32, data: usize) -> Self {
> +        // SAFETY: FFI type is valid to be zero-initialized.
> +        let mut amba: bindings::amba_id = unsafe { core::mem::zeroed() };

Same here.

> +        amba.id = id;
> +        amba.mask = mask;
> +        amba.data = data as *mut core::ffi::c_void;

What is this data? The driver specific data is derived from the index stored in
DRIVER_DATA_OFFSET. This does interfere with each other.

You already get driver specific data per entry from the generic code, you can
just drop this.

> +    /// Returns the memory resource.
> +    pub fn resource(&self) -> Option<&Resource> {

Why does this return an Option if you do not have a None case? Are you sure
resource can never be NULL?

> +        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct amba_device`.
> +        let resource = unsafe { &raw mut (*self.as_raw()).res };
> +        // SAFETY: `resource` is a valid pointer to a `struct resource`.
> +        Some(unsafe { Resource::from_raw(resource) })
> +    }
> +}
> +
> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> +    fn as_ref(&self) -> &device::Device<Ctx> {
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a
> +        // valid `struct amba_device`.
> +        let dev = unsafe { &raw mut (*self.as_raw()).dev };
> +
> +        // SAFETY: `dev` points to a valid `struct device`.
> +        unsafe { device::Device::from_raw(dev) }
> +    }
> +}
> +
> +// SAFETY: `Device` is a transparent wrapper that doesn't depend on its generic
> +// argument.
> +crate::impl_device_context_deref!(unsafe { Device });
> +crate::impl_device_context_into_aref!(Device);
> +
> +impl<Ctx: device::DeviceContext> TryFrom<&device::Device<Ctx>> for &Device<Ctx> {
> +    type Error = kernel::error::Error;
> +
> +    fn try_from(dev: &device::Device<Ctx>) -> Result<Self, Self::Error> {
> +        // SAFETY: By the type invariant of `Device`, `dev.as_raw()` is a valid pointer
> +        // to a `struct device`.
> +        if !unsafe { bindings::dev_is_amba(dev.as_raw()) } {
> +            return Err(crate::error::code::EINVAL);
> +        }
> +
> +        // SAFETY: We've just verified that the bus type of `dev` equals
> +        // `bindings::amba_bustype`, hence `dev` must be embedded in a valid
> +        // `struct amba_device` as guaranteed by the corresponding C code.
> +        let adev = unsafe { container_of!(dev.as_raw(), bindings::amba_device, dev) };
> +
> +        // SAFETY: `adev` is a valid pointer to a `struct amba_device`.
> +        Ok(unsafe { &*adev.cast() })
> +    }
> +}

Please implement the AsBusDevice trait instead, this TryFrom solution you
probably found in the platform and PCI bus are for very specific cases. For
instance, if you have a driver that exports and auxiliary device, but is
supported on two busses.

In your case, you simply want to derive an amba device from a generic device in
a class device abstraction (RTC device), hence please incorporate the
AsBusDevice trait.

> +impl Device<device::Core> {}
> +
> +impl Device<device::Bound> {
> +    /// Returns an [`IoRequest`] for the memory resource.
> +    pub fn io_request(&self) -> Option<IoRequest<'_>> {
> +        self.resource()
> +            // SAFETY: `resource` is valid for the lifetime of the `IoRequest`.
> +            .map(|resource| unsafe { IoRequest::new(self.as_ref(), resource) })
> +    }
> +
> +    /// Returns an [`IrqRequest`] for the IRQ at the given index.
> +    pub fn irq_by_index(&self, index: u32) -> Result<IrqRequest<'_>> {
> +        if index >= bindings::AMBA_NR_IRQS {
> +            return Err(crate::error::code::EINVAL);
> +        }
> +
> +        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct amba_device`.
> +        let irq = unsafe { (*self.as_raw()).irq[index as usize] };
> +
> +        if irq == 0 {
> +            return Err(crate::error::code::ENXIO);
> +        }
> +
> +        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
> +        Ok(unsafe { IrqRequest::new(self.as_ref(), irq) })
> +    }
> +
> +    /// Requests an IRQ at the given index and returns a [`irq::Registration`].
> +    pub fn request_irq_by_index<'a, T: irq::Handler + 'static>(
> +        &'a self,
> +        flags: irq::Flags,
> +        index: u32,
> +        name: &'static CStr,
> +        handler: impl PinInit<T, Error> + 'a,
> +    ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {

Please don't return a Result here, the error code is already within the impl
PinInit<T, Error>. Please use pin_init::pin_init_scope() instead.

> +        let request = self.irq_by_index(index)?;
> +
> +        Ok(irq::Registration::<T>::new(request, flags, name, handler))
> +    }
> +}