media: dt-bindings: starfive,jh7110-camss: add binding document
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 14 lines · jack.zhu
Add DT binding document for Starfive Camera subsystem driver
Signed-off-by jack.zhu <[email protected]>--- .../bindings/media/starfive,jh7110-camss.yaml | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yamlDocumentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +0
-0
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 6
Quoted 6 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +6
-0@@ -0,0 +1,150 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) + +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/media/starfive,jh7110-camss.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
Drop quotes from both.
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 14
Quoted 8 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +8
-0+ +title: Starfive SoC CAMSS ISP + +maintainers: + - Jack Zhu <[email protected]> + - Changhuang Liang <[email protected]> + +description: |
No need for '|'
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 17
Quoted 3 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ The Starfive CAMSS ISP is a Camera interface for Starfive JH7110 SoC.It + consists of a VIN controller(Video In Controller, a top-level control until) + and a ISP.
"an ISP", I think
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 24
Quoted 7 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +7
-0+ +properties: + compatible: + const: starfive,jh7110-camss + + reg: + minItems: 2
Drop minItems, no need.
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 33
Quoted 9 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +9
-0+ maxItems: 2 + + reg-names: + items: + - const: syscon + - const: isp + + clocks: + minItems: 7
Drop mintems
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 44
Quoted 11 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +11
-0+ maxItems: 7 + + clock-names: + items: + - const: clk_apb_func + - const: clk_wrapper_clk_c + - const: clk_dvp_inv + - const: clk_axiwr + - const: clk_mipi_rx0_pxl + - const: clk_ispcore_2x + - const: clk_isp_axi
Drop "clk" prefix
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 47
Quoted 3 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ + resets: + minItems: 6
Drop
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 52
Quoted 5 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +5
-0+ maxItems: 6 + + reset-names: + items: + - const: rst_wrapper_p
Drop rst prefix
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 61
Quoted 9 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +9
-0+ - const: rst_wrapper_c + - const: rst_axird + - const: rst_axiwr + - const: rst_isp_top_n + - const: rst_isp_top_axi + + power-domains: + items: + - description: JH7110 PD ISP - ISP Power Domain Switch Controller.
Drop redundant pieces, e.g. "PD ISP"
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 64
Quoted 3 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ + interrupts: + minItems: 4
Drop
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 71
Quoted 7 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +7
-0+ maxItems: 4 + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@1:
And what about port@0?
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 78
Quoted 7 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +7
-0+ $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: + Input port for receiving CSI data. + + properties: + endpoint@1:
Hm, do you have more than one endpoint in this port? Why unit address?
Quoted 12 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +12
-0+ $ref: video-interfaces.yaml# + unevaluatedProperties: false + + required: + - port@1 + +required: + - compatible + - reg + - reg-names + - clocks + - clock-names
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 100
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +10
-0+ - resets + - reset-names + - power-domains + - interrupts + +additionalProperties: false + +examples: + - | +
Drop blank line
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 101
Quoted 1 line
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +1
-0+ stfcamss: camss@19840000 {
isp@
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 104
Quoted 3 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ compatible = "starfive,jh7110-camss"; + reg = <0x19840000 0x10000>, + <0x19870000 0x30000>;
All this looks misaligned
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 107
Quoted 3 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ reg-names = "syscon", "isp"; + clocks = <&ispcrg 0>, + <&ispcrg 13>,
Looks even worse...
Quoted 33 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +33
-0+ <&ispcrg 2>, + <&ispcrg 12>, + <&ispcrg 1>, + <&syscrg 51>, + <&syscrg 52>; + clock-names = "clk_apb_func", + "clk_wrapper_clk_c", + "clk_dvp_inv", + "clk_axiwr", + "clk_mipi_rx0_pxl", + "clk_ispcore_2x", + "clk_isp_axi"; + resets = <&ispcrg 0>, + <&ispcrg 1>, + <&ispcrg 10>, + <&ispcrg 11>, + <&syscrg 41>, + <&syscrg 42>; + reset-names = "rst_wrapper_p", + "rst_wrapper_c", + "rst_axird", + "rst_axiwr", + "rst_isp_top_n", + "rst_isp_top_axi"; + power-domains = <&pwrc 5>; + interrupts = <92>, <87>, <88>, <90>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@1 { + reg = <1>;
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 150
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +10
-0+ #address-cells = <1>; + #size-cells = <0>; + + vin_from_csi2rx: endpoint@1 { + reg = <1>; + remote-endpoint = <&csi2rx_to_vin>; + }; + }; + }; + };
Best regards,
Krzysztof
Quoted 9 lines · Krzysztof Kozlowski
Quoted 8 lines · jack.zhu
Add DT binding document for Starfive Camera subsystem driver
Signed-off-by jack.zhu <[email protected]>--- .../bindings/media/starfive,jh7110-camss.yaml | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
Quoted 14 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 6
Quoted 12 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +6
-0@@ -0,0 +1,150 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) + +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/media/starfive,jh7110-camss.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#"Drop quotes from both.
OK, I will fix it in V2 version.
Quoted 11 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 14
Quoted 9 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +8
-0+ +title: Starfive SoC CAMSS ISP + +maintainers: + - Jack Zhu <[email protected]> + - Changhuang Liang <[email protected]> + +description: |No need for '|'
OK, I will fix it.
Quoted 6 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 17
Quoted 4 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ The Starfive CAMSS ISP is a Camera interface for Starfive JH7110 SoC.It + consists of a VIN controller(Video In Controller, a top-level control until) + and a ISP."an ISP", I think
I will revise it.
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 24
Quoted 8 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +7
-0+ +properties: + compatible: + const: starfive,jh7110-camss + + reg: + minItems: 2Drop minItems, no need.
OK, I will drop it.
Quoted 12 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 33
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +9
-0+ maxItems: 2 + + reg-names: + items: + - const: syscon + - const: isp + + clocks: + minItems: 7Drop mintems
I will drop it.
Quoted 13 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 44
Quoted 12 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +11
-0+ maxItems: 7 + + clock-names: + items: + - const: clk_apb_func + - const: clk_wrapper_clk_c + - const: clk_dvp_inv + - const: clk_axiwr + - const: clk_mipi_rx0_pxl + - const: clk_ispcore_2x + - const: clk_isp_axi
Quoted 1 line
Drop "clk" prefix
I will drop it.
Quoted 6 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 47
Quoted 4 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ + resets: + minItems: 6Drop
I will drop it.
Quoted 8 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 52
Quoted 6 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +5
-0+ maxItems: 6 + + reset-names: + items: + - const: rst_wrapper_pDrop rst prefix
OK, will fix it.
Quoted 11 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 61
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +9
-0+ - const: rst_wrapper_c + - const: rst_axird + - const: rst_axiwr + - const: rst_isp_top_n + - const: rst_isp_top_axi + + power-domains: + items: + - description: JH7110 PD ISP - ISP Power Domain Switch Controller.
Quoted 1 line
Drop redundant pieces, e.g. "PD ISP"
OK, will fix it.
Quoted 6 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 64
Quoted 4 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ + interrupts: + minItems: 4Drop
OK, will drop it.
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 71
Quoted 8 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +7
-0+ maxItems: 4 + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@1:And what about port@0?
port@0 is reserved for DVP sensor, although it is not supported yet.
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 78
Quoted 8 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +7
-0+ $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: + Input port for receiving CSI data. + + properties: + endpoint@1:Hm, do you have more than one endpoint in this port? Why unit address?
OK, I will change it to "endpoint:"
Quoted 17 lines
Quoted 17 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +16
-0+ $ref: video-interfaces.yaml# + unevaluatedProperties: false + + required: + - port@1 + +required: + - compatible + - reg + - reg-names + - clocks + - clock-names + - resets + - reset-names + - power-domains + - interrupts
Quoted 8 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 100
Quoted 6 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +6
-0+ +additionalProperties: false + +examples: + - | +Drop blank line
I will drop it.
Quoted 4 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 101
Quoted 2 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +1
-0+ stfcamss: camss@19840000 {isp@
I will alter it.
Quoted 11 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 104
Quoted 4 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ compatible = "starfive,jh7110-camss"; + reg = <0x19840000 0x10000>, + <0x19870000 0x30000>;All this looks misaligned
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 107
Quoted 3 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +3
-0+ reg-names = "syscon", "isp"; + clocks = <&ispcrg 0>, + <&ispcrg 13>,Looks even worse...
I will fix it.
Quoted 48 lines
Quoted 34 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +33
-0+ <&ispcrg 2>, + <&ispcrg 12>, + <&ispcrg 1>, + <&syscrg 51>, + <&syscrg 52>; + clock-names = "clk_apb_func", + "clk_wrapper_clk_c", + "clk_dvp_inv", + "clk_axiwr", + "clk_mipi_rx0_pxl", + "clk_ispcore_2x", + "clk_isp_axi"; + resets = <&ispcrg 0>, + <&ispcrg 1>, + <&ispcrg 10>, + <&ispcrg 11>, + <&syscrg 41>, + <&syscrg 42>; + reset-names = "rst_wrapper_p", + "rst_wrapper_c", + "rst_axird", + "rst_axiwr", + "rst_isp_top_n", + "rst_isp_top_axi"; + power-domains = <&pwrc 5>; + interrupts = <92>, <87>, <88>, <90>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@1 { + reg = <1>;Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml line 150
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +10
-0+ #address-cells = <1>; + #size-cells = <0>; + + vin_from_csi2rx: endpoint@1 { + reg = <1>; + remote-endpoint = <&csi2rx_to_vin>; + }; + }; + }; + };Best regards,
Krzysztof
Quoted 3 lines · jack.zhu
Add DT binding document for Starfive Camera subsystem driver
Signed-off-by jack.zhu <[email protected]>
Also:
Subject: drop second/last, redundant "add binding document". The "dt-bindings" prefix is already stating that these are bindings and it is a document. Write something useful instead.
Best regards,
Krzysztof
Quoted 10 lines · Krzysztof Kozlowski
Quoted 3 lines · jack.zhu
Add DT binding document for Starfive Camera subsystem driver
Signed-off-by jack.zhu <[email protected]>Also:
Subject: drop second/last, redundant "add binding document". The "dt-bindings" prefix is already stating that these are bindings and it is a document. Write something useful instead.
Ok, I will revise the Subject
Quoted 4 lines
Best regards,
Krzysztof
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent
Quoted 1 line · jack.zhu
Add DT binding document for Starfive MIPI CSI2 receiver
Subject: drop second/last, redundant "add binding document". The "dt-bindings" prefix is already stating that these are bindings and it is a document. Write something useful instead.
Quoted 13 lines
Signed-off-by jack.zhu <[email protected]>--- .../media/starfive,jh7110-mipi-csi2.yaml | 177 ++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yamlDocumentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +0
-0
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 1
Quoted 1 line
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +1
-0@@ -0,0 +1,177 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
Why for patch 1 and 2 you are using difference SPDX?
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 13
Quoted 12 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +12
-0+%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/starfive,jh7110-mipi-csi2.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Starfive JH7110 MIPI CSI-2 receiver + +maintainers: + - Jack Zhu <[email protected]> + - Changhuang Liang <[email protected]> + +description: |-
Drop |-
Quoted 18 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +18
-0+ The JH7110 MIPI CSI-2 receiver device is responsible for handling CSI2 + protocol based camera sensor data stream. + +properties: + compatible: + enum: + - starfive,jh7110-csi2rx + + reg: + maxItems: 1 + + clocks: + items: + - description: CSI2Rx system clock + - description: Gated Register bank clock for APB interface + - description: pixel Clock for Stream interface 0 + - description: pixel Clock for Stream interface 1 + - description: pixel Clock for Stream interface 2
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 41
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +10
-0+ - description: pixel Clock for Stream interface 3 + + clock-names: + items: + - const: sys_clk + - const: p_clk + - const: pixel_if0_clk + - const: pixel_if1_clk + - const: pixel_if2_clk + - const: pixel_if3_clk
Drop _clk suffixes
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 54
Quoted 13 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +13
-0+ + resets: + items: + - description: CSI2Rx system reset + - description: Gated Register bank reset for APB interface + - description: pixel reset for Stream interface 0 + - description: pixel reset for Stream interface 1 + - description: pixel reset for Stream interface 2 + - description: pixel reset for Stream interface 3 + + reset-names: + items: + - const: sys_rst
Drop _rst suffixes
Quoted 22 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +22
-0+ - const: p_rst + - const: pixel_if0_rst + - const: pixel_if1_rst + - const: pixel_if2_rst + - const: pixel_if3_rst + + phys: + maxItems: 1 + description: MIPI D-PHY + + phy-names: + items: + - const: dphy + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description:
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 86
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +10
-0+ Input port node, single endpoint describing the CSI-2 transmitter. + + properties: + endpoint: + $ref: video-interfaces.yaml# + unevaluatedProperties: false + + properties: + bus-type: + items:
This is not an array.
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 90
Quoted 4 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +4
-0+ - const: 4 + + clock-lanes: + maxItems: 1
Not an array...
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 99
Quoted 9 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +9
-0+ + data-lanes: + minItems: 1 + maxItems: 4 + items: + maximum: 4 + + required: + - bus-type
Since this is fixed 4, do you actually require it? Why?
Quoted 18 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +18
-0+ - clock-lanes + - data-lanes + + port@1: + $ref: /schemas/graph.yaml#/properties/port + description: + Output port node + + required: + - port@0 + - port@1 + +required: + - compatible + - reg + - clocks + - clock-names + - resets
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 127
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +10
-0+ - reset-names + - phys + - phy-names + - ports + +additionalProperties: false + +examples: + - | +
Drop blank line
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 128
Quoted 1 line
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +1
-0+ csi2rx: csi-bridge@19800000 {
Maybe just "csi@"?
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 132
Quoted 4 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +4
-0+ compatible = "starfive,jh7110-csi2rx"; + reg = <0x19800000 0x10000>; + clocks = <&ispcrg 7>, + <&ispcrg 6>,
Indentation looks odd... did you align it?
Quoted 8 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +8
-0+ <&ispcrg 8>, + <&ispcrg 9>, + <&ispcrg 10>, + <&ispcrg 11>; + clock-names = "sys_clk", "p_clk", + "pixel_if0_clk", "pixel_if1_clk", + "pixel_if2_clk", "pixel_if3_clk"; + resets = <&ispcrg 9>,
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml line 150
Quoted 10 lines
Documentation/devicetree/bindings/media/starfive,jh7110-mipi-csi2.yaml new file +10
-0+ <&ispcrg 4>, + <&ispcrg 5>, + <&ispcrg 6>, + <&ispcrg 7>, + <&ispcrg 8>; + reset-names = "sys_rst", "p_rst", + "pixel_if0_rst", "pixel_if1_rst", + "pixel_if2_rst", "pixel_if3_rst"; + phys = <&csi_phy>; + phy-names = "dphy";
Best regards,
Krzysztof
Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent
In reply to a message not in the archiveQuoted 1 line · jack.zhu
Add DT binding document for Starfive MIPI CSI2 receiver
Ehh... you have entire commit msg to explain what you do here. Yet there is nothing mentioning that you actually have Cadence MIPI CSI here.
Since you decided to add new bindings, you receive review matching new bindings. I don't think this is correct approach (duplicated bindings), but could work for me. However how are you going to solve all the points of my review?
Best regards,
Krzysztof
Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent
Quoted 10 lines · Krzysztof Kozlowski
Quoted 1 line · jack.zhu
Add DT binding document for Starfive MIPI CSI2 receiver
Ehh... you have entire commit msg to explain what you do here. Yet there is nothing mentioning that you actually have Cadence MIPI CSI here.
Since you decided to add new bindings, you receive review matching new bindings. I don't think this is correct approach (duplicated bindings), but could work for me. However how are you going to solve all the points of my review?
Maybe I don't need to add the CSI yaml file, since it already exists on the Linux mainline.
Quoted 4 lines
Best regards,
Krzysztof
Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent
Quoted 9 lines · Jack Zhu
Quoted 6 lines · Krzysztof Kozlowski
Quoted 1 line · jack.zhu
Add DT binding document for Starfive MIPI CSI2 receiver
Ehh... you have entire commit msg to explain what you do here. Yet there is nothing mentioning that you actually have Cadence MIPI CSI here.
Quoted 6 lines
Quoted 4 lines
Since you decided to add new bindings, you receive review matching new bindings. I don't think this is correct approach (duplicated bindings), but could work for me. However how are you going to solve all the points of my review?
Maybe I don't need to add the CSI yaml file, since it already exists on the Linux mainline.
If you add *only* new compatible, you do not need new binding. If you add any new properties, then depends, but old binding anyway would need conversion from TXT.
Best regards,
Krzysztof
Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent
Quoted 15 lines · Krzysztof Kozlowski
Quoted 14 lines · Jack Zhu
Quoted 10 lines · Krzysztof Kozlowski
Quoted 1 line · jack.zhu
Add DT binding document for Starfive MIPI CSI2 receiver
Ehh... you have entire commit msg to explain what you do here. Yet there is nothing mentioning that you actually have Cadence MIPI CSI here.
Since you decided to add new bindings, you receive review matching new bindings. I don't think this is correct approach (duplicated bindings), but could work for me. However how are you going to solve all the points of my review?
Quoted 5 lines
Quoted 1 line
Maybe I don't need to add the CSI yaml file, since it already exists on the Linux mainline.
If you add *only* new compatible, you do not need new binding. If you add any new properties, then depends, but old binding anyway would need conversion from TXT.
have some new properties, this means that
need to convert cdns,csi2rx.txt to cdns,csi2rx.yaml and add my new attributes?
Quoted 4 lines
Best regards,
Krzysztof
Re: [PATCH v1 02/11] media: dt-bindings: starfive,jh7110-mipi-csi2: add binding docmuent
Quoted 18 lines · Jack Zhu
Quoted 15 lines · Krzysztof Kozlowski
Quoted 14 lines · Jack Zhu
Quoted 10 lines · Krzysztof Kozlowski
Quoted 1 line · jack.zhu
Add DT binding document for Starfive MIPI CSI2 receiver
Ehh... you have entire commit msg to explain what you do here. Yet there is nothing mentioning that you actually have Cadence MIPI CSI here.
Since you decided to add new bindings, you receive review matching new bindings. I don't think this is correct approach (duplicated bindings), but could work for me. However how are you going to solve all the points of my review?
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Maybe I don't need to add the CSI yaml file, since it already exists on the Linux mainline.
If you add *only* new compatible, you do not need new binding. If you add any new properties, then depends, but old binding anyway would need conversion from TXT.
have some new properties, this means that
need to convert cdns,csi2rx.txt to cdns,csi2rx.yaml and add my new attributes?
Yes.
Best regards,
Krzysztof
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v1 04/11] MAINTAINERS: add Starfive Camera subsystem driver
Quoted 12 lines · jack.zhu
Add an entry for Starfive Camera subsystem driver.
Signed-off-by jack.zhu <[email protected]>--- MAINTAINERS | 9 +++++++++ 1 file changed, 9 insertions(+)MAINTAINERS +0
-0
MAINTAINERS M: Ion Badulescu <[email protected]> · line 19915
Quoted 10 lines
MAINTAINERS +7
-0@@ -19906,6 +19906,15 @@ M: Ion Badulescu <[email protected]> S: Odd Fixes F: drivers/net/ethernet/adaptec/starfire* +STARFIVE CAMERA SUBSYSTEM DRIVER +M: Jack Zhu <[email protected]> +M: Changhuang Liang <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/admin-guide/media/starfive_camss.rst +F: Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
Why only one binding, not all of them?
Best regards,
Krzysztof
Re: [PATCH v1 04/11] MAINTAINERS: add Starfive Camera subsystem driver
Quoted 16 lines · Krzysztof Kozlowski
Quoted 15 lines · jack.zhu
Add an entry for Starfive Camera subsystem driver.
Signed-off-by jack.zhu <[email protected]>--- MAINTAINERS | 9 +++++++++ 1 file changed, 9 insertions(+)MAINTAINERS +0
-0@@ -19906,6 +19906,15 @@ M: Ion Badulescu <[email protected]> S: Odd Fixes F: drivers/net/ethernet/adaptec/starfire*
Quoted 9 lines
MAINTAINERS M: Ion Badulescu <[email protected]> · line 19915
Quoted 7 lines
MAINTAINERS +7
-0+STARFIVE CAMERA SUBSYSTEM DRIVER +M: Jack Zhu <[email protected]> +M: Changhuang Liang <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/admin-guide/media/starfive_camss.rst +F: Documentation/devicetree/bindings/media/starfive,jh7110-camss.yamlWhy only one binding, not all of them?
As we communicated in the previous emails, The file starfive,jh7110-mipi-csi2.yaml will no longer be used. I will convert cdns,csi2rx.txt to cdns,csi2rx.yaml and add some new properties. The "cdns,csi2rx.yaml" will be added in "CADENCE MIPI-CSI2 BRIDGES".
Quoted 4 lines
Best regards,
Krzysztof
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v1 11/11] media: starfive: enable building
Quoted 9 lines · jack.zhu
Add
Kconfigand Makefie, update platform/Kconfig and platform/Makefile to enable building of the Starfive Camera subsystem driver.Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/starfive/Kconfig | 18 ++++++++++++++++++ drivers/media/platform/starfive/Makefile | 14 ++++++++++++++
This is not a separate commit. If it were, it would mean you just added dead code in previous commits, so why adding dead code in first place?
Squash it.
Best regards,
Krzysztof
Re: [PATCH v1 11/11] media: starfive: enable building
Quoted 11 lines · Krzysztof Kozlowski
Quoted 9 lines · jack.zhu
Add
Kconfigand Makefie, update platform/Kconfig and platform/Makefile to enable building of the Starfive Camera subsystem driver.Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/starfive/Kconfig | 18 ++++++++++++++++++ drivers/media/platform/starfive/Makefile | 14 ++++++++++++++
Quoted 3 lines
This is not a separate commit. If it were, it would mean you just added dead code in previous commits, so why adding dead code in first place?
The previous patches are made according to the module function.I think it is helpful to explain the composition of the code file.
stf_camss[patch 9] as a platform device manages all resources including ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other resources managed by stf_camss.There is mutual reference between them. Therefore, this patch is used for the overall compilation of the starfive directory.
Quoted 5 lines
Squash it.
Best regards,
Krzysztof
Re: [PATCH v1 11/11] media: starfive: enable building
Quoted 18 lines · Jack Zhu
Quoted 11 lines · Krzysztof Kozlowski
Quoted 9 lines · jack.zhu
Add
Kconfigand Makefie, update platform/Kconfig and platform/Makefile to enable building of the Starfive Camera subsystem driver.Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/starfive/Kconfig | 18 ++++++++++++++++++ drivers/media/platform/starfive/Makefile | 14 ++++++++++++++Quoted 3 lines
This is not a separate commit. If it were, it would mean you just added dead code in previous commits, so why adding dead code in first place?
Quoted 8 lines
The previous patches are made according to the module function.I think it is helpful to explain the composition of the code file.
stf_camss[patch 9] as a platform device manages all resources including ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other resources managed by stf_camss.There is mutual reference between them. Therefore, this patch is used for the overall compilation of the starfive directory.
So previous code is dead? Again, what is the reason for adding dead code? Mutual reference is not the answer.
Best regards,
Krzysztof
Re: [PATCH v1 11/11] media: starfive: enable building
Quoted 22 lines · Krzysztof Kozlowski
Quoted 21 lines · Jack Zhu
Quoted 11 lines · Krzysztof Kozlowski
Quoted 9 lines · jack.zhu
Add
Kconfigand Makefie, update platform/Kconfig and platform/Makefile to enable building of the Starfive Camera subsystem driver.Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/starfive/Kconfig | 18 ++++++++++++++++++ drivers/media/platform/starfive/Makefile | 14 ++++++++++++++Quoted 3 lines
This is not a separate commit. If it were, it would mean you just added dead code in previous commits, so why adding dead code in first place?
The previous patches are made according to the module function.I think it is helpful to explain the composition of the code file.
Quoted 9 lines
Quoted 5 lines
stf_camss[patch 9] as a platform device manages all resources including ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other resources managed by stf_camss.There is mutual reference between them. Therefore, this patch is used for the overall compilation of the starfive directory.
So previous code is dead? Again, what is the reason for adding dead code? Mutual reference is not the answer.
Maybe I need to merge the previous patches[6-11] into one patch to submit?
Quoted 3 lines
Best regards,
Krzysztof
Re: [PATCH v1 11/11] media: starfive: enable building
Quoted 31 lines · Jack Zhu
Quoted 28 lines · Krzysztof Kozlowski
Quoted 26 lines · Jack Zhu
Quoted 11 lines · Krzysztof Kozlowski
Quoted 9 lines · jack.zhu
Add
Kconfigand Makefie, update platform/Kconfig and platform/Makefile to enable building of the Starfive Camera subsystem driver.Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/starfive/Kconfig | 18 ++++++++++++++++++ drivers/media/platform/starfive/Makefile | 14 ++++++++++++++Quoted 3 lines
This is not a separate commit. If it were, it would mean you just added dead code in previous commits, so why adding dead code in first place?
The previous patches are made according to the module function.I think it is helpful to explain the composition of the code file.
stf_camss[patch 9] as a platform device manages all resources including ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other resources managed by stf_camss.There is mutual reference between them. Therefore, this patch is used for the overall compilation of the starfive directory.
Quoted 5 lines
Quoted 3 lines
So previous code is dead? Again, what is the reason for adding dead code? Mutual reference is not the answer.
Maybe I need to merge the previous patches[6-11] into one patch to submit?
I gave you the recommendation in my first reply. What's wrong with it?
Best regards,
Krzysztof
Re: [PATCH v1 11/11] media: starfive: enable building
Quoted 32 lines · Krzysztof Kozlowski
Quoted 31 lines · Jack Zhu
Quoted 28 lines · Krzysztof Kozlowski
Quoted 26 lines · Jack Zhu
Quoted 11 lines · Krzysztof Kozlowski
Quoted 9 lines · jack.zhu
Add
Kconfigand Makefie, update platform/Kconfig and platform/Makefile to enable building of the Starfive Camera subsystem driver.Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/starfive/Kconfig | 18 ++++++++++++++++++ drivers/media/platform/starfive/Makefile | 14 ++++++++++++++Quoted 3 lines
This is not a separate commit. If it were, it would mean you just added dead code in previous commits, so why adding dead code in first place?
The previous patches are made according to the module function.I think it is helpful to explain the composition of the code file.
stf_camss[patch 9] as a platform device manages all resources including ISP and VIN. ISP/VIN [patch 7/8]as a sub-device needs to access other resources managed by stf_camss.There is mutual reference between them. Therefore, this patch is used for the overall compilation of the starfive directory.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
So previous code is dead? Again, what is the reason for adding dead code? Mutual reference is not the answer.
Maybe I need to merge the previous patches[6-11] into one patch to submit?
I gave you the recommendation in my first reply. What's wrong with it?
Thank you for your suggestion and the recommended action which I just confirmed it in detail in our previous messages.I agree with you that this is not a separate commit.I will merge them in the next version to be a single commit.
This is my first time to submit code to the community.There are many things to learn. I sincerely thank you for your patient help and advice.
Quoted 3 lines
Best regards,
Krzysztof
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v1 10/11] media: uapi: add user space interfaces
Quoted 1 line · jack.zhu
This patch adds user space interfaces for ISP.
Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
Why this is a separate commit? User-space API is usually added with its implementation. Otherwise how does your code even compile?
Best regards,
Krzysztof
Re: [PATCH v1 10/11] media: uapi: add user space interfaces
Quoted 6 lines · Krzysztof Kozlowski
Quoted 1 line · jack.zhu
This patch adds user space interfaces for ISP.
Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
OK, I will fix it.
Quoted 4 lines
Why this is a separate commit? User-space API is usually added with its implementation. Otherwise how does your code even compile?
OK, I will merge them.
thank you for your comments.
Quoted 3 lines
Best regards,
Krzysztof
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v1 06/11] media: starfive: add ISP driver files
Quoted 14 lines · jack.zhu
Add base driver for Starfive Image Signal Processing Unit which handles the data streams from the CSI2 receiver.
Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/starfive/stf_isp.c | 1079 ++++++++++++++ drivers/media/platform/starfive/stf_isp.h | 183 +++ .../media/platform/starfive/stf_isp_hw_ops.c | 1286 +++++++++++++++++ 3 files changed, 2548 insertions(+) create mode 100644 drivers/media/platform/starfive/stf_isp.c create mode 100644 drivers/media/platform/starfive/stf_isp.h create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
Review
Quoted 6 lines
(quoted diff) +6
-0+} + +static int stf_isp_reg_read(struct stf_isp_dev *isp_dev, void *arg) +{ + void __iomem *ispbase = stf_isp_get_ispbase(isp_dev); + struct isp_reg_param *reg_param = arg;
Didn't you add now code which does not even build and is not bisectable?
Best regards,
Krzysztof
Re: [PATCH v1 06/11] media: starfive: add ISP driver files
Quoted 15 lines · Krzysztof Kozlowski
Quoted 12 lines · jack.zhu
Add base driver for Starfive Image Signal Processing Unit which handles the data streams from the CSI2 receiver.
Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/starfive/stf_isp.c | 1079 ++++++++++++++ drivers/media/platform/starfive/stf_isp.h | 183 +++ .../media/platform/starfive/stf_isp_hw_ops.c | 1286 +++++++++++++++++ 3 files changed, 2548 insertions(+) create mode 100644 drivers/media/platform/starfive/stf_isp.c create mode 100644 drivers/media/platform/starfive/stf_isp.h create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
Quoted 10 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+} + +static int stf_isp_reg_read(struct stf_isp_dev *isp_dev, void *arg) +{ + void __iomem *ispbase = stf_isp_get_ispbase(isp_dev); + struct isp_reg_param *reg_param = arg;Didn't you add now code which does not even build and is not bisectable?
use patch 11 to build all files in the starfive directory.
Quoted 4 lines
Best regards,
Krzysztof
Re: [PATCH v1 06/11] media: starfive: add ISP driver files
Quoted 29 lines · Jack Zhu
Quoted 15 lines · Krzysztof Kozlowski
Quoted 12 lines · jack.zhu
Add base driver for Starfive Image Signal Processing Unit which handles the data streams from the CSI2 receiver.
Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/starfive/stf_isp.c | 1079 ++++++++++++++ drivers/media/platform/starfive/stf_isp.h | 183 +++ .../media/platform/starfive/stf_isp_hw_ops.c | 1286 +++++++++++++++++ 3 files changed, 2548 insertions(+) create mode 100644 drivers/media/platform/starfive/stf_isp.c create mode 100644 drivers/media/platform/starfive/stf_isp.h create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.cQuoted 10 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+} + +static int stf_isp_reg_read(struct stf_isp_dev *isp_dev, void *arg) +{ + void __iomem *ispbase = stf_isp_get_ispbase(isp_dev); + struct isp_reg_param *reg_param = arg;Didn't you add now code which does not even build and is not bisectable?
Quoted 2 lines
use patch 11 to build all files in the starfive directory.
Not related. So let me put it very simple:
1. Does this code build or not?
2. Do you certify that code is 100% bisectable?
Best regards,
Krzysztof
Re: [PATCH v1 06/11] media: starfive: add ISP driver files
Quoted 30 lines · Krzysztof Kozlowski
Quoted 29 lines · Jack Zhu
Quoted 25 lines · Krzysztof Kozlowski
Quoted 14 lines · jack.zhu
Add base driver for Starfive Image Signal Processing Unit which handles the data streams from the CSI2 receiver.
Signed-off-by jack.zhu <[email protected]>--- drivers/media/platform/starfive/stf_isp.c | 1079 ++++++++++++++ drivers/media/platform/starfive/stf_isp.h | 183 +++ .../media/platform/starfive/stf_isp_hw_ops.c | 1286 +++++++++++++++++ 3 files changed, 2548 insertions(+) create mode 100644 drivers/media/platform/starfive/stf_isp.c create mode 100644 drivers/media/platform/starfive/stf_isp.h create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.cReview
Quoted 6 lines
(quoted diff) +6
-0+} + +static int stf_isp_reg_read(struct stf_isp_dev *isp_dev, void *arg) +{ + void __iomem *ispbase = stf_isp_get_ispbase(isp_dev); + struct isp_reg_param *reg_param = arg;Didn't you add now code which does not even build and is not bisectable?
Quoted 7 lines
Quoted 2 lines
use patch 11 to build all files in the starfive directory.
Not related. So let me put it very simple:
1. Does this code build or not?
2. Do you certify that code is 100% bisectable?
No, Maybe I need to merge the previous patches[6-11] into one patch to submit?
Quoted 5 lines
Best regards,
Krzysztof