[PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Peter Rosin]

Hi!

These two drivers share the fact that they wrap another iio channel,
and I use the first in combination with the second, which is why I'm
submitting them as a pair.

The first driver is a simple wrapper converting an iio dpot into an
iio dac. It only changes the unit and scale. It also does not add any
fancy iio buffer support that I don't need. I suppose that can be
added. By someone else :-)

Please look over the scale conversion, notably for the fractional log2
case that I don't need myself, so is untested. Maybe I should just
remove it?

Also, is there some agreed-upon way to dig out the maximum value from
an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from the
dt bindings, which would have been nice...

I'm also wondering if I'm somehow abusing the regulator? I only added
it to get rid of a "dpot-dac,max-voltage" thing from the dt bindings.
It feels right though, but maybe I should do more with it than check
its voltage? What?

The second driver (the envelope detector) is more involved. It also
explains why I need the dpot-dac driver. I wanted the envelope
detector to be generic and work with any dac, but I had a dpot...

The envelope detector was previously discussed late last year [1],
and this is what I came up with instead of that mess.

There are a couple of things to be said about the envelope detector,
one question is where it should live? I placed it in the adc directory,
but maybe it deserves an iio directory of its own? I'm also a bit
worried that the name is a wee bit too generic. But what is a good
name? I don't want it to be too long like dac-comp-envelope-detector
and something like dac-comp-env-det is just unreadable. Naming is
difficult... And suggestions?

Another thing is that I'm not 100% satisfied with the fact that you
have to decide at instantiation if you are going to invert the search
or not (search from below). But in order for that to be selectable
at runtime with a channel attribute of some sort, I need to be able
to rebind the interrupt to the other edge and I want to do that
without releasing the irq and grabbing it again (someone might
otherwise steal the irq, making the driver lose the irq all together).
I don't see any API to change the irq trigger condition. Is there
such a thing?

Anyway, despite all the above questions and remarks, this works for
me. Please consider applying.

Cheers,
Peter

[1] http://www.gossamer-threads.com/lists/linux/kernel/2320755

Peter Rosin (4):
  dt-bindings: iio: document dpot-dac bindings
  iio: dpot-dac: DAC driver based on a digital potentiometer
  dt-bindings: iio: document envelope-detector bindings
  iio: envelope-detector: ADC driver based on a DAC and a comparator

 .../bindings/iio/adc/envelope-detector.txt         |  65 +++++
 .../devicetree/bindings/iio/dac/dpot-dac.txt       |  43 +++
 MAINTAINERS                                        |  14 +
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/envelope-detector.c                | 305 +++++++++++++++++++++
 drivers/iio/dac/Kconfig                            |  10 +
 drivers/iio/dac/Makefile                           |   1 +
 drivers/iio/dac/dpot-dac.c                         | 219 +++++++++++++++
 9 files changed, 668 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
 create mode 100644 Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
 create mode 100644 drivers/iio/adc/envelope-detector.c
 create mode 100644 drivers/iio/dac/dpot-dac.c

-- 
2.1.4

[PATCH 1/4] dt-bindings: iio: document dpot-dac bindings

[Peter Rosin]

Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
---
 .../devicetree/bindings/iio/dac/dpot-dac.txt       | 43 ++++++++++++++++++++++
 MAINTAINERS                                        |  6 +++
 2 files changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/dpot-dac.txt

diff --git a/Documentation/devicetree/bindings/iio/dac/dpot-dac.txt b/Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
new file mode 100644
index 000000000000..a381188676c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
@@ -0,0 +1,43 @@
+Bindings for DAC emulation using a digital potentiometer
+
+It is assumed the that the dpot is used as a voltage divider between the
+current dpot wiper setting and the maximum resistance of the dpot. The
+divided voltage is provided by a vref regulator.
+
+                  .------.
+   .-----------.  |      |
+   | Vref      |--'    .---.
+   | regulator |--.    |   |
+   '-----------'  |    | d |
+                  |    | p |
+                  |    | o |  wiper
+                  |    | t |<---------+
+                  |    |   |
+                  |    '---'       dac output voltage
+                  |      |
+                  '------+------------+
+
+Required properties:
+- compatible: Should be "dpot-dac"
+- vref-supply: The regulator supplying the voltage divider.
+- io-channels: Channel node of the dpot to be used for the voltage division.
+- io-channel-names: Should be "dpot".
+- dpot-dac,max-ohms: The maximum resistance of the dpot.
+
+Example:
+
+	&i2c {
+		dpot: mcp4651-503@28 {
+			compatible = "microchip,mcp4651-503";
+			reg = <0x28>;
+			#io-channel-cells = <1>;
+		};
+	};
+
+	dac {
+		compatible = "dpot-dac";
+		vref-supply = <&reg_3v3>;
+		io-channels = <&dpot 0>;
+		io-channel-names = "dpot";
+		dpot-dac,max-ohms = <50000>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 1cd38a7e0064..c68b72088945 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6111,6 +6111,12 @@ L:	linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
 S:	Maintained
 F:	drivers/media/rc/iguanair.c
 
+IIO DIGITAL POTENTIOMETER DAC
+M:	Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
+L:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
+
 IIO SUBSYSTEM AND DRIVERS
 M:	Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
 R:	Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] dt-bindings: iio: document dpot-dac bindings

[Jonathan Cameron]

On 20/10/16 10:25, Peter Rosin wrote:
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> ---
I like this, but will need some device tree maintainer input on it.

To my mind we are explicitly describing hardware connectivity here so it should
be fine.  No where near as controversial as a certain other IIO binding ;)


>  .../devicetree/bindings/iio/dac/dpot-dac.txt       | 43 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 +++
>  2 files changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/dpot-dac.txt b/Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
> new file mode 100644
> index 000000000000..a381188676c3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
> @@ -0,0 +1,43 @@
> +Bindings for DAC emulation using a digital potentiometer
> +
> +It is assumed the that the dpot is used as a voltage divider between the
> +current dpot wiper setting and the maximum resistance of the dpot. The
> +divided voltage is provided by a vref regulator.
> +
> +                  .------.
> +   .-----------.  |      |
> +   | Vref      |--'    .---.
> +   | regulator |--.    |   |
> +   '-----------'  |    | d |
> +                  |    | p |
> +                  |    | o |  wiper
> +                  |    | t |<---------+
> +                  |    |   |
> +                  |    '---'       dac output voltage
> +                  |      |
> +                  '------+------------+
> +
> +Required properties:
> +- compatible: Should be "dpot-dac"
> +- vref-supply: The regulator supplying the voltage divider.
> +- io-channels: Channel node of the dpot to be used for the voltage division.
> +- io-channel-names: Should be "dpot".
> +- dpot-dac,max-ohms: The maximum resistance of the dpot.
> +
> +Example:
> +
> +	&i2c {
> +		dpot: mcp4651-503@28 {
> +			compatible = "microchip,mcp4651-503";
> +			reg = <0x28>;
> +			#io-channel-cells = <1>;
> +		};
> +	};
> +
> +	dac {
> +		compatible = "dpot-dac";
> +		vref-supply = <&reg_3v3>;
> +		io-channels = <&dpot 0>;
> +		io-channel-names = "dpot";
> +		dpot-dac,max-ohms = <50000>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1cd38a7e0064..c68b72088945 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6111,6 +6111,12 @@ L:	linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>  S:	Maintained
>  F:	drivers/media/rc/iguanair.c
>  
> +IIO DIGITAL POTENTIOMETER DAC
> +M:	Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> +L:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
> +
>  IIO SUBSYSTEM AND DRIVERS
>  M:	Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>  R:	Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] dt-bindings: iio: document envelope-detector bindings

[Peter Rosin]

Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
---
 .../bindings/iio/adc/envelope-detector.txt         | 65 ++++++++++++++++++++++
 MAINTAINERS                                        |  6 ++
 2 files changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/envelope-detector.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/envelope-detector.txt b/Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
new file mode 100644
index 000000000000..0e26299516fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
@@ -0,0 +1,65 @@
+Bindings for ADC envelope detector using a DAC and a comparator
+
+The DAC is used to find the peak level of an alternating voltage input
+signal by a binary search using the output of a comparator wired to
+an interrupt pin. Like so:
+                          _
+                         | \
+    input +------>-------|+ \
+                         |   \
+           .-------.     |    }---.
+           |       |     |   /    |
+           |    dac|-->--|- /     |
+           |       |     |_/      |
+           |       |              |
+           |       |              |
+           |    irq|------<-------'
+           |       |
+           '-------'
+
+Required properties:
+- compatible: Should be "envelope-detector"
+- io-channels: Channel node of the dac to be used for comparator input.
+- io-channel-names: Should be "dac".
+- interrupt specification for one client interrupt,
+  see ../../interrupt-controller/interrupts.txt for details.
+- interrupt-names: Should be "comp".
+- envelope-detector,dac-max: The maximum raw input value for the dac.
+- envelope-detector,comp-interval-ms: How long to wait for the comparator
+  to trigger before moving on to another DAC level. This interval needs to
+  relate to the lowest possible frequency of the above input signal.
+
+Optional properties:
+- envelope-detector,inverted: If the input signal is centered around the
+  dac-max voltage (instead of zero), this property causes the detector to
+  search for the lowest DAC value that does not triggers the comparator
+  (instead of the highest). The result is also inverted so that a lower DAC
+  reading yields a higher voltage value.
+
+
+Example:
+
+	&spi {
+		dac: ad7303@4 {
+			compatible = "adi,ad7303";
+			reg = <4>;
+			spi-max-frequency = <10000000>;
+			Vdd-supply = <&vdd_supply>;
+			adi,use-external-reference;
+			REF-supply = <&vref_supply>;
+			#io-channel-cells = <1>;
+		};
+	};
+
+	envelope-detector {
+		compatible = "envelope-detector";
+		io-channels = <&dac 0>;
+		io-channel-names = "dac";
+
+		interrupt-parent = <&gpio>;
+		interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
+		interrupt-names = "comp";
+
+		envelope-detector,dac-max = <255>;
+		envelope-detector,comp-interval-ms = <50>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 8c8aae24b96b..4b6f6ec1b703 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6118,6 +6118,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
 F:	drivers/iio/dac/dpot-dac.c
 
+IIO ENVELOPE DETECTOR
+M:	Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
+L:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
+
 IIO SUBSYSTEM AND DRIVERS
 M:	Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
 R:	Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>
-- 
2.1.4

Re: [PATCH 3/4] dt-bindings: iio: document envelope-detector bindings

[Peter Meerwald-Stadler]

On Thu, 20 Oct 2016, Peter Rosin wrote:

nitpicking below

> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> ---
>  .../bindings/iio/adc/envelope-detector.txt         | 65 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 ++
>  2 files changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/envelope-detector.txt b/Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> new file mode 100644
> index 000000000000..0e26299516fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> @@ -0,0 +1,65 @@
> +Bindings for ADC envelope detector using a DAC and a comparator
> +
> +The DAC is used to find the peak level of an alternating voltage input
> +signal by a binary search using the output of a comparator wired to
> +an interrupt pin. Like so:
> +                          _
> +                         | \
> +    input +------>-------|+ \
> +                         |   \
> +           .-------.     |    }---.
> +           |       |     |   /    |
> +           |    dac|-->--|- /     |
> +           |       |     |_/      |
> +           |       |              |
> +           |       |              |
> +           |    irq|------<-------'
> +           |       |
> +           '-------'
> +
> +Required properties:
> +- compatible: Should be "envelope-detector"
> +- io-channels: Channel node of the dac to be used for comparator input.
> +- io-channel-names: Should be "dac".
> +- interrupt specification for one client interrupt,
> +  see ../../interrupt-controller/interrupts.txt for details.
> +- interrupt-names: Should be "comp".
> +- envelope-detector,dac-max: The maximum raw input value for the dac.
> +- envelope-detector,comp-interval-ms: How long to wait for the comparator
> +  to trigger before moving on to another DAC level. This interval needs to
> +  relate to the lowest possible frequency of the above input signal.
> +
> +Optional properties:
> +- envelope-detector,inverted: If the input signal is centered around the
> +  dac-max voltage (instead of zero), this property causes the detector to
> +  search for the lowest DAC value that does not triggers the comparator

does not trigger

> +  (instead of the highest). The result is also inverted so that a lower DAC
> +  reading yields a higher voltage value.
> +
> +
> +Example:
> +
> +	&spi {
> +		dac: ad7303@4 {
> +			compatible = "adi,ad7303";
> +			reg = <4>;
> +			spi-max-frequency = <10000000>;
> +			Vdd-supply = <&vdd_supply>;
> +			adi,use-external-reference;
> +			REF-supply = <&vref_supply>;
> +			#io-channel-cells = <1>;
> +		};
> +	};
> +
> +	envelope-detector {
> +		compatible = "envelope-detector";
> +		io-channels = <&dac 0>;
> +		io-channel-names = "dac";
> +
> +		interrupt-parent = <&gpio>;
> +		interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
> +		interrupt-names = "comp";
> +
> +		envelope-detector,dac-max = <255>;
> +		envelope-detector,comp-interval-ms = <50>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8c8aae24b96b..4b6f6ec1b703 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6118,6 +6118,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
>  F:	drivers/iio/dac/dpot-dac.c
>  
> +IIO ENVELOPE DETECTOR
> +M:	Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> +L:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> +
>  IIO SUBSYSTEM AND DRIVERS
>  M:	Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>  R:	Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 3/4] dt-bindings: iio: document envelope-detector bindings

