| Message ID | 20230512-amlogic-v6-4-upstream-dsi-ccf-vim3-v4-4-2592c29ea263@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/meson: add support for MIPI DSI Display | expand |
On Fri, May 12, 2023 at 03:11:35PM +0200, Neil Armstrong wrote: > The Amlogic G12A, G12B & SM1 SoCs embeds a Synopsys DW-MIPI-DSI transceiver (ver 1.21a), > with a custom glue managing the IP resets, clock and data input similar to the DW-HDMI Glue > on the same Amlogic SoCs. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> This looks fine to me, Reviewed-by: Conor Dooley <conor.dooley@microchip.com> but I would like Krzysztof to take a look too to be on the safe side! Cheers, Conor. > --- > .../display/amlogic,meson-g12a-dw-mipi-dsi.yaml | 117 +++++++++++++++++++++ > 1 file changed, 117 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml > new file mode 100644 > index 000000000000..8169c7e93ff5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml > @@ -0,0 +1,117 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright 2020 BayLibre, SAS > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/amlogic,meson-g12a-dw-mipi-dsi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Amlogic specific extensions to the Synopsys Designware MIPI DSI Host Controller > + > +maintainers: > + - Neil Armstrong <neil.armstrong@linaro.org> > + > +description: | > + The Amlogic Meson Synopsys Designware Integration is composed of > + - A Synopsys DesignWare MIPI DSI Host Controller IP > + - A TOP control block controlling the Clocks & Resets of the IP > + > +allOf: > + - $ref: dsi-controller.yaml# > + > +properties: > + compatible: > + enum: > + - amlogic,meson-g12a-dw-mipi-dsi > + > + reg: > + maxItems: 1 > + > + clocks: > + minItems: 3 > + > + clock-names: > + minItems: 3 > + items: > + - const: pclk > + - const: bit_clk > + - const: px_clk > + - const: meas_clk > + > + resets: > + minItems: 1 > + > + reset-names: > + items: > + - const: top > + > + phys: > + minItems: 1 > + > + phy-names: > + items: > + - const: dphy > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + $ref: /schemas/graph.yaml#/properties/port > + description: Input node to receive pixel data. > + > + port@1: > + $ref: /schemas/graph.yaml#/properties/port > + description: DSI output node to panel. > + > + required: > + - port@0 > + - port@1 > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - resets > + - reset-names > + - phys > + - phy-names > + - ports > + > +unevaluatedProperties: false > + > +examples: > + - | > + dsi@7000 { > + compatible = "amlogic,meson-g12a-dw-mipi-dsi"; > + reg = <0x6000 0x400>; > + resets = <&reset_top>; > + reset-names = "top"; > + clocks = <&clk_pclk>, <&bit_clk>, <&clk_px>; > + clock-names = "pclk", "bit_clk", "px_clk"; > + phys = <&mipi_dphy>; > + phy-names = "dphy"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + /* VPU VENC Input */ > + mipi_dsi_venc_port: port@0 { > + reg = <0>; > + > + mipi_dsi_in: endpoint { > + remote-endpoint = <&dpi_out>; > + }; > + }; > + > + /* DSI Output */ > + mipi_dsi_panel_port: port@1 { > + reg = <1>; > + > + mipi_out_panel: endpoint { > + remote-endpoint = <&mipi_in_panel>; > + }; > + }; > + }; > + }; > > -- > 2.34.1 >
On 12/05/2023 15:11, Neil Armstrong wrote: > The Amlogic G12A, G12B & SM1 SoCs embeds a Synopsys DW-MIPI-DSI transceiver (ver 1.21a), > with a custom glue managing the IP resets, clock and data input similar to the DW-HDMI Glue > on the same Amlogic SoCs. Please wrap commit message according to Linux coding style / submission process (neither too early nor over the limit): https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 Subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > .../display/amlogic,meson-g12a-dw-mipi-dsi.yaml | 117 +++++++++++++++++++++ > 1 file changed, 117 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml > new file mode 100644 > index 000000000000..8169c7e93ff5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml > @@ -0,0 +1,117 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright 2020 BayLibre, SAS > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/amlogic,meson-g12a-dw-mipi-dsi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Amlogic specific extensions to the Synopsys Designware MIPI DSI Host Controller > + > +maintainers: > + - Neil Armstrong <neil.armstrong@linaro.org> > + > +description: | > + The Amlogic Meson Synopsys Designware Integration is composed of > + - A Synopsys DesignWare MIPI DSI Host Controller IP > + - A TOP control block controlling the Clocks & Resets of the IP > + > +allOf: > + - $ref: dsi-controller.yaml# > + > +properties: > + compatible: > + enum: > + - amlogic,meson-g12a-dw-mipi-dsi > + > + reg: > + maxItems: 1 > + > + clocks: > + minItems: 3 Missing maxItems > + > + clock-names: > + minItems: 3 > + items: > + - const: pclk > + - const: bit_clk > + - const: px_clk > + - const: meas_clk Drop _clk suffixes. pclk can stay, it's a bit odd but recently Rob clarified that suffix with underscore should not be there. > + > + resets: > + minItems: 1 maxItems instead > + > + reset-names: > + items: > + - const: top > + > + phys: > + minItems: 1 Ditto > + > + phy-names: > + items: > + - const: dphy > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + $ref: /schemas/graph.yaml#/properties/port > + description: Input node to receive pixel data. > + > + port@1: > + $ref: /schemas/graph.yaml#/properties/port > + description: DSI output node to panel. > + > + required: > + - port@0 > + - port@1 > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - resets > + - reset-names > + - phys > + - phy-names > + - ports > + > +unevaluatedProperties: false > + > +examples: > + - | > + dsi@7000 { > + compatible = "amlogic,meson-g12a-dw-mipi-dsi"; > + reg = <0x6000 0x400>; Your reg does not match unit address. The dt_binding_check should actually complain about it. Best regards, Krzysztof
On 13/05/2023 20:32, Krzysztof Kozlowski wrote: > On 12/05/2023 15:11, Neil Armstrong wrote: >> The Amlogic G12A, G12B & SM1 SoCs embeds a Synopsys DW-MIPI-DSI transceiver (ver 1.21a), >> with a custom glue managing the IP resets, clock and data input similar to the DW-HDMI Glue >> on the same Amlogic SoCs. > > Please wrap commit message according to Linux coding style / submission > process (neither too early nor over the limit): > https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 This message may be automatic, but context is always important when reviewing, this commit message is a re-spin on v3 that was reviewed by rob but I decided to remove the review tags since I added a new clock and did some other cleanups. While the process describes "how the patch itself *should* be formatted", it's a best effort and not a blocker. I'll fix the wrapping since you pointed out, but referring to the submitting-patches.rst file (from a very old v5.18-rc4 version) is kind of childish. > > Subject: drop second/last, redundant "bindings". The "dt-bindings" > prefix is already stating that these are bindings. > >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> .../display/amlogic,meson-g12a-dw-mipi-dsi.yaml | 117 +++++++++++++++++++++ >> 1 file changed, 117 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml >> new file mode 100644 >> index 000000000000..8169c7e93ff5 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml >> @@ -0,0 +1,117 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +# Copyright 2020 BayLibre, SAS >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/display/amlogic,meson-g12a-dw-mipi-dsi.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Amlogic specific extensions to the Synopsys Designware MIPI DSI Host Controller >> + >> +maintainers: >> + - Neil Armstrong <neil.armstrong@linaro.org> >> + >> +description: | >> + The Amlogic Meson Synopsys Designware Integration is composed of >> + - A Synopsys DesignWare MIPI DSI Host Controller IP >> + - A TOP control block controlling the Clocks & Resets of the IP >> + >> +allOf: >> + - $ref: dsi-controller.yaml# >> + >> +properties: >> + compatible: >> + enum: >> + - amlogic,meson-g12a-dw-mipi-dsi >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + minItems: 3 > > Missing maxItems Ack > >> + >> + clock-names: >> + minItems: 3 >> + items: >> + - const: pclk >> + - const: bit_clk >> + - const: px_clk >> + - const: meas_clk > > Drop _clk suffixes. pclk can stay, it's a bit odd but recently Rob > clarified that suffix with underscore should not be there. Ack > >> + >> + resets: >> + minItems: 1 > > maxItems instead Ack > >> + >> + reset-names: >> + items: >> + - const: top >> + >> + phys: >> + minItems: 1 > > Ditto Ack > >> + >> + phy-names: >> + items: >> + - const: dphy >> + >> + ports: >> + $ref: /schemas/graph.yaml#/properties/ports >> + >> + properties: >> + port@0: >> + $ref: /schemas/graph.yaml#/properties/port >> + description: Input node to receive pixel data. >> + >> + port@1: >> + $ref: /schemas/graph.yaml#/properties/port >> + description: DSI output node to panel. >> + >> + required: >> + - port@0 >> + - port@1 >> + >> +required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + - resets >> + - reset-names >> + - phys >> + - phy-names >> + - ports >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + dsi@7000 { >> + compatible = "amlogic,meson-g12a-dw-mipi-dsi"; >> + reg = <0x6000 0x400>; > > Your reg does not match unit address. The dt_binding_check should > actually complain about it. Well, it doesn't, will fix Thanks, Neil > > Best regards, > Krzysztof >
On 15/05/2023 18:15, Neil Armstrong wrote: > On 13/05/2023 20:32, Krzysztof Kozlowski wrote: >> On 12/05/2023 15:11, Neil Armstrong wrote: >>> The Amlogic G12A, G12B & SM1 SoCs embeds a Synopsys DW-MIPI-DSI transceiver (ver 1.21a), >>> with a custom glue managing the IP resets, clock and data input similar to the DW-HDMI Glue >>> on the same Amlogic SoCs. >> >> Please wrap commit message according to Linux coding style / submission >> process (neither too early nor over the limit): >> https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 > > This message may be automatic, but context is always important when reviewing, > this commit message is a re-spin on v3 that was reviewed by rob but I decided to remove the review > tags since I added a new clock and did some other cleanups. > > While the process describes "how the patch itself *should* be formatted", it's a best effort > and not a blocker. Other issues are blockers. > > I'll fix the wrapping since you pointed out, but referring to the submitting-patches.rst > file (from a very old v5.18-rc4 version) is kind of childish. It's just a link stored in automated responses, what's here childish? It's still valid in current cycle! Look: https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 What's the difference? Srsly, I can point you to submitting patches without reference to specific line if you wish... Or you can check by yourself. I give the same reviews to so many people that have templates and Elixir happens to be the only place allowing bookmarking specific line. Which is helpful for beginners because the entire doc is huge. I can make an exception for you and never paste direct links. Best regards, Krzysztof
On 15/05/2023 18:22, Krzysztof Kozlowski wrote: > On 15/05/2023 18:15, Neil Armstrong wrote: >> On 13/05/2023 20:32, Krzysztof Kozlowski wrote: >>> On 12/05/2023 15:11, Neil Armstrong wrote: >>>> The Amlogic G12A, G12B & SM1 SoCs embeds a Synopsys DW-MIPI-DSI transceiver (ver 1.21a), >>>> with a custom glue managing the IP resets, clock and data input similar to the DW-HDMI Glue >>>> on the same Amlogic SoCs. >>> >>> Please wrap commit message according to Linux coding style / submission >>> process (neither too early nor over the limit): >>> https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 >> >> This message may be automatic, but context is always important when reviewing, >> this commit message is a re-spin on v3 that was reviewed by rob but I decided to remove the review >> tags since I added a new clock and did some other cleanups. >> >> While the process describes "how the patch itself *should* be formatted", it's a best effort >> and not a blocker. > > Other issues are blockers. I agree with that > >> >> I'll fix the wrapping since you pointed out, but referring to the submitting-patches.rst >> file (from a very old v5.18-rc4 version) is kind of childish. > > It's just a link stored in automated responses, what's here childish? > It's still valid in current cycle! Look: > > https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 > > What's the difference? Srsly, I can point you to submitting patches > without reference to specific line if you wish... Or you can check by > yourself. > > I give the same reviews to so many people that have templates and Elixir > happens to be the only place allowing bookmarking specific line. Which > is helpful for beginners because the entire doc is huge. > > I can make an exception for you and never paste direct links. I value those kind of links for beginners and newcomers, really, it's a good thing to do and we should all do the same. But I always take in account who I'm reviewing, and adapt my comments, I think it's sane to not appear as rude because we all forget to check some stuff when send patches upstream, or at least I often forget... Anyway, don't make exceptions or change your process for me, I'll live with it. Neil > > Best regards, > Krzysztof >
On 15/05/2023 18:28, neil.armstrong@linaro.org wrote: >> It's just a link stored in automated responses, what's here childish? >> It's still valid in current cycle! Look: >> >> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 >> >> What's the difference? Srsly, I can point you to submitting patches >> without reference to specific line if you wish... Or you can check by >> yourself. >> >> I give the same reviews to so many people that have templates and Elixir >> happens to be the only place allowing bookmarking specific line. Which >> is helpful for beginners because the entire doc is huge. >> >> I can make an exception for you and never paste direct links. > > I value those kind of links for beginners and newcomers, really, it's a good > thing to do and we should all do the same. Hm, if I understand correctly, you felt being patronized by my link? I apologize for that. It was not my intention and there is really no need to feel like that. Look, I have many, many templates so I can speed up review. This one I gave to many: https://lore.kernel.org/all/?q=f%3Akrzysztof+%22Please+wrap+commit+message+according+to+Linux+coding+style%22 Writing same review every damn time is a boring, absolutely huge waste of time. People just make too many same mistakes. Better to hit key shortcut. Over the time most of my templates grew a bit, because when I wrote "Please wrap to 75" submitter did not know what to wrap or why. To save myself work I extend the template to something more. The entire text and link is for the beginner, not for you. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml new file mode 100644 index 000000000000..8169c7e93ff5 --- /dev/null +++ b/Documentation/devicetree/bindings/display/amlogic,meson-g12a-dw-mipi-dsi.yaml @@ -0,0 +1,117 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2020 BayLibre, SAS +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/amlogic,meson-g12a-dw-mipi-dsi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic specific extensions to the Synopsys Designware MIPI DSI Host Controller + +maintainers: + - Neil Armstrong <neil.armstrong@linaro.org> + +description: | + The Amlogic Meson Synopsys Designware Integration is composed of + - A Synopsys DesignWare MIPI DSI Host Controller IP + - A TOP control block controlling the Clocks & Resets of the IP + +allOf: + - $ref: dsi-controller.yaml# + +properties: + compatible: + enum: + - amlogic,meson-g12a-dw-mipi-dsi + + reg: + maxItems: 1 + + clocks: + minItems: 3 + + clock-names: + minItems: 3 + items: + - const: pclk + - const: bit_clk + - const: px_clk + - const: meas_clk + + resets: + minItems: 1 + + reset-names: + items: + - const: top + + phys: + minItems: 1 + + phy-names: + items: + - const: dphy + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/properties/port + description: Input node to receive pixel data. + + port@1: + $ref: /schemas/graph.yaml#/properties/port + description: DSI output node to panel. + + required: + - port@0 + - port@1 + +required: + - compatible + - reg + - clocks + - clock-names + - resets + - reset-names + - phys + - phy-names + - ports + +unevaluatedProperties: false + +examples: + - | + dsi@7000 { + compatible = "amlogic,meson-g12a-dw-mipi-dsi"; + reg = <0x6000 0x400>; + resets = <&reset_top>; + reset-names = "top"; + clocks = <&clk_pclk>, <&bit_clk>, <&clk_px>; + clock-names = "pclk", "bit_clk", "px_clk"; + phys = <&mipi_dphy>; + phy-names = "dphy"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + /* VPU VENC Input */ + mipi_dsi_venc_port: port@0 { + reg = <0>; + + mipi_dsi_in: endpoint { + remote-endpoint = <&dpi_out>; + }; + }; + + /* DSI Output */ + mipi_dsi_panel_port: port@1 { + reg = <1>; + + mipi_out_panel: endpoint { + remote-endpoint = <&mipi_in_panel>; + }; + }; + }; + };