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

[Depeng Shao <quic_depengs@quicinc.com>]

Hi Vladimir,

On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote:

>> +            compatible = "qcom,sm8550-camss";
>> +
>> +            reg = <0 0x0acb7000 0 0xd00>,
>> +                  <0 0x0acb9000 0 0xd00>,
>> +                  <0 0x0acbb000 0 0xd00>,
>> +                  <0 0x0acca000 0 0xa00>,
>> +                  <0 0x0acce000 0 0xa00>,
>> +                  <0 0x0acb6000 0 0x1000>,
>> +                  <0 0x0ace4000 0 0x2000>,
>> +                  <0 0x0ace6000 0 0x2000>,
>> +                  <0 0x0ace8000 0 0x2000>,
>> +                  <0 0x0acea000 0 0x2000>,
>> +                  <0 0x0acec000 0 0x2000>,
>> +                  <0 0x0acee000 0 0x2000>,
>> +                  <0 0x0acf0000 0 0x2000>,
>> +                  <0 0x0acf2000 0 0x2000>,
>> +                  <0 0x0ac62000 0 0xf000>,
>> +                  <0 0x0ac71000 0 0xf000>,
>> +                  <0 0x0ac80000 0 0xf000>,
>> +                  <0 0x0accb000 0 0x2800>,
>> +                  <0 0x0accf000 0 0x2800>;
> 
> Please sort the list above in numerical order, this will change positions
> of "vfe_lite0", "vfe_lite1" etc.
> 
> Another note, since it's not possible to map less than a page, so I believe
> it might make sense to align all sizes to 0x1000.
> 

Sure, I previously sorted by the alphabetical order of reg_name.
I will update it based on your suggestion. And will also make sure the 
align all sizes to 0x1000.


>> +                     <&camcc CAM_CC_IFE_LITE_CLK>,
>> +                     <&camcc CAM_CC_IFE_LITE_AHB_CLK>,
>> +                     <&camcc CAM_CC_IFE_LITE_CPHY_RX_CLK>,
>> +                     <&camcc CAM_CC_IFE_LITE_CSID_CLK>,
>> +                     <&gcc GCC_CAMERA_HF_AXI_CLK>;
> 
> Could you please put the &gcc provided clock as the first one in the list?
> 

Sure, will do.

>> +
>> +            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_ICP 0 &mc_virt 
>> SLAVE_EBI1 0>,
>> +                            <&mmss_noc MASTER_CAMNOC_SF 0 &mc_virt 
>> SLAVE_EBI1 0>;
>> +            interconnect-names = "ahb",
>> +                                 "hf_0_mnoc",
>> +                                 "icp_mnoc",
>> +                                 "sf_0_mnoc";
> 
> Just a note for myself, interconnect names lost "cam_" prefix, and it 
> should
> be fine.

Krzysztof posted a comment in a SC7280 camss change and asked to remove 
the "cam_" prefix.


https://lore.kernel.org/all/087e7f29-1fa8-4bc2-bb3d-acb941432381@kernel.org/


>> +            ports {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
> 
> In case of a single child node #address-cells/#size-cells could be omitted,
> if I'm not mistaken about it...
> 

I tried it, but dt_binding_check report below warning.

Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:221.29-39: 
Warning (reg_format): 
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0:reg: property has 
invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: 
Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: 
Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: 
Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: 
Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: 
Warning (spi_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27: 
Warning (avoid_default_addr_size): 
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0: Relying on default 
#address-cells value
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27: 
Warning (avoid_default_addr_size): 
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0: Relying on default 
#size-cells value
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb: 
Warning (unique_unit_address_if_enabled): Failed prerequisite 
'avoid_default_addr_size'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27: 
Warning (graph_endpoint): 
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0: graph node 
'#address-cells' is -1, must be 1
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27: 
Warning (graph_endpoint): 
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0: graph node 
'#size-cells' is -1, must be 0

>> +                port@0 {
>> +                    reg = <0>;
>> +                    #address-cells = <1>;
>> +                    #size-cells = <0>;
> 
> Same as above.
> 

Same warning..

>> +
>> +                    csiphy_ep0: endpoint@0 {
>> +                        reg = <0>;
>> +                        clock-lanes = <7>;
>> +                        data-lanes = <0 1>;
>> +                        remote-endpoint = <&sensor_ep>;
>> +                    };
>> +                };
>> +            };
>> +        };
>> +    };
> 

---
Thanks,
Depeng