[PATCH v2 0/2] iio: add unit converter

[PATCH v2 0/2] iio: add unit converter

[Peter Rosin]

Hi!

This driver implements support for voltage dividers and current
sense circuits. It's pretty generic and should be easily adaptable
to other linear scaling purposes...

The driver is still named "unit converter", because it was not
clear to me that there was a real problem with the driver being
named that. I got the impression that the naming discussion in
v1 was mainly about the category, and that it kind of looked odd
and non-specific with unit-converter in the DT bindings, but
what do I know?

Cheers,
Peter

Changes since v1:    https://lkml.org/lkml/2018/3/19/801
- Put the driver in the new afe category (Analog Front Ends) and do not
  move the iio-mux driver.
- Do not refer to the source channel as "parent", use "source" instead.
- Have the DT compatible drive the target unit, instead of relying on a
  "type" DT-property for that.
- In the DT bindings, use an unnamed source channel.
- Do not set up writes to _RAW (sorry Phil) as I don't need it and have
  not tested it. It's easy to add back if needed.
- Fail if the source channel does not support _RAW or _SCALE.
- Fix various spelling issues.
- Fix various code style issues.

Peter Rosin (2):
  dt-bindings: iio: afe: add current-sense-cuicuit and voltage-divider
  iio: afe: unit-converter: new driver

 .../bindings/iio/afe/current-sense-circuit.txt     |  45 ++++
 .../bindings/iio/afe/voltage-divider.txt           |  45 ++++
 MAINTAINERS                                        |   8 +
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/afe/Kconfig                            |  18 ++
 drivers/iio/afe/Makefile                           |   6 +
 drivers/iio/afe/iio-unit-converter.c               | 257 +++++++++++++++++++++
 8 files changed, 381 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
 create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
 create mode 100644 drivers/iio/afe/Kconfig
 create mode 100644 drivers/iio/afe/Makefile
 create mode 100644 drivers/iio/afe/iio-unit-converter.c

-- 
2.11.0

[PATCH v2 1/2] dt-bindings: iio: afe: add current-sense-cuicuit and voltage-divider

[Peter Rosin]

An ADC is often used to measure other quantities indirectly. These
bindings describe two cases, a current through a sense resistor, and
a "big" voltage measured with the help of a voltage divider.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../bindings/iio/afe/current-sense-circuit.txt     | 45 ++++++++++++++++++++++
 .../bindings/iio/afe/voltage-divider.txt           | 45 ++++++++++++++++++++++
 MAINTAINERS                                        |  7 ++++
 3 files changed, 97 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
 create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt

diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
new file mode 100644
index 000000000000..0bc7d89387c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
@@ -0,0 +1,45 @@
+Current Sense Curcuit
+=====================
+
+When an io-channel measures the voltage over a current sense resistor,
+the interesting mesaurement is often the current through the resistor,
+not the voltage over it. This binding describes such a current sense
+curcuit.
+
+Required properties:
+- compatible : "current-sense-circuit"
+- io-channels : Channel node of a voltage io-channel.
+
+Optional properties:
+- numerator : The io-channel scale is multiplied by this value (default 1).
+- denominator : The io-channel scale is divided by this value (default 1).
+
+Example:
+The system current is measured by measuring the voltage over a
+3.3 ohms sense resistor.
+
+sysi {
+	compatible = "current-sense-circuit";
+	io-channels = <&tiadc 0>;
+
+	/* Divide the ADC voltage by 33/10 (i.e. 3.3) to get the current. */
+	numerator = <10>;
+	denominator = <33>;
+};
+
+&i2c {
+	tiadc: adc@48 {
+		compatible = "ti,ads1015";
+	      	reg = <0x48>;
+		#io-channel-cells = <1>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		channel@0 { /* IN0,IN1 differential */
+			reg = <0>;
+			ti,gain = <1>;
+			ti,datarate = <4>;
+		};
+	};
+};
diff --git a/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
new file mode 100644
index 000000000000..fd4a215d9e6d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
@@ -0,0 +1,45 @@
+Voltage divider
+===============
+
+When an io-channel measures the midpoint of a voltage divider, the
+interesting voltage is often the voltage over the full resistance
+of the divider. This binding describes the voltage divider in such
+a curcuit.
+
+Required properties:
+- compatible : "voltage-divider"
+- io-channels : Channel node of a voltage io-channel.
+
+Optional properties:
+- numerator : The io-channel scale is multiplied by this value (default 1).
+- denominator : The io-channel scale is divided by this value (default 1).
+
+Example:
+The system voltage is circa 12V, but divided down with a 22/200
+voltage divider to adjust it to the ADC range.
+
+SYSV        ADC       GND
+  +          +         +
+  |  .-----. | .----.  |
+  '--| 200 |-+-| 22 |--'
+     '-----'   '----'
+
+sysv {
+	compatible = "voltage-divider";
+	io-channels = <&maxadc 1>;
+
+	/* Multiply the ADC voltage by 222/22 to get the system voltage. */
+	numerator = <222>; /* 200 + 22 */
+	denominator = <22>;
+};
+
+&spi {
+	maxadc: adc@0 {
+		compatible = "maxim,max1027";
+	      	reg = <0>;
+		#io-channel-cells = <1>;
+		interrupt-parent = <&gpio5>;
+		interrupts = <15 IRQ_TYPE_EDGE_RISING>;
+		spi-max-frequency = <1000000>;
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 36a28e979e9a..9dbe5019c6bd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6889,6 +6889,13 @@ F:	drivers/staging/iio/
 F:	include/linux/iio/
 F:	tools/iio/
 
+IIO UNIT CONVERTER
+M:	Peter Rosin <peda@axentia.se>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
+F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
+
 IKANOS/ADI EAGLE ADSL USB DRIVER
 M:	Matthieu Castet <castet.matthieu@free.fr>
 M:	Stanislaw Gruszka <stf_xl@wp.pl>
-- 
2.11.0

[PATCH v2 2/2] iio: afe: unit-converter: new driver

[Peter Rosin]

If an ADC channel measures the midpoint of a voltage divider, the
interesting voltage is often the voltage over the full resistance.
E.g. if the full voltage is too big for the ADC to handle.
Likewise, if an ADC channel measures the voltage across a resistor,
the interesting value is often the current through the resistor.

This driver solves both problems by allowing to linearly scale a channel
and by allowing changes to the type of the channel. Or both.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 MAINTAINERS                          |   1 +
 drivers/iio/Kconfig                  |   1 +
 drivers/iio/Makefile                 |   1 +
 drivers/iio/afe/Kconfig              |  18 +++
 drivers/iio/afe/Makefile             |   6 +
 drivers/iio/afe/iio-unit-converter.c | 257 +++++++++++++++++++++++++++++++++++
 6 files changed, 284 insertions(+)
 create mode 100644 drivers/iio/afe/Kconfig
 create mode 100644 drivers/iio/afe/Makefile
 create mode 100644 drivers/iio/afe/iio-unit-converter.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9dbe5019c6bd..f9835521eec6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6895,6 +6895,7 @@ L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
 F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
+F:	drivers/iio/afe/iio-unit-converter.c
 
 IKANOS/ADI EAGLE ADSL USB DRIVER
 M:	Matthieu Castet <castet.matthieu@free.fr>
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index b3c8c6ef0dff..d69e85a8bdc3 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -70,6 +70,7 @@ config IIO_TRIGGERED_EVENT
 
 source "drivers/iio/accel/Kconfig"
 source "drivers/iio/adc/Kconfig"
+source "drivers/iio/afe/Kconfig"
 source "drivers/iio/amplifiers/Kconfig"
 source "drivers/iio/chemical/Kconfig"
 source "drivers/iio/common/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index b16b2e9ddc40..d8cba9c229c0 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_IIO_TRIGGERED_EVENT) += industrialio-triggered-event.o
 
 obj-y += accel/
 obj-y += adc/
+obj-y += afe/
 obj-y += amplifiers/
 obj-y += buffer/
 obj-y += chemical/
diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
new file mode 100644
index 000000000000..75acbe7eed15
--- /dev/null
+++ b/drivers/iio/afe/Kconfig
@@ -0,0 +1,18 @@
+#
+# Analog Front End drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Analog Front Ends"
+
+config IIO_UNIT_CONVERTER
+	tristate "IIO unit converter"
+	depends on OF || COMPILE_TEST
+	help
+	  Say yes here to build support for the IIO unit converter
+	  that handles voltage dividers and current sense circuits.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called iio-unit-converter.
+
+endmenu
diff --git a/drivers/iio/afe/Makefile b/drivers/iio/afe/Makefile
new file mode 100644
index 000000000000..7691cc5b809a
--- /dev/null
+++ b/drivers/iio/afe/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for industrial I/O Analog Front Ends (AFE)
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_IIO_UNIT_CONVERTER) += iio-unit-converter.o
diff --git a/drivers/iio/afe/iio-unit-converter.c b/drivers/iio/afe/iio-unit-converter.c
new file mode 100644
index 000000000000..43429543cc29
--- /dev/null
+++ b/drivers/iio/afe/iio-unit-converter.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IIO unit converter
+ *
+ * Copyright (C) 2018 Axentia Technologies AB
+ *
+ * Author: Peter Rosin <peda@axentia.se>
+ */
+
+#include <linux/err.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+struct unit_converter_cfg {
+	enum iio_chan_type type;
+};
+
+enum unit_converter_variant {
+	CURRENT_SENSE_CIRCUIT,
+	VOLTAGE_DIVIDER,
+};
+
+static const struct unit_converter_cfg unit_converter_cfg[] = {
+	[CURRENT_SENSE_CIRCUIT] = {
+		.type = IIO_CURRENT,
+	},
+	[VOLTAGE_DIVIDER] = {
+		.type = IIO_VOLTAGE,
+	},
+};
+
+struct unit_converter {
+	const struct unit_converter_cfg *cfg;
+	struct iio_channel *source;
+	struct iio_dev *indio_dev;
+	struct iio_chan_spec chan;
+	struct iio_chan_spec_ext_info *ext_info;
+	s32 numerator;
+	s32 denominator;
+};
+
+static int unit_converter_read_raw(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan,
+				   int *val, int *val2, long mask)
+{
+	struct unit_converter *uc = iio_priv(indio_dev);
+	unsigned long long tmp;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return iio_read_channel_raw(uc->source, val);
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = iio_read_channel_scale(uc->source, val, val2);
+		switch (ret) {
+		case IIO_VAL_FRACTIONAL:
+			*val *= uc->numerator;
+			*val2 *= uc->denominator;
+			return ret;
+		case IIO_VAL_INT:
+			*val *= uc->numerator;
+			if (uc->denominator == 1)
+				return ret;
+			*val2 = uc->denominator;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_VAL_FRACTIONAL_LOG2:
+			tmp = *val * 1000000000LL;
+			do_div(tmp, uc->denominator);
+			tmp *= uc->numerator;
+			do_div(tmp, 1000000000LL);
+			*val = tmp;
+			return ret;
+		default:
+			return -EOPNOTSUPP;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int unit_converter_read_avail(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan,
+				     const int **vals, int *type, int *length,
+				     long mask)
+{
+	struct unit_converter *uc = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*type = IIO_VAL_INT;
+		return iio_read_avail_channel_raw(uc->source, vals, length);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info unit_converter_info = {
+	.read_raw = unit_converter_read_raw,
+	.read_avail = unit_converter_read_avail,
+};
+
+static ssize_t unit_converter_read_ext_info(struct iio_dev *indio_dev,
+					    uintptr_t private,
+					    struct iio_chan_spec const *chan,
+					    char *buf)
+{
+	struct unit_converter *uc = iio_priv(indio_dev);
+
+	return iio_read_channel_ext_info(uc->source,
+					 uc->ext_info[private].name,
+					 buf);
+}
+
+static ssize_t unit_converter_write_ext_info(struct iio_dev *indio_dev,
+					     uintptr_t private,
+					     struct iio_chan_spec const *chan,
+					     const char *buf, size_t len)
+{
+	struct unit_converter *uc = iio_priv(indio_dev);
+
+	return iio_write_channel_ext_info(uc->source,
+					  uc->ext_info[private].name,
+					  buf, len);
+}
+
+static int unit_converter_configure_channel(struct device *dev,
+					    struct unit_converter *uc)
+{
+	struct iio_chan_spec *chan = &uc->chan;
+	struct iio_chan_spec const *schan = uc->source->channel;
+
+	chan->indexed = 1;
+	chan->output = schan->output;
+	chan->ext_info = uc->ext_info;
+	chan->type = uc->cfg->type;
+
+	if (!iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
+	    !iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) {
+		dev_err(dev, "source channel does not support raw/scale\n");
+		return -EINVAL;
+	}
+
+	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_SCALE);
+
+	if (iio_channel_has_available(schan, IIO_CHAN_INFO_RAW))
+		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
+
+	return 0;
+}
+
+static const struct of_device_id unit_converter_match[] = {
+	{ .compatible = "current-sense-circuit",
+	  .data = &unit_converter_cfg[CURRENT_SENSE_CIRCUIT], },
+	{ .compatible = "voltage-divider",
+	  .data = &unit_converter_cfg[VOLTAGE_DIVIDER], },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, unit_converter_match);
+
+static int unit_converter_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct iio_dev *indio_dev;
+	struct iio_channel *source;
+	struct unit_converter *uc;
+	int sizeof_ext_info;
+	int sizeof_priv;
+	int i;
+	int ret;
+
+	source = devm_iio_channel_get(dev, NULL);
+	if (IS_ERR(source)) {
+		if (PTR_ERR(source) != -EPROBE_DEFER)
+			dev_err(dev, "failed to get source channel\n");
+		return PTR_ERR(source);
+	}
+
+	sizeof_ext_info = iio_get_channel_ext_info_count(source);
+	if (sizeof_ext_info) {
+		sizeof_ext_info += 1; /* one extra entry for the sentinel */
+		sizeof_ext_info *= sizeof(*uc->ext_info);
+	}
+
+	sizeof_priv = sizeof(*uc) + sizeof_ext_info;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof_priv);
+	if (!indio_dev)
+		return -ENOMEM;
+
+	uc = iio_priv(indio_dev);
+
+	uc->cfg = of_device_get_match_data(dev);
+	uc->numerator = 1;
+	uc->denominator = 1;
+	device_property_read_u32(dev, "numerator", &uc->numerator);
+	device_property_read_u32(dev, "denominator", &uc->denominator);
+	if (!uc->numerator || !uc->denominator) {
+		dev_err(dev, "invalid scaling factor.\n");
+		return -EINVAL;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	uc->source = source;
+
+	indio_dev->name = dev_name(dev);
+	indio_dev->dev.parent = dev;
+	indio_dev->info = &unit_converter_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = &uc->chan;
+	indio_dev->num_channels = 1;
+	if (sizeof_ext_info) {
+		uc->ext_info = devm_kmemdup(dev,
+					    source->channel->ext_info,
+					    sizeof_ext_info, GFP_KERNEL);
+		if (!uc->ext_info)
+			return -ENOMEM;
+
+		for (i = 0; uc->ext_info[i].name; ++i) {
+			if (source->channel->ext_info[i].read)
+				uc->ext_info[i].read = unit_converter_read_ext_info;
+			if (source->channel->ext_info[i].write)
+				uc->ext_info[i].write = unit_converter_write_ext_info;
+			uc->ext_info[i].private = i;
+		}
+	}
+
+	ret = unit_converter_configure_channel(dev, uc);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		dev_err(dev, "failed to register iio device\n");
+
+	return ret;
+}
+
+static struct platform_driver unit_converter_driver = {
+	.probe = unit_converter_probe,
+	.driver = {
+		.name = "iio-unit-converter",
+		.of_match_table = unit_converter_match,
+	},
+};
+module_platform_driver(unit_converter_driver);
+
+MODULE_DESCRIPTION("IIO unit converter driver");
+MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH v2 0/2] iio: add unit converter

