Re: [PATCH v14 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The binding claims register regions already managed by the existing `qcom,x1e80100-camss` binding, creating mutually exclusive overlapping device nodes.
- [Medium] The schema explicitly allows CPHY operation, contradicting the commit message which states CPHY is omitted.
- [Low] The `#phy-cells` property is omitted for a PHY binding.
--

--- Patch [1]: [PATCH v14 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema ---
commit cbf6f1512f36b1e20121afe66ff586d3cb88cde0
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema

This commit adds a base schema for the MIPI CSI2 PHYs on Qualcomm SoCs.
The PHY supports both DPHY and CPHY operation, including a split-mode
allowing two sensors to connect to one PHY. The submitted binding is
intended to describe DPHY modes only.

> diff --git a/Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml
> new file mode 100644
> index 0000000000000..7d0363812aa89
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,x1e80100-csi2-phy.yaml
> @@ -0,0 +1,198 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,x1e80100-csi2-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm X1E80100 SoC CSI2 PHY
> +
> +maintainers:
> +  - Bryan O'Donoghue <bod@kernel.org>
> +
> +description:
> +  Qualcomm MIPI CSI2 C-PHY/D-PHY combination PHY. Connects MIPI CSI2 sensors
> +  to Qualcomm's Camera CSI Decoder. The PHY supports both C-PHY and D-PHY
> +  modes.
> +
> +properties:
> +  compatible:
> +    const: qcom,x1e80100-csi2-phy
> +

[Severity: Low]
Does this binding need a #phy-cells property?  Since it is located in the
phy/ subsystem, a PHY provider binding typically requires #phy-cells to be
defined and added to the required list for standard usage via the phys
property.

> +  reg:
> +    maxItems: 1

[ ... ]

> +        patternProperties:
> +          "^endpoint(@[0-9a-f]+)?$":
> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +            properties:
> +              bus-type:
> +                enum:
> +                  - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
> +                  - 4 # MEDIA_BUS_TYPE_CSI2_DPHY

[Severity: Medium]
The commit message mentions that CPHY is left to future work, but does this
enum explicitly permit CPHY (bus-type 1) in the schema?  This might allow
devicetrees to pass validation when using CPHY even though the necessary
constraints are not defined yet.

> +
> +              data-lanes:
> +                minItems: 1

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,x1e80100-camcc.h>
> +    #include <dt-bindings/clock/qcom,x1e80100-gcc.h>
> +    #include <dt-bindings/media/video-interfaces.h>
> +    #include <dt-bindings/power/qcom,rpmhpd.h>
> +
> +    phy@ace4000 {
> +        compatible = "qcom,x1e80100-csi2-phy";
> +        reg = <0x0ace4000 0x2000>;

[Severity: High]
Does this new standalone binding claim register regions that are already
managed by the existing qcom,x1e80100-camss binding?

If a device tree instantiates both the CAMSS node (which requires csiphy0)
and this new CSI2 PHY node, they might both attempt to map the overlapping
0x0ace4000 region.  This could lead to an -EBUSY resource request failure
and prevent the devices from probing.

> +
> +        clocks = <&camcc CAM_CC_CSIPHY0_CLK>,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-x1e-csi2-phy-v14-0-27d1bf477593@linaro.org?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy