Re: [PATCH v12 1/5] dt-bindings: clock: airoha: Document support for AN7583 clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] dt-bindings: clock: airoha: unwieldy per-compatible conditionals
  in schema
- [Low] dt-bindings: clock: airoha: inaccurate description of
  airoha,chip-scu
--

commit 078856d9f139a456969b8b085cbf1d617f56642a
Author: Christian Marangi <ansuelsmth@gmail.com>

dt-bindings: clock: airoha: Document support for AN7583 clock

This commit documents Device Tree bindings for the Airoha AN7583 clock and
reset controller, adding the airoha,chip-scu property and clock/reset ID
definitions.

> diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> index fc37786ac7ed8..05cd20ce3ab91 100644
> --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml
> +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml

[ ... ]

> @@ -57,12 +58,36 @@ properties:
>        available serdes port.
>      const: 1
>  
> +  airoha,chip-scu:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the Chip SCU providing the registers required
> +      for configuring the PCIe related clocks and resets.

[Severity: Low]
As reported by Brian Masney on v9 [1], is the description of
airoha,chip-scu swapped with the main SCU register block?  As noted in the
commit message and implemented in drivers/clk/clk-en7523.c, the Chip SCU
provides base clock rate and divider registers, whereas PCIe clocks and
resets live in the main SCU region.

>  required:
>    - compatible
>    - reg
>    - '#clock-cells'
>  
>  allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: airoha,an7583-scu
> +
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 1
> +
> +      required:
> +        - airoha,chip-scu
> +        - '#reset-cells'
> +        - '#phy-cells'
> +
> +    else:
> +      properties:
> +        airoha,chip-scu: false

[Severity: Low]
This isn't a bug, but with airoha,an7583-scu added, every compatible in
this binding has its own if/then block under allOf with different rules for
reg, #phy-cells, #reset-cells, and airoha,chip-scu.  Would it be cleaner to
split the diverging variants into a separate schema document (or at least
consolidate the shared constraints) rather than adding another
per-compatible conditional branch here?

[1] https://lore.kernel.org/r/179383990045.753844.10400171550739497565.b4-review@b4

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110000753.508-1-ansuelsmth@gmail.com?part=1