[Jonathan Cameron]

On 20/10/16 10:26, Peter Rosin wrote:
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Definitely going to need device tree maintainer input!  (though as Lars
and you discussed some mods to this in the cover letter discussion,
perhaps they should wait for V2).
> ---
>  .../bindings/iio/adc/envelope-detector.txt         | 65 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 ++
>  2 files changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/envelope-detector.txt b/Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> new file mode 100644
> index 000000000000..0e26299516fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> @@ -0,0 +1,65 @@
> +Bindings for ADC envelope detector using a DAC and a comparator
> +
> +The DAC is used to find the peak level of an alternating voltage input
> +signal by a binary search using the output of a comparator wired to
> +an interrupt pin. Like so:
> +                          _
> +                         | \
> +    input +------>-------|+ \
> +                         |   \
> +           .-------.     |    }---.
> +           |       |     |   /    |
> +           |    dac|-->--|- /     |
> +           |       |     |_/      |
> +           |       |              |
> +           |       |              |
> +           |    irq|------<-------'
> +           |       |
> +           '-------'
> +
> +Required properties:
> +- compatible: Should be "envelope-detector"
Agreed with the discussion in the cover letter responses.  Needs
to be more specific as lots of ways of skinning this cat.
Hehe, just occurred to me that your Maintainer entry below
makes you responsible for all of them.. Acked!

> +- io-channels: Channel node of the dac to be used for comparator input.
> +- io-channel-names: Should be "dac".
> +- interrupt specification for one client interrupt,
> +  see ../../interrupt-controller/interrupts.txt for details.
> +- interrupt-names: Should be "comp".
> +- envelope-detector,dac-max: The maximum raw input value for the dac.
I guess the available stuff you have brought up to date should deal with this
one without needing another param.

> +- envelope-detector,comp-interval-ms: How long to wait for the comparator
> +  to trigger before moving on to another DAC level. This interval needs to
> +  relate to the lowest possible frequency of the above input signal.
This is an odd one and definitely a case for the device tree guys.

It's not generic enough to not have a 'manufacturer' in the param name,
but there isn't really a manufacturer as anyone with a few chips can build
one of these (using the 'wiring' diagram above ;)

So what should the prefix be?

> +
> +Optional properties:
> +- envelope-detector,inverted: If the input signal is centered around the
> +  dac-max voltage (instead of zero), this property causes the detector to
> +  search for the lowest DAC value that does not triggers the comparator
> +  (instead of the highest). The result is also inverted so that a lower DAC
> +  reading yields a higher voltage value.
As discussed, this is fundamental enough that it should probably
be based on the compatible string rather than a separate item.
> +
> +
> +Example:
> +
> +	&spi {
> +		dac: ad7303@4 {
> +			compatible = "adi,ad7303";
> +			reg = <4>;
> +			spi-max-frequency = <10000000>;
> +			Vdd-supply = <&vdd_supply>;
> +			adi,use-external-reference;
> +			REF-supply = <&vref_supply>;
> +			#io-channel-cells = <1>;
> +		};
> +	};
> +
> +	envelope-detector {
> +		compatible = "envelope-detector";
> +		io-channels = <&dac 0>;
> +		io-channel-names = "dac";
> +
> +		interrupt-parent = <&gpio>;
> +		interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
> +		interrupt-names = "comp";
> +
> +		envelope-detector,dac-max = <255>;
> +		envelope-detector,comp-interval-ms = <50>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8c8aae24b96b..4b6f6ec1b703 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6118,6 +6118,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
>  F:	drivers/iio/dac/dpot-dac.c
>  
> +IIO ENVELOPE DETECTOR
> +M:	Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> +L:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> +
>  IIO SUBSYSTEM AND DRIVERS
>  M:	Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>  R:	Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>
> 

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Lars-Peter Clausen]

On 10/20/2016 11:25 AM, Peter Rosin wrote:
> Hi!
> 
> These two drivers share the fact that they wrap another iio channel,
> and I use the first in combination with the second, which is why I'm
> submitting them as a pair.
> 
> The first driver is a simple wrapper converting an iio dpot into an
> iio dac. It only changes the unit and scale. It also does not add any
> fancy iio buffer support that I don't need. I suppose that can be
> added. By someone else :-)
> 
> Please look over the scale conversion, notably for the fractional log2
> case that I don't need myself, so is untested. Maybe I should just
> remove it?
> 
> Also, is there some agreed-upon way to dig out the maximum value from
> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from the
> dt bindings, which would have been nice...

Yes, this is something we could really use. In a sense it exists for the
devices with buffer-capable channels where there is the real_bits field
which tells us the data width of the channel. But a dedicated mechanism for
querying the maximum (and minimum) valid code seems like a useful feature.
Not only for in-kernel clients, but also for userspace.

> 
> I'm also wondering if I'm somehow abusing the regulator? I only added
> it to get rid of a "dpot-dac,max-voltage" thing from the dt bindings.
> It feels right though, but maybe I should do more with it than check
> its voltage? What?

Enable the regulator when it is in use?

> 
> The second driver (the envelope detector) is more involved. It also
> explains why I need the dpot-dac driver. I wanted the envelope
> detector to be generic and work with any dac, but I had a dpot...
> 
> The envelope detector was previously discussed late last year [1],
> and this is what I came up with instead of that mess.
> 
> There are a couple of things to be said about the envelope detector,
> one question is where it should live? I placed it in the adc directory,
> but maybe it deserves an iio directory of its own? I'm also a bit
> worried that the name is a wee bit too generic. But what is a good
> name? I don't want it to be too long like dac-comp-envelope-detector
> and something like dac-comp-env-det is just unreadable. Naming is
> difficult... And suggestions?

Yeah, it is a bit tricky. It is a envelope detector built from discrete
components, but of course there are many more ways to build one. If you have
a codename for your platform you could use this for the DT compatible
string, like 'vendor,foobar-envelope-detector'.

> 
> Another thing is that I'm not 100% satisfied with the fact that you
> have to decide at instantiation if you are going to invert the search
> or not (search from below). But in order for that to be selectable
> at runtime with a channel attribute of some sort, I need to be able
> to rebind the interrupt to the other edge and I want to do that
> without releasing the irq and grabbing it again (someone might
> otherwise steal the irq, making the driver lose the irq all together).
> I don't see any API to change the irq trigger condition. Is there
> such a thing?
> 
> Anyway, despite all the above questions and remarks, this works for
> me. Please consider applying.

In general this series looks really good, good and clear implementation as
well as documentation. A few minor bits here and there, but that is normal.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Peter Rosin]

On 2016-10-20 14:55, Lars-Peter Clausen wrote:
> On 10/20/2016 11:25 AM, Peter Rosin wrote:
>> Also, is there some agreed-upon way to dig out the maximum value from
>> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from the
>> dt bindings, which would have been nice...
> 
> Yes, this is something we could really use. In a sense it exists for the
> devices with buffer-capable channels where there is the real_bits field
> which tells us the data width of the channel. But a dedicated mechanism for
> querying the maximum (and minimum) valid code seems like a useful feature.
> Not only for in-kernel clients, but also for userspace.

For the dpot I have, real_bits (if provided) would not be too great since
the maximum value is 256 (i.e. 257 possible wiper positions). I doesn't
feel like I'm the most qualified person to add these new min/max attributes
though, as I'm not familiar with most parts of the iio code. I'll happily
jump on board if they are somehow magically available, of course :-)

>> I'm also wondering if I'm somehow abusing the regulator? I only added
>> it to get rid of a "dpot-dac,max-voltage" thing from the dt bindings.
>> It feels right though, but maybe I should do more with it than check
>> its voltage? What?
> 
> Enable the regulator when it is in use?

Right, I didn't express myself all that clearly, I do in fact already
enable the regulator in ->probe and disable it in ->remove. Anything
else?

>> There are a couple of things to be said about the envelope detector,
>> one question is where it should live? I placed it in the adc directory,
>> but maybe it deserves an iio directory of its own? I'm also a bit
>> worried that the name is a wee bit too generic. But what is a good
>> name? I don't want it to be too long like dac-comp-envelope-detector
>> and something like dac-comp-env-det is just unreadable. Naming is
>> difficult... And suggestions?
> 
> Yeah, it is a bit tricky. It is a envelope detector built from discrete
> components, but of course there are many more ways to build one. If you have
> a codename for your platform you could use this for the DT compatible
> string, like 'vendor,foobar-envelope-detector'.

Good idea! Then the "envelope-detector,inverted" bool can go, and be
implied by the compatible string. If some way to rebind the irq trigger
is later discovered that can be added as a channel attr without
deprecating any dt bindings stuff. While at it, the other properties
("envelope-detector,dac-max" and "envelope-detector,comp-interval-ms")
could also be implied from the compatible string. Would that be better?
I think so.

But, the compatible string is one thing and the driver name is another.
"axentia,tse850-envelope-detector" doesn't seem like the best of driver
names...

Are there any existing examples of drivers for (generic) things built
with discrete components like this that could perhaps provide guidance?

>> Anyway, despite all the above questions and remarks, this works for
>> me. Please consider applying.
> 
> In general this series looks really good, good and clear implementation as
> well as documentation. A few minor bits here and there, but that is normal.

Thanks, appreciated!

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Lars-Peter Clausen]

On 10/20/2016 04:53 PM, Peter Rosin wrote:
[...]
> Good idea! Then the "envelope-detector,inverted" bool can go, and be
> implied by the compatible string. If some way to rebind the irq trigger
> is later discovered that can be added as a channel attr without
> deprecating any dt bindings stuff. While at it, the other properties
> ("envelope-detector,dac-max" and "envelope-detector,comp-interval-ms")
> could also be implied from the compatible string. Would that be better?
> I think so.
> 
> But, the compatible string is one thing and the driver name is another.
> "axentia,tse850-envelope-detector" doesn't seem like the best of driver
> names...

The driver name is not that important we can still change that later if we
have to, the DT compatible string on the other hand is fixed.

> 
> Are there any existing examples of drivers for (generic) things built
> with discrete components like this that could perhaps provide guidance?

Not that I'm aware of.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Jonathan Cameron]

On 20/10/16 16:29, Lars-Peter Clausen wrote:
> On 10/20/2016 04:53 PM, Peter Rosin wrote:
> [...]
>> Good idea! Then the "envelope-detector,inverted" bool can go, and be
>> implied by the compatible string. If some way to rebind the irq trigger
>> is later discovered that can be added as a channel attr without
>> deprecating any dt bindings stuff. While at it, the other properties
>> ("envelope-detector,dac-max" and "envelope-detector,comp-interval-ms")
>> could also be implied from the compatible string. Would that be better?
>> I think so.
>>
>> But, the compatible string is one thing and the driver name is another.
>> "axentia,tse850-envelope-detector" doesn't seem like the best of driver
>> names...
> 
> The driver name is not that important we can still change that later if we
> have to, the DT compatible string on the other hand is fixed.
> 
>>
>> Are there any existing examples of drivers for (generic) things built
>> with discrete components like this that could perhaps provide guidance?
> 
> Not that I'm aware of.
> 
Me neither.  Always interesting to break new ground ;)

J

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Jonathan Cameron]

