[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e3ce055d-13c9-4e25-a039-dd4a58c8dd7a@collabora.com>
Date: Thu, 1 Aug 2024 12:29:52 +0300
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: kernel@...labora.com, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Heiko Stübner <heiko@...ech.de>,
Andy Yan <andy.yan@...k-chips.com>, Alexandre ARNOUD <aarnoud@...com>,
Luis de Arquer <ldearquer@...il.com>
Subject: Re: [PATCH 1/2] dt-bindings: display: bridge: Add schema for Synopsys
DW HDMI QP TX IP
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@...labora.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@...labora.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/
Powered by blists - more mailing lists