[PATCH 0/4] Make max expected current configurable for ina2xx drivers

[PATCH 0/4] Make max expected current configurable for ina2xx drivers

[Maciej Purski]

Hi all,

this patchset makes it possible to calibrate ina2xx drivers with different
calibration values, which are calculated using max expected current value.
It can be read from device tree, platform data or changed during run-time
using sysfs. If it isn't specified in device tree or platform data, the
driver uses default value, thanks to which the behaviour of the driver
don't change in this case.

There are two drivers for ina2xx: hwmon and iio. Changes are made
for both of them and their bindings as well.

These changes allow setting sensor's precision to the required by the board.
It is useful in Odroid XU3. Therefore this patchset also sets
max-expected-current in OdroidXU3 device tree to values from documentation.

Best Regards,

	Maciej Purski

Maciej Purski (4):
  iio: adc: ina2xx: Make max expected current configurable
  hwmon: (ina2xx) Make max expected current configurable
  dt-bindings: hwmon: Add max-expected-current property to ina2xx
  ARM: dts: Add max-expected-current properties for ina231 in Odroid XU3

 Documentation/devicetree/bindings/hwmon/ina2xx.txt |   4 +-
 Documentation/hwmon/ina2xx                         |   9 +-
 arch/arm/boot/dts/exynos5422-odroidxu3.dts         |   4 +
 drivers/hwmon/ina2xx.c                             | 105 +++++++++++++++++---
 drivers/iio/adc/ina2xx-adc.c                       | 110 ++++++++++++++++++---
 include/linux/platform_data/ina2xx.h               |   2 +
 6 files changed, 203 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Maciej Purski]

Max expected current is used for calculating calibration register value,
Current LSB and Power LSB according to equations found in ina datasheet.
Max expected current is now implicitly set to default value,
which is 2^15, thanks to which Current LSB is equal to 1 mA and
Power LSB is equal to 20000 uW or 25000 uW depending on ina model.

Make max expected current configurable, just like it's already done
with shunt resistance: from device tree, platform_data or later
from sysfs. On each max_expected_current change, calculate new values
for Current LSB and Power LSB. According to datasheet Current LSB should
be calculated by dividing max expected current by 2^15, as values read
from device registers are in this case 16-bit integers. Power LSB
is calculated by multiplying Current LSB by a factor, which is defined
in ina documentation.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 drivers/iio/adc/ina2xx-adc.c         | 110 ++++++++++++++++++++++++++++++-----
 include/linux/platform_data/ina2xx.h |   2 +
 2 files changed, 98 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index f387b97..883fede 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -56,6 +56,7 @@
 #define INA226_DEFAULT_IT		1110
 
 #define INA2XX_RSHUNT_DEFAULT           10000
+#define INA2XX_MAX_EXPECTED_A_DEFAULT	(1 << 15)	/* current_lsb = 1 mA */
 
 /*
  * bit masks for reading the settings in the configuration register
@@ -114,7 +115,7 @@ struct ina2xx_config {
 	int shunt_div;
 	int bus_voltage_shift;
 	int bus_voltage_lsb;	/* uV */
-	int power_lsb;		/* uW */
+	int power_lsb_factor;
 	enum ina2xx_ids chip_id;
 };
 
@@ -123,7 +124,10 @@ struct ina2xx_chip_info {
 	struct task_struct *task;
 	const struct ina2xx_config *config;
 	struct mutex state_lock;
-	unsigned int shunt_resistor;
+	unsigned int shunt_resistor;		/* uOhms */
+	unsigned int max_expected_current;	/* mA */
+	int current_lsb;			/* uA */
+	int power_lsb;				/* uW */
 	int avg;
 	int int_time_vbus; /* Bus voltage integration time uS */
 	int int_time_vshunt; /* Shunt voltage integration time uS */
@@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.shunt_div = 100,
 		.bus_voltage_shift = 3,
 		.bus_voltage_lsb = 4000,
-		.power_lsb = 20000,
+		.power_lsb_factor = 20,
 		.chip_id = ina219,
 	},
 	[ina226] = {
@@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.shunt_div = 400,
 		.bus_voltage_shift = 0,
 		.bus_voltage_lsb = 1250,
-		.power_lsb = 25000,
+		.power_lsb_factor = 25,
 		.chip_id = ina226,
 	},
 };
@@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
 
 		case INA2XX_POWER:
 			/* processed (mW) = raw*lsb (uW) / 1000 */
-			*val = chip->config->power_lsb;
+			*val = chip->power_lsb;
 			*val2 = 1000;
 			return IIO_VAL_FRACTIONAL;
 
 		case INA2XX_CURRENT:
-			/* processed (mA) = raw (mA) */
-			*val = 1;
-			return IIO_VAL_INT;
+			/* processed (mA) = raw*lsb (uA) / 1000 */
+			*val = chip->current_lsb;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
 		}
 	}
 
@@ -434,24 +439,47 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
 }
 
 /*
- * Set current LSB to 1mA, shunt is in uOhms
- * (equation 13 in datasheet). We hardcode a Current_LSB
- * of 1.0 x10-6. The only remaining parameter is RShunt.
+ * Calculate calibration value according to equation 1 in ina226 datasheet
+ * http://www.ti.com/lit/ds/symlink/ina226.pdf.
+ * Current LSB is in uA and RShunt is in uOhms, so RShunt should be
+ * converted to mOhms in order to keep the scale.
  * There is no need to expose the CALIBRATION register
  * to the user for now. But we need to reset this register
- * if the user updates RShunt after driver init, e.g upon
- * reading an EEPROM/Probe-type value.
+ * if the user updates RShunt or max expected current after driver
+ * init, e.g upon reading an EEPROM/Probe-type value.
  */
 static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
 {
+	unsigned int rshunt = DIV_ROUND_CLOSEST(chip->shunt_resistor, 1000);
 	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
-				   chip->shunt_resistor);
+				     chip->current_lsb * rshunt);
 
 	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
 }
 
+/*
+ * Set max_expected_current (mA) and calculate current_lsb (uA),
+ * according to equation 2 in ina226 datasheet. Power LSB is calculated
+ * by multiplying Current LSB by a given factor, which may vary depending
+ * on ina version.
+ */
+static int set_max_expected_current(struct ina2xx_chip_info *chip,
+				    unsigned int val)
+{
+	if (val <= 0 || val > chip->config->calibration_factor)
+		return -EINVAL;
+
+	chip->max_expected_current = val;
+	chip->current_lsb = DIV_ROUND_CLOSEST(chip->max_expected_current * 1000,
+					      1 << 15);
+	chip->power_lsb = chip->current_lsb * chip->config->power_lsb_factor;
+
+	return 0;
+}
+
 static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
 {
+
 	if (val <= 0 || val > chip->config->calibration_factor)
 		return -EINVAL;
 
@@ -493,6 +521,39 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
 	return len;
 }
 
+static ssize_t ina2xx_max_expected_current_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%d\n", chip->max_expected_current);
+}
+
+static ssize_t ina2xx_max_expected_current_store(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t len)
+{
+	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul((const char *) buf, 10, &val);
+	if (ret)
+		return ret;
+
+	ret = set_max_expected_current(chip, val);
+	if (ret)
+		return ret;
+
+	/* Update the Calibration register */
+	ret = ina2xx_set_calibration(chip);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
 #define INA219_CHAN(_type, _index, _address) { \
 	.type = (_type), \
 	.address = (_address), \
@@ -755,10 +816,15 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
 		       ina2xx_shunt_resistor_show,
 		       ina2xx_shunt_resistor_store, 0);
 
+static IIO_DEVICE_ATTR(in_max_expected_current, S_IRUGO | S_IWUSR,
+		       ina2xx_max_expected_current_show,
+		       ina2xx_max_expected_current_store, 0);
+
 static struct attribute *ina219_attributes[] = {
 	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
 	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
 	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
+	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
 	NULL,
 };
 
@@ -766,6 +832,7 @@ static struct attribute *ina226_attributes[] = {
 	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
 	&iio_const_attr_ina226_integration_time_available.dev_attr.attr,
 	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
+	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
 	NULL,
 };
 
@@ -851,6 +918,21 @@ static int ina2xx_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	if (of_property_read_u32(client->dev.of_node,
+				  "max-expected-current", &val) < 0) {
+		struct ina2xx_platform_data *pdata =
+		    dev_get_platdata(&client->dev);
+
+		if (pdata && pdata->max_mA != 0)
+			val = pdata->max_mA;
+		else
+			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
+	}
+
+	ret = set_max_expected_current(chip, val);
+	if (ret)
+		return ret;
+
 	/* Patch the current config register with default. */
 	val = chip->config->config_default;
 
diff --git a/include/linux/platform_data/ina2xx.h b/include/linux/platform_data/ina2xx.h
index 9abc0ca..f02b1d8 100644
--- a/include/linux/platform_data/ina2xx.h
+++ b/include/linux/platform_data/ina2xx.h
@@ -13,7 +13,9 @@
 /**
  * struct ina2xx_platform_data - ina2xx info
  * @shunt_uohms		shunt resistance in microohms
+ * @max_mA		max expected current in mA
  */
 struct ina2xx_platform_data {
 	long shunt_uohms;
+	int max_mA;
 };
-- 
2.7.4

Re: [PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Jonathan Cameron]

On Thu, 28 Sep 2017 14:50:12 +0200
Maciej Purski <m.purski@samsung.com> wrote:

> Max expected current is used for calculating calibration register value,
> Current LSB and Power LSB according to equations found in ina datasheet.
> Max expected current is now implicitly set to default value,
> which is 2^15, thanks to which Current LSB is equal to 1 mA and
> Power LSB is equal to 20000 uW or 25000 uW depending on ina model.
> 
> Make max expected current configurable, just like it's already done
> with shunt resistance: from device tree, platform_data or later
> from sysfs. On each max_expected_current change, calculate new values
> for Current LSB and Power LSB. According to datasheet Current LSB should
> be calculated by dividing max expected current by 2^15, as values read
> from device registers are in this case 16-bit integers. Power LSB
> is calculated by multiplying Current LSB by a factor, which is defined
> in ina documentation.

One odd bit of casting inline.  Also this is new userspace ABI.
It needs documenting in 

Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
I'm also unclear on one element about this - is it a value used only
for calibration or are we talking about the actual 'range' of the device?

The interpretation of this value isn't clear against the more general
ABI.

In particular it is it in raw units (adc counts) or mA?  Docs say
that but the naming of the attribute doesn't make this clear.

Also I'm unconvinced this isn't better represented using the
range specifications available for any IIO attribute on the raw
value in combination with adjusting the scale value.
Note not many drivers yet provide ranges on their raw outputs
but we do have core support for it.  I've been meaning to start
pushing this out into drivers, but been busy since we introduced
the core support.  The dpot-dac driver does use it for examplel

This moves the burden of calculating the 1lsb value to userspace,
but importantly it would give us a consistent ABI where this fits
in with existing elements (largely buy not introducing any new
ones :).

Thanks,

Jonathan
> 
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
>  drivers/iio/adc/ina2xx-adc.c         | 110 ++++++++++++++++++++++++++++++-----
>  include/linux/platform_data/ina2xx.h |   2 +
>  2 files changed, 98 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index f387b97..883fede 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -56,6 +56,7 @@
>  #define INA226_DEFAULT_IT		1110
>  
>  #define INA2XX_RSHUNT_DEFAULT           10000
> +#define INA2XX_MAX_EXPECTED_A_DEFAULT	(1 << 15)	/* current_lsb = 1 mA */
>  
>  /*
>   * bit masks for reading the settings in the configuration register
> @@ -114,7 +115,7 @@ struct ina2xx_config {
>  	int shunt_div;
>  	int bus_voltage_shift;
>  	int bus_voltage_lsb;	/* uV */
> -	int power_lsb;		/* uW */
> +	int power_lsb_factor;
>  	enum ina2xx_ids chip_id;
>  };
>  
> @@ -123,7 +124,10 @@ struct ina2xx_chip_info {
>  	struct task_struct *task;
>  	const struct ina2xx_config *config;
>  	struct mutex state_lock;
> -	unsigned int shunt_resistor;
> +	unsigned int shunt_resistor;		/* uOhms */
> +	unsigned int max_expected_current;	/* mA */
> +	int current_lsb;			/* uA */
> +	int power_lsb;				/* uW */
>  	int avg;
>  	int int_time_vbus; /* Bus voltage integration time uS */
>  	int int_time_vshunt; /* Shunt voltage integration time uS */
> @@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>  		.shunt_div = 100,
>  		.bus_voltage_shift = 3,
>  		.bus_voltage_lsb = 4000,
> -		.power_lsb = 20000,
> +		.power_lsb_factor = 20,
>  		.chip_id = ina219,
>  	},
>  	[ina226] = {
> @@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>  		.shunt_div = 400,
>  		.bus_voltage_shift = 0,
>  		.bus_voltage_lsb = 1250,
> -		.power_lsb = 25000,
> +		.power_lsb_factor = 25,
>  		.chip_id = ina226,
>  	},
>  };
> @@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>  
>  		case INA2XX_POWER:
>  			/* processed (mW) = raw*lsb (uW) / 1000 */
> -			*val = chip->config->power_lsb;
> +			*val = chip->power_lsb;
>  			*val2 = 1000;
>  			return IIO_VAL_FRACTIONAL;
>  
>  		case INA2XX_CURRENT:
> -			/* processed (mA) = raw (mA) */
> -			*val = 1;
> -			return IIO_VAL_INT;
> +			/* processed (mA) = raw*lsb (uA) / 1000 */
> +			*val = chip->current_lsb;
> +			*val2 = 1000;
> +			return IIO_VAL_FRACTIONAL;
>  		}
>  	}
>  
> @@ -434,24 +439,47 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
>  }
>  
>  /*
> - * Set current LSB to 1mA, shunt is in uOhms
> - * (equation 13 in datasheet). We hardcode a Current_LSB
> - * of 1.0 x10-6. The only remaining parameter is RShunt.
> + * Calculate calibration value according to equation 1 in ina226 datasheet
> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> + * Current LSB is in uA and RShunt is in uOhms, so RShunt should be
> + * converted to mOhms in order to keep the scale.
>   * There is no need to expose the CALIBRATION register
>   * to the user for now. But we need to reset this register
> - * if the user updates RShunt after driver init, e.g upon
> - * reading an EEPROM/Probe-type value.
> + * if the user updates RShunt or max expected current after driver
> + * init, e.g upon reading an EEPROM/Probe-type value.
>   */
>  static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
>  {
> +	unsigned int rshunt = DIV_ROUND_CLOSEST(chip->shunt_resistor, 1000);
>  	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> -				   chip->shunt_resistor);
> +				     chip->current_lsb * rshunt);
>  
>  	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>  }
>  
> +/*
> + * Set max_expected_current (mA) and calculate current_lsb (uA),
> + * according to equation 2 in ina226 datasheet. Power LSB is calculated
> + * by multiplying Current LSB by a given factor, which may vary depending
> + * on ina version.
> + */
> +static int set_max_expected_current(struct ina2xx_chip_info *chip,
> +				    unsigned int val)
> +{
> +	if (val <= 0 || val > chip->config->calibration_factor)
> +		return -EINVAL;
> +
> +	chip->max_expected_current = val;
> +	chip->current_lsb = DIV_ROUND_CLOSEST(chip->max_expected_current * 1000,
> +					      1 << 15);
> +	chip->power_lsb = chip->current_lsb * chip->config->power_lsb_factor;
> +
> +	return 0;
> +}
> +
>  static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
>  {
> +
>  	if (val <= 0 || val > chip->config->calibration_factor)
>  		return -EINVAL;
>  
> @@ -493,6 +521,39 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>  	return len;
>  }
>  
> +static ssize_t ina2xx_max_expected_current_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", chip->max_expected_current);
> +}
> +
> +static ssize_t ina2xx_max_expected_current_store(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t len)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul((const char *) buf, 10, &val);

Odd bit of casting given that's what it already is...

