[PATCH v4 0/4] Add PinePhone Pro display support

[PATCH v4 0/4] Add PinePhone Pro display support

[Javier Martinez Canillas]

This series add support for the display present in the PinePhone Pro.

Patch #1 adds a driver for panels using the Himax HX8394 panel controller,
such as the HSD060BHW4 720x1440 TFT LCD panel present in the PinePhone Pro.

Patch #2 adds a devicetree binding schema for this driver and patch #3 adds
an entry for the driver in the MAINTAINERS file.

Finally patch #4 adds the needed devicetree nodes in the PinePhone Pro DTS,
to enable both the display and the touchscreen. This makes the upstream DTS
much more usable and will allow for example to enable support for the phone
in the Fedora distribution.

I only added myself as the maintainer for the driver because I don't know
if Kamil and Ondrej that worked in the driver would be interested. Please
let me know folks if you are, and I can add you too in the next revision.

This is a v4 of the patch-set that addresses issues pointed out in v3:

https://lists.freedesktop.org/archives/dri-devel/2022-December/384560.html

The patches were tested on a PinePhone Pro Explorer Edition using a Fedora
37 Workstation image.

Best regards,
Javier

Changes in v4:
- Add fallback "himax,hx8394" compatible for the panel controller (Jagan Teki).
- Add Tom Fitzhenry's Tested-by tag.
- Add Sam Ravnborg's Acked-by tag.
- Add Tom Fitzhenry's Tested-by tag.
- Keep the DTS nodes sorted alphabetically (Tom Fitzhenry).

Changes in v3:
- Fix example snippet for `make dt_binding_check` to pass (Krzysztof Kozlowski).
- Add Sam Ravnborg's reviwed-by tag.
- Move driver patch after one introducing the DT binding (Sam Ravnborg).

Changes in v2:
- Drop redundant "bindings" in subject (Krzysztof Kozlowski).
- Drop "device tree bindings" in title (Krzysztof Kozlowski).
- Put port next to other "true" properties (Krzysztof Kozlowski).
- Add Krzysztof Kozlowski's Reviewed-by tag.
- Add year to driver's copyright notice (Sam Ravnborg)
- Remove unused <video/display_timing.h> header include (Sam Ravnborg).
- Use mipi_dsi_dcs_write_seq() helper and drop custom macro (Sam Ravnborg).
- Drop unnecessary info messages and move useful one to debug (Sam Ravnborg).
- Fix regulator node names (Maya Matuszczyk).
- Drop non-existent "poweroff-in-suspend" property (Maya Matuszczyk).
- Remove unnecessary comments in panel node (Maya Matuszczyk).

Javier Martinez Canillas (2):
  dt-bindings: display: Add Himax HX8394 panel controller
  MAINTAINERS: Add entry for Himax HX8394 panel controller driver

Kamil Trzciński (1):
  drm: panel: Add Himax HX8394 panel controller driver

Ondrej Jirman (1):
  arm64: dts: rk3399-pinephone-pro: Add internal display support

 .../bindings/display/panel/himax,hx8394.yaml  |  75 +++
 MAINTAINERS                                   |   7 +
 .../dts/rockchip/rk3399-pinephone-pro.dts     | 123 +++++
 drivers/gpu/drm/panel/Kconfig                 |  12 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-himax-hx8394.c    | 446 ++++++++++++++++++
 6 files changed, 664 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-himax-hx8394.c

-- 
2.38.1

[PATCH v4 1/4] dt-bindings: display: Add Himax HX8394 panel controller

[Javier Martinez Canillas]

Add device tree bindings for panels based on the Himax HX8394 controller,
such as the HannStar HSD060BHW4 720x1440 TFT LCD panel that is connected
through a MIPI-DSI video interface.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

Changes in v4:
- Add fallback "himax,hx8394" compatible for the panel controller (Jagan Teki).

Changes in v3:
- Fix example snippet for `make dt_binding_check` to pass (Krzysztof Kozlowski).

Changes in v2:
- Drop redundant "bindings" in subject (Krzysztof Kozlowski).
- Drop "device tree bindings" in title (Krzysztof Kozlowski).
- Put port next to other "true" properties (Krzysztof Kozlowski).
- Add Krzysztof Kozlowski's Reviewed-by tag.

 .../bindings/display/panel/himax,hx8394.yaml  | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml b/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
