[PATCH 1/3] iio: ina2xx: re-instate a sysfs show/store for the shunt resistor value

[PATCH 1/3] iio: ina2xx: re-instate a sysfs show/store for the shunt resistor value

[Marc Titinger]

Different probe modules use different resistor values. The front-end
application may read a probe ID (from eeprom) and set the shunt value
accordingly.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/iio/adc/ina2xx-adc.c | 52 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 615c203..fe42872 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -106,7 +106,7 @@ struct ina2xx_chip_info {
 	struct task_struct *task;
 	const struct ina2xx_config *config;
 	struct mutex state_lock;
-	long rshunt;
+	unsigned int shunt_resistor;
 	int avg;
 	s64 prev_ns;	/* track buffer capture time, check for underruns*/
 	int int_time_vbus; /* Bus voltage integration time uS */
@@ -353,6 +353,42 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
 	return len;
 }
 
+static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
+{
+	if (val <= 0 || val > chip->config->calibration_factor)
+		return -EINVAL;
+
+	chip->shunt_resistor = val;
+	return 0;
+}
+
+static ssize_t ina2xx_shunt_resistor_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->shunt_resistor);
+}
+
+static ssize_t ina2xx_shunt_resistor_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_shunt_resistor(chip, val);
+	if (ret)
+		return ret;
+
+	return len;
+}
 
 #define INA2XX_CHAN(_type, _index, _address) { \
 	.type = (_type), \
@@ -547,9 +583,14 @@ static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
 		       ina2xx_allow_async_readout_show,
 		       ina2xx_allow_async_readout_store, 0);
 
+static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
+		       ina2xx_shunt_resistor_show,
+		       ina2xx_shunt_resistor_store, 0);
+
 static struct attribute *ina2xx_attributes[] = {
 	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
 	&iio_const_attr_integration_time_available.dev_attr.attr,
+	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
 	NULL,
 };
 
@@ -581,7 +622,7 @@ static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
 	 * to the user for now.
 	 */
 	regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
-			    chip->rshunt);
+			    chip->shunt_resistor);
 
 	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
 }
@@ -614,10 +655,9 @@ static int ina2xx_probe(struct i2c_client *client,
 			val = INA2XX_RSHUNT_DEFAULT;
 	}
 
-	if (val <= 0 || val > chip->config->calibration_factor)
-		return -ENODEV;
-
-	chip->rshunt = val;
+	ret = set_shunt_resistor(chip, val);
+	if (ret)
+		return ret;
 
 	mutex_init(&chip->state_lock);
 
-- 
1.9.1

[PATCH 2/3] iio: ina2xx: give the capture kthread a more useful name string.

[Marc Titinger]

  PID  PPID USER     STAT   VSZ %VSZ %CPU COMMAND
  144     2 root     DW       0   0%  33% [ina226:1-8800us]
  141     2 root     DW       0   0%  25% [ina226:0-8800us]
   40     2 root     SW       0   0%  15% [irq/156-4802a00]
  147     2 root     DW       0   0%   7% [ina226:2-8800us]
  145     1 root     S     1236   0%   6% dd if /dev/iio:device1 of /dev/null
  148     1 root     S     1236   0%   4% dd if /dev/iio:device2 of /dev/null
  149   137 root     R     1244   0%   3% top -d 1
  142     1 root     S     1236   0%   2% dd if /dev/iio:device0 of /dev/null

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/iio/adc/ina2xx-adc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index fe42872..99afa6e 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -542,7 +542,8 @@ int ina2xx_buffer_enable(struct iio_dev *indio_dev)
 	chip->prev_ns = iio_get_time_ns();
 
 	chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
-				 "ina2xx-%uus", sampling_us);
+				 "%s:%d-%uus", indio_dev->name, indio_dev->id,
+				 sampling_us);
 
 	return PTR_ERR_OR_ZERO(chip->task);
 }
-- 
1.9.1

[PATCH 3/3] iio: ina2xx: add support for CHAN_INFO_SCALE

[Marc Titinger]

Provide client apps with the scales to apply to the register values
read from the software buffer.

Follow the ABI documentation so that values are in milli-unit after scales
are applied.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/iio/adc/ina2xx-adc.c | 85 +++++++++++++++++++++-----------------------
 1 file changed, 41 insertions(+), 44 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 99afa6e..98939ba 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -82,6 +82,11 @@ static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
 	return (reg != INA2XX_CONFIG);
 }
 
