Re: [PATCH v6 6/6] drm/sprd: add Unisoc's drm mipi dsi&dphy driver

On Sun, Sep 26, 2021 at 10:31:53PM +0800, Kevin Tang wrote:

Maxime Ripard [off-list ref] 于2021年9月17日周五 下午11:40写道：

quoted

quoted

+static void sprd_dsi_encoder_mode_set(struct drm_encoder *encoder,
+                              struct drm_display_mode *mode,
+                              struct drm_display_mode *adj_mode)
+{
+     struct sprd_dsi *dsi = encoder_to_dsi(encoder);
+
+     drm_dbg(dsi->drm, "%s() set mode: %s\n", __func__, dsi->mode->name);
+}

You don't need that function?

No need for now. need to delete it?

Yes

quoted

quoted

+static int sprd_dsi_encoder_atomic_check(struct drm_encoder *encoder,
+                                 struct drm_crtc_state *crtc_state,
+                                 struct drm_connector_state *conn_state)
+{
+     return 0;
+}

Ditto

No need for now. need to delete it?

Yep

quoted

quoted

+static int sprd_dsi_find_panel(struct sprd_dsi *dsi)
+{
+     struct device *dev = dsi->host.dev;
+     struct device_node *child, *lcds_node;
+     struct drm_panel *panel;
+
+     /* search /lcds child node first */
+     lcds_node = of_find_node_by_path("/lcds");
+     for_each_child_of_node(lcds_node, child) {
+             panel = of_drm_find_panel(child);
+             if (!IS_ERR(panel)) {
+                     dsi->panel = panel;
+                     return 0;
+             }
+     }
+
+     /*
+      * If /lcds child node search failed, we search
+      * the child of dsi host node.
+      */
+     for_each_child_of_node(dev->of_node, child) {
+             panel = of_drm_find_panel(child);
+             if (!IS_ERR(panel)) {
+                     dsi->panel = panel;
+                     return 0;
+             }
+     }
+
+     drm_err(dsi->drm, "of_drm_find_panel() failed\n");
+     return -ENODEV;
+}

Just use devm_drm_of_get_bridge there

We use drm_panel_init and drm_panel_add API to add panel, so here is a
panel device, not a bridge.

Like Sam said, the panel API is on its way out and is being superseded
by bridge_panels.

quoted

quoted

+static int sprd_dsi_host_init(struct sprd_dsi *dsi, struct device *dev)
+{
+     int ret;
+
+     dsi->host.dev = dev;
+     dsi->host.ops = &sprd_dsi_host_ops;
+
+     ret = mipi_dsi_host_register(&dsi->host);
+     if (ret)
+             drm_err(dsi->drm, "failed to register dsi host\n");
+
+     return ret;
+}

[...]

+static int sprd_dsi_connector_init(struct drm_device *drm, struct sprd_dsi *dsi)
+{
+     struct drm_encoder *encoder = &dsi->encoder;
+     struct drm_connector *connector = &dsi->connector;
+     int ret;
+
+     connector->polled = DRM_CONNECTOR_POLL_HPD;
+
+     ret = drm_connector_init(drm, connector,
+                              &sprd_dsi_atomic_connector_funcs,
+                              DRM_MODE_CONNECTOR_DSI);
+     if (ret) {
+             drm_err(drm, "drm_connector_init() failed\n");
+             return ret;
+     }
+
+     drm_connector_helper_add(connector,
+                              &sprd_dsi_connector_helper_funcs);
+
+     drm_connector_attach_encoder(connector, encoder);
+
+     return 0;
+}
+
+static int sprd_dsi_context_init(struct sprd_dsi *dsi,
+                     struct device *dev)
+{
+     struct platform_device *pdev = to_platform_device(dev);
+     struct dsi_context *ctx = &dsi->ctx;
+     struct resource *res;
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     ctx->base = devm_ioremap(dev, res->start, resource_size(res));
+     if (!ctx->base) {
+             drm_err(dsi->drm, "failed to map dsi host registers\n");
+             return -ENXIO;
+     }
+
+     ctx->pll = devm_kzalloc(dev, sizeof(*ctx->pll), GFP_KERNEL);
+     if (!ctx->pll)
+             return -ENOMEM;
+
+     ctx->regmap = devm_regmap_init(dev, &regmap_tst_io, dsi, &byte_config);
+     if (IS_ERR(ctx->regmap)) {
+             drm_err(dsi->drm, "dphy regmap init failed\n");
+             return PTR_ERR(ctx->regmap);
+     }
+
+     ctx->data_hs2lp = 120;
+     ctx->data_lp2hs = 500;
+     ctx->clk_hs2lp = 4;
+     ctx->clk_lp2hs = 15;
+     ctx->max_rd_time = 6000;
+     ctx->int0_mask = 0xffffffff;
+     ctx->int1_mask = 0xffffffff;
+     ctx->enabled = true;
+
+     return 0;
+}
+
+static int sprd_dsi_bind(struct device *dev, struct device *master, void *data)
+{
+     struct drm_device *drm = data;
+     struct sprd_dsi *dsi;
+     int ret;
+
+     dsi = sprd_dsi_encoder_init(drm, dev);
+     if (IS_ERR(dsi))
+             return PTR_ERR(dsi);
+
+     dsi->drm = drm;
+     dev_set_drvdata(dev, dsi);
+
+     ret = sprd_dsi_connector_init(drm, dsi);
+     if (ret)
+             return ret;
+
+     ret = sprd_dsi_context_init(dsi, dev);
+     if (ret)
+             return ret;
+
+     ret = sprd_dsi_host_init(dsi, dev);
+     if (ret)
+             return ret;
+
+     return 0;
+}
+
+static void sprd_dsi_unbind(struct device *dev,
+                     struct device *master, void *data)
+{
+     struct sprd_dsi *dsi = dev_get_drvdata(dev);
+
+     mipi_dsi_host_unregister(&dsi->host);
+}
+
+static const struct component_ops dsi_component_ops = {
+     .bind   = sprd_dsi_bind,
+     .unbind = sprd_dsi_unbind,
+};
+
+static const struct of_device_id dsi_match_table[] = {
+     { .compatible = "sprd,sharkl3-dsi-host" },
+     { /* sentinel */ },
+};
+
+static int sprd_dsi_probe(struct platform_device *pdev)
+{
+     return component_add(&pdev->dev, &dsi_component_ops);

In order to prevent probe issues, you need to register you mipi_dsi_host
here, see:
https://lore.kernel.org/dri-devel/20210910101218.1632297-3-maxime@cerno.tech/ (local)

We register mipi_dsi_hot on our panel driver, like this:

1092   ret = mipi_dsi_attach(slave);
1093   if (ret) {
1094   DRM_ERROR("failed to attach dsi panel to host\n");
1095   drm_panel_remove(&panel->base);
1096   return ret;
1097   }

It's not about when you attach, but when you call
mipi_dsi_host_register. You're doing it in sprd_dsi_host_init that you
call in bind(), which is against the best practices and will create
probing issues in the future.

Maxime