Date:   Sun, 8 Dec 2019 10:31:50 +0100
From:   Sam Ravnborg <sam@...nborg.org>
To:     Rob Clark <robdclark@...il.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
        aarch64-laptops@...ts.linaro.org,
        Rob Clark <robdclark@...omium.org>,
        Jeffrey Hugo <jhugo@...eaurora.org>,
        open list <linux-kernel@...r.kernel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Vasily Khoruzhick <anarsoul@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Sean Paul <sean@...rly.run>
Subject: Re: [PATCH 2/4] drm/of: add support to find any enabled endpoint

Hi Rob.

Patch looks good, one small improvement proposal.

On Sat, Dec 07, 2019 at 12:35:51PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@...omium.org>
> 
> To handle the case where there are multiple panel endpoints, only one of
> which is enabled/installed, add support for a wildcard endpoint value to
> request finding whichever endpoint is enabled.
> 
> Signed-off-by: Rob Clark <robdclark@...omium.org>
> ---
>  drivers/gpu/drm/drm_of.c | 41 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 0ca58803ba46..2baf44e401b8 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -219,11 +219,44 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
>  }
>  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>  
> +static int find_enabled_endpoint(const struct device_node *node, u32 port)
> +{
> +	struct device_node *endpoint_node, *remote;
> +	u32 endpoint = 0;
> +
> +	for (endpoint = 0; ; endpoint++) {
> +		endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
> +		if (!endpoint_node) {
> +			pr_debug("No more endpoints!\n");
> +			return -ENODEV;
> +		}
> +
> +		remote = of_graph_get_remote_port_parent(endpoint_node);
> +		of_node_put(endpoint_node);
> +		if (!remote) {
> +			pr_debug("no valid remote node\n");
> +			continue;
> +		}
> +
> +		if (!of_device_is_available(remote)) {
> +			pr_debug("not available for remote node\n");
> +			of_node_put(remote);
> +			continue;
> +		}
> +
> +		pr_debug("found enabled endpoint %d for %s\n", endpoint, remote->name);
> +		of_node_put(remote);
> +		return endpoint;
> +	}
> +
> +	return -ENODEV;
> +}
This function seems pretty generic. Should this be part of
drivers/of/property.c - as others may have the same need?

> +
>  /**
>   * drm_of_find_panel_or_bridge - return connected panel or bridge device
>   * @np: device tree node containing encoder output ports
>   * @port: port in the device tree node
> - * @endpoint: endpoint in the device tree node
> + * @endpoint: endpoint in the device tree node, or -1 to find an enabled endpoint
>   * @panel: pointer to hold returned drm_panel
>   * @bridge: pointer to hold returned drm_bridge

Introducing a define would make it easier to use this function in the
right way.
For example:
#define OF_ENDPOINT_FIRST	-1


Then we would see this code in the user side:
+       ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1,
					  OF_ENDPOINT_FIRST,
					  &pdata->panel, NULL);

Or something like this.

	Sam


>   *
> @@ -246,6 +279,12 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>  	if (panel)
>  		*panel = NULL;
>  
> +	if (endpoint == -1) {
> +		endpoint = find_enabled_endpoint(np, port);
> +		if (endpoint < 0)
> +			return endpoint;
> +	}
> +
>  	remote = of_graph_get_remote_node(np, port, endpoint);
>  	if (!remote)
>  		return -ENODEV;
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Powered by blists - more mailing lists