| Message ID | 1511364735-16818-3-git-send-email-m.purski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/22/2017 07:32 AM, Maciej Purski wrote: > Calibration register is used for calculating current register in > hardware according to datasheet: > current = shunt_volt * calib_register / 2048 (ina 226) > current = shunt_volt * calib_register / 4096 (ina 219) > > Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in > order to avoid truncation error and provide best precision allowed > by shunt_voltage measurement. Make current scale value follow changes > of shunt_resistor from sysfs as calib_register value is now fixed. > > Power_lsb value should also follow shunt_resistor changes as stated in > datasheet: > power_lsb = 25 * current_lsb (ina 226) > power_lsb = 20 * current_lsb (ina 219) > > Signed-off-by: Maciej Purski <m.purski@samsung.com> Setting the calibration register to a specific value may optimize precision, but limits the supported value range, which is the whole point of providing a calibration register. What am I missing here ? Guenter > --- > drivers/hwmon/ina2xx.c | 87 +++++++++++++++++++++++++++++--------------------- > 1 file changed, 50 insertions(+), 37 deletions(-) > > diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c > index 62e38fa..e362a93 100644 > --- a/drivers/hwmon/ina2xx.c > +++ b/drivers/hwmon/ina2xx.c > @@ -95,18 +95,20 @@ enum ina2xx_ids { ina219, ina226 }; > > struct ina2xx_config { > u16 config_default; > - int calibration_factor; > + int calibration_value; > int registers; > 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 current_lsb_uA; > + long power_lsb_uW; > struct mutex config_lock; > struct regmap *regmap; > > @@ -116,21 +118,21 @@ struct ina2xx_data { > static const struct ina2xx_config ina2xx_config[] = { > [ina219] = { > .config_default = INA219_CONFIG_DEFAULT, > - .calibration_factor = 40960000, > + .calibration_value = 4096, > .registers = INA219_REGISTERS, > .shunt_div = 100, > .bus_voltage_shift = 3, > .bus_voltage_lsb = 4000, > - .power_lsb = 20000, > + .power_lsb_factor = 20, > }, > [ina226] = { > .config_default = INA226_CONFIG_DEFAULT, > - .calibration_factor = 5120000, > + .calibration_value = 2048, > .registers = INA226_REGISTERS, > .shunt_div = 400, > .bus_voltage_shift = 0, > .bus_voltage_lsb = 1250, > - .power_lsb = 25000, > + .power_lsb_factor = 25, > }, > }; > > @@ -169,12 +171,16 @@ static u16 ina226_interval_to_reg(int interval) > return INA226_SHIFT_AVG(avg_bits); > } > > +/* > + * Calibration register is set to the best value, which eliminates > + * truncation errors on calculating current register in hardware. > + * According to datasheet (eq. 3) the best values are 2048 for > + * ina226 and 4096 for ina219. They are hardcoded as calibration_value. > + */ > static int ina2xx_calibrate(struct ina2xx_data *data) > { > - u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor, > - data->rshunt); > - > - return regmap_write(data->regmap, INA2XX_CALIBRATION, val); > + return regmap_write(data->regmap, INA2XX_CALIBRATION, > + data->config->calibration_value); > } > > /* > @@ -187,10 +193,6 @@ 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); > } > > @@ -268,15 +270,15 @@ 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_uW; > break; > case INA2XX_CURRENT: > - /* signed register, LSB=1mA (selected), in mA */ > - val = (s16)regval; > + /* signed register, result in mA */ > + val = regval * data->current_lsb_uA; > + val = DIV_ROUND_CLOSEST(val, 1000); > break; > case INA2XX_CALIBRATION: > - val = DIV_ROUND_CLOSEST(data->config->calibration_factor, > - regval); > + val = regval; > break; > default: > /* programmer goofed */ > @@ -304,9 +306,32 @@ static ssize_t ina2xx_show_value(struct device *dev, > ina2xx_get_value(data, attr->index, regval)); > } > > -static ssize_t ina2xx_set_shunt(struct device *dev, > - struct device_attribute *da, > - const char *buf, size_t count) > +/* > + * In order to keep calibration register value fixed, the product > + * of current_lsb and shunt_resistor should also be fixed and equal > + * to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order > + * to keep the scale. > + */ > +static int ina2xx_set_shunt(struct ina2xx_data *data, long val) > +{ > + unsigned int dividend = DIV_ROUND_CLOSEST(1000000000, > + data->config->shunt_div); > + if (val <= 0 || val > dividend) > + return -EINVAL; > + > + mutex_lock(&data->config_lock); > + data->rshunt = val; > + data->current_lsb_uA = DIV_ROUND_CLOSEST(dividend, val); > + data->power_lsb_uW = data->config->power_lsb_factor * > + data->current_lsb_uA; > + mutex_unlock(&data->config_lock); > + > + return 0; > +} > + > +static ssize_t ina2xx_store_shunt(struct device *dev, > + struct device_attribute *da, > + const char *buf, size_t count) > { > unsigned long val; > int status; > @@ -316,18 +341,9 @@ static ssize_t ina2xx_set_shunt(struct device *dev, > if (status < 0) > return status; > > - if (val == 0 || > - /* Values greater than the calibration factor make no sense. */ > - val > data->config->calibration_factor) > - return -EINVAL; > - > - mutex_lock(&data->config_lock); > - data->rshunt = val; > - status = ina2xx_calibrate(data); > - mutex_unlock(&data->config_lock); > + status = ina2xx_set_shunt(data, val); > if (status < 0) > return status; > - > return count; > } > > @@ -387,7 +403,7 @@ static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL, > > /* shunt resistance */ > static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR, > - ina2xx_show_value, ina2xx_set_shunt, > + ina2xx_show_value, ina2xx_store_shunt, > INA2XX_CALIBRATION); > > /* update interval (ina226 only) */ > @@ -448,10 +464,7 @@ static int ina2xx_probe(struct i2c_client *client, > val = INA2XX_RSHUNT_DEFAULT; > } > > - if (val <= 0 || val > data->config->calibration_factor) > - return -ENODEV; > - > - data->rshunt = val; > + ina2xx_set_shunt(data, val); > > ina2xx_regmap_config.max_register = data->config->registers; > > -- 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
On Saturday, November 25, 2017 7:50:16 PM CET Guenter Roeck wrote: > On 11/22/2017 07:32 AM, Maciej Purski wrote: > > Calibration register is used for calculating current register in > > hardware according to datasheet: > > current = shunt_volt * calib_register / 2048 (ina 226) > > current = shunt_volt * calib_register / 4096 (ina 219) > > > > Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in > > order to avoid truncation error and provide best precision allowed > > by shunt_voltage measurement. Make current scale value follow changes > > of shunt_resistor from sysfs as calib_register value is now fixed. > > > > Power_lsb value should also follow shunt_resistor changes as stated in > > datasheet: > > power_lsb = 25 * current_lsb (ina 226) > > power_lsb = 20 * current_lsb (ina 219) > > > > Signed-off-by: Maciej Purski <m.purski@samsung.com> > > Setting the calibration register to a specific value may optimize precision, > but limits the supported value range, which is the whole point of providing > a calibration register. What am I missing here ? For the current register, any different calibration register value is completely useless - smaller values just truncate the register value, larger values adds noise in the lsbs. Both registers (current/shunt voltage) are 16bit, and nothing is going to change that. There is a *very* small allowed power operating area where scaling down would be of any use: 1. Bus voltage exceeds 25 (20) Volts 2. Shunt voltage is about 82 (320) mV In this case the power register overflows. More specifically, if: (shunt_voltage [mv] / 81.92) * (bus_voltage [V] / 25) > 1 Maximum allowed bus voltage is 36 V resp 26 V. (ina226/219). Kind regards, Stefan
On Wed, Nov 22, 2017 at 04:32:15PM +0100, Maciej Purski wrote: > Calibration register is used for calculating current register in > hardware according to datasheet: > current = shunt_volt * calib_register / 2048 (ina 226) > current = shunt_volt * calib_register / 4096 (ina 219) > > Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in > order to avoid truncation error and provide best precision allowed > by shunt_voltage measurement. Make current scale value follow changes > of shunt_resistor from sysfs as calib_register value is now fixed. > > Power_lsb value should also follow shunt_resistor changes as stated in > datasheet: > power_lsb = 25 * current_lsb (ina 226) > power_lsb = 20 * current_lsb (ina 219) > > Signed-off-by: Maciej Purski <m.purski@samsung.com> Applied. Thanks, Guenter > --- > drivers/hwmon/ina2xx.c | 87 +++++++++++++++++++++++++++++--------------------- > 1 file changed, 50 insertions(+), 37 deletions(-) > > diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c > index 62e38fa..e362a93 100644 > --- a/drivers/hwmon/ina2xx.c > +++ b/drivers/hwmon/ina2xx.c > @@ -95,18 +95,20 @@ enum ina2xx_ids { ina219, ina226 }; > > struct ina2xx_config { > u16 config_default; > - int calibration_factor; > + int calibration_value; > int registers; > 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 current_lsb_uA; > + long power_lsb_uW; > struct mutex config_lock; > struct regmap *regmap; > > @@ -116,21 +118,21 @@ struct ina2xx_data { > static const struct ina2xx_config ina2xx_config[] = { > [ina219] = { > .config_default = INA219_CONFIG_DEFAULT, > - .calibration_factor = 40960000, > + .calibration_value = 4096, > .registers = INA219_REGISTERS, > .shunt_div = 100, > .bus_voltage_shift = 3, > .bus_voltage_lsb = 4000, > - .power_lsb = 20000, > + .power_lsb_factor = 20, > }, > [ina226] = { > .config_default = INA226_CONFIG_DEFAULT, > - .calibration_factor = 5120000, > + .calibration_value = 2048, > .registers = INA226_REGISTERS, > .shunt_div = 400, > .bus_voltage_shift = 0, > .bus_voltage_lsb = 1250, > - .power_lsb = 25000, > + .power_lsb_factor = 25, > }, > }; > > @@ -169,12 +171,16 @@ static u16 ina226_interval_to_reg(int interval) > return INA226_SHIFT_AVG(avg_bits); > } > > +/* > + * Calibration register is set to the best value, which eliminates > + * truncation errors on calculating current register in hardware. > + * According to datasheet (eq. 3) the best values are 2048 for > + * ina226 and 4096 for ina219. They are hardcoded as calibration_value. > + */ > static int ina2xx_calibrate(struct ina2xx_data *data) > { > - u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor, > - data->rshunt); > - > - return regmap_write(data->regmap, INA2XX_CALIBRATION, val); > + return regmap_write(data->regmap, INA2XX_CALIBRATION, > + data->config->calibration_value); > } > > /* > @@ -187,10 +193,6 @@ 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); > } > > @@ -268,15 +270,15 @@ 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_uW; > break; > case INA2XX_CURRENT: > - /* signed register, LSB=1mA (selected), in mA */ > - val = (s16)regval; > + /* signed register, result in mA */ > + val = regval * data->current_lsb_uA; > + val = DIV_ROUND_CLOSEST(val, 1000); > break; > case INA2XX_CALIBRATION: > - val = DIV_ROUND_CLOSEST(data->config->calibration_factor, > - regval); > + val = regval; > break; > default: > /* programmer goofed */ > @@ -304,9 +306,32 @@ static ssize_t ina2xx_show_value(struct device *dev, > ina2xx_get_value(data, attr->index, regval)); > } > > -static ssize_t ina2xx_set_shunt(struct device *dev, > - struct device_attribute *da, > - const char *buf, size_t count) > +/* > + * In order to keep calibration register value fixed, the product > + * of current_lsb and shunt_resistor should also be fixed and equal > + * to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order > + * to keep the scale. > + */ > +static int ina2xx_set_shunt(struct ina2xx_data *data, long val) > +{ > + unsigned int dividend = DIV_ROUND_CLOSEST(1000000000, > + data->config->shunt_div); > + if (val <= 0 || val > dividend) > + return -EINVAL; > + > + mutex_lock(&data->config_lock); > + data->rshunt = val; > + data->current_lsb_uA = DIV_ROUND_CLOSEST(dividend, val); > + data->power_lsb_uW = data->config->power_lsb_factor * > + data->current_lsb_uA; > + mutex_unlock(&data->config_lock); > + > + return 0; > +} > + > +static ssize_t ina2xx_store_shunt(struct device *dev, > + struct device_attribute *da, > + const char *buf, size_t count) > { > unsigned long val; > int status; > @@ -316,18 +341,9 @@ static ssize_t ina2xx_set_shunt(struct device *dev, > if (status < 0) > return status; > > - if (val == 0 || > - /* Values greater than the calibration factor make no sense. */ > - val > data->config->calibration_factor) > - return -EINVAL; > - > - mutex_lock(&data->config_lock); > - data->rshunt = val; > - status = ina2xx_calibrate(data); > - mutex_unlock(&data->config_lock); > + status = ina2xx_set_shunt(data, val); > if (status < 0) > return status; > - > return count; > } > > @@ -387,7 +403,7 @@ static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL, > > /* shunt resistance */ > static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR, > - ina2xx_show_value, ina2xx_set_shunt, > + ina2xx_show_value, ina2xx_store_shunt, > INA2XX_CALIBRATION); > > /* update interval (ina226 only) */ > @@ -448,10 +464,7 @@ static int ina2xx_probe(struct i2c_client *client, > val = INA2XX_RSHUNT_DEFAULT; > } > > - if (val <= 0 || val > data->config->calibration_factor) > - return -ENODEV; > - > - data->rshunt = val; > + ina2xx_set_shunt(data, val); > > ina2xx_regmap_config.max_register = data->config->registers; > -- 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
diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c index 62e38fa..e362a93 100644 --- a/drivers/hwmon/ina2xx.c +++ b/drivers/hwmon/ina2xx.c @@ -95,18 +95,20 @@ enum ina2xx_ids { ina219, ina226 }; struct ina2xx_config { u16 config_default; - int calibration_factor; + int calibration_value; int registers; 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 current_lsb_uA; + long power_lsb_uW; struct mutex config_lock; struct regmap *regmap; @@ -116,21 +118,21 @@ struct ina2xx_data { static const struct ina2xx_config ina2xx_config[] = { [ina219] = { .config_default = INA219_CONFIG_DEFAULT, - .calibration_factor = 40960000, + .calibration_value = 4096, .registers = INA219_REGISTERS, .shunt_div = 100, .bus_voltage_shift = 3, .bus_voltage_lsb = 4000, - .power_lsb = 20000, + .power_lsb_factor = 20, }, [ina226] = { .config_default = INA226_CONFIG_DEFAULT, - .calibration_factor = 5120000, + .calibration_value = 2048, .registers = INA226_REGISTERS, .shunt_div = 400, .bus_voltage_shift = 0, .bus_voltage_lsb = 1250, - .power_lsb = 25000, + .power_lsb_factor = 25, }, }; @@ -169,12 +171,16 @@ static u16 ina226_interval_to_reg(int interval) return INA226_SHIFT_AVG(avg_bits); } +/* + * Calibration register is set to the best value, which eliminates + * truncation errors on calculating current register in hardware. + * According to datasheet (eq. 3) the best values are 2048 for + * ina226 and 4096 for ina219. They are hardcoded as calibration_value. + */ static int ina2xx_calibrate(struct ina2xx_data *data) { - u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor, - data->rshunt); - - return regmap_write(data->regmap, INA2XX_CALIBRATION, val); + return regmap_write(data->regmap, INA2XX_CALIBRATION, + data->config->calibration_value); } /* @@ -187,10 +193,6 @@ 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); } @@ -268,15 +270,15 @@ 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_uW; break; case INA2XX_CURRENT: - /* signed register, LSB=1mA (selected), in mA */ - val = (s16)regval; + /* signed register, result in mA */ + val = regval * data->current_lsb_uA; + val = DIV_ROUND_CLOSEST(val, 1000); break; case INA2XX_CALIBRATION: - val = DIV_ROUND_CLOSEST(data->config->calibration_factor, - regval); + val = regval; break; default: /* programmer goofed */ @@ -304,9 +306,32 @@ static ssize_t ina2xx_show_value(struct device *dev, ina2xx_get_value(data, attr->index, regval)); } -static ssize_t ina2xx_set_shunt(struct device *dev, - struct device_attribute *da, - const char *buf, size_t count) +/* + * In order to keep calibration register value fixed, the product + * of current_lsb and shunt_resistor should also be fixed and equal + * to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order + * to keep the scale. + */ +static int ina2xx_set_shunt(struct ina2xx_data *data, long val) +{ + unsigned int dividend = DIV_ROUND_CLOSEST(1000000000, + data->config->shunt_div); + if (val <= 0 || val > dividend) + return -EINVAL; + + mutex_lock(&data->config_lock); + data->rshunt = val; + data->current_lsb_uA = DIV_ROUND_CLOSEST(dividend, val); + data->power_lsb_uW = data->config->power_lsb_factor * + data->current_lsb_uA; + mutex_unlock(&data->config_lock); + + return 0; +} + +static ssize_t ina2xx_store_shunt(struct device *dev, + struct device_attribute *da, + const char *buf, size_t count) { unsigned long val; int status; @@ -316,18 +341,9 @@ static ssize_t ina2xx_set_shunt(struct device *dev, if (status < 0) return status; - if (val == 0 || - /* Values greater than the calibration factor make no sense. */ - val > data->config->calibration_factor) - return -EINVAL; - - mutex_lock(&data->config_lock); - data->rshunt = val; - status = ina2xx_calibrate(data); - mutex_unlock(&data->config_lock); + status = ina2xx_set_shunt(data, val); if (status < 0) return status; - return count; } @@ -387,7 +403,7 @@ static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL, /* shunt resistance */ static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR, - ina2xx_show_value, ina2xx_set_shunt, + ina2xx_show_value, ina2xx_store_shunt, INA2XX_CALIBRATION); /* update interval (ina226 only) */ @@ -448,10 +464,7 @@ static int ina2xx_probe(struct i2c_client *client, val = INA2XX_RSHUNT_DEFAULT; } - if (val <= 0 || val > data->config->calibration_factor) - return -ENODEV; - - data->rshunt = val; + ina2xx_set_shunt(data, val); ina2xx_regmap_config.max_register = data->config->registers;
Calibration register is used for calculating current register in hardware according to datasheet: current = shunt_volt * calib_register / 2048 (ina 226) current = shunt_volt * calib_register / 4096 (ina 219) Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in order to avoid truncation error and provide best precision allowed by shunt_voltage measurement. Make current scale value follow changes of shunt_resistor from sysfs as calib_register value is now fixed. Power_lsb value should also follow shunt_resistor changes as stated in datasheet: power_lsb = 25 * current_lsb (ina 226) power_lsb = 20 * current_lsb (ina 219) Signed-off-by: Maciej Purski <m.purski@samsung.com> --- drivers/hwmon/ina2xx.c | 87 +++++++++++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 37 deletions(-)