[PATCH v2 0/3] drm/panel: Add MIPI DBI compatible SPI driver

[PATCH v2 0/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Hi,

This patchset adds a driver that will work with most MIPI DBI compatible
SPI panels out there.

It's a follow up on 'drm/tiny/st7735r: Match up with staging/fbtft
driver'[1] which aimed at making the st7735r driver work with all panels
adding DT properties.

Maxime gave[2] a good overview of the situation with these displays and
proposed to make a driver that works with all MIPI DBI compatible
controllers and use a firmware file to provide the controller setup for
a particular panel.

Main change since previous version:
- Drop model property and use the compatible property instead (Rob)

Noralf.

[1] https://lore.kernel.org/dri-devel/20211124150757.17929-1-noralf@tronnes.org/
[2] https://lore.kernel.org/dri-devel/20211129093946.xhp22mvdut3m67sc@houat/


Noralf Trønnes (3):
  dt-bindings: display: add bindings for MIPI DBI compatible SPI panels
  drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev
  drm/panel: Add MIPI DBI compatible SPI driver

 .../display/panel/panel-mipi-dbi-spi.yaml     |  59 +++
 MAINTAINERS                                   |   8 +
 drivers/gpu/drm/panel/Kconfig                 |  11 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-mipi-dbi.c        | 394 ++++++++++++++++++
 include/drm/drm_mipi_dbi.h                    |   2 +
 6 files changed, 475 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c

-- 
2.33.0

[PATCH v2 0/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Hi,

This patchset adds a driver that will work with most MIPI DBI compatible
SPI panels out there.

It's a follow up on 'drm/tiny/st7735r: Match up with staging/fbtft
driver'[1] which aimed at making the st7735r driver work with all panels
adding DT properties.

Maxime gave[2] a good overview of the situation with these displays and
proposed to make a driver that works with all MIPI DBI compatible
controllers and use a firmware file to provide the controller setup for
a particular panel.

Main change since previous version:
- Drop model property and use the compatible property instead (Rob)

Noralf.

[1] https://lore.kernel.org/dri-devel/20211124150757.17929-1-noralf@tronnes.org/
[2] https://lore.kernel.org/dri-devel/20211129093946.xhp22mvdut3m67sc@houat/


Noralf Trønnes (3):
  dt-bindings: display: add bindings for MIPI DBI compatible SPI panels
  drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev
  drm/panel: Add MIPI DBI compatible SPI driver

 .../display/panel/panel-mipi-dbi-spi.yaml     |  59 +++
 MAINTAINERS                                   |   8 +
 drivers/gpu/drm/panel/Kconfig                 |  11 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-mipi-dbi.c        | 394 ++++++++++++++++++
 include/drm/drm_mipi_dbi.h                    |   2 +
 6 files changed, 475 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c

-- 
2.33.0

[PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Noralf Trønnes]

Add binding for MIPI DBI compatible SPI panels.

v2:
- Fix path for panel-common.yaml
- Use unevaluatedProperties
- Drop properties which are in the allOf section
- Drop model property (Rob)

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
new file mode 100644
index 000000000000..b7cbeea0f8aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MIPI DBI SPI Panels Device Tree Bindings
+
+maintainers:
+  - Noralf Trønnes <noralf@tronnes.org>
+
+description:
+  This binding is for display panels using a MIPI DBI controller
+  in SPI mode.
+
+allOf:
+  - $ref: panel-common.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: panel-mipi-dbi-spi
+
+  write-only:
+    type: boolean
+    description:
+      Controller is not readable (ie. MISO is not wired up).
+
+  dc-gpios:
+    maxItems: 1
+    description: |
+      Controller data/command selection (D/CX) in 4-line SPI mode.
+      If not set, the controller is in 3-line SPI mode.
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            display@0{
+                    compatible = "panel-mipi-dbi-spi";
+                    reg = <0>;
+                    spi-max-frequency = <40000000>;
+                    dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
+                    reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
+                    write-only;
+            };
+    };
+
+...
-- 
2.33.0

[PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Noralf Trønnes]

Add binding for MIPI DBI compatible SPI panels.

v2:
- Fix path for panel-common.yaml
- Use unevaluatedProperties
- Drop properties which are in the allOf section
- Drop model property (Rob)

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
new file mode 100644
index 000000000000..b7cbeea0f8aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MIPI DBI SPI Panels Device Tree Bindings
+
+maintainers:
+  - Noralf Trønnes <noralf@tronnes.org>
+
+description:
+  This binding is for display panels using a MIPI DBI controller
+  in SPI mode.
+
+allOf:
+  - $ref: panel-common.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: panel-mipi-dbi-spi
+
+  write-only:
+    type: boolean
+    description:
+      Controller is not readable (ie. MISO is not wired up).
+
+  dc-gpios:
+    maxItems: 1
+    description: |
+      Controller data/command selection (D/CX) in 4-line SPI mode.
+      If not set, the controller is in 3-line SPI mode.
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            display@0{
+                    compatible = "panel-mipi-dbi-spi";
+                    reg = <0>;
+                    spi-max-frequency = <40000000>;
+                    dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
+                    reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
+                    write-only;
+            };
+    };
+
+...
-- 
2.33.0

[PATCH v2 2/3] drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev

[Noralf Trønnes]

devm_drm_dev_alloc() can't allocate structures that embed a structure
which then again embeds drm_device. Workaround this by adding a
driver_private pointer to struct mipi_dbi_dev which the driver can use for
its additional state.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 include/drm/drm_mipi_dbi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 6fe13cce2670..76aac6decfde 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -130,6 +130,8 @@ struct mipi_dbi_dev {
 	 * @dbi: MIPI DBI interface
 	 */
 	struct mipi_dbi dbi;
+
+	void *driver_private;
 };
 
 static inline struct mipi_dbi_dev *drm_to_mipi_dbi_dev(struct drm_device *drm)
-- 
2.33.0

[PATCH v2 2/3] drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev

[Noralf Trønnes]

devm_drm_dev_alloc() can't allocate structures that embed a structure
which then again embeds drm_device. Workaround this by adding a
driver_private pointer to struct mipi_dbi_dev which the driver can use for
its additional state.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 include/drm/drm_mipi_dbi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 6fe13cce2670..76aac6decfde 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -130,6 +130,8 @@ struct mipi_dbi_dev {
 	 * @dbi: MIPI DBI interface
 	 */
 	struct mipi_dbi dbi;
+
+	void *driver_private;
 };
 
 static inline struct mipi_dbi_dev *drm_to_mipi_dbi_dev(struct drm_device *drm)
-- 
2.33.0

[PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Add a driver that will work with most MIPI DBI compatible SPI panels.
This avoids adding a driver for every new MIPI DBI compatible controller
that is to be used by Linux. The 'compatible' Device Tree property with
a '.bin' suffix will be used to load a firmware file that contains the
controller configuration.

Example (driver will load sainsmart18.bin):

display@0 {
	compatible = "sainsmart18", "panel-mipi-dbi-spi";
	reg = <0>;
	reset-gpios = <&gpio 25 0>;
	dc-gpios = <&gpio 24 0>;
};

v2:
- Drop model property and use compatible instead (Rob)
- Add wiki entry in MAINTAINERS

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 MAINTAINERS                            |   8 +
 drivers/gpu/drm/panel/Kconfig          |  11 +
 drivers/gpu/drm/panel/Makefile         |   1 +
 drivers/gpu/drm/panel/panel-mipi-dbi.c | 394 +++++++++++++++++++++++++
 4 files changed, 414 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d03ad8da1f36..8baa98723bdc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6047,6 +6047,14 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
 F:	drivers/gpu/drm/tiny/mi0283qt.c
 
+DRM DRIVER FOR MIPI DBI compatible panels
+M:	Noralf Trønnes <noralf@tronnes.org>
+S:	Maintained
+W:	https://github.com/notro/panel-mipi-dbi/wiki
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
+F:	drivers/gpu/drm/panel/panel-mipi-dbi.c
+
 DRM DRIVER FOR MSM ADRENO GPU
 M:	Rob Clark <robdclark@gmail.com>
 M:	Sean Paul <sean@poorly.run>
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 434c2861bb40..1851cda5f877 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -274,6 +274,17 @@ config DRM_PANEL_LG_LG4573
 	  Say Y here if you want to enable support for LG4573 RGB panel.
 	  To compile this driver as a module, choose M here.
 
+config DRM_PANEL_MIPI_DBI
+	tristate "MIPI DBI compatible panel"
+	depends on SPI
+	depends on BACKLIGHT_CLASS_DEVICE
+	depends on DRM_KMS_HELPER
+	select DRM_KMS_CMA_HELPER
+	select DRM_MIPI_DBI
+	help
+	  Say Y here if you want to enable support for MIPI DBI compatible panels.
+	  To compile this driver as a module, choose M here.
+
 config DRM_PANEL_NEC_NL8048HL11
 	tristate "NEC NL8048HL11 RGB panel"
 	depends on GPIOLIB && OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index d99fbbce49d1..a90c30459964 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -25,6 +25,7 @@ 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
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
+obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o
 obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
 obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
 obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35950) += panel-novatek-nt35950.o
