| Message ID | 20241127100421.3447601-5-quic_vikramsa@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: qcom: camss: Add sc7280 support | expand |
On 27/11/2024 11:04, Vikram Sharma wrote: > Add changes to support the camera subsystem on the SC7280. > > Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com> > Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com> > Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 170 +++++++++++++++++++++++++++ > 1 file changed, 170 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 55db1c83ef55..9376755ac43e 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -4426,6 +4426,176 @@ cci1_i2c1: i2c-bus@1 { > }; > }; > > + camss: camss@acaf000 { > + compatible = "qcom,sc7280-camss"; > + > + reg = <0x0 0x0acb3000 0x0 0x1000>, You received feedback to really start testing your code with tools. Nothing improved. Do you have troubles running tools or something in feedback was not clear? If first - ask. If second - respond to our comments, so we can clarify. Repeating the same mistake over and over and asking us to point you the same issue is waste of our time. Sorry, that's not acceptable to me. NAK. Best regards, Krzysztof
On 27/11/2024 19:32, Krzysztof Kozlowski wrote: > On 27/11/2024 11:04, Vikram Sharma wrote: >> Add changes to support the camera subsystem on the SC7280. >> >> Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com> >> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com> >> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sc7280.dtsi | 170 +++++++++++++++++++++++++++ >> 1 file changed, 170 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> index 55db1c83ef55..9376755ac43e 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> @@ -4426,6 +4426,176 @@ cci1_i2c1: i2c-bus@1 { >> }; >> }; >> >> + camss: camss@acaf000 { >> + compatible = "qcom,sc7280-camss"; >> + >> + reg = <0x0 0x0acb3000 0x0 0x1000>, > > You received feedback to really start testing your code with tools. > Nothing improved. Do you have troubles running tools or something in > feedback was not clear? If first - ask. If second - respond to our > comments, so we can clarify. > > Repeating the same mistake over and over and asking us to point you the > same issue is waste of our time. Sorry, that's not acceptable to me. > > NAK. I saw now previous threads where multiple times I asked the same, e.g. following coding style. Only at v6 you started doing it. About the tools, let's recap - we expect absolutely 0 warnings for: 1. `make dtbs_check W=1` (see Documentation/devicetree/bindings/writing-schema.rst or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ for instructions). 2. standard kernel tools for static analysis, like coccinelle, smatch and sparse, and fix reported warnings. Also please check for warnings when building with W=1. Most of these commands (checks or W=1 build) can build specific targets, like some directory, to narrow the scope to only your code. The code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. (so 4 tools/checks above) 3. clang with W=1 Best regards, Krzysztof
On 27/11/2024 21:33, Vikram Sharma wrote: > Hi Krzysztof, > > Thanks for your comments. > > I used below tools on my changes and here are the results. > > 1) checkpatch.pl : > /scripts/checkpatch.pl > patches_camss_v6/v6-0001-media-dt-bindings-Add-qcom-sc7280-camss.patch > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? > #18: > new file mode 100644 > total: 0 errors, 1 warnings, 415 lines checked > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or > --fix-inplace. > patches_camss_v6/v6-0001-media-dt-bindings-Add-qcom-sc7280-camss.patch > has style problems, please review. > NOTE: If any of the errors are false positives, please report > them to the maintainer, see CHECKPATCH in MAINTAINERS. > > For maintainer related warnings I cross-checked that MAINTAINERS are > already updated properly. > Patch 2,3, 4 had zero errors and warnings. > Patch 5 have same Maintainer Warnings. > > For DT validation i am using these tools as per below link: > https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ > > 2) dt_binding_check : > touch Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml > result: > *make DT_CHECKER_FLAGS=-m DT_SCHEMA_FILES=media/qcom,sc7280-camss.yaml > dt_binding_check* > SCHEMA Documentation/devicetree/bindings/processed-schema.json > */local/mnt/workspace/k2c_build/vikram/v6_submit/linux-next/Documentation/devicetree/bindings/net/snps,dwmac.yaml: > mac-mode: missing type definition -> Not related to this change.* > CHKDT ./Documentation/devicetree/bindings > LINT ./Documentation/devicetree/bindings > DTEX > Documentation/devicetree/bindings/media/qcom,sc7280-camss.example.dts > DTC [C] > Documentation/devicetree/bindings/media/qcom,sc7280-camss.example.dtb > > 3) CHECK_DTBS: > result: > touch arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso > touch arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts > > *make CHECK_DTBS=y qcom/qcs6490-rb3gen2-vision-mezzanine.dtb* > DTC [C] arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dtb > DTC arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtbo > OVL [C] arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtb > > To confirm if my tools are working properly. I tried inducing errors too. > Till this point there were no issues reported by tools. > > As per your comment I have added 'W=1' which gave 1 warning. > arch/arm64/boot/dts/qcom/sc7280.dtsi:4429.24-4597.5: Warning > (simple_bus_reg): /soc@0/camss@acaf000: simple-bus unit address format > error, expected "acb3000" This is the warning which we are not suppose to remind you more than once. > > Till this point I missed to add 'W=1' as I was following the blog. I > will address that and submit v7 Blog is just short instruction, you should refer to SoC maintainer profiles in Linux kernel. Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi index 55db1c83ef55..9376755ac43e 100644 --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi @@ -4426,6 +4426,176 @@ cci1_i2c1: i2c-bus@1 { }; }; + camss: camss@acaf000 { + compatible = "qcom,sc7280-camss"; + + reg = <0x0 0x0acb3000 0x0 0x1000>, + <0x0 0x0acc8000 0x0 0x1000>, + <0x0 0x0acba000 0x0 0x1000>, + <0x0 0x0accf000 0x0 0x1000>, + <0x0 0x0acc1000 0x0 0x1000>, + <0x0 0x0ace0000 0x0 0x2000>, + <0x0 0x0ace2000 0x0 0x2000>, + <0x0 0x0ace4000 0x0 0x2000>, + <0x0 0x0ace6000 0x0 0x2000>, + <0x0 0x0ace8000 0x0 0x2000>, + <0x0 0x0acaf000 0x0 0x4000>, + <0x0 0x0acc4000 0x0 0x4000>, + <0x0 0x0acb6000 0x0 0x4000>, + <0x0 0x0accb000 0x0 0x4000>, + <0x0 0x0acbd000 0x0 0x4000>; + reg-names = "csid0", + "csid0_lite", + "csid1", + "csid1_lite", + "csid2", + "csiphy0", + "csiphy1", + "csiphy2", + "csiphy3", + "csiphy4", + "vfe0", + "vfe0_lite", + "vfe1", + "vfe1_lite", + "vfe2"; + + clocks = <&camcc CAM_CC_CAMNOC_AXI_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>, + <&camcc CAM_CC_CSIPHY4_CLK>, + <&camcc CAM_CC_CSI4PHYTIMER_CLK>, + <&gcc GCC_CAMERA_AHB_CLK>, + <&gcc GCC_CAMERA_HF_AXI_CLK>, + <&camcc CAM_CC_CPAS_AHB_CLK>, + <&camcc CAM_CC_IFE_0_CLK>, + <&camcc CAM_CC_IFE_0_AXI_CLK>, + <&camcc CAM_CC_IFE_0_CPHY_RX_CLK>, + <&camcc CAM_CC_IFE_0_CSID_CLK>, + <&camcc CAM_CC_IFE_LITE_0_CLK>, + <&camcc CAM_CC_IFE_LITE_0_CPHY_RX_CLK>, + <&camcc CAM_CC_IFE_LITE_0_CSID_CLK>, + <&camcc CAM_CC_IFE_1_CLK>, + <&camcc CAM_CC_IFE_1_AXI_CLK>, + <&camcc CAM_CC_IFE_1_CPHY_RX_CLK>, + <&camcc CAM_CC_IFE_1_CSID_CLK>, + <&camcc CAM_CC_IFE_LITE_1_CLK>, + <&camcc CAM_CC_IFE_LITE_1_CPHY_RX_CLK>, + <&camcc CAM_CC_IFE_LITE_1_CSID_CLK>, + <&camcc CAM_CC_IFE_2_CLK>, + <&camcc CAM_CC_IFE_2_AXI_CLK>, + <&camcc CAM_CC_IFE_2_CPHY_RX_CLK>, + <&camcc CAM_CC_IFE_2_CSID_CLK>; + clock-names = "camnoc_axi", + "csiphy0", + "csiphy0_timer", + "csiphy1", + "csiphy1_timer", + "csiphy2", + "csiphy2_timer", + "csiphy3", + "csiphy3_timer", + "csiphy4", + "csiphy4_timer", + "gcc_camera_ahb", + "gcc_cam_hf_axi", + "soc_ahb", + "vfe0", + "vfe0_axi", + "vfe0_cphy_rx", + "vfe0_csid", + "vfe0_lite", + "vfe0_lite_cphy_rx", + "vfe0_lite_csid", + "vfe1", + "vfe1_axi", + "vfe1_cphy_rx", + "vfe1_csid", + "vfe1_lite", + "vfe1_lite_cphy_rx", + "vfe1_lite_csid", + "vfe2", + "vfe2_axi", + "vfe2_cphy_rx", + "vfe2_csid"; + + interrupts = <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 640 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 122 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 641 IRQ_TYPE_EDGE_RISING>; + interrupt-names = "csid0", + "csid0_lite", + "csid1", + "csid1_lite", + "csid2", + "csiphy0", + "csiphy1", + "csiphy2", + "csiphy3", + "csiphy4", + "vfe0", + "vfe0_lite", + "vfe1", + "vfe1_lite", + "vfe2"; + + interconnects = <&gem_noc MASTER_APPSS_PROC 0 &cnoc2 SLAVE_CAMERA_CFG 0>, + <&mmss_noc MASTER_CAMNOC_HF 0 &mc_virt SLAVE_EBI1 0>; + interconnect-names = "ahb", "hf_0"; + + iommus = <&apps_smmu 0x800 0x4e0>; + + power-domains = <&camcc CAM_CC_IFE_0_GDSC>, + <&camcc CAM_CC_IFE_1_GDSC>, + <&camcc CAM_CC_IFE_2_GDSC>, + <&camcc CAM_CC_TITAN_TOP_GDSC>; + power-domain-names = "ife0", "ife1", "ife2", "top"; + + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + }; + + port@1 { + reg = <1>; + }; + + port@2 { + reg = <2>; + }; + + port@3 { + reg = <3>; + }; + + port@4 { + reg = <4>; + }; + }; + }; + camcc: clock-controller@ad00000 { compatible = "qcom,sc7280-camcc"; reg = <0 0x0ad00000 0 0x10000>;