| Message ID | 20240801-dw-hdmi-qp-tx-v1-1-148f542de5fd@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for Synopsys DW HDMI QP TX Controller | expand |
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
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/
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
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
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
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(+)