diff --git a/drivers/gpu/drm/panel/panel-mipi-dbi.c b/drivers/gpu/drm/panel/panel-mipi-dbi.c
new file mode 100644
index 000000000000..6e3dc2de21d2
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-mipi-dbi.c
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DRM driver for MIPI DBI compatible display panels
+ *
+ * Copyright 2022 Noralf Trønnes
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_mipi_dbi.h>
+#include <drm/drm_modeset_helper.h>
+#include <video/mipi_display.h>
+
+static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
+					     0, 0, 0, 0, 0, 0, 0 };
+
+/*
+ * The display panel configuration is stored in a firmware file. The Device Tree 'compatible'
+ * property value with a '.bin' suffix is passed to request_firmware() to fetch this file.
+ */
+struct panel_mipi_dbi_config {
+	/* Magic string: panel_mipi_dbi_magic */
+	u8 magic[15];
+
+	/* Config file format version */
+	u8 file_format_version;
+
+	/* Width in pixels */
+	__be16 width;
+	/* Height in pixels */
+	__be16 height;
+
+	/* Width in millimeters (optional) */
+	__be16 width_mm;
+	/* Height in millimeters (optional) */
+	__be16 height_mm;
+
+	/* X-axis panel offset */
+	__be16 x_offset;
+	/* Y-axis panel offset */
+	__be16 y_offset;
+
+	/* 4 pad bytes, must be zero */
+	u8 pad[4];
+
+	/*
+	 * Optional MIPI commands to execute when the display pipeline is enabled.
+	 * This can be used to configure the display controller.
+	 *
+	 * The commands are stored in a byte array with the format:
+	 *     command, num_parameters, [ parameter, ...], command, ...
+	 *
+	 * Some commands require a pause before the next command can be received.
+	 * Inserting a delay in the command sequence is done by using the NOP command with one
+	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
+	 * Command Set where it has no parameters).
+	 *
+	 * Example:
+	 *     command 0x11
+	 *     sleep 120ms
+	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
+	 *     command 0x29
+	 *
+	 * Byte sequence:
+	 *     0x11 0x00
+	 *     0x00 0x01 0x78
+	 *     0xb1 0x03 0x01 0x2c 0x2d
+	 *     0x29 0x00
+	 */
+	u8 commands[];
+};
+
+struct panel_mipi_dbi_commands {
+	const u8 *buf;
+	size_t len;
+};
+
+static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
+				  struct drm_crtc_state *crtc_state,
+				  struct drm_plane_state *plane_state)
+{
+	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+	struct panel_mipi_dbi_commands *commands = dbidev->driver_private;
+	struct mipi_dbi *dbi = &dbidev->dbi;
+	unsigned int i = 0;
+	int ret, idx;
+
+	if (!drm_dev_enter(pipe->crtc.dev, &idx))
+		return;
+
+	drm_dbg(pipe->crtc.dev, "\n");
+
+	ret = mipi_dbi_poweron_conditional_reset(dbidev);
+	if (ret < 0)
+		goto out_exit;
+	if (ret == 1)
+		goto out_enable;
+
+	if (!commands)
+		goto out_enable;
+
+	while (i < commands->len) {
+		u8 command = commands->buf[i++];
+		u8 num_parameters = commands->buf[i++];
+		const u8 *parameters = &commands->buf[i];
+
+		if (command == 0x00 && num_parameters == 1)
+			msleep(parameters[0]);
+		else if (num_parameters)
+			mipi_dbi_command_stackbuf(dbi, command, parameters, num_parameters);
+		else
+			mipi_dbi_command(dbi, command);
+
+		i += num_parameters;
+	}
+
+out_enable:
+	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
+out_exit:
+	drm_dev_exit(idx);
+}
+
+static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
+	.enable = panel_mipi_dbi_enable,
+	.disable = mipi_dbi_pipe_disable,
+	.update = mipi_dbi_pipe_update,
+};
+
+DEFINE_DRM_GEM_CMA_FOPS(panel_mipi_dbi_fops);
+
+static const struct drm_driver panel_mipi_dbi_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.fops			= &panel_mipi_dbi_fops,
+	DRM_GEM_CMA_DRIVER_OPS_VMAP,
+	.debugfs_init		= mipi_dbi_debugfs_init,
+	.name			= "panel-mipi-dbi",
+	.desc			= "MIPI DBI compatible display panel",
+	.date			= "20220103",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static int panel_mipi_dbi_parse_config(struct mipi_dbi_dev *dbidev,
+				       struct drm_display_mode *mode,
+				       const struct firmware *fw)
+{
+	const struct panel_mipi_dbi_config *config = (struct panel_mipi_dbi_config *)fw->data;
+	unsigned int width, height, x_offset, y_offset;
+	struct panel_mipi_dbi_commands *commands;
+	struct drm_device *drm = &dbidev->drm;
+	struct device *dev = dbidev->drm.dev;
+	size_t size = fw->size, commands_len;
+	unsigned int i = 0;
+
+	if (size < sizeof(*config)) {
+		dev_err(dev, "config: file size=%zu is too small\n", size);
+		return -EINVAL;
+	}
+
+	if (memcmp(config->magic, panel_mipi_dbi_magic, sizeof(config->magic))) {
+		dev_err(dev, "config: Bad magic: %15ph\n", config->magic);
+		return -EINVAL;
+	}
+
+	if (config->file_format_version != 1) {
+		dev_err(dev, "config: version=%u is not supported\n", config->file_format_version);
+		return -EINVAL;
+	}
+
+	width = be16_to_cpu(config->width);
+	height = be16_to_cpu(config->height);
+	x_offset = be16_to_cpu(config->x_offset);
+	y_offset = be16_to_cpu(config->y_offset);
+
+	drm_dbg(drm, "size=%zu version=%u\n", size, config->file_format_version);
+	drm_dbg(drm, "width=%u height=%u\n", width, height);
+	drm_dbg(drm, "x_offset=%u y_offset=%u\n", x_offset, y_offset);
+
+	if (width && height) {
+		struct drm_display_mode simple_mode = {
+			DRM_SIMPLE_MODE(width, height, be16_to_cpu(config->width_mm),
+					be16_to_cpu(config->height_mm))
+		};
+
+		*mode = simple_mode;
+	} else {
+		dev_err(dev, "config: width or height can't be zero\n");
+		return -EINVAL;
+	}
+
+	dbidev->left_offset = x_offset;
+	dbidev->top_offset = y_offset;
+
+	commands_len = size - sizeof(*config);
+	if (!commands_len)
+		return 0;
+
+	while ((i + 1) < commands_len) {
+		u8 command = config->commands[i++];
+		u8 num_parameters = config->commands[i++];
+		const u8 *parameters = &config->commands[i];
+
+		i += num_parameters;
+		if (i > commands_len) {
+			dev_err(dev, "config: command=0x%02x num_parameters=%u overflows\n",
+				command, num_parameters);
+			return -EINVAL;
+		}
+
+		if (command == 0x00 && num_parameters == 1)
+			drm_dbg(drm, "sleep %ums\n", parameters[0]);
+		else
+			drm_dbg(drm, "command %02x %*ph\n", command, num_parameters, parameters);
+	}
+
+	if (i != commands_len) {
+		dev_err(dev, "config: malformed command array\n");
+		return -EINVAL;
+	}
+
+	commands = devm_kzalloc(dev, sizeof(*commands), GFP_KERNEL);
+	if (!commands)
+		return -ENOMEM;
+
+	commands->len = commands_len;
+	commands->buf = devm_kmemdup(dev, config->commands, commands->len, GFP_KERNEL);
+	if (!commands->buf)
+		return -ENOMEM;
+
+	dbidev->driver_private = commands;
+
+	return 0;
+}
+
+static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct drm_display_mode mode;
+	struct mipi_dbi_dev *dbidev;
+	const struct firmware *fw;
+	const char *compatible;
+	struct drm_device *drm;
+	struct property *prop;
+	bool fw_found = false;
+	struct mipi_dbi *dbi;
+	struct gpio_desc *dc;
+	char fw_name[40];
+	int ret;
+
+	dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct mipi_dbi_dev, drm);
+	if (IS_ERR(dbidev))
+		return PTR_ERR(dbidev);
+
+	dbi = &dbidev->dbi;
+	drm = &dbidev->drm;
+
+	of_property_for_each_string(dev->of_node, "compatible", prop, compatible) {
+		snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
+
+		ret = firmware_request_nowarn(&fw, fw_name, dev);
+		if (ret) {
+			drm_dbg(drm, "No config file found for compatible: '%s' (error=%d)\n",
+				compatible, ret);
+			continue;
+		}
+
+		ret = panel_mipi_dbi_parse_config(dbidev, &mode, fw);
+		release_firmware(fw);
+		if (ret)
+			return ret;
+
+		fw_found = true;
+		break;
+	}
+
+	if (!fw_found) {
+		dev_err(dev, "No config file found\n");
+		return -ENOENT;
+	}
+
+	dbidev->regulator = devm_regulator_get(dev, "power");
+	if (IS_ERR(dbidev->regulator))
+		return dev_err_probe(dev, PTR_ERR(dbidev->regulator),
+				     "Failed to get regulator 'power'\n");
+
+	dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(dbi->reset))
+		return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get GPIO 'reset'\n");
+
+	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(dc))
+		return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO 'dc'\n");
+
+	dbidev->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(dbidev->backlight))
+		return dev_err_probe(dev, PTR_ERR(dbidev->backlight), "Failed to get backlight\n");
+
+	ret = mipi_dbi_spi_init(spi, dbi, dc);
+	if (ret)
+		return ret;
+
+	if (device_property_present(dev, "write-only"))
+		dbi->read_commands = NULL;
+
+	ret = mipi_dbi_dev_init(dbidev, &panel_mipi_dbi_pipe_funcs, &mode, 0);
+	if (ret)
+		return ret;
+
+	drm_mode_config_reset(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		return ret;
+
+	spi_set_drvdata(spi, drm);
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+
+static int panel_mipi_dbi_spi_remove(struct spi_device *spi)
+{
+	struct drm_device *drm = spi_get_drvdata(spi);
+
+	drm_dev_unplug(drm);
+	drm_atomic_helper_shutdown(drm);
+
+	return 0;
+}
+
+static void panel_mipi_dbi_spi_shutdown(struct spi_device *spi)
+{
+	drm_atomic_helper_shutdown(spi_get_drvdata(spi));
+}
+
+static int __maybe_unused panel_mipi_dbi_pm_suspend(struct device *dev)
+{
+	return drm_mode_config_helper_suspend(dev_get_drvdata(dev));
+}
+
+static int __maybe_unused panel_mipi_dbi_pm_resume(struct device *dev)
+{
+	drm_mode_config_helper_resume(dev_get_drvdata(dev));
+
+	return 0;
+}
+
+static const struct dev_pm_ops panel_mipi_dbi_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(panel_mipi_dbi_pm_suspend, panel_mipi_dbi_pm_resume)
+};
+
+static const struct of_device_id panel_mipi_dbi_spi_of_match[] = {
+	{ .compatible = "panel-mipi-dbi-spi" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, panel_mipi_dbi_spi_of_match);
+
+static const struct spi_device_id panel_mipi_dbi_spi_id[] = {
+	{ "panel-mipi-dbi-spi", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, panel_mipi_dbi_spi_id);
+
+static struct spi_driver panel_mipi_dbi_spi_driver = {
+	.driver = {
+		.name = "panel-mipi-dbi-spi",
+		.owner = THIS_MODULE,
+		.of_match_table = panel_mipi_dbi_spi_of_match,
+		.pm = &panel_mipi_dbi_pm_ops,
+	},
+	.id_table = panel_mipi_dbi_spi_id,
+	.probe = panel_mipi_dbi_spi_probe,
+	.remove = panel_mipi_dbi_spi_remove,
+	.shutdown = panel_mipi_dbi_spi_shutdown,
+};
+module_spi_driver(panel_mipi_dbi_spi_driver);
+
+MODULE_DESCRIPTION("MIPI DBI compatible display panel driver");
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("GPL");
-- 
2.33.0

[PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Add a driver that will work with most MIPI DBI compatible SPI panels.
This avoids adding a driver for every new MIPI DBI compatible controller
that is to be used by Linux. The 'compatible' Device Tree property with
a '.bin' suffix will be used to load a firmware file that contains the
controller configuration.

Example (driver will load sainsmart18.bin):

display@0 {
	compatible = "sainsmart18", "panel-mipi-dbi-spi";
	reg = <0>;
	reset-gpios = <&gpio 25 0>;
	dc-gpios = <&gpio 24 0>;
};

v2:
- Drop model property and use compatible instead (Rob)
- Add wiki entry in MAINTAINERS

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 MAINTAINERS                            |   8 +
 drivers/gpu/drm/panel/Kconfig          |  11 +
 drivers/gpu/drm/panel/Makefile         |   1 +
 drivers/gpu/drm/panel/panel-mipi-dbi.c | 394 +++++++++++++++++++++++++
 4 files changed, 414 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d03ad8da1f36..8baa98723bdc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6047,6 +6047,14 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
 F:	drivers/gpu/drm/tiny/mi0283qt.c
 
+DRM DRIVER FOR MIPI DBI compatible panels
+M:	Noralf Trønnes <noralf@tronnes.org>
+S:	Maintained
+W:	https://github.com/notro/panel-mipi-dbi/wiki
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
+F:	drivers/gpu/drm/panel/panel-mipi-dbi.c
+
 DRM DRIVER FOR MSM ADRENO GPU
 M:	Rob Clark <robdclark@gmail.com>
 M:	Sean Paul <sean@poorly.run>
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 434c2861bb40..1851cda5f877 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -274,6 +274,17 @@ config DRM_PANEL_LG_LG4573
 	  Say Y here if you want to enable support for LG4573 RGB panel.
 	  To compile this driver as a module, choose M here.
 
+config DRM_PANEL_MIPI_DBI
+	tristate "MIPI DBI compatible panel"
+	depends on SPI
+	depends on BACKLIGHT_CLASS_DEVICE
+	depends on DRM_KMS_HELPER
+	select DRM_KMS_CMA_HELPER
+	select DRM_MIPI_DBI
+	help
+	  Say Y here if you want to enable support for MIPI DBI compatible panels.
+	  To compile this driver as a module, choose M here.
+
 config DRM_PANEL_NEC_NL8048HL11
 	tristate "NEC NL8048HL11 RGB panel"
 	depends on GPIOLIB && OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index d99fbbce49d1..a90c30459964 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -25,6 +25,7 @@ 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
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
+obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o
 obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
 obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
 obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35950) += panel-novatek-nt35950.o
diff --git a/drivers/gpu/drm/panel/panel-mipi-dbi.c b/drivers/gpu/drm/panel/panel-mipi-dbi.c
new file mode 100644
index 000000000000..6e3dc2de21d2
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-mipi-dbi.c
@@ -0,0 +1,394 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DRM driver for MIPI DBI compatible display panels
+ *
+ * Copyright 2022 Noralf Trønnes
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_mipi_dbi.h>
+#include <drm/drm_modeset_helper.h>
+#include <video/mipi_display.h>
+
+static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
+					     0, 0, 0, 0, 0, 0, 0 };
+
+/*
+ * The display panel configuration is stored in a firmware file. The Device Tree 'compatible'
+ * property value with a '.bin' suffix is passed to request_firmware() to fetch this file.
+ */
+struct panel_mipi_dbi_config {
+	/* Magic string: panel_mipi_dbi_magic */
+	u8 magic[15];
+
+	/* Config file format version */
+	u8 file_format_version;
+
+	/* Width in pixels */
+	__be16 width;
+	/* Height in pixels */
+	__be16 height;
+
+	/* Width in millimeters (optional) */
+	__be16 width_mm;
+	/* Height in millimeters (optional) */
+	__be16 height_mm;
+
+	/* X-axis panel offset */
+	__be16 x_offset;
+	/* Y-axis panel offset */
+	__be16 y_offset;
+
+	/* 4 pad bytes, must be zero */
+	u8 pad[4];
+
+	/*
+	 * Optional MIPI commands to execute when the display pipeline is enabled.
+	 * This can be used to configure the display controller.
+	 *
+	 * The commands are stored in a byte array with the format:
+	 *     command, num_parameters, [ parameter, ...], command, ...
+	 *
+	 * Some commands require a pause before the next command can be received.
+	 * Inserting a delay in the command sequence is done by using the NOP command with one
+	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
+	 * Command Set where it has no parameters).
+	 *
+	 * Example:
+	 *     command 0x11
+	 *     sleep 120ms
+	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
+	 *     command 0x29
+	 *
+	 * Byte sequence:
+	 *     0x11 0x00
+	 *     0x00 0x01 0x78
+	 *     0xb1 0x03 0x01 0x2c 0x2d
+	 *     0x29 0x00
+	 */
+	u8 commands[];
+};
+
+struct panel_mipi_dbi_commands {
+	const u8 *buf;
+	size_t len;
+};
+
+static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
+				  struct drm_crtc_state *crtc_state,
+				  struct drm_plane_state *plane_state)
+{
+	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+	struct panel_mipi_dbi_commands *commands = dbidev->driver_private;
+	struct mipi_dbi *dbi = &dbidev->dbi;
+	unsigned int i = 0;
+	int ret, idx;
+
+	if (!drm_dev_enter(pipe->crtc.dev, &idx))
+		return;
+
+	drm_dbg(pipe->crtc.dev, "\n");
+
+	ret = mipi_dbi_poweron_conditional_reset(dbidev);
+	if (ret < 0)
+		goto out_exit;
+	if (ret == 1)
+		goto out_enable;
+
+	if (!commands)
+		goto out_enable;
+
+	while (i < commands->len) {
+		u8 command = commands->buf[i++];
+		u8 num_parameters = commands->buf[i++];
+		const u8 *parameters = &commands->buf[i];
+
+		if (command == 0x00 && num_parameters == 1)
+			msleep(parameters[0]);
+		else if (num_parameters)
+			mipi_dbi_command_stackbuf(dbi, command, parameters, num_parameters);
+		else
+			mipi_dbi_command(dbi, command);
+
+		i += num_parameters;
+	}
+
+out_enable:
+	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
+out_exit:
+	drm_dev_exit(idx);
+}
+
+static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
+	.enable = panel_mipi_dbi_enable,
+	.disable = mipi_dbi_pipe_disable,
+	.update = mipi_dbi_pipe_update,
+};
+
+DEFINE_DRM_GEM_CMA_FOPS(panel_mipi_dbi_fops);
+
+static const struct drm_driver panel_mipi_dbi_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.fops			= &panel_mipi_dbi_fops,
+	DRM_GEM_CMA_DRIVER_OPS_VMAP,
+	.debugfs_init		= mipi_dbi_debugfs_init,
+	.name			= "panel-mipi-dbi",
+	.desc			= "MIPI DBI compatible display panel",
+	.date			= "20220103",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static int panel_mipi_dbi_parse_config(struct mipi_dbi_dev *dbidev,
+				       struct drm_display_mode *mode,
+				       const struct firmware *fw)
+{
+	const struct panel_mipi_dbi_config *config = (struct panel_mipi_dbi_config *)fw->data;
+	unsigned int width, height, x_offset, y_offset;
+	struct panel_mipi_dbi_commands *commands;
+	struct drm_device *drm = &dbidev->drm;
+	struct device *dev = dbidev->drm.dev;
+	size_t size = fw->size, commands_len;
+	unsigned int i = 0;
+
+	if (size < sizeof(*config)) {
+		dev_err(dev, "config: file size=%zu is too small\n", size);
+		return -EINVAL;
+	}
+
+	if (memcmp(config->magic, panel_mipi_dbi_magic, sizeof(config->magic))) {
+		dev_err(dev, "config: Bad magic: %15ph\n", config->magic);
+		return -EINVAL;
+	}
+
+	if (config->file_format_version != 1) {
+		dev_err(dev, "config: version=%u is not supported\n", config->file_format_version);
+		return -EINVAL;
+	}
+
+	width = be16_to_cpu(config->width);
+	height = be16_to_cpu(config->height);
+	x_offset = be16_to_cpu(config->x_offset);
+	y_offset = be16_to_cpu(config->y_offset);
+
+	drm_dbg(drm, "size=%zu version=%u\n", size, config->file_format_version);
+	drm_dbg(drm, "width=%u height=%u\n", width, height);
+	drm_dbg(drm, "x_offset=%u y_offset=%u\n", x_offset, y_offset);
+
+	if (width && height) {
+		struct drm_display_mode simple_mode = {
+			DRM_SIMPLE_MODE(width, height, be16_to_cpu(config->width_mm),
+					be16_to_cpu(config->height_mm))
+		};
+
+		*mode = simple_mode;
+	} else {
+		dev_err(dev, "config: width or height can't be zero\n");
+		return -EINVAL;
+	}
+
+	dbidev->left_offset = x_offset;
+	dbidev->top_offset = y_offset;
+
+	commands_len = size - sizeof(*config);
+	if (!commands_len)
+		return 0;
+
+	while ((i + 1) < commands_len) {
+		u8 command = config->commands[i++];
+		u8 num_parameters = config->commands[i++];
+		const u8 *parameters = &config->commands[i];
+
+		i += num_parameters;
+		if (i > commands_len) {
+			dev_err(dev, "config: command=0x%02x num_parameters=%u overflows\n",
+				command, num_parameters);
+			return -EINVAL;
+		}
+
+		if (command == 0x00 && num_parameters == 1)
+			drm_dbg(drm, "sleep %ums\n", parameters[0]);
+		else
+			drm_dbg(drm, "command %02x %*ph\n", command, num_parameters, parameters);
+	}
+
+	if (i != commands_len) {
+		dev_err(dev, "config: malformed command array\n");
+		return -EINVAL;
+	}
+
+	commands = devm_kzalloc(dev, sizeof(*commands), GFP_KERNEL);
+	if (!commands)
+		return -ENOMEM;
+
+	commands->len = commands_len;
+	commands->buf = devm_kmemdup(dev, config->commands, commands->len, GFP_KERNEL);
+	if (!commands->buf)
+		return -ENOMEM;
+
+	dbidev->driver_private = commands;
+
+	return 0;
+}
+
+static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct drm_display_mode mode;
+	struct mipi_dbi_dev *dbidev;
+	const struct firmware *fw;
+	const char *compatible;
+	struct drm_device *drm;
+	struct property *prop;
+	bool fw_found = false;
+	struct mipi_dbi *dbi;
+	struct gpio_desc *dc;
+	char fw_name[40];
+	int ret;
+
+	dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct mipi_dbi_dev, drm);
+	if (IS_ERR(dbidev))
+		return PTR_ERR(dbidev);
+
+	dbi = &dbidev->dbi;
+	drm = &dbidev->drm;
+
+	of_property_for_each_string(dev->of_node, "compatible", prop, compatible) {
+		snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
+
+		ret = firmware_request_nowarn(&fw, fw_name, dev);
+		if (ret) {
+			drm_dbg(drm, "No config file found for compatible: '%s' (error=%d)\n",
+				compatible, ret);
+			continue;
+		}
+
+		ret = panel_mipi_dbi_parse_config(dbidev, &mode, fw);
+		release_firmware(fw);
+		if (ret)
+			return ret;
+
+		fw_found = true;
+		break;
+	}
+
+	if (!fw_found) {
+		dev_err(dev, "No config file found\n");
+		return -ENOENT;
+	}
+
+	dbidev->regulator = devm_regulator_get(dev, "power");
+	if (IS_ERR(dbidev->regulator))
+		return dev_err_probe(dev, PTR_ERR(dbidev->regulator),
+				     "Failed to get regulator 'power'\n");
+
+	dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(dbi->reset))
+		return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get GPIO 'reset'\n");
+
+	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(dc))
+		return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO 'dc'\n");
+
+	dbidev->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(dbidev->backlight))
+		return dev_err_probe(dev, PTR_ERR(dbidev->backlight), "Failed to get backlight\n");
+
+	ret = mipi_dbi_spi_init(spi, dbi, dc);
+	if (ret)
+		return ret;
+
+	if (device_property_present(dev, "write-only"))
+		dbi->read_commands = NULL;
+
+	ret = mipi_dbi_dev_init(dbidev, &panel_mipi_dbi_pipe_funcs, &mode, 0);
+	if (ret)
+		return ret;
+
+	drm_mode_config_reset(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		return ret;
+
+	spi_set_drvdata(spi, drm);
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+
+static int panel_mipi_dbi_spi_remove(struct spi_device *spi)
+{
+	struct drm_device *drm = spi_get_drvdata(spi);
+
+	drm_dev_unplug(drm);
+	drm_atomic_helper_shutdown(drm);
+
+	return 0;
+}
+
+static void panel_mipi_dbi_spi_shutdown(struct spi_device *spi)
+{
+	drm_atomic_helper_shutdown(spi_get_drvdata(spi));
+}
+
+static int __maybe_unused panel_mipi_dbi_pm_suspend(struct device *dev)
+{
+	return drm_mode_config_helper_suspend(dev_get_drvdata(dev));
+}
+
+static int __maybe_unused panel_mipi_dbi_pm_resume(struct device *dev)
+{
+	drm_mode_config_helper_resume(dev_get_drvdata(dev));
+
+	return 0;
+}
+
+static const struct dev_pm_ops panel_mipi_dbi_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(panel_mipi_dbi_pm_suspend, panel_mipi_dbi_pm_resume)
+};
+
+static const struct of_device_id panel_mipi_dbi_spi_of_match[] = {
+	{ .compatible = "panel-mipi-dbi-spi" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, panel_mipi_dbi_spi_of_match);
+
+static const struct spi_device_id panel_mipi_dbi_spi_id[] = {
+	{ "panel-mipi-dbi-spi", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, panel_mipi_dbi_spi_id);
+
+static struct spi_driver panel_mipi_dbi_spi_driver = {
+	.driver = {
+		.name = "panel-mipi-dbi-spi",
+		.owner = THIS_MODULE,
+		.of_match_table = panel_mipi_dbi_spi_of_match,
+		.pm = &panel_mipi_dbi_pm_ops,
+	},
+	.id_table = panel_mipi_dbi_spi_id,
+	.probe = panel_mipi_dbi_spi_probe,
+	.remove = panel_mipi_dbi_spi_remove,
+	.shutdown = panel_mipi_dbi_spi_shutdown,
+};
+module_spi_driver(panel_mipi_dbi_spi_driver);
+
+MODULE_DESCRIPTION("MIPI DBI compatible display panel driver");
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("GPL");
-- 
2.33.0

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Maxime Ripard]

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

Hi,

On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
> Add binding for MIPI DBI compatible SPI panels.
> 
> v2:
> - Fix path for panel-common.yaml
> - Use unevaluatedProperties
> - Drop properties which are in the allOf section
> - Drop model property (Rob)
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> new file mode 100644
> index 000000000000..b7cbeea0f8aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MIPI DBI SPI Panels Device Tree Bindings
> +
> +maintainers:
> +  - Noralf Trønnes <noralf@tronnes.org>
> +
> +description:
> +  This binding is for display panels using a MIPI DBI controller
> +  in SPI mode.
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: panel-mipi-dbi-spi

You need contains here, otherwise it will error out if you have two compatibles.

> +  write-only:
> +    type: boolean
> +    description:
> +      Controller is not readable (ie. MISO is not wired up).
> +
> +  dc-gpios:
> +    maxItems: 1
> +    description: |
> +      Controller data/command selection (D/CX) in 4-line SPI mode.
> +      If not set, the controller is in 3-line SPI mode.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            display@0{
> +                    compatible = "panel-mipi-dbi-spi";

We should have two compatibles in the example too

Maxime

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

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Maxime Ripard]

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

Hi,

On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
> Add binding for MIPI DBI compatible SPI panels.
> 
> v2:
> - Fix path for panel-common.yaml
> - Use unevaluatedProperties
> - Drop properties which are in the allOf section
> - Drop model property (Rob)
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> new file mode 100644
> index 000000000000..b7cbeea0f8aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MIPI DBI SPI Panels Device Tree Bindings
> +
> +maintainers:
> +  - Noralf Trønnes <noralf@tronnes.org>
> +
> +description:
> +  This binding is for display panels using a MIPI DBI controller
> +  in SPI mode.
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: panel-mipi-dbi-spi

You need contains here, otherwise it will error out if you have two compatibles.

> +  write-only:
> +    type: boolean
> +    description:
> +      Controller is not readable (ie. MISO is not wired up).
> +
> +  dc-gpios:
> +    maxItems: 1
> +    description: |
> +      Controller data/command selection (D/CX) in 4-line SPI mode.
> +      If not set, the controller is in 3-line SPI mode.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            display@0{
> +                    compatible = "panel-mipi-dbi-spi";

We should have two compatibles in the example too

Maxime

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

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Maxime Ripard]

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

On Tue, Jan 25, 2022 at 06:57:00PM +0100, Noralf Trønnes wrote:
> Add a driver that will work with most MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Linux. The 'compatible' Device Tree property with
> a '.bin' suffix will be used to load a firmware file that contains the
> controller configuration.
> 
> Example (driver will load sainsmart18.bin):
> 
> display@0 {
> 	compatible = "sainsmart18", "panel-mipi-dbi-spi";
> 	reg = <0>;
> 	reset-gpios = <&gpio 25 0>;
> 	dc-gpios = <&gpio 24 0>;
> };
> 
> v2:
> - Drop model property and use compatible instead (Rob)
> - Add wiki entry in MAINTAINERS
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  MAINTAINERS                            |   8 +
>  drivers/gpu/drm/panel/Kconfig          |  11 +
>  drivers/gpu/drm/panel/Makefile         |   1 +
>  drivers/gpu/drm/panel/panel-mipi-dbi.c | 394 +++++++++++++++++++++++++
>  4 files changed, 414 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d03ad8da1f36..8baa98723bdc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6047,6 +6047,14 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
>  F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
>  F:	drivers/gpu/drm/tiny/mi0283qt.c
>  
> +DRM DRIVER FOR MIPI DBI compatible panels
> +M:	Noralf Trønnes <noralf@tronnes.org>
> +S:	Maintained
> +W:	https://github.com/notro/panel-mipi-dbi/wiki
> +T:	git git://anongit.freedesktop.org/drm/drm-misc
> +F:	Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> +F:	drivers/gpu/drm/panel/panel-mipi-dbi.c
> +
>  DRM DRIVER FOR MSM ADRENO GPU
>  M:	Rob Clark <robdclark@gmail.com>
>  M:	Sean Paul <sean@poorly.run>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 434c2861bb40..1851cda5f877 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -274,6 +274,17 @@ config DRM_PANEL_LG_LG4573
>  	  Say Y here if you want to enable support for LG4573 RGB panel.
>  	  To compile this driver as a module, choose M here.
>  
> +config DRM_PANEL_MIPI_DBI
> +	tristate "MIPI DBI compatible panel"
> +	depends on SPI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on DRM_KMS_HELPER
> +	select DRM_KMS_CMA_HELPER
> +	select DRM_MIPI_DBI
> +	help
> +	  Say Y here if you want to enable support for MIPI DBI compatible panels.
> +	  To compile this driver as a module, choose M here.
> +
>  config DRM_PANEL_NEC_NL8048HL11
>  	tristate "NEC NL8048HL11 RGB panel"
>  	depends on GPIOLIB && OF && SPI
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index d99fbbce49d1..a90c30459964 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -25,6 +25,7 @@ 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
>  obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
> +obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o
>  obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35950) += panel-novatek-nt35950.o
> diff --git a/drivers/gpu/drm/panel/panel-mipi-dbi.c b/drivers/gpu/drm/panel/panel-mipi-dbi.c
> new file mode 100644
> index 000000000000..6e3dc2de21d2
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-mipi-dbi.c
> @@ -0,0 +1,394 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DRM driver for MIPI DBI compatible display panels
> + *
> + * Copyright 2022 Noralf Trønnes
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_modeset_helper.h>
> +#include <video/mipi_display.h>
> +
> +static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
> +					     0, 0, 0, 0, 0, 0, 0 };
> +
> +/*
> + * The display panel configuration is stored in a firmware file. The Device Tree 'compatible'
> + * property value with a '.bin' suffix is passed to request_firmware() to fetch this file.
> + */
> +struct panel_mipi_dbi_config {
> +	/* Magic string: panel_mipi_dbi_magic */
> +	u8 magic[15];
> +
> +	/* Config file format version */
> +	u8 file_format_version;
> +
> +	/* Width in pixels */
> +	__be16 width;
> +	/* Height in pixels */
> +	__be16 height;
> +
> +	/* Width in millimeters (optional) */
> +	__be16 width_mm;
> +	/* Height in millimeters (optional) */
> +	__be16 height_mm;
> +
> +	/* X-axis panel offset */
> +	__be16 x_offset;
> +	/* Y-axis panel offset */
> +	__be16 y_offset;
> +
> +	/* 4 pad bytes, must be zero */
> +	u8 pad[4];
> +
> +	/*
> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
> +	 * This can be used to configure the display controller.
> +	 *
> +	 * The commands are stored in a byte array with the format:
> +	 *     command, num_parameters, [ parameter, ...], command, ...
> +	 *
> +	 * Some commands require a pause before the next command can be received.
> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
> +	 * Command Set where it has no parameters).
> +	 *
> +	 * Example:
> +	 *     command 0x11
> +	 *     sleep 120ms
> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
> +	 *     command 0x29
> +	 *
> +	 * Byte sequence:
> +	 *     0x11 0x00
> +	 *     0x00 0x01 0x78
> +	 *     0xb1 0x03 0x01 0x2c 0x2d
> +	 *     0x29 0x00
> +	 */
> +	u8 commands[];
> +};

