On Fri, May 12, 2023 at 05:23:22PM +0200, Krzysztof Kozlowski wrote: > On 12/05/2023 14:28, Charles Keepax wrote: > > +$id: http://devicetree.org/schemas/mfd/cirrus,cs42l43.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Cirrus Logic CS42L43 Audio CODEC > > That's audio codec, so it should be in sound, not MFD. > Is this true even despite the device being implemented as an MFD? I am happy to move it, and will do so unless I hear otherwise. > > + - VDD_P-supply > > + - VDD_A-supply > > + - VDD_D-supply > > + - VDD_IO-supply > > + - VDD_CP-supply > > lowercase, no underscores in all property names. I guess we can rename all the regulators to lower case. > > +additionalProperties: false > > This order is quite unexpected... please do not invent your own layout. > Use example-schema as your starting point. I suspect there will be many > things to fix, so limited review follows (not complete). > > > Missing ref to dai-common Apologies for that I was a little hesitant about this but this order did make the binding document much more readable, the intentation got really hard to follow in the traditional order. I guess since I have things working now I can put it back, again I will do so unless I hear otherwise. > > + pinctrl: > > + type: object > > additionalProperties: false > Can do. > > + > > + allOf: > > + - $ref: "../pinctrl/pinctrl.yaml#" > > No quotes, absolute path, so /schemas/pinctrl/.... > Can do. > > + > > + properties: > > + pin-settings: > > What's this node about? pinctrl/pinctrl/pins? One level too much. > codec/pinctrl/pins The device is a codec, so the main node should be called codec, then it has a subnode called pinctrl to satisfy the pinctrl DT binding. > > +examples: > > + - | > > + i2c@e0004000 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0xe0004000 0x1000>; > > Drop, just i2c > Can do. > > + > > + cs42l43: codec@1a { > > + compatible = "cirrus,cs42l43"; > > + reg = <0x1a>; > > + > > + VDD_P-supply = <&vdd5v0>; > > + VDD_D-supply = <&vdd1v8>; > > + VDD_A-supply = <&vdd1v8>; > > + VDD_IO-supply = <&vdd1v8>; > > + VDD_CP-supply = <&vdd1v8>; > > + VDD_AMP-supply = <&vdd5v0>; > > + > > + reset-gpios = <&gpio 0>; > > Use proper defines for flags. Can do. Thanks, Charles
