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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230512161545.GL68926@ediswmail.ad.cirrus.com>
Date:   Fri, 12 May 2023 16:15:45 +0000
From:   Charles Keepax <ckeepax@...nsource.cirrus.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
CC:     <broonie@...nel.org>, <lee@...nel.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <tglx@...utronix.de>, <maz@...nel.org>, <linus.walleij@...aro.org>,
        <vkoul@...nel.org>, <lgirdwood@...il.com>,
        <yung-chuan.liao@...ux.intel.com>, <sanyog.r.kale@...el.com>,
        <pierre-louis.bossart@...ux.intel.com>,
        <alsa-devel@...a-project.org>, <patches@...nsource.cirrus.com>,
        <devicetree@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
        <linux-spi@...r.kernel.org>, <linux-kernel@...r.kernel.org>
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@...04000 {
> > +        #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