Re: [PATCH v2 1/5] rust: Move property_present to separate file

[Remo Senekowitsch]

On Mon Apr 14, 2025 at 8:00 PM CEST, Danilo Krummrich wrote:
> On Mon, Apr 14, 2025 at 06:40:54PM +0200, Remo Senekowitsch wrote:
>> On Mon Apr 14, 2025 at 6:00 PM CEST, Danilo Krummrich wrote:
>> > On Mon, Apr 14, 2025 at 05:26:26PM +0200, Remo Senekowitsch wrote:
>> >> +impl Device {
>> >> +    /// Obtain the fwnode corresponding to the device.
>> >> +    fn fwnode(&self) -> &FwNode {
>> >> +        // SAFETY: `self` is valid.
>> >> +        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
>> >> +        if fwnode_handle.is_null() {
>> >> +            panic!("fwnode_handle cannot be null");
>> >
>> > Please don't use panic!() unless you hit an error condition that is impossible
>> > to handle.
>> >
>> > If __dev_fwnode() can ever return NULL, make this function return
>> > Option<&FwNode> instead.
>> 
>> Rob Herring mentioned this in the previous thread already:
>> 
>> > Users/drivers testing fwnode_handle/of_node for NULL is pretty common.
>> > Though often that's a legacy code path, so maybe not allowing NULL is
>> > fine for now.
>> 
>> I asked a follow-up question about how to proceed but got no reply[3].
>
> I NULL is a possible return value of __dev_fwnode() (which seems to be the
> case), there's no other option than makeing fwnode() fallible.

That makes sense. In that case, I think we shouldn't duplicate the
methods on `Device` and `FwNode`, because their signatures would be
different. E.g. `Device::property_present` would have to return
`Option<bool>` while `FwNode::property_present` would return `bool`.
I think it would be cleaner to force driver authors to do something
like `if let Some(fwnode) = dev.fwnode() { /* read properties */ }`.