[Andrew F. Davis]

On 04/03/2018 10:36 AM, Peter Rosin wrote:
> Hi!
> 
> This driver implements support for voltage dividers and current
> sense circuits. It's pretty generic and should be easily adaptable
> to other linear scaling purposes...
> 

I really like this idea, defining channel scaling / channel type
conversion in DT will be very useful. So much so that I would recommend
this not be a use specific driver but instead moved into the IIO core.

This would allow defining these channel conversions in the device node
itself, so as to not need a separate node just for the converter (the
conversion is not a device and probably should not have it's own node
anyway). It would also help with enabling this support for buffered
readers/writers and not just devices only supporting _raw reads/writes.

Andrew

> The driver is still named "unit converter", because it was not
> clear to me that there was a real problem with the driver being
> named that. I got the impression that the naming discussion in
> v1 was mainly about the category, and that it kind of looked odd
> and non-specific with unit-converter in the DT bindings, but
> what do I know?
> 
> Cheers,
> Peter
> 
> Changes since v1:    https://lkml.org/lkml/2018/3/19/801
> - Put the driver in the new afe category (Analog Front Ends) and do not
>   move the iio-mux driver.
> - Do not refer to the source channel as "parent", use "source" instead.
> - Have the DT compatible drive the target unit, instead of relying on a
>   "type" DT-property for that.
> - In the DT bindings, use an unnamed source channel.
> - Do not set up writes to _RAW (sorry Phil) as I don't need it and have
>   not tested it. It's easy to add back if needed.
> - Fail if the source channel does not support _RAW or _SCALE.
> - Fix various spelling issues.
> - Fix various code style issues.
> 
> Peter Rosin (2):
>   dt-bindings: iio: afe: add current-sense-cuicuit and voltage-divider
>   iio: afe: unit-converter: new driver
> 
>  .../bindings/iio/afe/current-sense-circuit.txt     |  45 ++++
>  .../bindings/iio/afe/voltage-divider.txt           |  45 ++++
>  MAINTAINERS                                        |   8 +
>  drivers/iio/Kconfig                                |   1 +
>  drivers/iio/Makefile                               |   1 +
>  drivers/iio/afe/Kconfig                            |  18 ++
>  drivers/iio/afe/Makefile                           |   6 +
>  drivers/iio/afe/iio-unit-converter.c               | 257 +++++++++++++++++++++
>  8 files changed, 381 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
>  create mode 100644 drivers/iio/afe/Kconfig
>  create mode 100644 drivers/iio/afe/Makefile
>  create mode 100644 drivers/iio/afe/iio-unit-converter.c
> 

Re: [PATCH v2 0/2] iio: add unit converter

[Peter Rosin]

On 2018-04-03 19:41, Andrew F. Davis wrote:
> On 04/03/2018 10:36 AM, Peter Rosin wrote:
>> Hi!
>>
>> This driver implements support for voltage dividers and current
>> sense circuits. It's pretty generic and should be easily adaptable
>> to other linear scaling purposes...
>>
> 
> I really like this idea, defining channel scaling / channel type
> conversion in DT will be very useful. So much so that I would recommend
> this not be a use specific driver but instead moved into the IIO core.
> 
> This would allow defining these channel conversions in the device node
> itself, so as to not need a separate node just for the converter (the
> conversion is not a device and probably should not have it's own node
> anyway). It would also help with enabling this support for buffered
> readers/writers and not just devices only supporting _raw reads/writes.

