[PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[Noralf Trønnes]

Hi,

This patchset adds a missing piece for decommissioning the
staging/fbtft/fb_st7735r.c driver namely a way to configure the
controller from Device Tree.

All fbtft drivers have builtin support for one display panel and all
other panels using that controller are configured using the Device Tree
'init' property. This property is supported by all fbtft drivers and
provides a generic way to set register values or issue commands
(depending on the type of controller).

It is common for these types of displays to have a datasheet listing the
necessary controller settings/commands or some example code doing the
same.

This is how the panel directly supported by the fb_st7735r staging
driver is described using Device Tree with that driver:

    width = <160>;
    height = <128>;

    init = <0x1000001
            0x2000096
            0x1000011
            0x20000ff
            0x10000B1 0x01 0x2C 0x2D
            0x10000B4 0x07
            0x10000C0 0xA2 0x02 0x84
            0x10000C1 0xC5
            0x10000C2 0x0A 0x00
            0x10000C5 0x0E
            0x100003a 0x55
            0x1000036 0x60
            0x10000E0 0x0F 0x1A 0x0F 0x18 0x2F 0x28 0x20 0x22
                      0x1F 0x1B 0x23 0x37 0x00 0x07 0x02 0x10
            0x10000E1 0x0F 0x1B 0x0F 0x17 0x33 0x2C 0x29 0x2E
                      0x30 0x30 0x39 0x3F 0x00 0x07 0x03 0x10
            0x1000029
            0x2000064>;


This is how the same panel is described using the st7735r drm driver and
this patchset:

    width = <160>;
    height = <128>;

    frmctr1 = [ 01 2C 2D ];
    invctr = [ 07 ];
    pwctr1 = [ A2 02 84 ];
    pwctr2 = [ C5 ];
    pwctr3 = [ 0A 00 ];
    vmctr1 = [ 0E ];
    madctl = [ 60 ];
    gamctrp1 = [ 0F 1A 0F 18 2F 28 20 22 1F 1B 23 37 00 07 02 10 ];
    gamctrn1 = [ 0F 1B 0F 17 33 2C 29 2E 30 30 39 3F 00 07 03 10 ];


Back when the fbtft drivers were added to staging I asked on the DT
mailinglist if it was OK to use the 'init' property but it was turned
down. In this patchset I'm trying the same approach used by the
solomon,ssd1307fb.yaml binding in describing the attached panel. That
binding prefixes the properties with the vendor name, not sure why that
is and I think it looks strange so I try without it.

Noralf.


Noralf Trønnes (6):
  dt-bindings: display: sitronix,st7735r: Fix backlight in example
  dt-bindings: display: sitronix,st7735r: Make reset-gpios optional
  dt-bindings: display: sitronix,st7735r: Remove spi-max-frequency limit
  dt-bindings: display: sitronix,st7735r: Add initialization properties
  drm/mipi-dbi: Add device property functions
  drm: tiny: st7735r: Support DT initialization of controller

 .../bindings/display/sitronix,st7735r.yaml    | 122 ++++++++++++++-
 drivers/gpu/drm/drm_mipi_dbi.c                | 139 ++++++++++++++++++
 drivers/gpu/drm/tiny/st7735r.c                |  87 +++++++++--
 include/drm/drm_mipi_dbi.h                    |   3 +
 4 files changed, 334 insertions(+), 17 deletions(-)

-- 
2.33.0

[PATCH 1/6] dt-bindings: display: sitronix,st7735r: Fix backlight in example

[Noralf Trønnes]

The backlight property was lost during conversion to yaml in commit
abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT schema").
Put it back.

Fixes: abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT schema")
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/devicetree/bindings/display/sitronix,st7735r.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
index 0cebaaefda03..419c3b2ac5a6 100644
--- a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
+++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
@@ -72,6 +72,7 @@ examples:
                     dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
                     reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
                     rotation = <270>;
+                    backlight = <&backlight>;
             };
     };
 
-- 
2.33.0

[PATCH 2/6] dt-bindings: display: sitronix,st7735r: Make reset-gpios optional

[Noralf Trønnes]

There are other ways than using a gpio to reset the controller so make
this property optional.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/devicetree/bindings/display/sitronix,st7735r.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
index 419c3b2ac5a6..f81d0d0d51fe 100644
--- a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
+++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
@@ -48,7 +48,6 @@ required:
   - compatible
   - reg
   - dc-gpios
-  - reset-gpios
 
 additionalProperties: false
 
-- 
2.33.0

[PATCH 3/6] dt-bindings: display: sitronix,st7735r: Remove spi-max-frequency limit

[Noralf Trønnes]

The datasheet lists the minimum Serial clock cycle (Write) as 66ns which is
15MHz. Mostly it can do much better than that and is in fact often run at
32MHz. With a clever driver that runs configuration commands at a low speed
and only the pixel data at the maximum speed the configuration can't be
messed up by transfer errors and the speed is only limited by the amount of
pixel glitches that one is able to tolerate.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 .../devicetree/bindings/display/sitronix,st7735r.yaml         | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
index f81d0d0d51fe..157b1a7b18f9 100644
--- a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
+++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
@@ -32,15 +32,13 @@ properties:
               - okaya,rh128128t
           - const: sitronix,st7715r
 
-  spi-max-frequency:
-    maximum: 32000000
-
   dc-gpios:
     maxItems: 1
     description: Display data/command selection (D/CX)
 
   backlight: true
   reg: true
+  spi-max-frequency: true
   reset-gpios: true
   rotation: true
 
-- 
2.33.0

[PATCH 4/6] dt-bindings: display: sitronix,st7735r: Add initialization properties

[Noralf Trønnes]

Add initialization properties that are commonly used to initialize the
controller for a specific display panel. It is common for displays to have
a datasheet listing the necessary controller settings or some example code
doing the same. These settings can be matched directly to the DT
properties.

The commands FRMCTR2, FRMCTR3, PWCTR4 and PWCTR5 are usually part of the
setup examples but they are skipped here since they deal with partial and
idle mode which are powersaving modes for very special use cases.

dc-gpios is made optional because its absence indicates 3-line mode.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 .../bindings/display/sitronix,st7735r.yaml    | 118 +++++++++++++++++-
 1 file changed, 116 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
index 157b1a7b18f9..2db1cfe6ae30 100644
--- a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
+++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
@@ -19,6 +19,10 @@ allOf:
 properties:
   compatible:
     oneOf:
+      - description:
+          Sitronix ST7735R 262K Color Single-Chip TFT Controller/Driver
+        items:
+          - const: sitronix,st7735r
       - description:
           Adafruit 1.8" 160x128 Color TFT LCD (Product ID 358 or 618)
         items:
@@ -32,20 +36,99 @@ properties:
               - okaya,rh128128t
           - const: sitronix,st7715r
 
+  width:
+    description:
+      Width of display panel in pixels
+
+  height:
+    description:
+      Height of display panel in pixels
+
+  frmctr1:
+    $ref: /schemas/types.yaml#definitions/uint8-array
+    description:
+      Frame Rate Control (In normal mode/Full colors) (B1h)
+    minItems: 3
+    maxItems: 3
+
+  invctr:
+    $ref: /schemas/types.yaml#definitions/uint8-array
+    description:
+      Display Inversion Control (B4h)
+    minItems: 1
+    maxItems: 1
+
+  pwctr1:
+    $ref: /schemas/types.yaml#definitions/uint8-array
+    description:
+      Power Control 1 (C0h)
+    minItems: 3
+    maxItems: 3
+
+  pwctr2:
+    $ref: /schemas/types.yaml#definitions/uint8-array
+    description:
+      Power Control 2 (C1h)
+    minItems: 1
+    maxItems: 1
+
+  pwctr3:
+    $ref: /schemas/types.yaml#definitions/uint8-array
+    description:
+      Power Control 3 (in Normal mode/Full colors) (C2h)
+    minItems: 2
+    maxItems: 2
+
+  vmctr1:
+    $ref: /schemas/types.yaml#definitions/uint8-array
+    description:
+      VCOM Control 1 (C5h)
+    minItems: 1
+    maxItems: 1
+
+  madctl:
+    $ref: /schemas/types.yaml#definitions/uint8-array
+    description:
+      Memory Data Access Control (36h)
+    minItems: 1
+    maxItems: 1
+
+  gamctrp1:
+    $ref: /schemas/types.yaml#definitions/uint8-array
+    description:
+      Gamma Positive Polarity Correction Characteristics Setting (E0h)
+    minItems: 16
+    maxItems: 16
+
+  gamctrn1:
+    $ref: /schemas/types.yaml#definitions/uint8-array
+    description:
+      Gamma Negative Polarity Correction Characteristics Setting (E1h)
+    minItems: 16
+    maxItems: 16
+
+  write-only:
+    type: boolean
+    description:
+      Controller is not readable (ie. MISO is not wired up).
+
   dc-gpios:
     maxItems: 1
-    description: Display data/command selection (D/CX)
+    description: |
+      Controller data/command selection (D/CX) in 4-line SPI mode.
+      If not set, the controller is in 3-line SPI mode.
 
   backlight: true
   reg: true
   spi-max-frequency: true
   reset-gpios: true
   rotation: true
+  width-mm: true
+  height-mm: true
 
 required:
   - compatible
   - reg
-  - dc-gpios
 
 additionalProperties: false
 
