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
> >
>
