[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9acab683-dd6e-4257-8a52-fe2be8945290@quicinc.com>
Date: Wed, 10 Jul 2024 18:59:21 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <rfoss@...nel.org>,
<todor.too@...il.com>, <mchehab@...nel.org>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <conor+dt@...nel.org>
CC: <quic_eberman@...cinc.com>, <linux-media@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <kernel@...cinc.com>,
Yongsheng Li
<quic_yon@...cinc.com>
Subject: Re: [PATCH 07/13] dt-bindings: media: camss: Add qcom,sm8550-camss
binding
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@...cinc.com>
>> Signed-off-by: Yongsheng Li <quic_yon@...cinc.com>
>> Signed-off-by: Depeng Shao <quic_depengs@...cinc.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
Powered by blists - more mailing lists