@@ -72,5 +155,36 @@ examples:
                     backlight = <&backlight>;
             };
     };
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            sainsmart18@0{
+                    compatible = "sitronix,st7735r";
+                    reg = <0>;
+                    spi-max-frequency = <40000000>;
+
+                    width = <160>;
+                    height = <128>;
+
+                    frmctr1 = [ 01 2C 2D ];
+                    invctr = [ 07 ];
+                    pwctr1 = [ A2 02 84 ];
+                    pwctr2 = [ C5 ];
+                    pwctr3 = [ 0A 00 ];
+                    vmctr1 = [ 0E ];
+                    madctl = [ 60 ];
+                    gamctrp1 = [ 0F 1A 0F 18 2F 28 20 22 1F 1B 23 37 00 07 02 10 ];
+                    gamctrn1 = [ 0F 1B 0F 17 33 2C 29 2E 30 30 39 3F 00 07 03 10 ];
+
+                    dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
+                    reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
+                    write-only;
+            };
+    };
+
 
 ...
-- 
2.33.0

[PATCH 5/6] drm/mipi-dbi: Add device property functions

[Noralf Trønnes]

Add helper functions for configuring a MIPI DBI controller from device
properties.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 139 +++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dbi.h     |   3 +
 2 files changed, 142 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 71b646c4131f..41362e1d4231 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -137,6 +137,24 @@ int mipi_dbi_command_read(struct mipi_dbi *dbi, u8 cmd, u8 *val)
 }
 EXPORT_SYMBOL(mipi_dbi_command_read);
 
