Re: [PATCH 1/3] media: dt-bindings: qcom: add JPEG encoder binding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] `clock-names` is omitted from the `required` list, which can lead to runtime driver probe failures for otherwise valid DT nodes.
- [Low] The `cell-index` property is used in the example but is not defined in the `properties` block, causing a schema validation error.
- [Low] The example uses C preprocessor macros without including the necessary headers, causing `dt_binding_check` compilation to fail.
- [Low] The example uses a non-generic node name (`qcom,jpegenc@ac4e000`).
- [Low] The `reg` property in the example uses 64-bit addresses and sizes without a parent node specifying the cell context, causing a schema validation failure.
--

commit 8198eafe99ce53eb80a55ddd2b50d88c47749ec9
Author: Atanas Filipov <atanas.filipov@oss.qualcomm.com>

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

Add YAML device tree binding for the Qualcomm JPEG encoder hardware. This
binding is used by the newly added driver and describes all required
resources, including clocks, power domains, IOMMU mappings and interconnect
paths.

> 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..fac3e654458d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,jpeg-encoder.yaml
[ ... ]
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +  - power-domains
> +  - iommus
> +  - interconnects
> +  - interconnect-names

Should clock-names be included in this required list? 

The binding mandates exactly 6 clocks and defines clock-names, but omits
clock-names from the required list. If a device tree node conforms to the
schema by omitting clock-names, the driver might fail to probe when fetching
clocks by string name.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |

Does this example need dt-bindings header includes? 

It uses macros like GIC_SPI, IRQ_TYPE_EDGE_RISING, CAM_CC_TITAN_TOP_GDSC, and
GCC_CAMERA_HF_AXI_CLK, but the headers defining them are missing. The device
tree compiler will fail with syntax errors because it expects integer values
rather than string tokens.

> +    qcom_jpeg_enc: qcom,jpegenc@ac4e000 {

This isn't a bug, but the node name qcom,jpegenc@ac4e000 does not use a
generic term. Would it be better to use a generic name like
jpeg-encoder@ac4e000 or video-codec@ac4e000 according to standard Device Tree
naming conventions?

> +        cell-index = <0>;

Should cell-index be defined in the properties block above? 

Because additionalProperties is false at the top level, having cell-index in
the example without defining it in the schema will cause a schema validation
error.

> +        compatible = "qcom,qcm6490-jenc";
> +        reg =
> +            <0 0xac4e000 0 0x4000>,
> +            <0 0xac40000 0 0x1000>;

Will this reg property pass validation without a parent node specifying
#address-cells and #size-cells? 

Since it uses four cells per entry, dtc will default to 1 for both address
and size, interpreting these 8 cells as 4 separate regions. This would
violate the maxItems: 2 constraint on the reg property.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-qcom-jpeg-v4l2-v1-0-f38c2e1b3555@oss.qualcomm.com?part=1