This abstraction is a way to force users to specify whether a property
is supposed to be required or not. This allows us to move error
logging of missing required properties into core, preventing a lot of
boilerplate in drivers.

It will be used by upcoming methods for reading device properties.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index 6ccc7947f9c31..59c61e2493831 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
         unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
     }
 }
+
+/// A helper for reading device properties.
+///
+/// Use [`Self::required_by`] if a missing property is considered a bug and
+/// [`Self::optional`] otherwise.
+///
+/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
+pub struct PropertyGuard<'fwnode, 'name, T> {
+    /// The result of reading the property.
+    inner: Result<T>,
+    /// The fwnode of the property, used for logging in the "required" case.
+    fwnode: &'fwnode FwNode,
+    /// The name of the property, used for logging in the "required" case.
+    name: &'name CStr,
+}
+
+impl<T> PropertyGuard<'_, '_, T> {
+    /// Access the property, indicating it is required.
+    ///
+    /// If the property is not present, the error is automatically logged. If a
+    /// missing property is not an error, use [`Self::optional`] instead. The
+    /// device is required to associate the log with it.
+    pub fn required_by(self, dev: &super::Device) -> Result<T> {
+        if self.inner.is_err() {
+            dev_err!(
+                dev,
+                "{}: property '{}' is missing\n",
+                self.fwnode.display_path(),
+                self.name
+            );
+        }
+        self.inner
+    }
+
+    /// Access the property, indicating it is optional.
+    ///
+    /// In contrast to [`Self::required_by`], no error message is logged if
+    /// the property is not present.
+    pub fn optional(self) -> Option<T> {
+        self.inner.ok()
+    }
+
+    /// Access the property or the specified default value.
+    ///
+    /// Do not pass a sentinel value as default to detect a missing property.
+    /// Use [`Self::required_by`] or [`Self::optional`] instead.
+    pub fn or(self, default: T) -> T {
+        self.inner.unwrap_or(default)
+    }
+}
+
+impl<T: Default> PropertyGuard<'_, '_, T> {
+    /// Access the property or a default value.
+    ///
+    /// Use [`Self::or`] to specify a custom default value.
+    pub fn or_default(self) -> T {
+        self.inner.unwrap_or_default()
+    }
+}
-- 
2.49.0

