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

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

[Peter Rosin]

Hi!

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

Cheers,
Peter

Changes since v2:    https://lkml.org/lkml/2018/4/3/461
- Rename from current-sence-circuit to current-sense-shunt (this
  should also fix all the typing problems I had with curciut).
- Describe the shunt resistance directly (instead of indirectly
  in the form of a quotient).
- Add a ->props() op to struct unit_converter_cfg to enable simple
  separation of properties for different converters.
- Shuffle the code around to minimize forward declarations.
- Drop the unused indio member from struct unit_converter.
- Drop error report on iio driver registration failure.

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-shunt and voltage-divider
  iio: afe: unit-converter: new driver

 .../bindings/iio/afe/current-sense-shunt.txt       |  41 +++
 .../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               | 291 +++++++++++++++++++++
 8 files changed, 411 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-shunt.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 v3 1/2] dt-bindings: iio: afe: add current-sense-shunt and voltage-divider

[Peter Rosin]

An ADC is often used to measure other quantities indirectly. These
bindings describe two cases, a current through a shunt 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-shunt.txt       | 41 ++++++++++++++++++++
 .../bindings/iio/afe/voltage-divider.txt           | 45 ++++++++++++++++++++++
 MAINTAINERS                                        |  7 ++++
 3 files changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-shunt.txt
 create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt

diff --git a/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.txt b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.txt
new file mode 100644
index 000000000000..4d842aa85040
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/current-sense-shunt.txt
@@ -0,0 +1,41 @@
+Current Sense Shunt
+===================
+
+When an io-channel measures the voltage over a current sense shunt,
+the interesting mesaurement is almost always the current through the
+shunt, not the voltage over it. This binding describes such a current
+sense circuit.
+
+Required properties:
+- compatible : "current-sense-shunt"
+- io-channels : Channel node of a voltage io-channel.
+- shunt-resistor-micro-ohms : The shunt resistance in microohms.
+
+Example:
+The system current is measured by measuring the voltage over a
+3.3 ohms shunt resistor.
+
+sysi {
+	compatible = "current-sense-shunt";
+	io-channels = <&tiadc 0>;
+
+	/* Divide the voltage by 3300000/1000000 (or 3.3) for the current. */
+	shunt-resistor-micro-ohms = <3300000>;
+};
+
+&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 6e950b8b4a41..237fcdfdddc6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6887,6 +6887,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-shunt.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

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

[Rob Herring]

On Tue, Apr 10, 2018 at 05:28:01PM +0200, Peter Rosin wrote:
> An ADC is often used to measure other quantities indirectly. These
> bindings describe two cases, a current through a shunt 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-shunt.txt       | 41 ++++++++++++++++++++
>  .../bindings/iio/afe/voltage-divider.txt           | 45 ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 ++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/current-sense-shunt.txt
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/voltage-divider.txt

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Peter Rosin]

On 2018-04-10 17:28, Peter Rosin wrote:
> +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>;
> +};

While I already got a reviewed-by from Rob, and maybe I shouldn't be
stirring the pot, but I had an umpteenth look and I now think this
one looks a bit odd. It shows a bit that it originates from when the
compatible was the very generic "io-channel-unit-converter" in v1
of the series. What I mean is that a voltage divider presumable always
gets you a lower voltage. Therefore, one would assume that the
denominator should be larger than the numerator. The fact that this
translates into the inverted fraction when calculating backwards
through the voltage divider should probably not affect the binding.

So, in the above example, I think it would make more sense to have

	numerator = <22>;
	denominator = <222>; /* 200 + 22 */

(and then, naturally, adjust the driver to invert the fraction)

Comments?

Cheers,
Peter

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

[Jonathan Cameron]

On Mon, 16 Apr 2018 16:00:44 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2018-04-10 17:28, Peter Rosin wrote:
> > +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>;
> > +};  
> 
> While I already got a reviewed-by from Rob, and maybe I shouldn't be
> stirring the pot, but I had an umpteenth look and I now think this
> one looks a bit odd. It shows a bit that it originates from when the
> compatible was the very generic "io-channel-unit-converter" in v1
> of the series. What I mean is that a voltage divider presumable always
> gets you a lower voltage. Therefore, one would assume that the
> denominator should be larger than the numerator. The fact that this
> translates into the inverted fraction when calculating backwards
> through the voltage divider should probably not affect the binding.
> 
> So, in the above example, I think it would make more sense to have
> 
> 	numerator = <22>;
> 	denominator = <222>; /* 200 + 22 */
> 
> (and then, naturally, adjust the driver to invert the fraction)
> 
> Comments?
Agreed - it is odd as we currently have it.  I wouldn't have
a particular problem with renaming this compatible as voltage
scaler (voltage multiplier has a specific meaning we should avoid
stepping on!)  Anyhow, either way is fine with me, but I agree
the current version is odd.

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

