Add abstractions for devm_of_platform_populate and
devm_of_platform_depopulate.

Can only be used with a bound device.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
Allow calling platform_populate/depopulate from Rust code.

To see how the bindings look in usage, see my working tree [0] for a
connector driver  I am working on.

[0]: https://github.com/Ayush1325/linux/commits/b4/beagle-cape/
---
Changes in v2:
- Rename to devm_of_platform_populate/depopulate.
- Add SAFETY comments.
- Implement only for bound device
- Import to_result
- Link to v1: https://lore.kernel.org/r/20250428-rust-of-populate-v1-1-1d33777427c4@beagleboard.org
---
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/device.rs           | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8a2add69e5d66d1c2ebed9d2c950380e61c48842..51ec0754960377e5fc6bc0703487bf2086eff0e6 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -25,6 +25,7 @@
 #include <linux/mdio.h>
 #include <linux/miscdevice.h>
 #include <linux/of_device.h>
+#include <linux/of_platform.h>
 #include <linux/pci.h>
 #include <linux/phy.h>
 #include <linux/pid_namespace.h>
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 40c1f549b0bae9fd9aa3f41539ccb69896c2560d..dc52abeb114792fcecce469c11e336c23c4b1c00 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,6 +6,7 @@
 
 use crate::{
     bindings,
+    error::to_result,
     str::CStr,
     types::{ARef, Opaque},
 };
@@ -209,6 +210,20 @@ pub fn property_present(&self, name: &CStr) -> bool {
     }
 }
 
+impl Device<Bound> {
+    /// Populate platform_devices from device tree data
+    pub fn devm_of_platform_populate(&self) -> crate::error::Result<()> {
+        // SAFETY: self is valid bound Device reference
+        to_result(unsafe { bindings::devm_of_platform_populate(self.as_raw()) })
+    }
+
+    /// Remove devices populated from device tree
+    pub fn devm_of_platform_depopulate(&self) {
+        // SAFETY: self is valid bound Device reference
+        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
+    }
+}
+
 // SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic
 // argument.
 kernel::impl_device_context_deref!(unsafe { Device });

---
base-commit: 33035b665157558254b3c21c3f049fd728e72368
change-id: 20250428-rust-of-populate-b2f961783441

Best regards,
-- 
Ayush Singh <ayush@beagleboard.org>

On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
> +    /// Remove devices populated from device tree
> +    pub fn devm_of_platform_depopulate(&self) {
> +        // SAFETY: self is valid bound Device reference
> +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
> +    }
> +}

One additional question regarding devm_of_platform_depopulate(). This function
is only used once throughout the whole kernel (in [1]), and at a first glance
the usage there seems unnecessary.

In your upcoming driver you call devm_of_platform_depopulate() from a fallible
path [2].

So, I think we should change devm_of_platform_depopulate() to return an error
instead of WARN(ret).

If [1] needs it for some subtle reason I don't see, then I think we can still
call it from there as

	WARN(devm_of_platform_depopulate())

[1] https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/soc/ti/pruss.c#L558
[2] https://github.com/Ayush1325/linux/commit/cdb1322b7166532445c54b601ad0a252866e574d#diff-7b9e3179e36732d5f3a681034d70c2fda4ff57745c79ad4a656f328c91e54b77R71

On Tue, Apr 29, 2025 at 04:31:52PM +0200, Danilo Krummrich wrote:
> On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
> > +    /// Remove devices populated from device tree
> > +    pub fn devm_of_platform_depopulate(&self) {
> > +        // SAFETY: self is valid bound Device reference
> > +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
> > +    }
> > +}
> 
> One additional question regarding devm_of_platform_depopulate(). This function
> is only used once throughout the whole kernel (in [1]), and at a first glance
> the usage there seems unnecessary.
> 
> In your upcoming driver you call devm_of_platform_depopulate() from a fallible
> path [2].
> 
> So, I think we should change devm_of_platform_depopulate() to return an error
> instead of WARN(ret).
> 
> If [1] needs it for some subtle reason I don't see, then I think we can still
> call it from there as
> 
> 	WARN(devm_of_platform_depopulate())
> 
> [1] https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/soc/ti/pruss.c#L558
> [2] https://github.com/Ayush1325/linux/commit/cdb1322b7166532445c54b601ad0a252866e574d#diff-7b9e3179e36732d5f3a681034d70c2fda4ff57745c79ad4a656f328c91e54b77R71

