Re: [PATCH v9 1/6] dt-bindings: rtc: Add pinctrl for S35390A

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: missing #clock-cells in required list
- [Medium] dt-bindings: undocumented clock provider capability
- [Medium] dt-bindings: missing constraint for pmin2 pin function
--

commit ee787a668e377f31a7722c1acf8927b88011aa60
Author: Markus Probst <markus.probst@posteo.de>
Subject: dt-bindings: rtc: Add pinctrl for S35390A

This commit moves the devicetree bindings for the S-35390A RTC into its own
schema file. It adds the necessary properties to configure individual pins via
pinctrl, allowing the interrupt signal to be used for a wakeup alarm.

> diff --git a/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
> new file mode 100644
> index 0000000000000..f3bf5900fb3ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
> @@ -0,0 +1,121 @@

[ ... ]

> +  wakeup-parent: true
> +
> +  wakeup-source: true
> +
> +  "#clock-cells":
> +    const: 1

[Severity: Medium]
The commit message states the change adds pinctrl to allow the interrupt
signal 1 to be used for a wakeup alarm. However, the schema also introduces
"#clock-cells" and a "clock" output pin function.

Does this introduce a significant undocumented capability (clock provider
functionality) which is unstated in the commit description?

[ ... ]

> +          function:
> +            description: |
> +              Pin function:
> +              - ignore:  Preserve the previous state.
> +              - disable: Disable pin output.
> +              - wakeup:  Output wakes up the system.
> +              - clock:   Output clock pulse.
> +              - pmin1:   Minute periodical output with 50% duty.
> +              - pmin2:   Minute periodical output L for 7.81 ms.
> +                         Can only be used with pin int1.
> +            enum:
> +              - ignore
> +              - disable
> +              - wakeup
> +              - clock
> +              - pmin1
> +              - pmin2

[Severity: Medium]
The description for the "pmin2" function explicitly states it can only be used
with pin int1.

Because there is no conditional allOf/if/then block at the pinmux node level
linking them, will a device tree incorrectly using pins = "int2"; function =
"pmin2"; silently pass schema validation despite being an invalid hardware
configuration?

[ ... ]

> +required:
> +  - compatible
> +  - reg

[Severity: Medium]
According to the dt-bindings subsystem rules, when hardware gains new provider
capabilities, the corresponding standard properties must be added to the
required list with appropriate constraints.

Should "#clock-cells" be added to this required list to prevent incomplete
device tree nodes from silently passing schema validation?

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rtc@30 {

[Severity: Medium]
According to the dt-bindings subsystem rules, when hardware gains new provider
capabilities, the corresponding standard properties must appear in the examples.

Should the example device tree node be updated to include the required
"#clock-cells" property?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-rtc_s35390a_int1-v9-0-b2567a3e4959@posteo.de?part=1