> +	if (ret)
> +		return ret;
> +
> +	ret = set_max_expected_current(chip, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Update the Calibration register */
> +	ret = ina2xx_set_calibration(chip);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
>  #define INA219_CHAN(_type, _index, _address) { \
>  	.type = (_type), \
>  	.address = (_address), \
> @@ -755,10 +816,15 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
>  		       ina2xx_shunt_resistor_show,
>  		       ina2xx_shunt_resistor_store, 0);
>  
> +static IIO_DEVICE_ATTR(in_max_expected_current, S_IRUGO | S_IWUSR,
> +		       ina2xx_max_expected_current_show,
> +		       ina2xx_max_expected_current_store, 0);
> +
>  static struct attribute *ina219_attributes[] = {
>  	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>  	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
>  	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
>  	NULL,
>  };
>  
> @@ -766,6 +832,7 @@ static struct attribute *ina226_attributes[] = {
>  	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>  	&iio_const_attr_ina226_integration_time_available.dev_attr.attr,
>  	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
>  	NULL,
>  };
>  
> @@ -851,6 +918,21 @@ static int ina2xx_probe(struct i2c_client *client,
>  	if (ret)
>  		return ret;
>  
> +	if (of_property_read_u32(client->dev.of_node,
> +				  "max-expected-current", &val) < 0) {
> +		struct ina2xx_platform_data *pdata =
> +		    dev_get_platdata(&client->dev);
> +
> +		if (pdata && pdata->max_mA != 0)
> +			val = pdata->max_mA;
> +		else
> +			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
> +	}
> +
> +	ret = set_max_expected_current(chip, val);
> +	if (ret)
> +		return ret;
> +
>  	/* Patch the current config register with default. */
>  	val = chip->config->config_default;
>  
> diff --git a/include/linux/platform_data/ina2xx.h b/include/linux/platform_data/ina2xx.h
> index 9abc0ca..f02b1d8 100644
> --- a/include/linux/platform_data/ina2xx.h
> +++ b/include/linux/platform_data/ina2xx.h
> @@ -13,7 +13,9 @@
>  /**
>   * struct ina2xx_platform_data - ina2xx info
>   * @shunt_uohms		shunt resistance in microohms
> + * @max_mA		max expected current in mA
>   */
>  struct ina2xx_platform_data {
>  	long shunt_uohms;
> +	int max_mA;
>  };

Re: [PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Maciej Purski]

On 10/01/2017 12:29 PM, Jonathan Cameron wrote:
> On Thu, 28 Sep 2017 14:50:12 +0200
> Maciej Purski <m.purski@samsung.com> wrote:
> 
>> Max expected current is used for calculating calibration register value,
>> Current LSB and Power LSB according to equations found in ina datasheet.
>> Max expected current is now implicitly set to default value,
>> which is 2^15, thanks to which Current LSB is equal to 1 mA and
>> Power LSB is equal to 20000 uW or 25000 uW depending on ina model.
>>
>> Make max expected current configurable, just like it's already done
>> with shunt resistance: from device tree, platform_data or later
>> from sysfs. On each max_expected_current change, calculate new values
>> for Current LSB and Power LSB. According to datasheet Current LSB should
>> be calculated by dividing max expected current by 2^15, as values read
>> from device registers are in this case 16-bit integers. Power LSB
>> is calculated by multiplying Current LSB by a factor, which is defined
>> in ina documentation.
> 
> One odd bit of casting inline.  Also this is new userspace ABI.
> It needs documenting in
> 
> Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
> I'm also unclear on one element about this - is it a value used only
> for calibration or are we talking about the actual 'range' of the device?
> 

This is used for calibration. The device measures directly only voltage.
However, it has also current and power registers. Their values are
calculated by the device using the calibration register which is calculated
using max expected current. So I guess that it's not what you mean
by the actual 'range' of the device.

> The interpretation of this value isn't clear against the more general
> ABI.
> 
> In particular it is it in raw units (adc counts) or mA?  Docs say
> that but the naming of the attribute doesn't make this clear.
> 

It's in mA. I can make it clear in the attribute name.

> Also I'm unconvinced this isn't better represented using the
> range specifications available for any IIO attribute on the raw
> value in combination with adjusting the scale value.
> Note not many drivers yet provide ranges on their raw outputs
> but we do have core support for it.  I've been meaning to start
> pushing this out into drivers, but been busy since we introduced
> the core support.  The dpot-dac driver does use it for examplel
>


I'm not sure if what I'm about to add is similar to what is done
in the mentioned dpot-dac driver. It seems that the callback read_avail
returns information on raw values which can be obtained from the device.
What I need is an adjustable value, which is then used by the device internally
in order to calculate current with the requested precision. Max expected current
is also used for calculating the scale value.
Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO and
your solution fits in here.

Best regards,

	Maciej
> This moves the burden of calculating the 1lsb value to userspace,
> but importantly it would give us a consistent ABI where this fits
> in with existing elements (largely buy not introducing any new
> ones :).
> 
> Thanks,
> 
> Jonathan
>>
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>> ---
>>   drivers/iio/adc/ina2xx-adc.c         | 110 ++++++++++++++++++++++++++++++-----
>>   include/linux/platform_data/ina2xx.h |   2 +
>>   2 files changed, 98 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>> index f387b97..883fede 100644
>> --- a/drivers/iio/adc/ina2xx-adc.c
>> +++ b/drivers/iio/adc/ina2xx-adc.c
>> @@ -56,6 +56,7 @@
>>   #define INA226_DEFAULT_IT		1110
>>   
>>   #define INA2XX_RSHUNT_DEFAULT           10000
>> +#define INA2XX_MAX_EXPECTED_A_DEFAULT	(1 << 15)	/* current_lsb = 1 mA */
>>   
>>   /*
>>    * bit masks for reading the settings in the configuration register
>> @@ -114,7 +115,7 @@ struct ina2xx_config {
>>   	int shunt_div;
>>   	int bus_voltage_shift;
>>   	int bus_voltage_lsb;	/* uV */
>> -	int power_lsb;		/* uW */
>> +	int power_lsb_factor;
>>   	enum ina2xx_ids chip_id;
>>   };
>>   
>> @@ -123,7 +124,10 @@ struct ina2xx_chip_info {
>>   	struct task_struct *task;
>>   	const struct ina2xx_config *config;
>>   	struct mutex state_lock;
>> -	unsigned int shunt_resistor;
>> +	unsigned int shunt_resistor;		/* uOhms */
>> +	unsigned int max_expected_current;	/* mA */
>> +	int current_lsb;			/* uA */
>> +	int power_lsb;				/* uW */
>>   	int avg;
>>   	int int_time_vbus; /* Bus voltage integration time uS */
>>   	int int_time_vshunt; /* Shunt voltage integration time uS */
>> @@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>   		.shunt_div = 100,
>>   		.bus_voltage_shift = 3,
>>   		.bus_voltage_lsb = 4000,
>> -		.power_lsb = 20000,
>> +		.power_lsb_factor = 20,
>>   		.chip_id = ina219,
>>   	},
>>   	[ina226] = {
>> @@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>   		.shunt_div = 400,
>>   		.bus_voltage_shift = 0,
>>   		.bus_voltage_lsb = 1250,
>> -		.power_lsb = 25000,
>> +		.power_lsb_factor = 25,
>>   		.chip_id = ina226,
>>   	},
>>   };
>> @@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>   
>>   		case INA2XX_POWER:
>>   			/* processed (mW) = raw*lsb (uW) / 1000 */
>> -			*val = chip->config->power_lsb;
>> +			*val = chip->power_lsb;
>>   			*val2 = 1000;
>>   			return IIO_VAL_FRACTIONAL;
>>   
>>   		case INA2XX_CURRENT:
>> -			/* processed (mA) = raw (mA) */
>> -			*val = 1;
>> -			return IIO_VAL_INT;
>> +			/* processed (mA) = raw*lsb (uA) / 1000 */
>> +			*val = chip->current_lsb;
>> +			*val2 = 1000;
>> +			return IIO_VAL_FRACTIONAL;
>>   		}
>>   	}
>>   
>> @@ -434,24 +439,47 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
>>   }
>>   
>>   /*
>> - * Set current LSB to 1mA, shunt is in uOhms
>> - * (equation 13 in datasheet). We hardcode a Current_LSB
>> - * of 1.0 x10-6. The only remaining parameter is RShunt.
>> + * Calculate calibration value according to equation 1 in ina226 datasheet
>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
>> + * Current LSB is in uA and RShunt is in uOhms, so RShunt should be
>> + * converted to mOhms in order to keep the scale.
>>    * There is no need to expose the CALIBRATION register
>>    * to the user for now. But we need to reset this register
>> - * if the user updates RShunt after driver init, e.g upon
>> - * reading an EEPROM/Probe-type value.
>> + * if the user updates RShunt or max expected current after driver
>> + * init, e.g upon reading an EEPROM/Probe-type value.
>>    */
>>   static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
>>   {
>> +	unsigned int rshunt = DIV_ROUND_CLOSEST(chip->shunt_resistor, 1000);
>>   	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
>> -				   chip->shunt_resistor);
>> +				     chip->current_lsb * rshunt);
>>   
>>   	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>>   }
>>   
>> +/*
>> + * Set max_expected_current (mA) and calculate current_lsb (uA),
>> + * according to equation 2 in ina226 datasheet. Power LSB is calculated
>> + * by multiplying Current LSB by a given factor, which may vary depending
>> + * on ina version.
>> + */
>> +static int set_max_expected_current(struct ina2xx_chip_info *chip,
>> +				    unsigned int val)
>> +{
>> +	if (val <= 0 || val > chip->config->calibration_factor)
>> +		return -EINVAL;
>> +
>> +	chip->max_expected_current = val;
>> +	chip->current_lsb = DIV_ROUND_CLOSEST(chip->max_expected_current * 1000,
>> +					      1 << 15);
>> +	chip->power_lsb = chip->current_lsb * chip->config->power_lsb_factor;
>> +
>> +	return 0;
>> +}
>> +
>>   static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
>>   {
>> +
>>   	if (val <= 0 || val > chip->config->calibration_factor)
>>   		return -EINVAL;
>>   
>> @@ -493,6 +521,39 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>>   	return len;
>>   }
>>   
>> +static ssize_t ina2xx_max_expected_current_show(struct device *dev,
>> +					  struct device_attribute *attr,
>> +					  char *buf)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
>> +
>> +	return sprintf(buf, "%d\n", chip->max_expected_current);
>> +}
>> +
>> +static ssize_t ina2xx_max_expected_current_store(struct device *dev,
>> +					   struct device_attribute *attr,
>> +					   const char *buf, size_t len)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
>> +	unsigned long val;
>> +	int ret;
>> +
>> +	ret = kstrtoul((const char *) buf, 10, &val);
> 
> Odd bit of casting given that's what it already is...
> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = set_max_expected_current(chip, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Update the Calibration register */
>> +	ret = ina2xx_set_calibration(chip);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return len;
>> +}
>> +
>>   #define INA219_CHAN(_type, _index, _address) { \
>>   	.type = (_type), \
>>   	.address = (_address), \
>> @@ -755,10 +816,15 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
>>   		       ina2xx_shunt_resistor_show,
>>   		       ina2xx_shunt_resistor_store, 0);
>>   
>> +static IIO_DEVICE_ATTR(in_max_expected_current, S_IRUGO | S_IWUSR,
>> +		       ina2xx_max_expected_current_show,
>> +		       ina2xx_max_expected_current_store, 0);
>> +
>>   static struct attribute *ina219_attributes[] = {
>>   	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>>   	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
>>   	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
>>   	NULL,
>>   };
>>   
>> @@ -766,6 +832,7 @@ static struct attribute *ina226_attributes[] = {
>>   	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>>   	&iio_const_attr_ina226_integration_time_available.dev_attr.attr,
>>   	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
>>   	NULL,
>>   };
>>   
>> @@ -851,6 +918,21 @@ static int ina2xx_probe(struct i2c_client *client,
>>   	if (ret)
>>   		return ret;
>>   
>> +	if (of_property_read_u32(client->dev.of_node,
>> +				  "max-expected-current", &val) < 0) {
>> +		struct ina2xx_platform_data *pdata =
>> +		    dev_get_platdata(&client->dev);
>> +
>> +		if (pdata && pdata->max_mA != 0)
>> +			val = pdata->max_mA;
>> +		else
>> +			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
>> +	}
>> +
>> +	ret = set_max_expected_current(chip, val);
>> +	if (ret)
>> +		return ret;
>> +
>>   	/* Patch the current config register with default. */
>>   	val = chip->config->config_default;
>>   
>> diff --git a/include/linux/platform_data/ina2xx.h b/include/linux/platform_data/ina2xx.h
>> index 9abc0ca..f02b1d8 100644
>> --- a/include/linux/platform_data/ina2xx.h
>> +++ b/include/linux/platform_data/ina2xx.h
>> @@ -13,7 +13,9 @@
>>   /**
>>    * struct ina2xx_platform_data - ina2xx info
>>    * @shunt_uohms		shunt resistance in microohms
>> + * @max_mA		max expected current in mA
>>    */
>>   struct ina2xx_platform_data {
>>   	long shunt_uohms;
>> +	int max_mA;
>>   };
> 
> 
> 
> 

Re: [PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Jonathan Cameron]

On Wed, 04 Oct 2017 09:11:31 +0200
Maciej Purski <m.purski@samsung.com> wrote:

> On 10/01/2017 12:29 PM, Jonathan Cameron wrote:
> > On Thu, 28 Sep 2017 14:50:12 +0200
> > Maciej Purski <m.purski@samsung.com> wrote:
> >   
> >> Max expected current is used for calculating calibration register value,
> >> Current LSB and Power LSB according to equations found in ina datasheet.
> >> Max expected current is now implicitly set to default value,
> >> which is 2^15, thanks to which Current LSB is equal to 1 mA and
> >> Power LSB is equal to 20000 uW or 25000 uW depending on ina model.
> >>
> >> Make max expected current configurable, just like it's already done
> >> with shunt resistance: from device tree, platform_data or later
> >> from sysfs. On each max_expected_current change, calculate new values
> >> for Current LSB and Power LSB. According to datasheet Current LSB should
> >> be calculated by dividing max expected current by 2^15, as values read
> >> from device registers are in this case 16-bit integers. Power LSB
> >> is calculated by multiplying Current LSB by a factor, which is defined
> >> in ina documentation.  
> > 
> > One odd bit of casting inline.  Also this is new userspace ABI.
> > It needs documenting in
> > 
> > Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
> > I'm also unclear on one element about this - is it a value used only
> > for calibration or are we talking about the actual 'range' of the device?
> >   
> 
> This is used for calibration. The device measures directly only voltage.
> However, it has also current and power registers. Their values are
> calculated by the device using the calibration register which is calculated
> using max expected current. So I guess that it's not what you mean
> by the actual 'range' of the device.
> 
> > The interpretation of this value isn't clear against the more general
> > ABI.
> > 
> > In particular it is it in raw units (adc counts) or mA?  Docs say
> > that but the naming of the attribute doesn't make this clear.
> >   
> 
> It's in mA. I can make it clear in the attribute name.
> 
> > Also I'm unconvinced this isn't better represented using the
> > range specifications available for any IIO attribute on the raw
> > value in combination with adjusting the scale value.
> > Note not many drivers yet provide ranges on their raw outputs
> > but we do have core support for it.  I've been meaning to start
> > pushing this out into drivers, but been busy since we introduced
> > the core support.  The dpot-dac driver does use it for examplel
> >  
> 
> 
> I'm not sure if what I'm about to add is similar to what is done
> in the mentioned dpot-dac driver. It seems that the callback read_avail
> returns information on raw values which can be obtained from the device.
> What I need is an adjustable value, which is then used by the device internally
> in order to calculate current with the requested precision. Max expected current
> is also used for calculating the scale value.
> Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO and
> your solution fits in here.
> 

I think I answered this in the other branch of the thread.
_calibscale is what you want here as it's internal to the device.

It's not one often used for ADCs but quite a few other types of
device provide some front end analog adjustment (whilst it is digital
here, it is applied within the device - so we don't need to care).

Jonathan

> Best regards,
> 
> 	Maciej
> > This moves the burden of calculating the 1lsb value to userspace,
> > but importantly it would give us a consistent ABI where this fits
> > in with existing elements (largely buy not introducing any new
> > ones :).
> > 
> > Thanks,
> > 
> > Jonathan  
> >>
> >> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> >> ---
> >>   drivers/iio/adc/ina2xx-adc.c         | 110 ++++++++++++++++++++++++++++++-----
> >>   include/linux/platform_data/ina2xx.h |   2 +
> >>   2 files changed, 98 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> >> index f387b97..883fede 100644
> >> --- a/drivers/iio/adc/ina2xx-adc.c
> >> +++ b/drivers/iio/adc/ina2xx-adc.c
> >> @@ -56,6 +56,7 @@
> >>   #define INA226_DEFAULT_IT		1110
> >>   
> >>   #define INA2XX_RSHUNT_DEFAULT           10000
> >> +#define INA2XX_MAX_EXPECTED_A_DEFAULT	(1 << 15)	/* current_lsb = 1 mA */
> >>   
> >>   /*
> >>    * bit masks for reading the settings in the configuration register
> >> @@ -114,7 +115,7 @@ struct ina2xx_config {
> >>   	int shunt_div;
> >>   	int bus_voltage_shift;
> >>   	int bus_voltage_lsb;	/* uV */
> >> -	int power_lsb;		/* uW */
> >> +	int power_lsb_factor;
> >>   	enum ina2xx_ids chip_id;
> >>   };
> >>   
> >> @@ -123,7 +124,10 @@ struct ina2xx_chip_info {
> >>   	struct task_struct *task;
> >>   	const struct ina2xx_config *config;
> >>   	struct mutex state_lock;
> >> -	unsigned int shunt_resistor;
> >> +	unsigned int shunt_resistor;		/* uOhms */
> >> +	unsigned int max_expected_current;	/* mA */
> >> +	int current_lsb;			/* uA */
> >> +	int power_lsb;				/* uW */
> >>   	int avg;
> >>   	int int_time_vbus; /* Bus voltage integration time uS */
> >>   	int int_time_vshunt; /* Shunt voltage integration time uS */
> >> @@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> >>   		.shunt_div = 100,
> >>   		.bus_voltage_shift = 3,
> >>   		.bus_voltage_lsb = 4000,
> >> -		.power_lsb = 20000,
> >> +		.power_lsb_factor = 20,
> >>   		.chip_id = ina219,
> >>   	},
> >>   	[ina226] = {
> >> @@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> >>   		.shunt_div = 400,
> >>   		.bus_voltage_shift = 0,
> >>   		.bus_voltage_lsb = 1250,
> >> -		.power_lsb = 25000,
> >> +		.power_lsb_factor = 25,
> >>   		.chip_id = ina226,
> >>   	},
> >>   };
> >> @@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> >>   
> >>   		case INA2XX_POWER:
> >>   			/* processed (mW) = raw*lsb (uW) / 1000 */
> >> -			*val = chip->config->power_lsb;
> >> +			*val = chip->power_lsb;
> >>   			*val2 = 1000;
> >>   			return IIO_VAL_FRACTIONAL;
> >>   
> >>   		case INA2XX_CURRENT:
> >> -			/* processed (mA) = raw (mA) */
> >> -			*val = 1;
> >> -			return IIO_VAL_INT;
> >> +			/* processed (mA) = raw*lsb (uA) / 1000 */
> >> +			*val = chip->current_lsb;
> >> +			*val2 = 1000;
> >> +			return IIO_VAL_FRACTIONAL;
> >>   		}
> >>   	}
> >>   
> >> @@ -434,24 +439,47 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
> >>   }
> >>   
> >>   /*
> >> - * Set current LSB to 1mA, shunt is in uOhms
> >> - * (equation 13 in datasheet). We hardcode a Current_LSB
> >> - * of 1.0 x10-6. The only remaining parameter is RShunt.
> >> + * Calculate calibration value according to equation 1 in ina226 datasheet
> >> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> >> + * Current LSB is in uA and RShunt is in uOhms, so RShunt should be
> >> + * converted to mOhms in order to keep the scale.
> >>    * There is no need to expose the CALIBRATION register
> >>    * to the user for now. But we need to reset this register
> >> - * if the user updates RShunt after driver init, e.g upon
> >> - * reading an EEPROM/Probe-type value.
> >> + * if the user updates RShunt or max expected current after driver
> >> + * init, e.g upon reading an EEPROM/Probe-type value.
> >>    */
> >>   static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
> >>   {
> >> +	unsigned int rshunt = DIV_ROUND_CLOSEST(chip->shunt_resistor, 1000);
> >>   	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> >> -				   chip->shunt_resistor);
> >> +				     chip->current_lsb * rshunt);
> >>   
> >>   	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> >>   }
> >>   
> >> +/*
> >> + * Set max_expected_current (mA) and calculate current_lsb (uA),
> >> + * according to equation 2 in ina226 datasheet. Power LSB is calculated
> >> + * by multiplying Current LSB by a given factor, which may vary depending
> >> + * on ina version.
> >> + */
> >> +static int set_max_expected_current(struct ina2xx_chip_info *chip,
> >> +				    unsigned int val)
> >> +{
> >> +	if (val <= 0 || val > chip->config->calibration_factor)
> >> +		return -EINVAL;
> >> +
> >> +	chip->max_expected_current = val;
> >> +	chip->current_lsb = DIV_ROUND_CLOSEST(chip->max_expected_current * 1000,
> >> +					      1 << 15);
> >> +	chip->power_lsb = chip->current_lsb * chip->config->power_lsb_factor;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
> >>   {
> >> +
> >>   	if (val <= 0 || val > chip->config->calibration_factor)
> >>   		return -EINVAL;
> >>   
> >> @@ -493,6 +521,39 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> >>   	return len;
> >>   }
> >>   
> >> +static ssize_t ina2xx_max_expected_current_show(struct device *dev,
> >> +					  struct device_attribute *attr,
> >> +					  char *buf)
> >> +{
> >> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> >> +
> >> +	return sprintf(buf, "%d\n", chip->max_expected_current);
> >> +}
> >> +
> >> +static ssize_t ina2xx_max_expected_current_store(struct device *dev,
> >> +					   struct device_attribute *attr,
> >> +					   const char *buf, size_t len)
> >> +{
> >> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> >> +	unsigned long val;
> >> +	int ret;
> >> +
> >> +	ret = kstrtoul((const char *) buf, 10, &val);  
> > 
> > Odd bit of casting given that's what it already is...
> >   
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = set_max_expected_current(chip, val);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* Update the Calibration register */
> >> +	ret = ina2xx_set_calibration(chip);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return len;
> >> +}
> >> +
> >>   #define INA219_CHAN(_type, _index, _address) { \
> >>   	.type = (_type), \
> >>   	.address = (_address), \
> >> @@ -755,10 +816,15 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
> >>   		       ina2xx_shunt_resistor_show,
> >>   		       ina2xx_shunt_resistor_store, 0);
> >>   
> >> +static IIO_DEVICE_ATTR(in_max_expected_current, S_IRUGO | S_IWUSR,
> >> +		       ina2xx_max_expected_current_show,
> >> +		       ina2xx_max_expected_current_store, 0);
> >> +
> >>   static struct attribute *ina219_attributes[] = {
> >>   	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
> >>   	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
> >>   	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> >> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
> >>   	NULL,
> >>   };
> >>   
> >> @@ -766,6 +832,7 @@ static struct attribute *ina226_attributes[] = {
> >>   	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
> >>   	&iio_const_attr_ina226_integration_time_available.dev_attr.attr,
> >>   	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> >> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
> >>   	NULL,
> >>   };
> >>   
> >> @@ -851,6 +918,21 @@ static int ina2xx_probe(struct i2c_client *client,
> >>   	if (ret)
> >>   		return ret;
> >>   
> >> +	if (of_property_read_u32(client->dev.of_node,
> >> +				  "max-expected-current", &val) < 0) {
> >> +		struct ina2xx_platform_data *pdata =
> >> +		    dev_get_platdata(&client->dev);
> >> +
> >> +		if (pdata && pdata->max_mA != 0)
> >> +			val = pdata->max_mA;
> >> +		else
> >> +			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
> >> +	}
> >> +
> >> +	ret = set_max_expected_current(chip, val);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >>   	/* Patch the current config register with default. */
> >>   	val = chip->config->config_default;
> >>   
> >> diff --git a/include/linux/platform_data/ina2xx.h b/include/linux/platform_data/ina2xx.h
> >> index 9abc0ca..f02b1d8 100644
> >> --- a/include/linux/platform_data/ina2xx.h
> >> +++ b/include/linux/platform_data/ina2xx.h
> >> @@ -13,7 +13,9 @@
> >>   /**
> >>    * struct ina2xx_platform_data - ina2xx info
> >>    * @shunt_uohms		shunt resistance in microohms
> >> + * @max_mA		max expected current in mA
> >>    */
> >>   struct ina2xx_platform_data {
> >>   	long shunt_uohms;
> >> +	int max_mA;
> >>   };  
> > 
> > 
> > 
> >   

Re: [PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Maciej Purski]

On 10/08/2017 11:47 AM, Jonathan Cameron wrote:
> On Wed, 04 Oct 2017 09:11:31 +0200
> Maciej Purski <m.purski@samsung.com> wrote:
> 
>> On 10/01/2017 12:29 PM, Jonathan Cameron wrote:
>>> On Thu, 28 Sep 2017 14:50:12 +0200
>>> Maciej Purski <m.purski@samsung.com> wrote:
>>>    
>>>> Max expected current is used for calculating calibration register value,
>>>> Current LSB and Power LSB according to equations found in ina datasheet.
>>>> Max expected current is now implicitly set to default value,
>>>> which is 2^15, thanks to which Current LSB is equal to 1 mA and
>>>> Power LSB is equal to 20000 uW or 25000 uW depending on ina model.
>>>>
>>>> Make max expected current configurable, just like it's already done
>>>> with shunt resistance: from device tree, platform_data or later
>>>> from sysfs. On each max_expected_current change, calculate new values
>>>> for Current LSB and Power LSB. According to datasheet Current LSB should
>>>> be calculated by dividing max expected current by 2^15, as values read
>>>> from device registers are in this case 16-bit integers. Power LSB
>>>> is calculated by multiplying Current LSB by a factor, which is defined
>>>> in ina documentation.
>>>
>>> One odd bit of casting inline.  Also this is new userspace ABI.
>>> It needs documenting in
>>>
>>> Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
>>> I'm also unclear on one element about this - is it a value used only
>>> for calibration or are we talking about the actual 'range' of the device?
>>>    
>>
>> This is used for calibration. The device measures directly only voltage.
>> However, it has also current and power registers. Their values are
>> calculated by the device using the calibration register which is calculated
>> using max expected current. So I guess that it's not what you mean
>> by the actual 'range' of the device.
>>
>>> The interpretation of this value isn't clear against the more general
>>> ABI.
>>>
>>> In particular it is it in raw units (adc counts) or mA?  Docs say
>>> that but the naming of the attribute doesn't make this clear.
>>>    
>>
>> It's in mA. I can make it clear in the attribute name.
>>
>>> Also I'm unconvinced this isn't better represented using the
>>> range specifications available for any IIO attribute on the raw
>>> value in combination with adjusting the scale value.
>>> Note not many drivers yet provide ranges on their raw outputs
>>> but we do have core support for it.  I've been meaning to start
>>> pushing this out into drivers, but been busy since we introduced
>>> the core support.  The dpot-dac driver does use it for examplel
>>>   
>>
>>
>> I'm not sure if what I'm about to add is similar to what is done
>> in the mentioned dpot-dac driver. It seems that the callback read_avail
>> returns information on raw values which can be obtained from the device.
>> What I need is an adjustable value, which is then used by the device internally
>> in order to calculate current with the requested precision. Max expected current
>> is also used for calculating the scale value.
>> Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO and
>> your solution fits in here.
>>
> 
> I think I answered this in the other branch of the thread.
> _calibscale is what you want here as it's internal to the device.
> 
> It's not one often used for ADCs but quite a few other types of
> device provide some front end analog adjustment (whilst it is digital
> here, it is applied within the device - so we don't need to care).
> 
> Jonathan

Thank you for your explanation. Calibscale seems suitable for me in this case,
but what do you think I should do then with SCALE attribute? Should I get rid of 
it for current and use only calibscale? Or should I use both calibscale and 
scale attributes and for current they will be the same value?

I should mention that currenst_lsb value is also used for calculating power_lsb
as they have a fixed ratio (20 or 25) given in the documentation.

Thanks,

Maciej

> 
>> Best regards,
>>
>> 	Maciej
>>> This moves the burden of calculating the 1lsb value to userspace,
>>> but importantly it would give us a consistent ABI where this fits
>>> in with existing elements (largely buy not introducing any new
>>> ones :).
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>>
>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>>> ---
>>>>    drivers/iio/adc/ina2xx-adc.c         | 110 ++++++++++++++++++++++++++++++-----
>>>>    include/linux/platform_data/ina2xx.h |   2 +
>>>>    2 files changed, 98 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>>>> index f387b97..883fede 100644
>>>> --- a/drivers/iio/adc/ina2xx-adc.c
>>>> +++ b/drivers/iio/adc/ina2xx-adc.c
>>>> @@ -56,6 +56,7 @@
>>>>    #define INA226_DEFAULT_IT		1110
>>>>    
>>>>    #define INA2XX_RSHUNT_DEFAULT           10000
>>>> +#define INA2XX_MAX_EXPECTED_A_DEFAULT	(1 << 15)	/* current_lsb = 1 mA */
>>>>    
>>>>    /*
>>>>     * bit masks for reading the settings in the configuration register
>>>> @@ -114,7 +115,7 @@ struct ina2xx_config {
>>>>    	int shunt_div;
>>>>    	int bus_voltage_shift;
>>>>    	int bus_voltage_lsb;	/* uV */
>>>> -	int power_lsb;		/* uW */
>>>> +	int power_lsb_factor;
>>>>    	enum ina2xx_ids chip_id;
>>>>    };
>>>>    
>>>> @@ -123,7 +124,10 @@ struct ina2xx_chip_info {
>>>>    	struct task_struct *task;
>>>>    	const struct ina2xx_config *config;
>>>>    	struct mutex state_lock;
>>>> -	unsigned int shunt_resistor;
>>>> +	unsigned int shunt_resistor;		/* uOhms */
>>>> +	unsigned int max_expected_current;	/* mA */
>>>> +	int current_lsb;			/* uA */
>>>> +	int power_lsb;				/* uW */
>>>>    	int avg;
>>>>    	int int_time_vbus; /* Bus voltage integration time uS */
>>>>    	int int_time_vshunt; /* Shunt voltage integration time uS */
>>>> @@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>>>    		.shunt_div = 100,
>>>>    		.bus_voltage_shift = 3,
>>>>    		.bus_voltage_lsb = 4000,
>>>> -		.power_lsb = 20000,
>>>> +		.power_lsb_factor = 20,
>>>>    		.chip_id = ina219,
>>>>    	},
>>>>    	[ina226] = {
>>>> @@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>>>    		.shunt_div = 400,
>>>>    		.bus_voltage_shift = 0,
>>>>    		.bus_voltage_lsb = 1250,
>>>> -		.power_lsb = 25000,
>>>> +		.power_lsb_factor = 25,
>>>>    		.chip_id = ina226,
>>>>    	},
>>>>    };
>>>> @@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>>>    
>>>>    		case INA2XX_POWER:
>>>>    			/* processed (mW) = raw*lsb (uW) / 1000 */
>>>> -			*val = chip->config->power_lsb;
>>>> +			*val = chip->power_lsb;
>>>>    			*val2 = 1000;
>>>>    			return IIO_VAL_FRACTIONAL;
>>>>    
>>>>    		case INA2XX_CURRENT:
>>>> -			/* processed (mA) = raw (mA) */
>>>> -			*val = 1;
>>>> -			return IIO_VAL_INT;
>>>> +			/* processed (mA) = raw*lsb (uA) / 1000 */
>>>> +			*val = chip->current_lsb;
>>>> +			*val2 = 1000;
>>>> +			return IIO_VAL_FRACTIONAL;
>>>>    		}
>>>>    	}
>>>>    
>>>> @@ -434,24 +439,47 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
>>>>    }
>>>>    
>>>>    /*
>>>> - * Set current LSB to 1mA, shunt is in uOhms
>>>> - * (equation 13 in datasheet). We hardcode a Current_LSB
>>>> - * of 1.0 x10-6. The only remaining parameter is RShunt.
>>>> + * Calculate calibration value according to equation 1 in ina226 datasheet
>>>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
>>>> + * Current LSB is in uA and RShunt is in uOhms, so RShunt should be
>>>> + * converted to mOhms in order to keep the scale.
>>>>     * There is no need to expose the CALIBRATION register
>>>>     * to the user for now. But we need to reset this register
>>>> - * if the user updates RShunt after driver init, e.g upon
>>>> - * reading an EEPROM/Probe-type value.
>>>> + * if the user updates RShunt or max expected current after driver
>>>> + * init, e.g upon reading an EEPROM/Probe-type value.
>>>>     */
>>>>    static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
>>>>    {
>>>> +	unsigned int rshunt = DIV_ROUND_CLOSEST(chip->shunt_resistor, 1000);
>>>>    	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
>>>> -				   chip->shunt_resistor);
>>>> +				     chip->current_lsb * rshunt);
>>>>    
>>>>    	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>>>>    }
>>>>    
>>>> +/*
>>>> + * Set max_expected_current (mA) and calculate current_lsb (uA),
>>>> + * according to equation 2 in ina226 datasheet. Power LSB is calculated
>>>> + * by multiplying Current LSB by a given factor, which may vary depending
>>>> + * on ina version.
>>>> + */
>>>> +static int set_max_expected_current(struct ina2xx_chip_info *chip,
>>>> +				    unsigned int val)
>>>> +{
>>>> +	if (val <= 0 || val > chip->config->calibration_factor)
>>>> +		return -EINVAL;
>>>> +
>>>> +	chip->max_expected_current = val;
>>>> +	chip->current_lsb = DIV_ROUND_CLOSEST(chip->max_expected_current * 1000,
>>>> +					      1 << 15);
>>>> +	chip->power_lsb = chip->current_lsb * chip->config->power_lsb_factor;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
>>>>    {
>>>> +
>>>>    	if (val <= 0 || val > chip->config->calibration_factor)
>>>>    		return -EINVAL;
>>>>    
>>>> @@ -493,6 +521,39 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>>>>    	return len;
>>>>    }
>>>>    
>>>> +static ssize_t ina2xx_max_expected_current_show(struct device *dev,
>>>> +					  struct device_attribute *attr,
>>>> +					  char *buf)
>>>> +{
>>>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
>>>> +
>>>> +	return sprintf(buf, "%d\n", chip->max_expected_current);
>>>> +}
>>>> +
>>>> +static ssize_t ina2xx_max_expected_current_store(struct device *dev,
>>>> +					   struct device_attribute *attr,
>>>> +					   const char *buf, size_t len)
>>>> +{
>>>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
>>>> +	unsigned long val;
>>>> +	int ret;
>>>> +
>>>> +	ret = kstrtoul((const char *) buf, 10, &val);
>>>
>>> Odd bit of casting given that's what it already is...
>>>    
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = set_max_expected_current(chip, val);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	/* Update the Calibration register */
>>>> +	ret = ina2xx_set_calibration(chip);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return len;
>>>> +}
>>>> +
>>>>    #define INA219_CHAN(_type, _index, _address) { \
>>>>    	.type = (_type), \
>>>>    	.address = (_address), \
>>>> @@ -755,10 +816,15 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
>>>>    		       ina2xx_shunt_resistor_show,
>>>>    		       ina2xx_shunt_resistor_store, 0);
>>>>    
>>>> +static IIO_DEVICE_ATTR(in_max_expected_current, S_IRUGO | S_IWUSR,
>>>> +		       ina2xx_max_expected_current_show,
>>>> +		       ina2xx_max_expected_current_store, 0);
>>>> +
>>>>    static struct attribute *ina219_attributes[] = {
>>>>    	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>>>>    	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
>>>>    	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>>>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
>>>>    	NULL,
>>>>    };
>>>>    
>>>> @@ -766,6 +832,7 @@ static struct attribute *ina226_attributes[] = {
>>>>    	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>>>>    	&iio_const_attr_ina226_integration_time_available.dev_attr.attr,
>>>>    	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>>>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
>>>>    	NULL,
>>>>    };
>>>>    
>>>> @@ -851,6 +918,21 @@ static int ina2xx_probe(struct i2c_client *client,
>>>>    	if (ret)
>>>>    		return ret;
>>>>    
>>>> +	if (of_property_read_u32(client->dev.of_node,
>>>> +				  "max-expected-current", &val) < 0) {
>>>> +		struct ina2xx_platform_data *pdata =
>>>> +		    dev_get_platdata(&client->dev);
>>>> +
>>>> +		if (pdata && pdata->max_mA != 0)
>>>> +			val = pdata->max_mA;
>>>> +		else
>>>> +			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
>>>> +	}
>>>> +
>>>> +	ret = set_max_expected_current(chip, val);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>>    	/* Patch the current config register with default. */
>>>>    	val = chip->config->config_default;
>>>>    
>>>> diff --git a/include/linux/platform_data/ina2xx.h b/include/linux/platform_data/ina2xx.h
>>>> index 9abc0ca..f02b1d8 100644
>>>> --- a/include/linux/platform_data/ina2xx.h
>>>> +++ b/include/linux/platform_data/ina2xx.h
>>>> @@ -13,7 +13,9 @@
>>>>    /**
>>>>     * struct ina2xx_platform_data - ina2xx info
>>>>     * @shunt_uohms		shunt resistance in microohms
>>>> + * @max_mA		max expected current in mA
>>>>     */
>>>>    struct ina2xx_platform_data {
>>>>    	long shunt_uohms;
>>>> +	int max_mA;
>>>>    };
>>>
>>>
>>>
>>>    
> 
> 
> 
> 

Re: [PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Jonathan Cameron]

On Mon, 9 Oct 2017 10:08:42 +0200
Maciej Purski <m.purski@samsung.com> wrote:

> On 10/08/2017 11:47 AM, Jonathan Cameron wrote:
> > On Wed, 04 Oct 2017 09:11:31 +0200
> > Maciej Purski <m.purski@samsung.com> wrote:
> >   
> >> On 10/01/2017 12:29 PM, Jonathan Cameron wrote:  
> >>> On Thu, 28 Sep 2017 14:50:12 +0200
> >>> Maciej Purski <m.purski@samsung.com> wrote:
> >>>      
> >>>> Max expected current is used for calculating calibration register value,
> >>>> Current LSB and Power LSB according to equations found in ina datasheet.
> >>>> Max expected current is now implicitly set to default value,
> >>>> which is 2^15, thanks to which Current LSB is equal to 1 mA and
> >>>> Power LSB is equal to 20000 uW or 25000 uW depending on ina model.
> >>>>
> >>>> Make max expected current configurable, just like it's already done
> >>>> with shunt resistance: from device tree, platform_data or later
> >>>> from sysfs. On each max_expected_current change, calculate new values
> >>>> for Current LSB and Power LSB. According to datasheet Current LSB should
> >>>> be calculated by dividing max expected current by 2^15, as values read
> >>>> from device registers are in this case 16-bit integers. Power LSB
> >>>> is calculated by multiplying Current LSB by a factor, which is defined
> >>>> in ina documentation.  
> >>>
> >>> One odd bit of casting inline.  Also this is new userspace ABI.
> >>> It needs documenting in
> >>>
> >>> Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
> >>> I'm also unclear on one element about this - is it a value used only
> >>> for calibration or are we talking about the actual 'range' of the device?
> >>>      
> >>
> >> This is used for calibration. The device measures directly only voltage.
> >> However, it has also current and power registers. Their values are
> >> calculated by the device using the calibration register which is calculated
> >> using max expected current. So I guess that it's not what you mean
> >> by the actual 'range' of the device.
> >>  
> >>> The interpretation of this value isn't clear against the more general
> >>> ABI.
> >>>
> >>> In particular it is it in raw units (adc counts) or mA?  Docs say
> >>> that but the naming of the attribute doesn't make this clear.
> >>>      
> >>
> >> It's in mA. I can make it clear in the attribute name.
> >>  
> >>> Also I'm unconvinced this isn't better represented using the
> >>> range specifications available for any IIO attribute on the raw
> >>> value in combination with adjusting the scale value.
> >>> Note not many drivers yet provide ranges on their raw outputs
> >>> but we do have core support for it.  I've been meaning to start
> >>> pushing this out into drivers, but been busy since we introduced
> >>> the core support.  The dpot-dac driver does use it for examplel
> >>>     
> >>
> >>
> >> I'm not sure if what I'm about to add is similar to what is done
> >> in the mentioned dpot-dac driver. It seems that the callback read_avail
> >> returns information on raw values which can be obtained from the device.
> >> What I need is an adjustable value, which is then used by the device internally
> >> in order to calculate current with the requested precision. Max expected current
> >> is also used for calculating the scale value.
> >> Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO and
> >> your solution fits in here.
> >>  
> > 
> > I think I answered this in the other branch of the thread.
> > _calibscale is what you want here as it's internal to the device.
> > 
> > It's not one often used for ADCs but quite a few other types of
> > device provide some front end analog adjustment (whilst it is digital
> > here, it is applied within the device - so we don't need to care).
> > 
> > Jonathan  
> 
> Thank you for your explanation. Calibscale seems suitable for me in this case,
> but what do you think I should do then with SCALE attribute? Should I get rid of 
> it for current and use only calibscale? Or should I use both calibscale and 
> scale attributes and for current they will be the same value?

You'll have to leave it as it is existing ABI.  It won't have the same value
as calibscale.  Calibscale is for internal changes that don't effect the raw
value.  scale is to be applied by userspace to the raw value. As I understand it
here the calibscale value should have no effect on scale.
> 
> I should mention that currenst_lsb value is also used for calculating power_lsb
> as they have a fixed ratio (20 or 25) given in the documentation.

Then expose it for power as well (appropriately adjusted).
In IIO ABI it is fine to have two elements addressing the same underlying hardware
value - rule is you change something, you check everything else hasn't changed.

Jonathan 
> 
> Thanks,
> 
> Maciej
> 
> >   
> >> Best regards,
> >>
> >> 	Maciej  
> >>> This moves the burden of calculating the 1lsb value to userspace,
> >>> but importantly it would give us a consistent ABI where this fits
> >>> in with existing elements (largely buy not introducing any new
> >>> ones :).
> >>>
> >>> Thanks,
> >>>
> >>> Jonathan  
> >>>>
> >>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> >>>> ---
> >>>>    drivers/iio/adc/ina2xx-adc.c         | 110 ++++++++++++++++++++++++++++++-----
> >>>>    include/linux/platform_data/ina2xx.h |   2 +
> >>>>    2 files changed, 98 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> >>>> index f387b97..883fede 100644
> >>>> --- a/drivers/iio/adc/ina2xx-adc.c
> >>>> +++ b/drivers/iio/adc/ina2xx-adc.c
> >>>> @@ -56,6 +56,7 @@
> >>>>    #define INA226_DEFAULT_IT		1110
> >>>>    
> >>>>    #define INA2XX_RSHUNT_DEFAULT           10000
> >>>> +#define INA2XX_MAX_EXPECTED_A_DEFAULT	(1 << 15)	/* current_lsb = 1 mA */
> >>>>    
> >>>>    /*
> >>>>     * bit masks for reading the settings in the configuration register
> >>>> @@ -114,7 +115,7 @@ struct ina2xx_config {
> >>>>    	int shunt_div;
> >>>>    	int bus_voltage_shift;
> >>>>    	int bus_voltage_lsb;	/* uV */
> >>>> -	int power_lsb;		/* uW */
> >>>> +	int power_lsb_factor;
> >>>>    	enum ina2xx_ids chip_id;
> >>>>    };
> >>>>    
> >>>> @@ -123,7 +124,10 @@ struct ina2xx_chip_info {
> >>>>    	struct task_struct *task;
> >>>>    	const struct ina2xx_config *config;
> >>>>    	struct mutex state_lock;
> >>>> -	unsigned int shunt_resistor;
> >>>> +	unsigned int shunt_resistor;		/* uOhms */
> >>>> +	unsigned int max_expected_current;	/* mA */
> >>>> +	int current_lsb;			/* uA */
> >>>> +	int power_lsb;				/* uW */
> >>>>    	int avg;
> >>>>    	int int_time_vbus; /* Bus voltage integration time uS */
> >>>>    	int int_time_vshunt; /* Shunt voltage integration time uS */
> >>>> @@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> >>>>    		.shunt_div = 100,
> >>>>    		.bus_voltage_shift = 3,
> >>>>    		.bus_voltage_lsb = 4000,
> >>>> -		.power_lsb = 20000,
> >>>> +		.power_lsb_factor = 20,
> >>>>    		.chip_id = ina219,
> >>>>    	},
> >>>>    	[ina226] = {
> >>>> @@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> >>>>    		.shunt_div = 400,
> >>>>    		.bus_voltage_shift = 0,
> >>>>    		.bus_voltage_lsb = 1250,
> >>>> -		.power_lsb = 25000,
> >>>> +		.power_lsb_factor = 25,
> >>>>    		.chip_id = ina226,
> >>>>    	},
> >>>>    };
> >>>> @@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> >>>>    
> >>>>    		case INA2XX_POWER:
> >>>>    			/* processed (mW) = raw*lsb (uW) / 1000 */
> >>>> -			*val = chip->config->power_lsb;
> >>>> +			*val = chip->power_lsb;
> >>>>    			*val2 = 1000;
> >>>>    			return IIO_VAL_FRACTIONAL;
> >>>>    
> >>>>    		case INA2XX_CURRENT:
> >>>> -			/* processed (mA) = raw (mA) */
> >>>> -			*val = 1;
> >>>> -			return IIO_VAL_INT;
> >>>> +			/* processed (mA) = raw*lsb (uA) / 1000 */
> >>>> +			*val = chip->current_lsb;
> >>>> +			*val2 = 1000;
> >>>> +			return IIO_VAL_FRACTIONAL;
> >>>>    		}
> >>>>    	}
> >>>>    
> >>>> @@ -434,24 +439,47 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
> >>>>    }
> >>>>    
> >>>>    /*
> >>>> - * Set current LSB to 1mA, shunt is in uOhms
> >>>> - * (equation 13 in datasheet). We hardcode a Current_LSB
> >>>> - * of 1.0 x10-6. The only remaining parameter is RShunt.
> >>>> + * Calculate calibration value according to equation 1 in ina226 datasheet
> >>>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> >>>> + * Current LSB is in uA and RShunt is in uOhms, so RShunt should be
> >>>> + * converted to mOhms in order to keep the scale.
> >>>>     * There is no need to expose the CALIBRATION register
> >>>>     * to the user for now. But we need to reset this register
> >>>> - * if the user updates RShunt after driver init, e.g upon
> >>>> - * reading an EEPROM/Probe-type value.
> >>>> + * if the user updates RShunt or max expected current after driver
> >>>> + * init, e.g upon reading an EEPROM/Probe-type value.
> >>>>     */
> >>>>    static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
> >>>>    {
> >>>> +	unsigned int rshunt = DIV_ROUND_CLOSEST(chip->shunt_resistor, 1000);
> >>>>    	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> >>>> -				   chip->shunt_resistor);
> >>>> +				     chip->current_lsb * rshunt);
> >>>>    
> >>>>    	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> >>>>    }
> >>>>    
> >>>> +/*
> >>>> + * Set max_expected_current (mA) and calculate current_lsb (uA),
> >>>> + * according to equation 2 in ina226 datasheet. Power LSB is calculated
> >>>> + * by multiplying Current LSB by a given factor, which may vary depending
> >>>> + * on ina version.
> >>>> + */
> >>>> +static int set_max_expected_current(struct ina2xx_chip_info *chip,
> >>>> +				    unsigned int val)
> >>>> +{
> >>>> +	if (val <= 0 || val > chip->config->calibration_factor)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	chip->max_expected_current = val;
> >>>> +	chip->current_lsb = DIV_ROUND_CLOSEST(chip->max_expected_current * 1000,
> >>>> +					      1 << 15);
> >>>> +	chip->power_lsb = chip->current_lsb * chip->config->power_lsb_factor;
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>    static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
> >>>>    {
> >>>> +
> >>>>    	if (val <= 0 || val > chip->config->calibration_factor)
> >>>>    		return -EINVAL;
> >>>>    
> >>>> @@ -493,6 +521,39 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> >>>>    	return len;
> >>>>    }
> >>>>    
> >>>> +static ssize_t ina2xx_max_expected_current_show(struct device *dev,
> >>>> +					  struct device_attribute *attr,
> >>>> +					  char *buf)
> >>>> +{
> >>>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> >>>> +
> >>>> +	return sprintf(buf, "%d\n", chip->max_expected_current);
> >>>> +}
> >>>> +
> >>>> +static ssize_t ina2xx_max_expected_current_store(struct device *dev,
> >>>> +					   struct device_attribute *attr,
> >>>> +					   const char *buf, size_t len)
> >>>> +{
> >>>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> >>>> +	unsigned long val;
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = kstrtoul((const char *) buf, 10, &val);  
> >>>
> >>> Odd bit of casting given that's what it already is...
> >>>      
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	ret = set_max_expected_current(chip, val);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	/* Update the Calibration register */
> >>>> +	ret = ina2xx_set_calibration(chip);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	return len;
> >>>> +}
> >>>> +
> >>>>    #define INA219_CHAN(_type, _index, _address) { \
> >>>>    	.type = (_type), \
> >>>>    	.address = (_address), \
> >>>> @@ -755,10 +816,15 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
> >>>>    		       ina2xx_shunt_resistor_show,
> >>>>    		       ina2xx_shunt_resistor_store, 0);
> >>>>    
> >>>> +static IIO_DEVICE_ATTR(in_max_expected_current, S_IRUGO | S_IWUSR,
> >>>> +		       ina2xx_max_expected_current_show,
> >>>> +		       ina2xx_max_expected_current_store, 0);
> >>>> +
> >>>>    static struct attribute *ina219_attributes[] = {
> >>>>    	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
> >>>>    	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
> >>>>    	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> >>>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
> >>>>    	NULL,
> >>>>    };
> >>>>    
> >>>> @@ -766,6 +832,7 @@ static struct attribute *ina226_attributes[] = {
> >>>>    	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
> >>>>    	&iio_const_attr_ina226_integration_time_available.dev_attr.attr,
> >>>>    	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> >>>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
> >>>>    	NULL,
> >>>>    };
> >>>>    
> >>>> @@ -851,6 +918,21 @@ static int ina2xx_probe(struct i2c_client *client,
> >>>>    	if (ret)
> >>>>    		return ret;
> >>>>    
> >>>> +	if (of_property_read_u32(client->dev.of_node,
> >>>> +				  "max-expected-current", &val) < 0) {
> >>>> +		struct ina2xx_platform_data *pdata =
> >>>> +		    dev_get_platdata(&client->dev);
> >>>> +
> >>>> +		if (pdata && pdata->max_mA != 0)
> >>>> +			val = pdata->max_mA;
> >>>> +		else
> >>>> +			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
> >>>> +	}
> >>>> +
> >>>> +	ret = set_max_expected_current(chip, val);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>>    	/* Patch the current config register with default. */
> >>>>    	val = chip->config->config_default;
> >>>>    
> >>>> diff --git a/include/linux/platform_data/ina2xx.h b/include/linux/platform_data/ina2xx.h
> >>>> index 9abc0ca..f02b1d8 100644
> >>>> --- a/include/linux/platform_data/ina2xx.h
> >>>> +++ b/include/linux/platform_data/ina2xx.h
> >>>> @@ -13,7 +13,9 @@
> >>>>    /**
> >>>>     * struct ina2xx_platform_data - ina2xx info
> >>>>     * @shunt_uohms		shunt resistance in microohms
> >>>> + * @max_mA		max expected current in mA
> >>>>     */
> >>>>    struct ina2xx_platform_data {
> >>>>    	long shunt_uohms;
> >>>> +	int max_mA;
> >>>>    };  
> >>>
> >>>
> >>>
> >>>      
> > 
> > 
> > 
> >   
> --
> 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] iio: adc: ina2xx: Make max expected current configurable

