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

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 6037 bytes --]

Hi Dirk,

On Sun, 2025-12-21 at 10:11 +0100, Dirk Behme wrote:
> 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@kernel.org/
> 
> 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?
It was meant to show, that this is the function in which fwnode
properties should be parsed. As I did indeed base the sample on the
platform driver sample, I left it as-is. I will minimize it down to 1
property without an extra function, given that the platform driver
sample already shows how to use properties.
> 
> 
> > +
> > +        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()`?
This sample driver echos any data it receives back to the serial
device.

It shows how to receive data from the serial device and how to send
data to the serial device.

It depends on the driver, if it is necessary to call write inside
receive. The serial device may want a reply or ack from the driver
after it sent a message.

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

Thanks
- Markus Probst

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]