lkml.org 
[lkml]   [2017]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [2/3] iio: adc: ina2xx: Adhere to documented ABI, use Ohm instead of uOhm
From
Date


On 10/01/2017 09:48 PM, Stefan Brüns wrote:
> According to the ABI documentation, the shunt resistor value should be
> specificied in Ohm. As this is also used/documented for the MAX9611,
> use the same for the INA2xx driver.
>
> This poses an ABI break for anyone actually altering the shunt value
> through the sysfs interface, it does not alter the default value nor
> a value set from the devicetree.
>
> Minor change: Fix comment, 1mA is 10^-3A.
>

I have just a minor issue. There could be an inconsistency with units as in my
patch I make current_lsb adjustable and I need it to be in uA (it used to be
hardcoded as 1 mA so to achieve better precision we need smaller units). So in
order to keep calibration register properly scaled, I convert uOhms to mOhms on
each set_calibration(). So if both my changes and your changes were applied, on
each shunt_resistore_store we would be performing multiplication by 10^6 and
then in set_calibration() division by 10^3 which seems odd to me.

I guess we could keep it as shunt_resistor_ohms instead of shunt_resistor_uohm.
We could avoid performing division on each shunt_resistor_show() and perform
multiplication by 10^3 only once in set_calibration() on each
shunt_resistore_store(). We could then change the default value and perform
division only on probing, when reading the shunt_resistance from device tree.

There are many other options. It's not a major issue so maybe we could leave it
as it is or you could suggest some changes in my patch.

> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>
> drivers/iio/adc/ina2xx-adc.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 361fb4e459d5..1930f853e8c5 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -127,7 +127,7 @@ 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_uohm;
> int avg;
> int int_time_vbus; /* Bus voltage integration time uS */
> int int_time_vshunt; /* Shunt voltage integration time uS */
> @@ -444,7 +444,7 @@ 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.
> + * of 1.0 x10-3. The only remaining parameter is RShunt.
> * 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
> @@ -453,7 +453,7 @@ static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
> static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
> {
> u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> - chip->shunt_resistor);
> + chip->shunt_resistor_uohm);
>
> return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> }
> @@ -463,7 +463,7 @@ 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;
> + chip->shunt_resistor_uohm = val;
>
> return 0;
> }
> @@ -473,8 +473,9 @@ static ssize_t ina2xx_shunt_resistor_show(struct device *dev,
> char *buf)
> {
> struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> + int vals[2] = { chip->shunt_resistor_uohm, 1000000 };
>
> - return sprintf(buf, "%d\n", chip->shunt_resistor);
> + return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
> }
>
> static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> @@ -482,14 +483,13 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> const char *buf, size_t len)
> {
> struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> - unsigned long val;
> - int ret;
> + int val, val_fract, ret;
>
> - ret = kstrtoul((const char *) buf, 10, &val);
> + ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract);
> if (ret)
> return ret;
>
> - ret = set_shunt_resistor(chip, val);
> + ret = set_shunt_resistor(chip, val * 1000000 + val_fract);
> if (ret)
> return ret;
>
>

\
 
 \ /
  Last update: 2017-10-09 11:30    [W:0.113 / U:1.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site