I'm not really a fan of parsing raw data in the kernel. I guess we can't
really avoid the introduction of a special case to sleep, but we already
have dt properties for all of the other properties (but X and Y offset,
maybe?)

Maybe we should use those instead?

Maxime

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

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Maxime Ripard]

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

On Tue, Jan 25, 2022 at 06:57:00PM +0100, Noralf Trønnes wrote:
> Add a driver that will work with most MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Linux. The 'compatible' Device Tree property with
> a '.bin' suffix will be used to load a firmware file that contains the
> controller configuration.
> 
> Example (driver will load sainsmart18.bin):
> 
> display@0 {
> 	compatible = "sainsmart18", "panel-mipi-dbi-spi";
> 	reg = <0>;
> 	reset-gpios = <&gpio 25 0>;
> 	dc-gpios = <&gpio 24 0>;
> };
> 
> v2:
> - Drop model property and use compatible instead (Rob)
> - Add wiki entry in MAINTAINERS
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  MAINTAINERS                            |   8 +
>  drivers/gpu/drm/panel/Kconfig          |  11 +
>  drivers/gpu/drm/panel/Makefile         |   1 +
>  drivers/gpu/drm/panel/panel-mipi-dbi.c | 394 +++++++++++++++++++++++++
>  4 files changed, 414 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d03ad8da1f36..8baa98723bdc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6047,6 +6047,14 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
>  F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
>  F:	drivers/gpu/drm/tiny/mi0283qt.c
>  
> +DRM DRIVER FOR MIPI DBI compatible panels
> +M:	Noralf Trønnes <noralf@tronnes.org>
> +S:	Maintained
> +W:	https://github.com/notro/panel-mipi-dbi/wiki
> +T:	git git://anongit.freedesktop.org/drm/drm-misc
> +F:	Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> +F:	drivers/gpu/drm/panel/panel-mipi-dbi.c
> +
>  DRM DRIVER FOR MSM ADRENO GPU
>  M:	Rob Clark <robdclark@gmail.com>
>  M:	Sean Paul <sean@poorly.run>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 434c2861bb40..1851cda5f877 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -274,6 +274,17 @@ config DRM_PANEL_LG_LG4573
>  	  Say Y here if you want to enable support for LG4573 RGB panel.
>  	  To compile this driver as a module, choose M here.
>  
> +config DRM_PANEL_MIPI_DBI
> +	tristate "MIPI DBI compatible panel"
> +	depends on SPI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on DRM_KMS_HELPER
> +	select DRM_KMS_CMA_HELPER
> +	select DRM_MIPI_DBI
> +	help
> +	  Say Y here if you want to enable support for MIPI DBI compatible panels.
> +	  To compile this driver as a module, choose M here.
> +
>  config DRM_PANEL_NEC_NL8048HL11
>  	tristate "NEC NL8048HL11 RGB panel"
>  	depends on GPIOLIB && OF && SPI
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index d99fbbce49d1..a90c30459964 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -25,6 +25,7 @@ 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
>  obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
> +obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o
>  obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35950) += panel-novatek-nt35950.o
> diff --git a/drivers/gpu/drm/panel/panel-mipi-dbi.c b/drivers/gpu/drm/panel/panel-mipi-dbi.c
> new file mode 100644
> index 000000000000..6e3dc2de21d2
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-mipi-dbi.c
> @@ -0,0 +1,394 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DRM driver for MIPI DBI compatible display panels
> + *
> + * Copyright 2022 Noralf Trønnes
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_modeset_helper.h>
> +#include <video/mipi_display.h>
> +
> +static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
> +					     0, 0, 0, 0, 0, 0, 0 };
> +
> +/*
> + * The display panel configuration is stored in a firmware file. The Device Tree 'compatible'
> + * property value with a '.bin' suffix is passed to request_firmware() to fetch this file.
> + */
> +struct panel_mipi_dbi_config {
> +	/* Magic string: panel_mipi_dbi_magic */
> +	u8 magic[15];
> +
> +	/* Config file format version */
> +	u8 file_format_version;
> +
> +	/* Width in pixels */
> +	__be16 width;
> +	/* Height in pixels */
> +	__be16 height;
> +
> +	/* Width in millimeters (optional) */
> +	__be16 width_mm;
> +	/* Height in millimeters (optional) */
> +	__be16 height_mm;
> +
> +	/* X-axis panel offset */
> +	__be16 x_offset;
> +	/* Y-axis panel offset */
> +	__be16 y_offset;
> +
> +	/* 4 pad bytes, must be zero */
> +	u8 pad[4];
> +
> +	/*
> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
> +	 * This can be used to configure the display controller.
> +	 *
> +	 * The commands are stored in a byte array with the format:
> +	 *     command, num_parameters, [ parameter, ...], command, ...
> +	 *
> +	 * Some commands require a pause before the next command can be received.
> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
> +	 * Command Set where it has no parameters).
> +	 *
> +	 * Example:
> +	 *     command 0x11
> +	 *     sleep 120ms
> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
> +	 *     command 0x29
> +	 *
> +	 * Byte sequence:
> +	 *     0x11 0x00
> +	 *     0x00 0x01 0x78
> +	 *     0xb1 0x03 0x01 0x2c 0x2d
> +	 *     0x29 0x00
> +	 */
> +	u8 commands[];
> +};

I'm not really a fan of parsing raw data in the kernel. I guess we can't
really avoid the introduction of a special case to sleep, but we already
have dt properties for all of the other properties (but X and Y offset,
maybe?)

Maybe we should use those instead?

Maxime

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

Re: [PATCH v2 0/3] drm/panel: Add MIPI DBI compatible SPI driver

[David Lechner]

On 1/25/22 11:56 AM, Noralf Trønnes wrote:
> Hi,
> 
> This patchset adds a driver that will work with most MIPI DBI compatible
> SPI panels out there.
> 
> It's a follow up on 'drm/tiny/st7735r: Match up with staging/fbtft
> driver'[1] which aimed at making the st7735r driver work with all panels
> adding DT properties.
> 
> Maxime gave[2] a good overview of the situation with these displays and
> proposed to make a driver that works with all MIPI DBI compatible
> controllers and use a firmware file to provide the controller setup for
> a particular panel.
> 
> Main change since previous version:
> - Drop model property and use the compatible property instead (Rob)
> 
> Noralf.
> 
> [1] https://lore.kernel.org/dri-devel/20211124150757.17929-1-noralf@tronnes.org/
> [2] https://lore.kernel.org/dri-devel/20211129093946.xhp22mvdut3m67sc@houat/
> 
> 
> Noralf Trønnes (3):
>    dt-bindings: display: add bindings for MIPI DBI compatible SPI panels
>    drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev
>    drm/panel: Add MIPI DBI compatible SPI driver
> 
>   .../display/panel/panel-mipi-dbi-spi.yaml     |  59 +++
>   MAINTAINERS                                   |   8 +
>   drivers/gpu/drm/panel/Kconfig                 |  11 +
>   drivers/gpu/drm/panel/Makefile                |   1 +
>   drivers/gpu/drm/panel/panel-mipi-dbi.c        | 394 ++++++++++++++++++
>   include/drm/drm_mipi_dbi.h                    |   2 +
>   6 files changed, 475 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>   create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c
> 

It would be useful to also include a patch for a tool to create
these "firmware" files. For example a Python script that takes
a more human-readable input and generates a .bin file.

Re: [PATCH v2 0/3] drm/panel: Add MIPI DBI compatible SPI driver

[David Lechner]

On 1/25/22 11:56 AM, Noralf Trønnes wrote:
> Hi,
> 
> This patchset adds a driver that will work with most MIPI DBI compatible
> SPI panels out there.
> 
> It's a follow up on 'drm/tiny/st7735r: Match up with staging/fbtft
> driver'[1] which aimed at making the st7735r driver work with all panels
> adding DT properties.
> 
> Maxime gave[2] a good overview of the situation with these displays and
> proposed to make a driver that works with all MIPI DBI compatible
> controllers and use a firmware file to provide the controller setup for
> a particular panel.
> 
> Main change since previous version:
> - Drop model property and use the compatible property instead (Rob)
> 
> Noralf.
> 
> [1] https://lore.kernel.org/dri-devel/20211124150757.17929-1-noralf@tronnes.org/
> [2] https://lore.kernel.org/dri-devel/20211129093946.xhp22mvdut3m67sc@houat/
> 
> 
> Noralf Trønnes (3):
>    dt-bindings: display: add bindings for MIPI DBI compatible SPI panels
>    drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev
>    drm/panel: Add MIPI DBI compatible SPI driver
> 
>   .../display/panel/panel-mipi-dbi-spi.yaml     |  59 +++
>   MAINTAINERS                                   |   8 +
>   drivers/gpu/drm/panel/Kconfig                 |  11 +
>   drivers/gpu/drm/panel/Makefile                |   1 +
>   drivers/gpu/drm/panel/panel-mipi-dbi.c        | 394 ++++++++++++++++++
>   include/drm/drm_mipi_dbi.h                    |   2 +
>   6 files changed, 475 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>   create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c
> 

It would be useful to also include a patch for a tool to create
these "firmware" files. For example a Python script that takes
a more human-readable input and generates a .bin file.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[David Lechner]

On 1/25/22 11:57 AM, Noralf Trønnes wrote:
> Add a driver that will work with most MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Linux. The 'compatible' Device Tree property with
> a '.bin' suffix will be used to load a firmware file that contains the
> controller configuration.
> 
> Example (driver will load sainsmart18.bin):
> 
> display@0 {
> 	compatible = "sainsmart18", "panel-mipi-dbi-spi";
> 	reg = <0>;
> 	reset-gpios = <&gpio 25 0>;
> 	dc-gpios = <&gpio 24 0>;
> };
> 

...

