Re: [PATCH v2 1/3] dt-binding: aspeed: Add LPC PCC controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 04/03/2025 11:44, Kevin Chen wrote:
> Add dt-bindings for Aspeed for Aspeed LPC POST code capture controller.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

Missing 's'.

> 
> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/mfd/aspeed-lpc.yaml   | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml b/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
> index 5dfe77aca167..367847bd7e75 100644
> --- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
> +++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
> @@ -149,6 +149,35 @@ patternProperties:
>        - interrupts
>        - snoop-ports
>  
> +  "^lpc-pcc@[0-9a-f]+$":
> +    type: object
> +    additionalProperties: false
> +
> +    description:
> +      The LPC pcc interface allows the BMC to listen on and record the data
> +      bytes written by the Host to the targeted LPC I/O pots.
> +
> +    properties:
> +      compatible:
> +        items:
> +          - enum:
> +              - aspeed,ast2600-lpc-pcc
> +
> +      reg:
> +        maxItems: 1
> +
> +      interrupts:
> +        maxItems: 1
> +
> +      pcc-ports:

Missing vendor prefix

> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        description: The LPC I/O ports to pcc

Description is too vague. Why would we encode I/O ports as some numbers
instead of GPIOs for example? If these are ports, why this is not a graph?

Missing constraints - min/maxItems, defaults, minimum/maximum etc.

> +
> +    required:
> +      - compatible
> +      - interrupts
> +      - pcc-ports
> +
>    "^uart-routing@[0-9a-f]+$":
>      $ref: /schemas/soc/aspeed/uart-routing.yaml#
>      description: The UART routing control under LPC register space
> @@ -176,6 +205,13 @@ examples:
>          #size-cells = <1>;
>          ranges = <0x0 0x1e789000 0x1000>;
>  
> +        lpc_pcc: lpc-pcc@0 {
> +            compatible = "aspeed,ast2600-lpc-pcc";
> +            reg = <0x0 0x140>;
> +            interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>;
> +            pcc-ports = <0x80>;

So what 0x80 stands for?


Best regards,
Krzysztof