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
>