In my case, the voltage dividers and the current sense circuit are very
much real, and I can point my finger at them on the board. Sure, they
are not ICs, but to not call them devices is wrong IMHO, and the ADC
which is involved have very little to do with the voltage dividers and
the fact that it is involved in sensing current. This is not an argument
for not moving the functionally to the core though, but that has some
problems AFAICT. Because you need some kind of clever and generic
binding to make the core do its thing, and it might not be easy to come
up with something that fits all devices? And if we do put this in the
core, that opens the door for more complex unit converters later on
(non-linear etc), further complicating the generic bindings.

That said, I'm obviously biased since I want this to get in sooner
rather than later...

Cheers,
Peter

Re: [PATCH v2 0/2] iio: add unit converter

[Andrew F. Davis]

On 04/03/2018 01:09 PM, Peter Rosin wrote:
> On 2018-04-03 19:41, Andrew F. Davis wrote:
>> On 04/03/2018 10:36 AM, Peter Rosin wrote:
>>> Hi!
>>>
>>> This driver implements support for voltage dividers and current
>>> sense circuits. It's pretty generic and should be easily adaptable
>>> to other linear scaling purposes...
>>>
>>
>> I really like this idea, defining channel scaling / channel type
>> conversion in DT will be very useful. So much so that I would recommend
>> this not be a use specific driver but instead moved into the IIO core.
>>
>> This would allow defining these channel conversions in the device node
>> itself, so as to not need a separate node just for the converter (the
>> conversion is not a device and probably should not have it's own node
>> anyway). It would also help with enabling this support for buffered
>> readers/writers and not just devices only supporting _raw reads/writes.
> 
> In my case, the voltage dividers and the current sense circuit are very
> much real, and I can point my finger at them on the board. Sure, they
> are not ICs, but to not call them devices is wrong IMHO, and the ADC
> which is involved have very little to do with the voltage dividers and
> the fact that it is involved in sensing current. 


I would argue support resistors are still part of the "current sensing
device", even if they are physically external to the ADC IC. As you say
though, it doesn't really matter that much to this discussion.


> This is not an argument
> for not moving the functionally to the core though, but that has some
> problems AFAICT. Because you need some kind of clever and generic
> binding to make the core do its thing, and it might not be easy to come
> up with something that fits all devices? And if we do put this in the
> core, that opens the door for more complex unit converters later on
> (non-linear etc), further complicating the generic bindings.
> 


For the simple case of linear scaling of the channel input/output, and
type conversion I don't see much problem. They will not need to be
extended, new bindings can be added for more complex cases. When more
complex cases show up additional bindings will be needed either way.


> That said, I'm obviously biased since I want this to get in sooner
> rather than later...
> 


Adding this functionality to the IIO core doesn't really block this
patch-set, it will just make this patch-set unneeded in the future
if/when support goes into the core..

Andrew


> Cheers,
> Peter
> 

Re: [PATCH v2 1/2] dt-bindings: iio: afe: add current-sense-cuicuit and voltage-divider

[Jonathan Cameron]

On Tue,  3 Apr 2018 17:36:34 +0200
Peter Rosin <peda@axentia.se> wrote:

circuit in the patch title is spelled wrong.

> An ADC is often used to measure other quantities indirectly. These
> bindings describe two cases, a current through a sense resistor, and
> a "big" voltage measured with the help of a voltage divider.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Will definitely be wanting wide opinions on this one - Rob in particularly
for the binding side.

One comment inline. What we have here is nice and generic, but is
it what would be 'expected' for current sense circuit?  Should we
also be more specific in the naming.  There are lots of options for
current sense circuits and this is just the simplest (current loop
for example).

One option would be to use current-sense-shunt perhaps?

https://en.wikipedia.org/wiki/Current_sensing_techniques
Gives a few that I didn't think of beyond current loops etc.


> ---
>  .../bindings/iio/afe/current-sense-circuit.txt     | 45 ++++++++++++++++++++++
>  .../bindings/iio/afe/voltage-divider.txt           | 45 ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 ++++
>  3 files changed, 97 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> new file mode 100644
> index 000000000000..0bc7d89387c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> @@ -0,0 +1,45 @@
> +Current Sense Curcuit
> +=====================
> +
> +When an io-channel measures the voltage over a current sense resistor,
> +the interesting mesaurement is often the current through the resistor,
> +not the voltage over it. This binding describes such a current sense
> +curcuit.
> +
> +Required properties:
> +- compatible : "current-sense-circuit"
> +- io-channels : Channel node of a voltage io-channel.
> +
> +Optional properties:
> +- numerator : The io-channel scale is multiplied by this value (default 1).
> +- denominator : The io-channel scale is divided by this value (default 1).
> +
> +Example:
> +The system current is measured by measuring the voltage over a
> +3.3 ohms sense resistor.

Hmm. It sort of feels like the binding doesn't really reflect the
hardware as directly as it might.   Should we be explicitly having the
resistance in this case?  (with some more mapping logic needed in the
driver to figure out the scaling this causes).

