[PATCH v2 1/3] dt-bindings: display: Add Sitronix ST7571 panel

Marcus Folkesson posted 3 patches 10 months, 1 week ago
There is a newer version of this series
[PATCH v2 1/3] dt-bindings: display: Add Sitronix ST7571 panel
Posted by Marcus Folkesson 10 months, 1 week ago
Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 .../bindings/display/sitronix,st7571.yaml          | 73 ++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/sitronix,st7571.yaml b/Documentation/devicetree/bindings/display/sitronix,st7571.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..11575b820c59c5ada427fbb6b015c331215c8db6
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/sitronix,st7571.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/sitronix,st7571.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sitronix ST7571 Display Panels
+
+maintainers:
+  - Marcus Folkesson <marcus.folkesson@gmail.com>
+
+description:
+  This binding is for display panels using a Sitronix ST7571 controller in I2C
+  mode.
+
+allOf:
+  - $ref: panel/panel-common.yaml#
+
+properties:
+  compatible:
+    const: sitronix,st7571
+
+  reg: true
+  reset-gpios: true
+  width-mm: true
+  height-mm: true
+
+  panel-timing:
+    $ref: panel/panel-timing.yaml#
+    description: |
+      The panel-timing node specifies the display resolution and timing
+      parameters. The hactive and vactive properties are mandatory.
+      The vback-porch property specifies the start line of the display.
+      The other properties should be set to zero.
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - width-mm
+  - height-mm
+  - panel-timing
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        display@3f {
+          compatible = "sitronix,st7571";
+          reg = <0x3f>;
+          reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>;
+          width-mm = <37>;
+          height-mm = <27>;
+
+            panel-timing {
+                hactive = <128>;
+                vactive = <96>;
+                hback-porch = <0>;
+                vback-porch = <0>;
+                clock-frequency = <0>;
+                hfront-porch = <0>;
+                hsync-len = <0>;
+                vfront-porch = <0>;
+                vsync-len = <0>;
+            };
+        };
+    };

-- 
2.49.0
Re: [PATCH v2 1/3] dt-bindings: display: Add Sitronix ST7571 panel
Posted by Krzysztof Kozlowski 10 months, 1 week ago
On 04/04/2025 15:50, Marcus Folkesson wrote:
> Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>  .../bindings/display/sitronix,st7571.yaml          | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7571.yaml b/Documentation/devicetree/bindings/display/sitronix,st7571.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..11575b820c59c5ada427fbb6b015c331215c8db6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sitronix,st7571.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/sitronix,st7571.yaml#

Why isn't this in panels directory?

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sitronix ST7571 Display Panels
> +
> +maintainers:
> +  - Marcus Folkesson <marcus.folkesson@gmail.com>
> +
> +description:
> +  This binding is for display panels using a Sitronix ST7571 controller in I2C

Do not explain for a binding that it is a binding. Redundant. Instead
describe the hardware.

> +  mode.
> +
> +allOf:
> +  - $ref: panel/panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: sitronix,st7571
> +
> +  reg: true
> +  reset-gpios: true
> +  width-mm: true
> +  height-mm: true
> +
> +  panel-timing:
> +    $ref: panel/panel-timing.yaml#

Drop, already part of panel.

> +    description: |
> +      The panel-timing node specifies the display resolution and timing
> +      parameters. The hactive and vactive properties are mandatory.
> +      The vback-porch property specifies the start line of the display.
> +      The other properties should be set to zero.

Drop description as well.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - width-mm
> +  - height-mm
> +  - panel-timing
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        display@3f {

Not much improved. How is this called in every other binding? panel.

> +          compatible = "sitronix,st7571";

Messed indentation.

> +          reg = <0x3f>;
> +          reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>;
> +          width-mm = <37>;
> +          height-mm = <27>;
> +
> +            panel-timing {

And here is even more confusing.

Best regards,
Krzysztof
Re: [PATCH v2 1/3] dt-bindings: display: Add Sitronix ST7571 panel
Posted by Krzysztof Kozlowski 10 months, 1 week ago
On 04/04/2025 19:30, Krzysztof Kozlowski wrote:
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        display@3f {
> 
> Not much improved. How is this called in every other binding? panel.

Hmmm, unless this is not a panel, but it looks like a panel and
description partially suggests it. Other sitronix devices are split
between these two, but OTOH your driver is more complex than just simple
panel.

Your commit msg is one sentence and binding description is basically
non-existing, so not sure how to help. You need to describe the hardware
so people understand what this device is.

Best regards,
Krzysztof
Re: [PATCH v2 1/3] dt-bindings: display: Add Sitronix ST7571 panel
Posted by Marcus Folkesson 10 months, 1 week ago
Hi Krzysztof,

On Fri, Apr 04, 2025 at 07:36:12PM +0200, Krzysztof Kozlowski wrote:
> On 04/04/2025 19:30, Krzysztof Kozlowski wrote:
> >> +
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/gpio/gpio.h>
> >> +    
> >> +    i2c {
> >> +        #address-cells = <1>;
> >> +        #size-cells = <0>;
> >> +
> >> +        display@3f {
> > 
> > Not much improved. How is this called in every other binding? panel.
> 
> Hmmm, unless this is not a panel, but it looks like a panel and
> description partially suggests it. Other sitronix devices are split
> between these two, but OTOH your driver is more complex than just simple
> panel.

I've counted this as a display, but the border is not crystal
clear, and, as you say, other Sitronix devices are split between the two.
It is a controller/driver for a LCD panel.

> 
> Your commit msg is one sentence and binding description is basically
> non-existing, so not sure how to help. You need to describe the hardware
> so people understand what this device is.

I've prepared this description for the next version of the patch:

description:
  Sitronix ST7571 is a driver and controller for up to 4-level gray
  scale dot-matrix LCD panels.
  It drives 128 segment outputs and 128+1 common outputs.
  It provides several system interfaces like SPI, I2C and 8-bit parallel bus.

But still, it is not obvious if I should move it to panel or not.

> 
> Best regards,
> Krzysztof

Best regards,
Marcus Folkesson
Re: [PATCH v2 1/3] dt-bindings: display: Add Sitronix ST7571 panel
Posted by Conor Dooley 10 months, 1 week ago
On Fri, Apr 04, 2025 at 03:50:32PM +0200, Marcus Folkesson wrote:
> Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>  .../bindings/display/sitronix,st7571.yaml          | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7571.yaml b/Documentation/devicetree/bindings/display/sitronix,st7571.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..11575b820c59c5ada427fbb6b015c331215c8db6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sitronix,st7571.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/sitronix,st7571.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sitronix ST7571 Display Panels
> +
> +maintainers:
> +  - Marcus Folkesson <marcus.folkesson@gmail.com>
> +
> +description:
> +  This binding is for display panels using a Sitronix ST7571 controller in I2C
> +  mode.
> +
> +allOf:
> +  - $ref: panel/panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: sitronix,st7571
> +
> +  reg: true

You need to constrain this, so maxItems: 1.
Otherwise, seems okay.