| Message ID | 20240813230037.84004-8-mailingradian@gmail.com |
|---|---|
| Headers | show |
| Series | Add SDM670 camera subsystem | expand |
On 14/08/2024 00:00, Richard Acayan wrote: > Add the camera subsystem and CCI used to interface with cameras on the > Snapdragon 670. > > Signed-off-by: Richard Acayan <mailingradian@gmail.com> > --- > + clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>, > + <&camcc CAM_CC_SOC_AHB_CLK>, > + <&camcc CAM_CC_SLOW_AHB_CLK_SRC>, > + <&camcc CAM_CC_CPAS_AHB_CLK>, > + <&camcc CAM_CC_CCI_CLK>, > + <&camcc CAM_CC_CCI_CLK_SRC>; > + clock-names = "camnoc_axi", > + "soc_ahb", > + "slow_ahb_src", > + "cpas_ahb", > + "cci", > + "cci_src"; These "_src" clocks should be dropped, per similar comment on &camss{}; in the previous. > + > + assigned-clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>, > + <&camcc CAM_CC_CCI_CLK>; > + assigned-clock-rates = <80000000>, <37500000>; Do you need to assign the CAMNOC_AXI_CLK here ? More usual to specify it in drivers/media/platform/qcom/camss/camss.c for your SoC grep camnoc_axi drivers/media/platform/qcom/camss/* | wc -l 26 Other than that looks fine. Will wait your v3 before R/B. --- bod
On 14/08/2024 00:00, Richard Acayan wrote: > The camera subsystem for the SDM670 the same as on SDM845 except with > 3 CSIPHY ports instead of 4. Add support for the SDM670 camera > subsystem. > > Signed-off-by: Richard Acayan <mailingradian@gmail.com> > --- > drivers/media/platform/qcom/camss/camss.c | 194 ++++++++++++++++++++++ > 1 file changed, 194 insertions(+) > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index 51b1d3550421..f5d8443d4157 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -584,6 +584,188 @@ static const struct camss_subdev_resources vfe_res_660[] = { > } > }; > > +static const struct camss_subdev_resources csiphy_res_670[] = { > + /* CSIPHY0 */ > + { > + .regulators = {}, > + .clock = { "camnoc_axi", "soc_ahb", "cpas_ahb", > + "csiphy0", "csiphy0_timer" }, Per previous comment, you're specifying camnoc_axi here, so you can just set it to whatever it was 80MHz I think. You shouldn't need the Camera NoC clock to do an I2C/CCI transaction ... nor TBH for the CSIPHY. Should probably live in the CSID alone. > + .clock_rate = { { 0 }, > + { 0 }, > + { 0 }, > + { 0 }, > + { 19200000, 240000000, 269333333 } }, > + .reg = { "csiphy0" }, > + .interrupt = { "csiphy0" }, > + .csiphy = { > + .hw_ops = &csiphy_ops_3ph_1_0, > + .formats = &csiphy_formats_sdm845 > + } > + }, > + > + /* CSIPHY1 */ > + { > + .regulators = {}, > + .clock = { "camnoc_axi", "soc_ahb", "cpas_ahb", > + "csiphy1", "csiphy1_timer" }, > + .clock_rate = { { 0 }, > + { 0 }, > + { 0 }, > + { 0 }, > + { 19200000, 240000000, 269333333 } }, > + .reg = { "csiphy1" }, > + .interrupt = { "csiphy1" }, > + .csiphy = { > + .hw_ops = &csiphy_ops_3ph_1_0, > + .formats = &csiphy_formats_sdm845 > + } > + }, > + > + /* CSIPHY2 */ > + { > + .regulators = {}, > + .clock = { "camnoc_axi", "soc_ahb", "cpas_ahb", > + "csiphy2", "csiphy2_timer" }, > + .clock_rate = { { 0 }, > + { 0 }, > + { 0 }, > + { 0 }, > + { 19200000, 240000000, 269333333 } }, > + .reg = { "csiphy2" }, > + .interrupt = { "csiphy2" }, > + .csiphy = { > + .hw_ops = &csiphy_ops_3ph_1_0, > + .formats = &csiphy_formats_sdm845 > + } > + } > +}; > + > +static const struct camss_subdev_resources csid_res_670[] = { > + /* CSID0 */ > + { > + .regulators = { "vdda-phy", "vdda-pll" }, > + .clock = { "cpas_ahb", "soc_ahb", "vfe0", > + "vfe0_cphy_rx", "csi0" }, You don't need csiX clock in both VFE and CSID. Should almost certainly only be in CSID. > + .clock_rate = { { 0 }, > + { 0 }, > + { 100000000, 320000000, 404000000, 480000000, 600000000 }, > + { 384000000 }, > + { 19200000, 75000000, 384000000, 538666667 } }, > + .reg = { "csid0" }, > + .interrupt = { "csid0" }, > + .csid = { > + .hw_ops = &csid_ops_gen2, > + .formats = &csid_formats_gen2 > + } > + }, > + > + /* CSID1 */ > + { > + .regulators = { "vdda-phy", "vdda-pll" }, > + .clock = { "cpas_ahb", "soc_ahb", "vfe1", > + "vfe1_cphy_rx", "csi1" }, > + .clock_rate = { { 0 }, > + { 0 }, > + { 100000000, 320000000, 404000000, 480000000, 600000000 }, > + { 384000000 }, > + { 19200000, 75000000, 384000000, 538666667 } }, > + .reg = { "csid1" }, > + .interrupt = { "csid1" }, > + .csid = { > + .hw_ops = &csid_ops_gen2, > + .formats = &csid_formats_gen2 > + } > + }, > + > + /* CSID2 */ > + { > + .regulators = { "vdda-phy", "vdda-pll" }, > + .clock = { "cpas_ahb", "soc_ahb", "vfe_lite", > + "vfe_lite_cphy_rx", "csi2" }, > + .clock_rate = { { 0 }, > + { 0 }, > + { 100000000, 320000000, 404000000, 480000000, 600000000 }, > + { 384000000 }, > + { 19200000, 75000000, 384000000, 538666667 } }, > + .reg = { "csid2" }, > + .interrupt = { "csid2" }, > + .csid = { > + .is_lite = true, > + .hw_ops = &csid_ops_gen2, > + .formats = &csid_formats_gen2 > + } > + } > +}; > + > +static const struct camss_subdev_resources vfe_res_670[] = { > + /* VFE0 */ > + { > + .regulators = {}, > + .clock = { "camnoc_axi", "cpas_ahb", "soc_ahb", > + "vfe0", "vfe0_axi", "csi0" }, Please try to zap that csi0 clock for your v3, only specifying it in CSID. --- bod
On 14/08/2024 00:00, Richard Acayan wrote: > The CCI on the Snapdragon 670 is the interface for controlling camera > hardware over I2C. Add the compatible so it can be added to the SDM670 > device tree. > > Signed-off-by: Richard Acayan <mailingradian@gmail.com> > --- > Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml > index c33ae7b63b84..87f5e5bdbbe7 100644 > --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml > +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml > @@ -27,6 +27,7 @@ properties: > - enum: > - qcom,sc7280-cci > - qcom,sc8280xp-cci > + - qcom,sdm670-cci > - qcom,sdm845-cci > - qcom,sm6350-cci > - qcom,sm8250-cci > @@ -143,6 +144,7 @@ allOf: > compatible: > contains: > enum: > + - qcom,sdm670-cci > - qcom,sdm845-cci > - qcom,sm6350-cci > then: Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>