Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 12 May 2023 16:15:45 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
CC:     <broonie@kernel.org>, <lee@kernel.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <tglx@linutronix.de>, <maz@kernel.org>, <linus.walleij@linaro.org>,
        <vkoul@kernel.org>, <lgirdwood@gmail.com>,
        <yung-chuan.liao@linux.intel.com>, <sanyog.r.kale@intel.com>,
        <pierre-louis.bossart@linux.intel.com>,
        <alsa-devel@alsa-project.org>, <patches@opensource.cirrus.com>,
        <devicetree@vger.kernel.org>, <linux-gpio@vger.kernel.org>,
        <linux-spi@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 05/10] dt-bindings: mfd: cirrus,cs42l43: Add initial DT
 binding
Message-ID: <20230512161545.GL68926@ediswmail.ad.cirrus.com>
References: <20230512122838.243002-1-ckeepax@opensource.cirrus.com>
 <20230512122838.243002-6-ckeepax@opensource.cirrus.com>
 <25c92476-7bca-90c4-9130-cb765495a783@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <25c92476-7bca-90c4-9130-cb765495a783@linaro.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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