On Sun, May 19, 2024 at 01:40:25PM GMT, Krzysztof Kozlowski wrote: > On 17/05/2024 14:58, Alvin Šipraga wrote: > > From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx> > > > > Add device tree bindings for the AD24xx series A2B transceiver chips, > > including their functional blocks. > > > > Signed-off-by: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx> > > --- > > .../devicetree/bindings/a2b/adi,ad24xx-clk.yaml | 53 +++++ > > What is a2b and why clock bindings are not in clock? > > > .../devicetree/bindings/a2b/adi,ad24xx-codec.yaml | 52 +++++ > > .../devicetree/bindings/a2b/adi,ad24xx-gpio.yaml | 76 +++++++ > > .../devicetree/bindings/a2b/adi,ad24xx-i2c.yaml | 55 +++++ > > .../devicetree/bindings/a2b/adi,ad24xx.yaml | 253 +++++++++++++++++++++ > > Sorry, all this looks weirdly placed. Alright, I'll move the bindings to their respective directories. Wasn't sure what is preferred. > > > 5 files changed, 489 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/a2b/adi,ad24xx-clk.yaml b/Documentation/devicetree/bindings/a2b/adi,ad24xx-clk.yaml > > new file mode 100644 > > index 000000000000..819efaa6a3f9 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/a2b/adi,ad24xx-clk.yaml > > @@ -0,0 +1,53 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/a2b/adi,ad24xx-clk.yaml > > +$schema: http://devicetree.org/meta-schemas/core.yaml > > + > > +title: Analog Devices Inc. AD24xx clock functional block > > + > > +maintainers: > > + - Alvin Šipraga <alsi@xxxxxxxxxxxxxxx> > > + > > +allOf: > > + - $ref: /schemas/clock/clock.yaml > > Drop. There is no single binding doing this, which is usually a hint you > do something not correct. > > > + > > +properties: > > + compatible: > > + enum: > > + - adi,ad2420-clk > > + - adi,ad2421-clk > > + - adi,ad2422-clk > > + - adi,ad2425-clk > > + - adi,ad2426-clk > > + - adi,ad2427-clk > > + - adi,ad2428-clk > > + - adi,ad2429-clk > > + > > This is just incomplete. See other bindings how clock controller is written. OK, will review. > > > +required: > > + - compatible > > + - clock-output-names > > + > > +unevaluatedProperties: false > > additionalProperties: false OK > > + > > +examples: > > + - | > > + a2b { > > + #address-cells = <1>; > > + #size-cells = <0>; > > Not related, drop entire node. OK > > > + > > + node@1 { > > + compatible = "adi,ad2425-node"; > > Not related, drop entire node. OK > > > + reg = <1>; > > + interrupts = <1>; > > + adi,tdm-mode = <16>; > > + adi,tdm-slot-size = <32>; > > + > > + clock { > > + compatible = "adi,ad2425-clk"; > > + #clock-cells = <1>; > > + clock-indices = <1>; > > + clock-output-names = "A2B1 CLKOUT2"; > > + }; > > + }; > > + }; > > diff --git a/Documentation/devicetree/bindings/a2b/adi,ad24xx-codec.yaml b/Documentation/devicetree/bindings/a2b/adi,ad24xx-codec.yaml > > new file mode 100644 > > index 000000000000..eee12f1c810e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/a2b/adi,ad24xx-codec.yaml > > @@ -0,0 +1,52 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/a2b/adi,ad24xx-codec.yaml > > +$schema: http://devicetree.org/meta-schemas/core.yaml > > + > > +title: Analog Devices Inc. AD24xx I2S/TDM functional block > > + > > +maintainers: > > + - Alvin Šipraga <alsi@xxxxxxxxxxxxxxx> > > + > > +allOf: > > + - $ref: /schemas/sound/dai-common.yaml# > > Why full path? It's the same directory, isn't it? In this case no, but when I move it into sound, yes. So your comment is acknowledged and will be addressed in v2. > > > + > > +properties: > > + compatible: > > + enum: > > + - adi,ad2403-codec > > + - adi,ad2410-codec > > + - adi,ad2425-codec > > + - adi,ad2428-codec > > + - adi,ad2429-codec > > + > > + '#sound-dai-cells': > > + const: 0 > > + > > +required: > > + - compatible > > + - '#sound-dai-cells' > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + a2b { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + node@2 { > > + compatible = "adi,ad2428-node"; > > + reg = <2>; > > + interrupts = <2>; > > + adi,tdm-mode = <8>; > > + adi,tdm-slot-size = <32>; > > Same comments. Limited review follows. Ack > > > ... > > > > + > > +required: > > + - compatible > > + > > +unevaluatedProperties: false > > Sorry, but not. No resources, nothing here. Do not create bindings just > to instantiate drivers. Do you mean that there is no need to introduce a binding for this codec if it has the same bindings as dai-common.yaml? Basically that is the case, but #sound-dai-cells should be <0>. Is that not enough? I am OK to just drop the binding if you think so, but I would think that the compatible string should be somewhere in the bindings. Could you explain a little more what you mean? > > > ... > > > diff --git a/Documentation/devicetree/bindings/a2b/adi,ad24xx.yaml b/Documentation/devicetree/bindings/a2b/adi,ad24xx.yaml > > new file mode 100644 > > index 000000000000..dcda15e8032a > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/a2b/adi,ad24xx.yaml > > @@ -0,0 +1,253 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/a2b/adi,ad24xx.yaml > > +$schema: http://devicetree.org/meta-schemas/core.yaml > > + > > +title: Analog Devices Inc. AD24xx Automotive Audio Bus A2B Transceiver > > + > > +description: | > > + AD24xx chips provide A2B bus functionality together with several peripheral > > What is A2B? I will improve the description per your review comments. > > > + functions, including GPIO, I2S/TDM, an I2C controller interface, and > > + programmable clock outputs. > > + > > +maintainers: > > + - Alvin Šipraga <alsi@xxxxxxxxxxxxxxx> > > + > > +properties: > > + compatible: > > + enum: > > + - adi,ad2403 > > + - adi,ad2410 > > + - adi,ad2425 > > + - adi,ad2428 > > + - adi,ad2429 > > + > > reg: is second property. Ack > > > + reg-names: > > + items: > > + - const: base > > + - const: bus > > + > > + reg: > > + items: > > + - description: Normal I2C address of the chip > > + - description: Auxiliary BUS_ADDR I2C address of the chip > > + > > + '#address-cells': > > + const: 1 > > + > > + '#size-cells': > > + const: 0 > > + > > + clock-names: > > + items: > > + - const: sync > > Again misordered. -names always follow main entry. Anyway, clock-names > for just one entry is not really useful. Ack > > > + > > + clocks: > > + items: > > + - description: SYNC input pin clock source > > + > > + vin-supply: > > + description: Optional regulator for supply voltage to VIN pin > > + > > + bus-supply: > > + description: Optional regulator for out-of-band supply voltage to > > + subodrinate nodes' VIN pins > > + > > + interrupts: true > > ??? This must be specific. Right, it should be: maxItems: 1 That's specific, right? > > > + > > + interrupt-controller: true > > + > > + '#interrupt-cells': > > + const: 1 > > + > > +patternProperties: > > + '^node@[0-9]+$': > > + type: object > > + unevaluatedProperties: false > > Why? This must be additionalProperties: false, or I miss something... I think you are right, but I will review this before sending v2. Thanks. > > > + > > + properties: > > + compatible: > > + enum: > > + - adi,ad2401-node > > + - adi,ad2402-node > > + - adi,ad2403-node > > + - adi,ad2410-node > > + - adi,ad2420-node > > + - adi,ad2421-node > > + - adi,ad2422-node > > + - adi,ad2425-node > > + - adi,ad2426-node > > + - adi,ad2427-node > > + - adi,ad2428-node > > + - adi,ad2429-node > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + interrupt-controller: true > > + > > + '#interrupt-cells': > > + const: 1 > > + > > + gpio: > > + $ref: adi,ad24xx-gpio.yaml# > > + > > + codec: > > + $ref: adi,ad24xx-codec.yaml# > > + > > + i2c: > > + $ref: adi,ad24xx-i2c.yaml# > > + > > + clock: > > + $ref: adi,ad24xx-clk.yaml# > > + > > + adi,tdm-mode: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: TDM mode > > Please do not add descriptions which are copies of property name. You > basically said ZERO here. Say something useful... Ack > > > + enum: [2, 4, 8, 12, 16, 20, 24, 32] > > + > > + adi,tdm-slot-size: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: TDM slot size > > + enum: [16, 32] > > + > > + adi,invert-sync: > > + description: Falling edge of SYNC pin indicates the start of an audio > > + frame, as opposed to rising edge. > > + type: boolean > > + > > + adi,early-sync: > > + description: The SYNC pin changes one cycle before the MSB of the first > > + data slot. > > + type: boolean > > + > > + adi,alternating-sync: > > + description: Drive SYNC pin during first half of I2S/TDM data > > + transmission rather than just pulsing it for once cycle. > > + type: boolean > > + > > + adi,rx-on-dtx1: > > + description: Use the DTX1 pin for I2S/TDM RX in place of the DRX1 pin. > > + type: boolean > > + > > + adi,a2b-external-switch-mode-1: > > + description: Use external switch mode 1 instead of 0 on the assumption > > + that the downstream node is not using A2B bus power. > > + type: boolean > > + > > + adi,drive-strength: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: Configures drive strength low (0) or high (1, default). > > + enum: [0, 1] > > + default: 1 > > + > > + adi,invert-interrupt: > > + description: Invert polarity of IRQ pin, making it active low. > > + type: boolean > > + > > + adi,tristate-interrupt: > > + description: Rather than always actively driving the IRQ pin, only drive > > + when the interrupt is active and otherwise set to tristate (high-Z). > > + type: boolean > > It looks you put all children properties into parent node... With so > little explanation tricky to judge. I will put some more information into the binding so that it is more understandable without reading the cover letter. Hopefully things will be clearer for the next review and you can reconsider. > > > + > > + required: > > + - compatible > > + - reg > > + - adi,tdm-mode > > + - adi,tdm-slot-size > > + > > + dependencies: > > + interrupt-controller: [ '#interrupt-cells' ] > > + '#interrupt-cells': [ interrupt-controller ] > > + > > +required: > > + - compatible > > + - reg-names > > + - reg > > + - clock-names > > + - clocks > > + - '#address-cells' > > + - '#size-cells' > > + - interrupts > > + - interrupt-controller > > + - '#interrupt-cells' > > + - node@0 > > + > > +unevaluatedProperties: false > > + > > +examples: > > + - | > > + sync_clk: sync-clock { > > Drop, not related. If the clock is required (as it is) then I have to reference some phandle in the example, else the example will fail the check (missing required property 'clocks'). That's why I put it here. Please advise. > > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <48000>; > > + }; > > + > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + a2b@68 { > > + compatible = "adi,ad2428"; > > + reg-names = "base", "bus"; > > + reg = <0x68>, <0x69>; > > > Please follow DTS coding style. Do not introduce entire different style > and order of properties. reg-names IS NEVER the second property. OK
