Re: [PATCH v7 1/2] dt-bindings: clock: gpio-gate-clock: Add a new compatible string

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: commit message and compatible string mismatch
- [Low] dt-bindings: incomplete conditional validation of mutually exclusive properties
- [Low] dt-bindings: non-generic node name in example
--

commit 91b1494a8737e6c880936e93a4ab375dedf59e96
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

dt-bindings: clock: gpio-gate-clock: Add a new compatible string

The patch extends the binding to support a new clock provider that exposes a
fixed-rate clock whose availability depends on GPIO lock-status signals. It
updates the yaml to include properties for this new type of clock.

>     dt-bindings: clock: gpio-gate-clock: Add a new compatible string
>     
>     Extend the binding to be used also for gpio-locked-fixed-clock.
>     A GPIO-locked fixed clock provider exposes a fixed-rate clock whose

[Severity: Medium]
This isn't a bug, but there is a mismatch between the commit message and the
implementation. The commit message explicitly states it extends the binding
for "gpio-locked-fixed-clock", but the added compatible string in the yaml
is "gpio-locked-clock". Could the commit message or the compatible string be
updated to match?

> diff --git a/Documentation/devicetree/bindings/clock/gpio-gate-clock.yaml b/Documentation/devicetree/bindings/clock/gpio-gate-clock.yaml
> index d09d0e3f0c6ec..1ad87c8b36b9f 100644
> --- a/Documentation/devicetree/bindings/clock/gpio-gate-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/gpio-gate-clock.yaml

[ ... ]

>  required:
>    - compatible
>    - '#clock-cells'
> -  - enable-gpios
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: gpio-gate-clock
> +    then:
> +      required:
> +        - enable-gpios
> +  - if:
> +      properties:
> +        compatible:
> +          const: gpio-locked-clock
> +    then:
> +      required:
> +        - enabled-gpios

[Severity: Low]
Does this schema sufficiently restrict invalid property combinations?

By defining "enabled-gpios" globally, "additionalProperties: false" no longer
rejects it for the original "gpio-gate-clock" compatible. Since the mutually
exclusive properties are not explicitly forbidden in the conditional blocks, a
device tree containing compatible = "gpio-gate-clock" and an erroneously added
"enabled-gpios" property will silently pass schema validation.

Should these blocks forbid the mutually exclusive properties (e.g., using
enabled-gpios: false for gpio-gate-clock)?

>  additionalProperties: false
>  
> @@ -40,3 +61,10 @@ examples:
>          #clock-cells = <0>;
>          enable-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
>      };
> +
> +    clk_gpio_locked {
> +        compatible = "gpio-locked-clock";
> +        #clock-cells = <0>;
> +        clocks = <&pll>;
> +        enabled-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
> +    };

[Severity: Low]
Does this example violate the standard Device Tree naming conventions?

The node name "clk_gpio_locked" is non-generic and contains underscores.
Should this use a generic node name describing the device function (like
"clock" or "clock-xxx") to avoid introducing an anti-pattern that users may
copy-paste and generate dtc warnings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927-feature-clock-guard-v7-0-8dd19568d9a2@bruker.com?part=1