On Fri, Mar 10, 2023 at 07:10:21PM +0100, Konrad Dybcio wrote:
>
> > +#define mipi_dsi_dual_dcs_write_seq(dsi0, dsi1, cmd, seq...) \
> > + do { \
> > + mipi_dsi_dcs_write_seq(dsi0, cmd, seq); \
> > + mipi_dsi_dcs_write_seq(dsi1, cmd, seq); \
> > + } while (0)
> This should be in the same file as mipi_dsi_dcs_write_seq, imo
I have sent a patch to do it, upstream don't think this wrapper is a proper
approach to deal with dual dsi and wrap all of mipi_dsi_* function is
useless.
https://lore.kernel.org/lkml/20230310110542.6649-1-lujianhua000@xxxxxxxxx/
>
> [...]
>
> > +static int elish_boe_init_sequence(struct panel_info *pinfo)
> > +{
> > + struct mipi_dsi_device *dsi0 = pinfo->dsi[0];
> > + struct mipi_dsi_device *dsi1 = pinfo->dsi[1];
> > + /* No datasheet, so write magic init sequence directly */
> > + mipi_dsi_dual_dcs_write_seq(dsi0, dsi1, 0xFF, 0x10);
> > + mipi_dsi_dual_dcs_write_seq(dsi0, dsi1, 0xFB, 0x01);
> > + mipi_dsi_dual_dcs_write_seq(dsi0, dsi1, 0xB9, 0x05);
> Non-#defines should use lowercase hex
Acked
>
> [...]
>
> > + msleep(70);
> > + mipi_dsi_dual_dcs_write_seq(dsi0, dsi1, 0x29);
> > + return 0;
> I think it's a general practice to add a newline before the return
> statement, but that's just a nit that triggered my OCD..
Acked
>
> [...]
>
> > + struct drm_connector *connector)
> > +{
> > + struct panel_info *pinfo = to_panel_info(panel);
> > + int i;
> > +
> > + for (i =0; i < pinfo->desc->num_modes; i++) {
> missing space after =
Acked
> > + const struct drm_display_mode *m = &pinfo->desc->modes[i];
> > + struct drm_display_mode *mode;
> Missing newline between declarations and code
Acked
>
> > + mode = drm_mode_duplicate(connector->dev, m);
> > + if (!mode) {
> > + dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> > + m->hdisplay, m->vdisplay, drm_mode_vrefresh(m));
> > + return -ENOMEM;
> > + }
> > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> DRM_MODE_TYPE_PREFERRED: Preferred mode, usually the native resolution of an LCD panel. There should only be one preferred mode per connector at any given time.
>
> https://docs.kernel.org/gpu/drm-kms.html
>
> I'd suggest adding something like:
>
> if (i == 0)
> mode->type |= DRM_MODE_TYPE_PREFERRED
>
Acked
>
> I think I've ran out of things to complain about.. And that's
> a good thing! :D
>
> Konrad
> > + drm_mode_set_name(mode);
> > + drm_mode_probed_add(connector, mode);
> > + }
> > +
> > + connector->display_info.width_mm = pinfo->desc->width_mm;
> > + connector->display_info.height_mm = pinfo->desc->height_mm;
> > + connector->display_info.bpc = pinfo->desc->bpc;
> > +
> > + return pinfo->desc->num_modes;
> > +}
> > +
> > +static const struct drm_panel_funcs nt36523_panel_funcs = {
> > + .disable = nt36523_disable,
> > + .prepare = nt36523_prepare,
> > + .unprepare = nt36523_unprepare,
> > + .get_modes = nt36523_get_modes,
> > +};
> > +
> > +static int nt36523_probe(struct mipi_dsi_device *dsi)
> > +{
> > + struct device *dev = &dsi->dev;
> > + struct device_node *dsi1;
> > + struct mipi_dsi_host *dsi1_host;
> > + struct panel_info *pinfo;
> > + const struct mipi_dsi_device_info *info;
> > + int i, ret;
> > +
> > + pinfo = devm_kzalloc(dev, sizeof(*pinfo), GFP_KERNEL);
> > + if (!pinfo)
> > + return -ENOMEM;
> > +
> > + pinfo->vddio = devm_regulator_get(dev, "vddio");
> > + if (IS_ERR(pinfo->vddio))
> > + return dev_err_probe(dev, PTR_ERR(pinfo->vddio), "failed to get vddio regulator\n");
> > +
> > + pinfo->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(pinfo->reset_gpio))
> > + return dev_err_probe(dev, PTR_ERR(pinfo->reset_gpio), "failed to get reset gpio\n");
> > +
> > + pinfo->desc = of_device_get_match_data(dev);
> > + if (!pinfo->desc)
> > + return -ENODEV;
> > +
> > + /* If the panel is dual dsi, register DSI1 */
> > + if (pinfo->desc->is_dual_dsi) {
> > + info = &pinfo->desc->dsi_info;
> > +
> > + dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
> > + if (!dsi1) {
> > + dev_err(dev, "cannot get secondary DSI node.\n");
> > + return -ENODEV;
> > + }
> > +
> > + dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
> > + of_node_put(dsi1);
> > + if (!dsi1_host) {
> > + return dev_err_probe(dev, -EPROBE_DEFER, "cannot get secondary DSI host\n");
> > + }
> > +
> > + pinfo->dsi[1] = mipi_dsi_device_register_full(dsi1_host, info);
> > + if (!pinfo->dsi[1]) {
> > + dev_err(dev, "cannot get secondary DSI device\n");
> > + return -ENODEV;
> > + }
> > + }
> > +
> > + pinfo->dsi[0] = dsi;
> > + mipi_dsi_set_drvdata(dsi, pinfo);
> > + drm_panel_init(&pinfo->panel, dev, &nt36523_panel_funcs, DRM_MODE_CONNECTOR_DSI);
> > +
> > + ret = drm_panel_of_backlight(&pinfo->panel);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to get backlight\n");
> > +
> > + drm_panel_add(&pinfo->panel);
> > +
> > + for (i = 0; i < DSI_NUM_MIN + pinfo->desc->is_dual_dsi; i++) {
> > + pinfo->dsi[i]->lanes = pinfo->desc->lanes;
> > + pinfo->dsi[i]->format = pinfo->desc->format;
> > + pinfo->dsi[i]->mode_flags = pinfo->desc->mode_flags;
> > +
> > + ret = mipi_dsi_attach(pinfo->dsi[i]);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret, "cannot attach to DSI%d host.\n", i);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id nt36523_of_match[] = {
> > + {
> > + .compatible = "xiaomi,elish-boe-nt36523",
> > + .data = &elish_boe_desc,
> > + },
> > + {
> > + .compatible = "xiaomi,elish-csot-nt36523",
> > + .data = &elish_csot_desc,
> > + },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, nt36523_of_match);
> > +
> > +static struct mipi_dsi_driver nt36523_driver = {
> > + .probe = nt36523_probe,
> > + .remove = nt36523_remove,
> > + .driver = {
> > + .name = "panel-novatek-nt36523",
> > + .of_match_table = nt36523_of_match,
> > + },
> > +};
> > +module_mipi_dsi_driver(nt36523_driver);
> > +
> > +MODULE_AUTHOR("Jianhua Lu <lujianhua000@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("DRM driver for Novatek NT36523 based MIPI DSI panels");
> > +MODULE_LICENSE("GPL");