+static inline bool is_signed_reg(unsigned int reg)
+{
+	return (reg == INA2XX_SHUNT_VOLTAGE) || (reg == INA2XX_CURRENT);
+}
+
 static const struct regmap_config ina2xx_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 16,
@@ -133,43 +138,6 @@ static const struct ina2xx_config ina2xx_config[] = {
 		    },
 };
 
-static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
-			    unsigned int regval, int *val, int *uval)
-{
-	*val = 0;
-
-	switch (reg) {
-	case INA2XX_SHUNT_VOLTAGE:
-		/* signed register */
-		*uval = DIV_ROUND_CLOSEST((s16) regval,
-					  chip->config->shunt_div);
-		return IIO_VAL_INT_PLUS_MICRO;
-
-	case INA2XX_BUS_VOLTAGE:
-		*uval = (regval >> chip->config->bus_voltage_shift)
-			* chip->config->bus_voltage_lsb;
-		*val = *uval / 1000000;
-		*uval = *uval % 1000000;
-		return IIO_VAL_INT_PLUS_MICRO;
-
-	case INA2XX_POWER:
-		*uval = regval * chip->config->power_lsb;
-		*val = *uval / 1000000;
-		*uval = *uval % 1000000;
-		return IIO_VAL_INT_PLUS_MICRO;
-
-	case INA2XX_CURRENT:
-		/* signed register, LSB=1mA (selected), in mA */
-		*uval = (s16) regval * 1000;
-		return IIO_VAL_INT_PLUS_MICRO;
-
-	default:
-		/* programmer goofed */
-		WARN_ON_ONCE(1);
-	}
-	return -EINVAL;
-}
-
 static int ina2xx_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long mask)
@@ -184,7 +152,12 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		return ina2xx_get_value(chip, chan->address, regval, val, val2);
+		if (is_signed_reg(chan->address))
+			*val = (s16) regval;
+		else
+			*val  = regval;
+
+		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		*val = chip->avg;
@@ -208,11 +181,34 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 
-	default:
-		return -EINVAL;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->address) {
+		case INA2XX_SHUNT_VOLTAGE:
+			/* processed (mV) = raw*1000/shunt_div */
+			*val2 = chip->config->shunt_div;
+			*val = 1000;
+			return IIO_VAL_FRACTIONAL;
+
+		case INA2XX_BUS_VOLTAGE:
+			/* processed (mV) = raw*lsb (uV) / (1000 << shift) */
+			*val = chip->config->bus_voltage_lsb;
+			*val2 = 1000 << chip->config->bus_voltage_shift;
+			return IIO_VAL_FRACTIONAL;
+
+		case INA2XX_POWER:
+			/* processed (mW) = raw*lsb (uW) / 1000 */
+			*val = chip->config->power_lsb;
+			*val2 = 1000;
+			return IIO_VAL_FRACTIONAL;
+
+		case INA2XX_CURRENT:
+			/* processed (mA) = raw (mA) */
+			*val = 1;
+			return IIO_VAL_INT;
+		}
 	}
 
