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

[Dirk Behme <dirk.behme@gmail.com>]

On 06.12.24 09:33, Danilo Krummrich wrote:
> 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@kernel.org>
>>
>> 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.


Yes, ack. Rob said the same :)

Thanks

Dirk


>> 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@ti.com>
>> 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",
>> -}
>>