* [PATCH v1 0/2] Add kd101ne3 bindings and driver.
@ 2024-04-18 8:15 lvzhaoxiong
2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong
2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong
0 siblings, 2 replies; 19+ messages in thread
From: lvzhaoxiong @ 2024-04-18 8:15 UTC (permalink / raw)
To: mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt,
dianders, hsinyi
Cc: dri-devel, devicetree, linux-kernel, lvzhaoxiong
Add bindings and driver for kd101ne3.
lvzhaoxiong (2):
dt-bindings: display: panel: Add KD101NE3-40TI support
drm/panel: kd101ne3: add new panel driver
.../panel/kingdisplay,kd101ne3-40ti.yaml | 63 ++
drivers/gpu/drm/panel/Kconfig | 9 +
drivers/gpu/drm/panel/Makefile | 1 +
.../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++
4 files changed, 680 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml
create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support
2024-04-18 8:15 [PATCH v1 0/2] Add kd101ne3 bindings and driver lvzhaoxiong
@ 2024-04-18 8:15 ` lvzhaoxiong
2024-04-18 22:59 ` Krzysztof Kozlowski
2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong
1 sibling, 1 reply; 19+ messages in thread
From: lvzhaoxiong @ 2024-04-18 8:15 UTC (permalink / raw)
To: mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt,
dianders, hsinyi
Cc: dri-devel, devicetree, linux-kernel, lvzhaoxiong
Create a new dt-scheam for the kd101ne3-40ti.
Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com>
---
.../panel/kingdisplay,kd101ne3-40ti.yaml | 63 +++++++++++++++++++
1 file changed, 63 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml
diff --git a/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml b/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml
new file mode 100644
index 000000000000..dc79a49eea3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/kingdisplay,kd101ne3-40ti.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: King Display KD035G6-40TI based MIPI-DSI panels
+
+description: |
+ -This binding is for display panels using an JD9365DA controller
+
+maintainers:
+ - Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>
+
+allOf:
+ - $ref: panel-common.yaml#
+
+properties:
+ compatible:
+ const: kingdisplay,kd101ne3-40ti
+
+ backlight: true
+ port: true
+ pp3300-supply: true
+ reg: true
+ enable-gpios: true
+ rotation: true
+
+required:
+ - compatible
+ - reg
+ - enable-gpios
+ - pp3300-supply
+ - backlight
+ - port
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ dsi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ panel: panel@0 {
+ compatible = "kingdisplay,kd101ne3-40ti";
+ reg = <0>;
+ enable-gpios = <&pio 98 0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&panel_pins_default>;
+ pp3300-supply = <&en_pp6000_mipi_disp>;
+ backlight = <&backlight_lcd0>;
+ rotation = <90>;
+ port {
+ panel_in: endpoint {
+ remote-endpoint = <&dsi_out>;
+ };
+ };
+ };
+ };
+
+...
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-18 8:15 [PATCH v1 0/2] Add kd101ne3 bindings and driver lvzhaoxiong
2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong
@ 2024-04-18 8:15 ` lvzhaoxiong
2024-04-18 11:46 ` Dmitry Baryshkov
1 sibling, 1 reply; 19+ messages in thread
From: lvzhaoxiong @ 2024-04-18 8:15 UTC (permalink / raw)
To: mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt,
dianders, hsinyi
Cc: dri-devel, devicetree, linux-kernel, lvzhaoxiong
The kingdisplay panel is based on JD9365DA controller.
Add a driver for it.
Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com>
---
drivers/gpu/drm/panel/Kconfig | 9 +
drivers/gpu/drm/panel/Makefile | 1 +
.../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++
3 files changed, 617 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 154f5bf82980..2c73086cf102 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04
24 bit RGB per pixel. It provides a MIPI DSI interface to
the host and has a built-in LED backlight.
+config DRM_PANEL_KINGDISPLAY_KD101NE3
+ tristate "Kingdisplay kd101ne3 panel"
+ 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
+ Kingdisplay kd101ne3 controller.
+
config DRM_PANEL_LEADTEK_LTK050H3146W
tristate "Leadtek LTK050H3146W panel"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 24a02655d726..cbd414b98bb0 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o
obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o
obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o
obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
+obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o
obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
new file mode 100644
index 000000000000..dbf0992f8b81
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
@@ -0,0 +1,607 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Panels based on the JD9365DA display controller.
+ * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#include <video/mipi_display.h>
+
+struct panel_desc {
+ const struct drm_display_mode *modes;
+ unsigned int bpc;
+
+ /**
+ * @width_mm: width of the panel's active display area
+ * @height_mm: height of the panel's active display area
+ */
+ struct {
+ unsigned int width_mm;
+ unsigned int height_mm;
+ } size;
+
+ unsigned long mode_flags;
+ enum mipi_dsi_pixel_format format;
+ const struct panel_init_cmd *init_cmds;
+ unsigned int lanes;
+ bool discharge_on_disable;
+ bool lp11_before_reset;
+};
+
+struct kingdisplay_panel {
+ struct drm_panel base;
+ struct mipi_dsi_device *dsi;
+
+ const struct panel_desc *desc;
+
+ enum drm_panel_orientation orientation;
+ struct regulator *pp3300;
+ struct gpio_desc *enable_gpio;
+};
+
+enum dsi_cmd_type {
+ INIT_DCS_CMD,
+ DELAY_CMD,
+};
+
+struct panel_init_cmd {
+ enum dsi_cmd_type type;
+ size_t len;
+ const char *data;
+};
+
+#define _INIT_DCS_CMD(...) { \
+ .type = INIT_DCS_CMD, \
+ .len = sizeof((char[]){__VA_ARGS__}), \
+ .data = (char[]){__VA_ARGS__} }
+
+#define _INIT_DELAY_CMD(...) { \
+ .type = DELAY_CMD,\
+ .len = sizeof((char[]){__VA_ARGS__}), \
+ .data = (char[]){__VA_ARGS__} }
+
+static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
+ _INIT_DELAY_CMD(50),
+ _INIT_DCS_CMD(0xE0, 0x00),
+ _INIT_DCS_CMD(0xE1, 0x93),
+ _INIT_DCS_CMD(0xE2, 0x65),
+ _INIT_DCS_CMD(0xE3, 0xF8),
+ _INIT_DCS_CMD(0x80, 0x03),
+ _INIT_DCS_CMD(0xE0, 0x01),
+ _INIT_DCS_CMD(0x0C, 0x74),
+ _INIT_DCS_CMD(0x17, 0x00),
+ _INIT_DCS_CMD(0x18, 0xC7),
+ _INIT_DCS_CMD(0x19, 0x01),
+ _INIT_DCS_CMD(0x1A, 0x00),
+ _INIT_DCS_CMD(0x1B, 0xC7),
+ _INIT_DCS_CMD(0x1C, 0x01),
+ _INIT_DCS_CMD(0x24, 0xFE),
+ _INIT_DCS_CMD(0x37, 0x19),
+ _INIT_DCS_CMD(0x35, 0x28),
+ _INIT_DCS_CMD(0x38, 0x05),
+ _INIT_DCS_CMD(0x39, 0x08),
+ _INIT_DCS_CMD(0x3A, 0x12),
+ _INIT_DCS_CMD(0x3C, 0x7E),
+ _INIT_DCS_CMD(0x3D, 0xFF),
+ _INIT_DCS_CMD(0x3E, 0xFF),
+ _INIT_DCS_CMD(0x3F, 0x7F),
+ _INIT_DCS_CMD(0x40, 0x06),
+ _INIT_DCS_CMD(0x41, 0xA0),
+ _INIT_DCS_CMD(0x43, 0x1E),
+ _INIT_DCS_CMD(0x44, 0x0B),
+ _INIT_DCS_CMD(0x55, 0x02),
+ _INIT_DCS_CMD(0x57, 0x6A),
+ _INIT_DCS_CMD(0x59, 0x0A),
+ _INIT_DCS_CMD(0x5A, 0x2E),
+ _INIT_DCS_CMD(0x5B, 0x1A),
+ _INIT_DCS_CMD(0x5C, 0x15),
+ _INIT_DCS_CMD(0x5D, 0x7F),
+ _INIT_DCS_CMD(0x5E, 0x61),
+ _INIT_DCS_CMD(0x5F, 0x50),
+ _INIT_DCS_CMD(0x60, 0x43),
+ _INIT_DCS_CMD(0x61, 0x3F),
+ _INIT_DCS_CMD(0x62, 0x32),
+ _INIT_DCS_CMD(0x63, 0x35),
+ _INIT_DCS_CMD(0x64, 0x1F),
+ _INIT_DCS_CMD(0x65, 0x38),
+ _INIT_DCS_CMD(0x66, 0x36),
+ _INIT_DCS_CMD(0x67, 0x36),
+ _INIT_DCS_CMD(0x68, 0x54),
+ _INIT_DCS_CMD(0x69, 0x42),
+ _INIT_DCS_CMD(0x6A, 0x48),
+ _INIT_DCS_CMD(0x6B, 0x39),
+ _INIT_DCS_CMD(0x6C, 0x34),
+ _INIT_DCS_CMD(0x6D, 0x26),
+ _INIT_DCS_CMD(0x6E, 0x14),
+ _INIT_DCS_CMD(0x6F, 0x02),
+ _INIT_DCS_CMD(0x70, 0x7F),
+ _INIT_DCS_CMD(0x71, 0x61),
+ _INIT_DCS_CMD(0x72, 0x50),
+ _INIT_DCS_CMD(0x73, 0x43),
+ _INIT_DCS_CMD(0x74, 0x3F),
+ _INIT_DCS_CMD(0x75, 0x32),
+ _INIT_DCS_CMD(0x76, 0x35),
+ _INIT_DCS_CMD(0x77, 0x1F),
+ _INIT_DCS_CMD(0x78, 0x38),
+ _INIT_DCS_CMD(0x79, 0x36),
+ _INIT_DCS_CMD(0x7A, 0x36),
+ _INIT_DCS_CMD(0x7B, 0x54),
+ _INIT_DCS_CMD(0x7C, 0x42),
+ _INIT_DCS_CMD(0x7D, 0x48),
+ _INIT_DCS_CMD(0x7E, 0x39),
+ _INIT_DCS_CMD(0x7F, 0x34),
+ _INIT_DCS_CMD(0x80, 0x26),
+ _INIT_DCS_CMD(0x81, 0x14),
+ _INIT_DCS_CMD(0x82, 0x02),
+ _INIT_DCS_CMD(0xE0, 0x02),
+ _INIT_DCS_CMD(0x00, 0x52),
+ _INIT_DCS_CMD(0x01, 0x5F),
+ _INIT_DCS_CMD(0x02, 0x5F),
+ _INIT_DCS_CMD(0x03, 0x50),
+ _INIT_DCS_CMD(0x04, 0x77),
+ _INIT_DCS_CMD(0x05, 0x57),
+ _INIT_DCS_CMD(0x06, 0x5F),
+ _INIT_DCS_CMD(0x07, 0x4E),
+ _INIT_DCS_CMD(0x08, 0x4C),
+ _INIT_DCS_CMD(0x09, 0x5F),
+ _INIT_DCS_CMD(0x0A, 0x4A),
+ _INIT_DCS_CMD(0x0B, 0x48),
+ _INIT_DCS_CMD(0x0C, 0x5F),
+ _INIT_DCS_CMD(0x0D, 0x46),
+ _INIT_DCS_CMD(0x0E, 0x44),
+ _INIT_DCS_CMD(0x0F, 0x40),
+ _INIT_DCS_CMD(0x10, 0x5F),
+ _INIT_DCS_CMD(0x11, 0x5F),
+ _INIT_DCS_CMD(0x12, 0x5F),
+ _INIT_DCS_CMD(0x13, 0x5F),
+ _INIT_DCS_CMD(0x14, 0x5F),
+ _INIT_DCS_CMD(0x15, 0x5F),
+ _INIT_DCS_CMD(0x16, 0x53),
+ _INIT_DCS_CMD(0x17, 0x5F),
+ _INIT_DCS_CMD(0x18, 0x5F),
+ _INIT_DCS_CMD(0x19, 0x51),
+ _INIT_DCS_CMD(0x1A, 0x77),
+ _INIT_DCS_CMD(0x1B, 0x57),
+ _INIT_DCS_CMD(0x1C, 0x5F),
+ _INIT_DCS_CMD(0x1D, 0x4F),
+ _INIT_DCS_CMD(0x1E, 0x4D),
+ _INIT_DCS_CMD(0x1F, 0x5F),
+ _INIT_DCS_CMD(0x20, 0x4B),
+ _INIT_DCS_CMD(0x21, 0x49),
+ _INIT_DCS_CMD(0x22, 0x5F),
+ _INIT_DCS_CMD(0x23, 0x47),
+ _INIT_DCS_CMD(0x24, 0x45),
+ _INIT_DCS_CMD(0x25, 0x41),
+ _INIT_DCS_CMD(0x26, 0x5F),
+ _INIT_DCS_CMD(0x27, 0x5F),
+ _INIT_DCS_CMD(0x28, 0x5F),
+ _INIT_DCS_CMD(0x29, 0x5F),
+ _INIT_DCS_CMD(0x2A, 0x5F),
+ _INIT_DCS_CMD(0x2B, 0x5F),
+ _INIT_DCS_CMD(0x2C, 0x13),
+ _INIT_DCS_CMD(0x2D, 0x1F),
+ _INIT_DCS_CMD(0x2E, 0x1F),
+ _INIT_DCS_CMD(0x2F, 0x01),
+ _INIT_DCS_CMD(0x30, 0x17),
+ _INIT_DCS_CMD(0x31, 0x17),
+ _INIT_DCS_CMD(0x32, 0x1F),
+ _INIT_DCS_CMD(0x33, 0x0D),
+ _INIT_DCS_CMD(0x34, 0x0F),
+ _INIT_DCS_CMD(0x35, 0x1F),
+ _INIT_DCS_CMD(0x36, 0x05),
+ _INIT_DCS_CMD(0x37, 0x07),
+ _INIT_DCS_CMD(0x38, 0x1F),
+ _INIT_DCS_CMD(0x39, 0x09),
+ _INIT_DCS_CMD(0x3A, 0x0B),
+ _INIT_DCS_CMD(0x3B, 0x11),
+ _INIT_DCS_CMD(0x3C, 0x1F),
+ _INIT_DCS_CMD(0x3D, 0x1F),
+ _INIT_DCS_CMD(0x3E, 0x1F),
+ _INIT_DCS_CMD(0x3F, 0x1F),
+ _INIT_DCS_CMD(0x40, 0x1F),
+ _INIT_DCS_CMD(0x41, 0x1F),
+ _INIT_DCS_CMD(0x42, 0x12),
+ _INIT_DCS_CMD(0x43, 0x1F),
+ _INIT_DCS_CMD(0x44, 0x1F),
+ _INIT_DCS_CMD(0x45, 0x00),
+ _INIT_DCS_CMD(0x46, 0x17),
+ _INIT_DCS_CMD(0x47, 0x17),
+ _INIT_DCS_CMD(0x48, 0x1F),
+ _INIT_DCS_CMD(0x49, 0x0C),
+ _INIT_DCS_CMD(0x4A, 0x0E),
+ _INIT_DCS_CMD(0x4B, 0x1F),
+ _INIT_DCS_CMD(0x4C, 0x04),
+ _INIT_DCS_CMD(0x4D, 0x06),
+ _INIT_DCS_CMD(0x4E, 0x1F),
+ _INIT_DCS_CMD(0x4F, 0x08),
+ _INIT_DCS_CMD(0x50, 0x0A),
+ _INIT_DCS_CMD(0x51, 0x10),
+ _INIT_DCS_CMD(0x52, 0x1F),
+ _INIT_DCS_CMD(0x53, 0x1F),
+ _INIT_DCS_CMD(0x54, 0x1F),
+ _INIT_DCS_CMD(0x55, 0x1F),
+ _INIT_DCS_CMD(0x56, 0x1F),
+ _INIT_DCS_CMD(0x57, 0x1F),
+ _INIT_DCS_CMD(0x58, 0x40),
+ _INIT_DCS_CMD(0x5B, 0x10),
+ _INIT_DCS_CMD(0x5C, 0x06),
+ _INIT_DCS_CMD(0x5D, 0x40),
+ _INIT_DCS_CMD(0x5E, 0x00),
+ _INIT_DCS_CMD(0x5F, 0x00),
+ _INIT_DCS_CMD(0x60, 0x40),
+ _INIT_DCS_CMD(0x61, 0x03),
+ _INIT_DCS_CMD(0x62, 0x04),
+ _INIT_DCS_CMD(0x63, 0x6C),
+ _INIT_DCS_CMD(0x64, 0x6C),
+ _INIT_DCS_CMD(0x65, 0x75),
+ _INIT_DCS_CMD(0x66, 0x08),
+ _INIT_DCS_CMD(0x67, 0xB4),
+ _INIT_DCS_CMD(0x68, 0x08),
+ _INIT_DCS_CMD(0x69, 0x6C),
+ _INIT_DCS_CMD(0x6A, 0x6C),
+ _INIT_DCS_CMD(0x6B, 0x0C),
+ _INIT_DCS_CMD(0x6D, 0x00),
+ _INIT_DCS_CMD(0x6E, 0x00),
+ _INIT_DCS_CMD(0x6F, 0x88),
+ _INIT_DCS_CMD(0x75, 0xBB),
+ _INIT_DCS_CMD(0x76, 0x00),
+ _INIT_DCS_CMD(0x77, 0x05),
+ _INIT_DCS_CMD(0x78, 0x2A),
+ _INIT_DCS_CMD(0xE0, 0x04),
+ _INIT_DCS_CMD(0x00, 0x0E),
+ _INIT_DCS_CMD(0x02, 0xB3),
+ _INIT_DCS_CMD(0x09, 0x61),
+ _INIT_DCS_CMD(0x0E, 0x48),
+
+ _INIT_DCS_CMD(0xE0, 0x00),
+ _INIT_DCS_CMD(0X11),
+ /* T6: 120ms */
+ _INIT_DELAY_CMD(120),
+ _INIT_DCS_CMD(0X29),
+ _INIT_DELAY_CMD(20),
+ {},
+};
+
+static inline struct kingdisplay_panel *to_kingdisplay_panel(struct drm_panel *panel)
+{
+ return container_of(panel, struct kingdisplay_panel, base);
+}
+
+static int kingdisplay_panel_init_dcs_cmd(struct kingdisplay_panel *kingdisplay)
+{
+ struct mipi_dsi_device *dsi = kingdisplay->dsi;
+ struct drm_panel *panel = &kingdisplay->base;
+ int i, err = 0;
+
+ if (kingdisplay->desc->init_cmds) {
+ const struct panel_init_cmd *init_cmds = kingdisplay->desc->init_cmds;
+
+ for (i = 0; init_cmds[i].len != 0; i++) {
+ const struct panel_init_cmd *cmd = &init_cmds[i];
+
+ switch (cmd->type) {
+ case DELAY_CMD:
+ msleep(cmd->data[0]);
+ err = 0;
+ break;
+
+ case INIT_DCS_CMD:
+ err = mipi_dsi_dcs_write(dsi, cmd->data[0],
+ cmd->len <= 1 ? NULL :
+ &cmd->data[1],
+ cmd->len - 1);
+ break;
+
+ default:
+ err = -EINVAL;
+ }
+
+ if (err < 0) {
+ dev_err(panel->dev,
+ "failed to write command %u\n", i);
+ return err;
+ }
+ }
+ }
+ return 0;
+}
+
+static int kingdisplay_panel_enter_sleep_mode(struct kingdisplay_panel *kingdisplay)
+{
+ struct mipi_dsi_device *dsi = kingdisplay->dsi;
+ int ret;
+
+ dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+ usleep_range(1000, 2000);
+ ret = mipi_dsi_dcs_set_display_off(dsi);
+ if (ret < 0)
+ return ret;
+
+ msleep(50);
+
+ ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int kingdisplay_panel_disable(struct drm_panel *panel)
+{
+ struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
+ int ret;
+
+ ret = kingdisplay_panel_enter_sleep_mode(kingdisplay);
+ if (ret < 0) {
+ dev_err(panel->dev, "failed to set panel off: %d\n", ret);
+ return ret;
+ }
+
+ msleep(100);
+
+ return 0;
+}
+
+static int kingdisplay_panel_unprepare(struct drm_panel *panel)
+{
+ struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
+ int err;
+
+ gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0);
+
+ /* T15: 2ms */
+ usleep_range(1000, 2000);
+
+ err = regulator_disable(kingdisplay->pp3300);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
+static int kingdisplay_panel_prepare(struct drm_panel *panel)
+{
+ struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
+ int ret;
+
+ gpiod_set_value(kingdisplay->enable_gpio, 0);
+
+ ret = regulator_enable(kingdisplay->pp3300);
+ if (ret < 0)
+ return ret;
+
+ /* T1: 5ms */
+ usleep_range(5000, 6000);
+
+ if (kingdisplay->desc->lp11_before_reset) {
+ mipi_dsi_dcs_nop(kingdisplay->dsi);
+ usleep_range(1000, 2000);
+ }
+
+ /* T2: 10ms, T1 + T2 > 5ms*/
+ usleep_range(10000, 11000);
+
+ gpiod_set_value_cansleep(kingdisplay->enable_gpio, 1);
+
+ ret = kingdisplay_panel_init_dcs_cmd(kingdisplay);
+ if (ret < 0) {
+ dev_err(panel->dev, "failed to init panel: %d\n", ret);
+ goto poweroff;
+ }
+
+ return 0;
+
+poweroff:
+ regulator_disable(kingdisplay->pp3300);
+ /* T6: 2ms */
+ usleep_range(1000, 2000);
+ gpiod_set_value(kingdisplay->enable_gpio, 0);
+
+ return ret;
+}
+
+static int kingdisplay_panel_enable(struct drm_panel *panel)
+{
+ msleep(130);
+ return 0;
+}
+
+static const struct drm_display_mode kingdisplay_kd101ne3_40ti_default_mode = {
+ .clock = 70595,
+ .hdisplay = 800,
+ .hsync_start = 800 + 30,
+ .hsync_end = 800 + 30 + 30,
+ .htotal = 800 + 30 + 30 + 30,
+ .vdisplay = 1280,
+ .vsync_start = 1280 + 30,
+ .vsync_end = 1280 + 30 + 4,
+ .vtotal = 1280 + 30 + 4 + 8,
+ .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static const struct panel_desc kingdisplay_kd101ne3_40ti_desc = {
+ .modes = &kingdisplay_kd101ne3_40ti_default_mode,
+ .bpc = 8,
+ .size = {
+ .width_mm = 135,
+ .height_mm = 216,
+ },
+ .lanes = 4,
+ .format = MIPI_DSI_FMT_RGB888,
+ .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+ MIPI_DSI_MODE_LPM,
+ .init_cmds = kingdisplay_kd101ne3_init_cmd,
+ .lp11_before_reset = true,
+};
+
+static int kingdisplay_panel_get_modes(struct drm_panel *panel,
+ struct drm_connector *connector)
+{
+ struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
+ const struct drm_display_mode *m = kingdisplay->desc->modes;
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_duplicate(connector->dev, m);
+ if (!mode) {
+ dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
+ m->hdisplay, m->vdisplay, drm_mode_vrefresh(m));
+ return -ENOMEM;
+ }
+
+ mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+ drm_mode_set_name(mode);
+ drm_mode_probed_add(connector, mode);
+
+ connector->display_info.width_mm = kingdisplay->desc->size.width_mm;
+ connector->display_info.height_mm = kingdisplay->desc->size.height_mm;
+ connector->display_info.bpc = kingdisplay->desc->bpc;
+ /*
+ * TODO: Remove once all drm drivers call
+ * drm_connector_set_orientation_from_panel()
+ */
+ drm_connector_set_panel_orientation(connector, kingdisplay->orientation);
+
+ return 1;
+}
+
+static enum drm_panel_orientation kingdisplay_panel_get_orientation(struct drm_panel *panel)
+{
+ struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
+
+ return kingdisplay->orientation;
+}
+
+static const struct drm_panel_funcs kingdisplay_panel_funcs = {
+ .disable = kingdisplay_panel_disable,
+ .unprepare = kingdisplay_panel_unprepare,
+ .prepare = kingdisplay_panel_prepare,
+ .enable = kingdisplay_panel_enable,
+ .get_modes = kingdisplay_panel_get_modes,
+ .get_orientation = kingdisplay_panel_get_orientation,
+};
+
+static int kingdisplay_panel_add(struct kingdisplay_panel *kingdisplay)
+{
+ struct device *dev = &kingdisplay->dsi->dev;
+ int err;
+
+ kingdisplay->pp3300 = devm_regulator_get(dev, "pp3300");
+ if (IS_ERR(kingdisplay->pp3300))
+ return PTR_ERR(kingdisplay->pp3300);
+
+
+ kingdisplay->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+ if (IS_ERR(kingdisplay->enable_gpio)) {
+ dev_err(dev, "cannot get reset-gpios %ld\n",
+ PTR_ERR(kingdisplay->enable_gpio));
+ return PTR_ERR(kingdisplay->enable_gpio);
+ }
+
+ gpiod_set_value(kingdisplay->enable_gpio, 0);
+
+ drm_panel_init(&kingdisplay->base, dev, &kingdisplay_panel_funcs,
+ DRM_MODE_CONNECTOR_DSI);
+ err = of_drm_get_panel_orientation(dev->of_node, &kingdisplay->orientation);
+ if (err < 0) {
+ dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, err);
+ return err;
+ }
+
+ err = drm_panel_of_backlight(&kingdisplay->base);
+ if (err)
+ return err;
+
+ kingdisplay->base.funcs = &kingdisplay_panel_funcs;
+ kingdisplay->base.dev = &kingdisplay->dsi->dev;
+
+ drm_panel_add(&kingdisplay->base);
+
+ return 0;
+}
+
+static int kingdisplay_panel_probe(struct mipi_dsi_device *dsi)
+{
+ struct kingdisplay_panel *kingdisplay;
+ int ret;
+ const struct panel_desc *desc;
+
+ kingdisplay = devm_kzalloc(&dsi->dev, sizeof(*kingdisplay), GFP_KERNEL);
+ if (!kingdisplay)
+ return -ENOMEM;
+
+ desc = of_device_get_match_data(&dsi->dev);
+ dsi->lanes = desc->lanes;
+ dsi->format = desc->format;
+ dsi->mode_flags = desc->mode_flags;
+ kingdisplay->desc = desc;
+ kingdisplay->dsi = dsi;
+ ret = kingdisplay_panel_add(kingdisplay);
+ if (ret < 0)
+ return ret;
+
+ mipi_dsi_set_drvdata(dsi, kingdisplay);
+
+ ret = mipi_dsi_attach(dsi);
+ if (ret)
+ drm_panel_remove(&kingdisplay->base);
+
+ return ret;
+}
+
+static void kingdisplay_panel_shutdown(struct mipi_dsi_device *dsi)
+{
+ struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
+
+ drm_panel_disable(&kingdisplay->base);
+ drm_panel_unprepare(&kingdisplay->base);
+}
+
+static void kingdisplay_panel_remove(struct mipi_dsi_device *dsi)
+{
+ struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
+ int ret;
+
+ kingdisplay_panel_shutdown(dsi);
+
+ ret = mipi_dsi_detach(dsi);
+ if (ret < 0)
+ dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
+
+ if (kingdisplay->base.dev)
+ drm_panel_remove(&kingdisplay->base);
+}
+
+static const struct of_device_id kingdisplay_of_match[] = {
+ { .compatible = "kingdisplay,kd101ne3-40ti",
+ .data = &kingdisplay_kd101ne3_40ti_desc
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, kingdisplay_of_match);
+
+static struct mipi_dsi_driver kingdisplay_panel_driver = {
+ .driver = {
+ .name = "panel-kingdisplay-kd101ne3",
+ .of_match_table = kingdisplay_of_match,
+ },
+ .probe = kingdisplay_panel_probe,
+ .remove = kingdisplay_panel_remove,
+ .shutdown = kingdisplay_panel_shutdown,
+};
+module_mipi_dsi_driver(kingdisplay_panel_driver);
+
+MODULE_AUTHOR("Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>");
+MODULE_DESCRIPTION("kingdisplay kd101ne3-40ti 800x1280 video mode panel driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong
@ 2024-04-18 11:46 ` Dmitry Baryshkov
2024-04-18 13:11 ` Hsin-Yi Wang
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Baryshkov @ 2024-04-18 11:46 UTC (permalink / raw)
To: lvzhaoxiong
Cc: mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt,
dianders, hsinyi, dri-devel, devicetree, linux-kernel
On Thu, Apr 18, 2024 at 04:15:48PM +0800, lvzhaoxiong wrote:
> The kingdisplay panel is based on JD9365DA controller.
> Add a driver for it.
>
> Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com>
> ---
> drivers/gpu/drm/panel/Kconfig | 9 +
> drivers/gpu/drm/panel/Makefile | 1 +
> .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++
> 3 files changed, 617 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 154f5bf82980..2c73086cf102 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04
> 24 bit RGB per pixel. It provides a MIPI DSI interface to
> the host and has a built-in LED backlight.
>
> +config DRM_PANEL_KINGDISPLAY_KD101NE3
> + tristate "Kingdisplay kd101ne3 panel"
> + 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
> + Kingdisplay kd101ne3 controller.
> +
> config DRM_PANEL_LEADTEK_LTK050H3146W
> tristate "Leadtek LTK050H3146W panel"
> depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 24a02655d726..cbd414b98bb0 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o
> obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o
> obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o
> obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
> +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o
> obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
> obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
> obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
> diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> new file mode 100644
> index 000000000000..dbf0992f8b81
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> @@ -0,0 +1,607 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Panels based on the JD9365DA display controller.
> + * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +struct panel_desc {
> + const struct drm_display_mode *modes;
> + unsigned int bpc;
> +
> + /**
> + * @width_mm: width of the panel's active display area
> + * @height_mm: height of the panel's active display area
> + */
> + struct {
> + unsigned int width_mm;
> + unsigned int height_mm;
Please move to the declared mode;
> + } size;
> +
> + unsigned long mode_flags;
> + enum mipi_dsi_pixel_format format;
> + const struct panel_init_cmd *init_cmds;
> + unsigned int lanes;
> + bool discharge_on_disable;
> + bool lp11_before_reset;
> +};
> +
> +struct kingdisplay_panel {
> + struct drm_panel base;
> + struct mipi_dsi_device *dsi;
> +
> + const struct panel_desc *desc;
> +
> + enum drm_panel_orientation orientation;
> + struct regulator *pp3300;
> + struct gpio_desc *enable_gpio;
> +};
> +
> +enum dsi_cmd_type {
> + INIT_DCS_CMD,
> + DELAY_CMD,
> +};
> +
> +struct panel_init_cmd {
> + enum dsi_cmd_type type;
> + size_t len;
> + const char *data;
> +};
> +
> +#define _INIT_DCS_CMD(...) { \
> + .type = INIT_DCS_CMD, \
> + .len = sizeof((char[]){__VA_ARGS__}), \
> + .data = (char[]){__VA_ARGS__} }
> +
> +#define _INIT_DELAY_CMD(...) { \
> + .type = DELAY_CMD,\
> + .len = sizeof((char[]){__VA_ARGS__}), \
> + .data = (char[]){__VA_ARGS__} }
This is the third panel driver using the same appoach. Can you use
mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
the table, we should extract this framework to a common helper.
(my preference is shifted towards mipi_dsi_generic_write_seq()).
> +
> +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> + _INIT_DELAY_CMD(50),
> + _INIT_DCS_CMD(0xE0, 0x00),
> + _INIT_DCS_CMD(0xE1, 0x93),
> + _INIT_DCS_CMD(0xE2, 0x65),
> + _INIT_DCS_CMD(0xE3, 0xF8),
> + _INIT_DCS_CMD(0x80, 0x03),
> + _INIT_DCS_CMD(0xE0, 0x01),
> + _INIT_DCS_CMD(0x0C, 0x74),
> + _INIT_DCS_CMD(0x17, 0x00),
> + _INIT_DCS_CMD(0x18, 0xC7),
> + _INIT_DCS_CMD(0x19, 0x01),
> + _INIT_DCS_CMD(0x1A, 0x00),
> + _INIT_DCS_CMD(0x1B, 0xC7),
> + _INIT_DCS_CMD(0x1C, 0x01),
> + _INIT_DCS_CMD(0x24, 0xFE),
> + _INIT_DCS_CMD(0x37, 0x19),
> + _INIT_DCS_CMD(0x35, 0x28),
> + _INIT_DCS_CMD(0x38, 0x05),
> + _INIT_DCS_CMD(0x39, 0x08),
> + _INIT_DCS_CMD(0x3A, 0x12),
> + _INIT_DCS_CMD(0x3C, 0x7E),
> + _INIT_DCS_CMD(0x3D, 0xFF),
> + _INIT_DCS_CMD(0x3E, 0xFF),
> + _INIT_DCS_CMD(0x3F, 0x7F),
> + _INIT_DCS_CMD(0x40, 0x06),
> + _INIT_DCS_CMD(0x41, 0xA0),
> + _INIT_DCS_CMD(0x43, 0x1E),
> + _INIT_DCS_CMD(0x44, 0x0B),
> + _INIT_DCS_CMD(0x55, 0x02),
> + _INIT_DCS_CMD(0x57, 0x6A),
> + _INIT_DCS_CMD(0x59, 0x0A),
> + _INIT_DCS_CMD(0x5A, 0x2E),
> + _INIT_DCS_CMD(0x5B, 0x1A),
> + _INIT_DCS_CMD(0x5C, 0x15),
> + _INIT_DCS_CMD(0x5D, 0x7F),
> + _INIT_DCS_CMD(0x5E, 0x61),
> + _INIT_DCS_CMD(0x5F, 0x50),
> + _INIT_DCS_CMD(0x60, 0x43),
> + _INIT_DCS_CMD(0x61, 0x3F),
> + _INIT_DCS_CMD(0x62, 0x32),
> + _INIT_DCS_CMD(0x63, 0x35),
> + _INIT_DCS_CMD(0x64, 0x1F),
> + _INIT_DCS_CMD(0x65, 0x38),
> + _INIT_DCS_CMD(0x66, 0x36),
> + _INIT_DCS_CMD(0x67, 0x36),
> + _INIT_DCS_CMD(0x68, 0x54),
> + _INIT_DCS_CMD(0x69, 0x42),
> + _INIT_DCS_CMD(0x6A, 0x48),
> + _INIT_DCS_CMD(0x6B, 0x39),
> + _INIT_DCS_CMD(0x6C, 0x34),
> + _INIT_DCS_CMD(0x6D, 0x26),
> + _INIT_DCS_CMD(0x6E, 0x14),
> + _INIT_DCS_CMD(0x6F, 0x02),
> + _INIT_DCS_CMD(0x70, 0x7F),
> + _INIT_DCS_CMD(0x71, 0x61),
> + _INIT_DCS_CMD(0x72, 0x50),
> + _INIT_DCS_CMD(0x73, 0x43),
> + _INIT_DCS_CMD(0x74, 0x3F),
> + _INIT_DCS_CMD(0x75, 0x32),
> + _INIT_DCS_CMD(0x76, 0x35),
> + _INIT_DCS_CMD(0x77, 0x1F),
> + _INIT_DCS_CMD(0x78, 0x38),
> + _INIT_DCS_CMD(0x79, 0x36),
> + _INIT_DCS_CMD(0x7A, 0x36),
> + _INIT_DCS_CMD(0x7B, 0x54),
> + _INIT_DCS_CMD(0x7C, 0x42),
> + _INIT_DCS_CMD(0x7D, 0x48),
> + _INIT_DCS_CMD(0x7E, 0x39),
> + _INIT_DCS_CMD(0x7F, 0x34),
> + _INIT_DCS_CMD(0x80, 0x26),
> + _INIT_DCS_CMD(0x81, 0x14),
> + _INIT_DCS_CMD(0x82, 0x02),
> + _INIT_DCS_CMD(0xE0, 0x02),
> + _INIT_DCS_CMD(0x00, 0x52),
> + _INIT_DCS_CMD(0x01, 0x5F),
> + _INIT_DCS_CMD(0x02, 0x5F),
> + _INIT_DCS_CMD(0x03, 0x50),
> + _INIT_DCS_CMD(0x04, 0x77),
> + _INIT_DCS_CMD(0x05, 0x57),
> + _INIT_DCS_CMD(0x06, 0x5F),
> + _INIT_DCS_CMD(0x07, 0x4E),
> + _INIT_DCS_CMD(0x08, 0x4C),
> + _INIT_DCS_CMD(0x09, 0x5F),
> + _INIT_DCS_CMD(0x0A, 0x4A),
> + _INIT_DCS_CMD(0x0B, 0x48),
> + _INIT_DCS_CMD(0x0C, 0x5F),
> + _INIT_DCS_CMD(0x0D, 0x46),
> + _INIT_DCS_CMD(0x0E, 0x44),
> + _INIT_DCS_CMD(0x0F, 0x40),
> + _INIT_DCS_CMD(0x10, 0x5F),
> + _INIT_DCS_CMD(0x11, 0x5F),
> + _INIT_DCS_CMD(0x12, 0x5F),
> + _INIT_DCS_CMD(0x13, 0x5F),
> + _INIT_DCS_CMD(0x14, 0x5F),
> + _INIT_DCS_CMD(0x15, 0x5F),
> + _INIT_DCS_CMD(0x16, 0x53),
> + _INIT_DCS_CMD(0x17, 0x5F),
> + _INIT_DCS_CMD(0x18, 0x5F),
> + _INIT_DCS_CMD(0x19, 0x51),
> + _INIT_DCS_CMD(0x1A, 0x77),
> + _INIT_DCS_CMD(0x1B, 0x57),
> + _INIT_DCS_CMD(0x1C, 0x5F),
> + _INIT_DCS_CMD(0x1D, 0x4F),
> + _INIT_DCS_CMD(0x1E, 0x4D),
> + _INIT_DCS_CMD(0x1F, 0x5F),
> + _INIT_DCS_CMD(0x20, 0x4B),
> + _INIT_DCS_CMD(0x21, 0x49),
> + _INIT_DCS_CMD(0x22, 0x5F),
> + _INIT_DCS_CMD(0x23, 0x47),
> + _INIT_DCS_CMD(0x24, 0x45),
> + _INIT_DCS_CMD(0x25, 0x41),
> + _INIT_DCS_CMD(0x26, 0x5F),
> + _INIT_DCS_CMD(0x27, 0x5F),
> + _INIT_DCS_CMD(0x28, 0x5F),
> + _INIT_DCS_CMD(0x29, 0x5F),
> + _INIT_DCS_CMD(0x2A, 0x5F),
> + _INIT_DCS_CMD(0x2B, 0x5F),
> + _INIT_DCS_CMD(0x2C, 0x13),
> + _INIT_DCS_CMD(0x2D, 0x1F),
> + _INIT_DCS_CMD(0x2E, 0x1F),
> + _INIT_DCS_CMD(0x2F, 0x01),
> + _INIT_DCS_CMD(0x30, 0x17),
> + _INIT_DCS_CMD(0x31, 0x17),
> + _INIT_DCS_CMD(0x32, 0x1F),
> + _INIT_DCS_CMD(0x33, 0x0D),
> + _INIT_DCS_CMD(0x34, 0x0F),
> + _INIT_DCS_CMD(0x35, 0x1F),
> + _INIT_DCS_CMD(0x36, 0x05),
> + _INIT_DCS_CMD(0x37, 0x07),
> + _INIT_DCS_CMD(0x38, 0x1F),
> + _INIT_DCS_CMD(0x39, 0x09),
> + _INIT_DCS_CMD(0x3A, 0x0B),
> + _INIT_DCS_CMD(0x3B, 0x11),
> + _INIT_DCS_CMD(0x3C, 0x1F),
> + _INIT_DCS_CMD(0x3D, 0x1F),
> + _INIT_DCS_CMD(0x3E, 0x1F),
> + _INIT_DCS_CMD(0x3F, 0x1F),
> + _INIT_DCS_CMD(0x40, 0x1F),
> + _INIT_DCS_CMD(0x41, 0x1F),
> + _INIT_DCS_CMD(0x42, 0x12),
> + _INIT_DCS_CMD(0x43, 0x1F),
> + _INIT_DCS_CMD(0x44, 0x1F),
> + _INIT_DCS_CMD(0x45, 0x00),
> + _INIT_DCS_CMD(0x46, 0x17),
> + _INIT_DCS_CMD(0x47, 0x17),
> + _INIT_DCS_CMD(0x48, 0x1F),
> + _INIT_DCS_CMD(0x49, 0x0C),
> + _INIT_DCS_CMD(0x4A, 0x0E),
> + _INIT_DCS_CMD(0x4B, 0x1F),
> + _INIT_DCS_CMD(0x4C, 0x04),
> + _INIT_DCS_CMD(0x4D, 0x06),
> + _INIT_DCS_CMD(0x4E, 0x1F),
> + _INIT_DCS_CMD(0x4F, 0x08),
> + _INIT_DCS_CMD(0x50, 0x0A),
> + _INIT_DCS_CMD(0x51, 0x10),
> + _INIT_DCS_CMD(0x52, 0x1F),
> + _INIT_DCS_CMD(0x53, 0x1F),
> + _INIT_DCS_CMD(0x54, 0x1F),
> + _INIT_DCS_CMD(0x55, 0x1F),
> + _INIT_DCS_CMD(0x56, 0x1F),
> + _INIT_DCS_CMD(0x57, 0x1F),
> + _INIT_DCS_CMD(0x58, 0x40),
> + _INIT_DCS_CMD(0x5B, 0x10),
> + _INIT_DCS_CMD(0x5C, 0x06),
> + _INIT_DCS_CMD(0x5D, 0x40),
> + _INIT_DCS_CMD(0x5E, 0x00),
> + _INIT_DCS_CMD(0x5F, 0x00),
> + _INIT_DCS_CMD(0x60, 0x40),
> + _INIT_DCS_CMD(0x61, 0x03),
> + _INIT_DCS_CMD(0x62, 0x04),
> + _INIT_DCS_CMD(0x63, 0x6C),
> + _INIT_DCS_CMD(0x64, 0x6C),
> + _INIT_DCS_CMD(0x65, 0x75),
> + _INIT_DCS_CMD(0x66, 0x08),
> + _INIT_DCS_CMD(0x67, 0xB4),
> + _INIT_DCS_CMD(0x68, 0x08),
> + _INIT_DCS_CMD(0x69, 0x6C),
> + _INIT_DCS_CMD(0x6A, 0x6C),
> + _INIT_DCS_CMD(0x6B, 0x0C),
> + _INIT_DCS_CMD(0x6D, 0x00),
> + _INIT_DCS_CMD(0x6E, 0x00),
> + _INIT_DCS_CMD(0x6F, 0x88),
> + _INIT_DCS_CMD(0x75, 0xBB),
> + _INIT_DCS_CMD(0x76, 0x00),
> + _INIT_DCS_CMD(0x77, 0x05),
> + _INIT_DCS_CMD(0x78, 0x2A),
> + _INIT_DCS_CMD(0xE0, 0x04),
> + _INIT_DCS_CMD(0x00, 0x0E),
> + _INIT_DCS_CMD(0x02, 0xB3),
> + _INIT_DCS_CMD(0x09, 0x61),
> + _INIT_DCS_CMD(0x0E, 0x48),
> +
> + _INIT_DCS_CMD(0xE0, 0x00),
> + _INIT_DCS_CMD(0X11),
> + /* T6: 120ms */
> + _INIT_DELAY_CMD(120),
> + _INIT_DCS_CMD(0X29),
> + _INIT_DELAY_CMD(20),
> + {},
> +};
> +
> +static inline struct kingdisplay_panel *to_kingdisplay_panel(struct drm_panel *panel)
> +{
> + return container_of(panel, struct kingdisplay_panel, base);
> +}
> +
> +static int kingdisplay_panel_init_dcs_cmd(struct kingdisplay_panel *kingdisplay)
> +{
> + struct mipi_dsi_device *dsi = kingdisplay->dsi;
> + struct drm_panel *panel = &kingdisplay->base;
> + int i, err = 0;
> +
> + if (kingdisplay->desc->init_cmds) {
> + const struct panel_init_cmd *init_cmds = kingdisplay->desc->init_cmds;
> +
> + for (i = 0; init_cmds[i].len != 0; i++) {
> + const struct panel_init_cmd *cmd = &init_cmds[i];
> +
> + switch (cmd->type) {
> + case DELAY_CMD:
> + msleep(cmd->data[0]);
> + err = 0;
> + break;
> +
> + case INIT_DCS_CMD:
> + err = mipi_dsi_dcs_write(dsi, cmd->data[0],
> + cmd->len <= 1 ? NULL :
> + &cmd->data[1],
> + cmd->len - 1);
> + break;
> +
> + default:
> + err = -EINVAL;
> + }
> +
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to write command %u\n", i);
> + return err;
> + }
> + }
> + }
> + return 0;
> +}
> +
> +static int kingdisplay_panel_enter_sleep_mode(struct kingdisplay_panel *kingdisplay)
> +{
> + struct mipi_dsi_device *dsi = kingdisplay->dsi;
> + int ret;
> +
> + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> + usleep_range(1000, 2000);
> + ret = mipi_dsi_dcs_set_display_off(dsi);
> + if (ret < 0)
> + return ret;
> +
> + msleep(50);
> +
> + ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int kingdisplay_panel_disable(struct drm_panel *panel)
> +{
> + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> + int ret;
> +
> + ret = kingdisplay_panel_enter_sleep_mode(kingdisplay);
> + if (ret < 0) {
> + dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> + return ret;
> + }
> +
> + msleep(100);
> +
> + return 0;
> +}
> +
> +static int kingdisplay_panel_unprepare(struct drm_panel *panel)
> +{
> + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> + int err;
> +
> + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0);
> +
> + /* T15: 2ms */
> + usleep_range(1000, 2000);
> +
> + err = regulator_disable(kingdisplay->pp3300);
> + if (err < 0)
> + return err;
> +
> + return 0;
> +}
> +
> +static int kingdisplay_panel_prepare(struct drm_panel *panel)
> +{
> + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> + int ret;
> +
> + gpiod_set_value(kingdisplay->enable_gpio, 0);
> +
> + ret = regulator_enable(kingdisplay->pp3300);
> + if (ret < 0)
> + return ret;
> +
> + /* T1: 5ms */
> + usleep_range(5000, 6000);
> +
> + if (kingdisplay->desc->lp11_before_reset) {
> + mipi_dsi_dcs_nop(kingdisplay->dsi);
> + usleep_range(1000, 2000);
> + }
> +
> + /* T2: 10ms, T1 + T2 > 5ms*/
> + usleep_range(10000, 11000);
> +
> + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 1);
> +
> + ret = kingdisplay_panel_init_dcs_cmd(kingdisplay);
> + if (ret < 0) {
> + dev_err(panel->dev, "failed to init panel: %d\n", ret);
> + goto poweroff;
> + }
> +
> + return 0;
> +
> +poweroff:
> + regulator_disable(kingdisplay->pp3300);
> + /* T6: 2ms */
> + usleep_range(1000, 2000);
> + gpiod_set_value(kingdisplay->enable_gpio, 0);
> +
> + return ret;
> +}
> +
> +static int kingdisplay_panel_enable(struct drm_panel *panel)
> +{
> + msleep(130);
> + return 0;
> +}
> +
> +static const struct drm_display_mode kingdisplay_kd101ne3_40ti_default_mode = {
> + .clock = 70595,
> + .hdisplay = 800,
> + .hsync_start = 800 + 30,
> + .hsync_end = 800 + 30 + 30,
> + .htotal = 800 + 30 + 30 + 30,
> + .vdisplay = 1280,
> + .vsync_start = 1280 + 30,
> + .vsync_end = 1280 + 30 + 4,
> + .vtotal = 1280 + 30 + 4 + 8,
> + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +};
> +
> +static const struct panel_desc kingdisplay_kd101ne3_40ti_desc = {
> + .modes = &kingdisplay_kd101ne3_40ti_default_mode,
> + .bpc = 8,
> + .size = {
> + .width_mm = 135,
> + .height_mm = 216,
> + },
> + .lanes = 4,
> + .format = MIPI_DSI_FMT_RGB888,
> + .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> + MIPI_DSI_MODE_LPM,
> + .init_cmds = kingdisplay_kd101ne3_init_cmd,
> + .lp11_before_reset = true,
> +};
> +
> +static int kingdisplay_panel_get_modes(struct drm_panel *panel,
> + struct drm_connector *connector)
> +{
> + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> + const struct drm_display_mode *m = kingdisplay->desc->modes;
> + struct drm_display_mode *mode;
> +
> + mode = drm_mode_duplicate(connector->dev, m);
> + if (!mode) {
> + dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> + m->hdisplay, m->vdisplay, drm_mode_vrefresh(m));
> + return -ENOMEM;
> + }
> +
> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> + drm_mode_set_name(mode);
> + drm_mode_probed_add(connector, mode);
> +
> + connector->display_info.width_mm = kingdisplay->desc->size.width_mm;
> + connector->display_info.height_mm = kingdisplay->desc->size.height_mm;
Please use drm_connector_helper_get_modes_fixed()
> + connector->display_info.bpc = kingdisplay->desc->bpc;
> + /*
> + * TODO: Remove once all drm drivers call
> + * drm_connector_set_orientation_from_panel()
What is your usecase / platform? Are you using drm_bridge_connector? If not, why?
> + */
> + drm_connector_set_panel_orientation(connector, kingdisplay->orientation);
> +
> + return 1;
> +}
> +
> +static enum drm_panel_orientation kingdisplay_panel_get_orientation(struct drm_panel *panel)
> +{
> + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> +
> + return kingdisplay->orientation;
> +}
> +
> +static const struct drm_panel_funcs kingdisplay_panel_funcs = {
> + .disable = kingdisplay_panel_disable,
> + .unprepare = kingdisplay_panel_unprepare,
> + .prepare = kingdisplay_panel_prepare,
> + .enable = kingdisplay_panel_enable,
> + .get_modes = kingdisplay_panel_get_modes,
> + .get_orientation = kingdisplay_panel_get_orientation,
> +};
> +
> +static int kingdisplay_panel_add(struct kingdisplay_panel *kingdisplay)
> +{
> + struct device *dev = &kingdisplay->dsi->dev;
> + int err;
> +
> + kingdisplay->pp3300 = devm_regulator_get(dev, "pp3300");
> + if (IS_ERR(kingdisplay->pp3300))
> + return PTR_ERR(kingdisplay->pp3300);
> +
> +
> + kingdisplay->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> + if (IS_ERR(kingdisplay->enable_gpio)) {
> + dev_err(dev, "cannot get reset-gpios %ld\n",
> + PTR_ERR(kingdisplay->enable_gpio));
> + return PTR_ERR(kingdisplay->enable_gpio);
> + }
> +
> + gpiod_set_value(kingdisplay->enable_gpio, 0);
> +
> + drm_panel_init(&kingdisplay->base, dev, &kingdisplay_panel_funcs,
> + DRM_MODE_CONNECTOR_DSI);
> + err = of_drm_get_panel_orientation(dev->of_node, &kingdisplay->orientation);
> + if (err < 0) {
> + dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, err);
> + return err;
> + }
> +
> + err = drm_panel_of_backlight(&kingdisplay->base);
> + if (err)
> + return err;
> +
> + kingdisplay->base.funcs = &kingdisplay_panel_funcs;
> + kingdisplay->base.dev = &kingdisplay->dsi->dev;
> +
> + drm_panel_add(&kingdisplay->base);
> +
> + return 0;
> +}
> +
> +static int kingdisplay_panel_probe(struct mipi_dsi_device *dsi)
> +{
> + struct kingdisplay_panel *kingdisplay;
> + int ret;
> + const struct panel_desc *desc;
> +
> + kingdisplay = devm_kzalloc(&dsi->dev, sizeof(*kingdisplay), GFP_KERNEL);
> + if (!kingdisplay)
> + return -ENOMEM;
> +
> + desc = of_device_get_match_data(&dsi->dev);
> + dsi->lanes = desc->lanes;
> + dsi->format = desc->format;
> + dsi->mode_flags = desc->mode_flags;
> + kingdisplay->desc = desc;
> + kingdisplay->dsi = dsi;
> + ret = kingdisplay_panel_add(kingdisplay);
> + if (ret < 0)
> + return ret;
> +
> + mipi_dsi_set_drvdata(dsi, kingdisplay);
> +
> + ret = mipi_dsi_attach(dsi);
> + if (ret)
> + drm_panel_remove(&kingdisplay->base);
> +
> + return ret;
> +}
> +
> +static void kingdisplay_panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
> +
> + drm_panel_disable(&kingdisplay->base);
> + drm_panel_unprepare(&kingdisplay->base);
> +}
> +
> +static void kingdisplay_panel_remove(struct mipi_dsi_device *dsi)
> +{
> + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
> + int ret;
> +
> + kingdisplay_panel_shutdown(dsi);
> +
> + ret = mipi_dsi_detach(dsi);
> + if (ret < 0)
> + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> +
> + if (kingdisplay->base.dev)
> + drm_panel_remove(&kingdisplay->base);
You are adding it unconditionally, so there should be no condition on
removal of the panel.
> +}
> +
> +static const struct of_device_id kingdisplay_of_match[] = {
> + { .compatible = "kingdisplay,kd101ne3-40ti",
> + .data = &kingdisplay_kd101ne3_40ti_desc
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, kingdisplay_of_match);
> +
> +static struct mipi_dsi_driver kingdisplay_panel_driver = {
> + .driver = {
> + .name = "panel-kingdisplay-kd101ne3",
> + .of_match_table = kingdisplay_of_match,
> + },
> + .probe = kingdisplay_panel_probe,
> + .remove = kingdisplay_panel_remove,
> + .shutdown = kingdisplay_panel_shutdown,
> +};
> +module_mipi_dsi_driver(kingdisplay_panel_driver);
> +
> +MODULE_AUTHOR("Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>");
> +MODULE_DESCRIPTION("kingdisplay kd101ne3-40ti 800x1280 video mode panel driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-18 11:46 ` Dmitry Baryshkov
@ 2024-04-18 13:11 ` Hsin-Yi Wang
2024-04-18 14:11 ` Dmitry Baryshkov
0 siblings, 1 reply; 19+ messages in thread
From: Hsin-Yi Wang @ 2024-04-18 13:11 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dianders, dri-devel, devicetree,
linux-kernel
On Thu, Apr 18, 2024 at 7:46 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, Apr 18, 2024 at 04:15:48PM +0800, lvzhaoxiong wrote:
> > The kingdisplay panel is based on JD9365DA controller.
> > Add a driver for it.
> >
> > Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com>
> > ---
> > drivers/gpu/drm/panel/Kconfig | 9 +
> > drivers/gpu/drm/panel/Makefile | 1 +
> > .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++
> > 3 files changed, 617 insertions(+)
> > create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> >
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index 154f5bf82980..2c73086cf102 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04
> > 24 bit RGB per pixel. It provides a MIPI DSI interface to
> > the host and has a built-in LED backlight.
> >
> > +config DRM_PANEL_KINGDISPLAY_KD101NE3
> > + tristate "Kingdisplay kd101ne3 panel"
> > + 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
> > + Kingdisplay kd101ne3 controller.
> > +
> > config DRM_PANEL_LEADTEK_LTK050H3146W
> > tristate "Leadtek LTK050H3146W panel"
> > depends on OF
> > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > index 24a02655d726..cbd414b98bb0 100644
> > --- a/drivers/gpu/drm/panel/Makefile
> > +++ b/drivers/gpu/drm/panel/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o
> > obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o
> > obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o
> > obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
> > +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o
> > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
> > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
> > obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
> > diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > new file mode 100644
> > index 000000000000..dbf0992f8b81
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > @@ -0,0 +1,607 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Panels based on the JD9365DA display controller.
> > + * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <drm/drm_connector.h>
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_mipi_dsi.h>
> > +#include <drm/drm_panel.h>
> > +
> > +#include <video/mipi_display.h>
> > +
> > +struct panel_desc {
> > + const struct drm_display_mode *modes;
> > + unsigned int bpc;
> > +
> > + /**
> > + * @width_mm: width of the panel's active display area
> > + * @height_mm: height of the panel's active display area
> > + */
> > + struct {
> > + unsigned int width_mm;
> > + unsigned int height_mm;
>
> Please move to the declared mode;
>
> > + } size;
> > +
> > + unsigned long mode_flags;
> > + enum mipi_dsi_pixel_format format;
> > + const struct panel_init_cmd *init_cmds;
> > + unsigned int lanes;
> > + bool discharge_on_disable;
> > + bool lp11_before_reset;
> > +};
> > +
> > +struct kingdisplay_panel {
> > + struct drm_panel base;
> > + struct mipi_dsi_device *dsi;
> > +
> > + const struct panel_desc *desc;
> > +
> > + enum drm_panel_orientation orientation;
> > + struct regulator *pp3300;
> > + struct gpio_desc *enable_gpio;
> > +};
> > +
> > +enum dsi_cmd_type {
> > + INIT_DCS_CMD,
> > + DELAY_CMD,
> > +};
> > +
> > +struct panel_init_cmd {
> > + enum dsi_cmd_type type;
> > + size_t len;
> > + const char *data;
> > +};
> > +
> > +#define _INIT_DCS_CMD(...) { \
> > + .type = INIT_DCS_CMD, \
> > + .len = sizeof((char[]){__VA_ARGS__}), \
> > + .data = (char[]){__VA_ARGS__} }
> > +
> > +#define _INIT_DELAY_CMD(...) { \
> > + .type = DELAY_CMD,\
> > + .len = sizeof((char[]){__VA_ARGS__}), \
> > + .data = (char[]){__VA_ARGS__} }
>
> This is the third panel driver using the same appoach. Can you use
> mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> the table, we should extract this framework to a common helper.
> (my preference is shifted towards mipi_dsi_generic_write_seq()).
>
The drawback of mipi_dsi_generic_write_seq() is that it can cause the
kernel size grows a lot since every sequence will be expanded.
Similar discussion in here:
https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
This patch would increase the module size from 157K to 572K.
scripts/bloat-o-meter shows chg +235.95%.
So maybe the common helper is better regarding the kernel module size?
> > +
> > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > + _INIT_DELAY_CMD(50),
> > + _INIT_DCS_CMD(0xE0, 0x00),
> > + _INIT_DCS_CMD(0xE1, 0x93),
> > + _INIT_DCS_CMD(0xE2, 0x65),
> > + _INIT_DCS_CMD(0xE3, 0xF8),
> > + _INIT_DCS_CMD(0x80, 0x03),
> > + _INIT_DCS_CMD(0xE0, 0x01),
> > + _INIT_DCS_CMD(0x0C, 0x74),
> > + _INIT_DCS_CMD(0x17, 0x00),
> > + _INIT_DCS_CMD(0x18, 0xC7),
> > + _INIT_DCS_CMD(0x19, 0x01),
> > + _INIT_DCS_CMD(0x1A, 0x00),
> > + _INIT_DCS_CMD(0x1B, 0xC7),
> > + _INIT_DCS_CMD(0x1C, 0x01),
> > + _INIT_DCS_CMD(0x24, 0xFE),
> > + _INIT_DCS_CMD(0x37, 0x19),
> > + _INIT_DCS_CMD(0x35, 0x28),
> > + _INIT_DCS_CMD(0x38, 0x05),
> > + _INIT_DCS_CMD(0x39, 0x08),
> > + _INIT_DCS_CMD(0x3A, 0x12),
> > + _INIT_DCS_CMD(0x3C, 0x7E),
> > + _INIT_DCS_CMD(0x3D, 0xFF),
> > + _INIT_DCS_CMD(0x3E, 0xFF),
> > + _INIT_DCS_CMD(0x3F, 0x7F),
> > + _INIT_DCS_CMD(0x40, 0x06),
> > + _INIT_DCS_CMD(0x41, 0xA0),
> > + _INIT_DCS_CMD(0x43, 0x1E),
> > + _INIT_DCS_CMD(0x44, 0x0B),
> > + _INIT_DCS_CMD(0x55, 0x02),
> > + _INIT_DCS_CMD(0x57, 0x6A),
> > + _INIT_DCS_CMD(0x59, 0x0A),
> > + _INIT_DCS_CMD(0x5A, 0x2E),
> > + _INIT_DCS_CMD(0x5B, 0x1A),
> > + _INIT_DCS_CMD(0x5C, 0x15),
> > + _INIT_DCS_CMD(0x5D, 0x7F),
> > + _INIT_DCS_CMD(0x5E, 0x61),
> > + _INIT_DCS_CMD(0x5F, 0x50),
> > + _INIT_DCS_CMD(0x60, 0x43),
> > + _INIT_DCS_CMD(0x61, 0x3F),
> > + _INIT_DCS_CMD(0x62, 0x32),
> > + _INIT_DCS_CMD(0x63, 0x35),
> > + _INIT_DCS_CMD(0x64, 0x1F),
> > + _INIT_DCS_CMD(0x65, 0x38),
> > + _INIT_DCS_CMD(0x66, 0x36),
> > + _INIT_DCS_CMD(0x67, 0x36),
> > + _INIT_DCS_CMD(0x68, 0x54),
> > + _INIT_DCS_CMD(0x69, 0x42),
> > + _INIT_DCS_CMD(0x6A, 0x48),
> > + _INIT_DCS_CMD(0x6B, 0x39),
> > + _INIT_DCS_CMD(0x6C, 0x34),
> > + _INIT_DCS_CMD(0x6D, 0x26),
> > + _INIT_DCS_CMD(0x6E, 0x14),
> > + _INIT_DCS_CMD(0x6F, 0x02),
> > + _INIT_DCS_CMD(0x70, 0x7F),
> > + _INIT_DCS_CMD(0x71, 0x61),
> > + _INIT_DCS_CMD(0x72, 0x50),
> > + _INIT_DCS_CMD(0x73, 0x43),
> > + _INIT_DCS_CMD(0x74, 0x3F),
> > + _INIT_DCS_CMD(0x75, 0x32),
> > + _INIT_DCS_CMD(0x76, 0x35),
> > + _INIT_DCS_CMD(0x77, 0x1F),
> > + _INIT_DCS_CMD(0x78, 0x38),
> > + _INIT_DCS_CMD(0x79, 0x36),
> > + _INIT_DCS_CMD(0x7A, 0x36),
> > + _INIT_DCS_CMD(0x7B, 0x54),
> > + _INIT_DCS_CMD(0x7C, 0x42),
> > + _INIT_DCS_CMD(0x7D, 0x48),
> > + _INIT_DCS_CMD(0x7E, 0x39),
> > + _INIT_DCS_CMD(0x7F, 0x34),
> > + _INIT_DCS_CMD(0x80, 0x26),
> > + _INIT_DCS_CMD(0x81, 0x14),
> > + _INIT_DCS_CMD(0x82, 0x02),
> > + _INIT_DCS_CMD(0xE0, 0x02),
> > + _INIT_DCS_CMD(0x00, 0x52),
> > + _INIT_DCS_CMD(0x01, 0x5F),
> > + _INIT_DCS_CMD(0x02, 0x5F),
> > + _INIT_DCS_CMD(0x03, 0x50),
> > + _INIT_DCS_CMD(0x04, 0x77),
> > + _INIT_DCS_CMD(0x05, 0x57),
> > + _INIT_DCS_CMD(0x06, 0x5F),
> > + _INIT_DCS_CMD(0x07, 0x4E),
> > + _INIT_DCS_CMD(0x08, 0x4C),
> > + _INIT_DCS_CMD(0x09, 0x5F),
> > + _INIT_DCS_CMD(0x0A, 0x4A),
> > + _INIT_DCS_CMD(0x0B, 0x48),
> > + _INIT_DCS_CMD(0x0C, 0x5F),
> > + _INIT_DCS_CMD(0x0D, 0x46),
> > + _INIT_DCS_CMD(0x0E, 0x44),
> > + _INIT_DCS_CMD(0x0F, 0x40),
> > + _INIT_DCS_CMD(0x10, 0x5F),
> > + _INIT_DCS_CMD(0x11, 0x5F),
> > + _INIT_DCS_CMD(0x12, 0x5F),
> > + _INIT_DCS_CMD(0x13, 0x5F),
> > + _INIT_DCS_CMD(0x14, 0x5F),
> > + _INIT_DCS_CMD(0x15, 0x5F),
> > + _INIT_DCS_CMD(0x16, 0x53),
> > + _INIT_DCS_CMD(0x17, 0x5F),
> > + _INIT_DCS_CMD(0x18, 0x5F),
> > + _INIT_DCS_CMD(0x19, 0x51),
> > + _INIT_DCS_CMD(0x1A, 0x77),
> > + _INIT_DCS_CMD(0x1B, 0x57),
> > + _INIT_DCS_CMD(0x1C, 0x5F),
> > + _INIT_DCS_CMD(0x1D, 0x4F),
> > + _INIT_DCS_CMD(0x1E, 0x4D),
> > + _INIT_DCS_CMD(0x1F, 0x5F),
> > + _INIT_DCS_CMD(0x20, 0x4B),
> > + _INIT_DCS_CMD(0x21, 0x49),
> > + _INIT_DCS_CMD(0x22, 0x5F),
> > + _INIT_DCS_CMD(0x23, 0x47),
> > + _INIT_DCS_CMD(0x24, 0x45),
> > + _INIT_DCS_CMD(0x25, 0x41),
> > + _INIT_DCS_CMD(0x26, 0x5F),
> > + _INIT_DCS_CMD(0x27, 0x5F),
> > + _INIT_DCS_CMD(0x28, 0x5F),
> > + _INIT_DCS_CMD(0x29, 0x5F),
> > + _INIT_DCS_CMD(0x2A, 0x5F),
> > + _INIT_DCS_CMD(0x2B, 0x5F),
> > + _INIT_DCS_CMD(0x2C, 0x13),
> > + _INIT_DCS_CMD(0x2D, 0x1F),
> > + _INIT_DCS_CMD(0x2E, 0x1F),
> > + _INIT_DCS_CMD(0x2F, 0x01),
> > + _INIT_DCS_CMD(0x30, 0x17),
> > + _INIT_DCS_CMD(0x31, 0x17),
> > + _INIT_DCS_CMD(0x32, 0x1F),
> > + _INIT_DCS_CMD(0x33, 0x0D),
> > + _INIT_DCS_CMD(0x34, 0x0F),
> > + _INIT_DCS_CMD(0x35, 0x1F),
> > + _INIT_DCS_CMD(0x36, 0x05),
> > + _INIT_DCS_CMD(0x37, 0x07),
> > + _INIT_DCS_CMD(0x38, 0x1F),
> > + _INIT_DCS_CMD(0x39, 0x09),
> > + _INIT_DCS_CMD(0x3A, 0x0B),
> > + _INIT_DCS_CMD(0x3B, 0x11),
> > + _INIT_DCS_CMD(0x3C, 0x1F),
> > + _INIT_DCS_CMD(0x3D, 0x1F),
> > + _INIT_DCS_CMD(0x3E, 0x1F),
> > + _INIT_DCS_CMD(0x3F, 0x1F),
> > + _INIT_DCS_CMD(0x40, 0x1F),
> > + _INIT_DCS_CMD(0x41, 0x1F),
> > + _INIT_DCS_CMD(0x42, 0x12),
> > + _INIT_DCS_CMD(0x43, 0x1F),
> > + _INIT_DCS_CMD(0x44, 0x1F),
> > + _INIT_DCS_CMD(0x45, 0x00),
> > + _INIT_DCS_CMD(0x46, 0x17),
> > + _INIT_DCS_CMD(0x47, 0x17),
> > + _INIT_DCS_CMD(0x48, 0x1F),
> > + _INIT_DCS_CMD(0x49, 0x0C),
> > + _INIT_DCS_CMD(0x4A, 0x0E),
> > + _INIT_DCS_CMD(0x4B, 0x1F),
> > + _INIT_DCS_CMD(0x4C, 0x04),
> > + _INIT_DCS_CMD(0x4D, 0x06),
> > + _INIT_DCS_CMD(0x4E, 0x1F),
> > + _INIT_DCS_CMD(0x4F, 0x08),
> > + _INIT_DCS_CMD(0x50, 0x0A),
> > + _INIT_DCS_CMD(0x51, 0x10),
> > + _INIT_DCS_CMD(0x52, 0x1F),
> > + _INIT_DCS_CMD(0x53, 0x1F),
> > + _INIT_DCS_CMD(0x54, 0x1F),
> > + _INIT_DCS_CMD(0x55, 0x1F),
> > + _INIT_DCS_CMD(0x56, 0x1F),
> > + _INIT_DCS_CMD(0x57, 0x1F),
> > + _INIT_DCS_CMD(0x58, 0x40),
> > + _INIT_DCS_CMD(0x5B, 0x10),
> > + _INIT_DCS_CMD(0x5C, 0x06),
> > + _INIT_DCS_CMD(0x5D, 0x40),
> > + _INIT_DCS_CMD(0x5E, 0x00),
> > + _INIT_DCS_CMD(0x5F, 0x00),
> > + _INIT_DCS_CMD(0x60, 0x40),
> > + _INIT_DCS_CMD(0x61, 0x03),
> > + _INIT_DCS_CMD(0x62, 0x04),
> > + _INIT_DCS_CMD(0x63, 0x6C),
> > + _INIT_DCS_CMD(0x64, 0x6C),
> > + _INIT_DCS_CMD(0x65, 0x75),
> > + _INIT_DCS_CMD(0x66, 0x08),
> > + _INIT_DCS_CMD(0x67, 0xB4),
> > + _INIT_DCS_CMD(0x68, 0x08),
> > + _INIT_DCS_CMD(0x69, 0x6C),
> > + _INIT_DCS_CMD(0x6A, 0x6C),
> > + _INIT_DCS_CMD(0x6B, 0x0C),
> > + _INIT_DCS_CMD(0x6D, 0x00),
> > + _INIT_DCS_CMD(0x6E, 0x00),
> > + _INIT_DCS_CMD(0x6F, 0x88),
> > + _INIT_DCS_CMD(0x75, 0xBB),
> > + _INIT_DCS_CMD(0x76, 0x00),
> > + _INIT_DCS_CMD(0x77, 0x05),
> > + _INIT_DCS_CMD(0x78, 0x2A),
> > + _INIT_DCS_CMD(0xE0, 0x04),
> > + _INIT_DCS_CMD(0x00, 0x0E),
> > + _INIT_DCS_CMD(0x02, 0xB3),
> > + _INIT_DCS_CMD(0x09, 0x61),
> > + _INIT_DCS_CMD(0x0E, 0x48),
> > +
> > + _INIT_DCS_CMD(0xE0, 0x00),
> > + _INIT_DCS_CMD(0X11),
> > + /* T6: 120ms */
> > + _INIT_DELAY_CMD(120),
> > + _INIT_DCS_CMD(0X29),
> > + _INIT_DELAY_CMD(20),
> > + {},
> > +};
> > +
> > +static inline struct kingdisplay_panel *to_kingdisplay_panel(struct drm_panel *panel)
> > +{
> > + return container_of(panel, struct kingdisplay_panel, base);
> > +}
> > +
> > +static int kingdisplay_panel_init_dcs_cmd(struct kingdisplay_panel *kingdisplay)
> > +{
> > + struct mipi_dsi_device *dsi = kingdisplay->dsi;
> > + struct drm_panel *panel = &kingdisplay->base;
> > + int i, err = 0;
> > +
> > + if (kingdisplay->desc->init_cmds) {
> > + const struct panel_init_cmd *init_cmds = kingdisplay->desc->init_cmds;
> > +
> > + for (i = 0; init_cmds[i].len != 0; i++) {
> > + const struct panel_init_cmd *cmd = &init_cmds[i];
> > +
> > + switch (cmd->type) {
> > + case DELAY_CMD:
> > + msleep(cmd->data[0]);
> > + err = 0;
> > + break;
> > +
> > + case INIT_DCS_CMD:
> > + err = mipi_dsi_dcs_write(dsi, cmd->data[0],
> > + cmd->len <= 1 ? NULL :
> > + &cmd->data[1],
> > + cmd->len - 1);
> > + break;
> > +
> > + default:
> > + err = -EINVAL;
> > + }
> > +
> > + if (err < 0) {
> > + dev_err(panel->dev,
> > + "failed to write command %u\n", i);
> > + return err;
> > + }
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +static int kingdisplay_panel_enter_sleep_mode(struct kingdisplay_panel *kingdisplay)
> > +{
> > + struct mipi_dsi_device *dsi = kingdisplay->dsi;
> > + int ret;
> > +
> > + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> > +
> > + usleep_range(1000, 2000);
> > + ret = mipi_dsi_dcs_set_display_off(dsi);
> > + if (ret < 0)
> > + return ret;
> > +
> > + msleep(50);
> > +
> > + ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int kingdisplay_panel_disable(struct drm_panel *panel)
> > +{
> > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> > + int ret;
> > +
> > + ret = kingdisplay_panel_enter_sleep_mode(kingdisplay);
> > + if (ret < 0) {
> > + dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + msleep(100);
> > +
> > + return 0;
> > +}
> > +
> > +static int kingdisplay_panel_unprepare(struct drm_panel *panel)
> > +{
> > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> > + int err;
> > +
> > + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0);
> > +
> > + /* T15: 2ms */
> > + usleep_range(1000, 2000);
> > +
> > + err = regulator_disable(kingdisplay->pp3300);
> > + if (err < 0)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > +static int kingdisplay_panel_prepare(struct drm_panel *panel)
> > +{
> > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> > + int ret;
> > +
> > + gpiod_set_value(kingdisplay->enable_gpio, 0);
> > +
> > + ret = regulator_enable(kingdisplay->pp3300);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* T1: 5ms */
> > + usleep_range(5000, 6000);
> > +
> > + if (kingdisplay->desc->lp11_before_reset) {
> > + mipi_dsi_dcs_nop(kingdisplay->dsi);
> > + usleep_range(1000, 2000);
> > + }
> > +
> > + /* T2: 10ms, T1 + T2 > 5ms*/
> > + usleep_range(10000, 11000);
> > +
> > + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 1);
> > +
> > + ret = kingdisplay_panel_init_dcs_cmd(kingdisplay);
> > + if (ret < 0) {
> > + dev_err(panel->dev, "failed to init panel: %d\n", ret);
> > + goto poweroff;
> > + }
> > +
> > + return 0;
> > +
> > +poweroff:
> > + regulator_disable(kingdisplay->pp3300);
> > + /* T6: 2ms */
> > + usleep_range(1000, 2000);
> > + gpiod_set_value(kingdisplay->enable_gpio, 0);
> > +
> > + return ret;
> > +}
> > +
> > +static int kingdisplay_panel_enable(struct drm_panel *panel)
> > +{
> > + msleep(130);
> > + return 0;
> > +}
> > +
> > +static const struct drm_display_mode kingdisplay_kd101ne3_40ti_default_mode = {
> > + .clock = 70595,
> > + .hdisplay = 800,
> > + .hsync_start = 800 + 30,
> > + .hsync_end = 800 + 30 + 30,
> > + .htotal = 800 + 30 + 30 + 30,
> > + .vdisplay = 1280,
> > + .vsync_start = 1280 + 30,
> > + .vsync_end = 1280 + 30 + 4,
> > + .vtotal = 1280 + 30 + 4 + 8,
> > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> > +};
> > +
> > +static const struct panel_desc kingdisplay_kd101ne3_40ti_desc = {
> > + .modes = &kingdisplay_kd101ne3_40ti_default_mode,
> > + .bpc = 8,
> > + .size = {
> > + .width_mm = 135,
> > + .height_mm = 216,
> > + },
> > + .lanes = 4,
> > + .format = MIPI_DSI_FMT_RGB888,
> > + .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> > + MIPI_DSI_MODE_LPM,
> > + .init_cmds = kingdisplay_kd101ne3_init_cmd,
> > + .lp11_before_reset = true,
> > +};
> > +
> > +static int kingdisplay_panel_get_modes(struct drm_panel *panel,
> > + struct drm_connector *connector)
> > +{
> > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> > + const struct drm_display_mode *m = kingdisplay->desc->modes;
> > + struct drm_display_mode *mode;
> > +
> > + mode = drm_mode_duplicate(connector->dev, m);
> > + if (!mode) {
> > + dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> > + m->hdisplay, m->vdisplay, drm_mode_vrefresh(m));
> > + return -ENOMEM;
> > + }
> > +
> > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> > + drm_mode_set_name(mode);
> > + drm_mode_probed_add(connector, mode);
> > +
> > + connector->display_info.width_mm = kingdisplay->desc->size.width_mm;
> > + connector->display_info.height_mm = kingdisplay->desc->size.height_mm;
>
> Please use drm_connector_helper_get_modes_fixed()
>
> > + connector->display_info.bpc = kingdisplay->desc->bpc;
> > + /*
> > + * TODO: Remove once all drm drivers call
> > + * drm_connector_set_orientation_from_panel()
>
> What is your usecase / platform? Are you using drm_bridge_connector? If not, why?
>
> > + */
> > + drm_connector_set_panel_orientation(connector, kingdisplay->orientation);
> > +
> > + return 1;
> > +}
> > +
> > +static enum drm_panel_orientation kingdisplay_panel_get_orientation(struct drm_panel *panel)
> > +{
> > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
> > +
> > + return kingdisplay->orientation;
> > +}
> > +
> > +static const struct drm_panel_funcs kingdisplay_panel_funcs = {
> > + .disable = kingdisplay_panel_disable,
> > + .unprepare = kingdisplay_panel_unprepare,
> > + .prepare = kingdisplay_panel_prepare,
> > + .enable = kingdisplay_panel_enable,
> > + .get_modes = kingdisplay_panel_get_modes,
> > + .get_orientation = kingdisplay_panel_get_orientation,
> > +};
> > +
> > +static int kingdisplay_panel_add(struct kingdisplay_panel *kingdisplay)
> > +{
> > + struct device *dev = &kingdisplay->dsi->dev;
> > + int err;
> > +
> > + kingdisplay->pp3300 = devm_regulator_get(dev, "pp3300");
> > + if (IS_ERR(kingdisplay->pp3300))
> > + return PTR_ERR(kingdisplay->pp3300);
> > +
> > +
> > + kingdisplay->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> > + if (IS_ERR(kingdisplay->enable_gpio)) {
> > + dev_err(dev, "cannot get reset-gpios %ld\n",
> > + PTR_ERR(kingdisplay->enable_gpio));
> > + return PTR_ERR(kingdisplay->enable_gpio);
> > + }
> > +
> > + gpiod_set_value(kingdisplay->enable_gpio, 0);
> > +
> > + drm_panel_init(&kingdisplay->base, dev, &kingdisplay_panel_funcs,
> > + DRM_MODE_CONNECTOR_DSI);
> > + err = of_drm_get_panel_orientation(dev->of_node, &kingdisplay->orientation);
> > + if (err < 0) {
> > + dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, err);
> > + return err;
> > + }
> > +
> > + err = drm_panel_of_backlight(&kingdisplay->base);
> > + if (err)
> > + return err;
> > +
> > + kingdisplay->base.funcs = &kingdisplay_panel_funcs;
> > + kingdisplay->base.dev = &kingdisplay->dsi->dev;
> > +
> > + drm_panel_add(&kingdisplay->base);
> > +
> > + return 0;
> > +}
> > +
> > +static int kingdisplay_panel_probe(struct mipi_dsi_device *dsi)
> > +{
> > + struct kingdisplay_panel *kingdisplay;
> > + int ret;
> > + const struct panel_desc *desc;
> > +
> > + kingdisplay = devm_kzalloc(&dsi->dev, sizeof(*kingdisplay), GFP_KERNEL);
> > + if (!kingdisplay)
> > + return -ENOMEM;
> > +
> > + desc = of_device_get_match_data(&dsi->dev);
> > + dsi->lanes = desc->lanes;
> > + dsi->format = desc->format;
> > + dsi->mode_flags = desc->mode_flags;
> > + kingdisplay->desc = desc;
> > + kingdisplay->dsi = dsi;
> > + ret = kingdisplay_panel_add(kingdisplay);
> > + if (ret < 0)
> > + return ret;
> > +
> > + mipi_dsi_set_drvdata(dsi, kingdisplay);
> > +
> > + ret = mipi_dsi_attach(dsi);
> > + if (ret)
> > + drm_panel_remove(&kingdisplay->base);
> > +
> > + return ret;
> > +}
> > +
> > +static void kingdisplay_panel_shutdown(struct mipi_dsi_device *dsi)
> > +{
> > + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
> > +
> > + drm_panel_disable(&kingdisplay->base);
> > + drm_panel_unprepare(&kingdisplay->base);
> > +}
> > +
> > +static void kingdisplay_panel_remove(struct mipi_dsi_device *dsi)
> > +{
> > + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
> > + int ret;
> > +
> > + kingdisplay_panel_shutdown(dsi);
> > +
> > + ret = mipi_dsi_detach(dsi);
> > + if (ret < 0)
> > + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> > +
> > + if (kingdisplay->base.dev)
> > + drm_panel_remove(&kingdisplay->base);
>
> You are adding it unconditionally, so there should be no condition on
> removal of the panel.
>
> > +}
> > +
> > +static const struct of_device_id kingdisplay_of_match[] = {
> > + { .compatible = "kingdisplay,kd101ne3-40ti",
> > + .data = &kingdisplay_kd101ne3_40ti_desc
> > + },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, kingdisplay_of_match);
> > +
> > +static struct mipi_dsi_driver kingdisplay_panel_driver = {
> > + .driver = {
> > + .name = "panel-kingdisplay-kd101ne3",
> > + .of_match_table = kingdisplay_of_match,
> > + },
> > + .probe = kingdisplay_panel_probe,
> > + .remove = kingdisplay_panel_remove,
> > + .shutdown = kingdisplay_panel_shutdown,
> > +};
> > +module_mipi_dsi_driver(kingdisplay_panel_driver);
> > +
> > +MODULE_AUTHOR("Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>");
> > +MODULE_DESCRIPTION("kingdisplay kd101ne3-40ti 800x1280 video mode panel driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-18 13:11 ` Hsin-Yi Wang
@ 2024-04-18 14:11 ` Dmitry Baryshkov
2024-04-23 18:10 ` Hsin-Yi Wang
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Baryshkov @ 2024-04-18 14:11 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dianders, dri-devel, devicetree,
linux-kernel
On Thu, Apr 18, 2024 at 09:11:37PM +0800, Hsin-Yi Wang wrote:
> On Thu, Apr 18, 2024 at 7:46 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Thu, Apr 18, 2024 at 04:15:48PM +0800, lvzhaoxiong wrote:
> > > The kingdisplay panel is based on JD9365DA controller.
> > > Add a driver for it.
> > >
> > > Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com>
> > > ---
> > > drivers/gpu/drm/panel/Kconfig | 9 +
> > > drivers/gpu/drm/panel/Makefile | 1 +
> > > .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++
> > > 3 files changed, 617 insertions(+)
> > > create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > >
> > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > > index 154f5bf82980..2c73086cf102 100644
> > > --- a/drivers/gpu/drm/panel/Kconfig
> > > +++ b/drivers/gpu/drm/panel/Kconfig
> > > @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04
> > > 24 bit RGB per pixel. It provides a MIPI DSI interface to
> > > the host and has a built-in LED backlight.
> > >
> > > +config DRM_PANEL_KINGDISPLAY_KD101NE3
> > > + tristate "Kingdisplay kd101ne3 panel"
> > > + 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
> > > + Kingdisplay kd101ne3 controller.
> > > +
> > > config DRM_PANEL_LEADTEK_LTK050H3146W
> > > tristate "Leadtek LTK050H3146W panel"
> > > depends on OF
> > > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > > index 24a02655d726..cbd414b98bb0 100644
> > > --- a/drivers/gpu/drm/panel/Makefile
> > > +++ b/drivers/gpu/drm/panel/Makefile
> > > @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o
> > > obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o
> > > obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o
> > > obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
> > > +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o
> > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
> > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
> > > obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
> > > diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > > new file mode 100644
> > > index 000000000000..dbf0992f8b81
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > > @@ -0,0 +1,607 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Panels based on the JD9365DA display controller.
> > > + * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#include <drm/drm_connector.h>
> > > +#include <drm/drm_crtc.h>
> > > +#include <drm/drm_mipi_dsi.h>
> > > +#include <drm/drm_panel.h>
> > > +
> > > +#include <video/mipi_display.h>
> > > +
> > > +struct panel_desc {
> > > + const struct drm_display_mode *modes;
> > > + unsigned int bpc;
> > > +
> > > + /**
> > > + * @width_mm: width of the panel's active display area
> > > + * @height_mm: height of the panel's active display area
> > > + */
> > > + struct {
> > > + unsigned int width_mm;
> > > + unsigned int height_mm;
> >
> > Please move to the declared mode;
> >
> > > + } size;
> > > +
> > > + unsigned long mode_flags;
> > > + enum mipi_dsi_pixel_format format;
> > > + const struct panel_init_cmd *init_cmds;
> > > + unsigned int lanes;
> > > + bool discharge_on_disable;
> > > + bool lp11_before_reset;
> > > +};
> > > +
> > > +struct kingdisplay_panel {
> > > + struct drm_panel base;
> > > + struct mipi_dsi_device *dsi;
> > > +
> > > + const struct panel_desc *desc;
> > > +
> > > + enum drm_panel_orientation orientation;
> > > + struct regulator *pp3300;
> > > + struct gpio_desc *enable_gpio;
> > > +};
> > > +
> > > +enum dsi_cmd_type {
> > > + INIT_DCS_CMD,
> > > + DELAY_CMD,
> > > +};
> > > +
> > > +struct panel_init_cmd {
> > > + enum dsi_cmd_type type;
> > > + size_t len;
> > > + const char *data;
> > > +};
> > > +
> > > +#define _INIT_DCS_CMD(...) { \
> > > + .type = INIT_DCS_CMD, \
> > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > + .data = (char[]){__VA_ARGS__} }
> > > +
> > > +#define _INIT_DELAY_CMD(...) { \
> > > + .type = DELAY_CMD,\
> > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > + .data = (char[]){__VA_ARGS__} }
> >
> > This is the third panel driver using the same appoach. Can you use
> > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > the table, we should extract this framework to a common helper.
> > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> >
> The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> kernel size grows a lot since every sequence will be expanded.
>
> Similar discussion in here:
> https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
>
> This patch would increase the module size from 157K to 572K.
> scripts/bloat-o-meter shows chg +235.95%.
>
> So maybe the common helper is better regarding the kernel module size?
Yes, let's get a framework done in a useful way.
I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
used instead (and it's up to the developer to select correct delay
function).
>
> > > +
> > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > + _INIT_DELAY_CMD(50),
> > > + _INIT_DCS_CMD(0xE0, 0x00),
[skipped the body of the table]
> > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > +
> > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > + _INIT_DCS_CMD(0X11),
Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > + /* T6: 120ms */
> > > + _INIT_DELAY_CMD(120),
> > > + _INIT_DCS_CMD(0X29),
And this is mipi_dsi_dcs_set_display_on().
Having a single table enourages people to put known commands into the
table, the practice that must be frowned upon and forbidden.
We have functions for some of the standard DCS commands. So, maybe
instead of adding a single-table based approach we can improve
mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
error handling to a common part of enable() / prepare() function.
> > > + _INIT_DELAY_CMD(20),
> > > + {},
> > > +};
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support
2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong
@ 2024-04-18 22:59 ` Krzysztof Kozlowski
0 siblings, 0 replies; 19+ messages in thread
From: Krzysztof Kozlowski @ 2024-04-18 22:59 UTC (permalink / raw)
To: lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dianders, hsinyi
Cc: dri-devel, devicetree, linux-kernel
On 18/04/2024 10:15, lvzhaoxiong wrote:
> Create a new dt-scheam for the kd101ne3-40ti.
There is another thread like this, which is confusing. Please version
your patches. patman solves it. b4 as well.
Read the guidelines provided to you by Google.
>
> Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com>
Same comment as for all your other patches: please use full name.
> ---
> .../panel/kingdisplay,kd101ne3-40ti.yaml | 63 +++++++++++++++++++
> 1 file changed, 63 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml b/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml
> new file mode 100644
> index 000000000000..dc79a49eea3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/kingdisplay,kd101ne3-40ti.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: King Display KD035G6-40TI based MIPI-DSI panels
> +
> +description: |
> + -This binding is for display panels using an JD9365DA controller
> +
> +maintainers:
> + - Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>
> +
> +allOf:
> + - $ref: panel-common.yaml#
> +
> +properties:
> + compatible:
> + const: kingdisplay,kd101ne3-40ti
> +
> + backlight: true
> + port: true
Drop both
> + pp3300-supply: true
> + reg: true
> + enable-gpios: true
> + rotation: true
Drop these three. panel-common defines them.
> +
> +required:
> + - compatible
> + - reg
> + - enable-gpios
> + - pp3300-supply
> + - backlight
> + - port
These can stay.
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + dsi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + panel: panel@0 {
> + compatible = "kingdisplay,kd101ne3-40ti";
> + reg = <0>;
> + enable-gpios = <&pio 98 0>;
You included the header, so use it.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-18 14:11 ` Dmitry Baryshkov
@ 2024-04-23 18:10 ` Hsin-Yi Wang
2024-04-23 20:41 ` Doug Anderson
0 siblings, 1 reply; 19+ messages in thread
From: Hsin-Yi Wang @ 2024-04-23 18:10 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dianders, dri-devel, devicetree,
linux-kernel
On Thu, Apr 18, 2024 at 7:11 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, Apr 18, 2024 at 09:11:37PM +0800, Hsin-Yi Wang wrote:
> > On Thu, Apr 18, 2024 at 7:46 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Thu, Apr 18, 2024 at 04:15:48PM +0800, lvzhaoxiong wrote:
> > > > The kingdisplay panel is based on JD9365DA controller.
> > > > Add a driver for it.
> > > >
> > > > Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com>
> > > > ---
> > > > drivers/gpu/drm/panel/Kconfig | 9 +
> > > > drivers/gpu/drm/panel/Makefile | 1 +
> > > > .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++
> > > > 3 files changed, 617 insertions(+)
> > > > create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > > >
> > > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > > > index 154f5bf82980..2c73086cf102 100644
> > > > --- a/drivers/gpu/drm/panel/Kconfig
> > > > +++ b/drivers/gpu/drm/panel/Kconfig
> > > > @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04
> > > > 24 bit RGB per pixel. It provides a MIPI DSI interface to
> > > > the host and has a built-in LED backlight.
> > > >
> > > > +config DRM_PANEL_KINGDISPLAY_KD101NE3
> > > > + tristate "Kingdisplay kd101ne3 panel"
> > > > + 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
> > > > + Kingdisplay kd101ne3 controller.
> > > > +
> > > > config DRM_PANEL_LEADTEK_LTK050H3146W
> > > > tristate "Leadtek LTK050H3146W panel"
> > > > depends on OF
> > > > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > > > index 24a02655d726..cbd414b98bb0 100644
> > > > --- a/drivers/gpu/drm/panel/Makefile
> > > > +++ b/drivers/gpu/drm/panel/Makefile
> > > > @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o
> > > > obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o
> > > > obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o
> > > > obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
> > > > +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o
> > > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
> > > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
> > > > obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
> > > > diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > > > new file mode 100644
> > > > index 000000000000..dbf0992f8b81
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c
> > > > @@ -0,0 +1,607 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Panels based on the JD9365DA display controller.
> > > > + * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>
> > > > + */
> > > > +
> > > > +#include <linux/delay.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/regulator/consumer.h>
> > > > +
> > > > +#include <drm/drm_connector.h>
> > > > +#include <drm/drm_crtc.h>
> > > > +#include <drm/drm_mipi_dsi.h>
> > > > +#include <drm/drm_panel.h>
> > > > +
> > > > +#include <video/mipi_display.h>
> > > > +
> > > > +struct panel_desc {
> > > > + const struct drm_display_mode *modes;
> > > > + unsigned int bpc;
> > > > +
> > > > + /**
> > > > + * @width_mm: width of the panel's active display area
> > > > + * @height_mm: height of the panel's active display area
> > > > + */
> > > > + struct {
> > > > + unsigned int width_mm;
> > > > + unsigned int height_mm;
> > >
> > > Please move to the declared mode;
> > >
> > > > + } size;
> > > > +
> > > > + unsigned long mode_flags;
> > > > + enum mipi_dsi_pixel_format format;
> > > > + const struct panel_init_cmd *init_cmds;
> > > > + unsigned int lanes;
> > > > + bool discharge_on_disable;
> > > > + bool lp11_before_reset;
> > > > +};
> > > > +
> > > > +struct kingdisplay_panel {
> > > > + struct drm_panel base;
> > > > + struct mipi_dsi_device *dsi;
> > > > +
> > > > + const struct panel_desc *desc;
> > > > +
> > > > + enum drm_panel_orientation orientation;
> > > > + struct regulator *pp3300;
> > > > + struct gpio_desc *enable_gpio;
> > > > +};
> > > > +
> > > > +enum dsi_cmd_type {
> > > > + INIT_DCS_CMD,
> > > > + DELAY_CMD,
> > > > +};
> > > > +
> > > > +struct panel_init_cmd {
> > > > + enum dsi_cmd_type type;
> > > > + size_t len;
> > > > + const char *data;
> > > > +};
> > > > +
> > > > +#define _INIT_DCS_CMD(...) { \
> > > > + .type = INIT_DCS_CMD, \
> > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > + .data = (char[]){__VA_ARGS__} }
> > > > +
> > > > +#define _INIT_DELAY_CMD(...) { \
> > > > + .type = DELAY_CMD,\
> > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > + .data = (char[]){__VA_ARGS__} }
> > >
> > > This is the third panel driver using the same appoach. Can you use
> > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > the table, we should extract this framework to a common helper.
> > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > >
> > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > kernel size grows a lot since every sequence will be expanded.
> >
> > Similar discussion in here:
> > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> >
> > This patch would increase the module size from 157K to 572K.
> > scripts/bloat-o-meter shows chg +235.95%.
> >
> > So maybe the common helper is better regarding the kernel module size?
>
> Yes, let's get a framework done in a useful way.
> I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> used instead (and it's up to the developer to select correct delay
> function).
>
> >
> > > > +
> > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > + _INIT_DELAY_CMD(50),
> > > > + _INIT_DCS_CMD(0xE0, 0x00),
>
> [skipped the body of the table]
>
> > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > +
> > > > + _INIT_DCS_CMD(0xE0, 0x00),
>
> > > > + _INIT_DCS_CMD(0X11),
>
> Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
>
> > > > + /* T6: 120ms */
> > > > + _INIT_DELAY_CMD(120),
> > > > + _INIT_DCS_CMD(0X29),
>
> And this is mipi_dsi_dcs_set_display_on().
>
> Having a single table enourages people to put known commands into the
> table, the practice that must be frowned upon and forbidden.
>
> We have functions for some of the standard DCS commands. So, maybe
> instead of adding a single-table based approach we can improve
> mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> error handling to a common part of enable() / prepare() function.
>
For this panel, I think it can also refer to how
panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
not what operation to use, and use mipi_dsi_generic_write_seq() when
looping through the table.
> > > > + _INIT_DELAY_CMD(20),
> > > > + {},
> > > > +};
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-23 18:10 ` Hsin-Yi Wang
@ 2024-04-23 20:41 ` Doug Anderson
2024-04-23 21:20 ` Dmitry Baryshkov
0 siblings, 1 reply; 19+ messages in thread
From: Doug Anderson @ 2024-04-23 20:41 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: Dmitry Baryshkov, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
Hi,
On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
>
> > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > + .type = INIT_DCS_CMD, \
> > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > +
> > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > + .type = DELAY_CMD,\
> > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > + .data = (char[]){__VA_ARGS__} }
> > > >
> > > > This is the third panel driver using the same appoach. Can you use
> > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > the table, we should extract this framework to a common helper.
> > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > >
> > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > kernel size grows a lot since every sequence will be expanded.
> > >
> > > Similar discussion in here:
> > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > >
> > > This patch would increase the module size from 157K to 572K.
> > > scripts/bloat-o-meter shows chg +235.95%.
> > >
> > > So maybe the common helper is better regarding the kernel module size?
> >
> > Yes, let's get a framework done in a useful way.
> > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > used instead (and it's up to the developer to select correct delay
> > function).
> >
> > >
> > > > > +
> > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > + _INIT_DELAY_CMD(50),
> > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> >
> > [skipped the body of the table]
> >
> > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > +
> > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> >
> > > > > + _INIT_DCS_CMD(0X11),
> >
> > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> >
> > > > > + /* T6: 120ms */
> > > > > + _INIT_DELAY_CMD(120),
> > > > > + _INIT_DCS_CMD(0X29),
> >
> > And this is mipi_dsi_dcs_set_display_on().
> >
> > Having a single table enourages people to put known commands into the
> > table, the practice that must be frowned upon and forbidden.
> >
> > We have functions for some of the standard DCS commands. So, maybe
> > instead of adding a single-table based approach we can improve
> > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > error handling to a common part of enable() / prepare() function.
> >
>
> For this panel, I think it can also refer to how
> panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> not what operation to use, and use mipi_dsi_generic_write_seq() when
> looping through the table.
Even more similar discussion:
https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-23 20:41 ` Doug Anderson
@ 2024-04-23 21:20 ` Dmitry Baryshkov
2024-04-24 21:04 ` Doug Anderson
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Baryshkov @ 2024-04-23 21:20 UTC (permalink / raw)
To: Doug Anderson
Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> >
> > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > + .type = INIT_DCS_CMD, \
> > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > +
> > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > + .type = DELAY_CMD,\
> > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > >
> > > > > This is the third panel driver using the same appoach. Can you use
> > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > the table, we should extract this framework to a common helper.
> > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > >
> > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > kernel size grows a lot since every sequence will be expanded.
> > > >
> > > > Similar discussion in here:
> > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > >
> > > > This patch would increase the module size from 157K to 572K.
> > > > scripts/bloat-o-meter shows chg +235.95%.
> > > >
> > > > So maybe the common helper is better regarding the kernel module size?
> > >
> > > Yes, let's get a framework done in a useful way.
> > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > used instead (and it's up to the developer to select correct delay
> > > function).
> > >
> > > >
> > > > > > +
> > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > + _INIT_DELAY_CMD(50),
> > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > >
> > > [skipped the body of the table]
> > >
> > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > +
> > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > >
> > > > > > + _INIT_DCS_CMD(0X11),
> > >
> > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > >
> > > > > > + /* T6: 120ms */
> > > > > > + _INIT_DELAY_CMD(120),
> > > > > > + _INIT_DCS_CMD(0X29),
> > >
> > > And this is mipi_dsi_dcs_set_display_on().
> > >
> > > Having a single table enourages people to put known commands into the
> > > table, the practice that must be frowned upon and forbidden.
> > >
> > > We have functions for some of the standard DCS commands. So, maybe
> > > instead of adding a single-table based approach we can improve
> > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > error handling to a common part of enable() / prepare() function.
> > >
> >
> > For this panel, I think it can also refer to how
> > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > looping through the table.
>
> Even more similar discussion:
>
> https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
It seems I skipped that thread.
I'd still suggest a code-based solution compared to table-based one, for
the reasons I've outlined before. Having a tables puts a pressure on the
developer to put commands there for which we already have a
command-specific function.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-23 21:20 ` Dmitry Baryshkov
@ 2024-04-24 21:04 ` Doug Anderson
2024-04-24 21:18 ` Hsin-Yi Wang
2024-04-24 21:49 ` Dmitry Baryshkov
0 siblings, 2 replies; 19+ messages in thread
From: Doug Anderson @ 2024-04-24 21:04 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
Hi,
On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> > >
> > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > +
> > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > + .type = DELAY_CMD,\
> > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > >
> > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > > the table, we should extract this framework to a common helper.
> > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > >
> > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > kernel size grows a lot since every sequence will be expanded.
> > > > >
> > > > > Similar discussion in here:
> > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > > >
> > > > > This patch would increase the module size from 157K to 572K.
> > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > >
> > > > > So maybe the common helper is better regarding the kernel module size?
> > > >
> > > > Yes, let's get a framework done in a useful way.
> > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > used instead (and it's up to the developer to select correct delay
> > > > function).
> > > >
> > > > >
> > > > > > > +
> > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > >
> > > > [skipped the body of the table]
> > > >
> > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > +
> > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > >
> > > > > > > + _INIT_DCS_CMD(0X11),
> > > >
> > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > >
> > > > > > > + /* T6: 120ms */
> > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > + _INIT_DCS_CMD(0X29),
> > > >
> > > > And this is mipi_dsi_dcs_set_display_on().
> > > >
> > > > Having a single table enourages people to put known commands into the
> > > > table, the practice that must be frowned upon and forbidden.
> > > >
> > > > We have functions for some of the standard DCS commands. So, maybe
> > > > instead of adding a single-table based approach we can improve
> > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > error handling to a common part of enable() / prepare() function.
> > > >
> > >
> > > For this panel, I think it can also refer to how
> > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > looping through the table.
> >
> > Even more similar discussion:
> >
> > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
>
> It seems I skipped that thread.
>
> I'd still suggest a code-based solution compared to table-based one, for
> the reasons I've outlined before. Having a tables puts a pressure on the
> developer to put commands there for which we already have a
> command-specific function.
The problem is that with these panels that need big init sequences the
code based solution is _a lot_ bigger. If it were a few bytes or a
1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
from a table to code it was 100K difference in code [1]. I would also
say that having these long init sequences done as separate commands
encourages people to skip checking the return values of each of the
transfer functions and I don't love that idea.
It would be ideal if these panels didn't need these long init
sequences, but I don't have any inside knowledge here saying that they
could be removed. So assume we can't get rid of the init sequences it
feels like we have to find some way to make the tables work for at
least the large chunks of init code and encourage people to make the
tables readable...
[1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-24 21:04 ` Doug Anderson
@ 2024-04-24 21:18 ` Hsin-Yi Wang
2024-04-24 21:49 ` Dmitry Baryshkov
1 sibling, 0 replies; 19+ messages in thread
From: Hsin-Yi Wang @ 2024-04-24 21:18 UTC (permalink / raw)
To: Doug Anderson
Cc: Dmitry Baryshkov, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
On Wed, Apr 24, 2024 at 2:05 PM Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> > > >
> > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > +
> > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > >
> > > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > > > the table, we should extract this framework to a common helper.
> > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > > >
> > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > > kernel size grows a lot since every sequence will be expanded.
> > > > > >
> > > > > > Similar discussion in here:
> > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > > > >
> > > > > > This patch would increase the module size from 157K to 572K.
> > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > >
> > > > > > So maybe the common helper is better regarding the kernel module size?
> > > > >
> > > > > Yes, let's get a framework done in a useful way.
> > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > > used instead (and it's up to the developer to select correct delay
> > > > > function).
> > > > >
> > > > > >
> > > > > > > > +
> > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > >
> > > > > [skipped the body of the table]
> > > > >
> > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > > +
> > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > >
> > > > > > > > + _INIT_DCS_CMD(0X11),
> > > > >
> > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > > >
> > > > > > > > + /* T6: 120ms */
> > > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > > + _INIT_DCS_CMD(0X29),
> > > > >
> > > > > And this is mipi_dsi_dcs_set_display_on().
> > > > >
> > > > > Having a single table enourages people to put known commands into the
> > > > > table, the practice that must be frowned upon and forbidden.
> > > > >
> > > > > We have functions for some of the standard DCS commands. So, maybe
> > > > > instead of adding a single-table based approach we can improve
> > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > > error handling to a common part of enable() / prepare() function.
> > > > >
> > > >
> > > > For this panel, I think it can also refer to how
> > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > > looping through the table.
> > >
> > > Even more similar discussion:
> > >
> > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
> >
> > It seems I skipped that thread.
> >
> > I'd still suggest a code-based solution compared to table-based one, for
> > the reasons I've outlined before. Having a tables puts a pressure on the
> > developer to put commands there for which we already have a
> > command-specific function.
>
> The problem is that with these panels that need big init sequences the
> code based solution is _a lot_ bigger. If it were a few bytes or a
> 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> from a table to code it was 100K difference in code [1]. I would also
> say that having these long init sequences done as separate commands
> encourages people to skip checking the return values of each of the
> transfer functions and I don't love that idea.
>
> It would be ideal if these panels didn't need these long init
> sequences, but I don't have any inside knowledge here saying that they
> could be removed. So assume we can't get rid of the init sequences it
> feels like we have to find some way to make the tables work for at
> least the large chunks of init code and encourage people to make the
> tables readable...
>
For the init sequence of the panel from this patch, using the table
approach, we can still use mipi_dsi_generic_write_seq() and not invent
new macro or make the code complicated.
>
> [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-24 21:04 ` Doug Anderson
2024-04-24 21:18 ` Hsin-Yi Wang
@ 2024-04-24 21:49 ` Dmitry Baryshkov
2024-04-24 22:15 ` Hsin-Yi Wang
2024-04-24 22:27 ` Doug Anderson
1 sibling, 2 replies; 19+ messages in thread
From: Dmitry Baryshkov @ 2024-04-24 21:49 UTC (permalink / raw)
To: Doug Anderson
Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> > > >
> > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > +
> > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > >
> > > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > > > the table, we should extract this framework to a common helper.
> > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > > >
> > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > > kernel size grows a lot since every sequence will be expanded.
> > > > > >
> > > > > > Similar discussion in here:
> > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > > > >
> > > > > > This patch would increase the module size from 157K to 572K.
> > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > >
> > > > > > So maybe the common helper is better regarding the kernel module size?
> > > > >
> > > > > Yes, let's get a framework done in a useful way.
> > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > > used instead (and it's up to the developer to select correct delay
> > > > > function).
> > > > >
> > > > > >
> > > > > > > > +
> > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > >
> > > > > [skipped the body of the table]
> > > > >
> > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > > +
> > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > >
> > > > > > > > + _INIT_DCS_CMD(0X11),
> > > > >
> > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > > >
> > > > > > > > + /* T6: 120ms */
> > > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > > + _INIT_DCS_CMD(0X29),
> > > > >
> > > > > And this is mipi_dsi_dcs_set_display_on().
> > > > >
> > > > > Having a single table enourages people to put known commands into the
> > > > > table, the practice that must be frowned upon and forbidden.
> > > > >
> > > > > We have functions for some of the standard DCS commands. So, maybe
> > > > > instead of adding a single-table based approach we can improve
> > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > > error handling to a common part of enable() / prepare() function.
> > > > >
> > > >
> > > > For this panel, I think it can also refer to how
> > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > > looping through the table.
> > >
> > > Even more similar discussion:
> > >
> > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
> >
> > It seems I skipped that thread.
> >
> > I'd still suggest a code-based solution compared to table-based one, for
> > the reasons I've outlined before. Having a tables puts a pressure on the
> > developer to put commands there for which we already have a
> > command-specific function.
>
> The problem is that with these panels that need big init sequences the
> code based solution is _a lot_ bigger. If it were a few bytes or a
> 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> from a table to code it was 100K difference in code [1]. I would also
> say that having these long init sequences done as separate commands
> encourages people to skip checking the return values of each of the
> transfer functions and I don't love that idea.
>
> It would be ideal if these panels didn't need these long init
> sequences, but I don't have any inside knowledge here saying that they
> could be removed. So assume we can't get rid of the init sequences it
> feels like we have to find some way to make the tables work for at
> least the large chunks of init code and encourage people to make the
> tables readable...
I did a quick check on the boe-tv101wum-nl6 driver by converting the
writes to use the following macro:
#define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...)
\
do { \
static const u8 d[] = { cmd, seq }; \
ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
if (ret < 0) \
goto err; \
} while (0)
And then at the end of the init funciton having
err:
dev_err(panel->dev,
"failed to write command %d\n", ret);
return ret;
}
Size comparison:
text data bss dec hex filename
before
34109 10410 18 44537 adf9
../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
making init data const
44359 184 0 44543 adff
../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
with new macros
44353 184 0 44537 adf9
../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
As you can see, there is literally no size difference with this macro in place.
The only drawback is that the init stops on the first write rather
than going through the sequence.
WDYT? I can turn this into a proper patch if you think this makes sense.
>
>
> [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-24 21:49 ` Dmitry Baryshkov
@ 2024-04-24 22:15 ` Hsin-Yi Wang
2024-04-24 22:50 ` Dmitry Baryshkov
2024-04-24 22:27 ` Doug Anderson
1 sibling, 1 reply; 19+ messages in thread
From: Hsin-Yi Wang @ 2024-04-24 22:15 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Doug Anderson, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote:
> >
> > Hi,
> >
> > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> > > > >
> > > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > +
> > > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > >
> > > > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > > > > the table, we should extract this framework to a common helper.
> > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > > > >
> > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > > > kernel size grows a lot since every sequence will be expanded.
> > > > > > >
> > > > > > > Similar discussion in here:
> > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > > > > >
> > > > > > > This patch would increase the module size from 157K to 572K.
> > > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > > >
> > > > > > > So maybe the common helper is better regarding the kernel module size?
> > > > > >
> > > > > > Yes, let's get a framework done in a useful way.
> > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > > > used instead (and it's up to the developer to select correct delay
> > > > > > function).
> > > > > >
> > > > > > >
> > > > > > > > > +
> > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > >
> > > > > > [skipped the body of the table]
> > > > > >
> > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > > > +
> > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > >
> > > > > > > > > + _INIT_DCS_CMD(0X11),
> > > > > >
> > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > > > >
> > > > > > > > > + /* T6: 120ms */
> > > > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > > > + _INIT_DCS_CMD(0X29),
> > > > > >
> > > > > > And this is mipi_dsi_dcs_set_display_on().
> > > > > >
> > > > > > Having a single table enourages people to put known commands into the
> > > > > > table, the practice that must be frowned upon and forbidden.
> > > > > >
> > > > > > We have functions for some of the standard DCS commands. So, maybe
> > > > > > instead of adding a single-table based approach we can improve
> > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > > > error handling to a common part of enable() / prepare() function.
> > > > > >
> > > > >
> > > > > For this panel, I think it can also refer to how
> > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > > > looping through the table.
> > > >
> > > > Even more similar discussion:
> > > >
> > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
> > >
> > > It seems I skipped that thread.
> > >
> > > I'd still suggest a code-based solution compared to table-based one, for
> > > the reasons I've outlined before. Having a tables puts a pressure on the
> > > developer to put commands there for which we already have a
> > > command-specific function.
> >
> > The problem is that with these panels that need big init sequences the
> > code based solution is _a lot_ bigger. If it were a few bytes or a
> > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> > from a table to code it was 100K difference in code [1]. I would also
> > say that having these long init sequences done as separate commands
> > encourages people to skip checking the return values of each of the
> > transfer functions and I don't love that idea.
> >
> > It would be ideal if these panels didn't need these long init
> > sequences, but I don't have any inside knowledge here saying that they
> > could be removed. So assume we can't get rid of the init sequences it
> > feels like we have to find some way to make the tables work for at
> > least the large chunks of init code and encourage people to make the
> > tables readable...
>
>
> I did a quick check on the boe-tv101wum-nl6 driver by converting the
> writes to use the following macro:
>
> #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...)
> \
> do { \
> static const u8 d[] = { cmd, seq }; \
> ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> if (ret < 0) \
> goto err; \
> } while (0)
>
> And then at the end of the init funciton having
>
> err:
> dev_err(panel->dev,
> "failed to write command %d\n", ret);
> return ret;
> }
>
I'm not sure about the coding style rule here, would it be considered
unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err
block, but the block may not be directly used in that caller and is
only jumped from the macro?
> Size comparison:
> text data bss dec hex filename
> before
> 34109 10410 18 44537 adf9
> ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
> making init data const
> 44359 184 0 44543 adff
> ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
> with new macros
> 44353 184 0 44537 adf9
> ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
>
> As you can see, there is literally no size difference with this macro in place.
> The only drawback is that the init stops on the first write rather
> than going through the sequence.
>
> WDYT? I can turn this into a proper patch if you think this makes sense.
>
> >
> >
> > [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com
>
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-24 21:49 ` Dmitry Baryshkov
2024-04-24 22:15 ` Hsin-Yi Wang
@ 2024-04-24 22:27 ` Doug Anderson
1 sibling, 0 replies; 19+ messages in thread
From: Doug Anderson @ 2024-04-24 22:27 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
Hi,
On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> > The problem is that with these panels that need big init sequences the
> > code based solution is _a lot_ bigger. If it were a few bytes or a
> > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> > from a table to code it was 100K difference in code [1]. I would also
> > say that having these long init sequences done as separate commands
> > encourages people to skip checking the return values of each of the
> > transfer functions and I don't love that idea.
> >
> > It would be ideal if these panels didn't need these long init
> > sequences, but I don't have any inside knowledge here saying that they
> > could be removed. So assume we can't get rid of the init sequences it
> > feels like we have to find some way to make the tables work for at
> > least the large chunks of init code and encourage people to make the
> > tables readable...
>
>
> I did a quick check on the boe-tv101wum-nl6 driver by converting the
> writes to use the following macro:
>
> #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...)
> \
> do { \
> static const u8 d[] = { cmd, seq }; \
> ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> if (ret < 0) \
> goto err; \
> } while (0)
>
> And then at the end of the init funciton having
>
> err:
> dev_err(panel->dev,
> "failed to write command %d\n", ret);
> return ret;
> }
>
> Size comparison:
> text data bss dec hex filename
> before
> 34109 10410 18 44537 adf9
> ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
> making init data const
> 44359 184 0 44543 adff
> ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
> with new macros
> 44353 184 0 44537 adf9
> ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
>
> As you can see, there is literally no size difference with this macro in place.
> The only drawback is that the init stops on the first write rather
> than going through the sequence.
>
> WDYT? I can turn this into a proper patch if you think this makes sense.
Ah, so what you're saying is that the big bloat from using the
existing mipi_dsi_dcs_write_seq() is the error printing. That makes
sense. ...and by relying on the caller to provide an error handling
label we can get rid of the overhead and still get the error prints.
Yes, that seems pretty reasonable to me. I guess I'd perhaps make the
error label a parameter to the macro (so it's obvious that the caller
needs to define it) and maybe name it in such a way to make it obvious
the difference between this macro and mipi_dsi_dcs_write_seq().
With that and your measurements then this seems perfectly reasonable
to me and I'm good with fully moving away from the table-based
approach. I'd be happy if you sent a patch for it and happy to review
it.
-Doug
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-24 22:15 ` Hsin-Yi Wang
@ 2024-04-24 22:50 ` Dmitry Baryshkov
2024-04-24 23:25 ` Doug Anderson
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Baryshkov @ 2024-04-24 22:50 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: Doug Anderson, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang <hsinyi@google.com> wrote:
>
> On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> > > > > >
> > > > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > > +
> > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > >
> > > > > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > > > > > the table, we should extract this framework to a common helper.
> > > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > > > > >
> > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > > > > kernel size grows a lot since every sequence will be expanded.
> > > > > > > >
> > > > > > > > Similar discussion in here:
> > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > > > > > >
> > > > > > > > This patch would increase the module size from 157K to 572K.
> > > > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > > > >
> > > > > > > > So maybe the common helper is better regarding the kernel module size?
> > > > > > >
> > > > > > > Yes, let's get a framework done in a useful way.
> > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > > > > used instead (and it's up to the developer to select correct delay
> > > > > > > function).
> > > > > > >
> > > > > > > >
> > > > > > > > > > +
> > > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > >
> > > > > > > [skipped the body of the table]
> > > > > > >
> > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > > > > +
> > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > >
> > > > > > > > > > + _INIT_DCS_CMD(0X11),
> > > > > > >
> > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > > > > >
> > > > > > > > > > + /* T6: 120ms */
> > > > > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > > > > + _INIT_DCS_CMD(0X29),
> > > > > > >
> > > > > > > And this is mipi_dsi_dcs_set_display_on().
> > > > > > >
> > > > > > > Having a single table enourages people to put known commands into the
> > > > > > > table, the practice that must be frowned upon and forbidden.
> > > > > > >
> > > > > > > We have functions for some of the standard DCS commands. So, maybe
> > > > > > > instead of adding a single-table based approach we can improve
> > > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > > > > error handling to a common part of enable() / prepare() function.
> > > > > > >
> > > > > >
> > > > > > For this panel, I think it can also refer to how
> > > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > > > > looping through the table.
> > > > >
> > > > > Even more similar discussion:
> > > > >
> > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
> > > >
> > > > It seems I skipped that thread.
> > > >
> > > > I'd still suggest a code-based solution compared to table-based one, for
> > > > the reasons I've outlined before. Having a tables puts a pressure on the
> > > > developer to put commands there for which we already have a
> > > > command-specific function.
> > >
> > > The problem is that with these panels that need big init sequences the
> > > code based solution is _a lot_ bigger. If it were a few bytes or a
> > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> > > from a table to code it was 100K difference in code [1]. I would also
> > > say that having these long init sequences done as separate commands
> > > encourages people to skip checking the return values of each of the
> > > transfer functions and I don't love that idea.
> > >
> > > It would be ideal if these panels didn't need these long init
> > > sequences, but I don't have any inside knowledge here saying that they
> > > could be removed. So assume we can't get rid of the init sequences it
> > > feels like we have to find some way to make the tables work for at
> > > least the large chunks of init code and encourage people to make the
> > > tables readable...
> >
> >
> > I did a quick check on the boe-tv101wum-nl6 driver by converting the
> > writes to use the following macro:
> >
> > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...)
> > \
> > do { \
> > static const u8 d[] = { cmd, seq }; \
> > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> > if (ret < 0) \
> > goto err; \
> > } while (0)
> >
> > And then at the end of the init funciton having
> >
> > err:
> > dev_err(panel->dev,
> > "failed to write command %d\n", ret);
> > return ret;
> > }
> >
>
> I'm not sure about the coding style rule here, would it be considered
> unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err
> block, but the block may not be directly used in that caller and is
> only jumped from the macro?
I'm also not sure here. It was a quick and dirty test.
We might as well do something like
ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...);
if (ret)
goto err;
all over the place.
>
>
> > Size comparison:
> > text data bss dec hex filename
> > before
> > 34109 10410 18 44537 adf9
> > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
> > making init data const
> > 44359 184 0 44543 adff
> > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
> > with new macros
> > 44353 184 0 44537 adf9
> > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o
> >
> > As you can see, there is literally no size difference with this macro in place.
> > The only drawback is that the init stops on the first write rather
> > than going through the sequence.
> >
> > WDYT? I can turn this into a proper patch if you think this makes sense.
> >
> > >
> > >
> > > [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com
> >
> >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-24 22:50 ` Dmitry Baryshkov
@ 2024-04-24 23:25 ` Doug Anderson
2024-04-24 23:37 ` Dmitry Baryshkov
0 siblings, 1 reply; 19+ messages in thread
From: Doug Anderson @ 2024-04-24 23:25 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
Hi,
On Wed, Apr 24, 2024 at 3:51 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang <hsinyi@google.com> wrote:
> >
> > On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> > > > > > >
> > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > > > +
> > > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > >
> > > > > > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > > > > > > the table, we should extract this framework to a common helper.
> > > > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > > > > > >
> > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > > > > > kernel size grows a lot since every sequence will be expanded.
> > > > > > > > >
> > > > > > > > > Similar discussion in here:
> > > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > > > > > > >
> > > > > > > > > This patch would increase the module size from 157K to 572K.
> > > > > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > > > > >
> > > > > > > > > So maybe the common helper is better regarding the kernel module size?
> > > > > > > >
> > > > > > > > Yes, let's get a framework done in a useful way.
> > > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > > > > > used instead (and it's up to the developer to select correct delay
> > > > > > > > function).
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > > >
> > > > > > > > [skipped the body of the table]
> > > > > > > >
> > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > > > > > +
> > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > > >
> > > > > > > > > > > + _INIT_DCS_CMD(0X11),
> > > > > > > >
> > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > > > > > >
> > > > > > > > > > > + /* T6: 120ms */
> > > > > > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > > > > > + _INIT_DCS_CMD(0X29),
> > > > > > > >
> > > > > > > > And this is mipi_dsi_dcs_set_display_on().
> > > > > > > >
> > > > > > > > Having a single table enourages people to put known commands into the
> > > > > > > > table, the practice that must be frowned upon and forbidden.
> > > > > > > >
> > > > > > > > We have functions for some of the standard DCS commands. So, maybe
> > > > > > > > instead of adding a single-table based approach we can improve
> > > > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > > > > > error handling to a common part of enable() / prepare() function.
> > > > > > > >
> > > > > > >
> > > > > > > For this panel, I think it can also refer to how
> > > > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > > > > > looping through the table.
> > > > > >
> > > > > > Even more similar discussion:
> > > > > >
> > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
> > > > >
> > > > > It seems I skipped that thread.
> > > > >
> > > > > I'd still suggest a code-based solution compared to table-based one, for
> > > > > the reasons I've outlined before. Having a tables puts a pressure on the
> > > > > developer to put commands there for which we already have a
> > > > > command-specific function.
> > > >
> > > > The problem is that with these panels that need big init sequences the
> > > > code based solution is _a lot_ bigger. If it were a few bytes or a
> > > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> > > > from a table to code it was 100K difference in code [1]. I would also
> > > > say that having these long init sequences done as separate commands
> > > > encourages people to skip checking the return values of each of the
> > > > transfer functions and I don't love that idea.
> > > >
> > > > It would be ideal if these panels didn't need these long init
> > > > sequences, but I don't have any inside knowledge here saying that they
> > > > could be removed. So assume we can't get rid of the init sequences it
> > > > feels like we have to find some way to make the tables work for at
> > > > least the large chunks of init code and encourage people to make the
> > > > tables readable...
> > >
> > >
> > > I did a quick check on the boe-tv101wum-nl6 driver by converting the
> > > writes to use the following macro:
> > >
> > > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...)
> > > \
> > > do { \
> > > static const u8 d[] = { cmd, seq }; \
> > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> > > if (ret < 0) \
> > > goto err; \
> > > } while (0)
> > >
> > > And then at the end of the init funciton having
> > >
> > > err:
> > > dev_err(panel->dev,
> > > "failed to write command %d\n", ret);
> > > return ret;
> > > }
> > >
> >
> > I'm not sure about the coding style rule here, would it be considered
> > unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err
> > block, but the block may not be directly used in that caller and is
> > only jumped from the macro?
>
> I'm also not sure here. It was a quick and dirty test.
> We might as well do something like
>
> ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...);
> if (ret)
> goto err;
>
> all over the place.
Oh. This is actually very simple to fix and requires no code changes
to clients. :-P We just need to hoist the printing out of the macro
and into "drm_mipi_dsi.c". Let me double-confirm and then I'll post a
patch.
-Doug
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-24 23:25 ` Doug Anderson
@ 2024-04-24 23:37 ` Dmitry Baryshkov
2024-04-25 0:21 ` Doug Anderson
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Baryshkov @ 2024-04-24 23:37 UTC (permalink / raw)
To: Doug Anderson
Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
On Thu, 25 Apr 2024 at 02:25, Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Wed, Apr 24, 2024 at 3:51 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang <hsinyi@google.com> wrote:
> > >
> > > On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> > > > > > > >
> > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > > > > +
> > > > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > > >
> > > > > > > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > > > > > > > the table, we should extract this framework to a common helper.
> > > > > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > > > > > > >
> > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > > > > > > kernel size grows a lot since every sequence will be expanded.
> > > > > > > > > >
> > > > > > > > > > Similar discussion in here:
> > > > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > > > > > > > >
> > > > > > > > > > This patch would increase the module size from 157K to 572K.
> > > > > > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > > > > > >
> > > > > > > > > > So maybe the common helper is better regarding the kernel module size?
> > > > > > > > >
> > > > > > > > > Yes, let's get a framework done in a useful way.
> > > > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > > > > > > used instead (and it's up to the developer to select correct delay
> > > > > > > > > function).
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > > > >
> > > > > > > > > [skipped the body of the table]
> > > > > > > > >
> > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > > > > > > +
> > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > > > >
> > > > > > > > > > > > + _INIT_DCS_CMD(0X11),
> > > > > > > > >
> > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > > > > > > >
> > > > > > > > > > > > + /* T6: 120ms */
> > > > > > > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > > > > > > + _INIT_DCS_CMD(0X29),
> > > > > > > > >
> > > > > > > > > And this is mipi_dsi_dcs_set_display_on().
> > > > > > > > >
> > > > > > > > > Having a single table enourages people to put known commands into the
> > > > > > > > > table, the practice that must be frowned upon and forbidden.
> > > > > > > > >
> > > > > > > > > We have functions for some of the standard DCS commands. So, maybe
> > > > > > > > > instead of adding a single-table based approach we can improve
> > > > > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > > > > > > error handling to a common part of enable() / prepare() function.
> > > > > > > > >
> > > > > > > >
> > > > > > > > For this panel, I think it can also refer to how
> > > > > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > > > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > > > > > > looping through the table.
> > > > > > >
> > > > > > > Even more similar discussion:
> > > > > > >
> > > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
> > > > > >
> > > > > > It seems I skipped that thread.
> > > > > >
> > > > > > I'd still suggest a code-based solution compared to table-based one, for
> > > > > > the reasons I've outlined before. Having a tables puts a pressure on the
> > > > > > developer to put commands there for which we already have a
> > > > > > command-specific function.
> > > > >
> > > > > The problem is that with these panels that need big init sequences the
> > > > > code based solution is _a lot_ bigger. If it were a few bytes or a
> > > > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> > > > > from a table to code it was 100K difference in code [1]. I would also
> > > > > say that having these long init sequences done as separate commands
> > > > > encourages people to skip checking the return values of each of the
> > > > > transfer functions and I don't love that idea.
> > > > >
> > > > > It would be ideal if these panels didn't need these long init
> > > > > sequences, but I don't have any inside knowledge here saying that they
> > > > > could be removed. So assume we can't get rid of the init sequences it
> > > > > feels like we have to find some way to make the tables work for at
> > > > > least the large chunks of init code and encourage people to make the
> > > > > tables readable...
> > > >
> > > >
> > > > I did a quick check on the boe-tv101wum-nl6 driver by converting the
> > > > writes to use the following macro:
> > > >
> > > > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...)
> > > > \
> > > > do { \
> > > > static const u8 d[] = { cmd, seq }; \
> > > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> > > > if (ret < 0) \
> > > > goto err; \
> > > > } while (0)
> > > >
> > > > And then at the end of the init funciton having
> > > >
> > > > err:
> > > > dev_err(panel->dev,
> > > > "failed to write command %d\n", ret);
> > > > return ret;
> > > > }
> > > >
> > >
> > > I'm not sure about the coding style rule here, would it be considered
> > > unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err
> > > block, but the block may not be directly used in that caller and is
> > > only jumped from the macro?
> >
> > I'm also not sure here. It was a quick and dirty test.
> > We might as well do something like
> >
> > ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...);
> > if (ret)
> > goto err;
> >
> > all over the place.
>
> Oh. This is actually very simple to fix and requires no code changes
> to clients. :-P We just need to hoist the printing out of the macro
> and into "drm_mipi_dsi.c". Let me double-confirm and then I'll post a
> patch.
Sounds good. I have converted boe-tv101wum-nl6, ilitek-ili9882t and
innolux-p079zca drivers. Once you post your patch I can post those
too.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver
2024-04-24 23:37 ` Dmitry Baryshkov
@ 2024-04-25 0:21 ` Doug Anderson
0 siblings, 0 replies; 19+ messages in thread
From: Doug Anderson @ 2024-04-25 0:21 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh,
krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree,
linux-kernel, cong yang
Hi,
On Wed, Apr 24, 2024 at 4:37 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 25 Apr 2024 at 02:25, Doug Anderson <dianders@google.com> wrote:
> >
> > Hi,
> >
> > On Wed, Apr 24, 2024 at 3:51 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang <hsinyi@google.com> wrote:
> > > >
> > > > On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov
> > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote:
> > > > > > > > >
> > > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \
> > > > > > > > > > > > > + .type = INIT_DCS_CMD, \
> > > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \
> > > > > > > > > > > > > + .type = DELAY_CMD,\
> > > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \
> > > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} }
> > > > > > > > > > > >
> > > > > > > > > > > > This is the third panel driver using the same appoach. Can you use
> > > > > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer
> > > > > > > > > > > > the table, we should extract this framework to a common helper.
> > > > > > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()).
> > > > > > > > > > > >
> > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the
> > > > > > > > > > > kernel size grows a lot since every sequence will be expanded.
> > > > > > > > > > >
> > > > > > > > > > > Similar discussion in here:
> > > > > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/
> > > > > > > > > > >
> > > > > > > > > > > This patch would increase the module size from 157K to 572K.
> > > > > > > > > > > scripts/bloat-o-meter shows chg +235.95%.
> > > > > > > > > > >
> > > > > > > > > > > So maybe the common helper is better regarding the kernel module size?
> > > > > > > > > >
> > > > > > > > > > Yes, let's get a framework done in a useful way.
> > > > > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be
> > > > > > > > > > used instead (and it's up to the developer to select correct delay
> > > > > > > > > > function).
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = {
> > > > > > > > > > > > > + _INIT_DELAY_CMD(50),
> > > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > > > > >
> > > > > > > > > > [skipped the body of the table]
> > > > > > > > > >
> > > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48),
> > > > > > > > > > > > > +
> > > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00),
> > > > > > > > > >
> > > > > > > > > > > > > + _INIT_DCS_CMD(0X11),
> > > > > > > > > >
> > > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode().
> > > > > > > > > >
> > > > > > > > > > > > > + /* T6: 120ms */
> > > > > > > > > > > > > + _INIT_DELAY_CMD(120),
> > > > > > > > > > > > > + _INIT_DCS_CMD(0X29),
> > > > > > > > > >
> > > > > > > > > > And this is mipi_dsi_dcs_set_display_on().
> > > > > > > > > >
> > > > > > > > > > Having a single table enourages people to put known commands into the
> > > > > > > > > > table, the practice that must be frowned upon and forbidden.
> > > > > > > > > >
> > > > > > > > > > We have functions for some of the standard DCS commands. So, maybe
> > > > > > > > > > instead of adding a single-table based approach we can improve
> > > > > > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the
> > > > > > > > > > error handling to a common part of enable() / prepare() function.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > For this panel, I think it can also refer to how
> > > > > > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data,
> > > > > > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when
> > > > > > > > > looping through the table.
> > > > > > > >
> > > > > > > > Even more similar discussion:
> > > > > > > >
> > > > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com
> > > > > > >
> > > > > > > It seems I skipped that thread.
> > > > > > >
> > > > > > > I'd still suggest a code-based solution compared to table-based one, for
> > > > > > > the reasons I've outlined before. Having a tables puts a pressure on the
> > > > > > > developer to put commands there for which we already have a
> > > > > > > command-specific function.
> > > > > >
> > > > > > The problem is that with these panels that need big init sequences the
> > > > > > code based solution is _a lot_ bigger. If it were a few bytes or a
> > > > > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move
> > > > > > from a table to code it was 100K difference in code [1]. I would also
> > > > > > say that having these long init sequences done as separate commands
> > > > > > encourages people to skip checking the return values of each of the
> > > > > > transfer functions and I don't love that idea.
> > > > > >
> > > > > > It would be ideal if these panels didn't need these long init
> > > > > > sequences, but I don't have any inside knowledge here saying that they
> > > > > > could be removed. So assume we can't get rid of the init sequences it
> > > > > > feels like we have to find some way to make the tables work for at
> > > > > > least the large chunks of init code and encourage people to make the
> > > > > > tables readable...
> > > > >
> > > > >
> > > > > I did a quick check on the boe-tv101wum-nl6 driver by converting the
> > > > > writes to use the following macro:
> > > > >
> > > > > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...)
> > > > > \
> > > > > do { \
> > > > > static const u8 d[] = { cmd, seq }; \
> > > > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> > > > > if (ret < 0) \
> > > > > goto err; \
> > > > > } while (0)
> > > > >
> > > > > And then at the end of the init funciton having
> > > > >
> > > > > err:
> > > > > dev_err(panel->dev,
> > > > > "failed to write command %d\n", ret);
> > > > > return ret;
> > > > > }
> > > > >
> > > >
> > > > I'm not sure about the coding style rule here, would it be considered
> > > > unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err
> > > > block, but the block may not be directly used in that caller and is
> > > > only jumped from the macro?
> > >
> > > I'm also not sure here. It was a quick and dirty test.
> > > We might as well do something like
> > >
> > > ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...);
> > > if (ret)
> > > goto err;
> > >
> > > all over the place.
> >
> > Oh. This is actually very simple to fix and requires no code changes
> > to clients. :-P We just need to hoist the printing out of the macro
> > and into "drm_mipi_dsi.c". Let me double-confirm and then I'll post a
> > patch.
>
> Sounds good. I have converted boe-tv101wum-nl6, ilitek-ili9882t and
> innolux-p079zca drivers. Once you post your patch I can post those
> too.
I sent out a patch for it, though I didn't have time to do testing on
real hardware:
https://lore.kernel.org/r/20240424172017.1.Id15fae80582bc74a0d4f1338987fa375738f45b9@changeid
I can poke more at it tomorrow...
-Doug
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-04-25 0:22 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-18 8:15 [PATCH v1 0/2] Add kd101ne3 bindings and driver lvzhaoxiong
2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong
2024-04-18 22:59 ` Krzysztof Kozlowski
2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong
2024-04-18 11:46 ` Dmitry Baryshkov
2024-04-18 13:11 ` Hsin-Yi Wang
2024-04-18 14:11 ` Dmitry Baryshkov
2024-04-23 18:10 ` Hsin-Yi Wang
2024-04-23 20:41 ` Doug Anderson
2024-04-23 21:20 ` Dmitry Baryshkov
2024-04-24 21:04 ` Doug Anderson
2024-04-24 21:18 ` Hsin-Yi Wang
2024-04-24 21:49 ` Dmitry Baryshkov
2024-04-24 22:15 ` Hsin-Yi Wang
2024-04-24 22:50 ` Dmitry Baryshkov
2024-04-24 23:25 ` Doug Anderson
2024-04-24 23:37 ` Dmitry Baryshkov
2024-04-25 0:21 ` Doug Anderson
2024-04-24 22:27 ` Doug Anderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).