1. Patchew
  2. linux
  3. [PATCH v7 0/9] rust: Add ACPI match table support for Rust drivers
  4. View patch

[PATCH v7 3/9] samples: rust: platform: conditionally call Self::properties_parse()

Igor Korotin posted 9 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH v7 3/9] samples: rust: platform: conditionally call Self::properties_parse()
Posted by Igor Korotin 3 months, 3 weeks ago 
From: Danilo Krummrich <dakr@kernel.org>

Only call Self::properties_parse() when the device is compatible with
"test,rust-device".

Once we add ACPI support, we don't want the ACPI device to fail probing
in Self::properties_parse().

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 samples/rust/rust_driver_platform.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
index 000bb915af60..036dd0b899b0 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -40,7 +40,12 @@ fn probe(
             dev_info!(dev, "Probed with info: '{}'.\n", info.0);
         }
 
-        Self::properties_parse(dev)?;
+        if dev
+            .fwnode()
+            .is_some_and(|node| node.is_compatible(c_str!("test,rust-device")))
+        {
+            Self::properties_parse(dev)?;
+        }
 
         let drvdata = KBox::new(Self { pdev: pdev.into() }, GFP_KERNEL)?;
 
-- 
2.43.0
Re: [PATCH v7 3/9] samples: rust: platform: conditionally call Self::properties_parse()
Posted by Rob Herring 3 months, 3 weeks ago 
On Wed, Jun 18, 2025 at 11:13:25AM +0100, Igor Korotin wrote:
> From: Danilo Krummrich <dakr@kernel.org>
> 
> Only call Self::properties_parse() when the device is compatible with
> "test,rust-device".
> 
> Once we add ACPI support, we don't want the ACPI device to fail probing
> in Self::properties_parse().
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

This needs your S-o-b as well since you sent the patch.

> ---
>  samples/rust/rust_driver_platform.rs | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
> index 000bb915af60..036dd0b899b0 100644
> --- a/samples/rust/rust_driver_platform.rs
> +++ b/samples/rust/rust_driver_platform.rs
> @@ -40,7 +40,12 @@ fn probe(
>              dev_info!(dev, "Probed with info: '{}'.\n", info.0);
>          }
>  
> -        Self::properties_parse(dev)?;
> +        if dev
> +            .fwnode()
> +            .is_some_and(|node| node.is_compatible(c_str!("test,rust-device")))

I think you should be checking just is this ACPI or DT rather than 
compatible. It's kind of an anti-pattern to test compatible in probe. 
The reason is we've already matched to a compatible and have match data 
to use, so we don't need to do it again. It becomes quite messy when 
there are numerous possible compatibles.

In C, we just do 'if (dev->of_node)' to check for DT or not.

It is just a sample, but I'm sure folks will copy it.

Rob
Re: [PATCH v7 3/9] samples: rust: platform: conditionally call Self::properties_parse()
Posted by Danilo Krummrich 3 months, 3 weeks ago 
On Wed, Jun 18, 2025 at 08:19:58AM -0500, Rob Herring wrote:
> On Wed, Jun 18, 2025 at 11:13:25AM +0100, Igor Korotin wrote:
> > From: Danilo Krummrich <dakr@kernel.org>
> > 
> > Only call Self::properties_parse() when the device is compatible with
> > "test,rust-device".
> > 
> > Once we add ACPI support, we don't want the ACPI device to fail probing
> > in Self::properties_parse().
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> 
> This needs your S-o-b as well since you sent the patch.
> 
> > ---
> >  samples/rust/rust_driver_platform.rs | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
> > index 000bb915af60..036dd0b899b0 100644
> > --- a/samples/rust/rust_driver_platform.rs
> > +++ b/samples/rust/rust_driver_platform.rs
> > @@ -40,7 +40,12 @@ fn probe(
> >              dev_info!(dev, "Probed with info: '{}'.\n", info.0);
> >          }
> >  
> > -        Self::properties_parse(dev)?;
> > +        if dev
> > +            .fwnode()
> > +            .is_some_and(|node| node.is_compatible(c_str!("test,rust-device")))
> 
> I think you should be checking just is this ACPI or DT rather than 
> compatible. It's kind of an anti-pattern to test compatible in probe. 
> The reason is we've already matched to a compatible and have match data 
> to use, so we don't need to do it again. It becomes quite messy when 
> there are numerous possible compatibles.

Yeah, that was my first approach; here's the patch from a few days ago [1].

The reason why I decided against this, was that all the properties we check in
Self::properties_parse() in a fallible way *only* apply to the device with this
compatible string.

