Re: [PATCH v1 2/4] arm64: dts: qcom: sm8550: Add JPEG encoder node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Unnecessary presence of the deprecated `cell-index` property in the newly added JPEG encoder node.
- [Low] The `interconnects` property uses the hardcoded magic number `0` for path tags instead of semantic macros.
- [Low] Device tree node `jpeg-encoder@ac4e000` is placed out of order based on its unit-address.
--

commit 7c79d5580891bdc3ade6113afec02f777f964fdb
Author: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
    
arm64: dts: qcom: sm8550: Add JPEG encoder node
    
This patch adds the JPEG encoder hardware node to the SM8550 device tree.
It configures resources like clocks, power domains, IOMMUs, and interconnects
to allow the qcom-jpeg V4L2 encoder driver to bind and operate.

> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> index 912525e9bca6f..8090b8b1d7bda 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
[ ... ]
> @@ -3677,6 +3677,48 @@ port@7 {
>  			};
>  		};
>  
> +		qcom_jpeg_enc: jpeg-encoder@ac4e000 {

[Severity: Low]
Is this node placed out of order according to its unit-address?

Usually, nodes within the soc container are strictly sorted by their unit
address. Since this is at 0xac4e000, should it be placed before
isp@acb7000 rather than appended after it?

> +			cell-index = <0>;

[Severity: Low]
Is the cell-index property still needed here?

The device tree binding typically describes this as a legacy hardware instance
index, and modern upstream drivers usually do not rely on it. Could we remove
it to align with modern device tree conventions?

> +			compatible = "qcom,sm8550-jenc";
> +
> +			reg = <0 0xac4e000 0 0x4000>;
[ ... ]
> +			iommus = <&apps_smmu 0x20C0 0x20>,
> +				 <&apps_smmu 0x20E0 0x20>;
> +
> +			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>;

[Severity: Low]
Would it be better to use the standard semantic macros for the interconnect
path tags instead of hardcoding 0?

While the Qualcomm RPMh driver might map 0 to QCOM_ICC_TAG_ALWAYS internally,
using the macro explicitly would maintain consistency with the other
interconnect nodes in the tree and clarify the intended power and bandwidth
configuration.

> +
> +			interconnect-names = "cam_ahb",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612194417.1737009-1-atanas.filipov@oss.qualcomm.com?part=2