[PATCH 0/4] iio: adc: Maxim max961x driver

[PATCH 0/4] iio: adc: Maxim max961x driver

[Jacopo Mondi]

Hello!

This series adds driver and documentation for Maxim max9611/max9612
high-side current sense amplifier with 12-bit ADC and I2c interface.
It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
in Renesas r87796 Salvator-X board.

The device provides several functionalities, and only some of them are
currently supported by this driver.
Particularly, the on-board op-amp and analog comparator are not currently
supported.

A simplified integration schematic drawing is here reported:

 ----o----/\/\/-----o-------|LOAD|---
     |    shunt     |
 ____|______________|___
 |  RS+            RS-  |
 |   |-----gain-----|   |
 |          |           |
 |          |           |
 |max961x   |->| ADC |===== I2c
 |______________________|


The device provides though its 12-bits ADC the following informations:
- Common input voltage on RS+
- Voltage drop between RS+ and RS- ends
- Die temperature

All of the above ones are exposed though IIO with _raw and _scale values
(plus _input for milli degree Celsius die temperature).

>From the above values the driver calculates the current flowing between
RS+ and RS- ends, using the shunt resistor value provided by device tree, and
the power load. Again this values are exposed through _raw and _scale
attributes, which I'm not completely sure it's acceptables as they are
calculated values and not natively provided by the current sense amplifier.
I would like to hear IIO people opinions on this, if they should be better
exposed though some other attributes which are not _raw and _scale, or if
their calculation should be completely left to userspace tools.

Thanks
   j

Jacopo Mondi (4):
  Documentation: dt-bindings: iio: Add max961x
  iio: Documentation: Add max961x sysfs documentation
  iio: adc: Add max9611/9612 ADC driver
  arm64: dts: salvator-x: Add current sense amplifiers

 .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
 .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
 arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
 6 files changed, 708 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
 create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
 create mode 100644 drivers/iio/adc/max961x.c

-- 
2.7.4

[PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Jacopo Mondi]

Add device tree bindings documentation for Maxim max961x current sense
amplifier.

Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/iio/adc/max961x.txt        | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/max961x.txt b/Documentation/devicetree/bindings/iio/adc/max961x.txt
new file mode 100644
index 0000000..abbc6c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
@@ -0,0 +1,27 @@
+* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
+
+Maxim max9611/max9612 is an high-side current sense amplifier with integrated
+12-bits ADC communicating over I2c bus.
+The device node for this driver shall be a child of a I2c controller.
+
+Required properties
+  - compatible: Should be "maxim,max961x"
+  - reg: The 7-bits long I2c address of the device
+  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
+		    resistor.
+
+Example:
+
+&i2c4 {
+	csa: max9611@7c {
+		compatible = "maxim,max961x";
+		reg = <0x7c>;
+
+		shunt-resistor = <5000>;
+	};
+};
+
+This device node describes a current sense amplifier sitting on I2c4 bus
+with address 0x7c (read address is 0xf9, write address is 0xf8).
+A sense resistor with 0,005 Ohm is installed between RS+ and RS-
+current-sensing inputs.
-- 
2.7.4

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

[PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Jacopo Mondi]

Add device tree bindings documentation for Maxim max961x current sense
amplifier.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../devicetree/bindings/iio/adc/max961x.txt        | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/max961x.txt b/Documentation/devicetree/bindings/iio/adc/max961x.txt
new file mode 100644
index 0000000..abbc6c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
@@ -0,0 +1,27 @@
+* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
+
+Maxim max9611/max9612 is an high-side current sense amplifier with integrated
+12-bits ADC communicating over I2c bus.
+The device node for this driver shall be a child of a I2c controller.
+
+Required properties
+  - compatible: Should be "maxim,max961x"
+  - reg: The 7-bits long I2c address of the device
+  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
+		    resistor.
+
+Example:
+
+&i2c4 {
+	csa: max9611@7c {
+		compatible = "maxim,max961x";
+		reg = <0x7c>;
+
+		shunt-resistor = <5000>;
+	};
+};
+
+This device node describes a current sense amplifier sitting on I2c4 bus
+with address 0x7c (read address is 0xf9, write address is 0xf8).
+A sense resistor with 0,005 Ohm is installed between RS+ and RS-
+current-sensing inputs.
-- 
2.7.4

[PATCH 2/4] iio: Documentation: Add max961x sysfs documentation

[Jacopo Mondi]

Add documentation for max961x driver.
The only attribute to document is the current sense shunt resistor
value.

Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
new file mode 100644
index 0000000..dbd5e75
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
@@ -0,0 +1,5 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
+Date:		February 2017
+KernelVersion:	4.10
+Contact:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description: 	The value of the shunt resistor in micro Ohms.
-- 
2.7.4

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

[PATCH 2/4] iio: Documentation: Add max961x sysfs documentation

[Jacopo Mondi]

Add documentation for max961x driver.
The only attribute to document is the current sense shunt resistor
value.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
new file mode 100644
index 0000000..dbd5e75
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
@@ -0,0 +1,5 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
+Date:		February 2017
+KernelVersion:	4.10
+Contact:	linux-iio@vger.kernel.org
+Description: 	The value of the shunt resistor in micro Ohms.
-- 
2.7.4

[PATCH 3/4] iio: adc: Add max9611/9612 ADC driver

[Jacopo Mondi]

Add iio driver for Maxim max9611/9612 current-sense amplifiers with
12-bits ADC.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/iio/adc/Kconfig   |  10 +
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/max961x.c | 648 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 659 insertions(+)
 create mode 100644 drivers/iio/adc/max961x.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dedae7a..f86026a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -354,6 +354,16 @@ config MAX1363
 	  To compile this driver as a module, choose M here: the module will be
 	  called max1363.
 
+config	MAX961x
+	tristate "Maxim max9611/9612 ADC driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Maxim max9611/9612 current sense
+	  amplifiers with 12-bits ADC interface.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max961x.
+
 config MCP320X
 	tristate "Microchip Technology MCP3x01/02/04/08"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d001262..ff19250 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MAX961x) += max961x.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
 obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
diff --git a/drivers/iio/adc/max961x.c b/drivers/iio/adc/max961x.c
new file mode 100644
index 0000000..a544e69
--- /dev/null
+++ b/drivers/iio/adc/max961x.c
@@ -0,0 +1,648 @@
+/*
+ * iio/adc/max961x.c
+ *
+ * Maxim max9611/9612 high side current sense amplifier with
+ * 12-bit ADC interface.
+ *
+ * Copyright (C) 2017 Jacopo Mondi
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ * This driver supports input common-mode voltage, current-sense
+ * amplifier with programmable gains and die temperature reading from
+ * Maxim max9611/9612.
+ * Op-amp, analog comparator, and watchdog functionalities are not
+ * supported by this driver.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/module.h>
+
+#define DRIVER_NAME "max961x"
+
+/* max961x register addresses */
+#define REG_CSA_DATA			0x00
+#define REG_RS_DATA			0x02
+#define REG_TEMP_DATA			0x08
+#define REG_CTRL1			0x0a
+#define REG_CTRL2			0x0b
+
+/* max961x REG1 mux configuration options */
+#define MUX_MASK			0x07
+#define MUX_SENSE_1x			0x00
+#define MUX_SENSE_4x			0x01
+#define MUX_SENSE_8x			0x02
+#define MUX_INPUT_VOLTAGE		0x03
+#define MUX_TEMPERATURE			0x06
+
+/* max961x voltage (both sense and input) helper macros */
+#define MAX961x_VOLTAGE_SHIFT		0x04
+#define MAX961x_VOLTAGE_RAW(_r)		((_r) >> MAX961x_VOLTAGE_SHIFT)
+
+/*
+ * max961x current sense amplifier voltage output:
+ * LSB and offset values depends on selected gain (1x, 4x, 8x)
+ *
+ * GAIN		LSB (nV)	OFFSET (LSB steps)
+ * 1x		107500		1
+ * 4x		26880		1
+ * 8x		13440		3
+ *
+ * The complete formula to calculate current sense voltage is:
+ *     (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
+ */
+#define CSA_VOLT_1x_LSB_nV		107500
+#define CSA_VOLT_4x_LSB_nV		26880
+#define CSA_VOLT_8x_LSB_nV		13440
+#define CSA_VOLT_1x_LSB_DIV		9302
+#define CSA_VOLT_4x_LSB_DIV		37202
+#define CSA_VOLT_8x_LSB_DIV		74404
+
+#define CSA_VOLT_1x_OFFS_RAW		1
+#define CSA_VOLT_4x_OFFS_RAW		1
+#define CSA_VOLT_8x_OFFS_RAW		3
+#define CSA_VOLT_1x_OFFS_uV		100
+#define CSA_VOLT_4x_OFFS_uV		45
+#define CSA_VOLT_8x_OFFS_uV		45
+
+#define CSA_VOLT_IIO_RAW(_r, _offs)	\
+	((MAX961x_VOLTAGE_RAW(_r) - (_offs)) * 1000)
+
+/*
+ * max961x common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
+ *
+ * The complete formula to calculate input common voltage is:
+ *     (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
+ */
+#define CIM_VOLTAGE_LSB_mV		14
+#define CIM_VOLTAGE_OFFSET_mV		14
+#define CIM_VOLTAGE_OFFSET_RAW		1
+
+/*
+ * max961x temperature reading: LSB is 0.48 degrees Celsius
+ *
+ * The complete formula to calculate temperature is:
+ *     ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
+ */
+#define TEMP_SHIFT			0x07
+#define TEMP_MAX_RAW_POS		0x7f80
+#define TEMP_MAX_RAW_NEG		0xff80
+#define TEMP_MIN_RAW_NEG		0xd980
+#define TEMP_MASK			((1 << TEMP_SHIFT) - 1)
+#define TEMP_RAW(_r)			((_r) >> TEMP_SHIFT)
+#define TEMP_LSB_mC			480
+#define TEMP_SCALE_NUM			1000
+#define TEMP_SCALE_DIV			2083
+
+struct max961x_dev {
+	struct device *dev;
+	struct i2c_client *i2c_client;
+	unsigned int shunt_resistor_uOhm;
+};
+
+enum max961x_conf_ids {
+	CONF_SENSE_1x,
+	CONF_SENSE_4x,
+	CONF_SENSE_8x,
+	CONF_IN_VOLT,
+	CONF_TEMP,
+};
+
+/**
+ * max961x_conf - associate ADC mux configuration with register address where
+ *		  data shall be read from
+ */
+static unsigned int max961x_conf[][2] = {
+	/* CONF_SENSE_1x */
+	{ MUX_SENSE_1x, REG_CSA_DATA },
+	/* CONF_SENSE_4x */
+	{ MUX_SENSE_4x, REG_CSA_DATA },
+	/* CONF_SENSE_8x */
+	{ MUX_SENSE_8x, REG_CSA_DATA },
+	/* CONF_IN_VOLT */
+	{ MUX_INPUT_VOLTAGE, REG_RS_DATA },
+	/* CONF_TEMP */
+	{ MUX_TEMPERATURE, REG_TEMP_DATA },
+};
+
+enum max961x_csa_gain {
+	CSA_GAIN_1x,
+	CSA_GAIN_4x,
+	CSA_GAIN_8x,
+};
+
+enum max961x_csa_gain_params {
+	CSA_GAIN_LSB_nV,
+	CSA_GAIN_LSB_DIV,
+	CSA_GAIN_OFFS_nV,
+	CSA_GAIN_OFFS_RAW,
+};
+
+/**
+ * max961x_csa_gain_conf - associate gain multiplier with LSB and
+ *			   offset values.
+ *
+ * Group together parameters associated with configurable gain
+ * on current sense amplifier path to ADC interface.
+ * Current sense read routine adjusts gain until it gets a meaningful
+ * value; use this structure to retrieve the correct LSB and offset values.
+ */
+static unsigned int max961x_gain_conf[][4] = {
+	{ /* [0] CSA_GAIN_1x */
+		CSA_VOLT_1x_LSB_nV,
+		CSA_VOLT_1x_LSB_DIV,
+		CSA_VOLT_1x_OFFS_uV,
+		CSA_VOLT_1x_OFFS_RAW,
+	},
+	{ /* [1] CSA_GAIN_4x */
+		CSA_VOLT_4x_LSB_nV,
+		CSA_VOLT_4x_LSB_DIV,
+		CSA_VOLT_4x_OFFS_uV,
+		CSA_VOLT_4x_OFFS_RAW,
+	},
+	{ /* [2] CSA_GAIN_8x */
+		CSA_VOLT_8x_LSB_nV,
+		CSA_VOLT_8x_LSB_DIV,
+		CSA_VOLT_8x_OFFS_uV,
+		CSA_VOLT_8x_OFFS_RAW,
+	},
+};
+
+/**
+ * max961x_chan_ids - Identify IIO channels
+ */
+enum max961x_chan_ids {
+	MAX961x_CHAN_VOLTAGE_INPUT,
+	MAX961x_CHAN_VOLTAGE_SENSE,
+	MAX961x_CHAN_TEMPERATURE,
+	MAX961x_CHAN_CURRENT_LOAD,
+	MAX961x_CHAN_POWER_LOAD,
+};
+
+static struct iio_chan_spec max961x_channels[] = {
+	{ /* [0] die temperature */
+	  .type			= IIO_TEMP,
+	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+				  BIT(IIO_CHAN_INFO_SCALE) |
+				  BIT(IIO_CHAN_INFO_PROCESSED),
+	  .address		= MAX961x_CHAN_TEMPERATURE,
+	},
+	{ /* [1] common voltage input */
+	  .type			= IIO_VOLTAGE,
+	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+				  BIT(IIO_CHAN_INFO_SCALE) |
+				  BIT(IIO_CHAN_INFO_OFFSET),
+	  .address		= MAX961x_CHAN_VOLTAGE_INPUT,
+	  .channel		= MAX961x_CHAN_VOLTAGE_INPUT,
+	  .indexed		= 1,
+	},
+	{ /* [2] current sense amplifier voltage output */
+	  .type			= IIO_VOLTAGE,
+	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+				  BIT(IIO_CHAN_INFO_SCALE) |
+				  BIT(IIO_CHAN_INFO_OFFSET),
+	  .address		= MAX961x_CHAN_VOLTAGE_SENSE,
+	  .channel		= MAX961x_CHAN_VOLTAGE_SENSE,
+	  .indexed		= 1,
+	},
+	{ /* [3] load current measurement */
+	  .type			= IIO_CURRENT,
+	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+				  BIT(IIO_CHAN_INFO_SCALE),
+	  .address		= MAX961x_CHAN_CURRENT_LOAD,
+	},
+	{ /* [4] load current measurement */
+	  .type			= IIO_POWER,
+	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
+				  BIT(IIO_CHAN_INFO_SCALE),
+	  .address		= MAX961x_CHAN_POWER_LOAD,
+	},
+};
+
+static int max961x_reg_read(struct max961x_dev *max961x, u8 reg, u16 *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_word_swapped(max961x->i2c_client, reg);
+	if (ret < 0) {
+		dev_err(max961x->dev, "i2c read word from 0x%2x failed\n",
+			reg);
+		return ret;
+	}
+
+	*val = ret;
+
+	return 0;
+}
+
+static int max961x_reg_write(struct max961x_dev *max961x, u8 reg, u8 val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(max961x->i2c_client, reg, val);
+	if (ret) {
+		dev_err(max961x->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
+			reg, val);
+		return ret;
+	}
+
+	/* FIXME: need a delay here to make register configuration
+	 *	  stabilize. 1 msec at least, from empirical testing.
+	 */
+	usleep_range(1000, 2000);
+
+	return 0;
+}
+
+/**
+ * max961x_read_single() - read a single vale from ADC interface
+ *
+ * Data registers are 16 bit long, spread between two 8 bit registers
+ * with consecutive addresses.
+ * Configure ADC mux first, then read register at address "reg_addr".
+ * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
+ * to return values from "reg_addr" and "reg_addr + 1" consecutively.
+ *
+ * @max961x: max961x device
+ * @selector: index for mux and register configuration
+ * @raw_val: the value returned from ADC
+ */
+static int max961x_read_single(struct max961x_dev *max961x,
+			       enum max961x_conf_ids selector,
+			       u16 *raw_val)
+{
+	int ret;
+
+	u8 mux_conf = max961x_conf[selector][0] & MUX_MASK;
+	u8 reg_addr = max961x_conf[selector][1];
+
+	ret = max961x_reg_write(max961x, REG_CTRL1, mux_conf);
+	if (ret)
+		return ret;
+
+	ret = max961x_reg_read(max961x, reg_addr, raw_val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/**
+ * max961x_read_csa_voltage() - read current sense amplifier output voltage
+ *
+ * Current sense amplifier output voltage is read through a configurable
+ * 1x, 4x or 8x gain.
+ * Start with plain 1x gain, and adjust gain control properly until a
+ * meaningful value is read from ADC output.
+ *
+ * @max961x: max961x device
+ * @adc_raw: raw value read from ADC output
+ * @csa_gain: gain configuration option selector
+ */
+static int max961x_read_csa_voltage(struct max961x_dev *max961x,
+				    u16 *adc_raw,
+				    enum max961x_csa_gain *csa_gain)
+{
+	int ret;
+	unsigned int i;
+	enum max961x_conf_ids gain_selectors[] = {
+		CONF_SENSE_1x,
+		CONF_SENSE_4x,
+		CONF_SENSE_8x
+	};
+
+	for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
+		ret = max961x_read_single(max961x, gain_selectors[i], adc_raw);
+		if (ret)
+			return ret;
+
+		if (*adc_raw > 0) {
+			*csa_gain = gain_selectors[i];
+			return 0;
+		}
+	}
+
+	return -EIO;
+}
+
+static int max961x_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	int ret;
+	u16 adc_data;
+	enum max961x_csa_gain gain_selector;
+	unsigned int *csa_gain;
+	struct max961x_dev *dev = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->address) {
+		case MAX961x_CHAN_TEMPERATURE:
+			ret = max961x_read_single(dev, CONF_TEMP,
+						  &adc_data);
+			if (ret)
+				return ret;
+
+			*val = TEMP_RAW(adc_data);
+
+			return IIO_VAL_INT;
+
+		case MAX961x_CHAN_VOLTAGE_INPUT:
+			ret = max961x_read_single(dev, CONF_IN_VOLT,
+						  &adc_data);
+			if (ret)
+				return ret;
+
+			*val = MAX961x_VOLTAGE_RAW(adc_data);
+
+			return IIO_VAL_INT;
+
+		case MAX961x_CHAN_VOLTAGE_SENSE:
+			ret = max961x_read_csa_voltage(dev, &adc_data,
+						       &gain_selector);
+			if (ret)
+				return ret;
+
+			*val = MAX961x_VOLTAGE_RAW(adc_data);
+
+			return IIO_VAL_INT;
+
+		case MAX961x_CHAN_CURRENT_LOAD:
+			/* raw (nV): raw * LSB (nV) */
+			ret = max961x_read_csa_voltage(dev, &adc_data,
+						       &gain_selector);
+			if (ret)
+				return ret;
+
+			csa_gain = max961x_gain_conf[gain_selector];
+
+			*val = MAX961x_VOLTAGE_RAW(adc_data) *
+			       csa_gain[CSA_GAIN_LSB_nV];
+
+			return IIO_VAL_INT;
+
+		case MAX961x_CHAN_POWER_LOAD:
+			/* raw (mV): raw * LSB (mV) */
+			ret = max961x_read_single(dev, CONF_IN_VOLT,
+						  &adc_data);
+
+			*val = MAX961x_VOLTAGE_RAW(adc_data) *
+			       CIM_VOLTAGE_LSB_mV;
+
+			return IIO_VAL_INT;
+		}
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->address) {
+
+		case MAX961x_CHAN_TEMPERATURE:
+			/* processed (C): raw * scale (C) */
+			*val = TEMP_SCALE_NUM;
+			*val2 = TEMP_SCALE_DIV;
+
+			return IIO_VAL_FRACTIONAL;
+
+		case MAX961x_CHAN_VOLTAGE_INPUT:
+			/* processed (mV): raw * scale_mV */
+			ret = max961x_read_single(dev, CONF_IN_VOLT,
+						  &adc_data);
+			if (ret)
+				return ret;
+
+			*val = CIM_VOLTAGE_LSB_mV;
+
+			return IIO_VAL_INT;
+
+		case MAX961x_CHAN_VOLTAGE_SENSE:
+			/* processed (mV): raw (mV) * (scale (nV) / 10^6) */
+			ret = max961x_read_csa_voltage(dev, &adc_data,
+						       &gain_selector);
+			if (ret)
+				return ret;
+
+			csa_gain = max961x_gain_conf[gain_selector];
+
+			*val = csa_gain[CSA_GAIN_LSB_nV];
+			*val2 = 1000000;
+
+			return IIO_VAL_FRACTIONAL;
+
+		case MAX961x_CHAN_CURRENT_LOAD:
+			/* processed (mA): raw (nV)  / scale (uOhm)  */
+			*val = 1;
+			*val2 = dev->shunt_resistor_uOhm;
+
+			return IIO_VAL_FRACTIONAL;
+
+		case MAX961x_CHAN_POWER_LOAD:
+			/* processed (mW): raw (mV) * (scale (mA) / 1000) */
+			ret = max961x_read_csa_voltage(dev, &adc_data,
+						       &gain_selector);
+			if (ret)
+				return ret;
+
+			csa_gain = max961x_gain_conf[gain_selector];
+
+			/* val = (nV / uOhm) = mA */
+			*val = MAX961x_VOLTAGE_RAW(adc_data) *
+			       csa_gain[CSA_GAIN_LSB_nV];
+			*val /= dev->shunt_resistor_uOhm;
+
+			*val2 = 1000;
+
+			return IIO_VAL_FRACTIONAL;
+		}
+
+	case IIO_CHAN_INFO_OFFSET:
+		switch (chan->address) {
+		case MAX961x_CHAN_VOLTAGE_INPUT:
+			*val = CIM_VOLTAGE_OFFSET_RAW;
+
+			return IIO_VAL_INT;
+
+		case MAX961x_CHAN_VOLTAGE_SENSE:
+			ret = max961x_read_csa_voltage(dev, &adc_data,
+						       &gain_selector);
+			if (ret)
+				return ret;
+
+			*val = max961x_gain_conf[gain_selector]
+						[CSA_GAIN_OFFS_RAW];
+			*val2 = 1000;
+
+			return IIO_VAL_FRACTIONAL;
+		}
+
+	case IIO_CHAN_INFO_PROCESSED:
+		/* processed (mC): raw * LSB (mC) */
+		if (chan->address != MAX961x_CHAN_TEMPERATURE)
+			return -EINVAL;
+
+		ret = max961x_read_single(dev, CONF_TEMP,
+					  &adc_data);
+		if (ret)
+			return ret;
+
+		*val = TEMP_RAW(adc_data) * TEMP_LSB_mC;
+
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t max961x_shunt_resistor_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct max961x_dev *max961x = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%d\n", max961x->shunt_resistor_uOhm);
+}
+
+static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO,
+		       max961x_shunt_resistor_show, NULL, 0);
+
+static struct attribute *max961x_attributes[] = {
+	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group max961x_attribute_group = {
+	.attrs = max961x_attributes,
+};
+
+static const struct iio_info indio_info = {
+	.driver_module	= THIS_MODULE,
+	.read_raw	= max961x_read_raw,
+	.attrs		= &max961x_attribute_group,
+};
+
+static int max961x_init(struct max961x_dev *max961x)
+{
+	int ret;
+	u16 regval;
+	struct i2c_client *client = max961x->i2c_client;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_WRITE_BYTE	|
+				     I2C_FUNC_SMBUS_READ_WORD_DATA)) {
+		dev_err(max961x->dev,
+			"I2c adapter does not support smbus write_byte and "\
+			"read_word_data functionalities. Aborting probe.\n");
+		return -EINVAL;
+	}
+
+	max961x_reg_write(max961x, REG_CTRL1, MUX_TEMPERATURE);
+	max961x_reg_write(max961x, REG_CTRL2, 0);
+
+	/* Make sure die temperature is in range to test communications. */
+	regval = 0;
+	ret = max961x_reg_read(max961x, REG_TEMP_DATA, &regval);
+	if (ret)
+		return ret;
+
+	regval = regval & ~TEMP_MASK;
+	if ((regval > TEMP_MAX_RAW_POS &&
+	     regval < TEMP_MIN_RAW_NEG) ||
+	     regval > TEMP_MAX_RAW_NEG) {
+		dev_err(max961x->dev,
+			"In-valid value received from ADC 0x%4x: aborting\n",
+			regval);
+		return -EIO;
+	}
+
+	/* Mux shall be zeroed back before applying other configurations */
+	max961x_reg_write(max961x, REG_CTRL1, 0);
+
+	return 0;
+}
+
+static int max961x_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	int ret;
+	struct max961x_dev *max961x;
+	struct iio_dev *indio_dev;
+	struct device_node *of_node = client->dev.of_node;
+	unsigned int of_shunt;
+	const char * const shunt_res_prop = "shunt-resistor";
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max961x));
+	if (IS_ERR(indio_dev))
+		return PTR_ERR(indio_dev);
+
+	i2c_set_clientdata(client, indio_dev);
+
+	max961x		= iio_priv(indio_dev);
+	max961x->dev	= &client->dev;
+	max961x->i2c_client = client;
+
+	ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
+	if (ret) {
+		dev_err(&client->dev,
+			"Missing %s property for %s node\n",
+			shunt_res_prop, of_node->full_name);
+		return ret;
+	}
+	max961x->shunt_resistor_uOhm = of_shunt;
+
+	ret = max961x_init(max961x);
+	if (ret)
+		return ret;
+
+	indio_dev->dev.parent	= &client->dev;
+	indio_dev->dev.of_node	= client->dev.of_node;
+	indio_dev->name		= DRIVER_NAME;
+	indio_dev->modes	= INDIO_DIRECT_MODE;
+	indio_dev->info		= &indio_info;
+	indio_dev->channels	= max961x_channels;
+	indio_dev->num_channels	= ARRAY_SIZE(max961x_channels);
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	dev_info(&client->dev, "%s: probed\n", DRIVER_NAME);
+
+	return 0;
+}
+
+static int max961x_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+
+	return 0;
+}
+
+static const struct of_device_id max961x_of_table[] = {
+	{.compatible = "maxim,max961x"},
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, max961x_of_table);
+
+static struct i2c_driver max961x_driver = {
+	.driver = {
+		   .name = DRIVER_NAME,
+		   .owner = THIS_MODULE,
+		   .of_match_table = max961x_of_table,
+	},
+	.probe = max961x_probe,
+	.remove = max961x_remove,
+};
+module_i2c_driver(max961x_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
+MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH 4/4] arm64: dts: salvator-x: Add current sense amplifiers

[Jacopo Mondi]

Add device nodes for two Maxim max961x current sense amplifiers
sensing VDD_08 and DVFS_08 lines.

Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
---
 arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
index fd05c2b..c533aef 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
@@ -261,6 +261,23 @@
 	status = "okay";
 };
 
