Re: [PATCH v3 16/16] samples: rust: add Rust platform sample driver

[Rob Herring <robh@kernel.org>]

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. :) )

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.

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@kernel.org>
> ---
>  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@ti.com>
> 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.

> +    )]
> +);
> +
> +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.

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

> +            (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.

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