| Message ID | 20250326171411.590681-11-remo@buenzli.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | More Rust bindings for device property reads | expand |
On Wed, Mar 26, 2025 at 06:13:49PM +0100, Remo Senekowitsch wrote: > Add some example usage of the device property read methods for > DT/ACPI/swnode properties. > > Co-developed-by: Rob Herring (Arm) <robh@kernel.org> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org> > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev> > --- > drivers/of/unittest-data/tests-platform.dtsi | 3 ++ > samples/rust/rust_driver_platform.rs | 56 +++++++++++++++++++- > 2 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi > index 4171f43cf..50a51f38a 100644 > --- a/drivers/of/unittest-data/tests-platform.dtsi > +++ b/drivers/of/unittest-data/tests-platform.dtsi > @@ -37,6 +37,9 @@ dev@100 { > test-device@2 { > compatible = "test,rust-device"; > reg = <0x2>; > + > + test,u32-prop = <0xdeadbeef>; > + test,i16-array = /bits/ 16 <1 2 (-3) (-4)>; > }; > }; > > diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs > index 8120609e2..ed25a3781 100644 > --- a/samples/rust/rust_driver_platform.rs > +++ b/samples/rust/rust_driver_platform.rs > @@ -2,7 +2,7 @@ > > //! Rust Platform driver sample. > > -use kernel::{c_str, of, platform, prelude::*}; > +use kernel::{c_str, of, platform, prelude::*, str::CString}; > > struct SampleDriver { > pdev: platform::Device, > @@ -28,6 +28,60 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin > dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0); > } > > + let dev = pdev.as_ref(); We should move this to the top and replace all the 'pdev.as_ref()' with 'dev'. > + if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device")) > + { > + dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx); Like here. (Looks like this is my fault.) > + } > + > + if let Ok(str) = dev > + .property_read::<CString>(c_str!("compatible")) > + .required() > + { > + dev_info!(pdev.as_ref(), "compatible string = {:?}\n", str); > + } > + > + let prop = dev > + .property_read::<bool>(c_str!("test,bool-prop")) > + .required()?; The 'required' is kind of odd for boolean properties. They are never required as not present is the only way to to get false. > + dev_info!(dev, "bool prop is {}\n", prop); > + > + if dev.property_present(c_str!("test,u32-prop")) { > + dev_info!(dev, "'test,u32-prop' is present\n"); > + } > + > + let prop = dev > + .property_read::<u32>(c_str!("test,u32-optional-prop")) > + .or(0x12); > + dev_info!( > + dev, > + "'test,u32-optional-prop' is {:#x} (default = {:#x})\n", > + prop, > + 0x12 > + ); > + > + // Missing property without a default will print an error > + let _ = dev > + .property_read::<u32>(c_str!("test,u32-required-prop")) > + .required()?; > + > + let prop: u32 = dev.property_read(c_str!("test,u32-prop")).required()?; > + dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop); > + > + let prop: [i16; 4] = dev.property_read(c_str!("test,i16-array")).required()?; > + dev_info!(dev, "'test,i16-array' is {:?}\n", prop); > + dev_info!( > + dev, > + "'test,i16-array' length is {}\n", > + dev.property_count_elem::<u16>(c_str!("test,i16-array")) > + .unwrap() > + ); > + > + let prop: KVec<i16> = dev > + .property_read_array_vec(c_str!("test,i16-array"), 4)? > + .required()?; > + dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop); > + > let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?; > > Ok(drvdata.into()) > -- > 2.49.0 >
On Wed Mar 26, 2025 at 11:01 PM CET, Rob Herring wrote: >> >> + let prop = dev >> + .property_read::<bool>(c_str!("test,bool-prop")) >> + .required()?; > > The 'required' is kind of odd for boolean properties. They are never > required as not present is the only way to to get false. Agreed. I can think of a few alternatives: * Make the trait `Property` more flexible to allow each implementor to specify what its output type for the `read` function is, via an associated type. I really don't like this idea, because overly generic APIs can mess with type inference and become less ergonomic because of it. * Use `propert_present` instead. That doesn't perfectly express the intention, because it doesn't warn if the property is present but has a type other than bool. * Add an additional inherent method `property_read_bool`, which returns a plain `bool` instead of `PropertyGuard<bool>`. Then there will be three slightly different ways to read a bool: `property_present`, `property_read_bool` and `property_read::<bool>`. Maybe that's confusing. * Add `property_read_bool` and remove `impl Property for bool`. That would avoid confusion between `property_read_bool` and `property_read::<bool>`, only the former would work.
On Wed, Mar 26, 2025 at 5:24 PM Remo Senekowitsch <remo@buenzli.dev> wrote: > > On Wed Mar 26, 2025 at 11:01 PM CET, Rob Herring wrote: > >> > >> + let prop = dev > >> + .property_read::<bool>(c_str!("test,bool-prop")) > >> + .required()?; > > > > The 'required' is kind of odd for boolean properties. They are never > > required as not present is the only way to to get false. > > Agreed. I can think of a few alternatives: > > * Make the trait `Property` more flexible to allow each implementor to specify > what its output type for the `read` function is, via an associated type. > I really don't like this idea, because overly generic APIs can mess with type > inference and become less ergonomic because of it. > > * Use `propert_present` instead. That doesn't perfectly express the intention, > because it doesn't warn if the property is present but has a type other than > bool. Right. I've been cleaning up the tree to use of_property_read_bool() on bools and of_property_present() on non-bools, so don't want to go back to 1 function. The C code now warns on a mismatch. > * Add an additional inherent method `property_read_bool`, which returns a plain > `bool` instead of `PropertyGuard<bool>`. Then there will be three slightly > different ways to read a bool: `property_present`, `property_read_bool` and > `property_read::<bool>`. Maybe that's confusing. > > * Add `property_read_bool` and remove `impl Property for bool`. That would avoid > confusion between `property_read_bool` and `property_read::<bool>`, only the > former would work. I think I would go with this option. Easier to add another way later than remove one. Rob
On Wed, Mar 26, 2025 at 06:13:49PM +0100, Remo Senekowitsch wrote: > --- a/samples/rust/rust_driver_platform.rs > +++ b/samples/rust/rust_driver_platform.rs > @@ -2,7 +2,7 @@ > > //! Rust Platform driver sample. > > -use kernel::{c_str, of, platform, prelude::*}; > +use kernel::{c_str, of, platform, prelude::*, str::CString}; > > struct SampleDriver { > pdev: platform::Device, > @@ -28,6 +28,60 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin > dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0); > } > > + let dev = pdev.as_ref(); > + if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device")) > + { > + dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx); > + } > + > + if let Ok(str) = dev > + .property_read::<CString>(c_str!("compatible")) > + .required() > + { > + dev_info!(pdev.as_ref(), "compatible string = {:?}\n", str); > + } > + > + let prop = dev > + .property_read::<bool>(c_str!("test,bool-prop")) > + .required()?; > + dev_info!(dev, "bool prop is {}\n", prop); > + > + if dev.property_present(c_str!("test,u32-prop")) { > + dev_info!(dev, "'test,u32-prop' is present\n"); > + } > + > + let prop = dev > + .property_read::<u32>(c_str!("test,u32-optional-prop")) > + .or(0x12); > + dev_info!( > + dev, > + "'test,u32-optional-prop' is {:#x} (default = {:#x})\n", > + prop, > + 0x12 > + ); Printing the default value looks more like a test, rather than a sample. > + > + // Missing property without a default will print an error > + let _ = dev > + .property_read::<u32>(c_str!("test,u32-required-prop")) > + .required()?; > + > + let prop: u32 = dev.property_read(c_str!("test,u32-prop")).required()?; > + dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop); > + > + let prop: [i16; 4] = dev.property_read(c_str!("test,i16-array")).required()?; > + dev_info!(dev, "'test,i16-array' is {:?}\n", prop); > + dev_info!( > + dev, > + "'test,i16-array' length is {}\n", > + dev.property_count_elem::<u16>(c_str!("test,i16-array")) > + .unwrap() Please no unwrap() in the kernel, it may panic. > + ); > + > + let prop: KVec<i16> = dev > + .property_read_array_vec(c_str!("test,i16-array"), 4)? > + .required()?; > + dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop); > + Please move this code to a new function, e.g. impl SampleDriver { fn properties_parse(dev: &device::Device)) -> Result; } in order to keep probe() as clean as possible. If we put too much stuff in there, it might become too confusing to serve as a simple example showing how to implement a platform driver in Rust. Besides that, are we sure we really want to print everything?
diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi index 4171f43cf..50a51f38a 100644 --- a/drivers/of/unittest-data/tests-platform.dtsi +++ b/drivers/of/unittest-data/tests-platform.dtsi @@ -37,6 +37,9 @@ dev@100 { test-device@2 { compatible = "test,rust-device"; reg = <0x2>; + + test,u32-prop = <0xdeadbeef>; + test,i16-array = /bits/ 16 <1 2 (-3) (-4)>; }; }; diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs index 8120609e2..ed25a3781 100644 --- a/samples/rust/rust_driver_platform.rs +++ b/samples/rust/rust_driver_platform.rs @@ -2,7 +2,7 @@ //! Rust Platform driver sample. -use kernel::{c_str, of, platform, prelude::*}; +use kernel::{c_str, of, platform, prelude::*, str::CString}; struct SampleDriver { pdev: platform::Device, @@ -28,6 +28,60 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0); } + let dev = pdev.as_ref(); + if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device")) + { + dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx); + } + + if let Ok(str) = dev + .property_read::<CString>(c_str!("compatible")) + .required() + { + dev_info!(pdev.as_ref(), "compatible string = {:?}\n", str); + } + + let prop = dev + .property_read::<bool>(c_str!("test,bool-prop")) + .required()?; + dev_info!(dev, "bool prop is {}\n", prop); + + if dev.property_present(c_str!("test,u32-prop")) { + dev_info!(dev, "'test,u32-prop' is present\n"); + } + + let prop = dev + .property_read::<u32>(c_str!("test,u32-optional-prop")) + .or(0x12); + dev_info!( + dev, + "'test,u32-optional-prop' is {:#x} (default = {:#x})\n", + prop, + 0x12 + ); + + // Missing property without a default will print an error + let _ = dev + .property_read::<u32>(c_str!("test,u32-required-prop")) + .required()?; + + let prop: u32 = dev.property_read(c_str!("test,u32-prop")).required()?; + dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop); + + let prop: [i16; 4] = dev.property_read(c_str!("test,i16-array")).required()?; + dev_info!(dev, "'test,i16-array' is {:?}\n", prop); + dev_info!( + dev, + "'test,i16-array' length is {}\n", + dev.property_count_elem::<u16>(c_str!("test,i16-array")) + .unwrap() + ); + + let prop: KVec<i16> = dev + .property_read_array_vec(c_str!("test,i16-array"), 4)? + .required()?; + dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop); + let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?; Ok(drvdata.into())