Re: [PATCH v4 2/7] dt-bindings: serial: 8250: aspeed: add aspeed,vuart-over-pci bool prop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing schema constraint for `syscon` property allows it on all generic 8250 devices.
--

--- Patch [2]: [PATCH v4 2/7] dt-bindings: serial: 8250: aspeed: add aspeed,vuart-over-pci bool prop ---
commit 4861ff4b770f95ffb3bbce902547b1d91b91628c
Author: Grégoire Layet <gregoire.layet@9elements.com>

dt-bindings: serial: 8250: aspeed: add aspeed,vuart-over-pci bool prop
    
This commit adds a boolean property to specify if a Virtual UART is used
over PCI on the ASPEED AST2600. It also adds a syscon phandle requirement
when this property is present.

> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index 8f22121381fc4..2ea8981bba80f 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -27,6 +27,16 @@ allOf:
>              enum:
>                - aspeed,ast2500-vuart
>                - aspeed,ast2600-vuart
> +  - if:
> +      required:
> +        - aspeed,vuart-over-pci
> +    then:
> +      required:
> +        - syscon
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2600-vuart

[ ... ]

> @@ -223,6 +233,12 @@ properties:
>            - const: uartclk
>            - const: reg
>  
> +  syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the SCU syscon, used to enable the VUART over the BMC PCI
> +      device. Only applicable to aspeed,ast2600-vuart.

[Severity: Medium]
Will this schema allow the syscon property on all generic 8250 devices?

Because syscon is defined in the top-level properties block, and the newly
added if block above only evaluates when aspeed,vuart-over-pci is present
in the node, it seems there is no constraint restricting the use of syscon on
other unrelated 8250 nodes (e.g., ns16550a). Device trees could specify a
syscon property on non-AST2600 UARTs and silently pass schema validation,
despite the documentation stating it is only applicable to
aspeed,ast2600-vuart.

Should there be an additional conditional block restricting syscon to
aspeed,ast2600-vuart as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783524645.git.gregoire.layet@9elements.com?part=2