Re: [PATCH 05/10] dt-bindings: mfd: cirrus,cs42l43: Add initial DT binding

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

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