Ugh, no, we should just delete this function entirely if only one driver
is using it.  That implies it's not really needed at all.

And we should NEVER have a WARN() call, that's just an excuse to reboot
the box and assign a CVE...

thanks,

greg k-h

On Tue, Apr 29, 2025 at 04:37:49PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 29, 2025 at 04:31:52PM +0200, Danilo Krummrich wrote:
> > On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
> > > +    /// Remove devices populated from device tree
> > > +    pub fn devm_of_platform_depopulate(&self) {
> > > +        // SAFETY: self is valid bound Device reference
> > > +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
> > > +    }
> > > +}
> > 
> > One additional question regarding devm_of_platform_depopulate(). This function
> > is only used once throughout the whole kernel (in [1]), and at a first glance
> > the usage there seems unnecessary.
> > 
> > In your upcoming driver you call devm_of_platform_depopulate() from a fallible
> > path [2].
> > 
> > So, I think we should change devm_of_platform_depopulate() to return an error
> > instead of WARN(ret).
> > 
> > If [1] needs it for some subtle reason I don't see, then I think we can still
> > call it from there as
> > 
> > 	WARN(devm_of_platform_depopulate())
> > 
> > [1] https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/soc/ti/pruss.c#L558
> > [2] https://github.com/Ayush1325/linux/commit/cdb1322b7166532445c54b601ad0a252866e574d#diff-7b9e3179e36732d5f3a681034d70c2fda4ff57745c79ad4a656f328c91e54b77R71
> 
> Ugh, no, we should just delete this function entirely if only one driver
> is using it.  That implies it's not really needed at all.

Ayush's driver calls {de}populate() from a sysfs store path [2]; not sure what
it's doing semantically or if this is a valid use-case though.

On Tue, Apr 29, 2025 at 04:44:54PM +0200, Danilo Krummrich wrote:
> On Tue, Apr 29, 2025 at 04:37:49PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 29, 2025 at 04:31:52PM +0200, Danilo Krummrich wrote:
> > > On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
> > > > +    /// Remove devices populated from device tree
> > > > +    pub fn devm_of_platform_depopulate(&self) {
> > > > +        // SAFETY: self is valid bound Device reference
> > > > +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
> > > > +    }
> > > > +}
> > > 
> > > One additional question regarding devm_of_platform_depopulate(). This function
> > > is only used once throughout the whole kernel (in [1]), and at a first glance
> > > the usage there seems unnecessary.
> > > 
> > > In your upcoming driver you call devm_of_platform_depopulate() from a fallible
> > > path [2].
> > > 
> > > So, I think we should change devm_of_platform_depopulate() to return an error
> > > instead of WARN(ret).
> > > 
> > > If [1] needs it for some subtle reason I don't see, then I think we can still
> > > call it from there as
> > > 
> > > 	WARN(devm_of_platform_depopulate())
> > > 
> > > [1] https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/soc/ti/pruss.c#L558
> > > [2] https://github.com/Ayush1325/linux/commit/cdb1322b7166532445c54b601ad0a252866e574d#diff-7b9e3179e36732d5f3a681034d70c2fda4ff57745c79ad4a656f328c91e54b77R71
> > 
> > Ugh, no, we should just delete this function entirely if only one driver
> > is using it.  That implies it's not really needed at all.
> 
> Ayush's driver calls {de}populate() from a sysfs store path [2]; not sure what
> it's doing semantically or if this is a valid use-case though.

That's going to be rough, and full of tricky corner-cases and probably
shouldn't be doing that at all :)