+/**
+ * mipi_dbi_set_writeonly - Set the controller write only state
+ * @dbi: MIPI DBI structure
+ * @writeonly: If true the controller is not readable
+ *
+ * This function sets whether the controller can be read from or not (ie. MISO connected or not).
+ * It also checks the 'write-only' device property which overrides @writeonly.
+ * The controller is assumed to be readable by default.
+ */
+void mipi_dbi_set_writeonly(struct mipi_dbi *dbi, bool writeonly)
+{
+	struct device *dev = &dbi->spi->dev;
+
+	if (writeonly || device_property_present(dev, "write-only"))
+		dbi->read_commands = NULL;
+}
+EXPORT_SYMBOL(mipi_dbi_set_writeonly);
+
 /**
  * mipi_dbi_command_buf - MIPI DCS command with parameter(s) in an array
  * @dbi: MIPI DBI structure
@@ -186,6 +204,40 @@ int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
 }
 EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
 
+/**
+ * mipi_dbi_command_from_property - MIPI DCS command with parameter(s) from a device property
+ * @dbi: MIPI DBI structure
+ * @cmd: Command
+ * @propname: Name of the device property
+ * @len: Data length
+ *
+ * This function will execute @cmd with parameters from @propname if it exist.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int mipi_dbi_command_from_property(struct mipi_dbi *dbi, u8 cmd, const char *propname, size_t len)
+{
+	struct device *dev = &dbi->spi->dev;
+	u8 data[64];
+	int ret;
+
+	if (WARN_ON_ONCE(len > sizeof(data)))
+		return -EINVAL;
+
+	if (!device_property_present(dev, propname))
+		return 0;
+
+	ret = device_property_read_u8_array(dev, propname, data, len);
+	if (ret) {
+		dev_err(dev, "Failed to read property '%s', error=%d\n", propname, ret);
+		return ret;
+	}
+
+	return mipi_dbi_command_stackbuf(dbi, cmd, data, len);
+}
+EXPORT_SYMBOL(mipi_dbi_command_from_property);
+
 /**
  * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
  * @dst: The destination buffer
@@ -571,6 +623,93 @@ int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
 }
 EXPORT_SYMBOL(mipi_dbi_dev_init);
 
+static int mipi_dbi_property_read_u32(struct device *dev, const char *propname,
+				      unsigned int *retval, bool required)
+{
+	u32 val32;
+	int ret;
+
+	if (!device_property_present(dev, propname)) {
+		if (required) {
+			dev_err(dev, "Missing required property '%s'\n", propname);
+			return -EINVAL;
+		}
+
+		return 0;
+	}
+
+	ret = device_property_read_u32(dev, propname, &val32);
+	if (ret) {
+		dev_err(dev, "Error reading property '%s', error=%d\n", propname, ret);
+		return ret;
+	}
+
+	*retval = val32;
+
+	return 0;
+}
+
+static void mipi_dbi_simple_mode(struct drm_display_mode *mode,
+				 unsigned int width, unsigned int height,
+				 unsigned int width_mm, unsigned int height_mm)
+{
+	struct drm_display_mode simple_mode = { DRM_SIMPLE_MODE(width, height, width_mm, height_mm) };
+
+	*mode = simple_mode;
+}
+
+/**
+ * mipi_dbi_read_device_properties - Read device properties
+ * @dbidev: MIPI DBI device structure
+ * @mode: Returned display mode
+ *
+ * This function reads device properties 'width', 'height', 'width_mm', 'height_mm'
+ * and returns them as a display mode in @mode.
+ * It also reads 'x-offset' and 'y-offset' whose values are set on @dbidev.
+ *
+ * The returned @mode can be passed on to mipi_dbi_dev_init().
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int mipi_dbi_read_device_properties(struct mipi_dbi_dev *dbidev, struct drm_display_mode *mode)
+{
+	unsigned int width, height, width_mm = 0, height_mm = 0;
+	struct device *dev = dbidev->drm.dev;
+	int ret;
+
+	ret = mipi_dbi_property_read_u32(dev, "width", &width, true);
+	if (ret)
+		return ret;
+
+	ret = mipi_dbi_property_read_u32(dev, "height", &height, true);
+	if (ret)
+		return ret;
+
+	if (device_property_present(dev, "width_mm") || device_property_present(dev, "height_mm")) {
+		ret = mipi_dbi_property_read_u32(dev, "width_mm", &width_mm, true);
+		if (ret)
+			return ret;
+
+		ret = mipi_dbi_property_read_u32(dev, "height_mm", &height_mm, true);
+		if (ret)
+			return ret;
+	}
+
+	mipi_dbi_simple_mode(mode, width, height, width_mm, height_mm);
+
+	ret = mipi_dbi_property_read_u32(dev, "x-offset", &dbidev->left_offset, false);
+	if (ret)
+		return ret;
+
+	ret = mipi_dbi_property_read_u32(dev, "y-offset", &dbidev->top_offset, false);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dbi_read_device_properties);
+
 /**
  * mipi_dbi_hw_reset - Hardware reset of controller
  * @dbi: MIPI DBI structure
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 05e194958265..c75f760d6de5 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -147,6 +147,7 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev *dbidev,
 int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
 		      const struct drm_simple_display_pipe_funcs *funcs,
 		      const struct drm_display_mode *mode, unsigned int rotation);
+int mipi_dbi_read_device_properties(struct mipi_dbi_dev *dbidev, struct drm_display_mode *mode);
 void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 			  struct drm_plane_state *old_state);
 void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
@@ -163,9 +164,11 @@ int mipi_dbi_spi_transfer(struct spi_device *spi, u32 speed_hz,
 			  u8 bpw, const void *buf, size_t len);
 
 int mipi_dbi_command_read(struct mipi_dbi *dbi, u8 cmd, u8 *val);
+void mipi_dbi_set_writeonly(struct mipi_dbi *dbi, bool writeonly);
 int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t len);
 int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
 			      size_t len);
+int mipi_dbi_command_from_property(struct mipi_dbi *dbi, u8 cmd, const char *propname, size_t len);
 int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 		      struct drm_rect *clip, bool swap);
 /**
-- 
2.33.0

[PATCH 6/6] drm: tiny: st7735r: Support DT initialization of controller

[Noralf Trønnes]

Add support for initializing the controller from device properties when
the compatible is "sitronix,st7735r".

The rotation property does not apply in this case since a matching
ADDRESS_MODE/madctl value is necessary.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tiny/st7735r.c | 87 +++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c
index fc40dd10efa8..7f4d880b8702 100644
--- a/drivers/gpu/drm/tiny/st7735r.c
+++ b/drivers/gpu/drm/tiny/st7735r.c
@@ -58,6 +58,52 @@ struct st7735r_priv {
 static void st7735r_pipe_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 mipi_dbi *dbi = &dbidev->dbi;
+	int ret, idx;
+
+	if (!drm_dev_enter(pipe->crtc.dev, &idx))
+		return;
+
+	DRM_DEBUG_KMS("\n");
+
+	ret = mipi_dbi_poweron_conditional_reset(dbidev);
+	if (ret < 0)
+		goto out_exit;
+	if (ret == 1)
+		goto out_enable;
+
+	mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(120);
+
+	mipi_dbi_command_from_property(dbi, ST7735R_FRMCTR1, "frmctr1", 3);
+	mipi_dbi_command_from_property(dbi, ST7735R_INVCTR,  "invctr", 1);
+	mipi_dbi_command_from_property(dbi, ST7735R_PWCTR1,  "pwctr1", 3);
+	mipi_dbi_command_from_property(dbi, ST7735R_PWCTR2,  "pwctr2", 1);
+	mipi_dbi_command_from_property(dbi, ST7735R_PWCTR3,  "pwctr3", 2);
+	mipi_dbi_command_from_property(dbi, ST7735R_VMCTR1,  "vmctr1", 1);
+	mipi_dbi_command_from_property(dbi, MIPI_DCS_SET_ADDRESS_MODE, "madctl", 1);
+	mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_16BIT);
+	mipi_dbi_command_from_property(dbi, ST7735R_GAMCTRP1, "gamctrp1", 16);
+	mipi_dbi_command_from_property(dbi, ST7735R_GAMCTRN1, "gamctrn1", 16);
+
+	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
+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 st7735r_pipe_funcs = {
+	.enable		= st7735r_pipe_enable,
+	.disable	= mipi_dbi_pipe_disable,
+	.update		= mipi_dbi_pipe_update,
+};
+
+static void jd_t18003_t01_pipe_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 st7735r_priv *priv = container_of(dbidev, struct st7735r_priv,
@@ -132,8 +178,8 @@ static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
 	drm_dev_exit(idx);
 }
 
-static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
-	.enable		= st7735r_pipe_enable,
+static const struct drm_simple_display_pipe_funcs jd_t18003_t01_pipe_funcs = {
+	.enable		= jd_t18003_t01_pipe_enable,
 	.disable	= mipi_dbi_pipe_disable,
 	.update		= mipi_dbi_pipe_update,
 };
@@ -168,6 +214,7 @@ static const struct drm_driver st7735r_driver = {
 static const struct of_device_id st7735r_of_match[] = {
 	{ .compatible = "jianda,jd-t18003-t01", .data = &jd_t18003_t01_cfg },
 	{ .compatible = "okaya,rh128128t", .data = &rh128128t_cfg },
+	{ .compatible = "sitronix,st7735r" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, st7735r_of_match);
@@ -180,6 +227,9 @@ MODULE_DEVICE_TABLE(spi, st7735r_id);
 
 static int st7735r_probe(struct spi_device *spi)
 {
+	const struct drm_simple_display_pipe_funcs *funcs;
+	const struct drm_display_mode *mode;
+	struct drm_display_mode dt_mode;
 	struct device *dev = &spi->dev;
 	const struct st7735r_cfg *cfg;
 	struct mipi_dbi_dev *dbidev;
@@ -191,8 +241,12 @@ static int st7735r_probe(struct spi_device *spi)
 	int ret;
 
 	cfg = device_get_match_data(&spi->dev);
-	if (!cfg)
-		cfg = (void *)spi_get_device_id(spi)->driver_data;
+	if (!cfg) {
+		const struct spi_device_id *spi_id = spi_get_device_id(spi);
+
+		if (spi_id)
+			cfg = (struct st7735r_cfg *)spi_id->driver_data;
+	}
 
 	priv = devm_drm_dev_alloc(dev, &st7735r_driver,
 				  struct st7735r_priv, dbidev.drm);
@@ -217,20 +271,29 @@ static int st7735r_probe(struct spi_device *spi)
 	if (IS_ERR(dbidev->backlight))
 		return PTR_ERR(dbidev->backlight);
 
-	device_property_read_u32(dev, "rotation", &rotation);
-
 	ret = mipi_dbi_spi_init(spi, dbi, dc);
 	if (ret)
 		return ret;
 
-	if (cfg->write_only)
-		dbi->read_commands = NULL;
+	if (cfg) {
+		device_property_read_u32(dev, "rotation", &rotation);
 
-	dbidev->left_offset = cfg->left_offset;
-	dbidev->top_offset = cfg->top_offset;
+		mode = &cfg->mode;
+		funcs = &jd_t18003_t01_pipe_funcs;
+		dbidev->left_offset = cfg->left_offset;
+		dbidev->top_offset = cfg->top_offset;
+	} else {
+		ret = mipi_dbi_read_device_properties(dbidev, &dt_mode);
+		if (ret)
+			return ret;
 
-	ret = mipi_dbi_dev_init(dbidev, &st7735r_pipe_funcs, &cfg->mode,
-				rotation);
+		mode = &dt_mode;
+		funcs = &st7735r_pipe_funcs;
+	}
+
+	mipi_dbi_set_writeonly(dbi, cfg ? cfg->write_only : false);
+
+	ret = mipi_dbi_dev_init(dbidev, funcs, mode, rotation);
 	if (ret)
 		return ret;
 
-- 
2.33.0

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[David Lechner]

On 11/24/21 9:07 AM, Noralf Trønnes wrote:
> Hi,
> 
> This patchset adds a missing piece for decommissioning the
> staging/fbtft/fb_st7735r.c driver namely a way to configure the
> controller from Device Tree.
> 
> All fbtft drivers have builtin support for one display panel and all
> other panels using that controller are configured using the Device Tree
> 'init' property. This property is supported by all fbtft drivers and
> provides a generic way to set register values or issue commands
> (depending on the type of controller).
> 
> It is common for these types of displays to have a datasheet listing the
> necessary controller settings/commands or some example code doing the
> same.
> 
> This is how the panel directly supported by the fb_st7735r staging
> driver is described using Device Tree with that driver:
> 
>      width = <160>;
>      height = <128>;
> 
>      init = <0x1000001
>              0x2000096
>              0x1000011
>              0x20000ff
>              0x10000B1 0x01 0x2C 0x2D
>              0x10000B4 0x07
>              0x10000C0 0xA2 0x02 0x84
>              0x10000C1 0xC5
>              0x10000C2 0x0A 0x00
>              0x10000C5 0x0E
>              0x100003a 0x55
>              0x1000036 0x60
>              0x10000E0 0x0F 0x1A 0x0F 0x18 0x2F 0x28 0x20 0x22
>                        0x1F 0x1B 0x23 0x37 0x00 0x07 0x02 0x10
>              0x10000E1 0x0F 0x1B 0x0F 0x17 0x33 0x2C 0x29 0x2E
>                        0x30 0x30 0x39 0x3F 0x00 0x07 0x03 0x10
>              0x1000029
>              0x2000064>;
> 
> 
> This is how the same panel is described using the st7735r drm driver and
> this patchset:
> 
>      width = <160>;
>      height = <128>;
> 
>      frmctr1 = [ 01 2C 2D ];
>      invctr = [ 07 ];
>      pwctr1 = [ A2 02 84 ];
>      pwctr2 = [ C5 ];
>      pwctr3 = [ 0A 00 ];
>      vmctr1 = [ 0E ];
>      madctl = [ 60 ];
>      gamctrp1 = [ 0F 1A 0F 18 2F 28 20 22 1F 1B 23 37 00 07 02 10 ];
>      gamctrn1 = [ 0F 1B 0F 17 33 2C 29 2E 30 30 39 3F 00 07 03 10 ];

Do these setting correspond to actual physical properties of the display?

What is the advantage of this compared to just adding a new compatible
string if a new display requires different settings? (Other than being
able to use a new display without compiling a new kernel/module.)

It is nice for the driver implementation to be able to use the byte
arrays from the binding directly, but it doesn't really make sense from
a "device tree describes the hardware" point of view.

For example, looking at the data sheet, frmctr1 looks like it is actually
multiple properties, the 1-line period, front porch and back porch.

> 
> 
> Back when the fbtft drivers were added to staging I asked on the DT
> mailinglist if it was OK to use the 'init' property but it was turned
> down. In this patchset I'm trying the same approach used by the
> solomon,ssd1307fb.yaml binding in describing the attached panel. That
> binding prefixes the properties with the vendor name, not sure why that
> is and I think it looks strange so I try without it.

Because [1] says so?

"DO use a vendor prefix on device-specific property names. Consider if
properties could be common among devices of the same class. Check other
existing bindings for similar devices."

Do all displays have "frmctr1" or only sitronix displays?


[1]: https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html

> 
> Noralf.
> 
> 
> Noralf Trønnes (6):
>    dt-bindings: display: sitronix,st7735r: Fix backlight in example
>    dt-bindings: display: sitronix,st7735r: Make reset-gpios optional
>    dt-bindings: display: sitronix,st7735r: Remove spi-max-frequency limit
>    dt-bindings: display: sitronix,st7735r: Add initialization properties
>    drm/mipi-dbi: Add device property functions
>    drm: tiny: st7735r: Support DT initialization of controller
> 
>   .../bindings/display/sitronix,st7735r.yaml    | 122 ++++++++++++++-
>   drivers/gpu/drm/drm_mipi_dbi.c                | 139 ++++++++++++++++++
>   drivers/gpu/drm/tiny/st7735r.c                |  87 +++++++++--
>   include/drm/drm_mipi_dbi.h                    |   3 +
>   4 files changed, 334 insertions(+), 17 deletions(-)
> 

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[Noralf Trønnes]

Den 24.11.2021 23.03, skrev David Lechner:
> On 11/24/21 9:07 AM, Noralf Trønnes wrote:
>> Hi,
>>
>> This patchset adds a missing piece for decommissioning the
>> staging/fbtft/fb_st7735r.c driver namely a way to configure the
>> controller from Device Tree.
>>
>> All fbtft drivers have builtin support for one display panel and all
>> other panels using that controller are configured using the Device Tree
>> 'init' property. This property is supported by all fbtft drivers and
>> provides a generic way to set register values or issue commands
>> (depending on the type of controller).
>>
>> It is common for these types of displays to have a datasheet listing the
>> necessary controller settings/commands or some example code doing the
>> same.
>>
>> This is how the panel directly supported by the fb_st7735r staging
>> driver is described using Device Tree with that driver:
>>
>>      width = <160>;
>>      height = <128>;
>>
>>      init = <0x1000001
>>              0x2000096
>>              0x1000011
>>              0x20000ff
>>              0x10000B1 0x01 0x2C 0x2D
>>              0x10000B4 0x07
>>              0x10000C0 0xA2 0x02 0x84
>>              0x10000C1 0xC5
>>              0x10000C2 0x0A 0x00
>>              0x10000C5 0x0E
>>              0x100003a 0x55
>>              0x1000036 0x60
>>              0x10000E0 0x0F 0x1A 0x0F 0x18 0x2F 0x28 0x20 0x22
>>                        0x1F 0x1B 0x23 0x37 0x00 0x07 0x02 0x10
>>              0x10000E1 0x0F 0x1B 0x0F 0x17 0x33 0x2C 0x29 0x2E
>>                        0x30 0x30 0x39 0x3F 0x00 0x07 0x03 0x10
>>              0x1000029
>>              0x2000064>;
>>
>>
>> This is how the same panel is described using the st7735r drm driver and
>> this patchset:
>>
>>      width = <160>;
>>      height = <128>;
>>
>>      frmctr1 = [ 01 2C 2D ];
>>      invctr = [ 07 ];
>>      pwctr1 = [ A2 02 84 ];
>>      pwctr2 = [ C5 ];
>>      pwctr3 = [ 0A 00 ];
>>      vmctr1 = [ 0E ];
>>      madctl = [ 60 ];
>>      gamctrp1 = [ 0F 1A 0F 18 2F 28 20 22 1F 1B 23 37 00 07 02 10 ];
>>      gamctrn1 = [ 0F 1B 0F 17 33 2C 29 2E 30 30 39 3F 00 07 03 10 ];
> 
> Do these setting correspond to actual physical properties of the display?
> 

Apart from width, height, porches, freq and gamma, no not directly, they
configure voltage levels, op-amps (charge pumps?), dividers and such.

> What is the advantage of this compared to just adding a new compatible
> string if a new display requires different settings? (Other than being
> able to use a new display without compiling a new kernel/module.)
> 

There is no other reason, the purpose is simplicity for the end user,
which is one of the reasons for fbtft's success.

> It is nice for the driver implementation to be able to use the byte
> arrays from the binding directly, but it doesn't really make sense from
> a "device tree describes the hardware" point of view.
> 
> For example, looking at the data sheet, frmctr1 looks like it is actually
> multiple properties, the 1-line period, front porch and back porch.
> 

Yes, one command can have several 8-bit parameters and often configures
multiple things even within one parameter.

>>
>>
>> Back when the fbtft drivers were added to staging I asked on the DT
>> mailinglist if it was OK to use the 'init' property but it was turned
>> down. In this patchset I'm trying the same approach used by the
>> solomon,ssd1307fb.yaml binding in describing the attached panel. That
>> binding prefixes the properties with the vendor name, not sure why that
>> is and I think it looks strange so I try without it.
> 
> Because [1] says so?
> 
> "DO use a vendor prefix on device-specific property names. Consider if
> properties could be common among devices of the same class. Check other
> existing bindings for similar devices."
> 

That's a good reason :)

> Do all displays have "frmctr1" or only sitronix displays?
> 

ILI9341 also has that command but with only 2 parameters.
ST7789V calls it FRCTRL2 but has only one parameter.
The FPA and BPA fields from "frmctr1" looks like they're set using other
commands on ILI9341 and ST7789v.

I looked at several datasheets some years back to see if I could see
some kind of pattern, but I couldn't back then at least. Someone with
initimate hw knowledge of these controllers could probably describe a
controller using more generic properties.
This would defeat the purpose of this exercise which is to make it easy
to use any panel. Generic properties would require a set of formulas in
order to go from the init sequence provided by the display manufcaturer
to the generic properties.

The whole point of this patchset is to see if something like the ssd1307
binding can still be done in mainline making it easy for users.

If this doesn't work out, we can start removing drivers from
staging/fbtft since some of them haven't been removed even if the native
panel is supported in drm because they can support any panel through the
init property.

Noralf.

> 
> [1]:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html
> 
> 
>>
>> Noralf.
>>
>>
>> Noralf Trønnes (6):
>>    dt-bindings: display: sitronix,st7735r: Fix backlight in example
>>    dt-bindings: display: sitronix,st7735r: Make reset-gpios optional
>>    dt-bindings: display: sitronix,st7735r: Remove spi-max-frequency limit
>>    dt-bindings: display: sitronix,st7735r: Add initialization properties
>>    drm/mipi-dbi: Add device property functions
>>    drm: tiny: st7735r: Support DT initialization of controller
>>
>>   .../bindings/display/sitronix,st7735r.yaml    | 122 ++++++++++++++-
>>   drivers/gpu/drm/drm_mipi_dbi.c                | 139 ++++++++++++++++++
>>   drivers/gpu/drm/tiny/st7735r.c                |  87 +++++++++--
>>   include/drm/drm_mipi_dbi.h                    |   3 +
>>   4 files changed, 334 insertions(+), 17 deletions(-)
>>
> 

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[Maxime Ripard]

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

Hi,

On Wed, Nov 24, 2021 at 04:03:07PM -0600, David Lechner wrote:
> On 11/24/21 9:07 AM, Noralf Trønnes wrote:
> > This patchset adds a missing piece for decommissioning the
> > staging/fbtft/fb_st7735r.c driver namely a way to configure the
> > controller from Device Tree.
> > 
> > All fbtft drivers have builtin support for one display panel and all
> > other panels using that controller are configured using the Device Tree
> > 'init' property. This property is supported by all fbtft drivers and
> > provides a generic way to set register values or issue commands
> > (depending on the type of controller).
> > 
> > It is common for these types of displays to have a datasheet listing the
> > necessary controller settings/commands or some example code doing the
> > same.
> > 
> > This is how the panel directly supported by the fb_st7735r staging
> > driver is described using Device Tree with that driver:
> > 
> >      width = <160>;
> >      height = <128>;
> > 
> >      init = <0x1000001
> >              0x2000096
> >              0x1000011
> >              0x20000ff
> >              0x10000B1 0x01 0x2C 0x2D
> >              0x10000B4 0x07
> >              0x10000C0 0xA2 0x02 0x84
> >              0x10000C1 0xC5
> >              0x10000C2 0x0A 0x00
> >              0x10000C5 0x0E
> >              0x100003a 0x55
> >              0x1000036 0x60
> >              0x10000E0 0x0F 0x1A 0x0F 0x18 0x2F 0x28 0x20 0x22
> >                        0x1F 0x1B 0x23 0x37 0x00 0x07 0x02 0x10
> >              0x10000E1 0x0F 0x1B 0x0F 0x17 0x33 0x2C 0x29 0x2E
> >                        0x30 0x30 0x39 0x3F 0x00 0x07 0x03 0x10
> >              0x1000029
> >              0x2000064>;
> > 
> > 
> > This is how the same panel is described using the st7735r drm driver and
> > this patchset:
> > 
> >      width = <160>;
> >      height = <128>;
> > 
> >      frmctr1 = [ 01 2C 2D ];
> >      invctr = [ 07 ];
> >      pwctr1 = [ A2 02 84 ];
> >      pwctr2 = [ C5 ];
> >      pwctr3 = [ 0A 00 ];
> >      vmctr1 = [ 0E ];
> >      madctl = [ 60 ];
> >      gamctrp1 = [ 0F 1A 0F 18 2F 28 20 22 1F 1B 23 37 00 07 02 10 ];
> >      gamctrn1 = [ 0F 1B 0F 17 33 2C 29 2E 30 30 39 3F 00 07 03 10 ];
> 
> Do these setting correspond to actual physical properties of the display?
> 
> What is the advantage of this compared to just adding a new compatible
> string if a new display requires different settings? (Other than being
> able to use a new display without compiling a new kernel/module.)
>
> It is nice for the driver implementation to be able to use the byte
> arrays from the binding directly, but it doesn't really make sense from
> a "device tree describes the hardware" point of view.
> 
> For example, looking at the data sheet, frmctr1 looks like it is actually
> multiple properties, the 1-line period, front porch and back porch.

You're right, but we have two sets of problems that we want to solve,
and so far the discussion has only been to address one while ignoring
the other.

The solution you suggested works great for the problem the kernel is
facing: we want a solution that is easy to maintain over the long run,
while being reliable. Thus, we want to introduce a compatible for each
panel, that will allow us to describe the panel in the DT without
exposing too much data, the data being in the kernel.

This works great over the long run because we can update and fix any
problem we might have had, send them to stable, etc. It's awesome, but
it's mostly centered on us, the developers and maintainers.


The problem that fbtft (and this series) wants to fix is completely
different though: it wants to address the issue the users are facing.
Namely, you get a cheap display from wherever, connect it to your shiny
new SBC and wants to get something on the display.

In this situation, the user probably doesn't have the knowledge to
introduce the compatible in the kernel in the first place. But there's
also some technical barriers there: if they use secure boot, they can't
change the kernel (well, at least the knowledge required is far above
what we can expect from the average user). If the platform doesn't allow
access to the DT, you can't change the DT either.

Let's set aside those constraints for a moment though. For most of these
devices, you wouldn't even be able to come up with a proper compatible.
All of those displays are typically a panel and a controller glued
together, and the exact initialization sequence depends on both. The
panel is never really mentioned, neither is its manufacturer, or its
exact product id. In other words, we wouldn't be able to come up with a
good compatible for them.

Let's now assume we do have access to all those info and can come up
with a good, upstreamable, compatible. We now require the user to
contribute it upstream, and then expect them to wait for 1-2 years for
that patch to show up in their distribution of choice.

And then, if we were to get those patches, chances are we don't really
want them anyway since we would be drowning in those small patches
no-one really wants to review.


So yeah, the solution we have is probably a good solution for "real"
panels, glued to a device (and even then, the recent discussion around
panel-edp shows that it has a few shortcomings). But it's a *terrible*
solution for all parties involved when it comes to those kind of
displays.


I agree that it doesn't really fit in the DT either though. Noralf, what
kind of data do we need to setup a display in fbtft? The init sequence,
and maybe some enable/reset GPIO, plus some timing duration maybe?

There's one similar situation I can think of: wifi chips. Those also
need a few infos from the DT (like what bus it's connected to, enable
GPIO, etc) and a different sequence (firmware), sometimes different from
one board to the other.

Could we have a binding that would be something like:

panel@42 {
	 compatible = "panel-spi";
	 model = "panel-from-random-place-42";
	 enable-gpios = <&...>;
}

And then, the driver would request the init sequence through the
firmware mechanism using a name generated from the model property.

It allows to support multiple devices in a given system, since the
firmware name wouldn't conflict, it makes a decent binding, and users
can adjust the init sequence easily (maybe with a bit of tooling)

Would that work?

Maxime

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

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[Geert Uytterhoeven]

Hi Maxime,

On Mon, Nov 29, 2021 at 11:17 PM Maxime Ripard <maxime@cerno.tech> wrote:
> The problem that fbtft (and this series) wants to fix is completely
> different though: it wants to address the issue the users are facing.
> Namely, you get a cheap display from wherever, connect it to your shiny
> new SBC and wants to get something on the display.
>
> In this situation, the user probably doesn't have the knowledge to
> introduce the compatible in the kernel in the first place. But there's
> also some technical barriers there: if they use secure boot, they can't
> change the kernel (well, at least the knowledge required is far above
> what we can expect from the average user). If the platform doesn't allow

If you can change the DT, you can introduce a vulnerability to change
the kernel ;-)

> access to the DT, you can't change the DT either.

How do people connect a cheap display from wherever to their shiny
new SBC and make it work, without modifying DT?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[Maxime Ripard]

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

Hi Geert,

On Tue, Nov 30, 2021 at 09:13:45AM +0100, Geert Uytterhoeven wrote:
> On Mon, Nov 29, 2021 at 11:17 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > The problem that fbtft (and this series) wants to fix is completely
> > different though: it wants to address the issue the users are facing.
> > Namely, you get a cheap display from wherever, connect it to your shiny
> > new SBC and wants to get something on the display.
> >
> > In this situation, the user probably doesn't have the knowledge to
> > introduce the compatible in the kernel in the first place. But there's
> > also some technical barriers there: if they use secure boot, they can't
> > change the kernel (well, at least the knowledge required is far above
> > what we can expect from the average user). If the platform doesn't allow
> 
> If you can change the DT, you can introduce a vulnerability to change
> the kernel ;-)

Indeed

> > access to the DT, you can't change the DT either.
> 
> How do people connect a cheap display from wherever to their shiny
> new SBC and make it work, without modifying DT?

Through overlays, usually. I guess it would still qualify as "DT", but
it's not the main DT

And the other issues remain the same: while the DT could be "easily"
patched, the kernel certainly isn't and we need both with the current
expectations.

Maxime

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

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[Noralf Trønnes]

Den 29.11.2021 10.39, skrev Maxime Ripard:
> Hi,
> 
> On Wed, Nov 24, 2021 at 04:03:07PM -0600, David Lechner wrote:
>> On 11/24/21 9:07 AM, Noralf Trønnes wrote:
>>> This patchset adds a missing piece for decommissioning the
>>> staging/fbtft/fb_st7735r.c driver namely a way to configure the
>>> controller from Device Tree.
>>>
>>> All fbtft drivers have builtin support for one display panel and all
>>> other panels using that controller are configured using the Device Tree
>>> 'init' property. This property is supported by all fbtft drivers and
>>> provides a generic way to set register values or issue commands
>>> (depending on the type of controller).
>>>
>>> It is common for these types of displays to have a datasheet listing the
>>> necessary controller settings/commands or some example code doing the
>>> same.
>>>
>>> This is how the panel directly supported by the fb_st7735r staging
>>> driver is described using Device Tree with that driver:
>>>
>>>      width = <160>;
>>>      height = <128>;
>>>
>>>      init = <0x1000001
>>>              0x2000096
>>>              0x1000011
>>>              0x20000ff
>>>              0x10000B1 0x01 0x2C 0x2D
>>>              0x10000B4 0x07
>>>              0x10000C0 0xA2 0x02 0x84
>>>              0x10000C1 0xC5
>>>              0x10000C2 0x0A 0x00
>>>              0x10000C5 0x0E
>>>              0x100003a 0x55
>>>              0x1000036 0x60
>>>              0x10000E0 0x0F 0x1A 0x0F 0x18 0x2F 0x28 0x20 0x22
>>>                        0x1F 0x1B 0x23 0x37 0x00 0x07 0x02 0x10
>>>              0x10000E1 0x0F 0x1B 0x0F 0x17 0x33 0x2C 0x29 0x2E
>>>                        0x30 0x30 0x39 0x3F 0x00 0x07 0x03 0x10
>>>              0x1000029
>>>              0x2000064>;
>>>
>>>
>>> This is how the same panel is described using the st7735r drm driver and
>>> this patchset:
>>>
>>>      width = <160>;
>>>      height = <128>;
>>>
>>>      frmctr1 = [ 01 2C 2D ];
>>>      invctr = [ 07 ];
>>>      pwctr1 = [ A2 02 84 ];
>>>      pwctr2 = [ C5 ];
>>>      pwctr3 = [ 0A 00 ];
>>>      vmctr1 = [ 0E ];
>>>      madctl = [ 60 ];
>>>      gamctrp1 = [ 0F 1A 0F 18 2F 28 20 22 1F 1B 23 37 00 07 02 10 ];
>>>      gamctrn1 = [ 0F 1B 0F 17 33 2C 29 2E 30 30 39 3F 00 07 03 10 ];
>>
>> Do these setting correspond to actual physical properties of the display?
>>
>> What is the advantage of this compared to just adding a new compatible
>> string if a new display requires different settings? (Other than being
>> able to use a new display without compiling a new kernel/module.)
>>
>> It is nice for the driver implementation to be able to use the byte
>> arrays from the binding directly, but it doesn't really make sense from
>> a "device tree describes the hardware" point of view.
>>
>> For example, looking at the data sheet, frmctr1 looks like it is actually
>> multiple properties, the 1-line period, front porch and back porch.
> 
> You're right, but we have two sets of problems that we want to solve,
> and so far the discussion has only been to address one while ignoring
> the other.
> 
> The solution you suggested works great for the problem the kernel is
> facing: we want a solution that is easy to maintain over the long run,
> while being reliable. Thus, we want to introduce a compatible for each
> panel, that will allow us to describe the panel in the DT without
> exposing too much data, the data being in the kernel.
> 
> This works great over the long run because we can update and fix any
> problem we might have had, send them to stable, etc. It's awesome, but
> it's mostly centered on us, the developers and maintainers.
> 
> 
> The problem that fbtft (and this series) wants to fix is completely
> different though: it wants to address the issue the users are facing.
> Namely, you get a cheap display from wherever, connect it to your shiny
> new SBC and wants to get something on the display.
> 
> In this situation, the user probably doesn't have the knowledge to
> introduce the compatible in the kernel in the first place. But there's
> also some technical barriers there: if they use secure boot, they can't
> change the kernel (well, at least the knowledge required is far above
> what we can expect from the average user). If the platform doesn't allow
> access to the DT, you can't change the DT either.
> 

Like Geert I wondered about this statement, since you need to change the
DT to use such a display. But if you count overlays as not changing the
DT, ok.

> Let's set aside those constraints for a moment though. For most of these
> devices, you wouldn't even be able to come up with a proper compatible.
> All of those displays are typically a panel and a controller glued
> together, and the exact initialization sequence depends on both. The
> panel is never really mentioned, neither is its manufacturer, or its
> exact product id. In other words, we wouldn't be able to come up with a
> good compatible for them.
> 
> Let's now assume we do have access to all those info and can come up
> with a good, upstreamable, compatible. We now require the user to
> contribute it upstream, and then expect them to wait for 1-2 years for
> that patch to show up in their distribution of choice.
> 
> And then, if we were to get those patches, chances are we don't really
> want them anyway since we would be drowning in those small patches
> no-one really wants to review.
> 
> 
> So yeah, the solution we have is probably a good solution for "real"
> panels, glued to a device (and even then, the recent discussion around
> panel-edp shows that it has a few shortcomings). But it's a *terrible*
> solution for all parties involved when it comes to those kind of
> displays.
> 

Really good writeup of the situation Maxime!

> 
> I agree that it doesn't really fit in the DT either though. Noralf, what
> kind of data do we need to setup a display in fbtft? The init sequence,
> and maybe some enable/reset GPIO, plus some timing duration maybe?
> 
> There's one similar situation I can think of: wifi chips. Those also
> need a few infos from the DT (like what bus it's connected to, enable
> GPIO, etc) and a different sequence (firmware), sometimes different from
> one board to the other.
> 
> Could we have a binding that would be something like:
> 
> panel@42 {
> 	 compatible = "panel-spi";
> 	 model = "panel-from-random-place-42";
> 	 enable-gpios = <&...>;
> }
> 
> And then, the driver would request the init sequence through the
> firmware mechanism using a name generated from the model property.
> 
> It allows to support multiple devices in a given system, since the
> firmware name wouldn't conflict, it makes a decent binding, and users
> can adjust the init sequence easily (maybe with a bit of tooling)
> 
> Would that work?
> 

I really like this idea. An added benefit is that one driver can handle
all MIPI DBI compatible controllers avoiding the need to do a patchset
like this for all the various MIPI DBI controllers. The firmware will
just contain numeric commands with parameters, so no need for different
controller drivers to handle the controller specific command names.

The following is a list of the MIPI DBI compatible controllers currently
in staging/fbtft: ili9341, hx8357d, st7735r, ili9163, ili9163, ili9163,
ili9163, ili9486, ili9481, tinylcd, s6d02a1, s6d02a1, hx8340bn, ili9340.

The compatible needs to be a bit more specific though since there are 2
major SPI protocols for these display: MIPI DBI and the one used by
ILI9325 and others.

The full binding would be something like this:

panel@42 {
	compatible = "panel-mipi-dbi-spi";
	model = "panel-from-random-place-42";

	/* The MIPI DBI spec lists these powers supply pins */
	vdd-supply = <&...>;
	vddi-supply = <&...>;

	/* Optional gpio to drive the RESX line */
	reset-gpios = <&...>;

	/*
	 * D/CX: Data/Command, Command is active low
	 * Abcense: Interface option 1 (D/C embedded in 9-bit word)
	 * Precense: Interface option 3
	 */
	dc-gpios = <&...>;

	/*
	 * If set the driver won't try to read from the controller to see
	 * if it's already configured by the bootloader or previously by
	 * the driver. A readable controller avoids flicker and/or delay
	 * enabling the pipeline.
	 *
	 * This property might not be necessary if we are guaranteed to
	 * always read back all 1's or 0's when MISO is not connected.
	 * I don't know if all setups can guarantee that.
	 */
	write-only;

	/* Optional ref to backlight node */
	backlight = <&...>;
}