-	return 0;
+	return -EINVAL;
 }
 
 /*
@@ -395,7 +391,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
 	.address = (_address), \
 	.indexed = 1, \
 	.channel = (_index), \
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)|BIT(IIO_CHAN_INFO_SCALE), \
 	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
 				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
 	.scan_index = (_index), \
@@ -403,7 +399,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
 		.sign = 'u', \
 		.realbits = 16, \
 		.storagebits = 16, \
-		.endianness = IIO_BE, \
+		.endianness = IIO_LE, \
 	} \
 }
 
@@ -417,13 +413,14 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
 	.indexed = 1, \
 	.channel = (_index), \
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+			      BIT(IIO_CHAN_INFO_SCALE) | \
 			      BIT(IIO_CHAN_INFO_INT_TIME), \
 	.scan_index = (_index), \
 	.scan_type = { \
 		.sign = 'u', \
 		.realbits = 16, \
 		.storagebits = 16, \
-		.endianness = IIO_BE, \
+		.endianness = IIO_LE, \
 	} \
 }
 
-- 
1.9.1

Re: [PATCH 1/3] iio: ina2xx: re-instate a sysfs show/store for the shunt resistor value

[Jonathan Cameron]

On 11/12/15 16:49, Marc Titinger wrote:
> Different probe modules use different resistor values. The front-end
> application may read a probe ID (from eeprom) and set the shunt value
> accordingly.
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
Fair enough.

Applied.
> ---
>  drivers/iio/adc/ina2xx-adc.c | 52 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 615c203..fe42872 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -106,7 +106,7 @@ struct ina2xx_chip_info {
>  	struct task_struct *task;
>  	const struct ina2xx_config *config;
>  	struct mutex state_lock;
> -	long rshunt;
> +	unsigned int shunt_resistor;
>  	int avg;
>  	s64 prev_ns;	/* track buffer capture time, check for underruns*/
>  	int int_time_vbus; /* Bus voltage integration time uS */
> @@ -353,6 +353,42 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
>  	return len;
>  }
>  
> +static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
> +{
> +	if (val <= 0 || val > chip->config->calibration_factor)
> +		return -EINVAL;
> +
> +	chip->shunt_resistor = val;
> +	return 0;
> +}
> +
> +static ssize_t ina2xx_shunt_resistor_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->shunt_resistor);
> +}
> +
> +static ssize_t ina2xx_shunt_resistor_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_shunt_resistor(chip, val);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
>  
>  #define INA2XX_CHAN(_type, _index, _address) { \
>  	.type = (_type), \
> @@ -547,9 +583,14 @@ static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
>  		       ina2xx_allow_async_readout_show,
>  		       ina2xx_allow_async_readout_store, 0);
>  
> +static IIO_DEVICE_ATTR(in_shunt_resistor, S_IRUGO | S_IWUSR,
> +		       ina2xx_shunt_resistor_show,
> +		       ina2xx_shunt_resistor_store, 0);
> +
>  static struct attribute *ina2xx_attributes[] = {
>  	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>  	&iio_const_attr_integration_time_available.dev_attr.attr,
> +	&iio_dev_attr_in_shunt_resistor.dev_attr.attr,
>  	NULL,
>  };
>  
> @@ -581,7 +622,7 @@ static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
>  	 * to the user for now.
>  	 */
>  	regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> -			    chip->rshunt);
> +			    chip->shunt_resistor);
>  
>  	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>  }
> @@ -614,10 +655,9 @@ static int ina2xx_probe(struct i2c_client *client,
>  			val = INA2XX_RSHUNT_DEFAULT;
>  	}
>  
> -	if (val <= 0 || val > chip->config->calibration_factor)
> -		return -ENODEV;
> -
> -	chip->rshunt = val;
> +	ret = set_shunt_resistor(chip, val);
> +	if (ret)
> +		return ret;
>  
>  	mutex_init(&chip->state_lock);
>  
> 

Re: [PATCH 2/3] iio: ina2xx: give the capture kthread a more useful name string.

[Jonathan Cameron]

On 11/12/15 16:49, Marc Titinger wrote:
>   PID  PPID USER     STAT   VSZ %VSZ %CPU COMMAND
>   144     2 root     DW       0   0%  33% [ina226:1-8800us]
>   141     2 root     DW       0   0%  25% [ina226:0-8800us]
>    40     2 root     SW       0   0%  15% [irq/156-4802a00]
>   147     2 root     DW       0   0%   7% [ina226:2-8800us]
>   145     1 root     S     1236   0%   6% dd if /dev/iio:device1 of /dev/null
>   148     1 root     S     1236   0%   4% dd if /dev/iio:device2 of /dev/null
>   149   137 root     R     1244   0%   3% top -d 1
>   142     1 root     S     1236   0%   2% dd if /dev/iio:device0 of /dev/null
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
Could have slightly improved your patch, but saying what it was before in the
description but otherwise seems reasonable so applied to the
togreg branch of iio.git - initially pushed out as testing for the
autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ina2xx-adc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index fe42872..99afa6e 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -542,7 +542,8 @@ int ina2xx_buffer_enable(struct iio_dev *indio_dev)
>  	chip->prev_ns = iio_get_time_ns();
>  
>  	chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
> -				 "ina2xx-%uus", sampling_us);
> +				 "%s:%d-%uus", indio_dev->name, indio_dev->id,
> +				 sampling_us);
>  
>  	return PTR_ERR_OR_ZERO(chip->task);
>  }
> 