So let's hold off on this entirely until we see a real user that can
actually pass review.  Trying to do system configuration like this in
sysfs is a much larger discussion than just adding rust bindings.

(hint, configfs is for system configuration, not sysfs...)

Anyway, worst case, you just "open code" the single function call that
this one binding was trying to "wrap".  which is what I think the
in-kernel user should be doing now.

thanks,

greg k-h

On 4/29/25 20:27, Greg Kroah-Hartman wrote:

> On Tue, Apr 29, 2025 at 04:44:54PM +0200, Danilo Krummrich wrote:
>> On Tue, Apr 29, 2025 at 04:37:49PM +0200, Greg Kroah-Hartman wrote:
>>> On Tue, Apr 29, 2025 at 04:31:52PM +0200, Danilo Krummrich wrote:
>>>> On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
>>>>> +    /// Remove devices populated from device tree
>>>>> +    pub fn devm_of_platform_depopulate(&self) {
>>>>> +        // SAFETY: self is valid bound Device reference
>>>>> +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
>>>>> +    }
>>>>> +}
>>>> One additional question regarding devm_of_platform_depopulate(). This function
>>>> is only used once throughout the whole kernel (in [1]), and at a first glance
>>>> the usage there seems unnecessary.
>>>>
>>>> In your upcoming driver you call devm_of_platform_depopulate() from a fallible
>>>> path [2].
>>>>
>>>> So, I think we should change devm_of_platform_depopulate() to return an error
>>>> instead of WARN(ret).
>>>>
>>>> If [1] needs it for some subtle reason I don't see, then I think we can still
>>>> call it from there as
>>>>
>>>> 	WARN(devm_of_platform_depopulate())
>>>>
>>>> [1] https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/soc/ti/pruss.c#L558
>>>> [2] https://github.com/Ayush1325/linux/commit/cdb1322b7166532445c54b601ad0a252866e574d#diff-7b9e3179e36732d5f3a681034d70c2fda4ff57745c79ad4a656f328c91e54b77R71
>>> Ugh, no, we should just delete this function entirely if only one driver
>>> is using it.  That implies it's not really needed at all.
>> Ayush's driver calls {de}populate() from a sysfs store path [2]; not sure what
>> it's doing semantically or if this is a valid use-case though.
> That's going to be rough, and full of tricky corner-cases and probably
> shouldn't be doing that at all :)
>
> So let's hold off on this entirely until we see a real user that can
> actually pass review.  Trying to do system configuration like this in
> sysfs is a much larger discussion than just adding rust bindings.
>
> (hint, configfs is for system configuration, not sysfs...)
>
> Anyway, worst case, you just "open code" the single function call that
> this one binding was trying to "wrap".  which is what I think the
> in-kernel user should be doing now.
>
> thanks,
>
> greg k-h


Well, I don't really want to convert this discussion to addon board 
connector setup discussions. So I will try to keep things as short as 
possible here while linking to all the other discussions for the same.

For starters, what the driver does is as follows:

1. Provide 3 sysfs entries:

     - New cape: Can write the name of the cape (I have not settled on 
the naming convention yet). This name is then used to load appropriate 
overlay from `/lib/firmware/` and populate all the devices. The overlay 
is applied to the connector node. Only one cape overlay can be used at a 
time.

     - Current cape: Just a ro entry to get the name of any active cape.

     - Delete cape: Remove cape overlay and registered devices.

It's a very basic driver, where I am trying to experiment with the 
following patches ([2], [3], [4]) to be able to provide a better picture 
of things, and get a read on what more needs to be added to devicetree 
spec and/or other infra to make connector setups possible, and provide a 
proof of concept that might move the needle a bit more than the past 
year has.


I do not think this should use configfs, but maybe I am wrong.


`of_platform_populate` is used to discover the devices that are added by 
the overlay. Without this function, I am not sure how a setup which is 
supposed to only modify the devicetree in a particular node is supposed 
to work.