On Sun, May 04, 2025 at 07:31:50PM +0200, Remo Senekowitsch wrote:
> This abstraction is a way to force users to specify whether a property
> is supposed to be required or not. This allows us to move error
> logging of missing required properties into core, preventing a lot of
> boilerplate in drivers.
> 
> It will be used by upcoming methods for reading device properties.
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 6ccc7947f9c31..59c61e2493831 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>      }
>  }
> +
> +/// A helper for reading device properties.
> +///
> +/// Use [`Self::required_by`] if a missing property is considered a bug and
> +/// [`Self::optional`] otherwise.
> +///
> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> +pub struct PropertyGuard<'fwnode, 'name, T> {
> +    /// The result of reading the property.
> +    inner: Result<T>,
> +    /// The fwnode of the property, used for logging in the "required" case.
> +    fwnode: &'fwnode FwNode,
> +    /// The name of the property, used for logging in the "required" case.
> +    name: &'name CStr,
> +}
> +
> +impl<T> PropertyGuard<'_, '_, T> {
> +    /// Access the property, indicating it is required.
> +    ///
> +    /// If the property is not present, the error is automatically logged. If a
> +    /// missing property is not an error, use [`Self::optional`] instead. The
> +    /// device is required to associate the log with it.
> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
> +        if self.inner.is_err() {
> +            dev_err!(
> +                dev,
> +                "{}: property '{}' is missing\n",
> +                self.fwnode.display_path(),

Is it possible to do "{self.fwnode}: property ..."?

Then if we want to modify the default, we could do something like 
"{self.fwnode:pn}"? It doesn't look like that's something which would 
work OOTB and is not something we need to solve now. So just the default 
formatter is good for now.

Rob

On 04/05/2025 19:31, Remo Senekowitsch wrote:
> This abstraction is a way to force users to specify whether a property
> is supposed to be required or not. This allows us to move error
> logging of missing required properties into core, preventing a lot of
> boilerplate in drivers.
> 
> It will be used by upcoming methods for reading device properties.
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 6ccc7947f9c31..59c61e2493831 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>      }
>  }
> +
> +/// A helper for reading device properties.
> +///
> +/// Use [`Self::required_by`] if a missing property is considered a bug and
> +/// [`Self::optional`] otherwise.
> +///
> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> +pub struct PropertyGuard<'fwnode, 'name, T> {
> +    /// The result of reading the property.
> +    inner: Result<T>,
> +    /// The fwnode of the property, used for logging in the "required" case.
> +    fwnode: &'fwnode FwNode,
> +    /// The name of the property, used for logging in the "required" case.
> +    name: &'name CStr,
> +}
> +
> +impl<T> PropertyGuard<'_, '_, T> {
> +    /// Access the property, indicating it is required.
> +    ///
> +    /// If the property is not present, the error is automatically logged. If a
> +    /// missing property is not an error, use [`Self::optional`] instead. The
> +    /// device is required to associate the log with it.
> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
> +        if self.inner.is_err() {
> +            dev_err!(
> +                dev,
> +                "{}: property '{}' is missing\n",
> +                self.fwnode.display_path(),
> +                self.name
> +            );
> +        }
> +        self.inner
> +    }

Thinking about the .required_by(dev) I wonder if there will be cases
where we do *not* have a device? I.e. where we really have a fwnode,
only. And therefore can't pass a device. If we have such cases do we
need to be able to pass e.g. Option(dev) and switch back to pr_err() in
case of None?

From the beginning of our discussion I think to remember that the C API
has both the fwnode_property_*() and device_property_*() because there
are use cases for the fwnode_property_*() API where is no device?

Thanks

Dirk

On Mon May 5, 2025 at 7:14 AM CEST, Dirk Behme wrote:
> On 04/05/2025 19:31, Remo Senekowitsch wrote:
>> This abstraction is a way to force users to specify whether a property
>> is supposed to be required or not. This allows us to move error
>> logging of missing required properties into core, preventing a lot of
>> boilerplate in drivers.
>> 
>> It will be used by upcoming methods for reading device properties.
>> 
>> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>> ---
>>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>> 
>> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
>> index 6ccc7947f9c31..59c61e2493831 100644
>> --- a/rust/kernel/device/property.rs
>> +++ b/rust/kernel/device/property.rs
>> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>>      }
>>  }
>> +
>> +/// A helper for reading device properties.
>> +///
>> +/// Use [`Self::required_by`] if a missing property is considered a bug and
>> +/// [`Self::optional`] otherwise.
>> +///
>> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
>> +pub struct PropertyGuard<'fwnode, 'name, T> {
>> +    /// The result of reading the property.
>> +    inner: Result<T>,
>> +    /// The fwnode of the property, used for logging in the "required" case.
>> +    fwnode: &'fwnode FwNode,
>> +    /// The name of the property, used for logging in the "required" case.
>> +    name: &'name CStr,
>> +}
>> +
>> +impl<T> PropertyGuard<'_, '_, T> {
>> +    /// Access the property, indicating it is required.
>> +    ///
>> +    /// If the property is not present, the error is automatically logged. If a
>> +    /// missing property is not an error, use [`Self::optional`] instead. The
>> +    /// device is required to associate the log with it.
>> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
>> +        if self.inner.is_err() {
>> +            dev_err!(
>> +                dev,
>> +                "{}: property '{}' is missing\n",
>> +                self.fwnode.display_path(),
>> +                self.name
>> +            );
>> +        }
>> +        self.inner
>> +    }
>
> Thinking about the .required_by(dev) I wonder if there will be cases
> where we do *not* have a device? I.e. where we really have a fwnode,
> only. And therefore can't pass a device. If we have such cases do we
> need to be able to pass e.g. Option(dev) and switch back to pr_err() in
> case of None?

