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:   Sat, 22 Apr 2023 17:18:07 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Herve Codina <herve.codina@bootlin.com>
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: <20230422171807.510d7fa3@jic23-huawei>
In-Reply-To: <20230421085245.302169-2-herve.codina@bootlin.com>
References: <20230421085245.302169-1-herve.codina@bootlin.com>
        <20230421085245.302169-2-herve.codina@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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.

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

Thanks,

Jonathan


> ---
>  .../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>;
> +        };
> +    };