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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <01f6c25a-5803-4986-b754-0a3661d435b0@linaro.org>
Date: Wed, 10 Jul 2024 10:37:07 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Depeng Shao <quic_depengs@...cinc.com>, 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 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.


> +  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/

> +
> +            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

---
bod