[PATCH 1/2] dt-bindings: display: bridge: Add schema for Synopsys DW HDMI QP TX IP

Cristian Ciocaltea posted 2 patches 1 year, 6 months ago
[PATCH 1/2] dt-bindings: display: bridge: Add schema for Synopsys DW HDMI QP TX IP
Posted by Cristian Ciocaltea 1 year, 6 months ago
Add dt-binding schema containing the common properties for the Synopsys
DesignWare HDMI QP TX controller.

Note this is not a full dt-binding specification, but is meant to be
referenced by platform-specific bindings for this IP core.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 .../display/bridge/synopsys,dw-hdmi-qp.yaml        | 66 ++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml
new file mode 100644
index 000000000000..d8aee12b121d
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/synopsys,dw-hdmi-qp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common Properties for Synopsys DesignWare HDMI QP TX Controller IP
+
+maintainers:
+  - Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
+
+description: |
+  This document defines device tree properties for the Synopsys DesignWare
+  HDMI 2.1 Quad-Pixel (QP) TX controller IP core.
+  It doesn't constitute a device tree binding specification by itself, but
+  is meant to be referenced by platform-specific device tree bindings.
+
+  When referenced from platform device tree bindings, the properties defined
+  in this document are defined as follows. The platform device tree bindings
+  are responsible for defining whether each property is required or optional.
+
+properties:
+  reg:
+    maxItems: 1
+
+  clocks:
+    minItems: 4
+    maxItems: 6
+    items:
+      - description: Peripheral/APB bus clock
+      - description: EARC RX biphase clock
+      - description: Reference clock
+      - description: Audio interface clock
+    additionalItems: true
+
+  clock-names:
+    minItems: 4
+    maxItems: 6
+    items:
+      - const: pclk
+      - const: earc
+      - const: ref
+      - const: aud
+    additionalItems: true
+
+  interrupts:
+    minItems: 4
+    maxItems: 5
+    items:
+      - description: AVP Unit interrupt
+      - description: CEC interrupt
+      - description: eARC RX interrupt
+      - description: Main Unit interrupt
+    additionalItems: true
+
+  interrupt-names:
+    minItems: 4
+    maxItems: 5
+    items:
+      - const: avp
+      - const: cec
+      - const: earc
+      - const: main
+    additionalItems: true
+
+additionalProperties: true

-- 
2.45.2
Re: [PATCH 1/2] dt-bindings: display: bridge: Add schema for Synopsys DW HDMI QP TX IP
Posted by Krzysztof Kozlowski 1 year, 6 months ago
On 01/08/2024 04:05, Cristian Ciocaltea wrote:
> Add dt-binding schema containing the common properties for the Synopsys
> DesignWare HDMI QP TX controller.
> 
> Note this is not a full dt-binding specification, but is meant to be
> referenced by platform-specific bindings for this IP core.

Please provide an user for this binding. Otherwise it is a no-op.

> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../display/bridge/synopsys,dw-hdmi-qp.yaml        | 66 ++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml
> new file mode 100644
> index 000000000000..d8aee12b121d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/synopsys,dw-hdmi-qp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common Properties for Synopsys DesignWare HDMI QP TX Controller IP
> +
> +maintainers:
> +  - Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> +
> +description: |
> +  This document defines device tree properties for the Synopsys DesignWare
> +  HDMI 2.1 Quad-Pixel (QP) TX controller IP core.
> +  It doesn't constitute a device tree binding specification by itself, but
> +  is meant to be referenced by platform-specific device tree bindings.
> +
> +  When referenced from platform device tree bindings, the properties defined
> +  in this document are defined as follows. The platform device tree bindings
> +  are responsible for defining whether each property is required or optional.

Drop this all description and re-write it not to say what bindings are
or are not. Describe the hardware.

> +
> +properties:
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 4
> +    maxItems: 6
> +    items:
> +      - description: Peripheral/APB bus clock
> +      - description: EARC RX biphase clock
> +      - description: Reference clock
> +      - description: Audio interface clock
> +    additionalItems: true

??? What's the point of such common schema if it is not common at all?

> +
> +  clock-names:
> +    minItems: 4
> +    maxItems: 6
> +    items:
> +      - const: pclk
> +      - const: earc
> +      - const: ref
> +      - const: aud
> +    additionalItems: true
> +
> +  interrupts:
> +    minItems: 4
> +    maxItems: 5
> +    items:
> +      - description: AVP Unit interrupt
> +      - description: CEC interrupt
> +      - description: eARC RX interrupt
> +      - description: Main Unit interrupt
> +    additionalItems: true
> +
> +  interrupt-names:
> +    minItems: 4
> +    maxItems: 5
> +    items:
> +      - const: avp
> +      - const: cec
> +      - const: earc
> +      - const: main
> +    additionalItems: true

Sorry, there is no user of this and nothing here is actually common
except first entries in clocks and interrupts properties.

I don't see any benefit of this.

Best regards,
Krzysztof
Re: [PATCH 1/2] dt-bindings: display: bridge: Add schema for Synopsys DW HDMI QP TX IP
Posted by Cristian Ciocaltea 1 year, 6 months ago
On 8/1/24 11:38 AM, Krzysztof Kozlowski wrote:> On 01/08/2024 04:05, Cristian Ciocaltea wrote:
>> Add dt-binding schema containing the common properties for the Synopsys
>> DesignWare HDMI QP TX controller.
>>
>> Note this is not a full dt-binding specification, but is meant to be
>> referenced by platform-specific bindings for this IP core.
> 
> Please provide an user for this binding. Otherwise it is a no-op.