> +static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct drm_display_mode mode;
> +	struct mipi_dbi_dev *dbidev;
> +	const struct firmware *fw;
> +	const char *compatible;
> +	struct drm_device *drm;
> +	struct property *prop;
> +	bool fw_found = false;
> +	struct mipi_dbi *dbi;
> +	struct gpio_desc *dc;
> +	char fw_name[40];
> +	int ret;
> +
> +	dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct mipi_dbi_dev, drm);
> +	if (IS_ERR(dbidev))
> +		return PTR_ERR(dbidev);
> +
> +	dbi = &dbidev->dbi;
> +	drm = &dbidev->drm;
> +
> +	of_property_for_each_string(dev->of_node, "compatible", prop, compatible) {
> +		snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
> +
> +		ret = firmware_request_nowarn(&fw, fw_name, dev);
> +		if (ret) {
> +			drm_dbg(drm, "No config file found for compatible: '%s' (error=%d)\n",
> +				compatible, ret);
> +			continue;
> +		}
> +

Should we add a directory prefix to the firmware file name to avoid the possibility of
file name clashes with unrelated firmwares?

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[David Lechner]

On 1/25/22 11:57 AM, Noralf Trønnes wrote:
> Add a driver that will work with most MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Linux. The 'compatible' Device Tree property with
> a '.bin' suffix will be used to load a firmware file that contains the
> controller configuration.
> 
> Example (driver will load sainsmart18.bin):
> 
> display@0 {
> 	compatible = "sainsmart18", "panel-mipi-dbi-spi";
> 	reg = <0>;
> 	reset-gpios = <&gpio 25 0>;
> 	dc-gpios = <&gpio 24 0>;
> };
> 

...

> +static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct drm_display_mode mode;
> +	struct mipi_dbi_dev *dbidev;
> +	const struct firmware *fw;
> +	const char *compatible;
> +	struct drm_device *drm;
> +	struct property *prop;
> +	bool fw_found = false;
> +	struct mipi_dbi *dbi;
> +	struct gpio_desc *dc;
> +	char fw_name[40];
> +	int ret;
> +
> +	dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct mipi_dbi_dev, drm);
> +	if (IS_ERR(dbidev))
> +		return PTR_ERR(dbidev);
> +
> +	dbi = &dbidev->dbi;
> +	drm = &dbidev->drm;
> +
> +	of_property_for_each_string(dev->of_node, "compatible", prop, compatible) {
> +		snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
> +
> +		ret = firmware_request_nowarn(&fw, fw_name, dev);
> +		if (ret) {
> +			drm_dbg(drm, "No config file found for compatible: '%s' (error=%d)\n",
> +				compatible, ret);
> +			continue;
> +		}
> +

Should we add a directory prefix to the firmware file name to avoid the possibility of
file name clashes with unrelated firmwares?

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 27.01.2022 11.04, skrev Maxime Ripard:
> On Tue, Jan 25, 2022 at 06:57:00PM +0100, Noralf Trønnes wrote:
>> Add a driver that will work with most MIPI DBI compatible SPI panels.
>> This avoids adding a driver for every new MIPI DBI compatible controller
>> that is to be used by Linux. The 'compatible' Device Tree property with
>> a '.bin' suffix will be used to load a firmware file that contains the
>> controller configuration.
>>
>> Example (driver will load sainsmart18.bin):
>>
>> display@0 {
>> 	compatible = "sainsmart18", "panel-mipi-dbi-spi";
>> 	reg = <0>;
>> 	reset-gpios = <&gpio 25 0>;
>> 	dc-gpios = <&gpio 24 0>;
>> };
>>
>> v2:
>> - Drop model property and use compatible instead (Rob)
>> - Add wiki entry in MAINTAINERS
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>  MAINTAINERS                            |   8 +
>>  drivers/gpu/drm/panel/Kconfig          |  11 +
>>  drivers/gpu/drm/panel/Makefile         |   1 +
>>  drivers/gpu/drm/panel/panel-mipi-dbi.c | 394 +++++++++++++++++++++++++
>>  4 files changed, 414 insertions(+)
>>  create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d03ad8da1f36..8baa98723bdc 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6047,6 +6047,14 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
>>  F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
>>  F:	drivers/gpu/drm/tiny/mi0283qt.c
>>  
>> +DRM DRIVER FOR MIPI DBI compatible panels
>> +M:	Noralf Trønnes <noralf@tronnes.org>
>> +S:	Maintained
>> +W:	https://github.com/notro/panel-mipi-dbi/wiki
>> +T:	git git://anongit.freedesktop.org/drm/drm-misc
>> +F:	Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>> +F:	drivers/gpu/drm/panel/panel-mipi-dbi.c
>> +
>>  DRM DRIVER FOR MSM ADRENO GPU
>>  M:	Rob Clark <robdclark@gmail.com>
>>  M:	Sean Paul <sean@poorly.run>
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 434c2861bb40..1851cda5f877 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -274,6 +274,17 @@ config DRM_PANEL_LG_LG4573
>>  	  Say Y here if you want to enable support for LG4573 RGB panel.
>>  	  To compile this driver as a module, choose M here.
>>  
>> +config DRM_PANEL_MIPI_DBI
>> +	tristate "MIPI DBI compatible panel"
>> +	depends on SPI
>> +	depends on BACKLIGHT_CLASS_DEVICE
>> +	depends on DRM_KMS_HELPER
>> +	select DRM_KMS_CMA_HELPER
>> +	select DRM_MIPI_DBI
>> +	help
>> +	  Say Y here if you want to enable support for MIPI DBI compatible panels.
>> +	  To compile this driver as a module, choose M here.
>> +
>>  config DRM_PANEL_NEC_NL8048HL11
>>  	tristate "NEC NL8048HL11 RGB panel"
>>  	depends on GPIOLIB && OF && SPI
>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>> index d99fbbce49d1..a90c30459964 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -25,6 +25,7 @@ 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
>>  obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
>> +obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o
>>  obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
>>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
>>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35950) += panel-novatek-nt35950.o
>> diff --git a/drivers/gpu/drm/panel/panel-mipi-dbi.c b/drivers/gpu/drm/panel/panel-mipi-dbi.c
>> new file mode 100644
>> index 000000000000..6e3dc2de21d2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-mipi-dbi.c
>> @@ -0,0 +1,394 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * DRM driver for MIPI DBI compatible display panels
>> + *
>> + * Copyright 2022 Noralf Trønnes
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/delay.h>
>> +#include <linux/firmware.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/property.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_fb_helper.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_gem_cma_helper.h>
>> +#include <drm/drm_managed.h>
>> +#include <drm/drm_mipi_dbi.h>
>> +#include <drm/drm_modeset_helper.h>
>> +#include <video/mipi_display.h>
>> +
>> +static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
>> +					     0, 0, 0, 0, 0, 0, 0 };
>> +
>> +/*
>> + * The display panel configuration is stored in a firmware file. The Device Tree 'compatible'
>> + * property value with a '.bin' suffix is passed to request_firmware() to fetch this file.
>> + */
>> +struct panel_mipi_dbi_config {
>> +	/* Magic string: panel_mipi_dbi_magic */
>> +	u8 magic[15];
>> +
>> +	/* Config file format version */
>> +	u8 file_format_version;
>> +
>> +	/* Width in pixels */
>> +	__be16 width;
>> +	/* Height in pixels */
>> +	__be16 height;
>> +
>> +	/* Width in millimeters (optional) */
>> +	__be16 width_mm;
>> +	/* Height in millimeters (optional) */
>> +	__be16 height_mm;
>> +
>> +	/* X-axis panel offset */
>> +	__be16 x_offset;
>> +	/* Y-axis panel offset */
>> +	__be16 y_offset;
>> +
>> +	/* 4 pad bytes, must be zero */
>> +	u8 pad[4];
>> +
>> +	/*
>> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
>> +	 * This can be used to configure the display controller.
>> +	 *
>> +	 * The commands are stored in a byte array with the format:
>> +	 *     command, num_parameters, [ parameter, ...], command, ...
>> +	 *
>> +	 * Some commands require a pause before the next command can be received.
>> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
>> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
>> +	 * Command Set where it has no parameters).
>> +	 *
>> +	 * Example:
>> +	 *     command 0x11
>> +	 *     sleep 120ms
>> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
>> +	 *     command 0x29
>> +	 *
>> +	 * Byte sequence:
>> +	 *     0x11 0x00
>> +	 *     0x00 0x01 0x78
>> +	 *     0xb1 0x03 0x01 0x2c 0x2d
>> +	 *     0x29 0x00
>> +	 */
>> +	u8 commands[];
>> +};
> 
> I'm not really a fan of parsing raw data in the kernel. I guess we can't
> really avoid the introduction of a special case to sleep, but we already
> have dt properties for all of the other properties (but X and Y offset,
> maybe?)
> 
> Maybe we should use those instead?
> 

I don't understand your reluctance to parsing data, lots of ioctls do
it. And this data can only be loaded by root. What I like about having
these properties in the config file is that the binding becomes a
fallback binding that can actually be made to work without changing the
Device Tree.

For arguments sake let's say tiny/st7735r.c was not built and we had
this node:

display@0{
	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
"panel-mipi-dbi-spi";
};

It will still be possible to use this display without changing the
Device Tree. Just add a firmware/config file.

Having the properties in DT it would have to look like this for the
fallback to work:

display@0{
	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
"panel-mipi-dbi-spi";
	panel-timing = {
		hactive = <128>;
		vactive = <128>;
	};
	width-mm = <25>;
	height-mm = <26>;
	x-offset = <2>;
	y-offset = <3>;
};

Is this important, I'm not sure. What do you think?

The users I care most about have DT overlays so for them it doesn't
matter much where the properties are.

Noralf.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 27.01.2022 11.04, skrev Maxime Ripard:
> On Tue, Jan 25, 2022 at 06:57:00PM +0100, Noralf Trønnes wrote:
>> Add a driver that will work with most MIPI DBI compatible SPI panels.
>> This avoids adding a driver for every new MIPI DBI compatible controller
>> that is to be used by Linux. The 'compatible' Device Tree property with
>> a '.bin' suffix will be used to load a firmware file that contains the
>> controller configuration.
>>
>> Example (driver will load sainsmart18.bin):
>>
>> display@0 {
>> 	compatible = "sainsmart18", "panel-mipi-dbi-spi";
>> 	reg = <0>;
>> 	reset-gpios = <&gpio 25 0>;
>> 	dc-gpios = <&gpio 24 0>;
>> };
>>
>> v2:
>> - Drop model property and use compatible instead (Rob)
>> - Add wiki entry in MAINTAINERS
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>  MAINTAINERS                            |   8 +
>>  drivers/gpu/drm/panel/Kconfig          |  11 +
>>  drivers/gpu/drm/panel/Makefile         |   1 +
>>  drivers/gpu/drm/panel/panel-mipi-dbi.c | 394 +++++++++++++++++++++++++
>>  4 files changed, 414 insertions(+)
>>  create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d03ad8da1f36..8baa98723bdc 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6047,6 +6047,14 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
>>  F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
>>  F:	drivers/gpu/drm/tiny/mi0283qt.c
>>  
>> +DRM DRIVER FOR MIPI DBI compatible panels
>> +M:	Noralf Trønnes <noralf@tronnes.org>
>> +S:	Maintained
>> +W:	https://github.com/notro/panel-mipi-dbi/wiki
>> +T:	git git://anongit.freedesktop.org/drm/drm-misc
>> +F:	Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>> +F:	drivers/gpu/drm/panel/panel-mipi-dbi.c
>> +
>>  DRM DRIVER FOR MSM ADRENO GPU
>>  M:	Rob Clark <robdclark@gmail.com>
>>  M:	Sean Paul <sean@poorly.run>
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 434c2861bb40..1851cda5f877 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -274,6 +274,17 @@ config DRM_PANEL_LG_LG4573
>>  	  Say Y here if you want to enable support for LG4573 RGB panel.
>>  	  To compile this driver as a module, choose M here.
>>  
>> +config DRM_PANEL_MIPI_DBI
>> +	tristate "MIPI DBI compatible panel"
>> +	depends on SPI
>> +	depends on BACKLIGHT_CLASS_DEVICE
>> +	depends on DRM_KMS_HELPER
>> +	select DRM_KMS_CMA_HELPER
>> +	select DRM_MIPI_DBI
>> +	help
>> +	  Say Y here if you want to enable support for MIPI DBI compatible panels.
>> +	  To compile this driver as a module, choose M here.
>> +
>>  config DRM_PANEL_NEC_NL8048HL11
>>  	tristate "NEC NL8048HL11 RGB panel"
>>  	depends on GPIOLIB && OF && SPI
>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>> index d99fbbce49d1..a90c30459964 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -25,6 +25,7 @@ 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
>>  obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
>> +obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o
>>  obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
>>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
>>  obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35950) += panel-novatek-nt35950.o
>> diff --git a/drivers/gpu/drm/panel/panel-mipi-dbi.c b/drivers/gpu/drm/panel/panel-mipi-dbi.c
>> new file mode 100644
>> index 000000000000..6e3dc2de21d2
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-mipi-dbi.c
>> @@ -0,0 +1,394 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * DRM driver for MIPI DBI compatible display panels
>> + *
>> + * Copyright 2022 Noralf Trønnes
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/delay.h>
>> +#include <linux/firmware.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/property.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_fb_helper.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_gem_cma_helper.h>
>> +#include <drm/drm_managed.h>
>> +#include <drm/drm_mipi_dbi.h>
>> +#include <drm/drm_modeset_helper.h>
>> +#include <video/mipi_display.h>
>> +
>> +static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
>> +					     0, 0, 0, 0, 0, 0, 0 };
>> +
>> +/*
>> + * The display panel configuration is stored in a firmware file. The Device Tree 'compatible'
>> + * property value with a '.bin' suffix is passed to request_firmware() to fetch this file.
>> + */
>> +struct panel_mipi_dbi_config {
>> +	/* Magic string: panel_mipi_dbi_magic */
>> +	u8 magic[15];
>> +
>> +	/* Config file format version */
>> +	u8 file_format_version;
>> +
>> +	/* Width in pixels */
>> +	__be16 width;
>> +	/* Height in pixels */
>> +	__be16 height;
>> +
>> +	/* Width in millimeters (optional) */
>> +	__be16 width_mm;
>> +	/* Height in millimeters (optional) */
>> +	__be16 height_mm;
>> +
>> +	/* X-axis panel offset */
>> +	__be16 x_offset;
>> +	/* Y-axis panel offset */
>> +	__be16 y_offset;
>> +
>> +	/* 4 pad bytes, must be zero */
>> +	u8 pad[4];
>> +
>> +	/*
>> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
>> +	 * This can be used to configure the display controller.
>> +	 *
>> +	 * The commands are stored in a byte array with the format:
>> +	 *     command, num_parameters, [ parameter, ...], command, ...
>> +	 *
>> +	 * Some commands require a pause before the next command can be received.
>> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
>> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
>> +	 * Command Set where it has no parameters).
>> +	 *
>> +	 * Example:
>> +	 *     command 0x11
>> +	 *     sleep 120ms
>> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
>> +	 *     command 0x29
>> +	 *
>> +	 * Byte sequence:
>> +	 *     0x11 0x00
>> +	 *     0x00 0x01 0x78
>> +	 *     0xb1 0x03 0x01 0x2c 0x2d
>> +	 *     0x29 0x00
>> +	 */
>> +	u8 commands[];
>> +};
> 
> I'm not really a fan of parsing raw data in the kernel. I guess we can't
> really avoid the introduction of a special case to sleep, but we already
> have dt properties for all of the other properties (but X and Y offset,
> maybe?)
> 
> Maybe we should use those instead?
> 

I don't understand your reluctance to parsing data, lots of ioctls do
it. And this data can only be loaded by root. What I like about having
these properties in the config file is that the binding becomes a
fallback binding that can actually be made to work without changing the
Device Tree.

For arguments sake let's say tiny/st7735r.c was not built and we had
this node:

display@0{
	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
"panel-mipi-dbi-spi";
};

It will still be possible to use this display without changing the
Device Tree. Just add a firmware/config file.

Having the properties in DT it would have to look like this for the
fallback to work:

display@0{
	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
"panel-mipi-dbi-spi";
	panel-timing = {
		hactive = <128>;
		vactive = <128>;
	};
	width-mm = <25>;
	height-mm = <26>;
	x-offset = <2>;
	y-offset = <3>;
};

Is this important, I'm not sure. What do you think?

The users I care most about have DT overlays so for them it doesn't
matter much where the properties are.

Noralf.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Sam Ravnborg]

Hi Noralf,

On Tue, Jan 25, 2022 at 06:57:00PM +0100, Noralf Trønnes wrote:
> Add a driver that will work with most MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Linux. The 'compatible' Device Tree property with
> a '.bin' suffix will be used to load a firmware file that contains the
> controller configuration.

Loading a configuration from a firmware file is very
elegant - I like.
This will be very useful in a million cases with a lot of small panels!

> +
> +	/*
> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
> +	 * This can be used to configure the display controller.
> +	 *
> +	 * The commands are stored in a byte array with the format:
> +	 *     command, num_parameters, [ parameter, ...], command, ...
> +	 *
> +	 * Some commands require a pause before the next command can be received.
> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
> +	 * Command Set where it has no parameters).
> +	 *
> +	 * Example:
> +	 *     command 0x11
> +	 *     sleep 120ms
> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
> +	 *     command 0x29
> +	 *
> +	 * Byte sequence:
> +	 *     0x11 0x00
> +	 *     0x00 0x01 0x78
> +	 *     0xb1 0x03 0x01 0x2c 0x2d
> +	 *     0x29 0x00
> +	 */
Using a binary file that is unreadable when it is first created is less
elegant.
I am sure you have considered a txt file - and I know parsing a txt file
in prone for more errros than parsing a binary file.


But if the text file looked like:
"
	# The is a comment
	cmd 0x11 0x00

	# We need to sleep
	sleepms 120

	# Do something more
	cmd 0xb1 0x03 0x01 0x2c 0x2d
	cmd 0x29 0x00
"

The file is easier to comment (document) and easier to read and
modify.
The suffix could be ".panel" to tell this is something specific for
a panel.
Using lib/parser could make the code somewhat simple but I did not try
to code it myself.

The code you included below for the binary file is very simple,
but you shift the burden to the people who have to create binary files.
And people using obscure displays are not always so good at this stuff.

Sorry that I do not include code to do the above, but let me know if
this would help to convince you.

	Sam - who has been absent due to house renovation and such

