Messages in this thread | | | Date | Fri, 12 May 2023 16:15:45 +0000 | From | Charles Keepax <> | Subject | Re: [PATCH 05/10] dt-bindings: mfd: cirrus,cs42l43: Add initial DT binding |
| |
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
| |