| Message ID | 20240202101326.876070-2-saravanak@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve remote-endpoint parsing | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 90 lines checked |
| robh/patch-applied | fail | build log |
On Fri, Feb 02, 2024 at 02:13:24AM -0800, Saravana Kannan wrote: > We have a more accurate function to find the right consumer of a > remote-endpoint property instead of searching for a parent with > compatible string property. So, use that instead. While at it, make the > code to find the consumer a bit more flexible and based on the property > being parsed. > > Fixes: f7514a663016 ("of: property: fw_devlink: Add support for remote-endpoint") > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/of/property.c | 52 +++++++++++++------------------------------ > 1 file changed, 15 insertions(+), 37 deletions(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 641a40cf5cf3..ba374a1f2072 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1063,36 +1063,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, > return of_device_get_match_data(dev); > } > > -static struct device_node *of_get_compat_node(struct device_node *np) > -{ > - of_node_get(np); > - > - while (np) { > - if (!of_device_is_available(np)) { > - of_node_put(np); > - np = NULL; > - } > - > - if (of_property_present(np, "compatible")) > - break; > - > - np = of_get_next_parent(np); > - } > - > - return np; > -} > - > -static struct device_node *of_get_compat_node_parent(struct device_node *np) > -{ > - struct device_node *parent, *node; > - > - parent = of_get_parent(np); > - node = of_get_compat_node(parent); > - of_node_put(parent); > - > - return node; > -} > - > static void of_link_to_phandle(struct device_node *con_np, > struct device_node *sup_np) > { > @@ -1222,10 +1192,10 @@ static struct device_node *parse_##fname(struct device_node *np, \ > * parse_prop.prop_name: Name of property holding a phandle value > * parse_prop.index: For properties holding a list of phandles, this is the > * index into the list > + * @get_con_dev: If the consumer node containing the property is never converted > + * to a struct device, implement this ops so fw_devlink can use it > + * to find the true consumer. > * @optional: Describes whether a supplier is mandatory or not > - * @node_not_dev: The consumer node containing the property is never converted > - * to a struct device. Instead, parse ancestor nodes for the > - * compatible property to find a node corresponding to a device. > * > * Returns: > * parse_prop() return values are > @@ -1236,8 +1206,8 @@ static struct device_node *parse_##fname(struct device_node *np, \ > struct supplier_bindings { > struct device_node *(*parse_prop)(struct device_node *np, > const char *prop_name, int index); > + struct device_node *(*get_con_dev)(struct device_node *np); > bool optional; > - bool node_not_dev; > }; > > DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells") > @@ -1328,6 +1298,11 @@ static struct device_node *parse_interrupts(struct device_node *np, > return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; > } > > +static struct device_node *get_remote_endpoint_dev(struct device_node *np) > +{ > + return to_of_node(fwnode_graph_get_port_parent(of_fwnode_handle(np))); DT APIs shouldn't depend on fwnode APIs. Otherwise, this series looks good.
On Mon, Feb 5, 2024 at 9:36 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Feb 02, 2024 at 02:13:24AM -0800, Saravana Kannan wrote: > > We have a more accurate function to find the right consumer of a > > remote-endpoint property instead of searching for a parent with > > compatible string property. So, use that instead. While at it, make the > > code to find the consumer a bit more flexible and based on the property > > being parsed. > > > > Fixes: f7514a663016 ("of: property: fw_devlink: Add support for remote-endpoint") > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/of/property.c | 52 +++++++++++++------------------------------ > > 1 file changed, 15 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index 641a40cf5cf3..ba374a1f2072 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1063,36 +1063,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, > > return of_device_get_match_data(dev); > > } > > > > -static struct device_node *of_get_compat_node(struct device_node *np) > > -{ > > - of_node_get(np); > > - > > - while (np) { > > - if (!of_device_is_available(np)) { > > - of_node_put(np); > > - np = NULL; > > - } > > - > > - if (of_property_present(np, "compatible")) > > - break; > > - > > - np = of_get_next_parent(np); > > - } > > - > > - return np; > > -} > > - > > -static struct device_node *of_get_compat_node_parent(struct device_node *np) > > -{ > > - struct device_node *parent, *node; > > - > > - parent = of_get_parent(np); > > - node = of_get_compat_node(parent); > > - of_node_put(parent); > > - > > - return node; > > -} > > - > > static void of_link_to_phandle(struct device_node *con_np, > > struct device_node *sup_np) > > { > > @@ -1222,10 +1192,10 @@ static struct device_node *parse_##fname(struct device_node *np, \ > > * parse_prop.prop_name: Name of property holding a phandle value > > * parse_prop.index: For properties holding a list of phandles, this is the > > * index into the list > > + * @get_con_dev: If the consumer node containing the property is never converted > > + * to a struct device, implement this ops so fw_devlink can use it > > + * to find the true consumer. > > * @optional: Describes whether a supplier is mandatory or not > > - * @node_not_dev: The consumer node containing the property is never converted > > - * to a struct device. Instead, parse ancestor nodes for the > > - * compatible property to find a node corresponding to a device. > > * > > * Returns: > > * parse_prop() return values are > > @@ -1236,8 +1206,8 @@ static struct device_node *parse_##fname(struct device_node *np, \ > > struct supplier_bindings { > > struct device_node *(*parse_prop)(struct device_node *np, > > const char *prop_name, int index); > > + struct device_node *(*get_con_dev)(struct device_node *np); > > bool optional; > > - bool node_not_dev; > > }; > > > > DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells") > > @@ -1328,6 +1298,11 @@ static struct device_node *parse_interrupts(struct device_node *np, > > return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; > > } > > > > +static struct device_node *get_remote_endpoint_dev(struct device_node *np) > > +{ > > + return to_of_node(fwnode_graph_get_port_parent(of_fwnode_handle(np))); > > DT APIs shouldn't depend on fwnode APIs. I get what you are saying, but is it an API though? It's a static internal function that's eventually used by .add_link to add fwnode links. Would it be better if I rolled this into two separate inline code where this function is getting called from? I'm trying to avoid duplicating the function that's already present in fwnode_graph_get_port_parent(), but it also feels too specific an op to introduce into drivers/of/property.c How would you like me to handle this? I don't see a good option. > Otherwise, this series looks good. Thanks, Saravana
On Mon, Feb 5, 2024 at 11:47 AM Saravana Kannan <saravanak@google.com> wrote: > > On Mon, Feb 5, 2024 at 9:36 AM Rob Herring <robh@kernel.org> wrote: > > > > On Fri, Feb 02, 2024 at 02:13:24AM -0800, Saravana Kannan wrote: > > > We have a more accurate function to find the right consumer of a > > > remote-endpoint property instead of searching for a parent with > > > compatible string property. So, use that instead. While at it, make the > > > code to find the consumer a bit more flexible and based on the property > > > being parsed. > > > > > > Fixes: f7514a663016 ("of: property: fw_devlink: Add support for remote-endpoint") > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > --- > > > drivers/of/property.c | 52 +++++++++++++------------------------------ > > > 1 file changed, 15 insertions(+), 37 deletions(-) > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > > index 641a40cf5cf3..ba374a1f2072 100644 > > > --- a/drivers/of/property.c > > > +++ b/drivers/of/property.c > > > @@ -1063,36 +1063,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, > > > return of_device_get_match_data(dev); > > > } > > > > > > -static struct device_node *of_get_compat_node(struct device_node *np) > > > -{ > > > - of_node_get(np); > > > - > > > - while (np) { > > > - if (!of_device_is_available(np)) { > > > - of_node_put(np); > > > - np = NULL; > > > - } > > > - > > > - if (of_property_present(np, "compatible")) > > > - break; > > > - > > > - np = of_get_next_parent(np); > > > - } > > > - > > > - return np; > > > -} > > > - > > > -static struct device_node *of_get_compat_node_parent(struct device_node *np) > > > -{ > > > - struct device_node *parent, *node; > > > - > > > - parent = of_get_parent(np); > > > - node = of_get_compat_node(parent); > > > - of_node_put(parent); > > > - > > > - return node; > > > -} > > > - > > > static void of_link_to_phandle(struct device_node *con_np, > > > struct device_node *sup_np) > > > { > > > @@ -1222,10 +1192,10 @@ static struct device_node *parse_##fname(struct device_node *np, \ > > > * parse_prop.prop_name: Name of property holding a phandle value > > > * parse_prop.index: For properties holding a list of phandles, this is the > > > * index into the list > > > + * @get_con_dev: If the consumer node containing the property is never converted > > > + * to a struct device, implement this ops so fw_devlink can use it > > > + * to find the true consumer. > > > * @optional: Describes whether a supplier is mandatory or not > > > - * @node_not_dev: The consumer node containing the property is never converted > > > - * to a struct device. Instead, parse ancestor nodes for the > > > - * compatible property to find a node corresponding to a device. > > > * > > > * Returns: > > > * parse_prop() return values are > > > @@ -1236,8 +1206,8 @@ static struct device_node *parse_##fname(struct device_node *np, \ > > > struct supplier_bindings { > > > struct device_node *(*parse_prop)(struct device_node *np, > > > const char *prop_name, int index); > > > + struct device_node *(*get_con_dev)(struct device_node *np); > > > bool optional; > > > - bool node_not_dev; > > > }; > > > > > > DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells") > > > @@ -1328,6 +1298,11 @@ static struct device_node *parse_interrupts(struct device_node *np, > > > return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; > > > } > > > > > > +static struct device_node *get_remote_endpoint_dev(struct device_node *np) > > > +{ > > > + return to_of_node(fwnode_graph_get_port_parent(of_fwnode_handle(np))); > > > > DT APIs shouldn't depend on fwnode APIs. > > I get what you are saying, but is it an API though? It's a static > internal function that's eventually used by .add_link to add fwnode > links. Would it be better if I rolled this into two separate inline > code where this function is getting called from? I'm trying to avoid > duplicating the function that's already present in > fwnode_graph_get_port_parent(), but it also feels too specific an op > to introduce into drivers/of/property.c > > How would you like me to handle this? I don't see a good option. Nevermind, there's already of_graph_get_port_parent() that does exactly the same as fwnode_graph_get_port_parent(). I'll just use that. -Saravana
diff --git a/drivers/of/property.c b/drivers/of/property.c index 641a40cf5cf3..ba374a1f2072 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1063,36 +1063,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, return of_device_get_match_data(dev); } -static struct device_node *of_get_compat_node(struct device_node *np) -{ - of_node_get(np); - - while (np) { - if (!of_device_is_available(np)) { - of_node_put(np); - np = NULL; - } - - if (of_property_present(np, "compatible")) - break; - - np = of_get_next_parent(np); - } - - return np; -} - -static struct device_node *of_get_compat_node_parent(struct device_node *np) -{ - struct device_node *parent, *node; - - parent = of_get_parent(np); - node = of_get_compat_node(parent); - of_node_put(parent); - - return node; -} - static void of_link_to_phandle(struct device_node *con_np, struct device_node *sup_np) { @@ -1222,10 +1192,10 @@ static struct device_node *parse_##fname(struct device_node *np, \ * parse_prop.prop_name: Name of property holding a phandle value * parse_prop.index: For properties holding a list of phandles, this is the * index into the list + * @get_con_dev: If the consumer node containing the property is never converted + * to a struct device, implement this ops so fw_devlink can use it + * to find the true consumer. * @optional: Describes whether a supplier is mandatory or not - * @node_not_dev: The consumer node containing the property is never converted - * to a struct device. Instead, parse ancestor nodes for the - * compatible property to find a node corresponding to a device. * * Returns: * parse_prop() return values are @@ -1236,8 +1206,8 @@ static struct device_node *parse_##fname(struct device_node *np, \ struct supplier_bindings { struct device_node *(*parse_prop)(struct device_node *np, const char *prop_name, int index); + struct device_node *(*get_con_dev)(struct device_node *np); bool optional; - bool node_not_dev; }; DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells") @@ -1328,6 +1298,11 @@ static struct device_node *parse_interrupts(struct device_node *np, return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; } +static struct device_node *get_remote_endpoint_dev(struct device_node *np) +{ + return to_of_node(fwnode_graph_get_port_parent(of_fwnode_handle(np))); +} + static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_clocks, }, { .parse_prop = parse_interconnects, }, @@ -1352,7 +1327,10 @@ static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_pinctrl6, }, { .parse_prop = parse_pinctrl7, }, { .parse_prop = parse_pinctrl8, }, - { .parse_prop = parse_remote_endpoint, .node_not_dev = true, }, + { + .parse_prop = parse_remote_endpoint, + .get_con_dev = get_remote_endpoint_dev, + }, { .parse_prop = parse_pwms, }, { .parse_prop = parse_resets, }, { .parse_prop = parse_leds, }, @@ -1403,8 +1381,8 @@ static int of_link_property(struct device_node *con_np, const char *prop_name) while ((phandle = s->parse_prop(con_np, prop_name, i))) { struct device_node *con_dev_np; - con_dev_np = s->node_not_dev - ? of_get_compat_node_parent(con_np) + con_dev_np = s->get_con_dev + ? s->get_con_dev(con_np) : of_node_get(con_np); matched = true; i++;
We have a more accurate function to find the right consumer of a remote-endpoint property instead of searching for a parent with compatible string property. So, use that instead. While at it, make the code to find the consumer a bit more flexible and based on the property being parsed. Fixes: f7514a663016 ("of: property: fw_devlink: Add support for remote-endpoint") Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/of/property.c | 52 +++++++++++++------------------------------ 1 file changed, 15 insertions(+), 37 deletions(-)