Hi Paul,
On Sun, Aug 30, 2020 at 06:48:12PM +0200, Paul Cercueil wrote:
> Le dim. 30 août 2020 à 16:36, 何小龙 (Leon He) a écrit :
> >> +struct ili9341 {
> >> + struct drm_panel panel;
> >> + struct mipi_dsi_device *dsi;
> >> + const struct ili9341_pdata *pdata;
> >> +
> >> + struct gpio_desc *reset_gpiod;
> >> + u32 rotation;
> >> +};
> >> +
> >
> > Hi Paul, you put the mipi_dsi_device inside the struct. I think it
> > maybe not
> > a good idea. That means the panel has a MIPI-DSI interface but it
> > doesn't
> > have actually.
> >
> >> +static int ili9341_probe(struct mipi_dsi_device *dsi)
> >> +{
> >> + struct device *dev = &dsi->dev;
> >> + struct ili9341 *priv;
> >> + int ret;
> >> +
> >> + /* See comment for mipi_dbi_spi_init() */
> >> + if (!dev->coherent_dma_mask) {
> >> + ret = dma_coerce_mask_and_coherent(dev,
> >> DMA_BIT_MASK(32));
> >> + if (ret) {
> >> + dev_warn(dev, "Failed to set dma mask
> >> %d\n", ret);
> >> + return ret;
> >> + }
> >> + }
> >> +
> >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> + if (!priv)
> >> + return -ENOMEM;
> >> +
> >> + mipi_dsi_set_drvdata(dsi, priv);
> >> + priv->dsi = dsi;
> >> +
> >> + device_property_read_u32(dev, "rotation", &priv->rotation);
> >> +
> >> + priv->pdata = device_get_match_data(dev);
> >> + if (!priv->pdata)
> >> + return -EINVAL;
> >> +
> >> + drm_panel_init(&priv->panel, dev, &ili9341_funcs,
> >> + DRM_MODE_CONNECTOR_DPI);
> >> +
> >> + priv->reset_gpiod = devm_gpiod_get(dev, "reset",
> >> GPIOD_OUT_HIGH);
> >> + if (IS_ERR(priv->reset_gpiod)) {
> >> + dev_err(dev, "Couldn't get our reset GPIO\n");
> >> + return PTR_ERR(priv->reset_gpiod);
> >> + }
> >> +
> >> + ret = drm_panel_of_backlight(&priv->panel);
> >> + if (ret < 0) {
> >> + if (ret != -EPROBE_DEFER)
> >> + dev_err(dev, "Failed to get backlight
> >> handle\n");
> >> + return ret;
> >> + }
> >> +
> >> + drm_panel_add(&priv->panel);
> >> +
> >> + dsi->bus_type = priv->pdata->bus_type;
> >> + dsi->lanes = priv->pdata->lanes;
> >> + dsi->format = MIPI_DSI_FMT_RGB565;
> >> +
> >> + ret = mipi_dsi_attach(dsi);
> >> + if (ret) {
> >> + dev_err(dev, "Failed to attach DSI panel\n");
> >> + goto err_panel_remove;
> >> + }
> >> +
> >> + ret = mipi_dsi_maybe_register_tiny_driver(dsi);
> >> + if (ret) {
> >> + dev_err(dev, "Failed to init TinyDRM driver\n");
> >> + goto err_mipi_dsi_detach;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +err_mipi_dsi_detach:
> >> + mipi_dsi_detach(dsi);
> >> +err_panel_remove:
> >> + drm_panel_remove(&priv->panel);
> >> + return ret;
> >> +}
> >> +
> >> +static int ili9341_remove(struct mipi_dsi_device *dsi)
> >> +{
> >> + struct ili9341 *priv = mipi_dsi_get_drvdata(dsi);
> >> +
> >> + mipi_dsi_detach(dsi);
> >> + drm_panel_remove(&priv->panel);
> >> +
> >> + drm_panel_disable(&priv->panel);
> >> + drm_panel_unprepare(&priv->panel);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct ili9341_pdata yx240qv29_pdata = {
> >> + .mode = { DRM_SIMPLE_MODE(240, 320, 37, 49) },
> >> + .width_mm = 0, // TODO
> >> + .height_mm = 0, // TODO
> >> + .bus_type = MIPI_DCS_BUS_TYPE_DBI_SPI_C3,
> >> + .lanes = 1,
> >> +};
> >> +
> >> +static const struct of_device_id ili9341_of_match[] = {
> >> + { .compatible = "adafruit,yx240qv29", .data =
> >> &yx240qv29_pdata },
> >> + { }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, ili9341_of_match);
> >> +
> >> +static struct mipi_dsi_driver ili9341_dsi_driver = {
> >> + .probe = ili9341_probe,
> >> + .remove = ili9341_remove,
> >> + .driver = {
> >> + .name = "ili9341-dsi",
> >> + .of_match_table = ili9341_of_match,
> >> + },
> >> +};
> >> +module_mipi_dsi_driver(ili9341_dsi_driver);
> >
> > Again, you treat this driver as a mipi dsi driver but for a MIPI-DBI
> > (I8080/SPI)
> > panel device. That will make developers confused.
> >
> > Is it possible to just add a mipi_dbi_driver for I8080/SPI interface
> > panel?
> > Thanks!
>
> Please read the cover letter, it explains why it's done this way. The
> whole point of this patchset is to merge DSI and DBI frameworks in a
> way that can be maintained.
I think this proves the point that the proposed naming is confusing. At
least a rename would be required.
--
Regards,
Laurent Pinchart