On 20/10/16 15:53, Peter Rosin wrote:
> On 2016-10-20 14:55, Lars-Peter Clausen wrote:
>> On 10/20/2016 11:25 AM, Peter Rosin wrote:
>>> Also, is there some agreed-upon way to dig out the maximum value from
>>> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from the
>>> dt bindings, which would have been nice...
>>
>> Yes, this is something we could really use. In a sense it exists for the
>> devices with buffer-capable channels where there is the real_bits field
>> which tells us the data width of the channel. But a dedicated mechanism for
>> querying the maximum (and minimum) valid code seems like a useful feature.
>> Not only for in-kernel clients, but also for userspace.
> 
> For the dpot I have, real_bits (if provided) would not be too great since
> the maximum value is 256 (i.e. 257 possible wiper positions). I doesn't
> feel like I'm the most qualified person to add these new min/max attributes
> though, as I'm not familiar with most parts of the iio code. I'll happily
> jump on board if they are somehow magically available, of course :-)
> 
>>> I'm also wondering if I'm somehow abusing the regulator? I only added
>>> it to get rid of a "dpot-dac,max-voltage" thing from the dt bindings.
>>> It feels right though, but maybe I should do more with it than check
>>> its voltage? What?
>>
>> Enable the regulator when it is in use?
> 
> Right, I didn't express myself all that clearly, I do in fact already
> enable the regulator in ->probe and disable it in ->remove. Anything
> else?
Nope.  This is the same thing we do with ADCs that take a reference voltage.
Sometimes we even query the voltage ever time or handle notifiers that
tell use when it changes... (only example I can immediately think of for
that is the sht15 driver in hwmon - my fault a long time ago ;)
> 
>>> There are a couple of things to be said about the envelope detector,
>>> one question is where it should live? I placed it in the adc directory,
>>> but maybe it deserves an iio directory of its own? I'm also a bit
>>> worried that the name is a wee bit too generic. But what is a good
>>> name? I don't want it to be too long like dac-comp-envelope-detector
>>> and something like dac-comp-env-det is just unreadable. Naming is
>>> difficult... And suggestions?
>>
>> Yeah, it is a bit tricky. It is a envelope detector built from discrete
>> components, but of course there are many more ways to build one. If you have
>> a codename for your platform you could use this for the DT compatible
>> string, like 'vendor,foobar-envelope-detector'.
> 
> Good idea! Then the "envelope-detector,inverted" bool can go, and be
> implied by the compatible string. If some way to rebind the irq trigger
> is later discovered that can be added as a channel attr without
> deprecating any dt bindings stuff. While at it, the other properties
> ("envelope-detector,dac-max" and "envelope-detector,comp-interval-ms")
> could also be implied from the compatible string. Would that be better?
> I think so.
> 
> But, the compatible string is one thing and the driver name is another.
> "axentia,tse850-envelope-detector" doesn't seem like the best of driver
> names...
> 
> Are there any existing examples of drivers for (generic) things built
> with discrete components like this that could perhaps provide guidance?
> 
>>> Anyway, despite all the above questions and remarks, this works for
>>> me. Please consider applying.
>>
>> In general this series looks really good, good and clear implementation as
>> well as documentation. A few minor bits here and there, but that is normal.
> 
> Thanks, appreciated!
> 
> Cheers,
> Peter
> --
> 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
> 

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Jonathan Cameron]

On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 10/20/2016 11:25 AM, Peter Rosin wrote:
>> Hi!
>> 
>> These two drivers share the fact that they wrap another iio channel,
>> and I use the first in combination with the second, which is why I'm
>> submitting them as a pair.
>> 
>> The first driver is a simple wrapper converting an iio dpot into an
>> iio dac. It only changes the unit and scale. It also does not add any
>> fancy iio buffer support that I don't need. I suppose that can be
>> added. By someone else :-)
>> 
>> Please look over the scale conversion, notably for the fractional
>log2
>> case that I don't need myself, so is untested. Maybe I should just
>> remove it?
>> 
>> Also, is there some agreed-upon way to dig out the maximum value from
>> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from the
>> dt bindings, which would have been nice...
>
>Yes, this is something we could really use. In a sense it exists for
>the
>devices with buffer-capable channels where there is the real_bits field
>which tells us the data width of the channel. But a dedicated mechanism
>for
>querying the maximum (and minimum) valid code seems like a useful
>feature.
>Not only for in-kernel clients, but also for userspace.
This was something that was addressed by the rather ancient patch series i posted that added 
an available call back which provided info on range and values for all info mask elements.
Series got buried by there being a lot of precursors but quite a few of those have merged since.

Hmm Google won't let me find it on my phone. Was a while back now. Will try to get on pc with
 decent email archive later and dig out a reference.

>
>> 
>> I'm also wondering if I'm somehow abusing the regulator? I only added
>> it to get rid of a "dpot-dac,max-voltage" thing from the dt bindings.
>> It feels right though, but maybe I should do more with it than check
>> its voltage? What?
>
>Enable the regulator when it is in use?
>
>> 
>> The second driver (the envelope detector) is more involved. It also
>> explains why I need the dpot-dac driver. I wanted the envelope
>> detector to be generic and work with any dac, but I had a dpot...
>> 
>> The envelope detector was previously discussed late last year [1],
>> and this is what I came up with instead of that mess.
>> 
>> There are a couple of things to be said about the envelope detector,
>> one question is where it should live? I placed it in the adc
>directory,
>> but maybe it deserves an iio directory of its own? I'm also a bit
>> worried that the name is a wee bit too generic. But what is a good
>> name? I don't want it to be too long like dac-comp-envelope-detector
>> and something like dac-comp-env-det is just unreadable. Naming is
>> difficult... And suggestions?
>
>Yeah, it is a bit tricky. It is a envelope detector built from discrete
>components, but of course there are many more ways to build one. If you
>have
>a codename for your platform you could use this for the DT compatible
>string, like 'vendor,foobar-envelope-detector'.
>
>> 
>> Another thing is that I'm not 100% satisfied with the fact that you
>> have to decide at instantiation if you are going to invert the search
>> or not (search from below). But in order for that to be selectable
>> at runtime with a channel attribute of some sort, I need to be able
>> to rebind the interrupt to the other edge and I want to do that
>> without releasing the irq and grabbing it again (someone might
>> otherwise steal the irq, making the driver lose the irq all
>together).
>> I don't see any API to change the irq trigger condition. Is there
>> such a thing?
>> 
>> Anyway, despite all the above questions and remarks, this works for
>> me. Please consider applying.
>
>In general this series looks really good, good and clear implementation
>as
>well as documentation. A few minor bits here and there, but that is
>normal.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Jonathan Cameron]

On 20 October 2016 18:30:19 BST, Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
>
>
>On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars@metafoo.de>
>wrote:
>>On 10/20/2016 11:25 AM, Peter Rosin wrote:
>>> Hi!
>>> 
>>> These two drivers share the fact that they wrap another iio channel,
>>> and I use the first in combination with the second, which is why I'm
>>> submitting them as a pair.
>>> 
>>> The first driver is a simple wrapper converting an iio dpot into an
>>> iio dac. It only changes the unit and scale. It also does not add
>any
>>> fancy iio buffer support that I don't need. I suppose that can be
>>> added. By someone else :-)
>>> 
>>> Please look over the scale conversion, notably for the fractional
>>log2
>>> case that I don't need myself, so is untested. Maybe I should just
>>> remove it?
>>> 
>>> Also, is there some agreed-upon way to dig out the maximum value
>from
>>> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from
>the
>>> dt bindings, which would have been nice...
>>
>>Yes, this is something we could really use. In a sense it exists for
>>the
>>devices with buffer-capable channels where there is the real_bits
>field
>>which tells us the data width of the channel. But a dedicated
>mechanism
>>for
>>querying the maximum (and minimum) valid code seems like a useful
>>feature.
>>Not only for in-kernel clients, but also for userspace.
>This was something that was addressed by the rather ancient patch
>series i posted that added 
>an available call back which provided info on range and values for all
>info mask elements.
>Series got buried by there being a lot of precursors but quite a few of
>those have merged since.
>
>Hmm Google won't let me find it on my phone. Was a while back now. Will
>try to get on pc with
> decent email archive later and dig out a reference.
http://marc.info/?l=linux-iio&m=138469765309868&w=2 I think...
>
>>
>>> 
>>> I'm also wondering if I'm somehow abusing the regulator? I only
>added
>>> it to get rid of a "dpot-dac,max-voltage" thing from the dt
>bindings.
>>> It feels right though, but maybe I should do more with it than check
>>> its voltage? What?
>>
>>Enable the regulator when it is in use?
>>
>>> 
>>> The second driver (the envelope detector) is more involved. It also
>>> explains why I need the dpot-dac driver. I wanted the envelope
>>> detector to be generic and work with any dac, but I had a dpot...
>>> 
>>> The envelope detector was previously discussed late last year [1],
>>> and this is what I came up with instead of that mess.
>>> 
>>> There are a couple of things to be said about the envelope detector,
>>> one question is where it should live? I placed it in the adc
>>directory,
>>> but maybe it deserves an iio directory of its own? I'm also a bit
>>> worried that the name is a wee bit too generic. But what is a good
>>> name? I don't want it to be too long like dac-comp-envelope-detector
>>> and something like dac-comp-env-det is just unreadable. Naming is
>>> difficult... And suggestions?
>>
>>Yeah, it is a bit tricky. It is a envelope detector built from
>discrete
>>components, but of course there are many more ways to build one. If
>you
>>have
>>a codename for your platform you could use this for the DT compatible
>>string, like 'vendor,foobar-envelope-detector'.
>>
>>> 
>>> Another thing is that I'm not 100% satisfied with the fact that you
>>> have to decide at instantiation if you are going to invert the
>search
>>> or not (search from below). But in order for that to be selectable
>>> at runtime with a channel attribute of some sort, I need to be able
>>> to rebind the interrupt to the other edge and I want to do that
>>> without releasing the irq and grabbing it again (someone might
>>> otherwise steal the irq, making the driver lose the irq all
>>together).
>>> I don't see any API to change the irq trigger condition. Is there
>>> such a thing?
>>> 
>>> Anyway, despite all the above questions and remarks, this works for
>>> me. Please consider applying.
>>
>>In general this series looks really good, good and clear
>implementation
>>as
>>well as documentation. A few minor bits here and there, but that is
>>normal.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Peter Rosin]

On 2016-10-20 19:37, Jonathan Cameron wrote:
> On 20 October 2016 18:30:19 BST, Jonathan Cameron <jic23-tko9wxEg+fIOOJlXag/Snyp2UmYkHbXO@public.gmane.org> wrote:
>> On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
>>> On 10/20/2016 11:25 AM, Peter Rosin wrote:
>>>> Also, is there some agreed-upon way to dig out the maximum value from
>>>> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from the
>>>> dt bindings, which would have been nice...
>>>
>>> Yes, this is something we could really use. In a sense it exists for
>>> the
>>> devices with buffer-capable channels where there is the real_bits field
>>> which tells us the data width of the channel. But a dedicated mechanism
>>> for
>>> querying the maximum (and minimum) valid code seems like a useful
>>> feature.
>>> Not only for in-kernel clients, but also for userspace.
>>
>> This was something that was addressed by the rather ancient patch
>> series i posted that added 
>> an available call back which provided info on range and values for all
>> info mask elements.
>> Series got buried by there being a lot of precursors but quite a few of
>> those have merged since.
>>
>> Hmm Google won't let me find it on my phone. Was a while back now. Will
>> try to get on pc with
>> decent email archive later and dig out a reference.
> http://marc.info/?l=linux-iio&m=138469765309868&w=2 I think...

Interesting, one issue with that is that it is all in real world
units, while I'd rather have the raw value. So, I would need to
convert back to the raw value using the scale, which sounds boring
but doable. However, I wonder if calibration may also be involved
with that conversion back to raw for some channels? That sounds a
bit more driver specific and potentially troublesome...

Cheers,
Peter

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[jic23]

On 20.10.2016 19:17, Peter Rosin wrote:
> On 2016-10-20 19:37, Jonathan Cameron wrote:
>> On 20 October 2016 18:30:19 BST, Jonathan Cameron 
>> <jic23@jic23.retrosnub.co.uk> wrote:
>>> On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars@metafoo.de> 
>>> wrote:
>>>> On 10/20/2016 11:25 AM, Peter Rosin wrote:
>>>>> Also, is there some agreed-upon way to dig out the maximum value 
>>>>> from
>>>>> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from 
>>>>> the
>>>>> dt bindings, which would have been nice...
>>>> 
>>>> Yes, this is something we could really use. In a sense it exists for
>>>> the
>>>> devices with buffer-capable channels where there is the real_bits 
>>>> field
>>>> which tells us the data width of the channel. But a dedicated 
>>>> mechanism
>>>> for
>>>> querying the maximum (and minimum) valid code seems like a useful
>>>> feature.
>>>> Not only for in-kernel clients, but also for userspace.
>>> 
>>> This was something that was addressed by the rather ancient patch
>>> series i posted that added
>>> an available call back which provided info on range and values for 
>>> all
>>> info mask elements.
>>> Series got buried by there being a lot of precursors but quite a few 
>>> of
>>> those have merged since.
>>> 
>>> Hmm Google won't let me find it on my phone. Was a while back now. 
>>> Will
>>> try to get on pc with
>>> decent email archive later and dig out a reference.
>> http://marc.info/?l=linux-iio&m=138469765309868&w=2 I think...
> 
> Interesting, one issue with that is that it is all in real world
> units, while I'd rather have the raw value.
Um.. It's been a while, but the principle was (IIRC) that every
_available would match the units fo the associated info mask element.
Thus if you have a _raw element it would be in adc counts (most likely).

_input would be in relevant real world units, scale etc in the whatever
units the value itself is in.

