Fannal C3004 is a 2 lane MIPI DSI 480x800 panel which requires initialization with DSI DCS
commands. After few initialization commands delay is required.
Paulo Pavacic (3):
dt-bindings: add fannal vendor prefix
dt-bindings: display: panel: add fannal,c3004
drm/panel-fannal-c3004: Add fannal c3004 DSI panel
.../bindings/display/panel/fannal,c3004.yaml | 78 +++++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 6 +
drivers/gpu/drm/panel/Kconfig | 11 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-fannal-c3004.c | 314 ++++++++++++++++++
6 files changed, 412 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml
create mode 100644 drivers/gpu/drm/panel/panel-fannal-c3004.c
--
2.40.1
Fannal is a one-stop-solution provider for touch displays in industrial
application, committed to delivering a variety of professional high-quality
products and technical services globally.
Website: http://www.fannal.com
Signed-off-by: Paulo Pavacic <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 82d39ab0231b..f962750f630a 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -462,6 +462,8 @@ patternProperties:
description: Facebook
"^fairphone,.*":
description: Fairphone B.V.
+ "^fannal,.*":
+ description: Fannal Electronics Co., Ltd
"^faraday,.*":
description: Faraday Technology Corporation
"^fastrax,.*":
--
2.40.1
Fannal C3004 is a 480x800 display made by fannal that requires
DCS initialization sequences.
Signed-off-by: Paulo Pavacic <[email protected]>
---
v4 changelog:
- removal of blank lines, code wrapping changes
- reset pin initialization handling changes
v3 changelog:
- formatting and style changes
- remove BIT_NONEXCLUSIVE flag from reset pin
- use dev_err_probe instead of dev_err
v2 changelog:
- using generic mipi_dsi_dcs_write_seq
- removed success prints
- removed some comments
- simplified code/removed support for different panels
- changed namespace from fann to fannal
v1 changelog:
- renamed from panel-mipi-dsi-bringup
- only one MAINTAINER e-mail
---
MAINTAINERS | 1 +
drivers/gpu/drm/panel/Kconfig | 11 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-fannal-c3004.c | 314 +++++++++++++++++++++
4 files changed, 327 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-fannal-c3004.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 978133f87c5e..7ad37ecd7e09 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6431,6 +6431,7 @@ DRM DRIVER FOR FANNAL C3004373132019A
M: Paulo Pavacic <[email protected]>
S: Maintained
F: Documentation/devicetree/bindings/display/panel/panel-fannal,c3004.yaml
+F: drivers/gpu/drm/panel/panel-fannal-c3004.c
DRM DRIVER FOR FARADAY TVE200 TV ENCODER
M: Linus Walleij <[email protected]>
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 2b9d6db7860b..a1041c1e6bf6 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -144,6 +144,17 @@ config DRM_PANEL_ELIDA_KD35T133
KD35T133 controller for 320x480 LCD panels with MIPI-DSI
system interfaces.
+config DRM_PANEL_FANNAL_C3004
+ tristate "Fannal C3004 panel"
+ depends on OF
+ depends on DRM_MIPI_DSI
+ help
+ Say Y here if you want to enable support for the Fannal C3004
+ 2-lane 480x800 MIPI DSI panel which requires initialization
+ sequence.
+
+ If M is selected the module will be called panel-fannal-c3004.
+
config DRM_PANEL_FEIXIN_K101_IM2BA02
tristate "Feixin K101 IM2BA02 panel"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index ff169781e82d..13c0f00038b5 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
obj-$(CONFIG_DRM_PANEL_EDP) += panel-edp.o
obj-$(CONFIG_DRM_PANEL_EBBG_FT8719) += panel-ebbg-ft8719.o
obj-$(CONFIG_DRM_PANEL_ELIDA_KD35T133) += panel-elida-kd35t133.o
+obj-$(CONFIG_DRM_PANEL_FANNAL_C3004) += panel-fannal-c3004.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_HIMAX_HX8394) += panel-himax-hx8394.o
diff --git a/drivers/gpu/drm/panel/panel-fannal-c3004.c b/drivers/gpu/drm/panel/panel-fannal-c3004.c
new file mode 100644
index 000000000000..002c424f2486
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-fannal-c3004.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MIPI DSI driver for fannal C3004.
+ * Copyright (C) 2023, Zenitel
+ * Author: Paulo Pavacic <[email protected]>
+ */
+
+// ↓ include headers, static values, static functions ↓
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/media-bus-format.h>
+
+#include <video/mipi_display.h>
+#include <video/of_videomode.h>
+#include <video/videomode.h>
+
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+struct fannal_panel_data {
+ struct drm_panel panel;
+ struct gpio_desc *reset;
+};
+
+static struct fannal_panel_data *
+get_fannal_panel_data_from_panel(struct drm_panel *panel)
+{
+ return container_of(panel, struct fannal_panel_data, panel);
+}
+
+static const u32 fannal_bus_formats[] = {
+ MEDIA_BUS_FMT_RGB888_1X24,
+};
+
+// resolution 480p x 800p, 56mmx93mm
+static const struct drm_display_mode fannal_c3004_display_mode = {
+ .clock = 27000,
+ .hdisplay = 480, // display height pixels
+ .hsync_start = 480 + 30, // hdisplay + HBP
+ .hsync_end = 480 + 30 + 8, // hdisplay + HBP + HSync
+ .htotal = 480 + 30 + 8 + 30, // hdisplay + HBP + HSync + HFP
+ .vdisplay = 800, // display width pixels
+ .vsync_start = 800 + 20, // vdisplay + VBP
+ .vsync_end = 800 + 20 + 8, // vdisplay + VBP + VSync
+ .vtotal = 800 + 20 + 8 + 20, // vdisplay + VBP + VSync + VFP
+ .width_mm = 93,
+ .height_mm = 56,
+ .flags = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static void fannal_panel_remove(struct mipi_dsi_device *dsi)
+{
+ struct fannal_panel_data *panel_data = mipi_dsi_get_drvdata(dsi);
+ struct device *dev = &dsi->dev;
+ int ret;
+
+ ret = mipi_dsi_detach(dsi);
+ if (ret)
+ dev_err(dev, "error: disable: mipi detach (%d)\n", ret);
+
+ drm_panel_remove(&panel_data->panel);
+}
+
+static int fannal_panel_disable(struct drm_panel *panel)
+{
+ struct mipi_dsi_device *dsi = to_mipi_dsi_device(panel->dev);
+ struct device *dev = panel->dev;
+ int ret;
+
+ dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+ ret = mipi_dsi_dcs_set_display_off(dsi);
+ if (ret < 0) {
+ dev_err(dev, "error: disable: turn display OFF (%d)\n", ret);
+ return ret;
+ }
+
+ usleep_range(5000, 10000);
+
+ ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
+ if (ret < 0) {
+ dev_err(dev, "error: disable: enter sleep mode (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int fannal_panel_unprepare(struct drm_panel *panel)
+{
+ struct fannal_panel_data *panel_data =
+ get_fannal_panel_data_from_panel(panel);
+
+ if (panel_data->reset) {
+ gpiod_set_value_cansleep(panel_data->reset, 1);
+ usleep_range(15000, 17000);
+ gpiod_set_value_cansleep(panel_data->reset, 0);
+ }
+
+ return 0;
+}
+
+static void fannal_panel_shutdown(struct mipi_dsi_device *dsi)
+{
+ struct fannal_panel_data *panel_data = mipi_dsi_get_drvdata(dsi);
+
+ fannal_panel_disable(&panel_data->panel);
+ fannal_panel_unprepare(&panel_data->panel);
+}
+
+static int fannal_panel_get_modes(struct drm_panel *panel,
+ struct drm_connector *connector)
+{
+ struct drm_display_mode *mode;
+ const struct drm_display_mode *panel_display_mode =
+ &fannal_c3004_display_mode;
+
+ mode = drm_mode_duplicate(connector->dev, &fannal_c3004_display_mode);
+ if (!mode) {
+ dev_err(panel->dev, "error: get_modes: add drm mode %ux%u@%u\n",
+ panel_display_mode->hdisplay,
+ panel_display_mode->vdisplay,
+ drm_mode_vrefresh(panel_display_mode));
+ return -ENOMEM;
+ }
+
+ drm_mode_set_name(mode);
+ mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+ drm_mode_probed_add(connector, mode);
+
+ connector->display_info.width_mm = mode->width_mm;
+ connector->display_info.height_mm = mode->height_mm;
+ connector->display_info.bus_flags = DRM_BUS_FLAG_DE_LOW |
+ DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE;
+
+ drm_display_info_set_bus_formats(&connector->display_info,
+ fannal_bus_formats,
+ ARRAY_SIZE(fannal_bus_formats));
+ return 1;
+}
+
+static const struct of_device_id fannal_of_match[] = {
+ { .compatible = "fannal,c3004" },
+ { /* sentinel */ }
+};
+
+static int fannal_panel_enable(struct drm_panel *panel)
+{
+ struct mipi_dsi_device *dsi = to_mipi_dsi_device(panel->dev);
+
+ mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x13);
+ mipi_dsi_generic_write_seq(dsi, 0xEF, 0x08);
+
+ mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x10);
+ mipi_dsi_generic_write_seq(dsi, 0xC0, 0x63, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xC1, 0x0A, 0x0C);
+ mipi_dsi_generic_write_seq(dsi, 0xC2, 0x31, 0x08);
+ mipi_dsi_generic_write_seq(dsi, 0xCC, 0x18);
+
+ mipi_dsi_generic_write_seq(dsi, 0xB0, 0x00, 0x08, 0x10, 0x0E, 0x11,
+ 0x07, 0x08, 0x08, 0x08, 0x25, 0x04, 0x12,
+ 0x0F, 0x2C, 0x30, 0x1F);
+ mipi_dsi_generic_write_seq(dsi, 0xB1, 0x00, 0x11, 0x18, 0x0C, 0x10,
+ 0x05, 0x07, 0x09, 0x08, 0x24, 0x04, 0x11,
+ 0x10, 0x2B, 0x30, 0x1F);
+
+ mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x11);
+ mipi_dsi_generic_write_seq(dsi, 0xB0, 0x4D);
+ mipi_dsi_generic_write_seq(dsi, 0xB1, 0x39);
+ mipi_dsi_generic_write_seq(dsi, 0xB2, 0x87);
+ mipi_dsi_generic_write_seq(dsi, 0xB3, 0x80);
+ mipi_dsi_generic_write_seq(dsi, 0xB5, 0x47);
+ mipi_dsi_generic_write_seq(dsi, 0xB7, 0x8A);
+ mipi_dsi_generic_write_seq(dsi, 0xB8, 0x20);
+ mipi_dsi_generic_write_seq(dsi, 0xB9, 0x10, 0x13);
+ mipi_dsi_generic_write_seq(dsi, 0xC1, 0x78);
+ mipi_dsi_generic_write_seq(dsi, 0xC2, 0x78);
+ mipi_dsi_generic_write_seq(dsi, 0xD0, 0x88);
+
+ //PANEL
+ mipi_dsi_generic_write_seq(dsi, 0xE0, 0x00, 0x00, 0x02);
+ mipi_dsi_generic_write_seq(dsi, 0xE1, 0x04, 0x00, 0x00, 0x00, 0x05,
+ 0x00, 0x00, 0x00, 0x00, 0x20, 0x20);
+ mipi_dsi_generic_write_seq(dsi, 0xE2, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xE3, 0x00, 0x00, 0x33, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xE4, 0x22, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xE5, 0x04, 0x34, 0xAA, 0xAA, 0x06,
+ 0x34, 0xAA, 0xAA, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xE6, 0x00, 0x00, 0x33, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xE7, 0x22, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xE8, 0x05, 0x34, 0xAA, 0xAA, 0x07,
+ 0x34, 0xAA, 0xAA, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xEB, 0x02, 0x00, 0x40, 0x40, 0x00,
+ 0x00, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xEC, 0x00, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0xED, 0xFA, 0x45, 0x0B, 0xFF, 0xFF,
+ 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
+ 0xFF, 0xB0, 0x54, 0xAF);
+ mipi_dsi_generic_write_seq(dsi, 0xEF, 0x10, 0x0D, 0x04, 0x08, 0x3F,
+ 0x1F);
+
+ mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x13);
+ mipi_dsi_generic_write_seq(dsi, 0xE8, 0x00, 0x0E);
+
+ mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0x11); //MIPI_DCS_EXIT_SLEEP_MODE
+
+ msleep(600);
+
+ mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x13);
+ mipi_dsi_generic_write_seq(dsi, 0xE8, 0x00, 0x0C);
+ msleep(50);
+ mipi_dsi_generic_write_seq(dsi, 0xE8, 0x00, 0x00);
+
+ mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x00);
+ mipi_dsi_generic_write_seq(dsi, 0x29); //MIPI_DCS_SET_DISPLAY_ON
+ msleep(100);
+ return 0;
+}
+
+static int fannal_panel_prepare(struct drm_panel *panel)
+{
+ struct fannal_panel_data *panel_data =
+ get_fannal_panel_data_from_panel(panel);
+
+ /* At lest 10ms needed between power-on and reset-out as RM specifies */
+ usleep_range(10000, 12000);
+
+ if (panel_data->reset) {
+ gpiod_set_value_cansleep(panel_data->reset, 0);
+ /*
+ * 50ms delay after reset-out, as per manufacturer initalization
+ * sequence.
+ */
+ msleep(50);
+ }
+
+ return 0;
+}
+
+static const struct drm_panel_funcs fannal_panel_funcs = {
+ .prepare = fannal_panel_prepare,
+ .unprepare = fannal_panel_unprepare,
+ .enable = fannal_panel_enable,
+ .disable = fannal_panel_disable,
+ .get_modes = fannal_panel_get_modes,
+};
+
+static int fannal_panel_probe(struct mipi_dsi_device *dsi)
+{
+ struct device *dev = &dsi->dev;
+ struct fannal_panel_data *panel_data;
+ int ret;
+
+ panel_data = devm_kzalloc(&dsi->dev, sizeof(*panel_data), GFP_KERNEL);
+ if (!panel_data)
+ return -ENOMEM;
+
+ panel_data->reset = devm_gpiod_get_optional(dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(panel_data->reset)) {
+ return dev_err_probe(
+ dev, PTR_ERR(panel_data->reset),
+ "error: probe: get reset GPIO: (%d) Check the fdt\n",
+ ret);
+ }
+
+ mipi_dsi_set_drvdata(dsi, panel_data);
+
+ dsi->format = MIPI_DSI_FMT_RGB888;
+ dsi->mode_flags = MIPI_DSI_CLOCK_NON_CONTINUOUS |
+ MIPI_DSI_MODE_VSYNC_FLUSH | MIPI_DSI_MODE_VIDEO_HSE |
+ MIPI_DSI_MODE_NO_EOT_PACKET |
+ MIPI_DSI_MODE_VIDEO_SYNC_PULSE | MIPI_DSI_MODE_VIDEO;
+ dsi->lanes = 2;
+
+ drm_panel_init(&panel_data->panel, dev, &fannal_panel_funcs,
+ DRM_MODE_CONNECTOR_DSI);
+ dev_set_drvdata(dev, panel_data);
+
+ drm_panel_add(&panel_data->panel);
+
+ ret = mipi_dsi_attach(dsi);
+ if (ret) {
+ drm_panel_remove(&panel_data->panel);
+ dev_err(dev, "error: probe fail: can't attach mipi_dsi\n");
+ }
+
+ return ret;
+}
+
+static struct mipi_dsi_driver fannal_panel_driver = {
+ .driver = {
+ .name = "panel-fannal-c3004",
+ .of_match_table = fannal_of_match,
+ },
+ .probe = fannal_panel_probe,
+ .remove = fannal_panel_remove,
+ .shutdown = fannal_panel_shutdown,
+};
+
+module_mipi_dsi_driver(fannal_panel_driver);
+
+MODULE_AUTHOR("Paulo Pavacic <[email protected]>");
+MODULE_DESCRIPTION("fannal C3004 panel driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, fannal_of_match);
--
2.40.1
Added fannal to vendor-prefixes and dt bindings for Fannal C3004.
Fannal C3004 is a 480x800 MIPI DSI Panel which requires
DCS initialization sequences with certain delays between certain
commands.
Signed-off-by: Paulo Pavacic <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
v4 changelog:
- removed driver's community from MAINTAINERS file
v2 changelog:
- added empty lines between properties in yml
- renamed yml file
- refactored yml file to describe fannal,c3004
- added matrix URI to MAINTAINERS
v1 changelog:
- revised driver title, now describes purpose
- revised description, now describes hw
- revised maintainers, now has only 1 mail
- removed diacritics from commit/commit author
- properties/compatible is now enum
- compatible using only lowercase
- revised dts example
- modified MAINTAINERS in this commit (instead of driver commit)
- dt_bindings_check checked yml
- checkpatch warning fixed
---
.../bindings/display/panel/fannal,c3004.yaml | 78 +++++++++++++++++++
MAINTAINERS | 5 ++
2 files changed, 83 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml
diff --git a/Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml b/Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml
new file mode 100644
index 000000000000..bbddb036094b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/fannal,c3004.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Fannal C3004 MIPI-DSI
+
+maintainers:
+ - Paulo Pavacic <[email protected]>
+
+description: |
+ Fannal C3004 is a 480x800 panel which requires DSI DCS
+ initialization sequences.
+
+allOf:
+ - $ref: panel-common.yaml#
+
+properties:
+ compatible:
+ items:
+ - const: fannal,c3004
+
+ reg: true
+
+ reset-gpios: true
+
+ vdd-supply:
+ description: power supply voltage
+
+ vddio-supply:
+ description: power supply voltage for IO
+
+ width-mm:
+ description: physical panel width [mm]
+
+ height-mm:
+ description: physical panel height [mm]
+
+ panel-timing: true
+
+required:
+ - compatible
+ - reg
+ - reset-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ dsi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ panel@0 {
+ compatible = "fannal,c3004";
+ reg = <0>;
+ pinctrl-0 = <&pinctrl_mipi_dsi_rst>;
+ pinctrl-names = "default";
+ reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
+ vdd-supply = <®1>;
+ vddio-supply = <®2>;
+ width-mm = <93>;
+ height-mm = <56>;
+ panel-timing {
+ clock-frequency = <27000000>;
+ hactive = <480>;
+ vactive = <800>;
+ hfront-porch = <30>;
+ hback-porch = <30>;
+ hsync-len = <8>;
+ vback-porch = <30>;
+ vfront-porch = <30>;
+ vsync-len = <8>;
+ };
+ };
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 5c22c828ab46..978133f87c5e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6427,6 +6427,11 @@ T: git git://anongit.freedesktop.org/drm/drm-misc
F: Documentation/devicetree/bindings/display/panel/ebbg,ft8719.yaml
F: drivers/gpu/drm/panel/panel-ebbg-ft8719.c
+DRM DRIVER FOR FANNAL C3004373132019A
+M: Paulo Pavacic <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/display/panel/panel-fannal,c3004.yaml
+
DRM DRIVER FOR FARADAY TVE200 TV ENCODER
M: Linus Walleij <[email protected]>
S: Maintained
--
2.40.1
Hi Paulo,
thanks for your patch!
Overall this looks very good.
I doubt that the display controller is actually by Fannal, but I guess
you tried to find out? We usually try to identify the underlying display
controller so the driver can be named after it and reused for more
display panels.
Some minor questions/nitpicks below.
On Wed, Jun 7, 2023 at 5:11 PM Paulo Pavacic <[email protected]> wrote:
> +static int fannal_panel_enable(struct drm_panel *panel)
> +{
> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(panel->dev);
> +
> + mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x13);
> + mipi_dsi_generic_write_seq(dsi, 0xEF, 0x08);
Why is everything using mipi_dsi_generic_write_seq() instead of
mipi_dsi_dcs_write_seq()?
Especially here, because 0x11 is certainly a command:
> + mipi_dsi_generic_write_seq(dsi, 0x11); //MIPI_DCS_EXIT_SLEEP_MODE
Instead use:
ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
if (ret)
return ret;
> + mipi_dsi_generic_write_seq(dsi, 0x29); //MIPI_DCS_SET_DISPLAY_ON
Instead use:
ret = mipi_dsi_dcs_set_display_on(dsi);
if (ret)
return ret;
Yours,
Linus Walleij
Hello Linus,
thank you for the comments.
čet, 15. lip 2023. u 21:55 Linus Walleij <[email protected]> napisao je:
>
> Hi Paulo,
>
> thanks for your patch!
>
> Overall this looks very good.
>
> I doubt that the display controller is actually by Fannal, but I guess
> you tried to find out? We usually try to identify the underlying display
> controller so the driver can be named after it and reused for more
> display panels.
Yes, of course, the controller is ST7701S.
>
> Some minor questions/nitpicks below.
>
> On Wed, Jun 7, 2023 at 5:11 PM Paulo Pavacic <[email protected]> wrote:
>
> > +static int fannal_panel_enable(struct drm_panel *panel)
> > +{
> > + struct mipi_dsi_device *dsi = to_mipi_dsi_device(panel->dev);
> > +
> > + mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x13);
> > + mipi_dsi_generic_write_seq(dsi, 0xEF, 0x08);
>
> Why is everything using mipi_dsi_generic_write_seq() instead of
> mipi_dsi_dcs_write_seq()?
Okay, I will replace it.
>
> Especially here, because 0x11 is certainly a command:
>
> > + mipi_dsi_generic_write_seq(dsi, 0x11); //MIPI_DCS_EXIT_SLEEP_MODE
>
> Instead use:
>
> ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> if (ret)
> return ret;
>
>
> > + mipi_dsi_generic_write_seq(dsi, 0x29); //MIPI_DCS_SET_DISPLAY_ON
>
> Instead use:
>
> ret = mipi_dsi_dcs_set_display_on(dsi);
> if (ret)
> return ret;
>
Okay I will replace all commands with a functions found here:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_mipi_dsi.c#L995
> Yours,
> Linus Walleij
Best regards,
Paulo
On Fri, Jun 16, 2023 at 11:57 AM Paulo Pavacic <[email protected]> wrote:
> čet, 15. lip 2023. u 21:55 Linus Walleij <[email protected]> napisao je:
> >
> > I doubt that the display controller is actually by Fannal, but I guess
> > you tried to find out? We usually try to identify the underlying display
> > controller so the driver can be named after it and reused for more
> > display panels.
>
> Yes, of course, the controller is ST7701S.
Hm did you try to just refactor
drivers/gpu/drm/panel/panel-sitronix-st7701.c
to support your new panel?
One major reason would be that that driver knows what
commands actually mean and have #defines for them.
Yours,
Linus Walleij
pet, 16. lip 2023. u 13:44 Linus Walleij <[email protected]> napisao je:
>
> On Fri, Jun 16, 2023 at 11:57 AM Paulo Pavacic <[email protected]> wrote:
> > čet, 15. lip 2023. u 21:55 Linus Walleij <[email protected]> napisao je:
> > >
> > > I doubt that the display controller is actually by Fannal, but I guess
> > > you tried to find out? We usually try to identify the underlying display
> > > controller so the driver can be named after it and reused for more
> > > display panels.
> >
> > Yes, of course, the controller is ST7701S.
>
> Hm did you try to just refactor
> drivers/gpu/drm/panel/panel-sitronix-st7701.c
> to support your new panel?
>
Yes I have tried, but there are too many changes needed and I wasn't
sure whether I would be breaking compatibility with st7701 based
panels.
> One major reason would be that that driver knows what
> commands actually mean and have #defines for them.
>
> Yours,
> Linus Walleij
Best regards,
Paulo
On Fri, Jun 16, 2023 at 2:31 PM Paulo Pavacic <[email protected]> wrote:
> pet, 16. lip 2023. u 13:44 Linus Walleij <[email protected]> napisao je:
> >
> > On Fri, Jun 16, 2023 at 11:57 AM Paulo Pavacic <[email protected]> wrote:
> > > čet, 15. lip 2023. u 21:55 Linus Walleij <[email protected]> napisao je:
> > > >
> > > > I doubt that the display controller is actually by Fannal, but I guess
> > > > you tried to find out? We usually try to identify the underlying display
> > > > controller so the driver can be named after it and reused for more
> > > > display panels.
> > >
> > > Yes, of course, the controller is ST7701S.
> >
> > Hm did you try to just refactor
> > drivers/gpu/drm/panel/panel-sitronix-st7701.c
> > to support your new panel?
>
> Yes I have tried, but there are too many changes needed and I wasn't
> sure whether I would be breaking compatibility with st7701 based
> panels.
For the community it creates a problem that now two drivers for similar
hardware need to be maintained, and that burden will land on the DRM
maintainers. For this reason it would be better if a joint driver could
be created.
I am sure the users of the old driver will be willing to test patches to
make sure their devices keep working.
Yours,
Linus Walleij
pet, 16. lip 2023. u 14:53 Linus Walleij <[email protected]> napisao je:
>
> On Fri, Jun 16, 2023 at 2:31 PM Paulo Pavacic <[email protected]> wrote:
> > pet, 16. lip 2023. u 13:44 Linus Walleij <[email protected]> napisao je:
> > >
> > > On Fri, Jun 16, 2023 at 11:57 AM Paulo Pavacic <[email protected]> wrote:
> > > > čet, 15. lip 2023. u 21:55 Linus Walleij <[email protected]> napisao je:
> > > > >
> > > > > I doubt that the display controller is actually by Fannal, but I guess
> > > > > you tried to find out? We usually try to identify the underlying display
> > > > > controller so the driver can be named after it and reused for more
> > > > > display panels.
> > > >
> > > > Yes, of course, the controller is ST7701S.
> > >
> > > Hm did you try to just refactor
> > > drivers/gpu/drm/panel/panel-sitronix-st7701.c
> > > to support your new panel?
> >
> > Yes I have tried, but there are too many changes needed and I wasn't
> > sure whether I would be breaking compatibility with st7701 based
> > panels.
>
> For the community it creates a problem that now two drivers for similar
> hardware need to be maintained, and that burden will land on the DRM
> maintainers. For this reason it would be better if a joint driver could
> be created.
I will try modifying st7701, but that seems like a big task since
currently st7701 crashes kernel (5.15) for me and I have seen
suggestions to use raydium driver over st7701.
Also I guess I should first read some more documentation and compare
st7701 to st7701s. I currently can't reserve a lot of time for that.
>
> I am sure the users of the old driver will be willing to test patches to
> make sure their devices keep working.
A lot of modifications to st7701 are required. I believe it would
result in a driver that doesn't look or work the same. e.g compare
delays between initialization sequences of panel-fannal-c3004 and
panel-st7701. I think it would be optimal to create st7701s driver and
have special handling for st7701s panels. If there was a flag for
whether panel is st7701 or st7701s it would end up looking like a
mess.
>
> Yours,
> Linus Walleij
Thank you for your time,
Paulo
On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
> A lot of modifications to st7701 are required. I believe it would
> result in a driver that doesn't look or work the same. e.g compare
> delays between initialization sequences of panel-fannal-c3004 and
> panel-st7701. I think it would be optimal to create st7701s driver and
> have special handling for st7701s panels. If there was a flag for
> whether panel is st7701 or st7701s it would end up looking like a
> mess.
What matters is if the original authors of the old st7701 driver are
around and reviewing and testing patches at all. What we need is
active maintainers. (Added Jagan, Marek & Maya).
I buy the reasoning that the st7701s is perhaps substantially different
from st7701.
If st7701s is very different then I suppose it needs a separate driver,
then all we need to to name the driver properly, i.e.
panel-sitronix-st7701s.c.
Yours,
Linus Walleij
On 7/6/23 17:18, Paulo Pavacic wrote:
> Hello Linus,
>
> čet, 22. lip 2023. u 10:22 Linus Walleij <[email protected]> napisao je:
>>
>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
>>
>>> A lot of modifications to st7701 are required. I believe it would
>>> result in a driver that doesn't look or work the same. e.g compare
>>> delays between initialization sequences of panel-fannal-c3004 and
>>> panel-st7701. I think it would be optimal to create st7701s driver and
>>> have special handling for st7701s panels. If there was a flag for
>>> whether panel is st7701 or st7701s it would end up looking like a
>>> mess.
>>
>> What matters is if the original authors of the old st7701 driver are
>> around and reviewing and testing patches at all. What we need is
>> active maintainers. (Added Jagan, Marek & Maya).
>>
>> I buy the reasoning that the st7701s is perhaps substantially different
>> from st7701.
>>
>> If st7701s is very different then I suppose it needs a separate driver,
>> then all we need to to name the driver properly, i.e.
>> panel-sitronix-st7701s.c.
>
> I had in person talk with Paul Kocialkowski and I have concluded that
> this is the best solution.
> I believe I should rename it to st7701s due to the hardware changes. I
> would like to create V5 patch with driver renamed to st7701s.
> Please let me know if you agree / disagree.
If I recall it right, the ST7701 and ST7701S are basically the same
chip, aren't they ?
Hello Linus,
čet, 22. lip 2023. u 10:22 Linus Walleij <[email protected]> napisao je:
>
> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
>
> > A lot of modifications to st7701 are required. I believe it would
> > result in a driver that doesn't look or work the same. e.g compare
> > delays between initialization sequences of panel-fannal-c3004 and
> > panel-st7701. I think it would be optimal to create st7701s driver and
> > have special handling for st7701s panels. If there was a flag for
> > whether panel is st7701 or st7701s it would end up looking like a
> > mess.
>
> What matters is if the original authors of the old st7701 driver are
> around and reviewing and testing patches at all. What we need is
> active maintainers. (Added Jagan, Marek & Maya).
>
> I buy the reasoning that the st7701s is perhaps substantially different
> from st7701.
>
> If st7701s is very different then I suppose it needs a separate driver,
> then all we need to to name the driver properly, i.e.
> panel-sitronix-st7701s.c.
I had in person talk with Paul Kocialkowski and I have concluded that
this is the best solution.
I believe I should rename it to st7701s due to the hardware changes. I
would like to create V5 patch with driver renamed to st7701s.
Please let me know if you agree / disagree.
>
> Yours,
> Linus Walleij
Thank you for your time,
Paulo Pavačić
Hello Marek,
čet, 6. srp 2023. u 17:26 Marek Vasut <[email protected]> napisao je:
>
> On 7/6/23 17:18, Paulo Pavacic wrote:
> > Hello Linus,
> >
> > čet, 22. lip 2023. u 10:22 Linus Walleij <[email protected]> napisao je:
> >>
> >> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
> >>
> >>> A lot of modifications to st7701 are required. I believe it would
> >>> result in a driver that doesn't look or work the same. e.g compare
> >>> delays between initialization sequences of panel-fannal-c3004 and
> >>> panel-st7701. I think it would be optimal to create st7701s driver and
> >>> have special handling for st7701s panels. If there was a flag for
> >>> whether panel is st7701 or st7701s it would end up looking like a
> >>> mess.
> >>
> >> What matters is if the original authors of the old st7701 driver are
> >> around and reviewing and testing patches at all. What we need is
> >> active maintainers. (Added Jagan, Marek & Maya).
> >>
> >> I buy the reasoning that the st7701s is perhaps substantially different
> >> from st7701.
> >>
> >> If st7701s is very different then I suppose it needs a separate driver,
> >> then all we need to to name the driver properly, i.e.
> >> panel-sitronix-st7701s.c.
> >
> > I had in person talk with Paul Kocialkowski and I have concluded that
> > this is the best solution.
> > I believe I should rename it to st7701s due to the hardware changes. I
> > would like to create V5 patch with driver renamed to st7701s.
> > Please let me know if you agree / disagree.
>
> If I recall it right, the ST7701 and ST7701S are basically the same
> chip, aren't they ?
I'm currently exploring all the differences. There aren't a lot of
differences, but there are some.
So far I can see that default register values are different, new
previously unused registers are now used and there has been some
reordering of how info is placed in registers [1] (data bits are in
different order). Moreover, instructions to some commands have been
changed and meaning of what data bits mean [2][3]. Also, new features
have been added [2]; there is now PCLKS 3 for example.
You can see few differences in following images. Same images were
attached in this mail:
[1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png
[2] https://ibb.co/G79y235 - PCLKS2.png
Thank you for your time,
Paulo
Hello Marek,
Jul 6, 2023 5:26:15 PM Marek Vasut <[email protected]>:
> On 7/6/23 17:18, Paulo Pavacic wrote:
>> Hello Linus,
>> čet, 22. lip 2023. u 10:22 Linus Walleij <[email protected]> napisao je:
>>>
>>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
>>>
>>>> A lot of modifications to st7701 are required. I believe it would
>>>> result in a driver that doesn't look or work the same. e.g compare
>>>> delays between initialization sequences of panel-fannal-c3004 and
>>>> panel-st7701. I think it would be optimal to create st7701s driver and
>>>> have special handling for st7701s panels. If there was a flag for
>>>> whether panel is st7701 or st7701s it would end up looking like a
>>>> mess.
>>>
>>> What matters is if the original authors of the old st7701 driver are
>>> around and reviewing and testing patches at all. What we need is
>>> active maintainers. (Added Jagan, Marek & Maya).
>>>
>>> I buy the reasoning that the st7701s is perhaps substantially different
>>> from st7701.
>>>
>>> If st7701s is very different then I suppose it needs a separate driver,
>>> then all we need to to name the driver properly, i.e.
>>> panel-sitronix-st7701s.c.
>> I had in person talk with Paul Kocialkowski and I have concluded that
>> this is the best solution.
>> I believe I should rename it to st7701s due to the hardware changes. I
>> would like to create V5 patch with driver renamed to st7701s.
>> Please let me know if you agree / disagree.
>
> If I recall it right, the ST7701 and ST7701S are basically the same chip, aren't they ?
I'm currently exploring all the differences. There aren't a lot of them, but there are some.
So far I can see that default register values are different, previously unused registers are now used and there has been some reordering of how info is placed in registers [1] (return value for some commands is different). E.g AJ1N[1:0] has been moved from B102h to B101h [1]
Moreover, instructions to some commands have been changed as well as meaning of what data bits mean [2][3]. Also, new features have been added [2]; there is now PCLKS 3 for example.
You can see few differences in following images:
[1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png
[2] https://ibb.co/G79y235 - PCLKS2.png
P.S. this is second time I'm trying to send this e-mail so some of you might have received e-mail with the same text twice
Thank you for your time,
Paulo
On Thu, 22 Jun 2023 at 13:52, Linus Walleij <[email protected]> wrote:
>
> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
>
> > A lot of modifications to st7701 are required. I believe it would
> > result in a driver that doesn't look or work the same. e.g compare
> > delays between initialization sequences of panel-fannal-c3004 and
> > panel-st7701. I think it would be optimal to create st7701s driver and
> > have special handling for st7701s panels. If there was a flag for
> > whether panel is st7701 or st7701s it would end up looking like a
> > mess.
>
> What matters is if the original authors of the old st7701 driver are
> around and reviewing and testing patches at all. What we need is
> active maintainers. (Added Jagan, Marek & Maya).
>
> I buy the reasoning that the st7701s is perhaps substantially different
> from st7701.
>
> If st7701s is very different then I suppose it needs a separate driver,
> then all we need to to name the driver properly, i.e.
> panel-sitronix-st7701s.c.
I agree with what Linus mentioned.
1. If the panel is designed on top of ST7701 then add driver data on
the existing panel-st7701 driver with this panel.
2. If the panel is designed on top of ST7701S - ST7701 and ST7701S are
completely different in terms of the command set and init sequence
then add panel-sitronix-st7701s.c
3. If the panel is designed on top ST7701S and if the commands set is
the same as ST7701 but the init sequence is different then it is
possible to use the existing st7701 driver with the init sequence as
in driver data.
Thanks,
Jagan.
On 7/7/23 17:26, Paulo Pavacic wrote:
> Hello Marek,
Hi,
> čet, 6. srp 2023. u 17:26 Marek Vasut <[email protected]> napisao je:
>>
>> On 7/6/23 17:18, Paulo Pavacic wrote:
>>> Hello Linus,
>>>
>>> čet, 22. lip 2023. u 10:22 Linus Walleij <[email protected]> napisao je:
>>>>
>>>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
>>>>
>>>>> A lot of modifications to st7701 are required. I believe it would
>>>>> result in a driver that doesn't look or work the same. e.g compare
>>>>> delays between initialization sequences of panel-fannal-c3004 and
>>>>> panel-st7701. I think it would be optimal to create st7701s driver and
>>>>> have special handling for st7701s panels. If there was a flag for
>>>>> whether panel is st7701 or st7701s it would end up looking like a
>>>>> mess.
>>>>
>>>> What matters is if the original authors of the old st7701 driver are
>>>> around and reviewing and testing patches at all. What we need is
>>>> active maintainers. (Added Jagan, Marek & Maya).
>>>>
>>>> I buy the reasoning that the st7701s is perhaps substantially different
>>>> from st7701.
>>>>
>>>> If st7701s is very different then I suppose it needs a separate driver,
>>>> then all we need to to name the driver properly, i.e.
>>>> panel-sitronix-st7701s.c.
>>>
>>> I had in person talk with Paul Kocialkowski and I have concluded that
>>> this is the best solution.
>>> I believe I should rename it to st7701s due to the hardware changes. I
>>> would like to create V5 patch with driver renamed to st7701s.
>>> Please let me know if you agree / disagree.
>>
>> If I recall it right, the ST7701 and ST7701S are basically the same
>> chip, aren't they ?
>
> I'm currently exploring all the differences. There aren't a lot of
> differences, but there are some.
> So far I can see that default register values are different, new
> previously unused registers are now used and there has been some
> reordering of how info is placed in registers [1] (data bits are in
> different order). Moreover, instructions to some commands have been
> changed and meaning of what data bits mean [2][3]. Also, new features
> have been added [2]; there is now PCLKS 3 for example.
>
> You can see few differences in following images. Same images were
> attached in this mail:
> [1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png
> [2] https://ibb.co/G79y235 - PCLKS2.png
Ouch. I wonder if this is still something that can be abstracted out
with some helper accessor functions like:
if (model == ST7701)
write something
else
write the other layout
Or whether it makes sense to outright have a separate driver. The later
would introduce duplication, but maybe that much duplication is OK.
Hello all,
sub, 8. srp 2023. u 14:53 Marek Vasut <[email protected]> napisao je:
>
> On 7/7/23 17:26, Paulo Pavacic wrote:
> > Hello Marek,
>
> Hi,
>
> > čet, 6. srp 2023. u 17:26 Marek Vasut <[email protected]> napisao je:
> >>
> >> On 7/6/23 17:18, Paulo Pavacic wrote:
> >>> Hello Linus,
> >>>
> >>> čet, 22. lip 2023. u 10:22 Linus Walleij <[email protected]> napisao je:
> >>>>
> >>>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
> >>>>
> >>>>> A lot of modifications to st7701 are required. I believe it would
> >>>>> result in a driver that doesn't look or work the same. e.g compare
> >>>>> delays between initialization sequences of panel-fannal-c3004 and
> >>>>> panel-st7701. I think it would be optimal to create st7701s driver and
> >>>>> have special handling for st7701s panels. If there was a flag for
> >>>>> whether panel is st7701 or st7701s it would end up looking like a
> >>>>> mess.
> >>>>
> >>>> What matters is if the original authors of the old st7701 driver are
> >>>> around and reviewing and testing patches at all. What we need is
> >>>> active maintainers. (Added Jagan, Marek & Maya).
> >>>>
> >>>> I buy the reasoning that the st7701s is perhaps substantially different
> >>>> from st7701.
> >>>>
> >>>> If st7701s is very different then I suppose it needs a separate driver,
> >>>> then all we need to to name the driver properly, i.e.
> >>>> panel-sitronix-st7701s.c.
> >>>
> >>> I had in person talk with Paul Kocialkowski and I have concluded that
> >>> this is the best solution.
> >>> I believe I should rename it to st7701s due to the hardware changes. I
> >>> would like to create V5 patch with driver renamed to st7701s.
> >>> Please let me know if you agree / disagree.
> >>
> >> If I recall it right, the ST7701 and ST7701S are basically the same
> >> chip, aren't they ?
> >
> > I'm currently exploring all the differences. There aren't a lot of
> > differences, but there are some.
> > So far I can see that default register values are different, new
> > previously unused registers are now used and there has been some
> > reordering of how info is placed in registers [1] (data bits are in
> > different order). Moreover, instructions to some commands have been
> > changed and meaning of what data bits mean [2][3]. Also, new features
> > have been added [2]; there is now PCLKS 3 for example.
> >
> > You can see few differences in following images. Same images were
> > attached in this mail:
> > [1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png
> > [2] https://ibb.co/G79y235 - PCLKS2.png
>
> Ouch. I wonder if this is still something that can be abstracted out
> with some helper accessor functions like:
>
> if (model == ST7701)
> write something
> else
> write the other layout
>
> Or whether it makes sense to outright have a separate driver. The later
> would introduce duplication, but maybe that much duplication is OK.
I would like to create new driver because panel-st7701 seems to be
outdated and is using non-standard macro (ST7701_WRITE()) and for me
it is crashing kernel 5.15.
Does anyone have similar issues with it?
Br,
Paulo
On 7/12/23 14:07, Paulo Pavacic wrote:
> Hello all,
>
> sub, 8. srp 2023. u 14:53 Marek Vasut <[email protected]> napisao je:
>>
>> On 7/7/23 17:26, Paulo Pavacic wrote:
>>> Hello Marek,
>>
>> Hi,
>>
>>> čet, 6. srp 2023. u 17:26 Marek Vasut <[email protected]> napisao je:
>>>>
>>>> On 7/6/23 17:18, Paulo Pavacic wrote:
>>>>> Hello Linus,
>>>>>
>>>>> čet, 22. lip 2023. u 10:22 Linus Walleij <[email protected]> napisao je:
>>>>>>
>>>>>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
>>>>>>
>>>>>>> A lot of modifications to st7701 are required. I believe it would
>>>>>>> result in a driver that doesn't look or work the same. e.g compare
>>>>>>> delays between initialization sequences of panel-fannal-c3004 and
>>>>>>> panel-st7701. I think it would be optimal to create st7701s driver and
>>>>>>> have special handling for st7701s panels. If there was a flag for
>>>>>>> whether panel is st7701 or st7701s it would end up looking like a
>>>>>>> mess.
>>>>>>
>>>>>> What matters is if the original authors of the old st7701 driver are
>>>>>> around and reviewing and testing patches at all. What we need is
>>>>>> active maintainers. (Added Jagan, Marek & Maya).
>>>>>>
>>>>>> I buy the reasoning that the st7701s is perhaps substantially different
>>>>>> from st7701.
>>>>>>
>>>>>> If st7701s is very different then I suppose it needs a separate driver,
>>>>>> then all we need to to name the driver properly, i.e.
>>>>>> panel-sitronix-st7701s.c.
>>>>>
>>>>> I had in person talk with Paul Kocialkowski and I have concluded that
>>>>> this is the best solution.
>>>>> I believe I should rename it to st7701s due to the hardware changes. I
>>>>> would like to create V5 patch with driver renamed to st7701s.
>>>>> Please let me know if you agree / disagree.
>>>>
>>>> If I recall it right, the ST7701 and ST7701S are basically the same
>>>> chip, aren't they ?
>>>
>>> I'm currently exploring all the differences. There aren't a lot of
>>> differences, but there are some.
>>> So far I can see that default register values are different, new
>>> previously unused registers are now used and there has been some
>>> reordering of how info is placed in registers [1] (data bits are in
>>> different order). Moreover, instructions to some commands have been
>>> changed and meaning of what data bits mean [2][3]. Also, new features
>>> have been added [2]; there is now PCLKS 3 for example.
>>>
>>> You can see few differences in following images. Same images were
>>> attached in this mail:
>>> [1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png
>>> [2] https://ibb.co/G79y235 - PCLKS2.png
>>
>> Ouch. I wonder if this is still something that can be abstracted out
>> with some helper accessor functions like:
>>
>> if (model == ST7701)
>> write something
>> else
>> write the other layout
>>
>> Or whether it makes sense to outright have a separate driver. The later
>> would introduce duplication, but maybe that much duplication is OK.
>
> I would like to create new driver because panel-st7701 seems to be
> outdated and is using non-standard macro (ST7701_WRITE()
There is no such macro:
$ git grep ST7701_WRITE drivers/gpu/drm/panel/ | wc -l
0
There never was such a macro used in the driver either, are you sure you
are not using some hacked up patched downstream fork of the driver ?
$ git log -p next/master --
drivers/gpu/drm/panel/panel-sitronix-st7701.c | grep ST7701_WRITE | wc -l
0
> ) and for me
> it is crashing kernel 5.15.
Have you based all the aforementioned discussion and argumentation on
year and half old Linux 5.15.y code base too ?
If so, you are missing many patches:
$ git log --oneline --no-merges v5.15..next/master --
drivers/gpu/drm/panel/panel-sitronix-st7701.c
5a2854e577dc2 drm: panel: Add orientation support for st7701
e89838968ee44 drm: panel: Add Elida KD50T048A to Sitronix ST7701 driver
c62102165dd79 drm/panel/panel-sitronix-st7701: Remove panel on DSI
attach failure
49ee766b364ed drm/panel/panel-sitronix-st7701: Clean up CMDnBKx selection
c1cdee9b685a1 drm/panel/panel-sitronix-st7701: Fix RTNI calculation
57b2efce45ef5 drm/panel/panel-sitronix-st7701: Add Densitron
DMT028VGHMCMI-1A TFT
42542c7904cf2 drm/panel/panel-sitronix-st7701: Split GIP and init sequences
83b7a8e7e88e7 drm/panel/panel-sitronix-st7701: Parametrize voltage and
timing
de2b4917843cd drm/panel/panel-sitronix-st7701: Infer horizontal pixel
count from TFT mode
82f9cee25598a drm/panel/panel-sitronix-st7701: Adjust porch control
bitfield name
1ba85119afb5e drm/panel/panel-sitronix-st7701: Infer vertical line count
from TFT mode
779c84fea3dbd drm/panel/panel-sitronix-st7701: Make gamma correction TFT
specific
7fa8e07128ed6 drm/panel/panel-sitronix-st7701: Make voltage supplies
common to ST7701
a6c225be3da7e drm/panel/panel-sitronix-st7701: Enable DSI burst mode,
LPM, non-continuous clock
6f481afe220d3 drm/panel/panel-sitronix-st7701: Make DSI mode flags
common to ST7701
79abca2b39900 drm/mipi-dsi: Make remove callback return void
> Does anyone have similar issues with it?
No, I am using it in production.
Hi Marek,
sri, 12. srp 2023. u 15:42 Marek Vasut <[email protected]> napisao je:
>
> On 7/12/23 14:07, Paulo Pavacic wrote:
> > Hello all,
> >
> > sub, 8. srp 2023. u 14:53 Marek Vasut <[email protected]> napisao je:
> >>
> >> On 7/7/23 17:26, Paulo Pavacic wrote:
> >>> Hello Marek,
> >>
> >> Hi,
> >>
> >>> čet, 6. srp 2023. u 17:26 Marek Vasut <[email protected]> napisao je:
> >>>>
> >>>> On 7/6/23 17:18, Paulo Pavacic wrote:
> >>>>> Hello Linus,
> >>>>>
> >>>>> čet, 22. lip 2023. u 10:22 Linus Walleij <[email protected]> napisao je:
> >>>>>>
> >>>>>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <[email protected]> wrote:
> >>>>>>
> >>>>>>> A lot of modifications to st7701 are required. I believe it would
> >>>>>>> result in a driver that doesn't look or work the same. e.g compare
> >>>>>>> delays between initialization sequences of panel-fannal-c3004 and
> >>>>>>> panel-st7701. I think it would be optimal to create st7701s driver and
> >>>>>>> have special handling for st7701s panels. If there was a flag for
> >>>>>>> whether panel is st7701 or st7701s it would end up looking like a
> >>>>>>> mess.
> >>>>>>
> >>>>>> What matters is if the original authors of the old st7701 driver are
> >>>>>> around and reviewing and testing patches at all. What we need is
> >>>>>> active maintainers. (Added Jagan, Marek & Maya).
> >>>>>>
> >>>>>> I buy the reasoning that the st7701s is perhaps substantially different
> >>>>>> from st7701.
> >>>>>>
> >>>>>> If st7701s is very different then I suppose it needs a separate driver,
> >>>>>> then all we need to to name the driver properly, i.e.
> >>>>>> panel-sitronix-st7701s.c.
> >>>>>
> >>>>> I had in person talk with Paul Kocialkowski and I have concluded that
> >>>>> this is the best solution.
> >>>>> I believe I should rename it to st7701s due to the hardware changes. I
> >>>>> would like to create V5 patch with driver renamed to st7701s.
> >>>>> Please let me know if you agree / disagree.
> >>>>
> >>>> If I recall it right, the ST7701 and ST7701S are basically the same
> >>>> chip, aren't they ?
> >>>
> >>> I'm currently exploring all the differences. There aren't a lot of
> >>> differences, but there are some.
> >>> So far I can see that default register values are different, new
> >>> previously unused registers are now used and there has been some
> >>> reordering of how info is placed in registers [1] (data bits are in
> >>> different order). Moreover, instructions to some commands have been
> >>> changed and meaning of what data bits mean [2][3]. Also, new features
> >>> have been added [2]; there is now PCLKS 3 for example.
> >>>
> >>> You can see few differences in following images. Same images were
> >>> attached in this mail:
> >>> [1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png
> >>> [2] https://ibb.co/G79y235 - PCLKS2.png
> >>
> >> Ouch. I wonder if this is still something that can be abstracted out
> >> with some helper accessor functions like:
> >>
> >> if (model == ST7701)
> >> write something
> >> else
> >> write the other layout
> >>
> >> Or whether it makes sense to outright have a separate driver. The later
> >> would introduce duplication, but maybe that much duplication is OK.
> >
> > I would like to create new driver because panel-st7701 seems to be
> > outdated and is using non-standard macro (ST7701_WRITE()
>
> There is no such macro:
>
> $ git grep ST7701_WRITE drivers/gpu/drm/panel/ | wc -l
> 0
>
> There never was such a macro used in the driver either, are you sure you
> are not using some hacked up patched downstream fork of the driver ?
I meant ST7701_DSI() macro; It can be replaced with
mipi_dsi_generic_write_seq from kernel 6.3. Sorry for the confusion.
>
> $ git log -p next/master --
> drivers/gpu/drm/panel/panel-sitronix-st7701.c | grep ST7701_WRITE | wc -l
> 0
>
> > ) and for me
> > it is crashing kernel 5.15.
>
> Have you based all the aforementioned discussion and argumentation on
> year and half old Linux 5.15.y code base too ?
>
> If so, you are missing many patches:
>
> $ git log --oneline --no-merges v5.15..next/master --
> drivers/gpu/drm/panel/panel-sitronix-st7701.c
> 5a2854e577dc2 drm: panel: Add orientation support for st7701
> e89838968ee44 drm: panel: Add Elida KD50T048A to Sitronix ST7701 driver
> c62102165dd79 drm/panel/panel-sitronix-st7701: Remove panel on DSI
> attach failure
> 49ee766b364ed drm/panel/panel-sitronix-st7701: Clean up CMDnBKx selection
> c1cdee9b685a1 drm/panel/panel-sitronix-st7701: Fix RTNI calculation
> 57b2efce45ef5 drm/panel/panel-sitronix-st7701: Add Densitron
> DMT028VGHMCMI-1A TFT
> 42542c7904cf2 drm/panel/panel-sitronix-st7701: Split GIP and init sequences
> 83b7a8e7e88e7 drm/panel/panel-sitronix-st7701: Parametrize voltage and
> timing
> de2b4917843cd drm/panel/panel-sitronix-st7701: Infer horizontal pixel
> count from TFT mode
> 82f9cee25598a drm/panel/panel-sitronix-st7701: Adjust porch control
> bitfield name
> 1ba85119afb5e drm/panel/panel-sitronix-st7701: Infer vertical line count
> from TFT mode
> 779c84fea3dbd drm/panel/panel-sitronix-st7701: Make gamma correction TFT
> specific
> 7fa8e07128ed6 drm/panel/panel-sitronix-st7701: Make voltage supplies
> common to ST7701
> a6c225be3da7e drm/panel/panel-sitronix-st7701: Enable DSI burst mode,
> LPM, non-continuous clock
> 6f481afe220d3 drm/panel/panel-sitronix-st7701: Make DSI mode flags
> common to ST7701
> 79abca2b39900 drm/mipi-dsi: Make remove callback return void
I will try backporting those patches to 5.15 and applying them to see
whether it will then work with initialization sequences provided in
this merge request just to be sure not to have duplication. We are
still working on transitioning to newer kernel so for the time being
I'm using mostly 5.15.
On 5.15 kernel I have following kernel panic only with st7701 from the
panel drivers I have tried:
[ 20.255322] Kernel panic - not syncing: Asynchronous SError Interrupt
[ 20.255326] CPU: 1 PID: 36 Comm: kworker/1:1 Tainted: G O
5.15.77-5.15.77-2.1.0 #1
[ 20.255330] Hardware name: XXX i.MX8XX board:XXX (DT)
[ 20.255333] Workqueue: events fbcon_register_existing_fbs
[ 20.255339] Call trace:
[ 20.255340] dump_backtrace+0x0/0x19c
[ 20.255349] show_stack+0x18/0x70
[ 20.255354] dump_stack_lvl+0x68/0x84
[ 20.255360] dump_stack+0x18/0x34
[ 20.255365] panic+0x15c/0x308
[ 20.255370] nmi_panic+0x8c/0x90
[ 20.255376] arm64_serror_panic+0x6c/0x7c
[ 20.255382] do_serror+0x58/0x5c
[ 20.255388] el1h_64_error_handler+0x30/0x50
[ 20.255395] el1h_64_error+0x78/0x7c
[ 20.255399] _raw_spin_unlock_irq+0x18/0x50
[ 20.255404] sec_mipi_dsim_host_transfer+0x1a4/0x460
[ 20.255411] mipi_dsi_device_transfer+0x44/0x60
[ 20.255418] mipi_dsi_dcs_write_buffer+0x64/0xa0
[ 20.255424] st7701_init_sequence+0x2b0/0x47c
[ 20.255431] st7701_prepare+0x54/0x70
[ 20.255436] drm_panel_prepare+0x28/0x40
[ 20.255431] st7701_prepare+0x54/0x70
[ 20.255436] drm_panel_prepare+0x28/0x40
[ 20.255441] sec_mipi_dsim_bridge_atomic_enable+0x308/0x520
[ 20.255446] drm_atomic_bridge_chain_enable+0x54/0xd0
[ 20.255453] drm_atomic_helper_commit_modeset_enables+0x13c/0x24c
[ 20.255458] lcdif_drm_atomic_commit_tail+0x30/0x70
[ 20.255464] commit_tail+0xa0/0x180
[ 20.255468] drm_atomic_helper_commit+0x160/0x370
[ 20.255473] drm_atomic_commit+0x4c/0x60
[ 20.255477] drm_client_modeset_commit_atomic+0x1c8/0x260
[ 20.255485] drm_client_modeset_commit_locked+0x5c/0x1a0
[ 20.255491] drm_client_modeset_commit+0x30/0x60
[ 20.255496] drm_fb_helper_set_par+0xc8/0x120
[ 20.255504] fbcon_init+0x3c8/0x510
[ 20.255508] visual_init+0xb4/0x104
[ 20.255513] do_bind_con_driver.isra.0+0x1c4/0x394
[ 20.255518] do_take_over_console+0x144/0x1fc
[ 20.255523] do_fbcon_takeover+0x6c/0xe0
[ 20.255530] fbcon_fb_registered+0xf4/0x140
[ 20.255534] fbcon_register_existing_fbs+0x48/0x64
[ 20.255539] process_one_work+0x1d0/0x354
[ 20.255545] worker_thread+0x13c/0x470
[ 20.255550] kthread+0x150/0x160
[ 20.255556] ret_from_fork+0x10/0x20
[ 20.255563] SMP: stopping secondary CPUs
[ 20.255571] Kernel Offset: 0x80000 from 0xffff800008000000
[ 20.255574] PHYS_OFFSET: 0x40000000
[ 20.255563] SMP: stopping secondary CPUs
[ 20.255571] Kernel Offset: 0x80000 from 0xffff800008000000
[ 20.255574] PHYS_OFFSET: 0x40000000
[ 20.255575] CPU features: 0x00002401,20000842
[ 20.255579] Memory Limit: none
Raydium driver, for example, doesn't crash. It even almost works with
few modifications (distorted screen).
>
> > Does anyone have similar issues with it?
>
> No, I am using it in production.
Okay, then it might be something with my setup, I will check it out.
Thank you for your time,
Paulo
On 7/12/23 17:10, Paulo Pavacic wrote:
Hi,
[...]
>>>> Or whether it makes sense to outright have a separate driver. The later
>>>> would introduce duplication, but maybe that much duplication is OK.
>>>
>>> I would like to create new driver because panel-st7701 seems to be
>>> outdated and is using non-standard macro (ST7701_WRITE()
>>
>> There is no such macro:
>>
>> $ git grep ST7701_WRITE drivers/gpu/drm/panel/ | wc -l
>> 0
>>
>> There never was such a macro used in the driver either, are you sure you
>> are not using some hacked up patched downstream fork of the driver ?
>
> I meant ST7701_DSI() macro; It can be replaced with
> mipi_dsi_generic_write_seq from kernel 6.3. Sorry for the confusion.
OK
>> $ git log -p next/master --
>> drivers/gpu/drm/panel/panel-sitronix-st7701.c | grep ST7701_WRITE | wc -l
>> 0
>>
>>> ) and for me
>>> it is crashing kernel 5.15.
>>
>> Have you based all the aforementioned discussion and argumentation on
>> year and half old Linux 5.15.y code base too ?
>>
>> If so, you are missing many patches:
>>
>> $ git log --oneline --no-merges v5.15..next/master --
>> drivers/gpu/drm/panel/panel-sitronix-st7701.c
>> 5a2854e577dc2 drm: panel: Add orientation support for st7701
>> e89838968ee44 drm: panel: Add Elida KD50T048A to Sitronix ST7701 driver
>> c62102165dd79 drm/panel/panel-sitronix-st7701: Remove panel on DSI
>> attach failure
>> 49ee766b364ed drm/panel/panel-sitronix-st7701: Clean up CMDnBKx selection
>> c1cdee9b685a1 drm/panel/panel-sitronix-st7701: Fix RTNI calculation
>> 57b2efce45ef5 drm/panel/panel-sitronix-st7701: Add Densitron
>> DMT028VGHMCMI-1A TFT
>> 42542c7904cf2 drm/panel/panel-sitronix-st7701: Split GIP and init sequences
>> 83b7a8e7e88e7 drm/panel/panel-sitronix-st7701: Parametrize voltage and
>> timing
>> de2b4917843cd drm/panel/panel-sitronix-st7701: Infer horizontal pixel
>> count from TFT mode
>> 82f9cee25598a drm/panel/panel-sitronix-st7701: Adjust porch control
>> bitfield name
>> 1ba85119afb5e drm/panel/panel-sitronix-st7701: Infer vertical line count
>> from TFT mode
>> 779c84fea3dbd drm/panel/panel-sitronix-st7701: Make gamma correction TFT
>> specific
>> 7fa8e07128ed6 drm/panel/panel-sitronix-st7701: Make voltage supplies
>> common to ST7701
>> a6c225be3da7e drm/panel/panel-sitronix-st7701: Enable DSI burst mode,
>> LPM, non-continuous clock
>> 6f481afe220d3 drm/panel/panel-sitronix-st7701: Make DSI mode flags
>> common to ST7701
>> 79abca2b39900 drm/mipi-dsi: Make remove callback return void
>
> I will try backporting those patches to 5.15 and applying them to see
> whether it will then work with initialization sequences provided in
> this merge request just to be sure not to have duplication. We are
> still working on transitioning to newer kernel so for the time being
> I'm using mostly 5.15.
>
> On 5.15 kernel I have following kernel panic only with st7701 from the
> panel drivers I have tried:
>
> [ 20.255322] Kernel panic - not syncing: Asynchronous SError Interrupt
> [ 20.255326] CPU: 1 PID: 36 Comm: kworker/1:1 Tainted: G O
> 5.15.77-5.15.77-2.1.0 #1
The latest 5.15.y is 5.15.120 , can you re-test on that version ?
> [ 20.255330] Hardware name: XXX i.MX8XX board:XXX (DT)
Is this some NXP downstream kernel fork with thousands of extra patches?
The version string 2.1.0 looks very much like NXP versioning scheme ...
[...]