[PATCH v7 0/9] More Rust bindings for device property reads

Remo Senekowitsch posted 9 patches 6 months, 2 weeks ago
There is a newer version of this series
MAINTAINERS                                  |   1 +
drivers/of/unittest-data/tests-platform.dtsi |   3 +
rust/helpers/helpers.c                       |   1 +
rust/helpers/property.c                      |   8 +
rust/kernel/device.rs                        |  17 +
rust/kernel/device/property.rs               | 590 +++++++++++++++++++
samples/rust/rust_driver_platform.rs         |  60 +-
7 files changed, 679 insertions(+), 1 deletion(-)
create mode 100644 rust/helpers/property.c
create mode 100644 rust/kernel/device/property.rs
[PATCH v7 0/9] More Rust bindings for device property reads
Posted by Remo Senekowitsch 6 months, 2 weeks ago
changes in v7:
* Fix a typo in a commit message.
* Fix bug in `FwNode::display_path`. I took a slightly different
  approach than the one suggested, using `Either` to handle the
  owned and borrowed case. That also removes the conditional
  `fwnode_handle_put` at the end.
* Move `FwNode::from_raw` to the commit which first introduces the
  `FwNode` abstraction. It is needed in an earlier commit than before
  and I think it fits better there.

Best regards,
Remo

Remo Senekowitsch (9):
  rust: device: Create FwNode abstraction for accessing device
    properties
  rust: device: Enable accessing the FwNode of a Device
  rust: device: Add property_present() to FwNode
  rust: device: Enable printing fwnode name and path
  rust: device: Introduce PropertyGuard
  rust: device: Implement accessors for firmware properties
  rust: device: Add child accessor and iterator
  rust: device: Add property_get_reference_args
  samples: rust: platform: Add property read examples

 MAINTAINERS                                  |   1 +
 drivers/of/unittest-data/tests-platform.dtsi |   3 +
 rust/helpers/helpers.c                       |   1 +
 rust/helpers/property.c                      |   8 +
 rust/kernel/device.rs                        |  17 +
 rust/kernel/device/property.rs               | 590 +++++++++++++++++++
 samples/rust/rust_driver_platform.rs         |  60 +-
 7 files changed, 679 insertions(+), 1 deletion(-)
 create mode 100644 rust/helpers/property.c
 create mode 100644 rust/kernel/device/property.rs

-- 
2.49.0
Re: [PATCH v7 0/9] More Rust bindings for device property reads
Posted by Dirk Behme 6 months, 2 weeks ago
Hi Remo,

On 30/05/2025 21:28, Remo Senekowitsch wrote:
> changes in v7:
> * Fix a typo in a commit message.
> * Fix bug in `FwNode::display_path`. I took a slightly different
>   approach than the one suggested, using `Either` to handle the
>   owned and borrowed case. That also removes the conditional
>   `fwnode_handle_put` at the end.
> * Move `FwNode::from_raw` to the commit which first introduces the
>   `FwNode` abstraction. It is needed in an earlier commit than before
>   and I think it fits better there.
> 
> Best regards,
> Remo
> 
> Remo Senekowitsch (9):
>   rust: device: Create FwNode abstraction for accessing device
>     properties
>   rust: device: Enable accessing the FwNode of a Device
>   rust: device: Add property_present() to FwNode
>   rust: device: Enable printing fwnode name and path
>   rust: device: Introduce PropertyGuard
>   rust: device: Implement accessors for firmware properties
>   rust: device: Add child accessor and iterator
>   rust: device: Add property_get_reference_args
>   samples: rust: platform: Add property read examples
> 
>  MAINTAINERS                                  |   1 +
>  drivers/of/unittest-data/tests-platform.dtsi |   3 +
>  rust/helpers/helpers.c                       |   1 +
>  rust/helpers/property.c                      |   8 +
>  rust/kernel/device.rs                        |  17 +
>  rust/kernel/device/property.rs               | 590 +++++++++++++++++++
>  samples/rust/rust_driver_platform.rs         |  60 +-
>  7 files changed, 679 insertions(+), 1 deletion(-)
>  create mode 100644 rust/helpers/property.c
>  create mode 100644 rust/kernel/device/property.rs
I have tested this series with x86/Qemu running the sample. And the same
on real custom ARM64 hardware. Additionally, I ported a custom driver on
ARM64 to use this (well, it just uses the trivial
property_read().required_by()). So if it helps

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>

for the whole series.

Btw, many thanks for working on this!

Dirk
Re: [PATCH v7 0/9] More Rust bindings for device property reads
Posted by Danilo Krummrich 6 months, 2 weeks ago
On Fri, May 30, 2025 at 09:28:47PM +0200, Remo Senekowitsch wrote:
> changes in v7:
> * Fix a typo in a commit message.
> * Fix bug in `FwNode::display_path`. I took a slightly different
>   approach than the one suggested, using `Either` to handle the
>   owned and borrowed case. That also removes the conditional
>   `fwnode_handle_put` at the end.

That's a good idea, but also a bit unfortunate; there are efforts to remove
Either [1] in favor of using - more descriptive - custom enum types.

Can you please replace this with e.g. an enum Node with a Borrowed and Owned
variant?