> So, I would need to
> convert back to the raw value using the scale, which sounds boring
> but doable. However, I wonder if calibration may also be involved
> with that conversion back to raw for some channels? That sounds a
> bit more driver specific and potentially troublesome...
I've not had a chance to look at your code (only picked up on this as
there was a fair length thread developing), but I wouldn't have thought 
we'd
need to deal with calibrations.   Might need them to move to real world
units from raw but that's always the case anyway (unfortunately).

Jonathan
> 
> Cheers,
> Peter
> 
> --
> 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

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Peter Rosin]

On 2016-10-21 09:17, jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
> On 20.10.2016 19:17, Peter Rosin wrote:
>> On 2016-10-20 19:37, Jonathan Cameron wrote:
>>> http://marc.info/?l=linux-iio&m=138469765309868&w=2 I think...
>>
>> Interesting, one issue with that is that it is all in real world
>> units, while I'd rather have the raw value.
> Um.. It's been a while, but the principle was (IIRC) that every
> _available would match the units fo the associated info mask element.
> Thus if you have a _raw element it would be in adc counts (most likely).
> 
> _input would be in relevant real world units, scale etc in the whatever
> units the value itself is in.

Ah, that was just me totally misreading the patch. I didn't realize
that the iio_format_value function is also used for raw values and
assumed it was all about real world units when that function was
used. Feel rather silly ATM...

>> So, I would need to
>> convert back to the raw value using the scale, which sounds boring
>> but doable. However, I wonder if calibration may also be involved
>> with that conversion back to raw for some channels? That sounds a
>> bit more driver specific and potentially troublesome...
> I've not had a chance to look at your code (only picked up on this as
> there was a fair length thread developing), but I wouldn't have thought 
> we'd
> need to deal with calibrations.   Might need them to move to real world
> units from raw but that's always the case anyway (unfortunately).

Right.

Cheers,
Peter

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Peter Rosin]

On 2016-10-21 09:17, jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
> On 20.10.2016 19:17, Peter Rosin wrote:
>> On 2016-10-20 19:37, Jonathan Cameron wrote:
>>> On 20 October 2016 18:30:19 BST, Jonathan Cameron 
>>> <jic23-tko9wxEg+fIOOJlXag/Snyp2UmYkHbXO@public.gmane.org> wrote:
>>>> On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> 
>>>> wrote:
>>>>> On 10/20/2016 11:25 AM, Peter Rosin wrote:
>>>>>> Also, is there some agreed-upon way to dig out the maximum value 
>>>>>> from
>>>>>> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from 
>>>>>> the
>>>>>> dt bindings, which would have been nice...
>>>>>
>>>>> Yes, this is something we could really use. In a sense it exists for
>>>>> the
>>>>> devices with buffer-capable channels where there is the real_bits 
>>>>> field
>>>>> which tells us the data width of the channel. But a dedicated 
>>>>> mechanism
>>>>> for
>>>>> querying the maximum (and minimum) valid code seems like a useful
>>>>> feature.
>>>>> Not only for in-kernel clients, but also for userspace.
>>>>
>>>> This was something that was addressed by the rather ancient patch
>>>> series i posted that added
>>>> an available call back which provided info on range and values for 
>>>> all info mask elements.
>>>> Series got buried by there being a lot of precursors but quite a few of
>>>> those have merged since.
>>>>
>>>> Hmm Google won't let me find it on my phone. Was a while back now. 
>>>> Will
>>>> try to get on pc with
>>>> decent email archive later and dig out a reference.
>>> http://marc.info/?l=linux-iio&m=138469765309868&w=2 I think...
>>
>> Interesting, one issue with that is that it is all in real world
>> units, while I'd rather have the raw value.
> Um.. It's been a while, but the principle was (IIRC) that every
> _available would match the units fo the associated info mask element.
> Thus if you have a _raw element it would be in adc counts (most likely).
> 
> _input would be in relevant real world units, scale etc in the whatever
> units the value itself is in.

Ok, so I forward ported that patch and added code so that the relevant
channels provide what is available. I also added code to turn the
rest of the parameter style devicetree properties into iio device/channel
attributes. So, it is now much neater from a bindings point of view.

Before I post the updated patches, I'm wondering what the status is
on that ancient patch? It didn't forward port without issues, but there
were no real difficulties that I noticed. Should I just start off my v2
series with that patch? I tend to think that that's the best option,
because I suspect that adding a "max-ohms" devicetree property as a
stop-gap pending some new infrastructure is pretty unrealistic...

Basically, my question is if that ancient patch as any chance of living
at all in a form close to what it is, or if should start looking for
an alternative right away?

Cheers,
Peter

Re: [PATCH 0/4] IIO wrapper drivers, dpot-dac and envelope-detector

[Jonathan Cameron]

On 21/10/16 23:58, Peter Rosin wrote:
> On 2016-10-21 09:17, jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
>> On 20.10.2016 19:17, Peter Rosin wrote:
>>> On 2016-10-20 19:37, Jonathan Cameron wrote:
>>>> On 20 October 2016 18:30:19 BST, Jonathan Cameron 
>>>> <jic23-tko9wxEg+fIOOJlXag/Snyp2UmYkHbXO@public.gmane.org> wrote:
>>>>> On 20 October 2016 13:55:12 BST, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> 
>>>>> wrote:
>>>>>> On 10/20/2016 11:25 AM, Peter Rosin wrote:
>>>>>>> Also, is there some agreed-upon way to dig out the maximum value 
>>>>>>> from
>>>>>>> an iio channel? If so, "dpot-dac,max-ohms" can be eliminated from 
>>>>>>> the
>>>>>>> dt bindings, which would have been nice...
>>>>>>
>>>>>> Yes, this is something we could really use. In a sense it exists for
>>>>>> the
>>>>>> devices with buffer-capable channels where there is the real_bits 
>>>>>> field
>>>>>> which tells us the data width of the channel. But a dedicated 
>>>>>> mechanism
>>>>>> for
>>>>>> querying the maximum (and minimum) valid code seems like a useful
>>>>>> feature.
>>>>>> Not only for in-kernel clients, but also for userspace.
>>>>>
>>>>> This was something that was addressed by the rather ancient patch
>>>>> series i posted that added
>>>>> an available call back which provided info on range and values for 
>>>>> all info mask elements.
>>>>> Series got buried by there being a lot of precursors but quite a few of
>>>>> those have merged since.
>>>>>
>>>>> Hmm Google won't let me find it on my phone. Was a while back now. 
>>>>> Will
>>>>> try to get on pc with
>>>>> decent email archive later and dig out a reference.
>>>> http://marc.info/?l=linux-iio&m=138469765309868&w=2 I think...
>>>
>>> Interesting, one issue with that is that it is all in real world
>>> units, while I'd rather have the raw value.
>> Um.. It's been a while, but the principle was (IIRC) that every
>> _available would match the units fo the associated info mask element.
>> Thus if you have a _raw element it would be in adc counts (most likely).
>>
>> _input would be in relevant real world units, scale etc in the whatever
>> units the value itself is in.
> 
> Ok, so I forward ported that patch and added code so that the relevant
> channels provide what is available. I also added code to turn the
> rest of the parameter style devicetree properties into iio device/channel
> attributes. So, it is now much neater from a bindings point of view.
> 
> Before I post the updated patches, I'm wondering what the status is
> on that ancient patch? It didn't forward port without issues, but there
> were no real difficulties that I noticed. Should I just start off my v2
> series with that patch? I tend to think that that's the best option,
> because I suspect that adding a "max-ohms" devicetree property as a
> stop-gap pending some new infrastructure is pretty unrealistic...
> 
> Basically, my question is if that ancient patch as any chance of living
> at all in a form close to what it is, or if should start looking for
> an alternative right away?
The stoppers (IIRC) were that at the time we had a lot of drivers not
making full use off the info_mask stuff so there were a whole load
of precursor patches.  A lot of those have been done since, so we are
probably much more ready for it.

The controversial bit was the question of how to describe ranges, but
I don't think that got all that much attention back then.

If you are happy to take on looking after that series I'd certainly
be very happy!   3 years kind of implies I'm not going to get to
it particularly soon myself :(

Will be interesting to see what reviews we get of it when you post it
though.   Perhaps we deliberately push it into drivers only slowly
initially so that if we decide it was a horrible mistake (or that we
need to make changes to the ABI) we only end up supporting obsolete
ABI in a few drivers...

So a slowly but surely one perhaps rather than mass adoption.

Jonathan
> 
> Cheers,
> Peter
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] iio: dpot-dac: DAC driver based on a digital potentiometer

[Peter Rosin]

It is assumed the that the dpot is used as a voltage divider between the
current dpot wiper setting and the maximum resistance of the dpot. The
divided voltage is provided by a vref regulator.

                  .------.
   .-----------.  |      |
   | Vref      |--'    .---.
   | regulator |--.    |   |
   '-----------'  |    | d |
                  |    | p |
                  |    | o |  wiper
                  |    | t |<---------+
                  |    |   |
                  |    '---'       dac output voltage
                  |      |
                  '------+------------+

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 MAINTAINERS                |   1 +
 drivers/iio/dac/Kconfig    |  10 +++
 drivers/iio/dac/Makefile   |   1 +
 drivers/iio/dac/dpot-dac.c | 219 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 drivers/iio/dac/dpot-dac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c68b72088945..8c8aae24b96b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6116,6 +6116,7 @@ M:	Peter Rosin <peda@axentia.se>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
+F:	drivers/iio/dac/dpot-dac.c
 
 IIO SUBSYSTEM AND DRIVERS
 M:	Jonathan Cameron <jic23@kernel.org>
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 120b24478469..934d4138fcdb 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -200,6 +200,16 @@ config AD8801
 	  To compile this driver as a module choose M here: the module will be called
 	  ad8801.
 
+config DPOT_DAC
+	tristate "DAC emulation using a DPOT"
+	depends on OF
+	help
+	  Say yes here to build support for DAC emulation using a digital
+	  potentiometer.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called iio-dpot-dac.
+
 config LPC18XX_DAC
 	tristate "NXP LPC18xx DAC driver"
 	depends on ARCH_LPC18XX || COMPILE_TEST
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 27642bbf75f2..f01bf4a99867 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_AD5686) += ad5686.o
 obj-$(CONFIG_AD7303) += ad7303.o
 obj-$(CONFIG_AD8801) += ad8801.o
 obj-$(CONFIG_CIO_DAC) += cio-dac.o
+obj-$(CONFIG_DPOT_DAC) += dpot-dac.o
 obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
 obj-$(CONFIG_M62332) += m62332.o
 obj-$(CONFIG_MAX517) += max517.o