[Maciej Purski]

On 10/09/2017 03:35 PM, Jonathan Cameron wrote:
> On Mon, 9 Oct 2017 10:08:42 +0200
> Maciej Purski <m.purski@samsung.com> wrote:
> 
>> On 10/08/2017 11:47 AM, Jonathan Cameron wrote:
>>> On Wed, 04 Oct 2017 09:11:31 +0200
>>> Maciej Purski <m.purski@samsung.com> wrote:
>>>    
>>>> On 10/01/2017 12:29 PM, Jonathan Cameron wrote:
>>>>> On Thu, 28 Sep 2017 14:50:12 +0200
>>>>> Maciej Purski <m.purski@samsung.com> wrote:
>>>>>       
>>>>>> Max expected current is used for calculating calibration register value,
>>>>>> Current LSB and Power LSB according to equations found in ina datasheet.
>>>>>> Max expected current is now implicitly set to default value,
>>>>>> which is 2^15, thanks to which Current LSB is equal to 1 mA and
>>>>>> Power LSB is equal to 20000 uW or 25000 uW depending on ina model.
>>>>>>
>>>>>> Make max expected current configurable, just like it's already done
>>>>>> with shunt resistance: from device tree, platform_data or later
>>>>>> from sysfs. On each max_expected_current change, calculate new values
>>>>>> for Current LSB and Power LSB. According to datasheet Current LSB should
>>>>>> be calculated by dividing max expected current by 2^15, as values read
>>>>>> from device registers are in this case 16-bit integers. Power LSB
>>>>>> is calculated by multiplying Current LSB by a factor, which is defined
>>>>>> in ina documentation.
>>>>>
>>>>> One odd bit of casting inline.  Also this is new userspace ABI.
>>>>> It needs documenting in
>>>>>
>>>>> Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
>>>>> I'm also unclear on one element about this - is it a value used only
>>>>> for calibration or are we talking about the actual 'range' of the device?
>>>>>       
>>>>
>>>> This is used for calibration. The device measures directly only voltage.
>>>> However, it has also current and power registers. Their values are
>>>> calculated by the device using the calibration register which is calculated
>>>> using max expected current. So I guess that it's not what you mean
>>>> by the actual 'range' of the device.
>>>>   
>>>>> The interpretation of this value isn't clear against the more general
>>>>> ABI.
>>>>>
>>>>> In particular it is it in raw units (adc counts) or mA?  Docs say
>>>>> that but the naming of the attribute doesn't make this clear.
>>>>>       
>>>>
>>>> It's in mA. I can make it clear in the attribute name.
>>>>   
>>>>> Also I'm unconvinced this isn't better represented using the
>>>>> range specifications available for any IIO attribute on the raw
>>>>> value in combination with adjusting the scale value.
>>>>> Note not many drivers yet provide ranges on their raw outputs
>>>>> but we do have core support for it.  I've been meaning to start
>>>>> pushing this out into drivers, but been busy since we introduced
>>>>> the core support.  The dpot-dac driver does use it for examplel
>>>>>      
>>>>
>>>>
>>>> I'm not sure if what I'm about to add is similar to what is done
>>>> in the mentioned dpot-dac driver. It seems that the callback read_avail
>>>> returns information on raw values which can be obtained from the device.
>>>> What I need is an adjustable value, which is then used by the device internally
>>>> in order to calculate current with the requested precision. Max expected current
>>>> is also used for calculating the scale value.
>>>> Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO and
>>>> your solution fits in here.
>>>>   
>>>
>>> I think I answered this in the other branch of the thread.
>>> _calibscale is what you want here as it's internal to the device.
>>>
>>> It's not one often used for ADCs but quite a few other types of
>>> device provide some front end analog adjustment (whilst it is digital
>>> here, it is applied within the device - so we don't need to care).
>>>
>>> Jonathan
>>
>> Thank you for your explanation. Calibscale seems suitable for me in this case,
>> but what do you think I should do then with SCALE attribute? Should I get rid of
>> it for current and use only calibscale? Or should I use both calibscale and
>> scale attributes and for current they will be the same value?
> 
> You'll have to leave it as it is existing ABI.  It won't have the same value
> as calibscale.  Calibscale is for internal changes that don't effect the raw
> value.  scale is to be applied by userspace to the raw value. As I understand it
> here the calibscale value should have no effect on scale.


Sorry, but now I'm a little bit confused. Which value would you
like me to substitute with calibscale - max_expected_current or current_lsb?
Both values do have effect on the scale, as scale is basically
current_lsb / 1000 and on the raw value, as current register is calculated
internally using current_lsb.

The content of current register is always RAW unless we set current_lsb to 1. 
See the documentation:
http://www.ti.com/lit/ds/symlink/ina231.pdf
page 15

Thanks,

Maciej

>>
>> I should mention that currenst_lsb value is also used for calculating power_lsb
>> as they have a fixed ratio (20 or 25) given in the documentation.
> 
> Then expose it for power as well (appropriately adjusted).
> In IIO ABI it is fine to have two elements addressing the same underlying hardware
> value - rule is you change something, you check everything else hasn't changed.
> 
> Jonathan >>
>> Thanks,
>>
>> Maciej
>>
>>>    
>>>> Best regards,
>>>>
>>>> 	Maciej
>>>>> This moves the burden of calculating the 1lsb value to userspace,
>>>>> but importantly it would give us a consistent ABI where this fits
>>>>> in with existing elements (largely buy not introducing any new
>>>>> ones :).
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jonathan
>>>>>>
>>>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>>>>> ---
>>>>>>     drivers/iio/adc/ina2xx-adc.c         | 110 ++++++++++++++++++++++++++++++-----
>>>>>>     include/linux/platform_data/ina2xx.h |   2 +
>>>>>>     2 files changed, 98 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>>>>>> index f387b97..883fede 100644
>>>>>> --- a/drivers/iio/adc/ina2xx-adc.c
>>>>>> +++ b/drivers/iio/adc/ina2xx-adc.c
>>>>>> @@ -56,6 +56,7 @@
>>>>>>     #define INA226_DEFAULT_IT		1110
>>>>>>     
>>>>>>     #define INA2XX_RSHUNT_DEFAULT           10000
>>>>>> +#define INA2XX_MAX_EXPECTED_A_DEFAULT	(1 << 15)	/* current_lsb = 1 mA */
>>>>>>     
>>>>>>     /*
>>>>>>      * bit masks for reading the settings in the configuration register
>>>>>> @@ -114,7 +115,7 @@ struct ina2xx_config {
>>>>>>     	int shunt_div;
>>>>>>     	int bus_voltage_shift;
>>>>>>     	int bus_voltage_lsb;	/* uV */
>>>>>> -	int power_lsb;		/* uW */
>>>>>> +	int power_lsb_factor;
>>>>>>     	enum ina2xx_ids chip_id;
>>>>>>     };
>>>>>>     
>>>>>> @@ -123,7 +124,10 @@ struct ina2xx_chip_info {
>>>>>>     	struct task_struct *task;
>>>>>>     	const struct ina2xx_config *config;
>>>>>>     	struct mutex state_lock;
>>>>>> -	unsigned int shunt_resistor;
>>>>>> +	unsigned int shunt_resistor;		/* uOhms */
>>>>>> +	unsigned int max_expected_current;	/* mA */
>>>>>> +	int current_lsb;			/* uA */
>>>>>> +	int power_lsb;				/* uW */
>>>>>>     	int avg;
>>>>>>     	int int_time_vbus; /* Bus voltage integration time uS */
>>>>>>     	int int_time_vshunt; /* Shunt voltage integration time uS */
>>>>>> @@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>>>>>     		.shunt_div = 100,
>>>>>>     		.bus_voltage_shift = 3,
>>>>>>     		.bus_voltage_lsb = 4000,
>>>>>> -		.power_lsb = 20000,
>>>>>> +		.power_lsb_factor = 20,
>>>>>>     		.chip_id = ina219,
>>>>>>     	},
>>>>>>     	[ina226] = {
>>>>>> @@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>>>>>     		.shunt_div = 400,
>>>>>>     		.bus_voltage_shift = 0,
>>>>>>     		.bus_voltage_lsb = 1250,
>>>>>> -		.power_lsb = 25000,
>>>>>> +		.power_lsb_factor = 25,
>>>>>>     		.chip_id = ina226,
>>>>>>     	},
>>>>>>     };
>>>>>> @@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>>>>>     
>>>>>>     		case INA2XX_POWER:
>>>>>>     			/* processed (mW) = raw*lsb (uW) / 1000 */
>>>>>> -			*val = chip->config->power_lsb;
>>>>>> +			*val = chip->power_lsb;
>>>>>>     			*val2 = 1000;
>>>>>>     			return IIO_VAL_FRACTIONAL;
>>>>>>     
>>>>>>     		case INA2XX_CURRENT:
>>>>>> -			/* processed (mA) = raw (mA) */
>>>>>> -			*val = 1;
>>>>>> -			return IIO_VAL_INT;
>>>>>> +			/* processed (mA) = raw*lsb (uA) / 1000 */
>>>>>> +			*val = chip->current_lsb;
>>>>>> +			*val2 = 1000;
>>>>>> +			return IIO_VAL_FRACTIONAL;
>>>>>>     		}
>>>>>>     	}
>>>>>>     
>>>>>> @@ -434,24 +439,47 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
>>>>>>     }
>>>>>>     
>>>>>>     /*
>>>>>> - * Set current LSB to 1mA, shunt is in uOhms
>>>>>> - * (equation 13 in datasheet). We hardcode a Current_LSB
>>>>>> - * of 1.0 x10-6. The only remaining parameter is RShunt.
>>>>>> + * Calculate calibration value according to equation 1 in ina226 datasheet
>>>>>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
>>>>>> + * Current LSB is in uA and RShunt is in uOhms, so RShunt should be
>>>>>> + * converted to mOhms in order to keep the scale.
>>>>>>      * There is no need to expose the CALIBRATION register
>>>>>>      * to the user for now. But we need to reset this register
>>>>>> - * if the user updates RShunt after driver init, e.g upon
>>>>>> - * reading an EEPROM/Probe-type value.
>>>>>> + * if the user updates RShunt or max expected current after driver
>>>>>> + * init, e.g upon reading an EEPROM/Probe-type value.
>>>>>>      */
>>>>>>     static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
>>>>>>     {
>>>>>> +	unsigned int rshunt = DIV_ROUND_CLOSEST(chip->shunt_resistor, 1000);
>>>>>>     	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
>>>>>> -				   chip->shunt_resistor);
>>>>>> +				     chip->current_lsb * rshunt);
>>>>>>     
>>>>>>     	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>>>>>>     }
>>>>>>     
>>>>>> +/*
>>>>>> + * Set max_expected_current (mA) and calculate current_lsb (uA),
>>>>>> + * according to equation 2 in ina226 datasheet. Power LSB is calculated
>>>>>> + * by multiplying Current LSB by a given factor, which may vary depending
>>>>>> + * on ina version.
>>>>>> + */
>>>>>> +static int set_max_expected_current(struct ina2xx_chip_info *chip,
>>>>>> +				    unsigned int val)
>>>>>> +{
>>>>>> +	if (val <= 0 || val > chip->config->calibration_factor)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	chip->max_expected_current = val;
>>>>>> +	chip->current_lsb = DIV_ROUND_CLOSEST(chip->max_expected_current * 1000,
>>>>>> +					      1 << 15);
>>>>>> +	chip->power_lsb = chip->current_lsb * chip->config->power_lsb_factor;
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>>     static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
>>>>>>     {
>>>>>> +
>>>>>>     	if (val <= 0 || val > chip->config->calibration_factor)
>>>>>>     		return -EINVAL;
>>>>>>     
>>>>>> @@ -493,6 +521,39 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>>>>>>     	return len;
>>>>>>     }
>>>>>>     
>>>>>> +static ssize_t ina2xx_max_expected_current_show(struct device *dev,
>>>>>> +					  struct device_attribute *attr,
>>>>>> +					  char *buf)
>>>>>> +{
>>>>>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
>>>>>> +
>>>>>> +	return sprintf(buf, "%d\n", chip->max_expected_current);
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t ina2xx_max_expected_current_store(struct device *dev,
>>>>>> +					   struct device_attribute *attr,
>>>>>> +					   const char *buf, size_t len)
>>>>>> +{
>>>>>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
>>>>>> +	unsigned long val;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = kstrtoul((const char *) buf, 10, &val);
>>>>>
>>>>> Odd bit of casting given that's what it already is...
>>>>>       
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	ret = set_max_expected_current(chip, val);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	/* Update the Calibration register */
>>>>>> +	ret = ina2xx_set_calibration(chip);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	return len;
>>>>>> +}
>>>>>> +
>>>>>>     #define INA219_CHAN(_type, _index, _address) { \
>>>>>>     	.type = (_type), \
>>>>>>     	.address = (_address), \
>>>>>> @@ -755,10 +816,15 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
>>>>>>     		       ina2xx_shunt_resistor_show,
>>>>>>     		       ina2xx_shunt_resistor_store, 0);
>>>>>>     
>>>>>> +static IIO_DEVICE_ATTR(in_max_expected_current, S_IRUGO | S_IWUSR,
>>>>>> +		       ina2xx_max_expected_current_show,
>>>>>> +		       ina2xx_max_expected_current_store, 0);
>>>>>> +
>>>>>>     static struct attribute *ina219_attributes[] = {
>>>>>>     	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>>>>>>     	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
>>>>>>     	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>>>>>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
>>>>>>     	NULL,
>>>>>>     };
>>>>>>     
>>>>>> @@ -766,6 +832,7 @@ static struct attribute *ina226_attributes[] = {
>>>>>>     	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>>>>>>     	&iio_const_attr_ina226_integration_time_available.dev_attr.attr,
>>>>>>     	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>>>>>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
>>>>>>     	NULL,
>>>>>>     };
>>>>>>     
>>>>>> @@ -851,6 +918,21 @@ static int ina2xx_probe(struct i2c_client *client,
>>>>>>     	if (ret)
>>>>>>     		return ret;
>>>>>>     
>>>>>> +	if (of_property_read_u32(client->dev.of_node,
>>>>>> +				  "max-expected-current", &val) < 0) {
>>>>>> +		struct ina2xx_platform_data *pdata =
>>>>>> +		    dev_get_platdata(&client->dev);
>>>>>> +
>>>>>> +		if (pdata && pdata->max_mA != 0)
>>>>>> +			val = pdata->max_mA;
>>>>>> +		else
>>>>>> +			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
>>>>>> +	}
>>>>>> +
>>>>>> +	ret = set_max_expected_current(chip, val);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>>     	/* Patch the current config register with default. */
>>>>>>     	val = chip->config->config_default;
>>>>>>     
>>>>>> diff --git a/include/linux/platform_data/ina2xx.h b/include/linux/platform_data/ina2xx.h
>>>>>> index 9abc0ca..f02b1d8 100644
>>>>>> --- a/include/linux/platform_data/ina2xx.h
>>>>>> +++ b/include/linux/platform_data/ina2xx.h
>>>>>> @@ -13,7 +13,9 @@
>>>>>>     /**
>>>>>>      * struct ina2xx_platform_data - ina2xx info
>>>>>>      * @shunt_uohms		shunt resistance in microohms
>>>>>> + * @max_mA		max expected current in mA
>>>>>>      */
>>>>>>     struct ina2xx_platform_data {
>>>>>>     	long shunt_uohms;
>>>>>> +	int max_mA;
>>>>>>     };
>>>>>
>>>>>
>>>>>
>>>>>       
>>>
>>>
>>>
>>>    
>> --
>> 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] iio: adc: ina2xx: Make max expected current configurable

[Jonathan Cameron]

On Wed, 11 Oct 2017 16:42:53 +0200
Maciej Purski <m.purski@samsung.com> wrote:

> On 10/09/2017 03:35 PM, Jonathan Cameron wrote:
> > On Mon, 9 Oct 2017 10:08:42 +0200
> > Maciej Purski <m.purski@samsung.com> wrote:
> >   
> >> On 10/08/2017 11:47 AM, Jonathan Cameron wrote:  
> >>> On Wed, 04 Oct 2017 09:11:31 +0200
> >>> Maciej Purski <m.purski@samsung.com> wrote:
> >>>      
> >>>> On 10/01/2017 12:29 PM, Jonathan Cameron wrote:  
> >>>>> On Thu, 28 Sep 2017 14:50:12 +0200
> >>>>> Maciej Purski <m.purski@samsung.com> wrote:
> >>>>>         
> >>>>>> Max expected current is used for calculating calibration register value,
> >>>>>> Current LSB and Power LSB according to equations found in ina datasheet.
> >>>>>> Max expected current is now implicitly set to default value,
> >>>>>> which is 2^15, thanks to which Current LSB is equal to 1 mA and
> >>>>>> Power LSB is equal to 20000 uW or 25000 uW depending on ina model.
> >>>>>>
> >>>>>> Make max expected current configurable, just like it's already done
> >>>>>> with shunt resistance: from device tree, platform_data or later
> >>>>>> from sysfs. On each max_expected_current change, calculate new values
> >>>>>> for Current LSB and Power LSB. According to datasheet Current LSB should
> >>>>>> be calculated by dividing max expected current by 2^15, as values read
> >>>>>> from device registers are in this case 16-bit integers. Power LSB
> >>>>>> is calculated by multiplying Current LSB by a factor, which is defined
> >>>>>> in ina documentation.  
> >>>>>
> >>>>> One odd bit of casting inline.  Also this is new userspace ABI.
> >>>>> It needs documenting in
> >>>>>
> >>>>> Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
> >>>>> I'm also unclear on one element about this - is it a value used only
> >>>>> for calibration or are we talking about the actual 'range' of the device?
> >>>>>         
> >>>>
> >>>> This is used for calibration. The device measures directly only voltage.
> >>>> However, it has also current and power registers. Their values are
> >>>> calculated by the device using the calibration register which is calculated
> >>>> using max expected current. So I guess that it's not what you mean
> >>>> by the actual 'range' of the device.
> >>>>     
> >>>>> The interpretation of this value isn't clear against the more general
> >>>>> ABI.
> >>>>>
> >>>>> In particular it is it in raw units (adc counts) or mA?  Docs say
> >>>>> that but the naming of the attribute doesn't make this clear.
> >>>>>         
> >>>>
> >>>> It's in mA. I can make it clear in the attribute name.
> >>>>     
> >>>>> Also I'm unconvinced this isn't better represented using the
> >>>>> range specifications available for any IIO attribute on the raw
> >>>>> value in combination with adjusting the scale value.
> >>>>> Note not many drivers yet provide ranges on their raw outputs
> >>>>> but we do have core support for it.  I've been meaning to start
> >>>>> pushing this out into drivers, but been busy since we introduced
> >>>>> the core support.  The dpot-dac driver does use it for examplel
> >>>>>        
> >>>>
> >>>>
> >>>> I'm not sure if what I'm about to add is similar to what is done
> >>>> in the mentioned dpot-dac driver. It seems that the callback read_avail
> >>>> returns information on raw values which can be obtained from the device.
> >>>> What I need is an adjustable value, which is then used by the device internally
> >>>> in order to calculate current with the requested precision. Max expected current
> >>>> is also used for calculating the scale value.
> >>>> Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO and
> >>>> your solution fits in here.
> >>>>     
> >>>
> >>> I think I answered this in the other branch of the thread.
> >>> _calibscale is what you want here as it's internal to the device.
> >>>
> >>> It's not one often used for ADCs but quite a few other types of
> >>> device provide some front end analog adjustment (whilst it is digital
> >>> here, it is applied within the device - so we don't need to care).
> >>>
> >>> Jonathan  
> >>
> >> Thank you for your explanation. Calibscale seems suitable for me in this case,
> >> but what do you think I should do then with SCALE attribute? Should I get rid of
> >> it for current and use only calibscale? Or should I use both calibscale and
> >> scale attributes and for current they will be the same value?  
> > 
> > You'll have to leave it as it is existing ABI.  It won't have the same value
> > as calibscale.  Calibscale is for internal changes that don't effect the raw
> > value.  scale is to be applied by userspace to the raw value. As I understand it
> > here the calibscale value should have no effect on scale.  
> 
> 
> Sorry, but now I'm a little bit confused. Which value would you
> like me to substitute with calibscale - max_expected_current or current_lsb?

As it is an internal scale it actually doesn't matter which one you use.  Both
are valid.

> Both values do have effect on the scale, as scale is basically
> current_lsb / 1000 and on the raw value, as current register is calculated
> internally using current_lsb.

Hmm The terminology in the datasheet is confusing.  I read it as these were
intended to 'calibrate' the system.  They aren't - they are intended to apply
a scaling to make the output use the full range. 

So they will effect the scale which is nasty.
The annoying bit about this is that means the key value you should be modifying
is scale and you should not use calibscale at all.  It isn't appropriate because
that is not meant to effect the scale of the exposed value.  It's for calibration
to correct device inaccuracies. That isn't true here.

Anyhow, so scale is all you should need.  A bit ugly but there we are.


> 
> The content of current register is always RAW unless we set current_lsb to 1. 
> See the documentation:
> http://www.ti.com/lit/ds/symlink/ina231.pdf
> page 15
> 
> Thanks,
> 
> Maciej
> 
> >>
> >> I should mention that currenst_lsb value is also used for calculating power_lsb
> >> as they have a fixed ratio (20 or 25) given in the documentation.  
> > 
> > Then expose it for power as well (appropriately adjusted).
> > In IIO ABI it is fine to have two elements addressing the same underlying hardware
> > value - rule is you change something, you check everything else hasn't changed.
> > 
> > Jonathan >>  
> >> Thanks,
> >>
> >> Maciej
> >>  
> >>>      
> >>>> Best regards,
> >>>>
> >>>> 	Maciej  
> >>>>> This moves the burden of calculating the 1lsb value to userspace,
> >>>>> but importantly it would give us a consistent ABI where this fits
> >>>>> in with existing elements (largely buy not introducing any new
> >>>>> ones :).
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Jonathan  
> >>>>>>
> >>>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> >>>>>> ---
> >>>>>>     drivers/iio/adc/ina2xx-adc.c         | 110 ++++++++++++++++++++++++++++++-----
> >>>>>>     include/linux/platform_data/ina2xx.h |   2 +
> >>>>>>     2 files changed, 98 insertions(+), 14 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> >>>>>> index f387b97..883fede 100644
> >>>>>> --- a/drivers/iio/adc/ina2xx-adc.c
> >>>>>> +++ b/drivers/iio/adc/ina2xx-adc.c
> >>>>>> @@ -56,6 +56,7 @@
> >>>>>>     #define INA226_DEFAULT_IT		1110
> >>>>>>     
> >>>>>>     #define INA2XX_RSHUNT_DEFAULT           10000
> >>>>>> +#define INA2XX_MAX_EXPECTED_A_DEFAULT	(1 << 15)	/* current_lsb = 1 mA */
> >>>>>>     
> >>>>>>     /*
> >>>>>>      * bit masks for reading the settings in the configuration register
> >>>>>> @@ -114,7 +115,7 @@ struct ina2xx_config {
> >>>>>>     	int shunt_div;
> >>>>>>     	int bus_voltage_shift;
> >>>>>>     	int bus_voltage_lsb;	/* uV */
> >>>>>> -	int power_lsb;		/* uW */
> >>>>>> +	int power_lsb_factor;
> >>>>>>     	enum ina2xx_ids chip_id;
> >>>>>>     };
> >>>>>>     
> >>>>>> @@ -123,7 +124,10 @@ struct ina2xx_chip_info {
> >>>>>>     	struct task_struct *task;
> >>>>>>     	const struct ina2xx_config *config;
> >>>>>>     	struct mutex state_lock;
> >>>>>> -	unsigned int shunt_resistor;
> >>>>>> +	unsigned int shunt_resistor;		/* uOhms */
> >>>>>> +	unsigned int max_expected_current;	/* mA */
> >>>>>> +	int current_lsb;			/* uA */
> >>>>>> +	int power_lsb;				/* uW */
> >>>>>>     	int avg;
> >>>>>>     	int int_time_vbus; /* Bus voltage integration time uS */
> >>>>>>     	int int_time_vshunt; /* Shunt voltage integration time uS */
> >>>>>> @@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> >>>>>>     		.shunt_div = 100,
> >>>>>>     		.bus_voltage_shift = 3,
> >>>>>>     		.bus_voltage_lsb = 4000,
> >>>>>> -		.power_lsb = 20000,
> >>>>>> +		.power_lsb_factor = 20,
> >>>>>>     		.chip_id = ina219,
> >>>>>>     	},
> >>>>>>     	[ina226] = {
> >>>>>> @@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = {
> >>>>>>     		.shunt_div = 400,
> >>>>>>     		.bus_voltage_shift = 0,
> >>>>>>     		.bus_voltage_lsb = 1250,
> >>>>>> -		.power_lsb = 25000,
> >>>>>> +		.power_lsb_factor = 25,
> >>>>>>     		.chip_id = ina226,
> >>>>>>     	},
> >>>>>>     };
> >>>>>> @@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> >>>>>>     
> >>>>>>     		case INA2XX_POWER:
> >>>>>>     			/* processed (mW) = raw*lsb (uW) / 1000 */
> >>>>>> -			*val = chip->config->power_lsb;
> >>>>>> +			*val = chip->power_lsb;
> >>>>>>     			*val2 = 1000;
> >>>>>>     			return IIO_VAL_FRACTIONAL;
> >>>>>>     
> >>>>>>     		case INA2XX_CURRENT:
> >>>>>> -			/* processed (mA) = raw (mA) */
> >>>>>> -			*val = 1;
> >>>>>> -			return IIO_VAL_INT;
> >>>>>> +			/* processed (mA) = raw*lsb (uA) / 1000 */
> >>>>>> +			*val = chip->current_lsb;
> >>>>>> +			*val2 = 1000;
> >>>>>> +			return IIO_VAL_FRACTIONAL;
> >>>>>>     		}
> >>>>>>     	}
> >>>>>>     
> >>>>>> @@ -434,24 +439,47 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
> >>>>>>     }
> >>>>>>     
> >>>>>>     /*
> >>>>>> - * Set current LSB to 1mA, shunt is in uOhms
> >>>>>> - * (equation 13 in datasheet). We hardcode a Current_LSB
> >>>>>> - * of 1.0 x10-6. The only remaining parameter is RShunt.
> >>>>>> + * Calculate calibration value according to equation 1 in ina226 datasheet
> >>>>>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> >>>>>> + * Current LSB is in uA and RShunt is in uOhms, so RShunt should be
> >>>>>> + * converted to mOhms in order to keep the scale.
> >>>>>>      * There is no need to expose the CALIBRATION register
> >>>>>>      * to the user for now. But we need to reset this register
> >>>>>> - * if the user updates RShunt after driver init, e.g upon
> >>>>>> - * reading an EEPROM/Probe-type value.
> >>>>>> + * if the user updates RShunt or max expected current after driver
> >>>>>> + * init, e.g upon reading an EEPROM/Probe-type value.
> >>>>>>      */
> >>>>>>     static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
> >>>>>>     {
> >>>>>> +	unsigned int rshunt = DIV_ROUND_CLOSEST(chip->shunt_resistor, 1000);
> >>>>>>     	u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> >>>>>> -				   chip->shunt_resistor);
> >>>>>> +				     chip->current_lsb * rshunt);
> >>>>>>     
> >>>>>>     	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> >>>>>>     }
> >>>>>>     
> >>>>>> +/*
> >>>>>> + * Set max_expected_current (mA) and calculate current_lsb (uA),
> >>>>>> + * according to equation 2 in ina226 datasheet. Power LSB is calculated
> >>>>>> + * by multiplying Current LSB by a given factor, which may vary depending
> >>>>>> + * on ina version.
> >>>>>> + */
> >>>>>> +static int set_max_expected_current(struct ina2xx_chip_info *chip,
> >>>>>> +				    unsigned int val)
> >>>>>> +{
> >>>>>> +	if (val <= 0 || val > chip->config->calibration_factor)
> >>>>>> +		return -EINVAL;
> >>>>>> +
> >>>>>> +	chip->max_expected_current = val;
> >>>>>> +	chip->current_lsb = DIV_ROUND_CLOSEST(chip->max_expected_current * 1000,
> >>>>>> +					      1 << 15);
> >>>>>> +	chip->power_lsb = chip->current_lsb * chip->config->power_lsb_factor;
> >>>>>> +
> >>>>>> +	return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>>     static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
> >>>>>>     {
> >>>>>> +
> >>>>>>     	if (val <= 0 || val > chip->config->calibration_factor)
> >>>>>>     		return -EINVAL;
> >>>>>>     
> >>>>>> @@ -493,6 +521,39 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> >>>>>>     	return len;
> >>>>>>     }
> >>>>>>     
> >>>>>> +static ssize_t ina2xx_max_expected_current_show(struct device *dev,
> >>>>>> +					  struct device_attribute *attr,
> >>>>>> +					  char *buf)
> >>>>>> +{
> >>>>>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> >>>>>> +
> >>>>>> +	return sprintf(buf, "%d\n", chip->max_expected_current);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static ssize_t ina2xx_max_expected_current_store(struct device *dev,
> >>>>>> +					   struct device_attribute *attr,
> >>>>>> +					   const char *buf, size_t len)
> >>>>>> +{
> >>>>>> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> >>>>>> +	unsigned long val;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	ret = kstrtoul((const char *) buf, 10, &val);  
> >>>>>
> >>>>> Odd bit of casting given that's what it already is...
> >>>>>         
> >>>>>> +	if (ret)
> >>>>>> +		return ret;
> >>>>>> +
> >>>>>> +	ret = set_max_expected_current(chip, val);
> >>>>>> +	if (ret)
> >>>>>> +		return ret;
> >>>>>> +
> >>>>>> +	/* Update the Calibration register */
> >>>>>> +	ret = ina2xx_set_calibration(chip);
> >>>>>> +	if (ret)
> >>>>>> +		return ret;
> >>>>>> +
> >>>>>> +	return len;
> >>>>>> +}
> >>>>>> +
> >>>>>>     #define INA219_CHAN(_type, _index, _address) { \
> >>>>>>     	.type = (_type), \
> >>>>>>     	.address = (_address), \
> >>>>>> @@ -755,10 +816,15 @@ static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
> >>>>>>     		       ina2xx_shunt_resistor_show,
> >>>>>>     		       ina2xx_shunt_resistor_store, 0);
> >>>>>>     
> >>>>>> +static IIO_DEVICE_ATTR(in_max_expected_current, S_IRUGO | S_IWUSR,
> >>>>>> +		       ina2xx_max_expected_current_show,
> >>>>>> +		       ina2xx_max_expected_current_store, 0);
> >>>>>> +
> >>>>>>     static struct attribute *ina219_attributes[] = {
> >>>>>>     	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
> >>>>>>     	&iio_const_attr_ina219_integration_time_available.dev_attr.attr,
> >>>>>>     	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> >>>>>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
> >>>>>>     	NULL,
> >>>>>>     };
> >>>>>>     
> >>>>>> @@ -766,6 +832,7 @@ static struct attribute *ina226_attributes[] = {
> >>>>>>     	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
> >>>>>>     	&iio_const_attr_ina226_integration_time_available.dev_attr.attr,
> >>>>>>     	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
> >>>>>> +	&iio_dev_attr_in_max_expected_current.dev_attr.attr,
> >>>>>>     	NULL,
> >>>>>>     };
> >>>>>>     
> >>>>>> @@ -851,6 +918,21 @@ static int ina2xx_probe(struct i2c_client *client,
> >>>>>>     	if (ret)
> >>>>>>     		return ret;
> >>>>>>     
> >>>>>> +	if (of_property_read_u32(client->dev.of_node,
> >>>>>> +				  "max-expected-current", &val) < 0) {
> >>>>>> +		struct ina2xx_platform_data *pdata =
> >>>>>> +		    dev_get_platdata(&client->dev);
> >>>>>> +
> >>>>>> +		if (pdata && pdata->max_mA != 0)
> >>>>>> +			val = pdata->max_mA;
> >>>>>> +		else
> >>>>>> +			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	ret = set_max_expected_current(chip, val);
> >>>>>> +	if (ret)
> >>>>>> +		return ret;
> >>>>>> +
> >>>>>>     	/* Patch the current config register with default. */
> >>>>>>     	val = chip->config->config_default;
> >>>>>>     
> >>>>>> diff --git a/include/linux/platform_data/ina2xx.h b/include/linux/platform_data/ina2xx.h
> >>>>>> index 9abc0ca..f02b1d8 100644
> >>>>>> --- a/include/linux/platform_data/ina2xx.h
> >>>>>> +++ b/include/linux/platform_data/ina2xx.h
> >>>>>> @@ -13,7 +13,9 @@
> >>>>>>     /**
> >>>>>>      * struct ina2xx_platform_data - ina2xx info
> >>>>>>      * @shunt_uohms		shunt resistance in microohms
> >>>>>> + * @max_mA		max expected current in mA
> >>>>>>      */
> >>>>>>     struct ina2xx_platform_data {
> >>>>>>     	long shunt_uohms;
> >>>>>> +	int max_mA;
> >>>>>>     };  
> >>>>>
> >>>>>
> >>>>>
> >>>>>         
> >>>
> >>>
> >>>
> >>>      
> >> --
> >> 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

[PATCH 2/4] hwmon: (ina2xx) Make max expected current configurable

[Maciej Purski]

Max expected current is used for calculating calibration register value,
Current LSB and Power LSB according to equations found in ina datasheet.
Max expected current is now implicitly set to default value,
which is 2^15, thanks to which Current LSB is equal to 1 mA and
Power LSB is equal to 20000 uW or 25000 uW depending on ina model.

Make max expected current configurable, just like it's already done
with shunt resistance: from device tree, platform_data or later
from sysfs. On each max_expected_current change, calculate new values
for Current LSB and Power LSB. According to datasheet Current LSB should
be calculated by dividing max expected current by 2^15, as values read
from device registers are in this case 16-bit integers. Power LSB
is calculated by multiplying Current LSB by a factor, which is defined
in ina documentation.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 drivers/hwmon/ina2xx.c | 105 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 93 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 62e38fa..d956013 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -80,6 +80,8 @@
 /* common attrs, ina226 attrs and NULL */
 #define INA2XX_MAX_ATTRIBUTE_GROUPS	3
 
+#define INA2XX_MAX_EXPECTED_A_DEFAULT  (1 << 15)       /* current_lsb = 1 mA */
+
 /*
  * Both bus voltage and shunt voltage conversion times for ina226 are set
  * to 0b0100 on POR, which translates to 2200 microseconds in total.
@@ -100,13 +102,16 @@ struct ina2xx_config {
 	int shunt_div;
 	int bus_voltage_shift;
 	int bus_voltage_lsb;	/* uV */
-	int power_lsb;		/* uW */
+	int power_lsb_factor;
 };
 
 struct ina2xx_data {
 	const struct ina2xx_config *config;
 
-	long rshunt;
+	long rshunt;				/* uOhms */
+	unsigned int max_expected_current;	/* mA */
+	int current_lsb;			/* uA */
+	int power_lsb;				/* uW */
 	struct mutex config_lock;
 	struct regmap *regmap;
 
@@ -121,7 +126,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.shunt_div = 100,
 		.bus_voltage_shift = 3,
 		.bus_voltage_lsb = 4000,
-		.power_lsb = 20000,
+		.power_lsb_factor = 20,
 	},
 	[ina226] = {
 		.config_default = INA226_CONFIG_DEFAULT,
@@ -130,7 +135,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.shunt_div = 400,
 		.bus_voltage_shift = 0,
 		.bus_voltage_lsb = 1250,
-		.power_lsb = 25000,
+		.power_lsb_factor = 25,
 	},
 };
 
@@ -169,10 +174,17 @@ static u16 ina226_interval_to_reg(int interval)
 	return INA226_SHIFT_AVG(avg_bits);
 }
 
