Re: [PATCH v6 1/6] dt-bindings: media: qcom: Add Shikra CAMSS compatible

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The YAML binding uses a global `oneOf` block for `iommus` instead of using `if-then` conditionals, failing to conditionally enforce the hardware-specific constraints for each compatible string.
- [Medium] The `qcom,shikra-camss` compatible string incorrectly uses `qcom,qcm2290-camss` as a fallback despite being a hardware subset, violating Device Tree compatibility principles.
--

--- Patch [1]: [PATCH v6 1/6] dt-bindings: media: qcom: Add Shikra CAMSS compatible ---
commit 945dc84434e7636d9e3d6c379f213e274cca7db3
Author: Nihal Kumar Gupta <[email protected]>

dt-bindings: media: qcom: Add Shikra CAMSS compatible

This commit adds a new compatible string for the Shikra CAMSS IP, using
qcom,qcm2290-camss as a fallback. It modifies the iommus property to allow
either one or four items, as Shikra omits the CDM and OPE blocks present in
QCM2290.

> diff --git a/Documentation/devicetree/bindings/media/qcom,qcm2290-camss.yaml b/Documentation/devicetree/bindings/media/qcom,qcm2290-camss.yaml
> index 391d0f6f67ef5..490a7f3a8c5ff 100644
> --- a/Documentation/devicetree/bindings/media/qcom,qcm2290-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,qcm2290-camss.yaml
> @@ -14,7 +14,11 @@ description:
>  
>  properties:
>    compatible:
> -    const: qcom,qcm2290-camss
> +    oneOf:
> +      - items:
> +          - const: qcom,shikra-camss
> +          - const: qcom,qcm2290-camss

[Severity: Medium]
Since the commit message states Shikra omits the CDM and OPE blocks, is it
accurate to use qcom,qcm2290-camss as a fallback?

If an operating system driver binds to the qcom,qcm2290-camss fallback and
assumes the presence of the CDM and OPE blocks, couldn't this lead to bus
faults or initialization failures since those blocks are absent on Shikra
hardware?

> +      - const: qcom,qcm2290-camss
>  
>    reg:
>      maxItems: 9

[ ... ]

> @@ -76,7 +80,14 @@ properties:
>        - const: sf_mnoc
>  
>    iommus:
> -    maxItems: 4
> +    oneOf:
> +      - items:
> +          - description: S1 HLOS VFE non-protected (VFE only)
> +      - items:
> +          - description: S1 HLOS VFE non-protected
> +          - description: S1 HLOS CDM non-protected
> +          - description: S1 HLOS OPE read non-protected
> +          - description: S1 HLOS OPE write non-protected

[Severity: Medium]
Does replacing maxItems with a global oneOf block weaken schema validation
for the existing qcom,qcm2290-camss hardware?

By using a globally permissive oneOf instead of using if-then conditionals
in an allOf block, a device tree specifying qcom,qcm2290-camss with only 1
IOMMU will now silently pass validation, even though it requires 4 IOMMUs.
Similarly, Shikra could incorrectly pass with 4 IOMMUs. Should these
hardware-specific constraints be enforced conditionally based on the
compatible string?

>  
>    power-domains:
>      items:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1