+&i2c4 {
+	status = "okay";
+
+	csa_vdd: max9611_vdd@7c {
+		compatible = "maxim,max961x";
+		reg = <0x7c>;
+
+		shunt-resistor = <5000>;
+	};
+
+	csa_dvfs: max9611_dvfs@7f {
+		compatible = "maxim,max961x";
+		reg = <0x7f>;
+		shunt-resistor = <5000>;
+	};
+};
+
 &wdt0 {
 	timeout-sec = <60>;
 	status = "okay";
-- 
2.7.4

[PATCH 4/4] arm64: dts: salvator-x: Add current sense amplifiers

[Jacopo Mondi]

Add device nodes for two Maxim max961x current sense amplifiers
sensing VDD_08 and DVFS_08 lines.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
index fd05c2b..c533aef 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
@@ -261,6 +261,23 @@
 	status = "okay";
 };
 
+&i2c4 {
+	status = "okay";
+
+	csa_vdd: max9611_vdd@7c {
+		compatible = "maxim,max961x";
+		reg = <0x7c>;
+
+		shunt-resistor = <5000>;
+	};
+
+	csa_dvfs: max9611_dvfs@7f {
+		compatible = "maxim,max961x";
+		reg = <0x7f>;
+		shunt-resistor = <5000>;
+	};
+};
+
 &wdt0 {
 	timeout-sec = <60>;
 	status = "okay";
-- 
2.7.4

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Geert Uytterhoeven]

On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> wrote:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
> @@ -0,0 +1,27 @@
> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
> +
> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
> +12-bits ADC communicating over I2c bus.
> +The device node for this driver shall be a child of a I2c controller.
> +
> +Required properties
> +  - compatible: Should be "maxim,max961x"
> +  - reg: The 7-bits long I2c address of the device
> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
> +                   resistor.

shunt-resistor-micro-ohms?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Geert Uytterhoeven]

On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
> @@ -0,0 +1,27 @@
> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
> +
> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
> +12-bits ADC communicating over I2c bus.
> +The device node for this driver shall be a child of a I2c controller.
> +
> +Required properties
> +  - compatible: Should be "maxim,max961x"
> +  - reg: The 7-bits long I2c address of the device
> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
> +                   resistor.

shunt-resistor-micro-ohms?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Geert Uytterhoeven]

Hi Jacopo,

[fixed Peter Meerwald's address]

On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> wrote:
> Add device tree bindings documentation for Maxim max961x current sense
> amplifier.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
> ---
>  .../devicetree/bindings/iio/adc/max961x.txt        | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/max961x.txt b/Documentation/devicetree/bindings/iio/adc/max961x.txt
> new file mode 100644
> index 0000000..abbc6c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
> @@ -0,0 +1,27 @@
> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
> +
> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
> +12-bits ADC communicating over I2c bus.
> +The device node for this driver shall be a child of a I2c controller.
> +
> +Required properties
> +  - compatible: Should be "maxim,max961x"

I'ts not a good idea to use wildcards in compatible values.
Ask yourself: is this valid for any (future) value of "x"?

Hence please use "maxim,max9611" or "maxim,max9612".
The only difference between these two is a noninverting vs. inverting
input-to-output configuration. Does the driver need to care?

> +  - reg: The 7-bits long I2c address of the device
> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
> +                   resistor.

shunt-resistor-micro-ohms?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Geert Uytterhoeven]

Hi Jacopo,

[fixed Peter Meerwald's address]

On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add device tree bindings documentation for Maxim max961x current sense
> amplifier.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/iio/adc/max961x.txt        | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/max961x.txt b/Documentation/devicetree/bindings/iio/adc/max961x.txt
> new file mode 100644
> index 0000000..abbc6c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
> @@ -0,0 +1,27 @@
> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
> +
> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
> +12-bits ADC communicating over I2c bus.
> +The device node for this driver shall be a child of a I2c controller.
> +
> +Required properties
> +  - compatible: Should be "maxim,max961x"

I'ts not a good idea to use wildcards in compatible values.
Ask yourself: is this valid for any (future) value of "x"?

Hence please use "maxim,max9611" or "maxim,max9612".
The only difference between these two is a noninverting vs. inverting
input-to-output configuration. Does the driver need to care?

> +  - reg: The 7-bits long I2c address of the device
> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
> +                   resistor.

shunt-resistor-micro-ohms?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Lars-Peter Clausen]

On 02/24/2017 04:22 PM, Geert Uytterhoeven wrote:
> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> wrote:
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>> @@ -0,0 +1,27 @@
>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>> +
>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>> +12-bits ADC communicating over I2c bus.
>> +The device node for this driver shall be a child of a I2c controller.
>> +
>> +Required properties
>> +  - compatible: Should be "maxim,max961x"
>> +  - reg: The 7-bits long I2c address of the device
>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>> +                   resistor.
> 
> shunt-resistor-micro-ohms?

I'll take this one further:

maxim,shunt-resistor-micro-ohms?

Although there is precedence for just 'shunt-resistor' in the ina2xx bindings.

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Lars-Peter Clausen]

On 02/24/2017 04:22 PM, Geert Uytterhoeven wrote:
> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>> @@ -0,0 +1,27 @@
>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>> +
>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>> +12-bits ADC communicating over I2c bus.
>> +The device node for this driver shall be a child of a I2c controller.
>> +
>> +Required properties
>> +  - compatible: Should be "maxim,max961x"
>> +  - reg: The 7-bits long I2c address of the device
>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>> +                   resistor.
> 
> shunt-resistor-micro-ohms?

I'll take this one further:

maxim,shunt-resistor-micro-ohms?

Although there is precedence for just 'shunt-resistor' in the ina2xx bindings.

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[jacopo mondi]

Hi Lars-Peter,

On 24/02/2017 16:33, Lars-Peter Clausen wrote:
> On 02/24/2017 04:22 PM, Geert Uytterhoeven wrote:
>> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> wrote:
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>>> @@ -0,0 +1,27 @@
>>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>>> +
>>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>>> +12-bits ADC communicating over I2c bus.
>>> +The device node for this driver shall be a child of a I2c controller.
>>> +
>>> +Required properties
>>> +  - compatible: Should be "maxim,max961x"
>>> +  - reg: The 7-bits long I2c address of the device
>>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>>> +                   resistor.
>>
>> shunt-resistor-micro-ohms?
>
> I'll take this one further:
>
> maxim,shunt-resistor-micro-ohms?
>
> Although there is precedence for just 'shunt-resistor' in the ina2xx bindings.
>

And that's where I took "inspiration" from :)

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[jacopo mondi]

Hi Lars-Peter,

On 24/02/2017 16:33, Lars-Peter Clausen wrote:
> On 02/24/2017 04:22 PM, Geert Uytterhoeven wrote:
>> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>>> @@ -0,0 +1,27 @@
>>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>>> +
>>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>>> +12-bits ADC communicating over I2c bus.
>>> +The device node for this driver shall be a child of a I2c controller.
>>> +
>>> +Required properties
>>> +  - compatible: Should be "maxim,max961x"
>>> +  - reg: The 7-bits long I2c address of the device
>>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>>> +                   resistor.
>>
>> shunt-resistor-micro-ohms?
>
> I'll take this one further:
>
> maxim,shunt-resistor-micro-ohms?
>
> Although there is precedence for just 'shunt-resistor' in the ina2xx bindings.
>

And that's where I took "inspiration" from :)

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[jacopo mondi]

Hi Geert,

On 24/02/2017 16:29, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> [fixed Peter Meerwald's address]
>
> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> wrote:
>> Add device tree bindings documentation for Maxim max961x current sense
>> amplifier.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
>> ---
>>  .../devicetree/bindings/iio/adc/max961x.txt        | 27 ++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/max961x.txt b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>> new file mode 100644
>> index 0000000..abbc6c4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>> @@ -0,0 +1,27 @@
>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>> +
>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>> +12-bits ADC communicating over I2c bus.
>> +The device node for this driver shall be a child of a I2c controller.
>> +
>> +Required properties
>> +  - compatible: Should be "maxim,max961x"
>
> I'ts not a good idea to use wildcards in compatible values.
> Ask yourself: is this valid for any (future) value of "x"?
>

I suspect you already know the answer here :)

> Hence please use "maxim,max9611" or "maxim,max9612".
> The only difference between these two is a noninverting vs. inverting
> input-to-output configuration. Does the driver need to care?

No, not right now, as the inverting/non-inverting output is on the 
op-amp/comparator side, which is currently not supported by this driver.

Instead of using one or the other, should we use both, as long as it 
does not makes any difference from driver perspective?

Thanks
   j


>
>> +  - reg: The 7-bits long I2c address of the device
>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>> +                   resistor.
>
> shunt-resistor-micro-ohms?
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[jacopo mondi]

Hi Geert,

On 24/02/2017 16:29, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> [fixed Peter Meerwald's address]
>
> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>> Add device tree bindings documentation for Maxim max961x current sense
>> amplifier.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> ---
>>  .../devicetree/bindings/iio/adc/max961x.txt        | 27 ++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/max961x.txt b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>> new file mode 100644
>> index 0000000..abbc6c4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>> @@ -0,0 +1,27 @@
>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>> +
>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>> +12-bits ADC communicating over I2c bus.
>> +The device node for this driver shall be a child of a I2c controller.
>> +
>> +Required properties
>> +  - compatible: Should be "maxim,max961x"
>
> I'ts not a good idea to use wildcards in compatible values.
> Ask yourself: is this valid for any (future) value of "x"?
>

I suspect you already know the answer here :)

> Hence please use "maxim,max9611" or "maxim,max9612".
> The only difference between these two is a noninverting vs. inverting
> input-to-output configuration. Does the driver need to care?

No, not right now, as the inverting/non-inverting output is on the 
op-amp/comparator side, which is currently not supported by this driver.

Instead of using one or the other, should we use both, as long as it 
does not makes any difference from driver perspective?

Thanks
   j


>
>> +  - reg: The 7-bits long I2c address of the device
>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>> +                   resistor.
>
> shunt-resistor-micro-ohms?
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Geert Uytterhoeven]

Hi Jacopo,

On Fri, Feb 24, 2017 at 4:50 PM, jacopo mondi <jacopo@jmondi.org> wrote:
> On 24/02/2017 16:29, Geert Uytterhoeven wrote:
>> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org>
>> wrote:
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>>> @@ -0,0 +1,27 @@
>>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC
>>> interface
>>> +
>>> +Maxim max9611/max9612 is an high-side current sense amplifier with
>>> integrated
>>> +12-bits ADC communicating over I2c bus.
>>> +The device node for this driver shall be a child of a I2c controller.
>>> +
>>> +Required properties
>>> +  - compatible: Should be "maxim,max961x"
>>
>>
>> I'ts not a good idea to use wildcards in compatible values.
>> Ask yourself: is this valid for any (future) value of "x"?
>
> I suspect you already know the answer here :)
>
>> Hence please use "maxim,max9611" or "maxim,max9612".
>> The only difference between these two is a noninverting vs. inverting
>> input-to-output configuration. Does the driver need to care?
>
> No, not right now, as the inverting/non-inverting output is on the
> op-amp/comparator side, which is currently not supported by this driver.
>
> Instead of using one or the other, should we use both, as long as it does
> not makes any difference from driver perspective?

DT describes the hardware, not current software limitations.
So yes, we want both (in the bindings/drivers, not in the same *.dts file ;-).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Jonathan Cameron]

On 24/02/17 15:48, jacopo mondi wrote:
> Hi Lars-Peter,
> 
> On 24/02/2017 16:33, Lars-Peter Clausen wrote:
>> On 02/24/2017 04:22 PM, Geert Uytterhoeven wrote:
>>> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>> @@ -0,0 +1,27 @@
>>>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>>>> +
>>>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>>>> +12-bits ADC communicating over I2c bus.
>>>> +The device node for this driver shall be a child of a I2c controller.
>>>> +
>>>> +Required properties
>>>> +  - compatible: Should be "maxim,max961x"
>>>> +  - reg: The 7-bits long I2c address of the device
>>>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>>>> +                   resistor.
>>>
>>> shunt-resistor-micro-ohms?
>>
>> I'll take this one further:
>>
>> maxim,shunt-resistor-micro-ohms?
>>
>> Although there is precedence for just 'shunt-resistor' in the ina2xx bindings.
>>
> 
> And that's where I took "inspiration" from :)
Which raises another open question.  Why IIO rather than hwmon like the ina2xx?

cc'd Guenter and hwmon list.

I don't typically have strong opinions on this but best to make sure everyone
is happy.  Always best to layout your thinking on this in the cover letter.

I'll raise it there as well but came to mind when seeing the ina2xx reference
here.

Whether we are better going for the existing binding without units, or
'fixing' that is a question for the device tree maintainers.  I guess that
one snuck through.

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

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Geert Uytterhoeven]

Hi Jonathan,

On Sat, Feb 25, 2017 at 4:19 PM, Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On 24/02/17 15:48, jacopo mondi wrote:
>> On 24/02/2017 16:33, Lars-Peter Clausen wrote:
>>> On 02/24/2017 04:22 PM, Geert Uytterhoeven wrote:
>>>> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> wrote:
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>>> @@ -0,0 +1,27 @@
>>>>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>>>>> +
>>>>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>>>>> +12-bits ADC communicating over I2c bus.
>>>>> +The device node for this driver shall be a child of a I2c controller.
>>>>> +
>>>>> +Required properties
>>>>> +  - compatible: Should be "maxim,max961x"
>>>>> +  - reg: The 7-bits long I2c address of the device
>>>>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>>>>> +                   resistor.
>>>>
>>>> shunt-resistor-micro-ohms?
>>>
>>> I'll take this one further:
>>>
>>> maxim,shunt-resistor-micro-ohms?
>>>
>>> Although there is precedence for just 'shunt-resistor' in the ina2xx bindings.
>>>
>>
>> And that's where I took "inspiration" from :)
> Which raises another open question.  Why IIO rather than hwmon like the ina2xx?

Actually we have both drivers/hwmon/ina2xx.c and drivers/iio/adc/ina2xx-adc.c.

> Whether we are better going for the existing binding without units, or
> 'fixing' that is a question for the device tree maintainers.  I guess that
> one snuck through.

ltc4151 uses shunt-resistor-micro-ohms.
sgtl5000 uses micbias-resistor-k-ohms.

So there's precedence for everything ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Geert Uytterhoeven]

Hi Jonathan,

On Sat, Feb 25, 2017 at 4:19 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 24/02/17 15:48, jacopo mondi wrote:
>> On 24/02/2017 16:33, Lars-Peter Clausen wrote:
>>> On 02/24/2017 04:22 PM, Geert Uytterhoeven wrote:
>>>> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>>> @@ -0,0 +1,27 @@
>>>>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>>>>> +
>>>>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>>>>> +12-bits ADC communicating over I2c bus.
>>>>> +The device node for this driver shall be a child of a I2c controller.
>>>>> +
>>>>> +Required properties
>>>>> +  - compatible: Should be "maxim,max961x"
>>>>> +  - reg: The 7-bits long I2c address of the device
>>>>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>>>>> +                   resistor.
>>>>
>>>> shunt-resistor-micro-ohms?
>>>
>>> I'll take this one further:
>>>
>>> maxim,shunt-resistor-micro-ohms?
>>>
>>> Although there is precedence for just 'shunt-resistor' in the ina2xx bindings.
>>>
>>
>> And that's where I took "inspiration" from :)
> Which raises another open question.  Why IIO rather than hwmon like the ina2xx?

Actually we have both drivers/hwmon/ina2xx.c and drivers/iio/adc/ina2xx-adc.c.

> Whether we are better going for the existing binding without units, or
> 'fixing' that is a question for the device tree maintainers.  I guess that
> one snuck through.

ltc4151 uses shunt-resistor-micro-ohms.
sgtl5000 uses micbias-resistor-k-ohms.

So there's precedence for everything ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] iio: adc: Add max9611/9612 ADC driver

[Jonathan Cameron]

On 24/02/17 15:05, Jacopo Mondi wrote:
> Add iio driver for Maxim max9611/9612 current-sense amplifiers with
> 12-bits ADC.
Data sheet link always good
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
I'm going to respond to the cover letter wrt to providing dynamic scales
and offsets in a moment as you raised the question there.

Otherwise, various bits and bobs inline.

Interesting part to handle as it can read a lot of values that are useful only
after some magic calculations are done.

Jonathan
> ---
>  drivers/iio/adc/Kconfig   |  10 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/max961x.c | 648 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 659 insertions(+)
>  create mode 100644 drivers/iio/adc/max961x.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7a..f86026a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -354,6 +354,16 @@ config MAX1363
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1363.
>  
> +config	MAX961x
> +	tristate "Maxim max9611/9612 ADC driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Maxim max9611/9612 current sense
> +	  amplifiers with 12-bits ADC interface.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max961x.
> +
>  config MCP320X
>  	tristate "Microchip Technology MCP3x01/02/04/08"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d001262..ff19250 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> +obj-$(CONFIG_MAX961x) += max961x.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
>  obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> diff --git a/drivers/iio/adc/max961x.c b/drivers/iio/adc/max961x.c
> new file mode 100644
> index 0000000..a544e69
> --- /dev/null
> +++ b/drivers/iio/adc/max961x.c
> @@ -0,0 +1,648 @@
> +/*
> + * iio/adc/max961x.c
> + *
> + * Maxim max9611/9612 high side current sense amplifier with
> + * 12-bit ADC interface.
> + *
> + * Copyright (C) 2017 Jacopo Mondi
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * This driver supports input common-mode voltage, current-sense
> + * amplifier with programmable gains and die temperature reading from
> + * Maxim max9611/9612.
> + * Op-amp, analog comparator, and watchdog functionalities are not
> + * supported by this driver.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +
> +#define DRIVER_NAME "max961x"
> +
> +/* max961x register addresses */
Prefix these with MAX9611_ to avoid possible clashes in future.

> +#define REG_CSA_DATA			0x00
> +#define REG_RS_DATA			0x02
> +#define REG_TEMP_DATA			0x08
> +#define REG_CTRL1			0x0a
> +#define REG_CTRL2			0x0b
> +
> +/* max961x REG1 mux configuration options */
> +#define MUX_MASK			0x07
> +#define MUX_SENSE_1x			0x00
> +#define MUX_SENSE_4x			0x01
> +#define MUX_SENSE_8x			0x02
> +#define MUX_INPUT_VOLTAGE		0x03
> +#define MUX_TEMPERATURE			0x06
> +
> +/* max961x voltage (both sense and input) helper macros */
> +#define MAX961x_VOLTAGE_SHIFT		0x04
> +#define MAX961x_VOLTAGE_RAW(_r)		((_r) >> MAX961x_VOLTAGE_SHIFT)
> +
> +/*
> + * max961x current sense amplifier voltage output:
> + * LSB and offset values depends on selected gain (1x, 4x, 8x)
> + *
> + * GAIN		LSB (nV)	OFFSET (LSB steps)
> + * 1x		107500		1
> + * 4x		26880		1
> + * 8x		13440		3
> + *
> + * The complete formula to calculate current sense voltage is:
> + *     (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
> + */
> +#define CSA_VOLT_1x_LSB_nV		107500
> +#define CSA_VOLT_4x_LSB_nV		26880
> +#define CSA_VOLT_8x_LSB_nV		13440
> +#define CSA_VOLT_1x_LSB_DIV		9302
> +#define CSA_VOLT_4x_LSB_DIV		37202
> +#define CSA_VOLT_8x_LSB_DIV		74404
> +
> +#define CSA_VOLT_1x_OFFS_RAW		1
> +#define CSA_VOLT_4x_OFFS_RAW		1
> +#define CSA_VOLT_8x_OFFS_RAW		3
> +#define CSA_VOLT_1x_OFFS_uV		100
> +#define CSA_VOLT_4x_OFFS_uV		45
> +#define CSA_VOLT_8x_OFFS_uV		45
> +
> +#define CSA_VOLT_IIO_RAW(_r, _offs)	\
> +	((MAX961x_VOLTAGE_RAW(_r) - (_offs)) * 1000)
> +
> +/*
> + * max961x common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
> + *
> + * The complete formula to calculate input common voltage is:
> + *     (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
> + */
> +#define CIM_VOLTAGE_LSB_mV		14
> +#define CIM_VOLTAGE_OFFSET_mV		14
> +#define CIM_VOLTAGE_OFFSET_RAW		1
> +
> +/*
> + * max961x temperature reading: LSB is 0.48 degrees Celsius
> + *
> + * The complete formula to calculate temperature is:
> + *     ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
> + */
> +#define TEMP_SHIFT			0x07
> +#define TEMP_MAX_RAW_POS		0x7f80
> +#define TEMP_MAX_RAW_NEG		0xff80
> +#define TEMP_MIN_RAW_NEG		0xd980
> +#define TEMP_MASK			((1 << TEMP_SHIFT) - 1)
> +#define TEMP_RAW(_r)			((_r) >> TEMP_SHIFT)
> +#define TEMP_LSB_mC			480
> +#define TEMP_SCALE_NUM			1000
> +#define TEMP_SCALE_DIV			2083
> +
> +struct max961x_dev {
> +	struct device *dev;
> +	struct i2c_client *i2c_client;
> +	unsigned int shunt_resistor_uOhm;
> +};
> +
> +enum max961x_conf_ids {
> +	CONF_SENSE_1x,
> +	CONF_SENSE_4x,
> +	CONF_SENSE_8x,
> +	CONF_IN_VOLT,
> +	CONF_TEMP,
> +};
> +
> +/**
> + * max961x_conf - associate ADC mux configuration with register address where
> + *		  data shall be read from
> + */
> +static unsigned int max961x_conf[][2] = {
> +	/* CONF_SENSE_1x */
> +	{ MUX_SENSE_1x, REG_CSA_DATA },
> +	/* CONF_SENSE_4x */
> +	{ MUX_SENSE_4x, REG_CSA_DATA },
> +	/* CONF_SENSE_8x */
> +	{ MUX_SENSE_8x, REG_CSA_DATA },
> +	/* CONF_IN_VOLT */
> +	{ MUX_INPUT_VOLTAGE, REG_RS_DATA },
> +	/* CONF_TEMP */
> +	{ MUX_TEMPERATURE, REG_TEMP_DATA },
> +};
> +
> +enum max961x_csa_gain {
> +	CSA_GAIN_1x,
> +	CSA_GAIN_4x,
> +	CSA_GAIN_8x,
> +};
> +
> +enum max961x_csa_gain_params {
> +	CSA_GAIN_LSB_nV,
> +	CSA_GAIN_LSB_DIV,
> +	CSA_GAIN_OFFS_nV,
> +	CSA_GAIN_OFFS_RAW,
> +};
> +
> +/**
> + * max961x_csa_gain_conf - associate gain multiplier with LSB and
> + *			   offset values.
> + *
> + * Group together parameters associated with configurable gain
> + * on current sense amplifier path to ADC interface.
> + * Current sense read routine adjusts gain until it gets a meaningful
> + * value; use this structure to retrieve the correct LSB and offset values.
> + */
> +static unsigned int max961x_gain_conf[][4] = {
> +	{ /* [0] CSA_GAIN_1x */
> +		CSA_VOLT_1x_LSB_nV,
> +		CSA_VOLT_1x_LSB_DIV,
> +		CSA_VOLT_1x_OFFS_uV,
> +		CSA_VOLT_1x_OFFS_RAW,
> +	},
> +	{ /* [1] CSA_GAIN_4x */
> +		CSA_VOLT_4x_LSB_nV,
> +		CSA_VOLT_4x_LSB_DIV,
> +		CSA_VOLT_4x_OFFS_uV,
> +		CSA_VOLT_4x_OFFS_RAW,
> +	},
> +	{ /* [2] CSA_GAIN_8x */
> +		CSA_VOLT_8x_LSB_nV,
> +		CSA_VOLT_8x_LSB_DIV,
> +		CSA_VOLT_8x_OFFS_uV,
> +		CSA_VOLT_8x_OFFS_RAW,
> +	},
> +};
> +
> +/**
> + * max961x_chan_ids - Identify IIO channels
> + */
> +enum max961x_chan_ids {
> +	MAX961x_CHAN_VOLTAGE_INPUT,
> +	MAX961x_CHAN_VOLTAGE_SENSE,
> +	MAX961x_CHAN_TEMPERATURE,
> +	MAX961x_CHAN_CURRENT_LOAD,
> +	MAX961x_CHAN_POWER_LOAD,
> +};
> +
> +static struct iio_chan_spec max961x_channels[] = {
> +	{ /* [0] die temperature */
> +	  .type			= IIO_TEMP,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE) |
The only cases we normally allow bother raw and processed output in
are ones where we are supporting some defunct (i.e. wrong usage) in the
userspace ABI.  So drop the processed path please.

> +				  BIT(IIO_CHAN_INFO_PROCESSED),
> +	  .address		= MAX961x_CHAN_TEMPERATURE,
> +	},
> +	{ /* [1] common voltage input */
> +	  .type			= IIO_VOLTAGE,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE) |
> +				  BIT(IIO_CHAN_INFO_OFFSET),
> +	  .address		= MAX961x_CHAN_VOLTAGE_INPUT,
> +	  .channel		= MAX961x_CHAN_VOLTAGE_INPUT,
For the channel it would be easier to read perhaps if you just put
the 0 in directly here.
> +	  .indexed		= 1,
> +	},
> +	{ /* [2] current sense amplifier voltage output */
> +	  .type			= IIO_VOLTAGE,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE) |
> +				  BIT(IIO_CHAN_INFO_OFFSET),
> +	  .address		= MAX961x_CHAN_VOLTAGE_SENSE,
> +	  .channel		= MAX961x_CHAN_VOLTAGE_SENSE,
> +	  .indexed		= 1,
> +	},

