Re: [PATCH v2 4/4] drm/panel: Add Novatek NT35596 panel driver
Hi Jagan.
Thanks for this new panel driver.
A few notes, please see inline comments below.
On Thu, Mar 21, 2019 at 07:29:55PM +0530, Jagan Teki wrote:
> Novatek NT35596 is a single-chip IC solution for small or medium-sized
> LTPS TFT LCD panels. NT35596 provides several system interfaces like
> MIPI/SPI/I2C.
>
> Currently added support for Microtech MTF050FHDI-03 is 1080x1920,
> 4-lane MIPI DSI LCD panel which has inbuilt NT35596 IC chip.
>
> NT35596 support several regulators on the chip, among those only 4
> regulators like VCI, VDDI/DVDD, AVDD, AVEE are used for power-on
> sequence.
>
> This driver added code for 3-regulator based power-on sequence as
> of now since MTF050FHDI-03 panel support it. This power-on sequence
> may be moved to panel_desc in future, if any new panel would come
> up with other type of sequence.
>
> Signed-off-by: Jagan Teki
> ---
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a6d18acda78a..4de80222cffb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4922,6 +4922,12 @@ S: Maintained
> F: drivers/gpu/drm/tinydrm/hx8357d.c
> F: Documentation/devicetree/bindings/display/himax,hx8357d.txt
>
> +DRM DRIVER FOR NOVATEK NT35596 MIPI-DSI LCD PANELS
> +M: Jagan Teki
> +S: Maintained
> +F: drivers/gpu/drm/panel/panel-novatek-nt35596.c
> +F: Documentation/devicetree/bindings/display/panel/novatek,nt35596.txt
> +
I recall the entries in MAINTAINERS come in alphabetic order
based on their headline.
So this is the wrong order as "I" comes before "N":
DRM DRIVER FOR NOVATEK NT35596 MIPI-DSI LCD PANELS
DRM DRIVER FOR INTEL I810 VIDEO CARDS
Please move
> +#define NT35596_CMD_LEN 2
> +
> +struct nt35596_panel_desc {
> + const struct drm_display_mode *mode;
> + unsigned intlanes;
> + unsigned long flags;
> + enum mipi_dsi_pixel_format format;
> + const struct nt35596_init_cmd *panel_cmds;
> + unsigned intnum_panel_cmds;
> +};
> +
> +struct nt35596 {
> + struct drm_panelpanel;
> + struct mipi_dsi_device *dsi;
> + const struct nt35596_panel_desc *desc;
> +
> + struct backlight_device *backlight;
> + struct regulator*dvdd;
> + struct regulator*avdd;
> + struct regulator*avee;
> + struct gpio_desc*reset;
> +
> + boolprepared;
> + boolenabled;
We should move these flags to the core as more and more
drivers seems to neeed them.
Something you could take a shot at?
> +};
> +
> +static inline struct nt35596 *panel_to_nt35596(struct drm_panel *panel)
> +{
> + return container_of(panel, struct nt35596, panel);
> +}
> +
> +struct nt35596_init_cmd {
> + u8 data[NT35596_CMD_LEN];
> +};
> +
> +static const struct nt35596_init_cmd microtech_mtf050fhdi_cmds[] = {
> + { .data = { 0xFF, 0xEE } },
...
> + /* Exit CMD1, Turn-off Tear ON */
> + { .data = { 0xFF, 0x00 } },
> + { .data = { 0x35, 0x00 } },
> +};
> +
> +static int nt35596_power_on(struct nt35596 *nt35596)
> +{
> + int ret;
> +
> + ret = regulator_enable(nt35596->dvdd);
> + if (ret)
> + return ret;
> +
> + /* T_power_ramp_up for VDDI */
> + msleep(2);
> +
> + ret = regulator_enable(nt35596->avdd);
> + if (ret)
> + return ret;
> +
> + /* T_power_ramp_up for AVDD/AVEE */
> + msleep(5);
> +
> + ret = regulator_enable(nt35596->avee);
> + if (ret)
> + return ret;
> +
> + msleep(10);
> +
> + gpiod_set_value(nt35596->reset, 0);
> +
> + msleep(120);
> +
> + gpiod_set_value(nt35596->reset, 1);
> +
> + /* wait for 120ms after reset deassert */
> + msleep(120);
> +
> + return 0;
> +}
> +
> +static int nt35596_power_off(struct nt35596 *nt35596)
This function never fails. Should you check regulator_disable()?
(Mst drivers skips check of regulator_disable() and it seems safe
to skip it here too.
Make it a void function if it cannot error out.
> +{
> + gpiod_set_value(nt35596->reset, 0);
> +
> + msleep(10);
> +
> + regulator_disable(nt35596->avee);
> +
> + /* T_power_ramp_down for AVEE/AVDD */
> + msleep(5);
> +
> + regulator_disable(nt35596->avdd);
> +
> + /* T_power_ramp_down for VDDI */
> + msleep(2);
> +
> + regulator_disable(nt35596->dvdd);
> +
> + return 0;
> +}
> +
> +static int nt35596_prepare(struct drm_panel *panel)
> +{
> + struct nt35596 *nt35596 = panel_to_nt35596(panel);
> + struct mipi_dsi_device *dsi = nt35596->dsi;
> + const struct nt35596_panel_desc *desc = nt35596->desc;
> + int ret, i;
> +
> + if (nt35596->prepared)
> + return 0;
> +
> + ret = nt35596_power_on(nt35596);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < desc->num_panel_cmds; i++) {
> + const struct nt3559
[PATCH v2 4/4] drm/panel: Add Novatek NT35596 panel driver
Novatek NT35596 is a single-chip IC solution for small or medium-sized
LTPS TFT LCD panels. NT35596 provides several system interfaces like
MIPI/SPI/I2C.
Currently added support for Microtech MTF050FHDI-03 is 1080x1920,
4-lane MIPI DSI LCD panel which has inbuilt NT35596 IC chip.
NT35596 support several regulators on the chip, among those only 4
regulators like VCI, VDDI/DVDD, AVDD, AVEE are used for power-on
sequence.
This driver added code for 3-regulator based power-on sequence as
of now since MTF050FHDI-03 panel support it. This power-on sequence
may be moved to panel_desc in future, if any new panel would come
up with other type of sequence.
Signed-off-by: Jagan Teki
---
MAINTAINERS | 6 +
drivers/gpu/drm/panel/Kconfig | 13 +
drivers/gpu/drm/panel/Makefile| 1 +
drivers/gpu/drm/panel/panel-novatek-nt35596.c | 895 ++
4 files changed, 915 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-novatek-nt35596.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a6d18acda78a..4de80222cffb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4922,6 +4922,12 @@ S: Maintained
F: drivers/gpu/drm/tinydrm/hx8357d.c
F: Documentation/devicetree/bindings/display/himax,hx8357d.txt
+DRM DRIVER FOR NOVATEK NT35596 MIPI-DSI LCD PANELS
+M: Jagan Teki
+S: Maintained
+F: drivers/gpu/drm/panel/panel-novatek-nt35596.c
+F: Documentation/devicetree/bindings/display/panel/novatek,nt35596.txt
+
DRM DRIVER FOR INTEL I810 VIDEO CARDS
S: Orphan / Obsolete
F: drivers/gpu/drm/i810/
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 5c2c7e8e6ade..c39aaf40445c 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -119,6 +119,19 @@ config DRM_PANEL_LG_LG4573
Say Y here if you want to enable support for LG4573 RGB panel.
To compile this driver as a module, choose M here.
+config DRM_PANEL_NOVATEK_NT35596
+ tristate "Novatek NT35596 panel driver"
+ depends on OF
+ depends on DRM_MIPI_DSI
+ depends on BACKLIGHT_CLASS_DEVICE
+ help
+ Say Y here if you want to enable support for the Novatek NT35596
+ panel controller driver.
+
+ Novatek NT35596 is a single-chip IC solution for small or medium
+ sized LTPS TFT LCD panels. NT35596 provides several system interfaces
+ like MIPI/SPI/I2C.
+
config DRM_PANEL_OLIMEX_LCD_OLINUXINO
tristate "Olimex LCD-OLinuXino panel"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 3a853ea5cff9..05bc93348345 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) +=
panel-innolux-p079zca.o
obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
+obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35596) += panel-novatek-nt35596.o
obj-$(CONFIG_DRM_PANEL_OLIMEX_LCD_OLINUXINO) += panel-olimex-lcd-olinuxino.o
obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) +=
panel-panasonic-vvx10f034n00.o
diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35596.c
b/drivers/gpu/drm/panel/panel-novatek-nt35596.c
new file mode 100644
index ..433508330321
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35596.c
@@ -0,0 +1,895 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Amarula Solutions
+ * Author: Jagan Teki
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#define NT35596_CMD_LEN2
+
+struct nt35596_panel_desc {
+ const struct drm_display_mode *mode;
+ unsigned intlanes;
+ unsigned long flags;
+ enum mipi_dsi_pixel_format format;
+ const struct nt35596_init_cmd *panel_cmds;
+ unsigned intnum_panel_cmds;
+};
+
+struct nt35596 {
+ struct drm_panelpanel;
+ struct mipi_dsi_device *dsi;
+ const struct nt35596_panel_desc *desc;
+
+ struct backlight_device *backlight;
+ struct regulator*dvdd;
+ struct regulator*avdd;
+ struct regulator*avee;
+ struct gpio_desc*reset;
+
+ boolprepared;
+ boolenabled;
+};
+
+static inline struct nt35596 *panel_to_nt35596(struct drm_panel *panel)
+{
+ return container_of(panel, struct nt35596, panel);
+}
+
+struct nt35596_init_cmd {
+ u8 data[NT35596_CMD_LEN];
+};
+
+static const struct nt35596_init_cmd micr
