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

[abhinavk@codeaurora.org]

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