| Message ID | 20200822163250.63664-1-paul@crapouillou.net |
|---|---|
| Headers | show |
| Series | DSI/DBI, panel drivers, & tinyDRM v2 | expand |
On Sat, Aug 22, 2020 at 6:33 PM Paul Cercueil <paul@crapouillou.net> wrote: > +static const struct nv3052c_reg nv3052c_regs[] = { > + { 0xff, 0x30 }, > + { 0xff, 0x52 }, > + { 0xff, 0x01 }, > + { 0xe3, 0x00 }, > + { 0x40, 0x00 }, (...) Well that's pretty opaque :D I suppose no datasheet (why do vendors keep doing this to us...) In other kernel code I have referred to this as a "jam table", e.g. drivers/net/dsa/rtl8366rb.c. I didn't make this up, the name comes from Bunnie Huang's book on hacking the Xbox, and he says it is common hardware engineer lingo. https://www.iacr.org/workshops/ches/ches2002/presentations/Huang.pdf What about naming it nv3052c_jam_table[] or nv3052c_jam[]? Yours, Linus Walleij
Hi Leon, Le dim. 30 août 2020 à 16:36, 何小龙 (Leon He) <Leon.He@unisoc.com> 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. Cheers, -Paul
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.
Hi Laurent. > > > > 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. Do you have any inputs on the amount of rename we are looking into. Is this a simple s/struct mipi_dsi_device/struct mipi_dxi_device/ or something more? We should script the rename as it will tocuh a lot of files, and without a script we would chase this. But once it is scripted it would be trivial to perform. I did not look at this enough, but I had an idea that we would have do to a s/dsi/dxi/ in a lot of places. (dxi is my best proposal at the moment for something covering both dsi and dbi). PS. I am travelling for a few days, so do not expect quick responses. Sam
Le dim. 30 août 2020 à 22:28, Sam Ravnborg <sam@ravnborg.org> a écrit : > Hi Laurent. > >> > >> > 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. > > Do you have any inputs on the amount of rename we are looking into. > Is this a simple s/struct mipi_dsi_device/struct mipi_dxi_device/ > or something more? > > We should script the rename as it will tocuh a lot of files, > and without a script we would chase this. But once it is scripted > it would be trivial to perform. > > I did not look at this enough, but I had an idea that we > would have do to a s/dsi/dxi/ in a lot of places. > > (dxi is my best proposal at the moment for something covering both dsi > and dbi). dcs? Since DBI and DSI panels generally all use DCS commands. -Paul
On 07/09/2020 14:57, Paul Cercueil wrote: > > > Le dim. 30 août 2020 à 22:28, Sam Ravnborg <sam@ravnborg.org> a écrit : >> Hi Laurent. >> >>> > >>> > 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. >> >> Do you have any inputs on the amount of rename we are looking into. >> Is this a simple s/struct mipi_dsi_device/struct mipi_dxi_device/ >> or something more? >> >> We should script the rename as it will tocuh a lot of files, >> and without a script we would chase this. But once it is scripted >> it would be trivial to perform. >> >> I did not look at this enough, but I had an idea that we >> would have do to a s/dsi/dxi/ in a lot of places. >> >> (dxi is my best proposal at the moment for something covering both dsi >> and dbi). > > dcs? > > Since DBI and DSI panels generally all use DCS commands. mipi_disp / mipi_display ? since it's all about mipi display interfaces with different transport protocols. Neil > > -Paul > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Paul, just a drive-by comment: On Sat, Aug 22, 2020 at 6:33 PM Paul Cercueil <paul@crapouillou.net> wrote: > + gpiod_set_value_cansleep(priv->reset_gpiod, 0); > + usleep_range(20, 1000); > + gpiod_set_value_cansleep(priv->reset_gpiod, 1); This implies that the reset line is active low. I would specify in the DT GPIO handle that it is active low and invert the above. So: reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>; gpiod_set_value_cansleep(priv->reset_gpiod, 1); usleep_range(20, 1000); gpiod_set_value_cansleep(priv->reset_gpiod, 0); > + 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); > + } This would then fetch the GPIO as asserted (device in reset) unless changed, but that may be the right thing to do actually. > +static const struct ili9341_pdata yx240qv29_pdata = { > + .mode = { DRM_SIMPLE_MODE(240, 320, 37, 49) }, > + .width_mm = 0, // TODO > + .height_mm = 0, // TODO When nothing else works and data sheets are incomplete I just take out a ruler and measure on the actual device. Yours, Linus Walleij
Hi Paul, I looked a bit at this patch On Sat, Aug 22, 2020 at 6:33 PM Paul Cercueil <paul@crapouillou.net> wrote: > +config DRM_MIPI_DBI_SPI > + tristate "SPI host support for MIPI DBI" > + depends on DRM && OF && SPI I think you want to depend on SPI_HOST actually. > + struct gpio_desc *dc; This dc is very much undocumented, so I can only guess what it is for, please add some kerneldoc explaining what it is. I suppose it is in the DBI spec but I don't have it. > + gpiod_set_value_cansleep(dbi->dc, 0); Since it is optional I usually prefer to do it like this: if (dbi->dc) gpiod_set_value_cansleep(dbi->dc, 0); > + gpiod_set_value_cansleep(dbi->dc, 1); Since you drive this low when you assert it and high when you de-assert it, inverse this and mark the GPIO line as GPIO_ACTIVE_LOW in the device tree instead. > + dbi->dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW); > + if (IS_ERR(dbi->dc)) { > + dev_err(dev, "Failed to get gpio 'dc'\n"); > + return PTR_ERR(dbi->dc); > + } Currently you are requesting the line asserted according to the above logic. Is that what you want? If you change the DT to GPIO_ACTIVE_LOW then this seems correct. But I am overall a bit curious about this "dc" thing. What is it really? It seems suspiciously similar to a SPI chip select, and then that is something that should be handled by the SPI core and set as cs-gpios in the device tree instead. It is certainly used exactly like a chip select as far as I can tell. Yours, Linus Walleij
Hi, Here's a V2 of my patchset that attempts to clean up the current situation with DSI/DBI panels drivers, and tinyDRM. For the record, here is a small sum-up of the current situation: - the current MIPI DBI code (drivers/gpu/drm/drm_mipi_dbi.c) is lagging way behind the MIPI DSI code (drivers/gpu/drm/drm_mipi_dsi.c). While the DSI code adds a proper bus support, with support for host drivers and client devices, there is no such thing with the DBI code. As such, it is currently impossible to write a standard DRM panel driver for a DBI panel. - Even if the MIPI DBI code was updated with a proper bus, many panels and MIPI controllers support both DSI and DBI, so it would be a pain to support them without resolving to duplicating each driver. - The panel drivers written against the DBI code are all "tinyDRM" drivers, which means that they will register a full yet simple DRM driver, and cannot be used as regular DRM panels for a different DRM driver. - These "tinyDRM" drivers all use SPI directly, even though the panels they're driving can work on other interfaces (e.g. i8080 bus). Which means that one driver written for e.g. a ILI9341 would not work if the control interface is not SPI. - The "tinyDRM" common code is entangled with DBI and there is no clear separation between the two. It could very well be moved to a single "tinyDRM" driver that works with a DRM panel obtained from devicetree, because the only requirement is that the panel supports a few given DCS commands. Noteworthy changes since V1: * The DT binding document for the NV3052C panel has been updated with the feedback I got from V1. It now supports multiple power supplies. * Instead of using macros to define bus types, we now have an enum mipi_dcs_bus_type. * The WARN_ONE_ONCE() that were in place to check that the host and client drivers provided the DCS bus bitmask is gone, we just default to DSI instead. * DBI/SPI driver code was moved out of drivers/gpu/drm/bridge/. * The DBI/SPI driver is registered as a driver by each client if needed, they just call module_mpi_dbi_spi_driver(). This addresses the issue in V1 that compatible strings had to be added to two different places. * NV3052C and ILI9341 panel drivers were updated to remove custom backlight handling, call drm_panel_{disable,unprepare} on module exit, and various small fixes. For a more detailed changelog, see the header of each individual patch. Paul Cercueil (6): dt-bindings: display: Document NewVision NV3052C DT node drm: dsi: Let host and device specify supported bus drm: Add SPI DBI host driver drm/tiny: Add TinyDRM for DSI/DBI panels drm/panel: Add panel driver for NewVision NV3052C based LCDs drm/panel: Add Ilitek ILI9341 DBI panel driver .../display/panel/newvision,nv3052c.yaml | 100 ++++ drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_mipi_dbi_spi.c | 247 +++++++++ drivers/gpu/drm/drm_mipi_dsi.c | 9 + drivers/gpu/drm/panel/Kconfig | 18 + drivers/gpu/drm/panel/Makefile | 2 + drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 318 +++++++++++ .../gpu/drm/panel/panel-newvision-nv3052c.c | 510 ++++++++++++++++++ drivers/gpu/drm/tiny/Kconfig | 8 + drivers/gpu/drm/tiny/Makefile | 1 + drivers/gpu/drm/tiny/tiny-dsi.c | 266 +++++++++ include/drm/drm_mipi_dbi_spi.h | 42 ++ include/drm/drm_mipi_dsi.h | 44 ++ 14 files changed, 1574 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/newvision,nv3052c.yaml create mode 100644 drivers/gpu/drm/drm_mipi_dbi_spi.c create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c create mode 100644 drivers/gpu/drm/panel/panel-newvision-nv3052c.c create mode 100644 drivers/gpu/drm/tiny/tiny-dsi.c create mode 100644 include/drm/drm_mipi_dbi_spi.h