Add ilitek ili9341 panel driver
Subsystems (4)
cover Add ilitek ili9341 panel driver
From: Dillon Min <[email protected]>
Since the st,sf-tc240t-9370-t dts binding already exist in stm32f429-disco.dts but, the panel driver didn't get accepted from mainline. it's time to submit patch fot it.
This driver can support two different interface by different dts bindings:
- spi+dpi, use spi to configure register, dpi for graphic data. st,sf-tc240t-9370-t
- only spi, just like tiny/ili9341.c (actually, this part is copy from tiny) adafruit,yx240qv29
I submited the first patch last year, you can find it at [1]. this patch has one major difference from that one, which is replace the low level communication way, from spi_sync() to mipi_dbi_{command, command_stackbuf}() interface, referred from Linus's patch [2].
both the two dpi/dbi interface was tested on stm32f429-disco board, if anyone want to verify this patch, you need apply the clk patch for this board first, you can get it from [3]
[1] "drm/panel: Add ilitek ili9341 panel driver"
https://lore.kernel.org/lkml/[email protected]/
[2] "drm/panel: s6e63m0: Switch to DBI abstraction for SPI"
https://lore.kernel.org/dri-devel/[email protected]/
[3]
https://lore.kernel.org/lkml/[email protected]/
Dillon Min (2):
dt-bindings: display: panel: Add ilitek ili9341 panel bindings drm/panel: Add ilitek ili9341 panel driver
.../bindings/display/panel/ilitek,ili9341.yaml | 69 ++ drivers/gpu/drm/panel/Kconfig | 12 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 778 +++++++++++++++++++++ 4 files changed, 860 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c
Signature · 1 line
2.7.4
PATCH 1/2 dt-bindings: display: panel: Add ilitek ili9341 panel bindings
From: Dillon Min <[email protected]>
Add documentation for "ilitek,ili9341" panel.
--- .../bindings/display/panel/ilitek,ili9341.yaml | 69 ++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml new file +69 -0
@@ -0,0 +1,69 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/panel/ilitek,ili9341.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Ilitek-9341 Display Panel + +maintainers: + - Dillon Min <[email protected]> + +description: | + Ilitek ILI9341 TFT panel driver with SPI control bus + This is a driver for 320x240 TFT panels, accepting a rgb input + streams with 16 bits or 18 bits. + +allOf: + - $ref: panel-common.yaml# + +properties: + compatible: + items: + - enum: + # ili9341 240*320 Color on stm32f429-disco board + - st,sf-tc240t-9370-t + - const: ilitek,ili9341 + + reg: true + + dc-gpios: + maxItems: 1 + description: Display data/command selection (D/CX) + + spi-3wire: true + + spi-max-frequency: + const: 10000000 + + port: true + +additionalProperties: false + +required: + - compatible + - reg + - dc-gpios + - port + +examples: + - |+ + spi { + #address-cells = <1>; + #size-cells = <0>; + panel: display@0 { + compatible = "st,sf-tc240t-9370-t", + "ilitek,ili9341"; + reg = <0>; + spi-3wire; + spi-max-frequency = <10000000>; + dc-gpios = <&gpiod 13 0>; + port { + panel_in: endpoint { + remote-endpoint = <&display_out>; + }; + }; + }; + }; +... +
Signature · 1 line
2.7.4
Re: [PATCH 1/2] dt-bindings: display: panel: Add ilitek ili9341 panel bindings
Hi Dillon,
thanks for your patch!
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml line 32
Quoted 9 lines · On Fri, Jul 16, 2021 at 12:20 PM <[email protected]> wrote:
From: Dillon Min <[email protected]>
Add documentation for "ilitek,ili9341" panel.
Signed-off-by Dillon Min <[email protected]>Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml new file +3
-0+ dc-gpios: + maxItems: 1 + description: Display data/command selection (D/CX)
This is a DBI feature so mention in the description that this is a DBI panel.
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml line 41
Quoted 8 lines
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml new file +8
-0+ spi-3wire: true + + spi-max-frequency: + const: 10000000 + + port: true + +additionalProperties: false
Please add regulator supplies for the power lines, it's fine not to implement code handling them in the driver but they should be in the bindings.
For the ili9341 it should be
vci-supply:
description: Analog voltage supply (2.5 .. 3.3V)
vddi-supply:
description: Voltage supply for interface logic (1.65 .. 3.3 V)
vddi-led-supply:
description: Voltage supply for the LED driver (1.65 .. 3.3 V)
Yours,
Linus Walleij
Re: [PATCH 1/2] dt-bindings: display: panel: Add ilitek ili9341 panel bindings
Hi Linus,
Thanks for your detailed reply.
Quoted 10 lines · Linus Walleij
Hi Dillon,
thanks for your patch!
Quoted 4 lines · On Fri, Jul 16, 2021 at 12:20 PM <[email protected]> wrote:
From: Dillon Min <[email protected]>
Add documentation for "ilitek,ili9341" panel.
Quoted 9 lines
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml line 32
Quoted 5 lines
Signed-off-by Dillon Min <[email protected]>Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml new file +3
-0+ dc-gpios: + maxItems: 1 + description: Display data/command selection (D/CX)This is a DBI feature so mention in the description that this is a DBI panel.
Okay, I will add the DBI panel to v2.
Quoted 16 lines
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml line 41
Quoted 9 lines
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml new file +8
-0+ spi-3wire: true + + spi-max-frequency: + const: 10000000 + + port: true + +additionalProperties: falsePlease add regulator supplies for the power lines, it's fine not to implement code handling them in the driver but they should be in the bindings.
For the ili9341 it should be
Quoted 10 lines
vci-supply:
description: Analog voltage supply (2.5 .. 3.3V)vddi-supply:
description: Voltage supply for interface logic (1.65 .. 3.3 V)vddi-led-supply:
description: Voltage supply for the LED driver (1.65 .. 3.3 V)
Thanks so much, I will add it to v2.
Quoted 3 lines
Yours,
Linus Walleij
PATCH 1/2 dt-bindings: display: panel: Add ilitek ili9341 panel bindings
Hi Linus,
Thanks for your detailed reply.
Quoted 11 lines · Linus Walleij
Hi Dillon,
thanks for your patch!
Quoted 4 lines · On Fri, Jul 16, 2021 at 12:20 PM <[email protected]> wrote:
From: Dillon Min <[email protected]>
Add documentation for "ilitek,ili9341" panel.
Quoted 8 lines
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml line 32
Quoted 5 lines
Signed-off-by Dillon Min <[email protected]>Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml new file +3
-0+ dc-gpios: + maxItems: 1 + description: Display data/command selection (D/CX)This is a DBI feature so mention in the description that this is a DBI panel.
Okay, I will add the DBI panel to v2.
Quoted 17 lines
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml line 41
Quoted 10 lines
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml new file +8
-0+ spi-3wire: true + + spi-max-frequency: + const: 10000000 + + port: true + +additionalProperties: falsePlease add regulator supplies for the power lines, it's fine not to implement code handling them in the driver but they should be in the bindings.
For the ili9341 it should be
Quoted 9 lines
vci-supply:
description: Analog voltage supply (2.5 .. 3.3V)vddi-supply:
description: Voltage supply for interface logic (1.65 .. 3.3 V)vddi-led-supply:
description: Voltage supply for the LED driver (1.65 .. 3.3 V)
Thanks so much, I will add it to v2.
Quoted 3 lines
Yours,
Linus Walleij
kernel test robot
build botRe: [PATCH 1/2] dt-bindings: display: panel: Add ilitek ili9341 panel bindings
In reply to [email protected]Thank you for the patch! Perhaps something to improve:
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.14-rc2 next-20210719] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/dillon-minfei-gmail-com/Add-ilitek-ili9341-panel-driver/20210718-103113 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next compiler: arm-linux-gnueabi-gcc (GCC) 10.3.0 reproduce: make ARCH=arm dtbs_check
If you fix the issue, kindly add following tag as appropriate
dtcheck warnings: (new ones prefixed by >>)
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /interrupt-controller(a)e000e100: failed to match any schema with compatible: ['arm,armv7m-nvic'] arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /timer(a)e000e010: failed to match any schema with compatible: ['arm,armv7m-systick'] arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/i2c(a)40005c00/stmpe811(a)41: failed to match any schema with compatible: ['st,stmpe811'] arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/i2c(a)40005c00/stmpe811(a)41/stmpe_touchscreen: failed to match any schema with compatible: ['st,stmpe-ts'] arch/arm/boot/dts/stm32f429-disco.dt.yaml: l3gd20(a)0: 'spi-max-frequency' does not match any of the regexes: 'pinctrl-[0-9]+'
From schema: Documentation/devicetree/bindings/iio/st,st-sensors.yaml
>> arch/arm/boot/dts/stm32f429-disco.dt.yaml: display(a)1: compatible: ['st,sf-tc240t-9370-t'] is too short
From schema: Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/crc(a)40023000: failed to match any schema with compatible: ['st,stm32f4-crc'] arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/rcc(a)40023800: failed to match any schema with compatible: ['st,stm32f42xx-rcc', 'st,stm32-rcc'] arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/rcc(a)40023800: failed to match any schema with compatible: ['st,stm32f42xx-rcc', 'st,stm32-rcc']
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Re: [PATCH 1/2] dt-bindings: display: panel: Add ilitek ili9341 panel bindings
Hi Kernel test robot,
Thanks for your time to test my patch.
I did the dtbs_check on my side, and got the same warnings.
But I guess these warnings do not come from this submission, So, I will not be going to fix it at this time, but will at other submissions
to fix these, add arm,armv7m-nvic.yaml ... to describe compatibles, etc.
by the way, i did find a warning from the stm32f429-disco.dts and fixt it in "[PATCH v2 2/3] ARM: dts: stm32: fix dtbs_check warning on ili9341 dts binding",
the warning:arch/arm/boot/dts/stm32f429-disco.dt.yaml: display(a)1: compatible:
['st,sf-tc240t-9370-t'] is too short
my make command:
make ARCH=arm dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml
Best regards
thanks
Dillon
Quoted 53 lines · kernel test robot
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.14-rc2 next-20210719] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]url:
https://github.com/0day-ci/linux/commits/dillon-minfei-gmail-com/Add-ilitek-ili9341-panel-driver/20210718-103113
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
for-next
compiler: arm-linux-gnueabi-gcc (GCC) 10.3.0
reproduce: make ARCH=armdtbs_checkIf you fix the issue, kindly add following tag as appropriate
Reported-by kernel test robot <[email protected]>dtcheck warnings: (new ones prefixed by >>)
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0:
/interrupt-controller(a)e000e100: failed to match any schema with
compatible: ['arm,armv7m-nvic']
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /timer(a)e000e010: failed
to match any schema with compatible: ['arm,armv7m-systick']
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/i2c(a)40005c00
/stmpe811(a)41: failed to match any schema with compatible: ['st,stmpe811']
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/i2c(a)40005c00
/stmpe811(a)41/stmpe_touchscreen: failed to match any schema with
compatible: ['st,stmpe-ts']
arch/arm/boot/dts/stm32f429-disco.dt.yaml: l3gd20(a)0:
'spi-max-frequency' does not match any of the regexes: 'pinctrl-[0-9]+'
From schema:Documentation/devicetree/bindings/iio/st,st-sensors.yamlQuoted 1 line
Quoted 1 line
arch/arm/boot/dts/stm32f429-disco.dt.yaml: display(a)1: compatible:['st,sf-tc240t-9370-t'] is too short
From schema:Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/crc(a)40023000:
failed to match any schema with compatible: ['st,stm32f4-crc']
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/rcc(a)40023800:
failed to match any schema with compatible: ['st,stm32f42xx-rcc', 'st,stm32-rcc']
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/rcc(a)40023800:
failed to match any schema with compatible: ['st,stm32f42xx-rcc', 'st,stm32-rcc']---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
PATCH 1/2 dt-bindings: display: panel: Add ilitek ili9341 panel bindings
Hi Kernel test robot,
Thanks for your time to test my patch.
I did the dtbs_check on my side, and got the same warnings.
But I guess these warnings do not come from this submission,
So, I will not be going to fix it at this time, but will at other submissions to fix these, add arm,armv7m-nvic.yaml ... to describe compatibles, etc.
by the way, i did find a warning from the stm32f429-disco.dts and fixt it in "[PATCH v2 2/3] ARM: dts: stm32: fix dtbs_check warning on ili9341 dts binding",
the warning:arch/arm/boot/dts/stm32f429-disco.dt.yaml: display@1: compatible:
['st,sf-tc240t-9370-t'] is too short
my make command:
make ARCH=arm dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml
Best regards
thanks
Dillon
Quoted 36 lines · kernel test robot
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.14-rc2 next-20210719] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]url: https://github.com/0day-ci/linux/commits/dillon-minfei-gmail-com/Add-ilitek-ili9341-panel-driver/20210718-103113 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next compiler: arm-linux-gnueabi-gcc (GCC) 10.3.0 reproduce: make ARCH=arm dtbs_checkIf you fix the issue, kindly add following tag as appropriate
Reported-by kernel test robot <[email protected]>dtcheck warnings: (new ones prefixed by >>)
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /interrupt-controller@e000e100: failed to match any schema with compatible: ['arm,armv7m-nvic']arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /timer@e000e010: failed to match any schema with compatible: ['arm,armv7m-systick']arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/i2c@40005c00/stmpe811@41: failed to match any schema with compatible: ['st,stmpe811']arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/i2c@40005c00/stmpe811@41/stmpe_touchscreen: failed to match any schema with compatible: ['st,stmpe-ts']arch/arm/boot/dts/stm32f429-disco.dt.yaml: l3gd20@0: 'spi-max-frequency' does not match any of the regexes: 'pinctrl-[0-9]+'
From schema:Documentation/devicetree/bindings/iio/st,st-sensors.yamlQuoted 1 line
Quoted 1 line
arch/arm/boot/dts/stm32f429-disco.dt.yaml: display@1: compatible: ['st,sf-tc240t-9370-t'] is too shortFrom schema:
Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml
arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/crc@40023000: failed to match any schema with compatible: ['st,stm32f4-crc']arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/rcc@40023800: failed to match any schema with compatible: ['st,stm32f42xx-rcc', 'st,stm32-rcc']arch/arm/boot/dts/stm32f429-disco.dt.yaml:0:0: /soc/rcc@40023800: failed to match any schema with compatible: ['st,stm32f42xx-rcc', 'st,stm32-rcc']---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
PATCH 2/2 drm/panel: Add ilitek ili9341 panel driver
In reply to [email protected]From: Dillon Min <[email protected]>
This driver combine tiny/ili9341.c mipi_dbi_interface driver with mipi_dpi_interface driver, can support ili9341 with serial mode or parallel rgb interface mode by register configuration.
--- drivers/gpu/drm/panel/Kconfig | 12 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 778 +++++++++++++++++++++++++++ 3 files changed, 791 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c
drivers/gpu/drm/panel/Kconfig +12 -0
@@ -124,6 +124,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
drivers/gpu/drm/panel/Makefile +1 -0
@@ -11,6 +11,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
drivers/gpu/drm/panel/panel-ilitek-ili9341.c new file +778 -0
778 lines not on this page · raw message
Signature · 1 line
2.7.4
Re: [PATCH 2/2] drm/panel: Add ilitek ili9341 panel driver
Hi Dillon,
thanks for your patch!
Quoted 7 lines · On Fri, Jul 16, 2021 at 12:20 PM <[email protected]> wrote:
From: Dillon Min <[email protected]>
This driver combine
tiny/ili9341.cmipi_dbi_interfacedriver withmipi_dpi_interfacedriver, can support ili9341 with serial mode or parallel rgb interface mode by register configuration.Signed-off-by Dillon Min <[email protected]>
Nice!
Quoted 5 lines
drivers/gpu/drm/panel/Kconfig +5
-0+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
(...)
drivers/gpu/drm/panel/panel-ilitek-ili9341.c line 29
Quoted 5 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c new file +5
-0+#include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_gem_cma_helper.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_gem_atomic_helper.h> +#include <drm/drm_atomic_helper.h>
Hm now there is a (partial) KMS driver in the panel driver, kinda, sorta. Is this the right split? I'm not the best with DRM infrastructure, just asking.
drivers/gpu/drm/panel/panel-ilitek-ili9341.c line 118
Quoted 6 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c new file +6
-0+struct ili9341_config { +u32 max_spi_speed; +/** @mode: the drm display mode */ +const struct drm_display_mode mode; +/* @ca: TODO: need comments for this register */ +u8 ca[ILI9341_CA_LEN ];
These are all in the datasheet but I guess you plan to add these TODOs later. (It's fine I suppose, the driver is already very nice.)
Quoted 1 line
+ struct regulator *vcc;
Use the right name of the pin for the regulator. I guess this is actually vci. I would implement all three regulators and get them as bulk. See e.g. drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
for an example on how to get and enable several regulators using bulk.
The regulator framework will provide dummy regulators if you didn't define some of them so it is fine to just provide one or two.
Yours,
Linus Walleij
Re: [PATCH 2/2] drm/panel: Add ilitek ili9341 panel driver
Hi Linus,
Thanks for your detailed review.
Quoted 22 lines · Linus Walleij
Hi Dillon,
thanks for your patch!
Quoted 7 lines · On Fri, Jul 16, 2021 at 12:20 PM <[email protected]> wrote:
From: Dillon Min <[email protected]>
This driver combine
tiny/ili9341.cmipi_dbi_interfacedriver withmipi_dpi_interfacedriver, can support ili9341 with serial mode or parallel rgb interface mode by register configuration.Signed-off-by Dillon Min <[email protected]>Nice!
Quoted 5 lines
drivers/gpu/drm/panel/Kconfig +5
-0+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(...)
Quoted 10 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c line 29
Quoted 5 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c new file +5
-0+#include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_gem_cma_helper.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_gem_atomic_helper.h> +#include <drm/drm_atomic_helper.h> Hm now there is a (partial) KMS driver in the panel driver, kinda, sorta. Is this the right split? I'm not the best with DRM infrastructure, just asking.
I tried to remove one of these two headers, but got compile errors:
linux/drivers/gpu/drm/panel/panel-ilitek-ili9341.c:719:3: error: implicit declaration of function 'drm_atomic_helper_shutdown' [-Werror =implicit-function-declaration]
719 | drm_atomic_helper_shutdown(drm);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~or
linux/drivers/gpu/drm/panel/panel-ilitek-ili9341.c:562:16: error:
'drm_gem_simple_display_pipe_prepare_fb' undeclared here (not in a function)
562 | .prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Actually, these two headers are merged from tiny/ili9341.c to support only- DBI interface, I'm
not sure whether the maintainers will ask me to remove (tiny/ili9341.c) code from this patch.
If so, I will remove these headers.
But, It's a little strange to support different interfaces from different drivers.
Quoted 10 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c line 118
Quoted 6 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c new file +6
-0+struct ili9341_config { +u32 max_spi_speed; +/** @mode: the drm display mode */ +const struct drm_display_mode mode; +/* @ca: TODO: need comments for this register */ +u8 ca[ILI9341_CA_LEN ];These are all in the datasheet but I guess you plan to add these TODOs later. (It's fine I suppose, the driver is already very nice.)
Yes, I didn't get detailed information about these registers from this panel datasheet, so leave TODOs here.
Quoted 12 lines
Quoted 1 line
+ struct regulator *vcc;
Use the right name of the pin for the regulator. I guess this is actually vci. I would implement all three regulators and get them as bulk. See e.g.
drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
for an example on how to get and enable several regulators using bulk.The regulator framework will provide dummy regulators if you didn't define some of them so it is fine to just provide one or two.
Really appreciate your suggestion, will add to v2.
Quoted 3 lines
Yours,
Linus Walleij
PATCH 2/2 drm/panel: Add ilitek ili9341 panel driver
Hi Linus,
Thanks for your detailed review.
Quoted 23 lines · Linus Walleij
Hi Dillon,
thanks for your patch!
Quoted 7 lines · On Fri, Jul 16, 2021 at 12:20 PM <[email protected]> wrote:
From: Dillon Min <[email protected]>
This driver combine
tiny/ili9341.cmipi_dbi_interfacedriver withmipi_dpi_interfacedriver, can support ili9341 with serial mode or parallel rgb interface mode by register configuration.Signed-off-by Dillon Min <[email protected]>Nice!
Quoted 5 lines
drivers/gpu/drm/panel/Kconfig +5
-0+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(...)
Quoted 9 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c line 29
Quoted 5 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c new file +5
-0+#include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_gem_cma_helper.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_gem_atomic_helper.h> +#include <drm/drm_atomic_helper.h> Hm now there is a (partial) KMS driver in the panel driver, kinda, sorta. Is this the right split? I'm not the best with DRM infrastructure, just asking.
I tried to remove one of these two headers, but got compile errors:
linux/drivers/gpu/drm/panel/panel-ilitek-ili9341.c:719:3: error:
implicit declaration of function 'drm_atomic_helper_shutdown' [-Werror=implicit-function-declaration]
719 | drm_atomic_helper_shutdown(drm);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~or
linux/drivers/gpu/drm/panel/panel-ilitek-ili9341.c:562:16: error:
'drm_gem_simple_display_pipe_prepare_fb' undeclared here (not in a function)
562 | .prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Actually, these two headers are merged from tiny/ili9341.c to support only-DBI interface, I'm
not sure whether the maintainers will ask me to remove (tiny/ili9341.c) code from this patch.
If so, I will remove these headers.
But, It's a little strange to support different interfaces from different drivers.
Quoted 10 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c line 118
Quoted 7 lines
drivers/gpu/drm/panel/panel-ilitek-ili9341.c new file +6
-0+struct ili9341_config { +u32 max_spi_speed; +/** @mode: the drm display mode */ +const struct drm_display_mode mode; +/* @ca: TODO: need comments for this register */ +u8 ca[ILI9341_CA_LEN ];These are all in the datasheet but I guess you plan to add these TODOs later. (It's fine I suppose, the driver is already very nice.)
Yes, I didn't get detailed information about these registers from this panel datasheet, so leave TODOs here.
Quoted 12 lines
Quoted 2 lines
+ struct regulator *vcc;
Use the right name of the pin for the regulator. I guess this is actually vci. I would implement all three regulators and get them as bulk. See e.g.
drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
for an example on how to get and enable several regulators using bulk.The regulator framework will provide dummy regulators if you didn't define some of them so it is fine to just provide one or two.
Really appreciate your suggestion, will add to v2.
Quoted 3 lines
Yours,
Linus Walleij