As you say, the next one is a computed channel.
> +	{ /* [3] load current measurement */
> +	  .type			= IIO_CURRENT,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE),
> +	  .address		= MAX961x_CHAN_CURRENT_LOAD,
> +	},
As is this one.
> +	{ /* [4] load current measurement */
> +	  .type			= IIO_POWER,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE),
> +	  .address		= MAX961x_CHAN_POWER_LOAD,
> +	},
> +};
> +
> +static int max961x_reg_read(struct max961x_dev *max961x, u8 reg, u16 *val)
> +{
I'm not convinced this particular wrapper adds anything.
I'd be tempted to just use the i2c read directly as needed rather than
bounce through this.

> +	int ret;
> +
> +	ret = i2c_smbus_read_word_swapped(max961x->i2c_client, reg);
> +	if (ret < 0) {
> +		dev_err(max961x->dev, "i2c read word from 0x%2x failed\n",
> +			reg);
> +		return ret;
> +	}
> +
> +	*val = ret;
> +
> +	return 0;
> +}
> +
> +static int max961x_reg_write(struct max961x_dev *max961x, u8 reg, u8 val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(max961x->i2c_client, reg, val);
> +	if (ret) {
> +		dev_err(max961x->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> +			reg, val);
> +		return ret;
> +	}
> +
> +	/* FIXME: need a delay here to make register configuration
> +	 *	  stabilize. 1 msec at least, from empirical testing.
Why fixme?  Sounds like it really needs to be the case to me!

Also, fix up your multiline comment syntax please.
> +	 */
> +	usleep_range(1000, 2000);
> +
> +	return 0;
> +}
> +
> +/**
> + * max961x_read_single() - read a single vale from ADC interface
> + *
> + * Data registers are 16 bit long, spread between two 8 bit registers
> + * with consecutive addresses.
> + * Configure ADC mux first, then read register at address "reg_addr".
> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
> + *
> + * @max961x: max961x device
> + * @selector: index for mux and register configuration
> + * @raw_val: the value returned from ADC
> + */
> +static int max961x_read_single(struct max961x_dev *max961x,
> +			       enum max961x_conf_ids selector,
> +			       u16 *raw_val)
> +{
> +	int ret;
> +
> +	u8 mux_conf = max961x_conf[selector][0] & MUX_MASK;
> +	u8 reg_addr = max961x_conf[selector][1];
> +
You have no lockign around these various sequences.
Nothing prevents multiple sysfs reads at the same time so you definitely
want to prevent this stuff from running concurrently.

A mutex should do the job, either at this low level or up in the calling
functions.
> +	ret = max961x_reg_write(max961x, REG_CTRL1, mux_conf);
> +	if (ret)
> +		return ret;
> +
> +	ret = max961x_reg_read(max961x, reg_addr, raw_val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/**
> + * max961x_read_csa_voltage() - read current sense amplifier output voltage
> + *
> + * Current sense amplifier output voltage is read through a configurable
> + * 1x, 4x or 8x gain.
> + * Start with plain 1x gain, and adjust gain control properly until a
> + * meaningful value is read from ADC output.
> + *
> + * @max961x: max961x device
> + * @adc_raw: raw value read from ADC output
> + * @csa_gain: gain configuration option selector
> + */
> +static int max961x_read_csa_voltage(struct max961x_dev *max961x,
> +				    u16 *adc_raw,
> +				    enum max961x_csa_gain *csa_gain)
> +{
> +	int ret;
> +	unsigned int i;
> +	enum max961x_conf_ids gain_selectors[] = {
> +		CONF_SENSE_1x,
> +		CONF_SENSE_4x,
> +		CONF_SENSE_8x
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
> +		ret = max961x_read_single(max961x, gain_selectors[i], adc_raw);
> +		if (ret)
> +			return ret;
> +
> +		if (*adc_raw > 0) {
> +			*csa_gain = gain_selectors[i];
> +			return 0;
> +		}
> +	}
> +
> +	return -EIO;
> +}
> +
> +static int max961x_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	int ret;
> +	u16 adc_data;
> +	enum max961x_csa_gain gain_selector;
> +	unsigned int *csa_gain;
> +	struct max961x_dev *dev = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->address) {
> +		case MAX961x_CHAN_TEMPERATURE:
> +			ret = max961x_read_single(dev, CONF_TEMP,
> +						  &adc_data);
> +			if (ret)
> +				return ret;
> +
> +			*val = TEMP_RAW(adc_data);
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_VOLTAGE_INPUT:
> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
> +						  &adc_data);
> +			if (ret)
> +				return ret;
> +
> +			*val = MAX961x_VOLTAGE_RAW(adc_data);
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_VOLTAGE_SENSE:
> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return ret;
> +
> +			*val = MAX961x_VOLTAGE_RAW(adc_data);
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_CURRENT_LOAD:
> +			/* raw (nV): raw * LSB (nV) */
> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return ret;
> +
> +			csa_gain = max961x_gain_conf[gain_selector];
> +
> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
> +			       csa_gain[CSA_GAIN_LSB_nV];
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_POWER_LOAD:
> +			/* raw (mV): raw * LSB (mV) */
> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
> +						  &adc_data);
> +
> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
> +			       CIM_VOLTAGE_LSB_mV;
> +
> +			return IIO_VAL_INT;
> +		}
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->address) {
> +
> +		case MAX961x_CHAN_TEMPERATURE:
> +			/* processed (C): raw * scale (C) */
> +			*val = TEMP_SCALE_NUM;
> +			*val2 = TEMP_SCALE_DIV;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX961x_CHAN_VOLTAGE_INPUT:
> +			/* processed (mV): raw * scale_mV */
Err, I'm confused.  We read adc_data then do nothing with it?
> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
> +						  &adc_data);
> +			if (ret)
> +				return ret;
> +
> +			*val = CIM_VOLTAGE_LSB_mV;
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_VOLTAGE_SENSE:
> +			/* processed (mV): raw (mV) * (scale (nV) / 10^6) */
> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return ret;
As with the offset below, this is a non starter to my mind as we have
no way of knowing it was the 'same' gain value.  That has to be the case
or we'll get all sorts of horrible instabilities in apparent output as
we pass from one gain setting to another.
> +
> +			csa_gain = max961x_gain_conf[gain_selector];
> +
> +			*val = csa_gain[CSA_GAIN_LSB_nV];
> +			*val2 = 1000000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX961x_CHAN_CURRENT_LOAD:
> +			/* processed (mA): raw (nV)  / scale (uOhm)  */
> +			*val = 1;
> +			*val2 = dev->shunt_resistor_uOhm;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX961x_CHAN_POWER_LOAD:
> +			/* processed (mW): raw (mV) * (scale (mA) / 1000) */
> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
Again, nothing says the gain_selector value is the one used when we
took the measurement (or will be if we take it in future) so we can't do
it this way.  Either we need to remove the computed value and provide
enough info to userspace to allow it to do the job, or we need to ensure
that the kernel data manipulation doesn't allow for any instabilities around
gain transitions.
> +			if (ret)
> +				return ret;
> +
> +			csa_gain = max961x_gain_conf[gain_selector];
> +
> +			/* val = (nV / uOhm) = mA */
> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
> +			       csa_gain[CSA_GAIN_LSB_nV];
> +			*val /= dev->shunt_resistor_uOhm;
> +
> +			*val2 = 1000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		switch (chan->address) {
> +		case MAX961x_CHAN_VOLTAGE_INPUT:
> +			*val = CIM_VOLTAGE_OFFSET_RAW;
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_VOLTAGE_SENSE:
Interesting. Nothing here ensures the reported scale is correct for
the reading we are looking it up for which is an issue.

For a compound value - i.e. one where the userspace interpretation is
dependent on several different readings (the classic being light sensors
where illuminance is often computed from two different light sensing
elementents) we tend to hide the complexity by doing it in kernel.

So my gut feeling is this should be hidden away.  That should ensure
the value used is the correct one as well.

> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return ret;
> +
> +			*val = max961x_gain_conf[gain_selector]
> +						[CSA_GAIN_OFFS_RAW];
> +			*val2 = 1000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +

As stated above, please drop this as userspace can do this trivially from
the raw and scale provided.
> +	case IIO_CHAN_INFO_PROCESSED:
> +		/* processed (mC): raw * LSB (mC) */
> +		if (chan->address != MAX961x_CHAN_TEMPERATURE)
> +			return -EINVAL;
> +
> +		ret = max961x_read_single(dev, CONF_TEMP,
> +					  &adc_data);
> +		if (ret)
> +			return ret;
> +
> +		*val = TEMP_RAW(adc_data) * TEMP_LSB_mC;
> +
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t max961x_shunt_resistor_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct max961x_dev *max961x = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", max961x->shunt_resistor_uOhm);
> +}
> +
> +static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO,
> +		       max961x_shunt_resistor_show, NULL, 0);
> +
> +static struct attribute *max961x_attributes[] = {
> +	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group max961x_attribute_group = {
> +	.attrs = max961x_attributes,
> +};
> +
> +static const struct iio_info indio_info = {
> +	.driver_module	= THIS_MODULE,
> +	.read_raw	= max961x_read_raw,
> +	.attrs		= &max961x_attribute_group,
> +};
> +
> +static int max961x_init(struct max961x_dev *max961x)
> +{
> +	int ret;
> +	u16 regval;
> +	struct i2c_client *client = max961x->i2c_client;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WRITE_BYTE	|
> +				     I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> +		dev_err(max961x->dev,
> +			"I2c adapter does not support smbus write_byte and "\
Whilst it'll break the 80 char limit by quite a way, it's preferable to do that
than to break the ability of someone to grep for an error message.  So keep
this on one line please.
> +			"read_word_data functionalities. Aborting probe.\n");
> +		return -EINVAL;
> +	}
> +
> +	max961x_reg_write(max961x, REG_CTRL1, MUX_TEMPERATURE);
> +	max961x_reg_write(max961x, REG_CTRL2, 0);
> +
> +	/* Make sure die temperature is in range to test communications. */
> +	regval = 0;
> +	ret = max961x_reg_read(max961x, REG_TEMP_DATA, &regval);
> +	if (ret)
> +		return ret;
> +
> +	regval = regval & ~TEMP_MASK;
> +	if ((regval > TEMP_MAX_RAW_POS &&
> +	     regval < TEMP_MIN_RAW_NEG) ||
> +	     regval > TEMP_MAX_RAW_NEG) {
> +		dev_err(max961x->dev,
> +			"In-valid value received from ADC 0x%4x: aborting\n",
> +			regval);
> +		return -EIO;
> +	}
> +
> +	/* Mux shall be zeroed back before applying other configurations */
> +	max961x_reg_write(max961x, REG_CTRL1, 0);
> +
> +	return 0;
> +}
> +
> +static int max961x_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct max961x_dev *max961x;
> +	struct iio_dev *indio_dev;
> +	struct device_node *of_node = client->dev.of_node;
> +	unsigned int of_shunt;
> +	const char * const shunt_res_prop = "shunt-resistor";
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max961x));
> +	if (IS_ERR(indio_dev))
> +		return PTR_ERR(indio_dev);
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	max961x		= iio_priv(indio_dev);
> +	max961x->dev	= &client->dev;
> +	max961x->i2c_client = client;
> +
> +	ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Missing %s property for %s node\n",
> +			shunt_res_prop, of_node->full_name);
> +		return ret;
> +	}
> +	max961x->shunt_resistor_uOhm = of_shunt;
> +
> +	ret = max961x_init(max961x);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->dev.parent	= &client->dev;
> +	indio_dev->dev.of_node	= client->dev.of_node;
> +	indio_dev->name		= DRIVER_NAME;
> +	indio_dev->modes	= INDIO_DIRECT_MODE;
> +	indio_dev->info		= &indio_info;
> +	indio_dev->channels	= max961x_channels;
> +	indio_dev->num_channels	= ARRAY_SIZE(max961x_channels);
> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(&client->dev, "%s: probed\n", DRIVER_NAME);
This one is always hotly debated.  To my mind, this provides
no information, but if you are particularly attached to it I don't
really mind.  If not, then return devm_iio_* directly.
> +
> +	return 0;
> +}
> +
> +static int max961x_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
If all you have in a remove function is a call to iio_device_unregister
my immediate thought is could you use devm_iio_device_register.

Ah, you are.  Double unregister!  If you are using the devm form of register
it will be unregistered automatically as part of the removal of the
underlying struct device.  Thus you don't need to do it explicitly.

Thus you can drop this remove function completely as it should be
empty.
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id max961x_of_table[] = {
> +	{.compatible = "maxim,max961x"},
Already been raised in the bindings reviews, but you shouldn't
use wild cards in compatible strings.

Actually as it almost always goes wrong, you shouldn't used them
for naming of anything.  Pick a part and use that were you
currently have max961x throughout the driver.

i.e. max9611_of_table etc.

Avoids possible issues down the line when maxim decide to
release something incompatible and call it a max9619..
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, max961x_of_table);
> +
> +static struct i2c_driver max961x_driver = {
> +	.driver = {
> +		   .name = DRIVER_NAME,
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = max961x_of_table,
> +	},
> +	.probe = max961x_probe,
> +	.remove = max961x_remove,
> +};
> +module_i2c_driver(max961x_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>");
> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> +MODULE_LICENSE("GPL v2");
> 

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

Re: [PATCH 3/4] iio: adc: Add max9611/9612 ADC driver

[Jonathan Cameron]

On 24/02/17 15:05, Jacopo Mondi wrote:
> Add iio driver for Maxim max9611/9612 current-sense amplifiers with
> 12-bits ADC.
Data sheet link always good
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
I'm going to respond to the cover letter wrt to providing dynamic scales
and offsets in a moment as you raised the question there.

Otherwise, various bits and bobs inline.

Interesting part to handle as it can read a lot of values that are useful only
after some magic calculations are done.

Jonathan
> ---
>  drivers/iio/adc/Kconfig   |  10 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/max961x.c | 648 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 659 insertions(+)
>  create mode 100644 drivers/iio/adc/max961x.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7a..f86026a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -354,6 +354,16 @@ config MAX1363
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1363.
>  
> +config	MAX961x
> +	tristate "Maxim max9611/9612 ADC driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Maxim max9611/9612 current sense
> +	  amplifiers with 12-bits ADC interface.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max961x.
> +
>  config MCP320X
>  	tristate "Microchip Technology MCP3x01/02/04/08"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d001262..ff19250 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> +obj-$(CONFIG_MAX961x) += max961x.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
>  obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> diff --git a/drivers/iio/adc/max961x.c b/drivers/iio/adc/max961x.c
> new file mode 100644
> index 0000000..a544e69
> --- /dev/null
> +++ b/drivers/iio/adc/max961x.c
> @@ -0,0 +1,648 @@
> +/*
> + * iio/adc/max961x.c
> + *
> + * Maxim max9611/9612 high side current sense amplifier with
> + * 12-bit ADC interface.
> + *
> + * Copyright (C) 2017 Jacopo Mondi
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * This driver supports input common-mode voltage, current-sense
> + * amplifier with programmable gains and die temperature reading from
> + * Maxim max9611/9612.
> + * Op-amp, analog comparator, and watchdog functionalities are not
> + * supported by this driver.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +
> +#define DRIVER_NAME "max961x"
> +
> +/* max961x register addresses */
Prefix these with MAX9611_ to avoid possible clashes in future.

> +#define REG_CSA_DATA			0x00
> +#define REG_RS_DATA			0x02
> +#define REG_TEMP_DATA			0x08
> +#define REG_CTRL1			0x0a
> +#define REG_CTRL2			0x0b
> +
> +/* max961x REG1 mux configuration options */
> +#define MUX_MASK			0x07
> +#define MUX_SENSE_1x			0x00
> +#define MUX_SENSE_4x			0x01
> +#define MUX_SENSE_8x			0x02
> +#define MUX_INPUT_VOLTAGE		0x03
> +#define MUX_TEMPERATURE			0x06
> +
> +/* max961x voltage (both sense and input) helper macros */
> +#define MAX961x_VOLTAGE_SHIFT		0x04
> +#define MAX961x_VOLTAGE_RAW(_r)		((_r) >> MAX961x_VOLTAGE_SHIFT)
> +
> +/*
> + * max961x current sense amplifier voltage output:
> + * LSB and offset values depends on selected gain (1x, 4x, 8x)
> + *
> + * GAIN		LSB (nV)	OFFSET (LSB steps)
> + * 1x		107500		1
> + * 4x		26880		1
> + * 8x		13440		3
> + *
> + * The complete formula to calculate current sense voltage is:
> + *     (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
> + */
> +#define CSA_VOLT_1x_LSB_nV		107500
> +#define CSA_VOLT_4x_LSB_nV		26880
> +#define CSA_VOLT_8x_LSB_nV		13440
> +#define CSA_VOLT_1x_LSB_DIV		9302
> +#define CSA_VOLT_4x_LSB_DIV		37202
> +#define CSA_VOLT_8x_LSB_DIV		74404
> +
> +#define CSA_VOLT_1x_OFFS_RAW		1
> +#define CSA_VOLT_4x_OFFS_RAW		1
> +#define CSA_VOLT_8x_OFFS_RAW		3
> +#define CSA_VOLT_1x_OFFS_uV		100
> +#define CSA_VOLT_4x_OFFS_uV		45
> +#define CSA_VOLT_8x_OFFS_uV		45
> +
> +#define CSA_VOLT_IIO_RAW(_r, _offs)	\
> +	((MAX961x_VOLTAGE_RAW(_r) - (_offs)) * 1000)
> +
> +/*
> + * max961x common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
> + *
> + * The complete formula to calculate input common voltage is:
> + *     (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
> + */
> +#define CIM_VOLTAGE_LSB_mV		14
> +#define CIM_VOLTAGE_OFFSET_mV		14
> +#define CIM_VOLTAGE_OFFSET_RAW		1
> +
> +/*
> + * max961x temperature reading: LSB is 0.48 degrees Celsius
> + *
> + * The complete formula to calculate temperature is:
> + *     ((adc_read >> 7) * 1000) / (1 / 0.48 * 1000)
> + */
> +#define TEMP_SHIFT			0x07
> +#define TEMP_MAX_RAW_POS		0x7f80
> +#define TEMP_MAX_RAW_NEG		0xff80
> +#define TEMP_MIN_RAW_NEG		0xd980
> +#define TEMP_MASK			((1 << TEMP_SHIFT) - 1)
> +#define TEMP_RAW(_r)			((_r) >> TEMP_SHIFT)
> +#define TEMP_LSB_mC			480
> +#define TEMP_SCALE_NUM			1000
> +#define TEMP_SCALE_DIV			2083
> +
> +struct max961x_dev {
> +	struct device *dev;
> +	struct i2c_client *i2c_client;
> +	unsigned int shunt_resistor_uOhm;
> +};
> +
> +enum max961x_conf_ids {
> +	CONF_SENSE_1x,
> +	CONF_SENSE_4x,
> +	CONF_SENSE_8x,
> +	CONF_IN_VOLT,
> +	CONF_TEMP,
> +};
> +
> +/**
> + * max961x_conf - associate ADC mux configuration with register address where
> + *		  data shall be read from
> + */
> +static unsigned int max961x_conf[][2] = {
> +	/* CONF_SENSE_1x */
> +	{ MUX_SENSE_1x, REG_CSA_DATA },
> +	/* CONF_SENSE_4x */
> +	{ MUX_SENSE_4x, REG_CSA_DATA },
> +	/* CONF_SENSE_8x */
> +	{ MUX_SENSE_8x, REG_CSA_DATA },
> +	/* CONF_IN_VOLT */
> +	{ MUX_INPUT_VOLTAGE, REG_RS_DATA },
> +	/* CONF_TEMP */
> +	{ MUX_TEMPERATURE, REG_TEMP_DATA },
> +};
> +
> +enum max961x_csa_gain {
> +	CSA_GAIN_1x,
> +	CSA_GAIN_4x,
> +	CSA_GAIN_8x,
> +};
> +
> +enum max961x_csa_gain_params {
> +	CSA_GAIN_LSB_nV,
> +	CSA_GAIN_LSB_DIV,
> +	CSA_GAIN_OFFS_nV,
> +	CSA_GAIN_OFFS_RAW,
> +};
> +
> +/**
> + * max961x_csa_gain_conf - associate gain multiplier with LSB and
> + *			   offset values.
> + *
> + * Group together parameters associated with configurable gain
> + * on current sense amplifier path to ADC interface.
> + * Current sense read routine adjusts gain until it gets a meaningful
> + * value; use this structure to retrieve the correct LSB and offset values.
> + */
> +static unsigned int max961x_gain_conf[][4] = {
> +	{ /* [0] CSA_GAIN_1x */
> +		CSA_VOLT_1x_LSB_nV,
> +		CSA_VOLT_1x_LSB_DIV,
> +		CSA_VOLT_1x_OFFS_uV,
> +		CSA_VOLT_1x_OFFS_RAW,
> +	},
> +	{ /* [1] CSA_GAIN_4x */
> +		CSA_VOLT_4x_LSB_nV,
> +		CSA_VOLT_4x_LSB_DIV,
> +		CSA_VOLT_4x_OFFS_uV,
> +		CSA_VOLT_4x_OFFS_RAW,
> +	},
> +	{ /* [2] CSA_GAIN_8x */
> +		CSA_VOLT_8x_LSB_nV,
> +		CSA_VOLT_8x_LSB_DIV,
> +		CSA_VOLT_8x_OFFS_uV,
> +		CSA_VOLT_8x_OFFS_RAW,
> +	},
> +};
> +
> +/**
> + * max961x_chan_ids - Identify IIO channels
> + */
> +enum max961x_chan_ids {
> +	MAX961x_CHAN_VOLTAGE_INPUT,
> +	MAX961x_CHAN_VOLTAGE_SENSE,
> +	MAX961x_CHAN_TEMPERATURE,
> +	MAX961x_CHAN_CURRENT_LOAD,
> +	MAX961x_CHAN_POWER_LOAD,
> +};
> +
> +static struct iio_chan_spec max961x_channels[] = {
> +	{ /* [0] die temperature */
> +	  .type			= IIO_TEMP,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE) |
The only cases we normally allow bother raw and processed output in
are ones where we are supporting some defunct (i.e. wrong usage) in the
userspace ABI.  So drop the processed path please.

> +				  BIT(IIO_CHAN_INFO_PROCESSED),
> +	  .address		= MAX961x_CHAN_TEMPERATURE,
> +	},
> +	{ /* [1] common voltage input */
> +	  .type			= IIO_VOLTAGE,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE) |
> +				  BIT(IIO_CHAN_INFO_OFFSET),
> +	  .address		= MAX961x_CHAN_VOLTAGE_INPUT,
> +	  .channel		= MAX961x_CHAN_VOLTAGE_INPUT,
For the channel it would be easier to read perhaps if you just put
the 0 in directly here.
> +	  .indexed		= 1,
> +	},
> +	{ /* [2] current sense amplifier voltage output */
> +	  .type			= IIO_VOLTAGE,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE) |
> +				  BIT(IIO_CHAN_INFO_OFFSET),
> +	  .address		= MAX961x_CHAN_VOLTAGE_SENSE,
> +	  .channel		= MAX961x_CHAN_VOLTAGE_SENSE,
> +	  .indexed		= 1,
> +	},

