diff mbox series

[v2,4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source

Message ID 20210709155856.1732245-5-liambeguin@gmail.com (mailing list archive)
State Superseded
Delegated to: Jonathan Cameron
Headers show
Series AD7949 Fixes | expand

Commit Message

Liam Beguin July 9, 2021, 3:58 p.m. UTC 
From: Liam Beguin <lvb@xiphos.com>

Add bindings documentation for the adi,reference-source property.
This property is required to properly configure the ADC sample request
based on which reference source should be used for the calculation.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 .../bindings/iio/adc/adi,ad7949.yaml          | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Jonathan Cameron July 10, 2021, 5 p.m. UTC | #1 
On Fri,  9 Jul 2021 11:58:56 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> From: Liam Beguin <lvb@xiphos.com>
> 
> Add bindings documentation for the adi,reference-source property.
> This property is required to properly configure the ADC sample request
> based on which reference source should be used for the calculation.

Should this be per channel? That will effect some of what I say below...

> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  .../bindings/iio/adc/adi,ad7949.yaml          | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> index 9b56bd4d5510..eae3121cad01 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> @@ -35,6 +35,27 @@ properties:
>    "#io-channel-cells":
>      const: 1
>  
> +  adi,reference-select:

This is one field in the register, but it's not one thing, so lets break it up
in DT.  We should do this both to make for more readable dts files and to
enforce the requirements on regulators...

> +    description: |
> +      Select the reference voltage source to use when converting samples.
> +      Acceptable values are:
> +      - 0: Internal reference and temperature sensor enabled.
> +           Vref=2.5V, buffered output
> +      - 1: Internal reference and temperature sensor enabled.
> +           Vref=4.096V, buffered output
> +      - 2: Use external reference, temperature sensor enabled.
> +           Internal buffer disabled
> +      - 3: Use external reference, internal buffer and temperature sensor
> +           enabled.
> +      - 6: Use external reference, internal buffer and temperature sensor
> +           disabled.
> +      - 7: Use external reference, internal buffer enabled.
> +           Internal reference and temperature sensor disabled.

So question 1 is whether to use an external or internal reference.
Normally we'd make the coarse decision of whether to use an external reference
by whether there is a regulator provided.  That won't work so well if we make
this per channel.

Question 2, assuming internal reference, what voltage?  Those should take
an actual voltage (probably in mV and match against an enum of the two possible values).
Binding should check to make sure this isn't specified as well as saying we
are using an external refernce.

Question 3, assuming external reference, is temperature sensor enabled?
- actually dumb question, but why would anyone not want this enabled?  Maybe turn it
off in runtime pm, but in general if you've fitted a chip with a temperature sensor
you at least sometimes want to measure temperature!  So my gut feeling is don't
allow this to be controlled (effectively drop cases 6 and 7 above as being
unlikely to be of interest to anyone)

Question 4, Is the internal buffer enabled when using and external reference.
This one is interesting.   We could just expose it in general, but I wonder
if we can do something that reflects how it is used.  From the various figures in
the datasheet this seems to be coupled to whether the external reference is on
pin REF_IN or pin REF.  If that's the case can we have two optional regs only
one of which should be supplied?  However, this gets more fiddly because
the default right now is vref-supply actually being connected to the vrefin connection.
That's annoying as it stops us using the obvious naming...
Hence I think we can have
vref-supply (actually connected to vrefin) and vref-unbuffered-supply



> +
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3, 6, 7]
> +    default: 7
> +
>  required:
>    - compatible
>    - reg
Liam Beguin July 12, 2021, 5:05 p.m. UTC | #2 
Hi Jonathan,

On Sat Jul 10, 2021 at 1:00 PM EDT, Jonathan Cameron wrote:
> On Fri, 9 Jul 2021 11:58:56 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
>
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > Add bindings documentation for the adi,reference-source property.
> > This property is required to properly configure the ADC sample request
> > based on which reference source should be used for the calculation.
>
> Should this be per channel? That will effect some of what I say below...
>

We could make it per channel. Ideally, I'd also like to add support for
differential channels, so might as well add per channel configurations
now.

> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  .../bindings/iio/adc/adi,ad7949.yaml          | 21 +++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > index 9b56bd4d5510..eae3121cad01 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > @@ -35,6 +35,27 @@ properties:
> >    "#io-channel-cells":
> >      const: 1
> >  
> > +  adi,reference-select:
>
> This is one field in the register, but it's not one thing, so lets break
> it up
> in DT. We should do this both to make for more readable dts files and to
> enforce the requirements on regulators...
>
> > +    description: |
> > +      Select the reference voltage source to use when converting samples.
> > +      Acceptable values are:
> > +      - 0: Internal reference and temperature sensor enabled.
> > +           Vref=2.5V, buffered output
> > +      - 1: Internal reference and temperature sensor enabled.
> > +           Vref=4.096V, buffered output
> > +      - 2: Use external reference, temperature sensor enabled.
> > +           Internal buffer disabled
> > +      - 3: Use external reference, internal buffer and temperature sensor
> > +           enabled.
> > +      - 6: Use external reference, internal buffer and temperature sensor
> > +           disabled.
> > +      - 7: Use external reference, internal buffer enabled.
> > +           Internal reference and temperature sensor disabled.
>
> So question 1 is whether to use an external or internal reference.
> Normally we'd make the coarse decision of whether to use an external
> reference
> by whether there is a regulator provided. That won't work so well if we
> make
> this per channel.
>
> Question 2, assuming internal reference, what voltage? Those should take
> an actual voltage (probably in mV and match against an enum of the two
> possible values).
> Binding should check to make sure this isn't specified as well as saying
> we
> are using an external refernce.
>
> Question 3, assuming external reference, is temperature sensor enabled?
> - actually dumb question, but why would anyone not want this enabled?
> Maybe turn it
> off in runtime pm, but in general if you've fitted a chip with a
> temperature sensor
> you at least sometimes want to measure temperature! So my gut feeling is
> don't
> allow this to be controlled (effectively drop cases 6 and 7 above as
> being
> unlikely to be of interest to anyone)
>

I like your suggestion of breaking this down so far, it would look
something like this:

	ad7949: adc@0 {
		compatible = "adi,ad7949";
		reg = <0>;

		vref-supply = <&vdd_supply>;

		channel@0 {
			adi,internal-ref-mv = <2500>;
			reg = <0>;
		};

		channel@1 {
			reg = <1>;
			/*
			 * defaults to vref-supply if defined or error
			 * out
			 */
		};
	};

> Question 4, Is the internal buffer enabled when using and external
> reference.
> This one is interesting. We could just expose it in general, but I
> wonder
> if we can do something that reflects how it is used. From the various
> figures in
> the datasheet this seems to be coupled to whether the external reference
> is on
> pin REF_IN or pin REF. If that's the case can we have two optional regs
> only
> one of which should be supplied? However, this gets more fiddly because
> the default right now is vref-supply actually being connected to the
> vrefin connection.
> That's annoying as it stops us using the obvious naming...
> Hence I think we can have
> vref-supply (actually connected to vrefin) and vref-unbuffered-supply
>

I really like the idea of using the same names as the datasheet
(vref-supply and vrefin-supply), to infer the buffered state,
but it's annoying (and confusing) that it's setup the other way
right now.

I wonder what happens if the reference is connected to refin and we're
configured as unbuffered (and the other way around).
I looked around and I might be able to test it on one setup I have where
the external reference is connected to REF.

If it's not a breaking change, would it be okay with you to follow the
datasheet naming?

Liam