Re: [PATCH v2 0/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 27.01.2022 18.13, skrev David Lechner:
> On 1/25/22 11:56 AM, Noralf Trønnes wrote:
>> Hi,
>>
>> This patchset adds a driver that will work with most MIPI DBI compatible
>> SPI panels out there.
>>
>> It's a follow up on 'drm/tiny/st7735r: Match up with staging/fbtft
>> driver'[1] which aimed at making the st7735r driver work with all panels
>> adding DT properties.
>>
>> Maxime gave[2] a good overview of the situation with these displays and
>> proposed to make a driver that works with all MIPI DBI compatible
>> controllers and use a firmware file to provide the controller setup for
>> a particular panel.
>>
>> Main change since previous version:
>> - Drop model property and use the compatible property instead (Rob)
>>
>> Noralf.
>>
>> [1]
>> https://lore.kernel.org/dri-devel/20211124150757.17929-1-noralf@tronnes.org/
>>
>> [2]
>> https://lore.kernel.org/dri-devel/20211129093946.xhp22mvdut3m67sc@houat/
>>
>>
>> Noralf Trønnes (3):
>>    dt-bindings: display: add bindings for MIPI DBI compatible SPI panels
>>    drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev
>>    drm/panel: Add MIPI DBI compatible SPI driver
>>
>>   .../display/panel/panel-mipi-dbi-spi.yaml     |  59 +++
>>   MAINTAINERS                                   |   8 +
>>   drivers/gpu/drm/panel/Kconfig                 |  11 +
>>   drivers/gpu/drm/panel/Makefile                |   1 +
>>   drivers/gpu/drm/panel/panel-mipi-dbi.c        | 394 ++++++++++++++++++
>>   include/drm/drm_mipi_dbi.h                    |   2 +
>>   6 files changed, 475 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>   create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c
>>
> 
> It would be useful to also include a patch for a tool to create
> these "firmware" files. For example a Python script that takes
> a more human-readable input and generates a .bin file.

I will put a script on the github repo that holds the wiki for the
driver. I haven't made the script yet, just using a hack for now until
the format is decided upon.

Noralf.

Re: [PATCH v2 0/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 27.01.2022 18.13, skrev David Lechner:
> On 1/25/22 11:56 AM, Noralf Trønnes wrote:
>> Hi,
>>
>> This patchset adds a driver that will work with most MIPI DBI compatible
>> SPI panels out there.
>>
>> It's a follow up on 'drm/tiny/st7735r: Match up with staging/fbtft
>> driver'[1] which aimed at making the st7735r driver work with all panels
>> adding DT properties.
>>
>> Maxime gave[2] a good overview of the situation with these displays and
>> proposed to make a driver that works with all MIPI DBI compatible
>> controllers and use a firmware file to provide the controller setup for
>> a particular panel.
>>
>> Main change since previous version:
>> - Drop model property and use the compatible property instead (Rob)
>>
>> Noralf.
>>
>> [1]
>> https://lore.kernel.org/dri-devel/20211124150757.17929-1-noralf@tronnes.org/
>>
>> [2]
>> https://lore.kernel.org/dri-devel/20211129093946.xhp22mvdut3m67sc@houat/
>>
>>
>> Noralf Trønnes (3):
>>    dt-bindings: display: add bindings for MIPI DBI compatible SPI panels
>>    drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev
>>    drm/panel: Add MIPI DBI compatible SPI driver
>>
>>   .../display/panel/panel-mipi-dbi-spi.yaml     |  59 +++
>>   MAINTAINERS                                   |   8 +
>>   drivers/gpu/drm/panel/Kconfig                 |  11 +
>>   drivers/gpu/drm/panel/Makefile                |   1 +
>>   drivers/gpu/drm/panel/panel-mipi-dbi.c        | 394 ++++++++++++++++++
>>   include/drm/drm_mipi_dbi.h                    |   2 +
>>   6 files changed, 475 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>   create mode 100644 drivers/gpu/drm/panel/panel-mipi-dbi.c
>>
> 
> It would be useful to also include a patch for a tool to create
> these "firmware" files. For example a Python script that takes
> a more human-readable input and generates a .bin file.

I will put a script on the github repo that holds the wiki for the
driver. I haven't made the script yet, just using a hack for now until
the format is decided upon.

Noralf.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 27.01.2022 18.19, skrev David Lechner:
> On 1/25/22 11:57 AM, Noralf Trønnes wrote:
>> Add a driver that will work with most MIPI DBI compatible SPI panels.
>> This avoids adding a driver for every new MIPI DBI compatible controller
>> that is to be used by Linux. The 'compatible' Device Tree property with
>> a '.bin' suffix will be used to load a firmware file that contains the
>> controller configuration.
>>
>> Example (driver will load sainsmart18.bin):
>>
>> display@0 {
>>     compatible = "sainsmart18", "panel-mipi-dbi-spi";
>>     reg = <0>;
>>     reset-gpios = <&gpio 25 0>;
>>     dc-gpios = <&gpio 24 0>;
>> };
>>
> 
> ...
> 
>> +static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
>> +{
>> +    struct device *dev = &spi->dev;
>> +    struct drm_display_mode mode;
>> +    struct mipi_dbi_dev *dbidev;
>> +    const struct firmware *fw;
>> +    const char *compatible;
>> +    struct drm_device *drm;
>> +    struct property *prop;
>> +    bool fw_found = false;
>> +    struct mipi_dbi *dbi;
>> +    struct gpio_desc *dc;
>> +    char fw_name[40];
>> +    int ret;
>> +
>> +    dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct
>> mipi_dbi_dev, drm);
>> +    if (IS_ERR(dbidev))
>> +        return PTR_ERR(dbidev);
>> +
>> +    dbi = &dbidev->dbi;
>> +    drm = &dbidev->drm;
>> +
>> +    of_property_for_each_string(dev->of_node, "compatible", prop,
>> compatible) {
>> +        snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
>> +
>> +        ret = firmware_request_nowarn(&fw, fw_name, dev);
>> +        if (ret) {
>> +            drm_dbg(drm, "No config file found for compatible: '%s'
>> (error=%d)\n",
>> +                compatible, ret);
>> +            continue;
>> +        }
>> +
> 
> Should we add a directory prefix to the firmware file name to avoid the
> possibility of
> file name clashes with unrelated firmwares?

I did consider this but I think it very unlikely that there would be a
collision between the name of display/panel and some other firmware file
which usually have the product name/model in the filename. And in the
unlikelihood that there is a collision it's possible to choose another
name for the compatible.

Noralf.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 27.01.2022 18.19, skrev David Lechner:
> On 1/25/22 11:57 AM, Noralf Trønnes wrote:
>> Add a driver that will work with most MIPI DBI compatible SPI panels.
>> This avoids adding a driver for every new MIPI DBI compatible controller
>> that is to be used by Linux. The 'compatible' Device Tree property with
>> a '.bin' suffix will be used to load a firmware file that contains the
>> controller configuration.
>>
>> Example (driver will load sainsmart18.bin):
>>
>> display@0 {
>>     compatible = "sainsmart18", "panel-mipi-dbi-spi";
>>     reg = <0>;
>>     reset-gpios = <&gpio 25 0>;
>>     dc-gpios = <&gpio 24 0>;
>> };
>>
> 
> ...
> 
>> +static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
>> +{
>> +    struct device *dev = &spi->dev;
>> +    struct drm_display_mode mode;
>> +    struct mipi_dbi_dev *dbidev;
>> +    const struct firmware *fw;
>> +    const char *compatible;
>> +    struct drm_device *drm;
>> +    struct property *prop;
>> +    bool fw_found = false;
>> +    struct mipi_dbi *dbi;
>> +    struct gpio_desc *dc;
>> +    char fw_name[40];
>> +    int ret;
>> +
>> +    dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct
>> mipi_dbi_dev, drm);
>> +    if (IS_ERR(dbidev))
>> +        return PTR_ERR(dbidev);
>> +
>> +    dbi = &dbidev->dbi;
>> +    drm = &dbidev->drm;
>> +
>> +    of_property_for_each_string(dev->of_node, "compatible", prop,
>> compatible) {
>> +        snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
>> +
>> +        ret = firmware_request_nowarn(&fw, fw_name, dev);
>> +        if (ret) {
>> +            drm_dbg(drm, "No config file found for compatible: '%s'
>> (error=%d)\n",
>> +                compatible, ret);
>> +            continue;
>> +        }
>> +
> 
> Should we add a directory prefix to the firmware file name to avoid the
> possibility of
> file name clashes with unrelated firmwares?

I did consider this but I think it very unlikely that there would be a
collision between the name of display/panel and some other firmware file
which usually have the product name/model in the filename. And in the
unlikelihood that there is a collision it's possible to choose another
name for the compatible.

Noralf.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 27.01.2022 20.59, skrev Sam Ravnborg:
> Hi Noralf,
> 
> On Tue, Jan 25, 2022 at 06:57:00PM +0100, Noralf Trønnes wrote:
>> Add a driver that will work with most MIPI DBI compatible SPI panels.
>> This avoids adding a driver for every new MIPI DBI compatible controller
>> that is to be used by Linux. The 'compatible' Device Tree property with
>> a '.bin' suffix will be used to load a firmware file that contains the
>> controller configuration.
> 
> Loading a configuration from a firmware file is very
> elegant - I like.
> This will be very useful in a million cases with a lot of small panels!
> 

Yes I really hope we can find a way to get this accepted.

>> +
>> +	/*
>> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
>> +	 * This can be used to configure the display controller.
>> +	 *
>> +	 * The commands are stored in a byte array with the format:
>> +	 *     command, num_parameters, [ parameter, ...], command, ...
>> +	 *
>> +	 * Some commands require a pause before the next command can be received.
>> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
>> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
>> +	 * Command Set where it has no parameters).
>> +	 *
>> +	 * Example:
>> +	 *     command 0x11
>> +	 *     sleep 120ms
>> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
>> +	 *     command 0x29
>> +	 *
>> +	 * Byte sequence:
>> +	 *     0x11 0x00
>> +	 *     0x00 0x01 0x78
>> +	 *     0xb1 0x03 0x01 0x2c 0x2d
>> +	 *     0x29 0x00
>> +	 */
> Using a binary file that is unreadable when it is first created is less
> elegant.
> I am sure you have considered a txt file - and I know parsing a txt file
> in prone for more errros than parsing a binary file.
> 
> 
> But if the text file looked like:
> "
> 	# The is a comment
> 	cmd 0x11 0x00
> 
> 	# We need to sleep
> 	sleepms 120
> 
> 	# Do something more
> 	cmd 0xb1 0x03 0x01 0x2c 0x2d
> 	cmd 0x29 0x00
> "
> 
> The file is easier to comment (document) and easier to read and
> modify.
> The suffix could be ".panel" to tell this is something specific for
> a panel.
> Using lib/parser could make the code somewhat simple but I did not try
> to code it myself.
> 
> The code you included below for the binary file is very simple,
> but you shift the burden to the people who have to create binary files.
> And people using obscure displays are not always so good at this stuff.
> 

Parsing text files in the kernel sounds very scary, not something that I
would like to try.

I will make a script that generates and parses the binary representation
(which is big endian so it's somewhat readable with xxd or the like).
There's a wiki link in the MAINTAINERS entry that will have info about
the format including the script. It will also serve as a place to share
config snippets/script incantations for displays.

I will make the script when the file format is decided upon. Here's the
hack I currently use:
https://gist.github.com/notro/3ca61c48e7dcc4a0ef34dbadbc30bfa5

Noralf.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 27.01.2022 20.59, skrev Sam Ravnborg:
> Hi Noralf,
> 
> On Tue, Jan 25, 2022 at 06:57:00PM +0100, Noralf Trønnes wrote:
>> Add a driver that will work with most MIPI DBI compatible SPI panels.
>> This avoids adding a driver for every new MIPI DBI compatible controller
>> that is to be used by Linux. The 'compatible' Device Tree property with
>> a '.bin' suffix will be used to load a firmware file that contains the
>> controller configuration.
> 
> Loading a configuration from a firmware file is very
> elegant - I like.
> This will be very useful in a million cases with a lot of small panels!
> 

Yes I really hope we can find a way to get this accepted.

>> +
>> +	/*
>> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
>> +	 * This can be used to configure the display controller.
>> +	 *
>> +	 * The commands are stored in a byte array with the format:
>> +	 *     command, num_parameters, [ parameter, ...], command, ...
>> +	 *
>> +	 * Some commands require a pause before the next command can be received.
>> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
>> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
>> +	 * Command Set where it has no parameters).
>> +	 *
>> +	 * Example:
>> +	 *     command 0x11
>> +	 *     sleep 120ms
>> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
>> +	 *     command 0x29
>> +	 *
>> +	 * Byte sequence:
>> +	 *     0x11 0x00
>> +	 *     0x00 0x01 0x78
>> +	 *     0xb1 0x03 0x01 0x2c 0x2d
>> +	 *     0x29 0x00
>> +	 */
> Using a binary file that is unreadable when it is first created is less
> elegant.
> I am sure you have considered a txt file - and I know parsing a txt file
> in prone for more errros than parsing a binary file.
> 
> 
> But if the text file looked like:
> "
> 	# The is a comment
> 	cmd 0x11 0x00
> 
> 	# We need to sleep
> 	sleepms 120
> 
> 	# Do something more
> 	cmd 0xb1 0x03 0x01 0x2c 0x2d
> 	cmd 0x29 0x00
> "
> 
> The file is easier to comment (document) and easier to read and
> modify.
> The suffix could be ".panel" to tell this is something specific for
> a panel.
> Using lib/parser could make the code somewhat simple but I did not try
> to code it myself.
> 
> The code you included below for the binary file is very simple,
> but you shift the burden to the people who have to create binary files.
> And people using obscure displays are not always so good at this stuff.
> 

Parsing text files in the kernel sounds very scary, not something that I
would like to try.

I will make a script that generates and parses the binary representation
(which is big endian so it's somewhat readable with xxd or the like).
There's a wiki link in the MAINTAINERS entry that will have info about
the format including the script. It will also serve as a place to share
config snippets/script incantations for displays.

I will make the script when the file format is decided upon. Here's the
hack I currently use:
https://gist.github.com/notro/3ca61c48e7dcc4a0ef34dbadbc30bfa5

Noralf.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Maxime Ripard]

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

On Thu, Jan 27, 2022 at 06:53:48PM +0100, Noralf Trønnes wrote:
> >> +struct panel_mipi_dbi_config {
> >> +	/* Magic string: panel_mipi_dbi_magic */
> >> +	u8 magic[15];
> >> +
> >> +	/* Config file format version */
> >> +	u8 file_format_version;
> >> +
> >> +	/* Width in pixels */
> >> +	__be16 width;
> >> +	/* Height in pixels */
> >> +	__be16 height;
> >> +
> >> +	/* Width in millimeters (optional) */
> >> +	__be16 width_mm;
> >> +	/* Height in millimeters (optional) */
> >> +	__be16 height_mm;
> >> +
> >> +	/* X-axis panel offset */
> >> +	__be16 x_offset;
> >> +	/* Y-axis panel offset */
> >> +	__be16 y_offset;
> >> +
> >> +	/* 4 pad bytes, must be zero */
> >> +	u8 pad[4];
> >> +
> >> +	/*
> >> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
> >> +	 * This can be used to configure the display controller.
> >> +	 *
> >> +	 * The commands are stored in a byte array with the format:
> >> +	 *     command, num_parameters, [ parameter, ...], command, ...
> >> +	 *
> >> +	 * Some commands require a pause before the next command can be received.
> >> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
> >> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
> >> +	 * Command Set where it has no parameters).
> >> +	 *
> >> +	 * Example:
> >> +	 *     command 0x11
> >> +	 *     sleep 120ms
> >> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
> >> +	 *     command 0x29
> >> +	 *
> >> +	 * Byte sequence:
> >> +	 *     0x11 0x00
> >> +	 *     0x00 0x01 0x78
> >> +	 *     0xb1 0x03 0x01 0x2c 0x2d
> >> +	 *     0x29 0x00
> >> +	 */
> >> +	u8 commands[];
> >> +};
> > 
> > I'm not really a fan of parsing raw data in the kernel. I guess we can't
> > really avoid the introduction of a special case to sleep, but we already
> > have dt properties for all of the other properties (but X and Y offset,
> > maybe?)
> > 
> > Maybe we should use those instead?
>
> I don't understand your reluctance to parsing data, lots of ioctls do
> it.

The reluctance comes from the parsing itself: you need to have input
validation, and it's hard to get right. The less we have, the easier it
gets.

> And this data can only be loaded by root. What I like about having
> these properties in the config file is that the binding becomes a
> fallback binding that can actually be made to work without changing the
> Device Tree.
> 
> For arguments sake let's say tiny/st7735r.c was not built and we had
> this node:
> 
> display@0{
> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
> "panel-mipi-dbi-spi";
> };
> 
> It will still be possible to use this display without changing the
> Device Tree. Just add a firmware/config file.
> 
> Having the properties in DT it would have to look like this for the
> fallback to work:
> 
> display@0{
> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
> "panel-mipi-dbi-spi";
> 	panel-timing = {
> 		hactive = <128>;
> 		vactive = <128>;
> 	};
> 	width-mm = <25>;
> 	height-mm = <26>;
> 	x-offset = <2>;
> 	y-offset = <3>;
> };
> 
> Is this important, I'm not sure. What do you think?

Parts of it is ergonomics I guess. We're used to having all those
properties either in the DT or the driver, but here we introduce a new
way that isn't done anywhere else.

And I don't see any real downside to putting it in the DT? It's going to
be in an overlay, under the user's control anyway, right?

Maxime

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

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Maxime Ripard]

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

On Thu, Jan 27, 2022 at 06:53:48PM +0100, Noralf Trønnes wrote:
> >> +struct panel_mipi_dbi_config {
> >> +	/* Magic string: panel_mipi_dbi_magic */
> >> +	u8 magic[15];
> >> +
> >> +	/* Config file format version */
> >> +	u8 file_format_version;
> >> +
> >> +	/* Width in pixels */
> >> +	__be16 width;
> >> +	/* Height in pixels */
> >> +	__be16 height;
> >> +
> >> +	/* Width in millimeters (optional) */
> >> +	__be16 width_mm;
> >> +	/* Height in millimeters (optional) */
> >> +	__be16 height_mm;
> >> +
> >> +	/* X-axis panel offset */
> >> +	__be16 x_offset;
> >> +	/* Y-axis panel offset */
> >> +	__be16 y_offset;
> >> +
> >> +	/* 4 pad bytes, must be zero */
> >> +	u8 pad[4];
> >> +
> >> +	/*
> >> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
> >> +	 * This can be used to configure the display controller.
> >> +	 *
> >> +	 * The commands are stored in a byte array with the format:
> >> +	 *     command, num_parameters, [ parameter, ...], command, ...
> >> +	 *
> >> +	 * Some commands require a pause before the next command can be received.
> >> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
> >> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
> >> +	 * Command Set where it has no parameters).
> >> +	 *
> >> +	 * Example:
> >> +	 *     command 0x11
> >> +	 *     sleep 120ms
> >> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
> >> +	 *     command 0x29
> >> +	 *
> >> +	 * Byte sequence:
> >> +	 *     0x11 0x00
> >> +	 *     0x00 0x01 0x78
> >> +	 *     0xb1 0x03 0x01 0x2c 0x2d
> >> +	 *     0x29 0x00
> >> +	 */
> >> +	u8 commands[];
> >> +};
> > 
> > I'm not really a fan of parsing raw data in the kernel. I guess we can't
> > really avoid the introduction of a special case to sleep, but we already
> > have dt properties for all of the other properties (but X and Y offset,
> > maybe?)
> > 
> > Maybe we should use those instead?
>
> I don't understand your reluctance to parsing data, lots of ioctls do
> it.

The reluctance comes from the parsing itself: you need to have input
validation, and it's hard to get right. The less we have, the easier it
gets.

> And this data can only be loaded by root. What I like about having
> these properties in the config file is that the binding becomes a
> fallback binding that can actually be made to work without changing the
> Device Tree.
> 
> For arguments sake let's say tiny/st7735r.c was not built and we had
> this node:
> 
> display@0{
> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
> "panel-mipi-dbi-spi";
> };
> 
> It will still be possible to use this display without changing the
> Device Tree. Just add a firmware/config file.
> 
> Having the properties in DT it would have to look like this for the
> fallback to work:
> 
> display@0{
> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
> "panel-mipi-dbi-spi";
> 	panel-timing = {
> 		hactive = <128>;
> 		vactive = <128>;
> 	};
> 	width-mm = <25>;
> 	height-mm = <26>;
> 	x-offset = <2>;
> 	y-offset = <3>;
> };
> 
> Is this important, I'm not sure. What do you think?

Parts of it is ergonomics I guess. We're used to having all those
properties either in the DT or the driver, but here we introduce a new
way that isn't done anywhere else.

And I don't see any real downside to putting it in the DT? It's going to
be in an overlay, under the user's control anyway, right?

Maxime

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

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 02.02.2022 11.09, skrev Maxime Ripard:
> On Thu, Jan 27, 2022 at 06:53:48PM +0100, Noralf Trønnes wrote:
>>>> +struct panel_mipi_dbi_config {
>>>> +	/* Magic string: panel_mipi_dbi_magic */
>>>> +	u8 magic[15];
>>>> +
>>>> +	/* Config file format version */
>>>> +	u8 file_format_version;
>>>> +
>>>> +	/* Width in pixels */
>>>> +	__be16 width;
>>>> +	/* Height in pixels */
>>>> +	__be16 height;
>>>> +
>>>> +	/* Width in millimeters (optional) */
>>>> +	__be16 width_mm;
>>>> +	/* Height in millimeters (optional) */
>>>> +	__be16 height_mm;
>>>> +
>>>> +	/* X-axis panel offset */
>>>> +	__be16 x_offset;
>>>> +	/* Y-axis panel offset */
>>>> +	__be16 y_offset;
>>>> +
>>>> +	/* 4 pad bytes, must be zero */
>>>> +	u8 pad[4];
>>>> +
>>>> +	/*
>>>> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
>>>> +	 * This can be used to configure the display controller.
>>>> +	 *
>>>> +	 * The commands are stored in a byte array with the format:
>>>> +	 *     command, num_parameters, [ parameter, ...], command, ...
>>>> +	 *
>>>> +	 * Some commands require a pause before the next command can be received.
>>>> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
>>>> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
>>>> +	 * Command Set where it has no parameters).
>>>> +	 *
>>>> +	 * Example:
>>>> +	 *     command 0x11
>>>> +	 *     sleep 120ms
>>>> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
>>>> +	 *     command 0x29
>>>> +	 *
>>>> +	 * Byte sequence:
>>>> +	 *     0x11 0x00
>>>> +	 *     0x00 0x01 0x78
>>>> +	 *     0xb1 0x03 0x01 0x2c 0x2d
>>>> +	 *     0x29 0x00
>>>> +	 */
>>>> +	u8 commands[];
>>>> +};
>>>
>>> I'm not really a fan of parsing raw data in the kernel. I guess we can't
>>> really avoid the introduction of a special case to sleep, but we already
>>> have dt properties for all of the other properties (but X and Y offset,
>>> maybe?)
>>>
>>> Maybe we should use those instead?
>>
>> I don't understand your reluctance to parsing data, lots of ioctls do
>> it.
> 
> The reluctance comes from the parsing itself: you need to have input
> validation, and it's hard to get right. The less we have, the easier it
> gets.
> 
>> And this data can only be loaded by root. What I like about having
>> these properties in the config file is that the binding becomes a
>> fallback binding that can actually be made to work without changing the
>> Device Tree.
>>
>> For arguments sake let's say tiny/st7735r.c was not built and we had
>> this node:
>>
>> display@0{
>> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
>> "panel-mipi-dbi-spi";
>> };
>>
>> It will still be possible to use this display without changing the
>> Device Tree. Just add a firmware/config file.
>>
>> Having the properties in DT it would have to look like this for the
>> fallback to work:
>>
>> display@0{
>> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
>> "panel-mipi-dbi-spi";
>> 	panel-timing = {
>> 		hactive = <128>;
>> 		vactive = <128>;
>> 	};
>> 	width-mm = <25>;
>> 	height-mm = <26>;
>> 	x-offset = <2>;
>> 	y-offset = <3>;
>> };
>>
>> Is this important, I'm not sure. What do you think?
> 
> Parts of it is ergonomics I guess. We're used to having all those
> properties either in the DT or the driver, but here we introduce a new
> way that isn't done anywhere else.
> 
> And I don't see any real downside to putting it in the DT? It's going to
> be in an overlay, under the user's control anyway, right?
> 

Ok, I'll spin a new version using DT properties.

Noralf.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 02.02.2022 11.09, skrev Maxime Ripard:
> On Thu, Jan 27, 2022 at 06:53:48PM +0100, Noralf Trønnes wrote:
>>>> +struct panel_mipi_dbi_config {
>>>> +	/* Magic string: panel_mipi_dbi_magic */
>>>> +	u8 magic[15];
>>>> +
>>>> +	/* Config file format version */
>>>> +	u8 file_format_version;
>>>> +
>>>> +	/* Width in pixels */
>>>> +	__be16 width;
>>>> +	/* Height in pixels */
>>>> +	__be16 height;
>>>> +
>>>> +	/* Width in millimeters (optional) */
>>>> +	__be16 width_mm;
>>>> +	/* Height in millimeters (optional) */
>>>> +	__be16 height_mm;
>>>> +
>>>> +	/* X-axis panel offset */
>>>> +	__be16 x_offset;
>>>> +	/* Y-axis panel offset */
>>>> +	__be16 y_offset;
>>>> +
>>>> +	/* 4 pad bytes, must be zero */
>>>> +	u8 pad[4];
>>>> +
>>>> +	/*
>>>> +	 * Optional MIPI commands to execute when the display pipeline is enabled.
>>>> +	 * This can be used to configure the display controller.
>>>> +	 *
>>>> +	 * The commands are stored in a byte array with the format:
>>>> +	 *     command, num_parameters, [ parameter, ...], command, ...
>>>> +	 *
>>>> +	 * Some commands require a pause before the next command can be received.
>>>> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
>>>> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
>>>> +	 * Command Set where it has no parameters).
>>>> +	 *
>>>> +	 * Example:
>>>> +	 *     command 0x11
>>>> +	 *     sleep 120ms
>>>> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
>>>> +	 *     command 0x29
>>>> +	 *
>>>> +	 * Byte sequence:
>>>> +	 *     0x11 0x00
>>>> +	 *     0x00 0x01 0x78
>>>> +	 *     0xb1 0x03 0x01 0x2c 0x2d
>>>> +	 *     0x29 0x00
>>>> +	 */
>>>> +	u8 commands[];
>>>> +};
>>>
>>> I'm not really a fan of parsing raw data in the kernel. I guess we can't
>>> really avoid the introduction of a special case to sleep, but we already
>>> have dt properties for all of the other properties (but X and Y offset,
>>> maybe?)
>>>
>>> Maybe we should use those instead?
>>
>> I don't understand your reluctance to parsing data, lots of ioctls do
>> it.
> 
> The reluctance comes from the parsing itself: you need to have input
> validation, and it's hard to get right. The less we have, the easier it
> gets.
> 
>> And this data can only be loaded by root. What I like about having
>> these properties in the config file is that the binding becomes a
>> fallback binding that can actually be made to work without changing the
>> Device Tree.
>>
>> For arguments sake let's say tiny/st7735r.c was not built and we had
>> this node:
>>
>> display@0{
>> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
>> "panel-mipi-dbi-spi";
>> };
>>
>> It will still be possible to use this display without changing the
>> Device Tree. Just add a firmware/config file.
>>
>> Having the properties in DT it would have to look like this for the
>> fallback to work:
>>
>> display@0{
>> 	compatible = "jianda,jd-t18003-t01", "sitronix,st7735r",
>> "panel-mipi-dbi-spi";
>> 	panel-timing = {
>> 		hactive = <128>;
>> 		vactive = <128>;
>> 	};
>> 	width-mm = <25>;
>> 	height-mm = <26>;
>> 	x-offset = <2>;
>> 	y-offset = <3>;
>> };
>>
>> Is this important, I'm not sure. What do you think?
> 
> Parts of it is ergonomics I guess. We're used to having all those
> properties either in the DT or the driver, but here we introduce a new
> way that isn't done anywhere else.
> 
> And I don't see any real downside to putting it in the DT? It's going to
> be in an overlay, under the user's control anyway, right?
> 

Ok, I'll spin a new version using DT properties.

Noralf.

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Sam Ravnborg]

Hi Noralf,

> > 
> > Parts of it is ergonomics I guess. We're used to having all those
> > properties either in the DT or the driver, but here we introduce a new
> > way that isn't done anywhere else.
> > 
> > And I don't see any real downside to putting it in the DT? It's going to
> > be in an overlay, under the user's control anyway, right?
> > 
> 
> Ok, I'll spin a new version using DT properties.

I like this better than anything else as we then have everything in
a single place when we add a new panel and more.
I just recall some resistance to add such HW specific setup - but the
usecase here is kinda special.

Looks forward to see the updated patch!

	Sam

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Sam Ravnborg]

Hi Noralf,

> > 
> > Parts of it is ergonomics I guess. We're used to having all those
> > properties either in the DT or the driver, but here we introduce a new
> > way that isn't done anywhere else.
> > 
> > And I don't see any real downside to putting it in the DT? It's going to
> > be in an overlay, under the user's control anyway, right?
> > 
> 
> Ok, I'll spin a new version using DT properties.

I like this better than anything else as we then have everything in
a single place when we add a new panel and more.
I just recall some resistance to add such HW specific setup - but the
usecase here is kinda special.

Looks forward to see the updated patch!

	Sam

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Maxime Ripard]

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

Hi Sam,

On Wed, Feb 02, 2022 at 06:14:16PM +0100, Sam Ravnborg wrote:
> Hi Noralf,
> 
> > > 
> > > Parts of it is ergonomics I guess. We're used to having all those
> > > properties either in the DT or the driver, but here we introduce a new
> > > way that isn't done anywhere else.
> > > 
> > > And I don't see any real downside to putting it in the DT? It's going to
> > > be in an overlay, under the user's control anyway, right?
> > > 
> > 
> > Ok, I'll spin a new version using DT properties.
> 
> I like this better than anything else as we then have everything in
> a single place when we add a new panel and more.
> I just recall some resistance to add such HW specific setup - but the
> usecase here is kinda special.

The main issue was to put the initialisation sequence in the device
tree. We solved that with by using a firmware instead. Given that the
timings, size and so on are already used by multiple, widely used,
panels, it should work just fine

Maxime

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

Re: [PATCH v2 3/3] drm/panel: Add MIPI DBI compatible SPI driver

[Maxime Ripard]

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

Hi Sam,

On Wed, Feb 02, 2022 at 06:14:16PM +0100, Sam Ravnborg wrote:
> Hi Noralf,
> 
> > > 
> > > Parts of it is ergonomics I guess. We're used to having all those
> > > properties either in the DT or the driver, but here we introduce a new
> > > way that isn't done anywhere else.
> > > 
> > > And I don't see any real downside to putting it in the DT? It's going to
> > > be in an overlay, under the user's control anyway, right?
> > > 
> > 
> > Ok, I'll spin a new version using DT properties.
> 
> I like this better than anything else as we then have everything in
> a single place when we add a new panel and more.
> I just recall some resistance to add such HW specific setup - but the
> usecase here is kinda special.

The main issue was to put the initialisation sequence in the device
tree. We solved that with by using a firmware instead. Given that the
timings, size and so on are already used by multiple, widely used,
panels, it should work just fine

Maxime

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

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Rob Herring]