diff --git a/drivers/iio/dac/dpot-dac.c b/drivers/iio/dac/dpot-dac.c
new file mode 100644
index 000000000000..94fbb4b27287
--- /dev/null
+++ b/drivers/iio/dac/dpot-dac.c
@@ -0,0 +1,219 @@
+/*
+ * IIO DAC emulation driver using a digital potentiometer
+ *
+ * Copyright (C) 2016 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ * It is assumed the that the dpot is used as a voltage divider between the
+ * current dpot wiper setting and the maximum resistance of the dpot. The
+ * divided voltage is provided by a vref regulator.
+ *
+ *                   .------.
+ *    .-----------.  |      |
+ *    | Vref      |--'    .---.
+ *    | regulator |--.    |   |
+ *    '-----------'  |    | d |
+ *                   |    | p |
+ *                   |    | o |  wiper
+ *                   |    | t |<---------+
+ *                   |    |   |
+ *                   |    '---'       dac output voltage
+ *                   |      |
+ *                   '------+------------+
+ */
+
+#include <linux/err.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+struct dpot_dac {
+	struct regulator *vref;
+	struct iio_channel *dpot;
+	u32 max_ohms;
+};
+
+static const struct iio_chan_spec dpot_dac_iio_channel = {
+	.type = IIO_VOLTAGE,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
+			    | BIT(IIO_CHAN_INFO_SCALE),
+	.output = 1,
+};
+
+static int dpot_dac_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct dpot_dac *dac = iio_priv(indio_dev);
+	int ret;
+	unsigned long long tmp;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return iio_read_channel_raw(dac->dpot, val);
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = iio_read_channel_scale(dac->dpot, val, val2);
+		switch (ret) {
+		case IIO_VAL_FRACTIONAL_LOG2:
+			tmp = (s64)*val * 1000000000LL;
+			do_div(tmp, dac->max_ohms);
+			tmp *= regulator_get_voltage(dac->vref) / 1000;
+			do_div(tmp, 1000000000LL);
+			*val = tmp;
+			return ret;
+		case IIO_VAL_INT:
+			*val2 = 1;
+			ret = IIO_VAL_FRACTIONAL;
+			break;
+		}
+		*val *= regulator_get_voltage(dac->vref) / 1000;
+		*val2 *= dac->max_ohms;
+
+		return ret;
+	}
+
+	return -EINVAL;
+}
+
+static int dpot_dac_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	struct dpot_dac *dac = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return iio_write_channel_raw(dac->dpot, val);
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info dpot_dac_info = {
+	.read_raw = dpot_dac_read_raw,
+	.write_raw = dpot_dac_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int dpot_dac_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct iio_dev *indio_dev;
+	struct dpot_dac *dac;
+	enum iio_chan_type type;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*dac));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, indio_dev);
+	dac = iio_priv(indio_dev);
+
+	indio_dev->name = dev_name(dev);
+	indio_dev->dev.parent = dev;
+	indio_dev->info = &dpot_dac_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = &dpot_dac_iio_channel;
+	indio_dev->num_channels = 1;
+
+	dac->vref = devm_regulator_get(dev, "vref");
+	if (IS_ERR(dac->vref)) {
+		if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "failed to get vref regulator\n");
+		return PTR_ERR(dac->vref);
+	}
+
+	dac->dpot = devm_iio_channel_get(dev, "dpot");
+	if (IS_ERR(dac->dpot)) {
+		if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
+			dev_err(dev, "failed to get dpot input channel\n");
+		return PTR_ERR(dac->dpot);
+	}
+
+	switch (iio_read_channel_scale(dac->dpot, &ret, NULL)) {
+	case IIO_VAL_INT:
+	case IIO_VAL_FRACTIONAL:
+	case IIO_VAL_FRACTIONAL_LOG2:
+		break;
+	default:
+		dev_err(dev, "dpot has a scale that is too weird\n");
+		return -EINVAL;
+	}
+
+	ret = iio_get_channel_type(dac->dpot, &type);
+	if (ret < 0)
+		return ret;
+
+	if (type != IIO_RESISTANCE) {
+		dev_err(dev, "dpot is of the wrong type\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "dpot-dac,max-ohms",
+				   &dac->max_ohms);
+	if (ret) {
+		dev_err(dev, "the dpot-dac,max-ohms property is missing\n");
+		return ret;
+	}
+
+	ret = regulator_enable(dac->vref);
+	if (ret) {
+		dev_err(dev, "failed to enable the vref regulator\n");
+		return ret;
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(dev, "failed to register iio device\n");
+		goto disable_reg;
+	}
+
+	return 0;
+
+disable_reg:
+	regulator_disable(dac->vref);
+	return ret;
+}
+
+static int dpot_dac_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct dpot_dac *dac = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	regulator_disable(dac->vref);
+
+	return 0;
+}
+
+static const struct of_device_id dpot_dac_match[] = {
+	{ .compatible = "dpot-dac" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, dpot_dac_match);
+
+static struct platform_driver dpot_dac_driver = {
+	.probe = dpot_dac_probe,
+	.remove = dpot_dac_remove,
+	.driver = {
+		.name = "iio-dpot-dac",
+		.of_match_table = dpot_dac_match,
+	},
+};
+module_platform_driver(dpot_dac_driver);
+
+MODULE_DESCRIPTION("DAC emulation driver using a digital potentiometer");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [PATCH 2/4] iio: dpot-dac: DAC driver based on a digital potentiometer

[Peter Meerwald-Stadler]

> It is assumed the that the dpot is used as a voltage divider between the
> current dpot wiper setting and the maximum resistance of the dpot. The
> divided voltage is provided by a vref regulator.
> 
>                   .------.
>    .-----------.  |      |
>    | Vref      |--'    .---.
>    | regulator |--.    |   |
>    '-----------'  |    | d |
>                   |    | p |
>                   |    | o |  wiper
>                   |    | t |<---------+
>                   |    |   |
>                   |    '---'       dac output voltage
>                   |      |
>                   '------+------------+

nice ASCII art :)
comments below, just nitpicking

> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  MAINTAINERS                |   1 +
>  drivers/iio/dac/Kconfig    |  10 +++
>  drivers/iio/dac/Makefile   |   1 +
>  drivers/iio/dac/dpot-dac.c | 219 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 231 insertions(+)
>  create mode 100644 drivers/iio/dac/dpot-dac.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c68b72088945..8c8aae24b96b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6116,6 +6116,7 @@ M:	Peter Rosin <peda@axentia.se>
>  L:	linux-iio@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
> +F:	drivers/iio/dac/dpot-dac.c
>  
>  IIO SUBSYSTEM AND DRIVERS
>  M:	Jonathan Cameron <jic23@kernel.org>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 120b24478469..934d4138fcdb 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -200,6 +200,16 @@ config AD8801
>  	  To compile this driver as a module choose M here: the module will be called
>  	  ad8801.
>  
> +config DPOT_DAC
> +	tristate "DAC emulation using a DPOT"
> +	depends on OF
> +	help
> +	  Say yes here to build support for DAC emulation using a digital
> +	  potentiometer.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called iio-dpot-dac.

I guess the name will be dpot-dac?

> +
>  config LPC18XX_DAC
>  	tristate "NXP LPC18xx DAC driver"
>  	depends on ARCH_LPC18XX || COMPILE_TEST
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 27642bbf75f2..f01bf4a99867 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_AD5686) += ad5686.o
>  obj-$(CONFIG_AD7303) += ad7303.o
>  obj-$(CONFIG_AD8801) += ad8801.o
>  obj-$(CONFIG_CIO_DAC) += cio-dac.o
> +obj-$(CONFIG_DPOT_DAC) += dpot-dac.o
>  obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
>  obj-$(CONFIG_M62332) += m62332.o
>  obj-$(CONFIG_MAX517) += max517.o
> diff --git a/drivers/iio/dac/dpot-dac.c b/drivers/iio/dac/dpot-dac.c
> new file mode 100644
> index 000000000000..94fbb4b27287
> --- /dev/null
> +++ b/drivers/iio/dac/dpot-dac.c
> @@ -0,0 +1,219 @@
> +/*
> + * IIO DAC emulation driver using a digital potentiometer
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * It is assumed the that the dpot is used as a voltage divider between the

delete first 'the'

> + * current dpot wiper setting and the maximum resistance of the dpot. The
> + * divided voltage is provided by a vref regulator.
> + *
> + *                   .------.
> + *    .-----------.  |      |
> + *    | Vref      |--'    .---.

vref maybe lowercase

> + *    | regulator |--.    |   |
> + *    '-----------'  |    | d |
> + *                   |    | p |
> + *                   |    | o |  wiper
> + *                   |    | t |<---------+
> + *                   |    |   |
> + *                   |    '---'       dac output voltage
> + *                   |      |
> + *                   '------+------------+
> + */
> +
> +#include <linux/err.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct dpot_dac {
> +	struct regulator *vref;
> +	struct iio_channel *dpot;

const

> +	u32 max_ohms;
> +};
> +
> +static const struct iio_chan_spec dpot_dac_iio_channel = {
> +	.type = IIO_VOLTAGE,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> +			    | BIT(IIO_CHAN_INFO_SCALE),
> +	.output = 1,
> +};
> +
> +static int dpot_dac_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct dpot_dac *dac = iio_priv(indio_dev);
> +	int ret;
> +	unsigned long long tmp;

s64 (which is used to cast below)

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return iio_read_channel_raw(dac->dpot, val);
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = iio_read_channel_scale(dac->dpot, val, val2);
> +		switch (ret) {
> +		case IIO_VAL_FRACTIONAL_LOG2:
> +			tmp = (s64)*val * 1000000000LL;
> +			do_div(tmp, dac->max_ohms);
> +			tmp *= regulator_get_voltage(dac->vref) / 1000;
> +			do_div(tmp, 1000000000LL);
> +			*val = tmp;
> +			return ret;
> +		case IIO_VAL_INT:
> +			*val2 = 1;

I think this needs a comment to clarify what's going on...

> +			ret = IIO_VAL_FRACTIONAL;
> +			break;
> +		}
> +		*val *= regulator_get_voltage(dac->vref) / 1000;
> +		*val2 *= dac->max_ohms;
> +
> +		return ret;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int dpot_dac_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct dpot_dac *dac = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return iio_write_channel_raw(dac->dpot, val);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info dpot_dac_info = {
> +	.read_raw = dpot_dac_read_raw,
> +	.write_raw = dpot_dac_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int dpot_dac_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct dpot_dac *dac;
> +	enum iio_chan_type type;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*dac));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +	dac = iio_priv(indio_dev);
> +
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &dpot_dac_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &dpot_dac_iio_channel;
> +	indio_dev->num_channels = 1;

maybe ARRAY_SIZE(dpot_dac_iio_channel) to future-proof it

> +
> +	dac->vref = devm_regulator_get(dev, "vref");
> +	if (IS_ERR(dac->vref)) {
> +		if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "failed to get vref regulator\n");
> +		return PTR_ERR(dac->vref);
> +	}
> +
> +	dac->dpot = devm_iio_channel_get(dev, "dpot");
> +	if (IS_ERR(dac->dpot)) {
> +		if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get dpot input channel\n");
> +		return PTR_ERR(dac->dpot);
> +	}
> +
> +	switch (iio_read_channel_scale(dac->dpot, &ret, NULL)) {
> +	case IIO_VAL_INT:
> +	case IIO_VAL_FRACTIONAL:
> +	case IIO_VAL_FRACTIONAL_LOG2:
> +		break;
> +	default:
> +		dev_err(dev, "dpot has a scale that is too weird\n");

:-)

> +		return -EINVAL;
> +	}
> +
> +	ret = iio_get_channel_type(dac->dpot, &type);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (type != IIO_RESISTANCE) {
> +		dev_err(dev, "dpot is of the wrong type\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "dpot-dac,max-ohms",
> +				   &dac->max_ohms);
> +	if (ret) {
> +		dev_err(dev, "the dpot-dac,max-ohms property is missing\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(dac->vref);
> +	if (ret) {
> +		dev_err(dev, "failed to enable the vref regulator\n");
> +		return ret;
> +	}
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to register iio device\n");
> +		goto disable_reg;
> +	}
> +
> +	return 0;
> +
> +disable_reg:
> +	regulator_disable(dac->vref);
> +	return ret;
> +}
> +
> +static int dpot_dac_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct dpot_dac *dac = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	regulator_disable(dac->vref);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dpot_dac_match[] = {
> +	{ .compatible = "dpot-dac" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, dpot_dac_match);
> +
> +static struct platform_driver dpot_dac_driver = {
> +	.probe = dpot_dac_probe,
> +	.remove = dpot_dac_remove,
> +	.driver = {
> +		.name = "iio-dpot-dac",
> +		.of_match_table = dpot_dac_match,
> +	},
> +};
> +module_platform_driver(dpot_dac_driver);
> +
> +MODULE_DESCRIPTION("DAC emulation driver using a digital potentiometer");
> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 2/4] iio: dpot-dac: DAC driver based on a digital potentiometer

[Peter Rosin]

On 2016-10-20 13:29, Peter Meerwald-Stadler wrote:
>> It is assumed the that the dpot is used as a voltage divider between the
>> current dpot wiper setting and the maximum resistance of the dpot. The
>> divided voltage is provided by a vref regulator.
>>
>>                   .------.
>>    .-----------.  |      |
>>    | Vref      |--'    .---.
>>    | regulator |--.    |   |
>>    '-----------'  |    | d |
>>                   |    | p |
>>                   |    | o |  wiper
>>                   |    | t |<---------+
>>                   |    |   |
>>                   |    '---'       dac output voltage
>>                   |      |
>>                   '------+------------+
> 
> nice ASCII art :)

Thanks!