But I don't mind if we replace it with [1] either.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?h=rust/is_of_node
Re: [PATCH v7 3/9] samples: rust: platform: conditionally call Self::properties_parse()
Posted by Danilo Krummrich 3 months, 3 weeks ago 
On Wed, Jun 18, 2025 at 04:11:57PM +0200, Danilo Krummrich wrote:
> On Wed, Jun 18, 2025 at 08:19:58AM -0500, Rob Herring wrote:
> > On Wed, Jun 18, 2025 at 11:13:25AM +0100, Igor Korotin wrote:
> > > From: Danilo Krummrich <dakr@kernel.org>
> > > 
> > > Only call Self::properties_parse() when the device is compatible with
> > > "test,rust-device".
> > > 
> > > Once we add ACPI support, we don't want the ACPI device to fail probing
> > > in Self::properties_parse().
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > 
> > This needs your S-o-b as well since you sent the patch.
> > 
> > > ---
> > >  samples/rust/rust_driver_platform.rs | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
> > > index 000bb915af60..036dd0b899b0 100644
> > > --- a/samples/rust/rust_driver_platform.rs
> > > +++ b/samples/rust/rust_driver_platform.rs
> > > @@ -40,7 +40,12 @@ fn probe(
> > >              dev_info!(dev, "Probed with info: '{}'.\n", info.0);
> > >          }
> > >  
> > > -        Self::properties_parse(dev)?;
> > > +        if dev
> > > +            .fwnode()
> > > +            .is_some_and(|node| node.is_compatible(c_str!("test,rust-device")))
> > 
> > I think you should be checking just is this ACPI or DT rather than 
> > compatible. It's kind of an anti-pattern to test compatible in probe. 
> > The reason is we've already matched to a compatible and have match data 
> > to use, so we don't need to do it again. It becomes quite messy when 
> > there are numerous possible compatibles.
> 
> Yeah, that was my first approach; here's the patch from a few days ago [1].
> 
> The reason why I decided against this, was that all the properties we check in
> Self::properties_parse() in a fallible way *only* apply to the device with this
> compatible string.
> 
> But I don't mind if we replace it with [1] either.

As mentioned, I don't mind either, so let's change it up.

@Igor, can you please pick up the patch in [1] and at the same time drop the
patch introducing FwNode::is_compatible() and replace node.is_compatible() with
node.is_of_node() in this one?

Please also remember to add your SoB to the patches not authored by yourself.

Thanks,
Danilo

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?h=rust/is_of_node
Re: [PATCH v7 3/9] samples: rust: platform: conditionally call Self::properties_parse()
Posted by Igor Korotin 3 months, 2 weeks ago 
On 6/19/25 18:59, Danilo Krummrich wrote:
> On Wed, Jun 18, 2025 at 04:11:57PM +0200, Danilo Krummrich wrote:
>> On Wed, Jun 18, 2025 at 08:19:58AM -0500, Rob Herring wrote:
>>> On Wed, Jun 18, 2025 at 11:13:25AM +0100, Igor Korotin wrote:
>>>> From: Danilo Krummrich <dakr@kernel.org>
>>>>
>>>> Only call Self::properties_parse() when the device is compatible with
>>>> "test,rust-device".
>>>>
>>>> Once we add ACPI support, we don't want the ACPI device to fail probing
>>>> in Self::properties_parse().
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>>
>>> This needs your S-o-b as well since you sent the patch.
>>>
>>>> ---
>>>>  samples/rust/rust_driver_platform.rs | 7 ++++++-
>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
>>>> index 000bb915af60..036dd0b899b0 100644
>>>> --- a/samples/rust/rust_driver_platform.rs
>>>> +++ b/samples/rust/rust_driver_platform.rs
>>>> @@ -40,7 +40,12 @@ fn probe(
>>>>              dev_info!(dev, "Probed with info: '{}'.\n", info.0);
>>>>          }
>>>>  
>>>> -        Self::properties_parse(dev)?;
>>>> +        if dev
>>>> +            .fwnode()
>>>> +            .is_some_and(|node| node.is_compatible(c_str!("test,rust-device")))
>>>
>>> I think you should be checking just is this ACPI or DT rather than 
>>> compatible. It's kind of an anti-pattern to test compatible in probe. 
>>> The reason is we've already matched to a compatible and have match data 
>>> to use, so we don't need to do it again. It becomes quite messy when 
>>> there are numerous possible compatibles.
>>
>> Yeah, that was my first approach; here's the patch from a few days ago [1].
>>
>> The reason why I decided against this, was that all the properties we check in
>> Self::properties_parse() in a fallible way *only* apply to the device with this
>> compatible string.
>>
>> But I don't mind if we replace it with [1] either.
> 
> As mentioned, I don't mind either, so let's change it up.
> 
> @Igor, can you please pick up the patch in [1] and at the same time drop the
> patch introducing FwNode::is_compatible() and replace node.is_compatible() with
> node.is_of_node() in this one?

Just to make sure that we're on the same page, I replace this:

    if dev.fwnode().is_some_and(|node|
node.is_compatible(c_str!("test,rust-device")))

with:

    if dev.fwnode().is_some_and(|node| node.is_of_node())

And reworded the comment as follows:

    samples: rust: platform: conditionally call Self::properties_parse()

    Only call Self::properties_parse() when the device is OF node

    Once we add ACPI support, we don't want the ACPI device to fail probing
    in Self::properties_parse().


Thanks
Igor
Re: [PATCH v7 3/9] samples: rust: platform: conditionally call Self::properties_parse()
Posted by Danilo Krummrich 3 months, 2 weeks ago 
On Fri, Jun 20, 2025 at 12:30:37PM +0100, Igor Korotin wrote:
> Just to make sure that we're on the same page, I replace this:
> 
>     if dev.fwnode().is_some_and(|node|
> node.is_compatible(c_str!("test,rust-device")))
> 
> with:
> 
>     if dev.fwnode().is_some_and(|node| node.is_of_node())
> 
> And reworded the comment as follows:
> 
>     samples: rust: platform: conditionally call Self::properties_parse()
> 
>     Only call Self::properties_parse() when the device is OF node
> 
>     Once we add ACPI support, we don't want the ACPI device to fail probing
>     in Self::properties_parse().

LGTM!

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.