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@xxxxxxxxxx>
>>>> Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>> ---
>>>> 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.