+/*
+ * Calculate calibration value according to equation 1 in ina226 datasheet
+ * http://www.ti.com/lit/ds/symlink/ina226.pdf.
+ * Current LSB is in uA and RShunt is in uOhms, so in order to keep
+ * calibration value scaled RShunt must be converted to mOhms.
+ */
 static int ina2xx_calibrate(struct ina2xx_data *data)
 {
+	int r_shunt = DIV_ROUND_CLOSEST(data->rshunt, 1000);
 	u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
-				    data->rshunt);
+				    data->current_lsb * r_shunt);
 
 	return regmap_write(data->regmap, INA2XX_CALIBRATION, val);
 }
@@ -187,13 +199,28 @@ static int ina2xx_init(struct ina2xx_data *data)
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * Set current LSB to 1mA, shunt is in uOhms
-	 * (equation 13 in datasheet).
-	 */
 	return ina2xx_calibrate(data);
 }
 
+/*
+ * Set max_expected_current (mA) and calculate current_lsb (uA),
+ * according to equation 2 in ina226 datasheet. Power LSB is calculated
+ * by multiplying Current LSB by a given factor, which may vary depending
+ * on ina version.
+ */
+static int set_max_expected_current(struct ina2xx_data *data, unsigned int val)
+{
+	if (val <= 0 || val > data->config->calibration_factor)
+		return -EINVAL;
+
+	data->max_expected_current = val;
+	data->current_lsb = DIV_ROUND_CLOSEST(data->max_expected_current * 1000,
+					      1 << 15);
+	data->power_lsb = data->current_lsb * data->config->power_lsb_factor;
+
+	return 0;
+}
+
 static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
