Re: [PATCH 07/13] dt-bindings: media: camss: Add qcom,sm8550-camss binding

[Depeng Shao <quic_depengs@quicinc.com>]

On 7/10/2024 5:37 PM, Bryan O'Donoghue wrote:
> On 09/07/2024 17:06, Depeng Shao wrote:
>> Add bindings for qcom,sm8550-camss in order to support the camera
>> subsystem for sm8550
>>
>> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
>> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
>> ---
> 
> Hey Depeng,
> 
> Thank you for your submission, looks good. I have a few comments below.
> 
>> +  clock-names:
>> +    items:
>> +      - const: cam_ahb_clk
>> +      - const: cam_hf_axi
>> +      - const: cam_sf_axi
>> +      - const: camnoc_axi
>> +      - const: camnoc_axi_src
> 
> These "_src" clocks are almost certainly not necessary. The CAMCC should 
> have camnoc_axi_src as the parent clock of camnoc_axi, so you don't need 
> ot list the "_src" clock.
> 
> Please go through this list in your yaml, dts and .c code and remove.
> 
> There may be an exception where a _src clock is required but my 
> expectation is that all of those _src clocks can be removed.
> 
> 

Hey Bryan,

Thanks for the reviewing. I will try to move all the _src clk and verify 
again.

>> +  power-domains:
>> +    items:
>> +      - description: IFE0 GDSC - Image Front End, Global Distributed 
>> Switch Controller.
>> +      - description: IFE1 GDSC - Image Front End, Global Distributed 
>> Switch Controller.
>> +      - description: IFE2 GDSC - Image Front End, Global Distributed 
>> Switch Controller.
>> +      - description: Titan GDSC - Titan ISP Block, Global Distributed 
>> Switch Controller.
> 
> Please name these power-domains in the next iteration of the series.
> 
> The dependency on the ordering of the power-domains is a bug which new 
> SoC submissions won't be replicating.
> 
> https://lore.kernel.org/linux-arm-msm/fcdb072d-6099-4423-b4b5-21e9052b82cc@linaro.org/
> 

Sure, will add this in new patch set.
power-domain-names = "ife0",
		    "ife1",
                     "ife2",
		    "top";

>> +
>> +            interconnects =
>> +                    <&gem_noc         MASTER_APPSS_PROC          0 
>> &config_noc SLAVE_CAMERA_CFG  0>,
>> +                    <&mmss_noc        MASTER_CAMNOC_HF           0 
>> &mc_virt SLAVE_EBI1           0>,
>> +                    <&mmss_noc        MASTER_CAMNOC_SF           0 
>> &mc_virt SLAVE_EBI1           0>,
>> +                    <&mmss_noc        MASTER_CAMNOC_ICP          0 
>> &mc_virt SLAVE_EBI1           0>;
> 
> This tabbing looks weird, do none of the checking tools complain about it ?
> 
> See:
> Documentation/devicetree/bindings/media/qcom,sc8280xp-camss.yaml
> 

Looks like no complain about this, but will update it.

> ---
> bod

Thanks,
Depeng