Re: [PATCH v3 5/5] drm/panel: Add ilitek ili9341 driver
Hi Linus, Linus Walleij 于2020年5月14日周四 下午10:08写道： > > On Thu, May 14, 2020 at 12:22 PM dillon min wrote: > > > > > + /* 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? > > > for dbi interface, currently i just copy the code from tiny/ili9341.c. > > as so many boards use this driver now, like raspberry pi, etc > > i'm afraid it's will not work after modification. so, just leave the > > original code there. > > OK if you move it to ili9341_config it will be clear which panels > need this gamma and which panels need another gamma. > > I think there should be one ili9341_config for the new st,* > variant and one for the old DBI variant. > Okay, it's a good idea to setup two different configs. ili9341_dbi_config, ili9341_dpi_config. first for old dbi variant, second for dpi variant. > > anther question: > > is there any panel driver have dbi & dpi or dpi & dsi supported? which > > i mean support > > two different panel interface in one driver. > > thanks > > Usually you split the driver in three files becuase a driver can > only list one initcall, and also it makes it modularized. > > There is nothing in-tree but look at my branch here: > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/log/?h=ux500-skomer-v5.7-rc1 > > You see how I split up the s6e63m0 driver in one SPI part > and one DSI part: > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/commit/?h=ux500-skomer-v5.7-rc1=6c0e1fb5df5fa8fa857dee65f6c0f54d06d158a7 > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/commit/?h=ux500-skomer-v5.7-rc1=cfbf562cc709b53b62f5fbc7fedf176ffa17b088 > > The overall idea should work the same with DBI. > > Yours, > Linus Walleij > > Yours, > Linus Walleij Thanks for share, but i thinks it's not suitable to my case. the difference is for panel s6e63m0 1 spi only for panel register configuration 2 dpi for rgb video data transfer (drm_panel_init(..., DRM_MODE_CONNECTOR_DPI);) my case: 1 support ili9341 by drm mipi dbi (only spi bus, "3/4 line serial interface", pdf chap 7.6.1), Ie, raspberry pi + ili9341 + spi 2 support ili9341 by spi & dpi (spi for register set, dpi for rgb data, "6/16/18 bit parallel rgb interface", pdf chap 7.6.8) Ie, stm32f429+ili9341+ltdc+spi these two communication type has no dependency with each other. DBI has much more difference than DPI & SPI, or DSI & SPI in drm. drm_mipi_dbi support both panel register configuration and video data transfer via spi with or without dc pins. also the registration to drm is difference, for dbi is mipi_dbi_dev_init and drm_fbdev_generic_setup for panel driver is drm_panel_init and drm_panel_add at soc's view, we can drive ili9341 to work just by drm mipi dbi (actually it's dbi -> spi bus) without host's lcd or dsi controller but for some panel can't transfer video data by spi, and with dpi or dsi interface, must connect to host's lcd or dsi controller. so, for ilitek-ili9341 , it's use different dts binding drive to register different client type (dbi, or dpi & spi) to drm. actually, there is a driver tiny/ili9341.c support this panel by spi bus only already. but, from Sam's suggestion, should extend this driver to support dpi & spi interface, this is the background of panel-ilitek-ili9341.c thanks. best regards. dillon ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 5/5] drm/panel: Add ilitek ili9341 driver
Hi Linus,
Thanks for reviewing.
On Thu, May 14, 2020 at 4:14 PM Linus Walleij wrote:
>
> Hi Dillon,
>
> thanks for your patch! Overall this looks like a good start.
>
> On Tue, May 12, 2020 at 9:04 AM wrote:
>
> > #define ILI9341_SLEEP_OUT0x11 /* Sleep out register */
>
> This is not a register, also just use MIPI_DCS_EXIT_SLEEP_MODE
> in the code.
>
Yes, i will try to reuse MIPI_DCS_xxx.
> > +#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
>
Ok, will change this comments.
> > +/**
> > + * struct ili9341_config - the system specific ILI9341 configuration
>
> Nice with this per-system config, it makes the driver easy to maintain
> for new users.
>
Yes, will try to move more system related configurations to this part,
instead of hard code.
> > +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
>
Yes, "ili9341_command(ili, 0xca, 0xc3, 0x08, 0x50);" i ported from st's sdk.
will use ILI9341_XXX to replace these magic numbers
> 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 hard coded 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.
>
Ok, will go deeper to find out some register configuration move to
system config like rgb bus 16/18 bits
> > + 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.
>
ok, will refer to panel-ilitek-ili9322 and panel-novatek-nt35510 driver.
> 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.
>
ok
> > +/* This is
Re: [PATCH v3 5/5] drm/panel: Add ilitek ili9341 driver
On Thu, May 14, 2020 at 12:22 PM dillon min wrote: > > > + /* 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? > for dbi interface, currently i just copy the code from tiny/ili9341.c. > as so many boards use this driver now, like raspberry pi, etc > i'm afraid it's will not work after modification. so, just leave the > original code there. OK if you move it to ili9341_config it will be clear which panels need this gamma and which panels need another gamma. I think there should be one ili9341_config for the new st,* variant and one for the old DBI variant. > anther question: > is there any panel driver have dbi & dpi or dpi & dsi supported? which > i mean support > two different panel interface in one driver. > thanks Usually you split the driver in three files becuase a driver can only list one initcall, and also it makes it modularized. There is nothing in-tree but look at my branch here: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/log/?h=ux500-skomer-v5.7-rc1 You see how I split up the s6e63m0 driver in one SPI part and one DSI part: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/commit/?h=ux500-skomer-v5.7-rc1=6c0e1fb5df5fa8fa857dee65f6c0f54d06d158a7 https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/commit/?h=ux500-skomer-v5.7-rc1=cfbf562cc709b53b62f5fbc7fedf176ffa17b088 The overall idea should work the same with DBI. Yours, Linus Walleij Yours, Linus Walleij ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 5/5] drm/panel: Add ilitek ili9341 driver
Hi Dillon,
thanks for your patch! Overall this looks like a good start.
On Tue, May 12, 2020 at 9:04 AM wrote:
> #define ILI9341_SLEEP_OUT0x11 /* 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
[PATCH v3 5/5] drm/panel: Add ilitek ili9341 driver
From: dillon min Currently, we can use tinydrm ili9341 driver to drive ili9341 panel by spi interface (both register configuration and video) ili9341 have parallel and mcu interface as well, we extend the support to parallel rgb interface with DRM_MODE_CONNECTOR_DPI this driver can work as parallel rgb or serial spi mode by different dts binding. for serial spi interface dts binding configuration, refer to: Documentation/devicetree/bindings/display/ilitek,ili9341.txt Signed-off-by: dillon min --- Changes: V3: 1 Add support for original dts binding "adafruit,yx240qv29", merged from tiny/ili9341.c, which is serial spi interface for register configuration and video trasfer. 2 change the dts binding to st,sf-tc240t-9370-t for parallel rgb interface. V2: none V1: Add support for parallel rgb interface drivers/gpu/drm/panel/Kconfig| 12 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 700 +++ 3 files changed, 713 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index a1723c1..c938bee 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -95,6 +95,18 @@ config DRM_PANEL_ILITEK_IL9322 Say Y here if you want to enable support for Ilitek IL9322 QVGA (320x240) RGB, YUV and ITU-T BT.656 panels. +config DRM_PANEL_ILITEK_ILI9341 + tristate "Ilitek ILI9341 240x320 QVGA panels" + depends on OF && SPI + depends on DRM_KMS_HELPER + depends on DRM_KMS_CMA_HELPER + depends on BACKLIGHT_CLASS_DEVICE + select DRM_MIPI_DBI + help + Say Y here if you want to enable support for Ilitek IL9341 + QVGA (240x320) RGB panels. support serial & parallel rgb + interface. + config DRM_PANEL_ILITEK_ILI9881C tristate "Ilitek ILI9881C-based panels" depends on OF diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 96a883c..16947d7 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PANEL_ELIDA_KD35T133) += panel-elida-kd35t133.o obj-$(CONFIG_DRM_PANEL_FEIXIN_K101_IM2BA02) += panel-feixin-k101-im2ba02.o obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += panel-feiyang-fy07024di26a30d.o obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o +obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9341) += panel-ilitek-ili9341.o obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c new file mode 100644 index 000..17339db --- /dev/null +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c @@ -0,0 +1,700 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Ilitek ILI9341 TFT LCD drm_panel driver. + * + * This panel can be configured to support: + * - 16-bit parallel RGB interface + * - 18-bit parallel RGB interface + * + * Copyright (C) 2020 Dillon Min + * Derived from drivers/drm/gpu/panel/panel-ilitek-ili9322.c + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#define ILI9341_SLEEP_OUT0x11 /* Sleep out register */ +#define ILI9341_GAMMA0x26 /* Gamma register */ +#define ILI9341_DISPLAY_OFF 0x28 /* Display off register */ +#define ILI9341_DISPLAY_ON 0x29 /* Display on register */ +#define ILI9341_COLUMN_ADDR 0x2a /* Colomn address register */ +#define ILI9341_PAGE_ADDR0x2b /* Page address register */ +#define ILI9341_GRAM 0x2c /* GRAM register */ +#define ILI9341_MAC 0x36 /* Memory Access Control register*/ +#define ILI9341_PIXEL_FORMAT 0x3A /* Pixel Format register */ +#define ILI9341_RGB_INTERFACE0xb0 /* RGB Interface Signal Control */ +#define ILI9341_FRC 0xb1 /* Frame Rate Control register */ +#define ILI9341_DFC 0xb6 /* Display Function Control +* register +*/ +#define ILI9341_POWER1 0xc0 /* Power Control 1 register */ +#define ILI9341_POWER2 0xc1 /* Power Control 2 register */ +#define ILI9341_VCOM10xc5 /* VCOM Control 1 register */ +#define ILI9341_VCOM20xc7 /* VCOM Control 2 register */ +#define ILI9341_POWERA 0xcb /* Power control A register */ +#define ILI9341_POWERB 0xcf /* Power control B register */ +#define ILI9341_PGAMMA
