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 :)
>> > + 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?
>
> A driver potentially has lots of them, but the device is only matching a single
> entry in one of the driver's ID tables and hence a single ID info.
Ah so if `of_id_info` and `acpi_id_info` return `Some(_)`, then both
values are the same?
---
Cheers,
Benno
