diff mbox series

[2/2] dt-bindings: display: Add Loongson display controller

Message ID 20230222165514.684729-2-suijingfeng@loongson.cn (mailing list archive)
State New, archived
Headers show
Series [1/2] Mips: ls2k1000: dts: add the display controller device node | expand

Commit Message

Sui Jingfeng Feb. 22, 2023, 4:55 p.m. UTC 
This patch add a trival DT usages for loongson display controller found
in LS2k1000 SoC.

Signed-off-by: suijingfeng <suijingfeng@loongson.cn>
---
 .../loongson/loongson,display-controller.yaml | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml

Comments

Krzysztof Kozlowski Feb. 22, 2023, 6:30 p.m. UTC | #1 
On 22/02/2023 17:55, suijingfeng wrote:
> This patch add a trival DT usages for loongson display controller found
> in LS2k1000 SoC.

Trivial yet so many things to improve... if you only started from recent
kernel tree (since you Cced wrong address, I doubt you did) and bindings
you would avoid half of these comments.

> 
> Signed-off-by: suijingfeng <suijingfeng@loongson.cn>
> ---
>  .../loongson/loongson,display-controller.yaml | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> new file mode 100644
> index 000000000000..98b78f449a80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml

Filename based on compatible, so "loongson,ls2k1000-dc.yaml"

> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#


> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Loongson Display Controller Device Tree Bindings

Drop "Device Tree Bindings"

> +
> +maintainers:
> +  - Sui Jingfeng <suijingfeng@loongson.cn>
> +
> +description: |+

Drop |+

> +

No need for blank line. Do you see it anywhere else in the bindings?

> +  The display controller is a PCI device, it has two display pipe.
> +  For the DC in LS2K1000 each way has a DVO output interface which
> +  provide RGB888 signals, vertical & horizontal synchronisations
> +  and the pixel clock. Each CRTC is able to support 1920x1080@60Hz,
> +  the maximum resolution is 2048x2048 according to the hardware spec.
> +
> +properties:
> +  $nodename:
> +    pattern: "^display-controller@[0-9a-f],[0-9a-f]$"

Drop nodename.

> +
> +  compatible:
> +    oneOf:

Drop oneOf

> +      - items:

and items...

> +          - enum:
> +              - loongson,ls2k1000-dc
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    bus {
> +

Drop blank line.

> +        #address-cells = <3>;
> +        #size-cells = <2>;
> +        #interrupt-cells = <2>;

Why do you need interrupt-cells?

> +
> +        display-controller@6,0 {
> +            compatible = "loongson,ls2k1000-dc";
> +            reg = <0x3000 0x0 0x0 0x0 0x0>;> +            interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
> +        };
> +    };
> +
> +...

Best regards,
Krzysztof
Sui Jingfeng Feb. 23, 2023, 9:51 a.m. UTC | #2 
Hi,

On 2023/2/23 02:30, Krzysztof Kozlowski wrote:
> On 22/02/2023 17:55, suijingfeng wrote:
>> This patch add a trival DT usages for loongson display controller found
>> in LS2k1000 SoC.
> Trivial yet so many things to improve... if you only started from recent
> kernel tree (since you Cced wrong address, I doubt you did) and bindings
> you would avoid half of these comments.

Yes, you are right.

I will double check the Cced next time, I'm start from drm-tip.

but Cced using a record before.

>> Signed-off-by: suijingfeng <suijingfeng@loongson.cn>
>> ---
>>   .../loongson/loongson,display-controller.yaml | 58 +++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>> new file mode 100644
>> index 000000000000..98b78f449a80
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
> Filename based on compatible, so "loongson,ls2k1000-dc.yaml"

what if we have more than one SoC,

we have  loongson,ls2k1000-dc, loongson,ls2k2000-dc and loongson,ls2k0500-dc

we will have loongson,ls2k3000-dc in the future, then how should i write 
this?

I want a single file yaml file include them all.

I'm asking because we don't know which method is good, write three piece 
of yaml or just one.

Just tell me how to write this, i will follow you instruction.

>> @@ -0,0 +1,58 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
>>
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Loongson Display Controller Device Tree Bindings
> Drop "Device Tree Bindings"
OK,
>> +
>> +maintainers:
>> +  - Sui Jingfeng <suijingfeng@loongson.cn>
>> +
>> +description: |+
> Drop |+
>
>> +
> No need for blank line. Do you see it anywhere else in the bindings?
OK, acceptable.
>> +  The display controller is a PCI device, it has two display pipe.
>> +  For the DC in LS2K1000 each way has a DVO output interface which
>> +  provide RGB888 signals, vertical & horizontal synchronisations
>> +  and the pixel clock. Each CRTC is able to support 1920x1080@60Hz,
>> +  the maximum resolution is 2048x2048 according to the hardware spec.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
> Drop nodename.

Are you sure about this?  When i  write this property, I'm reference the 
ingenic,lcd.yaml .

ingenic,lcd.yaml has nodename too.

If I delete $nodename, then the test results say 
'^display-controller@[0-9a-f],[0-9a-f]$'  is not of type 'object'.