In that case, bringing back the previous .required() method seems
reasonable to me. But only if we definitely know such cases exist.

> From the beginning of our discussion I think to remember that the C API
> has both the fwnode_property_*() and device_property_*() because there
> are use cases for the fwnode_property_*() API where is no device?

I'm not sure what you're referring to, the closest thing I can think of
is this comment by Rob [1] where he mentions the device_property_*()
functions only exist in C for a minimal convenience gain and we may not
want to keep that in Rust.

[1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697

On Mon, May 5, 2025 at 8:02 AM Remo Senekowitsch <remo@buenzli.dev> wrote:
>
> On Mon May 5, 2025 at 7:14 AM CEST, Dirk Behme wrote:
> > On 04/05/2025 19:31, Remo Senekowitsch wrote:
> >> This abstraction is a way to force users to specify whether a property
> >> is supposed to be required or not. This allows us to move error
> >> logging of missing required properties into core, preventing a lot of
> >> boilerplate in drivers.
> >>
> >> It will be used by upcoming methods for reading device properties.
> >>
> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> >> ---
> >>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 59 insertions(+)
> >>
> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> >> index 6ccc7947f9c31..59c61e2493831 100644
> >> --- a/rust/kernel/device/property.rs
> >> +++ b/rust/kernel/device/property.rs
> >> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> >>      }
> >>  }
> >> +
> >> +/// A helper for reading device properties.
> >> +///
> >> +/// Use [`Self::required_by`] if a missing property is considered a bug and
> >> +/// [`Self::optional`] otherwise.
> >> +///
> >> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> >> +pub struct PropertyGuard<'fwnode, 'name, T> {
> >> +    /// The result of reading the property.
> >> +    inner: Result<T>,
> >> +    /// The fwnode of the property, used for logging in the "required" case.
> >> +    fwnode: &'fwnode FwNode,
> >> +    /// The name of the property, used for logging in the "required" case.
> >> +    name: &'name CStr,
> >> +}
> >> +
> >> +impl<T> PropertyGuard<'_, '_, T> {
> >> +    /// Access the property, indicating it is required.
> >> +    ///
> >> +    /// If the property is not present, the error is automatically logged. If a
> >> +    /// missing property is not an error, use [`Self::optional`] instead. The
> >> +    /// device is required to associate the log with it.
> >> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
> >> +        if self.inner.is_err() {
> >> +            dev_err!(
> >> +                dev,
> >> +                "{}: property '{}' is missing\n",
> >> +                self.fwnode.display_path(),
> >> +                self.name
> >> +            );
> >> +        }
> >> +        self.inner
> >> +    }
> >
> > Thinking about the .required_by(dev) I wonder if there will be cases
> > where we do *not* have a device? I.e. where we really have a fwnode,
> > only. And therefore can't pass a device. If we have such cases do we
> > need to be able to pass e.g. Option(dev) and switch back to pr_err() in
> > case of None?
>
> In that case, bringing back the previous .required() method seems
> reasonable to me. But only if we definitely know such cases exist.

They definitely exist. Any property in a child node of the device's
node when the child itself is not another device for example.

> > From the beginning of our discussion I think to remember that the C API
> > has both the fwnode_property_*() and device_property_*() because there
> > are use cases for the fwnode_property_*() API where is no device?

Correct.

> I'm not sure what you're referring to, the closest thing I can think of
> is this comment by Rob [1] where he mentions the device_property_*()
> functions only exist in C for a minimal convenience gain and we may not
> want to keep that in Rust.

The point there was if there's not the same convenience with Rust,
then we shouldn't keep the API.

I think this came up previously, but I don't think it matters whether
we print the device name or fwnode path. If you have either one, you
can figure out both the driver and node. Arguably the fwnode path is
more useful because that's likely where the error is.

Rob

