Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

["Benno Lossin" <lossin@kernel.org>]

On Fri Jun 6, 2025 at 7:08 PM CEST, Igor Korotin wrote:
> @@ -141,6 +141,38 @@ pub trait Adapter {
>      /// The type holding driver private data about each device id supported by the driver.
>      type IdInfo: 'static;
>  
> +    /// The [`acpi::IdTable`] of the corresponding driver
> +    fn acpi_id_table() -> Option<acpi::IdTable<Self::IdInfo>>;
> +
> +    /// Returns the driver's private data from the matching entry in the [`acpi::IdTable`], if any.
> +    ///
> +    /// If this returns `None`, it means there is no match with an entry in the [`acpi::IdTable`].
> +    #[cfg(CONFIG_ACPI)]
> +    fn acpi_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
> +        let table = Self::acpi_id_table()?;
> +
> +        // SAFETY:
> +        // - `table` has static lifetime, hence it's valid for read,
> +        // - `dev` is guaranteed to be valid while it's alive, and so is `pdev.as_ref().as_raw()`.
> +        let raw_id = unsafe { bindings::acpi_match_device(table.as_ptr(), dev.as_raw()) };
> +
> +        if raw_id.is_null() {
> +            None
> +        } else {
> +            // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and
> +            // does not add additional invariants, so it's safe to transmute.
> +            let id = unsafe { &*raw_id.cast::<acpi::DeviceId>() };
> +
> +            Some(table.info(<acpi::DeviceId as crate::device_id::RawDeviceId>::index(id)))
> +        }
> +    }
> +
> +    #[cfg(not(CONFIG_ACPI))]
> +    #[allow(missing_docs)]

I think we should change this to one single definition and do

    if cfg!(not(CONFIG_ACPI)) {
        return None;
    }
    /* body from above */

In a single function instead.

> +    fn acpi_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> {
> +        None
> +    }
> +
>      /// The [`of::IdTable`] of the corresponding driver.
>      fn of_id_table() -> Option<of::IdTable<Self::IdInfo>>;
>  
> @@ -178,6 +210,11 @@ fn of_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> {
>      /// If this returns `None`, it means that there is no match in any of the ID tables directly
>      /// associated with a [`device::Device`].
>      fn id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
> +        let id = Self::acpi_id_info(dev);
> +        if id.is_some() {
> +            return id;
> +        }

Is a driver only going to have one id_info? Or is there some kind of
precedence?

---
Cheers,
Benno