Many of these controllers also have a RGB interface option for the
pixels and only do configuration over SPI.
Maybe the compatible should reflect these 2 options somehow?

Noralf.

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[Maxime Ripard]

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

Hi Noralf,

On Tue, Nov 30, 2021 at 03:30:11PM +0100, Noralf Trønnes wrote:
> Den 29.11.2021 10.39, skrev Maxime Ripard:
> > On Wed, Nov 24, 2021 at 04:03:07PM -0600, David Lechner wrote:
> >> On 11/24/21 9:07 AM, Noralf Trønnes wrote:
> > I agree that it doesn't really fit in the DT either though. Noralf, what
> > kind of data do we need to setup a display in fbtft? The init sequence,
> > and maybe some enable/reset GPIO, plus some timing duration maybe?
> > 
> > There's one similar situation I can think of: wifi chips. Those also
> > need a few infos from the DT (like what bus it's connected to, enable
> > GPIO, etc) and a different sequence (firmware), sometimes different from
> > one board to the other.
> > 
> > Could we have a binding that would be something like:
> > 
> > panel@42 {
> > 	 compatible = "panel-spi";
> > 	 model = "panel-from-random-place-42";
> > 	 enable-gpios = <&...>;
> > }
> > 
> > And then, the driver would request the init sequence through the
> > firmware mechanism using a name generated from the model property.
> > 
> > It allows to support multiple devices in a given system, since the
> > firmware name wouldn't conflict, it makes a decent binding, and users
> > can adjust the init sequence easily (maybe with a bit of tooling)
> > 
> > Would that work?
>
> I really like this idea. An added benefit is that one driver can handle
> all MIPI DBI compatible controllers avoiding the need to do a patchset
> like this for all the various MIPI DBI controllers. The firmware will
> just contain numeric commands with parameters, so no need for different
> controller drivers to handle the controller specific command names.
> 
> The following is a list of the MIPI DBI compatible controllers currently
> in staging/fbtft: ili9341, hx8357d, st7735r, ili9163, ili9163, ili9163,
> ili9163, ili9486, ili9481, tinylcd, s6d02a1, s6d02a1, hx8340bn, ili9340.
> 
> The compatible needs to be a bit more specific though since there are 2
> major SPI protocols for these display: MIPI DBI and the one used by
> ILI9325 and others.
> 
> The full binding would be something like this:
> 
> panel@42 {
> 	compatible = "panel-mipi-dbi-spi";
> 	model = "panel-from-random-place-42";
> 
> 	/* The MIPI DBI spec lists these powers supply pins */
> 	vdd-supply = <&...>;
> 	vddi-supply = <&...>;
> 
> 	/* Optional gpio to drive the RESX line */
> 	reset-gpios = <&...>;
> 
> 	/*
> 	 * D/CX: Data/Command, Command is active low
> 	 * Abcense: Interface option 1 (D/C embedded in 9-bit word)
> 	 * Precense: Interface option 3
> 	 */
> 	dc-gpios = <&...>;
> 
> 	/*
> 	 * If set the driver won't try to read from the controller to see
> 	 * if it's already configured by the bootloader or previously by
> 	 * the driver. A readable controller avoids flicker and/or delay
> 	 * enabling the pipeline.
> 	 *
> 	 * This property might not be necessary if we are guaranteed to
> 	 * always read back all 1's or 0's when MISO is not connected.
> 	 * I don't know if all setups can guarantee that.
> 	 */
> 	write-only;
> 
> 	/* Optional ref to backlight node */
> 	backlight = <&...>;
> }