@@ -268,11 +295,11 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
 		val = DIV_ROUND_CLOSEST(val, 1000);
 		break;
 	case INA2XX_POWER:
-		val = regval * data->config->power_lsb;
+		val = regval * data->power_lsb;
 		break;
 	case INA2XX_CURRENT:
-		/* signed register, LSB=1mA (selected), in mA */
-		val = (s16)regval;
+		val = (s16)regval * data->current_lsb;
+		val = DIV_ROUND_CLOSEST(val, 1000);
 		break;
 	case INA2XX_CALIBRATION:
 		val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
@@ -369,6 +396,39 @@ static ssize_t ina226_show_interval(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", ina226_reg_to_interval(regval));
 }
 
+static ssize_t ina2xx_max_expected_current_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", data->max_expected_current);
+}
+
+static ssize_t ina2xx_max_expected_current_set(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf, size_t len)
+{
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul((const char *) buf, 10, &val);
+	if (ret)
+		return ret;
+
+	ret = set_max_expected_current(data, val);
+	if (ret)
+		return ret;
+
+	/* Update the Calibration register */
+	ret = ina2xx_calibrate(data);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
 /* shunt voltage */
 static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
 			  INA2XX_SHUNT_VOLTAGE);
@@ -390,6 +450,11 @@ static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR,
 			  ina2xx_show_value, ina2xx_set_shunt,
 			  INA2XX_CALIBRATION);
 
