Re: [PATCH v2 2/2] dt-bindings: Add GPIO locked fixed clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unnecessary new binding for a trivial GPIO polarity inversion, fundamentally misunderstanding Device Tree abstractions.
- [High] Broken `anyOf` constraint allows incomplete and invalid clock configurations.
- [Medium] Ambiguous clock aggregation logic and missing size constraints on array properties.
- [Low] Improper use of the generic `gpio-names` property for custom GPIO arrays.
- [Low] Non-generic device tree node name used in the example.
--

commit 616ccdf709928d91bd05dbe7b50cd0691ab27dd7
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

dt-bindings: Add GPIO locked fixed clock

This commit introduces a new device tree binding for a fixed clock whose
state is controlled by an external GPIO.

>     Describe device tree binding for a fixed clock, which state can only be
>     determined by the external GPIO. It's similar to gated-fixed-clock, but
>     the GPIO direction is inverted.

Device tree natively handles signal polarities via the GPIO specifier flags
like GPIO_ACTIVE_LOW. If a gated clock has an active-low enable signal,
could the existing gpio-gate-clock binding be used instead of creating a new
binding just for an inverted GPIO direction?

> diff --git a/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml b/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml
> new file mode 100644
> index 0000000000000..e0256bbd441de
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/gpio-locked-fixed-clock.yaml
> @@ -0,0 +1,77 @@

[ ... ]

> +  clocks:
> +    description: Input clocks that externally locked.

Since #clock-cells is set to 0, meaning this controller provides exactly
one output clock, how do multiple parent clocks logically map to the single
output? Should this property have a maxItems: 1 constraint?

[ ... ]

> +  gpio-names:
> +    description: Names corresponding to each GPIO.
> +    minItems: 1
> +    maxItems: 32

Is it appropriate to use the generic gpio-names property here? In device
tree bindings, gpio-names is typically strictly intended to pair with the
standard gpios property, rather than naming custom prefixed arrays like
locked-gpios.

[ ... ]

> +anyOf:
> +  - required:
> +      - clocks
> +  - required:
> +      - locked-gpios

Does this anyOf constraint allow for invalid clock configurations?

If clocks is omitted, there is no parent clock and no clock-frequency property
supported to define an intrinsic rate. If locked-gpios is omitted, the clock
operates without a locking GPIO, defeating the purpose of the binding.

Should both of these properties be explicitly required instead?

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    clk_gpio_locked: gpio-locked-fixed-clock {

This isn't a bug, but device tree specifications generally require node names
to be generic and reflect the class of the device (like clock or
clock-controller) rather than mirroring the compatible string.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-feature-clock-guard-v2-0-6c25458d5340@bruker.com?part=2