On Thu, Jan 27, 2022 at 10:37:22AM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
> > Add binding for MIPI DBI compatible SPI panels.
> > 
> > v2:
> > - Fix path for panel-common.yaml
> > - Use unevaluatedProperties
> > - Drop properties which are in the allOf section
> > - Drop model property (Rob)
> > 
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> >  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> > new file mode 100644
> > index 000000000000..b7cbeea0f8aa
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MIPI DBI SPI Panels Device Tree Bindings
> > +
> > +maintainers:
> > +  - Noralf Trønnes <noralf@tronnes.org>
> > +
> > +description:
> > +  This binding is for display panels using a MIPI DBI controller
> > +  in SPI mode.
> > +
> > +allOf:
> > +  - $ref: panel-common.yaml#
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: panel-mipi-dbi-spi
> 
> You need contains here, otherwise it will error out if you have two compatibles.

Shouldn't it always have 2?

Either way, this has to be split up between a common, shareable schema 
and specific, complete schema(s). Like this:

- A schema for everything common (that allows additional properties)

- A schema for 'panel-mipi-dbi-spi' referencing the common schema plus 
  'unevaluatedProperties: false'

- Schemas for panels with their own additional properties (regulators, 
  GPIOs, etc.)

LVDS was restructured like this IIRC.

