On Sun, Jun 8, 2025 at 12:50 PM Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
>
> On Sun, Jun 08, 2025 at 01:46:17PM +0200, Benno Lossin wrote:
> > On Sun Jun 8, 2025 at 12:48 PM CEST, Danilo Krummrich wrote:
> > > On Sun, Jun 08, 2025 at 09:54:30AM +0200, Benno Lossin wrote:
> > >> 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.
> > >
> > > Generally, that's fine, but in this case I'd rather keep it as it is for
> > > consistency with the rest of the file.
> >
> > Then let's also change the OF bindings in this file to that style :)
>
> Fine for me.
>
> @Igor: If you do so, please do it in a seaparate patch.
That's definitely not possible with `if cfg!(not(CONFIG_ACPI))`
because it is a runtime condition and it breaks compilation in case
either `CONFIG_OF` or `CONFIG_ACPI` is not set, the Rust compiler
can't find `of_match_device`/`acpi_match_device` because in that case
they are defined as `static inline` and are not parsed by bindgen.
Alternatively the following pattern works:
fn foo() {
#[cfg(not(CONFIG_ACPI))] {
None
}
#[cfg(CONFIG_ACPI)] {
// some logic.
}
}
Also the argument `dev` of `both of_id_info` and `acpi_id_info` must be
renamed into `_dev` to mark it as unused in case CONFIG_ACPI/CONFIG_OF
is off and the argument is not used.
Let me know if this pattern is acceptable. To my opinion it is not much
different from the original 2 functions conditioned by #[cfg] and
requires some nasty changes.
Thanks
Igor
