diff mbox series

[RFC,v2,1/1] rust: Add bindings for device properties

Message ID 20241122081257.1776925-1-dirk.behme@de.bosch.com
State Changes Requested
Headers show
Series [RFC,v2,1/1] rust: Add bindings for device properties | expand

Checks

Context Check Description
robh/checkpatch warning total: 0 errors, 1 warnings, 432 lines checked
robh/patch-applied success

Commit Message

Dirk Behme Nov. 22, 2024, 8:12 a.m. UTC
From: "Rob Herring (Arm)" <robh@kernel.org>

The device property API is a firmware agnostic API for reading
properties from firmware (DT/ACPI) devices nodes and swnodes.

While the C API takes a pointer to a caller allocated variable/buffer,
the rust API is designed to return a value and can be used in struct
initialization. Rust generics are also utilized to support different
sizes of properties (e.g. u8, u16, u32).

To build and run the Examples as `rustdoc` tests the kernel Kconfig
options `CONFIG_OF` and `CONFIG_OF_UNITTEST` need to be enabled
additionally. Besides the default `rustdoc` test options
`CONFIG_KUNIT` and `CONFIG_RUST_KERNEL_DOCTESTS`. This even works
on non-ARM architectures as a test device tree is built into the
kernel, then.

The Integer trait is proposed by Alic Ryhl [1].

Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiXPZqKpWSSNdx-Ww-E9h2tOLcF3_8Y4C_JQ0eU8EMwFw@mail.gmail.com/ [1]
Co-developed-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---

This is an update of Rob's initial patch

https://lore.kernel.org/rust-for-linux/20241025-rust-platform-dev-v1-0-0df8dcf7c20b@kernel.org/

condensed in one patch (see below). Rob's initial cover
letter still stands, esp. the part regarding the dependency
on Danilo's PCI and platform device series [2].

Changes in v2:

* Move the major parts to property.rs
* Use the Integer Trait proposed by Alice
* Use MaybeUninit proposed by Alex
* Use Option<> to distinguish between optional and mandatory properties
  proposed by Rob
* Introduce a FwProperty trait. The prefix 'Fw' reads as 'Firmware'.
  Just 'Property' seems to be conflicting with existing.
* Add some rustdoc documentation and Examples (based on Danilo's
  platform sample module). With that I squashed the test device tree
  changes into this patch as we don't need to change Danilo's platform
  sample any more. That change is trivial. Please let me know if you
  rather like a separate patch for it.
* Some more I most probably missed to mention ;)

It would be nice to get some improvement hints for the FIXMEs :)
Trying to use the assert_eq!() fails with

error[E0425]: cannot find value `__DOCTEST_ANCHOR` in this scope
    --> rust/doctests_kernel_generated.rs:4252:102
     |
4252 |             kernel::kunit_assert_eq!("rust_doctest_kernel_property_rs_0", "rust/kernel/property.rs", __DOCTEST_ANCHOR - 150, $left, $right);
     |                                                                                                      ^^^^^^^^^^^^^^^^ not found in this scope
...
4369 | assert_eq!(idx, Ok(0)); // FIXME: How to build this?
     | ---------------------- in this macro invocation
     |
     = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)

  CC      drivers/base/firmware_loader/main.o
  CC      kernel/module/main.o
error: aborting due to 1 previous error

[2] https://lore.kernel.org/all/20241022213221.2383-1-dakr@kernel.org/

 drivers/of/unittest-data/tests-platform.dtsi |   8 +
 rust/bindings/bindings_helper.h              |   1 +
 rust/kernel/device.rs                        |  22 ++
 rust/kernel/lib.rs                           |   1 +
 rust/kernel/property.rs                      | 370 +++++++++++++++++++
 5 files changed, 402 insertions(+)
 create mode 100644 rust/kernel/property.rs

Comments

Alice Ryhl Nov. 22, 2024, 8:47 a.m. UTC | #1
On Fri, Nov 22, 2024 at 9:13 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
> From: "Rob Herring (Arm)" <robh@kernel.org>
>
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
>
> While the C API takes a pointer to a caller allocated variable/buffer,

nit: "caller-allocated" or "to a variable/buffer allocated by the caller"

> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> sizes of properties (e.g. u8, u16, u32).
>
> To build and run the Examples as `rustdoc` tests the kernel Kconfig
> options `CONFIG_OF` and `CONFIG_OF_UNITTEST` need to be enabled
> additionally. Besides the default `rustdoc` test options
> `CONFIG_KUNIT` and `CONFIG_RUST_KERNEL_DOCTESTS`. This even works
> on non-ARM architectures as a test device tree is built into the
> kernel, then.
>
> The Integer trait is proposed by Alic Ryhl [1].

typo ;)

>
> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiXPZqKpWSSNdx-Ww-E9h2tOLcF3_8Y4C_JQ0eU8EMwFw@mail.gmail.com/ [1]
> Co-developed-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>
> This is an update of Rob's initial patch
>
> https://lore.kernel.org/rust-for-linux/20241025-rust-platform-dev-v1-0-0df8dcf7c20b@kernel.org/
>
> condensed in one patch (see below). Rob's initial cover
> letter still stands, esp. the part regarding the dependency
> on Danilo's PCI and platform device series [2].
>
> Changes in v2:
>
> * Move the major parts to property.rs
> * Use the Integer Trait proposed by Alice
> * Use MaybeUninit proposed by Alex
> * Use Option<> to distinguish between optional and mandatory properties
>   proposed by Rob
> * Introduce a FwProperty trait. The prefix 'Fw' reads as 'Firmware'.
>   Just 'Property' seems to be conflicting with existing.
> * Add some rustdoc documentation and Examples (based on Danilo's
>   platform sample module). With that I squashed the test device tree
>   changes into this patch as we don't need to change Danilo's platform
>   sample any more. That change is trivial. Please let me know if you
>   rather like a separate patch for it.
> * Some more I most probably missed to mention ;)
>
> It would be nice to get some improvement hints for the FIXMEs :)

See below :)

> Trying to use the assert_eq!() fails with
>
> error[E0425]: cannot find value `__DOCTEST_ANCHOR` in this scope
>     --> rust/doctests_kernel_generated.rs:4252:102
>      |
> 4252 |             kernel::kunit_assert_eq!("rust_doctest_kernel_property_rs_0", "rust/kernel/property.rs", __DOCTEST_ANCHOR - 150, $left, $right);
>      |                                                                                                      ^^^^^^^^^^^^^^^^ not found in this scope
> ...
> 4369 | assert_eq!(idx, Ok(0)); // FIXME: How to build this?
>      | ---------------------- in this macro invocation
>      |
>      = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
>
>   CC      drivers/base/firmware_loader/main.o
>   CC      kernel/module/main.o
> error: aborting due to 1 previous error
>
> [2] https://lore.kernel.org/all/20241022213221.2383-1-dakr@kernel.org/

I don't know about this one.

> +/// A trait for integer types.
> +///
> +/// This trait limits [`FromBytes`] and [`AsBytes`] to integer types only. Excluding the
> +/// array types. The integer types are `u8`, `u16`, `u32`, `u64`, `usize`, `i8`, `i16`,
> +/// `i32`, `i64` and `isize`. Additionally, the size of these types is encoded in the
> +/// `IntSize` enum.
> +trait Integer: FromBytes + AsBytes + Copy {
> +    /// The size of the integer type.
> +    const SIZE: IntSize;
> +}

This trait should be unsafe with a safety requirement saying that SIZE
must be correct.

Is it intentional that Integer is a private trait? I guess you're
using it as an implementation detail of FwProperty? It would be nice
to mention that 128-bit ints are intentionally not included here.

