Re: [PATCH v3 5/5] drm/panel: Add ilitek ili9341 driver
From: Linus Walleij
Date: Thu May 14 2020 - 04:14:48 EST
Hi Dillon,
thanks for your patch! Overall this looks like a good start.
On Tue, May 12, 2020 at 9:04 AM <dillon.minfei@xxxxxxxxx> 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