Re: [PATCH v7 1/6] of_graph: add of_graph_get_local_port()

[Dmitry Osipenko]

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.