diff mbox

[v2,2/2] dt-bindings: iio: dac: Add docs for AD5758 DAC

Message ID 1528377065-647-1-git-send-email-stefan.popa@analog.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stefan Popa June 7, 2018, 1:11 p.m. UTC 
Signed-off-by: Stefan Popa <stefan.popa@analog.com>
---
Changes in v2:
	- Nothing changed, just to follow the patch set version.

 .../devicetree/bindings/iio/dac/ad5758.txt         | 84 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/ad5758.txt

Comments

Jonathan Cameron June 10, 2018, 1:59 p.m. UTC | #1 
On Thu, 7 Jun 2018 16:11:05 +0300
Stefan Popa <stefan.popa@analog.com> wrote:

> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Hmm. The first part number I've ever googled where the top hit has
been a flight number. 
Rio de Janeiro to Campinas is apparently on time today.

> ---
> Changes in v2:
> 	- Nothing changed, just to follow the patch set version.
> 
>  .../devicetree/bindings/iio/dac/ad5758.txt         | 84 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/ad5758.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/ad5758.txt b/Documentation/devicetree/bindings/iio/dac/ad5758.txt
> new file mode 100644
> index 0000000..75fff6c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/ad5758.txt
> @@ -0,0 +1,84 @@
> +Analog Devices AD5758 DAC device driver
> +
> +Required properties for the AD5758:
> +	- compatible: Must be "adi,ad5758"
> +	- reg: SPI chip select number for the device
> +	- spi-max-frequency: Max SPI frequency to use (< 50000000)
> +	- spi-cpha: is the only mode that is supported
> +
> +Optional properties:
> +
> + - adi,dc-dc-mode: Mode of operation of the dc-to-dc converter
> +		   The following values are currently supported:
> +		   	* 0: DC-to-DC converter powered off
> +			* 1: DPC current mode
Perhaps expand those acronyms?
Dynamic Power Control  and perhaps a bit of explanation so people
who are ready DT files don't have to go find the data sheet to have
some idea of what these are?

Otherwise, binding is fine.

> +			* 2: DPC voltage mode
> +			* 3: PPC current mode
> +
> + - adi,dc-dc-ilim: The dc-to-dc converter current limit
> +		   The following values are currently supported [mA]:
Unit should be in the name.
adi,dc-dc-ilim_ma
> +		   	* 150
> +			* 200
> +			* 250
> +			* 300
> +			* 350
> +			* 400
> +
> + - adi,slew: Array of slewrate settings should contain 3 fields:
> +	1: Should be either 0 or 1 in order to enable or disable slewrate.

If it is disabled, why not just detect that as the 'not here' version
and then you can drop this element?

> +	2: Slew rate clock:
> +		Valid values for the slew rate update frequency [Hz]:
> +		* 240000
> +		* 200000
> +		* 150000
> +		* 128000
> +		* 64000
> +		* 32000
> +		* 16000
> +		* 8000
> +		* 4000
> +		* 2000
> +		* 1000
> +		* 512
> +		* 256
> +		* 128
> +		* 64
> +		* 16
> +	3: Slew rate step:
> +		Defines by how much the output value changes at each update.
> +		Valid values for the step size LSBs:
> +		* 4
> +		* 12
> +		* 64
> +		* 120
> +		* 256
> +		* 500
> +		* 1820
> +		* 2048

I think these last two are kind of separate parameters, would be interested
to hear if Rob thinks this should be separate or not.  They are both
related to slew rate, but different elements that add up to it.

> +
> + - adi,range: The output range
> +	      The following values are currently supported:
> +	      	* 0: 0 V to 5 V voltage range
> +		* 1: 0 V to 10 V voltage range
> +		* 2: ±5 V voltage range
> +		* 3: ±10 V voltage range
> +		* 8: 0 mA to 20 mA current range
> +		* 9: 0 mA to 24 mA current range
> +		* 10: 4 mA to 20 mA current range
> +		* 11: ±20 mA current range
> +		* 12: ±24 mA current range
> +		* 13: −1 mA to +22 mA current range

Not sure if we can't represent this better.  It's either
a current or voltage output device.

adi,voltage_output or something like that or use

adi,voltage_range_v = <0 5>
adi,current_range_ma = <-1 22> etc
with it only being valid to specify one or the other.

I much prefer DT elements that don't require looking
at the docs where possible.

Again, Rob's input on this would be good.

