Hi Archit, > Hi Hai, > > On 03/19/2015 02:35 AM, hali@xxxxxxxxxxxxxx wrote: >> 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". > > Okay, thanks for that clarification. > >> >>>> +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. > > Ah right! I forgot about that. However, I don't think the panel would be > a phandle entry under the dsi device node, right? Also, "qcom,panel" > seems more like a compatible string to me. Should it rather be: > > node = of_get_child_by_name(msm_host->pdev->dev.of_node, "panel"); > if (node) > ... > ... > I was referring to what's done in other drm drivers, but i agree of_get_child_by_name is a better way to avoid adding another entry in host node. Thanks! >> >>>> +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. >> > > I think it will be a bit hard to incorporate fake panel support. I liked > what's done in exynos/exynos_dp_core.c: > > The driver figures out if the device node supports a panel connected via > a bridge, or a DP connector. Based on this info, it registers either a > bridge driver, or a usual DP connector. > > I think it is fine to get this pulled as is. We can think later about > whether the above or a fake panel approach would make more sense. > The problem of using bridge is that bridge has been mapped to DSI host, like HDMI and eDP drivers, to set_mode/enable/disable to the host, as the encoder is mapped to INTF in MDP. Yes, we can think it later. > Thanks, > Archit > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > Thanks, Hai -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html