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

[Tomer Maimon]

Hi Krzysztof,

Thanks a lot for your comments.

On Wed, 23 Nov 2022 at 12:03, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 22/11/2022 21:12, Tomer Maimon wrote:
>
> 1. Subject: drop second, redundant "documentation" (dt-bindings are
> documentation).
O.K.
>
> 2. Use subject prefixes matching the subsystem (git log --oneline -- ...).
this is what I did dt-binding: soc: nuvoton... do you mean dt-binding: nuvoton.
>
> > 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.
The BPC can run also on the eSPI bus therefore I think it better to
remove the LPC and describe only the BPC module it self.
>
> > +$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?
As mention above, next patch I will describe only the BPC device.
>
> Best regards,
> Krzysztof
>

In general, I waiting for Arnd approval for adding the NPCM BPC driver to SoC.
After Arnd approval, I will send a new patch revision.

Best regards,

Tomer