> comments below, just nitpicking
> 
>> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>> ---
>>  MAINTAINERS                |   1 +
>>  drivers/iio/dac/Kconfig    |  10 +++
>>  drivers/iio/dac/Makefile   |   1 +
>>  drivers/iio/dac/dpot-dac.c | 219 +++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 231 insertions(+)
>>  create mode 100644 drivers/iio/dac/dpot-dac.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c68b72088945..8c8aae24b96b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6116,6 +6116,7 @@ M:	Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>>  L:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>  S:	Maintained
>>  F:	Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
>> +F:	drivers/iio/dac/dpot-dac.c
>>  
>>  IIO SUBSYSTEM AND DRIVERS
>>  M:	Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>> index 120b24478469..934d4138fcdb 100644
>> --- a/drivers/iio/dac/Kconfig
>> +++ b/drivers/iio/dac/Kconfig
>> @@ -200,6 +200,16 @@ config AD8801
>>  	  To compile this driver as a module choose M here: the module will be called
>>  	  ad8801.
>>  
>> +config DPOT_DAC
>> +	tristate "DAC emulation using a DPOT"
>> +	depends on OF
>> +	help
>> +	  Say yes here to build support for DAC emulation using a digital
>> +	  potentiometer.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called iio-dpot-dac.
> 
> I guess the name will be dpot-dac?
> 
>> +
>>  config LPC18XX_DAC
>>  	tristate "NXP LPC18xx DAC driver"
>>  	depends on ARCH_LPC18XX || COMPILE_TEST
>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>> index 27642bbf75f2..f01bf4a99867 100644
>> --- a/drivers/iio/dac/Makefile
>> +++ b/drivers/iio/dac/Makefile
>> @@ -22,6 +22,7 @@ obj-$(CONFIG_AD5686) += ad5686.o
>>  obj-$(CONFIG_AD7303) += ad7303.o
>>  obj-$(CONFIG_AD8801) += ad8801.o
>>  obj-$(CONFIG_CIO_DAC) += cio-dac.o
>> +obj-$(CONFIG_DPOT_DAC) += dpot-dac.o
>>  obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
>>  obj-$(CONFIG_M62332) += m62332.o
>>  obj-$(CONFIG_MAX517) += max517.o
>> diff --git a/drivers/iio/dac/dpot-dac.c b/drivers/iio/dac/dpot-dac.c
>> new file mode 100644
>> index 000000000000..94fbb4b27287
>> --- /dev/null
>> +++ b/drivers/iio/dac/dpot-dac.c
>> @@ -0,0 +1,219 @@
>> +/*
>> + * IIO DAC emulation driver using a digital potentiometer
>> + *
>> + * Copyright (C) 2016 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +/*
>> + * It is assumed the that the dpot is used as a voltage divider between the
> 
> delete first 'the'

Right. Lining this and a few other things up for v2. I'm hoping
for more feedback on other stuff though, so I'll hold that off for
a bit.

>> + * current dpot wiper setting and the maximum resistance of the dpot. The
>> + * divided voltage is provided by a vref regulator.
>> + *
>> + *                   .------.
>> + *    .-----------.  |      |
>> + *    | Vref      |--'    .---.
> 
> vref maybe lowercase

Right, I'm changing Vref to vref all over the map.

>> + *    | regulator |--.    |   |
>> + *    '-----------'  |    | d |
>> + *                   |    | p |
>> + *                   |    | o |  wiper
>> + *                   |    | t |<---------+
>> + *                   |    |   |
>> + *                   |    '---'       dac output voltage
>> + *                   |      |
>> + *                   '------+------------+
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/iio/consumer.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +struct dpot_dac {
>> +	struct regulator *vref;
>> +	struct iio_channel *dpot;
> 
> const

const? devm_iio_channel_get doesn't return a const iio_channel. What
am I missing?

>> +	u32 max_ohms;
>> +};
>> +
>> +static const struct iio_chan_spec dpot_dac_iio_channel = {
>> +	.type = IIO_VOLTAGE,
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
>> +			    | BIT(IIO_CHAN_INFO_SCALE),
>> +	.output = 1,
>> +};
>> +
>> +static int dpot_dac_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val, int *val2, long mask)
>> +{
>> +	struct dpot_dac *dac = iio_priv(indio_dev);
>> +	int ret;
>> +	unsigned long long tmp;
> 
> s64 (which is used to cast below)

I assume that what you really want is to get rid of the cast, and
that you aren't really bothered if tmp is ull or s32, right? Given
that assumption, I'm just dropping the cast instead. The LL specifier
on the constant should promote *val anyway.

>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		return iio_read_channel_raw(dac->dpot, val);
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		ret = iio_read_channel_scale(dac->dpot, val, val2);
>> +		switch (ret) {
>> +		case IIO_VAL_FRACTIONAL_LOG2:
>> +			tmp = (s64)*val * 1000000000LL;
>> +			do_div(tmp, dac->max_ohms);
>> +			tmp *= regulator_get_voltage(dac->vref) / 1000;
>> +			do_div(tmp, 1000000000LL);
>> +			*val = tmp;
>> +			return ret;
>> +		case IIO_VAL_INT:
>> +			*val2 = 1;
> 
> I think this needs a comment to clarify what's going on...

How about this:

/*
 * Convert integer scale to fractional scale by
 * setting the denominator (*val2) to 1 (one) and
 * fall through to the handler for fractional scale.
 */

>> +			ret = IIO_VAL_FRACTIONAL;
>> +			break;
>> +		}
>> +		*val *= regulator_get_voltage(dac->vref) / 1000;
>> +		*val2 *= dac->max_ohms;
>> +
>> +		return ret;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int dpot_dac_write_raw(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan,
>> +			      int val, int val2, long mask)
>> +{
>> +	struct dpot_dac *dac = iio_priv(indio_dev);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		return iio_write_channel_raw(dac->dpot, val);
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info dpot_dac_info = {
>> +	.read_raw = dpot_dac_read_raw,
>> +	.write_raw = dpot_dac_write_raw,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +static int dpot_dac_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct iio_dev *indio_dev;
>> +	struct dpot_dac *dac;
>> +	enum iio_chan_type type;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*dac));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +	dac = iio_priv(indio_dev);
>> +
>> +	indio_dev->name = dev_name(dev);
>> +	indio_dev->dev.parent = dev;
>> +	indio_dev->info = &dpot_dac_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = &dpot_dac_iio_channel;
>> +	indio_dev->num_channels = 1;
> 
> maybe ARRAY_SIZE(dpot_dac_iio_channel) to future-proof it

Except dpot_dac_iio_channel isn't an array. Does ARRAY_SIZE
work on non-arrays?

Cheers,
Peter

>> +
>> +	dac->vref = devm_regulator_get(dev, "vref");
>> +	if (IS_ERR(dac->vref)) {
>> +		if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev, "failed to get vref regulator\n");
>> +		return PTR_ERR(dac->vref);
>> +	}
>> +
>> +	dac->dpot = devm_iio_channel_get(dev, "dpot");
>> +	if (IS_ERR(dac->dpot)) {
>> +		if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
>> +			dev_err(dev, "failed to get dpot input channel\n");
>> +		return PTR_ERR(dac->dpot);
>> +	}
>> +
>> +	switch (iio_read_channel_scale(dac->dpot, &ret, NULL)) {
>> +	case IIO_VAL_INT:
>> +	case IIO_VAL_FRACTIONAL:
>> +	case IIO_VAL_FRACTIONAL_LOG2:
>> +		break;
>> +	default:
>> +		dev_err(dev, "dpot has a scale that is too weird\n");
> 
> :-)
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = iio_get_channel_type(dac->dpot, &type);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (type != IIO_RESISTANCE) {
>> +		dev_err(dev, "dpot is of the wrong type\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = of_property_read_u32(dev->of_node, "dpot-dac,max-ohms",
>> +				   &dac->max_ohms);
>> +	if (ret) {
>> +		dev_err(dev, "the dpot-dac,max-ohms property is missing\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_enable(dac->vref);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable the vref regulator\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register iio device\n");
>> +		goto disable_reg;
>> +	}
>> +
>> +	return 0;
>> +
>> +disable_reg:
>> +	regulator_disable(dac->vref);
>> +	return ret;
>> +}
>> +
>> +static int dpot_dac_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +	struct dpot_dac *dac = iio_priv(indio_dev);
>> +
>> +	iio_device_unregister(indio_dev);
>> +	regulator_disable(dac->vref);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id dpot_dac_match[] = {
>> +	{ .compatible = "dpot-dac" },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, dpot_dac_match);
>> +
>> +static struct platform_driver dpot_dac_driver = {
>> +	.probe = dpot_dac_probe,
>> +	.remove = dpot_dac_remove,
>> +	.driver = {
>> +		.name = "iio-dpot-dac",
>> +		.of_match_table = dpot_dac_match,
>> +	},
>> +};
>> +module_platform_driver(dpot_dac_driver);
>> +
>> +MODULE_DESCRIPTION("DAC emulation driver using a digital potentiometer");
>> +MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>");
>> +MODULE_LICENSE("GPL v2");
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] iio: dpot-dac: DAC driver based on a digital potentiometer

[Peter Meerwald-Stadler]

> >> +struct dpot_dac {
> >> +	struct regulator *vref;
> >> +	struct iio_channel *dpot;
> > 
> > const
> 
> const? devm_iio_channel_get doesn't return a const iio_channel. What
> am I missing?

ah, I mixed this up with iio_chan_spec
 
> >> +	u32 max_ohms;
> >> +};
> >> +
> >> +static const struct iio_chan_spec dpot_dac_iio_channel = {
> >> +	.type = IIO_VOLTAGE,
> >> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> >> +			    | BIT(IIO_CHAN_INFO_SCALE),
> >> +	.output = 1,
> >> +};
> >> +
> >> +static int dpot_dac_read_raw(struct iio_dev *indio_dev,
> >> +			     struct iio_chan_spec const *chan,
> >> +			     int *val, int *val2, long mask)
> >> +{
> >> +	struct dpot_dac *dac = iio_priv(indio_dev);
> >> +	int ret;
> >> +	unsigned long long tmp;
> > 
> > s64 (which is used to cast below)
> 
> I assume that what you really want is to get rid of the cast, and
> that you aren't really bothered if tmp is ull or s32, right? Given
> that assumption, I'm just dropping the cast instead. The LL specifier
> on the constant should promote *val anyway.

what I meant to pick upon was mixing
s64 with unsigned long long with LL in the code that follows

anything simpler and more consistent would be good

thanks, p.

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] iio: dpot-dac: DAC driver based on a digital potentiometer

[Peter Rosin]

On 2016-10-20 16:08, Peter Meerwald-Stadler wrote:
>>>> +	u32 max_ohms;
>>>> +};
>>>> +
>>>> +static const struct iio_chan_spec dpot_dac_iio_channel = {
>>>> +	.type = IIO_VOLTAGE,
>>>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
>>>> +			    | BIT(IIO_CHAN_INFO_SCALE),
>>>> +	.output = 1,
>>>> +};
>>>> +
>>>> +static int dpot_dac_read_raw(struct iio_dev *indio_dev,
>>>> +			     struct iio_chan_spec const *chan,
>>>> +			     int *val, int *val2, long mask)
>>>> +{
>>>> +	struct dpot_dac *dac = iio_priv(indio_dev);
>>>> +	int ret;
>>>> +	unsigned long long tmp;
>>>
>>> s64 (which is used to cast below)
>>
>> I assume that what you really want is to get rid of the cast, and
>> that you aren't really bothered if tmp is ull or s32, right? Given
>> that assumption, I'm just dropping the cast instead. The LL specifier
>> on the constant should promote *val anyway.
> 
> what I meant to pick upon was mixing
> s64 with unsigned long long with LL in the code that follows
> 
> anything simpler and more consistent would be good

Right, I should however note that I copied the pattern from the
industrialio-core.c:iio_format_value function. So, in my defense
I was consistent with that...

Cheers,
Peter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] iio: dpot-dac: DAC driver based on a digital potentiometer

[Jonathan Cameron]

On 20/10/16 10:26, Peter Rosin wrote:
> It is assumed the that the dpot is used as a voltage divider between the
> current dpot wiper setting and the maximum resistance of the dpot. The
> divided voltage is provided by a vref regulator.
> 
>                   .------.
>    .-----------.  |      |
>    | Vref      |--'    .---.
>    | regulator |--.    |   |
>    '-----------'  |    | d |
>                   |    | p |
>                   |    | o |  wiper
>                   |    | t |<---------+
>                   |    |   |
>                   |    '---'       dac output voltage
>                   |      |
>                   '------+------------+
The world needs more Asci art ;)
> 
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
I've added various comments, but not actual changes.  Peter covered the few
bits I noticed already.  Very nice.