On Mon May 5, 2025 at 5:37 PM CEST, Rob Herring wrote:
> On Mon, May 5, 2025 at 8:02 AM Remo Senekowitsch <remo@buenzli.dev> wrote:
>>
>> On Mon May 5, 2025 at 7:14 AM CEST, Dirk Behme wrote:
>> > On 04/05/2025 19:31, Remo Senekowitsch wrote:
>> >> This abstraction is a way to force users to specify whether a property
>> >> is supposed to be required or not. This allows us to move error
>> >> logging of missing required properties into core, preventing a lot of
>> >> boilerplate in drivers.
>> >>
>> >> It will be used by upcoming methods for reading device properties.
>> >>
>> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>> >> ---
>> >>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 59 insertions(+)
>> >>
>> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
>> >> index 6ccc7947f9c31..59c61e2493831 100644
>> >> --- a/rust/kernel/device/property.rs
>> >> +++ b/rust/kernel/device/property.rs
>> >> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>> >>      }
>> >>  }
>> >> +
>> >> +/// A helper for reading device properties.
>> >> +///
>> >> +/// Use [`Self::required_by`] if a missing property is considered a bug and
>> >> +/// [`Self::optional`] otherwise.
>> >> +///
>> >> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
>> >> +pub struct PropertyGuard<'fwnode, 'name, T> {
>> >> +    /// The result of reading the property.
>> >> +    inner: Result<T>,
>> >> +    /// The fwnode of the property, used for logging in the "required" case.
>> >> +    fwnode: &'fwnode FwNode,
>> >> +    /// The name of the property, used for logging in the "required" case.
>> >> +    name: &'name CStr,
>> >> +}
>> >> +
>> >> +impl<T> PropertyGuard<'_, '_, T> {
>> >> +    /// Access the property, indicating it is required.
>> >> +    ///
>> >> +    /// If the property is not present, the error is automatically logged. If a
>> >> +    /// missing property is not an error, use [`Self::optional`] instead. The
>> >> +    /// device is required to associate the log with it.
>> >> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
>> >> +        if self.inner.is_err() {
>> >> +            dev_err!(
>> >> +                dev,
>> >> +                "{}: property '{}' is missing\n",
>> >> +                self.fwnode.display_path(),
>> >> +                self.name
>> >> +            );
>> >> +        }
>> >> +        self.inner
>> >> +    }
>> >
>> > Thinking about the .required_by(dev) I wonder if there will be cases
>> > where we do *not* have a device? I.e. where we really have a fwnode,
>> > only. And therefore can't pass a device. If we have such cases do we
>> > need to be able to pass e.g. Option(dev) and switch back to pr_err() in
>> > case of None?
>>
>> In that case, bringing back the previous .required() method seems
>> reasonable to me. But only if we definitely know such cases exist.
>
> They definitely exist. Any property in a child node of the device's
> node when the child itself is not another device for example.

I don't think that counts, because you do have a device in that
situation. The log should be assicated with that. So callers are
responsible to propagate a reference to the device to wherever the call
to .required_by(dev) is happening.

>> > From the beginning of our discussion I think to remember that the C API
>> > has both the fwnode_property_*() and device_property_*() because there
>> > are use cases for the fwnode_property_*() API where is no device?
>
> Correct.
>
>> I'm not sure what you're referring to, the closest thing I can think of
>> is this comment by Rob [1] where he mentions the device_property_*()
>> functions only exist in C for a minimal convenience gain and we may not
>> want to keep that in Rust.
>
> The point there was if there's not the same convenience with Rust,
> then we shouldn't keep the API.
>
> I think this came up previously, but I don't think it matters whether
> we print the device name or fwnode path. If you have either one, you
> can figure out both the driver and node. Arguably the fwnode path is
> more useful because that's likely where the error is.
>
> Rob

