On Tue, Oct 29, 2024 at 10:19:08AM +0100, Dirk Behme wrote: > On 29.10.2024 09:50, Danilo Krummrich wrote: > > On Tue, Oct 29, 2024 at 08:20:55AM +0100, Dirk Behme wrote: > > > On 28.10.2024 11:19, Danilo Krummrich wrote: > > > > On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote: > > > > > > +/// IdTable type for platform drivers. > > > > > > +pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<of::DeviceId, T>; > > > > > > + > > > > > > +/// The platform driver trait. > > > > > > +/// > > > > > > +/// # Example > > > > > > +/// > > > > > > +///``` > > > > > > +/// # use kernel::{bindings, c_str, of, platform}; > > > > > > +/// > > > > > > +/// struct MyDriver; > > > > > > +/// > > > > > > +/// kernel::of_device_table!( > > > > > > +/// OF_TABLE, > > > > > > +/// MODULE_OF_TABLE, > > > > > > > > > > It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names > > > > > used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER > > > > > and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other > > > > > examples/samples in this patch series. Found that while using the *same* > > > > > somewhere else ;) > > > > > > > > I think the names by themselves are fine. They're local to the module. However, > > > > we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e. > > > > "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols. > > > > > > > > I think we somehow need to build the module name into the symbol name as well. > > > > > > Something like this? > > > > No, I think we should just encode the Rust module name / path, which should make > > this a unique symbol name. > > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > index 5b1329fba528..63e81ec2d6fd 100644 > > --- a/rust/kernel/device_id.rs > > +++ b/rust/kernel/device_id.rs > > @@ -154,7 +154,7 @@ macro_rules! module_device_table { > > ($table_type: literal, $module_table_name:ident, $table_name:ident) => { > > #[rustfmt::skip] > > #[export_name = > > - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") > > + concat!("__mod_", $table_type, "__", module_path!(), "_", stringify!($table_name), "_device_table") > > ] > > static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = > > unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; > > > > For the doctests for instance this > > > > "__mod_of__OF_TABLE_device_table" > > > > becomes > > > > "__mod_of__doctests_kernel_generated_OF_TABLE_device_table". > > > What implies *one* OF/PCI_TABLE per path (file)? No, you can still have as many as you want for the same file, you just have to give them different identifier names -- you can't have two statics with the same name in one file anyways. Well, I guess you somehow can (just like the doctests do), but it does make sense to declare drivers in such a way. I think as long as we take care that separate Rust modules can't interfere with each other it's good enough. > > For example adding a second FooDriver example to platform.rs won't be > possible? Not unless you change the identifier name unfortunately. But that might be fixable by putting doctests in separate `mod $(DOCTEST) {}` blocks. > > +/// struct FooDriver; > +/// > +/// kernel::of_device_table!( > +/// OF_TABLE, > +/// MODULE_OF_TABLE, > +/// <FooDriver as platform::Driver>::IdInfo, > +/// [ > +/// (of::DeviceId::new(c_str!("test,rust-device2")), ()) > +/// ] > +/// ); > > Best regards > > Dirk > > > >