The reason why local devicetree overlay is being used instead of 
modifying the local tree is the discussion here regarding how global 
tree modification is a security problem and any approach using it will 
be difficult to upstream [0]. That is one of the reasons for not 
pursuing the approach described here [1].


I am okay with maintaining the patches for Rust side out of tree, 
because well, at this point, it's a much smaller list than the number of 
out of tree patches I need to have for the C side to be able to show a 
semi complete connector setup anyway. And nothing is going to be merged 
until a so called perfect solution is found. But just want to list out 
why I at least do not want the C side of 
`of_platform_populate/depopulate` to not disappear.


Of course, feel free to list out any better alternatives than having to 
use `of_platform_populate/depopulate`, which can be used for this purpose.


Best Regards,

Ayush Singh


[0]: 
https://lore.kernel.org/all/9c326bb7-e09a-4c21-944f-006b3fad1870@beagleboard.org/

[1]: https://lore.kernel.org/lkml/20240702164403.29067-1-afd@ti.com/

[2]: 
https://lore.kernel.org/devicetree-spec/20250415122453.68e4c50f@bootlin.com/T/#m591e737b48ebe96aafa39d87652e07eef99dff90

[3]: 
https://lore.kernel.org/all/20241209151830.95723-1-herve.codina@bootlin.com/

[4]: 
https://lore.kernel.org/devicetree-spec/20250401081041.114333-1-herve.codina@bootlin.com/T/#t

On Tue, Apr 29, 2025 at 6:07 PM Ayush Singh <ayush@beagleboard.org> wrote:
>
> I am okay with maintaining the patches for Rust side out of tree,
> because well, at this point, it's a much smaller list than the number of
> out of tree patches I need to have for the C side to be able to show a
> semi complete connector setup anyway. And nothing is going to be merged

In general, abstractions are not upstreamed unless there is an
expected user in-tree, i.e. the relevant maintainers already need to
agree on merging the user, and just having something out-of-tree does
not count, see:

    https://rust-for-linux.com/contributing#submitting-new-abstractions-and-modules

Thus, given what you say about the C side, it does sound like the
first step will be there anyway.

Cheers,
Miguel

On Tue, Apr 29, 2025 at 09:37:04PM +0530, Ayush Singh wrote:
> On 4/29/25 20:27, Greg Kroah-Hartman wrote:
> 
> > On Tue, Apr 29, 2025 at 04:44:54PM +0200, Danilo Krummrich wrote:
> > > On Tue, Apr 29, 2025 at 04:37:49PM +0200, Greg Kroah-Hartman wrote:
> > > > On Tue, Apr 29, 2025 at 04:31:52PM +0200, Danilo Krummrich wrote:
> > > > > On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
> > > > > > +    /// Remove devices populated from device tree
> > > > > > +    pub fn devm_of_platform_depopulate(&self) {
> > > > > > +        // SAFETY: self is valid bound Device reference
> > > > > > +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
> > > > > > +    }
> > > > > > +}
> > > > > One additional question regarding devm_of_platform_depopulate(). This function
> > > > > is only used once throughout the whole kernel (in [1]), and at a first glance
> > > > > the usage there seems unnecessary.
> > > > > 
> > > > > In your upcoming driver you call devm_of_platform_depopulate() from a fallible
> > > > > path [2].
> > > > > 
> > > > > So, I think we should change devm_of_platform_depopulate() to return an error
> > > > > instead of WARN(ret).
> > > > > 
> > > > > If [1] needs it for some subtle reason I don't see, then I think we can still
> > > > > call it from there as
> > > > > 
> > > > > 	WARN(devm_of_platform_depopulate())
> > > > > 
> > > > > [1] https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/soc/ti/pruss.c#L558
> > > > > [2] https://github.com/Ayush1325/linux/commit/cdb1322b7166532445c54b601ad0a252866e574d#diff-7b9e3179e36732d5f3a681034d70c2fda4ff57745c79ad4a656f328c91e54b77R71
> > > > Ugh, no, we should just delete this function entirely if only one driver
> > > > is using it.  That implies it's not really needed at all.
> > > Ayush's driver calls {de}populate() from a sysfs store path [2]; not sure what
> > > it's doing semantically or if this is a valid use-case though.
> > That's going to be rough, and full of tricky corner-cases and probably
> > shouldn't be doing that at all :)
> > 
> > So let's hold off on this entirely until we see a real user that can
> > actually pass review.  Trying to do system configuration like this in
> > sysfs is a much larger discussion than just adding rust bindings.
> > 
> > (hint, configfs is for system configuration, not sysfs...)
> > 
> > Anyway, worst case, you just "open code" the single function call that
> > this one binding was trying to "wrap".  which is what I think the
> > in-kernel user should be doing now.
> > 
> > thanks,
> > 
> > greg k-h
> 
> 
> Well, I don't really want to convert this discussion to addon board
> connector setup discussions. So I will try to keep things as short as
> possible here while linking to all the other discussions for the same.
> 
> For starters, what the driver does is as follows:
> 
> 1. Provide 3 sysfs entries:
> 
>     - New cape: Can write the name of the cape (I have not settled on the
> naming convention yet). This name is then used to load appropriate overlay
> from `/lib/firmware/` and populate all the devices. The overlay is applied
> to the connector node. Only one cape overlay can be used at a time.
> 
>     - Current cape: Just a ro entry to get the name of any active cape.
> 
>     - Delete cape: Remove cape overlay and registered devices.