> +
> +sysi {
> +	compatible = "current-sense-circuit";
> +	io-channels = <&tiadc 0>;
> +
> +	/* Divide the ADC voltage by 33/10 (i.e. 3.3) to get the current. */
> +	numerator = <10>;
> +	denominator = <33>;
> +};
> +
> +&i2c {
> +	tiadc: adc@48 {
> +		compatible = "ti,ads1015";
> +	      	reg = <0x48>;
> +		#io-channel-cells = <1>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		channel@0 { /* IN0,IN1 differential */
> +			reg = <0>;
> +			ti,gain = <1>;
> +			ti,datarate = <4>;
> +		};
> +	};
> +};
> diff --git a/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> new file mode 100644
> index 000000000000..fd4a215d9e6d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> @@ -0,0 +1,45 @@
> +Voltage divider
> +===============
> +
> +When an io-channel measures the midpoint of a voltage divider, the
> +interesting voltage is often the voltage over the full resistance
> +of the divider. This binding describes the voltage divider in such
> +a curcuit.
> +
> +Required properties:
> +- compatible : "voltage-divider"
> +- io-channels : Channel node of a voltage io-channel.
> +
> +Optional properties:
> +- numerator : The io-channel scale is multiplied by this value (default 1).
> +- denominator : The io-channel scale is divided by this value (default 1).
> +
> +Example:
> +The system voltage is circa 12V, but divided down with a 22/200
> +voltage divider to adjust it to the ADC range.
> +
> +SYSV        ADC       GND
> +  +          +         +
> +  |  .-----. | .----.  |
> +  '--| 200 |-+-| 22 |--'
> +     '-----'   '----'
> +
> +sysv {
> +	compatible = "voltage-divider";
> +	io-channels = <&maxadc 1>;
> +
> +	/* Multiply the ADC voltage by 222/22 to get the system voltage. */
> +	numerator = <222>; /* 200 + 22 */
> +	denominator = <22>;
> +};
> +
> +&spi {
> +	maxadc: adc@0 {
> +		compatible = "maxim,max1027";
> +	      	reg = <0>;
> +		#io-channel-cells = <1>;
> +		interrupt-parent = <&gpio5>;
> +		interrupts = <15 IRQ_TYPE_EDGE_RISING>;
> +		spi-max-frequency = <1000000>;
> +	};
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 36a28e979e9a..9dbe5019c6bd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6889,6 +6889,13 @@ F:	drivers/staging/iio/
>  F:	include/linux/iio/
>  F:	tools/iio/
>  
> +IIO UNIT CONVERTER
> +M:	Peter Rosin <peda@axentia.se>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> +F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> +
>  IKANOS/ADI EAGLE ADSL USB DRIVER
>  M:	Matthieu Castet <castet.matthieu@free.fr>
>  M:	Stanislaw Gruszka <stf_xl@wp.pl>

Re: [PATCH v2 2/2] iio: afe: unit-converter: new driver

[Jonathan Cameron]

On Tue,  3 Apr 2018 17:36:35 +0200
Peter Rosin <peda@axentia.se> wrote:

> If an ADC channel measures the midpoint of a voltage divider, the
> interesting voltage is often the voltage over the full resistance.
> E.g. if the full voltage is too big for the ADC to handle.
> Likewise, if an ADC channel measures the voltage across a resistor,
> the interesting value is often the current through the resistor.
> 
> This driver solves both problems by allowing to linearly scale a channel
> and by allowing changes to the type of the channel. Or both.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
One passing comment inline but nothing that really matters..

Looks good to me and I'll be happy to take it once we are
sure everyone is happy with the devicetree bindings.

Thanks,

Jonathan

> ---
>  MAINTAINERS                          |   1 +
>  drivers/iio/Kconfig                  |   1 +
>  drivers/iio/Makefile                 |   1 +
>  drivers/iio/afe/Kconfig              |  18 +++
>  drivers/iio/afe/Makefile             |   6 +
>  drivers/iio/afe/iio-unit-converter.c | 257 +++++++++++++++++++++++++++++++++++
>  6 files changed, 284 insertions(+)
>  create mode 100644 drivers/iio/afe/Kconfig
>  create mode 100644 drivers/iio/afe/Makefile
>  create mode 100644 drivers/iio/afe/iio-unit-converter.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9dbe5019c6bd..f9835521eec6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6895,6 +6895,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
>  F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> +F:	drivers/iio/afe/iio-unit-converter.c
>  
>  IKANOS/ADI EAGLE ADSL USB DRIVER
>  M:	Matthieu Castet <castet.matthieu@free.fr>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index b3c8c6ef0dff..d69e85a8bdc3 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -70,6 +70,7 @@ config IIO_TRIGGERED_EVENT
>  
>  source "drivers/iio/accel/Kconfig"
>  source "drivers/iio/adc/Kconfig"
> +source "drivers/iio/afe/Kconfig"
>  source "drivers/iio/amplifiers/Kconfig"
>  source "drivers/iio/chemical/Kconfig"
>  source "drivers/iio/common/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index b16b2e9ddc40..d8cba9c229c0 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_IIO_TRIGGERED_EVENT) += industrialio-triggered-event.o
>  
>  obj-y += accel/
>  obj-y += adc/
> +obj-y += afe/
>  obj-y += amplifiers/
>  obj-y += buffer/
>  obj-y += chemical/
> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
> new file mode 100644
> index 000000000000..75acbe7eed15
> --- /dev/null
> +++ b/drivers/iio/afe/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# Analog Front End drivers
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Analog Front Ends"
> +
> +config IIO_UNIT_CONVERTER
> +	tristate "IIO unit converter"
> +	depends on OF || COMPILE_TEST
> +	help
> +	  Say yes here to build support for the IIO unit converter
> +	  that handles voltage dividers and current sense circuits.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called iio-unit-converter.
> +
> +endmenu
> diff --git a/drivers/iio/afe/Makefile b/drivers/iio/afe/Makefile
> new file mode 100644
> index 000000000000..7691cc5b809a
> --- /dev/null
> +++ b/drivers/iio/afe/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for industrial I/O Analog Front Ends (AFE)
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_IIO_UNIT_CONVERTER) += iio-unit-converter.o
> diff --git a/drivers/iio/afe/iio-unit-converter.c b/drivers/iio/afe/iio-unit-converter.c
> new file mode 100644
> index 000000000000..43429543cc29
> --- /dev/null
> +++ b/drivers/iio/afe/iio-unit-converter.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IIO unit converter
> + *
> + * Copyright (C) 2018 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +struct unit_converter_cfg {
> +	enum iio_chan_type type;
> +};
> +
> +enum unit_converter_variant {
> +	CURRENT_SENSE_CIRCUIT,
> +	VOLTAGE_DIVIDER,
> +};
> +
> +static const struct unit_converter_cfg unit_converter_cfg[] = {
> +	[CURRENT_SENSE_CIRCUIT] = {
> +		.type = IIO_CURRENT,
> +	},
> +	[VOLTAGE_DIVIDER] = {
> +		.type = IIO_VOLTAGE,
> +	},
> +};
> +
> +struct unit_converter {
> +	const struct unit_converter_cfg *cfg;
> +	struct iio_channel *source;
> +	struct iio_dev *indio_dev;
> +	struct iio_chan_spec chan;
> +	struct iio_chan_spec_ext_info *ext_info;
> +	s32 numerator;
> +	s32 denominator;
> +};
> +
> +static int unit_converter_read_raw(struct iio_dev *indio_dev,
> +				   struct iio_chan_spec const *chan,
> +				   int *val, int *val2, long mask)
> +{
> +	struct unit_converter *uc = iio_priv(indio_dev);
> +	unsigned long long tmp;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return iio_read_channel_raw(uc->source, val);
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = iio_read_channel_scale(uc->source, val, val2);
> +		switch (ret) {
> +		case IIO_VAL_FRACTIONAL:
> +			*val *= uc->numerator;
> +			*val2 *= uc->denominator;
> +			return ret;
> +		case IIO_VAL_INT:
> +			*val *= uc->numerator;
> +			if (uc->denominator == 1)
> +				return ret;
> +			*val2 = uc->denominator;
> +			return IIO_VAL_FRACTIONAL;
> +		case IIO_VAL_FRACTIONAL_LOG2:
> +			tmp = *val * 1000000000LL;
> +			do_div(tmp, uc->denominator);
> +			tmp *= uc->numerator;
> +			do_div(tmp, 1000000000LL);
> +			*val = tmp;
> +			return ret;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int unit_converter_read_avail(struct iio_dev *indio_dev,
> +				     struct iio_chan_spec const *chan,
> +				     const int **vals, int *type, int *length,
> +				     long mask)
> +{
> +	struct unit_converter *uc = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*type = IIO_VAL_INT;
> +		return iio_read_avail_channel_raw(uc->source, vals, length);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info unit_converter_info = {
> +	.read_raw = unit_converter_read_raw,
> +	.read_avail = unit_converter_read_avail,
> +};
> +
> +static ssize_t unit_converter_read_ext_info(struct iio_dev *indio_dev,
> +					    uintptr_t private,
> +					    struct iio_chan_spec const *chan,
> +					    char *buf)
> +{
> +	struct unit_converter *uc = iio_priv(indio_dev);
> +
> +	return iio_read_channel_ext_info(uc->source,
> +					 uc->ext_info[private].name,
> +					 buf);
> +}
> +
> +static ssize_t unit_converter_write_ext_info(struct iio_dev *indio_dev,
> +					     uintptr_t private,
> +					     struct iio_chan_spec const *chan,
> +					     const char *buf, size_t len)
> +{
> +	struct unit_converter *uc = iio_priv(indio_dev);
> +
> +	return iio_write_channel_ext_info(uc->source,
> +					  uc->ext_info[private].name,
> +					  buf, len);
> +}
> +
> +static int unit_converter_configure_channel(struct device *dev,
> +					    struct unit_converter *uc)
> +{
> +	struct iio_chan_spec *chan = &uc->chan;
> +	struct iio_chan_spec const *schan = uc->source->channel;
> +
> +	chan->indexed = 1;
> +	chan->output = schan->output;
> +	chan->ext_info = uc->ext_info;
> +	chan->type = uc->cfg->type;
> +
> +	if (!iio_channel_has_info(schan, IIO_CHAN_INFO_RAW) ||
> +	    !iio_channel_has_info(schan, IIO_CHAN_INFO_SCALE)) {
> +		dev_err(dev, "source channel does not support raw/scale\n");
> +		return -EINVAL;
> +	}
> +
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +		BIT(IIO_CHAN_INFO_SCALE);
> +
> +	if (iio_channel_has_available(schan, IIO_CHAN_INFO_RAW))
> +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id unit_converter_match[] = {
> +	{ .compatible = "current-sense-circuit",
> +	  .data = &unit_converter_cfg[CURRENT_SENSE_CIRCUIT], },
> +	{ .compatible = "voltage-divider",
> +	  .data = &unit_converter_cfg[VOLTAGE_DIVIDER], },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, unit_converter_match);
> +
> +static int unit_converter_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct iio_channel *source;
> +	struct unit_converter *uc;
> +	int sizeof_ext_info;
> +	int sizeof_priv;
> +	int i;
> +	int ret;
> +
> +	source = devm_iio_channel_get(dev, NULL);
> +	if (IS_ERR(source)) {
> +		if (PTR_ERR(source) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get source channel\n");
> +		return PTR_ERR(source);
> +	}
> +
> +	sizeof_ext_info = iio_get_channel_ext_info_count(source);
> +	if (sizeof_ext_info) {
> +		sizeof_ext_info += 1; /* one extra entry for the sentinel */
> +		sizeof_ext_info *= sizeof(*uc->ext_info);
> +	}
> +
> +	sizeof_priv = sizeof(*uc) + sizeof_ext_info;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof_priv);
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	uc = iio_priv(indio_dev);
> +
> +	uc->cfg = of_device_get_match_data(dev);
> +	uc->numerator = 1;
> +	uc->denominator = 1;
> +	device_property_read_u32(dev, "numerator", &uc->numerator);
> +	device_property_read_u32(dev, "denominator", &uc->denominator);
> +	if (!uc->numerator || !uc->denominator) {
> +		dev_err(dev, "invalid scaling factor.\n");
> +		return -EINVAL;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	uc->source = source;
> +
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &unit_converter_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &uc->chan;
> +	indio_dev->num_channels = 1;
> +	if (sizeof_ext_info) {
> +		uc->ext_info = devm_kmemdup(dev,
> +					    source->channel->ext_info,
> +					    sizeof_ext_info, GFP_KERNEL);
> +		if (!uc->ext_info)
> +			return -ENOMEM;
> +
> +		for (i = 0; uc->ext_info[i].name; ++i) {
> +			if (source->channel->ext_info[i].read)
> +				uc->ext_info[i].read = unit_converter_read_ext_info;
> +			if (source->channel->ext_info[i].write)
> +				uc->ext_info[i].write = unit_converter_write_ext_info;
> +			uc->ext_info[i].private = i;
> +		}
> +	}
> +
> +	ret = unit_converter_configure_channel(dev, uc);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		dev_err(dev, "failed to register iio device\n");
This made me check if we already report errors if registration fails.
We do in a lot of cases but not quite all.

It might make sense to improve that error reporting in the core and
drop it in any drivers.

I don't care that strongly about it though...

> +
> +	return ret;
> +}
> +
> +static struct platform_driver unit_converter_driver = {
> +	.probe = unit_converter_probe,
> +	.driver = {
> +		.name = "iio-unit-converter",
> +		.of_match_table = unit_converter_match,
> +	},
> +};
> +module_platform_driver(unit_converter_driver);
> +
> +MODULE_DESCRIPTION("IIO unit converter driver");
> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v2 1/2] dt-bindings: iio: afe: add current-sense-cuicuit and voltage-divider

[Fabio Estevam]

On Tue, Apr 3, 2018 at 12:36 PM, Peter Rosin <peda@axentia.se> wrote:

> diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> new file mode 100644
> index 000000000000..0bc7d89387c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> @@ -0,0 +1,45 @@
> +Current Sense Curcuit

s/Curcuit/Circuit

> +=====================
> +
> +When an io-channel measures the voltage over a current sense resistor,
> +the interesting mesaurement is often the current through the resistor,
> +not the voltage over it. This binding describes such a current sense
> +curcuit.

s/curcuit/circuit

Re: [PATCH v2 1/2] dt-bindings: iio: afe: add current-sense-cuicuit and voltage-divider

[Peter Rosin]

On 2018-04-08 18:50, Jonathan Cameron wrote:
> On Tue,  3 Apr 2018 17:36:34 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
> circuit in the patch title is spelled wrong.
> 
>> An ADC is often used to measure other quantities indirectly. These
>> bindings describe two cases, a current through a sense resistor, and
>> a "big" voltage measured with the help of a voltage divider.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> 
> Will definitely be wanting wide opinions on this one - Rob in particularly
> for the binding side.
> 
> One comment inline. What we have here is nice and generic, but is
> it what would be 'expected' for current sense circuit?  Should we
> also be more specific in the naming.  There are lots of options for
> current sense circuits and this is just the simplest (current loop
> for example).
> 
> One option would be to use current-sense-shunt perhaps?

Heh, I'm obviously happy to not use anything "circuit", because I fail
to type it correctly most of the time. I don't know how many instances
I fixed before sending and there were still a few lingering. Anyway,
I have fixed those locally and I have also locally removed the error
message on registration failure in patch 2/2 so that will be in v3.

But I'm holding off that v3, since I expect more input on the naming.

> https://en.wikipedia.org/wiki/Current_sensing_techniques
> Gives a few that I didn't think of beyond current loops etc.

I'll give it a read.

Cheers,
Peter

>> ---
>>  .../bindings/iio/afe/current-sense-circuit.txt     | 45 ++++++++++++++++++++++
>>  .../bindings/iio/afe/voltage-divider.txt           | 45 ++++++++++++++++++++++
>>  MAINTAINERS                                        |  7 ++++
>>  3 files changed, 97 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
>>  create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
>> new file mode 100644
>> index 000000000000..0bc7d89387c0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
>> @@ -0,0 +1,45 @@
>> +Current Sense Curcuit
>> +=====================
>> +
>> +When an io-channel measures the voltage over a current sense resistor,
>> +the interesting mesaurement is often the current through the resistor,
>> +not the voltage over it. This binding describes such a current sense
>> +curcuit.
>> +
>> +Required properties:
>> +- compatible : "current-sense-circuit"
>> +- io-channels : Channel node of a voltage io-channel.
>> +
>> +Optional properties:
>> +- numerator : The io-channel scale is multiplied by this value (default 1).
>> +- denominator : The io-channel scale is divided by this value (default 1).
>> +
>> +Example:
>> +The system current is measured by measuring the voltage over a
>> +3.3 ohms sense resistor.
> 
> Hmm. It sort of feels like the binding doesn't really reflect the
> hardware as directly as it might.   Should we be explicitly having the
> resistance in this case?  (with some more mapping logic needed in the
> driver to figure out the scaling this causes).
> 
>> +
>> +sysi {
>> +	compatible = "current-sense-circuit";
>> +	io-channels = <&tiadc 0>;
>> +
>> +	/* Divide the ADC voltage by 33/10 (i.e. 3.3) to get the current. */
>> +	numerator = <10>;
>> +	denominator = <33>;
>> +};
>> +
>> +&i2c {
>> +	tiadc: adc@48 {
>> +		compatible = "ti,ads1015";
>> +	      	reg = <0x48>;
>> +		#io-channel-cells = <1>;
>> +
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		channel@0 { /* IN0,IN1 differential */
>> +			reg = <0>;
>> +			ti,gain = <1>;
>> +			ti,datarate = <4>;
>> +		};
>> +	};
>> +};
>> diff --git a/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
>> new file mode 100644
>> index 000000000000..fd4a215d9e6d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
>> @@ -0,0 +1,45 @@
>> +Voltage divider
>> +===============
>> +
>> +When an io-channel measures the midpoint of a voltage divider, the
>> +interesting voltage is often the voltage over the full resistance
>> +of the divider. This binding describes the voltage divider in such
>> +a curcuit.
>> +
>> +Required properties:
>> +- compatible : "voltage-divider"
>> +- io-channels : Channel node of a voltage io-channel.
>> +
>> +Optional properties:
>> +- numerator : The io-channel scale is multiplied by this value (default 1).
>> +- denominator : The io-channel scale is divided by this value (default 1).
>> +
>> +Example:
>> +The system voltage is circa 12V, but divided down with a 22/200
>> +voltage divider to adjust it to the ADC range.
>> +
>> +SYSV        ADC       GND
>> +  +          +         +
>> +  |  .-----. | .----.  |
>> +  '--| 200 |-+-| 22 |--'
>> +     '-----'   '----'
>> +
>> +sysv {
>> +	compatible = "voltage-divider";
>> +	io-channels = <&maxadc 1>;
>> +
>> +	/* Multiply the ADC voltage by 222/22 to get the system voltage. */
>> +	numerator = <222>; /* 200 + 22 */
>> +	denominator = <22>;
>> +};
>> +
>> +&spi {
>> +	maxadc: adc@0 {
>> +		compatible = "maxim,max1027";
>> +	      	reg = <0>;
>> +		#io-channel-cells = <1>;
>> +		interrupt-parent = <&gpio5>;
>> +		interrupts = <15 IRQ_TYPE_EDGE_RISING>;
>> +		spi-max-frequency = <1000000>;
>> +	};
>> +};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 36a28e979e9a..9dbe5019c6bd 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6889,6 +6889,13 @@ F:	drivers/staging/iio/
>>  F:	include/linux/iio/
>>  F:	tools/iio/
>>  
>> +IIO UNIT CONVERTER
>> +M:	Peter Rosin <peda@axentia.se>
>> +L:	linux-iio@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
>> +F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
>> +
>>  IKANOS/ADI EAGLE ADSL USB DRIVER
>>  M:	Matthieu Castet <castet.matthieu@free.fr>
>>  M:	Stanislaw Gruszka <stf_xl@wp.pl>
> 

Re: [PATCH v2 0/2] iio: add unit converter

[Rob Herring]

On Tue, Apr 03, 2018 at 08:09:08PM +0200, Peter Rosin wrote:
> On 2018-04-03 19:41, Andrew F. Davis wrote:
> > On 04/03/2018 10:36 AM, Peter Rosin wrote:
> >> Hi!
> >>
> >> This driver implements support for voltage dividers and current
> >> sense circuits. It's pretty generic and should be easily adaptable
> >> to other linear scaling purposes...
> >>
> > 
> > I really like this idea, defining channel scaling / channel type
> > conversion in DT will be very useful. So much so that I would recommend
> > this not be a use specific driver but instead moved into the IIO core.
> > 
> > This would allow defining these channel conversions in the device node
> > itself, so as to not need a separate node just for the converter (the
> > conversion is not a device and probably should not have it's own node
> > anyway). It would also help with enabling this support for buffered
> > readers/writers and not just devices only supporting _raw reads/writes.
> 
> In my case, the voltage dividers and the current sense circuit are very
> much real, and I can point my finger at them on the board. Sure, they
> are not ICs, but to not call them devices is wrong IMHO, and the ADC
> which is involved have very little to do with the voltage dividers and
> the fact that it is involved in sensing current. This is not an argument
> for not moving the functionally to the core though, but that has some
> problems AFAICT. Because you need some kind of clever and generic
> binding to make the core do its thing, and it might not be easy to come
> up with something that fits all devices? And if we do put this in the
> core, that opens the door for more complex unit converters later on
> (non-linear etc), further complicating the generic bindings.

I agree. We've learned the hard way that even things like the LED diode 
itself need to be described and not just the controller.

Rob

Re: [PATCH v2 1/2] dt-bindings: iio: afe: add current-sense-cuicuit and voltage-divider

[Rob Herring]

On Sun, Apr 08, 2018 at 05:50:31PM +0100, Jonathan Cameron wrote:
> On Tue,  3 Apr 2018 17:36:34 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
> circuit in the patch title is spelled wrong.
> 
> > An ADC is often used to measure other quantities indirectly. These
> > bindings describe two cases, a current through a sense resistor, and
> > a "big" voltage measured with the help of a voltage divider.
> > 
> > Signed-off-by: Peter Rosin <peda@axentia.se>
> 
> Will definitely be wanting wide opinions on this one - Rob in particularly
> for the binding side.
> 
> One comment inline. What we have here is nice and generic, but is
> it what would be 'expected' for current sense circuit?  Should we
> also be more specific in the naming.  There are lots of options for
> current sense circuits and this is just the simplest (current loop
> for example).
> 
> One option would be to use current-sense-shunt perhaps?
> 
> https://en.wikipedia.org/wiki/Current_sensing_techniques
> Gives a few that I didn't think of beyond current loops etc.
> 
> 
> > ---
> >  .../bindings/iio/afe/current-sense-circuit.txt     | 45 ++++++++++++++++++++++
> >  .../bindings/iio/afe/voltage-divider.txt           | 45 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  7 ++++
> >  3 files changed, 97 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> >  create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> > new file mode 100644
> > index 000000000000..0bc7d89387c0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> > @@ -0,0 +1,45 @@
> > +Current Sense Curcuit
> > +=====================
> > +
> > +When an io-channel measures the voltage over a current sense resistor,
> > +the interesting mesaurement is often the current through the resistor,
> > +not the voltage over it. This binding describes such a current sense
> > +curcuit.
> > +
> > +Required properties:
> > +- compatible : "current-sense-circuit"
> > +- io-channels : Channel node of a voltage io-channel.
> > +
> > +Optional properties:
> > +- numerator : The io-channel scale is multiplied by this value (default 1).
> > +- denominator : The io-channel scale is divided by this value (default 1).
> > +
> > +Example:
> > +The system current is measured by measuring the voltage over a
> > +3.3 ohms sense resistor.
> 
> Hmm. It sort of feels like the binding doesn't really reflect the
> hardware as directly as it might.   Should we be explicitly having the
> resistance in this case?  (with some more mapping logic needed in the
> driver to figure out the scaling this causes).

I think having the resistance would be better as least in this case. For 
other cases like voltage dividers I'm not sure though. Those would be 
different compatibles so the associated properties don't necessarily 
need to be the same.

Rob

Re: [PATCH v2 1/2] dt-bindings: iio: afe: add current-sense-cuicuit and voltage-divider

[Jonathan Cameron]

On Tue, 10 Apr 2018 08:13:38 -0500
Rob Herring <robh@kernel.org> wrote:

> On Sun, Apr 08, 2018 at 05:50:31PM +0100, Jonathan Cameron wrote:
> > On Tue,  3 Apr 2018 17:36:34 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> > 
> > circuit in the patch title is spelled wrong.
> >   
> > > An ADC is often used to measure other quantities indirectly. These
> > > bindings describe two cases, a current through a sense resistor, and
> > > a "big" voltage measured with the help of a voltage divider.
> > > 
> > > Signed-off-by: Peter Rosin <peda@axentia.se>  
> > 
> > Will definitely be wanting wide opinions on this one - Rob in particularly
> > for the binding side.
> > 
> > One comment inline. What we have here is nice and generic, but is
> > it what would be 'expected' for current sense circuit?  Should we
> > also be more specific in the naming.  There are lots of options for
> > current sense circuits and this is just the simplest (current loop
> > for example).
> > 
> > One option would be to use current-sense-shunt perhaps?
> > 
> > https://en.wikipedia.org/wiki/Current_sensing_techniques
> > Gives a few that I didn't think of beyond current loops etc.
> > 
> >   
> > > ---
> > >  .../bindings/iio/afe/current-sense-circuit.txt     | 45 ++++++++++++++++++++++
> > >  .../bindings/iio/afe/voltage-divider.txt           | 45 ++++++++++++++++++++++
> > >  MAINTAINERS                                        |  7 ++++
> > >  3 files changed, 97 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> > >  create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> > > new file mode 100644
> > > index 000000000000..0bc7d89387c0
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/afe/current-sense-circuit.txt
> > > @@ -0,0 +1,45 @@
> > > +Current Sense Curcuit
> > > +=====================
> > > +
> > > +When an io-channel measures the voltage over a current sense resistor,
> > > +the interesting mesaurement is often the current through the resistor,
> > > +not the voltage over it. This binding describes such a current sense
> > > +curcuit.
> > > +
> > > +Required properties:
> > > +- compatible : "current-sense-circuit"
> > > +- io-channels : Channel node of a voltage io-channel.
> > > +
> > > +Optional properties:
> > > +- numerator : The io-channel scale is multiplied by this value (default 1).
> > > +- denominator : The io-channel scale is divided by this value (default 1).
> > > +
> > > +Example:
> > > +The system current is measured by measuring the voltage over a
> > > +3.3 ohms sense resistor.  
> > 
> > Hmm. It sort of feels like the binding doesn't really reflect the
> > hardware as directly as it might.   Should we be explicitly having the
> > resistance in this case?  (with some more mapping logic needed in the
> > driver to figure out the scaling this causes).  
> 
> I think having the resistance would be better as least in this case. For 
> other cases like voltage dividers I'm not sure though. Those would be 
> different compatibles so the associated properties don't necessarily 
> need to be the same.

Agreed, that was my thought as well.  Given with divider it really is the
ratios that matter, I'd keep it defined in terms of them.  We'll have
some slightly interesting bindings to interpret to get to what matters
internally, but I don't think that will be too bad.

So nicer bindings at the slight cost of driver complexity.  Sounds fine
to me!
> 
> Rob
> --
> 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