On Tue, Oct 22, 2024 at 07:04:08PM -0500, Rob Herring wrote: > On Tue, Oct 22, 2024 at 11:31:53PM +0200, Danilo Krummrich wrote: > > Add a sample Rust platform driver illustrating the usage of the platform > > bus abstractions. > > > > This driver probes through either a match of device / driver name or a > > match within the OF ID table. > > I know if rust compiles it works, but how does one actually use/test > this? (I know ways, but I might be in the minority. :) ) For testing a name match I just used platform_device_register_simple() in a separate module. Probing through the OF table is indeed a bit more tricky. Since I was too lazy to pull out a random ARM device of my cupboard I just used QEMU on x86 and did what drivers/of/unittest.c does. If you're smart you can also just enable those unit tests and change the compatible string to "unittest". :) > > The DT unittests already define test platform devices. I'd be happy to > add a device node there. Then you don't have to muck with the DT on some > device and it even works on x86 or UML. Sounds good, I'll add one in there for this sample driver -- any preferences? > > And I've started working on DT (fwnode really) property API bindings as > well, and this will be great to test them with. > > > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx> > > --- > > MAINTAINERS | 1 + > > samples/rust/Kconfig | 10 +++++ > > samples/rust/Makefile | 1 + > > samples/rust/rust_driver_platform.rs | 62 ++++++++++++++++++++++++++++ > > 4 files changed, 74 insertions(+) > > create mode 100644 samples/rust/rust_driver_platform.rs > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 173540375863..583b6588fd1e 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -6986,6 +6986,7 @@ F: rust/kernel/device_id.rs > > F: rust/kernel/devres.rs > > F: rust/kernel/driver.rs > > F: rust/kernel/platform.rs > > +F: samples/rust/rust_driver_platform.rs > > > > DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS) > > M: Nishanth Menon <nm@xxxxxx> > > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig > > index 6d468193cdd8..70126b750426 100644 > > --- a/samples/rust/Kconfig > > +++ b/samples/rust/Kconfig > > @@ -41,6 +41,16 @@ config SAMPLE_RUST_DRIVER_PCI > > > > If unsure, say N. > > > > +config SAMPLE_RUST_DRIVER_PLATFORM > > + tristate "Platform Driver" > > + help > > + This option builds the Rust Platform driver sample. > > + > > + To compile this as a module, choose M here: > > + the module will be called rust_driver_platform. > > + > > + If unsure, say N. > > + > > config SAMPLE_RUST_HOSTPROGS > > bool "Host programs" > > help > > diff --git a/samples/rust/Makefile b/samples/rust/Makefile > > index b66767f4a62a..11fcb312ed36 100644 > > --- a/samples/rust/Makefile > > +++ b/samples/rust/Makefile > > @@ -3,5 +3,6 @@ > > obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o > > obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o > > obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o > > +obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o > > > > subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS) += hostprogs > > diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs > > new file mode 100644 > > index 000000000000..55caaaa4f216 > > --- /dev/null > > +++ b/samples/rust/rust_driver_platform.rs > > @@ -0,0 +1,62 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Rust Platform driver sample. > > + > > +use kernel::{c_str, of, platform, prelude::*}; > > + > > +struct SampleDriver { > > + pdev: platform::Device, > > +} > > + > > +struct Info(u32); > > + > > +kernel::of_device_table!( > > + OF_TABLE, > > + MODULE_OF_TABLE, > > + <SampleDriver as platform::Driver>::IdInfo, > > + [( > > + of::DeviceId::new(c_str!("redhat,rust-sample-platform-driver")), > > Perhaps use the same compatible as the commented example. Same comments > on that apply to this. > > > + Info(42) > > Most of the time this is a pointer to a struct. It would be better to > show how to do that. No, this should never be a raw pointer. There is no reason for a driver to perfom this kind of unsafe operation to store any ID info data. This ID info data is moved into the `IdArray` on compile time. And the bus abstraction takes care of providing a reference to this structure in `Driver::probe`. So, technically, this example is fine. But if you have ideas for more meaningful data to store there, I happy to change it. > > > + )] > > +); > > + > > +impl platform::Driver for SampleDriver { > > + type IdInfo = Info; > > + const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE; > > Probably want to name this OF_ID_TABLE for when ACPI_ID_TABLE is added. Yes, makes sense. > > > + > > + fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> { > > + dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n"); > > + > > + match (Self::of_match_device(pdev), info) { > > That answers my question on being exposed to drivers. This is a big no > for me. Agreed, we don't need it. Please also see my previous reply in the platform bus abstraction. > > > + (Some(id), Some(info)) => { > > + dev_info!( > > + pdev.as_ref(), > > + "Probed by OF compatible match: '{}' with info: '{}'.\n", > > + id.compatible(), > > As I mentioned, "real" drivers don't need the compatible string. Same here. > > > + info.0 > > + ); > > + } > > + _ => { > > + dev_info!(pdev.as_ref(), "Probed by name.\n"); > > + } > > + }; > > + > > + let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?; > > + > > + Ok(drvdata.into()) > > + } > > +} > > + > > +impl Drop for SampleDriver { > > + fn drop(&mut self) { > > + dev_dbg!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n"); > > + } > > +} > > + > > +kernel::module_platform_driver! { > > + type: SampleDriver, > > + name: "rust_driver_platform", > > + author: "Danilo Krummrich", > > + description: "Rust Platform driver", > > + license: "GPL v2", > > +} > > -- > > 2.46.2 > > >