Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/11/2017 03:18 PM, Noralf Trønnes wrote:

Den 10.12.2017 23.10, skrev David Lechner:
This adds a new driver for Sitronix ST7735R display panels.

This has been tested using an Adafruit 1.8" TFT.

Signed-off-by: David Lechner <david@xxxxxxxxxxxxxx>
---

+static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
+                struct drm_crtc_state *crtc_state)
+{
+    struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+    struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+    struct device *dev = tdev->drm->dev;
+    int ret;
+    u8 addr_mode;
+
+    DRM_DEBUG_KMS("\n");
+
+    mipi_dbi_hw_reset(mipi);
+
+    ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+    if (ret) {
+        DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+        return;
+    }
+
+    msleep(150);
+
+    mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+    msleep(500);
+
+    mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
+    mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
+    mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 0x2c,
+             0x2d);
+    mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
+    mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
+    mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
+    mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
+    mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
+    mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
+    mipi_dbi_command(mipi, ST7735R_VMCTR1, 0x0e);
+    mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
+    switch (mipi->rotation) {
+    default:
+        addr_mode = ST7735R_MX | ST7735R_MY;
+        break;
+    case 90:
+        addr_mode = ST7735R_MX | ST7735R_MV;
+        break;
+    case 180:
+        addr_mode = 0;
+        break;
+    case 270:
+        addr_mode = ST7735R_MY | ST7735R_MV;
+        break;
+    }
+    mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+    mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT,
+             MIPI_DCS_PIXEL_FMT_16BIT);
+    mipi_dbi_command(mipi, ST7735R_GAMCTRP1, 0x0f, 0x1a, 0x0f, 0x18, 0x2f,
+             0x28, 0x20, 0x22, 0x1f, 0x1b, 0x23, 0x37, 0x00, 0x07,
+             0x02, 0x10);
+    mipi_dbi_command(mipi, ST7735R_GAMCTRN1, 0x0f, 0x1b, 0x0f, 0x17, 0x33,
+             0x2c, 0x29, 0x2e, 0x30, 0x30, 0x39, 0x3f, 0x00, 0x07,
+             0x03, 0x10);
+    mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+
+    msleep(100);
+
+    mipi_dbi_command(mipi, MIPI_DCS_ENTER_NORMAL_MODE);
+
+    msleep(20);
+
+    mipi_dbi_pipe_enable(pipe, crtc_state);
+}
+
+static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
+    .enable        = st7735r_pipe_enable,
+    .disable    = mipi_dbi_pipe_disable,
+    .update        = tinydrm_display_pipe_update,
+    .prepare_fb    = tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode st7735r_mode = {
+    TINYDRM_MODE(128, 160, 28, 35),
+};

This naming talk has made me realise that these should be named after
the panel since they're not generic controller functions.
Maybe the mode can be generic, not sure if the physical size can/will
differ, the resolution is very likely to be the same for all.

Adafruit has a 1.44" 128x128px display [1] with the same controller, so the answer is no, the mode cannot be generic.

[1]: https://www.adafruit.com/product/2088

What's your view on my descision to split the MIPI DBI compatible
controllers into per controller drivers instead of having one driver
that supports them all?

My first impression was that I didn't like it so much. But now, I actually think that it is a good idea. I think it is a good compromise between some boilerplate code in every driver and a driver with so many quirks that it is very difficult to work on. It may make sense to combine some very similar controllers, but as a rule of thumb, I think one driver per controller will work nicely.

I've been afraid that it will be a very big file with macro definitions
per controller and enable functions per panel.

This driver has 15 macros and ~80 panel specific lines.
With one panel supported, half the driver is boilerplate.
The mi0283qt panel (MIPI DBI compatible) is in the same ballpark.

Adding helpers for panel reset/exit_sleep, for MIPI_DCS_SET_ADDRESS_MODE
and for display_on/enter_normal/flush could cut it down some more if we
made one driver to rule them all. Maybe the enable function per panel
could be cut in half that way.

I'm starting to wonder if one driver isn't such a bad solution after all...

I think as we add more drivers, the parts that get duplicated will become obvious candidates for helper functions to refactor, but I don't think trying to cram everything all into one driver is such a good idea.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux