| Message ID | 20240624-b4-sc7180-camss-v3-1-89ece6471431@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add sc7180 camss subsys support | expand |
On 24/06/2024 13:13, George Chan via B4 Relay wrote: > From: George Chan <gchan9527@gmail.com> > > Add bindings for qcom,sc7180-camss in order to support the camera > subsystem for sm7125 as found in the Xiaomi Redmi 9 Pro cellphone. > > Signed-off-by: George Chan <gchan9527@gmail.com> > --- > .../bindings/media/qcom,sc7180-camss.yaml | 328 +++++++++++++++++++++ > 1 file changed, 328 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml > new file mode 100644 > index 000000000000..58ffa4944857 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml > @@ -0,0 +1,328 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > + > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Camera SubSystem > + > +maintainers: > + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> Please add yourself here. Other than that Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On Wed, Jun 26, 2024 at 7:32 AM Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 24/06/2024 13:13, George Chan via B4 Relay wrote: > > From: George Chan <gchan9527@gmail.com> > > > > Add bindings for qcom,sc7180-camss in order to support the camera > > subsystem for sm7125 as found in the Xiaomi Redmi 9 Pro cellphone. > > > > Signed-off-by: George Chan <gchan9527@gmail.com> > > --- > > .../bindings/media/qcom,sc7180-camss.yaml | 328 +++++++++++++++++++++ > > 1 file changed, 328 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml > > new file mode 100644 > > index 000000000000..58ffa4944857 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml > > @@ -0,0 +1,328 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > + > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Qualcomm Camera SubSystem > > + > > +maintainers: > > + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > Please add yourself here. > > Other than that > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Would you please be the maintainer afterward? I foresee that there will be difficulties maintaining it my side in the long run.
On 24/06/2024 14:13, George Chan via B4 Relay wrote: > From: George Chan <gchan9527@gmail.com> > > Add bindings for qcom,sc7180-camss in order to support the camera > subsystem for sm7125 as found in the Xiaomi Redmi 9 Pro cellphone. ... > + > +required: > + - clock-names > + - clocks > + - compatible Nothing improved here. I asked you at v2 to go through all comments and respond to each of them or implement each of them. BTW, I asked for subject to keep only one, first "media" prefix: "Subject: just one media (first). " but you kept the second "media". Best regards, Krzysztof
On Wed, Jun 26, 2024 at 2:12 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 24/06/2024 14:13, George Chan via B4 Relay wrote: > > From: George Chan <gchan9527@gmail.com> > > > > Add bindings for qcom,sc7180-camss in order to support the camera > > subsystem for sm7125 as found in the Xiaomi Redmi 9 Pro cellphone. > > > ... > > > + > > +required: > > + - clock-names > > + - clocks > > + - compatible > > Nothing improved here. > > I asked you at v2 to go through all comments and respond to each of them > or implement each of them. > >>> Keep the list ordered, the same as list properties. I am a bit confused. Is it by ascending order or by particular order like below the same ordering to the example node? required: - compatible - reg - reg-names - clock-names - clocks > BTW, I asked for subject to keep only one, first "media" prefix: > "Subject: just one media (first). " > but you kept the second "media". Sorry I can't get it. Could you choose one? _ORIGINAL_ dt-bindings: media: camss: Add qcom,sc7180-camss _PREFERED_ media: camss: dt-bindings: Add qcom,sc7180-camss _OR_ dt-bindings: media: Add qcom,sc7180-camss > > > Best regards, > Krzysztof >
On 26/06/2024 08:46, george chan wrote: > On Wed, Jun 26, 2024 at 2:12 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On 24/06/2024 14:13, George Chan via B4 Relay wrote: >>> From: George Chan <gchan9527@gmail.com> >>> >>> Add bindings for qcom,sc7180-camss in order to support the camera >>> subsystem for sm7125 as found in the Xiaomi Redmi 9 Pro cellphone. >> >> >> ... >> >>> + >>> +required: >>> + - clock-names >>> + - clocks >>> + - compatible >> >> Nothing improved here. >> >> I asked you at v2 to go through all comments and respond to each of them >> or implement each of them. >> >>>> Keep the list ordered, the same as list properties. > I am a bit confused. Is it by ascending order or by particular order > like below the same ordering to the example node? Feel free to ask a question if comment is not clear. Keep the list in "required:" in the same order as the list in "properties:". > required: > - compatible > - reg > - reg-names > - clock-names > - clocks > >> BTW, I asked for subject to keep only one, first "media" prefix: >> "Subject: just one media (first). " >> but you kept the second "media". > > Sorry I can't get it. Could you choose one? > > _ORIGINAL_ > dt-bindings: media: camss: Add qcom,sc7180-camss No, original was different. Go back to your first posting. I asked to remove one media and keep only one - the first. I did not ask to re-shuffle the prefixes. Best regards, Krzysztof
On Wed, Jun 26, 2024 at 3:15 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > Keep the list in "required:" in the same order as the list in "properties:". ok gotcha > >> BTW, I asked for subject to keep only one, first "media" prefix: > >> "Subject: just one media (first). " > >> but you kept the second "media". > > > > Sorry I can't get it. Could you choose one? > > > > _ORIGINAL_ > > dt-bindings: media: camss: Add qcom,sc7180-camss > > No, original was different. Go back to your first posting. I asked to > remove one media and keep only one - the first. I did not ask to > re-shuffle the prefixes. Yes, let me sum it up v1 title is w.r.t https://patchwork.kernel.org/project/linux-arm-msm/patch/20240222-b4-camss-sc8280xp-v6-1-0e0e6a2f8962@linaro.org/ then extra "camss" pre-fix keyword and "binding" post-fix is not needed. v2 wrongly remove all prefixes and correctly removed post-fix v3 added correct prefix, removed redundancy "camss" prefixes but changelog still refer to old sc8280xp style The title now should be fine. So I will modify the changelog only. So there are 2 todo items as above. Other than above, all review items are addressed. Plz confirm.
On Wed, Jun 26, 2024 at 4:17 PM george chan <gchan9527@gmail.com> wrote: > > On Wed, Jun 26, 2024 at 3:15 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > Keep the list in "required:" in the same order as the list in "properties:". > > ok gotcha btw, i checked "required:" and "properties:" are aligned, both of them are in ascending order. I am wondering if you are talking about two things, 1st one is to align both property, and 2nd is having the ordering like below. Plz confirm. required: - compatible - reg - reg-names - interrupt-names - interrupts - clock-names - clocks - iommus - power-domains - power-domain-names - vdda-phy-supply - vdda-pll-supply > > >> BTW, I asked for subject to keep only one, first "media" prefix: > > >> "Subject: just one media (first). " > > >> but you kept the second "media". > > > > > > Sorry I can't get it. Could you choose one? > > > > > > _ORIGINAL_ > > > dt-bindings: media: camss: Add qcom,sc7180-camss > > > > No, original was different. Go back to your first posting. I asked to > > remove one media and keep only one - the first. I did not ask to > > re-shuffle the prefixes. > Yes, let me sum it up > > v1 title is w.r.t > https://patchwork.kernel.org/project/linux-arm-msm/patch/20240222-b4-camss-sc8280xp-v6-1-0e0e6a2f8962@linaro.org/ > then extra "camss" pre-fix keyword and "binding" post-fix is not needed. > v2 wrongly remove all prefixes and correctly removed post-fix > v3 added correct prefix, removed redundancy "camss" prefixes but > changelog still refer to old sc8280xp style > > The title now should be fine. So I will modify the changelog only. > > So there are 2 todo items as above. Other than above, all review items > are addressed. Plz confirm.
On 26/06/2024 10:17, george chan wrote: > On Wed, Jun 26, 2024 at 3:15 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> Keep the list in "required:" in the same order as the list in "properties:". > > ok gotcha > >>>> BTW, I asked for subject to keep only one, first "media" prefix: >>>> "Subject: just one media (first). " >>>> but you kept the second "media". >>> >>> Sorry I can't get it. Could you choose one? >>> >>> _ORIGINAL_ >>> dt-bindings: media: camss: Add qcom,sc7180-camss >> >> No, original was different. Go back to your first posting. I asked to >> remove one media and keep only one - the first. I did not ask to >> re-shuffle the prefixes. > Yes, let me sum it up > > v1 title is w.r.t > https://patchwork.kernel.org/project/linux-arm-msm/patch/20240222-b4-camss-sc8280xp-v6-1-0e0e6a2f8962@linaro.org/ > then extra "camss" pre-fix keyword and "binding" post-fix is not needed. Where did I write anything about camss? I already said it twice that I meant "media". Best regards, Krzysztof
On 26/06/2024 10:38, george chan wrote: > On Wed, Jun 26, 2024 at 4:17 PM george chan <gchan9527@gmail.com> wrote: >> >> On Wed, Jun 26, 2024 at 3:15 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> Keep the list in "required:" in the same order as the list in "properties:". >> >> ok gotcha > btw, i checked "required:" and "properties:" are aligned, both of No, they are not. Which is the first entry in "properties"? Which is the first entry in "required"? Please stop wasting reviewers time by disagreeing on every little piece of this. The feedback was quite clear but somehow you do not read it and respond with some inaccurate statements. Best regards, Krzysztof
On Wed, Jun 26, 2024 at 4:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 26/06/2024 10:38, george chan wrote: > > On Wed, Jun 26, 2024 at 4:17 PM george chan <gchan9527@gmail.com> wrote: > >> > >> On Wed, Jun 26, 2024 at 3:15 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > >>> Keep the list in "required:" in the same order as the list in "properties:". > >> > >> ok gotcha > > btw, i checked "required:" and "properties:" are aligned, both of > > No, they are not. > > Which is the first entry in "properties"? > > Which is the first entry in "required"? > > Please stop wasting reviewers time by disagreeing on every little piece > of this. The feedback was quite clear but somehow you do not read it and > respond with some inaccurate statements. > > Best regards, > Krzysztof > Then my apology. I might take a break here. Appreciated if some developer is willing to take over it too.
On Wed, Jun 26, 2024 at 04:38:48PM GMT, george chan wrote: > On Wed, Jun 26, 2024 at 4:17 PM george chan <gchan9527@gmail.com> wrote: > > > > On Wed, Jun 26, 2024 at 3:15 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > Keep the list in "required:" in the same order as the list in "properties:". > > > > ok gotcha > btw, i checked "required:" and "properties:" are aligned, both of > them are in ascending order. I am wondering if you are talking about > two things, 1st one is to align both property, and 2nd is having the > ordering like below. Plz confirm. > > required: > - compatible > - reg > - reg-names > - interrupt-names > - interrupts > - clock-names > - clocks > - iommus > - power-domains > - power-domain-names > - vdda-phy-supply > - vdda-pll-supply Yes, now this looks like a correct order.
On 26/06/2024 10:04, george chan wrote: > On Wed, Jun 26, 2024 at 4:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On 26/06/2024 10:38, george chan wrote: >>> On Wed, Jun 26, 2024 at 4:17 PM george chan <gchan9527@gmail.com> wrote: >>>> >>>> On Wed, Jun 26, 2024 at 3:15 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >>>>> Keep the list in "required:" in the same order as the list in "properties:". >>>> >>>> ok gotcha >>> btw, i checked "required:" and "properties:" are aligned, both of >> >> No, they are not. >> >> Which is the first entry in "properties"? >> >> Which is the first entry in "required"? >> >> Please stop wasting reviewers time by disagreeing on every little piece >> of this. The feedback was quite clear but somehow you do not read it and >> respond with some inaccurate statements. >> >> Best regards, >> Krzysztof >> > > Then my apology. I might take a break here. Appreciated if some > developer is willing to take over it too. George are you resending this with Krzysztof's comments addressed ? I'm trying to figure out what we are targeting for merge. --- bod
diff --git a/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml new file mode 100644 index 000000000000..58ffa4944857 --- /dev/null +++ b/Documentation/devicetree/bindings/media/qcom,sc7180-camss.yaml @@ -0,0 +1,328 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) + +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/qcom,sc7180-camss.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Camera SubSystem + +maintainers: + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> + +description: + The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms + +properties: + compatible: + const: qcom,sc7180-camss + + clocks: + maxItems: 24 + + clock-names: + items: + - const: camnoc_axi + - const: cpas_ahb + - const: csi0 + - const: csi1 + - const: csi2 + - const: csiphy0 + - const: csiphy0_timer + - const: csiphy1 + - const: csiphy1_timer + - const: csiphy2 + - const: csiphy2_timer + - const: csiphy3 + - const: csiphy3_timer + - const: gcc_camera_ahb + - const: gcc_camera_axi + - const: soc_ahb + - const: vfe0_axi + - const: vfe0 + - const: vfe0_cphy_rx + - const: vfe1_axi + - const: vfe1 + - const: vfe1_cphy_rx + - const: vfe_lite + - const: vfe_lite_cphy_rx + + interrupts: + maxItems: 10 + + interrupt-names: + items: + - const: csid0 + - const: csid1 + - const: csid2 + - const: csiphy0 + - const: csiphy1 + - const: csiphy2 + - const: csiphy3 + - const: vfe0 + - const: vfe1 + - const: vfe_lite + + iommus: + maxItems: 3 + + power-domains: + items: + - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller. + - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller. + - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller. + + power-domain-names: + items: + - const: ife0 + - const: ife1 + - const: top + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + description: + CSI input ports. + + properties: + port@0: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: + Input port for receiving CSI data. + + properties: + endpoint: + $ref: video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + maxItems: 4 + + required: + - data-lanes + + port@1: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: + Input port for receiving CSI data. + + properties: + endpoint: + $ref: video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + maxItems: 4 + + required: + - data-lanes + + port@2: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: + Input port for receiving CSI data. + + properties: + endpoint: + $ref: video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + maxItems: 4 + + required: + - data-lanes + + port@3: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: + Input port for receiving CSI data. + + properties: + endpoint: + $ref: video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + maxItems: 4 + + required: + - data-lanes + + reg: + maxItems: 10 + + reg-names: + items: + - const: csid0 + - const: csid1 + - const: csid2 + - const: csiphy0 + - const: csiphy1 + - const: csiphy2 + - const: csiphy3 + - const: vfe0 + - const: vfe1 + - const: vfe_lite + + vdda-phy-supply: + description: + Phandle to a regulator supply to PHY core block. + + vdda-pll-supply: + description: + Phandle to 1.8V regulator supply to PHY refclk pll block. + +required: + - clock-names + - clocks + - compatible + - interrupt-names + - interrupts + - iommus + - power-domains + - power-domain-names + - reg + - reg-names + - vdda-phy-supply + - vdda-pll-supply + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/qcom,camcc-sc7180.h> + #include <dt-bindings/clock/qcom,gcc-sc7180.h> + #include <dt-bindings/power/qcom-rpmpd.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + camss: camss@acb3000 { + compatible = "qcom,sc7180-camss"; + + reg = <0 0xacb3000 0 0x1000>, + <0 0xacba000 0 0x1000>, + <0 0xacc8000 0 0x1000>, + <0 0xac65000 0 0x1000>, + <0 0xac66000 0 0x1000>, + <0 0xac67000 0 0x1000>, + <0 0xac68000 0 0x1000>, + <0 0xacaf000 0 0x4000>, + <0 0xacb6000 0 0x4000>, + <0 0xacc4000 0 0x4000>; + + reg-names = "csid0", + "csid1", + "csid2", + "csiphy0", + "csiphy1", + "csiphy2", + "csiphy3", + "vfe0", + "vfe1", + "vfe_lite"; + + vdda-phy-supply = <&vreg_l1a_0p875>; + vdda-pll-supply = <&vreg_l26a_1p2>; + + interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-names = "csid0", + "csid1", + "csid2", + "csiphy0", + "csiphy1", + "csiphy2", + "csiphy3", + "vfe0", + "vfe1", + "vfe_lite"; + + power-domains = <&camcc IFE_0_GDSC>, + <&camcc IFE_1_GDSC>, + <&camcc TITAN_TOP_GDSC>; + + power-domain-names = "ife0", + "ife1", + "top"; + + clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>, + <&camcc CAM_CC_CPAS_AHB_CLK>, + <&camcc CAM_CC_IFE_0_CSID_CLK>, + <&camcc CAM_CC_IFE_1_CSID_CLK>, + <&camcc CAM_CC_IFE_LITE_CSID_CLK>, + <&camcc CAM_CC_CSIPHY0_CLK>, + <&camcc CAM_CC_CSI0PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY1_CLK>, + <&camcc CAM_CC_CSI1PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY2_CLK>, + <&camcc CAM_CC_CSI2PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY3_CLK>, + <&camcc CAM_CC_CSI3PHYTIMER_CLK>, + <&gcc GCC_CAMERA_AHB_CLK>, + <&gcc GCC_CAMERA_HF_AXI_CLK>, + <&camcc CAM_CC_SOC_AHB_CLK>, + <&camcc CAM_CC_IFE_0_AXI_CLK>, + <&camcc CAM_CC_IFE_0_CLK>, + <&camcc CAM_CC_IFE_0_CPHY_RX_CLK>, + <&camcc CAM_CC_IFE_1_AXI_CLK>, + <&camcc CAM_CC_IFE_1_CLK>, + <&camcc CAM_CC_IFE_1_CPHY_RX_CLK>, + <&camcc CAM_CC_IFE_LITE_CLK>, + <&camcc CAM_CC_IFE_LITE_CPHY_RX_CLK>; + + clock-names = "camnoc_axi", + "cpas_ahb", + "csi0", + "csi1", + "csi2", + "csiphy0", + "csiphy0_timer", + "csiphy1", + "csiphy1_timer", + "csiphy2", + "csiphy2_timer", + "csiphy3", + "csiphy3_timer", + "gcc_camera_ahb", + "gcc_camera_axi", + "soc_ahb", + "vfe0_axi", + "vfe0", + "vfe0_cphy_rx", + "vfe1_axi", + "vfe1", + "vfe1_cphy_rx", + "vfe_lite", + "vfe_lite_cphy_rx"; + + iommus = <&apps_smmu 0x820 0x0>, + <&apps_smmu 0x840 0x0>, + <&apps_smmu 0x860 0x0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + }; + }; + };