It looks decent to me. We'll want Rob to give his opinion though, but it
looks in a much better shape compared to what we usually have :)

> Many of these controllers also have a RGB interface option for the
> pixels and only do configuration over SPI.
> Maybe the compatible should reflect these 2 options somehow?

I think we'll want a "real" panel for RGB, with its own compatible
though. We have a few of these drivers in tree already, so it's better
to remain consistent.

Maxime

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

Re: [PATCH 1/6] dt-bindings: display: sitronix,st7735r: Fix backlight in example

[Rob Herring]

On Wed, 24 Nov 2021 16:07:52 +0100, Noralf Trønnes wrote:
> The backlight property was lost during conversion to yaml in commit
> abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT schema").
> Put it back.
> 
> Fixes: abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT schema")
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  Documentation/devicetree/bindings/display/sitronix,st7735r.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/6] dt-bindings: display: sitronix,st7735r: Make reset-gpios optional

[Rob Herring]

On Wed, 24 Nov 2021 16:07:53 +0100, Noralf Trønnes wrote:
> There are other ways than using a gpio to reset the controller so make
> this property optional.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  Documentation/devicetree/bindings/display/sitronix,st7735r.yaml | 1 -
>  1 file changed, 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 3/6] dt-bindings: display: sitronix,st7735r: Remove spi-max-frequency limit

