| Message ID | 20230421085245.302169-2-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add the Renesas X9250 potentiometers IIO support | expand |
On 21/04/2023 10:52, Herve Codina wrote: > The Renesas X9250 is a quad digitally controlled potentiometers. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > .../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 Apologies, I missed it last time - you do not need iio.yaml. It's coming from core schema and is always selected. You won't find its usage anywhere in the kernel (git grep iio.yaml) With iio.yaml dropped: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
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>; > + }; > + };
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>; > > + }; > > + }; >
On Mon, 24 Apr 2023 09:03:18 +0200 Herve Codina <herve.codina@bootlin.com> wrote: > 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) ? I think so. I'm not totally sure on how DT maintainers think we should deal with a two voltage level reference though. Perhaps add some description to make it very clear what is going on and we'll see what review comments we get! Jonathan
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>; + }; + };
The Renesas X9250 is a quad digitally controlled potentiometers. Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- .../iio/potentiometer/renesas,x9250.yaml | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml