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 backlighthandle\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.