[DPU PATCH] drm/msm/dsi: add only dsi nodes with a valid device to list

[DPU PATCH] drm/msm/dsi: add only dsi nodes with a valid device to list

[Abhinav Kumar]

Before adding a DSI node to the private list check if the
node has a valid device connected to it through an endpoint.

This is required in cases where the chipset supports multiple
DSI hosts but only one of them is being used.

In the current implementation even inactive nodes get added
resulting in creation of redundant connectors.

Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 drivers/gpu/drm/msm/dsi/dsi.c      |  6 +++++-
 drivers/gpu/drm/msm/dsi/dsi.h      |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index b744bcc..46a4906 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -120,7 +120,11 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
 	if (IS_ERR(msm_dsi))
 		return PTR_ERR(msm_dsi);
 
-	priv->dsi[msm_dsi->id] = msm_dsi;
+	/* Add only the host which has a device attached to it */
+	if (msm_dsi_has_valid_device(msm_dsi->host)) {
+		pr_info("id = %d has valid device\n", msm_dsi->id);
+		priv->dsi[msm_dsi->id] = msm_dsi;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 70d9a9a..aa198ef 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -180,6 +180,7 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
 int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
 					struct drm_device *dev);
 int msm_dsi_host_init(struct msm_dsi *msm_dsi);
+bool msm_dsi_has_valid_device(struct mipi_dsi_host *host);
 int msm_dsi_runtime_suspend(struct device *dev);
 int msm_dsi_runtime_resume(struct device *dev);
 
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 2f1a278..25d65e5 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -470,6 +470,16 @@ static void dsi_bus_clk_disable(struct msm_dsi_host *msm_host)
 		clk_disable_unprepare(msm_host->bus_clks[i]);
 }
 
+bool msm_dsi_has_valid_device(struct mipi_dsi_host *host)
+{
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+	if (msm_host->device_node)
+		return true;
+
+	return false;
+}
+
 int msm_dsi_runtime_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH] drm/msm/dsi: add only dsi nodes with a valid device to list

[Jordan Crouse]