Re: [PATCH 3/3] iio: ina2xx: add support for CHAN_INFO_SCALE

[Jonathan Cameron]

On 11/12/15 16:49, Marc Titinger wrote:
> Provide client apps with the scales to apply to the register values
> read from the software buffer.
> 
> Follow the ABI documentation so that values are in milli-unit after scales
> are applied.
Umm. The below looks like it is doing rather more than this..

There is an endian switch!   I'm going to assume that is correct
for now and merge it into the original patch (rather than as a follow
up).  If this is wrong and should not be here shout quickly and loudly!

Will take the rest of this patch as is though I would much have
prefered that this one was rolled into the original driver as
I hadn't taken that when you sent this change...

Actually never mind I'll smash it into the original driver as git
seems to be happy to handle that bit of reordering and I haven't
pushed out yet.

So applied as a fixup to the original driver patch.
Hmm. a few mor bits came up build testing this - such as lack of static on the
the buffer enable / disable functions.

Please check nothing went wrong in my making various minor changes.
I've done basic build tests but obviously that doesn't catch everything!

Jonathan

> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  drivers/iio/adc/ina2xx-adc.c | 85 +++++++++++++++++++++-----------------------
>  1 file changed, 41 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 99afa6e..98939ba 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -82,6 +82,11 @@ static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
>  	return (reg != INA2XX_CONFIG);
>  }
>  
> +static inline bool is_signed_reg(unsigned int reg)
> +{
> +	return (reg == INA2XX_SHUNT_VOLTAGE) || (reg == INA2XX_CURRENT);
> +}
> +
>  static const struct regmap_config ina2xx_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 16,
> @@ -133,43 +138,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>  		    },
>  };
>  
> -static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
> -			    unsigned int regval, int *val, int *uval)
> -{
> -	*val = 0;
> -
> -	switch (reg) {
> -	case INA2XX_SHUNT_VOLTAGE:
> -		/* signed register */
> -		*uval = DIV_ROUND_CLOSEST((s16) regval,
> -					  chip->config->shunt_div);
> -		return IIO_VAL_INT_PLUS_MICRO;
> -
> -	case INA2XX_BUS_VOLTAGE:
> -		*uval = (regval >> chip->config->bus_voltage_shift)
> -			* chip->config->bus_voltage_lsb;
> -		*val = *uval / 1000000;
> -		*uval = *uval % 1000000;
> -		return IIO_VAL_INT_PLUS_MICRO;
> -
> -	case INA2XX_POWER:
> -		*uval = regval * chip->config->power_lsb;
> -		*val = *uval / 1000000;
> -		*uval = *uval % 1000000;
> -		return IIO_VAL_INT_PLUS_MICRO;
> -
> -	case INA2XX_CURRENT:
> -		/* signed register, LSB=1mA (selected), in mA */
> -		*uval = (s16) regval * 1000;
> -		return IIO_VAL_INT_PLUS_MICRO;
> -
> -	default:
> -		/* programmer goofed */
> -		WARN_ON_ONCE(1);
> -	}
> -	return -EINVAL;
> -}
> -
>  static int ina2xx_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long mask)
> @@ -184,7 +152,12 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		return ina2xx_get_value(chip, chan->address, regval, val, val2);
> +		if (is_signed_reg(chan->address))
> +			*val = (s16) regval;
> +		else
> +			*val  = regval;
> +
> +		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		*val = chip->avg;
> @@ -208,11 +181,34 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>  
>  		return IIO_VAL_INT;
>  
> -	default:
> -		return -EINVAL;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->address) {
> +		case INA2XX_SHUNT_VOLTAGE:
> +			/* processed (mV) = raw*1000/shunt_div */
> +			*val2 = chip->config->shunt_div;
> +			*val = 1000;
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case INA2XX_BUS_VOLTAGE:
> +			/* processed (mV) = raw*lsb (uV) / (1000 << shift) */
> +			*val = chip->config->bus_voltage_lsb;
> +			*val2 = 1000 << chip->config->bus_voltage_shift;
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case INA2XX_POWER:
> +			/* processed (mW) = raw*lsb (uW) / 1000 */
> +			*val = chip->config->power_lsb;
> +			*val2 = 1000;
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case INA2XX_CURRENT:
> +			/* processed (mA) = raw (mA) */
> +			*val = 1;
> +			return IIO_VAL_INT;
> +		}
>  	}
>  
> -	return 0;
> +	return -EINVAL;
>  }
>  
>  /*
> @@ -395,7 +391,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>  	.address = (_address), \
>  	.indexed = 1, \
>  	.channel = (_index), \
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)|BIT(IIO_CHAN_INFO_SCALE), \
Spacing wrong about the |
>  	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>  				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>  	.scan_index = (_index), \
> @@ -403,7 +399,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>  		.sign = 'u', \
>  		.realbits = 16, \
>  		.storagebits = 16, \
> -		.endianness = IIO_BE, \
> +		.endianness = IIO_LE, \
>  	} \
>  }
>  
> @@ -417,13 +413,14 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>  	.indexed = 1, \
>  	.channel = (_index), \
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			      BIT(IIO_CHAN_INFO_SCALE) | \
>  			      BIT(IIO_CHAN_INFO_INT_TIME), \
>  	.scan_index = (_index), \
>  	.scan_type = { \
>  		.sign = 'u', \
>  		.realbits = 16, \
>  		.storagebits = 16, \
> -		.endianness = IIO_BE, \
> +		.endianness = IIO_LE, \
>  	} \
>  }
>  
> 

Re: [PATCH 3/3] iio: ina2xx: add support for CHAN_INFO_SCALE

[Marc Titinger]

On 12/12/2015 18:14, Jonathan Cameron wrote:
> On 11/12/15 16:49, Marc Titinger wrote:
>> Provide client apps with the scales to apply to the register values
>> read from the software buffer.
>>
>> Follow the ABI documentation so that values are in milli-unit after scales
>> are applied.
> Umm. The below looks like it is doing rather more than this..
>
> There is an endian switch!   I'm going to assume that is correct
> for now and merge it into the original patch (rather than as a follow
> up).  If this is wrong and should not be here shout quickly and loudly!
>

I know...I think the endianess issue was not spotted in my tests until I 
had the scales coded-in: values in direct read mode were processed 
before, and hence ok.

The endianness hint was wrong in streamed mode, but I did only 
functional check so far. This is now tested with he sigrok-iio draft 
from my colleague and values are now fine with scales applied on the 
buffer samples.


> Will take the rest of this patch as is though I would much have
> prefered that this one was rolled into the original driver as
> I hadn't taken that when you sent this change...
>
> Actually never mind I'll smash it into the original driver as git
> seems to be happy to handle that bit of reordering and I haven't
> pushed out yet.

Yes thank you for doing that, it's the good option I think.

>
> So applied as a fixup to the original driver patch.
> Hmm. a few mor bits came up build testing this - such as lack of static on the
> the buffer enable / disable functions.
>
> Please check nothing went wrong in my making various minor changes.
> I've done basic build tests but obviously that doesn't catch everything!

Tested ok with iio utils (using iio_monitor for scales) and  and the 
sigrok-iio draft using 3 instances of the driver.

Many thanks,
Marc.


>
> Jonathan
>
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> ---
>>   drivers/iio/adc/ina2xx-adc.c | 85 +++++++++++++++++++++-----------------------
>>   1 file changed, 41 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>> index 99afa6e..98939ba 100644
>> --- a/drivers/iio/adc/ina2xx-adc.c
>> +++ b/drivers/iio/adc/ina2xx-adc.c
>> @@ -82,6 +82,11 @@ static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
>>   	return (reg != INA2XX_CONFIG);
>>   }
>>
>> +static inline bool is_signed_reg(unsigned int reg)
>> +{
>> +	return (reg == INA2XX_SHUNT_VOLTAGE) || (reg == INA2XX_CURRENT);
>> +}
>> +
>>   static const struct regmap_config ina2xx_regmap_config = {
>>   	.reg_bits = 8,
>>   	.val_bits = 16,
>> @@ -133,43 +138,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>>   		    },
>>   };
>>
>> -static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
>> -			    unsigned int regval, int *val, int *uval)
>> -{
>> -	*val = 0;
>> -
>> -	switch (reg) {
>> -	case INA2XX_SHUNT_VOLTAGE:
>> -		/* signed register */
>> -		*uval = DIV_ROUND_CLOSEST((s16) regval,
>> -					  chip->config->shunt_div);
>> -		return IIO_VAL_INT_PLUS_MICRO;
>> -
>> -	case INA2XX_BUS_VOLTAGE:
>> -		*uval = (regval >> chip->config->bus_voltage_shift)
>> -			* chip->config->bus_voltage_lsb;
>> -		*val = *uval / 1000000;
>> -		*uval = *uval % 1000000;
>> -		return IIO_VAL_INT_PLUS_MICRO;
>> -
>> -	case INA2XX_POWER:
>> -		*uval = regval * chip->config->power_lsb;
>> -		*val = *uval / 1000000;
>> -		*uval = *uval % 1000000;
>> -		return IIO_VAL_INT_PLUS_MICRO;
>> -
>> -	case INA2XX_CURRENT:
>> -		/* signed register, LSB=1mA (selected), in mA */
>> -		*uval = (s16) regval * 1000;
>> -		return IIO_VAL_INT_PLUS_MICRO;
>> -
>> -	default:
>> -		/* programmer goofed */
>> -		WARN_ON_ONCE(1);
>> -	}
>> -	return -EINVAL;
>> -}
>> -
>>   static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>   			   struct iio_chan_spec const *chan,
>>   			   int *val, int *val2, long mask)
>> @@ -184,7 +152,12 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>   		if (ret < 0)
>>   			return ret;
>>
>> -		return ina2xx_get_value(chip, chan->address, regval, val, val2);
>> +		if (is_signed_reg(chan->address))
>> +			*val = (s16) regval;
>> +		else
>> +			*val  = regval;
>> +
>> +		return IIO_VAL_INT;
>>
>>   	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>   		*val = chip->avg;
>> @@ -208,11 +181,34 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>
>>   		return IIO_VAL_INT;
>>
>> -	default:
>> -		return -EINVAL;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->address) {
>> +		case INA2XX_SHUNT_VOLTAGE:
>> +			/* processed (mV) = raw*1000/shunt_div */
>> +			*val2 = chip->config->shunt_div;
>> +			*val = 1000;
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case INA2XX_BUS_VOLTAGE:
>> +			/* processed (mV) = raw*lsb (uV) / (1000 << shift) */
>> +			*val = chip->config->bus_voltage_lsb;
>> +			*val2 = 1000 << chip->config->bus_voltage_shift;
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case INA2XX_POWER:
>> +			/* processed (mW) = raw*lsb (uW) / 1000 */
>> +			*val = chip->config->power_lsb;
>> +			*val2 = 1000;
>> +			return IIO_VAL_FRACTIONAL;
>> +
>> +		case INA2XX_CURRENT:
>> +			/* processed (mA) = raw (mA) */
>> +			*val = 1;
>> +			return IIO_VAL_INT;
>> +		}
>>   	}
>>
>> -	return 0;
>> +	return -EINVAL;
>>   }
>>
>>   /*
>> @@ -395,7 +391,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>>   	.address = (_address), \
>>   	.indexed = 1, \
>>   	.channel = (_index), \
>> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)|BIT(IIO_CHAN_INFO_SCALE), \
> Spacing wrong about the |
>>   	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>>   				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>>   	.scan_index = (_index), \
>> @@ -403,7 +399,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>>   		.sign = 'u', \
>>   		.realbits = 16, \
>>   		.storagebits = 16, \
>> -		.endianness = IIO_BE, \
>> +		.endianness = IIO_LE, \
>>   	} \
>>   }
>>
>> @@ -417,13 +413,14 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>>   	.indexed = 1, \
>>   	.channel = (_index), \
>>   	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> +			      BIT(IIO_CHAN_INFO_SCALE) | \
>>   			      BIT(IIO_CHAN_INFO_INT_TIME), \
>>   	.scan_index = (_index), \
>>   	.scan_type = { \
>>   		.sign = 'u', \
>>   		.realbits = 16, \
>>   		.storagebits = 16, \
>> -		.endianness = IIO_BE, \
>> +		.endianness = IIO_LE, \
>>   	} \
>>   }
>>
>>
>