That's great, but I don't think that's what sysfs is good for, we can
discuss that later when you submit your driver for review.

Again, look at configfs please, that's for "configuring" things.  sysfs
is for basic device properties and some tunables, but is NOT a major api
interface that requires a lot of logic like loading an overlay would
require.

Also, circumventing the "normal" device tree overlay interface and
discussion isn't ok either, this needs to work for all types of devices,
not just for "capes" like you have here.  To accept something like this
would be going around all of those other maintainers with their strong
views of how things should be done.

Anyway, we can discuss that when you submit your code for review, but
for now, I don't want to take this binding either as I think the whole
function should just be removed from the kernel anyway :)

thanks,

greg k-h

On 4/29/25 22:09, Greg Kroah-Hartman wrote:

> On Tue, Apr 29, 2025 at 09:37:04PM +0530, Ayush Singh wrote:
>> On 4/29/25 20:27, Greg Kroah-Hartman wrote:
>>
>>> On Tue, Apr 29, 2025 at 04:44:54PM +0200, Danilo Krummrich wrote:
>>>> On Tue, Apr 29, 2025 at 04:37:49PM +0200, Greg Kroah-Hartman wrote:
>>>>> On Tue, Apr 29, 2025 at 04:31:52PM +0200, Danilo Krummrich wrote:
>>>>>> On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
>>>>>>> +    /// Remove devices populated from device tree
>>>>>>> +    pub fn devm_of_platform_depopulate(&self) {
>>>>>>> +        // SAFETY: self is valid bound Device reference
>>>>>>> +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
>>>>>>> +    }
>>>>>>> +}
>>>>>> One additional question regarding devm_of_platform_depopulate(). This function
>>>>>> is only used once throughout the whole kernel (in [1]), and at a first glance
>>>>>> the usage there seems unnecessary.
>>>>>>
>>>>>> In your upcoming driver you call devm_of_platform_depopulate() from a fallible
>>>>>> path [2].
>>>>>>
>>>>>> So, I think we should change devm_of_platform_depopulate() to return an error
>>>>>> instead of WARN(ret).
>>>>>>
>>>>>> If [1] needs it for some subtle reason I don't see, then I think we can still
>>>>>> call it from there as
>>>>>>
>>>>>> 	WARN(devm_of_platform_depopulate())
>>>>>>
>>>>>> [1] https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/soc/ti/pruss.c#L558
>>>>>> [2] https://github.com/Ayush1325/linux/commit/cdb1322b7166532445c54b601ad0a252866e574d#diff-7b9e3179e36732d5f3a681034d70c2fda4ff57745c79ad4a656f328c91e54b77R71
>>>>> Ugh, no, we should just delete this function entirely if only one driver
>>>>> is using it.  That implies it's not really needed at all.
>>>> Ayush's driver calls {de}populate() from a sysfs store path [2]; not sure what
>>>> it's doing semantically or if this is a valid use-case though.
>>> That's going to be rough, and full of tricky corner-cases and probably
>>> shouldn't be doing that at all :)
>>>
>>> So let's hold off on this entirely until we see a real user that can
>>> actually pass review.  Trying to do system configuration like this in
>>> sysfs is a much larger discussion than just adding rust bindings.
>>>
>>> (hint, configfs is for system configuration, not sysfs...)
>>>
>>> Anyway, worst case, you just "open code" the single function call that
>>> this one binding was trying to "wrap".  which is what I think the
>>> in-kernel user should be doing now.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Well, I don't really want to convert this discussion to addon board
>> connector setup discussions. So I will try to keep things as short as
>> possible here while linking to all the other discussions for the same.
>>
>> For starters, what the driver does is as follows:
>>
>> 1. Provide 3 sysfs entries:
>>
>>      - New cape: Can write the name of the cape (I have not settled on the
>> naming convention yet). This name is then used to load appropriate overlay
>> from `/lib/firmware/` and populate all the devices. The overlay is applied
>> to the connector node. Only one cape overlay can be used at a time.
>>
>>      - Current cape: Just a ro entry to get the name of any active cape.
>>
>>      - Delete cape: Remove cape overlay and registered devices.
> That's great, but I don't think that's what sysfs is good for, we can
> discuss that later when you submit your driver for review.
>
> Again, look at configfs please, that's for "configuring" things.  sysfs
> is for basic device properties and some tunables, but is NOT a major api
> interface that requires a lot of logic like loading an overlay would
> require.
>
> Also, circumventing the "normal" device tree overlay interface and
> discussion isn't ok either, this needs to work for all types of devices,
> not just for "capes" like you have here.  To accept something like this
> would be going around all of those other maintainers with their strong
> views of how things should be done.