On Fri, Jun 15, 2018 at 11:05:57PM -0700, Abhinav Kumar wrote:
> Before adding a DSI node to the private list check if the
> node has a valid device connected to it through an endpoint.
> 
> This is required in cases where the chipset supports multiple
> DSI hosts but only one of them is being used.
> 
> In the current implementation even inactive nodes get added
> resulting in creation of redundant connectors.
> 
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.c      |  6 +++++-
>  drivers/gpu/drm/msm/dsi/dsi.h      |  1 +
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index b744bcc..46a4906 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -120,7 +120,11 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
>  	if (IS_ERR(msm_dsi))
>  		return PTR_ERR(msm_dsi);
>  
> -	priv->dsi[msm_dsi->id] = msm_dsi;
> +	/* Add only the host which has a device attached to it */
> +	if (msm_dsi_has_valid_device(msm_dsi->host)) {
> +		pr_info("id = %d has valid device\n", msm_dsi->id);

This seems like a debug message that slipped through.

> +		priv->dsi[msm_dsi->id] = msm_dsi;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 70d9a9a..aa198ef 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -180,6 +180,7 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
>  int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>  					struct drm_device *dev);
>  int msm_dsi_host_init(struct msm_dsi *msm_dsi);
> +bool msm_dsi_has_valid_device(struct mipi_dsi_host *host);
>  int msm_dsi_runtime_suspend(struct device *dev);
>  int msm_dsi_runtime_resume(struct device *dev);
>  
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 2f1a278..25d65e5 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -470,6 +470,16 @@ static void dsi_bus_clk_disable(struct msm_dsi_host *msm_host)
>  		clk_disable_unprepare(msm_host->bus_clks[i]);
>  }
>  
> +bool msm_dsi_has_valid_device(struct mipi_dsi_host *host)
> +{
> +	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> +
> +	if (msm_host->device_node)
> +		return true;
> +
> +	return false;

Nit, return (msm_host->device_node) ? true : false; works just as well.

> +}
> +
>  int msm_dsi_runtime_suspend(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [DPU PATCH] drm/msm/dsi: add only dsi nodes with a valid device to list

[abhinavk]

Hi Jordan

Thanks for the review.

Comments inline.

Abhinav
On 2018-06-18 07:23, Jordan Crouse wrote:
> On Fri, Jun 15, 2018 at 11:05:57PM -0700, Abhinav Kumar wrote:
>> Before adding a DSI node to the private list check if the
>> node has a valid device connected to it through an endpoint.
>> 
>> This is required in cases where the chipset supports multiple
>> DSI hosts but only one of them is being used.
>> 
>> In the current implementation even inactive nodes get added
>> resulting in creation of redundant connectors.
>> 
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi.c      |  6 +++++-
>>  drivers/gpu/drm/msm/dsi/dsi.h      |  1 +
>>  drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
>>  3 files changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
>> b/drivers/gpu/drm/msm/dsi/dsi.c
>> index b744bcc..46a4906 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>> @@ -120,7 +120,11 @@ static int dsi_bind(struct device *dev, struct 
>> device *master, void *data)
>>  	if (IS_ERR(msm_dsi))
>>  		return PTR_ERR(msm_dsi);
>> 
>> -	priv->dsi[msm_dsi->id] = msm_dsi;
>> +	/* Add only the host which has a device attached to it */
>> +	if (msm_dsi_has_valid_device(msm_dsi->host)) {
>> +		pr_info("id = %d has valid device\n", msm_dsi->id);
> 
> This seems like a debug message that slipped through.
[Abhinav] No, this was intentionally added. This will be printed only at 
probe and at most 2 times.
This gives the information about how many devices have been initialized.

> 
>> +		priv->dsi[msm_dsi->id] = msm_dsi;
>> +	}
>> 
>>  	return 0;
>>  }
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>> b/drivers/gpu/drm/msm/dsi/dsi.h
>> index 70d9a9a..aa198ef 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>> @@ -180,6 +180,7 @@ void msm_dsi_host_get_phy_clk_req(struct 
>> mipi_dsi_host *host,
>>  int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>>  					struct drm_device *dev);
>>  int msm_dsi_host_init(struct msm_dsi *msm_dsi);
>> +bool msm_dsi_has_valid_device(struct mipi_dsi_host *host);
>>  int msm_dsi_runtime_suspend(struct device *dev);
>>  int msm_dsi_runtime_resume(struct device *dev);
>> 
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 2f1a278..25d65e5 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -470,6 +470,16 @@ static void dsi_bus_clk_disable(struct 
>> msm_dsi_host *msm_host)
>>  		clk_disable_unprepare(msm_host->bus_clks[i]);
>>  }
>> 
>> +bool msm_dsi_has_valid_device(struct mipi_dsi_host *host)
>> +{
>> +	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>> +
>> +	if (msm_host->device_node)
>> +		return true;
>> +
>> +	return false;
> 
> Nit, return (msm_host->device_node) ? true : false; works just as well.
[Abhinav] Alright, will make this change in V2
> 
>> +}
>> +
>>  int msm_dsi_runtime_suspend(struct device *dev)
>>  {
>>  	struct platform_device *pdev = to_platform_device(dev);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [DPU PATCH] drm/msm/dsi: add only dsi nodes with a valid device to list

[Jordan Crouse]

On Mon, Jun 18, 2018 at 03:11:44PM -0700, abhinavk@codeaurora.org wrote:
> Hi Jordan
> 
> Thanks for the review.
> 
> Comments inline.
> 
> Abhinav
> On 2018-06-18 07:23, Jordan Crouse wrote:
> >On Fri, Jun 15, 2018 at 11:05:57PM -0700, Abhinav Kumar wrote:
> >>Before adding a DSI node to the private list check if the
> >>node has a valid device connected to it through an endpoint.
> >>
> >>This is required in cases where the chipset supports multiple
> >>DSI hosts but only one of them is being used.
> >>
> >>In the current implementation even inactive nodes get added
> >>resulting in creation of redundant connectors.
> >>
> >>Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> >>---
> >> drivers/gpu/drm/msm/dsi/dsi.c      |  6 +++++-
> >> drivers/gpu/drm/msm/dsi/dsi.h      |  1 +
> >> drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
> >> 3 files changed, 16 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/gpu/drm/msm/dsi/dsi.c
> >>b/drivers/gpu/drm/msm/dsi/dsi.c
> >>index b744bcc..46a4906 100644
> >>--- a/drivers/gpu/drm/msm/dsi/dsi.c
> >>+++ b/drivers/gpu/drm/msm/dsi/dsi.c
> >>@@ -120,7 +120,11 @@ static int dsi_bind(struct device *dev,
> >>struct device *master, void *data)
> >> 	if (IS_ERR(msm_dsi))
> >> 		return PTR_ERR(msm_dsi);
> >>
> >>-	priv->dsi[msm_dsi->id] = msm_dsi;
> >>+	/* Add only the host which has a device attached to it */
> >>+	if (msm_dsi_has_valid_device(msm_dsi->host)) {
> >>+		pr_info("id = %d has valid device\n", msm_dsi->id);
> >
> >This seems like a debug message that slipped through.
> [Abhinav] No, this was intentionally added. This will be printed
> only at probe and at most 2 times.
> This gives the information about how many devices have been initialized.

If that is the case then it should be properly constructed. It should use
dev_info like the rest of the messages in this file and it should be written
in simple present tense with the end user in mind as an audience. 
Something like:

dev_info(dev, "DSI device %d is valid\n", msm_dsi->id);

or

dev_info(dev, ""DSI device %d is initialized\n", msm_dsi->id);

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [DPU PATCH] drm/msm/dsi: add only dsi nodes with a valid device to list

[Archit Taneja]

On Saturday 16 June 2018 11:35 AM, Abhinav Kumar wrote:
> Before adding a DSI node to the private list check if the
> node has a valid device connected to it through an endpoint.
> 
> This is required in cases where the chipset supports multiple
> DSI hosts but only one of them is being used.
> 
> In the current implementation even inactive nodes get added
> resulting in creation of redundant connectors.

Reviewed-by: Archit Taneja <architt@codeauora.org>

> 
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi.c      |  6 +++++-
>   drivers/gpu/drm/msm/dsi/dsi.h      |  1 +
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
>   3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index b744bcc..46a4906 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -120,7 +120,11 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
>   	if (IS_ERR(msm_dsi))
>   		return PTR_ERR(msm_dsi);
>   
> -	priv->dsi[msm_dsi->id] = msm_dsi;
> +	/* Add only the host which has a device attached to it */
> +	if (msm_dsi_has_valid_device(msm_dsi->host)) {
> +		pr_info("id = %d has valid device\n", msm_dsi->id);
> +		priv->dsi[msm_dsi->id] = msm_dsi;
> +	}
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 70d9a9a..aa198ef 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -180,6 +180,7 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
>   int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
>   					struct drm_device *dev);
>   int msm_dsi_host_init(struct msm_dsi *msm_dsi);
> +bool msm_dsi_has_valid_device(struct mipi_dsi_host *host);
>   int msm_dsi_runtime_suspend(struct device *dev);
>   int msm_dsi_runtime_resume(struct device *dev);
>   
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 2f1a278..25d65e5 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -470,6 +470,16 @@ static void dsi_bus_clk_disable(struct msm_dsi_host *msm_host)
>   		clk_disable_unprepare(msm_host->bus_clks[i]);
>   }
>   
> +bool msm_dsi_has_valid_device(struct mipi_dsi_host *host)
> +{
> +	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> +
> +	if (msm_host->device_node)
> +		return true;
> +
> +	return false;
> +}
> +
>   int msm_dsi_runtime_suspend(struct device *dev)
>   {
>   	struct platform_device *pdev = to_platform_device(dev);
> 
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno