| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 21 May 2024 07:24:05 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: Alvin Šipraga <alvin@...s.dk>, Mark Brown
<broonie@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski
<brgl@...ev.pl>, Liam Girdwood <lgirdwood@...il.com>, Jaroslav Kysela
<perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>, Michael Turquette
<mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, Andi Shyti
<andi.shyti@...nel.org>, Saravana Kannan <saravanak@...gle.com>, Emil
Abildgaard Svendsen <EMAS@...g-olufsen.dk>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-gpio@...r.kernel.org"
<linux-gpio@...r.kernel.org>, "linux-sound@...r.kernel.org"
<linux-sound@...r.kernel.org>, "linux-clk@...r.kernel.org"
<linux-clk@...r.kernel.org>, "linux-i2c@...r.kernel.org"
<linux-i2c@...r.kernel.org>
Subject: Re: [PATCH 03/13] dt-bindings: a2b: Analog Devices AD24xx devices
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@...g-olufsen.dk>
> >
> > Add device tree bindings for the AD24xx series A2B transceiver chips,
> > including their functional blocks.
> >
> > Signed-off-by: Alvin Šipraga <alsi@...g-olufsen.dk>
> > ---
> > .../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@...g-olufsen.dk>
> > +
> > +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@...g-olufsen.dk>
> > +
> > +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@...g-olufsen.dk>
> > +
> > +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
Powered by blists - more mailing lists