Re: [PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

On 03/05/16 12:22, Harald Geyer wrote:

Hi Stefan!

On 03.05.2016 13:07, Stefan Wahren wrote:

quoted

Am 22.04.2016 um 15:52 schrieb Harald Geyer:

quoted

The hardware has two current sources ISRC0 and ISRC1 to allow measuring
resistors without additional circuitry. This commit makes them available
as regulators.

Tested on an imx233-olinuxino board.

Signed-off-by: Harald Geyer <redacted>
---
The current regulator API doesn't fit this type of device very well: Typically
consumers will want to set a defined current, ie. min_uA == max_uA, but they
can't without help from configuration data, because the valid values aren't
reported by the API for current regulators. I have been thinking about
extending the API, but currently AFAIK no such consumers exist and most
users, like myself, will force the regulator to a defined value in
devicetree anyway.

 .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
 drivers/iio/adc/Kconfig                            |   1 +
 drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
 3 files changed, 182 insertions(+)

diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
index 555fb11..983952c 100644
--- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
+++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt

@@ -19,6 +19,15 @@ Optional properties:
 - fsl,settling: delay between plate switch to next sample. Allowed value is
                 1 ... 2047. It counts at 2 kHz and its default is
                 10 (= 5 ms)
+- ISRC0: A node describing the regulator of internal current source 0
+- ISRC1: A node describing the regulator of internal current source 1
+
+Required properties for the ISRCx sub-nodes:
+- regulator-max-microamp: See standard regulator binding documentation.
+                          Valid values are from 0 to 300 in steps of 20.
+
+Optional properties for the ISRCx sub-nodes:
+Any standard regulator properties that apply to current regulators.

 Example for i.MX23 SoC:

@@ -31,6 +40,16 @@ Example for i.MX23 SoC:
         fsl,ave-ctrl = <4>;
         fsl,ave-delay = <2>;
         fsl,settling = <10>;
+
+        isrc_lradc0: ISRC0 {
+            regulator-max-microamp = <300>;
+        };
+
+        isrc_lradc1: ISRC1 {
+            regulator-max-microamp = <120>;
+            regulator-min-microamp = <120>;
+            regulator-always-on;
+        };

IMHO the binding should represent the direct connection between the
current source and the physical channel. I think a node label isn't
enough. Unfortunately i don't have an idea to do it the "right way".

I strictly followed the names of the data sheet here: imx23 and imx28
reference manuals both call the current sources ISRC0 and ISRC1 even
if they are attached to different channels of the ADC. I think
inventing some ISRC6 for imx28 would do more harm then good.

So just to check... These two current sources are effectively
pushing current out of the ADC input pin?  Assumption being that
this causes an appropriate voltage across a component between there
and 0V?

I wonder if we are better off not treating these as regulators at
all - whilst you could in theory use them for general purposes, it
would be a bit nuts and sometimes the 'it is nuts argument' is
enough to mean we don't support something :)

I wouldn't have any problem with an additional attribute for the
adc channels - maybe something like...

in_voltage6_sourcecurrent?  Then the control could be pushed to
userspace. 

Or if it only makes sense to do it as a fixed value in DT then
define optional child nodes for the individual channels, with
the relevant ones have current-source-microamp or
something like that as an parameter.

What do you think?

Jonathan

quoted

quoted

     };

 Example for i.MX28 SoC:

@@ -44,4 +63,14 @@ Example for i.MX28 SoC:
         fsl,ave-ctrl = <4>;
         fsl,ave-delay = <2>;
         fsl,settling = <10>;
+
+        isrc_lradc0: ISRC0 {
+            regulator-max-microamp = <300>;
+        };
+
+        isrc_lradc6: ISRC1 {
+            regulator-max-microamp = <120>;
+            regulator-min-microamp = <120>;
+            regulator-always-on;
+        };
     };

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5937030..1968d1c 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig

@@ -319,6 +319,7 @@ config MXS_LRADC
         tristate "Freescale i.MX23/i.MX28 LRADC"
         depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
         depends on INPUT
+        depends on REGULATOR

Any chance to avoid this dependency for an optional feature?

I haven't looked into it yet, but I expect that that the mfd
conversion will avoid this neatly.

Thanks,
Harald

quoted

Stefan

quoted

         select STMP_DEVICE
         select IIO_BUFFER
         select IIO_TRIGGERED_BUFFER