J
> ---
>  MAINTAINERS                |   1 +
>  drivers/iio/dac/Kconfig    |  10 +++
>  drivers/iio/dac/Makefile   |   1 +
>  drivers/iio/dac/dpot-dac.c | 219 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 231 insertions(+)
>  create mode 100644 drivers/iio/dac/dpot-dac.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c68b72088945..8c8aae24b96b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6116,6 +6116,7 @@ M:	Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>  L:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
> +F:	drivers/iio/dac/dpot-dac.c
>  
>  IIO SUBSYSTEM AND DRIVERS
>  M:	Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 120b24478469..934d4138fcdb 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -200,6 +200,16 @@ config AD8801
>  	  To compile this driver as a module choose M here: the module will be called
>  	  ad8801.
>  
> +config DPOT_DAC
> +	tristate "DAC emulation using a DPOT"
> +	depends on OF
> +	help
> +	  Say yes here to build support for DAC emulation using a digital
> +	  potentiometer.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called iio-dpot-dac.
> +
>  config LPC18XX_DAC
>  	tristate "NXP LPC18xx DAC driver"
>  	depends on ARCH_LPC18XX || COMPILE_TEST
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 27642bbf75f2..f01bf4a99867 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_AD5686) += ad5686.o
>  obj-$(CONFIG_AD7303) += ad7303.o
>  obj-$(CONFIG_AD8801) += ad8801.o
>  obj-$(CONFIG_CIO_DAC) += cio-dac.o
> +obj-$(CONFIG_DPOT_DAC) += dpot-dac.o
>  obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
>  obj-$(CONFIG_M62332) += m62332.o
>  obj-$(CONFIG_MAX517) += max517.o
> diff --git a/drivers/iio/dac/dpot-dac.c b/drivers/iio/dac/dpot-dac.c
> new file mode 100644
> index 000000000000..94fbb4b27287
> --- /dev/null
> +++ b/drivers/iio/dac/dpot-dac.c
> @@ -0,0 +1,219 @@
> +/*
> + * IIO DAC emulation driver using a digital potentiometer
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * It is assumed the that the dpot is used as a voltage divider between the
> + * current dpot wiper setting and the maximum resistance of the dpot. The
> + * divided voltage is provided by a vref regulator.
> + *
> + *                   .------.
> + *    .-----------.  |      |
> + *    | Vref      |--'    .---.
> + *    | regulator |--.    |   |
> + *    '-----------'  |    | d |
> + *                   |    | p |
> + *                   |    | o |  wiper
> + *                   |    | t |<---------+
> + *                   |    |   |
> + *                   |    '---'       dac output voltage
> + *                   |      |
> + *                   '------+------------+
> + */
> +
> +#include <linux/err.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct dpot_dac {
> +	struct regulator *vref;
> +	struct iio_channel *dpot;
> +	u32 max_ohms;
> +};
> +
> +static const struct iio_chan_spec dpot_dac_iio_channel = {
> +	.type = IIO_VOLTAGE,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> +			    | BIT(IIO_CHAN_INFO_SCALE),
> +	.output = 1,
> +};
> +
> +static int dpot_dac_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct dpot_dac *dac = iio_priv(indio_dev);
> +	int ret;
> +	unsigned long long tmp;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return iio_read_channel_raw(dac->dpot, val);
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = iio_read_channel_scale(dac->dpot, val, val2);
> +		switch (ret) {
> +		case IIO_VAL_FRACTIONAL_LOG2:
> +			tmp = (s64)*val * 1000000000LL;
> +			do_div(tmp, dac->max_ohms);
> +			tmp *= regulator_get_voltage(dac->vref) / 1000;
> +			do_div(tmp, 1000000000LL);
> +			*val = tmp;
> +			return ret;
> +		case IIO_VAL_INT:
> +			*val2 = 1;
> +			ret = IIO_VAL_FRACTIONAL;
> +			break;
> +		}
> +		*val *= regulator_get_voltage(dac->vref) / 1000;
> +		*val2 *= dac->max_ohms;
> +
> +		return ret;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int dpot_dac_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct dpot_dac *dac = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return iio_write_channel_raw(dac->dpot, val);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info dpot_dac_info = {
> +	.read_raw = dpot_dac_read_raw,
> +	.write_raw = dpot_dac_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int dpot_dac_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct dpot_dac *dac;
> +	enum iio_chan_type type;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*dac));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +	dac = iio_priv(indio_dev);
> +
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &dpot_dac_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &dpot_dac_iio_channel;
> +	indio_dev->num_channels = 1;
> +
> +	dac->vref = devm_regulator_get(dev, "vref");
> +	if (IS_ERR(dac->vref)) {
> +		if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "failed to get vref regulator\n");
> +		return PTR_ERR(dac->vref);
> +	}
> +
> +	dac->dpot = devm_iio_channel_get(dev, "dpot");
> +	if (IS_ERR(dac->dpot)) {
> +		if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get dpot input channel\n");
> +		return PTR_ERR(dac->dpot);
> +	}
> +
> +	switch (iio_read_channel_scale(dac->dpot, &ret, NULL)) {
> +	case IIO_VAL_INT:
> +	case IIO_VAL_FRACTIONAL:
> +	case IIO_VAL_FRACTIONAL_LOG2:
> +		break;
> +	default:
> +		dev_err(dev, "dpot has a scale that is too weird\n");
int plus micro is kind of the default, so should probably handle that
one as well if possible.  Can be added later when someone needs it though.
> +		return -EINVAL;
> +	}
> +
> +	ret = iio_get_channel_type(dac->dpot, &type);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (type != IIO_RESISTANCE) {
> +		dev_err(dev, "dpot is of the wrong type\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "dpot-dac,max-ohms",
> +				   &dac->max_ohms);
> +	if (ret) {
> +		dev_err(dev, "the dpot-dac,max-ohms property is missing\n");
> +		return ret;
> +	}
Now I know why you wanted that range ;)  Was wondering a bit when reading
your cover letter...  All makes sense now.
> +
> +	ret = regulator_enable(dac->vref);
> +	if (ret) {
> +		dev_err(dev, "failed to enable the vref regulator\n");
> +		return ret;
> +	}
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to register iio device\n");
> +		goto disable_reg;
> +	}
> +
> +	return 0;
> +
> +disable_reg:
> +	regulator_disable(dac->vref);
> +	return ret;
> +}
> +
> +static int dpot_dac_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct dpot_dac *dac = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	regulator_disable(dac->vref);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id dpot_dac_match[] = {
> +	{ .compatible = "dpot-dac" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, dpot_dac_match);
> +
> +static struct platform_driver dpot_dac_driver = {
> +	.probe = dpot_dac_probe,
> +	.remove = dpot_dac_remove,
> +	.driver = {
> +		.name = "iio-dpot-dac",
> +		.of_match_table = dpot_dac_match,
> +	},
> +};
> +module_platform_driver(dpot_dac_driver);
> +
> +MODULE_DESCRIPTION("DAC emulation driver using a digital potentiometer");
> +MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] iio: envelope-detector: ADC driver based on a DAC and a comparator

[Peter Rosin]

The DAC is used to find the peak level of an alternating voltage input
signal by a binary search using the output of a comparator wired to
an interrupt pin. Like so:
                          _
                         | \
    input +------>-------|+ \
                         |   \
           .-------.     |    }---.
           |       |     |   /    |
           |    dac|-->--|- /     |
           |       |     |_/      |
           |       |              |
           |       |              |
           |    irq|------<-------'
           |       |
           '-------'

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 MAINTAINERS                         |   1 +
 drivers/iio/adc/Kconfig             |  10 ++
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/envelope-detector.c | 305 ++++++++++++++++++++++++++++++++++++
 4 files changed, 317 insertions(+)
 create mode 100644 drivers/iio/adc/envelope-detector.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4b6f6ec1b703..d9a58b5f2b6f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6123,6 +6123,7 @@ M:	Peter Rosin <peda@axentia.se>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
+F:	drivers/iio/adc/envelope-detector.c
 
 IIO SUBSYSTEM AND DRIVERS
 M:	Jonathan Cameron <jic23@kernel.org>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 7edcf3238620..d5c4a95855c2 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -195,6 +195,16 @@ config DA9150_GPADC
 	  To compile this driver as a module, choose M here: the module will be
 	  called berlin2-adc.
 
+config ENVELOPE_DETECTOR
+	tristate "Envelope detector using a DAC and a comparator"
+	depends on OF
+	help
+	  Say yes here to build support for an envelope detector using a DAC
+	  and a comparator.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called iio-envelope-detector.
+
 config EXYNOS_ADC
 	tristate "Exynos ADC driver support"
 	depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || (OF && COMPILE_TEST)
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 7a40c04c311f..0d773c6a0578 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o
 obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
 obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
+obj-$(CONFIG_ENVELOPE_DETECTOR) += envelope-detector.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
 obj-$(CONFIG_HI8435) += hi8435.o