[Rob Herring]

On Wed, 24 Nov 2021 16:07:54 +0100, Noralf Trønnes wrote:
> The datasheet lists the minimum Serial clock cycle (Write) as 66ns which is
> 15MHz. Mostly it can do much better than that and is in fact often run at
> 32MHz. With a clever driver that runs configuration commands at a low speed
> and only the pixel data at the maximum speed the configuration can't be
> messed up by transfer errors and the speed is only limited by the amount of
> pixel glitches that one is able to tolerate.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  .../devicetree/bindings/display/sitronix,st7735r.yaml         | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 4/6] dt-bindings: display: sitronix,st7735r: Add initialization properties

[Rob Herring]

On Wed, Nov 24, 2021 at 04:07:55PM +0100, Noralf Trønnes wrote:
> Add initialization properties that are commonly used to initialize the
> controller for a specific display panel. It is common for displays to have
> a datasheet listing the necessary controller settings or some example code
> doing the same. These settings can be matched directly to the DT
> properties.
> 
> The commands FRMCTR2, FRMCTR3, PWCTR4 and PWCTR5 are usually part of the
> setup examples but they are skipped here since they deal with partial and
> idle mode which are powersaving modes for very special use cases.
> 
> dc-gpios is made optional because its absence indicates 3-line mode.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  .../bindings/display/sitronix,st7735r.yaml    | 118 +++++++++++++++++-
>  1 file changed, 116 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
> index 157b1a7b18f9..2db1cfe6ae30 100644
> --- a/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
> +++ b/Documentation/devicetree/bindings/display/sitronix,st7735r.yaml
> @@ -19,6 +19,10 @@ allOf:
>  properties:
>    compatible:
>      oneOf:
> +      - description:
> +          Sitronix ST7735R 262K Color Single-Chip TFT Controller/Driver
> +        items:
> +          - const: sitronix,st7735r
>        - description:
>            Adafruit 1.8" 160x128 Color TFT LCD (Product ID 358 or 618)
>          items:
> @@ -32,20 +36,99 @@ properties:
>                - okaya,rh128128t
>            - const: sitronix,st7715r
>  
> +  width:
> +    description:
> +      Width of display panel in pixels
> +
> +  height:
> +    description:
> +      Height of display panel in pixels