> +
> +AD5758 Example:
> +
> +	ad5758@0 {
> +		compatible = "adi,ad5758";
> +		reg = <0>;
> +		spi-max-frequency = <1000000>;
> +		spi-cpha;
> +
> +		adi,dc-dc-mode = <2>;
> +		adi,dc-dc-ilim = <200>;
> +		adi,slew = <1 200000 12>;
> +		adi,range = <1>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1993779..f640146 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -808,6 +808,7 @@ L:	linux-iio@vger.kernel.org
>  W:	http://ez.analog.com/community/linux-device-drivers
>  S:	Supported
>  F:	drivers/iio/dac/ad5758.c
> +F:	Documentation/devicetree/bindings/iio/dac/ad5758.txt
>  
>  ANALOG DEVICES INC AD9389B DRIVER
>  M:	Hans Verkuil <hans.verkuil@cisco.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 11, 2018, 8:42 a.m. UTC | #2 
On Thu, Jun 7, 2018 at 3:11 PM, Stefan Popa <stefan.popa@analog.com> wrote:

> + - adi,slew: Array of slewrate settings should contain 3 fields:
> +       1: Should be either 0 or 1 in order to enable or disable slewrate.

We have a standard binding in pin control for "slew-rate".

It doesn't have a unit today though.

> +       2: Slew rate clock:
> +               Valid values for the slew rate update frequency [Hz]:
> +               * 240000
> +               * 200000
> +               * 150000
> +               * 128000
> +               * 64000
> +               * 32000
> +               * 16000
> +               * 8000
> +               * 4000
> +               * 2000
> +               * 1000
> +               * 512
> +               * 256
> +               * 128
> +               * 64
> +               * 16
> +       3: Slew rate step:
> +               Defines by how much the output value changes at each update.
> +               Valid values for the step size LSBs:
> +               * 4
> +               * 12
> +               * 64
> +               * 120
> +               * 256
> +               * 500
> +               * 1820
> +               * 2048

This is pretty idiomatic and just encodes the configuration fields from the
hardware into the device tree.

The normal slew rate unit is Volts per microsecond.

I get the feeling that we could define that simply, and then calculate
the slew rate clock and step using that to find the best match for
a certain slew rate.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) June 12, 2018, 6:21 p.m. UTC | #3 
On Sun, Jun 10, 2018 at 02:59:23PM +0100, Jonathan Cameron wrote:
> On Thu, 7 Jun 2018 16:11:05 +0300
> Stefan Popa <stefan.popa@analog.com> wrote:
> 
> > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Hmm. The first part number I've ever googled where the top hit has
> been a flight number. 
> Rio de Janeiro to Campinas is apparently on time today.
> 
> > ---
> > Changes in v2:
> > 	- Nothing changed, just to follow the patch set version.
> > 
> >  .../devicetree/bindings/iio/dac/ad5758.txt         | 84 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  2 files changed, 85 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/dac/ad5758.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/dac/ad5758.txt b/Documentation/devicetree/bindings/iio/dac/ad5758.txt
> > new file mode 100644
> > index 0000000..75fff6c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/dac/ad5758.txt
> > @@ -0,0 +1,84 @@
> > +Analog Devices AD5758 DAC device driver
> > +
> > +Required properties for the AD5758:
> > +	- compatible: Must be "adi,ad5758"
> > +	- reg: SPI chip select number for the device
> > +	- spi-max-frequency: Max SPI frequency to use (< 50000000)
> > +	- spi-cpha: is the only mode that is supported
> > +
> > +Optional properties:
> > +
> > + - adi,dc-dc-mode: Mode of operation of the dc-to-dc converter
> > +		   The following values are currently supported:
> > +		   	* 0: DC-to-DC converter powered off
> > +			* 1: DPC current mode
> Perhaps expand those acronyms?
> Dynamic Power Control  and perhaps a bit of explanation so people
> who are ready DT files don't have to go find the data sheet to have
> some idea of what these are?
> 
> Otherwise, binding is fine.
> 
> > +			* 2: DPC voltage mode
> > +			* 3: PPC current mode
> > +
> > + - adi,dc-dc-ilim: The dc-to-dc converter current limit
> > +		   The following values are currently supported [mA]:
> Unit should be in the name.
> adi,dc-dc-ilim_ma