On Mon, May 05, 2025 at 05:53:33PM +0200, Remo Senekowitsch wrote:
> On Mon May 5, 2025 at 5:37 PM CEST, Rob Herring wrote:
> > On Mon, May 5, 2025 at 8:02 AM Remo Senekowitsch <remo@buenzli.dev> wrote:
> >>
> >> On Mon May 5, 2025 at 7:14 AM CEST, Dirk Behme wrote:
> >> > On 04/05/2025 19:31, Remo Senekowitsch wrote:
> >> >> This abstraction is a way to force users to specify whether a property
> >> >> is supposed to be required or not. This allows us to move error
> >> >> logging of missing required properties into core, preventing a lot of
> >> >> boilerplate in drivers.
> >> >>
> >> >> It will be used by upcoming methods for reading device properties.
> >> >>
> >> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> >> >> ---
> >> >>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
> >> >>  1 file changed, 59 insertions(+)
> >> >>
> >> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> >> >> index 6ccc7947f9c31..59c61e2493831 100644
> >> >> --- a/rust/kernel/device/property.rs
> >> >> +++ b/rust/kernel/device/property.rs
> >> >> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> >> >>      }
> >> >>  }
> >> >> +
> >> >> +/// A helper for reading device properties.
> >> >> +///
> >> >> +/// Use [`Self::required_by`] if a missing property is considered a bug and
> >> >> +/// [`Self::optional`] otherwise.
> >> >> +///
> >> >> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> >> >> +pub struct PropertyGuard<'fwnode, 'name, T> {
> >> >> +    /// The result of reading the property.
> >> >> +    inner: Result<T>,
> >> >> +    /// The fwnode of the property, used for logging in the "required" case.
> >> >> +    fwnode: &'fwnode FwNode,
> >> >> +    /// The name of the property, used for logging in the "required" case.
> >> >> +    name: &'name CStr,
> >> >> +}
> >> >> +
> >> >> +impl<T> PropertyGuard<'_, '_, T> {
> >> >> +    /// Access the property, indicating it is required.
> >> >> +    ///
> >> >> +    /// If the property is not present, the error is automatically logged. If a
> >> >> +    /// missing property is not an error, use [`Self::optional`] instead. The
> >> >> +    /// device is required to associate the log with it.
> >> >> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
> >> >> +        if self.inner.is_err() {
> >> >> +            dev_err!(
> >> >> +                dev,
> >> >> +                "{}: property '{}' is missing\n",
> >> >> +                self.fwnode.display_path(),
> >> >> +                self.name
> >> >> +            );
> >> >> +        }
> >> >> +        self.inner
> >> >> +    }
> >> >
> >> > Thinking about the .required_by(dev) I wonder if there will be cases
> >> > where we do *not* have a device? I.e. where we really have a fwnode,
> >> > only. And therefore can't pass a device. If we have such cases do we
> >> > need to be able to pass e.g. Option(dev) and switch back to pr_err() in
> >> > case of None?
> >>
> >> In that case, bringing back the previous .required() method seems
> >> reasonable to me. But only if we definitely know such cases exist.
> >
> > They definitely exist. Any property in a child node of the device's
> > node when the child itself is not another device for example.
> 
> I don't think that counts, because you do have a device in that
> situation. The log should be assicated with that. So callers are
> responsible to propagate a reference to the device to wherever the call
> to .required_by(dev) is happening.

Ah, right. So it would just be cases that aren't a driver at all. That's 
limited to the OF_DECLARE cases. I agree we can worry about those later.

Rob

On Mon, May 05, 2025 at 05:53:33PM +0200, Remo Senekowitsch wrote:
> On Mon May 5, 2025 at 5:37 PM CEST, Rob Herring wrote:
> > They definitely exist. Any property in a child node of the device's
> > node when the child itself is not another device for example.
> 
> I don't think that counts, because you do have a device in that
> situation. The log should be assicated with that. So callers are
> responsible to propagate a reference to the device to wherever the call
> to .required_by(dev) is happening.

Exactly, let's keep things as they are for now. We can still add further methods
when we run into a real use-case. Currently, I can't see one coming in any time
soon.

- Danilo