Re: [PATCH v1 04/17] dt-bindings: display: mediatek: add MT8195 hdmi bindings

[Guillaume Ranquet <granquet@baylibre.com>]

On Thu, 22 Sep 2022 09:18, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>On 19/09/2022 18:56, Guillaume Ranquet wrote:
>> Add mt8195 SoC bindings for hdmi and hdmi-ddc
>>
>> Make port1 optional for mt8195 as it only supports HDMI tx for now.
>> Requires a ddc-i2c-bus phandle.
>> Requires a power-domains phandle.
>>
>> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
>>
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml
>> index bdaf0b51e68c..abb231a0694b 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml
>> @@ -21,6 +21,10 @@ properties:
>>        - mediatek,mt7623-hdmi
>>        - mediatek,mt8167-hdmi
>>        - mediatek,mt8173-hdmi
>> +      - mediatek,mt8195-hdmi
>> +
>> +  clocks: true
>> +  clock-names: true
>
>????
>Why is this moved?
>
>>
>>    reg:
>>      maxItems: 1
>> @@ -28,20 +32,6 @@ properties:
>>    interrupts:
>>      maxItems: 1
>>
>> -  clocks:
>> -    items:
>> -      - description: Pixel Clock
>> -      - description: HDMI PLL
>> -      - description: Bit Clock
>> -      - description: S/PDIF Clock
>> -
>> -  clock-names:
>> -    items:
>> -      - const: pixel
>> -      - const: pll
>> -      - const: bclk
>> -      - const: spdif
>
>Clock definition with constraints should stay here. You just customize
>it per variant.
>
Clocks are different between the two hardwares, so I've tried moving everything
inside the if/else block.

Is there a better way to express this?

>> -
>>    phys:
>>      maxItems: 1
>>
>> @@ -58,6 +48,16 @@ properties:
>>      description: |
>>        phandle link and register offset to the system configuration registers.
>>
>> +  ddc-i2c-bus:
>> +    $ref: '/schemas/types.yaml#/definitions/phandle'
>
>Drop quotes
>
>> +    description: Phandle to the ddc-i2c device
>
>Isn't this property of panel?
>
It's a property used in panels and connectors.
But since this IP doesn't use a connector per say, I've added the property here.

Which doesn't sound reasonnable when I'm explaining it like this...
I'll see what I can do to fit a connector and have things look a bit
more standard.

>> +
>> +  power-domains:
>> +    description:
>> +      A phandle and PM domain specifier as defined by bindings
>> +      of the power controller specified by phandle. See
>> +      Documentation/devicetree/bindings/power/power-domain.yaml for details.
>
>No need for this text. This is standard property. You miss maxItems.
>
>
>> +
>>    ports:
>>      $ref: /schemas/graph.yaml#/properties/ports
>>
>> @@ -76,7 +76,6 @@ properties:
>>
>>      required:
>>        - port@0
>> -      - port@1
>>
>>  required:
>>    - compatible
>> @@ -86,9 +85,55 @@ required:
>>    - clock-names
>>    - phys
>>    - phy-names
>> -  - mediatek,syscon-hdmi
>>    - ports
>>
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: mediatek,mt8195-hdmi
>> +    then:
>> +      properties:
>> +        clocks:
>> +          items:
>> +            - description: APB
>> +            - description: HDCP
>> +            - description: HDCP 24M
>> +            - description: Split HDMI
>> +        clock-names:
>> +          items:
>> +            - const: hdmi_apb_sel
>> +            - const: hdcp_sel
>> +            - const: hdcp24_sel
>> +            - const: split_hdmi
>
>Clocks are entirely different. I am not sure there is benefit in keeping
>these devices in one bindings.
>

I agree with that, but it was requested by CK that the driver and bindings be
as common as possible.

>> +
>> +      required:
>> +        - power-domains
>> +        - ddc-i2c-bus
>
>Blank line,
>
>> +    else:
>> +      properties:
>> +        clocks:
>> +          items:
>> +            - description: Pixel Clock
>> +            - description: HDMI PLL
>> +            - description: Bit Clock
>> +            - description: S/PDIF Clock
>> +
>> +        clock-names:
>> +          items:
>> +            - const: pixel
>> +            - const: pll
>> +            - const: bclk
>> +            - const: spdif
>> +
>> +        ports:
>> +          required:
>> +            - port@1
>> +
>> +      required:
>> +        - mediatek,syscon-hdmi
>> +
>>  additionalProperties: false
>>
>>  examples:
>> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml
>> new file mode 100644
>> index 000000000000..3c80bcebe6d3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Mediatek HDMI DDC Device Tree Bindings for mt8195
>
>Drop Device Tree Bindings
>
>> +
>> +maintainers:
>> +  - CK Hu <ck.hu@mediatek.com>
>> +  - Jitao shi <jitao.shi@mediatek.com>
>> +
>> +description: |
>> +  The HDMI DDC i2c controller is used to interface with the HDMI DDC pins.
>
>Why is this different than existing ddc bindings?
>
This ddc is actually part of the MT8195 hdmi IP.

So it is a bit simpler than the mediatek,hdmi-ddc.yaml
As it has only one clock, no reg, no interrupts.

>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - mediatek,mt8195-hdmi-ddc
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    items:
>> +      - const: ddc-i2c
>> +
>> +required:
>> +  - compatible
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    hdmiddc0: ddc_i2c {
>
>No underscores in node names. Generic node names.
>
>
>Best regards,
>Krzysztof
>