[1] https://lore.kernel.org/lkml/20250519124304.79237-1-lossin@kernel.org/
Re: [PATCH v7 0/9] More Rust bindings for device property reads
Posted by Remo Senekowitsch 6 months, 2 weeks ago
On Fri May 30, 2025 at 9:56 PM CEST, Danilo Krummrich wrote:
> On Fri, May 30, 2025 at 09:28:47PM +0200, Remo Senekowitsch wrote:
>> changes in v7:
>> * Fix a typo in a commit message.
>> * Fix bug in `FwNode::display_path`. I took a slightly different
>>   approach than the one suggested, using `Either` to handle the
>>   owned and borrowed case. That also removes the conditional
>>   `fwnode_handle_put` at the end.
>
> That's a good idea, but also a bit unfortunate; there are efforts to remove
> Either [1] in favor of using - more descriptive - custom enum types.
>
> Can you please replace this with e.g. an enum Node with a Borrowed and Owned
> variant?
>
> [1] https://lore.kernel.org/lkml/20250519124304.79237-1-lossin@kernel.org/

Sure, that seems reasonable.

Btw. what's the normal waiting time before posting a new version of a
patch series? The requested changes have been getting fewer and I could
crank these out much faster, but my gut feeling tells me not to spam the
list too much. Or is that wrong and people can deal with quick updates
just fine?

Best regards,
Remo
Re: [PATCH v7 0/9] More Rust bindings for device property reads
Posted by Danilo Krummrich 6 months, 2 weeks ago
On Fri, May 30, 2025 at 11:45:38PM +0200, Remo Senekowitsch wrote:
> On Fri May 30, 2025 at 9:56 PM CEST, Danilo Krummrich wrote:
> > On Fri, May 30, 2025 at 09:28:47PM +0200, Remo Senekowitsch wrote:
> >> changes in v7:
> >> * Fix a typo in a commit message.
> >> * Fix bug in `FwNode::display_path`. I took a slightly different
> >>   approach than the one suggested, using `Either` to handle the
> >>   owned and borrowed case. That also removes the conditional
> >>   `fwnode_handle_put` at the end.
> >
> > That's a good idea, but also a bit unfortunate; there are efforts to remove
> > Either [1] in favor of using - more descriptive - custom enum types.
> >
> > Can you please replace this with e.g. an enum Node with a Borrowed and Owned
> > variant?
> >
> > [1] https://lore.kernel.org/lkml/20250519124304.79237-1-lossin@kernel.org/
> 
> Sure, that seems reasonable.
> 
> Btw. what's the normal waiting time before posting a new version of a
> patch series? The requested changes have been getting fewer and I could
> crank these out much faster, but my gut feeling tells me not to spam the
> list too much. Or is that wrong and people can deal with quick updates
> just fine?

I think the pace was appropriate. For the current state, I don't expect much
more feedback, so it'd be fine to send an update for the enum change rather
quicky.

However, we're anyways in the merge window currently, so I'd recomment to leave
the patch series as is and send a v8 once the merge window closes -- I'll pick
it up then unless there's some further feedback.
Re: [PATCH v7 0/9] More Rust bindings for device property reads
Posted by Rob Herring 6 months, 1 week ago
On Fri, May 30, 2025 at 11:50:39PM +0200, Danilo Krummrich wrote:
> On Fri, May 30, 2025 at 11:45:38PM +0200, Remo Senekowitsch wrote:
> > On Fri May 30, 2025 at 9:56 PM CEST, Danilo Krummrich wrote:
> > > On Fri, May 30, 2025 at 09:28:47PM +0200, Remo Senekowitsch wrote:
> > >> changes in v7:
> > >> * Fix a typo in a commit message.
> > >> * Fix bug in `FwNode::display_path`. I took a slightly different
> > >>   approach than the one suggested, using `Either` to handle the
> > >>   owned and borrowed case. That also removes the conditional
> > >>   `fwnode_handle_put` at the end.
> > >
> > > That's a good idea, but also a bit unfortunate; there are efforts to remove
> > > Either [1] in favor of using - more descriptive - custom enum types.
> > >
> > > Can you please replace this with e.g. an enum Node with a Borrowed and Owned
> > > variant?
> > >
> > > [1] https://lore.kernel.org/lkml/20250519124304.79237-1-lossin@kernel.org/
> > 
> > Sure, that seems reasonable.
> > 
> > Btw. what's the normal waiting time before posting a new version of a
> > patch series? The requested changes have been getting fewer and I could
> > crank these out much faster, but my gut feeling tells me not to spam the
> > list too much. Or is that wrong and people can deal with quick updates
> > just fine?
> 
> I think the pace was appropriate. For the current state, I don't expect much
> more feedback, so it'd be fine to send an update for the enum change rather
> quicky.

Yes. General rules are no more frequent than 24 hours, but generally 1-2 
weeks. It also is a function of amount of review. No review, wait. If 
there's enough review that the current version isn't going to get more 
review, then go ahead and send another version. When things are close to 
merging and the changes are small, you can pick up the pace a little 
(outside of the merge window). 

> 
> However, we're anyways in the merge window currently, so I'd recomment to leave
> the patch series as is and send a v8 once the merge window closes -- I'll pick
> it up then unless there's some further feedback.

You'll need to fixup the user drivers/cpufreq/rcpufreq_dt.rs that 
landed in the merge window.

Overall, this all looks great to me. Thanks for continuing to push this 
forward.

Rob