Umm, can you please explain how exactly I am circumventing "normal" 
devicetree overlay interface and discussion here. If I submit spec or 
devicetree compiler changes, nobody responds. And when I submit the full 
driver, it ends up with it not being the way upstream wants. And I have 
not seen any of Herve and Luca's subsystem specific patches for addon 
board + connector setups accepted yet. So if there is a place where I 
can discuss things regarding the addon board + connector setup more 
before writing full implementations, I would really love that.

I have added Herve and Luca here as well, if they want to add something.

> Anyway, we can discuss that when you submit your code for review, but
> for now, I don't want to take this binding either as I think the whole
> function should just be removed from the kernel anyway :)
>
> thanks,
>
> greg k-h


Best Regards,

Ayush Singh

On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
> +impl Device<Bound> {
> +    /// Populate platform_devices from device tree data
> +    pub fn devm_of_platform_populate(&self) -> crate::error::Result<()> {
> +        // SAFETY: self is valid bound Device reference

Here and below, I'd write something along the lines of:

	// SAFETY: By its type invariant `self.as_raw()` is a valid pointer to a
	// `struct device`.

> +        to_result(unsafe { bindings::devm_of_platform_populate(self.as_raw()) })
> +    }
> +
> +    /// Remove devices populated from device tree
> +    pub fn devm_of_platform_depopulate(&self) {
> +        // SAFETY: self is valid bound Device reference
> +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
> +    }
> +}

On Tue, Apr 29, 2025 at 1:41 PM Ayush Singh <ayush@beagleboard.org> wrote:
>
> +    /// Populate platform_devices from device tree data
> +    pub fn devm_of_platform_populate(&self) -> crate::error::Result<()> {
> +        // SAFETY: self is valid bound Device reference

If you import the prelude, then you could use just `Result`. Also,
please try to follow the style of the rest of the code, e.g. use
Markdown in both comments and documentation and finish sentences with
a period.

Thanks!

Cheers,
Miguel