Re: [PATCH 2/7] media: dt-bindings: Add CAMSS device for SM8750

Krzysztof Kozlowski
2025-11-27T08:10:10Z

On Wed, Nov 26, 2025 at 01:38:35AM -0800, Hangxiang Ma wrote:
> Add the compatible string "qcom,sm8750-camss" to support the Camera

s/to support the/for the/

Bindings do not support hardware.

> Subsystem (CAMSS) on the Qualcomm SM8750 platform.
>
> The SM8750 platform provides:
> - 3 x VFE, 5 RDI per VFE
> - 2 x VFE Lite, 4 RDI per VFE Lite
> - 3 x CSID
> - 2 x CSID Lite
> - 6 x CSIPHY
> - 2 x ICP
> - 1 x IPE
> - 2 x JPEG DMA & Downscaler
> - 2 x JPEG Encoder
> - 1 x OFE
> - 5 x RT CDM
> - 3 x TPG
>
> Signed-off-by: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
> ---
> .../bindings/media/qcom,sm8750-camss.yaml | 664 +++++++++++++++++++++
> 1 file changed, 664 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8750-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm8750-camss.yaml
> new file mode 100644
> index 000000000000..6b2b0b5a7e19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,sm8750-camss.yaml
> @@ -0,0 +1,664 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/qcom,sm8750-camss.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SM8750 Camera Subsystem (CAMSS)
> +
> +maintainers:
> + - Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
> +
> +description:
> + This binding describes the camera subsystem hardware found on SM8750 Qualcomm

s/This binding ..../SM8750 CAMSS (Camera Subsystem) is foo bar..../

or any other form which will describe the hardware. There is no point to
say that binding describes hardware. It cannot describe anything else.

> + SoCs. It includes submodules such as CSIPHY (CSI Physical layer) and CSID
> + (CSI Decoder), which comply with the MIPI CSI2 protocol.
> +
> + The subsystem also integrates a set of real-time image processing engines and
> + their associated configuration modules, as well as non-real-time engines.
> +
> + Additionally, it encompasses a test pattern generator (TPG) submodule.
> +
> +properties:
> + compatible:
> + const: qcom,sm8750-camss
> +
> + reg:
> + items:
> + - description: Registers for CSID 0
> + - description: Registers for CSID 1
> + - description: Registers for CSID 2
> + - description: Registers for CSID Lite 0
> + - description: Registers for CSID Lite 1
> + - description: Registers for CSIPHY 0
> + - description: Registers for CSIPHY 1
> + - description: Registers for CSIPHY 2
> + - description: Registers for CSIPHY 3
> + - description: Registers for CSIPHY 4
> + - description: Registers for CSIPHY 5
> + - description: Registers for VFE (Video Front End) 0
> + - description: Registers for VFE 1
> + - description: Registers for VFE 2
> + - description: Registers for VFE Lite 0
> + - description: Registers for VFE Lite 1
> + - description: Registers for ICP (Imaging Control Processor) 0
> + - description: Registers for ICP SYS 0
> + - description: Registers for ICP 1
> + - description: Registers for ICP SYS 1
> + - description: Registers for IPE (Image Processing Engine)
> + - description: Registers for JPEG DMA & Downscaler 0
> + - description: Registers for JPEG Encoder 0
> + - description: Registers for JPEG DMA & Downscaler 1
> + - description: Registers for JPEG Encoder 1
> + - description: Registers for OFE (Offline Front End)
> + - description: Registers for RT CDM (Camera Data Mover) 0
> + - description: Registers for RT CDM 1
> + - description: Registers for RT CDM 2
> + - description: Registers for RT CDM 3
> + - description: Registers for RT CDM 4
> + - description: Registers for TPG 0
> + - description: Registers for TPG 1
> + - description: Registers for TPG 2
> +
> + reg-names:
> + items:
> + - const: csid0
> + - const: csid1
> + - const: csid2
> + - const: csid_lite0
> + - const: csid_lite1
> + - const: csiphy0
> + - const: csiphy1
> + - const: csiphy2
> + - const: csiphy3
> + - const: csiphy4
> + - const: csiphy5

I had impression there were talks and plans to split CSI PHY out of
camss. Some other patches got blocked by this, so unfortunately this as
well. Your cover letter does not answer on this, so unfortuntaly this
concludes the review.

> + - const: vfe0
> + - const: vfe1
> + - const: vfe2
> + - const: vfe_lite0
> + - const: vfe_lite1
> + - const: icp0
> + - const: icp0_sys
> + - const: icp1
> + - const: icp1_sys
> + - const: ipe
> + - const: jpeg_dma0
> + - const: jpeg_enc0
> + - const: jpeg_dma1
> + - const: jpeg_enc1
> + - const: ofe
> + - const: rt_cdm0
> + - const: rt_cdm1
> + - const: rt_cdm2
> + - const: rt_cdm3
> + - const: rt_cdm4
> + - const: tpg0
> + - const: tpg1
> + - const: tpg2
> +
> + clocks:
> + maxItems: 61
> +
> + clock-names:
> + items:
> + - const: camnoc_nrt_axi
> + - const: camnoc_rt_axi
> + - const: camnoc_rt_vfe0
> + - const: camnoc_rt_vfe1
> + - const: camnoc_rt_vfe2
> + - const: camnoc_rt_vfe_lite
> + - const: cam_top_ahb