>
>
> > +
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1, 2, 3, 6, 7]
> > +    default: 7
> > +
> >  required:
> >    - compatible
> >    - reg
Jonathan Cameron July 13, 2021, 9:16 a.m. UTC | #3 
On Mon, 12 Jul 2021 13:05:23 -0400
"Liam Beguin" <liambeguin@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sat Jul 10, 2021 at 1:00 PM EDT, Jonathan Cameron wrote:
> > On Fri, 9 Jul 2021 11:58:56 -0400
> > Liam Beguin <liambeguin@gmail.com> wrote:
> >  
> > > From: Liam Beguin <lvb@xiphos.com>
> > > 
> > > Add bindings documentation for the adi,reference-source property.
> > > This property is required to properly configure the ADC sample request
> > > based on which reference source should be used for the calculation.  
> >
> > Should this be per channel? That will effect some of what I say below...
> >  
> 
> We could make it per channel. Ideally, I'd also like to add support for
> differential channels, so might as well add per channel configurations
> now.
> 
> > > 
> > > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > > ---
> > >  .../bindings/iio/adc/adi,ad7949.yaml          | 21 +++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > > index 9b56bd4d5510..eae3121cad01 100644
> > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > > @@ -35,6 +35,27 @@ properties:
> > >    "#io-channel-cells":
> > >      const: 1
> > >  
> > > +  adi,reference-select:  
> >
> > This is one field in the register, but it's not one thing, so lets break
> > it up
> > in DT. We should do this both to make for more readable dts files and to
> > enforce the requirements on regulators...
> >  
> > > +    description: |
> > > +      Select the reference voltage source to use when converting samples.
> > > +      Acceptable values are:
> > > +      - 0: Internal reference and temperature sensor enabled.
> > > +           Vref=2.5V, buffered output
> > > +      - 1: Internal reference and temperature sensor enabled.
> > > +           Vref=4.096V, buffered output
> > > +      - 2: Use external reference, temperature sensor enabled.
> > > +           Internal buffer disabled
> > > +      - 3: Use external reference, internal buffer and temperature sensor
> > > +           enabled.
> > > +      - 6: Use external reference, internal buffer and temperature sensor
> > > +           disabled.
> > > +      - 7: Use external reference, internal buffer enabled.
> > > +           Internal reference and temperature sensor disabled.  
> >
> > So question 1 is whether to use an external or internal reference.
> > Normally we'd make the coarse decision of whether to use an external
> > reference
> > by whether there is a regulator provided. That won't work so well if we
> > make
> > this per channel.
> >
> > Question 2, assuming internal reference, what voltage? Those should take
> > an actual voltage (probably in mV and match against an enum of the two
> > possible values).
> > Binding should check to make sure this isn't specified as well as saying
> > we
> > are using an external refernce.
> >
> > Question 3, assuming external reference, is temperature sensor enabled?
> > - actually dumb question, but why would anyone not want this enabled?
> > Maybe turn it
> > off in runtime pm, but in general if you've fitted a chip with a
> > temperature sensor
> > you at least sometimes want to measure temperature! So my gut feeling is
> > don't
> > allow this to be controlled (effectively drop cases 6 and 7 above as
> > being
> > unlikely to be of interest to anyone)
> >  
> 
> I like your suggestion of breaking this down so far, it would look
> something like this:
> 
> 	ad7949: adc@0 {
> 		compatible = "adi,ad7949";
> 		reg = <0>;
> 
> 		vref-supply = <&vdd_supply>;
> 
> 		channel@0 {
> 			adi,internal-ref-mv = <2500>;
> 			reg = <0>;
> 		};
> 
> 		channel@1 {
> 			reg = <1>;
> 			/*
> 			 * defaults to vref-supply if defined or error
> 			 * out
> 			 */
> 		};
> 	};
> 
> > Question 4, Is the internal buffer enabled when using and external
> > reference.
> > This one is interesting. We could just expose it in general, but I
> > wonder
> > if we can do something that reflects how it is used. From the various
> > figures in
> > the datasheet this seems to be coupled to whether the external reference
> > is on
> > pin REF_IN or pin REF. If that's the case can we have two optional regs
> > only
> > one of which should be supplied? However, this gets more fiddly because
> > the default right now is vref-supply actually being connected to the
> > vrefin connection.
> > That's annoying as it stops us using the obvious naming...
> > Hence I think we can have
> > vref-supply (actually connected to vrefin) and vref-unbuffered-supply
> >  
> 
> I really like the idea of using the same names as the datasheet
> (vref-supply and vrefin-supply), to infer the buffered state,
> but it's annoying (and confusing) that it's setup the other way
> right now.
> 
> I wonder what happens if the reference is connected to refin and we're
> configured as unbuffered (and the other way around).
> I looked around and I might be able to test it on one setup I have where
> the external reference is connected to REF.
> 
> If it's not a breaking change, would it be okay with you to follow the
> datasheet naming?

Absolutely. If we can get away with fixing that it would be great.

Jonathan

> 
> Liam
> 
> >
> >  
> > > +
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [0, 1, 2, 3, 6, 7]
> > > +    default: 7
> > > +
> > >  required:
> > >    - compatible
> > >    - reg
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
index 9b56bd4d5510..eae3121cad01 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
@@ -35,6 +35,27 @@  properties:
   "#io-channel-cells":
     const: 1
 
+  adi,reference-select:
+    description: |
+      Select the reference voltage source to use when converting samples.
+      Acceptable values are:
+      - 0: Internal reference and temperature sensor enabled.
+           Vref=2.5V, buffered output
+      - 1: Internal reference and temperature sensor enabled.
+           Vref=4.096V, buffered output
+      - 2: Use external reference, temperature sensor enabled.
+           Internal buffer disabled
+      - 3: Use external reference, internal buffer and temperature sensor
+           enabled.
+      - 6: Use external reference, internal buffer and temperature sensor
+           disabled.
+      - 7: Use external reference, internal buffer enabled.
+           Internal reference and temperature sensor disabled.
+
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 6, 7]
+    default: 7
+
 required:
   - compatible
   - reg