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)
...
...
+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.
Thanks,
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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