log is pasted at below.


make -j$(nproc) ARCH=loongarch 
CROSS_COMPILE=loongarch64-unknown-linux-gnu- dt_binding_check 
DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml 
dtbs_check 
DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
   LINT    Documentation/devicetree/bindings
   DTEX 
Documentation/devicetree/bindings/display/loongson/loongson,display-controller.example.dts
   CHKDT   Documentation/devicetree/bindings/processed-schema.json
/home/suijingfeng/UpStream/drm-tip/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml: 
properties:pattern: '^display-controller@[0-9a-f],[0-9a-f]$' is not of 
type 'object', 'boolean'
     from schema $id: http://json-schema.org/draft-07/schema#
/home/suijingfeng/UpStream/drm-tip/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml: 
properties: 'pattern' should not be valid under {'$ref': 
'#/definitions/json-schema-prop-names'}
     hint: A json-schema keyword was found instead of a DT property name.
     from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/home/suijingfeng/UpStream/drm-tip/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml: 
ignoring, error in schema: properties: pattern
   DTC_CHK 
Documentation/devicetree/bindings/display/loongson/loongson,display-controller.example.dtb

>
>> +
>> +  compatible:
>> +    oneOf:
> Drop oneOf
>
>> +      - items:
> and items...
>
>> +          - enum:
>> +              - loongson,ls2k1000-dc
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    bus {
>> +
> Drop blank line.
>
>> +        #address-cells = <3>;
>> +        #size-cells = <2>;
>> +        #interrupt-cells = <2>;
> Why do you need interrupt-cells?
>
>> +
>> +        display-controller@6,0 {
>> +            compatible = "loongson,ls2k1000-dc";
>> +            reg = <0x3000 0x0 0x0 0x0 0x0>;> +            interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
>> +        };
>> +    };
>> +
>> +...
> Best regards,
> Krzysztof
Krzysztof Kozlowski Feb. 23, 2023, 10:09 a.m. UTC | #3 
On 23/02/2023 10:51, suijingfeng wrote:
>>> diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>>> new file mode 100644
>>> index 000000000000..98b78f449a80
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
>> Filename based on compatible, so "loongson,ls2k1000-dc.yaml"
> 
> what if we have more than one SoC,
> 
> we have  loongson,ls2k1000-dc, loongson,ls2k2000-dc and loongson,ls2k0500-dc
> 
> we will have loongson,ls2k3000-dc in the future, then how should i write 
> this?

Then it is fine.

> 
> I want a single file yaml file include them all.
> 
> I'm asking because we don't know which method is good, write three piece 
> of yaml or just one.
> 
> Just tell me how to write this, i will follow you instruction.
> 
>>> @@ -0,0 +1,58 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
>>>
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Loongson Display Controller Device Tree Bindings
>> Drop "Device Tree Bindings"
> OK,
>>> +
>>> +maintainers:
>>> +  - Sui Jingfeng <suijingfeng@loongson.cn>
>>> +
>>> +description: |+
>> Drop |+
>>
>>> +
>> No need for blank line. Do you see it anywhere else in the bindings?
> OK, acceptable.
>>> +  The display controller is a PCI device, it has two display pipe.
>>> +  For the DC in LS2K1000 each way has a DVO output interface which
>>> +  provide RGB888 signals, vertical & horizontal synchronisations
>>> +  and the pixel clock. Each CRTC is able to support 1920x1080@60Hz,
>>> +  the maximum resolution is 2048x2048 according to the hardware spec.
>>> +
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
>> Drop nodename.
> 
> Are you sure about this?  When i  write this property, I'm reference the 
> ingenic,lcd.yaml .
> 
> ingenic,lcd.yaml has nodename too.
> 
> If I delete $nodename, then the test results say 
> '^display-controller@[0-9a-f],[0-9a-f]$'  is not of type 'object'.
> 
> log is pasted at below.

I meant, drop entire nodename and pattern.



Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
new file mode 100644
index 000000000000..98b78f449a80
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml
@@ -0,0 +1,58 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson Display Controller Device Tree Bindings
+
+maintainers:
+  - Sui Jingfeng <suijingfeng@loongson.cn>
+
+description: |+
+
+  The display controller is a PCI device, it has two display pipe.
+  For the DC in LS2K1000 each way has a DVO output interface which
+  provide RGB888 signals, vertical & horizontal synchronisations
+  and the pixel clock. Each CRTC is able to support 1920x1080@60Hz,
+  the maximum resolution is 2048x2048 according to the hardware spec.
+
+properties:
+  $nodename:
+    pattern: "^display-controller@[0-9a-f],[0-9a-f]$"
+
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - loongson,ls2k1000-dc
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    bus {
+
+        #address-cells = <3>;
+        #size-cells = <2>;
+        #interrupt-cells = <2>;
+
+        display-controller@6,0 {
+            compatible = "loongson,ls2k1000-dc";
+            reg = <0x3000 0x0 0x0 0x0 0x0>;
+            interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
+        };
+    };
+
+...