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

[Jordan Crouse <jcrouse-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

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