We already have width-mm and height-mm for physical size so this might 
be a bit confusing. There's also panel-timing 'vactive' and 'hactive' 
which is effectively the same thing you are defining.

> +
> +  frmctr1:

Are all these standardized by MIPI or otherwise common? If not, they 
need vendor prefixes.

> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +    description:
> +      Frame Rate Control (In normal mode/Full colors) (B1h)
> +    minItems: 3
> +    maxItems: 3
> +
> +  invctr:
> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +    description:
> +      Display Inversion Control (B4h)
> +    minItems: 1
> +    maxItems: 1
> +
> +  pwctr1:
> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +    description:
> +      Power Control 1 (C0h)
> +    minItems: 3
> +    maxItems: 3
> +
> +  pwctr2:
> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +    description:
> +      Power Control 2 (C1h)
> +    minItems: 1
> +    maxItems: 1
> +
> +  pwctr3:
> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +    description:
> +      Power Control 3 (in Normal mode/Full colors) (C2h)
> +    minItems: 2
> +    maxItems: 2
> +
> +  vmctr1:
> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +    description:
> +      VCOM Control 1 (C5h)
> +    minItems: 1
> +    maxItems: 1
> +
> +  madctl:
> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +    description:
> +      Memory Data Access Control (36h)
> +    minItems: 1
> +    maxItems: 1
> +
> +  gamctrp1:
> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +    description:
> +      Gamma Positive Polarity Correction Characteristics Setting (E0h)
> +    minItems: 16
> +    maxItems: 16
> +
> +  gamctrn1:
> +    $ref: /schemas/types.yaml#definitions/uint8-array
> +    description:
> +      Gamma Negative Polarity Correction Characteristics Setting (E1h)
> +    minItems: 16
> +    maxItems: 16
> +
> +  write-only:
> +    type: boolean
> +    description:
> +      Controller is not readable (ie. MISO is not wired up).
> +
>    dc-gpios:
>      maxItems: 1
> -    description: Display data/command selection (D/CX)
> +    description: |
> +      Controller data/command selection (D/CX) in 4-line SPI mode.
> +      If not set, the controller is in 3-line SPI mode.
>  
>    backlight: true
>    reg: true
>    spi-max-frequency: true
>    reset-gpios: true
>    rotation: true
> +  width-mm: true
> +  height-mm: true
>  
>  required:
>    - compatible
>    - reg
> -  - dc-gpios
>  
>  additionalProperties: false
>  
> @@ -72,5 +155,36 @@ examples:
>                      backlight = <&backlight>;
>              };
>      };
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            sainsmart18@0{
> +                    compatible = "sitronix,st7735r";
> +                    reg = <0>;
> +                    spi-max-frequency = <40000000>;
> +
> +                    width = <160>;
> +                    height = <128>;
> +
> +                    frmctr1 = [ 01 2C 2D ];
> +                    invctr = [ 07 ];
> +                    pwctr1 = [ A2 02 84 ];
> +                    pwctr2 = [ C5 ];
> +                    pwctr3 = [ 0A 00 ];
> +                    vmctr1 = [ 0E ];
> +                    madctl = [ 60 ];
> +                    gamctrp1 = [ 0F 1A 0F 18 2F 28 20 22 1F 1B 23 37 00 07 02 10 ];
> +                    gamctrn1 = [ 0F 1B 0F 17 33 2C 29 2E 30 30 39 3F 00 07 03 10 ];
> +
> +                    dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> +                    reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> +                    write-only;
> +            };
> +    };
> +
>  
>  ...
> -- 
> 2.33.0
> 
> 

Re: [PATCH 1/6] dt-bindings: display: sitronix,st7735r: Fix backlight in example

[Geert Uytterhoeven]

On Thu, Nov 25, 2021 at 4:17 PM Noralf Trønnes <noralf@tronnes.org> wrote:
> The backlight property was lost during conversion to yaml in commit
> abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT schema").
> Put it back.
>
> Fixes: abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT schema")
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Mea culpa
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] dt-bindings: display: sitronix,st7735r: Fix backlight in example

[David Lechner]

On 11/24/21 9:07 AM, Noralf Trønnes wrote:
> The backlight property was lost during conversion to yaml in commit
> abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT schema").
> Put it back.
> 
> Fixes: abdd9e3705c8 ("dt-bindings: display: sitronix,st7735r: Convert to DT schema")
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

Acked-by: David Lechner <david@lechnology.com>

Re: [PATCH 2/6] dt-bindings: display: sitronix,st7735r: Make reset-gpios optional

[David Lechner]

On 11/24/21 9:07 AM, Noralf Trønnes wrote:
> There are other ways than using a gpio to reset the controller so make
> this property optional.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
Acked-by: David Lechner <david@lechnology.com>

Re: [PATCH 3/6] dt-bindings: display: sitronix,st7735r: Remove spi-max-frequency limit

[David Lechner]

On 11/24/21 9:07 AM, Noralf Trønnes wrote:
> The datasheet lists the minimum Serial clock cycle (Write) as 66ns which is

Is this supposed to say "maximum" rather than "minimum"?

> 15MHz. Mostly it can do much better than that and is in fact often run at
> 32MHz. With a clever driver that runs configuration commands at a low speed
> and only the pixel data at the maximum speed the configuration can't be
> messed up by transfer errors and the speed is only limited by the amount of
> pixel glitches that one is able to tolerate.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
Acked-by: David Lechner <david@lechnology.com>

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[David Lechner]

On 12/1/21 8:52 AM, Maxime Ripard wrote:
> Hi Noralf,
> 
> On Tue, Nov 30, 2021 at 03:30:11PM +0100, Noralf Trønnes wrote:
>> Den 29.11.2021 10.39, skrev Maxime Ripard:
>>> On Wed, Nov 24, 2021 at 04:03:07PM -0600, David Lechner wrote:
>>>> On 11/24/21 9:07 AM, Noralf Trønnes wrote:
>>> I agree that it doesn't really fit in the DT either though. Noralf, what
>>> kind of data do we need to setup a display in fbtft? The init sequence,
>>> and maybe some enable/reset GPIO, plus some timing duration maybe?
>>>
>>> There's one similar situation I can think of: wifi chips. Those also
>>> need a few infos from the DT (like what bus it's connected to, enable
>>> GPIO, etc) and a different sequence (firmware), sometimes different from
>>> one board to the other.
>>>
>>> Could we have a binding that would be something like:
>>>
>>> panel@42 {
>>> 	 compatible = "panel-spi";
>>> 	 model = "panel-from-random-place-42";
>>> 	 enable-gpios = <&...>;
>>> }
>>>
>>> And then, the driver would request the init sequence through the
>>> firmware mechanism using a name generated from the model property.
>>>
>>> It allows to support multiple devices in a given system, since the
>>> firmware name wouldn't conflict, it makes a decent binding, and users
>>> can adjust the init sequence easily (maybe with a bit of tooling)
>>>
>>> Would that work?
>>
>> I really like this idea. An added benefit is that one driver can handle
>> all MIPI DBI compatible controllers avoiding the need to do a patchset
>> like this for all the various MIPI DBI controllers. The firmware will
>> just contain numeric commands with parameters, so no need for different
>> controller drivers to handle the controller specific command names.
>>
>> The following is a list of the MIPI DBI compatible controllers currently
>> in staging/fbtft: ili9341, hx8357d, st7735r, ili9163, ili9163, ili9163,
>> ili9163, ili9486, ili9481, tinylcd, s6d02a1, s6d02a1, hx8340bn, ili9340.
>>
>> The compatible needs to be a bit more specific though since there are 2
>> major SPI protocols for these display: MIPI DBI and the one used by
>> ILI9325 and others.
>>
>> The full binding would be something like this:
>>
>> panel@42 {
>> 	compatible = "panel-mipi-dbi-spi";
>> 	model = "panel-from-random-place-42";
>>
>> 	/* The MIPI DBI spec lists these powers supply pins */
>> 	vdd-supply = <&...>;
>> 	vddi-supply = <&...>;
>>
>> 	/* Optional gpio to drive the RESX line */
>> 	reset-gpios = <&...>;
>>
>> 	/*
>> 	 * D/CX: Data/Command, Command is active low
>> 	 * Abcense: Interface option 1 (D/C embedded in 9-bit word)
>> 	 * Precense: Interface option 3
>> 	 */
>> 	dc-gpios = <&...>;
>>
>> 	/*
>> 	 * If set the driver won't try to read from the controller to see
>> 	 * if it's already configured by the bootloader or previously by
>> 	 * the driver. A readable controller avoids flicker and/or delay
>> 	 * enabling the pipeline.
>> 	 *
>> 	 * This property might not be necessary if we are guaranteed to
>> 	 * always read back all 1's or 0's when MISO is not connected.
>> 	 * I don't know if all setups can guarantee that.
>> 	 */
>> 	write-only;
>>
>> 	/* Optional ref to backlight node */
>> 	backlight = <&...>;
>> }
> 
> It looks decent to me. We'll want Rob to give his opinion though, but it
> looks in a much better shape compared to what we usually have :)
> 
>> Many of these controllers also have a RGB interface option for the
>> pixels and only do configuration over SPI.
>> Maybe the compatible should reflect these 2 options somehow?
> 
> I think we'll want a "real" panel for RGB, with its own compatible
> though. We have a few of these drivers in tree already, so it's better
> to remain consistent.
> 
> Maxime
> 

I'm on board with the idea of the init sequence as firmware as well.

