From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933477AbbCRVFm (ORCPT ); Wed, 18 Mar 2015 17:05:42 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:42699 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932889AbbCRVFk (ORCPT ); Wed, 18 Mar 2015 17:05:40 -0400 Message-ID: <0a0f6120c9944ada787c13cd52a069e9.squirrel@www.codeaurora.org> In-Reply-To: <55092F9C.3050004@codeaurora.org> References: <1426289058-6663-1-git-send-email-hali@codeaurora.org> <1426289058-6663-4-git-send-email-hali@codeaurora.org> <55092F9C.3050004@codeaurora.org> Date: Wed, 18 Mar 2015 21:05:38 -0000 Subject: Re: [PATCH 3/4] drm/msm: Initial add DSI connector support From: hali@codeaurora.org To: "Archit Taneja" Cc: "Hai Li" , dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org User-Agent: SquirrelMail/1.4.22-4.el6 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Priority: 3 (Normal) Importance: Normal Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Archit, Thanks for your comments. Please see my response for some comments below. Comments without response will be addressed in patch version 2. I will wait for other comments if any to push patch V2. >> +static int dsi_gpio_init(struct msm_dsi_host *msm_host) >> +{ >> + int ret; >> + >> + msm_host->disp_en_gpio = devm_gpiod_get(&msm_host->pdev->dev, >> + "disp-enable"); >> + if (IS_ERR(msm_host->disp_en_gpio)) { >> + pr_warn("%s: cannot get disp-enable-gpios %ld\n", >> + __func__, PTR_ERR(msm_host->disp_en_gpio)); >> + msm_host->disp_en_gpio = NULL; >> + } >> + if (msm_host->disp_en_gpio) { >> + ret = gpiod_direction_output(msm_host->disp_en_gpio, 0); >> + if (ret) { >> + pr_err("cannot set dir to disp-en-gpios %d\n", ret); >> + return ret; >> + } >> + } >> + >> + msm_host->te_gpio = devm_gpiod_get(&msm_host->pdev->dev, "disp-te"); >> + if (IS_ERR(msm_host->te_gpio)) { >> + pr_warn("%s: cannot get disp-te-gpios %ld\n", >> + __func__, PTR_ERR(msm_host->te_gpio)); > > Video mode panels won't have te_gpio, we could probably make this > pr_debug() > >> + msm_host->te_gpio = NULL; >> + } >> + >> + if (msm_host->te_gpio) { >> + ret = gpiod_direction_input(msm_host->te_gpio); >> + if (ret) { >> + pr_err("%s: cannot set dir to disp-te-gpios, %d\n", >> + __func__, ret); >> + return ret; >> + } >> + } > > These gpios currently need to be declared under the dsi DT node. Even if > these are controlled via the dsi host, the gpios should still come under > the panel DT node. > > We shout get the panel's of_node here and look for these. > >> +static void dsi_sw_reset(struct msm_dsi_host *msm_host) >> +{ >> + dsi_write(msm_host, REG_DSI_CLK_CTRL, >> + DSI_CLK_CTRL_AHBS_HCLK_ON | DSI_CLK_CTRL_AHBM_SCLK_ON | >> + DSI_CLK_CTRL_PCLK_ON | DSI_CLK_CTRL_DSICLK_ON | >> + DSI_CLK_CTRL_BYTECLK_ON | DSI_CLK_CTRL_ESCCLK_ON | >> + DSI_CLK_CTRL_FORCE_ON_DYN_AHBM_HCLK); > > The same 7 bits seem to be set elsewhere, maybe make this a macro > DSI_ENABLE_CLKS or something similar? > >> +int msm_dsi_host_init(struct msm_dsi *msm_dsi) >> +{ >> + struct msm_dsi_host *msm_host = NULL; >> + struct platform_device *pdev = msm_dsi->pdev; >> + int ret = 0; >> + >> + msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL); >> + if (!msm_host) { >> + pr_err("%s: FAILED: cannot alloc dsi host\n", >> + __func__); >> + ret = -ENOMEM; >> + goto fail; >> + } >> + >> + ret = of_property_read_u32(pdev->dev.of_node, >> + "cell-index", &msm_host->id); > > retrieving the instance number of a peripheral via a DT property like > 'cell-index' has been debated quite a bit in the past. I suppose it's > not the best thing to do. > > However, since the DSI instances in MDSS aren't completely identical(one > acts a master and other slave in dual dsi mode), maybe we can get away > with having a property like "qcom,dsi-master;", and that can be further > used to identify whether this node is DSI_0 or DSI_1 > 2 DSIs are not always master-slave mode. It is possible that a single panel is connected to any of the hosts, or 2 panels are controlled independently. If 'cell-index' is not allowed to specify the instance number, i would prefer to have a simple property like "qcom,dsi-host-index". >> +int msm_dsi_host_register(struct mipi_dsi_host *host, bool check_defer) >> +{ >> + struct msm_dsi_host *msm_host = to_msm_dsi_host(host); >> + struct device_node *node; >> + int ret; >> + >> + /* Register mipi dsi host */ >> + if (!msm_host->registered) { >> + host->dev = &msm_host->pdev->dev; >> + host->ops = &dsi_host_ops; >> + ret = mipi_dsi_host_register(host); >> + if (ret) >> + return ret; >> + >> + msm_host->registered = true; >> + >> + /* If the panel driver has not been probed after host register, >> + * we should defer the host's probe. >> + * It makes sure panel is connected when fbcon detects >> + * connector status and gets the proper display mode to >> + * create framebuffer. >> + */ >> + if (check_defer) { >> + node = of_parse_phandle(msm_host->pdev->dev.of_node, >> + "qcom,panel", 0); >> + if (node) { >> + if (!of_drm_find_panel(node)) >> + return -EPROBE_DEFER; >> + } >> + } >> + } > > We might have to defer probe multiple times before another dependency is > met. The above approach will let the driver defer only once without the > panel driver registered. During the second probe attempt, we'd just > return since 'msm_host->registered' would be true. > > I think we could move this check to end of dsi_init(). > Once probe deferred, the private data structures will be cleaned up and re-allocated at the next probe. It should support multiple times probe deferral. >> +static int dsi_mgr_connector_mode_valid(struct drm_connector >> *connector, >> + struct drm_display_mode *mode) >> +{ >> + int id = dsi_mgr_connector_get_id(connector); >> + struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); >> + struct drm_encoder *encoder = >> + (msm_dsi->panel_flags & MIPI_DSI_MODE_VIDEO) ? >> + msm_dsi->base.encoders[MSM_DSI_VIDEO_ENCODER_ID] : >> + msm_dsi->base.encoders[MSM_DSI_CMD_ENCODER_ID]; > > Maybe you could make a helper 'msm_dsi_get_encoder(msm_dsi)' out of > this? It seems to be used elsewhere too. > >> +int msm_dsi_manager_register(struct msm_dsi *msm_dsi) >> +{ >> + struct msm_dsi_manager *msm_dsim = &msm_dsim_glb; >> + int id = msm_dsi->id; >> + struct msm_dsi *other_dsi = dsi_mgr_get_other_dsi(id); >> + int ret; >> + >> + if (id > DSI_MAX) { >> + pr_err("%s: invalid id %d\n", __func__, id); >> + return -EINVAL; >> + } >> + >> + if (msm_dsim->dsi[id]) { >> + pr_err("%s: dsi%d already registered\n", __func__, id); >> + return -EBUSY; >> + } >> + >> + msm_dsim->dsi[id] = msm_dsi; >> + >> + ret = dsi_mgr_parse_dual_panel(msm_dsi->pdev->dev.of_node, id); >> + if (ret) { >> + pr_err("%s: failed to parse dual panel info\n", __func__); >> + return ret; >> + } >> + >> + if (!IS_DUAL_PANEL()) { >> + ret = msm_dsi_host_register(msm_dsi->host, true); >> + } else if (!other_dsi) { >> + return 0; >> + } else { >> + struct msm_dsi *mdsi = IS_MASTER_PANEL(id) ? >> + msm_dsi : other_dsi; >> + struct msm_dsi *sdsi = IS_MASTER_PANEL(id) ? >> + other_dsi : msm_dsi; >> + /* Register slave host first, so that slave DSI device >> + * has a chance to probe, and do not block the master >> + * DSI device's probe. >> + * Also, do not check defer for the slave host, >> + * because only master DSI device adds the panel to global >> + * panel list. The panel's device is the master DSI device. >> + */ >> + ret = msm_dsi_host_register(sdsi->host, false); >> + if (ret) >> + return ret; >> + ret = msm_dsi_host_register(mdsi->host, true); >> + } >> + >> + return ret; >> +} > > The dual panel checks in these functions are quite intrusive at the > moment. We'd use DSI later where there won't be a panel at all. That > would result in ever more complicated checks. > > Is it possible to separate out the dual panel functionality such that it > becomes cleaner? > > For example msm_dsi_manager_phy_disable can look like: > > void msm_dsi_manager_phy_disable(int id) > { > ... > ... > > if (msm_dual_dsi_mode()) > msm_dual_dsi_phy_disable(id); > else > msm_dsi_phy_disable(phy); > > ... > } > > There might be repetition of some code between the normal and dual mode > case, but it will make things quite legible. > I think even we separate out the dual DSI functionality, we still need to check dual DSI mode like the code above. The purpose of dsi_manager module is to centralize dual DSI handler, so there has to be many checks. The current code should work with different cases, single-host-single-panel, dual-host-single-panel, dual-host-dual-panel and dual-host-independent-two-panels. If there is no panel for the host, we will report disconnected connector. If we want to convert to other interface type, i would prefer to have a fake dsi panel driver, to follow the current framework. > > Thanks for the patch. > > Archit > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project >