Re: [PATCH v1 1/4] dt-bindings: media: qcom: Add JPEG encoder binding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Schema validation weakness and missing required properties cause driver probe failure.
- [Medium] Chronologically backward compatible fallback with mismatched hardware constraints.
- [Low] The binding introduces the deprecated `cell-index` property.
--

commit 8d581d21294da3512a77773ccfed01692993fa7b
Author: Atanas Filipov <atanas.filipov@oss.qualcomm.com>

dt-bindings: media: qcom: Add JPEG encoder binding

Add device-tree binding for the standalone Qualcomm JPEG encoder hardware
block (separate from CAMSS media pipelines). Document required resources
briefly and scope initial support to currently used compatibles in this
series, including SM8250, QCM6490, and SM8550 class platforms.

> diff --git a/Documentation/devicetree/bindings/media/qcom,jpeg-encoder.yaml b/Documentation/devicetree/bindings/media/qcom,jpeg-encoder.yaml
> new file mode 100644
> index 0000000000000..83f19719a8693
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,jpeg-encoder.yaml
> @@ -0,0 +1,142 @@

[ ... ]

> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: qcom,sm8250-jenc
> +      - items:
> +          - enum:
> +              - qcom,qcm6490-jenc
> +          - const: qcom,sm8550-jenc

[Severity: Medium]
Is it safe to use qcom,sm8550-jenc as a fallback for qcom,qcm6490-jenc?

It looks like the driver uses different performance and clock configuration
data for these two SoCs (qcom_t680_jpeg_drvdata vs qcom_t780_jpeg_drvdata
in drivers/media/platform/qcom/jpeg/qcom_jenc_res.c). 

If a generic driver relies on this fallback, it would apply SM8550 clock
frequencies to QCM6490 hardware, which might cause instability. Also, newer
hardware falling back to older hardware is usually the standard devicetree
convention.

> +      - items:
> +          - const: qcom,sm8550-jenc
> +
> +  cell-index:
> +    description: Legacy JPEG HW instance index.
> +    $ref: /schemas/types.yaml#/definitions/uint32

[Severity: Low]
Is this deprecated cell-index property still required?

The introduced Linux driver does not appear to read or use this property.
Could instance identification be handled dynamically or via standardized
devicetree aliases nodes instead?

> +  reg:
> +    maxItems: 1
> +
> +  reg-names:
> +    oneOf:
> +      - items:
> +          - const: jpeg
> +      - items:
> +          - const: jpeg-regs
> +      - items:
> +          - const: jpeg_regs
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 6
> +    maxItems: 6
> +
> +  clock-names:
> +    oneOf:
> +      - items:
> +          - const: gcc_hf_axi
> +          - const: gcc_sf_axi
> +          - const: core_ahb
> +          - const: cpas_ahb
> +          - const: camnoc_axi
> +          - const: jpeg
> +      - items:
> +          - const: gcc_hf_axi_clk
> +          - const: gcc_sf_axi_clk
> +          - const: core_ahb_clk
> +          - const: cpas_ahb_clk
> +          - const: camnoc_axi_clk
> +          - const: jpeg_clk

[Severity: High]
Should the binding strictly enforce the exact resource names the driver expects?

The schema allows variations like "jpeg-regs" and "_clk" suffixes, but the
driver probe in qcom_jpeg_probe() strictly calls:

devm_platform_ioremap_resource_byname(pdev, "jpeg")

and expects non-suffix clock names (e.g., "core_ahb"). If a devicetree uses
the alternative schema-approved names, won't the driver probe fail?

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +  - power-domains
> +  - iommus
> +  - interconnects
> +  - interconnect-names

[Severity: High]
Should reg-names and clock-names be added to this required list?

Since the driver strictly relies on these names to find its resources during
probe, omitting them from the devicetree will cause probe failures.

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