Re: [PATCH RFC 3/4] samples: rust: add Rust serial device bus sample device driver

[Dirk Behme]

Hi Markus,

On 20.12.25 19:44, Markus Probst wrote:
> Add a sample Rust serial device bus device driver illustrating the usage
> of the platform bus abstractions.
> 
> This drivers probes through either a match of device / driver name or a
> match within the OF ID table.
> ---
>  samples/rust/Kconfig               |  10 +++
>  samples/rust/Makefile              |   1 +
>  samples/rust/rust_driver_serdev.rs | 175 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 186 insertions(+)
...
> diff --git a/samples/rust/rust_driver_serdev.rs b/samples/rust/rust_driver_serdev.rs
> new file mode 100644
> index 000000000000..f23b38a26c32
> --- /dev/null
> +++ b/samples/rust/rust_driver_serdev.rs
> @@ -0,0 +1,175 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust Serial device bus device driver sample.
> +
> +use kernel::{
> +    acpi,
> +    device::{
> +        self,
> +        property::{
> +            FwNodeReferenceArgs,
> +            NArgs, //
> +        },
> +        Bound,
> +        Core, //
> +    },
> +    of,
> +    prelude::*,
> +    serdev,
> +    str::CString,
> +    sync::aref::ARef, //
> +};
> +
> +struct SampleDriver {
> +    sdev: ARef<serdev::Device>,
> +}
> +
> +struct Info(u32);
> +
> +kernel::of_device_table!(
> +    OF_TABLE,
> +    MODULE_OF_TABLE,
> +    <SampleDriver as serdev::Driver>::IdInfo,
> +    [(of::DeviceId::new(c"test,rust_driver_serdev"), Info(42))]

I stopped reading here regarding the new "rust_driver_serdev" but
re-reading Rob's

https://lore.kernel.org/rust-for-linux/20241022234712.GB1848992-robh@xxxxxxxxxx/

adding "test,<whatever>" should be fine as-is without any documenation.

> +);
> +
> +kernel::acpi_device_table!(
> +    ACPI_TABLE,
> +    MODULE_ACPI_TABLE,
> +    <SampleDriver as serdev::Driver>::IdInfo,
> +    [(acpi::DeviceId::new(c"LNUXBEEF"), Info(0))]
> +);
> +
> +#[vtable]
> +impl serdev::Driver for SampleDriver {
> +    type IdInfo = Info;
> +    type InitialData = ();
> +    type LateProbeData = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +    const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
> +
> +    fn probe(sdev: &serdev::Device, info: Option<&Self::IdInfo>) -> impl PinInit<Self, Error> {
> +        let dev = sdev.as_ref();
> +
> +        dev_dbg!(dev, "Probe Rust Serial device bus device driver sample.\n");
> +
> +        if let Some(info) = info {
> +            dev_info!(dev, "Probed with info: '{}'.\n", info.0);
> +        }

Last time I had a look to the log output from rust_driver_platform.rs
(where this is copied from?) I was slightly confused to see the
"Probed with ..." in the log but not the "Probe Rust ...". Well, I
hadn't DEBUG enabled. So I wonder if the combination of `dev_dbg!()`
and `dev_info!()` this way should be improved? At least in
rust_driver_platform.rs because we could drop that here completely? I
mean in rust_driver_platform.rs it makes sense to demonstrate how
`info` is supposed to work. But do we need that here?


> +        if dev.fwnode().is_some_and(|node| node.is_of_node()) {
> +            Self::properties_parse(dev)?;
> +        }


This is a left over from copy & paste? I mean having all this
`properties_parse()` below and calling it here does not make any sense
here? And should be dropped completely?


> +
> +        Ok(Self { sdev: sdev.into() })
> +    }
> +
> +    fn configure(
> +        sdev: &serdev::Device<Core>,
> +        _this: Pin<&Self>,
> +        _id_info: Option<&Self::IdInfo>,
> +    ) -> Result {
> +        dev_dbg!(
> +            sdev.as_ref(),
> +            "Configure Rust Serial device bus device driver sample.\n"
> +        );
> +
> +        sdev.set_baudrate(115200);
> +        sdev.set_flow_control(false);
> +        sdev.set_parity(serdev::Parity::None)?;
> +        Ok(())
> +    }
> +
> +    fn late_probe(
> +        sdev: &serdev::Device<Bound>,
> +        _this: Pin<&Self>,
> +        _initial_data: Self::InitialData,
> +    ) -> impl PinInit<Self::LateProbeData, Error> {
> +        dev_dbg!(
> +            sdev.as_ref(),
> +            "Late Probe Rust Serial device bus device driver sample.\n"
> +        );
> +        Ok(())
> +    }
> +
> +    fn receive(
> +        sdev: &serdev::Device<Bound>,
> +        _this: Pin<&Self>,
> +        _late_probe_this: Pin<&Self::LateProbeData>,
> +        data: &[u8],
> +    ) -> usize {
> +        let _ = sdev.write_all(data, serdev::Timeout::MaxScheduleTimeout);


Is it intended to have a function with the name `receive()`calling
`write()`?

> +        data.len()
> +    }
> +}
> +
> +impl SampleDriver {
> +    fn properties_parse(dev: &device::Device) -> Result {


As mentioned above I think this is a left over from copy & paste and
should be dropped?

Cheers,

Dirk