Re: [PATCH v3 5/5] drm/panel: Add ilitek ili9341 driver
From: Linus Walleij <hidden>
Date: 2020-05-14 08:14:47
Also in:
dri-devel, linux-clk, linux-devicetree, lkml
Hi Dillon, thanks for your patch! Overall this looks like a good start. On Tue, May 12, 2020 at 9:04 AM [off-list ref] wrote:
#define ILI9341_SLEEP_OUT 0x11 /* Sleep out register */
This is not a register, also just use MIPI_DCS_EXIT_SLEEP_MODE in the code.
+#define ILI9341_DFC 0xb6 /* Display Function Control + * register + */
This commenting style doesn't work, either just put it after /* the define */ and don't care if the line gets a bit long and checkpatch complains, or /* * Put it above the define like this */ #define FOO 0x00
+/** + * struct ili9341_config - the system specific ILI9341 configuration
Nice with this per-system config, it makes the driver easy to maintain for new users.
+static int ili9341_dpi_init(struct ili9341 *ili)
+{
+ ili9341_command(ili, 0xca, 0xc3, 0x08, 0x50);This stuff is a bit hard to understand, don't you think? But given that register 0xCA seems undocumented I don't know if there is anything more you can do, so it is OK I suppose.
+ ili9341_command(ili, ILI9341_POWERB, 0x00, 0xc1, 0x30);
This command is described in the manual page 196. Version: V1.11 Document No.: ILI9341_DS_V1.11.pdf https://dflund.se/~triad/ILI9341_v1.11.pdf And this goes for all the below commands. Please add some more defines from the datasheet and have less magic numbers in the driver.
+ ili9341_command(ili, ILI9341_POWER_SEQ, 0x64, 0x03, 0x12, 0x81); + ili9341_command(ili, ILI9341_DTCA, 0x85, 0x00, 0x78); + ili9341_command(ili, ILI9341_POWERA, 0x39, 0x2c, 0x00, 0x34, 0x02); + ili9341_command(ili, ILI9341_PRC, 0x20); + ili9341_command(ili, ILI9341_DTCB, 0x00, 0x00); + ili9341_command(ili, ILI9341_FRC, 0x00, 0x1b); + ili9341_command(ili, ILI9341_DFC, 0x0a, 0xa2); + ili9341_command(ili, ILI9341_POWER1, 0x10); + ili9341_command(ili, ILI9341_POWER2, 0x10); + ili9341_command(ili, ILI9341_VCOM1, 0x45, 0x15); + ili9341_command(ili, ILI9341_VCOM2, 0x90); + ili9341_command(ili, ILI9341_MAC, 0xc8); + ili9341_command(ili, ILI9341_3GAMMA_EN, 0x00); + ili9341_command(ili, ILI9341_RGB_INTERFACE, 0xc2); + ili9341_command(ili, ILI9341_DFC, 0x0a, 0xa7, 0x27, 0x04); + ili9341_command(ili, ILI9341_COLUMN_ADDR, 0x00, 0x00, 0x00, 0xef); + ili9341_command(ili, ILI9341_PAGE_ADDR, 0x00, 0x00, 0x01, 0x3f); + ili9341_command(ili, ILI9341_INTERFACE, 0x01, 0x00, 0x06); + if (ili->input == ILI9341_INPUT_PRGB_18_BITS) + ili9341_command(ili, ILI9341_PIXEL_FORMAT, 0x66); + else + ili9341_command(ili, ILI9341_PIXEL_FORMAT, 0x56); + ili9341_command(ili, ILI9341_GRAM); + msleep(200);
I think some of the above should not be hardcoded but should instead be stored in fields in struct ili9341_config. I know it can be a bit tedious but it makes things much more clear.
+ ili9341_command(ili, ILI9341_GAMMA, 0x01); + ili9341_command(ili, ILI9341_PGAMMA, 0x0f, 0x29, 0x24, 0x0c, 0x0e, + 0x09, 0x4e, 0x78, 0x3c, 0x09, + 0x13, 0x05, 0x17, 0x11, 0x00); + ili9341_command(ili, ILI9341_NGAMMA, 0x00, 0x16, 0x1b, 0x04, 0x11, + 0x07, 0x31, 0x33, 0x42, 0x05, + 0x0c, 0x0a, 0x28, 0x2f, 0x0f);
This should definately be in ili9341_config, as it is a screen property. In the long run I would like these panels to support setting gamma from userspace etc but it is a big tedious work to get that right so hard-coding a default per-variant is fine. You can check in e.g. panel-novatek-nt35510.c how I encoded such sequences in per-variant data.
+static int ili9341_dpi_power_off(struct ili9341 *ili)
+{
+ /* Disable power */
+ if (!IS_ERR(ili->vcc))
+ return regulator_disable(ili->vcc);
+
+ return 0;
+}Usually you should also assert RESET when disabling power.
+/* This is the only mode listed for parallel RGB in the datasheet */
+static const struct drm_display_mode rgb_240x320_mode = {
+ .clock = 6100,
+ .hdisplay = 240,
+ .hsync_start = 240 + 10,
+ .hsync_end = 240 + 10 + 10,
+ .htotal = 240 + 10 + 10 + 20,
+ .vdisplay = 320,
+ .vsync_start = 320 + 4,
+ .vsync_end = 320 + 4 + 2,
+ .vtotal = 320 + 4 + 2 + 2,
+ .vrefresh = 60,
+ .flags = 0,
+ .width_mm = 65,
+ .height_mm = 50,The width and height should certainly be om the ili9341_config as it is a per-panel property. You can just fill in in in the below .get_modes() function. Or assign the whole mode as part of the ili9341_config if that is easier.
+ return drm_panel_add(&ili->panel); +} + + +
Surplus whitespace here.
+ mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_OFF); + + mipi_dbi_command(dbi, ILI9341_POWERB, 0x00, 0xc1, 0x30); + mipi_dbi_command(dbi, ILI9341_POWER_SEQ, 0x64, 0x03, 0x12, 0x81);
Some of these are just copies of the above init sequence, so it makes even more sense to just have these settings stored in ili9341_config.
+ mipi_dbi_command(dbi, ILI9341_DTCA, 0x85, 0x00, 0x78); + mipi_dbi_command(dbi, ILI9341_POWERA, 0x39, 0x2c, 0x00, 0x34, 0x02); + mipi_dbi_command(dbi, ILI9341_PRC, 0x20); + mipi_dbi_command(dbi, ILI9341_DTCB, 0x00, 0x00); + + /* Power Control */ + mipi_dbi_command(dbi, ILI9341_POWER1, 0x23); + mipi_dbi_command(dbi, ILI9341_POWER2, 0x10); + /* VCOM */ + mipi_dbi_command(dbi, ILI9341_VCOM1, 0x3e, 0x28); + mipi_dbi_command(dbi, ILI9341_VCOM2, 0x86); + + /* Memory Access Control */ + mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, + MIPI_DCS_PIXEL_FMT_16BIT); + + /* Frame Rate */ + mipi_dbi_command(dbi, ILI9341_FRC, 0x00, 0x1b); + + /* Gamma */ + mipi_dbi_command(dbi, ILI9341_3GAMMA_EN, 0x00); + mipi_dbi_command(dbi, MIPI_DCS_SET_GAMMA_CURVE, 0x01); + mipi_dbi_command(dbi, ILI9341_PGAMMA, + 0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1, + 0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00); + mipi_dbi_command(dbi, ILI9341_NGAMMA, + 0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1, + 0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
It seems to be copies of the stuff above, but why is there a different gamma if you use DBI? I suspect only one of them is really needed and it is not even necessary to set if up in two places.
+out_enable:
+ switch (dbidev->rotation) {
+ default:
+ addr_mode = ILI9341_MADCTL_MX;
+ break;> +out_enable:
+ switch (dbidev->rotation) {
+ default:
+ addr_mode = ILI9341_MADCTL_MX;
+ break;
+ case 90:
+ addr_mode = ILI9341_MADCTL_MV;
+ break;
+ case 180:
+ addr_mode = ILI9341_MADCTL_MY;
+ break;
+ case 270:
+ addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
+ ILI9341_MADCTL_MX;
+ break;
+ }
+ addr_mode |= ILI9341_MADCTL_BGR;
+ mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+ mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
+ DRM_DEBUG_KMS("initialized display serial interface\n");
+out_exit:
+ drm_dev_exit(idx);
+}
++ case 90: + addr_mode = ILI9341_MADCTL_MV; + break; + case 180: + addr_mode = ILI9341_MADCTL_MY; + break; + case 270: + addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY | + ILI9341_MADCTL_MX; + break; + } + addr_mode |= ILI9341_MADCTL_BGR; + mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
Since you use MIPI_DCS_* define here, check if this applies to more of the commands above so you don't need custom defines for them. e.g. ILI9341_SLEEP_OUT 0x11 = MIPI_DCS_EXIT_SLEEP_MODE and that isn't even a register right, it is just a command? (Noted in the beginning.) Yours, Linus Walleij _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel