| Message ID | 20200614172234.8856-2-digetx@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Support DRM bridges on NVIDIA Tegra | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
Hi Dmitry, Thank you for the patch. On Sun, Jun 14, 2020 at 08:22:29PM +0300, Dmitry Osipenko wrote: > In some case, like a DRM display code for example, it's useful to silently > check whether port node exists at all in a device-tree before proceeding > with parsing the graph. > > This patch adds of_graph_get_local_port() which returns pointer to a local > port node, or NULL if graph isn't specified in a device-tree for a given > device node. > > Reviewed-by: Rob Herring <robh@kernel.org> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/of/property.c | 32 +++++++++++++++++++++++--------- > include/linux/of_graph.h | 7 +++++++ > 2 files changed, 30 insertions(+), 9 deletions(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 1f2086f4e7ce..05c5f619b8bb 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > * parent port node. > */ > if (!prev) { > - struct device_node *node; > - > - node = of_get_child_by_name(parent, "ports"); > - if (node) > - parent = node; > - > - port = of_get_child_by_name(parent, "port"); > - of_node_put(node); > - > + port = of_graph_get_local_port(parent); > if (!port) { > pr_err("graph: no port node found in %pOF\n", parent); > return NULL; > @@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) > } > EXPORT_SYMBOL(of_graph_get_remote_port); > > +/** > + * of_graph_get_local_port() - get local port node > + * @node: pointer to a local endpoint device_node > + * > + * Return: First local port node associated with local endpoint node linked > + * to @node. Use of_node_put() on it when done. > + */ > +struct device_node *of_graph_get_local_port(const struct device_node *node) > +{ > + struct device_node *ports, *port; > + > + ports = of_get_child_by_name(node, "ports"); > + if (ports) > + node = ports; > + > + port = of_get_child_by_name(node, "port"); > + of_node_put(ports); > + > + return port; The implementation doesn't seem to match the documentation. If node is a pointer to an endpoint, it should not have any ports child. > +} > +EXPORT_SYMBOL(of_graph_get_local_port); > + > int of_graph_get_endpoint_count(const struct device_node *np) > { > struct device_node *endpoint; > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > index 01038a6aade0..1fdeb72c7765 100644 > --- a/include/linux/of_graph.h > +++ b/include/linux/of_graph.h > @@ -54,6 +54,7 @@ struct device_node *of_graph_get_remote_port_parent( > struct device_node *of_graph_get_remote_port(const struct device_node *node); > struct device_node *of_graph_get_remote_node(const struct device_node *node, > u32 port, u32 endpoint); > +struct device_node *of_graph_get_local_port(const struct device_node *node); > #else > > static inline int of_graph_parse_endpoint(const struct device_node *node, > @@ -116,6 +117,12 @@ static inline struct device_node *of_graph_get_remote_node( > return NULL; > } > > +static inline struct device_node *of_graph_get_local_port( > + const struct device_node *node) > +{ > + return NULL; > +} > + > #endif /* CONFIG_OF */ > > #endif /* __LINUX_OF_GRAPH_H */ > -- > 2.26.0 >
On Tue, Jun 16, 2020 at 04:21:12AM +0300, Laurent Pinchart wrote: > Hi Dmitry, > > Thank you for the patch. > > On Sun, Jun 14, 2020 at 08:22:29PM +0300, Dmitry Osipenko wrote: > > In some case, like a DRM display code for example, it's useful to silently > > check whether port node exists at all in a device-tree before proceeding > > with parsing the graph. > > > > This patch adds of_graph_get_local_port() which returns pointer to a local > > port node, or NULL if graph isn't specified in a device-tree for a given > > device node. > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > drivers/of/property.c | 32 +++++++++++++++++++++++--------- > > include/linux/of_graph.h | 7 +++++++ > > 2 files changed, 30 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index 1f2086f4e7ce..05c5f619b8bb 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > > * parent port node. > > */ > > if (!prev) { > > - struct device_node *node; > > - > > - node = of_get_child_by_name(parent, "ports"); > > - if (node) > > - parent = node; > > - > > - port = of_get_child_by_name(parent, "port"); > > - of_node_put(node); > > - > > + port = of_graph_get_local_port(parent); > > if (!port) { > > pr_err("graph: no port node found in %pOF\n", parent); > > return NULL; > > @@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) > > } > > EXPORT_SYMBOL(of_graph_get_remote_port); > > > > +/** > > + * of_graph_get_local_port() - get local port node > > + * @node: pointer to a local endpoint device_node > > + * > > + * Return: First local port node associated with local endpoint node linked > > + * to @node. Use of_node_put() on it when done. > > + */ > > +struct device_node *of_graph_get_local_port(const struct device_node *node) I forgot to mention that, given that there could be multiple 'port' nodes, this function would be better named of_graph_get_first_local_port(). 'first' here would refer to the nodes order in the device tree, which I believe may not match the port number. For instance, in the following case ports { #address-cells = <1>; #size-cells = <1>; port@1 { reg = <1>; }; port@0 { reg = <0>; }; }; the function would I believe return port@1. It may be a good idea to explain this in the documentation. Depending on how you use this function, if your only use case is to test for the presence of port nodes, it may be best to return a bool and name it of_graph_has_port() or something similar. > > +{ > > + struct device_node *ports, *port; > > + > > + ports = of_get_child_by_name(node, "ports"); > > + if (ports) > > + node = ports; > > + > > + port = of_get_child_by_name(node, "port"); > > + of_node_put(ports); > > + > > + return port; > > The implementation doesn't seem to match the documentation. If node is a > pointer to an endpoint, it should not have any ports child. > > > +} > > +EXPORT_SYMBOL(of_graph_get_local_port); > > + > > int of_graph_get_endpoint_count(const struct device_node *np) > > { > > struct device_node *endpoint; > > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > > index 01038a6aade0..1fdeb72c7765 100644 > > --- a/include/linux/of_graph.h > > +++ b/include/linux/of_graph.h > > @@ -54,6 +54,7 @@ struct device_node *of_graph_get_remote_port_parent( > > struct device_node *of_graph_get_remote_port(const struct device_node *node); > > struct device_node *of_graph_get_remote_node(const struct device_node *node, > > u32 port, u32 endpoint); > > +struct device_node *of_graph_get_local_port(const struct device_node *node); > > #else > > > > static inline int of_graph_parse_endpoint(const struct device_node *node, > > @@ -116,6 +117,12 @@ static inline struct device_node *of_graph_get_remote_node( > > return NULL; > > } > > > > +static inline struct device_node *of_graph_get_local_port( > > + const struct device_node *node) > > +{ > > + return NULL; > > +} > > + > > #endif /* CONFIG_OF */ > > > > #endif /* __LINUX_OF_GRAPH_H */
16.06.2020 04:25, Laurent Pinchart пишет: > On Tue, Jun 16, 2020 at 04:21:12AM +0300, Laurent Pinchart wrote: >> Hi Dmitry, >> >> Thank you for the patch. >> >> On Sun, Jun 14, 2020 at 08:22:29PM +0300, Dmitry Osipenko wrote: >>> In some case, like a DRM display code for example, it's useful to silently >>> check whether port node exists at all in a device-tree before proceeding >>> with parsing the graph. >>> >>> This patch adds of_graph_get_local_port() which returns pointer to a local >>> port node, or NULL if graph isn't specified in a device-tree for a given >>> device node. >>> >>> Reviewed-by: Rob Herring <robh@kernel.org> >>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> drivers/of/property.c | 32 +++++++++++++++++++++++--------- >>> include/linux/of_graph.h | 7 +++++++ >>> 2 files changed, 30 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/of/property.c b/drivers/of/property.c >>> index 1f2086f4e7ce..05c5f619b8bb 100644 >>> --- a/drivers/of/property.c >>> +++ b/drivers/of/property.c >>> @@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, >>> * parent port node. >>> */ >>> if (!prev) { >>> - struct device_node *node; >>> - >>> - node = of_get_child_by_name(parent, "ports"); >>> - if (node) >>> - parent = node; >>> - >>> - port = of_get_child_by_name(parent, "port"); >>> - of_node_put(node); >>> - >>> + port = of_graph_get_local_port(parent); >>> if (!port) { >>> pr_err("graph: no port node found in %pOF\n", parent); >>> return NULL; >>> @@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) >>> } >>> EXPORT_SYMBOL(of_graph_get_remote_port); >>> >>> +/** >>> + * of_graph_get_local_port() - get local port node >>> + * @node: pointer to a local endpoint device_node >>> + * >>> + * Return: First local port node associated with local endpoint node linked >>> + * to @node. Use of_node_put() on it when done. >>> + */ >>> +struct device_node *of_graph_get_local_port(const struct device_node *node) > > I forgot to mention that, given that there could be multiple 'port' > nodes, this function would be better named > of_graph_get_first_local_port(). 'first' here would refer to the nodes > order in the device tree, which I believe may not match the port number. > For instance, in the following case > > ports { > #address-cells = <1>; > #size-cells = <1>; > port@1 { > reg = <1>; > }; > port@0 { > reg = <0>; > }; > }; > > the function would I believe return port@1. It may be a good idea to > explain this in the documentation. Hello Laurent, It's correct that the port@1 will be returned in yours example. I'll improve the doc and the function's name in the next revision, thank you for the suggestions! > Depending on how you use this > function, if your only use case is to test for the presence of port > nodes, it may be best to return a bool and name it of_graph_has_port() > or something similar. > >>> +{ >>> + struct device_node *ports, *port; >>> + >>> + ports = of_get_child_by_name(node, "ports"); >>> + if (ports) >>> + node = ports; >>> + >>> + port = of_get_child_by_name(node, "port"); >>> + of_node_put(ports); >>> + >>> + return port; >> >> The implementation doesn't seem to match the documentation. If node is a >> pointer to an endpoint, it should not have any ports child. Right, I'll reword the doc in v8. This function doesn't differentiate between start / end points. It's up to a user of this function to check whether node is endpoint or something else if needed. Thank you very much for the comments!
16.06.2020 16:56, Dmitry Osipenko пишет: > 16.06.2020 04:25, Laurent Pinchart пишет: >> On Tue, Jun 16, 2020 at 04:21:12AM +0300, Laurent Pinchart wrote: >>> Hi Dmitry, >>> >>> Thank you for the patch. >>> >>> On Sun, Jun 14, 2020 at 08:22:29PM +0300, Dmitry Osipenko wrote: >>>> In some case, like a DRM display code for example, it's useful to silently >>>> check whether port node exists at all in a device-tree before proceeding >>>> with parsing the graph. >>>> >>>> This patch adds of_graph_get_local_port() which returns pointer to a local >>>> port node, or NULL if graph isn't specified in a device-tree for a given >>>> device node. >>>> >>>> Reviewed-by: Rob Herring <robh@kernel.org> >>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/of/property.c | 32 +++++++++++++++++++++++--------- >>>> include/linux/of_graph.h | 7 +++++++ >>>> 2 files changed, 30 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/of/property.c b/drivers/of/property.c >>>> index 1f2086f4e7ce..05c5f619b8bb 100644 >>>> --- a/drivers/of/property.c >>>> +++ b/drivers/of/property.c >>>> @@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, >>>> * parent port node. >>>> */ >>>> if (!prev) { >>>> - struct device_node *node; >>>> - >>>> - node = of_get_child_by_name(parent, "ports"); >>>> - if (node) >>>> - parent = node; >>>> - >>>> - port = of_get_child_by_name(parent, "port"); >>>> - of_node_put(node); >>>> - >>>> + port = of_graph_get_local_port(parent); >>>> if (!port) { >>>> pr_err("graph: no port node found in %pOF\n", parent); >>>> return NULL; >>>> @@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) >>>> } >>>> EXPORT_SYMBOL(of_graph_get_remote_port); >>>> >>>> +/** >>>> + * of_graph_get_local_port() - get local port node >>>> + * @node: pointer to a local endpoint device_node >>>> + * >>>> + * Return: First local port node associated with local endpoint node linked >>>> + * to @node. Use of_node_put() on it when done. >>>> + */ >>>> +struct device_node *of_graph_get_local_port(const struct device_node *node) >> >> I forgot to mention that, given that there could be multiple 'port' >> nodes, this function would be better named >> of_graph_get_first_local_port(). 'first' here would refer to the nodes >> order in the device tree, which I believe may not match the port number. >> For instance, in the following case >> >> ports { >> #address-cells = <1>; >> #size-cells = <1>; >> port@1 { >> reg = <1>; >> }; >> port@0 { >> reg = <0>; >> }; >> }; >> >> the function would I believe return port@1. It may be a good idea to >> explain this in the documentation. > > Hello Laurent, > > It's correct that the port@1 will be returned in yours example. > > I'll improve the doc and the function's name in the next revision, thank > you for the suggestions! > >> Depending on how you use this >> function, if your only use case is to test for the presence of port >> nodes, it may be best to return a bool and name it of_graph_has_port() >> or something similar. >> >>>> +{ >>>> + struct device_node *ports, *port; >>>> + >>>> + ports = of_get_child_by_name(node, "ports"); >>>> + if (ports) >>>> + node = ports; >>>> + >>>> + port = of_get_child_by_name(node, "port"); >>>> + of_node_put(ports); >>>> + >>>> + return port; >>> >>> The implementation doesn't seem to match the documentation. If node is a >>> pointer to an endpoint, it should not have any ports child. > > Right, I'll reword the doc in v8. This function doesn't differentiate > between start / end points. It's up to a user of this function to check > whether node is endpoint or something else if needed. > > Thank you very much for the comments! > Although, I re-read the doc comment and compared it to the wording of the other of_graph_*() functions and it's already good as-is to me because the doc explicitly says the "Return: First local port associated with local endpoint...", which is exactly what this function does. But still the function name and it's brief description could be improved.
diff --git a/drivers/of/property.c b/drivers/of/property.c index 1f2086f4e7ce..05c5f619b8bb 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, * parent port node. */ if (!prev) { - struct device_node *node; - - node = of_get_child_by_name(parent, "ports"); - if (node) - parent = node; - - port = of_get_child_by_name(parent, "port"); - of_node_put(node); - + port = of_graph_get_local_port(parent); if (!port) { pr_err("graph: no port node found in %pOF\n", parent); return NULL; @@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) } EXPORT_SYMBOL(of_graph_get_remote_port); +/** + * of_graph_get_local_port() - get local port node + * @node: pointer to a local endpoint device_node + * + * Return: First local port node associated with local endpoint node linked + * to @node. Use of_node_put() on it when done. + */ +struct device_node *of_graph_get_local_port(const struct device_node *node) +{ + struct device_node *ports, *port; + + ports = of_get_child_by_name(node, "ports"); + if (ports) + node = ports; + + port = of_get_child_by_name(node, "port"); + of_node_put(ports); + + return port; +} +EXPORT_SYMBOL(of_graph_get_local_port); + int of_graph_get_endpoint_count(const struct device_node *np) { struct device_node *endpoint; diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h index 01038a6aade0..1fdeb72c7765 100644 --- a/include/linux/of_graph.h +++ b/include/linux/of_graph.h @@ -54,6 +54,7 @@ struct device_node *of_graph_get_remote_port_parent( struct device_node *of_graph_get_remote_port(const struct device_node *node); struct device_node *of_graph_get_remote_node(const struct device_node *node, u32 port, u32 endpoint); +struct device_node *of_graph_get_local_port(const struct device_node *node); #else static inline int of_graph_parse_endpoint(const struct device_node *node, @@ -116,6 +117,12 @@ static inline struct device_node *of_graph_get_remote_node( return NULL; } +static inline struct device_node *of_graph_get_local_port( + const struct device_node *node) +{ + return NULL; +} + #endif /* CONFIG_OF */ #endif /* __LINUX_OF_GRAPH_H */