As you say, the next one is a computed channel.
> +	{ /* [3] load current measurement */
> +	  .type			= IIO_CURRENT,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE),
> +	  .address		= MAX961x_CHAN_CURRENT_LOAD,
> +	},
As is this one.
> +	{ /* [4] load current measurement */
> +	  .type			= IIO_POWER,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE),
> +	  .address		= MAX961x_CHAN_POWER_LOAD,
> +	},
> +};
> +
> +static int max961x_reg_read(struct max961x_dev *max961x, u8 reg, u16 *val)
> +{
I'm not convinced this particular wrapper adds anything.
I'd be tempted to just use the i2c read directly as needed rather than
bounce through this.

> +	int ret;
> +
> +	ret = i2c_smbus_read_word_swapped(max961x->i2c_client, reg);
> +	if (ret < 0) {
> +		dev_err(max961x->dev, "i2c read word from 0x%2x failed\n",
> +			reg);
> +		return ret;
> +	}
> +
> +	*val = ret;
> +
> +	return 0;
> +}
> +
> +static int max961x_reg_write(struct max961x_dev *max961x, u8 reg, u8 val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(max961x->i2c_client, reg, val);
> +	if (ret) {
> +		dev_err(max961x->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> +			reg, val);
> +		return ret;
> +	}
> +
> +	/* FIXME: need a delay here to make register configuration
> +	 *	  stabilize. 1 msec at least, from empirical testing.
Why fixme?  Sounds like it really needs to be the case to me!

Also, fix up your multiline comment syntax please.
> +	 */
> +	usleep_range(1000, 2000);
> +
> +	return 0;
> +}
> +
> +/**
> + * max961x_read_single() - read a single vale from ADC interface
> + *
> + * Data registers are 16 bit long, spread between two 8 bit registers
> + * with consecutive addresses.
> + * Configure ADC mux first, then read register at address "reg_addr".
> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
> + *
> + * @max961x: max961x device
> + * @selector: index for mux and register configuration
> + * @raw_val: the value returned from ADC
> + */
> +static int max961x_read_single(struct max961x_dev *max961x,
> +			       enum max961x_conf_ids selector,
> +			       u16 *raw_val)
> +{
> +	int ret;
> +
> +	u8 mux_conf = max961x_conf[selector][0] & MUX_MASK;
> +	u8 reg_addr = max961x_conf[selector][1];
> +
You have no lockign around these various sequences.
Nothing prevents multiple sysfs reads at the same time so you definitely
want to prevent this stuff from running concurrently.

A mutex should do the job, either at this low level or up in the calling
functions.
> +	ret = max961x_reg_write(max961x, REG_CTRL1, mux_conf);
> +	if (ret)
> +		return ret;
> +
> +	ret = max961x_reg_read(max961x, reg_addr, raw_val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/**
> + * max961x_read_csa_voltage() - read current sense amplifier output voltage
> + *
> + * Current sense amplifier output voltage is read through a configurable
> + * 1x, 4x or 8x gain.
> + * Start with plain 1x gain, and adjust gain control properly until a
> + * meaningful value is read from ADC output.
> + *
> + * @max961x: max961x device
> + * @adc_raw: raw value read from ADC output
> + * @csa_gain: gain configuration option selector
> + */
> +static int max961x_read_csa_voltage(struct max961x_dev *max961x,
> +				    u16 *adc_raw,
> +				    enum max961x_csa_gain *csa_gain)
> +{
> +	int ret;
> +	unsigned int i;
> +	enum max961x_conf_ids gain_selectors[] = {
> +		CONF_SENSE_1x,
> +		CONF_SENSE_4x,
> +		CONF_SENSE_8x
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
> +		ret = max961x_read_single(max961x, gain_selectors[i], adc_raw);
> +		if (ret)
> +			return ret;
> +
> +		if (*adc_raw > 0) {
> +			*csa_gain = gain_selectors[i];
> +			return 0;
> +		}
> +	}
> +
> +	return -EIO;
> +}
> +
> +static int max961x_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	int ret;
> +	u16 adc_data;
> +	enum max961x_csa_gain gain_selector;
> +	unsigned int *csa_gain;
> +	struct max961x_dev *dev = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->address) {
> +		case MAX961x_CHAN_TEMPERATURE:
> +			ret = max961x_read_single(dev, CONF_TEMP,
> +						  &adc_data);
> +			if (ret)
> +				return ret;
> +
> +			*val = TEMP_RAW(adc_data);
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_VOLTAGE_INPUT:
> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
> +						  &adc_data);
> +			if (ret)
> +				return ret;
> +
> +			*val = MAX961x_VOLTAGE_RAW(adc_data);
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_VOLTAGE_SENSE:
> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return ret;
> +
> +			*val = MAX961x_VOLTAGE_RAW(adc_data);
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_CURRENT_LOAD:
> +			/* raw (nV): raw * LSB (nV) */
> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return ret;
> +
> +			csa_gain = max961x_gain_conf[gain_selector];
> +
> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
> +			       csa_gain[CSA_GAIN_LSB_nV];
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_POWER_LOAD:
> +			/* raw (mV): raw * LSB (mV) */
> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
> +						  &adc_data);
> +
> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
> +			       CIM_VOLTAGE_LSB_mV;
> +
> +			return IIO_VAL_INT;
> +		}
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->address) {
> +
> +		case MAX961x_CHAN_TEMPERATURE:
> +			/* processed (C): raw * scale (C) */
> +			*val = TEMP_SCALE_NUM;
> +			*val2 = TEMP_SCALE_DIV;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX961x_CHAN_VOLTAGE_INPUT:
> +			/* processed (mV): raw * scale_mV */
Err, I'm confused.  We read adc_data then do nothing with it?
> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
> +						  &adc_data);
> +			if (ret)
> +				return ret;
> +
> +			*val = CIM_VOLTAGE_LSB_mV;
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_VOLTAGE_SENSE:
> +			/* processed (mV): raw (mV) * (scale (nV) / 10^6) */
> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return ret;
As with the offset below, this is a non starter to my mind as we have
no way of knowing it was the 'same' gain value.  That has to be the case
or we'll get all sorts of horrible instabilities in apparent output as
we pass from one gain setting to another.
> +
> +			csa_gain = max961x_gain_conf[gain_selector];
> +
> +			*val = csa_gain[CSA_GAIN_LSB_nV];
> +			*val2 = 1000000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX961x_CHAN_CURRENT_LOAD:
> +			/* processed (mA): raw (nV)  / scale (uOhm)  */
> +			*val = 1;
> +			*val2 = dev->shunt_resistor_uOhm;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX961x_CHAN_POWER_LOAD:
> +			/* processed (mW): raw (mV) * (scale (mA) / 1000) */
> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
Again, nothing says the gain_selector value is the one used when we
took the measurement (or will be if we take it in future) so we can't do
it this way.  Either we need to remove the computed value and provide
enough info to userspace to allow it to do the job, or we need to ensure
that the kernel data manipulation doesn't allow for any instabilities around
gain transitions.
> +			if (ret)
> +				return ret;
> +
> +			csa_gain = max961x_gain_conf[gain_selector];
> +
> +			/* val = (nV / uOhm) = mA */
> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
> +			       csa_gain[CSA_GAIN_LSB_nV];
> +			*val /= dev->shunt_resistor_uOhm;
> +
> +			*val2 = 1000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		switch (chan->address) {
> +		case MAX961x_CHAN_VOLTAGE_INPUT:
> +			*val = CIM_VOLTAGE_OFFSET_RAW;
> +
> +			return IIO_VAL_INT;
> +
> +		case MAX961x_CHAN_VOLTAGE_SENSE:
Interesting. Nothing here ensures the reported scale is correct for
the reading we are looking it up for which is an issue.

For a compound value - i.e. one where the userspace interpretation is
dependent on several different readings (the classic being light sensors
where illuminance is often computed from two different light sensing
elementents) we tend to hide the complexity by doing it in kernel.

So my gut feeling is this should be hidden away.  That should ensure
the value used is the correct one as well.

> +			ret = max961x_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return ret;
> +
> +			*val = max961x_gain_conf[gain_selector]
> +						[CSA_GAIN_OFFS_RAW];
> +			*val2 = 1000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +

As stated above, please drop this as userspace can do this trivially from
the raw and scale provided.
> +	case IIO_CHAN_INFO_PROCESSED:
> +		/* processed (mC): raw * LSB (mC) */
> +		if (chan->address != MAX961x_CHAN_TEMPERATURE)
> +			return -EINVAL;
> +
> +		ret = max961x_read_single(dev, CONF_TEMP,
> +					  &adc_data);
> +		if (ret)
> +			return ret;
> +
> +		*val = TEMP_RAW(adc_data) * TEMP_LSB_mC;
> +
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t max961x_shunt_resistor_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct max961x_dev *max961x = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", max961x->shunt_resistor_uOhm);
> +}
> +
> +static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO,
> +		       max961x_shunt_resistor_show, NULL, 0);
> +
> +static struct attribute *max961x_attributes[] = {
> +	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group max961x_attribute_group = {
> +	.attrs = max961x_attributes,
> +};
> +
> +static const struct iio_info indio_info = {
> +	.driver_module	= THIS_MODULE,
> +	.read_raw	= max961x_read_raw,
> +	.attrs		= &max961x_attribute_group,
> +};
> +
> +static int max961x_init(struct max961x_dev *max961x)
> +{
> +	int ret;
> +	u16 regval;
> +	struct i2c_client *client = max961x->i2c_client;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WRITE_BYTE	|
> +				     I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> +		dev_err(max961x->dev,
> +			"I2c adapter does not support smbus write_byte and "\
Whilst it'll break the 80 char limit by quite a way, it's preferable to do that
than to break the ability of someone to grep for an error message.  So keep
this on one line please.
> +			"read_word_data functionalities. Aborting probe.\n");
> +		return -EINVAL;
> +	}
> +
> +	max961x_reg_write(max961x, REG_CTRL1, MUX_TEMPERATURE);
> +	max961x_reg_write(max961x, REG_CTRL2, 0);
> +
> +	/* Make sure die temperature is in range to test communications. */
> +	regval = 0;
> +	ret = max961x_reg_read(max961x, REG_TEMP_DATA, &regval);
> +	if (ret)
> +		return ret;
> +
> +	regval = regval & ~TEMP_MASK;
> +	if ((regval > TEMP_MAX_RAW_POS &&
> +	     regval < TEMP_MIN_RAW_NEG) ||
> +	     regval > TEMP_MAX_RAW_NEG) {
> +		dev_err(max961x->dev,
> +			"In-valid value received from ADC 0x%4x: aborting\n",
> +			regval);
> +		return -EIO;
> +	}
> +
> +	/* Mux shall be zeroed back before applying other configurations */
> +	max961x_reg_write(max961x, REG_CTRL1, 0);
> +
> +	return 0;
> +}
> +
> +static int max961x_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct max961x_dev *max961x;
> +	struct iio_dev *indio_dev;
> +	struct device_node *of_node = client->dev.of_node;
> +	unsigned int of_shunt;
> +	const char * const shunt_res_prop = "shunt-resistor";
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max961x));
> +	if (IS_ERR(indio_dev))
> +		return PTR_ERR(indio_dev);
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	max961x		= iio_priv(indio_dev);
> +	max961x->dev	= &client->dev;
> +	max961x->i2c_client = client;
> +
> +	ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Missing %s property for %s node\n",
> +			shunt_res_prop, of_node->full_name);
> +		return ret;
> +	}
> +	max961x->shunt_resistor_uOhm = of_shunt;
> +
> +	ret = max961x_init(max961x);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->dev.parent	= &client->dev;
> +	indio_dev->dev.of_node	= client->dev.of_node;
> +	indio_dev->name		= DRIVER_NAME;
> +	indio_dev->modes	= INDIO_DIRECT_MODE;
> +	indio_dev->info		= &indio_info;
> +	indio_dev->channels	= max961x_channels;
> +	indio_dev->num_channels	= ARRAY_SIZE(max961x_channels);
> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(&client->dev, "%s: probed\n", DRIVER_NAME);
This one is always hotly debated.  To my mind, this provides
no information, but if you are particularly attached to it I don't
really mind.  If not, then return devm_iio_* directly.
> +
> +	return 0;
> +}
> +
> +static int max961x_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
If all you have in a remove function is a call to iio_device_unregister
my immediate thought is could you use devm_iio_device_register.

Ah, you are.  Double unregister!  If you are using the devm form of register
it will be unregistered automatically as part of the removal of the
underlying struct device.  Thus you don't need to do it explicitly.

Thus you can drop this remove function completely as it should be
empty.
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id max961x_of_table[] = {
> +	{.compatible = "maxim,max961x"},
Already been raised in the bindings reviews, but you shouldn't
use wild cards in compatible strings.

Actually as it almost always goes wrong, you shouldn't used them
for naming of anything.  Pick a part and use that were you
currently have max961x throughout the driver.

i.e. max9611_of_table etc.

Avoids possible issues down the line when maxim decide to
release something incompatible and call it a max9619..
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, max961x_of_table);
> +
> +static struct i2c_driver max961x_driver = {
> +	.driver = {
> +		   .name = DRIVER_NAME,
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = max961x_of_table,
> +	},
> +	.probe = max961x_probe,
> +	.remove = max961x_remove,
> +};
> +module_i2c_driver(max961x_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH 2/4] iio: Documentation: Add max961x sysfs documentation

[Jonathan Cameron]

On 24/02/17 15:05, Jacopo Mondi wrote:
> Add documentation for max961x driver.
> The only attribute to document is the current sense shunt resistor
> value.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
Interestingly generic as it only applies to some of the channels.
We might need to have the channel where it is effecting the output specified in the
name, even if we end up with several repeats of the same thing for different
computed channels.

Jonathan
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-adc-max961x | 5 +++++
>  1 file changed, 5 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> new file mode 100644
> index 0000000..dbd5e75
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> @@ -0,0 +1,5 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
> +Date:		February 2017
> +KernelVersion:	4.10
> +Contact:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +Description: 	The value of the shunt resistor in micro Ohms.
> 

Re: [PATCH 2/4] iio: Documentation: Add max961x sysfs documentation

[Jonathan Cameron]

On 24/02/17 15:05, Jacopo Mondi wrote:
> Add documentation for max961x driver.
> The only attribute to document is the current sense shunt resistor
> value.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Interestingly generic as it only applies to some of the channels.
We might need to have the channel where it is effecting the output specified in the
name, even if we end up with several repeats of the same thing for different
computed channels.

Jonathan
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-adc-max961x | 5 +++++
>  1 file changed, 5 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> new file mode 100644
> index 0000000..dbd5e75
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> @@ -0,0 +1,5 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
> +Date:		February 2017
> +KernelVersion:	4.10
> +Contact:	linux-iio@vger.kernel.org
> +Description: 	The value of the shunt resistor in micro Ohms.
> 

Re: [PATCH 1/4] Documentation: dt-bindings: iio: Add max961x

[Jonathan Cameron]

On 25/02/17 15:34, Geert Uytterhoeven wrote:
> Hi Jonathan,
> 
> On Sat, Feb 25, 2017 at 4:19 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 24/02/17 15:48, jacopo mondi wrote:
>>> On 24/02/2017 16:33, Lars-Peter Clausen wrote:
>>>> On 02/24/2017 04:22 PM, Geert Uytterhoeven wrote:
>>>>> On Fri, Feb 24, 2017 at 4:05 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>>>> @@ -0,0 +1,27 @@
>>>>>> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
>>>>>> +
>>>>>> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
>>>>>> +12-bits ADC communicating over I2c bus.
>>>>>> +The device node for this driver shall be a child of a I2c controller.
>>>>>> +
>>>>>> +Required properties
>>>>>> +  - compatible: Should be "maxim,max961x"
>>>>>> +  - reg: The 7-bits long I2c address of the device
>>>>>> +  - shunt-resistor: Resistor value, in uOhm, of the current sense shunt
>>>>>> +                   resistor.
>>>>>
>>>>> shunt-resistor-micro-ohms?
>>>>
>>>> I'll take this one further:
>>>>
>>>> maxim,shunt-resistor-micro-ohms?
>>>>
>>>> Although there is precedence for just 'shunt-resistor' in the ina2xx bindings.
>>>>
>>>
>>> And that's where I took "inspiration" from :)
>> Which raises another open question.  Why IIO rather than hwmon like the ina2xx?
> 
> Actually we have both drivers/hwmon/ina2xx.c and drivers/iio/adc/ina2xx-adc.c.
Oops,. I'd forgotten about that.

Need that discussion to happen though before I take it into IIO though.
> 
>> Whether we are better going for the existing binding without units, or
>> 'fixing' that is a question for the device tree maintainers.  I guess that
>> one snuck through.
> 
> ltc4151 uses shunt-resistor-micro-ohms.
> sgtl5000 uses micbias-resistor-k-ohms.
> 
> So there's precedence for everything ;-)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[Jonathan Cameron]

On 24/02/17 15:05, Jacopo Mondi wrote:
> Hello!
> 
> This series adds driver and documentation for Maxim max9611/max9612
> high-side current sense amplifier with 12-bit ADC and I2c interface.
> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
> in Renesas r87796 Salvator-X board.
> 
> The device provides several functionalities, and only some of them are
> currently supported by this driver.
> Particularly, the on-board op-amp and analog comparator are not currently
> supported.
More fun to come ;)
> 
> A simplified integration schematic drawing is here reported:
> 
>  ----o----/\/\/-----o-------|LOAD|---
>      |    shunt     |
>  ____|______________|___
>  |  RS+            RS-  |
>  |   |-----gain-----|   |
>  |          |           |
>  |          |           |
>  |max961x   |->| ADC |===== I2c
>  |______________________|
> 
> 
> The device provides though its 12-bits ADC the following informations:
information is it's own plural (silly English quirk of the day!)

> - Common input voltage on RS+
> - Voltage drop between RS+ and RS- ends
> - Die temperature
> 
> All of the above ones are exposed though IIO with _raw and _scale values
> (plus _input for milli degree Celsius die temperature).
> 
> From the above values the driver calculates the current flowing between
> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
> the power load. Again this values are exposed through _raw and _scale
> attributes, which I'm not completely sure it's acceptables as they are
> calculated values and not natively provided by the current sense amplifier.
> I would like to hear IIO people opinions on this, if they should be better
> exposed though some other attributes which are not _raw and _scale, or if
> their calculation should be completely left to userspace tools.
So one element of the implementation is a problem.
Because of the dynamic gain adjustment you are doing in the driver (which I
rather like BTW) there will be instabilities in the computed values that
userspace comes up with when we are near a transition for in the current
sense amplifier gain.  We can't do that as crazy outputs will result
(read offset and scale for possible different values of that gain then
read the actual ADC value for a possible 3rd choice resulting in complete
garbage).

So there are two ways to avoid this:
1. Move all that magic into the original reads and apply the gain and offset
before they get to userspace.
2. Provide enough information for userspace to be able to compute everything
iff it is using buffered mode with a 'scan' covering all the channels in one
go.  Even then we'd have to explicitly allow reading of the PGA gain as a
channel, or make userspace responsible for doing your autogain stuff.

1 is probably easier but will make implementing 2 as a follow up will be tricky
and would be needed if you want to read this stuff faster than sysfs will
allow. 

It's a shame, but my gut feeling is you want to drop the autogain stuff
as then it all becomes straight forward.

What do others think?

Jonathan
> 
> Thanks
>    j
> 
> Jacopo Mondi (4):
>   Documentation: dt-bindings: iio: Add max961x
>   iio: Documentation: Add max961x sysfs documentation
>   iio: adc: Add max9611/9612 ADC driver
>   arm64: dts: salvator-x: Add current sense amplifiers
> 
>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>  drivers/iio/adc/Kconfig                            |  10 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>  6 files changed, 708 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>  create mode 100644 drivers/iio/adc/max961x.c
> 

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[Jonathan Cameron]

On 24/02/17 15:05, Jacopo Mondi wrote:
> Hello!
> 
> This series adds driver and documentation for Maxim max9611/max9612
> high-side current sense amplifier with 12-bit ADC and I2c interface.
> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
> in Renesas r87796 Salvator-X board.
> 
> The device provides several functionalities, and only some of them are
> currently supported by this driver.
> Particularly, the on-board op-amp and analog comparator are not currently
> supported.
More fun to come ;)
> 
> A simplified integration schematic drawing is here reported:
> 
>  ----o----/\/\/-----o-------|LOAD|---
>      |    shunt     |
>  ____|______________|___
>  |  RS+            RS-  |
>  |   |-----gain-----|   |
>  |          |           |
>  |          |           |
>  |max961x   |->| ADC |===== I2c
>  |______________________|
> 
> 
> The device provides though its 12-bits ADC the following informations:
information is it's own plural (silly English quirk of the day!)

> - Common input voltage on RS+
> - Voltage drop between RS+ and RS- ends
> - Die temperature
> 
> All of the above ones are exposed though IIO with _raw and _scale values
> (plus _input for milli degree Celsius die temperature).
> 
> From the above values the driver calculates the current flowing between
> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
> the power load. Again this values are exposed through _raw and _scale
> attributes, which I'm not completely sure it's acceptables as they are
> calculated values and not natively provided by the current sense amplifier.
> I would like to hear IIO people opinions on this, if they should be better
> exposed though some other attributes which are not _raw and _scale, or if
> their calculation should be completely left to userspace tools.
So one element of the implementation is a problem.
Because of the dynamic gain adjustment you are doing in the driver (which I
rather like BTW) there will be instabilities in the computed values that
userspace comes up with when we are near a transition for in the current
sense amplifier gain.  We can't do that as crazy outputs will result
(read offset and scale for possible different values of that gain then
read the actual ADC value for a possible 3rd choice resulting in complete
garbage).

So there are two ways to avoid this:
1. Move all that magic into the original reads and apply the gain and offset
before they get to userspace.
2. Provide enough information for userspace to be able to compute everything
iff it is using buffered mode with a 'scan' covering all the channels in one
go.  Even then we'd have to explicitly allow reading of the PGA gain as a
channel, or make userspace responsible for doing your autogain stuff.

1 is probably easier but will make implementing 2 as a follow up will be tricky
and would be needed if you want to read this stuff faster than sysfs will
allow. 

It's a shame, but my gut feeling is you want to drop the autogain stuff
as then it all becomes straight forward.

What do others think?

Jonathan
> 
> Thanks
>    j
> 
> Jacopo Mondi (4):
>   Documentation: dt-bindings: iio: Add max961x
>   iio: Documentation: Add max961x sysfs documentation
>   iio: adc: Add max9611/9612 ADC driver
>   arm64: dts: salvator-x: Add current sense amplifiers
> 
>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>  drivers/iio/adc/Kconfig                            |  10 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>  6 files changed, 708 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>  create mode 100644 drivers/iio/adc/max961x.c
> 

Re: [PATCH 3/4] iio: adc: Add max9611/9612 ADC driver

[jacopo mondi]

Hi Jonathan,
    thanks for review

On 25/02/2017 16:53, Jonathan Cameron wrote:
> On 24/02/17 15:05, Jacopo Mondi wrote:
>> Add iio driver for Maxim max9611/9612 current-sense amplifiers with
>> 12-bits ADC.
> Data sheet link always good
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
> I'm going to respond to the cover letter wrt to providing dynamic scales
> and offsets in a moment as you raised the question there.
>
> Otherwise, various bits and bobs inline.
>
> Interesting part to handle as it can read a lot of values that are useful only
> after some magic calculations are done.
>
> Jonathan
>> ---
>>  drivers/iio/adc/Kconfig   |  10 +
>>  drivers/iio/adc/Makefile  |   1 +
>>  drivers/iio/adc/max961x.c | 648 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 659 insertions(+)
>>  create mode 100644 drivers/iio/adc/max961x.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index dedae7a..f86026a 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -354,6 +354,16 @@ config MAX1363
>>  	  To compile this driver as a module, choose M here: the module will be
>>  	  called max1363.
>>
>> +config	MAX961x
>> +	tristate "Maxim max9611/9612 ADC driver"
>> +	depends on I2C
>> +	help
>> +	  Say yes here to build support for Maxim max9611/9612 current sense
>> +	  amplifiers with 12-bits ADC interface.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called max961x.
>> +
>>  config MCP320X
>>  	tristate "Microchip Technology MCP3x01/02/04/08"
>>  	depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index d001262..ff19250 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>>  obj-$(CONFIG_MAX1027) += max1027.o
>>  obj-$(CONFIG_MAX11100) += max11100.o
>>  obj-$(CONFIG_MAX1363) += max1363.o
>> +obj-$(CONFIG_MAX961x) += max961x.o
>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>>  obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>> diff --git a/drivers/iio/adc/max961x.c b/drivers/iio/adc/max961x.c
>> new file mode 100644
>> index 0000000..a544e69
>> --- /dev/null
>> +++ b/drivers/iio/adc/max961x.c
>> @@ -0,0 +1,648 @@
>> +/*
>> + * iio/adc/max961x.c
>> + *
>> + * Maxim max9611/9612 high side current sense amplifier with
>> + * 12-bit ADC interface.
>> + *
>> + * Copyright (C) 2017 Jacopo Mondi
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +/*
>> + * This driver supports input common-mode voltage, current-sense
>> + * amplifier with programmable gains and die temperature reading from
>> + * Maxim max9611/9612.
>> + * Op-amp, analog comparator, and watchdog functionalities are not
>> + * supported by this driver.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/module.h>
>> +
>> +#define DRIVER_NAME "max961x"
>> +
>> +/* max961x register addresses */
> Prefix these with MAX9611_ to avoid possible clashes in future.
>

I initially did, then removed that to counterbalance my tendency to be a 
bit verbose in naming stuff.

I'll use the MAX9611_ prefix where appropriate

>> +#define REG_CSA_DATA			0x00
[snip
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE) |
> The only cases we normally allow bother raw and processed output in
> are ones where we are supporting some defunct (i.e. wrong usage) in the
> userspace ABI.  So drop the processed path please.

Will do

>
>> +				  BIT(IIO_CHAN_INFO_PROCESSED),
>> +	  .address		= MAX961x_CHAN_TEMPERATURE,
>> +	},
>> +	{ /* [1] common voltage input */
>> +	  .type			= IIO_VOLTAGE,
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE) |
>> +				  BIT(IIO_CHAN_INFO_OFFSET),
>> +	  .address		= MAX961x_CHAN_VOLTAGE_INPUT,
>> +	  .channel		= MAX961x_CHAN_VOLTAGE_INPUT,
> For the channel it would be easier to read perhaps if you just put
> the 0 in directly here.
>> +	  .indexed		= 1,
>> +	},
>> +	{ /* [2] current sense amplifier voltage output */
>> +	  .type			= IIO_VOLTAGE,
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE) |
>> +				  BIT(IIO_CHAN_INFO_OFFSET),
>> +	  .address		= MAX961x_CHAN_VOLTAGE_SENSE,
>> +	  .channel		= MAX961x_CHAN_VOLTAGE_SENSE,
>> +	  .indexed		= 1,
>> +	},
>
> As you say, the next one is a computed channel.
>> +	{ /* [3] load current measurement */
>> +	  .type			= IIO_CURRENT,
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE),
>> +	  .address		= MAX961x_CHAN_CURRENT_LOAD,
>> +	},
> As is this one.

It's not clear to me if the fact of being a "computed channel" implies I 
have to use a different channel type to expose it (and, what channel 
type should I use in place of _RAW and _SCALE).
Please not my question is independent on the auto-gain discussion 
started in response the patch series cover letter.

>> +	{ /* [4] load current measurement */
>> +	  .type			= IIO_POWER,
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE),
>> +	  .address		= MAX961x_CHAN_POWER_LOAD,
>> +	},
>> +};
>> +
>> +static int max961x_reg_read(struct max961x_dev *max961x, u8 reg, u16 *val)
>> +{
> I'm not convinced this particular wrapper adds anything.
> I'd be tempted to just use the i2c read directly as needed rather than
> bounce through this.
>

I would say "inline it then", but I'm not sure trying to outsmart 
complier is desirable.

>> +	int ret;
>> +
>> +	ret = i2c_smbus_read_word_swapped(max961x->i2c_client, reg);
>> +	if (ret < 0) {
>> +		dev_err(max961x->dev, "i2c read word from 0x%2x failed\n",
>> +			reg);
>> +		return ret;
>> +	}
>> +
>> +	*val = ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int max961x_reg_write(struct max961x_dev *max961x, u8 reg, u8 val)
>> +{
>> +	int ret;
>> +
>> +	ret = i2c_smbus_write_byte_data(max961x->i2c_client, reg, val);
>> +	if (ret) {
>> +		dev_err(max961x->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>> +			reg, val);
>> +		return ret;
>> +	}
>> +
>> +	/* FIXME: need a delay here to make register configuration
>> +	 *	  stabilize. 1 msec at least, from empirical testing.
> Why fixme?  Sounds like it really needs to be the case to me!
>
> Also, fix up your multiline comment syntax please.

It's a FIXME as I found that out after some testing, I'm not sure about 
the root cause yet.
But yes, it's required to have driver work properly, so I'll drop this

>> +	 */
>> +	usleep_range(1000, 2000);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * max961x_read_single() - read a single vale from ADC interface
>> + *
>> + * Data registers are 16 bit long, spread between two 8 bit registers
>> + * with consecutive addresses.
>> + * Configure ADC mux first, then read register at address "reg_addr".
>> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
>> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
>> + *
>> + * @max961x: max961x device
>> + * @selector: index for mux and register configuration
>> + * @raw_val: the value returned from ADC
>> + */
>> +static int max961x_read_single(struct max961x_dev *max961x,
>> +			       enum max961x_conf_ids selector,
>> +			       u16 *raw_val)
>> +{
>> +	int ret;
>> +
>> +	u8 mux_conf = max961x_conf[selector][0] & MUX_MASK;
>> +	u8 reg_addr = max961x_conf[selector][1];
>> +
> You have no lockign around these various sequences.
> Nothing prevents multiple sysfs reads at the same time so you definitely
> want to prevent this stuff from running concurrently.
>
> A mutex should do the job, either at this low level or up in the calling
> functions.

I had verified the i2c_smbus_  functions where protected by their own 
mutexes and thought that was enough.
But yes, read and writes here shall be protected as well. Will re-add 
mutex back then


>> +	ret = max961x_reg_write(max961x, REG_CTRL1, mux_conf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = max961x_reg_read(max961x, reg_addr, raw_val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * max961x_read_csa_voltage() - read current sense amplifier output voltage
>> + *
>> + * Current sense amplifier output voltage is read through a configurable
>> + * 1x, 4x or 8x gain.
>> + * Start with plain 1x gain, and adjust gain control properly until a
>> + * meaningful value is read from ADC output.
>> + *
>> + * @max961x: max961x device
>> + * @adc_raw: raw value read from ADC output
>> + * @csa_gain: gain configuration option selector
>> + */
>> +static int max961x_read_csa_voltage(struct max961x_dev *max961x,
>> +				    u16 *adc_raw,
>> +				    enum max961x_csa_gain *csa_gain)
>> +{
>> +	int ret;
>> +	unsigned int i;
>> +	enum max961x_conf_ids gain_selectors[] = {
>> +		CONF_SENSE_1x,
>> +		CONF_SENSE_4x,
>> +		CONF_SENSE_8x
>> +	};
>> +
>> +	for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
>> +		ret = max961x_read_single(max961x, gain_selectors[i], adc_raw);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (*adc_raw > 0) {
>> +			*csa_gain = gain_selectors[i];
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -EIO;
>> +}
>> +
>> +static int max961x_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	int ret;
>> +	u16 adc_data;
>> +	enum max961x_csa_gain gain_selector;
>> +	unsigned int *csa_gain;
>> +	struct max961x_dev *dev = iio_priv(indio_dev);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		switch (chan->address) {
>> +		case MAX961x_CHAN_TEMPERATURE:
>> +			ret = max961x_read_single(dev, CONF_TEMP,
>> +						  &adc_data);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = TEMP_RAW(adc_data);
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_INPUT:
>> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
>> +						  &adc_data);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data);
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_SENSE:
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data);
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_CURRENT_LOAD:
>> +			/* raw (nV): raw * LSB (nV) */
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
>> +			if (ret)
>> +				return ret;
>> +
>> +			csa_gain = max961x_gain_conf[gain_selector];
>> +
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
>> +			       csa_gain[CSA_GAIN_LSB_nV];
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_POWER_LOAD:
>> +			/* raw (mV): raw * LSB (mV) */
>> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
>> +						  &adc_data);
>> +
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
>> +			       CIM_VOLTAGE_LSB_mV;
>> +
>> +			return IIO_VAL_INT;
>> +		}
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->address) {
>> +
>> +		case MAX961x_CHAN_TEMPERATURE:
>> +			/* processed (C): raw * scale (C) */
>> +			*val = TEMP_SCALE_NUM;
>> +			*val2 = TEMP_SCALE_DIV;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_INPUT:
>> +			/* processed (mV): raw * scale_mV */
> Err, I'm confused.  We read adc_data then do nothing with it?

leftover, sorry about this...

>> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
>> +						  &adc_data);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = CIM_VOLTAGE_LSB_mV;
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_SENSE:
>> +			/* processed (mV): raw (mV) * (scale (nV) / 10^6) */
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
>> +			if (ret)
>> +				return ret;
> As with the offset below, this is a non starter to my mind as we have
> no way of knowing it was the 'same' gain value.  That has to be the case
> or we'll get all sorts of horrible instabilities in apparent output as
> we pass from one gain setting to another.

Let's keep discussing this in response to the cover letter

>> +
>> +			csa_gain = max961x_gain_conf[gain_selector];
>> +
>> +			*val = csa_gain[CSA_GAIN_LSB_nV];
>> +			*val2 = 1000000;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case MAX961x_CHAN_CURRENT_LOAD:
>> +			/* processed (mA): raw (nV)  / scale (uOhm)  */
>> +			*val = 1;
>> +			*val2 = dev->shunt_resistor_uOhm;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case MAX961x_CHAN_POWER_LOAD:
>> +			/* processed (mW): raw (mV) * (scale (mA) / 1000) */
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
> Again, nothing says the gain_selector value is the one used when we
> took the measurement (or will be if we take it in future) so we can't do
> it this way.  Either we need to remove the computed value and provide
> enough info to userspace to allow it to do the job, or we need to ensure
> that the kernel data manipulation doesn't allow for any instabilities around
> gain transitions.
>> +			if (ret)
>> +				return ret;
>> +
>> +			csa_gain = max961x_gain_conf[gain_selector];
>> +
>> +			/* val = (nV / uOhm) = mA */
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
>> +			       csa_gain[CSA_GAIN_LSB_nV];
>> +			*val /= dev->shunt_resistor_uOhm;
>> +
>> +			*val2 = 1000;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +		}
>> +
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		switch (chan->address) {
>> +		case MAX961x_CHAN_VOLTAGE_INPUT:
>> +			*val = CIM_VOLTAGE_OFFSET_RAW;
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_SENSE:
> Interesting. Nothing here ensures the reported scale is correct for
> the reading we are looking it up for which is an issue.
>
> For a compound value - i.e. one where the userspace interpretation is
> dependent on several different readings (the classic being light sensors
> where illuminance is often computed from two different light sensing
> elementents) we tend to hide the complexity by doing it in kernel.
>
> So my gut feeling is this should be hidden away.  That should ensure
> the value used is the correct one as well.
>
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = max961x_gain_conf[gain_selector]
>> +						[CSA_GAIN_OFFS_RAW];
>> +			*val2 = 1000;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +		}
>> +
>
> As stated above, please drop this as userspace can do this trivially from
> the raw and scale provided.

