platform: move get_id_info from platform.rs to public

[Richardhongyu]

Signed-off-by: Li Hongyu [email protected]

[Richardhongyu]

In this PR, @wedsonaf suggests to move get_id_info function into public. I write a macro to implement this function for drivers that need to have info from of_id.

[bjorn3]

There are two other options to consider:

1. Create a standalone function that can be used by any adapter.
2. Create a bang macro used inside the impl. So for example impl<T: Driver> Adapter<T> { impl_get_id_info!(); /* ... */ }.

I think I prefer the first option.

[bjorn3]

#[derive(Foo)] generally implements a trait called Foo for the respective type. Not sure if we should introduce a trait GetIdInfo, or not.

[wedsonaf]

@Richardhongyu thanks for doing this. What I had in mind, however, was similar to what @bjorn3 said: a function that other adapters can call.

Is there a reason why you think such an option wouldn't work?

[Richardhongyu]

I tried this before. But the get_id_info function needs to use Driver and Device, which belong to each file, e.g. foo.rs, bar.rs. I think it's hard to use them in the get_id_info when moving get_id_info into a public file.

[bjorn3]

Then option 2

Create a bang macro used inside the impl. So for example impl<T: Driver> Adapter<T> { impl_get_id_info!(); /* ... */ }.

Should work.

[Richardhongyu]

Option 2 looks easier to read. I will change to this style.

[Richardhongyu]

The new commit uses Option 2.

[Richardhongyu]

@bjorn3 Thanks for reviewing and helping improve the code!

[Richardhongyu]

I pushed a new version to apply the above changes.

[wedsonaf]

I tried this before. But the get_id_info function needs to use Driver and Device, which belong to each file, e.g. foo.rs, bar.rs. I think it's hard to use them in the get_id_info when moving get_id_info into a public file.

Why can't these be passed in as arguments?

[wedsonaf]

Here's a working version (just the first few lines are different, the rest is exactly the same):

fn get_id_info<T>(
    dev: &impl RawDevice,
    table: Option<driver::IdTable<'static, of::DeviceId, T>>,
) -> Option<&'static T> {
    let table = table?;

    // SAFETY: `table` has static lifetime, so it is valid for read. `dev` is guaranteed to be
    // valid while it's alive, so is the raw device returned by it.
    let id = unsafe { bindings::of_match_device(table.as_ref(), dev.raw_device()) };
    if id.is_null() {
        return None;
    }

    // SAFETY: `id` is a pointer within the static table, so it's always valid.
    let offset = unsafe { (*id).data };
    if offset.is_null() {
        return None;
    }

    // SAFETY: The offset comes from a previous call to `offset_from` in `IdArray::new`, which
    // guarantees that the resulting pointer is within the table.
    let ptr = unsafe { id.cast::<u8>().offset(offset as _).cast::<Option<T>>() };

    // SAFETY: The id table has a static lifetime, so `ptr` is guaranteed to be valid for read.
    unsafe { (&*ptr).as_ref() }
}

probe_callback (and similar functions for other buses) would call the above function as follows:

let info = get_id_info(&dev, T::OF_DEVICE_ID_TABLE);

Do you see a reason why this wouldn't work?

[Richardhongyu]

@wedsonaf This is really cool! Thanks for your help!

I didn't think of passing IdTable in the arguments. I learn a lot from your design. Thanks again for reviewing and correcting me!