Re: [RFC PATCH v2 2/5] rust: add AMBA bus driver support

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

On Wed Jan 7, 2026 at 3:37 PM CET, Ke Sun wrote:
> +impl DeviceId {
> +    /// Creates a new device ID from an AMBA device ID and mask.
> +    #[inline(always)]
> +    pub const fn new(id: u32, mask: u32) -> Self {
> +        let mut amba: bindings::amba_id = pin_init::zeroed();
> +        amba.id = id;
> +        amba.mask = mask;
> +        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 {
> +        let mut amba: bindings::amba_id = pin_init::zeroed();
> +        amba.id = id;
> +        amba.mask = mask;
> +        amba.data = data as *mut core::ffi::c_void;

I already mentioned that in the last version, you don't need this function and
writing the data pointer here is wrong, as it is already handled by common code,
i.e. through the offset given to RawDeviceIdIndex above.

> +        Self(amba)
> +    }

<snip>

> +impl Device<device::Core> {}

No need to add an empty impl block.

> +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(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(ENXIO);
> +        }
> +        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
> +        Ok(unsafe { IrqRequest::new(self.as_ref(), irq) })
> +    }
> +
> +    /// Returns a [`irq::Registration`] for the IRQ at the given index.
> +    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,
> +    ) -> impl PinInit<irq::Registration<T>, Error> + 'a {
> +        pin_init::pin_init_scope(move || {
> +            let request = self.irq_by_index(index).map_err(|_| EINVAL)?;

Why do you remap the error code from irq_by_index() to EINVAL unconditionally?

> +            Ok(irq::Registration::<T>::new(request, flags, name, handler))
> +        })
> +    }
> +}

<snip>

> +    extern "C" fn shutdown_callback(adev: *mut bindings::amba_device) {
> +        // SAFETY: `shutdown_callback` is only ever called for a valid `adev`.
> +        let adev = unsafe { &*adev.cast::<Device<device::CoreInternal>>() };
> +        // SAFETY: `shutdown_callback` is only ever called after a successful call to
> +        // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
> +        // and stored a `Pin<KBox<T>>`.
> +        let data = unsafe { adev.as_ref().drvdata_obtain::<T>() };

Please use drvdata_borrow() instead, we must not free the device private data on
shutdown().

> +        T::shutdown(adev, data.as_ref());
> +    }
> +
> +    fn amba_id_table() -> Option<IdTable<<Self as driver::Adapter>::IdInfo>> {
> +        T::AMBA_ID_TABLE
> +    }
> +
> +    fn amba_id_info(
> +        _dev: &Device,
> +        id: *const bindings::amba_id,
> +    ) -> Option<&'static <Self as driver::Adapter>::IdInfo> {
> +        if id.is_null() {
> +            return None;
> +        }
> +        let table = Self::amba_id_table()?;
> +        // SAFETY: `id` is a valid pointer to a `struct amba_id` that was matched by the kernel.
> +        // `DeviceId` is a `#[repr(transparent)]` wrapper of `struct amba_id` and does not add
> +        // additional invariants, so it's safe to transmute.
> +        let device_id = unsafe { &*id.cast::<DeviceId>() };
> +        Some(table.info(<DeviceId as RawDeviceIdIndex>::index(device_id)))
> +    }
> +}
> +
> +impl<T: Driver + 'static> driver::Adapter for Adapter<T> {
> +    type IdInfo = T::IdInfo;
> +
> +    fn acpi_id_table() -> Option<acpi::IdTable<Self::IdInfo>> {
> +        None
> +    }
> +
> +    fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> {
> +        None
> +    }

There is no point implementing this trait with both functions returning None.

> +/// The AMBA driver trait.
> +///
> +/// Drivers must implement this trait in order to get an AMBA driver registered.
> +pub trait Driver: Send {
> +    /// The type holding information about each device id supported by the driver.
> +    type IdInfo: 'static;
> +    /// The table of device ids supported by the driver.
> +    const AMBA_ID_TABLE: Option<IdTable<Self::IdInfo>> = None;

If it is the only ID table an AMBA driver can be matched through, it does not
make sense for the ID table to be optional.