| Message ID | 20220905230406.30801-3-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add driver for CSI2 and CRU modules found on Renesas RZ/G2L SoC | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 06/09/2022 01:04, Lad Prabhakar wrote: > Document the CRU block found on Renesas RZ/G2L (and alike) SoCs. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Thank you for your patch. There is something to discuss/improve. > +properties: > + compatible: > + items: > + - enum: > + - renesas,r9a07g044-cru # RZ/G2{L,LC} > + - renesas,r9a07g054-cru # RZ/V2L > + - const: renesas,rzg2l-cru > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 3 > + > + interrupt-names: > + items: > + - const: image_conv > + - const: image_conv_err > + - const: axi_mst_err > + > + clocks: > + items: > + - description: CRU Main clock > + - description: CPU Register access clock > + - description: CRU image transfer clock > + > + clock-names: > + items: > + - const: vclk > + - const: pclk > + - const: aclk Drop the "clk" suffixes. Remaining names could be made a bit more readable. > + Best regards, Krzysztof
On Thu, Sep 08, 2022 at 01:40:39PM +0200, Krzysztof Kozlowski wrote: > On 06/09/2022 01:04, Lad Prabhakar wrote: > > Document the CRU block found on Renesas RZ/G2L (and alike) SoCs. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Thank you for your patch. There is something to discuss/improve. > > > +properties: > > + compatible: > > + items: > > + - enum: > > + - renesas,r9a07g044-cru # RZ/G2{L,LC} > > + - renesas,r9a07g054-cru # RZ/V2L > > + - const: renesas,rzg2l-cru > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 3 > > + > > + interrupt-names: > > + items: > > + - const: image_conv > > + - const: image_conv_err > > + - const: axi_mst_err > > + > > + clocks: > > + items: > > + - description: CRU Main clock > > + - description: CPU Register access clock > > + - description: CRU image transfer clock > > + > > + clock-names: > > + items: > > + - const: vclk > > + - const: pclk > > + - const: aclk > > Drop the "clk" suffixes. Remaining names could be made a bit more readable. These names come from the documentation, isn't it better to match the datasheet ? > > +
On 21/09/2022 14:43, Laurent Pinchart wrote: > On Thu, Sep 08, 2022 at 01:40:39PM +0200, Krzysztof Kozlowski wrote: >> On 06/09/2022 01:04, Lad Prabhakar wrote: >>> Document the CRU block found on Renesas RZ/G2L (and alike) SoCs. >>> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> >> Thank you for your patch. There is something to discuss/improve. >> >>> +properties: >>> + compatible: >>> + items: >>> + - enum: >>> + - renesas,r9a07g044-cru # RZ/G2{L,LC} >>> + - renesas,r9a07g054-cru # RZ/V2L >>> + - const: renesas,rzg2l-cru >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + interrupts: >>> + maxItems: 3 >>> + >>> + interrupt-names: >>> + items: >>> + - const: image_conv >>> + - const: image_conv_err >>> + - const: axi_mst_err >>> + >>> + clocks: >>> + items: >>> + - description: CRU Main clock >>> + - description: CPU Register access clock >>> + - description: CRU image transfer clock >>> + >>> + clock-names: >>> + items: >>> + - const: vclk >>> + - const: pclk >>> + - const: aclk >> >> Drop the "clk" suffixes. Remaining names could be made a bit more readable. > > These names come from the documentation, isn't it better to match the > datasheet ? If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the reason to use it. :) The "clk" is redundant even if the hardware engineer thought different. The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma"). Best regards, Krzysztof
On Wed, Sep 21, 2022 at 05:51:29PM +0200, Krzysztof Kozlowski wrote: > On 21/09/2022 14:43, Laurent Pinchart wrote: > > On Thu, Sep 08, 2022 at 01:40:39PM +0200, Krzysztof Kozlowski wrote: > >> On 06/09/2022 01:04, Lad Prabhakar wrote: > >>> Document the CRU block found on Renesas RZ/G2L (and alike) SoCs. > >>> > >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > >> > >> Thank you for your patch. There is something to discuss/improve. > >> > >>> +properties: > >>> + compatible: > >>> + items: > >>> + - enum: > >>> + - renesas,r9a07g044-cru # RZ/G2{L,LC} > >>> + - renesas,r9a07g054-cru # RZ/V2L > >>> + - const: renesas,rzg2l-cru > >>> + > >>> + reg: > >>> + maxItems: 1 > >>> + > >>> + interrupts: > >>> + maxItems: 3 > >>> + > >>> + interrupt-names: > >>> + items: > >>> + - const: image_conv > >>> + - const: image_conv_err > >>> + - const: axi_mst_err > >>> + > >>> + clocks: > >>> + items: > >>> + - description: CRU Main clock > >>> + - description: CPU Register access clock > >>> + - description: CRU image transfer clock > >>> + > >>> + clock-names: > >>> + items: > >>> + - const: vclk > >>> + - const: pclk > >>> + - const: aclk > >> > >> Drop the "clk" suffixes. Remaining names could be made a bit more readable. > > > > These names come from the documentation, isn't it better to match the > > datasheet ? > > If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the > reason to use it. :) > > The "clk" is redundant even if the hardware engineer thought different. > > The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma"). I'd argue that naming clocks "v", "p" and "a" would be less readable and more confusing. Is this a new rule ?
On 21/09/2022 19:29, Laurent Pinchart wrote: >>>>> + clock-names: >>>>> + items: >>>>> + - const: vclk >>>>> + - const: pclk >>>>> + - const: aclk >>>> >>>> Drop the "clk" suffixes. Remaining names could be made a bit more readable. >>> >>> These names come from the documentation, isn't it better to match the >>> datasheet ? >> >> If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the >> reason to use it. :) >> >> The "clk" is redundant even if the hardware engineer thought different. >> >> The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma"). > > I'd argue that naming clocks "v", "p" and "a" would be less readable and > more confusing. Is this a new rule ? Not really, but also it's only a style issue. Indeed "v" and "p" are not much better... but still "vclk" does not bring any additional information over "v". It's redundant. You can also drop entire entry - unless it helps in particular implementation. https://lore.kernel.org/all/20220517175958.GA1321687-robh@kernel.org/ https://lore.kernel.org/all/20210815133926.22860-1-biju.das.jz@bp.renesas.com/ https://lore.kernel.org/all/YYFCaHI%2FDASUz+Vu@robh.at.kernel.org/ https://lore.kernel.org/all/20220830182540.GA1797396-robh@kernel.org/ Best regards, Krzysztof
On Wed, Sep 21, 2022 at 08:58:26PM +0200, Krzysztof Kozlowski wrote: > On 21/09/2022 19:29, Laurent Pinchart wrote: > >>>>> + clock-names: > >>>>> + items: > >>>>> + - const: vclk > >>>>> + - const: pclk > >>>>> + - const: aclk > >>>> > >>>> Drop the "clk" suffixes. Remaining names could be made a bit more readable. > >>> > >>> These names come from the documentation, isn't it better to match the > >>> datasheet ? > >> > >> If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the > >> reason to use it. :) > >> > >> The "clk" is redundant even if the hardware engineer thought different. > >> > >> The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma"). > > > > I'd argue that naming clocks "v", "p" and "a" would be less readable and > > more confusing. Is this a new rule ? > > Not really, but also it's only a style issue. > > Indeed "v" and "p" are not much better... but still "vclk" does not > bring any additional information over "v". It's redundant. > > You can also drop entire entry - unless it helps in particular > implementation. There are multiple clocks, so I think names are better than indices. If you really insist we could name them "v", "p" and "a", but I think the clk suffix here improves readability. If those clocks were named "videoclk", "pixelclk" and "axiclk" I'd be fine dropping the suffix, but that's not the case here. > https://lore.kernel.org/all/20220517175958.GA1321687-robh@kernel.org/ > https://lore.kernel.org/all/20210815133926.22860-1-biju.das.jz@bp.renesas.com/ > https://lore.kernel.org/all/YYFCaHI%2FDASUz+Vu@robh.at.kernel.org/ > https://lore.kernel.org/all/20220830182540.GA1797396-robh@kernel.org/
Hi Laurent and Krzysztof, On Thu, Sep 22, 2022 at 2:46 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Wed, Sep 21, 2022 at 08:58:26PM +0200, Krzysztof Kozlowski wrote: > > On 21/09/2022 19:29, Laurent Pinchart wrote: > > >>>>> + clock-names: > > >>>>> + items: > > >>>>> + - const: vclk > > >>>>> + - const: pclk > > >>>>> + - const: aclk > > >>>> > > >>>> Drop the "clk" suffixes. Remaining names could be made a bit more readable. > > >>> > > >>> These names come from the documentation, isn't it better to match the > > >>> datasheet ? > > >> > > >> If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the > > >> reason to use it. :) > > >> > > >> The "clk" is redundant even if the hardware engineer thought different. > > >> > > >> The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma"). > > > > > > I'd argue that naming clocks "v", "p" and "a" would be less readable and > > > more confusing. Is this a new rule ? > > > > Not really, but also it's only a style issue. > > > > Indeed "v" and "p" are not much better... but still "vclk" does not > > bring any additional information over "v". It's redundant. > > > > You can also drop entire entry - unless it helps in particular > > implementation. > > There are multiple clocks, so I think names are better than indices. If > you really insist we could name them "v", "p" and "a", but I think the > clk suffix here improves readability. If those clocks were named > "videoclk", "pixelclk" and "axiclk" I'd be fine dropping the suffix, but > that's not the case here. > I have got the below details from the HW team: CRU_SYSCLK -> System clock for CSI-2 DPHY CRU_VCLK -> video clock CRU_PCLK -> APB clock CRU_ACLK -> AXI clock So I'll rename the clocks to below respectively: + clock-names: + items: + - const: system + - const: video + - const: apb + - const: axi Does the above sound good? Cheers, Prabhakar
On 30/09/2022 12:49, Lad, Prabhakar wrote: > I have got the below details from the HW team: > > CRU_SYSCLK -> System clock for CSI-2 DPHY > CRU_VCLK -> video clock > CRU_PCLK -> APB clock > CRU_ACLK -> AXI clock > > So I'll rename the clocks to below respectively: > > + clock-names: > + items: > + - const: system > + - const: video > + - const: apb > + - const: axi > > Does the above sound good? For me sounds awesome! Thank you. Best regards, Krzysztof
Hi Prabhakar, On Fri, Sep 30, 2022 at 11:49:22AM +0100, Lad, Prabhakar wrote: > On Thu, Sep 22, 2022 at 2:46 PM Laurent Pinchart wrote: > > On Wed, Sep 21, 2022 at 08:58:26PM +0200, Krzysztof Kozlowski wrote: > > > On 21/09/2022 19:29, Laurent Pinchart wrote: > > > >>>>> + clock-names: > > > >>>>> + items: > > > >>>>> + - const: vclk > > > >>>>> + - const: pclk > > > >>>>> + - const: aclk > > > >>>> > > > >>>> Drop the "clk" suffixes. Remaining names could be made a bit more readable. > > > >>> > > > >>> These names come from the documentation, isn't it better to match the > > > >>> datasheet ? > > > >> > > > >> If datasheet calls it "vclk_really_clk_it_is_clk_clk", it's not the > > > >> reason to use it. :) > > > >> > > > >> The "clk" is redundant even if the hardware engineer thought different. > > > >> > > > >> The same for IRQs ("tx" not "txirq"), for dmas ("tx" not "txdma"). > > > > > > > > I'd argue that naming clocks "v", "p" and "a" would be less readable and > > > > more confusing. Is this a new rule ? > > > > > > Not really, but also it's only a style issue. > > > > > > Indeed "v" and "p" are not much better... but still "vclk" does not > > > bring any additional information over "v". It's redundant. > > > > > > You can also drop entire entry - unless it helps in particular > > > implementation. > > > > There are multiple clocks, so I think names are better than indices. If > > you really insist we could name them "v", "p" and "a", but I think the > > clk suffix here improves readability. If those clocks were named > > "videoclk", "pixelclk" and "axiclk" I'd be fine dropping the suffix, but > > that's not the case here. > > I have got the below details from the HW team: > > CRU_SYSCLK -> System clock for CSI-2 DPHY > CRU_VCLK -> video clock > CRU_PCLK -> APB clock > CRU_ACLK -> AXI clock > > So I'll rename the clocks to below respectively: > > + clock-names: > + items: > + - const: system > + - const: video > + - const: apb > + - const: axi > > Does the above sound good? That's great, thank you !
diff --git a/Documentation/devicetree/bindings/media/renesas,rzg2l-cru.yaml b/Documentation/devicetree/bindings/media/renesas,rzg2l-cru.yaml new file mode 100644 index 000000000000..df18aeacfce3 --- /dev/null +++ b/Documentation/devicetree/bindings/media/renesas,rzg2l-cru.yaml @@ -0,0 +1,157 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (C) 2022 Renesas Electronics Corp. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/renesas,rzg2l-cru.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Renesas RZ/G2L (and alike SoC's) Camera Data Receiving Unit (CRU) Image processing + +maintainers: + - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> + +description: + The CRU image processing module is a data conversion module equipped with pixel + color space conversion, LUT, pixel format conversion, etc. An MIPI CSI-2 input and + parallel (including ITU-R BT.656) input are provided as the image sensor interface. + +properties: + compatible: + items: + - enum: + - renesas,r9a07g044-cru # RZ/G2{L,LC} + - renesas,r9a07g054-cru # RZ/V2L + - const: renesas,rzg2l-cru + + reg: + maxItems: 1 + + interrupts: + maxItems: 3 + + interrupt-names: + items: + - const: image_conv + - const: image_conv_err + - const: axi_mst_err + + clocks: + items: + - description: CRU Main clock + - description: CPU Register access clock + - description: CRU image transfer clock + + clock-names: + items: + - const: vclk + - const: pclk + - const: aclk + + power-domains: + maxItems: 1 + + resets: + items: + - description: CRU_PRESETN reset terminal + - description: CRU_ARESETN reset terminal + + reset-names: + items: + - const: presetn + - const: aresetn + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: + Input port node, single endpoint describing a parallel input source. + + properties: + endpoint: + $ref: video-interfaces.yaml# + unevaluatedProperties: false + + properties: + hsync-active: true + vsync-active: true + bus-width: true + data-shift: true + + port@1: + $ref: /schemas/graph.yaml#/properties/port + description: + Input port node, describing the Image Processing module connected to the + CSI-2 receiver. + + required: + - port@0 + - port@1 + +required: + - compatible + - reg + - interrupts + - interrupt-names + - clocks + - clock-names + - resets + - reset-names + - power-domains + +additionalProperties: false + +examples: + # Device node example with CSI-2 + - | + #include <dt-bindings/clock/r9a07g044-cpg.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + cru: video@10830000 { + compatible = "renesas,r9a07g044-cru", "renesas,rzg2l-cru"; + reg = <0x10830000 0x400>; + interrupts = <GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "image_conv", "image_conv_err", "axi_mst_err"; + clocks = <&cpg CPG_MOD R9A07G044_CRU_VCLK>, + <&cpg CPG_MOD R9A07G044_CRU_PCLK>, + <&cpg CPG_MOD R9A07G044_CRU_ACLK>; + clock-names = "vclk", "pclk", "aclk"; + power-domains = <&cpg>; + resets = <&cpg R9A07G044_CRU_PRESETN>, + <&cpg R9A07G044_CRU_ARESETN>; + reset-names = "presetn", "aresetn"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + + cru_parallel_in: endpoint@0 { + reg = <0>; + remote-endpoint= <&ov5642>; + hsync-active = <1>; + vsync-active = <1>; + }; + }; + + port@1 { + #address-cells = <1>; + #size-cells = <0>; + reg = <1>; + + cru_csi_in: endpoint@0 { + reg = <0>; + remote-endpoint= <&csi_cru_in>; + }; + }; + }; + };