Message-ID: <84796070-7740-eb69-65c0-9a3d8e464a0f@loongson.cn>
Date:   Thu, 23 Feb 2023 17:51:59 +0800
From:   suijingfeng <suijingfeng@...ngson.cn>
To:     Krzysztof Kozlowski <krzk@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Cc:     linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 2/2] dt-bindings: display: Add Loongson display controller

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@...ngson.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@...ngson.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@...z,
>> +  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

Powered by blists - more mailing lists