Re: [PATCH v1 1/2] dt-binding: soc: nuvoton: Add NPCM BPC LPC documentation

[Krzysztof Kozlowski]

On 22/11/2022 21:12, Tomer Maimon wrote:

1. Subject: drop second, redundant "documentation" (dt-bindings are
documentation).

2. Use subject prefixes matching the subsystem (git log --oneline -- ...).

> Added device tree binding documentation for Nuvoton BMC NPCM BIOS Post
> Code (BPC).
> 
> The NPCM BPC monitoring two configurable I/O addresses written by the
> host on Low Pin Count (LPC) bus.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx>
> ---
>  .../bindings/soc/nuvoton/npcm-lpc-bpc.yaml    | 112 ++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml b/Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml
> new file mode 100644
> index 000000000000..2c8e66546891
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/nuvoton/npcm-lpc-bpc.yaml

Filename should match compatibles, at least in the "vendor,device"
style, so for example: nuvoton,lpc.yaml

> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/nuvoton/npcm-lpc-bpc.yaml#

LPC is a generic bus, so this should not be in "soc" directory. Where?
Depends what is this... Generic bus bindings could be in "bus" directory
or dedicated "lpc", if we have more of these.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton Low Pin Count (LPC) Bus Controller
> +
> +maintainers:
> +  - Tomer Maimon <tmaimon77@xxxxxxxxx>
> +
> +description:
> +  The Low Pin Count (LPC) is a low bandwidth bus that is used to connect
> +  peripherals around the CPU and to replace the Industry Standard Architecture
> +  (ISA) bus.

You need to decide whether you describe here bus, bus controller or
device on the bus.

> +
> +  The Nuvoton NPCM LPC bus is a bridge of host CPU to a several of peripheral
> +  devices.

LPC bus is a bridge? It's either incorrect or so generic that every bus
is a "bridge"?

> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - nuvoton,npcm750-lpc
> +          - nuvoton,npcm845-lpc
> +      - const: simple-mfd
> +      - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  ranges: true
> +
> +patternProperties:
> +  "^lpc_bpc@[0-9a-f]+$":

No underscores in node names. Generic node names, so maybe "bpc"

This also does not match your example at all.


> +    type: object
> +    additionalProperties: false
> +
> +    description:
> +      Nuvoton BMC NPCM BIOS Post Code (BPC) monitoring two configurable I/O
> +      addresses written by the host on the Low Pin Count (LPC) bus, the capure

typo: capture

> +      data stored in 128-word FIFO.
> +
> +      NPCM BPC supports capture double words, when using capture
> +      double word only I/O address 1 is monitored.

This sentence is not grammatically correct. BPC supports capturing
double words when using double word capturing? Aren't these two sentences?

> +
> +    properties:
> +      compatible:
> +        items:

No items here.

> +          - enum:
> +              - nuvoton,npcm750-lpc-bpc
> +              - nuvoton,npcm845-lpc-bpc
> +
> +      reg:
> +        maxItems: 1
> +
> +      interrupts:
> +        maxItems: 1
> +
> +      nuvoton,monitor-ports:
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        description: 
> +          Contain monitor I/O addresses, at least one monitor I/O address 

Contains

But you need to explain what are these... I/O addresses on the bus?

> +          required.
> +
> +      nuvoton,bpc-en-dwcapture:
> +        description: If present, Enable capture double words support.

Is it the same as reg-io-width?

> +        type: boolean
> +
> +    required:
> +      - compatible
> +      - reg
> +      - interrupts
> +      - nuvoton,monitor-ports
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges
> +
> +additionalProperties:
> +  type: object

No, only bus schemas could have it. Here additionalProperties: false.

It seems there are already few LPC controllers and all are put in
different places:
Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
Documentation/devicetree/bindings/arm/hisilicon/low-pin-count.yaml

Maybe Rob why this was made not really as two bindings - for bus
controller and devices?

Best regards,
Krzysztof