> +/// The sizes of the integer types. Either 8, 16, 32 or 64 bits.
> +enum IntSize {
> +    /// 8 bits
> +    S8,
> +    /// 16 bits
> +    S16,
> +    /// 32 bits
> +    S32,
> +    /// 64 bits
> +    S64,
> +}
> +
> +macro_rules! impl_int {
> +    ($($typ:ty),* $(,)?) => {$(
> +        impl Integer for $typ {
> +            const SIZE: IntSize = match size_of::<Self>() {
> +                1 => IntSize::S8,
> +                2 => IntSize::S16,
> +                4 => IntSize::S32,
> +                8 => IntSize::S64,
> +                _ => panic!("invalid size"),
> +            };
> +        }
> +    )*};
> +}
> +
> +impl_int! {
> +    u8, u16, u32, u64, usize,
> +    i8, i16, i32, i64, isize,
> +}
> +
> +/// Reads an array of integers from the device firmware.
> +fn read_array<T: Integer>(
> +    device: &Device,
> +    name: &CStr,
> +    val: Option<&mut [MaybeUninit<T>]>,
> +) -> Result<usize> {
> +    let (ptr, len) = match val {
> +        // The read array data case.
> +        Some(val) => (val.as_mut_ptr(), val.len()),
> +        // The read count case.
> +        None => (core::ptr::null_mut(), 0_usize),
> +    };
> +    let ret = match T::SIZE {
> +        // SAFETY: `device_property_read_u8_array` is called with a valid device pointer and name.
> +        IntSize::S8 => unsafe {
> +            bindings::device_property_read_u8_array(
> +                device.as_raw(),
> +                name.as_ptr() as *const i8,
> +                ptr as *mut u8,
> +                len,

Instead of using the i8 type for the name, you should be using
crate::ffi::c_char. Actually, in this case, you can also use
`name.as_char_ptr()` instead, which requires no cast.

> +        },
> +        // SAFETY: `device_property_read_u16_array` is called with a valid device pointer and name.
> +        IntSize::S16 => unsafe {
> +            bindings::device_property_read_u16_array(
> +                device.as_raw(),
> +                name.as_ptr() as *const i8,
> +                ptr as *mut u16,
> +                len,
> +            )
> +        },
> +        // SAFETY: `device_property_read_u32_array` is called with a valid device pointer and name.
> +        IntSize::S32 => unsafe {
> +            bindings::device_property_read_u32_array(
> +                device.as_raw(),
> +                name.as_ptr() as *const i8,
> +                ptr as *mut u32,
> +                len,
> +            )
> +        },
> +        // SAFETY: `device_property_read_u64_array` is called with a valid device pointer and name.
> +        IntSize::S64 => unsafe {
> +            bindings::device_property_read_u64_array(
> +                device.as_raw(),
> +                name.as_ptr() as *const i8,
> +                ptr as *mut u64,
> +                len,
> +            )
> +        },
> +    };
> +    to_result(ret)?;
> +    Ok(ret.try_into()?)
> +}

[...]

> +/// *Note*: To build and run below examples as `rustdoc` tests the additional kernel
> +/// Kconfig options `CONFIG_OF` and `CONFIG_OF_UNITTEST` need to be enabled. This
> +/// even works on non-ARM architectures as a test device tree is built into the
> +/// kernel, then.

Incomplete sentence?

Does this mean that running kunit without those options enabled
results in an error?

> +/// The above examples intentionally don't print any error messages (e.g. with `dev_err!()`).
> +/// The called abstractions already print error messages if needed what is considered to be
> +/// sufficient. The goal is to be less verbose regarding error messages.

typo: "if needed, which is"

> +pub trait FwProperty: Sized {
> +    /// Reads a property from the device.
> +    fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self>;
> +
> +    /// Gets the properties element count.
> +    fn count_elem(device: &Device, name: &CStr) -> Result<usize>;
> +
> +    /// Returns if a firmware string property `name` has match for `match_str`.
> +    fn match_string(device: &Device, name: &CStr, match_str: &CStr) -> Result<usize> {

As far as I can tell, you never override this implementation anywhere.
In that case, it shouldn't be a trait method. You can just put its
implementation directly in property_match_string.

> +        // SAFETY: `device_property_match_string` is called with a valid device pointer and name.
> +        let ret = unsafe {
> +            bindings::device_property_match_string(
> +                device.as_raw(),
> +                name.as_ptr() as *const i8,
> +                match_str.as_ptr() as *const i8,
> +            )

Ditto here about i8.

> +    /// Gets the properties element count.
> +    // FIXME: Could this be made to be a build time error?
> +    fn count_elem(device: &Device, _name: &CStr) -> Result<usize> {

Yes, you should create two traits:

pub trait FwPropertyRead: Sized {
    fn read_property(device: &Device, name: &CStr, default:
Option<Self>) -> Result<Self>;
}

pub trait FwPropertyCount: Sized {
    fn count_elem(device: &Device, name: &CStr) -> Result<usize>;
}

Then don't implement FwPropertyCount for types that can't be counted.

I wonder if it also makes more sense to split FwPropertyRead into two traits:
FwPropertyReadMandatory
FwPropertyReadOptional
to handle the boolean case?

> +impl<T: Integer + Copy> FwProperty for T {
> +    fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self> {
> +        let mut val: [MaybeUninit<T>; 1] = [const { MaybeUninit::uninit() }; 1];
> +        match read_array(device, name, Some(&mut val)) {
> +            // SAFETY: `read_array` returns with valid data
> +            Ok(_) => Ok(unsafe { mem::transmute_copy(&val[0]) }),

This can be simplified:

fn read_property(device: &Device, name: &CStr, default: Option<Self>)
-> Result<Self> {
    let mut val: MaybeUninit<T> = MaybeUninit::uninit();
    match read_array(device, name, Some(core::array::from_mut(&mut val))) {
        // SAFETY: `read_array` returns with valid data
        Ok(()) => Ok(val.assume_init()),

> +impl<T: Integer, const N: usize> FwProperty for [T; N] {
> +    fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self> {
> +        let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
> +        match read_array(device, name, Some(&mut val)) {
> +            // SAFETY: `read_array` returns with valid data
> +            Ok(_) => Ok(unsafe { mem::transmute_copy(&val) }),

I don't think this one can avoid transmute_copy, though.

Alice
Rob Herring (Arm) Nov. 25, 2024, 3:35 p.m. UTC | #2
On Fri, Nov 22, 2024 at 2:13 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
> From: "Rob Herring (Arm)" <robh@kernel.org>
>
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
>
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> sizes of properties (e.g. u8, u16, u32).
>
> To build and run the Examples as `rustdoc` tests the kernel Kconfig
> options `CONFIG_OF` and `CONFIG_OF_UNITTEST` need to be enabled
> additionally. Besides the default `rustdoc` test options
> `CONFIG_KUNIT` and `CONFIG_RUST_KERNEL_DOCTESTS`. This even works
> on non-ARM architectures as a test device tree is built into the
> kernel, then.
>
> The Integer trait is proposed by Alic Ryhl [1].
>
> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiXPZqKpWSSNdx-Ww-E9h2tOLcF3_8Y4C_JQ0eU8EMwFw@mail.gmail.com/ [1]
> Co-developed-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>
> This is an update of Rob's initial patch

I have my own updates on it based on the discussion. It's a bit
different because I've reworked the C API to better mesh with Rust
needs. I just haven't sent it out because I've been busy with other
things and it's the merge window.

You asked me if I was going to work on this and I did. If you want to
do it, just say so. I'm always happy for less work.

Rob
Dirk Behme Nov. 25, 2024, 4:55 p.m. UTC | #3
Hi Rob,

On 25.11.24 16:35, Rob Herring wrote:
> On Fri, Nov 22, 2024 at 2:13 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>>
>> From: "Rob Herring (Arm)" <robh@kernel.org>
>>
>> The device property API is a firmware agnostic API for reading
>> properties from firmware (DT/ACPI) devices nodes and swnodes.
>>
>> While the C API takes a pointer to a caller allocated variable/buffer,
>> the rust API is designed to return a value and can be used in struct
>> initialization. Rust generics are also utilized to support different
>> sizes of properties (e.g. u8, u16, u32).
>>
>> To build and run the Examples as `rustdoc` tests the kernel Kconfig
>> options `CONFIG_OF` and `CONFIG_OF_UNITTEST` need to be enabled
>> additionally. Besides the default `rustdoc` test options
>> `CONFIG_KUNIT` and `CONFIG_RUST_KERNEL_DOCTESTS`. This even works
>> on non-ARM architectures as a test device tree is built into the
>> kernel, then.
>>
>> The Integer trait is proposed by Alic Ryhl [1].
>>
>> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiXPZqKpWSSNdx-Ww-E9h2tOLcF3_8Y4C_JQ0eU8EMwFw@mail.gmail.com/ [1]
>> Co-developed-by: Dirk Behme <dirk.behme@de.bosch.com>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
>> ---
>>
>> This is an update of Rob's initial patch
> 
> I have my own updates on it based on the discussion. It's a bit
> different because I've reworked the C API to better mesh with Rust
> needs. 

Great, that sounds really good!

> I just haven't sent it out because I've been busy with other
> things and it's the merge window.

No problem and totally understood!

> You asked me if I was going to work on this and I did. If you want to
> do it, just say so. I'm always happy for less work.

Sorry! I have worked on the review comments and thought sharing them
might help. If this is not the case, then sorry again. Of course,
please share what you have. And ignore this one, then.

Thanks

Dirk
diff mbox series

Patch

diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
index fa39611071b32..a5369b9343b8a 100644
--- a/drivers/of/unittest-data/tests-platform.dtsi
+++ b/drivers/of/unittest-data/tests-platform.dtsi
@@ -33,6 +33,14 @@  dev@100 {
 					reg = <0x100>;
 				};
 			};
+
+			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/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 217c776615b95..65717cc20a23c 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -19,6 +19,7 @@ 
 #include <linux/pci.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/refcount.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 0c28b1e6b004c..2dea5ac2a6a73 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,6 +6,9 @@ 
 
 use crate::{
     bindings,
+    error::Result,
+    property::FwProperty,
+    str::CStr,
     types::{ARef, Opaque},
 };
 use core::{fmt, ptr};
@@ -189,6 +192,25 @@  unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
             )
         };
     }
+
+    /// Returns the firmware property `name` value.
+    pub fn property_read<T: FwProperty>(&self, name: &CStr, default: Option<T>) -> Result<T> {
+        T::read_property(self, name, default)
+    }
+
+    /// Returns the array length for the firmware property `name`.
+    pub fn property_count_elem<T: FwProperty>(&self, name: &CStr) -> Result<usize> {
+        T::count_elem(self, name)
+    }
+
+    /// Returns the index if `match_str` is found in the firmware property array `name`.
+    pub fn property_match_string<T: FwProperty>(
+        &self,
+        name: &CStr,
+        match_str: &CStr,
+    ) -> Result<usize> {
+        T::match_string(self, name, match_str)
+    }
 }
 
 // SAFETY: Instances of `Device` are always reference-counted.
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 9e8dcd6d7c014..474f2eadd6616 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -56,6 +56,7 @@ 
 pub mod platform;
 pub mod prelude;
 pub mod print;
+pub mod property;
 pub mod rbtree;
 pub mod revocable;
 pub mod sizes;
diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
new file mode 100644
index 0000000000000..776d23e4d5985
--- /dev/null
+++ b/rust/kernel/property.rs
@@ -0,0 +1,370 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Accessing (DeviceTree/ACPI) firmware properties.
+//!
+//! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
+
+use crate::{
+    bindings,
+    device::Device,
+    error::{to_result, Result},
+    prelude::*,
+    str::CStr,
+    transmute::{AsBytes, FromBytes},
+};
+
+use core::mem;
+use core::mem::MaybeUninit;
+
+/// A trait for integer types.
+///
+/// This trait limits [`FromBytes`] and [`AsBytes`] to integer types only. Excluding the
+/// array types. The integer types are `u8`, `u16`, `u32`, `u64`, `usize`, `i8`, `i16`,
+/// `i32`, `i64` and `isize`. Additionally, the size of these types is encoded in the
+/// `IntSize` enum.
+trait Integer: FromBytes + AsBytes + Copy {
+    /// The size of the integer type.
+    const SIZE: IntSize;
+}
+
+/// The sizes of the integer types. Either 8, 16, 32 or 64 bits.
+enum IntSize {
+    /// 8 bits
+    S8,
+    /// 16 bits
+    S16,
+    /// 32 bits
+    S32,
+    /// 64 bits
+    S64,
+}
+
+macro_rules! impl_int {
+    ($($typ:ty),* $(,)?) => {$(
+        impl Integer for $typ {
+            const SIZE: IntSize = match size_of::<Self>() {
+                1 => IntSize::S8,
+                2 => IntSize::S16,
+                4 => IntSize::S32,
+                8 => IntSize::S64,
+                _ => panic!("invalid size"),
+            };
+        }
+    )*};
+}
+
+impl_int! {
+    u8, u16, u32, u64, usize,
+    i8, i16, i32, i64, isize,
+}
+
+/// Reads an array of integers from the device firmware.
+fn read_array<T: Integer>(
+    device: &Device,
+    name: &CStr,
+    val: Option<&mut [MaybeUninit<T>]>,
+) -> Result<usize> {
+    let (ptr, len) = match val {
+        // The read array data case.
+        Some(val) => (val.as_mut_ptr(), val.len()),
+        // The read count case.
+        None => (core::ptr::null_mut(), 0_usize),
+    };
+    let ret = match T::SIZE {
+        // SAFETY: `device_property_read_u8_array` is called with a valid device pointer and name.
+        IntSize::S8 => unsafe {
+            bindings::device_property_read_u8_array(
+                device.as_raw(),
+                name.as_ptr() as *const i8,
+                ptr as *mut u8,
+                len,
+            )
+        },
+        // SAFETY: `device_property_read_u16_array` is called with a valid device pointer and name.
+        IntSize::S16 => unsafe {
+            bindings::device_property_read_u16_array(
+                device.as_raw(),
+                name.as_ptr() as *const i8,
+                ptr as *mut u16,
+                len,
+            )
+        },
+        // SAFETY: `device_property_read_u32_array` is called with a valid device pointer and name.
+        IntSize::S32 => unsafe {
+            bindings::device_property_read_u32_array(
+                device.as_raw(),
+                name.as_ptr() as *const i8,
+                ptr as *mut u32,
+                len,
+            )
+        },
+        // SAFETY: `device_property_read_u64_array` is called with a valid device pointer and name.
+        IntSize::S64 => unsafe {
+            bindings::device_property_read_u64_array(
+                device.as_raw(),
+                name.as_ptr() as *const i8,
+                ptr as *mut u64,
+                len,
+            )
+        },
+    };
+    to_result(ret)?;
+    Ok(ret.try_into()?)
+}
+
+/// A trait for reading (DeviceTree/ACPI) firmware properties.
+///
+/// This trait serves as an interface to the device property API
+/// which is a firmware agnostic API for reading properties from
+/// firmware (DeviceTree/ACPI) device nodes and swnodes.
+///
+/// While the C API takes a pointer to a caller allocated variable/buffer,
+/// this trait is designed to return a value and can be used in struct
+/// initialization.
+///
+/// There are mandatory and optional properties. If a property is mandatory
+/// it has to be there. If it is not found an error is printed and returned.
+/// If a property is optional and found the value is returned. If it is not
+/// found the default value is returned. If the default value is not provided,
+/// an error is printed and returned.
+///
+/// This logic is controlled via the `default` parameter of type `Option<>`.
+/// If `default` is `None` the property is assumed to be mandatory. If `default`
+/// is `Some(<default_val>)` the property is assumed to be optional and the
+/// `default_val` is returned in case the optional property is not found.
+///
+/// However, this doesn't apply to boolean properties. For boolean properties
+/// the property exists or not. If it exists, it returns true, otherwise false.
+/// So `Some(<default_val>)` does not make sense in this case and should be `None`.
+/// An error is printed and returned if this not the case.
+///
+/// As errors are printed in above cases users of this trait are supposed
+/// to *not* do any additional error printing. Of course, appropriate error handling
+/// (without printing) needs to be implemented.
+///
+/// *Note*: To build and run below examples as `rustdoc` tests the additional kernel
+/// Kconfig options `CONFIG_OF` and `CONFIG_OF_UNITTEST` need to be enabled. This
+/// even works on non-ARM architectures as a test device tree is built into the
+/// kernel, then.
+///
+/// # Examples
+///
+/// ```
+/// # mod property_example {
+/// #
+/// # use kernel::{c_str, of, platform, prelude::*};
+/// #
+/// # struct PropertyExample {
+/// #     pdev: platform::Device,
+/// # }
+/// #
+/// # kernel::of_device_table!(
+/// #     OF_TABLE,
+/// #     MODULE_OF_TABLE,
+/// #     <PropertyExample as platform::Driver>::IdInfo,
+/// #     [(of::DeviceId::new(c_str!("test,rust-device")), ())]
+/// # );
+/// #
+/// # impl platform::Driver for PropertyExample {
+/// # type IdInfo = ();
+/// # const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
+/// #
+/// # fn probe(
+/// #   pdev: &mut platform::Device,
+/// #   info: Option<&Self::IdInfo>
+/// # ) -> Result<Pin<KBox<Self>>> {
+/// # let dev = pdev.as_ref();
+/// #
+/// # dev_err!(dev, "The following two error messages are intended and correct from the tests:\n");
+/// #
+/// // Read an existing property as bool. This has two use cases:
+/// // a) Get the value of a boolean property (true == exist, false == don't exist).
+/// // b) Check if a property exists. Not limited to boolean properties.
+/// let prop = dev.property_read::<bool>(c_str!("test,u32-prop"), None);
+/// # //assert_eq!(prop, Ok(true)); // FIXME: How to build this?
+/// # dev_info!(dev, "prop: {:?}\n", prop); // Debug only, drop later
+/// if prop != Ok(true) {return Err(EINVAL);}
+///
+/// // Reading a non-existing property as bool should return false.
+/// let prop = dev.property_read::<bool>(c_str!("test,bool-prop"), None);
+/// # //assert_eq!(prop, Ok(false)); // FIXME: How to build this?
+/// # dev_info!(dev, "prop: {:?}\n", prop); // Debug only, drop later
+/// if prop != Ok(false) {return Err(EINVAL);}
+///
+/// // Invalid, returns with error. Don't use `Some(<bool>)` here (correct would be `None`).
+/// // Should print an error.
+/// let prop = dev.property_read::<bool>(c_str!("test,bool-prop"), Some(true));
+/// # //assert_eq!(prop, Err(EINVAL)); // FIXME: How to build this?
+/// # dev_info!(dev, "prop: {:?}\n", prop); // Debug only, drop later
+/// if prop != Err(EINVAL) {return Err(EINVAL);}
+///
+/// // 'property_read::<integer>' or 'property_read::<array>' can read either optional or
+/// // mandatory properties. If the property is mandatory and not found, it will print an
+/// // error message and return with error. If the property is optional and not found, it
+/// // will return with the given default value.
+/// //
+/// // Assume 'test,u32-optional' is an optional property which does not exist.
+/// let prop: u32 = dev.property_read(c_str!("test,u32-optional"), Some(0xdb))?;
+/// # //assert_eq!(prop, 0xdb); // FIXME: How to build this?
+/// # dev_info!(dev, "prop: {:#x}\n", prop); // Debug only, drop later
+/// if prop != 0xdb {return Err(EINVAL);}
+///
+/// // Assume 'test,u32-mandatory' is a mandatory property which does not exist.
+/// // Should print an error.
+/// let prop = dev.property_read::<u32>(c_str!("test,u32-mandatory"), None);
+/// # //assert_eq!(prop, Err(EINVAL)); // FIXME: How to build this?
+/// # dev_info!(dev, "prop: {:?}\n", prop); // Debug only, drop later
+/// if prop != Err(EINVAL) {return Err(EINVAL);}
+///
+/// // Some examples for mandatory property which does exist.
+/// // First in Turbofish syntax.
+/// let prop = dev.property_read::<u32>(c_str!("test,u32-prop"), None)?;
+/// # //assert_eq!(prop, Err(EINVAL)); // FIXME: How to build this?
+/// # dev_info!(dev, "prop: {:#x}\n", prop); // Debug only, drop later
+/// if prop != 0xdeadbeef {return Err(EINVAL);}
+///
+/// // Second with type annotation.
+/// let prop: u32 = dev.property_read(c_str!("test,u32-prop"), None)?;
+/// # //assert_eq!(prop, Err(EINVAL)); // FIXME: How to build this?
+/// # dev_info!(dev, "prop: {:#x}\n", prop); // Debug only, drop later
+/// if prop != 0xdeadbeef {return Err(EINVAL);}
+///
+/// // Reading a mandatory array of integers.
+/// let prop: [i16; 4] = dev.property_read(c_str!("test,i16-array"), None)?;
+/// # //assert_eq!(prop, [1, 2, -3, -4]); // FIXME: How to build this?
+/// # dev_info!(dev, "prop: {:?}\n", prop); // Debug only, drop later
+/// if prop != [1, 2, -3, -4] {return Err(EINVAL);}
+///
+/// // Getting the length of the 16 bits array.
+/// let length = dev.property_count_elem::<i16>(c_str!("test,i16-array"))?;
+/// # //assert_eq!(length, 4); // FIXME: How to build this?
+/// # dev_info!(dev, "length: {}\n", length); // Debug only, drop later
+/// if length != 4 {return Err(EINVAL);}
+///
+/// // Match a string.
+/// let idx = dev.property_match_string::<usize>(c_str!("compatible"), c_str!("test,rust-device"));
+/// # //assert_eq!(idx, Ok(0)); // FIXME: How to build this?
+/// # dev_info!(dev, "idx: {:?}\n", idx); // Debug only, drop later
+/// if idx != Ok(0) {return Err(EINVAL);}
+/// #
+/// # let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
+/// #
+/// # Ok(drvdata.into())
+/// # }
+/// # }
+/// #
+/// # kernel::module_platform_driver! {
+/// #     type: PropertyExample,
+/// #     name: "rust_property_example",
+/// #     author: "Rob Herring and Dirk Behme",
+/// #     description: "Rust Property Example driver",
+/// #     license: "GPL v2",
+/// # }
+/// # }
+/// ```
+/// The above examples intentionally don't print any error messages (e.g. with `dev_err!()`).
+/// The called abstractions already print error messages if needed what is considered to be
+/// sufficient. The goal is to be less verbose regarding error messages.
+pub trait FwProperty: Sized {
+    /// Reads a property from the device.
+    fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self>;
+
+    /// Gets the properties element count.
+    fn count_elem(device: &Device, name: &CStr) -> Result<usize>;
+
+    /// Returns if a firmware string property `name` has match for `match_str`.
+    fn match_string(device: &Device, name: &CStr, match_str: &CStr) -> Result<usize> {
+        // SAFETY: `device_property_match_string` is called with a valid device pointer and name.
+        let ret = unsafe {
+            bindings::device_property_match_string(
+                device.as_raw(),
+                name.as_ptr() as *const i8,
+                match_str.as_ptr() as *const i8,
+            )
+        };
+        to_result(ret)?;
+        Ok(ret as usize)
+    }
+}
+
+impl FwProperty for bool {
+    fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self> {
+        if default.is_some() {
+            dev_err!(
+                device,
+                "Error: Default value should be 'None' for reading boolean property"
+            );
+            return Err(EINVAL);
+        }
+        // SAFETY: `device_property_present` is called with a valid device pointer and name.
+        Ok(unsafe {
+            bindings::device_property_present(device.as_raw(), name.as_ptr() as *const i8)
+        })
+    }
+
+    /// Gets the properties element count.
+    // FIXME: Could this be made to be a build time error?
+    fn count_elem(device: &Device, _name: &CStr) -> Result<usize> {
+        dev_err!(
+            device,
+            "Error: Boolean type does not implement element count"
+        );
+        Err(EINVAL)
+    }
+}
+
+impl<T: Integer + Copy> FwProperty for T {
+    fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self> {
+        let mut val: [MaybeUninit<T>; 1] = [const { MaybeUninit::uninit() }; 1];
+        match read_array(device, name, Some(&mut val)) {
+            // SAFETY: `read_array` returns with valid data
+            Ok(_) => Ok(unsafe { mem::transmute_copy(&val[0]) }),
+            Err(e) => match default {
+                Some(default) => Ok(default),
+                None => {
+                    dev_err!(
+                        device,
+                        "Error: Mandatory property '{}' does not exist ({:?})",
+                        name,
+                        e
+                    );
+                    Err(e)
+                }
+            },
+        }
+    }
+
+    /// Gets the properties element count.
+    fn count_elem(device: &Device, name: &CStr) -> Result<usize> {
+        read_array::<T>(device, name, None)
+    }
+}
+
+impl<T: Integer, const N: usize> FwProperty for [T; N] {
+    fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self> {
+        let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
+        match read_array(device, name, Some(&mut val)) {
+            // SAFETY: `read_array` returns with valid data
+            Ok(_) => Ok(unsafe { mem::transmute_copy(&val) }),
+            Err(e) => match default {
+                Some(default) => Ok(default),
+                None => {
+                    dev_err!(
+                        device,
+                        "Error: Mandatory property '{}' does not exist {:?}",
+                        name,
+                        e
+                    );
+                    Err(e)
+                }
+            },
+        }
+    }
+
+    /// Gets the properties element count.
+    // FIXME: Could this be made to be a build time error?
+    fn count_elem(device: &Device, _name: &CStr) -> Result<usize> {
+        dev_err!(device, "Error: Array type does not implement element count");
+        Err(EINVAL)
+    }
+}