[PATCH v3 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 shunt
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 | 291 +++++++++++++++++++++++++++++++++++
 6 files changed, 318 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 237fcdfdddc6..d2a28b915fca 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6893,6 +6893,7 @@ L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.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..642ce4eb12a6
--- /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 shunts.
+
+	  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..fc50290d7e5e
--- /dev/null
+++ b/drivers/iio/afe/iio-unit-converter.c
@@ -0,0 +1,291 @@
+// 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/gcd.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;
+
+struct unit_converter_cfg {
+	enum iio_chan_type type;
+	int (*props)(struct device *dev, struct unit_converter *uc);
+};
+
+struct unit_converter {
+	const struct unit_converter_cfg *cfg;
+	struct iio_channel *source;
+	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 int unit_converter_current_sense_shunt_props(struct device *dev,
+						    struct unit_converter *uc)
+{
+	u32 shunt;
+	u32 factor;
+	int ret;
+
+	ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
+				       &shunt);
+	if (ret) {
+		dev_err(dev, "failed to read the shunt resistance: %d\n", ret);
+		return ret;
+	}
+
+	factor = gcd(shunt, 1000000);
+	uc->numerator = 1000000 / factor;
+	uc->denominator = shunt / factor;
+
+	return 0;
+}
+
+static int unit_converter_voltage_divider_props(struct device *dev,
+						struct unit_converter *uc)
+{
+	device_property_read_u32(dev, "numerator", &uc->numerator);
+	device_property_read_u32(dev, "denominator", &uc->denominator);
+
+	return 0;
+}
+
+enum unit_converter_variant {
+	CURRENT_SENSE_SHUNT,
+	VOLTAGE_DIVIDER,
+};
+
+static const struct unit_converter_cfg unit_converter_cfg[] = {
+	[CURRENT_SENSE_SHUNT] = {
+		.type = IIO_CURRENT,
+		.props = unit_converter_current_sense_shunt_props,
+	},
+	[VOLTAGE_DIVIDER] = {
+		.type = IIO_VOLTAGE,
+		.props = unit_converter_voltage_divider_props,
+	},
+};
+
+static const struct of_device_id unit_converter_match[] = {
+	{ .compatible = "current-sense-shunt",
+	  .data = &unit_converter_cfg[CURRENT_SENSE_SHUNT], },
+	{ .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;
+
+	ret = uc->cfg->props(dev, uc);
+	if (ret)
+		return ret;
+
+	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;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+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 v3 2/2] iio: afe: unit-converter: new driver

[Jonathan Cameron]

On Tue, 10 Apr 2018 17:28:02 +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 shunt
> 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>
So I 'think' the only outstanding question is Andrew's one about the driver
name.  We aren't in a hurry at this point in the kernel cycle, so lets
wait until that discussion has ended.  Assuming that we do possibly end
up with a change, then please roll all the patches up into a single series
to avoid me getting confusion.

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 | 291 +++++++++++++++++++++++++++++++++++
>  6 files changed, 318 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 237fcdfdddc6..d2a28b915fca 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6893,6 +6893,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.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..642ce4eb12a6
> --- /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 shunts.
> +
> +	  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..fc50290d7e5e
> --- /dev/null
> +++ b/drivers/iio/afe/iio-unit-converter.c
> @@ -0,0 +1,291 @@
> +// 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/gcd.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;
> +
> +struct unit_converter_cfg {
> +	enum iio_chan_type type;
> +	int (*props)(struct device *dev, struct unit_converter *uc);
> +};
> +
> +struct unit_converter {
> +	const struct unit_converter_cfg *cfg;
> +	struct iio_channel *source;
> +	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 int unit_converter_current_sense_shunt_props(struct device *dev,
> +						    struct unit_converter *uc)
> +{
> +	u32 shunt;
> +	u32 factor;
> +	int ret;
> +
> +	ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> +				       &shunt);
> +	if (ret) {
> +		dev_err(dev, "failed to read the shunt resistance: %d\n", ret);
> +		return ret;
> +	}
> +
> +	factor = gcd(shunt, 1000000);
> +	uc->numerator = 1000000 / factor;
> +	uc->denominator = shunt / factor;
> +
> +	return 0;
> +}
> +
> +static int unit_converter_voltage_divider_props(struct device *dev,
> +						struct unit_converter *uc)
> +{
> +	device_property_read_u32(dev, "numerator", &uc->numerator);
> +	device_property_read_u32(dev, "denominator", &uc->denominator);
> +
> +	return 0;
> +}
> +
> +enum unit_converter_variant {
> +	CURRENT_SENSE_SHUNT,
> +	VOLTAGE_DIVIDER,
> +};
> +
> +static const struct unit_converter_cfg unit_converter_cfg[] = {
> +	[CURRENT_SENSE_SHUNT] = {
> +		.type = IIO_CURRENT,
> +		.props = unit_converter_current_sense_shunt_props,
> +	},
> +	[VOLTAGE_DIVIDER] = {
> +		.type = IIO_VOLTAGE,
> +		.props = unit_converter_voltage_divider_props,
> +	},
> +};
> +
> +static const struct of_device_id unit_converter_match[] = {
> +	{ .compatible = "current-sense-shunt",
> +	  .data = &unit_converter_cfg[CURRENT_SENSE_SHUNT], },
> +	{ .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;
> +
> +	ret = uc->cfg->props(dev, uc);
> +	if (ret)
> +		return ret;
> +
> +	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;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +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 v3 2/2] iio: afe: unit-converter: new driver

[Peter Rosin]

On 2018-04-15 19:31, Jonathan Cameron wrote:
> On Tue, 10 Apr 2018 17:28:02 +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 shunt
>> 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>
> So I 'think' the only outstanding question is Andrew's one about the driver
> name.  We aren't in a hurry at this point in the kernel cycle, so lets
> wait until that discussion has ended.  Assuming that we do possibly end
> up with a change, then please roll all the patches up into a single series
> to avoid me getting confusion.

Yeah, sure, sorry for the split series, but the lt6106 that's present in
one of our newer designs didn't occur to me until just seconds after
firing the first half of the series. Which is kind of typical...

Anyway, about the driver naming. The suggestion I like best so far is
linear-scaler from Linus W, but thinking about it some more I think I
like iio-rescale even better.

Any objections to iio-rescale?

Cheers,
Peter

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

[Jonathan Cameron]

On Mon, 16 Apr 2018 09:12:45 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2018-04-15 19:31, Jonathan Cameron wrote:
> > On Tue, 10 Apr 2018 17:28:02 +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 shunt
> >> 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>  
> > So I 'think' the only outstanding question is Andrew's one about the driver
> > name.  We aren't in a hurry at this point in the kernel cycle, so lets
> > wait until that discussion has ended.  Assuming that we do possibly end
> > up with a change, then please roll all the patches up into a single series
> > to avoid me getting confusion.  
> 
> Yeah, sure, sorry for the split series, but the lt6106 that's present in
> one of our newer designs didn't occur to me until just seconds after
> firing the first half of the series. Which is kind of typical...
> 
> Anyway, about the driver naming. The suggestion I like best so far is
> linear-scaler from Linus W, but thinking about it some more I think I
> like iio-rescale even better.
> 
> Any objections to iio-rescale?

Works for me. But then I rarely care 'that much' about naming and am
responsible for plenty of previous confusing choices ;)

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

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

[Peter Rosin]

Hi!

I'm now following up with one more binding for the unit-converter.
This time with a real IC, namely LT6106 from Analog Devices. It's
a current sense amplifier. I was a but unsure if I should have
the Rin and Rout resistors in the binding or if I instead should
have used the "gain". In the end I went with the resistors since
while the normal case is an integer gain, that may not always be
the case. And when it's not, that could get tricky. But I'm open
for arguments on that.

Cheers,
Peter

Peter Rosin (2):
  dt-bindings: iio: afe: add binding for adi,lt6106
  iio: afe: unit-converter: add support for adi,lt6106

 .../devicetree/bindings/iio/afe/adi,lt6106.txt     | 50 ++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 drivers/iio/afe/Kconfig                            |  3 +-
 drivers/iio/afe/iio-unit-converter.c               | 54 ++++++++++++++++++++++
 4 files changed, 107 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt

-- 
2.11.0

[PATCH 1/2] dt-bindings: iio: afe: add binding for adi,lt6106

[Peter Rosin]

This is a current sense amplifier from Analog Devices.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/iio/afe/adi,lt6106.txt     | 50 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt

diff --git a/Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt b/Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt
new file mode 100644
index 000000000000..98b6d93596f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt
@@ -0,0 +1,50 @@
+LT6106 36V High Side Current Sence Amplifier
+============================================
+
+This binding assumes the typical application of the current sense amplifier
+as described in the LT6106 datasheet.
+
+http://www.analog.com/media/en/technical-documentation/data-sheets/6106fb.pdf
+
+                                             .------.
+    Vin --------------------+--------------+-|Rsense|-+-----.
+                            |              | '------' |     |
+                            |              |          |   .---.
+                 .--------------------.  .---.        |   | L |
+                 |          V+        |  |Rin|        |   | O |
+                 |                    |  '---'        |   | A |
+                 |                    |    |          |   | D |
+                 |                -IN |----'          |   '---'
+    Vout ---+----| OUT   LT6106       |               |     |
+            |    |                +IN |---------------'    GND
+         .----.  |                    |
+         |Rout|  |                    |
+         '----'  |          V-        |
+            |    '--------------------'
+            |               |
+           GND             GND
+
+The voltage Vsense over Rsense is measured by looking at Vout. They
+are related as Vout = Vsense * Rout / Rin. The current Isense through
+Rsense is (almost) the same as that through the LOAD. Hence, the
+interesting LOAD current can be calculated as
+
+                 Vout * Rin / (Rout * Rsense)
+
+Required properties:
+- compatible : "adi,lt6106"
+- io-channels : Channel node of an io-channel measuring Vout.
+- sense-resistor-micro-ohms : The Rsense resistance in microohms.
+- input-resistor-ohms : The Rin resistance in ohms.
+- output-resistor-ohms : The Rout resistance in ohms.
+
+Example:
+
+sysi {
+	compatible = "adi,lt6106";
+	io-channels = <&tiadc 0>;
+
+	sense-resistor-micro-ohms = <20000>;
+	input-resistor-ohms = <200>;
+	output-resistor-ohms = <10000>;
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 21368749f3b0..76d1e03f350a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6893,6 +6893,7 @@ IIO UNIT CONVERTER
 M:	Peter Rosin <peda@axentia.se>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt
 F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.txt
 F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
 F:	drivers/iio/afe/iio-unit-converter.c
-- 
2.11.0

Re: [PATCH 1/2] dt-bindings: iio: afe: add binding for adi,lt6106

[Rob Herring]

On Wed, Apr 11, 2018 at 04:15:54PM +0200, Peter Rosin wrote:
> This is a current sense amplifier from Analog Devices.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  .../devicetree/bindings/iio/afe/adi,lt6106.txt     | 50 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt b/Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt
> new file mode 100644
> index 000000000000..98b6d93596f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt
> @@ -0,0 +1,50 @@
> +LT6106 36V High Side Current Sence Amplifier
> +============================================
> +
> +This binding assumes the typical application of the current sense amplifier
> +as described in the LT6106 datasheet.
> +
> +http://www.analog.com/media/en/technical-documentation/data-sheets/6106fb.pdf
> +
> +                                             .------.
> +    Vin --------------------+--------------+-|Rsense|-+-----.
> +                            |              | '------' |     |
> +                            |              |          |   .---.
> +                 .--------------------.  .---.        |   | L |
> +                 |          V+        |  |Rin|        |   | O |
> +                 |                    |  '---'        |   | A |
> +                 |                    |    |          |   | D |
> +                 |                -IN |----'          |   '---'
> +    Vout ---+----| OUT   LT6106       |               |     |
> +            |    |                +IN |---------------'    GND
> +         .----.  |                    |
> +         |Rout|  |                    |
> +         '----'  |          V-        |
> +            |    '--------------------'
> +            |               |
> +           GND             GND
> +
> +The voltage Vsense over Rsense is measured by looking at Vout. They
> +are related as Vout = Vsense * Rout / Rin. The current Isense through
> +Rsense is (almost) the same as that through the LOAD. Hence, the
> +interesting LOAD current can be calculated as
> +
> +                 Vout * Rin / (Rout * Rsense)
> +
> +Required properties:
> +- compatible : "adi,lt6106"
> +- io-channels : Channel node of an io-channel measuring Vout.
> +- sense-resistor-micro-ohms : The Rsense resistance in microohms.

> +- input-resistor-ohms : The Rin resistance in ohms.
> +- output-resistor-ohms : The Rout resistance in ohms.

Are these generic or specific to this binding? For the latter, they 
shoudl have vendor prefix.

> +
> +Example:
> +
> +sysi {
> +	compatible = "adi,lt6106";
> +	io-channels = <&tiadc 0>;
> +
> +	sense-resistor-micro-ohms = <20000>;
> +	input-resistor-ohms = <200>;
> +	output-resistor-ohms = <10000>;
> +};

[PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Peter Rosin]

This is a current sense amplifier from Analog Devices.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/afe/Kconfig              |  3 +-
 drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
index 642ce4eb12a6..0e10fe8f459a 100644
--- a/drivers/iio/afe/Kconfig
+++ b/drivers/iio/afe/Kconfig
@@ -10,7 +10,8 @@ config 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 shunts.
+	  that handles voltage dividers, current sense shunts and
+	  the LT6106 Current Sense Amplifier from Analog Devices.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called iio-unit-converter.
diff --git a/drivers/iio/afe/iio-unit-converter.c b/drivers/iio/afe/iio-unit-converter.c
index fc50290d7e5e..4b0ae5ab9c2d 100644
--- a/drivers/iio/afe/iio-unit-converter.c
+++ b/drivers/iio/afe/iio-unit-converter.c
@@ -144,6 +144,53 @@ static int unit_converter_configure_channel(struct device *dev,
 	return 0;
 }
 
+static int unit_converter_adi_lt6106_props(struct device *dev,
+					   struct unit_converter *uc)
+{
+	u32 sense;
+	u32 rin;
+	u32 rout;
+	u32 factor;
+	int ret;
+
+	ret = device_property_read_u32(dev, "sense-resistor-micro-ohms",
+				       &sense);
+	if (ret) {
+		dev_err(dev, "failed to read the sense resistance: %d\n", ret);
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev, "input-resistor-ohms", &rin);
+	if (ret) {
+		dev_err(dev, "failed to read the input resistance: %d\n", ret);
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev, "output-resistor-ohms", &rout);
+	if (ret) {
+		dev_err(dev, "failed to read the output resistance: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Calculate the scaling factor, rin / (rout * sense), while trying
+	 * to keep the fraction from overflowing.
+	 */
+	factor = gcd(sense, 1000000);
+	uc->numerator = 1000000 / factor;
+	uc->denominator = sense / factor;
+
+	factor = gcd(uc->numerator, rout);
+	uc->numerator /= factor;
+	uc->denominator *= rout / factor;
+
+	factor = gcd(uc->denominator, rin);
+	uc->numerator *= rin / factor;
+	uc->denominator /= factor;
+
+	return 0;
+}
+
 static int unit_converter_current_sense_shunt_props(struct device *dev,
 						    struct unit_converter *uc)
 {
@@ -175,11 +222,16 @@ static int unit_converter_voltage_divider_props(struct device *dev,
 }
 
 enum unit_converter_variant {
+	ADI_LT6106,
 	CURRENT_SENSE_SHUNT,
 	VOLTAGE_DIVIDER,
 };
 
 static const struct unit_converter_cfg unit_converter_cfg[] = {
+	[ADI_LT6106] = {
+		.type = IIO_CURRENT,
+		.props = unit_converter_adi_lt6106_props,
+	},
 	[CURRENT_SENSE_SHUNT] = {
 		.type = IIO_CURRENT,
 		.props = unit_converter_current_sense_shunt_props,
@@ -191,6 +243,8 @@ static const struct unit_converter_cfg unit_converter_cfg[] = {
 };
 
 static const struct of_device_id unit_converter_match[] = {
+	{ .compatible = "adi,lt6106",
+	  .data = &unit_converter_cfg[ADI_LT6106], },
 	{ .compatible = "current-sense-shunt",
 	  .data = &unit_converter_cfg[CURRENT_SENSE_SHUNT], },
 	{ .compatible = "voltage-divider",
-- 
2.11.0

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Andrew F. Davis]

On 04/11/2018 09:15 AM, Peter Rosin wrote:
> This is a current sense amplifier from Analog Devices.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/iio/afe/Kconfig              |  3 +-
>  drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
> index 642ce4eb12a6..0e10fe8f459a 100644
> --- a/drivers/iio/afe/Kconfig
> +++ b/drivers/iio/afe/Kconfig
> @@ -10,7 +10,8 @@ config 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 shunts.
> +	  that handles voltage dividers, current sense shunts and
> +	  the LT6106 Current Sense Amplifier from Analog Devices.

Could work better to split these out into separate drivers. Maybe a
iio-shunt-resistor.c that does just voltage->current with the
appropriate scaling. Then make a a separate lt6106.c.

"iio-unit-converter.c" isn't really doing what it says it is, it is not
a generic "unit converter" like one would assume. Having the driver name
describe what kind of device it physically represents will be better
when more complex AFEs show up that, for instance, have programmable
gains and need a larger driver.

Andrew

>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called iio-unit-converter.
> diff --git a/drivers/iio/afe/iio-unit-converter.c b/drivers/iio/afe/iio-unit-converter.c
> index fc50290d7e5e..4b0ae5ab9c2d 100644
> --- a/drivers/iio/afe/iio-unit-converter.c
> +++ b/drivers/iio/afe/iio-unit-converter.c
> @@ -144,6 +144,53 @@ static int unit_converter_configure_channel(struct device *dev,
>  	return 0;
>  }
>  
> +static int unit_converter_adi_lt6106_props(struct device *dev,
> +					   struct unit_converter *uc)
> +{
> +	u32 sense;
> +	u32 rin;
> +	u32 rout;
> +	u32 factor;
> +	int ret;
> +
> +	ret = device_property_read_u32(dev, "sense-resistor-micro-ohms",
> +				       &sense);
> +	if (ret) {
> +		dev_err(dev, "failed to read the sense resistance: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev, "input-resistor-ohms", &rin);
> +	if (ret) {
> +		dev_err(dev, "failed to read the input resistance: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev, "output-resistor-ohms", &rout);
> +	if (ret) {
> +		dev_err(dev, "failed to read the output resistance: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Calculate the scaling factor, rin / (rout * sense), while trying
> +	 * to keep the fraction from overflowing.
> +	 */
> +	factor = gcd(sense, 1000000);
> +	uc->numerator = 1000000 / factor;
> +	uc->denominator = sense / factor;
> +
> +	factor = gcd(uc->numerator, rout);
> +	uc->numerator /= factor;
> +	uc->denominator *= rout / factor;
> +
> +	factor = gcd(uc->denominator, rin);
> +	uc->numerator *= rin / factor;
> +	uc->denominator /= factor;
> +
> +	return 0;
> +}
> +
>  static int unit_converter_current_sense_shunt_props(struct device *dev,
>  						    struct unit_converter *uc)
>  {
> @@ -175,11 +222,16 @@ static int unit_converter_voltage_divider_props(struct device *dev,
>  }
>  
>  enum unit_converter_variant {
> +	ADI_LT6106,
>  	CURRENT_SENSE_SHUNT,
>  	VOLTAGE_DIVIDER,
>  };
>  
>  static const struct unit_converter_cfg unit_converter_cfg[] = {
> +	[ADI_LT6106] = {
> +		.type = IIO_CURRENT,
> +		.props = unit_converter_adi_lt6106_props,
> +	},
>  	[CURRENT_SENSE_SHUNT] = {
>  		.type = IIO_CURRENT,
>  		.props = unit_converter_current_sense_shunt_props,
> @@ -191,6 +243,8 @@ static const struct unit_converter_cfg unit_converter_cfg[] = {
>  };
>  
>  static const struct of_device_id unit_converter_match[] = {
> +	{ .compatible = "adi,lt6106",
> +	  .data = &unit_converter_cfg[ADI_LT6106], },
>  	{ .compatible = "current-sense-shunt",
>  	  .data = &unit_converter_cfg[CURRENT_SENSE_SHUNT], },
>  	{ .compatible = "voltage-divider",
> 

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Lars-Peter Clausen]

On 04/11/2018 05:43 PM, Andrew F. Davis wrote:
> On 04/11/2018 09:15 AM, Peter Rosin wrote:
>> This is a current sense amplifier from Analog Devices.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/iio/afe/Kconfig              |  3 +-
>>  drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
>> index 642ce4eb12a6..0e10fe8f459a 100644
>> --- a/drivers/iio/afe/Kconfig
>> +++ b/drivers/iio/afe/Kconfig
>> @@ -10,7 +10,8 @@ config 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 shunts.
>> +	  that handles voltage dividers, current sense shunts and
>> +	  the LT6106 Current Sense Amplifier from Analog Devices.
> 
> Could work better to split these out into separate drivers. Maybe a
> iio-shunt-resistor.c that does just voltage->current with the
> appropriate scaling. Then make a a separate lt6106.c.

I don't think we need a separate driver here. There are tons of circuits
that all work the same way and all require the same properties. If we'd add
a driver for each of them we'd get buried in boilerplate code.

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Andrew F. Davis]

On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote:
> On 04/11/2018 05:43 PM, Andrew F. Davis wrote:
>> On 04/11/2018 09:15 AM, Peter Rosin wrote:
>>> This is a current sense amplifier from Analog Devices.
>>>
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>>  drivers/iio/afe/Kconfig              |  3 +-
>>>  drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
>>> index 642ce4eb12a6..0e10fe8f459a 100644
>>> --- a/drivers/iio/afe/Kconfig
>>> +++ b/drivers/iio/afe/Kconfig
>>> @@ -10,7 +10,8 @@ config 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 shunts.
>>> +	  that handles voltage dividers, current sense shunts and
>>> +	  the LT6106 Current Sense Amplifier from Analog Devices.
>>
>> Could work better to split these out into separate drivers. Maybe a
>> iio-shunt-resistor.c that does just voltage->current with the
>> appropriate scaling. Then make a a separate lt6106.c.
> 
> I don't think we need a separate driver here. There are tons of circuits
> that all work the same way and all require the same properties. If we'd add
> a driver for each of them we'd get buried in boilerplate code.
> 

Fair enough, then it should at least be renamed to something generic
like current-sense-amplifier, as you said lots of circuits do this, not
just lt6106s. We will have then have support for:

current-sense-amplifier
current-sense-shunt
voltage-divider

compatibles in this driver called "unit-converter" which is still a
misnomer IMHO.

Andrew

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Peter Rosin]

On 2018-04-11 18:13, Andrew F. Davis wrote:
> On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote:
>> On 04/11/2018 05:43 PM, Andrew F. Davis wrote:
>>> On 04/11/2018 09:15 AM, Peter Rosin wrote:
>>>> This is a current sense amplifier from Analog Devices.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  drivers/iio/afe/Kconfig              |  3 +-
>>>>  drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
>>>> index 642ce4eb12a6..0e10fe8f459a 100644
>>>> --- a/drivers/iio/afe/Kconfig
>>>> +++ b/drivers/iio/afe/Kconfig
>>>> @@ -10,7 +10,8 @@ config 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 shunts.
>>>> +	  that handles voltage dividers, current sense shunts and
>>>> +	  the LT6106 Current Sense Amplifier from Analog Devices.
>>>
>>> Could work better to split these out into separate drivers. Maybe a
>>> iio-shunt-resistor.c that does just voltage->current with the
>>> appropriate scaling. Then make a a separate lt6106.c.
>>
>> I don't think we need a separate driver here. There are tons of circuits
>> that all work the same way and all require the same properties. If we'd add
>> a driver for each of them we'd get buried in boilerplate code.
>>
> 
> Fair enough, then it should at least be renamed to something generic
> like current-sense-amplifier, as you said lots of circuits do this, not
> just lt6106s. We will have then have support for:
> 
> current-sense-amplifier
> current-sense-shunt
> voltage-divider

For the compatible "current-sense-amplifier", I would advocate the
properties...

 sense-resistor-micro-ohms
 sense-gain

(or something close to that)

...and not input-resistor-ohms and output-resistor-ohms which are way
more particular to the LT6106.

But as I said in the cover letter, I didn't go with sense-gain since I
thought I would end up with requests for non-integer gains. There is
yet to be a comment on the non-integer gain problem, and before there
is a path forward for that case, I'm reluctant.

> compatibles in this driver called "unit-converter" which is still a
> misnomer IMHO.

I don't remember you having presented your preference, and I think
that goes against the established bike-shedding protocol?

Cheers,
Peter

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Andrew F. Davis]

On 04/12/2018 09:29 AM, Peter Rosin wrote:
> On 2018-04-11 18:13, Andrew F. Davis wrote:
>> On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote:
>>> On 04/11/2018 05:43 PM, Andrew F. Davis wrote:
>>>> On 04/11/2018 09:15 AM, Peter Rosin wrote:
>>>>> This is a current sense amplifier from Analog Devices.
>>>>>
>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>> ---
>>>>>  drivers/iio/afe/Kconfig              |  3 +-
>>>>>  drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
>>>>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
>>>>> index 642ce4eb12a6..0e10fe8f459a 100644
>>>>> --- a/drivers/iio/afe/Kconfig
>>>>> +++ b/drivers/iio/afe/Kconfig
>>>>> @@ -10,7 +10,8 @@ config 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 shunts.
>>>>> +	  that handles voltage dividers, current sense shunts and
>>>>> +	  the LT6106 Current Sense Amplifier from Analog Devices.
>>>>
>>>> Could work better to split these out into separate drivers. Maybe a
>>>> iio-shunt-resistor.c that does just voltage->current with the
>>>> appropriate scaling. Then make a a separate lt6106.c.
>>>
>>> I don't think we need a separate driver here. There are tons of circuits
>>> that all work the same way and all require the same properties. If we'd add
>>> a driver for each of them we'd get buried in boilerplate code.
>>>
>>
>> Fair enough, then it should at least be renamed to something generic
>> like current-sense-amplifier, as you said lots of circuits do this, not
>> just lt6106s. We will have then have support for:
>>
>> current-sense-amplifier
>> current-sense-shunt
>> voltage-divider
> 
> For the compatible "current-sense-amplifier", I would advocate the
> properties...
> 
>  sense-resistor-micro-ohms
>  sense-gain
> 
> (or something close to that)
> 
> ...and not input-resistor-ohms and output-resistor-ohms which are way
> more particular to the LT6106.
> 
> But as I said in the cover letter, I didn't go with sense-gain since I
> thought I would end up with requests for non-integer gains. There is
> yet to be a comment on the non-integer gain problem, and before there
> is a path forward for that case, I'm reluctant.
> 

Why not similar to what you had before with the resistor:

sense-gain-multiplier
sense-gain-divider

if either are missing assume they are 1.

>> compatibles in this driver called "unit-converter" which is still a
>> misnomer IMHO.
> 
> I don't remember you having presented your preference, and I think
> that goes against the established bike-shedding protocol?
> 

True, how about "current-sense-from-voltage" ?

Andrew

> Cheers,
> Peter
> 

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Peter Rosin]

On 2018-04-12 17:35, Andrew F. Davis wrote:
> On 04/12/2018 09:29 AM, Peter Rosin wrote:
>> On 2018-04-11 18:13, Andrew F. Davis wrote:
>>> On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote:
>>>> On 04/11/2018 05:43 PM, Andrew F. Davis wrote:
>>>>> On 04/11/2018 09:15 AM, Peter Rosin wrote:
>>>>>> This is a current sense amplifier from Analog Devices.
>>>>>>
>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>>> ---
>>>>>>  drivers/iio/afe/Kconfig              |  3 +-
>>>>>>  drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
>>>>>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
>>>>>> index 642ce4eb12a6..0e10fe8f459a 100644
>>>>>> --- a/drivers/iio/afe/Kconfig
>>>>>> +++ b/drivers/iio/afe/Kconfig
>>>>>> @@ -10,7 +10,8 @@ config 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 shunts.
>>>>>> +	  that handles voltage dividers, current sense shunts and
>>>>>> +	  the LT6106 Current Sense Amplifier from Analog Devices.
>>>>>
>>>>> Could work better to split these out into separate drivers. Maybe a
>>>>> iio-shunt-resistor.c that does just voltage->current with the
>>>>> appropriate scaling. Then make a a separate lt6106.c.
>>>>
>>>> I don't think we need a separate driver here. There are tons of circuits
>>>> that all work the same way and all require the same properties. If we'd add
>>>> a driver for each of them we'd get buried in boilerplate code.
>>>>
>>>
>>> Fair enough, then it should at least be renamed to something generic
>>> like current-sense-amplifier, as you said lots of circuits do this, not
>>> just lt6106s. We will have then have support for:
>>>
>>> current-sense-amplifier
>>> current-sense-shunt
>>> voltage-divider
>>
>> For the compatible "current-sense-amplifier", I would advocate the
>> properties...
>>
>>  sense-resistor-micro-ohms
>>  sense-gain
>>
>> (or something close to that)
>>
>> ...and not input-resistor-ohms and output-resistor-ohms which are way
>> more particular to the LT6106.
>>
>> But as I said in the cover letter, I didn't go with sense-gain since I
>> thought I would end up with requests for non-integer gains. There is
>> yet to be a comment on the non-integer gain problem, and before there
>> is a path forward for that case, I'm reluctant.
>>
> 
> Why not similar to what you had before with the resistor:
> 
> sense-gain-multiplier
> sense-gain-divider
> 
> if either are missing assume they are 1.

Hmm, how about sense-gain for the normal integer case, and then divide
by sense-attenuation if needed? I.e. exactly the same functionality as
you describe, just different names.

>>> compatibles in this driver called "unit-converter" which is still a
>>> misnomer IMHO.
>>
>> I don't remember you having presented your preference, and I think
>> that goes against the established bike-shedding protocol?
>>
> 
> True, how about "current-sense-from-voltage" ?

Doesn't cover "voltage-divider" (and we don't need separate drivers
doing the exact same calculations, that's a maintenance nightmare).

Cheers,
Peter

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Lars-Peter Clausen]

On 04/13/2018 12:31 AM, Peter Rosin wrote:
> On 2018-04-12 17:35, Andrew F. Davis wrote:
>> On 04/12/2018 09:29 AM, Peter Rosin wrote:
>>> On 2018-04-11 18:13, Andrew F. Davis wrote:
>>>> On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote:
>>>>> On 04/11/2018 05:43 PM, Andrew F. Davis wrote:
>>>>>> On 04/11/2018 09:15 AM, Peter Rosin wrote:
>>>>>>> This is a current sense amplifier from Analog Devices.
>>>>>>>
>>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>>>> ---
>>>>>>>  drivers/iio/afe/Kconfig              |  3 +-
>>>>>>>  drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
>>>>>>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
>>>>>>> index 642ce4eb12a6..0e10fe8f459a 100644
>>>>>>> --- a/drivers/iio/afe/Kconfig
>>>>>>> +++ b/drivers/iio/afe/Kconfig
>>>>>>> @@ -10,7 +10,8 @@ config 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 shunts.
>>>>>>> +	  that handles voltage dividers, current sense shunts and
>>>>>>> +	  the LT6106 Current Sense Amplifier from Analog Devices.
>>>>>>
>>>>>> Could work better to split these out into separate drivers. Maybe a
>>>>>> iio-shunt-resistor.c that does just voltage->current with the
>>>>>> appropriate scaling. Then make a a separate lt6106.c.
>>>>>
>>>>> I don't think we need a separate driver here. There are tons of circuits
>>>>> that all work the same way and all require the same properties. If we'd add
>>>>> a driver for each of them we'd get buried in boilerplate code.
>>>>>
>>>>
>>>> Fair enough, then it should at least be renamed to something generic
>>>> like current-sense-amplifier, as you said lots of circuits do this, not
>>>> just lt6106s. We will have then have support for:
>>>>
>>>> current-sense-amplifier
>>>> current-sense-shunt
>>>> voltage-divider
>>>
>>> For the compatible "current-sense-amplifier", I would advocate the
>>> properties...
>>>
>>>  sense-resistor-micro-ohms
>>>  sense-gain
>>>
>>> (or something close to that)
>>>
>>> ...and not input-resistor-ohms and output-resistor-ohms which are way
>>> more particular to the LT6106.
>>>
>>> But as I said in the cover letter, I didn't go with sense-gain since I
>>> thought I would end up with requests for non-integer gains. There is
>>> yet to be a comment on the non-integer gain problem, and before there
>>> is a path forward for that case, I'm reluctant.
>>>
>>
>> Why not similar to what you had before with the resistor:
>>
>> sense-gain-multiplier
>> sense-gain-divider
>>
>> if either are missing assume they are 1.
> 
> Hmm, how about sense-gain for the normal integer case, and then divide
> by sense-attenuation if needed? I.e. exactly the same functionality as
> you describe, just different names.

There is some precedence in the clock bindings for using -mult and -div as
the suffix for fractional scales. See fixed-factor-clock.txt.

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Peter Rosin]

On 2018-04-13 10:11, Lars-Peter Clausen wrote:
> On 04/13/2018 12:31 AM, Peter Rosin wrote:
>> On 2018-04-12 17:35, Andrew F. Davis wrote:
>>> On 04/12/2018 09:29 AM, Peter Rosin wrote:
>>>> But as I said in the cover letter, I didn't go with sense-gain since I
>>>> thought I would end up with requests for non-integer gains. There is
>>>> yet to be a comment on the non-integer gain problem, and before there
>>>> is a path forward for that case, I'm reluctant.
>>>
>>> Why not similar to what you had before with the resistor:
>>>
>>> sense-gain-multiplier
>>> sense-gain-divider
>>>
>>> if either are missing assume they are 1.
>>
>> Hmm, how about sense-gain for the normal integer case, and then divide
>> by sense-attenuation if needed? I.e. exactly the same functionality as
>> you describe, just different names.
> 
> There is some precedence in the clock bindings for using -mult and -div as
> the suffix for fractional scales. See fixed-factor-clock.txt.

Ok, I'm going with sense-gain-mult and sense-gain-div, but I'm going to
diverge a little bit from the clock binding and make them optional with
a default of 1 if missing.

I'm also going to use the compatible "current-sense-amplifier", and not
mention the LT6106, because I suspect that part can conceivable be used
in other ways...

But I'll wait for a bit with this and give everybody a last chance to
pitch suggestions and opinions.

Cheers,
Peter

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Andrew F. Davis]

On 04/12/2018 05:31 PM, Peter Rosin wrote:
> On 2018-04-12 17:35, Andrew F. Davis wrote:
>> On 04/12/2018 09:29 AM, Peter Rosin wrote:
>>> On 2018-04-11 18:13, Andrew F. Davis wrote:
>>>> On 04/11/2018 10:51 AM, Lars-Peter Clausen wrote:
>>>>> On 04/11/2018 05:43 PM, Andrew F. Davis wrote:
>>>>>> On 04/11/2018 09:15 AM, Peter Rosin wrote:
>>>>>>> This is a current sense amplifier from Analog Devices.
>>>>>>>
>>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>>>> ---
>>>>>>>  drivers/iio/afe/Kconfig              |  3 +-
>>>>>>>  drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
>>>>>>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
>>>>>>> index 642ce4eb12a6..0e10fe8f459a 100644
>>>>>>> --- a/drivers/iio/afe/Kconfig
>>>>>>> +++ b/drivers/iio/afe/Kconfig
>>>>>>> @@ -10,7 +10,8 @@ config 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 shunts.
>>>>>>> +	  that handles voltage dividers, current sense shunts and
>>>>>>> +	  the LT6106 Current Sense Amplifier from Analog Devices.
>>>>>>
>>>>>> Could work better to split these out into separate drivers. Maybe a
>>>>>> iio-shunt-resistor.c that does just voltage->current with the
>>>>>> appropriate scaling. Then make a a separate lt6106.c.
>>>>>
>>>>> I don't think we need a separate driver here. There are tons of circuits
>>>>> that all work the same way and all require the same properties. If we'd add
>>>>> a driver for each of them we'd get buried in boilerplate code.
>>>>>
>>>>
>>>> Fair enough, then it should at least be renamed to something generic
>>>> like current-sense-amplifier, as you said lots of circuits do this, not
>>>> just lt6106s. We will have then have support for:
>>>>
>>>> current-sense-amplifier
>>>> current-sense-shunt
>>>> voltage-divider
>>>
>>> For the compatible "current-sense-amplifier", I would advocate the
>>> properties...
>>>
>>>  sense-resistor-micro-ohms
>>>  sense-gain
>>>
>>> (or something close to that)
>>>
>>> ...and not input-resistor-ohms and output-resistor-ohms which are way
>>> more particular to the LT6106.
>>>
>>> But as I said in the cover letter, I didn't go with sense-gain since I
>>> thought I would end up with requests for non-integer gains. There is
>>> yet to be a comment on the non-integer gain problem, and before there
>>> is a path forward for that case, I'm reluctant.
>>>
>>
>> Why not similar to what you had before with the resistor:
>>
>> sense-gain-multiplier
>> sense-gain-divider
>>
>> if either are missing assume they are 1.
> 
> Hmm, how about sense-gain for the normal integer case, and then divide
> by sense-attenuation if needed? I.e. exactly the same functionality as
> you describe, just different names.
> 

I like these names, but I think gain/attenuation sound very analog and I
would be tempted to assume they are floating point numbers or the units
are logarithmic (dB).

To prevent any more needless bike-shedding on my part I'd like to say
either yours, mine, or Lars-Peter's suggestion all work for me.

>>>> compatibles in this driver called "unit-converter" which is still a
>>>> misnomer IMHO.
>>>
>>> I don't remember you having presented your preference, and I think
>>> that goes against the established bike-shedding protocol?
>>>
>>
>> True, how about "current-sense-from-voltage" ?
> 
> Doesn't cover "voltage-divider" (and we don't need separate drivers
> doing the exact same calculations, that's a maintenance nightmare).
> 


The driver name doesn't have to cover every use, just more than the
other name.


> Cheers,
> Peter
> 

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Peter Rosin]

On 2018-04-13 16:47, Andrew F. Davis wrote:
> On 04/12/2018 05:31 PM, Peter Rosin wrote:
>> On 2018-04-12 17:35, Andrew F. Davis wrote:
>>> True, how about "current-sense-from-voltage" ?
>>
>> Doesn't cover "voltage-divider" (and we don't need separate drivers
>> doing the exact same calculations, that's a maintenance nightmare).
> 
> The driver name doesn't have to cover every use, just more than the
> other name.

I also find current-sense-from-voltage unwieldy. It's simply too long.

Cheers,
Peter

Re: [PATCH 2/2] iio: afe: unit-converter: add support for adi,lt6106

[Peter Rosin]

On 2018-04-11 17:43, Andrew F. Davis wrote:
> On 04/11/2018 09:15 AM, Peter Rosin wrote:
>> This is a current sense amplifier from Analog Devices.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/iio/afe/Kconfig              |  3 +-
>>  drivers/iio/afe/iio-unit-converter.c | 54 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/afe/Kconfig b/drivers/iio/afe/Kconfig
>> index 642ce4eb12a6..0e10fe8f459a 100644
>> --- a/drivers/iio/afe/Kconfig
>> +++ b/drivers/iio/afe/Kconfig
>> @@ -10,7 +10,8 @@ config 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 shunts.
>> +	  that handles voltage dividers, current sense shunts and
>> +	  the LT6106 Current Sense Amplifier from Analog Devices.
> 
> Could work better to split these out into separate drivers. Maybe a
> iio-shunt-resistor.c that does just voltage->current with the
> appropriate scaling. Then make a a separate lt6106.c.
> 
> "iio-unit-converter.c" isn't really doing what it says it is, it is not
> a generic "unit converter" like one would assume. Having the driver name
> describe what kind of device it physically represents will be better
> when more complex AFEs show up that, for instance, have programmable
> gains and need a larger driver.

If an AFE needs programming etc, then it is definitely an option to
write a specific driver for it. It was never my intention to cover
all AFEs in this one driver, only those that make sense. Presumably
that ends up being those doing linear scaling and perhaps requiring
a change of the iio channel type.

Cheers,
Peter

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

[Andrew F. Davis]

On 04/11/2018 09:15 AM, Peter Rosin wrote:
> Hi!
> 
> I'm now following up with one more binding for the unit-converter.
> This time with a real IC, namely LT6106 from Analog Devices.

This makes more sense to me, I wasn't thinking about more complex ICs
like this when I made my "resistor being part of the sensing device"
argument.

I can see how having a new node will probably be needed. It just seems
strange to ask a resistor for its current, although it is the "device"
that "knows" how much current if flowing though it..

A case I'm more familiar with, fuel gauges, we have a gauge device that
has a handle to the battery it is gauging [0], we still ask the
"fuel-gauge" node to get out the state of charge, not the "battery"
node, the battery is not a "device" from our perspective.

Thinking more about that, I like your way better, so I recant my earlier
objections.

Andrew

[0] Documentation/devicetree/bindings/power/supply/bq27xxx.txt

> It's
> a current sense amplifier. I was a but unsure if I should have
> the Rin and Rout resistors in the binding or if I instead should
> have used the "gain". In the end I went with the resistors since
> while the normal case is an integer gain, that may not always be
> the case. And when it's not, that could get tricky. But I'm open
> for arguments on that.
> 
> Cheers,
> Peter
> 
> Peter Rosin (2):
>   dt-bindings: iio: afe: add binding for adi,lt6106
>   iio: afe: unit-converter: add support for adi,lt6106
> 
>  .../devicetree/bindings/iio/afe/adi,lt6106.txt     | 50 ++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  drivers/iio/afe/Kconfig                            |  3 +-
>  drivers/iio/afe/iio-unit-converter.c               | 54 ++++++++++++++++++++++
>  4 files changed, 107 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt
> 

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

[Jonathan Cameron]

On Wed, 11 Apr 2018 16:15:53 +0200
Peter Rosin <peda@axentia.se> wrote:

> Hi!
> 
> I'm now following up with one more binding for the unit-converter.
> This time with a real IC, namely LT6106 from Analog Devices. It's
> a current sense amplifier. I was a but unsure if I should have
> the Rin and Rout resistors in the binding or if I instead should
> have used the "gain". In the end I went with the resistors since
> while the normal case is an integer gain, that may not always be
> the case. And when it's not, that could get tricky. But I'm open
> for arguments on that.
> 
> Cheers,
> Peter
Trivial complaint of the day - why the same title for this email
as the previous series?  Had me really confused for a minute ;)

Seems like the whole driver is stabilizing.  If we do a v4 please
pull the whole lot into one series.

Jonathan

> 
> Peter Rosin (2):
>   dt-bindings: iio: afe: add binding for adi,lt6106
>   iio: afe: unit-converter: add support for adi,lt6106
> 
>  .../devicetree/bindings/iio/afe/adi,lt6106.txt     | 50 ++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  drivers/iio/afe/Kconfig                            |  3 +-
>  drivers/iio/afe/iio-unit-converter.c               | 54 ++++++++++++++++++++++
>  4 files changed, 107 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/afe/adi,lt6106.txt
>