> > +  write-only:
> > +    type: boolean
> > +    description:
> > +      Controller is not readable (ie. MISO is not wired up).
> > +
> > +  dc-gpios:
> > +    maxItems: 1
> > +    description: |
> > +      Controller data/command selection (D/CX) in 4-line SPI mode.
> > +      If not set, the controller is in 3-line SPI mode.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    spi {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            display@0{
> > +                    compatible = "panel-mipi-dbi-spi";
> 
> We should have two compatibles in the example too
> 
> Maxime

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Rob Herring]

On Thu, Jan 27, 2022 at 10:37:22AM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
> > Add binding for MIPI DBI compatible SPI panels.
> > 
> > v2:
> > - Fix path for panel-common.yaml
> > - Use unevaluatedProperties
> > - Drop properties which are in the allOf section
> > - Drop model property (Rob)
> > 
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> >  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> > new file mode 100644
> > index 000000000000..b7cbeea0f8aa
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MIPI DBI SPI Panels Device Tree Bindings
> > +
> > +maintainers:
> > +  - Noralf Trønnes <noralf@tronnes.org>
> > +
> > +description:
> > +  This binding is for display panels using a MIPI DBI controller
> > +  in SPI mode.
> > +
> > +allOf:
> > +  - $ref: panel-common.yaml#
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: panel-mipi-dbi-spi
> 
> You need contains here, otherwise it will error out if you have two compatibles.

Shouldn't it always have 2?

Either way, this has to be split up between a common, shareable schema 
and specific, complete schema(s). Like this:

- A schema for everything common (that allows additional properties)

- A schema for 'panel-mipi-dbi-spi' referencing the common schema plus 
  'unevaluatedProperties: false'

- Schemas for panels with their own additional properties (regulators, 
  GPIOs, etc.)

LVDS was restructured like this IIRC.

> > +  write-only:
> > +    type: boolean
> > +    description:
> > +      Controller is not readable (ie. MISO is not wired up).
> > +
> > +  dc-gpios:
> > +    maxItems: 1
> > +    description: |
> > +      Controller data/command selection (D/CX) in 4-line SPI mode.
> > +      If not set, the controller is in 3-line SPI mode.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    spi {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            display@0{
> > +                    compatible = "panel-mipi-dbi-spi";
> 
> We should have two compatibles in the example too
> 
> Maxime

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Noralf Trønnes]

Den 08.02.2022 00.20, skrev Rob Herring:
> On Thu, Jan 27, 2022 at 10:37:22AM +0100, Maxime Ripard wrote:
>> Hi,
>>
>> On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
>>> Add binding for MIPI DBI compatible SPI panels.
>>>
>>> v2:
>>> - Fix path for panel-common.yaml
>>> - Use unevaluatedProperties
>>> - Drop properties which are in the allOf section
>>> - Drop model property (Rob)
>>>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>>>  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
>>>  1 file changed, 59 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>> new file mode 100644
>>> index 000000000000..b7cbeea0f8aa
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>> @@ -0,0 +1,59 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: MIPI DBI SPI Panels Device Tree Bindings
>>> +
>>> +maintainers:
>>> +  - Noralf Trønnes <noralf@tronnes.org>
>>> +
>>> +description:
>>> +  This binding is for display panels using a MIPI DBI controller
>>> +  in SPI mode.
>>> +
>>> +allOf:
>>> +  - $ref: panel-common.yaml#
>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: panel-mipi-dbi-spi
>>
>> You need contains here, otherwise it will error out if you have two compatibles.
> 
> Shouldn't it always have 2?
> 
> Either way, this has to be split up between a common, shareable schema 
> and specific, complete schema(s). Like this:
> 
> - A schema for everything common (that allows additional properties)
> 
> - A schema for 'panel-mipi-dbi-spi' referencing the common schema plus 
>   'unevaluatedProperties: false'
> 
> - Schemas for panels with their own additional properties (regulators, 
>   GPIOs, etc.)
> 
> LVDS was restructured like this IIRC.
> 

The whole point of this exercise is to avoid the need for controller
specific bindings. This binding will cover all specifics about these
controllers except for one thing and that is the controller
configuration. Each controller has its own configuration commands. These
commands will be loaded as a firmware file based on the compatible and
applied by the driver.

So this binding, the panel-common and spi-peripheral-props covers
everything except for the controller configuration.

Here's a copy of the DBI spec: https://www.docin.com/p-219732497.html

This is my current version of the binding:

# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: MIPI DBI SPI Panel

maintainers:
  - Noralf Trønnes <noralf@tronnes.org>

description: |
  This binding is for display panels using a MIPI DBI compatible controller
  in SPI mode.

  The MIPI Alliance Standard for Display Bus Interface defines the
electrical
  and logical interfaces for display controllers historically used in mobile
  phones. The standard defines 4 display architecture types and this
binding is
  for type 1 which has full frame memory. There are 3 interface types in the
  standard and type C is the serial interface.

  The standard defines the following interface signals for type C:
  - Power:
    - Vdd: Power supply for display module
    - Vddi: Logic level supply for interface signals
    Combined into one in this binding called: power-supply
  - Interface:
    - CSx: Chip select
    - SCL: Serial clock
    - Dout: Serial out
    - Din: Serial in
    - SDA: Bidrectional in/out
    - D/CX: Data/command selection, high=data, low=command
      Called dc-gpios in this binding.
    - RESX: Reset when low
      Called reset-gpios in this binding.

  The type C interface has 3 options:

    - Option 1: 9-bit mode and D/CX as the 9th bit
      |              Command              |  the next command or
following data  |

|<0><D7><D6><D5><D4><D3><D2><D1><D0>|<D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|

    - Option 2: 16-bit mode and D/CX as a 9th bit
      |              Command or data                              |
      |<X><X><X><X><X><X><X><D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|

    - Option 3: 8-bit mode and D/CX as a separate interface line
      |        Command or data         |
      |<D7><D6><D5><D4><D3><D2><D1><D0>|

  The panel resolution is specified using the panel-timing node properties
  hactive (width) and vactive (height). The other mandatory panel-timing
  properties should be set to zero except clock-frequency which can be
  optionally set to inform about the actual pixel clock frequency.

  If the panel is wired to the controller at an offset specify this using
  hback-porch (x-offset) and vback-porch (y-offset).

allOf:
  - $ref: panel-common.yaml#
  - $ref: /schemas/spi/spi-peripheral-props.yaml#

properties:
  compatible:
    contains:
      const: panel-dbi-spi

  write-only:
    type: boolean
    description:
      Controller is not readable (ie. MISO is not wired up).

  dc-gpios:
    maxItems: 1
    description: |
      Controller data/command selection (D/CX) in 4-line SPI mode.
      If not set, the controller is in 3-line SPI mode.

required:
  - compatible
  - reg
  - panel-timing

unevaluatedProperties: false

examples:
  - |
    #include <dt-bindings/gpio/gpio.h>

    spi {
            #address-cells = <1>;
            #size-cells = <0>;

            display@0{
                    compatible = "sainsmart18", "panel-dbi-spi";
                    reg = <0>;
                    spi-max-frequency = <40000000>;

                    dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
                    reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
                    write-only;

                    backlight = <&backlight>;

                    width-mm = <35>;
                    height-mm = <28>;

                    panel-timing {
                        hactive = <160>;
                        vactive = <128>;
                        hback-porch = <0>;
                        vback-porch = <0>;

                        clock-frequency = <0>;
                        hfront-porch = <0>;
                        hsync-len = <0>;
                        vfront-porch = <0>;
                        vsync-len = <0>;
                    };
            };
    };

...


>>> +  write-only:
>>> +    type: boolean
>>> +    description:
>>> +      Controller is not readable (ie. MISO is not wired up).
>>> +
>>> +  dc-gpios:
>>> +    maxItems: 1
>>> +    description: |
>>> +      Controller data/command selection (D/CX) in 4-line SPI mode.
>>> +      If not set, the controller is in 3-line SPI mode.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/gpio/gpio.h>
>>> +
>>> +    spi {
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +
>>> +            display@0{
>>> +                    compatible = "panel-mipi-dbi-spi";
>>
>> We should have two compatibles in the example too
>>
>> Maxime
> 
> 

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Noralf Trønnes]

Den 08.02.2022 00.20, skrev Rob Herring:
> On Thu, Jan 27, 2022 at 10:37:22AM +0100, Maxime Ripard wrote:
>> Hi,
>>
>> On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
>>> Add binding for MIPI DBI compatible SPI panels.
>>>
>>> v2:
>>> - Fix path for panel-common.yaml
>>> - Use unevaluatedProperties
>>> - Drop properties which are in the allOf section
>>> - Drop model property (Rob)
>>>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>>>  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
>>>  1 file changed, 59 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>> new file mode 100644
>>> index 000000000000..b7cbeea0f8aa
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>> @@ -0,0 +1,59 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: MIPI DBI SPI Panels Device Tree Bindings
>>> +
>>> +maintainers:
>>> +  - Noralf Trønnes <noralf@tronnes.org>
>>> +
>>> +description:
>>> +  This binding is for display panels using a MIPI DBI controller
>>> +  in SPI mode.
>>> +
>>> +allOf:
>>> +  - $ref: panel-common.yaml#
>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: panel-mipi-dbi-spi
>>
>> You need contains here, otherwise it will error out if you have two compatibles.
> 
> Shouldn't it always have 2?
> 
> Either way, this has to be split up between a common, shareable schema 
> and specific, complete schema(s). Like this:
> 
> - A schema for everything common (that allows additional properties)
> 
> - A schema for 'panel-mipi-dbi-spi' referencing the common schema plus 
>   'unevaluatedProperties: false'
> 
> - Schemas for panels with their own additional properties (regulators, 
>   GPIOs, etc.)
> 
> LVDS was restructured like this IIRC.
> 

The whole point of this exercise is to avoid the need for controller
specific bindings. This binding will cover all specifics about these
controllers except for one thing and that is the controller
configuration. Each controller has its own configuration commands. These
commands will be loaded as a firmware file based on the compatible and
applied by the driver.

So this binding, the panel-common and spi-peripheral-props covers
everything except for the controller configuration.

Here's a copy of the DBI spec: https://www.docin.com/p-219732497.html

This is my current version of the binding:

# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: MIPI DBI SPI Panel

maintainers:
  - Noralf Trønnes <noralf@tronnes.org>

description: |
  This binding is for display panels using a MIPI DBI compatible controller
  in SPI mode.

  The MIPI Alliance Standard for Display Bus Interface defines the
electrical
  and logical interfaces for display controllers historically used in mobile
  phones. The standard defines 4 display architecture types and this
binding is
  for type 1 which has full frame memory. There are 3 interface types in the
  standard and type C is the serial interface.

  The standard defines the following interface signals for type C:
  - Power:
    - Vdd: Power supply for display module
    - Vddi: Logic level supply for interface signals
    Combined into one in this binding called: power-supply
  - Interface:
    - CSx: Chip select
    - SCL: Serial clock
    - Dout: Serial out
    - Din: Serial in
    - SDA: Bidrectional in/out
    - D/CX: Data/command selection, high=data, low=command
      Called dc-gpios in this binding.
    - RESX: Reset when low
      Called reset-gpios in this binding.

  The type C interface has 3 options:

    - Option 1: 9-bit mode and D/CX as the 9th bit
      |              Command              |  the next command or
following data  |

|<0><D7><D6><D5><D4><D3><D2><D1><D0>|<D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|

    - Option 2: 16-bit mode and D/CX as a 9th bit
      |              Command or data                              |
      |<X><X><X><X><X><X><X><D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|

    - Option 3: 8-bit mode and D/CX as a separate interface line
      |        Command or data         |
      |<D7><D6><D5><D4><D3><D2><D1><D0>|

  The panel resolution is specified using the panel-timing node properties
  hactive (width) and vactive (height). The other mandatory panel-timing
  properties should be set to zero except clock-frequency which can be
  optionally set to inform about the actual pixel clock frequency.

  If the panel is wired to the controller at an offset specify this using
  hback-porch (x-offset) and vback-porch (y-offset).

allOf:
  - $ref: panel-common.yaml#
  - $ref: /schemas/spi/spi-peripheral-props.yaml#

properties:
  compatible:
    contains:
      const: panel-dbi-spi

  write-only:
    type: boolean
    description:
      Controller is not readable (ie. MISO is not wired up).

  dc-gpios:
    maxItems: 1
    description: |
      Controller data/command selection (D/CX) in 4-line SPI mode.
      If not set, the controller is in 3-line SPI mode.

required:
  - compatible
  - reg
  - panel-timing

unevaluatedProperties: false

examples:
  - |
    #include <dt-bindings/gpio/gpio.h>

    spi {
            #address-cells = <1>;
            #size-cells = <0>;

            display@0{
                    compatible = "sainsmart18", "panel-dbi-spi";
                    reg = <0>;
                    spi-max-frequency = <40000000>;

                    dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
                    reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
                    write-only;

                    backlight = <&backlight>;

                    width-mm = <35>;
                    height-mm = <28>;

                    panel-timing {
                        hactive = <160>;
                        vactive = <128>;
                        hback-porch = <0>;
                        vback-porch = <0>;

                        clock-frequency = <0>;
                        hfront-porch = <0>;
                        hsync-len = <0>;
                        vfront-porch = <0>;
                        vsync-len = <0>;
                    };
            };
    };

...


>>> +  write-only:
>>> +    type: boolean
>>> +    description:
>>> +      Controller is not readable (ie. MISO is not wired up).
>>> +
>>> +  dc-gpios:
>>> +    maxItems: 1
>>> +    description: |
>>> +      Controller data/command selection (D/CX) in 4-line SPI mode.
>>> +      If not set, the controller is in 3-line SPI mode.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/gpio/gpio.h>
>>> +
>>> +    spi {
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +
>>> +            display@0{
>>> +                    compatible = "panel-mipi-dbi-spi";
>>
>> We should have two compatibles in the example too
>>
>> Maxime
> 
> 

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Maxime Ripard]

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

Hi Noralf,

On Tue, Feb 08, 2022 at 01:16:44PM +0100, Noralf Trønnes wrote:
> Den 08.02.2022 00.20, skrev Rob Herring:
> > On Thu, Jan 27, 2022 at 10:37:22AM +0100, Maxime Ripard wrote:
> >> Hi,
> >>
> >> On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
> >>> Add binding for MIPI DBI compatible SPI panels.
> >>>
> >>> v2:
> >>> - Fix path for panel-common.yaml
> >>> - Use unevaluatedProperties
> >>> - Drop properties which are in the allOf section
> >>> - Drop model property (Rob)
> >>>
> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>> ---
> >>>  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
> >>>  1 file changed, 59 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> >>> new file mode 100644
> >>> index 000000000000..b7cbeea0f8aa
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> >>> @@ -0,0 +1,59 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: MIPI DBI SPI Panels Device Tree Bindings
> >>> +
> >>> +maintainers:
> >>> +  - Noralf Trønnes <noralf@tronnes.org>
> >>> +
> >>> +description:
> >>> +  This binding is for display panels using a MIPI DBI controller
> >>> +  in SPI mode.
> >>> +
> >>> +allOf:
> >>> +  - $ref: panel-common.yaml#
> >>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: panel-mipi-dbi-spi
> >>
> >> You need contains here, otherwise it will error out if you have two compatibles.
> > 
> > Shouldn't it always have 2?
> > 
> > Either way, this has to be split up between a common, shareable schema 
> > and specific, complete schema(s). Like this:
> > 
> > - A schema for everything common (that allows additional properties)
> > 
> > - A schema for 'panel-mipi-dbi-spi' referencing the common schema plus 
> >   'unevaluatedProperties: false'
> > 
> > - Schemas for panels with their own additional properties (regulators, 
> >   GPIOs, etc.)
> > 
> > LVDS was restructured like this IIRC.
>
> The whole point of this exercise is to avoid the need for controller
> specific bindings.

I'm not sure to follow you here, nothing that either Rob or I discussed
would require a controller specific binding.

It would require a controller compatible, but the binding itself can
just mandate a controller compatible in addition to the
panel-mipi-dbi-spi compatible, without enforcing anything wrt the
compatible itself.

And the driver will just match panel-mipi-dbi-spi so there won't be any
driver change either?

In essence, it would be similar to the bindings of panel-lvds or the
AT24 EEPROM binding: you have two compatibles, but the driver is generic
and will just infer its behaviour based on the DT properties (and in our
case will load a firmware based on the specific compatible).

Wouldn't that work?

> This binding will cover all specifics about these
> controllers except for one thing and that is the controller
> configuration. Each controller has its own configuration commands. These
> commands will be loaded as a firmware file based on the compatible and
> applied by the driver.
> 
> So this binding, the panel-common and spi-peripheral-props covers
> everything except for the controller configuration.
> 
> Here's a copy of the DBI spec: https://www.docin.com/p-219732497.html
> 
> This is my current version of the binding:
> 
> # SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> %YAML 1.2
> ---
> $id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
> 
> title: MIPI DBI SPI Panel
> 
> maintainers:
>   - Noralf Trønnes <noralf@tronnes.org>
> 
> description: |
>   This binding is for display panels using a MIPI DBI compatible controller
>   in SPI mode.
> 
>   The MIPI Alliance Standard for Display Bus Interface defines the
> electrical
>   and logical interfaces for display controllers historically used in mobile
>   phones. The standard defines 4 display architecture types and this
> binding is
>   for type 1 which has full frame memory. There are 3 interface types in the
>   standard and type C is the serial interface.
> 
>   The standard defines the following interface signals for type C:
>   - Power:
>     - Vdd: Power supply for display module
>     - Vddi: Logic level supply for interface signals
>     Combined into one in this binding called: power-supply
>   - Interface:
>     - CSx: Chip select
>     - SCL: Serial clock
>     - Dout: Serial out
>     - Din: Serial in
>     - SDA: Bidrectional in/out
>     - D/CX: Data/command selection, high=data, low=command
>       Called dc-gpios in this binding.
>     - RESX: Reset when low
>       Called reset-gpios in this binding.
> 
>   The type C interface has 3 options:
> 
>     - Option 1: 9-bit mode and D/CX as the 9th bit
>       |              Command              |  the next command or
> following data  |
> 
> |<0><D7><D6><D5><D4><D3><D2><D1><D0>|<D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
> 
>     - Option 2: 16-bit mode and D/CX as a 9th bit
>       |              Command or data                              |
>       |<X><X><X><X><X><X><X><D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
> 
>     - Option 3: 8-bit mode and D/CX as a separate interface line
>       |        Command or data         |
>       |<D7><D6><D5><D4><D3><D2><D1><D0>|
> 
>   The panel resolution is specified using the panel-timing node properties
>   hactive (width) and vactive (height). The other mandatory panel-timing
>   properties should be set to zero except clock-frequency which can be
>   optionally set to inform about the actual pixel clock frequency.
> 
>   If the panel is wired to the controller at an offset specify this using
>   hback-porch (x-offset) and vback-porch (y-offset).
> 
> allOf:
>   - $ref: panel-common.yaml#
>   - $ref: /schemas/spi/spi-peripheral-props.yaml#
> 
> properties:
>   compatible:
>     contains:
>       const: panel-dbi-spi
> 
>   write-only:
>     type: boolean
>     description:
>       Controller is not readable (ie. MISO is not wired up).
> 
>   dc-gpios:
>     maxItems: 1
>     description: |
>       Controller data/command selection (D/CX) in 4-line SPI mode.
>       If not set, the controller is in 3-line SPI mode.
> 
> required:
>   - compatible
>   - reg
>   - panel-timing
> 
> unevaluatedProperties: false
> 
> examples:
>   - |
>     #include <dt-bindings/gpio/gpio.h>
> 
>     spi {
>             #address-cells = <1>;
>             #size-cells = <0>;
> 
>             display@0{
>                     compatible = "sainsmart18", "panel-dbi-spi";
>                     reg = <0>;
>                     spi-max-frequency = <40000000>;
> 
>                     dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
>                     reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
>                     write-only;
> 
>                     backlight = <&backlight>;
> 
>                     width-mm = <35>;
>                     height-mm = <28>;
> 
>                     panel-timing {
>                         hactive = <160>;
>                         vactive = <128>;
>                         hback-porch = <0>;
>                         vback-porch = <0>;
> 
>                         clock-frequency = <0>;
>                         hfront-porch = <0>;
>                         hsync-len = <0>;
>                         vfront-porch = <0>;
>                         vsync-len = <0>;
>                     };
>             };
>     };
> 
> ...

Yep, this looks good to me

Maxime

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

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Maxime Ripard]

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

Hi Noralf,

On Tue, Feb 08, 2022 at 01:16:44PM +0100, Noralf Trønnes wrote:
> Den 08.02.2022 00.20, skrev Rob Herring:
> > On Thu, Jan 27, 2022 at 10:37:22AM +0100, Maxime Ripard wrote:
> >> Hi,
> >>
> >> On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
> >>> Add binding for MIPI DBI compatible SPI panels.
> >>>
> >>> v2:
> >>> - Fix path for panel-common.yaml
> >>> - Use unevaluatedProperties
> >>> - Drop properties which are in the allOf section
> >>> - Drop model property (Rob)
> >>>
> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>> ---
> >>>  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
> >>>  1 file changed, 59 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> >>> new file mode 100644
> >>> index 000000000000..b7cbeea0f8aa
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> >>> @@ -0,0 +1,59 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: MIPI DBI SPI Panels Device Tree Bindings
> >>> +
> >>> +maintainers:
> >>> +  - Noralf Trønnes <noralf@tronnes.org>
> >>> +
> >>> +description:
> >>> +  This binding is for display panels using a MIPI DBI controller
> >>> +  in SPI mode.
> >>> +
> >>> +allOf:
> >>> +  - $ref: panel-common.yaml#
> >>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: panel-mipi-dbi-spi
> >>
> >> You need contains here, otherwise it will error out if you have two compatibles.
> > 
> > Shouldn't it always have 2?
> > 
> > Either way, this has to be split up between a common, shareable schema 
> > and specific, complete schema(s). Like this:
> > 
> > - A schema for everything common (that allows additional properties)
> > 
> > - A schema for 'panel-mipi-dbi-spi' referencing the common schema plus 
> >   'unevaluatedProperties: false'
> > 
> > - Schemas for panels with their own additional properties (regulators, 
> >   GPIOs, etc.)
> > 
> > LVDS was restructured like this IIRC.
>
> The whole point of this exercise is to avoid the need for controller
> specific bindings.

I'm not sure to follow you here, nothing that either Rob or I discussed
would require a controller specific binding.

It would require a controller compatible, but the binding itself can
just mandate a controller compatible in addition to the
panel-mipi-dbi-spi compatible, without enforcing anything wrt the
compatible itself.

And the driver will just match panel-mipi-dbi-spi so there won't be any
driver change either?

In essence, it would be similar to the bindings of panel-lvds or the
AT24 EEPROM binding: you have two compatibles, but the driver is generic
and will just infer its behaviour based on the DT properties (and in our
case will load a firmware based on the specific compatible).

Wouldn't that work?

> This binding will cover all specifics about these
> controllers except for one thing and that is the controller
> configuration. Each controller has its own configuration commands. These
> commands will be loaded as a firmware file based on the compatible and
> applied by the driver.
> 
> So this binding, the panel-common and spi-peripheral-props covers
> everything except for the controller configuration.
> 
> Here's a copy of the DBI spec: https://www.docin.com/p-219732497.html
> 
> This is my current version of the binding:
> 
> # SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> %YAML 1.2
> ---
> $id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
> 
> title: MIPI DBI SPI Panel
> 
> maintainers:
>   - Noralf Trønnes <noralf@tronnes.org>
> 
> description: |
>   This binding is for display panels using a MIPI DBI compatible controller
>   in SPI mode.
> 
>   The MIPI Alliance Standard for Display Bus Interface defines the
> electrical
>   and logical interfaces for display controllers historically used in mobile
>   phones. The standard defines 4 display architecture types and this
> binding is
>   for type 1 which has full frame memory. There are 3 interface types in the
>   standard and type C is the serial interface.
> 
>   The standard defines the following interface signals for type C:
>   - Power:
>     - Vdd: Power supply for display module
>     - Vddi: Logic level supply for interface signals
>     Combined into one in this binding called: power-supply
>   - Interface:
>     - CSx: Chip select
>     - SCL: Serial clock
>     - Dout: Serial out
>     - Din: Serial in
>     - SDA: Bidrectional in/out
>     - D/CX: Data/command selection, high=data, low=command
>       Called dc-gpios in this binding.
>     - RESX: Reset when low
>       Called reset-gpios in this binding.
> 
>   The type C interface has 3 options:
> 
>     - Option 1: 9-bit mode and D/CX as the 9th bit
>       |              Command              |  the next command or
> following data  |
> 
> |<0><D7><D6><D5><D4><D3><D2><D1><D0>|<D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
> 
>     - Option 2: 16-bit mode and D/CX as a 9th bit
>       |              Command or data                              |
>       |<X><X><X><X><X><X><X><D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
> 
>     - Option 3: 8-bit mode and D/CX as a separate interface line
>       |        Command or data         |
>       |<D7><D6><D5><D4><D3><D2><D1><D0>|
> 
>   The panel resolution is specified using the panel-timing node properties
>   hactive (width) and vactive (height). The other mandatory panel-timing
>   properties should be set to zero except clock-frequency which can be
>   optionally set to inform about the actual pixel clock frequency.
> 
>   If the panel is wired to the controller at an offset specify this using
>   hback-porch (x-offset) and vback-porch (y-offset).
> 
> allOf:
>   - $ref: panel-common.yaml#
>   - $ref: /schemas/spi/spi-peripheral-props.yaml#
> 
> properties:
>   compatible:
>     contains:
>       const: panel-dbi-spi
> 
>   write-only:
>     type: boolean
>     description:
>       Controller is not readable (ie. MISO is not wired up).
> 
>   dc-gpios:
>     maxItems: 1
>     description: |
>       Controller data/command selection (D/CX) in 4-line SPI mode.
>       If not set, the controller is in 3-line SPI mode.
> 
> required:
>   - compatible
>   - reg
>   - panel-timing
> 
> unevaluatedProperties: false
> 
> examples:
>   - |
>     #include <dt-bindings/gpio/gpio.h>
> 
>     spi {
>             #address-cells = <1>;
>             #size-cells = <0>;
> 
>             display@0{
>                     compatible = "sainsmart18", "panel-dbi-spi";
>                     reg = <0>;
>                     spi-max-frequency = <40000000>;
> 
>                     dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
>                     reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
>                     write-only;
> 
>                     backlight = <&backlight>;
> 
>                     width-mm = <35>;
>                     height-mm = <28>;
> 
>                     panel-timing {
>                         hactive = <160>;
>                         vactive = <128>;
>                         hback-porch = <0>;
>                         vback-porch = <0>;
> 
>                         clock-frequency = <0>;
>                         hfront-porch = <0>;
>                         hsync-len = <0>;
>                         vfront-porch = <0>;
>                         vsync-len = <0>;
>                     };
>             };
>     };
> 
> ...

Yep, this looks good to me

Maxime

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

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Noralf Trønnes]

Den 09.02.2022 10.04, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Tue, Feb 08, 2022 at 01:16:44PM +0100, Noralf Trønnes wrote:
>> Den 08.02.2022 00.20, skrev Rob Herring:
>>> On Thu, Jan 27, 2022 at 10:37:22AM +0100, Maxime Ripard wrote:
>>>> Hi,
>>>>
>>>> On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
>>>>> Add binding for MIPI DBI compatible SPI panels.
>>>>>
>>>>> v2:
>>>>> - Fix path for panel-common.yaml
>>>>> - Use unevaluatedProperties
>>>>> - Drop properties which are in the allOf section
>>>>> - Drop model property (Rob)
>>>>>
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> ---
>>>>>  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
>>>>>  1 file changed, 59 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..b7cbeea0f8aa
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>>>> @@ -0,0 +1,59 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: MIPI DBI SPI Panels Device Tree Bindings
>>>>> +
>>>>> +maintainers:
>>>>> +  - Noralf Trønnes <noralf@tronnes.org>
>>>>> +
>>>>> +description:
>>>>> +  This binding is for display panels using a MIPI DBI controller
>>>>> +  in SPI mode.
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: panel-common.yaml#
>>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: panel-mipi-dbi-spi
>>>>
>>>> You need contains here, otherwise it will error out if you have two compatibles.
>>>
>>> Shouldn't it always have 2?
>>>
>>> Either way, this has to be split up between a common, shareable schema 
>>> and specific, complete schema(s). Like this:
>>>
>>> - A schema for everything common (that allows additional properties)
>>>
>>> - A schema for 'panel-mipi-dbi-spi' referencing the common schema plus 
>>>   'unevaluatedProperties: false'
>>>
>>> - Schemas for panels with their own additional properties (regulators, 
>>>   GPIOs, etc.)
>>>
>>> LVDS was restructured like this IIRC.
>>
>> The whole point of this exercise is to avoid the need for controller
>> specific bindings.
> 
> I'm not sure to follow you here, nothing that either Rob or I discussed
> would require a controller specific binding.
> 
> It would require a controller compatible, but the binding itself can
> just mandate a controller compatible in addition to the
> panel-mipi-dbi-spi compatible, without enforcing anything wrt the
> compatible itself.
> 
> And the driver will just match panel-mipi-dbi-spi so there won't be any
> driver change either?
> 
> In essence, it would be similar to the bindings of panel-lvds or the
> AT24 EEPROM binding: you have two compatibles, but the driver is generic
> and will just infer its behaviour based on the DT properties (and in our
> case will load a firmware based on the specific compatible).
> 
> Wouldn't that work?
> 

Oh, I misunderstood then.
I looked at the panel-lvds binding and since it didn't have any example
it looked like a schema that controllers/panels would include and not
something that would be used on its own.

>> This binding will cover all specifics about these
>> controllers except for one thing and that is the controller
>> configuration. Each controller has its own configuration commands. These
>> commands will be loaded as a firmware file based on the compatible and
>> applied by the driver.
>>
>> So this binding, the panel-common and spi-peripheral-props covers
>> everything except for the controller configuration.
>>
>> Here's a copy of the DBI spec: https://www.docin.com/p-219732497.html
>>
>> This is my current version of the binding:
>>
>> # SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>> %YAML 1.2
>> ---
>> $id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
>> $schema: http://devicetree.org/meta-schemas/core.yaml#
>>
>> title: MIPI DBI SPI Panel
>>
>> maintainers:
>>   - Noralf Trønnes <noralf@tronnes.org>
>>
>> description: |
>>   This binding is for display panels using a MIPI DBI compatible controller
>>   in SPI mode.
>>
>>   The MIPI Alliance Standard for Display Bus Interface defines the
>> electrical
>>   and logical interfaces for display controllers historically used in mobile
>>   phones. The standard defines 4 display architecture types and this
>> binding is
>>   for type 1 which has full frame memory. There are 3 interface types in the
>>   standard and type C is the serial interface.
>>
>>   The standard defines the following interface signals for type C:
>>   - Power:
>>     - Vdd: Power supply for display module
>>     - Vddi: Logic level supply for interface signals
>>     Combined into one in this binding called: power-supply
>>   - Interface:
>>     - CSx: Chip select
>>     - SCL: Serial clock
>>     - Dout: Serial out
>>     - Din: Serial in
>>     - SDA: Bidrectional in/out
>>     - D/CX: Data/command selection, high=data, low=command
>>       Called dc-gpios in this binding.
>>     - RESX: Reset when low
>>       Called reset-gpios in this binding.
>>
>>   The type C interface has 3 options:
>>
>>     - Option 1: 9-bit mode and D/CX as the 9th bit
>>       |              Command              |  the next command or
>> following data  |
>>
>> |<0><D7><D6><D5><D4><D3><D2><D1><D0>|<D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
>>
>>     - Option 2: 16-bit mode and D/CX as a 9th bit
>>       |              Command or data                              |
>>       |<X><X><X><X><X><X><X><D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
>>
>>     - Option 3: 8-bit mode and D/CX as a separate interface line
>>       |        Command or data         |
>>       |<D7><D6><D5><D4><D3><D2><D1><D0>|
>>
>>   The panel resolution is specified using the panel-timing node properties
>>   hactive (width) and vactive (height). The other mandatory panel-timing
>>   properties should be set to zero except clock-frequency which can be
>>   optionally set to inform about the actual pixel clock frequency.
>>
>>   If the panel is wired to the controller at an offset specify this using
>>   hback-porch (x-offset) and vback-porch (y-offset).
>>
>> allOf:
>>   - $ref: panel-common.yaml#
>>   - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>
>> properties:
>>   compatible:
>>     contains:
>>       const: panel-dbi-spi
>>
>>   write-only:
>>     type: boolean
>>     description:
>>       Controller is not readable (ie. MISO is not wired up).
>>
>>   dc-gpios:
>>     maxItems: 1
>>     description: |
>>       Controller data/command selection (D/CX) in 4-line SPI mode.
>>       If not set, the controller is in 3-line SPI mode.
>>
>> required:
>>   - compatible
>>   - reg
>>   - panel-timing
>>
>> unevaluatedProperties: false
>>
>> examples:
>>   - |
>>     #include <dt-bindings/gpio/gpio.h>
>>
>>     spi {
>>             #address-cells = <1>;
>>             #size-cells = <0>;
>>
>>             display@0{
>>                     compatible = "sainsmart18", "panel-dbi-spi";
>>                     reg = <0>;
>>                     spi-max-frequency = <40000000>;
>>
>>                     dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
>>                     reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
>>                     write-only;
>>
>>                     backlight = <&backlight>;
>>
>>                     width-mm = <35>;
>>                     height-mm = <28>;
>>
>>                     panel-timing {
>>                         hactive = <160>;
>>                         vactive = <128>;
>>                         hback-porch = <0>;
>>                         vback-porch = <0>;
>>
>>                         clock-frequency = <0>;
>>                         hfront-porch = <0>;
>>                         hsync-len = <0>;
>>                         vfront-porch = <0>;
>>                         vsync-len = <0>;
>>                     };
>>             };
>>     };
>>
>> ...
> 
> Yep, this looks good to me
> 

Thanks, I'll spin a new version.

Noralf.

Re: [PATCH v2 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Noralf Trønnes]

Den 09.02.2022 10.04, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Tue, Feb 08, 2022 at 01:16:44PM +0100, Noralf Trønnes wrote:
>> Den 08.02.2022 00.20, skrev Rob Herring:
>>> On Thu, Jan 27, 2022 at 10:37:22AM +0100, Maxime Ripard wrote:
>>>> Hi,
>>>>
>>>> On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
>>>>> Add binding for MIPI DBI compatible SPI panels.
>>>>>
>>>>> v2:
>>>>> - Fix path for panel-common.yaml
>>>>> - Use unevaluatedProperties
>>>>> - Drop properties which are in the allOf section
>>>>> - Drop model property (Rob)
>>>>>
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> ---
>>>>>  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
>>>>>  1 file changed, 59 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..b7cbeea0f8aa
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>>>> @@ -0,0 +1,59 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: MIPI DBI SPI Panels Device Tree Bindings
>>>>> +
>>>>> +maintainers:
>>>>> +  - Noralf Trønnes <noralf@tronnes.org>
>>>>> +
>>>>> +description:
>>>>> +  This binding is for display panels using a MIPI DBI controller
>>>>> +  in SPI mode.
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: panel-common.yaml#
>>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: panel-mipi-dbi-spi
>>>>
>>>> You need contains here, otherwise it will error out if you have two compatibles.
>>>
>>> Shouldn't it always have 2?
>>>
>>> Either way, this has to be split up between a common, shareable schema 
>>> and specific, complete schema(s). Like this:
>>>
>>> - A schema for everything common (that allows additional properties)
>>>
>>> - A schema for 'panel-mipi-dbi-spi' referencing the common schema plus 
>>>   'unevaluatedProperties: false'
>>>
>>> - Schemas for panels with their own additional properties (regulators, 
>>>   GPIOs, etc.)
>>>
>>> LVDS was restructured like this IIRC.
>>
>> The whole point of this exercise is to avoid the need for controller
>> specific bindings.
> 
> I'm not sure to follow you here, nothing that either Rob or I discussed
> would require a controller specific binding.
> 
> It would require a controller compatible, but the binding itself can
> just mandate a controller compatible in addition to the
> panel-mipi-dbi-spi compatible, without enforcing anything wrt the
> compatible itself.
> 
> And the driver will just match panel-mipi-dbi-spi so there won't be any
> driver change either?
> 
> In essence, it would be similar to the bindings of panel-lvds or the
> AT24 EEPROM binding: you have two compatibles, but the driver is generic
> and will just infer its behaviour based on the DT properties (and in our
> case will load a firmware based on the specific compatible).
> 
> Wouldn't that work?
> 

Oh, I misunderstood then.
I looked at the panel-lvds binding and since it didn't have any example
it looked like a schema that controllers/panels would include and not
something that would be used on its own.

>> This binding will cover all specifics about these
>> controllers except for one thing and that is the controller
>> configuration. Each controller has its own configuration commands. These
>> commands will be loaded as a firmware file based on the compatible and
>> applied by the driver.
>>
>> So this binding, the panel-common and spi-peripheral-props covers
>> everything except for the controller configuration.
>>
>> Here's a copy of the DBI spec: https://www.docin.com/p-219732497.html
>>
>> This is my current version of the binding:
>>
>> # SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>> %YAML 1.2
>> ---
>> $id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
>> $schema: http://devicetree.org/meta-schemas/core.yaml#
>>
>> title: MIPI DBI SPI Panel
>>
>> maintainers:
>>   - Noralf Trønnes <noralf@tronnes.org>
>>
>> description: |
>>   This binding is for display panels using a MIPI DBI compatible controller
>>   in SPI mode.
>>
>>   The MIPI Alliance Standard for Display Bus Interface defines the
>> electrical
>>   and logical interfaces for display controllers historically used in mobile
>>   phones. The standard defines 4 display architecture types and this
>> binding is
>>   for type 1 which has full frame memory. There are 3 interface types in the
>>   standard and type C is the serial interface.
>>
>>   The standard defines the following interface signals for type C:
>>   - Power:
>>     - Vdd: Power supply for display module
>>     - Vddi: Logic level supply for interface signals
>>     Combined into one in this binding called: power-supply
>>   - Interface:
>>     - CSx: Chip select
>>     - SCL: Serial clock
>>     - Dout: Serial out
>>     - Din: Serial in
>>     - SDA: Bidrectional in/out
>>     - D/CX: Data/command selection, high=data, low=command
>>       Called dc-gpios in this binding.
>>     - RESX: Reset when low
>>       Called reset-gpios in this binding.
>>
>>   The type C interface has 3 options:
>>
>>     - Option 1: 9-bit mode and D/CX as the 9th bit
>>       |              Command              |  the next command or
>> following data  |
>>
>> |<0><D7><D6><D5><D4><D3><D2><D1><D0>|<D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
>>
>>     - Option 2: 16-bit mode and D/CX as a 9th bit
>>       |              Command or data                              |
>>       |<X><X><X><X><X><X><X><D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
>>
>>     - Option 3: 8-bit mode and D/CX as a separate interface line
>>       |        Command or data         |
>>       |<D7><D6><D5><D4><D3><D2><D1><D0>|
>>
>>   The panel resolution is specified using the panel-timing node properties
>>   hactive (width) and vactive (height). The other mandatory panel-timing
>>   properties should be set to zero except clock-frequency which can be
>>   optionally set to inform about the actual pixel clock frequency.
>>
>>   If the panel is wired to the controller at an offset specify this using
>>   hback-porch (x-offset) and vback-porch (y-offset).
>>
>> allOf:
>>   - $ref: panel-common.yaml#
>>   - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>
>> properties:
>>   compatible:
>>     contains:
>>       const: panel-dbi-spi
>>
>>   write-only:
>>     type: boolean
>>     description:
>>       Controller is not readable (ie. MISO is not wired up).
>>
>>   dc-gpios:
>>     maxItems: 1
>>     description: |
>>       Controller data/command selection (D/CX) in 4-line SPI mode.
>>       If not set, the controller is in 3-line SPI mode.
>>
>> required:
>>   - compatible
>>   - reg
>>   - panel-timing
>>
>> unevaluatedProperties: false
>>
>> examples:
>>   - |
>>     #include <dt-bindings/gpio/gpio.h>
>>
>>     spi {
>>             #address-cells = <1>;
>>             #size-cells = <0>;
>>
>>             display@0{
>>                     compatible = "sainsmart18", "panel-dbi-spi";
>>                     reg = <0>;
>>                     spi-max-frequency = <40000000>;
>>
>>                     dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
>>                     reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
>>                     write-only;
>>
>>                     backlight = <&backlight>;
>>
>>                     width-mm = <35>;
>>                     height-mm = <28>;
>>
>>                     panel-timing {
>>                         hactive = <160>;
>>                         vactive = <128>;
>>                         hback-porch = <0>;
>>                         vback-porch = <0>;
>>
>>                         clock-frequency = <0>;
>>                         hfront-porch = <0>;
>>                         hsync-len = <0>;
>>                         vfront-porch = <0>;
>>                         vsync-len = <0>;
>>                     };
>>             };
>>     };
>>
>> ...
> 
> Yep, this looks good to me
> 

Thanks, I'll spin a new version.

Noralf.