| Message ID | 20240624-b4-sc7180-camss-v3-1-89ece6471431@gmail.com |
|---|---|
| State | New |
| 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 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 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 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 Thu, Jun 27, 2024 at 11:40 PM Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > 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 Since my phone is EOL, I can't test new patches so I can only give up from here. My suggestion is to drop this atm and let sc7280 patches get in early.
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>; + }; + }; + };