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