Yes, but don't use '_' and follow what is in property-units.txt.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Cameron June 16, 2018, 5:49 p.m. UTC | #4 
On Mon, 11 Jun 2018 10:42:35 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Jun 7, 2018 at 3:11 PM, Stefan Popa <stefan.popa@analog.com> wrote:
> 
> > + - adi,slew: Array of slewrate settings should contain 3 fields:
> > +       1: Should be either 0 or 1 in order to enable or disable slewrate.  
> 
> We have a standard binding in pin control for "slew-rate".
> 
> It doesn't have a unit today though.
> 
> > +       2: Slew rate clock:
> > +               Valid values for the slew rate update frequency [Hz]:
> > +               * 240000
> > +               * 200000
> > +               * 150000
> > +               * 128000
> > +               * 64000
> > +               * 32000
> > +               * 16000
> > +               * 8000
> > +               * 4000
> > +               * 2000
> > +               * 1000
> > +               * 512
> > +               * 256
> > +               * 128
> > +               * 64
> > +               * 16
> > +       3: Slew rate step:
> > +               Defines by how much the output value changes at each update.
> > +               Valid values for the step size LSBs:
> > +               * 4
> > +               * 12
> > +               * 64
> > +               * 120
> > +               * 256
> > +               * 500
> > +               * 1820
> > +               * 2048  
> 
> This is pretty idiomatic and just encodes the configuration fields from the
> hardware into the device tree.
> 
> The normal slew rate unit is Volts per microsecond.
> 
> I get the feeling that we could define that simply, and then calculate
> the slew rate clock and step using that to find the best match for
> a certain slew rate.
> 
> Yours,
> Linus Walleij
Agreed. A single value would be nice if possible.

J

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/iio/dac/ad5758.txt b/Documentation/devicetree/bindings/iio/dac/ad5758.txt
new file mode 100644
index 0000000..75fff6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/ad5758.txt
@@ -0,0 +1,84 @@ 
+Analog Devices AD5758 DAC device driver
+
+Required properties for the AD5758:
+	- compatible: Must be "adi,ad5758"
+	- reg: SPI chip select number for the device
+	- spi-max-frequency: Max SPI frequency to use (< 50000000)
+	- spi-cpha: is the only mode that is supported
+
+Optional properties:
+
+ - adi,dc-dc-mode: Mode of operation of the dc-to-dc converter
+		   The following values are currently supported:
+		   	* 0: DC-to-DC converter powered off
+			* 1: DPC current mode
+			* 2: DPC voltage mode
+			* 3: PPC current mode
+
+ - adi,dc-dc-ilim: The dc-to-dc converter current limit
+		   The following values are currently supported [mA]:
+		   	* 150
+			* 200
+			* 250
+			* 300
+			* 350
+			* 400
+
+ - adi,slew: Array of slewrate settings should contain 3 fields:
+	1: Should be either 0 or 1 in order to enable or disable slewrate.
+	2: Slew rate clock:
+		Valid values for the slew rate update frequency [Hz]:
+		* 240000
+		* 200000
+		* 150000
+		* 128000
+		* 64000
+		* 32000
+		* 16000
+		* 8000
+		* 4000
+		* 2000
+		* 1000
+		* 512
+		* 256
+		* 128
+		* 64
+		* 16
+	3: Slew rate step:
+		Defines by how much the output value changes at each update.
+		Valid values for the step size LSBs:
+		* 4
+		* 12
+		* 64
+		* 120
+		* 256
+		* 500
+		* 1820
+		* 2048
+
+ - adi,range: The output range
+	      The following values are currently supported:
+	      	* 0: 0 V to 5 V voltage range
+		* 1: 0 V to 10 V voltage range
+		* 2: ±5 V voltage range
+		* 3: ±10 V voltage range
+		* 8: 0 mA to 20 mA current range
+		* 9: 0 mA to 24 mA current range
+		* 10: 4 mA to 20 mA current range
+		* 11: ±20 mA current range
+		* 12: ±24 mA current range
+		* 13: −1 mA to +22 mA current range
+
+AD5758 Example:
+
+	ad5758@0 {
+		compatible = "adi,ad5758";
+		reg = <0>;
+		spi-max-frequency = <1000000>;
+		spi-cpha;
+
+		adi,dc-dc-mode = <2>;
+		adi,dc-dc-ilim = <200>;
+		adi,slew = <1 200000 12>;
+		adi,range = <1>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 1993779..f640146 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -808,6 +808,7 @@  L:	linux-iio@vger.kernel.org
 W:	http://ez.analog.com/community/linux-device-drivers
 S:	Supported
 F:	drivers/iio/dac/ad5758.c
+F:	Documentation/devicetree/bindings/iio/dac/ad5758.txt
 
 ANALOG DEVICES INC AD9389B DRIVER
 M:	Hans Verkuil <hans.verkuil@cisco.com>