The first user of this is RK3588 HDMI TX Controller [1].

>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  .../display/bridge/synopsys,dw-hdmi-qp.yaml        | 66 ++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml
>> new file mode 100644
>> index 000000000000..d8aee12b121d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi-qp.yaml
>> @@ -0,0 +1,66 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/synopsys,dw-hdmi-qp.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Common Properties for Synopsys DesignWare HDMI QP TX Controller IP
>> +
>> +maintainers:
>> +  - Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> +
>> +description: |
>> +  This document defines device tree properties for the Synopsys DesignWare
>> +  HDMI 2.1 Quad-Pixel (QP) TX controller IP core.
>> +  It doesn't constitute a device tree binding specification by itself, but
>> +  is meant to be referenced by platform-specific device tree bindings.
>> +
>> +  When referenced from platform device tree bindings, the properties defined
>> +  in this document are defined as follows. The platform device tree bindings
>> +  are responsible for defining whether each property is required or optional.
> 
> Drop this all description and re-write it not to say what bindings are
> or are not. Describe the hardware.

I just tried to keep it similar with synopsys,dw-hdmi.yaml. 

> 
>> +
>> +properties:
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    minItems: 4
>> +    maxItems: 6
>> +    items:
>> +      - description: Peripheral/APB bus clock
>> +      - description: EARC RX biphase clock
>> +      - description: Reference clock
>> +      - description: Audio interface clock
>> +    additionalItems: true
> 
> ??? What's the point of such common schema if it is not common at all?

The schema is referenced by [1].
 
>> +
>> +  clock-names:
>> +    minItems: 4
>> +    maxItems: 6
>> +    items:
>> +      - const: pclk
>> +      - const: earc
>> +      - const: ref
>> +      - const: aud
>> +    additionalItems: true
>> +
>> +  interrupts:
>> +    minItems: 4
>> +    maxItems: 5
>> +    items:
>> +      - description: AVP Unit interrupt
>> +      - description: CEC interrupt
>> +      - description: eARC RX interrupt
>> +      - description: Main Unit interrupt
>> +    additionalItems: true
>> +
>> +  interrupt-names:
>> +    minItems: 4
>> +    maxItems: 5
>> +    items:
>> +      - const: avp
>> +      - const: cec
>> +      - const: earc
>> +      - const: main
>> +    additionalItems: true
> 
> Sorry, there is no user of this and nothing here is actually common
> except first entries in clocks and interrupts properties.
> 
> I don't see any benefit of this.

Sorry, I should have better indicated this is part of a larger changeset -
the cover mentions this is a reworked version of an initial (larger) series
and the split has been explicitly suggested during the review.

> Best regards,
> Krzysztof

Thanks for reviewing,
Cristian

[1]: https://lore.kernel.org/lkml/20240801-b4-rk3588-bridge-upstream-v2-1-9fa657a4e15b@collabora.com/
Re: [PATCH 1/2] dt-bindings: display: bridge: Add schema for Synopsys DW HDMI QP TX IP
Posted by Krzysztof Kozlowski 1 year, 6 months ago
On 01/08/2024 11:29, Cristian Ciocaltea wrote:
>>> +  interrupts:
>>> +    minItems: 4
>>> +    maxItems: 5
>>> +    items:
>>> +      - description: AVP Unit interrupt
>>> +      - description: CEC interrupt
>>> +      - description: eARC RX interrupt
>>> +      - description: Main Unit interrupt
>>> +    additionalItems: true
>>> +
>>> +  interrupt-names:
>>> +    minItems: 4
>>> +    maxItems: 5
>>> +    items:
>>> +      - const: avp
>>> +      - const: cec
>>> +      - const: earc
>>> +      - const: main
>>> +    additionalItems: true
>>
>> Sorry, there is no user of this and nothing here is actually common
>> except first entries in clocks and interrupts properties.
>>
>> I don't see any benefit of this.
> 
> Sorry, I should have better indicated this is part of a larger changeset -
> the cover mentions this is a reworked version of an initial (larger) series
> and the split has been explicitly suggested during the review.

This split is really odd. It creates unnecessary dependency, blocks
automated testing and confuses reviewers because reviewers expect common
code followed by its user.

Best regards,
Krzysztof
Re: [PATCH 1/2] dt-bindings: display: bridge: Add schema for Synopsys DW HDMI QP TX IP
Posted by Krzysztof Kozlowski 1 year, 6 months ago
On 01/08/2024 11:29, Cristian Ciocaltea wrote:
> On 8/1/24 11:38 AM, Krzysztof Kozlowski wrote:> On 01/08/2024 04:05, Cristian Ciocaltea wrote:
>>> Add dt-binding schema containing the common properties for the Synopsys
>>> DesignWare HDMI QP TX controller.
>>>
>>> Note this is not a full dt-binding specification, but is meant to be
>>> referenced by platform-specific bindings for this IP core.
>>
>> Please provide an user for this binding. Otherwise it is a no-op.
> 
> The first user of this is RK3588 HDMI TX Controller [1].

Patchsets do not work like this. Splitting things causes that your other
patch simply cannot even be tested because you introduced dependency.

Combine patches so bindings referencing common properties can properly
be tested by automation.

Best regards,
Krzysztof