I agree that the offset may not match values returned from _raw and 
_scale (because of the auto-gain selection), but how can userspace 
retrieve offset from _raw and _scale if not opening the device datasheet?

>> +	case IIO_CHAN_INFO_PROCESSED:
>> +		/* processed (mC): raw * LSB (mC) */
>> +		if (chan->address != MAX961x_CHAN_TEMPERATURE)
>> +			return -EINVAL;
>> +
>> +		ret = max961x_read_single(dev, CONF_TEMP,
>> +					  &adc_data);
>> +		if (ret)
>> +			return ret;
>> +
>> +		*val = TEMP_RAW(adc_data) * TEMP_LSB_mC;
>> +
>> +		return IIO_VAL_INT;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static ssize_t max961x_shunt_resistor_show(struct device *dev,
>> +					   struct device_attribute *attr,
>> +					   char *buf)
>> +{
>> +	struct max961x_dev *max961x = iio_priv(dev_to_iio_dev(dev));
>> +
>> +	return sprintf(buf, "%d\n", max961x->shunt_resistor_uOhm);
>> +}
>> +
>> +static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO,
>> +		       max961x_shunt_resistor_show, NULL, 0);
>> +
>> +static struct attribute *max961x_attributes[] = {
>> +	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group max961x_attribute_group = {
>> +	.attrs = max961x_attributes,
>> +};
>> +
>> +static const struct iio_info indio_info = {
>> +	.driver_module	= THIS_MODULE,
>> +	.read_raw	= max961x_read_raw,
>> +	.attrs		= &max961x_attribute_group,
>> +};
>> +
>> +static int max961x_init(struct max961x_dev *max961x)
>> +{
>> +	int ret;
>> +	u16 regval;
>> +	struct i2c_client *client = max961x->i2c_client;
>> +
>> +	if (!i2c_check_functionality(client->adapter,
>> +				     I2C_FUNC_SMBUS_WRITE_BYTE	|
>> +				     I2C_FUNC_SMBUS_READ_WORD_DATA)) {
>> +		dev_err(max961x->dev,
>> +			"I2c adapter does not support smbus write_byte and "\
> Whilst it'll break the 80 char limit by quite a way, it's preferable to do that
> than to break the ability of someone to grep for an error message.  So keep
> this on one line please.

I'll make this less verbose.

>> +			"read_word_data functionalities. Aborting probe.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	max961x_reg_write(max961x, REG_CTRL1, MUX_TEMPERATURE);
>> +	max961x_reg_write(max961x, REG_CTRL2, 0);
>> +
>> +	/* Make sure die temperature is in range to test communications. */
>> +	regval = 0;
>> +	ret = max961x_reg_read(max961x, REG_TEMP_DATA, &regval);
>> +	if (ret)
>> +		return ret;
>> +
>> +	regval = regval & ~TEMP_MASK;
>> +	if ((regval > TEMP_MAX_RAW_POS &&
>> +	     regval < TEMP_MIN_RAW_NEG) ||
>> +	     regval > TEMP_MAX_RAW_NEG) {
>> +		dev_err(max961x->dev,
>> +			"In-valid value received from ADC 0x%4x: aborting\n",
>> +			regval);
>> +		return -EIO;
>> +	}
>> +
>> +	/* Mux shall be zeroed back before applying other configurations */
>> +	max961x_reg_write(max961x, REG_CTRL1, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static int max961x_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	int ret;
>> +	struct max961x_dev *max961x;
>> +	struct iio_dev *indio_dev;
>> +	struct device_node *of_node = client->dev.of_node;
>> +	unsigned int of_shunt;
>> +	const char * const shunt_res_prop = "shunt-resistor";
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max961x));
>> +	if (IS_ERR(indio_dev))
>> +		return PTR_ERR(indio_dev);
>> +
>> +	i2c_set_clientdata(client, indio_dev);
>> +
>> +	max961x		= iio_priv(indio_dev);
>> +	max961x->dev	= &client->dev;
>> +	max961x->i2c_client = client;
>> +
>> +	ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
>> +	if (ret) {
>> +		dev_err(&client->dev,
>> +			"Missing %s property for %s node\n",
>> +			shunt_res_prop, of_node->full_name);
>> +		return ret;
>> +	}
>> +	max961x->shunt_resistor_uOhm = of_shunt;
>> +
>> +	ret = max961x_init(max961x);
>> +	if (ret)
>> +		return ret;
>> +
>> +	indio_dev->dev.parent	= &client->dev;
>> +	indio_dev->dev.of_node	= client->dev.of_node;
>> +	indio_dev->name		= DRIVER_NAME;
>> +	indio_dev->modes	= INDIO_DIRECT_MODE;
>> +	indio_dev->info		= &indio_info;
>> +	indio_dev->channels	= max961x_channels;
>> +	indio_dev->num_channels	= ARRAY_SIZE(max961x_channels);
>> +
>> +	ret = devm_iio_device_register(&client->dev, indio_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_info(&client->dev, "%s: probed\n", DRIVER_NAME);
> This one is always hotly debated.  To my mind, this provides
> no information, but if you are particularly attached to it I don't
> really mind.  If not, then return devm_iio_* directly.
>> +
>> +	return 0;
>> +}
>> +
>> +static int max961x_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +
>> +	iio_device_unregister(indio_dev);
> If all you have in a remove function is a call to iio_device_unregister
> my immediate thought is could you use devm_iio_device_register.
>
> Ah, you are.  Double unregister!  If you are using the devm form of register
> it will be unregistered automatically as part of the removal of the
> underlying struct device.  Thus you don't need to do it explicitly.
>
> Thus you can drop this remove function completely as it should be
> empty.

Uh that's bad. I added the remove just before sending this out and tried 
to insert remove the module a couple of time without visible issues (I 
guess a double unregister fails silently then)
I'll drop this...


>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id max961x_of_table[] = {
>> +	{.compatible = "maxim,max961x"},
> Already been raised in the bindings reviews, but you shouldn't
> use wild cards in compatible strings.
>
> Actually as it almost always goes wrong, you shouldn't used them
> for naming of anything.  Pick a part and use that were you
> currently have max961x throughout the driver.
>
> i.e. max9611_of_table etc.
>
> Avoids possible issues down the line when maxim decide to
> release something incompatible and call it a max9619..

I'll use max9611 everywhere I can, and add 2 "compatible" entries for 
both max9611 and max9612.

Thanks
    j


>> +	{ },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, max961x_of_table);
>> +
>> +static struct i2c_driver max961x_driver = {
>> +	.driver = {
>> +		   .name = DRIVER_NAME,
>> +		   .owner = THIS_MODULE,
>> +		   .of_match_table = max961x_of_table,
>> +	},
>> +	.probe = max961x_probe,
>> +	.remove = max961x_remove,
>> +};
>> +module_i2c_driver(max961x_driver);
>> +
>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>");
>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>> +MODULE_LICENSE("GPL v2");
>>
>

Re: [PATCH 3/4] iio: adc: Add max9611/9612 ADC driver

[jacopo mondi]

Hi Jonathan,
    thanks for review

On 25/02/2017 16:53, Jonathan Cameron wrote:
> On 24/02/17 15:05, Jacopo Mondi wrote:
>> Add iio driver for Maxim max9611/9612 current-sense amplifiers with
>> 12-bits ADC.
> Data sheet link always good
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> I'm going to respond to the cover letter wrt to providing dynamic scales
> and offsets in a moment as you raised the question there.
>
> Otherwise, various bits and bobs inline.
>
> Interesting part to handle as it can read a lot of values that are useful only
> after some magic calculations are done.
>
> Jonathan
>> ---
>>  drivers/iio/adc/Kconfig   |  10 +
>>  drivers/iio/adc/Makefile  |   1 +
>>  drivers/iio/adc/max961x.c | 648 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 659 insertions(+)
>>  create mode 100644 drivers/iio/adc/max961x.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index dedae7a..f86026a 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -354,6 +354,16 @@ config MAX1363
>>  	  To compile this driver as a module, choose M here: the module will be
>>  	  called max1363.
>>
>> +config	MAX961x
>> +	tristate "Maxim max9611/9612 ADC driver"
>> +	depends on I2C
>> +	help
>> +	  Say yes here to build support for Maxim max9611/9612 current sense
>> +	  amplifiers with 12-bits ADC interface.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called max961x.
>> +
>>  config MCP320X
>>  	tristate "Microchip Technology MCP3x01/02/04/08"
>>  	depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index d001262..ff19250 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>>  obj-$(CONFIG_MAX1027) += max1027.o
>>  obj-$(CONFIG_MAX11100) += max11100.o
>>  obj-$(CONFIG_MAX1363) += max1363.o
>> +obj-$(CONFIG_MAX961x) += max961x.o
>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>>  obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>> diff --git a/drivers/iio/adc/max961x.c b/drivers/iio/adc/max961x.c
>> new file mode 100644
>> index 0000000..a544e69
>> --- /dev/null
>> +++ b/drivers/iio/adc/max961x.c
>> @@ -0,0 +1,648 @@
>> +/*
>> + * iio/adc/max961x.c
>> + *
>> + * Maxim max9611/9612 high side current sense amplifier with
>> + * 12-bit ADC interface.
>> + *
>> + * Copyright (C) 2017 Jacopo Mondi
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +/*
>> + * This driver supports input common-mode voltage, current-sense
>> + * amplifier with programmable gains and die temperature reading from
>> + * Maxim max9611/9612.
>> + * Op-amp, analog comparator, and watchdog functionalities are not
>> + * supported by this driver.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/module.h>
>> +
>> +#define DRIVER_NAME "max961x"
>> +
>> +/* max961x register addresses */
> Prefix these with MAX9611_ to avoid possible clashes in future.
>

I initially did, then removed that to counterbalance my tendency to be a 
bit verbose in naming stuff.

I'll use the MAX9611_ prefix where appropriate

>> +#define REG_CSA_DATA			0x00
[snip
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE) |
> The only cases we normally allow bother raw and processed output in
> are ones where we are supporting some defunct (i.e. wrong usage) in the
> userspace ABI.  So drop the processed path please.

Will do

>
>> +				  BIT(IIO_CHAN_INFO_PROCESSED),
>> +	  .address		= MAX961x_CHAN_TEMPERATURE,
>> +	},
>> +	{ /* [1] common voltage input */
>> +	  .type			= IIO_VOLTAGE,
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE) |
>> +				  BIT(IIO_CHAN_INFO_OFFSET),
>> +	  .address		= MAX961x_CHAN_VOLTAGE_INPUT,
>> +	  .channel		= MAX961x_CHAN_VOLTAGE_INPUT,
> For the channel it would be easier to read perhaps if you just put
> the 0 in directly here.
>> +	  .indexed		= 1,
>> +	},
>> +	{ /* [2] current sense amplifier voltage output */
>> +	  .type			= IIO_VOLTAGE,
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE) |
>> +				  BIT(IIO_CHAN_INFO_OFFSET),
>> +	  .address		= MAX961x_CHAN_VOLTAGE_SENSE,
>> +	  .channel		= MAX961x_CHAN_VOLTAGE_SENSE,
>> +	  .indexed		= 1,
>> +	},
>
> As you say, the next one is a computed channel.
>> +	{ /* [3] load current measurement */
>> +	  .type			= IIO_CURRENT,
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE),
>> +	  .address		= MAX961x_CHAN_CURRENT_LOAD,
>> +	},
> As is this one.

It's not clear to me if the fact of being a "computed channel" implies I 
have to use a different channel type to expose it (and, what channel 
type should I use in place of _RAW and _SCALE).
Please not my question is independent on the auto-gain discussion 
started in response the patch series cover letter.