It looks like Rob might have missed this thread, so maybe just apply
the acked patches and submit a v2 with the firmware implementation?

Re: [PATCH 3/6] dt-bindings: display: sitronix,st7735r: Remove spi-max-frequency limit

[Noralf Trønnes]

Den 06.12.2021 16.19, skrev David Lechner:
> On 11/24/21 9:07 AM, Noralf Trønnes wrote:
>> The datasheet lists the minimum Serial clock cycle (Write) as 66ns
>> which is
> 
> Is this supposed to say "maximum" rather than "minimum"?
> 

Minimum cycle time == maximum frequency.

Noralf.

>> 15MHz. Mostly it can do much better than that and is in fact often run at
>> 32MHz. With a clever driver that runs configuration commands at a low
>> speed
>> and only the pixel data at the maximum speed the configuration can't be
>> messed up by transfer errors and the speed is only limited by the
>> amount of
>> pixel glitches that one is able to tolerate.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
> Acked-by: David Lechner <david@lechnology.com>

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[Noralf Trønnes]

Den 06.12.2021 16.26, skrev David Lechner:
> On 12/1/21 8:52 AM, Maxime Ripard wrote:
>> Hi Noralf,
>>
>> On Tue, Nov 30, 2021 at 03:30:11PM +0100, Noralf Trønnes wrote:
>>> Den 29.11.2021 10.39, skrev Maxime Ripard:
>>>> On Wed, Nov 24, 2021 at 04:03:07PM -0600, David Lechner wrote:
>>>>> On 11/24/21 9:07 AM, Noralf Trønnes wrote:
>>>> I agree that it doesn't really fit in the DT either though. Noralf,
>>>> what
>>>> kind of data do we need to setup a display in fbtft? The init sequence,
>>>> and maybe some enable/reset GPIO, plus some timing duration maybe?
>>>>
>>>> There's one similar situation I can think of: wifi chips. Those also
>>>> need a few infos from the DT (like what bus it's connected to, enable
>>>> GPIO, etc) and a different sequence (firmware), sometimes different
>>>> from
>>>> one board to the other.
>>>>
>>>> Could we have a binding that would be something like:
>>>>
>>>> panel@42 {
>>>>      compatible = "panel-spi";
>>>>      model = "panel-from-random-place-42";
>>>>      enable-gpios = <&...>;
>>>> }
>>>>
>>>> And then, the driver would request the init sequence through the
>>>> firmware mechanism using a name generated from the model property.
>>>>
>>>> It allows to support multiple devices in a given system, since the
>>>> firmware name wouldn't conflict, it makes a decent binding, and users
>>>> can adjust the init sequence easily (maybe with a bit of tooling)
>>>>
>>>> Would that work?
>>>
>>> I really like this idea. An added benefit is that one driver can handle
>>> all MIPI DBI compatible controllers avoiding the need to do a patchset
>>> like this for all the various MIPI DBI controllers. The firmware will
>>> just contain numeric commands with parameters, so no need for different
>>> controller drivers to handle the controller specific command names.
>>>
>>> The following is a list of the MIPI DBI compatible controllers currently
>>> in staging/fbtft: ili9341, hx8357d, st7735r, ili9163, ili9163, ili9163,
>>> ili9163, ili9486, ili9481, tinylcd, s6d02a1, s6d02a1, hx8340bn, ili9340.
>>>
>>> The compatible needs to be a bit more specific though since there are 2
>>> major SPI protocols for these display: MIPI DBI and the one used by
>>> ILI9325 and others.
>>>
>>> The full binding would be something like this:
>>>
>>> panel@42 {
>>>     compatible = "panel-mipi-dbi-spi";
>>>     model = "panel-from-random-place-42";
>>>
>>>     /* The MIPI DBI spec lists these powers supply pins */
>>>     vdd-supply = <&...>;
>>>     vddi-supply = <&...>;
>>>
>>>     /* Optional gpio to drive the RESX line */
>>>     reset-gpios = <&...>;
>>>
>>>     /*
>>>      * D/CX: Data/Command, Command is active low
>>>      * Abcense: Interface option 1 (D/C embedded in 9-bit word)
>>>      * Precense: Interface option 3
>>>      */
>>>     dc-gpios = <&...>;
>>>
>>>     /*
>>>      * If set the driver won't try to read from the controller to see
>>>      * if it's already configured by the bootloader or previously by
>>>      * the driver. A readable controller avoids flicker and/or delay
>>>      * enabling the pipeline.
>>>      *
>>>      * This property might not be necessary if we are guaranteed to
>>>      * always read back all 1's or 0's when MISO is not connected.
>>>      * I don't know if all setups can guarantee that.
>>>      */
>>>     write-only;
>>>
>>>     /* Optional ref to backlight node */
>>>     backlight = <&...>;
>>> }
>>
>> It looks decent to me. We'll want Rob to give his opinion though, but it
>> looks in a much better shape compared to what we usually have :)
>>
>>> Many of these controllers also have a RGB interface option for the
>>> pixels and only do configuration over SPI.
>>> Maybe the compatible should reflect these 2 options somehow?
>>
>> I think we'll want a "real" panel for RGB, with its own compatible
>> though. We have a few of these drivers in tree already, so it's better
>> to remain consistent.
>>
>> Maxime
>>
> 
> I'm on board with the idea of the init sequence as firmware as well.
> 
> It looks like Rob might have missed this thread, so maybe just apply
> the acked patches and submit a v2 with the firmware implementation?
> 

Yes, that's my plan.

Noralf.

Re: [PATCH 0/6] drm/tiny/st7735r: Match up with staging/fbtft driver

[Noralf Trønnes]

Den 24.11.2021 16.07, skrev Noralf Trønnes:
> Hi,
> 
> This patchset adds a missing piece for decommissioning the
> staging/fbtft/fb_st7735r.c driver namely a way to configure the
> controller from Device Tree.
> 
> All fbtft drivers have builtin support for one display panel and all
> other panels using that controller are configured using the Device Tree
> 'init' property. This property is supported by all fbtft drivers and
> provides a generic way to set register values or issue commands
> (depending on the type of controller).
> 
> It is common for these types of displays to have a datasheet listing the
> necessary controller settings/commands or some example code doing the
> same.
> 
> This is how the panel directly supported by the fb_st7735r staging
> driver is described using Device Tree with that driver:
> 
>     width = <160>;
>     height = <128>;
> 
>     init = <0x1000001
>             0x2000096
>             0x1000011
>             0x20000ff
>             0x10000B1 0x01 0x2C 0x2D
>             0x10000B4 0x07
>             0x10000C0 0xA2 0x02 0x84
>             0x10000C1 0xC5
>             0x10000C2 0x0A 0x00
>             0x10000C5 0x0E
>             0x100003a 0x55
>             0x1000036 0x60
>             0x10000E0 0x0F 0x1A 0x0F 0x18 0x2F 0x28 0x20 0x22
>                       0x1F 0x1B 0x23 0x37 0x00 0x07 0x02 0x10
>             0x10000E1 0x0F 0x1B 0x0F 0x17 0x33 0x2C 0x29 0x2E
>                       0x30 0x30 0x39 0x3F 0x00 0x07 0x03 0x10
>             0x1000029
>             0x2000064>;
> 
> 
> This is how the same panel is described using the st7735r drm driver and
> this patchset:
> 
>     width = <160>;
>     height = <128>;
> 
>     frmctr1 = [ 01 2C 2D ];
>     invctr = [ 07 ];
>     pwctr1 = [ A2 02 84 ];
>     pwctr2 = [ C5 ];
>     pwctr3 = [ 0A 00 ];
>     vmctr1 = [ 0E ];
>     madctl = [ 60 ];
>     gamctrp1 = [ 0F 1A 0F 18 2F 28 20 22 1F 1B 23 37 00 07 02 10 ];
>     gamctrn1 = [ 0F 1B 0F 17 33 2C 29 2E 30 30 39 3F 00 07 03 10 ];
> 
> 
> Back when the fbtft drivers were added to staging I asked on the DT
> mailinglist if it was OK to use the 'init' property but it was turned
> down. In this patchset I'm trying the same approach used by the
> solomon,ssd1307fb.yaml binding in describing the attached panel. That
> binding prefixes the properties with the vendor name, not sure why that
> is and I think it looks strange so I try without it.
> 
> Noralf.
> 
> 
> Noralf Trønnes (6):
>   dt-bindings: display: sitronix,st7735r: Fix backlight in example
>   dt-bindings: display: sitronix,st7735r: Make reset-gpios optional
>   dt-bindings: display: sitronix,st7735r: Remove spi-max-frequency limit

Patches 1-3 applied, thanks for reviewing.

The change to the driver has been replaced by a new generic driver
panel-mipi-dbi.

Noralf.

>   dt-bindings: display: sitronix,st7735r: Add initialization properties
>   drm/mipi-dbi: Add device property functions
>   drm: tiny: st7735r: Support DT initialization of controller
> 
>  .../bindings/display/sitronix,st7735r.yaml    | 122 ++++++++++++++-
>  drivers/gpu/drm/drm_mipi_dbi.c                | 139 ++++++++++++++++++
>  drivers/gpu/drm/tiny/st7735r.c                |  87 +++++++++--
>  include/drm/drm_mipi_dbi.h                    |   3 +
>  4 files changed, 334 insertions(+), 17 deletions(-)
>