cpas_ahb?

> + - const: cam_top_fast_ahb

Isn't this cpas_fast_ahb? Why every schema comes with its own naming...

> + - const: csid
> + - const: csid_csiphy_rx
> + - const: csiphy0
> + - const: csiphy0_timer
> + - const: csiphy1
> + - const: csiphy1_timer
> + - const: csiphy2
> + - const: csiphy2_timer
> + - const: csiphy3
> + - const: csiphy3_timer
> + - const: csiphy4
> + - const: csiphy4_timer
> + - const: csiphy5
> + - const: csiphy5_timer
> + - const: gcc_hf_axi

Look at previous generation how this is called: gcc_axi_hf. Use that
name.

> + - const: vfe0
> + - const: vfe0_fast_ahb
> + - const: vfe1
> + - const: vfe1_fast_ahb
> + - const: vfe2
> + - const: vfe2_fast_ahb
> + - const: vfe_lite
> + - const: vfe_lite_ahb
> + - const: vfe_lite_cphy_rx
> + - const: vfe_lite_csid
> + - const: qdss_debug_xo
> + - const: camnoc_ipe_nps
> + - const: camnoc_ofe
> + - const: gcc_sf_axi
> + - const: icp0
> + - const: icp0_ahb
> + - const: icp1
> + - const: icp1_ahb
> + - const: ipe_nps
> + - const: ipe_nps_ahb
> + - const: ipe_nps_fast_ahb
> + - const: ipe_pps
> + - const: ipe_pps_fast_ahb
> + - const: jpeg0
> + - const: jpeg1
> + - const: ofe_ahb
> + - const: ofe_anchor
> + - const: ofe_anchor_fast_ahb
> + - const: ofe_hdr
> + - const: ofe_hdr_fast_ahb
> + - const: ofe_main
> + - const: ofe_main_fast_ahb
> + - const: vfe0_bayer
> + - const: vfe0_bayer_fast_ahb
> + - const: vfe1_bayer
> + - const: vfe1_bayer_fast_ahb
> + - const: vfe2_bayer
> + - const: vfe2_bayer_fast_ahb
> +
> + interrupts:
> + maxItems: 32
> +
> + interrupt-names:
> + items:
> + - const: csid0
> + - const: csid1
> + - const: csid2
> + - const: csid_lite0
> + - const: csid_lite1
> + - const: csiphy0
> + - const: csiphy1
> + - const: csiphy2
> + - const: csiphy3
> + - const: csiphy4
> + - const: csiphy5
> + - const: vfe0
> + - const: vfe1
> + - const: vfe2
> + - const: vfe_lite0
> + - const: vfe_lite1
> + - const: camnoc_nrt
> + - const: camnoc_rt
> + - const: icp0
> + - const: icp1
> + - const: jpeg_dma0
> + - const: jpeg_enc0
> + - const: jpeg_dma1
> + - const: jpeg_enc1
> + - const: rt_cdm0
> + - const: rt_cdm1
> + - const: rt_cdm2
> + - const: rt_cdm3
> + - const: rt_cdm4
> + - const: tpg0
> + - const: tpg1
> + - const: tpg2
> +
> + interconnects:
> + maxItems: 4
> +
> + interconnect-names:
> + items:
> + - const: ahb
> + - const: hf_mnoc
> + - const: sf_icp_mnoc
> + - const: sf_mnoc

Which previous generation you used as ordering style? X1E has it
different.

> +
> + iommus:
> + items:
> + - description: VFE non-protected stream
> + - description: ICP0 shared stream
> + - description: ICP1 shared stream
> + - description: IPE CDM non-protected stream
> + - description: IPE non-protected stream
> + - description: JPEG non-protected stream
> + - description: OFE CDM non-protected stream
> + - description: OFE non-protected stream
> + - description: VFE / VFE Lite CDM non-protected stream
> +
> + power-domains:
> + items:
> + - description:
> + VFE0 GDSC - Global Distributed Switch Controller for VFE0.
> + - description:
> + VFE1 GDSC - Global Distributed Switch Controller for VFE1.
> + - description:
> + VFE2 GDSC - Global Distributed Switch Controller for VFE2.
> + - description:
> + Titan GDSC - Global Distributed Switch Controller for the entire camss.
> + - description:
> + IPE GDSC - Global Distributed Switch Controller for IPE.
> + - description:
> + OFE GDSC - Block Global Distributed Switch Controller for OFE.
> +
> + power-domain-names:
> + items:
> + - const: vfe0
> + - const: vfe1
> + - const: vfe2

Previous generations call these IFE, I already raised this and you
changed to ife in Kaanapali. So are all future devices going to use
rather VFE name?

> + - const: top
> + - const: ipe
> + - const: ofe
> +
> + vdd-csiphy0-0p88-supply:

88->8, so: vdd-csiphy0-0p8-supply:

Same in other places. This is how it is called for every binding.

> + description:
> + Phandle to a 0.88V regulator supply to CSIPHY0 core block.
> +
> + vdd-csiphy0-1p2-supply:
> + description:
> + Phandle to a 1.2V regulator supply to CSIPHY0 pll block.
> +
> + vdd-csiphy1-0p88-supply:
> + description:
> + Phandle to a 0.88V regulator supply to CSIPHY1 core block.

Best regards,
Krzysztof