new file mode 100644
index 000000000000..058b4eae68a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/himax,hx8394.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Himax HX8394 MIPI-DSI LCD panel controller
+
+maintainers:
+  - Javier Martinez Canillas <javierm@redhat.com>
+
+description:
+  Device tree bindings for panels based on the Himax HX8394 controller,
+  such as the HannStar HSD060BHW4 720x1440 TFT LCD panel connected with
+  a MIPI-DSI video interface.
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - hannstar,hsd060bhw4
+      - const: himax,hx8394
+
+  reg: true
+
+  reset-gpios: true
+
+  backlight: true
+
+  port: true
+
+  vcc-supply:
+    description: Panel power supply
+
+  iovcc-supply:
+    description: I/O voltage supply
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - backlight
+  - port
+  - vcc-supply
+  - iovcc-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    dsi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        panel@0 {
+            compatible = "hannstar,hsd060bhw4", "himax,hx8394";
+            reg = <0>;
+            vcc-supply = <&reg_2v8_p>;
+            iovcc-supply = <&reg_1v8_p>;
+            reset-gpios = <&gpio3 13 GPIO_ACTIVE_LOW>;
+            backlight = <&backlight>;
+
+            port {
+                mipi_in_panel: endpoint {
+                    remote-endpoint = <&mipi_out_panel>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.38.1

[PATCH v4 2/4] drm: panel: Add Himax HX8394 panel controller driver

[Javier Martinez Canillas]

From: Kamil Trzciński <ayufan@ayufan.eu>

The driver is for panels based on the Himax HX8394 controller, such as the
HannStar HSD060BHW4 720x1440 TFT LCD panel that uses a MIPI-DSI interface.

Signed-off-by: Kamil Trzciński <ayufan@ayufan.eu>
Co-developed-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Co-developed-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---

Changes in v4:
- Add Tom Fitzhenry's Tested-by tag.

Changes in v3:
- Add Sam Ravnborg's reviwed-by tag.
- Move driver patch after one introducing the DT binding (Sam Ravnborg).

Changes in v2:
- Add year to driver's copyright notice (Sam Ravnborg)
- Remove unused <video/display_timing.h> header include (Sam Ravnborg).
- Use mipi_dsi_dcs_write_seq() helper and drop custom macro (Sam Ravnborg).
- Drop unnecessary info messages and move useful one to debug (Sam Ravnborg).

 drivers/gpu/drm/panel/Kconfig              |  12 +
 drivers/gpu/drm/panel/Makefile             |   1 +
 drivers/gpu/drm/panel/panel-himax-hx8394.c | 446 +++++++++++++++++++++
 3 files changed, 459 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-himax-hx8394.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 737edcdf9eef..7ee9c83f09a7 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -154,6 +154,18 @@ config DRM_PANEL_FEIYANG_FY07024DI26A30D
 	  Say Y if you want to enable support for panels based on the
 	  Feiyang FY07024DI26A30-D MIPI-DSI interface.
 
+config DRM_PANEL_HIMAX_HX8394
+	tristate "HIMAX HX8394 MIPI-DSI LCD panels"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y if you want to enable support for panels based on the
+	  Himax HX8394 controller, such as the HannStar HSD060BHW4
+	  720x1440 TFT LCD panel that uses a MIPI-DSI interface.
+
+	  If M is selected the module will be called panel-himax-hx8394.
+
 config DRM_PANEL_ILITEK_IL9322
 	tristate "Ilitek ILI9322 320x240 QVGA panels"
 	depends on OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index f8f9d9f6a307..84c01adafd4c 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_DRM_PANEL_EBBG_FT8719) += panel-ebbg-ft8719.o
 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_HIMAX_HX8394) += panel-himax-hx8394.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
diff --git a/drivers/gpu/drm/panel/panel-himax-hx8394.c b/drivers/gpu/drm/panel/panel-himax-hx8394.c
new file mode 100644
index 000000000000..fc7a2c299f8d
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-himax-hx8394.c
@@ -0,0 +1,446 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for panels based on Himax HX8394 controller, such as:
+ *
+ * - HannStar HSD060BHW4 5.99" MIPI-DSI panel
+ *
+ * Copyright (C) 2021 Kamil Trzciński
+ *
+ * Based on drivers/gpu/drm/panel/panel-sitronix-st7703.c
+ * Copyright (C) Purism SPC 2019
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/media-bus-format.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+#define DRV_NAME "panel-himax-hx8394"
+
+/* Manufacturer specific commands sent via DSI, listed in HX8394-F datasheet */
+#define HX8394_CMD_SETSEQUENCE	  0xb0
+#define HX8394_CMD_SETPOWER	  0xb1
+#define HX8394_CMD_SETDISP	  0xb2
+#define HX8394_CMD_SETCYC	  0xb4
+#define HX8394_CMD_SETVCOM	  0xb6
+#define HX8394_CMD_SETTE	  0xb7
+#define HX8394_CMD_SETSENSOR	  0xb8
+#define HX8394_CMD_SETEXTC	  0xb9
+#define HX8394_CMD_SETMIPI	  0xba
+#define HX8394_CMD_SETOTP	  0xbb
+#define HX8394_CMD_SETREGBANK	  0xbd
+#define HX8394_CMD_UNKNOWN1	  0xc0
+#define HX8394_CMD_SETDGCLUT	  0xc1
+#define HX8394_CMD_SETID	  0xc3
+#define HX8394_CMD_SETDDB	  0xc4
+#define HX8394_CMD_UNKNOWN2	  0xc6
+#define HX8394_CMD_SETCABC	  0xc9
+#define HX8394_CMD_SETCABCGAIN	  0xca
+#define HX8394_CMD_SETPANEL	  0xcc
+#define HX8394_CMD_SETOFFSET	  0xd2
+#define HX8394_CMD_SETGIP0	  0xd3
+#define HX8394_CMD_UNKNOWN3	  0xd4
+#define HX8394_CMD_SETGIP1	  0xd5
+#define HX8394_CMD_SETGIP2	  0xd6
+#define HX8394_CMD_SETGPO	  0xd6
+#define HX8394_CMD_SETSCALING	  0xdd
+#define HX8394_CMD_SETIDLE	  0xdf
+#define HX8394_CMD_SETGAMMA	  0xe0
+#define HX8394_CMD_SETCHEMODE_DYN 0xe4
+#define HX8394_CMD_SETCHE	  0xe5
+#define HX8394_CMD_SETCESEL	  0xe6
+#define HX8394_CMD_SET_SP_CMD	  0xe9
+#define HX8394_CMD_SETREADINDEX	  0xfe
+#define HX8394_CMD_GETSPIREAD	  0xff
+
+struct hx8394 {
+	struct device *dev;
+	struct drm_panel panel;
+	struct gpio_desc *reset_gpio;
+	struct regulator *vcc;
+	struct regulator *iovcc;
+	bool prepared;
+
+	const struct hx8394_panel_desc *desc;
+};
+
+struct hx8394_panel_desc {
+	const struct drm_display_mode *mode;
+	unsigned int lanes;
+	unsigned long mode_flags;
+	enum mipi_dsi_pixel_format format;
+	int (*init_sequence)(struct hx8394 *ctx);
+};
+
+static inline struct hx8394 *panel_to_hx8394(struct drm_panel *panel)
+{
+	return container_of(panel, struct hx8394, panel);
+}
+
+static int hsd060bhw4_init_sequence(struct hx8394 *ctx)
+{
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+
+	/* 5.19.8 SETEXTC: Set extension command (B9h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETEXTC,
+			       0xff, 0x83, 0x94);
+
+	/* 5.19.2 SETPOWER: Set power (B1h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETPOWER,
+			  0x48, 0x11, 0x71, 0x09, 0x32, 0x24, 0x71, 0x31, 0x55, 0x30);
+
+	/* 5.19.9 SETMIPI: Set MIPI control (BAh) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETMIPI,
+			  0x63, 0x03, 0x68, 0x6b, 0xb2, 0xc0);
+
+	/* 5.19.3 SETDISP: Set display related register (B2h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETDISP,
+			  0x00, 0x80, 0x78, 0x0c, 0x07);
+
+	/* 5.19.4 SETCYC: Set display waveform cycles (B4h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETCYC,
+			  0x12, 0x63, 0x12, 0x63, 0x12, 0x63, 0x01, 0x0c, 0x7c, 0x55,
+			  0x00, 0x3f, 0x12, 0x6b, 0x12, 0x6b, 0x12, 0x6b, 0x01, 0x0c,
+			  0x7c);
+
+	/* 5.19.19 SETGIP0: Set GIP Option0 (D3h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETGIP0,
+			  0x00, 0x00, 0x00, 0x00, 0x3c, 0x1c, 0x00, 0x00, 0x32, 0x10,
+			  0x09, 0x00, 0x09, 0x32, 0x15, 0xad, 0x05, 0xad, 0x32, 0x00,
+			  0x00, 0x00, 0x00, 0x37, 0x03, 0x0b, 0x0b, 0x37, 0x00, 0x00,
+			  0x00, 0x0c, 0x40);
+
+	/* 5.19.20 Set GIP Option1 (D5h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETGIP1,
+			  0x19, 0x19, 0x18, 0x18, 0x1b, 0x1b, 0x1a, 0x1a, 0x00, 0x01,
+			  0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x20, 0x21, 0x18, 0x18,
+			  0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
+			  0x24, 0x25, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
+			  0x18, 0x18, 0x18, 0x18, 0x18, 0x18);
+
+	/* 5.19.21 Set GIP Option2 (D6h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETGIP2,
+			  0x18, 0x18, 0x19, 0x19, 0x1b, 0x1b, 0x1a, 0x1a, 0x07, 0x06,
+			  0x05, 0x04, 0x03, 0x02, 0x01, 0x00, 0x25, 0x24, 0x18, 0x18,
+			  0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
+			  0x21, 0x20, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
+			  0x18, 0x18, 0x18, 0x18, 0x18, 0x18);
+
+	/* 5.19.25 SETGAMMA: Set gamma curve related setting (E0h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETGAMMA,
+			  0x00, 0x04, 0x0c, 0x12, 0x14, 0x18, 0x1a, 0x18, 0x31, 0x3f,
+			  0x4d, 0x4c, 0x54, 0x65, 0x6b, 0x70, 0x7f, 0x82, 0x7e, 0x8a,
+			  0x99, 0x4a, 0x48, 0x49, 0x4b, 0x4a, 0x4c, 0x4b, 0x7f, 0x00,
+			  0x04, 0x0c, 0x11, 0x13, 0x17, 0x1a, 0x18, 0x31,
+			  0x3f, 0x4d, 0x4c, 0x54, 0x65, 0x6b, 0x70, 0x7f,
+			  0x82, 0x7e, 0x8a, 0x99, 0x4a, 0x48, 0x49, 0x4b,
+			  0x4a, 0x4c, 0x4b, 0x7f);
+
+	/* 5.19.17 SETPANEL (CCh) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETPANEL, 0x0b);
+
+	/* Unknown command, not listed in the HX8394-F datasheet */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_UNKNOWN1, 0x1f, 0x31);
+
+	/* 5.19.5 SETVCOM: Set VCOM voltage (B6h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETVCOM,
+			  0x7d, 0x7d);
+
+	/* Unknown command, not listed in the HX8394-F datasheet */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_UNKNOWN3,
+			  0x02);
+
+	/* 5.19.11 Set register bank (BDh) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETREGBANK, 0x01);
+
+	/* 5.19.2 SETPOWER: Set power (B1h) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETPOWER, 0x00);
+
+	/* 5.19.11 Set register bank (BDh) */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETREGBANK, 0x00);
+
+	/* Unknown command, not listed in the HX8394-F datasheet */
+	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_UNKNOWN3,
+			  0xed);
+
+	return 0;
+}
+
+static const struct drm_display_mode hsd060bhw4_mode = {
+	.hdisplay    = 720,
+	.hsync_start = 720 + 40,
+	.hsync_end   = 720 + 40 + 46,
+	.htotal	     = 720 + 40 + 46 + 40,
+	.vdisplay    = 1440,
+	.vsync_start = 1440 + 9,
+	.vsync_end   = 1440 + 9 + 7,
+	.vtotal	     = 1440 + 9 + 7 + 7,
+	.clock	     = 74250,
+	.flags	     = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	.width_mm    = 68,
+	.height_mm   = 136,
+};
+
+static const struct hx8394_panel_desc hsd060bhw4_desc = {
+	.mode = &hsd060bhw4_mode,
+	.lanes = 4,
+	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST,
+	.format = MIPI_DSI_FMT_RGB888,
+	.init_sequence = hsd060bhw4_init_sequence,
+};
+
+static int hx8394_enable(struct drm_panel *panel)
+{
+	struct hx8394 *ctx = panel_to_hx8394(panel);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	int ret;
+
+	ret = ctx->desc->init_sequence(ctx);
+	if (ret) {
+		dev_err(ctx->dev, "Panel init sequence failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
+	if (ret) {
+		dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
+		return ret;
+	}
+
+	/* Panel is operational 120 msec after reset */
+	msleep(120);
+
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret) {
+		dev_err(ctx->dev, "Failed to turn on the display: %d\n", ret);
+		goto sleep_in;
+	}
+
+	return 0;
+
+sleep_in:
+	/* This will probably fail, but let's try orderly power off anyway. */
+	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
+	if (!ret)
+		msleep(50);
+
+	return ret;
+}
+
+static int hx8394_disable(struct drm_panel *panel)
+{
+	struct hx8394 *ctx = panel_to_hx8394(panel);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	int ret;
+
+	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
+	if (ret) {
+		dev_err(ctx->dev, "Failed to enter sleep mode: %d\n", ret);
+		return ret;
+	}
+
+	msleep(50); /* about 3 frames */
+
+	return 0;
+}
+
+static int hx8394_unprepare(struct drm_panel *panel)
+{
+	struct hx8394 *ctx = panel_to_hx8394(panel);
+
+	if (!ctx->prepared)
+		return 0;
+
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+
+	regulator_disable(ctx->iovcc);
+	regulator_disable(ctx->vcc);
+
+	ctx->prepared = false;
+
+	return 0;
+}
+
+static int hx8394_prepare(struct drm_panel *panel)
+{
+	struct hx8394 *ctx = panel_to_hx8394(panel);
+	int ret;
+
+	if (ctx->prepared)
+		return 0;
+
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+
+	ret = regulator_enable(ctx->vcc);
+	if (ret) {
+		dev_err(ctx->dev, "Failed to enable vcc supply: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_enable(ctx->iovcc);
+	if (ret) {
+		dev_err(ctx->dev, "Failed to enable iovcc supply: %d\n", ret);
+		goto disable_vcc;
+	}
+
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+
+	msleep(180);
+
+	ctx->prepared = true;
+
+	return 0;
+
+disable_vcc:
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	regulator_disable(ctx->vcc);
+	return ret;
+}
+
+static int hx8394_get_modes(struct drm_panel *panel,
+			    struct drm_connector *connector)
+{
+	struct hx8394 *ctx = panel_to_hx8394(panel);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(connector->dev, ctx->desc->mode);
+	if (!mode) {
+		dev_err(ctx->dev, "Failed to add mode %ux%u@%u\n",
+			ctx->desc->mode->hdisplay, ctx->desc->mode->vdisplay,
+			drm_mode_vrefresh(ctx->desc->mode));
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	connector->display_info.width_mm = mode->width_mm;
+	connector->display_info.height_mm = mode->height_mm;
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs hx8394_drm_funcs = {
+	.disable   = hx8394_disable,
+	.unprepare = hx8394_unprepare,
+	.prepare   = hx8394_prepare,
+	.enable	   = hx8394_enable,
+	.get_modes = hx8394_get_modes,
+};
+
+static int hx8394_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct hx8394 *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ctx->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
+				     "Failed to get reset gpio\n");
+
+	mipi_dsi_set_drvdata(dsi, ctx);
+
+	ctx->dev = dev;
+	ctx->desc = of_device_get_match_data(dev);
+
+	dsi->mode_flags = ctx->desc->mode_flags;
+	dsi->format = ctx->desc->format;
+	dsi->lanes = ctx->desc->lanes;
+
+	ctx->vcc = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(ctx->vcc))
+		return dev_err_probe(dev, PTR_ERR(ctx->vcc),
+				     "Failed to request vcc regulator\n");
+
+	ctx->iovcc = devm_regulator_get(dev, "iovcc");
+	if (IS_ERR(ctx->iovcc))
+		return dev_err_probe(dev, PTR_ERR(ctx->iovcc),
+				     "Failed to request iovcc regulator\n");
+
+	drm_panel_init(&ctx->panel, dev, &hx8394_drm_funcs,
+		       DRM_MODE_CONNECTOR_DSI);
+
+	ret = drm_panel_of_backlight(&ctx->panel);
+	if (ret)
+		return ret;
+
+	drm_panel_add(&ctx->panel);
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "mipi_dsi_attach failed\n");
+		drm_panel_remove(&ctx->panel);
+		return ret;
+	}
+
+	dev_dbg(dev, "%ux%u@%u %ubpp dsi %udl - ready\n",
+		ctx->desc->mode->hdisplay, ctx->desc->mode->vdisplay,
+		drm_mode_vrefresh(ctx->desc->mode),
+		mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
+
+	return 0;
+}
+
+static void hx8394_shutdown(struct mipi_dsi_device *dsi)
+{
+	struct hx8394 *ctx = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	ret = drm_panel_disable(&ctx->panel);
+	if (ret < 0)
+		dev_err(&dsi->dev, "Failed to disable panel: %d\n", ret);
+
+	ret = drm_panel_unprepare(&ctx->panel);
+	if (ret < 0)
+		dev_err(&dsi->dev, "Failed to unprepare panel: %d\n", ret);
+}
+
+static void hx8394_remove(struct mipi_dsi_device *dsi)
+{
+	struct hx8394 *ctx = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	hx8394_shutdown(dsi);
+
+	ret = mipi_dsi_detach(dsi);
+	if (ret < 0)
+		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
+
+	drm_panel_remove(&ctx->panel);
+}
+
+static const struct of_device_id hx8394_of_match[] = {
+	{ .compatible = "hannstar,hsd060bhw4", .data = &hsd060bhw4_desc },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, hx8394_of_match);
+
+static struct mipi_dsi_driver hx8394_driver = {
+	.probe	= hx8394_probe,
+	.remove = hx8394_remove,
+	.shutdown = hx8394_shutdown,
+	.driver = {
+		.name = DRV_NAME,
+		.of_match_table = hx8394_of_match,
+	},
+};
+module_mipi_dsi_driver(hx8394_driver);
+
+MODULE_AUTHOR("Kamil Trzciński <ayufan@ayufan.eu>");
+MODULE_DESCRIPTION("DRM driver for Himax HX8394 based MIPI DSI panels");
+MODULE_LICENSE("GPL");
-- 
2.38.1

[PATCH v4 3/4] MAINTAINERS: Add entry for Himax HX8394 panel controller driver

[Javier Martinez Canillas]

Add myself as maintainer for the driver and devicetree bindings schema.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
---

Changes in v4:
- Add Sam Ravnborg's Acked-by tag.

 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7729a30b9609..c901e536303b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6551,6 +6551,13 @@ S:	Maintained
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	drivers/gpu/drm/tiny/gm12u320.c
 
+DRM DRIVER FOR HIMAX HX8394 MIPI-DSI LCD panels
+M:	Javier Martinez Canillas <javierm@redhat.com>
+S:	Maintained
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
+F:	drivers/gpu/drm/panel/panel-himax-hx8394.c
+
 DRM DRIVER FOR HX8357D PANELS
 M:	Emma Anholt <emma@anholt.net>
 S:	Maintained
-- 
2.38.1

[PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

From: Ondrej Jirman <megi@xff.cz>

The phone's display is using Hannstar LCD panel, and Goodix based
touchscreen. Support it.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
Co-developed-by: Martijn Braam <martijn@brixit.nl>
Signed-off-by: Martijn Braam <martijn@brixit.nl>
Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
Signed-off-by: Kamil Trzciński <ayufan@ayufan.eu>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: Tom Fitzhenry <tom@tom-fitzhenry.me.uk>
---

Changes in v4:
- Add Tom Fitzhenry's Tested-by tag.
- Keep the DTS nodes sorted alphabetically (Tom Fitzhenry).

Changes in v2:
- Fix regulator node names (Maya Matuszczyk).
- Drop non-existent "poweroff-in-suspend" property (Maya Matuszczyk).
- Remove unnecessary comments in panel node (Maya Matuszczyk).

 .../dts/rockchip/rk3399-pinephone-pro.dts     | 123 ++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
index 04403a76238b..0d48fbc5dbe4 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
@@ -25,6 +25,12 @@ aliases {
 		mmc2 = &sdhci;
 	};
 
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm0 0 1000000 0>;
+		pwm-delay-us = <10000>;
+	};
+
 	chosen {
 		stdout-path = "serial2:115200n8";
 	};
@@ -82,6 +88,32 @@ vcc1v8_codec: vcc1v8-codec-regulator {
 		vin-supply = <&vcc3v3_sys>;
 	};
 
+	/* MIPI DSI panel 1.8v supply */
+	vcc1v8_lcd: vcc1v8-lcd-regulator {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		regulator-name = "vcc1v8_lcd";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		vin-supply = <&vcc3v3_sys>;
+		gpio = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&display_pwren1>;
+	};
+
+	/* MIPI DSI panel 2.8v supply */
+	vcc2v8_lcd: vcc2v8-lcd-regulator {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		regulator-name = "vcc2v8_lcd";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		vin-supply = <&vcc3v3_sys>;
+		gpio = <&gpio3 RK_PA1 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&display_pwren>;
+	};
+
 	wifi_pwrseq: sdio-wifi-pwrseq {
 		compatible = "mmc-pwrseq-simple";
 		clocks = <&rk818 1>;
@@ -132,6 +164,11 @@ &emmc_phy {
 	status = "okay";
 };
 
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
+
 &i2c0 {
 	clock-frequency = <400000>;
 	i2c-scl-rising-time-ns = <168>;
@@ -214,6 +251,9 @@ vcc3v0_touch: LDO_REG2 {
 				regulator-name = "vcc3v0_touch";
 				regulator-min-microvolt = <3000000>;
 				regulator-max-microvolt = <3000000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			vcca1v8_codec: LDO_REG3 {
@@ -347,6 +387,25 @@ opp07 {
 	};
 };
 
+&i2c3 {
+	i2c-scl-rising-time-ns = <450>;
+	i2c-scl-falling-time-ns = <15>;
+	status = "okay";
+
+	touchscreen@14 {
+		compatible = "goodix,gt917s";
+		reg = <0x14>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <RK_PB5 IRQ_TYPE_EDGE_RISING>;
+		irq-gpios = <&gpio3 RK_PB5 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
+		AVDD28-supply = <&vcc3v0_touch>;
+		VDDIO-supply = <&vcc3v0_touch>;
+		touchscreen-size-x = <720>;
+		touchscreen-size-y = <1440>;
+	};
+};
+
 &io_domains {
 	bt656-supply = <&vcc1v8_dvp>;
 	audio-supply = <&vcca1v8_codec>;
@@ -355,6 +414,40 @@ &io_domains {
 	status = "okay";
 };
 
+&mipi_dsi {
+	status = "okay";
+	clock-master;
+
+	ports {
+		mipi_out: port@1 {
+			#address-cells = <0>;
+			#size-cells = <0>;
+			reg = <1>;
+
+			mipi_out_panel: endpoint {
+				remote-endpoint = <&mipi_in_panel>;
+			};
+		};
+	};
+
+	panel@0 {
+		compatible = "hannstar,hsd060bhw4", "himax,hx8394";
+		reg = <0>;
+		backlight = <&backlight>;
+		reset-gpios = <&gpio4 RK_PD1 GPIO_ACTIVE_LOW>;
+		vcc-supply = <&vcc2v8_lcd>;
+		iovcc-supply = <&vcc1v8_lcd>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&display_rst_l>;
+
+		port {
+			mipi_in_panel: endpoint {
+				remote-endpoint = <&mipi_out_panel>;
+			};
+		};
+	};
+};
+
 &pmu_io_domains {
 	pmu1830-supply = <&vcc_1v8>;
 	status = "okay";
@@ -367,6 +460,20 @@ pwrbtn_pin: pwrbtn-pin {
 		};
 	};
 
+	dsi {
+		display_rst_l: display-rst-l {
+			rockchip,pins = <4 RK_PD1 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		display_pwren: display-pwren {
+			rockchip,pins = <3 RK_PA1 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		display_pwren1: display-pwren1 {
+			rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+	};
+
 	pmic {
 		pmic_int_l: pmic-int-l {
 			rockchip,pins = <1 RK_PC5 RK_FUNC_GPIO &pcfg_pull_up>;
@@ -408,6 +515,10 @@ bt_reset_pin: bt-reset-pin {
 	};
 };
 
+&pwm0 {
+	status = "okay";
+};
+
 &sdio0 {
 	bus-width = <4>;
 	cap-sd-highspeed;
@@ -472,3 +583,15 @@ bluetooth {
 &uart2 {
 	status = "okay";
 };
+
+&vopb {
+	status = "okay";
+	assigned-clocks = <&cru DCLK_VOP0_DIV>, <&cru DCLK_VOP0>,
+			  <&cru ACLK_VOP0>, <&cru HCLK_VOP0>;
+	assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
+	assigned-clock-parents = <&cru PLL_CPLL>, <&cru DCLK_VOP0_FRAC>;
+};
+
+&vopb_mmu {
+	status = "okay";
+};
-- 
2.38.1

Re: [PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Ondřej Jirman]

Hi Javier,

On Fri, Dec 30, 2022 at 12:31:54PM +0100, Javier Martinez Canillas wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> The phone's display is using Hannstar LCD panel, and Goodix based
> touchscreen. Support it.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Co-developed-by: Martijn Braam <martijn@brixit.nl>
> Signed-off-by: Martijn Braam <martijn@brixit.nl>
> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: Tom Fitzhenry <tom@tom-fitzhenry.me.uk>
> ---
> 
> Changes in v4:
> - Add Tom Fitzhenry's Tested-by tag.
> - Keep the DTS nodes sorted alphabetically (Tom Fitzhenry).
> 
> Changes in v2:
> - Fix regulator node names (Maya Matuszczyk).
> - Drop non-existent "poweroff-in-suspend" property (Maya Matuszczyk).
> - Remove unnecessary comments in panel node (Maya Matuszczyk).
> 
>  .../dts/rockchip/rk3399-pinephone-pro.dts     | 123 ++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> index 04403a76238b..0d48fbc5dbe4 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -25,6 +25,12 @@ aliases {
>  		mmc2 = &sdhci;
>  	};
>  
> +	backlight: backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pwm0 0 1000000 0>;
> +		pwm-delay-us = <10000>;
> +	};
> +
>  	chosen {
>  		stdout-path = "serial2:115200n8";
>  	};
> @@ -82,6 +88,32 @@ vcc1v8_codec: vcc1v8-codec-regulator {
>  		vin-supply = <&vcc3v3_sys>;
>  	};
>  
> +	/* MIPI DSI panel 1.8v supply */
> +	vcc1v8_lcd: vcc1v8-lcd-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		regulator-name = "vcc1v8_lcd";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		vin-supply = <&vcc3v3_sys>;
> +		gpio = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&display_pwren1>;
> +	};
> +
> +	/* MIPI DSI panel 2.8v supply */
> +	vcc2v8_lcd: vcc2v8-lcd-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		regulator-name = "vcc2v8_lcd";
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <2800000>;
> +		vin-supply = <&vcc3v3_sys>;
> +		gpio = <&gpio3 RK_PA1 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&display_pwren>;
> +	};
> +
>  	wifi_pwrseq: sdio-wifi-pwrseq {
>  		compatible = "mmc-pwrseq-simple";
>  		clocks = <&rk818 1>;
> @@ -132,6 +164,11 @@ &emmc_phy {
>  	status = "okay";
>  };
>  
> +&gpu {
> +	mali-supply = <&vdd_gpu>;
> +	status = "okay";
> +};
> +
>  &i2c0 {
>  	clock-frequency = <400000>;
>  	i2c-scl-rising-time-ns = <168>;
> @@ -214,6 +251,9 @@ vcc3v0_touch: LDO_REG2 {
>  				regulator-name = "vcc3v0_touch";
>  				regulator-min-microvolt = <3000000>;
>  				regulator-max-microvolt = <3000000>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +				};

You're instructing RK818 to shut down the regulator for touch controller during
suspend, but I think Goodix driver expects touch controller to be kept powered on
during suspend. Am I missing something?

https://elixir.bootlin.com/linux/latest/source/drivers/input/touchscreen/goodix.c#L1405

>  			};
>  
>  			vcca1v8_codec: LDO_REG3 {
> @@ -347,6 +387,25 @@ opp07 {
>  	};
>  };
>  
> +&i2c3 {
> +	i2c-scl-rising-time-ns = <450>;
> +	i2c-scl-falling-time-ns = <15>;
> +	status = "okay";
> +
> +	touchscreen@14 {
> +		compatible = "goodix,gt917s";

This is not the correct compatible. Pinephone Pro uses Goodix GT1158:

Goodix-TS 3-0014: ID 1158, version: 0100
Goodix-TS 3-0014: Direct firmware load for goodix_1158_cfg.bin failed with error -2


> +		reg = <0x14>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <RK_PB5 IRQ_TYPE_EDGE_RISING>;
> +		irq-gpios = <&gpio3 RK_PB5 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio3 RK_PB4 GPIO_ACTIVE_HIGH>;
> +		AVDD28-supply = <&vcc3v0_touch>;
> +		VDDIO-supply = <&vcc3v0_touch>;
> +		touchscreen-size-x = <720>;
> +		touchscreen-size-y = <1440>;
> +	};
> +};
> +
>  &io_domains {
>  	bt656-supply = <&vcc1v8_dvp>;
>  	audio-supply = <&vcca1v8_codec>;
> @@ -355,6 +414,40 @@ &io_domains {
>  	status = "okay";
>  };
>  
> +&mipi_dsi {
> +	status = "okay";
> +	clock-master;
> +
> +	ports {
> +		mipi_out: port@1 {
> +			#address-cells = <0>;
> +			#size-cells = <0>;
> +			reg = <1>;
> +
> +			mipi_out_panel: endpoint {
> +				remote-endpoint = <&mipi_in_panel>;
> +			};
> +		};
> +	};
> +
> +	panel@0 {
> +		compatible = "hannstar,hsd060bhw4", "himax,hx8394";
> +		reg = <0>;
> +		backlight = <&backlight>;
> +		reset-gpios = <&gpio4 RK_PD1 GPIO_ACTIVE_LOW>;
> +		vcc-supply = <&vcc2v8_lcd>;
> +		iovcc-supply = <&vcc1v8_lcd>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&display_rst_l>;
> +
> +		port {
> +			mipi_in_panel: endpoint {
> +				remote-endpoint = <&mipi_out_panel>;
> +			};
> +		};
> +	};
> +};
> +
>  &pmu_io_domains {
>  	pmu1830-supply = <&vcc_1v8>;
>  	status = "okay";
> @@ -367,6 +460,20 @@ pwrbtn_pin: pwrbtn-pin {
>  		};
>  	};
>  
> +	dsi {
> +		display_rst_l: display-rst-l {
> +			rockchip,pins = <4 RK_PD1 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
> +
> +		display_pwren: display-pwren {
> +			rockchip,pins = <3 RK_PA1 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
> +
> +		display_pwren1: display-pwren1 {
> +			rockchip,pins = <3 RK_PA5 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
> +	};
> +
>  	pmic {
>  		pmic_int_l: pmic-int-l {
>  			rockchip,pins = <1 RK_PC5 RK_FUNC_GPIO &pcfg_pull_up>;
> @@ -408,6 +515,10 @@ bt_reset_pin: bt-reset-pin {
>  	};
>  };
>  
> +&pwm0 {
> +	status = "okay";
> +};
> +
>  &sdio0 {
>  	bus-width = <4>;
>  	cap-sd-highspeed;
> @@ -472,3 +583,15 @@ bluetooth {
>  &uart2 {
>  	status = "okay";
>  };
> +
> +&vopb {
> +	status = "okay";
> +	assigned-clocks = <&cru DCLK_VOP0_DIV>, <&cru DCLK_VOP0>,
> +			  <&cru ACLK_VOP0>, <&cru HCLK_VOP0>;
> +	assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
> +	assigned-clock-parents = <&cru PLL_CPLL>, <&cru DCLK_VOP0_FRAC>;
> +};

So here you're putting a fractional clock into path between CPLL -> VOP0_DIV
-> DCLK_VOP0_FRAC -> DCLK_VOP0.

Fractional clocks require 20x difference between input and output rates, and
CPLL is 800Mhz IIRC, while you require 74.25MHz DCLK, so this will not work
correctly.

Even if this somehow works by fractional clock being bypassed, I did not design
the panel mode to be used with CPLL's 800 MHz, but with GPLL frequecy of 594 MHz.

GPLL 594/74.25 = 8  (integral divider without the need for fractional clock)
CPLL 800/74.25 = ~10.77441077441077441077

If you really want to use fractional clock, you'd need to parent it to VPLL
and set VPLL really high, like close to 2GHz.

kind regards,
	o.

> +&vopb_mmu {
> +	status = "okay";
> +};
> -- 
> 2.38.1
> 

Re: [PATCH v4 2/4] drm: panel: Add Himax HX8394 panel controller driver

[Ondřej Jirman]

Hi Javier,

On Fri, Dec 30, 2022 at 12:31:52PM +0100, Javier Martinez Canillas wrote:
> From: Kamil Trzciński <ayufan@ayufan.eu>
> 
> The driver is for panels based on the Himax HX8394 controller, such as the
> HannStar HSD060BHW4 720x1440 TFT LCD panel that uses a MIPI-DSI interface.

I see you've removed debug printks from enable/disable/prepare/unprepare
hooks. Have you tested the driver thoroughly with various DRM apps,
with DPM/suspend/resume, etc.?

The dw-mipi-dsi driver does some unorthodox things[1], that can lead to unbalanced
calls to these functions in some situations, and that's why all these printks
were there. To ensure the driver hooks are called correctly, while preparing
the code for upstreaming. This lead to broken display in some situations during
suspend/resume.

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L868

Also, have you checked the clocks are actually configured correctly by the
rk3399 cru driver? I have a lot of trouble with that, too. clk driver sometimes
selects the fractional clock, but does not give it the necessary >20x difference
between input/output clock rates. You'll only notice if you measure clock rates
directly, by looking at actual refresh rate, by using some testing DRM app.
Clock subsystem sometimes shuffles things around if you switch VOPs and use big
VOP for mipi-dsi display, instead of the default small VOP.

I'll test this patchset in a few days against purely mainline code, but I'm
pretty sure looking at the modes you use, that this will not work on some
Pinephone Pro's, and will cause display corruption when you fix your clock
setup, so that CRU actually outputs 74.25MHz as requested by the mode. (Which
can be fixed by this patch https://github.com/megous/linux/commit/f7ee16f12ee8a44ee2472f2967ca27768106e00f)

regards,
	o.

> Signed-off-by: Kamil Trzciński <ayufan@ayufan.eu>
> Co-developed-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Co-developed-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> ---
> 
> Changes in v4:
> - Add Tom Fitzhenry's Tested-by tag.
> 
> Changes in v3:
> - Add Sam Ravnborg's reviwed-by tag.
> - Move driver patch after one introducing the DT binding (Sam Ravnborg).
> 
> Changes in v2:
> - Add year to driver's copyright notice (Sam Ravnborg)
> - Remove unused <video/display_timing.h> header include (Sam Ravnborg).
> - Use mipi_dsi_dcs_write_seq() helper and drop custom macro (Sam Ravnborg).
> - Drop unnecessary info messages and move useful one to debug (Sam Ravnborg).
> 
>  drivers/gpu/drm/panel/Kconfig              |  12 +
>  drivers/gpu/drm/panel/Makefile             |   1 +
>  drivers/gpu/drm/panel/panel-himax-hx8394.c | 446 +++++++++++++++++++++
>  3 files changed, 459 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-himax-hx8394.c
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 737edcdf9eef..7ee9c83f09a7 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -154,6 +154,18 @@ config DRM_PANEL_FEIYANG_FY07024DI26A30D
>  	  Say Y if you want to enable support for panels based on the
>  	  Feiyang FY07024DI26A30-D MIPI-DSI interface.
>  
> +config DRM_PANEL_HIMAX_HX8394
> +	tristate "HIMAX HX8394 MIPI-DSI LCD panels"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	help
> +	  Say Y if you want to enable support for panels based on the
> +	  Himax HX8394 controller, such as the HannStar HSD060BHW4
> +	  720x1440 TFT LCD panel that uses a MIPI-DSI interface.
> +
> +	  If M is selected the module will be called panel-himax-hx8394.
> +
>  config DRM_PANEL_ILITEK_IL9322
>  	tristate "Ilitek ILI9322 320x240 QVGA panels"
>  	depends on OF && SPI
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index f8f9d9f6a307..84c01adafd4c 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_DRM_PANEL_EBBG_FT8719) += panel-ebbg-ft8719.o
>  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_HIMAX_HX8394) += panel-himax-hx8394.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
> diff --git a/drivers/gpu/drm/panel/panel-himax-hx8394.c b/drivers/gpu/drm/panel/panel-himax-hx8394.c
> new file mode 100644
> index 000000000000..fc7a2c299f8d
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-himax-hx8394.c
> @@ -0,0 +1,446 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for panels based on Himax HX8394 controller, such as:
> + *
> + * - HannStar HSD060BHW4 5.99" MIPI-DSI panel
> + *
> + * Copyright (C) 2021 Kamil Trzciński
> + *
> + * Based on drivers/gpu/drm/panel/panel-sitronix-st7703.c
> + * Copyright (C) Purism SPC 2019
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <video/mipi_display.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +
> +#define DRV_NAME "panel-himax-hx8394"
> +
> +/* Manufacturer specific commands sent via DSI, listed in HX8394-F datasheet */
> +#define HX8394_CMD_SETSEQUENCE	  0xb0
> +#define HX8394_CMD_SETPOWER	  0xb1
> +#define HX8394_CMD_SETDISP	  0xb2
> +#define HX8394_CMD_SETCYC	  0xb4
> +#define HX8394_CMD_SETVCOM	  0xb6
> +#define HX8394_CMD_SETTE	  0xb7
> +#define HX8394_CMD_SETSENSOR	  0xb8
> +#define HX8394_CMD_SETEXTC	  0xb9
> +#define HX8394_CMD_SETMIPI	  0xba
> +#define HX8394_CMD_SETOTP	  0xbb
> +#define HX8394_CMD_SETREGBANK	  0xbd
> +#define HX8394_CMD_UNKNOWN1	  0xc0
> +#define HX8394_CMD_SETDGCLUT	  0xc1
> +#define HX8394_CMD_SETID	  0xc3
> +#define HX8394_CMD_SETDDB	  0xc4
> +#define HX8394_CMD_UNKNOWN2	  0xc6
> +#define HX8394_CMD_SETCABC	  0xc9
> +#define HX8394_CMD_SETCABCGAIN	  0xca
> +#define HX8394_CMD_SETPANEL	  0xcc
> +#define HX8394_CMD_SETOFFSET	  0xd2
> +#define HX8394_CMD_SETGIP0	  0xd3
> +#define HX8394_CMD_UNKNOWN3	  0xd4
> +#define HX8394_CMD_SETGIP1	  0xd5
> +#define HX8394_CMD_SETGIP2	  0xd6
> +#define HX8394_CMD_SETGPO	  0xd6
> +#define HX8394_CMD_SETSCALING	  0xdd
> +#define HX8394_CMD_SETIDLE	  0xdf
> +#define HX8394_CMD_SETGAMMA	  0xe0
> +#define HX8394_CMD_SETCHEMODE_DYN 0xe4
> +#define HX8394_CMD_SETCHE	  0xe5
> +#define HX8394_CMD_SETCESEL	  0xe6
> +#define HX8394_CMD_SET_SP_CMD	  0xe9
> +#define HX8394_CMD_SETREADINDEX	  0xfe
> +#define HX8394_CMD_GETSPIREAD	  0xff
> +
> +struct hx8394 {
> +	struct device *dev;
> +	struct drm_panel panel;
> +	struct gpio_desc *reset_gpio;
> +	struct regulator *vcc;
> +	struct regulator *iovcc;
> +	bool prepared;
> +
> +	const struct hx8394_panel_desc *desc;
> +};
> +
> +struct hx8394_panel_desc {
> +	const struct drm_display_mode *mode;
> +	unsigned int lanes;
> +	unsigned long mode_flags;
> +	enum mipi_dsi_pixel_format format;
> +	int (*init_sequence)(struct hx8394 *ctx);
> +};
> +
> +static inline struct hx8394 *panel_to_hx8394(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct hx8394, panel);
> +}
> +
> +static int hsd060bhw4_init_sequence(struct hx8394 *ctx)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +
> +	/* 5.19.8 SETEXTC: Set extension command (B9h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETEXTC,
> +			       0xff, 0x83, 0x94);
> +
> +	/* 5.19.2 SETPOWER: Set power (B1h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETPOWER,
> +			  0x48, 0x11, 0x71, 0x09, 0x32, 0x24, 0x71, 0x31, 0x55, 0x30);
> +
> +	/* 5.19.9 SETMIPI: Set MIPI control (BAh) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETMIPI,
> +			  0x63, 0x03, 0x68, 0x6b, 0xb2, 0xc0);
> +
> +	/* 5.19.3 SETDISP: Set display related register (B2h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETDISP,
> +			  0x00, 0x80, 0x78, 0x0c, 0x07);
> +
> +	/* 5.19.4 SETCYC: Set display waveform cycles (B4h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETCYC,
> +			  0x12, 0x63, 0x12, 0x63, 0x12, 0x63, 0x01, 0x0c, 0x7c, 0x55,
> +			  0x00, 0x3f, 0x12, 0x6b, 0x12, 0x6b, 0x12, 0x6b, 0x01, 0x0c,
> +			  0x7c);
> +
> +	/* 5.19.19 SETGIP0: Set GIP Option0 (D3h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETGIP0,
> +			  0x00, 0x00, 0x00, 0x00, 0x3c, 0x1c, 0x00, 0x00, 0x32, 0x10,
> +			  0x09, 0x00, 0x09, 0x32, 0x15, 0xad, 0x05, 0xad, 0x32, 0x00,
> +			  0x00, 0x00, 0x00, 0x37, 0x03, 0x0b, 0x0b, 0x37, 0x00, 0x00,
> +			  0x00, 0x0c, 0x40);
> +
> +	/* 5.19.20 Set GIP Option1 (D5h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETGIP1,
> +			  0x19, 0x19, 0x18, 0x18, 0x1b, 0x1b, 0x1a, 0x1a, 0x00, 0x01,
> +			  0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x20, 0x21, 0x18, 0x18,
> +			  0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
> +			  0x24, 0x25, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
> +			  0x18, 0x18, 0x18, 0x18, 0x18, 0x18);
> +
> +	/* 5.19.21 Set GIP Option2 (D6h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETGIP2,
> +			  0x18, 0x18, 0x19, 0x19, 0x1b, 0x1b, 0x1a, 0x1a, 0x07, 0x06,
> +			  0x05, 0x04, 0x03, 0x02, 0x01, 0x00, 0x25, 0x24, 0x18, 0x18,
> +			  0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
> +			  0x21, 0x20, 0x18, 0x18, 0x18, 0x18, 0x18, 0x18,
> +			  0x18, 0x18, 0x18, 0x18, 0x18, 0x18);
> +
> +	/* 5.19.25 SETGAMMA: Set gamma curve related setting (E0h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETGAMMA,
> +			  0x00, 0x04, 0x0c, 0x12, 0x14, 0x18, 0x1a, 0x18, 0x31, 0x3f,
> +			  0x4d, 0x4c, 0x54, 0x65, 0x6b, 0x70, 0x7f, 0x82, 0x7e, 0x8a,
> +			  0x99, 0x4a, 0x48, 0x49, 0x4b, 0x4a, 0x4c, 0x4b, 0x7f, 0x00,
> +			  0x04, 0x0c, 0x11, 0x13, 0x17, 0x1a, 0x18, 0x31,
> +			  0x3f, 0x4d, 0x4c, 0x54, 0x65, 0x6b, 0x70, 0x7f,
> +			  0x82, 0x7e, 0x8a, 0x99, 0x4a, 0x48, 0x49, 0x4b,
> +			  0x4a, 0x4c, 0x4b, 0x7f);
> +
> +	/* 5.19.17 SETPANEL (CCh) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETPANEL, 0x0b);
> +
> +	/* Unknown command, not listed in the HX8394-F datasheet */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_UNKNOWN1, 0x1f, 0x31);
> +
> +	/* 5.19.5 SETVCOM: Set VCOM voltage (B6h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETVCOM,
> +			  0x7d, 0x7d);
> +
> +	/* Unknown command, not listed in the HX8394-F datasheet */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_UNKNOWN3,
> +			  0x02);
> +
> +	/* 5.19.11 Set register bank (BDh) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETREGBANK, 0x01);
> +
> +	/* 5.19.2 SETPOWER: Set power (B1h) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETPOWER, 0x00);
> +
> +	/* 5.19.11 Set register bank (BDh) */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_SETREGBANK, 0x00);
> +
> +	/* Unknown command, not listed in the HX8394-F datasheet */
> +	mipi_dsi_dcs_write_seq(dsi, HX8394_CMD_UNKNOWN3,
> +			  0xed);
> +
> +	return 0;
> +}
> +
> +static const struct drm_display_mode hsd060bhw4_mode = {
> +	.hdisplay    = 720,
> +	.hsync_start = 720 + 40,
> +	.hsync_end   = 720 + 40 + 46,
> +	.htotal	     = 720 + 40 + 46 + 40,
> +	.vdisplay    = 1440,
> +	.vsync_start = 1440 + 9,
> +	.vsync_end   = 1440 + 9 + 7,
> +	.vtotal	     = 1440 + 9 + 7 + 7,
> +	.clock	     = 74250,
> +	.flags	     = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +	.width_mm    = 68,
> +	.height_mm   = 136,
> +};
> +
> +static const struct hx8394_panel_desc hsd060bhw4_desc = {
> +	.mode = &hsd060bhw4_mode,
> +	.lanes = 4,
> +	.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST,
> +	.format = MIPI_DSI_FMT_RGB888,
> +	.init_sequence = hsd060bhw4_init_sequence,
> +};
> +
> +static int hx8394_enable(struct drm_panel *panel)
> +{
> +	struct hx8394 *ctx = panel_to_hx8394(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	int ret;
> +
> +	ret = ctx->desc->init_sequence(ctx);
> +	if (ret) {
> +		dev_err(ctx->dev, "Panel init sequence failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret) {
> +		dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Panel is operational 120 msec after reset */
> +	msleep(120);
> +
> +	ret = mipi_dsi_dcs_set_display_on(dsi);
> +	if (ret) {
> +		dev_err(ctx->dev, "Failed to turn on the display: %d\n", ret);
> +		goto sleep_in;
> +	}
> +
> +	return 0;
> +
> +sleep_in:
> +	/* This will probably fail, but let's try orderly power off anyway. */
> +	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> +	if (!ret)
> +		msleep(50);
> +
> +	return ret;
> +}
> +
> +static int hx8394_disable(struct drm_panel *panel)
> +{
> +	struct hx8394 *ctx = panel_to_hx8394(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	int ret;
> +
> +	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> +	if (ret) {
> +		dev_err(ctx->dev, "Failed to enter sleep mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	msleep(50); /* about 3 frames */
> +
> +	return 0;
> +}
> +
> +static int hx8394_unprepare(struct drm_panel *panel)
> +{
> +	struct hx8394 *ctx = panel_to_hx8394(panel);
> +
> +	if (!ctx->prepared)
> +		return 0;
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +
> +	regulator_disable(ctx->iovcc);
> +	regulator_disable(ctx->vcc);
> +
> +	ctx->prepared = false;
> +
> +	return 0;
> +}
> +
> +static int hx8394_prepare(struct drm_panel *panel)
> +{
> +	struct hx8394 *ctx = panel_to_hx8394(panel);
> +	int ret;
> +
> +	if (ctx->prepared)
> +		return 0;
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +
> +	ret = regulator_enable(ctx->vcc);
> +	if (ret) {
> +		dev_err(ctx->dev, "Failed to enable vcc supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(ctx->iovcc);
> +	if (ret) {
> +		dev_err(ctx->dev, "Failed to enable iovcc supply: %d\n", ret);
> +		goto disable_vcc;
> +	}
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +
> +	msleep(180);
> +
> +	ctx->prepared = true;
> +
> +	return 0;
> +
> +disable_vcc:
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +	regulator_disable(ctx->vcc);
> +	return ret;
> +}
> +
> +static int hx8394_get_modes(struct drm_panel *panel,
> +			    struct drm_connector *connector)
> +{
> +	struct hx8394 *ctx = panel_to_hx8394(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(connector->dev, ctx->desc->mode);
> +	if (!mode) {
> +		dev_err(ctx->dev, "Failed to add mode %ux%u@%u\n",
> +			ctx->desc->mode->hdisplay, ctx->desc->mode->vdisplay,
> +			drm_mode_vrefresh(ctx->desc->mode));
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	connector->display_info.width_mm = mode->width_mm;
> +	connector->display_info.height_mm = mode->height_mm;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs hx8394_drm_funcs = {
> +	.disable   = hx8394_disable,
> +	.unprepare = hx8394_unprepare,
> +	.prepare   = hx8394_prepare,
> +	.enable	   = hx8394_enable,
> +	.get_modes = hx8394_get_modes,
> +};
> +
> +static int hx8394_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct hx8394 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ctx->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
> +				     "Failed to get reset gpio\n");
> +
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +
> +	ctx->dev = dev;
> +	ctx->desc = of_device_get_match_data(dev);
> +
> +	dsi->mode_flags = ctx->desc->mode_flags;
> +	dsi->format = ctx->desc->format;
> +	dsi->lanes = ctx->desc->lanes;
> +
> +	ctx->vcc = devm_regulator_get(dev, "vcc");
> +	if (IS_ERR(ctx->vcc))
> +		return dev_err_probe(dev, PTR_ERR(ctx->vcc),
> +				     "Failed to request vcc regulator\n");
> +
> +	ctx->iovcc = devm_regulator_get(dev, "iovcc");
> +	if (IS_ERR(ctx->iovcc))
> +		return dev_err_probe(dev, PTR_ERR(ctx->iovcc),
> +				     "Failed to request iovcc regulator\n");
> +
> +	drm_panel_init(&ctx->panel, dev, &hx8394_drm_funcs,
> +		       DRM_MODE_CONNECTOR_DSI);
> +
> +	ret = drm_panel_of_backlight(&ctx->panel);
> +	if (ret)
> +		return ret;
> +
> +	drm_panel_add(&ctx->panel);
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "mipi_dsi_attach failed\n");
> +		drm_panel_remove(&ctx->panel);
> +		return ret;
> +	}
> +
> +	dev_dbg(dev, "%ux%u@%u %ubpp dsi %udl - ready\n",
> +		ctx->desc->mode->hdisplay, ctx->desc->mode->vdisplay,
> +		drm_mode_vrefresh(ctx->desc->mode),
> +		mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);
> +
> +	return 0;
> +}
> +
> +static void hx8394_shutdown(struct mipi_dsi_device *dsi)
> +{
> +	struct hx8394 *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	ret = drm_panel_disable(&ctx->panel);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "Failed to disable panel: %d\n", ret);
> +
> +	ret = drm_panel_unprepare(&ctx->panel);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "Failed to unprepare panel: %d\n", ret);
> +}
> +
> +static void hx8394_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct hx8394 *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	hx8394_shutdown(dsi);
> +
> +	ret = mipi_dsi_detach(dsi);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
> +
> +	drm_panel_remove(&ctx->panel);
> +}
> +
> +static const struct of_device_id hx8394_of_match[] = {
> +	{ .compatible = "hannstar,hsd060bhw4", .data = &hsd060bhw4_desc },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, hx8394_of_match);
> +
> +static struct mipi_dsi_driver hx8394_driver = {
> +	.probe	= hx8394_probe,
> +	.remove = hx8394_remove,
> +	.shutdown = hx8394_shutdown,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = hx8394_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(hx8394_driver);
> +
> +MODULE_AUTHOR("Kamil Trzciński <ayufan@ayufan.eu>");
> +MODULE_DESCRIPTION("DRM driver for Himax HX8394 based MIPI DSI panels");
> +MODULE_LICENSE("GPL");
> -- 
> 2.38.1
> 

Re: [PATCH v4 3/4] MAINTAINERS: Add entry for Himax HX8394 panel controller driver

[Ondřej Jirman]

Hello,

On Fri, Dec 30, 2022 at 12:31:53PM +0100, Javier Martinez Canillas wrote:
> Add myself as maintainer for the driver and devicetree bindings schema.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> ---
> 
> Changes in v4:
> - Add Sam Ravnborg's Acked-by tag.
> 
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7729a30b9609..c901e536303b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6551,6 +6551,13 @@ S:	Maintained
>  T:	git git://anongit.freedesktop.org/drm/drm-misc
>  F:	drivers/gpu/drm/tiny/gm12u320.c
>  
> +DRM DRIVER FOR HIMAX HX8394 MIPI-DSI LCD panels
> +M:	Javier Martinez Canillas <javierm@redhat.com>

+M:	Ondrej Jirman <megi@xff.cz>

thank you,
	o.

> +S:	Maintained
> +T:	git git://anongit.freedesktop.org/drm/drm-misc
> +F:	Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
> +F:	drivers/gpu/drm/panel/panel-himax-hx8394.c
> +
>  DRM DRIVER FOR HX8357D PANELS
>  M:	Emma Anholt <emma@anholt.net>
>  S:	Maintained
> -- 
> 2.38.1
> 

Re: [PATCH v4 2/4] drm: panel: Add Himax HX8394 panel controller driver

[Javier Martinez Canillas]

Hello Ondřej,

Thanks a lot for your comments.

On 12/30/22 16:40, Ondřej Jirman wrote:
> Hi Javier,
> 
> On Fri, Dec 30, 2022 at 12:31:52PM +0100, Javier Martinez Canillas wrote:
>> From: Kamil Trzciński <ayufan@ayufan.eu>
>>
>> The driver is for panels based on the Himax HX8394 controller, such as the
>> HannStar HSD060BHW4 720x1440 TFT LCD panel that uses a MIPI-DSI interface.
> 
> I see you've removed debug printks from enable/disable/prepare/unprepare

Yes, because as you said were debug printks. Feel free to propose adding the
debug printks if you consider useful for normal usage and not just for devel
purposes.

> hooks. Have you tested the driver thoroughly with various DRM apps,
> with DPM/suspend/resume, etc.?
>

I did not. I wasn't expecting suspend and resume to work on the PPP given its
support is quite minimal currently.
 
> The dw-mipi-dsi driver does some unorthodox things[1], that can lead to unbalanced
> calls to these functions in some situations, and that's why all these printks
> were there. To ensure the driver hooks are called correctly, while preparing
> the code for upstreaming. This lead to broken display in some situations during
> suspend/resume.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L868
>

This needs to be fixed in the dw-mipi-dsi driver then. But at least we will get
a panel-himax-hx8394 driver in mainline to avoid having to use downstream trees
for development and testing.

> Also, have you checked the clocks are actually configured correctly by the
> rk3399 cru driver? I have a lot of trouble with that, too. clk driver sometimes
> selects the fractional clock, but does not give it the necessary >20x difference
> between input/output clock rates. You'll only notice if you measure clock rates
> directly, by looking at actual refresh rate, by using some testing DRM app.
> Clock subsystem sometimes shuffles things around if you switch VOPs and use big
> VOP for mipi-dsi display, instead of the default small VOP.
>

I have not. Just verified that the display was working on my PPP and could start
a mutter wayland session. We could fix the clock configuration as follow-up IMO.

> I'll test this patchset in a few days against purely mainline code, but I'm
> pretty sure looking at the modes you use, that this will not work on some
> Pinephone Pro's, and will cause display corruption when you fix your clock
> setup, so that CRU actually outputs 74.25MHz as requested by the mode. (Which
> can be fixed by this patch https://github.com/megous/linux/commit/f7ee16f12ee8a44ee2472f2967ca27768106e00f)
> 

As mentioned, I prefer to make the support incremental. First having the panel
driver and then we can fix any remaining issue as follow-up patch series.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 3/4] MAINTAINERS: Add entry for Himax HX8394 panel controller driver

[Javier Martinez Canillas]

On 12/30/22 16:43, Ondřej Jirman wrote:

[...]

>>  
>> +DRM DRIVER FOR HIMAX HX8394 MIPI-DSI LCD panels
>> +M:	Javier Martinez Canillas <javierm@redhat.com>
> 
> +M:	Ondrej Jirman <megi@xff.cz>
>

Great! I assume that you also are OK with listing you in the DT
binding doc. I'll include you in both places when posting a v5.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Hello Ondřej,

Thanks a lot for your feedback.

On 12/30/22 16:37, Ondřej Jirman wrote:

[...]

>>  &i2c0 {
>>  	clock-frequency = <400000>;
>>  	i2c-scl-rising-time-ns = <168>;
>> @@ -214,6 +251,9 @@ vcc3v0_touch: LDO_REG2 {
>>  				regulator-name = "vcc3v0_touch";
>>  				regulator-min-microvolt = <3000000>;
>>  				regulator-max-microvolt = <3000000>;
>> +				regulator-state-mem {
>> +					regulator-off-in-suspend;
>> +				};
> 
> You're instructing RK818 to shut down the regulator for touch controller during
> suspend, but I think Goodix driver expects touch controller to be kept powered on
> during suspend. Am I missing something?
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/input/touchscreen/goodix.c#L1405
>

You tell me, it is your patch :) I just cherry-picked this from your tree:

https://github.com/megous/linux/commit/11f8da60d6a5

But if that is not correct, then I can drop the regulator-off-in-suspend.
 
[...]

>> +
>> +	touchscreen@14 {
>> +		compatible = "goodix,gt917s";
> 
> This is not the correct compatible. Pinephone Pro uses Goodix GT1158:
> 
> Goodix-TS 3-0014: ID 1158, version: 0100
> Goodix-TS 3-0014: Direct firmware load for goodix_1158_cfg.bin failed with error -2
> 
>

Same thing. I wasn't aware of this since your patch was using this compatible
string. If "goodix,gt1158" is the correct compatible string, then I agree we
should have that instead even when the firmware is missing. Because the DT is
supposed to describe the hardware. The FW issue can be tackled as a follow-up.

[...] 

>> +
>> +&vopb {
>> +	status = "okay";
>> +	assigned-clocks = <&cru DCLK_VOP0_DIV>, <&cru DCLK_VOP0>,
>> +			  <&cru ACLK_VOP0>, <&cru HCLK_VOP0>;
>> +	assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
>> +	assigned-clock-parents = <&cru PLL_CPLL>, <&cru DCLK_VOP0_FRAC>;
>> +};
> 
> So here you're putting a fractional clock into path between CPLL -> VOP0_DIV
> -> DCLK_VOP0_FRAC -> DCLK_VOP0.
> 
> Fractional clocks require 20x difference between input and output rates, and
> CPLL is 800Mhz IIRC, while you require 74.25MHz DCLK, so this will not work
> correctly.
> 
> Even if this somehow works by fractional clock being bypassed, I did not design
> the panel mode to be used with CPLL's 800 MHz, but with GPLL frequecy of 594 MHz.
> 
> GPLL 594/74.25 = 8  (integral divider without the need for fractional clock)
> CPLL 800/74.25 = ~10.77441077441077441077
> 
> If you really want to use fractional clock, you'd need to parent it to VPLL
> and set VPLL really high, like close to 2GHz.
>

Thanks for the explanation. Then I just need to squash on top of this, the
following patch. Is that correct?

https://github.com/megous/linux/commit/f19ce7bb7d72

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 0/4] Add PinePhone Pro display support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

Hi!

> This series add support for the display present in the PinePhone Pro.
> 
> Patch #1 adds a driver for panels using the Himax HX8394 panel controller,
> such as the HSD060BHW4 720x1440 TFT LCD panel present in the PinePhone Pro.
> 
> Patch #2 adds a devicetree binding schema for this driver and patch #3 adds
> an entry for the driver in the MAINTAINERS file.
> 
> Finally patch #4 adds the needed devicetree nodes in the PinePhone Pro DTS,
> to enable both the display and the touchscreen. This makes the upstream DTS
> much more usable and will allow for example to enable support for the phone
> in the Fedora distribution.

Thanks for the series. Please cc: phone-devel@vger.kernel.org with
future patches.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v4 0/4] Add PinePhone Pro display support

[Javier Martinez Canillas]

Hello Pavel,

On 1/1/23 22:21, Pavel Machek wrote:
> Hi!
> 
>> This series add support for the display present in the PinePhone Pro.
>>
>> Patch #1 adds a driver for panels using the Himax HX8394 panel controller,
>> such as the HSD060BHW4 720x1440 TFT LCD panel present in the PinePhone Pro.
>>
>> Patch #2 adds a devicetree binding schema for this driver and patch #3 adds
>> an entry for the driver in the MAINTAINERS file.
>>
>> Finally patch #4 adds the needed devicetree nodes in the PinePhone Pro DTS,
>> to enable both the display and the touchscreen. This makes the upstream DTS
>> much more usable and will allow for example to enable support for the phone
>> in the Fedora distribution.
> 
> Thanks for the series. Please cc: phone-devel@vger.kernel.org with
> future patches.
>

Sure, I will.
 
> Best regards,
> 								Pavel

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Ondřej Jirman]

Hello Javier,

On Sat, Dec 31, 2022 at 04:29:49PM +0100, Javier Martinez Canillas wrote:
> Hello Ondřej,
> 
> Thanks a lot for your feedback.
> 
> On 12/30/22 16:37, Ondřej Jirman wrote:
> 
> [...]
> 
> >>  &i2c0 {
> >>  	clock-frequency = <400000>;
> >>  	i2c-scl-rising-time-ns = <168>;
> >> @@ -214,6 +251,9 @@ vcc3v0_touch: LDO_REG2 {
> >>  				regulator-name = "vcc3v0_touch";
> >>  				regulator-min-microvolt = <3000000>;
> >>  				regulator-max-microvolt = <3000000>;
> >> +				regulator-state-mem {
> >> +					regulator-off-in-suspend;
> >> +				};
> > 
> > You're instructing RK818 to shut down the regulator for touch controller during
> > suspend, but I think Goodix driver expects touch controller to be kept powered on
> > during suspend. Am I missing something?
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/input/touchscreen/goodix.c#L1405
> >
> 
> You tell me, it is your patch :) I just cherry-picked this from your tree:

I have other patches to goodix driver that do power off the touch sensor chip
during sleep, so that it doesn't consume excessinve amounts of power when
the phone is suspended. Mainline doesn't. You have to adapt this to mainline,
because you're not upstreaming the required Goodix patches, for regulator-off-in-suspend
to not break things.

> https://github.com/megous/linux/commit/11f8da60d6a5
> 
> But if that is not correct, then I can drop the regulator-off-in-suspend.
>  
> [...]
> 
> >> +
> >> +	touchscreen@14 {
> >> +		compatible = "goodix,gt917s";
> > 
> > This is not the correct compatible. Pinephone Pro uses Goodix GT1158:
> > 
> > Goodix-TS 3-0014: ID 1158, version: 0100
> > Goodix-TS 3-0014: Direct firmware load for goodix_1158_cfg.bin failed with error -2
> > 
> >
> 
> Same thing. I wasn't aware of this since your patch was using this compatible
> string. If "goodix,gt1158" is the correct compatible string, then I agree we
> should have that instead even when the firmware is missing. Because the DT is
> supposed to describe the hardware. The FW issue can be tackled as a follow-up.
> 
> [...] 

Yes, compatible string is sort of irrelevant, because the driver does runtime
auto-detection based on chip ID. I didn't bother with superficial issues in the
original code from Martijn/Kamil. Now that you're mainlining the code, this
should be sorted out, though.

There's no FW issue, I was just using the log to show you the actual chip ID the
driver detects.

(You should probably put my SoB after Kamil/Martijn, since I took the
maintenance/development of the driver after they wrote the base support
initially in secret. I'm not the original author of the code.)

> >> +
> >> +&vopb {
> >> +	status = "okay";
> >> +	assigned-clocks = <&cru DCLK_VOP0_DIV>, <&cru DCLK_VOP0>,
> >> +			  <&cru ACLK_VOP0>, <&cru HCLK_VOP0>;
> >> +	assigned-clock-rates = <0>, <0>, <400000000>, <100000000>;
> >> +	assigned-clock-parents = <&cru PLL_CPLL>, <&cru DCLK_VOP0_FRAC>;
> >> +};
> > 
> > So here you're putting a fractional clock into path between CPLL -> VOP0_DIV
> > -> DCLK_VOP0_FRAC -> DCLK_VOP0.
> > 
> > Fractional clocks require 20x difference between input and output rates, and
> > CPLL is 800Mhz IIRC, while you require 74.25MHz DCLK, so this will not work
> > correctly.
> > 
> > Even if this somehow works by fractional clock being bypassed, I did not design
> > the panel mode to be used with CPLL's 800 MHz, but with GPLL frequecy of 594 MHz.
> > 
> > GPLL 594/74.25 = 8  (integral divider without the need for fractional clock)
> > CPLL 800/74.25 = ~10.77441077441077441077
> > 
> > If you really want to use fractional clock, you'd need to parent it to VPLL
> > and set VPLL really high, like close to 2GHz.
> >
> 
> Thanks for the explanation. Then I just need to squash on top of this, the
> following patch. Is that correct?
> 
> https://github.com/megous/linux/commit/f19ce7bb7d72

Yes, and test the driver more thoroughly:

- look at clk_summary to verify clock rate the kernel thinks it's using
- test refresh rate, somehow, to again verify the actual clock rate (kernel can
  lie in debugfs)
- test power cycling the panel (eg. via system suspend/resume or other means)

thank you and kind regards,
	o.

> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Re: [PATCH v4 2/4] drm: panel: Add Himax HX8394 panel controller driver

[Ondřej Jirman]

Hello Javier,

On Sat, Dec 31, 2022 at 04:15:24PM +0100, Javier Martinez Canillas wrote:
> Hello Ondřej,
> 
> Thanks a lot for your comments.
> 
> On 12/30/22 16:40, Ondřej Jirman wrote:
> > Hi Javier,
> > 
> > On Fri, Dec 30, 2022 at 12:31:52PM +0100, Javier Martinez Canillas wrote:
> >> From: Kamil Trzciński <ayufan@ayufan.eu>
> >>
> >> The driver is for panels based on the Himax HX8394 controller, such as the
> >> HannStar HSD060BHW4 720x1440 TFT LCD panel that uses a MIPI-DSI interface.
> > 
> > I see you've removed debug printks from enable/disable/prepare/unprepare
> 
> Yes, because as you said were debug printks. Feel free to propose adding the
> debug printks if you consider useful for normal usage and not just for devel
> purposes.

I already did, and used them do debug and fix the issues. This submission just
doesn't include the fixes.

> > hooks. Have you tested the driver thoroughly with various DRM apps,
> > with DPM/suspend/resume, etc.?
> >
> 
> I did not. I wasn't expecting suspend and resume to work on the PPP given its
> support is quite minimal currently.

System suspend/resume works and is used by distributions. Display blanking is
also used by normal distros, even if you don't use system suspend/resume.

> > The dw-mipi-dsi driver does some unorthodox things[1], that can lead to unbalanced
> > calls to these functions in some situations, and that's why all these printks
> > were there. To ensure the driver hooks are called correctly, while preparing
> > the code for upstreaming. This lead to broken display in some situations during
> > suspend/resume.
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L868
> >
> 
> This needs to be fixed in the dw-mipi-dsi driver then. But at least we will get
> a panel-himax-hx8394 driver in mainline to avoid having to use downstream trees
> for development and testing.

The only thing this driver is supposed to do is to power up (+configure) and power
down the display, the rest is boilerplate. Powercycling via suspend/resume
and/or other means (like disabling the crtc via DRM API), has to be tested,
to verify the driver can at least do a power down/up cycle and not just initial
powerup.

> > Also, have you checked the clocks are actually configured correctly by the
> > rk3399 cru driver? I have a lot of trouble with that, too. clk driver sometimes
> > selects the fractional clock, but does not give it the necessary >20x difference
> > between input/output clock rates. You'll only notice if you measure clock rates
> > directly, by looking at actual refresh rate, by using some testing DRM app.
> > Clock subsystem sometimes shuffles things around if you switch VOPs and use big
> > VOP for mipi-dsi display, instead of the default small VOP.
> >
> 
> I have not. Just verified that the display was working on my PPP and could start
> a mutter wayland session. We could fix the clock configuration as follow-up IMO.

The display output will be broken after you fix the assigned-clocks in DT to
expected values (use GPLL parent, to make the HW generate the exact pixel clock
defined in the display mode). So this needs to be dealt with now, not later.


The driver issues are all known at this time and have fixes available, unlike
a year ago:

- panel mode not working with actual clock rate it requests (severe image
  corruption on some pinephone pro's)
- no display output after suspend/resume cycle or a blanking/unblanking cycle

So if you're submitting a known broken code, at least mention the issues in code
comments, so that people that will inevitably hit the bugs will not spend large
amount of time hunting for the cause again, when the issue and causes are known
already.

Just figuring out the image corruption took more than a year since it was
discovered. Better not inflict that on others.

regards,
	o.

> > I'll test this patchset in a few days against purely mainline code, but I'm
> > pretty sure looking at the modes you use, that this will not work on some
> > Pinephone Pro's, and will cause display corruption when you fix your clock
> > setup, so that CRU actually outputs 74.25MHz as requested by the mode. (Which
> > can be fixed by this patch https://github.com/megous/linux/commit/f7ee16f12ee8a44ee2472f2967ca27768106e00f)
> > 
> 
> As mentioned, I prefer to make the support incremental. First having the panel
> driver and then we can fix any remaining issue as follow-up patch series.
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Re: [PATCH v4 4/4] arm64: dts: rk3399-pinephone-pro: Add internal display support

[Javier Martinez Canillas]

Hello Ondřej,

On 1/2/23 11:57, Ondřej Jirman wrote:

[...]

>>
>> You tell me, it is your patch :) I just cherry-picked this from your tree:
> 
> I have other patches to goodix driver that do power off the touch sensor chip
> during sleep, so that it doesn't consume excessinve amounts of power when
> the phone is suspended. Mainline doesn't. You have to adapt this to mainline,
> because you're not upstreaming the required Goodix patches, for regulator-off-in-suspend
> to not break things.
> 
>> https://github.com/megous/linux/commit/11f8da60d6a5
>>
>> But if that is not correct, then I can drop the regulator-off-in-suspend.
>>

Ah, I see. Missed that. Then I guess that's better to drop the regulator-off-in-suspend
until the goodix driver patches are upstreamed.

>> [...]
>>
>>>> +
>>>> +	touchscreen@14 {
>>>> +		compatible = "goodix,gt917s";
>>>
>>> This is not the correct compatible. Pinephone Pro uses Goodix GT1158:
>>>
>>> Goodix-TS 3-0014: ID 1158, version: 0100
>>> Goodix-TS 3-0014: Direct firmware load for goodix_1158_cfg.bin failed with error -2
>>>
>>>
>>
>> Same thing. I wasn't aware of this since your patch was using this compatible
>> string. If "goodix,gt1158" is the correct compatible string, then I agree we
>> should have that instead even when the firmware is missing. Because the DT is
>> supposed to describe the hardware. The FW issue can be tackled as a follow-up.
>>
>> [...] 
> 
> Yes, compatible string is sort of irrelevant, because the driver does runtime
> auto-detection based on chip ID. I didn't bother with superficial issues in the
> original code from Martijn/Kamil. Now that you're mainlining the code, this
> should be sorted out, though.
> 
> There's no FW issue, I was just using the log to show you the actual chip ID the
> driver detects.
>

Gotcha.
 
> (You should probably put my SoB after Kamil/Martijn, since I took the
> maintenance/development of the driver after they wrote the base support
> initially in secret. I'm not the original author of the code.)
>

I wasn't aware of that. I just kept the author field as it's in your tree.
 
[...]

>> https://github.com/megous/linux/commit/f19ce7bb7d72
> 
> Yes, and test the driver more thoroughly:
> 
> - look at clk_summary to verify clock rate the kernel thinks it's using
> - test refresh rate, somehow, to again verify the actual clock rate (kernel can
>   lie in debugfs)
> - test power cycling the panel (eg. via system suspend/resume or other means)
> 

Agreed that the more testing the better.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 2/4] drm: panel: Add Himax HX8394 panel controller driver

[Javier Martinez Canillas]

Hello Ondřej,

On 1/2/23 11:59, Ondřej Jirman wrote:

[...]

>> Yes, because as you said were debug printks. Feel free to propose adding the
>> debug printks if you consider useful for normal usage and not just for devel
>> purposes.
> 
> I already did, and used them do debug and fix the issues. This submission just
> doesn't include the fixes.
>

I missed the fixes, I think that cherry-picked and squashed from your tree
before you added commit f19ce7bb7d72 ("arm64: dts: rk3399-pinephone-pro:
Use unused GPLL for VOPs DCLK") at least.
 
>>> hooks. Have you tested the driver thoroughly with various DRM apps,
>>> with DPM/suspend/resume, etc.?
>>>
>>
>> I did not. I wasn't expecting suspend and resume to work on the PPP given its
>> support is quite minimal currently.
> 
> System suspend/resume works and is used by distributions. Display blanking is
> also used by normal distros, even if you don't use system suspend/resume.
>

I know but my point was that the PPP mainline support isn't ready to be used
as a daily driver in practice. So I didn't consider susped/resume or display
blank as a requirement to upstream an initial support for the panel driver.

[...]

>>> Also, have you checked the clocks are actually configured correctly by the
>>> rk3399 cru driver? I have a lot of trouble with that, too. clk driver sometimes
>>> selects the fractional clock, but does not give it the necessary >20x difference
>>> between input/output clock rates. You'll only notice if you measure clock rates
>>> directly, by looking at actual refresh rate, by using some testing DRM app.
>>> Clock subsystem sometimes shuffles things around if you switch VOPs and use big
>>> VOP for mipi-dsi display, instead of the default small VOP.
>>>
>>
>> I have not. Just verified that the display was working on my PPP and could start
>> a mutter wayland session. We could fix the clock configuration as follow-up IMO.
> 
> The display output will be broken after you fix the assigned-clocks in DT to
> expected values (use GPLL parent, to make the HW generate the exact pixel clock
> defined in the display mode). So this needs to be dealt with now, not later.
> 
> 
> The driver issues are all known at this time and have fixes available, unlike
> a year ago:
> 

My goal was to have some initial support in mainline even if there could be some
issues. IMO it is better to use upstream as a baseline and attempt to support the
PPP incrementally.

But since you are aware of the issues and know what are the available fixes, I'll
let you continue with the effort and take care of the patches. Hopefully there may
be things that will be helpful, such as the binding schema patch and the collected
tags. I can also take care of pushing the DRM bits to the drm-misc-next tree once
you feel that those are ready to get merged.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v4 2/4] drm: panel: Add Himax HX8394 panel controller driver

[Ondřej Jirman]

On Mon, Jan 02, 2023 at 02:51:42PM +0100, Javier Martinez Canillas wrote:
> Hello Ondřej,
> 
> [...]
>
> My goal was to have some initial support in mainline even if there could be some
> issues. IMO it is better to use upstream as a baseline and attempt to support the
> PPP incrementally.
> 
> But since you are aware of the issues and know what are the available fixes, I'll
> let you continue with the effort and take care of the patches. Hopefully there may
> be things that will be helpful, such as the binding schema patch and the collected
> tags. I can also take care of pushing the DRM bits to the drm-misc-next tree once
> you feel that those are ready to get merged.

Ok. The panel driver itself works fine with some changes in other DRM drivers.
In fact, it will not need any changes, assuming the to be proposed fixes to
dw-mipi-dsi will pass, too. So I don't have many objections against this driver
itself.

I'm not sure I should be giving reviewed-by to driver I co-wrote. :) Anyway,
I checked it again, and only issue I found was that shutdown callback tries
to disable the panel even if it may already be disabled, which will lead to
unbalanced calls to regulator_disable functions, which may produce some needless
warnings on shutdown/reboot.

So if you want to commit this driver now, go ahead. DT will need one more round.

As you say, the overall usable support for Pinephone Pro in mainline is still
way off into the future, so I agree it's not necessary to get hung up on these
issues. I can do a DT revision + add in the other suggested DRM patches, so that
there's at least a searchable public record of the remaining issues.

kind regards,
	o.

> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Re: [PATCH v4 2/4] drm: panel: Add Himax HX8394 panel controller driver

[Javier Martinez Canillas]

On 1/2/23 16:20, Ondřej Jirman wrote:
> On Mon, Jan 02, 2023 at 02:51:42PM +0100, Javier Martinez Canillas wrote:
>> Hello Ondřej,
>>
>> [...]
>>
>> My goal was to have some initial support in mainline even if there could be some
>> issues. IMO it is better to use upstream as a baseline and attempt to support the
>> PPP incrementally.
>>
>> But since you are aware of the issues and know what are the available fixes, I'll
>> let you continue with the effort and take care of the patches. Hopefully there may
>> be things that will be helpful, such as the binding schema patch and the collected
>> tags. I can also take care of pushing the DRM bits to the drm-misc-next tree once
>> you feel that those are ready to get merged.
> 
> Ok. The panel driver itself works fine with some changes in other DRM drivers.
> In fact, it will not need any changes, assuming the to be proposed fixes to
> dw-mipi-dsi will pass, too. So I don't have many objections against this driver
> itself.
>

Exactly, that is what I was trying to say. Awesome that you agree with that.

> I'm not sure I should be giving reviewed-by to driver I co-wrote. :) Anyway,

Indeed :)

> I checked it again, and only issue I found was that shutdown callback tries
> to disable the panel even if it may already be disabled, which will lead to
> unbalanced calls to regulator_disable functions, which may produce some needless
> warnings on shutdown/reboot.
> 
> So if you want to commit this driver now, go ahead. DT will need one more round.
> 
> As you say, the overall usable support for Pinephone Pro in mainline is still
> way off into the future, so I agree it's not necessary to get hung up on these
> issues. I can do a DT revision + add in the other suggested DRM patches, so that
> there's at least a searchable public record of the remaining issues.
> 

Perfect, sounds like a plan. I'll re-spin a v5 that only includes the panel
patches then and drop the DTS. Thanks again for your feedback and comments!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat