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:   Mon, 24 Apr 2023 09:03:18 +0200
From:   Herve Codina <herve.codina@bootlin.com>
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v3 1/3] dt-bindings: iio: potentiometer: Add the Renesas
 X9250 potentiometers
Message-ID: <20230424090318.4750a5e7@bootlin.com>
In-Reply-To: <20230422171807.510d7fa3@jic23-huawei>
References: <20230421085245.302169-1-herve.codina@bootlin.com>
        <20230421085245.302169-2-herve.codina@bootlin.com>
        <20230422171807.510d7fa3@jic23-huawei>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Jonathan, Krzysztof,

On Sat, 22 Apr 2023 17:18:07 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 21 Apr 2023 10:52:43 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > The Renesas X9250 is a quad digitally controlled potentiometers.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>  
> 
> Hi Herve,
> 
> Historically we've been a bit lax in IIO bindings in always making
> sure the per supplies are included.  As a result we frequently get
> them added later and it just makes things messier than they should
> be.
> 
> So please add vcc-supply from the start.  V+ and V- are a little trickier.
> I was expecting datasheet to say they should be symmetric about 0 but it
> doesn't. So they could be two independent supplies.
> 
> Also make it required as my current understanding is that we should
> do that for supplies that are definitely present even if we could
> rely on the fallback to regulator stubs if they aren't supplied.
> So add the 3 supplies to required as well.

Yes, I will add the following supplies in the next iteration:
 - 'vcc-supply' for VCC
 - 'avp-supply' for the analog V+
 - 'avn-supply' for the analog V-

and add them in the required list of properties.

Are the names correct for these power supplies (avp and avn) ?

> 
> Less of a requirement, but you might want to also provide an optional 
> gpio for the not WP pin on basis someone might wire it up to the host processor.

I will add the 'wp-gpios' property.

> 
> Beyond the comment Krzystof made on iio.yaml this otherwise looks good to me.

And for the Krzystof comment on iio.yaml, as he suggested, I will drop iio.yaml.

Thanks for the review,
Hervé

> 
> 
> 
> 
> > ---
> >  .../iio/potentiometer/renesas,x9250.yaml      | 54 +++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> > new file mode 100644
> > index 000000000000..dfa36b23eb0d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> > @@ -0,0 +1,54 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/potentiometer/renesas,x9250.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas X9250 quad potentiometers
> > +
> > +maintainers:
> > +  - Herve Codina <herve.codina@bootlin.com>
> > +
> > +description:
> > +  The Renesas X9250 integrates four digitally controlled potentiometers.
> > +  On each potentiometer, the X9250T has a 100 kOhms total resistance and the
> > +  X9250U has a 50 kOhms total resistance.
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml
> > +  - $ref: /schemas/iio/iio.yaml
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - renesas,x9250t
> > +      - renesas,x9250u
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#io-channel-cells':
> > +    const: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 2000000
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#io-channel-cells'
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        potentiometer@0 {
> > +            compatible = "renesas,x9250t";
> > +            reg = <0>;
> > +            spi-max-frequency = <2000000>;
> > +            #io-channel-cells = <1>;
> > +        };
> > +    };  
>