>> +	{ /* [4] load current measurement */
>> +	  .type			= IIO_POWER,
>> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
>> +				  BIT(IIO_CHAN_INFO_SCALE),
>> +	  .address		= MAX961x_CHAN_POWER_LOAD,
>> +	},
>> +};
>> +
>> +static int max961x_reg_read(struct max961x_dev *max961x, u8 reg, u16 *val)
>> +{
> I'm not convinced this particular wrapper adds anything.
> I'd be tempted to just use the i2c read directly as needed rather than
> bounce through this.
>

I would say "inline it then", but I'm not sure trying to outsmart 
complier is desirable.

>> +	int ret;
>> +
>> +	ret = i2c_smbus_read_word_swapped(max961x->i2c_client, reg);
>> +	if (ret < 0) {
>> +		dev_err(max961x->dev, "i2c read word from 0x%2x failed\n",
>> +			reg);
>> +		return ret;
>> +	}
>> +
>> +	*val = ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int max961x_reg_write(struct max961x_dev *max961x, u8 reg, u8 val)
>> +{
>> +	int ret;
>> +
>> +	ret = i2c_smbus_write_byte_data(max961x->i2c_client, reg, val);
>> +	if (ret) {
>> +		dev_err(max961x->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>> +			reg, val);
>> +		return ret;
>> +	}
>> +
>> +	/* FIXME: need a delay here to make register configuration
>> +	 *	  stabilize. 1 msec at least, from empirical testing.
> Why fixme?  Sounds like it really needs to be the case to me!
>
> Also, fix up your multiline comment syntax please.

It's a FIXME as I found that out after some testing, I'm not sure about 
the root cause yet.
But yes, it's required to have driver work properly, so I'll drop this

>> +	 */
>> +	usleep_range(1000, 2000);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * max961x_read_single() - read a single vale from ADC interface
>> + *
>> + * Data registers are 16 bit long, spread between two 8 bit registers
>> + * with consecutive addresses.
>> + * Configure ADC mux first, then read register at address "reg_addr".
>> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
>> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
>> + *
>> + * @max961x: max961x device
>> + * @selector: index for mux and register configuration
>> + * @raw_val: the value returned from ADC
>> + */
>> +static int max961x_read_single(struct max961x_dev *max961x,
>> +			       enum max961x_conf_ids selector,
>> +			       u16 *raw_val)
>> +{
>> +	int ret;
>> +
>> +	u8 mux_conf = max961x_conf[selector][0] & MUX_MASK;
>> +	u8 reg_addr = max961x_conf[selector][1];
>> +
> You have no lockign around these various sequences.
> Nothing prevents multiple sysfs reads at the same time so you definitely
> want to prevent this stuff from running concurrently.
>
> A mutex should do the job, either at this low level or up in the calling
> functions.

I had verified the i2c_smbus_  functions where protected by their own 
mutexes and thought that was enough.
But yes, read and writes here shall be protected as well. Will re-add 
mutex back then


>> +	ret = max961x_reg_write(max961x, REG_CTRL1, mux_conf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = max961x_reg_read(max961x, reg_addr, raw_val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * max961x_read_csa_voltage() - read current sense amplifier output voltage
>> + *
>> + * Current sense amplifier output voltage is read through a configurable
>> + * 1x, 4x or 8x gain.
>> + * Start with plain 1x gain, and adjust gain control properly until a
>> + * meaningful value is read from ADC output.
>> + *
>> + * @max961x: max961x device
>> + * @adc_raw: raw value read from ADC output
>> + * @csa_gain: gain configuration option selector
>> + */
>> +static int max961x_read_csa_voltage(struct max961x_dev *max961x,
>> +				    u16 *adc_raw,
>> +				    enum max961x_csa_gain *csa_gain)
>> +{
>> +	int ret;
>> +	unsigned int i;
>> +	enum max961x_conf_ids gain_selectors[] = {
>> +		CONF_SENSE_1x,
>> +		CONF_SENSE_4x,
>> +		CONF_SENSE_8x
>> +	};
>> +
>> +	for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
>> +		ret = max961x_read_single(max961x, gain_selectors[i], adc_raw);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (*adc_raw > 0) {
>> +			*csa_gain = gain_selectors[i];
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -EIO;
>> +}
>> +
>> +static int max961x_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	int ret;
>> +	u16 adc_data;
>> +	enum max961x_csa_gain gain_selector;
>> +	unsigned int *csa_gain;
>> +	struct max961x_dev *dev = iio_priv(indio_dev);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		switch (chan->address) {
>> +		case MAX961x_CHAN_TEMPERATURE:
>> +			ret = max961x_read_single(dev, CONF_TEMP,
>> +						  &adc_data);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = TEMP_RAW(adc_data);
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_INPUT:
>> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
>> +						  &adc_data);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data);
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_SENSE:
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data);
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_CURRENT_LOAD:
>> +			/* raw (nV): raw * LSB (nV) */
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
>> +			if (ret)
>> +				return ret;
>> +
>> +			csa_gain = max961x_gain_conf[gain_selector];
>> +
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
>> +			       csa_gain[CSA_GAIN_LSB_nV];
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_POWER_LOAD:
>> +			/* raw (mV): raw * LSB (mV) */
>> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
>> +						  &adc_data);
>> +
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
>> +			       CIM_VOLTAGE_LSB_mV;
>> +
>> +			return IIO_VAL_INT;
>> +		}
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->address) {
>> +
>> +		case MAX961x_CHAN_TEMPERATURE:
>> +			/* processed (C): raw * scale (C) */
>> +			*val = TEMP_SCALE_NUM;
>> +			*val2 = TEMP_SCALE_DIV;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_INPUT:
>> +			/* processed (mV): raw * scale_mV */
> Err, I'm confused.  We read adc_data then do nothing with it?

leftover, sorry about this...

>> +			ret = max961x_read_single(dev, CONF_IN_VOLT,
>> +						  &adc_data);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = CIM_VOLTAGE_LSB_mV;
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_SENSE:
>> +			/* processed (mV): raw (mV) * (scale (nV) / 10^6) */
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
>> +			if (ret)
>> +				return ret;
> As with the offset below, this is a non starter to my mind as we have
> no way of knowing it was the 'same' gain value.  That has to be the case
> or we'll get all sorts of horrible instabilities in apparent output as
> we pass from one gain setting to another.

Let's keep discussing this in response to the cover letter

>> +
>> +			csa_gain = max961x_gain_conf[gain_selector];
>> +
>> +			*val = csa_gain[CSA_GAIN_LSB_nV];
>> +			*val2 = 1000000;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case MAX961x_CHAN_CURRENT_LOAD:
>> +			/* processed (mA): raw (nV)  / scale (uOhm)  */
>> +			*val = 1;
>> +			*val2 = dev->shunt_resistor_uOhm;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case MAX961x_CHAN_POWER_LOAD:
>> +			/* processed (mW): raw (mV) * (scale (mA) / 1000) */
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
> Again, nothing says the gain_selector value is the one used when we
> took the measurement (or will be if we take it in future) so we can't do
> it this way.  Either we need to remove the computed value and provide
> enough info to userspace to allow it to do the job, or we need to ensure
> that the kernel data manipulation doesn't allow for any instabilities around
> gain transitions.
>> +			if (ret)
>> +				return ret;
>> +
>> +			csa_gain = max961x_gain_conf[gain_selector];
>> +
>> +			/* val = (nV / uOhm) = mA */
>> +			*val = MAX961x_VOLTAGE_RAW(adc_data) *
>> +			       csa_gain[CSA_GAIN_LSB_nV];
>> +			*val /= dev->shunt_resistor_uOhm;
>> +
>> +			*val2 = 1000;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +		}
>> +
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		switch (chan->address) {
>> +		case MAX961x_CHAN_VOLTAGE_INPUT:
>> +			*val = CIM_VOLTAGE_OFFSET_RAW;
>> +
>> +			return IIO_VAL_INT;
>> +
>> +		case MAX961x_CHAN_VOLTAGE_SENSE:
> Interesting. Nothing here ensures the reported scale is correct for
> the reading we are looking it up for which is an issue.
>
> For a compound value - i.e. one where the userspace interpretation is
> dependent on several different readings (the classic being light sensors
> where illuminance is often computed from two different light sensing
> elementents) we tend to hide the complexity by doing it in kernel.
>
> So my gut feeling is this should be hidden away.  That should ensure
> the value used is the correct one as well.
>
>> +			ret = max961x_read_csa_voltage(dev, &adc_data,
>> +						       &gain_selector);
>> +			if (ret)
>> +				return ret;
>> +
>> +			*val = max961x_gain_conf[gain_selector]
>> +						[CSA_GAIN_OFFS_RAW];
>> +			*val2 = 1000;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +		}
>> +
>
> As stated above, please drop this as userspace can do this trivially from
> the raw and scale provided.

I agree that the offset may not match values returned from _raw and 
_scale (because of the auto-gain selection), but how can userspace 
retrieve offset from _raw and _scale if not opening the device datasheet?

>> +	case IIO_CHAN_INFO_PROCESSED:
>> +		/* processed (mC): raw * LSB (mC) */
>> +		if (chan->address != MAX961x_CHAN_TEMPERATURE)
>> +			return -EINVAL;
>> +
>> +		ret = max961x_read_single(dev, CONF_TEMP,
>> +					  &adc_data);
>> +		if (ret)
>> +			return ret;
>> +
>> +		*val = TEMP_RAW(adc_data) * TEMP_LSB_mC;
>> +
>> +		return IIO_VAL_INT;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static ssize_t max961x_shunt_resistor_show(struct device *dev,
>> +					   struct device_attribute *attr,
>> +					   char *buf)
>> +{
>> +	struct max961x_dev *max961x = iio_priv(dev_to_iio_dev(dev));
>> +
>> +	return sprintf(buf, "%d\n", max961x->shunt_resistor_uOhm);
>> +}
>> +
>> +static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO,
>> +		       max961x_shunt_resistor_show, NULL, 0);
>> +
>> +static struct attribute *max961x_attributes[] = {
>> +	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group max961x_attribute_group = {
>> +	.attrs = max961x_attributes,
>> +};
>> +
>> +static const struct iio_info indio_info = {
>> +	.driver_module	= THIS_MODULE,
>> +	.read_raw	= max961x_read_raw,
>> +	.attrs		= &max961x_attribute_group,
>> +};
>> +
>> +static int max961x_init(struct max961x_dev *max961x)
>> +{
>> +	int ret;
>> +	u16 regval;
>> +	struct i2c_client *client = max961x->i2c_client;
>> +
>> +	if (!i2c_check_functionality(client->adapter,
>> +				     I2C_FUNC_SMBUS_WRITE_BYTE	|
>> +				     I2C_FUNC_SMBUS_READ_WORD_DATA)) {
>> +		dev_err(max961x->dev,
>> +			"I2c adapter does not support smbus write_byte and "\
> Whilst it'll break the 80 char limit by quite a way, it's preferable to do that
> than to break the ability of someone to grep for an error message.  So keep
> this on one line please.

I'll make this less verbose.

>> +			"read_word_data functionalities. Aborting probe.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	max961x_reg_write(max961x, REG_CTRL1, MUX_TEMPERATURE);
>> +	max961x_reg_write(max961x, REG_CTRL2, 0);
>> +
>> +	/* Make sure die temperature is in range to test communications. */
>> +	regval = 0;
>> +	ret = max961x_reg_read(max961x, REG_TEMP_DATA, &regval);
>> +	if (ret)
>> +		return ret;
>> +
>> +	regval = regval & ~TEMP_MASK;
>> +	if ((regval > TEMP_MAX_RAW_POS &&
>> +	     regval < TEMP_MIN_RAW_NEG) ||
>> +	     regval > TEMP_MAX_RAW_NEG) {
>> +		dev_err(max961x->dev,
>> +			"In-valid value received from ADC 0x%4x: aborting\n",
>> +			regval);
>> +		return -EIO;
>> +	}
>> +
>> +	/* Mux shall be zeroed back before applying other configurations */
>> +	max961x_reg_write(max961x, REG_CTRL1, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static int max961x_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	int ret;
>> +	struct max961x_dev *max961x;
>> +	struct iio_dev *indio_dev;
>> +	struct device_node *of_node = client->dev.of_node;
>> +	unsigned int of_shunt;
>> +	const char * const shunt_res_prop = "shunt-resistor";
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max961x));
>> +	if (IS_ERR(indio_dev))
>> +		return PTR_ERR(indio_dev);
>> +
>> +	i2c_set_clientdata(client, indio_dev);
>> +
>> +	max961x		= iio_priv(indio_dev);
>> +	max961x->dev	= &client->dev;
>> +	max961x->i2c_client = client;
>> +
>> +	ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
>> +	if (ret) {
>> +		dev_err(&client->dev,
>> +			"Missing %s property for %s node\n",
>> +			shunt_res_prop, of_node->full_name);
>> +		return ret;
>> +	}
>> +	max961x->shunt_resistor_uOhm = of_shunt;
>> +
>> +	ret = max961x_init(max961x);
>> +	if (ret)
>> +		return ret;
>> +
>> +	indio_dev->dev.parent	= &client->dev;
>> +	indio_dev->dev.of_node	= client->dev.of_node;
>> +	indio_dev->name		= DRIVER_NAME;
>> +	indio_dev->modes	= INDIO_DIRECT_MODE;
>> +	indio_dev->info		= &indio_info;
>> +	indio_dev->channels	= max961x_channels;
>> +	indio_dev->num_channels	= ARRAY_SIZE(max961x_channels);
>> +
>> +	ret = devm_iio_device_register(&client->dev, indio_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_info(&client->dev, "%s: probed\n", DRIVER_NAME);
> This one is always hotly debated.  To my mind, this provides
> no information, but if you are particularly attached to it I don't
> really mind.  If not, then return devm_iio_* directly.
>> +
>> +	return 0;
>> +}
>> +
>> +static int max961x_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +
>> +	iio_device_unregister(indio_dev);
> If all you have in a remove function is a call to iio_device_unregister
> my immediate thought is could you use devm_iio_device_register.
>
> Ah, you are.  Double unregister!  If you are using the devm form of register
> it will be unregistered automatically as part of the removal of the
> underlying struct device.  Thus you don't need to do it explicitly.
>
> Thus you can drop this remove function completely as it should be
> empty.

Uh that's bad. I added the remove just before sending this out and tried 
to insert remove the module a couple of time without visible issues (I 
guess a double unregister fails silently then)
I'll drop this...


>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id max961x_of_table[] = {
>> +	{.compatible = "maxim,max961x"},
> Already been raised in the bindings reviews, but you shouldn't
> use wild cards in compatible strings.
>
> Actually as it almost always goes wrong, you shouldn't used them
> for naming of anything.  Pick a part and use that were you
> currently have max961x throughout the driver.
>
> i.e. max9611_of_table etc.
>
> Avoids possible issues down the line when maxim decide to
> release something incompatible and call it a max9619..

I'll use max9611 everywhere I can, and add 2 "compatible" entries for 
both max9611 and max9612.

Thanks
    j


>> +	{ },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, max961x_of_table);
>> +
>> +static struct i2c_driver max961x_driver = {
>> +	.driver = {
>> +		   .name = DRIVER_NAME,
>> +		   .owner = THIS_MODULE,
>> +		   .of_match_table = max961x_of_table,
>> +	},
>> +	.probe = max961x_probe,
>> +	.remove = max961x_remove,
>> +};
>> +module_i2c_driver(max961x_driver);
>> +
>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>> +MODULE_LICENSE("GPL v2");
>>
>

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[jacopo mondi]

Hi Jonathan,
On 25/02/2017 17:09, Jonathan Cameron wrote:
> On 24/02/17 15:05, Jacopo Mondi wrote:
>> Hello!
>>
>> This series adds driver and documentation for Maxim max9611/max9612
>> high-side current sense amplifier with 12-bit ADC and I2c interface.
>> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
>> in Renesas r87796 Salvator-X board.
>>
>> The device provides several functionalities, and only some of them are
>> currently supported by this driver.
>> Particularly, the on-board op-amp and analog comparator are not currently
>> supported.
> More fun to come ;)
>>
>> A simplified integration schematic drawing is here reported:
>>
>>  ----o----/\/\/-----o-------|LOAD|---
>>      |    shunt     |
>>  ____|______________|___
>>  |  RS+            RS-  |
>>  |   |-----gain-----|   |
>>  |          |           |
>>  |          |           |
>>  |max961x   |->| ADC |===== I2c
>>  |______________________|
>>
>>
>> The device provides though its 12-bits ADC the following informations:
> information is it's own plural (silly English quirk of the day!)
>
>> - Common input voltage on RS+
>> - Voltage drop between RS+ and RS- ends
>> - Die temperature
>>
>> All of the above ones are exposed though IIO with _raw and _scale values
>> (plus _input for milli degree Celsius die temperature).
>>
>> From the above values the driver calculates the current flowing between
>> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
>> the power load. Again this values are exposed through _raw and _scale
>> attributes, which I'm not completely sure it's acceptables as they are
>> calculated values and not natively provided by the current sense amplifier.
>> I would like to hear IIO people opinions on this, if they should be better
>> exposed though some other attributes which are not _raw and _scale, or if
>> their calculation should be completely left to userspace tools.
> So one element of the implementation is a problem.
> Because of the dynamic gain adjustment you are doing in the driver (which I
> rather like BTW) there will be instabilities in the computed values that
> userspace comes up with when we are near a transition for in the current
> sense amplifier gain.  We can't do that as crazy outputs will result
> (read offset and scale for possible different values of that gain then
> read the actual ADC value for a possible 3rd choice resulting in complete
> garbage).
>
> So there are two ways to avoid this:
> 1. Move all that magic into the original reads and apply the gain and offset
> before they get to userspace.
> 2. Provide enough information for userspace to be able to compute everything
> iff it is using buffered mode with a 'scan' covering all the channels in one
> go.  Even then we'd have to explicitly allow reading of the PGA gain as a
> channel, or make userspace responsible for doing your autogain stuff.
>
> 1 is probably easier but will make implementing 2 as a follow up will be tricky
> and would be needed if you want to read this stuff faster than sysfs will
> allow.
>
> It's a shame, but my gut feeling is you want to drop the autogain stuff
> as then it all becomes straight forward.
>
> What do others think?
>

Just to add the following element to the discussion.

The device supports cycling between channels by itself, sequentially 
cycling between them every 2 msec.
At the same time, gain setting is not involved in this procedure, and 
last applied is used on current sense voltage channel.

Still this does not help with un-synchronized reads of gain-dependent 
values, as offset and scale are.

Also, removing auto-gain setting routine will simplify stuff, but I 
don't see how all becomes straight forward then. I mean, if we keep gain 
at a fixed value, yes everything's easier, but it will work under a much 
more limited pre-conditions set...

Thanks
    j


> Jonathan
>>
>> Thanks
>>    j
>>
>> Jacopo Mondi (4):
>>   Documentation: dt-bindings: iio: Add max961x
>>   iio: Documentation: Add max961x sysfs documentation
>>   iio: adc: Add max9611/9612 ADC driver
>>   arm64: dts: salvator-x: Add current sense amplifiers
>>
>>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>>  drivers/iio/adc/Kconfig                            |  10 +
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>>  6 files changed, 708 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>  create mode 100644 drivers/iio/adc/max961x.c
>>
>

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[jacopo mondi]

Hi Jonathan,
On 25/02/2017 17:09, Jonathan Cameron wrote:
> On 24/02/17 15:05, Jacopo Mondi wrote:
>> Hello!
>>
>> This series adds driver and documentation for Maxim max9611/max9612
>> high-side current sense amplifier with 12-bit ADC and I2c interface.
>> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
>> in Renesas r87796 Salvator-X board.
>>
>> The device provides several functionalities, and only some of them are
>> currently supported by this driver.
>> Particularly, the on-board op-amp and analog comparator are not currently
>> supported.
> More fun to come ;)
>>
>> A simplified integration schematic drawing is here reported:
>>
>>  ----o----/\/\/-----o-------|LOAD|---
>>      |    shunt     |
>>  ____|______________|___
>>  |  RS+            RS-  |
>>  |   |-----gain-----|   |
>>  |          |           |
>>  |          |           |
>>  |max961x   |->| ADC |===== I2c
>>  |______________________|
>>
>>
>> The device provides though its 12-bits ADC the following informations:
> information is it's own plural (silly English quirk of the day!)
>
>> - Common input voltage on RS+
>> - Voltage drop between RS+ and RS- ends
>> - Die temperature
>>
>> All of the above ones are exposed though IIO with _raw and _scale values
>> (plus _input for milli degree Celsius die temperature).
>>
>> From the above values the driver calculates the current flowing between
>> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
>> the power load. Again this values are exposed through _raw and _scale
>> attributes, which I'm not completely sure it's acceptables as they are
>> calculated values and not natively provided by the current sense amplifier.
>> I would like to hear IIO people opinions on this, if they should be better
>> exposed though some other attributes which are not _raw and _scale, or if
>> their calculation should be completely left to userspace tools.
> So one element of the implementation is a problem.
> Because of the dynamic gain adjustment you are doing in the driver (which I
> rather like BTW) there will be instabilities in the computed values that
> userspace comes up with when we are near a transition for in the current
> sense amplifier gain.  We can't do that as crazy outputs will result
> (read offset and scale for possible different values of that gain then
> read the actual ADC value for a possible 3rd choice resulting in complete
> garbage).
>
> So there are two ways to avoid this:
> 1. Move all that magic into the original reads and apply the gain and offset
> before they get to userspace.
> 2. Provide enough information for userspace to be able to compute everything
> iff it is using buffered mode with a 'scan' covering all the channels in one
> go.  Even then we'd have to explicitly allow reading of the PGA gain as a
> channel, or make userspace responsible for doing your autogain stuff.
>
> 1 is probably easier but will make implementing 2 as a follow up will be tricky
> and would be needed if you want to read this stuff faster than sysfs will
> allow.
>
> It's a shame, but my gut feeling is you want to drop the autogain stuff
> as then it all becomes straight forward.
>
> What do others think?
>

Just to add the following element to the discussion.

The device supports cycling between channels by itself, sequentially 
cycling between them every 2 msec.
At the same time, gain setting is not involved in this procedure, and 
last applied is used on current sense voltage channel.

Still this does not help with un-synchronized reads of gain-dependent 
values, as offset and scale are.

Also, removing auto-gain setting routine will simplify stuff, but I 
don't see how all becomes straight forward then. I mean, if we keep gain 
at a fixed value, yes everything's easier, but it will work under a much 
more limited pre-conditions set...

Thanks
    j


> Jonathan
>>
>> Thanks
>>    j
>>
>> Jacopo Mondi (4):
>>   Documentation: dt-bindings: iio: Add max961x
>>   iio: Documentation: Add max961x sysfs documentation
>>   iio: adc: Add max9611/9612 ADC driver
>>   arm64: dts: salvator-x: Add current sense amplifiers
>>
>>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>>  drivers/iio/adc/Kconfig                            |  10 +
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>>  6 files changed, 708 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>  create mode 100644 drivers/iio/adc/max961x.c
>>
>

Re: [PATCH 3/4] iio: adc: Add max9611/9612 ADC driver

[Jonathan Cameron]

On 27/02/17 07:45, jacopo mondi wrote:
> Hi Jonathan,
>    thanks for review
> 
> On 25/02/2017 16:53, Jonathan Cameron wrote:
>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>> Add iio driver for Maxim max9611/9612 current-sense amplifiers with
>>> 12-bits ADC.
>> Data sheet link always good
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> I'm going to respond to the cover letter wrt to providing dynamic scales
>> and offsets in a moment as you raised the question there.
>>
>> Otherwise, various bits and bobs inline.
>>
>> Interesting part to handle as it can read a lot of values that are useful only
>> after some magic calculations are done.
>>
>> Jonathan
>>> ---
>>>  drivers/iio/adc/Kconfig   |  10 +
>>>  drivers/iio/adc/Makefile  |   1 +
>>>  drivers/iio/adc/max961x.c | 648 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 659 insertions(+)
>>>  create mode 100644 drivers/iio/adc/max961x.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index dedae7a..f86026a 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -354,6 +354,16 @@ config MAX1363
>>>        To compile this driver as a module, choose M here: the module will be
>>>        called max1363.
>>>
>>> +config    MAX961x
>>> +    tristate "Maxim max9611/9612 ADC driver"
>>> +    depends on I2C
>>> +    help
>>> +      Say yes here to build support for Maxim max9611/9612 current sense
>>> +      amplifiers with 12-bits ADC interface.
>>> +
>>> +      To compile this driver as a module, choose M here: the module will be
>>> +      called max961x.
>>> +
>>>  config MCP320X
>>>      tristate "Microchip Technology MCP3x01/02/04/08"
>>>      depends on SPI
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index d001262..ff19250 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>>>  obj-$(CONFIG_MAX1027) += max1027.o
>>>  obj-$(CONFIG_MAX11100) += max11100.o
>>>  obj-$(CONFIG_MAX1363) += max1363.o
>>> +obj-$(CONFIG_MAX961x) += max961x.o
>>>  obj-$(CONFIG_MCP320X) += mcp320x.o
>>>  obj-$(CONFIG_MCP3422) += mcp3422.o
>>>  obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>>> diff --git a/drivers/iio/adc/max961x.c b/drivers/iio/adc/max961x.c
>>> new file mode 100644
>>> index 0000000..a544e69
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/max961x.c
>>> @@ -0,0 +1,648 @@
>>> +/*
>>> + * iio/adc/max961x.c
>>> + *
>>> + * Maxim max9611/9612 high side current sense amplifier with
>>> + * 12-bit ADC interface.
>>> + *
>>> + * Copyright (C) 2017 Jacopo Mondi
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +/*
>>> + * This driver supports input common-mode voltage, current-sense
>>> + * amplifier with programmable gains and die temperature reading from
>>> + * Maxim max9611/9612.
>>> + * Op-amp, analog comparator, and watchdog functionalities are not
>>> + * supported by this driver.
>>> + */
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/module.h>
>>> +
>>> +#define DRIVER_NAME "max961x"
>>> +
>>> +/* max961x register addresses */
>> Prefix these with MAX9611_ to avoid possible clashes in future.
>>
> 
> I initially did, then removed that to counterbalance my tendency to be a bit verbose in naming stuff.
> 
> I'll use the MAX9611_ prefix where appropriate
> 
>>> +#define REG_CSA_DATA            0x00
> [snip
>>> +      .info_mask_separate    = BIT(IIO_CHAN_INFO_RAW) |
>>> +                  BIT(IIO_CHAN_INFO_SCALE) |
>> The only cases we normally allow bother raw and processed output in
>> are ones where we are supporting some defunct (i.e. wrong usage) in the
>> userspace ABI.  So drop the processed path please.
> 
> Will do
> 
>>
>>> +                  BIT(IIO_CHAN_INFO_PROCESSED),
>>> +      .address        = MAX961x_CHAN_TEMPERATURE,
>>> +    },
>>> +    { /* [1] common voltage input */
>>> +      .type            = IIO_VOLTAGE,
>>> +      .info_mask_separate    = BIT(IIO_CHAN_INFO_RAW) |
>>> +                  BIT(IIO_CHAN_INFO_SCALE) |
>>> +                  BIT(IIO_CHAN_INFO_OFFSET),
>>> +      .address        = MAX961x_CHAN_VOLTAGE_INPUT,
>>> +      .channel        = MAX961x_CHAN_VOLTAGE_INPUT,
>> For the channel it would be easier to read perhaps if you just put
>> the 0 in directly here.
>>> +      .indexed        = 1,
>>> +    },
>>> +    { /* [2] current sense amplifier voltage output */
>>> +      .type            = IIO_VOLTAGE,
>>> +      .info_mask_separate    = BIT(IIO_CHAN_INFO_RAW) |
>>> +                  BIT(IIO_CHAN_INFO_SCALE) |
>>> +                  BIT(IIO_CHAN_INFO_OFFSET),
>>> +      .address        = MAX961x_CHAN_VOLTAGE_SENSE,
>>> +      .channel        = MAX961x_CHAN_VOLTAGE_SENSE,
>>> +      .indexed        = 1,
>>> +    },
>>
>> As you say, the next one is a computed channel.
>>> +    { /* [3] load current measurement */
>>> +      .type            = IIO_CURRENT,
>>> +      .info_mask_separate    = BIT(IIO_CHAN_INFO_RAW) |
>>> +                  BIT(IIO_CHAN_INFO_SCALE),
>>> +      .address        = MAX961x_CHAN_CURRENT_LOAD,
>>> +    },
>> As is this one.
> 
> It's not clear to me if the fact of being a "computed channel" implies I have to use a different channel type to expose it (and, what channel type should I use in place of _RAW and _SCALE).

Basically boils down to whether the conversion is linear or not.  Doesn't
strictly speaking havie antyhing to do with whether it is computed from
a number of inputs or not. If the conversion is linear then leave it to
userspace (_raw and _scale + _offset if needed).  If it's non linear we
have no way to describe it so we have to do it in kernel and use _processed.

However, another element applies.  If either the scale or offset is not
independent of the reading (i.e. we can't cache a value indefinitely in
userspace code and expect it not to change unless userspace asks it to)
then we should wrap that up in the kernel and just used a processed output
directly. Otherwise userspace can not be sure it got a valid set of values
from which to compute the real reading.

> Please not my question is independent on the auto-gain discussion started in response the patch series cover letter.
> 
>>> +    { /* [4] load current measurement */
>>> +      .type            = IIO_POWER,
>>> +      .info_mask_separate    = BIT(IIO_CHAN_INFO_RAW) |
>>> +                  BIT(IIO_CHAN_INFO_SCALE),
>>> +      .address        = MAX961x_CHAN_POWER_LOAD,
>>> +    },
>>> +};
>>> +
>>> +static int max961x_reg_read(struct max961x_dev *max961x, u8 reg, u16 *val)
>>> +{
>> I'm not convinced this particular wrapper adds anything.
>> I'd be tempted to just use the i2c read directly as needed rather than
>> bounce through this.
>>
> 
> I would say "inline it then", but I'm not sure trying to outsmart complier is desirable.

My issue is that the wrapper adds code that needs to be reviewed, with no
actual benefit given it's a direct and simple call to the i2c core.
> 
>>> +    int ret;
>>> +
>>> +    ret = i2c_smbus_read_word_swapped(max961x->i2c_client, reg);
>>> +    if (ret < 0) {
>>> +        dev_err(max961x->dev, "i2c read word from 0x%2x failed\n",
>>> +            reg);
>>> +        return ret;
>>> +    }
>>> +
>>> +    *val = ret;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int max961x_reg_write(struct max961x_dev *max961x, u8 reg, u8 val)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = i2c_smbus_write_byte_data(max961x->i2c_client, reg, val);
>>> +    if (ret) {
>>> +        dev_err(max961x->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
>>> +            reg, val);
>>> +        return ret;
>>> +    }
>>> +
>>> +    /* FIXME: need a delay here to make register configuration
>>> +     *      stabilize. 1 msec at least, from empirical testing.
>> Why fixme?  Sounds like it really needs to be the case to me!
>>
>> Also, fix up your multiline comment syntax please.
> 
> It's a FIXME as I found that out after some testing, I'm not sure about the root cause yet.
> But yes, it's required to have driver work properly, so I'll drop this
> 
>>> +     */
>>> +    usleep_range(1000, 2000);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * max961x_read_single() - read a single vale from ADC interface
>>> + *
>>> + * Data registers are 16 bit long, spread between two 8 bit registers
>>> + * with consecutive addresses.
>>> + * Configure ADC mux first, then read register at address "reg_addr".
>>> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
>>> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
>>> + *
>>> + * @max961x: max961x device
>>> + * @selector: index for mux and register configuration
>>> + * @raw_val: the value returned from ADC
>>> + */
>>> +static int max961x_read_single(struct max961x_dev *max961x,
>>> +                   enum max961x_conf_ids selector,
>>> +                   u16 *raw_val)
>>> +{
>>> +    int ret;
>>> +
>>> +    u8 mux_conf = max961x_conf[selector][0] & MUX_MASK;
>>> +    u8 reg_addr = max961x_conf[selector][1];
>>> +
>> You have no lockign around these various sequences.
>> Nothing prevents multiple sysfs reads at the same time so you definitely
>> want to prevent this stuff from running concurrently.
>>
>> A mutex should do the job, either at this low level or up in the calling
>> functions.
> 
> I had verified the i2c_smbus_  functions where protected by their own mutexes and thought that was enough.
That protects the bus, but not a interacting sequences like this.  Could
easily have two running in parallel.

> But yes, read and writes here shall be protected as well. Will re-add mutex back then
> 
> 
>>> +    ret = max961x_reg_write(max961x, REG_CTRL1, mux_conf);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = max961x_reg_read(max961x, reg_addr, raw_val);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * max961x_read_csa_voltage() - read current sense amplifier output voltage
>>> + *
>>> + * Current sense amplifier output voltage is read through a configurable
>>> + * 1x, 4x or 8x gain.
>>> + * Start with plain 1x gain, and adjust gain control properly until a
>>> + * meaningful value is read from ADC output.
>>> + *
>>> + * @max961x: max961x device
>>> + * @adc_raw: raw value read from ADC output
>>> + * @csa_gain: gain configuration option selector
>>> + */
>>> +static int max961x_read_csa_voltage(struct max961x_dev *max961x,
>>> +                    u16 *adc_raw,
>>> +                    enum max961x_csa_gain *csa_gain)
>>> +{
>>> +    int ret;
>>> +    unsigned int i;
>>> +    enum max961x_conf_ids gain_selectors[] = {
>>> +        CONF_SENSE_1x,
>>> +        CONF_SENSE_4x,
>>> +        CONF_SENSE_8x
>>> +    };
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
>>> +        ret = max961x_read_single(max961x, gain_selectors[i], adc_raw);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        if (*adc_raw > 0) {
>>> +            *csa_gain = gain_selectors[i];
>>> +            return 0;
>>> +        }
>>> +    }
>>> +
>>> +    return -EIO;
>>> +}
>>> +
>>> +static int max961x_read_raw(struct iio_dev *indio_dev,
>>> +                struct iio_chan_spec const *chan,
>>> +                int *val, int *val2, long mask)
>>> +{
>>> +    int ret;
>>> +    u16 adc_data;
>>> +    enum max961x_csa_gain gain_selector;
>>> +    unsigned int *csa_gain;
>>> +    struct max961x_dev *dev = iio_priv(indio_dev);
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_RAW:
>>> +        switch (chan->address) {
>>> +        case MAX961x_CHAN_TEMPERATURE:
>>> +            ret = max961x_read_single(dev, CONF_TEMP,
>>> +                          &adc_data);
>>> +            if (ret)
>>> +                return ret;
>>> +
>>> +            *val = TEMP_RAW(adc_data);
>>> +
>>> +            return IIO_VAL_INT;
>>> +
>>> +        case MAX961x_CHAN_VOLTAGE_INPUT:
>>> +            ret = max961x_read_single(dev, CONF_IN_VOLT,
>>> +                          &adc_data);
>>> +            if (ret)
>>> +                return ret;
>>> +
>>> +            *val = MAX961x_VOLTAGE_RAW(adc_data);
>>> +
>>> +            return IIO_VAL_INT;
>>> +
>>> +        case MAX961x_CHAN_VOLTAGE_SENSE:
>>> +            ret = max961x_read_csa_voltage(dev, &adc_data,
>>> +                               &gain_selector);
>>> +            if (ret)
>>> +                return ret;
>>> +
>>> +            *val = MAX961x_VOLTAGE_RAW(adc_data);
>>> +
>>> +            return IIO_VAL_INT;
>>> +
>>> +        case MAX961x_CHAN_CURRENT_LOAD:
>>> +            /* raw (nV): raw * LSB (nV) */
>>> +            ret = max961x_read_csa_voltage(dev, &adc_data,
>>> +                               &gain_selector);
>>> +            if (ret)
>>> +                return ret;
>>> +
>>> +            csa_gain = max961x_gain_conf[gain_selector];
>>> +
>>> +            *val = MAX961x_VOLTAGE_RAW(adc_data) *
>>> +                   csa_gain[CSA_GAIN_LSB_nV];
>>> +
>>> +            return IIO_VAL_INT;
>>> +
>>> +        case MAX961x_CHAN_POWER_LOAD:
>>> +            /* raw (mV): raw * LSB (mV) */
>>> +            ret = max961x_read_single(dev, CONF_IN_VOLT,
>>> +                          &adc_data);
>>> +
>>> +            *val = MAX961x_VOLTAGE_RAW(adc_data) *
>>> +                   CIM_VOLTAGE_LSB_mV;
>>> +
>>> +            return IIO_VAL_INT;
>>> +        }
>>> +
>>> +    case IIO_CHAN_INFO_SCALE:
>>> +        switch (chan->address) {
>>> +
>>> +        case MAX961x_CHAN_TEMPERATURE:
>>> +            /* processed (C): raw * scale (C) */
>>> +            *val = TEMP_SCALE_NUM;
>>> +            *val2 = TEMP_SCALE_DIV;
>>> +
>>> +            return IIO_VAL_FRACTIONAL;
>>> +
>>> +        case MAX961x_CHAN_VOLTAGE_INPUT:
>>> +            /* processed (mV): raw * scale_mV */
>> Err, I'm confused.  We read adc_data then do nothing with it?
> 
> leftover, sorry about this...
> 
>>> +            ret = max961x_read_single(dev, CONF_IN_VOLT,
>>> +                          &adc_data);
>>> +            if (ret)
>>> +                return ret;
>>> +
>>> +            *val = CIM_VOLTAGE_LSB_mV;
>>> +
>>> +            return IIO_VAL_INT;
>>> +
>>> +        case MAX961x_CHAN_VOLTAGE_SENSE:
>>> +            /* processed (mV): raw (mV) * (scale (nV) / 10^6) */
>>> +            ret = max961x_read_csa_voltage(dev, &adc_data,
>>> +                               &gain_selector);
>>> +            if (ret)
>>> +                return ret;
>> As with the offset below, this is a non starter to my mind as we have
>> no way of knowing it was the 'same' gain value.  That has to be the case
>> or we'll get all sorts of horrible instabilities in apparent output as
>> we pass from one gain setting to another.
> 
> Let's keep discussing this in response to the cover letter
> 
>>> +
>>> +            csa_gain = max961x_gain_conf[gain_selector];
>>> +
>>> +            *val = csa_gain[CSA_GAIN_LSB_nV];
>>> +            *val2 = 1000000;
>>> +
>>> +            return IIO_VAL_FRACTIONAL;
>>> +
>>> +        case MAX961x_CHAN_CURRENT_LOAD:
>>> +            /* processed (mA): raw (nV)  / scale (uOhm)  */
>>> +            *val = 1;
>>> +            *val2 = dev->shunt_resistor_uOhm;
>>> +
>>> +            return IIO_VAL_FRACTIONAL;
>>> +
>>> +        case MAX961x_CHAN_POWER_LOAD:
>>> +            /* processed (mW): raw (mV) * (scale (mA) / 1000) */
>>> +            ret = max961x_read_csa_voltage(dev, &adc_data,
>>> +                               &gain_selector);
>> Again, nothing says the gain_selector value is the one used when we
>> took the measurement (or will be if we take it in future) so we can't do
>> it this way.  Either we need to remove the computed value and provide
>> enough info to userspace to allow it to do the job, or we need to ensure
>> that the kernel data manipulation doesn't allow for any instabilities around
>> gain transitions.
>>> +            if (ret)
>>> +                return ret;
>>> +
>>> +            csa_gain = max961x_gain_conf[gain_selector];
>>> +
>>> +            /* val = (nV / uOhm) = mA */
>>> +            *val = MAX961x_VOLTAGE_RAW(adc_data) *
>>> +                   csa_gain[CSA_GAIN_LSB_nV];
>>> +            *val /= dev->shunt_resistor_uOhm;
>>> +
>>> +            *val2 = 1000;
>>> +
>>> +            return IIO_VAL_FRACTIONAL;
>>> +        }
>>> +
>>> +    case IIO_CHAN_INFO_OFFSET:
>>> +        switch (chan->address) {
>>> +        case MAX961x_CHAN_VOLTAGE_INPUT:
>>> +            *val = CIM_VOLTAGE_OFFSET_RAW;
>>> +
>>> +            return IIO_VAL_INT;
>>> +
>>> +        case MAX961x_CHAN_VOLTAGE_SENSE:
>> Interesting. Nothing here ensures the reported scale is correct for
>> the reading we are looking it up for which is an issue.
>>
>> For a compound value - i.e. one where the userspace interpretation is
>> dependent on several different readings (the classic being light sensors
>> where illuminance is often computed from two different light sensing
>> elementents) we tend to hide the complexity by doing it in kernel.
>>
>> So my gut feeling is this should be hidden away.  That should ensure
>> the value used is the correct one as well.
>>
>>> +            ret = max961x_read_csa_voltage(dev, &adc_data,
>>> +                               &gain_selector);
>>> +            if (ret)
>>> +                return ret;
>>> +
>>> +            *val = max961x_gain_conf[gain_selector]
>>> +                        [CSA_GAIN_OFFS_RAW];
>>> +            *val2 = 1000;
>>> +
>>> +            return IIO_VAL_FRACTIONAL;
>>> +        }
>>> +
>>
>> As stated above, please drop this as userspace can do this trivially from
>> the raw and scale provided.
> 
> I agree that the offset may not match values returned from _raw and _scale (because of the auto-gain selection), but how can userspace retrieve offset from _raw and _scale if not opening the device datasheet?

Two crossed bits of review here.  If it had been the case that raw and
scale were fixed then having this made no sense.  As it turns out the
opposite is true, so we should not have raw output for any channels where
the scale might change.  Sorry I should have cleaned this comment up
after I understood what was going on in more detail.
> 
>>> +    case IIO_CHAN_INFO_PROCESSED:
>>> +        /* processed (mC): raw * LSB (mC) */
>>> +        if (chan->address != MAX961x_CHAN_TEMPERATURE)
>>> +            return -EINVAL;
>>> +
>>> +        ret = max961x_read_single(dev, CONF_TEMP,
>>> +                      &adc_data);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        *val = TEMP_RAW(adc_data) * TEMP_LSB_mC;
>>> +
>>> +        return IIO_VAL_INT;
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static ssize_t max961x_shunt_resistor_show(struct device *dev,
>>> +                       struct device_attribute *attr,
>>> +                       char *buf)
>>> +{
>>> +    struct max961x_dev *max961x = iio_priv(dev_to_iio_dev(dev));
>>> +
>>> +    return sprintf(buf, "%d\n", max961x->shunt_resistor_uOhm);
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO,
>>> +               max961x_shunt_resistor_show, NULL, 0);
>>> +
>>> +static struct attribute *max961x_attributes[] = {
>>> +    &iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>>> +    NULL,
>>> +};
>>> +
>>> +static const struct attribute_group max961x_attribute_group = {
>>> +    .attrs = max961x_attributes,
>>> +};
>>> +
>>> +static const struct iio_info indio_info = {
>>> +    .driver_module    = THIS_MODULE,
>>> +    .read_raw    = max961x_read_raw,
>>> +    .attrs        = &max961x_attribute_group,
>>> +};
>>> +
>>> +static int max961x_init(struct max961x_dev *max961x)
>>> +{
>>> +    int ret;
>>> +    u16 regval;
>>> +    struct i2c_client *client = max961x->i2c_client;
>>> +
>>> +    if (!i2c_check_functionality(client->adapter,
>>> +                     I2C_FUNC_SMBUS_WRITE_BYTE    |
>>> +                     I2C_FUNC_SMBUS_READ_WORD_DATA)) {
>>> +        dev_err(max961x->dev,
>>> +            "I2c adapter does not support smbus write_byte and "\
>> Whilst it'll break the 80 char limit by quite a way, it's preferable to do that
>> than to break the ability of someone to grep for an error message.  So keep
>> this on one line please.
> 
> I'll make this less verbose.
Can do or just have a long line!
> 
>>> +            "read_word_data functionalities. Aborting probe.\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    max961x_reg_write(max961x, REG_CTRL1, MUX_TEMPERATURE);
>>> +    max961x_reg_write(max961x, REG_CTRL2, 0);
>>> +
>>> +    /* Make sure die temperature is in range to test communications. */
>>> +    regval = 0;
>>> +    ret = max961x_reg_read(max961x, REG_TEMP_DATA, &regval);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    regval = regval & ~TEMP_MASK;
>>> +    if ((regval > TEMP_MAX_RAW_POS &&
>>> +         regval < TEMP_MIN_RAW_NEG) ||
>>> +         regval > TEMP_MAX_RAW_NEG) {
>>> +        dev_err(max961x->dev,
>>> +            "In-valid value received from ADC 0x%4x: aborting\n",
>>> +            regval);
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    /* Mux shall be zeroed back before applying other configurations */
>>> +    max961x_reg_write(max961x, REG_CTRL1, 0);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int max961x_probe(struct i2c_client *client,
>>> +             const struct i2c_device_id *id)
>>> +{
>>> +    int ret;
>>> +    struct max961x_dev *max961x;
>>> +    struct iio_dev *indio_dev;
>>> +    struct device_node *of_node = client->dev.of_node;
>>> +    unsigned int of_shunt;
>>> +    const char * const shunt_res_prop = "shunt-resistor";
>>> +
>>> +    indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max961x));
>>> +    if (IS_ERR(indio_dev))
>>> +        return PTR_ERR(indio_dev);
>>> +
>>> +    i2c_set_clientdata(client, indio_dev);
>>> +
>>> +    max961x        = iio_priv(indio_dev);
>>> +    max961x->dev    = &client->dev;
>>> +    max961x->i2c_client = client;
>>> +
>>> +    ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
>>> +    if (ret) {
>>> +        dev_err(&client->dev,
>>> +            "Missing %s property for %s node\n",
>>> +            shunt_res_prop, of_node->full_name);
>>> +        return ret;
>>> +    }
>>> +    max961x->shunt_resistor_uOhm = of_shunt;
>>> +
>>> +    ret = max961x_init(max961x);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    indio_dev->dev.parent    = &client->dev;
>>> +    indio_dev->dev.of_node    = client->dev.of_node;
>>> +    indio_dev->name        = DRIVER_NAME;
>>> +    indio_dev->modes    = INDIO_DIRECT_MODE;
>>> +    indio_dev->info        = &indio_info;
>>> +    indio_dev->channels    = max961x_channels;
>>> +    indio_dev->num_channels    = ARRAY_SIZE(max961x_channels);
>>> +
>>> +    ret = devm_iio_device_register(&client->dev, indio_dev);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    dev_info(&client->dev, "%s: probed\n", DRIVER_NAME);
>> This one is always hotly debated.  To my mind, this provides
>> no information, but if you are particularly attached to it I don't
>> really mind.  If not, then return devm_iio_* directly.
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int max961x_remove(struct i2c_client *client)
>>> +{
>>> +    struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +
>>> +    iio_device_unregister(indio_dev);
>> If all you have in a remove function is a call to iio_device_unregister
>> my immediate thought is could you use devm_iio_device_register.
>>
>> Ah, you are.  Double unregister!  If you are using the devm form of register
>> it will be unregistered automatically as part of the removal of the
>> underlying struct device.  Thus you don't need to do it explicitly.
>>
>> Thus you can drop this remove function completely as it should be
>> empty.
> 
> Uh that's bad. I added the remove just before sending this out and tried to insert remove the module a couple of time without visible issues (I guess a double unregister fails silently then)
> I'll drop this...
> 
> 
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct of_device_id max961x_of_table[] = {
>>> +    {.compatible = "maxim,max961x"},
>> Already been raised in the bindings reviews, but you shouldn't
>> use wild cards in compatible strings.
>>
>> Actually as it almost always goes wrong, you shouldn't used them
>> for naming of anything.  Pick a part and use that were you
>> currently have max961x throughout the driver.
>>
>> i.e. max9611_of_table etc.
>>
>> Avoids possible issues down the line when maxim decide to
>> release something incompatible and call it a max9619..
> 
> I'll use max9611 everywhere I can, and add 2 "compatible" entries for both max9611 and max9612.
Cool
> 
> Thanks
>    j
> 
> 
>>> +    { },
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(of, max961x_of_table);
>>> +
>>> +static struct i2c_driver max961x_driver = {
>>> +    .driver = {
>>> +           .name = DRIVER_NAME,
>>> +           .owner = THIS_MODULE,
>>> +           .of_match_table = max961x_of_table,
>>> +    },
>>> +    .probe = max961x_probe,
>>> +    .remove = max961x_remove,
>>> +};
>>> +module_i2c_driver(max961x_driver);
>>> +
>>> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org>");
>>> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[Jonathan Cameron]

On 27/02/17 09:09, jacopo mondi wrote:
> Hi Jonathan,
> On 25/02/2017 17:09, Jonathan Cameron wrote:
>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>> Hello!
>>>
>>> This series adds driver and documentation for Maxim max9611/max9612
>>> high-side current sense amplifier with 12-bit ADC and I2c interface.
>>> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
>>> in Renesas r87796 Salvator-X board.
>>>
>>> The device provides several functionalities, and only some of them are
>>> currently supported by this driver.
>>> Particularly, the on-board op-amp and analog comparator are not currently
>>> supported.
>> More fun to come ;)
>>>
>>> A simplified integration schematic drawing is here reported:
>>>
>>>  ----o----/\/\/-----o-------|LOAD|---
>>>      |    shunt     |
>>>  ____|______________|___
>>>  |  RS+            RS-  |
>>>  |   |-----gain-----|   |
>>>  |          |           |
>>>  |          |           |
>>>  |max961x   |->| ADC |===== I2c
>>>  |______________________|
>>>
>>>
>>> The device provides though its 12-bits ADC the following informations:
>> information is it's own plural (silly English quirk of the day!)
>>
>>> - Common input voltage on RS+
>>> - Voltage drop between RS+ and RS- ends
>>> - Die temperature
>>>
>>> All of the above ones are exposed though IIO with _raw and _scale values
>>> (plus _input for milli degree Celsius die temperature).
>>>
>>> From the above values the driver calculates the current flowing between
>>> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
>>> the power load. Again this values are exposed through _raw and _scale
>>> attributes, which I'm not completely sure it's acceptables as they are
>>> calculated values and not natively provided by the current sense amplifier.
>>> I would like to hear IIO people opinions on this, if they should be better
>>> exposed though some other attributes which are not _raw and _scale, or if
>>> their calculation should be completely left to userspace tools.
>> So one element of the implementation is a problem.
>> Because of the dynamic gain adjustment you are doing in the driver (which I
>> rather like BTW) there will be instabilities in the computed values that
>> userspace comes up with when we are near a transition for in the current
>> sense amplifier gain.  We can't do that as crazy outputs will result
>> (read offset and scale for possible different values of that gain then
>> read the actual ADC value for a possible 3rd choice resulting in complete
>> garbage).
>>
>> So there are two ways to avoid this:
>> 1. Move all that magic into the original reads and apply the gain and offset
>> before they get to userspace.
>> 2. Provide enough information for userspace to be able to compute everything
>> iff it is using buffered mode with a 'scan' covering all the channels in one
>> go.  Even then we'd have to explicitly allow reading of the PGA gain as a
>> channel, or make userspace responsible for doing your autogain stuff.
>>
>> 1 is probably easier but will make implementing 2 as a follow up will be tricky
>> and would be needed if you want to read this stuff faster than sysfs will
>> allow.
>>
>> It's a shame, but my gut feeling is you want to drop the autogain stuff
>> as then it all becomes straight forward.
>>
>> What do others think?
>>
> 
> Just to add the following element to the discussion.
> 
> The device supports cycling between channels by itself, sequentially cycling between them every 2 msec.
> At the same time, gain setting is not involved in this procedure, and last applied is used on current sense voltage channel.
> 
> Still this does not help with un-synchronized reads of gain-dependent values, as offset and scale are.
> 
> Also, removing auto-gain setting routine will simplify stuff, but I don't see how all becomes straight forward then. I mean, if we keep gain at a fixed value, yes everything's easier, but it will work under a much more limited pre-conditions set...

You'd be moving the issue to userspace.  This would be beneficial if you
were looking to use the buffered interfaces (through a kfifo).  In those
cases processed values are often a non starter as they don't have well
defined 'sizes' in the same way that a reading directly from the chip does.

So under those circumstances, your userspace code might elect to modify
the gain to get in the 'right' region only at startup, or after some initial
'calibration data capture'.  Then it could merily stream data out without
caring about it.  To run remotely fast and consistenty you'd have to stop
doing the multiple passes needed for autogain.

However, if that interface never makes sense for this device anyway then
it the approach of doing it all in kernel makes more sense.

With a 500sps device like this it's on the boundary to my mind...

Quick enough that using the chardev access makes sense, but the usecase
is perhaps such that we should leave this all in kernel space.

It's slightly horrible but there is a path forward eventually if we
were to add buffered support having gone with the do it all in the kernel:
* Add a 'autorange_enable' attribute to allow us to turn it off or implicitly
assume that it is turned off if we are doing buffered capture (which is a bit
nasty from a 'doing what you'd expect' viewpoint).
* Add scale attributes at that stage.

Now, if the auto range stuff was done on the actual chip we could figure
out how to export that as pseudo channels alongside the real ones but
that might get uggly and isn't the case here anyway.

Jonathan


> 
> Thanks
>    j
> 
> 
>> Jonathan
>>>
>>> Thanks
>>>    j
>>>
>>> Jacopo Mondi (4):
>>>   Documentation: dt-bindings: iio: Add max961x
>>>   iio: Documentation: Add max961x sysfs documentation
>>>   iio: adc: Add max9611/9612 ADC driver
>>>   arm64: dts: salvator-x: Add current sense amplifiers
>>>
>>>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>>>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>>>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>>>  drivers/iio/adc/Kconfig                            |  10 +
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>>>  6 files changed, 708 insertions(+)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>  create mode 100644 drivers/iio/adc/max961x.c
>>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[Jonathan Cameron]

On 27/02/17 09:09, jacopo mondi wrote:
> Hi Jonathan,
> On 25/02/2017 17:09, Jonathan Cameron wrote:
>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>> Hello!
>>>
>>> This series adds driver and documentation for Maxim max9611/max9612
>>> high-side current sense amplifier with 12-bit ADC and I2c interface.
>>> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
>>> in Renesas r87796 Salvator-X board.
>>>
>>> The device provides several functionalities, and only some of them are
>>> currently supported by this driver.
>>> Particularly, the on-board op-amp and analog comparator are not currently
>>> supported.
>> More fun to come ;)
>>>
>>> A simplified integration schematic drawing is here reported:
>>>
>>>  ----o----/\/\/-----o-------|LOAD|---
>>>      |    shunt     |
>>>  ____|______________|___
>>>  |  RS+            RS-  |
>>>  |   |-----gain-----|   |
>>>  |          |           |
>>>  |          |           |
>>>  |max961x   |->| ADC |===== I2c
>>>  |______________________|
>>>
>>>
>>> The device provides though its 12-bits ADC the following informations:
>> information is it's own plural (silly English quirk of the day!)
>>
>>> - Common input voltage on RS+
>>> - Voltage drop between RS+ and RS- ends
>>> - Die temperature
>>>
>>> All of the above ones are exposed though IIO with _raw and _scale values
>>> (plus _input for milli degree Celsius die temperature).
>>>
>>> From the above values the driver calculates the current flowing between
>>> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
>>> the power load. Again this values are exposed through _raw and _scale
>>> attributes, which I'm not completely sure it's acceptables as they are
>>> calculated values and not natively provided by the current sense amplifier.
>>> I would like to hear IIO people opinions on this, if they should be better
>>> exposed though some other attributes which are not _raw and _scale, or if
>>> their calculation should be completely left to userspace tools.
>> So one element of the implementation is a problem.
>> Because of the dynamic gain adjustment you are doing in the driver (which I
>> rather like BTW) there will be instabilities in the computed values that
>> userspace comes up with when we are near a transition for in the current
>> sense amplifier gain.  We can't do that as crazy outputs will result
>> (read offset and scale for possible different values of that gain then
>> read the actual ADC value for a possible 3rd choice resulting in complete
>> garbage).
>>
>> So there are two ways to avoid this:
>> 1. Move all that magic into the original reads and apply the gain and offset
>> before they get to userspace.
>> 2. Provide enough information for userspace to be able to compute everything
>> iff it is using buffered mode with a 'scan' covering all the channels in one
>> go.  Even then we'd have to explicitly allow reading of the PGA gain as a
>> channel, or make userspace responsible for doing your autogain stuff.
>>
>> 1 is probably easier but will make implementing 2 as a follow up will be tricky
>> and would be needed if you want to read this stuff faster than sysfs will
>> allow.
>>
>> It's a shame, but my gut feeling is you want to drop the autogain stuff
>> as then it all becomes straight forward.
>>
>> What do others think?
>>
> 
> Just to add the following element to the discussion.
> 
> The device supports cycling between channels by itself, sequentially cycling between them every 2 msec.
> At the same time, gain setting is not involved in this procedure, and last applied is used on current sense voltage channel.
> 
> Still this does not help with un-synchronized reads of gain-dependent values, as offset and scale are.
> 
> Also, removing auto-gain setting routine will simplify stuff, but I don't see how all becomes straight forward then. I mean, if we keep gain at a fixed value, yes everything's easier, but it will work under a much more limited pre-conditions set...

You'd be moving the issue to userspace.  This would be beneficial if you
were looking to use the buffered interfaces (through a kfifo).  In those
cases processed values are often a non starter as they don't have well
defined 'sizes' in the same way that a reading directly from the chip does.

So under those circumstances, your userspace code might elect to modify
the gain to get in the 'right' region only at startup, or after some initial
'calibration data capture'.  Then it could merily stream data out without
caring about it.  To run remotely fast and consistenty you'd have to stop
doing the multiple passes needed for autogain.

However, if that interface never makes sense for this device anyway then
it the approach of doing it all in kernel makes more sense.

With a 500sps device like this it's on the boundary to my mind...

Quick enough that using the chardev access makes sense, but the usecase
is perhaps such that we should leave this all in kernel space.

It's slightly horrible but there is a path forward eventually if we
were to add buffered support having gone with the do it all in the kernel:
* Add a 'autorange_enable' attribute to allow us to turn it off or implicitly
assume that it is turned off if we are doing buffered capture (which is a bit
nasty from a 'doing what you'd expect' viewpoint).
* Add scale attributes at that stage.

Now, if the auto range stuff was done on the actual chip we could figure
out how to export that as pseudo channels alongside the real ones but
that might get uggly and isn't the case here anyway.

Jonathan


> 
> Thanks
>    j
> 
> 
>> Jonathan
>>>
>>> Thanks
>>>    j
>>>
>>> Jacopo Mondi (4):
>>>   Documentation: dt-bindings: iio: Add max961x
>>>   iio: Documentation: Add max961x sysfs documentation
>>>   iio: adc: Add max9611/9612 ADC driver
>>>   arm64: dts: salvator-x: Add current sense amplifiers
>>>
>>>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>>>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>>>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>>>  drivers/iio/adc/Kconfig                            |  10 +
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>>>  6 files changed, 708 insertions(+)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>  create mode 100644 drivers/iio/adc/max961x.c
>>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[jacopo mondi]

Hi Jonathan,

    + Neil Amstrong from Baylibre

On 05/03/2017 11:49, Jonathan Cameron wrote:
> On 27/02/17 09:09, jacopo mondi wrote:
>> Hi Jonathan,
>> On 25/02/2017 17:09, Jonathan Cameron wrote:
>>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>>> Hello!
>>>>
>>>> This series adds driver and documentation for Maxim max9611/max9612
>>>> high-side current sense amplifier with 12-bit ADC and I2c interface.
>>>> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
>>>> in Renesas r87796 Salvator-X board.
>>>>
>>>> The device provides several functionalities, and only some of them are
>>>> currently supported by this driver.
>>>> Particularly, the on-board op-amp and analog comparator are not currently
>>>> supported.
>>> More fun to come ;)
>>>>
>>>> A simplified integration schematic drawing is here reported:
>>>>
>>>>  ----o----/\/\/-----o-------|LOAD|---
>>>>      |    shunt     |
>>>>  ____|______________|___
>>>>  |  RS+            RS-  |
>>>>  |   |-----gain-----|   |
>>>>  |          |           |
>>>>  |          |           |
>>>>  |max961x   |->| ADC |===== I2c
>>>>  |______________________|
>>>>
>>>>
>>>> The device provides though its 12-bits ADC the following informations:
>>> information is it's own plural (silly English quirk of the day!)
>>>
>>>> - Common input voltage on RS+
>>>> - Voltage drop between RS+ and RS- ends
>>>> - Die temperature
>>>>
>>>> All of the above ones are exposed though IIO with _raw and _scale values
>>>> (plus _input for milli degree Celsius die temperature).
>>>>
>>>> From the above values the driver calculates the current flowing between
>>>> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
>>>> the power load. Again this values are exposed through _raw and _scale
>>>> attributes, which I'm not completely sure it's acceptables as they are
>>>> calculated values and not natively provided by the current sense amplifier.
>>>> I would like to hear IIO people opinions on this, if they should be better
>>>> exposed though some other attributes which are not _raw and _scale, or if
>>>> their calculation should be completely left to userspace tools.
>>> So one element of the implementation is a problem.
>>> Because of the dynamic gain adjustment you are doing in the driver (which I
>>> rather like BTW) there will be instabilities in the computed values that
>>> userspace comes up with when we are near a transition for in the current
>>> sense amplifier gain.  We can't do that as crazy outputs will result
>>> (read offset and scale for possible different values of that gain then
>>> read the actual ADC value for a possible 3rd choice resulting in complete
>>> garbage).
>>>
>>> So there are two ways to avoid this:
>>> 1. Move all that magic into the original reads and apply the gain and offset
>>> before they get to userspace.
>>> 2. Provide enough information for userspace to be able to compute everything
>>> iff it is using buffered mode with a 'scan' covering all the channels in one
>>> go.  Even then we'd have to explicitly allow reading of the PGA gain as a
>>> channel, or make userspace responsible for doing your autogain stuff.
>>>
>>> 1 is probably easier but will make implementing 2 as a follow up will be tricky
>>> and would be needed if you want to read this stuff faster than sysfs will
>>> allow.
>>>
>>> It's a shame, but my gut feeling is you want to drop the autogain stuff
>>> as then it all becomes straight forward.
>>>
>>> What do others think?
>>>
>>
>> Just to add the following element to the discussion.
>>
>> The device supports cycling between channels by itself, sequentially cycling between them every 2 msec.
>> At the same time, gain setting is not involved in this procedure, and last applied is used on current sense voltage channel.
>>
>> Still this does not help with un-synchronized reads of gain-dependent values, as offset and scale are.
>>
>> Also, removing auto-gain setting routine will simplify stuff, but I don't see how all becomes straight forward then. I mean, if we keep gain at a fixed value, yes everything's easier, but it will work under a much more limited pre-conditions set...
>
> You'd be moving the issue to userspace.  This would be beneficial if you
> were looking to use the buffered interfaces (through a kfifo).  In those
> cases processed values are often a non starter as they don't have well
> defined 'sizes' in the same way that a reading directly from the chip does.
>
> So under those circumstances, your userspace code might elect to modify
> the gain to get in the 'right' region only at startup, or after some initial
> 'calibration data capture'.  Then it could merily stream data out without
> caring about it.  To run remotely fast and consistenty you'd have to stop
> doing the multiple passes needed for autogain.
>
> However, if that interface never makes sense for this device anyway then
> it the approach of doing it all in kernel makes more sense.
>
> With a 500sps device like this it's on the boundary to my mind...
>
> Quick enough that using the chardev access makes sense, but the usecase
> is perhaps such that we should leave this all in kernel space.
>
> It's slightly horrible but there is a path forward eventually if we
> were to add buffered support having gone with the do it all in the kernel:
> * Add a 'autorange_enable' attribute to allow us to turn it off or implicitly
> assume that it is turned off if we are doing buffered capture (which is a bit
> nasty from a 'doing what you'd expect' viewpoint).
> * Add scale attributes at that stage.
>
> Now, if the auto range stuff was done on the actual chip we could figure
> out how to export that as pseudo channels alongside the real ones but
> that might get uggly and isn't the case here anyway.
>
> Jonathan
>

Let me summarize this a bit, so to make sure we're on the same page.

- No buffered reads:
-- current and power are processed only channels.
-- processed channels do not live well with buffered reads.
-- no scale and offset attributes are exposed to userland
-- kernel takes care of autogain (introducing a little delay in each read)

- Buffered reads:
-- userspace needs to "calibrate" the autogain
-- scale and offset depends on gain settings and shall be created after 
gain selection

Now, I'm under the impression what really makes a difference here is the 
use case this driver has to address, and sincerely, I cannot define a 
precise one right now, at least not in terms of typical sampling 
frequency of power monitoring tools.

That's why I have now copied Neil Armstrong, as Baylibre's ACME is a 
sort-of-standard out there, and they may have faced the same issues when 
implementing ina2x driver (which is used for power sensing in ACME cape 
if I'm not wrong).
I hope he may be able to follow here, and provide some wisdom on 
required data access speed.

imho, if no strict speed requirements are present, and we can live with 
chrdev access only, exposing 2 processed channels, and let kernel deal 
with gain configuration is a more desirable solutions in terms of 
interface clearness.
But again, let's wait for more people to comment here!

Thanks
    j




>
>>
>> Thanks
>>    j
>>
>>
>>> Jonathan
>>>>
>>>> Thanks
>>>>    j
>>>>
>>>> Jacopo Mondi (4):
>>>>   Documentation: dt-bindings: iio: Add max961x
>>>>   iio: Documentation: Add max961x sysfs documentation
>>>>   iio: adc: Add max9611/9612 ADC driver
>>>>   arm64: dts: salvator-x: Add current sense amplifiers
>>>>
>>>>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>>>>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>>>>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>>>>  drivers/iio/adc/Kconfig                            |  10 +
>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>>>>  6 files changed, 708 insertions(+)
>>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>>  create mode 100644 drivers/iio/adc/max961x.c
>>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[jacopo mondi]

Hi Jonathan,

    + Neil Amstrong from Baylibre

On 05/03/2017 11:49, Jonathan Cameron wrote:
> On 27/02/17 09:09, jacopo mondi wrote:
>> Hi Jonathan,
>> On 25/02/2017 17:09, Jonathan Cameron wrote:
>>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>>> Hello!
>>>>
>>>> This series adds driver and documentation for Maxim max9611/max9612
>>>> high-side current sense amplifier with 12-bit ADC and I2c interface.
>>>> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
>>>> in Renesas r87796 Salvator-X board.
>>>>
>>>> The device provides several functionalities, and only some of them are
>>>> currently supported by this driver.
>>>> Particularly, the on-board op-amp and analog comparator are not currently
>>>> supported.
>>> More fun to come ;)
>>>>
>>>> A simplified integration schematic drawing is here reported:
>>>>
>>>>  ----o----/\/\/-----o-------|LOAD|---
>>>>      |    shunt     |
>>>>  ____|______________|___
>>>>  |  RS+            RS-  |
>>>>  |   |-----gain-----|   |
>>>>  |          |           |
>>>>  |          |           |
>>>>  |max961x   |->| ADC |===== I2c
>>>>  |______________________|
>>>>
>>>>
>>>> The device provides though its 12-bits ADC the following informations:
>>> information is it's own plural (silly English quirk of the day!)
>>>
>>>> - Common input voltage on RS+
>>>> - Voltage drop between RS+ and RS- ends
>>>> - Die temperature
>>>>
>>>> All of the above ones are exposed though IIO with _raw and _scale values
>>>> (plus _input for milli degree Celsius die temperature).
>>>>
>>>> From the above values the driver calculates the current flowing between
>>>> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
>>>> the power load. Again this values are exposed through _raw and _scale
>>>> attributes, which I'm not completely sure it's acceptables as they are
>>>> calculated values and not natively provided by the current sense amplifier.
>>>> I would like to hear IIO people opinions on this, if they should be better
>>>> exposed though some other attributes which are not _raw and _scale, or if
>>>> their calculation should be completely left to userspace tools.
>>> So one element of the implementation is a problem.
>>> Because of the dynamic gain adjustment you are doing in the driver (which I
>>> rather like BTW) there will be instabilities in the computed values that
>>> userspace comes up with when we are near a transition for in the current
>>> sense amplifier gain.  We can't do that as crazy outputs will result
>>> (read offset and scale for possible different values of that gain then
>>> read the actual ADC value for a possible 3rd choice resulting in complete
>>> garbage).
>>>
>>> So there are two ways to avoid this:
>>> 1. Move all that magic into the original reads and apply the gain and offset
>>> before they get to userspace.
>>> 2. Provide enough information for userspace to be able to compute everything
>>> iff it is using buffered mode with a 'scan' covering all the channels in one
>>> go.  Even then we'd have to explicitly allow reading of the PGA gain as a
>>> channel, or make userspace responsible for doing your autogain stuff.
>>>
>>> 1 is probably easier but will make implementing 2 as a follow up will be tricky
>>> and would be needed if you want to read this stuff faster than sysfs will
>>> allow.
>>>
>>> It's a shame, but my gut feeling is you want to drop the autogain stuff
>>> as then it all becomes straight forward.
>>>
>>> What do others think?
>>>
>>
>> Just to add the following element to the discussion.
>>
>> The device supports cycling between channels by itself, sequentially cycling between them every 2 msec.
>> At the same time, gain setting is not involved in this procedure, and last applied is used on current sense voltage channel.
>>
>> Still this does not help with un-synchronized reads of gain-dependent values, as offset and scale are.
>>
>> Also, removing auto-gain setting routine will simplify stuff, but I don't see how all becomes straight forward then. I mean, if we keep gain at a fixed value, yes everything's easier, but it will work under a much more limited pre-conditions set...
>
> You'd be moving the issue to userspace.  This would be beneficial if you
> were looking to use the buffered interfaces (through a kfifo).  In those
> cases processed values are often a non starter as they don't have well
> defined 'sizes' in the same way that a reading directly from the chip does.
>
> So under those circumstances, your userspace code might elect to modify
> the gain to get in the 'right' region only at startup, or after some initial
> 'calibration data capture'.  Then it could merily stream data out without
> caring about it.  To run remotely fast and consistenty you'd have to stop
> doing the multiple passes needed for autogain.
>
> However, if that interface never makes sense for this device anyway then
> it the approach of doing it all in kernel makes more sense.
>
> With a 500sps device like this it's on the boundary to my mind...
>
> Quick enough that using the chardev access makes sense, but the usecase
> is perhaps such that we should leave this all in kernel space.
>
> It's slightly horrible but there is a path forward eventually if we
> were to add buffered support having gone with the do it all in the kernel:
> * Add a 'autorange_enable' attribute to allow us to turn it off or implicitly
> assume that it is turned off if we are doing buffered capture (which is a bit
> nasty from a 'doing what you'd expect' viewpoint).
> * Add scale attributes at that stage.
>
> Now, if the auto range stuff was done on the actual chip we could figure
> out how to export that as pseudo channels alongside the real ones but
> that might get uggly and isn't the case here anyway.
>
> Jonathan
>

Let me summarize this a bit, so to make sure we're on the same page.

- No buffered reads:
-- current and power are processed only channels.
-- processed channels do not live well with buffered reads.
-- no scale and offset attributes are exposed to userland
-- kernel takes care of autogain (introducing a little delay in each read)

- Buffered reads:
-- userspace needs to "calibrate" the autogain
-- scale and offset depends on gain settings and shall be created after 
gain selection

Now, I'm under the impression what really makes a difference here is the 
use case this driver has to address, and sincerely, I cannot define a 
precise one right now, at least not in terms of typical sampling 
frequency of power monitoring tools.

That's why I have now copied Neil Armstrong, as Baylibre's ACME is a 
sort-of-standard out there, and they may have faced the same issues when 
implementing ina2x driver (which is used for power sensing in ACME cape 
if I'm not wrong).
I hope he may be able to follow here, and provide some wisdom on 
required data access speed.

imho, if no strict speed requirements are present, and we can live with 
chrdev access only, exposing 2 processed channels, and let kernel deal 
with gain configuration is a more desirable solutions in terms of 
interface clearness.
But again, let's wait for more people to comment here!

Thanks
    j




>
>>
>> Thanks
>>    j
>>
>>
>>> Jonathan
>>>>
>>>> Thanks
>>>>    j
>>>>
>>>> Jacopo Mondi (4):
>>>>   Documentation: dt-bindings: iio: Add max961x
>>>>   iio: Documentation: Add max961x sysfs documentation
>>>>   iio: adc: Add max9611/9612 ADC driver
>>>>   arm64: dts: salvator-x: Add current sense amplifiers
>>>>
>>>>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>>>>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>>>>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>>>>  drivers/iio/adc/Kconfig                            |  10 +
>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>>>>  6 files changed, 708 insertions(+)
>>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>>  create mode 100644 drivers/iio/adc/max961x.c
>>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[Jonathan Cameron]

On 07/03/17 19:12, jacopo mondi wrote:
> Hi Jonathan,
> 
>    + Neil Amstrong from Baylibre
> 
> On 05/03/2017 11:49, Jonathan Cameron wrote:
>> On 27/02/17 09:09, jacopo mondi wrote:
>>> Hi Jonathan,
>>> On 25/02/2017 17:09, Jonathan Cameron wrote:
>>>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>>>> Hello!
>>>>>
>>>>> This series adds driver and documentation for Maxim max9611/max9612
>>>>> high-side current sense amplifier with 12-bit ADC and I2c interface.
>>>>> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
>>>>> in Renesas r87796 Salvator-X board.
>>>>>
>>>>> The device provides several functionalities, and only some of them are
>>>>> currently supported by this driver.
>>>>> Particularly, the on-board op-amp and analog comparator are not currently
>>>>> supported.
>>>> More fun to come ;)
>>>>>
>>>>> A simplified integration schematic drawing is here reported:
>>>>>
>>>>>  ----o----/\/\/-----o-------|LOAD|---
>>>>>      |    shunt     |
>>>>>  ____|______________|___
>>>>>  |  RS+            RS-  |
>>>>>  |   |-----gain-----|   |
>>>>>  |          |           |
>>>>>  |          |           |
>>>>>  |max961x   |->| ADC |===== I2c
>>>>>  |______________________|
>>>>>
>>>>>
>>>>> The device provides though its 12-bits ADC the following informations:
>>>> information is it's own plural (silly English quirk of the day!)
>>>>
>>>>> - Common input voltage on RS+
>>>>> - Voltage drop between RS+ and RS- ends
>>>>> - Die temperature
>>>>>
>>>>> All of the above ones are exposed though IIO with _raw and _scale values
>>>>> (plus _input for milli degree Celsius die temperature).
>>>>>
>>>>> From the above values the driver calculates the current flowing between
>>>>> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
>>>>> the power load. Again this values are exposed through _raw and _scale
>>>>> attributes, which I'm not completely sure it's acceptables as they are
>>>>> calculated values and not natively provided by the current sense amplifier.
>>>>> I would like to hear IIO people opinions on this, if they should be better
>>>>> exposed though some other attributes which are not _raw and _scale, or if
>>>>> their calculation should be completely left to userspace tools.
>>>> So one element of the implementation is a problem.
>>>> Because of the dynamic gain adjustment you are doing in the driver (which I
>>>> rather like BTW) there will be instabilities in the computed values that
>>>> userspace comes up with when we are near a transition for in the current
>>>> sense amplifier gain.  We can't do that as crazy outputs will result
>>>> (read offset and scale for possible different values of that gain then
>>>> read the actual ADC value for a possible 3rd choice resulting in complete
>>>> garbage).
>>>>
>>>> So there are two ways to avoid this:
>>>> 1. Move all that magic into the original reads and apply the gain and offset
>>>> before they get to userspace.
>>>> 2. Provide enough information for userspace to be able to compute everything
>>>> iff it is using buffered mode with a 'scan' covering all the channels in one
>>>> go.  Even then we'd have to explicitly allow reading of the PGA gain as a
>>>> channel, or make userspace responsible for doing your autogain stuff.
>>>>
>>>> 1 is probably easier but will make implementing 2 as a follow up will be tricky
>>>> and would be needed if you want to read this stuff faster than sysfs will
>>>> allow.
>>>>
>>>> It's a shame, but my gut feeling is you want to drop the autogain stuff
>>>> as then it all becomes straight forward.
>>>>
>>>> What do others think?
>>>>
>>>
>>> Just to add the following element to the discussion.
>>>
>>> The device supports cycling between channels by itself, sequentially cycling between them every 2 msec.
>>> At the same time, gain setting is not involved in this procedure, and last applied is used on current sense voltage channel.
>>>
>>> Still this does not help with un-synchronized reads of gain-dependent values, as offset and scale are.
>>>
>>> Also, removing auto-gain setting routine will simplify stuff, but I don't see how all becomes straight forward then. I mean, if we keep gain at a fixed value, yes everything's easier, but it will work under a much more limited pre-conditions set...
>>
>> You'd be moving the issue to userspace.  This would be beneficial if you
>> were looking to use the buffered interfaces (through a kfifo).  In those
>> cases processed values are often a non starter as they don't have well
>> defined 'sizes' in the same way that a reading directly from the chip does.
>>
>> So under those circumstances, your userspace code might elect to modify
>> the gain to get in the 'right' region only at startup, or after some initial
>> 'calibration data capture'.  Then it could merily stream data out without
>> caring about it.  To run remotely fast and consistenty you'd have to stop
>> doing the multiple passes needed for autogain.
>>
>> However, if that interface never makes sense for this device anyway then
>> it the approach of doing it all in kernel makes more sense.
>>
>> With a 500sps device like this it's on the boundary to my mind...
>>
>> Quick enough that using the chardev access makes sense, but the usecase
>> is perhaps such that we should leave this all in kernel space.
>>
>> It's slightly horrible but there is a path forward eventually if we
>> were to add buffered support having gone with the do it all in the kernel:
>> * Add a 'autorange_enable' attribute to allow us to turn it off or implicitly
>> assume that it is turned off if we are doing buffered capture (which is a bit
>> nasty from a 'doing what you'd expect' viewpoint).
>> * Add scale attributes at that stage.
>>
>> Now, if the auto range stuff was done on the actual chip we could figure
>> out how to export that as pseudo channels alongside the real ones but
>> that might get uggly and isn't the case here anyway.
>>
>> Jonathan
>>
> 
> Let me summarize this a bit, so to make sure we're on the same page.
> 
> - No buffered reads:
> -- current and power are processed only channels.
> -- processed channels do not live well with buffered reads.
> -- no scale and offset attributes are exposed to userland
> -- kernel takes care of autogain (introducing a little delay in each read)
> 
> - Buffered reads:
> -- userspace needs to "calibrate" the autogain
> -- scale and offset depends on gain settings and shall be created after gain selection
spot on.
> 
> Now, I'm under the impression what really makes a difference here is
> the use case this driver has to address, and sincerely, I cannot
> define a precise one right now, at least not in terms of typical
> sampling frequency of power monitoring tools.
> 
> That's why I have now copied Neil Armstrong, as Baylibre's ACME is a
> sort-of-standard out there, and they may have faced the same issues
> when implementing ina2x driver (which is used for power sensing in
> ACME cape if I'm not wrong). I hope he may be able to follow here,
> and provide some wisdom on required data access speed.
> 
> imho, if no strict speed requirements are present, and we can live
> with chrdev access only, exposing 2 processed channels, and let
That would be sysfs only, buffered reads are the chrdev ones (confusing
terminology :()
> kernel deal with gain configuration is a more desirable solutions in
> terms of interface clearness. But again, let's wait for more people
> to comment here!
Agreed, guess we give it a little longer!

Jonathan
> 
Thanks
>    j
> 
> 
> 
> 
>>
>>>
>>> Thanks
>>>    j
>>>
>>>
>>>> Jonathan
>>>>>
>>>>> Thanks
>>>>>    j
>>>>>
>>>>> Jacopo Mondi (4):
>>>>>   Documentation: dt-bindings: iio: Add max961x
>>>>>   iio: Documentation: Add max961x sysfs documentation
>>>>>   iio: adc: Add max9611/9612 ADC driver
>>>>>   arm64: dts: salvator-x: Add current sense amplifiers
>>>>>
>>>>>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>>>>>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>>>>>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>>>>>  drivers/iio/adc/Kconfig                            |  10 +
>>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>>>>>  6 files changed, 708 insertions(+)
>>>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>>>  create mode 100644 drivers/iio/adc/max961x.c
>>>>>
>>>>
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[Jonathan Cameron]

On 07/03/17 19:12, jacopo mondi wrote:
> Hi Jonathan,
> 
>    + Neil Amstrong from Baylibre
> 
> On 05/03/2017 11:49, Jonathan Cameron wrote:
>> On 27/02/17 09:09, jacopo mondi wrote:
>>> Hi Jonathan,
>>> On 25/02/2017 17:09, Jonathan Cameron wrote:
>>>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>>>> Hello!
>>>>>
>>>>> This series adds driver and documentation for Maxim max9611/max9612
>>>>> high-side current sense amplifier with 12-bit ADC and I2c interface.
>>>>> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
>>>>> in Renesas r87796 Salvator-X board.
>>>>>
>>>>> The device provides several functionalities, and only some of them are
>>>>> currently supported by this driver.
>>>>> Particularly, the on-board op-amp and analog comparator are not currently
>>>>> supported.
>>>> More fun to come ;)
>>>>>
>>>>> A simplified integration schematic drawing is here reported:
>>>>>
>>>>>  ----o----/\/\/-----o-------|LOAD|---
>>>>>      |    shunt     |
>>>>>  ____|______________|___
>>>>>  |  RS+            RS-  |
>>>>>  |   |-----gain-----|   |
>>>>>  |          |           |
>>>>>  |          |           |
>>>>>  |max961x   |->| ADC |===== I2c
>>>>>  |______________________|
>>>>>
>>>>>
>>>>> The device provides though its 12-bits ADC the following informations:
>>>> information is it's own plural (silly English quirk of the day!)
>>>>
>>>>> - Common input voltage on RS+
>>>>> - Voltage drop between RS+ and RS- ends
>>>>> - Die temperature
>>>>>
>>>>> All of the above ones are exposed though IIO with _raw and _scale values
>>>>> (plus _input for milli degree Celsius die temperature).
>>>>>
>>>>> From the above values the driver calculates the current flowing between
>>>>> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
>>>>> the power load. Again this values are exposed through _raw and _scale
>>>>> attributes, which I'm not completely sure it's acceptables as they are
>>>>> calculated values and not natively provided by the current sense amplifier.
>>>>> I would like to hear IIO people opinions on this, if they should be better
>>>>> exposed though some other attributes which are not _raw and _scale, or if
>>>>> their calculation should be completely left to userspace tools.
>>>> So one element of the implementation is a problem.
>>>> Because of the dynamic gain adjustment you are doing in the driver (which I
>>>> rather like BTW) there will be instabilities in the computed values that
>>>> userspace comes up with when we are near a transition for in the current
>>>> sense amplifier gain.  We can't do that as crazy outputs will result
>>>> (read offset and scale for possible different values of that gain then
>>>> read the actual ADC value for a possible 3rd choice resulting in complete
>>>> garbage).
>>>>
>>>> So there are two ways to avoid this:
>>>> 1. Move all that magic into the original reads and apply the gain and offset
>>>> before they get to userspace.
>>>> 2. Provide enough information for userspace to be able to compute everything
>>>> iff it is using buffered mode with a 'scan' covering all the channels in one
>>>> go.  Even then we'd have to explicitly allow reading of the PGA gain as a
>>>> channel, or make userspace responsible for doing your autogain stuff.
>>>>
>>>> 1 is probably easier but will make implementing 2 as a follow up will be tricky
>>>> and would be needed if you want to read this stuff faster than sysfs will
>>>> allow.
>>>>
>>>> It's a shame, but my gut feeling is you want to drop the autogain stuff
>>>> as then it all becomes straight forward.
>>>>
>>>> What do others think?
>>>>
>>>
>>> Just to add the following element to the discussion.
>>>
>>> The device supports cycling between channels by itself, sequentially cycling between them every 2 msec.
>>> At the same time, gain setting is not involved in this procedure, and last applied is used on current sense voltage channel.
>>>
>>> Still this does not help with un-synchronized reads of gain-dependent values, as offset and scale are.
>>>
>>> Also, removing auto-gain setting routine will simplify stuff, but I don't see how all becomes straight forward then. I mean, if we keep gain at a fixed value, yes everything's easier, but it will work under a much more limited pre-conditions set...
>>
>> You'd be moving the issue to userspace.  This would be beneficial if you
>> were looking to use the buffered interfaces (through a kfifo).  In those
>> cases processed values are often a non starter as they don't have well
>> defined 'sizes' in the same way that a reading directly from the chip does.
>>
>> So under those circumstances, your userspace code might elect to modify
>> the gain to get in the 'right' region only at startup, or after some initial
>> 'calibration data capture'.  Then it could merily stream data out without
>> caring about it.  To run remotely fast and consistenty you'd have to stop
>> doing the multiple passes needed for autogain.
>>
>> However, if that interface never makes sense for this device anyway then
>> it the approach of doing it all in kernel makes more sense.
>>
>> With a 500sps device like this it's on the boundary to my mind...
>>
>> Quick enough that using the chardev access makes sense, but the usecase
>> is perhaps such that we should leave this all in kernel space.
>>
>> It's slightly horrible but there is a path forward eventually if we
>> were to add buffered support having gone with the do it all in the kernel:
>> * Add a 'autorange_enable' attribute to allow us to turn it off or implicitly
>> assume that it is turned off if we are doing buffered capture (which is a bit
>> nasty from a 'doing what you'd expect' viewpoint).
>> * Add scale attributes at that stage.
>>
>> Now, if the auto range stuff was done on the actual chip we could figure
>> out how to export that as pseudo channels alongside the real ones but
>> that might get uggly and isn't the case here anyway.
>>
>> Jonathan
>>
> 
> Let me summarize this a bit, so to make sure we're on the same page.
> 
> - No buffered reads:
> -- current and power are processed only channels.
> -- processed channels do not live well with buffered reads.
> -- no scale and offset attributes are exposed to userland
> -- kernel takes care of autogain (introducing a little delay in each read)
> 
> - Buffered reads:
> -- userspace needs to "calibrate" the autogain
> -- scale and offset depends on gain settings and shall be created after gain selection
spot on.
> 
> Now, I'm under the impression what really makes a difference here is
> the use case this driver has to address, and sincerely, I cannot
> define a precise one right now, at least not in terms of typical
> sampling frequency of power monitoring tools.
> 
> That's why I have now copied Neil Armstrong, as Baylibre's ACME is a
> sort-of-standard out there, and they may have faced the same issues
> when implementing ina2x driver (which is used for power sensing in
> ACME cape if I'm not wrong). I hope he may be able to follow here,
> and provide some wisdom on required data access speed.
> 
> imho, if no strict speed requirements are present, and we can live
> with chrdev access only, exposing 2 processed channels, and let
That would be sysfs only, buffered reads are the chrdev ones (confusing
terminology :()
> kernel deal with gain configuration is a more desirable solutions in
> terms of interface clearness. But again, let's wait for more people
> to comment here!
Agreed, guess we give it a little longer!

Jonathan
> 
Thanks
>    j
> 
> 
> 
> 
>>
>>>
>>> Thanks
>>>    j
>>>
>>>
>>>> Jonathan
>>>>>
>>>>> Thanks
>>>>>    j
>>>>>
>>>>> Jacopo Mondi (4):
>>>>>   Documentation: dt-bindings: iio: Add max961x
>>>>>   iio: Documentation: Add max961x sysfs documentation
>>>>>   iio: adc: Add max9611/9612 ADC driver
>>>>>   arm64: dts: salvator-x: Add current sense amplifiers
>>>>>
>>>>>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>>>>>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>>>>>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>>>>>  drivers/iio/adc/Kconfig                            |  10 +
>>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>>>>>  6 files changed, 708 insertions(+)
>>>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>>>  create mode 100644 drivers/iio/adc/max961x.c
>>>>>
>>>>
>>>
>>> -- 
>>> 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
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] iio: Documentation: Add max961x sysfs documentation

[jacopo]

Hi Jonathan,

On Sat, Feb 25, 2017 at 03:54:45PM +0000, Jonathan Cameron wrote:
> On 24/02/17 15:05, Jacopo Mondi wrote:
> > Add documentation for max961x driver.
> > The only attribute to document is the current sense shunt resistor
> > value.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
> Interestingly generic as it only applies to some of the channels.
> We might need to have the channel where it is effecting the output specified in the
> name, even if we end up with several repeats of the same thing for different
> computed channels.
> 

Sorry, I'm getting on this a bit late...

Do you still think we should have an attribute for each of the
channels this values affects?
We will end up having 2 distinct processed channels (power and
current) that depends on this value.
I can expose them as 2 distinct attributes if you think this is the
case...

A note on the series in general: given the lack of comments, I will
send v2 exposing two processed channels, with no buffered reads, and
gain selection performed in kernel space.
If we will have to add buffered reads to accomodate higher sampling
frequency we'll do that later on eventually.

Thanks
  j

> Jonathan
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio-adc-max961x | 5 +++++
> >  1 file changed, 5 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> > new file mode 100644
> > index 0000000..dbd5e75
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> > @@ -0,0 +1,5 @@
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
> > +Date:		February 2017
> > +KernelVersion:	4.10
> > +Contact:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > +Description: 	The value of the shunt resistor in micro Ohms.
> > 
> 

Re: [PATCH 2/4] iio: Documentation: Add max961x sysfs documentation

[jacopo]

Hi Jonathan,

On Sat, Feb 25, 2017 at 03:54:45PM +0000, Jonathan Cameron wrote:
> On 24/02/17 15:05, Jacopo Mondi wrote:
> > Add documentation for max961x driver.
> > The only attribute to document is the current sense shunt resistor
> > value.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Interestingly generic as it only applies to some of the channels.
> We might need to have the channel where it is effecting the output specified in the
> name, even if we end up with several repeats of the same thing for different
> computed channels.
> 

Sorry, I'm getting on this a bit late...

Do you still think we should have an attribute for each of the
channels this values affects?
We will end up having 2 distinct processed channels (power and
current) that depends on this value.
I can expose them as 2 distinct attributes if you think this is the
case...

A note on the series in general: given the lack of comments, I will
send v2 exposing two processed channels, with no buffered reads, and
gain selection performed in kernel space.
If we will have to add buffered reads to accomodate higher sampling
frequency we'll do that later on eventually.

Thanks
  j

> Jonathan
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio-adc-max961x | 5 +++++
> >  1 file changed, 5 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> > new file mode 100644
> > index 0000000..dbd5e75
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
> > @@ -0,0 +1,5 @@
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
> > +Date:		February 2017
> > +KernelVersion:	4.10
> > +Contact:	linux-iio@vger.kernel.org
> > +Description: 	The value of the shunt resistor in micro Ohms.
> > 
> 

Re: [PATCH 2/4] iio: Documentation: Add max961x sysfs documentation

[Jonathan Cameron]

On 21/03/17 16:09, jacopo wrote:
> Hi Jonathan,
> 
> On Sat, Feb 25, 2017 at 03:54:45PM +0000, Jonathan Cameron wrote:
>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>> Add documentation for max961x driver.
>>> The only attribute to document is the current sense shunt resistor
>>> value.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
>> Interestingly generic as it only applies to some of the channels.
>> We might need to have the channel where it is effecting the output specified in the
>> name, even if we end up with several repeats of the same thing for different
>> computed channels.
>>
> 
> Sorry, I'm getting on this a bit late...
> 
> Do you still think we should have an attribute for each of the
> channels this values affects?
> We will end up having 2 distinct processed channels (power and
> current) that depends on this value.
> I can expose them as 2 distinct attributes if you think this is the
> case...
Better to have two attributes exposing the same thing than imply it
is relevant to more of channels than it is...
> 
> A note on the series in general: given the lack of comments, I will
> send v2 exposing two processed channels, with no buffered reads, and
> gain selection performed in kernel space.
> If we will have to add buffered reads to accomodate higher sampling
> frequency we'll do that later on eventually.
Great.

Jonathan
> 
> Thanks
>   j
> 
>> Jonathan
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-iio-adc-max961x | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>> new file mode 100644
>>> index 0000000..dbd5e75
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>> @@ -0,0 +1,5 @@
>>> +What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
>>> +Date:		February 2017
>>> +KernelVersion:	4.10
>>> +Contact:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> +Description: 	The value of the shunt resistor in micro Ohms.
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: [PATCH 2/4] iio: Documentation: Add max961x sysfs documentation

[Jonathan Cameron]

On 21/03/17 16:09, jacopo wrote:
> Hi Jonathan,
> 
> On Sat, Feb 25, 2017 at 03:54:45PM +0000, Jonathan Cameron wrote:
>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>> Add documentation for max961x driver.
>>> The only attribute to document is the current sense shunt resistor
>>> value.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Interestingly generic as it only applies to some of the channels.
>> We might need to have the channel where it is effecting the output specified in the
>> name, even if we end up with several repeats of the same thing for different
>> computed channels.
>>
> 
> Sorry, I'm getting on this a bit late...
> 
> Do you still think we should have an attribute for each of the
> channels this values affects?
> We will end up having 2 distinct processed channels (power and
> current) that depends on this value.
> I can expose them as 2 distinct attributes if you think this is the
> case...
Better to have two attributes exposing the same thing than imply it
is relevant to more of channels than it is...
> 
> A note on the series in general: given the lack of comments, I will
> send v2 exposing two processed channels, with no buffered reads, and
> gain selection performed in kernel space.
> If we will have to add buffered reads to accomodate higher sampling
> frequency we'll do that later on eventually.
Great.

Jonathan
> 
> Thanks
>   j
> 
>> Jonathan
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-iio-adc-max961x | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>> new file mode 100644
>>> index 0000000..dbd5e75
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>> @@ -0,0 +1,5 @@
>>> +What:		/sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
>>> +Date:		February 2017
>>> +KernelVersion:	4.10
>>> +Contact:	linux-iio@vger.kernel.org
>>> +Description: 	The value of the shunt resistor in micro Ohms.
>>>
>>
> --
> 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
>