Groups > linux.kernel > #1290241 > unrolled thread

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

Started by	Jonathan Cameron <jic23@kernel.org>
First post	2015-12-12 18:20 +0100
Last post	2015-12-14 11:20 +0100
Articles	2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

  Re: [PATCH 3/3] iio: ina2xx: add support for CHAN_INFO_SCALE Jonathan Cameron <jic23@kernel.org> - 2015-12-12 18:20 +0100
    Re: [PATCH 3/3] iio: ina2xx: add support for CHAN_INFO_SCALE Marc Titinger <mtitinger@baylibre.com> - 2015-12-14 11:20 +0100

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

From	Jonathan Cameron <jic23@kernel.org>
Date	2015-12-12 18:20 +0100
Subject	Re: [PATCH 3/3] iio: ina2xx: add support for CHAN_INFO_SCALE
Message-ID	<qEVZT-50C-17@gated-at.bofh.it>

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, \
>  	} \
>  }
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1291049

From	Marc Titinger <mtitinger@baylibre.com>
Date	2015-12-14 11:20 +0100
Message-ID	<qFyoy-4Kh-23@gated-at.bofh.it>
In reply to	#1290241

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, \
>>   	} \
>>   }
>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

csiph-web

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

Contents

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

#1291049