diff --git a/drivers/iio/adc/envelope-detector.c b/drivers/iio/adc/envelope-detector.c
new file mode 100644
index 000000000000..837646107beb
--- /dev/null
+++ b/drivers/iio/adc/envelope-detector.c
@@ -0,0 +1,305 @@
+/*
+ * Driver for an envelope detector using a DAC and a comparator
+ *
+ * Copyright (C) 2016 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ * The DAC is used to find the peak level of an alternating voltage input
+ * signal by a binary search using the output of a comparator wired to
+ * an interrupt pin. Like so:
+ *                           _
+ *                          | \
+ *     input +------>-------|+ \
+ *                          |   \
+ *            .-------.     |    }---.
+ *            |       |     |   /    |
+ *            |    dac|-->--|- /     |
+ *            |       |     |_/      |
+ *            |       |              |
+ *            |       |              |
+ *            |    irq|------<-------'
+ *            |       |
+ *            '-------'
+ */
+
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+
+struct envelope {
+	struct iio_channel *dac;
+	struct delayed_work comp_timeout;
+	int comp_irq;
+
+	spinlock_t comp_lock; /* protects comp */
+	int comp;
+
+	struct mutex read_lock; /* protects everything below */
+
+	u32 dac_max;
+	u32 comp_interval;
+	bool invert;
+
+	int high;
+	int level;
+	int low;
+
+	struct completion done;
+};
+
+static int envelope_detector_latch(struct envelope *env)
+{
+	int comp;
+
+	spin_lock_irq(&env->comp_lock);
+	comp = env->comp;
+	env->comp = 0;
+	spin_unlock_irq(&env->comp_lock);
+
+	if (comp)
+		enable_irq(env->comp_irq);
+
+	return comp;
+}
+
+static irqreturn_t envelope_detector_isr(int irq, void *ctx)
+{
+	struct envelope *env = ctx;
+
+	spin_lock(&env->comp_lock);
+	env->comp = 1;
+	disable_irq_nosync(env->comp_irq);
+	spin_unlock(&env->comp_lock);
+
+	return IRQ_HANDLED;
+}
+
+static void envelope_detector_setup_compare(struct envelope *env)
+{
+	int ret;
+
+	/*
+	 * Do a binary search for the peak input level, and stop
+	 * when that level is "trapped" between two adjacent DAC
+	 * values.
+	 * When invert is active, use the midpoint floor so that
+	 * env->level ends up as env->low when the termination
+	 * criteria below is fulfilled, and use the midpoint
+	 * ceiling when invert is not active so that env->level
+	 * ends up as env->high in that case.
+	 */
+	env->level = (env->high + env->low + !env->invert) / 2;
+
+	if (env->high == env->low + 1) {
+		complete(&env->done);
+		return;
+	}
+
+	/* Set a "safe" DAC level (if there is such a thing)... */
+	ret = iio_write_channel_raw(env->dac, env->invert ? 0 : env->dac_max);
+	if (ret < 0)
+		goto err;
+
+	/* ...clear the comparison result... */
+	envelope_detector_latch(env);
+
+	/* ...set the real DAC level... */
+	ret = iio_write_channel_raw(env->dac, env->level);
+	if (ret < 0)
+		goto err;
+
+	/* ...and wait for a bit to see if the latch catches anything. */
+	schedule_delayed_work(&env->comp_timeout,
+			      msecs_to_jiffies(env->comp_interval));
+	return;
+
+err:
+	env->level = ret;
+	complete(&env->done);
+}
+
+static void envelope_detector_timeout(struct work_struct *work)
+{
+	struct envelope *env = container_of(work, struct envelope,
+					    comp_timeout.work);
+
+	/* Adjust low/high depending on the latch content... */
+	if (!envelope_detector_latch(env) ^ !env->invert)
+		env->low = env->level;
+	else
+		env->high = env->level;
+
+	/* ...and continue the search. */
+	envelope_detector_setup_compare(env);
+}
+
+static int envelope_detector_read_raw(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      int *val, int *val2, long mask)
+{
+	struct envelope *env = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		/*
+		 * When invert is active, start with high=max+1 and low=0
+		 * since we will end up with the low value when the
+		 * termination criteria is fulfilled (rounding down). And
+		 * start with high=max and low=-1 when invert is not active
+		 * since we will end up with the high value in that case.
+		 * This ensures that we in both cases return a value in the
+		 * same range as the DAC and that as not triggered the
+		 * comparator.
+		 */
+		mutex_lock(&env->read_lock);
+		env->high = env->dac_max + 1 - !env->invert;
+		env->low = 0 - !env->invert;
+		envelope_detector_setup_compare(env);
+		wait_for_completion(&env->done);
+		if (env->level < 0) {
+			ret = env->level;
+			goto err_unlock;
+		}
+		*val = env->invert ? env->dac_max - env->level : env->level;
+		mutex_unlock(&env->read_lock);
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		return iio_read_channel_scale(env->dac, val, val2);
+	}
+
+	return -EINVAL;
+
+err_unlock:
+	mutex_unlock(&env->read_lock);
+	return ret;
+}
+
+static const struct iio_chan_spec envelope_detector_iio_channel = {
+	.type = IIO_ALTVOLTAGE,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
+			    | BIT(IIO_CHAN_INFO_SCALE),
+	.output = 1,
+};
+
+static const struct iio_info envelope_detector_info = {
+	.read_raw = &envelope_detector_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int envelope_detector_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct iio_dev *indio_dev;
+	struct envelope *env;
+	enum iio_chan_type type;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*env));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, indio_dev);
+	env = iio_priv(indio_dev);
+
+	spin_lock_init(&env->comp_lock);
+	mutex_init(&env->read_lock);
+	init_completion(&env->done);
+	INIT_DELAYED_WORK(&env->comp_timeout, envelope_detector_timeout);
+
+	indio_dev->name = dev_name(dev);
+	indio_dev->dev.parent = dev;
+	indio_dev->dev.of_node = dev->of_node;
+	indio_dev->info = &envelope_detector_info;
+	indio_dev->channels = &envelope_detector_iio_channel;
+	indio_dev->num_channels = 1;
+
+	env->dac = devm_iio_channel_get(dev, "dac");
+	if (IS_ERR(env->dac)) {
+		if (PTR_ERR(env->dac) != -EPROBE_DEFER)
+			dev_err(dev, "failed to get dac input channel\n");
+		return PTR_ERR(env->dac);
+	}
+
+	env->comp_irq = platform_get_irq_byname(pdev, "comp");
+	if (env->comp_irq < 0) {
+		if (env->comp_irq != -EPROBE_DEFER)
+			dev_err(dev, "failed to get compare interrupt\n");
+		return env->comp_irq;
+	}
+
+	ret = devm_request_irq(dev, env->comp_irq, envelope_detector_isr, 0,
+			       "env-env-dac-comp", env);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to request interrupt\n");
+		return ret;
+	}
+
+	ret = iio_get_channel_type(env->dac, &type);
+	if (ret < 0)
+		return ret;
+
+	if (type != IIO_VOLTAGE) {
+		dev_err(dev, "dac is of the wrong type\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "envelope-detector,dac-max",
+				   &env->dac_max);
+	if (ret) {
+		dev_err(dev, "the dac-max property is missing\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(dev->of_node,
+				   "envelope-detector,comp-interval-ms",
+				   &env->comp_interval);
+	if (ret) {
+		dev_err(dev, "the comp-interval-ms property is missing\n");
+		return ret;
+	}
+
+	env->invert = of_property_read_bool(dev->of_node,
+					    "envelope-detector,inverted");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id envelope_detector_match[] = {
+	{ .compatible = "envelope-detector" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, envelope_detector_match);
+
+static struct platform_driver envelope_detector_driver = {
+	.probe = envelope_detector_probe,
+	.driver = {
+		.name = "iio-envelope-detector",
+		.of_match_table = envelope_detector_match,
+	},
+};
+module_platform_driver(envelope_detector_driver);
+
+MODULE_DESCRIPTION("Envelope detector using a DAC and a comparator");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [PATCH 4/4] iio: envelope-detector: ADC driver based on a DAC and a comparator

[Jonathan Cameron]

On 20/10/16 10:26, Peter Rosin wrote:
> The DAC is used to find the peak level of an alternating voltage input
> signal by a binary search using the output of a comparator wired to
> an interrupt pin. Like so:
>                           _
>                          | \
>     input +------>-------|+ \
>                          |   \
>            .-------.     |    }---.
>            |       |     |   /    |
>            |    dac|-->--|- /     |
>            |       |     |_/      |
>            |       |              |
>            |       |              |
>            |    irq|------<-------'
>            |       |
>            '-------'
> 
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Some really trivial bits inline that might have made it into your V2.

Jonathan
> ---
>  MAINTAINERS                         |   1 +
>  drivers/iio/adc/Kconfig             |  10 ++
>  drivers/iio/adc/Makefile            |   1 +
>  drivers/iio/adc/envelope-detector.c | 305 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 317 insertions(+)
>  create mode 100644 drivers/iio/adc/envelope-detector.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b6f6ec1b703..d9a58b5f2b6f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6123,6 +6123,7 @@ M:	Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>  L:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> +F:	drivers/iio/adc/envelope-detector.c
>  
>  IIO SUBSYSTEM AND DRIVERS
>  M:	Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7edcf3238620..d5c4a95855c2 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -195,6 +195,16 @@ config DA9150_GPADC
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called berlin2-adc.
>  
> +config ENVELOPE_DETECTOR
> +	tristate "Envelope detector using a DAC and a comparator"
> +	depends on OF
> +	help
> +	  Say yes here to build support for an envelope detector using a DAC
> +	  and a comparator.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called iio-envelope-detector.
> +
>  config EXYNOS_ADC
>  	tristate "Exynos ADC driver support"
>  	depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || (OF && COMPILE_TEST)
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7a40c04c311f..0d773c6a0578 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o
>  obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>  obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
> +obj-$(CONFIG_ENVELOPE_DETECTOR) += envelope-detector.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>  obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
>  obj-$(CONFIG_HI8435) += hi8435.o
> diff --git a/drivers/iio/adc/envelope-detector.c b/drivers/iio/adc/envelope-detector.c
> new file mode 100644
> index 000000000000..837646107beb
> --- /dev/null
> +++ b/drivers/iio/adc/envelope-detector.c
> @@ -0,0 +1,305 @@
> +/*
> + * Driver for an envelope detector using a DAC and a comparator
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * The DAC is used to find the peak level of an alternating voltage input
> + * signal by a binary search using the output of a comparator wired to
> + * an interrupt pin. Like so:
> + *                           _
> + *                          | \
> + *     input +------>-------|+ \
> + *                          |   \
> + *            .-------.     |    }---.
> + *            |       |     |   /    |
> + *            |    dac|-->--|- /     |
> + *            |       |     |_/      |
> + *            |       |              |
> + *            |       |              |
> + *            |    irq|------<-------'
> + *            |       |
> + *            '-------'
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +
> +struct envelope {
> +	struct iio_channel *dac;
> +	struct delayed_work comp_timeout;
> +	int comp_irq;
> +
> +	spinlock_t comp_lock; /* protects comp */
> +	int comp;
> +
> +	struct mutex read_lock; /* protects everything below */
> +
> +	u32 dac_max;
> +	u32 comp_interval;
> +	bool invert;
> +
> +	int high;
> +	int level;
> +	int low;
> +
> +	struct completion done;
> +};
> +
Perhaps a comment explaining what this function is doing..
The naming isn't 100% clear...
> +static int envelope_detector_latch(struct envelope *env)
> +{
> +	int comp;
> +
> +	spin_lock_irq(&env->comp_lock);
> +	comp = env->comp;
> +	env->comp = 0;
> +	spin_unlock_irq(&env->comp_lock);
> +
> +	if (comp)
> +		enable_irq(env->comp_irq);
> +
> +	return comp;
> +}
> +
> +static irqreturn_t envelope_detector_isr(int irq, void *ctx)
> +{
> +	struct envelope *env = ctx;
> +
> +	spin_lock(&env->comp_lock);
> +	env->comp = 1;
> +	disable_irq_nosync(env->comp_irq);
> +	spin_unlock(&env->comp_lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void envelope_detector_setup_compare(struct envelope *env)
> +{
> +	int ret;
> +
> +	/*
> +	 * Do a binary search for the peak input level, and stop
> +	 * when that level is "trapped" between two adjacent DAC
> +	 * values.
> +	 * When invert is active, use the midpoint floor so that
> +	 * env->level ends up as env->low when the termination
> +	 * criteria below is fulfilled, and use the midpoint
> +	 * ceiling when invert is not active so that env->level
> +	 * ends up as env->high in that case.
> +	 */
> +	env->level = (env->high + env->low + !env->invert) / 2;
> +
> +	if (env->high == env->low + 1) {
> +		complete(&env->done);
> +		return;
> +	}
> +
> +	/* Set a "safe" DAC level (if there is such a thing)... */
> +	ret = iio_write_channel_raw(env->dac, env->invert ? 0 : env->dac_max);
> +	if (ret < 0)
> +		goto err;
> +
> +	/* ...clear the comparison result... */
> +	envelope_detector_latch(env);
> +
> +	/* ...set the real DAC level... */
> +	ret = iio_write_channel_raw(env->dac, env->level);
> +	if (ret < 0)
> +		goto err;
> +
> +	/* ...and wait for a bit to see if the latch catches anything. */
> +	schedule_delayed_work(&env->comp_timeout,
> +			      msecs_to_jiffies(env->comp_interval));
> +	return;
> +
> +err:
> +	env->level = ret;
> +	complete(&env->done);
> +}
> +
> +static void envelope_detector_timeout(struct work_struct *work)
> +{
> +	struct envelope *env = container_of(work, struct envelope,
> +					    comp_timeout.work);
> +
> +	/* Adjust low/high depending on the latch content... */
> +	if (!envelope_detector_latch(env) ^ !env->invert)
> +		env->low = env->level;
> +	else
> +		env->high = env->level;
> +
> +	/* ...and continue the search. */
> +	envelope_detector_setup_compare(env);
> +}
> +
> +static int envelope_detector_read_raw(struct iio_dev *indio_dev,
> +				      struct iio_chan_spec const *chan,
> +				      int *val, int *val2, long mask)
> +{
> +	struct envelope *env = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		/*
> +		 * When invert is active, start with high=max+1 and low=0
> +		 * since we will end up with the low value when the
> +		 * termination criteria is fulfilled (rounding down). And
> +		 * start with high=max and low=-1 when invert is not active
> +		 * since we will end up with the high value in that case.
> +		 * This ensures that we in both cases return a value in the
> +		 * same range as the DAC and that as not triggered the
has

Impressive level of detail. I'd probably have missed the need for this!
> +		 * comparator.
> +		 */
> +		mutex_lock(&env->read_lock);
> +		env->high = env->dac_max + 1 - !env->invert;
> +		env->low = 0 - !env->invert;
> +		envelope_detector_setup_compare(env);
> +		wait_for_completion(&env->done);
> +		if (env->level < 0) {
> +			ret = env->level;
> +			goto err_unlock;
> +		}
> +		*val = env->invert ? env->dac_max - env->level : env->level;
> +		mutex_unlock(&env->read_lock);
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		return iio_read_channel_scale(env->dac, val, val2);
> +	}
> +
> +	return -EINVAL;
> +
> +err_unlock:
> +	mutex_unlock(&env->read_lock);
> +	return ret;
> +}
> +
> +static const struct iio_chan_spec envelope_detector_iio_channel = {
> +	.type = IIO_ALTVOLTAGE,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> +			    | BIT(IIO_CHAN_INFO_SCALE),
> +	.output = 1,
> +};
> +
> +static const struct iio_info envelope_detector_info = {
> +	.read_raw = &envelope_detector_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int envelope_detector_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct envelope *env;
> +	enum iio_chan_type type;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*env));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +	env = iio_priv(indio_dev);
> +
> +	spin_lock_init(&env->comp_lock);
> +	mutex_init(&env->read_lock);
> +	init_completion(&env->done);
> +	INIT_DELAYED_WORK(&env->comp_timeout, envelope_detector_timeout);
> +
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->dev.parent = dev;
> +	indio_dev->dev.of_node = dev->of_node;
> +	indio_dev->info = &envelope_detector_info;
> +	indio_dev->channels = &envelope_detector_iio_channel;
> +	indio_dev->num_channels = 1;
> +
> +	env->dac = devm_iio_channel_get(dev, "dac");
> +	if (IS_ERR(env->dac)) {
> +		if (PTR_ERR(env->dac) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get dac input channel\n");
> +		return PTR_ERR(env->dac);
> +	}
> +
> +	env->comp_irq = platform_get_irq_byname(pdev, "comp");
> +	if (env->comp_irq < 0) {
> +		if (env->comp_irq != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get compare interrupt\n");
> +		return env->comp_irq;
> +	}
> +
> +	ret = devm_request_irq(dev, env->comp_irq, envelope_detector_isr, 0,
> +			       "env-env-dac-comp", env);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to request interrupt\n");
> +		return ret;
> +	}
> +
> +	ret = iio_get_channel_type(env->dac, &type);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (type != IIO_VOLTAGE) {
> +		dev_err(dev, "dac is of the wrong type\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "envelope-detector,dac-max",
> +				   &env->dac_max);
> +	if (ret) {
> +		dev_err(dev, "the dac-max property is missing\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node,
> +				   "envelope-detector,comp-interval-ms",
> +				   &env->comp_interval);
> +	if (ret) {
> +		dev_err(dev, "the comp-interval-ms property is missing\n");
> +		return ret;
> +	}
> +
> +	env->invert = of_property_read_bool(dev->of_node,
> +					    "envelope-detector,inverted");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct of_device_id envelope_detector_match[] = {
> +	{ .compatible = "envelope-detector" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, envelope_detector_match);
> +
> +static struct platform_driver envelope_detector_driver = {
> +	.probe = envelope_detector_probe,
> +	.driver = {
> +		.name = "iio-envelope-detector",
> +		.of_match_table = envelope_detector_match,
> +	},
> +};
> +module_platform_driver(envelope_detector_driver);
> +
> +MODULE_DESCRIPTION("Envelope detector using a DAC and a comparator");
> +MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html