Re: [PATCH v4 13/13] samples: rust: add Rust platform sample driver

[Danilo Krummrich]

On Thu, Dec 05, 2024 at 06:09:10PM +0100, Dirk Behme wrote:
> Hi Danilo,
> 
> On 05.12.24 15:14, 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.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> 
> Not a review comment, but a question/proposal:
> 
> What do you think to convert the platform sample into an example/test?
> And drop it in samples/rust then? Like [1] below?

Generally, I think doctests are indeed preferrable. In this particular case
though, I think it's better to have a sample module, since this way it can serve
as go-to example of how to write a platform driver in Rust.

Especially for (kernel) folks who do not have a Rust (for Linux) background it's
way more accessible.

> 
> We would have (a) a complete example in the documentation and (b) some
> (KUnit) test coverage and (c) have one patch less in the series and
> (d) one file less to maintain long term.
> 
> I think to remember that it was mentioned somewhere that a
> documentation example / KUnit test is preferred over samples/rust (?).
> 
> Just an idea :)
> 
> Best regards
> 
> Dirk
> 
> [1]
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae576c842c51..365fc48b7041 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7035,7 +7035,6 @@ 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/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 868cfddb75a2..77aeb6fc2120 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -142,30 +142,55 @@ macro_rules! module_platform_driver {
>  /// # Example
>  ///
>  ///```
> -/// # use kernel::{bindings, c_str, of, platform};
> +/// # mod mydriver {
> +/// #
> +/// # // Get this into the scope of the module to make the
> assert_eq!() buildable
> +/// # static __DOCTEST_ANCHOR: i32 = core::line!() as i32 - 4;
> +/// #
> +/// # use kernel::{c_str, of, platform, prelude::*};
> +/// #
> +/// struct MyDriver {
> +///     pdev: platform::Device,
> +/// }
>  ///
> -/// struct MyDriver;
> +/// struct Info(u32);
>  ///
>  /// kernel::of_device_table!(
>  ///     OF_TABLE,
>  ///     MODULE_OF_TABLE,
>  ///     <MyDriver as platform::Driver>::IdInfo,
> -///     [
> -///         (of::DeviceId::new(c_str!("test,device")), ())
> -///     ]
> +///     [(of::DeviceId::new(c_str!("test,rust-device")), Info(42))]
>  /// );
>  ///
>  /// impl platform::Driver for MyDriver {
> -///     type IdInfo = ();
> +///     type IdInfo = Info;
>  ///     const OF_ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
>  ///
> -///     fn probe(
> -///         _pdev: &mut platform::Device,
> -///         _id_info: Option<&Self::IdInfo>,
> -///     ) -> Result<Pin<KBox<Self>>> {
> -///         Err(ENODEV)
> +///     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");
> +///
> +///         assert_eq!(info.unwrap().0, 42);
> +///
> +///         let drvdata = KBox::new(Self { pdev: pdev.clone() },
> GFP_KERNEL)?;
> +///
> +///         Ok(drvdata.into())
> +///     }
> +/// }
> +///
> +/// impl Drop for MyDriver {
> +///     fn drop(&mut self) {
> +///         dev_dbg!(self.pdev.as_ref(), "Remove Rust Platform driver
> sample.\n");
>  ///     }
>  /// }
> +///
> +/// kernel::module_platform_driver! {
> +///     type: MyDriver,
> +///     name: "rust_driver_platform",
> +///     author: "Danilo Krummrich",
> +///     description: "Rust Platform driver",
> +///     license: "GPL v2",
> +/// }
> +/// # }
>  ///```
>  pub trait Driver {
>      /// The type holding information about each device id supported
> by the driver.
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index 70126b750426..6d468193cdd8 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -41,16 +41,6 @@ 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 761d13fff018..2f5b6bdb2fa5 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -4,7 +4,6 @@ ccflags-y += -I$(src)				# needed for trace events
>  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
> 
>  rust_print-y := rust_print_main.o rust_print_events.o
> 
> diff --git a/samples/rust/rust_driver_platform.rs
> b/samples/rust/rust_driver_platform.rs
> deleted file mode 100644
> index 2f0dbbe69e10..000000000000
> --- a/samples/rust/rust_driver_platform.rs
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -// 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!("test,rust-device")), Info(42))]
> -);
> -
> -impl platform::Driver for SampleDriver {
> -    type IdInfo = Info;
> -    const OF_ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
> -
> -    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");
> -
> -        if let Some(info) = info {
> -            dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n",
> info.0);
> -        }
> -
> -        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",
> -}
>