Thread (5 messages) flat view 5 messages, 2 authors, 9d ago

Re: [PATCH v8 1/2] dt-bindings: clock: add TI CDCE6214 binding

From: sashiko-bot@kernel.org
Date: 2026-09-09 09:00:11
Also in: linux-devicetree

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Conditional constraints in the 'allOf' block target the non-existent property 'io-standard' instead of 'ti,io-standard', bypassing validation.
- [Low] The description for 'ti,cmosp-mode' incorrectly refers to 'CMOSN output' and contains typos.
- [Low] The 'ti,cmosp-mode' and 'ti,cmosn-mode' properties lack a minimum constraint, incorrectly allowing the value 0.
- [Low] The 'clock-names' property incorrectly permits an array with duplicate clock names (e.g., ['secref', 'secref']).
--

commit 3a564f8b01873582c4ee765638671ebe3d3d19b4
Author: Sascha Hauer [off-list ref]

dt-bindings: clock: add TI CDCE6214 binding
    
Add device tree binding for the CDCE6214, an Ultra-Low Power Clock
Generator With One PLL, Four Differential Outputs, Two Inputs, and
Internal EEPROM.
quoted hunk ↗ jump to hunk
diff --git a/Documentation/devicetree/bindings/clock/ti,cdce6214.yaml b/Documentation/devicetree/bindings/clock/ti,cdce6214.yaml
new file mode 100644
index 0000000000000..4a9c9155894af
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ti,cdce6214.yaml
[ ... ]
+  clock-names:
+    minItems: 1
+    items:
+      - enum: [ priref, secref ]
+      - const: secref
[Severity: Low]
Could this allow the clock-names array to have duplicate entries like
"secref", "secref"? Should the first item be restricted to just priref
to prevent defining the same physical clock reference twice?

[ ... ]
+          ti,cmosp-mode:
+            description: |
+              Driving mode for CMOSN output:
+              1: Low Polarity
+              2: High Polrity
+              3: Disable
[Severity: Low]
There appear to be a couple of typos in this description. It refers to
CMOSN output instead of CMOSP, and Polrity instead of Polarity.
+            $ref: /schemas/types.yaml#/definitions/uint32
+            maximum: 3
[Severity: Low]
Since this references uint32 with a maximum of 3, does it inadvertently
allow 0 as a valid state?

According to the description and the driver header, 0 is not a valid
mode. Would it be better to use an enum of [1, 2, 3] or define a
minimum of 1?
+
+          ti,cmosn-mode:
+            description: |
+              Driving mode for CMOSN output:
+              1: Low Polarity
+              2: High Polrity
+              3: Disable
+            $ref: /schemas/types.yaml#/definitions/uint32
+            maximum: 3
[Severity: Low]
Similarly, does this property need a minimum constraint to prevent 0
from passing schema validation?

[ ... ]
+        allOf:
+          - if:
+              properties:
+                pins:
+                  contains:
+                    const: priref
+            then:
+              properties:
+                io-standard:
+                  enum: [ 1, 5 ]
[Severity: Medium]
Do these constraints target the non-existent property io-standard instead
of ti,io-standard?

If so, an invalid ti,io-standard value in a device tree might erroneously
pass schema validation because the constraints aren't applied to it.
This pattern appears in the subsequent conditional blocks as well.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-clk-cdce6214-v8-0-4dfa355c1f65@pengutronix.de?part=1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help