| Message ID | 20250407223714.2287202-3-sean.anderson@linux.dev |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 4/8/25 09:00, Rob Herring wrote: > On Mon, Apr 7, 2025 at 5:37 PM Sean Anderson <sean.anderson@linux.dev> 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. --Sean
On 4/8/25 11:19, Rob Herring wrote: > On Tue, Apr 8, 2025 at 10:12 AM Sean Anderson <sean.anderson@linux.dev> wrote: >> >> On 4/8/25 09:00, Rob Herring wrote: >> > On Mon, Apr 7, 2025 at 5:37 PM Sean Anderson <sean.anderson@linux.dev> 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. --Sean
On Tue, Apr 8, 2025 at 10:28 AM Sean Anderson <sean.anderson@linux.dev> wrote: > > On 4/8/25 11:19, Rob Herring wrote: > > On Tue, Apr 8, 2025 at 10:12 AM Sean Anderson <sean.anderson@linux.dev> wrote: > >> > >> On 4/8/25 09:00, Rob Herring wrote: > >> > On Mon, Apr 7, 2025 at 5:37 PM Sean Anderson <sean.anderson@linux.dev> 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
diff --git a/drivers/base/property.c b/drivers/base/property.c index 049f8a6088a1..ef13ca32079b 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -618,6 +618,52 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode, } EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args); +/** + * fwnode_property_get_reference_optional_args() - Find a reference with optional arguments + * @fwnode: Firmware node where to look for the reference + * @prop: The name of the property + * @nargs_prop: The name of the property telling the number of + * arguments in the referred node. + * @index: Index of the reference, from zero onwards. + * @args: Result structure with reference and integer arguments. + * May be NULL. + * + * Obtain a reference based on a named property in an fwnode, with + * integer arguments. If @nargs_prop is absent from the referenced node, then + * number of arguments is be assumed to be 0. + * + * The caller is responsible for calling fwnode_handle_put() on the returned + * @args->fwnode pointer. + * + * Return: %0 on success + * %-ENOENT when the index is out of bounds, the index has an empty + * reference or the property was not found + * %-EINVAL on parse error + */ +int fwnode_property_get_reference_optional_args(const struct fwnode_handle *fwnode, + const char *prop, + const char *nargs_prop, + unsigned int index, + struct fwnode_reference_args *args) +{ + int ret; + + if (IS_ERR_OR_NULL(fwnode)) + return -ENOENT; + + ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop, + 0, index, args); + if (ret == 0) + return ret; + + if (IS_ERR_OR_NULL(fwnode->secondary)) + return ret; + + return fwnode_call_int_op(fwnode->secondary, get_reference_args, prop, nargs_prop, + 0, index, args); +} +EXPORT_SYMBOL_GPL(fwnode_property_get_reference_optional_args); + /** * fwnode_find_reference - Find named reference to a fwnode_handle * @fwnode: Firmware node where to look for the reference diff --git a/include/linux/property.h b/include/linux/property.h index e214ecd241eb..a1662b36d15f 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -139,6 +139,10 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode, const char *prop, const char *nargs_prop, unsigned int nargs, unsigned int index, struct fwnode_reference_args *args); +int fwnode_property_get_reference_optional_args(const struct fwnode_handle *fwnode, + const char *prop, const char *nargs_prop, + unsigned int index, + struct fwnode_reference_args *args); struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode, const char *name,
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. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- drivers/base/property.c | 46 ++++++++++++++++++++++++++++++++++++++++ include/linux/property.h | 4 ++++ 2 files changed, 50 insertions(+)