diff mbox series

[3/3] iio: adc: aspeed: Setting ref_voltage in probe

Message ID 20201013103245.16723-4-billy_tsai@aspeedtech.com
State Changes Requested, archived
Headers show
Series Make driver compatible with ast2600 | expand

Checks

Context Check Description
robh/checkpatch warning total: 1 errors, 0 warnings, 0 lines checked
robh/checkpatch success

Commit Message

Billy Tsai Oct. 13, 2020, 10:32 a.m. UTC
At ast2600 ref_voltage becomes configurable and this property is board
dependency.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../devicetree/bindings/iio/adc/aspeed_adc.txt   | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Rob Herring (Arm) Oct. 16, 2020, 5:16 p.m. UTC | #1
On Tue, Oct 13, 2020 at 06:32:45PM +0800, Billy Tsai wrote:
> At ast2600 ref_voltage becomes configurable and this property is board
> dependency.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  .../devicetree/bindings/iio/adc/aspeed_adc.txt   | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> index 034fc2ba100e..0ba1980c4e06 100644
> --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> @@ -3,8 +3,11 @@ Aspeed ADC
>  This device is a 10-bit converter for 16 voltage channels.  All inputs are
>  single ended.
>  
> +At ast2600, this device split into two individual IPs and each contains 8 voltage channels.
> +
> +Chip level dtsi:
>  Required properties:
> -- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
> +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" or "aspeed,ast2600-adc"
>  - reg: memory window mapping address and length
>  - clocks: Input clock used to derive the sample clock. Expected to be the
>            SoC's APB clock.
> @@ -20,3 +23,14 @@ Example:
>  		resets = <&syscon ASPEED_RESET_ADC>;
>  		#io-channel-cells = <1>;
>  	};
> +
> +Board level dts:

This split is convention, but not relevant to the binding.

> +Required properties:
> +- ref_voltage: (ast2600 only)

s/_/-/

And needs a vendor prefix.

> +	- Reference voltage in millivolts for the conversions.
> +	- The range of value is 900 to 2700 mv.
> +
> +Example:
> +&adc0 {
> +	ref_voltage = <2500>;
> +};
> \ No newline at end of file

Fix this.

> -- 
> 2.17.1
>
Jonathan Cameron Oct. 18, 2020, 10:55 a.m. UTC | #2
On Tue, 13 Oct 2020 18:32:45 +0800
Billy Tsai <billy_tsai@aspeedtech.com> wrote:

> At ast2600 ref_voltage becomes configurable and this property is board
> dependency.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

dt-bindings: etc for the patch title.  Look at naming of similar patches
for a reference.


> ---
>  .../devicetree/bindings/iio/adc/aspeed_adc.txt   | 16 +++++++++++++++-

This has been converted to yaml. 
It is now in Linus' tree (though only very recently!)

You will need to add the relevant logic to make the yaml binding express that this
only exists and is required for ast600

The binding is making me wonder a bit on what this voltage actually is...

I 'think' we have a setup where the reference voltage is either picked
from 2 available internal references or uses an external regulator (presumably
provided to a pin on the chip?)

As such, the binding should be an optional regulator.  If the regulator is present
we use that, even if it matches the internal regulator votlage.  Someone put it
down on the board, so presumably  they want to use it.

If it's not present, then have to provide either of the two internal regulator
voltages via a devicetree binding such as you have here.



>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> index 034fc2ba100e..0ba1980c4e06 100644
> --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> @@ -3,8 +3,11 @@ Aspeed ADC
>  This device is a 10-bit converter for 16 voltage channels.  All inputs are
>  single ended.
>  
> +At ast2600, this device split into two individual IPs and each contains 8 voltage channels.
> +
> +Chip level dtsi:
>  Required properties:
> -- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
> +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" or "aspeed,ast2600-adc"
>  - reg: memory window mapping address and length
>  - clocks: Input clock used to derive the sample clock. Expected to be the
>            SoC's APB clock.
> @@ -20,3 +23,14 @@ Example:
>  		resets = <&syscon ASPEED_RESET_ADC>;
>  		#io-channel-cells = <1>;
>  	};
> +
> +Board level dts:
> +Required properties:
> +- ref_voltage: (ast2600 only)
> +	- Reference voltage in millivolts for the conversions.
> +	- The range of value is 900 to 2700 mv.

See above for suggestion on on this. For the internal case, will need
a name that expresses both what it is and what the units are.

int_ref_voltage_mv perhaps?

Yaml binding should check that either we have a regulator, or
we have this element.  That way we'll ensure we don't have any confusion
over which one to use.

Thanks,

Jonathan

> +
> +Example:
> +&adc0 {
> +	ref_voltage = <2500>;
> +};
> \ No newline at end of file
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
index 034fc2ba100e..0ba1980c4e06 100644
--- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
@@ -3,8 +3,11 @@  Aspeed ADC
 This device is a 10-bit converter for 16 voltage channels.  All inputs are
 single ended.
 
+At ast2600, this device split into two individual IPs and each contains 8 voltage channels.
+
+Chip level dtsi:
 Required properties:
-- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
+- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" or "aspeed,ast2600-adc"
 - reg: memory window mapping address and length
 - clocks: Input clock used to derive the sample clock. Expected to be the
           SoC's APB clock.
@@ -20,3 +23,14 @@  Example:
 		resets = <&syscon ASPEED_RESET_ADC>;
 		#io-channel-cells = <1>;
 	};
+
+Board level dts:
+Required properties:
+- ref_voltage: (ast2600 only)
+	- Reference voltage in millivolts for the conversions.
+	- The range of value is 900 to 2700 mv.
+
+Example:
+&adc0 {
+	ref_voltage = <2500>;
+};
\ No newline at end of file