+/* max expected current */
+static SENSOR_DEVICE_ATTR(max_expected_current, S_IRUGO | S_IWUSR,
+			  ina2xx_max_expected_current_show,
+			  ina2xx_max_expected_current_set, 0);
+
 /* update interval (ina226 only) */
 static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
 			  ina226_show_interval, ina226_set_interval, 0);
@@ -401,6 +466,7 @@ static struct attribute *ina2xx_attrs[] = {
 	&sensor_dev_attr_curr1_input.dev_attr.attr,
 	&sensor_dev_attr_power1_input.dev_attr.attr,
 	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
+	&sensor_dev_attr_max_expected_current.dev_attr.attr,
 	NULL,
 };
 
@@ -453,6 +519,21 @@ static int ina2xx_probe(struct i2c_client *client,
 
 	data->rshunt = val;
 
+	if (of_property_read_u32(dev->of_node, "max-expected-current",
+				 &val) < 0) {
+		struct ina2xx_platform_data *pdata =
+		    dev_get_platdata(&client->dev);
+
+		if (pdata && pdata->max_mA != 0)
+			val = pdata->max_mA;
+		else
+			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
+	}
+
+	ret = set_max_expected_current(data, val);
+	if (ret < 0)
+		return ret;
+
 	ina2xx_regmap_config.max_register = data->config->registers;
 
 	data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
-- 
2.7.4

Re: [PATCH 2/4] hwmon: (ina2xx) Make max expected current configurable

[Guenter Roeck]

On 09/28/2017 05:50 AM, Maciej Purski wrote:
> Max expected current is used for calculating calibration register value,
> Current LSB and Power LSB according to equations found in ina datasheet.
> Max expected current is now implicitly set to default value,
> which is 2^15, thanks to which Current LSB is equal to 1 mA and
> Power LSB is equal to 20000 uW or 25000 uW depending on ina model.
> 
> Make max expected current configurable, just like it's already done
> with shunt resistance: from device tree, platform_data or later
> from sysfs. On each max_expected_current change, calculate new values
> for Current LSB and Power LSB. According to datasheet Current LSB should
> be calculated by dividing max expected current by 2^15, as values read
> from device registers are in this case 16-bit integers. Power LSB
> is calculated by multiplying Current LSB by a factor, which is defined
> in ina documentation.
> 
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
>   drivers/hwmon/ina2xx.c | 105 +++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 93 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 62e38fa..d956013 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -80,6 +80,8 @@
>   /* common attrs, ina226 attrs and NULL */
>   #define INA2XX_MAX_ATTRIBUTE_GROUPS	3
>   
> +#define INA2XX_MAX_EXPECTED_A_DEFAULT  (1 << 15)       /* current_lsb = 1 mA */
> +
>   /*
>    * Both bus voltage and shunt voltage conversion times for ina226 are set
>    * to 0b0100 on POR, which translates to 2200 microseconds in total.
> @@ -100,13 +102,16 @@ struct ina2xx_config {
>   	int shunt_div;
>   	int bus_voltage_shift;
>   	int bus_voltage_lsb;	/* uV */
> -	int power_lsb;		/* uW */
> +	int power_lsb_factor;
>   };
>   
>   struct ina2xx_data {
>   	const struct ina2xx_config *config;
>   
> -	long rshunt;
> +	long rshunt;				/* uOhms */
> +	unsigned int max_expected_current;	/* mA */
> +	int current_lsb;			/* uA */
> +	int power_lsb;				/* uW */
>   	struct mutex config_lock;
>   	struct regmap *regmap;
>   
> @@ -121,7 +126,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.shunt_div = 100,
>   		.bus_voltage_shift = 3,
>   		.bus_voltage_lsb = 4000,
> -		.power_lsb = 20000,
> +		.power_lsb_factor = 20,
>   	},
>   	[ina226] = {
>   		.config_default = INA226_CONFIG_DEFAULT,
> @@ -130,7 +135,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.shunt_div = 400,
>   		.bus_voltage_shift = 0,
>   		.bus_voltage_lsb = 1250,
> -		.power_lsb = 25000,
> +		.power_lsb_factor = 25,
>   	},
>   };
>   
> @@ -169,10 +174,17 @@ static u16 ina226_interval_to_reg(int interval)
>   	return INA226_SHIFT_AVG(avg_bits);
>   }
>   
> +/*
> + * Calculate calibration value according to equation 1 in ina226 datasheet
> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> + * Current LSB is in uA and RShunt is in uOhms, so in order to keep
> + * calibration value scaled RShunt must be converted to mOhms.
> + */
>   static int ina2xx_calibrate(struct ina2xx_data *data)
>   {
> +	int r_shunt = DIV_ROUND_CLOSEST(data->rshunt, 1000);
>   	u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
> -				    data->rshunt);
> +				    data->current_lsb * r_shunt);
>   
>   	return regmap_write(data->regmap, INA2XX_CALIBRATION, val);
>   }
> @@ -187,13 +199,28 @@ static int ina2xx_init(struct ina2xx_data *data)
>   	if (ret < 0)
>   		return ret;
>   
> -	/*
> -	 * Set current LSB to 1mA, shunt is in uOhms
> -	 * (equation 13 in datasheet).
> -	 */
>   	return ina2xx_calibrate(data);
>   }
>   
> +/*
> + * Set max_expected_current (mA) and calculate current_lsb (uA),
> + * according to equation 2 in ina226 datasheet. Power LSB is calculated
> + * by multiplying Current LSB by a given factor, which may vary depending
> + * on ina version.
> + */
> +static int set_max_expected_current(struct ina2xx_data *data, unsigned int val)
> +{
> +	if (val <= 0 || val > data->config->calibration_factor)
> +		return -EINVAL;
> +
> +	data->max_expected_current = val;
> +	data->current_lsb = DIV_ROUND_CLOSEST(data->max_expected_current * 1000,
> +					      1 << 15);
> +	data->power_lsb = data->current_lsb * data->config->power_lsb_factor;
> +
> +	return 0;
> +}
> +
>   static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
> @@ -268,11 +295,11 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
>   		val = DIV_ROUND_CLOSEST(val, 1000);
>   		break;
>   	case INA2XX_POWER:
> -		val = regval * data->config->power_lsb;
> +		val = regval * data->power_lsb;
>   		break;
>   	case INA2XX_CURRENT:
> -		/* signed register, LSB=1mA (selected), in mA */
> -		val = (s16)regval;
> +		val = (s16)regval * data->current_lsb;
> +		val = DIV_ROUND_CLOSEST(val, 1000);
>   		break;
>   	case INA2XX_CALIBRATION:
>   		val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
> @@ -369,6 +396,39 @@ static ssize_t ina226_show_interval(struct device *dev,
>   	return snprintf(buf, PAGE_SIZE, "%d\n", ina226_reg_to_interval(regval));
>   }
>   
> +static ssize_t ina2xx_max_expected_current_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", data->max_expected_current);
> +}
> +
> +static ssize_t ina2xx_max_expected_current_set(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t len)
> +{
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul((const char *) buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = set_max_expected_current(data, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Update the Calibration register */
> +	ret = ina2xx_calibrate(data);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
>   /* shunt voltage */
>   static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
>   			  INA2XX_SHUNT_VOLTAGE);
> @@ -390,6 +450,11 @@ static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR,
>   			  ina2xx_show_value, ina2xx_set_shunt,
>   			  INA2XX_CALIBRATION);
>   
> +/* max expected current */
> +static SENSOR_DEVICE_ATTR(max_expected_current, S_IRUGO | S_IWUSR,
> +			  ina2xx_max_expected_current_show,
> +			  ina2xx_max_expected_current_set, 0);
> +
>   /* update interval (ina226 only) */
>   static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
>   			  ina226_show_interval, ina226_set_interval, 0);
> @@ -401,6 +466,7 @@ static struct attribute *ina2xx_attrs[] = {
>   	&sensor_dev_attr_curr1_input.dev_attr.attr,
>   	&sensor_dev_attr_power1_input.dev_attr.attr,
>   	&sensor_dev_attr_shunt_resistor.dev_attr.attr,
> +	&sensor_dev_attr_max_expected_current.dev_attr.attr,

We do have standard attributes (presumably currX_max or possibly currX_crit).
Please use them, or explain in detail why you don't.

Guenter

>   	NULL,
>   };
>   
> @@ -453,6 +519,21 @@ static int ina2xx_probe(struct i2c_client *client,
>   
>   	data->rshunt = val;
>   
> +	if (of_property_read_u32(dev->of_node, "max-expected-current",
> +				 &val) < 0) {
> +		struct ina2xx_platform_data *pdata =
> +		    dev_get_platdata(&client->dev);
> +
> +		if (pdata && pdata->max_mA != 0)
> +			val = pdata->max_mA;
> +		else
> +			val = INA2XX_MAX_EXPECTED_A_DEFAULT;
> +	}
> +
> +	ret = set_max_expected_current(data, val);
> +	if (ret < 0)
> +		return ret;
> +
>   	ina2xx_regmap_config.max_register = data->config->registers;
>   
>   	data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
> 

[PATCH 3/4] dt-bindings: hwmon: Add max-expected-current property to ina2xx

[Maciej Purski]

Add optional max expected current property which allows calibrating
the ina sensor in order to achieve requested precision. Document
the changes in Documentation/hwmon/ina2xx.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 Documentation/devicetree/bindings/hwmon/ina2xx.txt | 4 +++-
 Documentation/hwmon/ina2xx                         | 9 +++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/ina2xx.txt b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
index 02af0d9..25ba372 100644
--- a/Documentation/devicetree/bindings/hwmon/ina2xx.txt
+++ b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
@@ -14,11 +14,13 @@ Optional properties:
 
 - shunt-resistor
 	Shunt resistor value in micro-Ohm
-
+- max-expected-current
+	Max expected current value in mA
 Example:
 
 ina220@44 {
 	compatible = "ti,ina220";
 	reg = <0x44>;
 	shunt-resistor = <1000>;
+	max-expected-current = <3000>;
 };
diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
index cfd31d9..e9ca838 100644
--- a/Documentation/hwmon/ina2xx
+++ b/Documentation/hwmon/ina2xx
@@ -51,10 +51,11 @@ INA230 and INA231 are high or low side current shunt and power monitors
 with an I2C interface. The chips monitor both a shunt voltage drop and
 bus supply voltage.
 
-The shunt value in micro-ohms can be set via platform data or device tree at
-compile-time or via the shunt_resistor attribute in sysfs at run-time. Please
-refer to the Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings
-if the device tree is used.
+The shunt value in micro-ohms and max expected current in mA can be set
+via platform data or device tree at compile-time or via the shunt_resistor
+and max_expected_current attributes in sysfs at run-time. Please refer to the
+Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings if the
+device tree is used.
 
 Additionally ina226 supports update_interval attribute as described in
 Documentation/hwmon/sysfs-interface. Internally the interval is the sum of
-- 
2.7.4

Re: [PATCH 3/4] dt-bindings: hwmon: Add max-expected-current property to ina2xx

[Jonathan Cameron]

On Thu, 28 Sep 2017 14:50:14 +0200
Maciej Purski <m.purski@samsung.com> wrote:

> Add optional max expected current property which allows calibrating
> the ina sensor in order to achieve requested precision. Document
> the changes in Documentation/hwmon/ina2xx.
> 

This is introducing new generic devicetree bindings..  
I'm not totally sure we want to do this rather than having a
manufacturer specific binding...  I don't have a good feeling for
how common this will be in other devices.

If it's generic, then this isn't necessarily the place to define it.

Jonathan

> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
>  Documentation/devicetree/bindings/hwmon/ina2xx.txt | 4 +++-
>  Documentation/hwmon/ina2xx                         | 9 +++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ina2xx.txt b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> index 02af0d9..25ba372 100644
> --- a/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> +++ b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> @@ -14,11 +14,13 @@ Optional properties:
>  
>  - shunt-resistor
>  	Shunt resistor value in micro-Ohm
> -
> +- max-expected-current
> +	Max expected current value in mA
>  Example:
>  
>  ina220@44 {
>  	compatible = "ti,ina220";
>  	reg = <0x44>;
>  	shunt-resistor = <1000>;
> +	max-expected-current = <3000>;
>  };
> diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
> index cfd31d9..e9ca838 100644
> --- a/Documentation/hwmon/ina2xx
> +++ b/Documentation/hwmon/ina2xx
> @@ -51,10 +51,11 @@ INA230 and INA231 are high or low side current shunt and power monitors
>  with an I2C interface. The chips monitor both a shunt voltage drop and
>  bus supply voltage.
>  
> -The shunt value in micro-ohms can be set via platform data or device tree at
> -compile-time or via the shunt_resistor attribute in sysfs at run-time. Please
> -refer to the Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings
> -if the device tree is used.
> +The shunt value in micro-ohms and max expected current in mA can be set
> +via platform data or device tree at compile-time or via the shunt_resistor
> +and max_expected_current attributes in sysfs at run-time. Please refer to the
> +Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings if the
> +device tree is used.
>  
>  Additionally ina226 supports update_interval attribute as described in
>  Documentation/hwmon/sysfs-interface. Internally the interval is the sum of

Re: [PATCH 3/4] dt-bindings: hwmon: Add max-expected-current property to ina2xx

[Maciej Purski]

On 10/01/2017 12:31 PM, Jonathan Cameron wrote:
> On Thu, 28 Sep 2017 14:50:14 +0200
> Maciej Purski <m.purski@samsung.com> wrote:
> 
>> Add optional max expected current property which allows calibrating
>> the ina sensor in order to achieve requested precision. Document
>> the changes in Documentation/hwmon/ina2xx.
>>
> 
> This is introducing new generic devicetree bindings..
> I'm not totally sure we want to do this rather than having a
> manufacturer specific binding...  I don't have a good feeling for
> how common this will be in other devices.
> 
> If it's generic, then this isn't necessarily the place to define it.
> 
> Jonathan
>

I agree, this should be manufacturer-specific property. Thanks.

>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>> ---
>>   Documentation/devicetree/bindings/hwmon/ina2xx.txt | 4 +++-
>>   Documentation/hwmon/ina2xx                         | 9 +++++----
>>   2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/ina2xx.txt b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
>> index 02af0d9..25ba372 100644
>> --- a/Documentation/devicetree/bindings/hwmon/ina2xx.txt
>> +++ b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
>> @@ -14,11 +14,13 @@ Optional properties:
>>   
>>   - shunt-resistor
>>   	Shunt resistor value in micro-Ohm
>> -
>> +- max-expected-current
>> +	Max expected current value in mA
>>   Example:
>>   
>>   ina220@44 {
>>   	compatible = "ti,ina220";
>>   	reg = <0x44>;
>>   	shunt-resistor = <1000>;
>> +	max-expected-current = <3000>;
>>   };
>> diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
>> index cfd31d9..e9ca838 100644
>> --- a/Documentation/hwmon/ina2xx
>> +++ b/Documentation/hwmon/ina2xx
>> @@ -51,10 +51,11 @@ INA230 and INA231 are high or low side current shunt and power monitors
>>   with an I2C interface. The chips monitor both a shunt voltage drop and
>>   bus supply voltage.
>>   
>> -The shunt value in micro-ohms can be set via platform data or device tree at
>> -compile-time or via the shunt_resistor attribute in sysfs at run-time. Please
>> -refer to the Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings
>> -if the device tree is used.
>> +The shunt value in micro-ohms and max expected current in mA can be set
>> +via platform data or device tree at compile-time or via the shunt_resistor
>> +and max_expected_current attributes in sysfs at run-time. Please refer to the
>> +Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings if the
>> +device tree is used.
>>   
>>   Additionally ina226 supports update_interval attribute as described in
>>   Documentation/hwmon/sysfs-interface. Internally the interval is the sum of
> 
> 
> 
> 

Re: [PATCH 3/4] dt-bindings: hwmon: Add max-expected-current property to ina2xx

[Rob Herring]

On Thu, Oct 05, 2017 at 05:15:07PM +0200, Maciej Purski wrote:
> On 10/01/2017 12:31 PM, Jonathan Cameron wrote:
> > On Thu, 28 Sep 2017 14:50:14 +0200
> > Maciej Purski <m.purski@samsung.com> wrote:
> > 
> > > Add optional max expected current property which allows calibrating
> > > the ina sensor in order to achieve requested precision. Document
> > > the changes in Documentation/hwmon/ina2xx.
> > > 
> > 
> > This is introducing new generic devicetree bindings..
> > I'm not totally sure we want to do this rather than having a
> > manufacturer specific binding...  I don't have a good feeling for
> > how common this will be in other devices.
> > 
> > If it's generic, then this isn't necessarily the place to define it.
> > 
> > Jonathan
> > 
> 
> I agree, this should be manufacturer-specific property. Thanks.
> 
> > > Signed-off-by: Maciej Purski <m.purski@samsung.com>
> > > ---
> > >   Documentation/devicetree/bindings/hwmon/ina2xx.txt | 4 +++-
> > >   Documentation/hwmon/ina2xx                         | 9 +++++----
> > >   2 files changed, 8 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwmon/ina2xx.txt b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> > > index 02af0d9..25ba372 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> > > +++ b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> > > @@ -14,11 +14,13 @@ Optional properties:
> > >   - shunt-resistor
> > >   	Shunt resistor value in micro-Ohm
> > > -
> > > +- max-expected-current
> > > +	Max expected current value in mA

Also needs a unit suffix as defined in property-units.txt.

> > >   Example:
> > >   ina220@44 {
> > >   	compatible = "ti,ina220";
> > >   	reg = <0x44>;
> > >   	shunt-resistor = <1000>;
> > > +	max-expected-current = <3000>;
> > >   };
> > > diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
> > > index cfd31d9..e9ca838 100644
> > > --- a/Documentation/hwmon/ina2xx
> > > +++ b/Documentation/hwmon/ina2xx
> > > @@ -51,10 +51,11 @@ INA230 and INA231 are high or low side current shunt and power monitors
> > >   with an I2C interface. The chips monitor both a shunt voltage drop and
> > >   bus supply voltage.
> > > -The shunt value in micro-ohms can be set via platform data or device tree at
> > > -compile-time or via the shunt_resistor attribute in sysfs at run-time. Please
> > > -refer to the Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings
> > > -if the device tree is used.
> > > +The shunt value in micro-ohms and max expected current in mA can be set
> > > +via platform data or device tree at compile-time or via the shunt_resistor
> > > +and max_expected_current attributes in sysfs at run-time. Please refer to the
> > > +Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings if the
> > > +device tree is used.
> > >   Additionally ina226 supports update_interval attribute as described in
> > >   Documentation/hwmon/sysfs-interface. Internally the interval is the sum of
> > 
> > 
> > 
> > 

[PATCH 4/4] ARM: dts: Add max-expected-current properties for ina231 in Odroid XU3

[Maciej Purski]

Set max-expected-current to values required by the documentation,
in order to achieve desired precision.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
---
 arch/arm/boot/dts/exynos5422-odroidxu3.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
index 9ed6564..aa19374 100644
--- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
+++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
@@ -28,6 +28,7 @@
 		compatible = "ti,ina231";
 		reg = <0x40>;
 		shunt-resistor = <10000>;
+		max-expected-current = <9000>;
 	};
 
 	/* memory: VDD_MEM */
@@ -35,6 +36,7 @@
 		compatible = "ti,ina231";
 		reg = <0x41>;
 		shunt-resistor = <10000>;
+		max-expected-current = <3000>;
 	};
 
 	/* GPU: VDD_G3D */
@@ -42,6 +44,7 @@
 		compatible = "ti,ina231";
 		reg = <0x44>;
 		shunt-resistor = <10000>;
+		max-expected-current = <5000>;
 	};
 
 	/* A7 cluster: VDD_KFC */
@@ -49,6 +52,7 @@
 		compatible = "ti,ina231";
 		reg = <0x45>;
 		shunt-resistor = <10000>;
+		max-expected-current = <2000>;
 	};
 };
 
-- 
2.7.4