Re: [PATCH 2/2] device property: Add fwnode_property_get_reference_optional_args

[Rob Herring]

On Tue, Apr 8, 2025 at 10:28 AM Sean Anderson <sean.anderson@xxxxxxxxx> wrote:
>
> On 4/8/25 11:19, Rob Herring wrote:
> > On Tue, Apr 8, 2025 at 10:12 AM Sean Anderson <sean.anderson@xxxxxxxxx> wrote:
> >>
> >> On 4/8/25 09:00, Rob Herring wrote:
> >> > On Mon, Apr 7, 2025 at 5:37 PM Sean Anderson <sean.anderson@xxxxxxxxx> wrote:
> >> >>
> >> >> Add a fwnode variant of of_parse_phandle_with_optional_args to allow
> >> >> nargs_prop to be absent from the referenced node. This improves
> >> >> compatibility for references where the devicetree might not always have
> >> >> nargs_prop.
> >> >
> >> > Can't we just make fwnode_property_get_reference_args() handle this
> >> > case? Or why is it not just a 1 line wrapper function?
> >>
> >> fwnode_property_get_reference_args ignores nargs when nargs_prop is
> >> non-NULL. So all the existing callers just pass 0 to nargs. Rather than
> >> convert them, I chose to add another function with different defaults.
> >> There are only four callers that pass nargs_prop, so I could just as
> >> easily change the callers instead.
> >
> > Why do you have to change the callers? nargs value won't matter
> > because they obviously have nargs_prop present or they would not have
> > worked in the first place. If behavior changes because there's an
> > error in their DT, who cares. That's their problem for not validating
> > the DT.
>
> Because the change would be to make nargs matter even when nargs_prop is
> present. For the sake of example, consider something like
>
>   foo: foo {
>     #my-cells = <1>;
>   };
>
>   bar: bar {
>   };
>
>   baz {
>     my-prop = <&bar>, <&foo 5>, ;
>     my-prop-names = "bar", "foo";
>   };
>
> Before we would have
>
> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "bar", args) <bar>
> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "foo", args) <foo>
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "bar", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "foo", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "bar", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "foo", args) ERROR
>
> and after we would have
>
> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "bar", args) <bar>
> fwnode_property_get_reference_args(baz, "my-prop", NULL, 0, "foo", args) <foo>
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "bar", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", -1, "foo", args) ERROR
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "bar", args) <bar>
> fwnode_property_get_reference_args(baz, "my-prop", "#my-cells", 0, "foo", args) <foo 5>
>
> The problem is that all existing callers pass nargs=0 when
> nargs_prop="#my-cells" so they will get the new behavior even when they
> shouldn't. So if we change the behavior we have to change the callers
> too. If we make a new function with new behavior the callers stay the
> same.

It does not matter! It is not the kernel's job to validate the DT. If
it was: it does a terrible job at it and we wouldn't have needed
dtschema. In your example, the change actually makes things work
despite the error! Why would you not want that? No one is relying on
the last 2 cases returning an error other than to debug their DT.

That being said, looking at the DT side, we have
of_parse_phandle_with_args(), of_parse_phandle_with_fixed_args(), and
of_parse_phandle_with_optional_args(). We should mirror that API here
as you have done. So to rephrase, make
fwnode_property_get_reference_args() contents a static function and
then both fwnode_property_get_reference_args() and
fwnode_property_get_reference_optional_args() call that static
function. IOW, similar to the DT API implementation using
__of_parse_phandle_with_args().

Rob