diff for duplicates of <3656355.n9BamPbOAV@sbruens-linux> diff --git a/a/1.txt b/N1/1.txt index bbfdc88..e7442bd 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,197 +1,205 @@ -On Donnerstag, 7. Dezember 2017 13:02:47 CET 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) -> -> This is a part of the patchset: https://lkml.org/lkml/2017/11/22/394 -> -> Signed-off-by: Maciej Purski <m.purski@samsung.com> -> -> --- -> Changes in v2: -> - rebase on top of the latest next -> - remove a redundant variable - power_lsb_uW -> - fix comments -> --- -> drivers/iio/adc/ina2xx-adc.c | 58 -> +++++++++++++++++++++++--------------------- 1 file changed, 31 -> insertions(+), 27 deletions(-) -> -> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c -> index ddf8781..3e4972f 100644 -> --- a/drivers/iio/adc/ina2xx-adc.c -> +++ b/drivers/iio/adc/ina2xx-adc.c -> @@ -124,11 +124,11 @@ enum ina2xx_ids { ina219, ina226 }; -> -> struct ina2xx_config { -> u16 config_default; -> - int calibration_factor; -> + int calibration_value; -> int shunt_voltage_lsb; /* nV */ -> int bus_voltage_shift; /* position of lsb */ -> int bus_voltage_lsb; /* uV */ -> - int power_lsb; /* uW */ -> + int power_lsb_factor; -> enum ina2xx_ids chip_id; -> }; -> -> @@ -138,6 +138,7 @@ struct ina2xx_chip_info { -> const struct ina2xx_config *config; -> struct mutex state_lock; -> unsigned int shunt_resistor_uohm; -> + unsigned int current_lsb_uA; - -I don't think you need current_lsb_uA, see below ... - -> int avg; -> int int_time_vbus; /* Bus voltage integration time uS */ -> int int_time_vshunt; /* Shunt voltage integration time uS */ -> @@ -149,20 +150,20 @@ struct ina2xx_chip_info { -> static const struct ina2xx_config ina2xx_config[] = { -> [ina219] = { -> .config_default = INA219_CONFIG_DEFAULT, -> - .calibration_factor = 40960000, -> + .calibration_value = 4096, -> .shunt_voltage_lsb = 10000, -> .bus_voltage_shift = INA219_BUS_VOLTAGE_SHIFT, -> .bus_voltage_lsb = 4000, -> - .power_lsb = 20000, -> + .power_lsb_factor = 20, -> .chip_id = ina219, -> }, -> [ina226] = { -> .config_default = INA226_CONFIG_DEFAULT, -> - .calibration_factor = 5120000, -> + .calibration_value = 2048, -> .shunt_voltage_lsb = 2500, -> .bus_voltage_shift = 0, -> .bus_voltage_lsb = 1250, -> - .power_lsb = 25000, -> + .power_lsb_factor = 25, -> .chip_id = ina226, -> }, -> }; -> @@ -229,14 +230,16 @@ 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->config->power_lsb_factor * -> + chip->current_lsb_uA; -> *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_uA; -> + *val2 = 1000; - -.. this is the only place where current_lsb_uA is used ... - -> + return IIO_VAL_FRACTIONAL; -> } -> -> case IIO_CHAN_INFO_HARDWAREGAIN: -> @@ -541,28 +544,34 @@ 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-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 -> - * reading an EEPROM/Probe-type value. -> + * Calibration register is set to the best value, which eliminates -> + * truncation errors on calculating current register in hardware. -> + * According to datasheet (INA 226: eq. 3, INA219: eq. 4) the best values -> + * are 2048 for ina226 and 4096 for ina219. They are hardcoded as -> + * calibration_value. -> */ -> static int ina2xx_set_calibration(struct ina2xx_chip_info *chip) -> { -> - u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor, -> - chip->shunt_resistor_uohm); -> - -> - return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval); -> + return regmap_write(chip->regmap, INA2XX_CALIBRATION, -> + chip->config->calibration_value); -> } -> -> +/* -> + * 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 set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int -> val) { -> - if (val <= 0 || val > chip->config->calibration_factor) -> + unsigned int dividend = DIV_ROUND_CLOSEST(1000000000, -> + chip->config->shunt_div); -> + -> + if (val <= 0 || val > dividend) -> return -EINVAL; -> -> chip->shunt_resistor_uohm = val; -> + chip->current_lsb_uA = DIV_ROUND_CLOSEST(dividend, val); - -1st: shunt_div no longer exists, but shunt_voltage_lsb[nV], so this code does -not even compile ... - -This function does two things - range check and calculation of the -curren_lsb_uA auxiliary variable. - -Lets start with the second part: - -read_raw returns the IIO_VAL_FRACTIONAL -val / val2 = current_lsb_uA / 1000 (1) - - -from set_shunt_resistor(...): - -dividend = 1e9 / shunt_div (2) -current_lsb_uA = dividend / shunt_resistor_uohm (3) - -shunt_div no longer exists, but can be substituted: -shunt_div = 1e6 / shunt_voltage_lsb (4) - -substituting (2), (3) and (4) in (1), we end up with - -val / val2 = 1e9 / (shunt_div * shunt_resistor_uohm * 1000) - -=> val / val2 = 1e6 / ((1e6 / shunt_voltage_lsb) * shunt_resistor_uohm) -=> val / val2 = shunt_voltage_lsb / shunt_resistor_uohm -=> [mA] = [nV] / [uOhm] - -The auxiliary variable is gone, as are the two DIV_ROUND_CLOSEST calls. - - -Part 2, range check: - -lower bound is obviously 1 uOhm. The upper bound is only limited by -typeof(val2) and typeof(shunt_resistor_uohm), both are ints. - -so the range check can be simplified to - -if (val == 0 || val > INT_MAX) - return -EINVAL - -Kind regards, - -Stefan +On Donnerstag, 7. Dezember 2017 13:02:47 CET Maciej Purski wrote:=0A= +> Calibration register is used for calculating current register in=0A= +> hardware according to datasheet:=0A= +> current =3D shunt_volt * calib_register / 2048 (ina 226)=0A= +> current =3D shunt_volt * calib_register / 4096 (ina 219)=0A= +> =0A= +> Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in=0A= +> order to avoid truncation error and provide best precision allowed=0A= +> by shunt_voltage measurement. Make current scale value follow changes=0A= +> of shunt_resistor from sysfs as calib_register value is now fixed.=0A= +> =0A= +> Power_lsb value should also follow shunt_resistor changes as stated in=0A= +> datasheet:=0A= +> power_lsb =3D 25 * current_lsb (ina 226)=0A= +> power_lsb =3D 20 * current_lsb (ina 219)=0A= +> =0A= +> This is a part of the patchset: https://lkml.org/lkml/2017/11/22/394=0A= +> =0A= +> Signed-off-by: Maciej Purski <m.purski@samsung.com>=0A= +> =0A= +> ---=0A= +> Changes in v2:=0A= +> - rebase on top of the latest next=0A= +> - remove a redundant variable - power_lsb_uW=0A= +> - fix comments=0A= +> ---=0A= +> drivers/iio/adc/ina2xx-adc.c | 58=0A= +> +++++++++++++++++++++++--------------------- 1 file changed, 31=0A= +> insertions(+), 27 deletions(-)=0A= +> =0A= +> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c= +=0A= +> index ddf8781..3e4972f 100644=0A= +> --- a/drivers/iio/adc/ina2xx-adc.c=0A= +> +++ b/drivers/iio/adc/ina2xx-adc.c=0A= +> @@ -124,11 +124,11 @@ enum ina2xx_ids { ina219, ina226 };=0A= +> =0A= +> struct ina2xx_config {=0A= +> u16 config_default;=0A= +> - int calibration_factor;=0A= +> + int calibration_value;=0A= +> int shunt_voltage_lsb; /* nV */=0A= +> int bus_voltage_shift; /* position of lsb */=0A= +> int bus_voltage_lsb; /* uV */=0A= +> - int power_lsb; /* uW */=0A= +> + int power_lsb_factor;=0A= +> enum ina2xx_ids chip_id;=0A= +> };=0A= +> =0A= +> @@ -138,6 +138,7 @@ struct ina2xx_chip_info {=0A= +> const struct ina2xx_config *config;=0A= +> struct mutex state_lock;=0A= +> unsigned int shunt_resistor_uohm;=0A= +> + unsigned int current_lsb_uA;=0A= +=0A= +I don't think you need current_lsb_uA, see below ...=0A= +=0A= +> int avg;=0A= +> int int_time_vbus; /* Bus voltage integration time uS */=0A= +> int int_time_vshunt; /* Shunt voltage integration time uS */=0A= +> @@ -149,20 +150,20 @@ struct ina2xx_chip_info {=0A= +> static const struct ina2xx_config ina2xx_config[] =3D {=0A= +> [ina219] =3D {=0A= +> .config_default =3D INA219_CONFIG_DEFAULT,=0A= +> - .calibration_factor =3D 40960000,=0A= +> + .calibration_value =3D 4096,=0A= +> .shunt_voltage_lsb =3D 10000,=0A= +> .bus_voltage_shift =3D INA219_BUS_VOLTAGE_SHIFT,=0A= +> .bus_voltage_lsb =3D 4000,=0A= +> - .power_lsb =3D 20000,=0A= +> + .power_lsb_factor =3D 20,=0A= +> .chip_id =3D ina219,=0A= +> },=0A= +> [ina226] =3D {=0A= +> .config_default =3D INA226_CONFIG_DEFAULT,=0A= +> - .calibration_factor =3D 5120000,=0A= +> + .calibration_value =3D 2048,=0A= +> .shunt_voltage_lsb =3D 2500,=0A= +> .bus_voltage_shift =3D 0,=0A= +> .bus_voltage_lsb =3D 1250,=0A= +> - .power_lsb =3D 25000,=0A= +> + .power_lsb_factor =3D 25,=0A= +> .chip_id =3D ina226,=0A= +> },=0A= +> };=0A= +> @@ -229,14 +230,16 @@ static int ina2xx_read_raw(struct iio_dev *indio_de= +v,=0A= +> =0A= +> case INA2XX_POWER:=0A= +> /* processed (mW) =3D raw*lsb (uW) / 1000 */=0A= +> - *val =3D chip->config->power_lsb;=0A= +> + *val =3D chip->config->power_lsb_factor *=0A= +> + chip->current_lsb_uA;=0A= +> *val2 =3D 1000;=0A= +> return IIO_VAL_FRACTIONAL;=0A= +> =0A= +> case INA2XX_CURRENT:=0A= +> - /* processed (mA) =3D raw (mA) */=0A= +> - *val =3D 1;=0A= +> - return IIO_VAL_INT;=0A= +> + /* processed (mA) =3D raw*lsb (uA) / 1000 */=0A= +> + *val =3D chip->current_lsb_uA;=0A= +> + *val2 =3D 1000;=0A= +=0A= +... this is the only place where current_lsb_uA is used ...=0A= +=0A= +> + return IIO_VAL_FRACTIONAL;=0A= +> }=0A= +> =0A= +> case IIO_CHAN_INFO_HARDWAREGAIN:=0A= +> @@ -541,28 +544,34 @@ static ssize_t ina2xx_allow_async_readout_store(str= +uct=0A= +> device *dev, }=0A= +> =0A= +> /*=0A= +> - * Set current LSB to 1mA, shunt is in uOhms=0A= +> - * (equation 13 in datasheet). We hardcode a Current_LSB=0A= +> - * of 1.0 x10-3. The only remaining parameter is RShunt.=0A= +> - * There is no need to expose the CALIBRATION register=0A= +> - * to the user for now. But we need to reset this register=0A= +> - * if the user updates RShunt after driver init, e.g upon=0A= +> - * reading an EEPROM/Probe-type value.=0A= +> + * Calibration register is set to the best value, which eliminates=0A= +> + * truncation errors on calculating current register in hardware.=0A= +> + * According to datasheet (INA 226: eq. 3, INA219: eq. 4) the best value= +s=0A= +> + * are 2048 for ina226 and 4096 for ina219. They are hardcoded as=0A= +> + * calibration_value.=0A= +> */=0A= +> static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)=0A= +> {=0A= +> - u16 regval =3D DIV_ROUND_CLOSEST(chip->config->calibration_factor,=0A= +> - chip->shunt_resistor_uohm);=0A= +> -=0A= +> - return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);=0A= +> + return regmap_write(chip->regmap, INA2XX_CALIBRATION,=0A= +> + chip->config->calibration_value);=0A= +> }=0A= +> =0A= +> +/*=0A= +> + * In order to keep calibration register value fixed, the product=0A= +> + * of current_lsb and shunt_resistor should also be fixed and equal=0A= +> + * to shunt_voltage_lsb =3D 1 / shunt_div multiplied by 10^9 in order=0A= +> + * to keep the scale.=0A= +> + */=0A= +> static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned in= +t=0A= +> val) {=0A= +> - if (val <=3D 0 || val > chip->config->calibration_factor)=0A= +> + unsigned int dividend =3D DIV_ROUND_CLOSEST(1000000000,=0A= +> + chip->config->shunt_div);=0A= +> +=0A= +> + if (val <=3D 0 || val > dividend)=0A= +> return -EINVAL;=0A= +> =0A= +> chip->shunt_resistor_uohm =3D val;=0A= +> + chip->current_lsb_uA =3D DIV_ROUND_CLOSEST(dividend, val);=0A= +=0A= +1st: shunt_div no longer exists, but shunt_voltage_lsb[nV], so this code do= +es =0A= +not even compile ...=0A= +=0A= +This function does two things - range check and calculation of the =0A= +curren_lsb_uA auxiliary variable.=0A= +=0A= +Lets start with the second part:=0A= +=0A= +read_raw returns the IIO_VAL_FRACTIONAL=0A= +val / val2 =3D current_lsb_uA / 1000 (1)=0A= +=0A= +=0A= +from set_shunt_resistor(...):=0A= +=0A= +dividend =3D 1e9 / shunt_div (2)=0A= +current_lsb_uA =3D dividend / shunt_resistor_uohm (3)=0A= +=0A= +shunt_div no longer exists, but can be substituted:=0A= +shunt_div =3D 1e6 / shunt_voltage_lsb (4)=0A= +=0A= +substituting (2), (3) and (4) in (1), we end up with=0A= +=0A= +val / val2 =3D 1e9 / (shunt_div * shunt_resistor_uohm * 1000)=0A= +=0A= +=3D> val / val2 =3D 1e6 / ((1e6 / shunt_voltage_lsb) * shunt_resistor_uohm= +)=0A= +=3D> val / val2 =3D shunt_voltage_lsb / shunt_resistor_uohm=0A= +=3D> [mA] =3D [nV] / [uOhm]=0A= +=0A= +The auxiliary variable is gone, as are the two DIV_ROUND_CLOSEST calls.=0A= +=0A= +=0A= +Part 2, range check:=0A= +=0A= +lower bound is obviously 1 uOhm. The upper bound is only limited by=0A= +typeof(val2) and typeof(shunt_resistor_uohm), both are ints.=0A= +=0A= +so the range check can be simplified to=0A= +=0A= +if (val =3D=3D 0 || val > INT_MAX)=0A= + return -EINVAL=0A= +=0A= +Kind regards,=0A= +=0A= +Stefan=0A= + =0A= diff --git a/a/content_digest b/N1/content_digest index 27662b3..993dd17 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -17,202 +17,210 @@ " Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>\0" "\00:1\0" "b\0" - "On Donnerstag, 7. Dezember 2017 13:02:47 CET Maciej Purski wrote:\n" - "> Calibration register is used for calculating current register in\n" - "> hardware according to datasheet:\n" - "> current = shunt_volt * calib_register / 2048 (ina 226)\n" - "> current = shunt_volt * calib_register / 4096 (ina 219)\n" - "> \n" - "> Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in\n" - "> order to avoid truncation error and provide best precision allowed\n" - "> by shunt_voltage measurement. Make current scale value follow changes\n" - "> of shunt_resistor from sysfs as calib_register value is now fixed.\n" - "> \n" - "> Power_lsb value should also follow shunt_resistor changes as stated in\n" - "> datasheet:\n" - "> power_lsb = 25 * current_lsb (ina 226)\n" - "> power_lsb = 20 * current_lsb (ina 219)\n" - "> \n" - "> This is a part of the patchset: https://lkml.org/lkml/2017/11/22/394\n" - "> \n" - "> Signed-off-by: Maciej Purski <m.purski@samsung.com>\n" - "> \n" - "> ---\n" - "> Changes in v2:\n" - "> - rebase on top of the latest next\n" - "> - remove a redundant variable - power_lsb_uW\n" - "> - fix comments\n" - "> ---\n" - "> drivers/iio/adc/ina2xx-adc.c | 58\n" - "> +++++++++++++++++++++++--------------------- 1 file changed, 31\n" - "> insertions(+), 27 deletions(-)\n" - "> \n" - "> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c\n" - "> index ddf8781..3e4972f 100644\n" - "> --- a/drivers/iio/adc/ina2xx-adc.c\n" - "> +++ b/drivers/iio/adc/ina2xx-adc.c\n" - "> @@ -124,11 +124,11 @@ enum ina2xx_ids { ina219, ina226 };\n" - "> \n" - "> struct ina2xx_config {\n" - "> \tu16 config_default;\n" - "> -\tint calibration_factor;\n" - "> +\tint calibration_value;\n" - "> \tint shunt_voltage_lsb;\t/* nV */\n" - "> \tint bus_voltage_shift;\t/* position of lsb */\n" - "> \tint bus_voltage_lsb;\t/* uV */\n" - "> -\tint power_lsb;\t\t/* uW */\n" - "> +\tint power_lsb_factor;\n" - "> \tenum ina2xx_ids chip_id;\n" - "> };\n" - "> \n" - "> @@ -138,6 +138,7 @@ struct ina2xx_chip_info {\n" - "> \tconst struct ina2xx_config *config;\n" - "> \tstruct mutex state_lock;\n" - "> \tunsigned int shunt_resistor_uohm;\n" - "> +\tunsigned int current_lsb_uA;\n" - "\n" - "I don't think you need current_lsb_uA, see below ...\n" - "\n" - "> \tint avg;\n" - "> \tint int_time_vbus; /* Bus voltage integration time uS */\n" - "> \tint int_time_vshunt; /* Shunt voltage integration time uS */\n" - "> @@ -149,20 +150,20 @@ struct ina2xx_chip_info {\n" - "> static const struct ina2xx_config ina2xx_config[] = {\n" - "> \t[ina219] = {\n" - "> \t\t.config_default = INA219_CONFIG_DEFAULT,\n" - "> -\t\t.calibration_factor = 40960000,\n" - "> +\t\t.calibration_value = 4096,\n" - "> \t\t.shunt_voltage_lsb = 10000,\n" - "> \t\t.bus_voltage_shift = INA219_BUS_VOLTAGE_SHIFT,\n" - "> \t\t.bus_voltage_lsb = 4000,\n" - "> -\t\t.power_lsb = 20000,\n" - "> +\t\t.power_lsb_factor = 20,\n" - "> \t\t.chip_id = ina219,\n" - "> \t},\n" - "> \t[ina226] = {\n" - "> \t\t.config_default = INA226_CONFIG_DEFAULT,\n" - "> -\t\t.calibration_factor = 5120000,\n" - "> +\t\t.calibration_value = 2048,\n" - "> \t\t.shunt_voltage_lsb = 2500,\n" - "> \t\t.bus_voltage_shift = 0,\n" - "> \t\t.bus_voltage_lsb = 1250,\n" - "> -\t\t.power_lsb = 25000,\n" - "> +\t\t.power_lsb_factor = 25,\n" - "> \t\t.chip_id = ina226,\n" - "> \t},\n" - "> };\n" - "> @@ -229,14 +230,16 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,\n" - "> \n" - "> \t\tcase INA2XX_POWER:\n" - "> \t\t\t/* processed (mW) = raw*lsb (uW) / 1000 */\n" - "> -\t\t\t*val = chip->config->power_lsb;\n" - "> +\t\t\t*val = chip->config->power_lsb_factor *\n" - "> +\t\t\t chip->current_lsb_uA;\n" - "> \t\t\t*val2 = 1000;\n" - "> \t\t\treturn IIO_VAL_FRACTIONAL;\n" - "> \n" - "> \t\tcase INA2XX_CURRENT:\n" - "> -\t\t\t/* processed (mA) = raw (mA) */\n" - "> -\t\t\t*val = 1;\n" - "> -\t\t\treturn IIO_VAL_INT;\n" - "> +\t\t\t/* processed (mA) = raw*lsb (uA) / 1000 */\n" - "> +\t\t\t*val = chip->current_lsb_uA;\n" - "> +\t\t\t*val2 = 1000;\n" - "\n" - ".. this is the only place where current_lsb_uA is used ...\n" - "\n" - "> +\t\t\treturn IIO_VAL_FRACTIONAL;\n" - "> \t\t}\n" - "> \n" - "> \tcase IIO_CHAN_INFO_HARDWAREGAIN:\n" - "> @@ -541,28 +544,34 @@ static ssize_t ina2xx_allow_async_readout_store(struct\n" - "> device *dev, }\n" - "> \n" - "> /*\n" - "> - * Set current LSB to 1mA, shunt is in uOhms\n" - "> - * (equation 13 in datasheet). We hardcode a Current_LSB\n" - "> - * of 1.0 x10-3. The only remaining parameter is RShunt.\n" - "> - * There is no need to expose the CALIBRATION register\n" - "> - * to the user for now. But we need to reset this register\n" - "> - * if the user updates RShunt after driver init, e.g upon\n" - "> - * reading an EEPROM/Probe-type value.\n" - "> + * Calibration register is set to the best value, which eliminates\n" - "> + * truncation errors on calculating current register in hardware.\n" - "> + * According to datasheet (INA 226: eq. 3, INA219: eq. 4) the best values\n" - "> + * are 2048 for ina226 and 4096 for ina219. They are hardcoded as\n" - "> + * calibration_value.\n" - "> */\n" - "> static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)\n" - "> {\n" - "> -\tu16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,\n" - "> -\t\t\t\t chip->shunt_resistor_uohm);\n" - "> -\n" - "> -\treturn regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);\n" - "> +\treturn regmap_write(chip->regmap, INA2XX_CALIBRATION,\n" - "> +\t\t\t chip->config->calibration_value);\n" - "> }\n" - "> \n" - "> +/*\n" - "> + * In order to keep calibration register value fixed, the product\n" - "> + * of current_lsb and shunt_resistor should also be fixed and equal\n" - "> + * to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order\n" - "> + * to keep the scale.\n" - "> + */\n" - "> static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int\n" - "> val) {\n" - "> -\tif (val <= 0 || val > chip->config->calibration_factor)\n" - "> +\tunsigned int dividend = DIV_ROUND_CLOSEST(1000000000,\n" - "> +\t\t\t\t\t\t chip->config->shunt_div);\n" - "> +\n" - "> +\tif (val <= 0 || val > dividend)\n" - "> \t\treturn -EINVAL;\n" - "> \n" - "> \tchip->shunt_resistor_uohm = val;\n" - "> +\tchip->current_lsb_uA = DIV_ROUND_CLOSEST(dividend, val);\n" - "\n" - "1st: shunt_div no longer exists, but shunt_voltage_lsb[nV], so this code does \n" - "not even compile ...\n" - "\n" - "This function does two things - range check and calculation of the \n" - "curren_lsb_uA auxiliary variable.\n" - "\n" - "Lets start with the second part:\n" - "\n" - "read_raw returns the IIO_VAL_FRACTIONAL\n" - "val / val2 = current_lsb_uA / 1000 (1)\n" - "\n" - "\n" - "from set_shunt_resistor(...):\n" - "\n" - "dividend = 1e9 / shunt_div\t (2)\n" - "current_lsb_uA = dividend / shunt_resistor_uohm (3)\n" - "\n" - "shunt_div no longer exists, but can be substituted:\n" - "shunt_div = 1e6 / shunt_voltage_lsb (4)\n" - "\n" - "substituting (2), (3) and (4) in (1), we end up with\n" - "\n" - "val / val2 = 1e9 / (shunt_div * shunt_resistor_uohm * 1000)\n" - "\n" - "=> val / val2 = 1e6 / ((1e6 / shunt_voltage_lsb) * shunt_resistor_uohm)\n" - "=> val / val2 = shunt_voltage_lsb / shunt_resistor_uohm\n" - "=> [mA] = [nV] / [uOhm]\n" - "\n" - "The auxiliary variable is gone, as are the two DIV_ROUND_CLOSEST calls.\n" - "\n" - "\n" - "Part 2, range check:\n" - "\n" - "lower bound is obviously 1 uOhm. The upper bound is only limited by\n" - "typeof(val2) and typeof(shunt_resistor_uohm), both are ints.\n" - "\n" - "so the range check can be simplified to\n" - "\n" - "if (val == 0 || val > INT_MAX)\n" - "\treturn -EINVAL\n" - "\n" - "Kind regards,\n" - "\n" - Stefan + "On Donnerstag, 7. Dezember 2017 13:02:47 CET Maciej Purski wrote:=0A=\n" + "> Calibration register is used for calculating current register in=0A=\n" + "> hardware according to datasheet:=0A=\n" + "> current =3D shunt_volt * calib_register / 2048 (ina 226)=0A=\n" + "> current =3D shunt_volt * calib_register / 4096 (ina 219)=0A=\n" + "> =0A=\n" + "> Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in=0A=\n" + "> order to avoid truncation error and provide best precision allowed=0A=\n" + "> by shunt_voltage measurement. Make current scale value follow changes=0A=\n" + "> of shunt_resistor from sysfs as calib_register value is now fixed.=0A=\n" + "> =0A=\n" + "> Power_lsb value should also follow shunt_resistor changes as stated in=0A=\n" + "> datasheet:=0A=\n" + "> power_lsb =3D 25 * current_lsb (ina 226)=0A=\n" + "> power_lsb =3D 20 * current_lsb (ina 219)=0A=\n" + "> =0A=\n" + "> This is a part of the patchset: https://lkml.org/lkml/2017/11/22/394=0A=\n" + "> =0A=\n" + "> Signed-off-by: Maciej Purski <m.purski@samsung.com>=0A=\n" + "> =0A=\n" + "> ---=0A=\n" + "> Changes in v2:=0A=\n" + "> - rebase on top of the latest next=0A=\n" + "> - remove a redundant variable - power_lsb_uW=0A=\n" + "> - fix comments=0A=\n" + "> ---=0A=\n" + "> drivers/iio/adc/ina2xx-adc.c | 58=0A=\n" + "> +++++++++++++++++++++++--------------------- 1 file changed, 31=0A=\n" + "> insertions(+), 27 deletions(-)=0A=\n" + "> =0A=\n" + "> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c=\n" + "=0A=\n" + "> index ddf8781..3e4972f 100644=0A=\n" + "> --- a/drivers/iio/adc/ina2xx-adc.c=0A=\n" + "> +++ b/drivers/iio/adc/ina2xx-adc.c=0A=\n" + "> @@ -124,11 +124,11 @@ enum ina2xx_ids { ina219, ina226 };=0A=\n" + "> =0A=\n" + "> struct ina2xx_config {=0A=\n" + "> \tu16 config_default;=0A=\n" + "> -\tint calibration_factor;=0A=\n" + "> +\tint calibration_value;=0A=\n" + "> \tint shunt_voltage_lsb;\t/* nV */=0A=\n" + "> \tint bus_voltage_shift;\t/* position of lsb */=0A=\n" + "> \tint bus_voltage_lsb;\t/* uV */=0A=\n" + "> -\tint power_lsb;\t\t/* uW */=0A=\n" + "> +\tint power_lsb_factor;=0A=\n" + "> \tenum ina2xx_ids chip_id;=0A=\n" + "> };=0A=\n" + "> =0A=\n" + "> @@ -138,6 +138,7 @@ struct ina2xx_chip_info {=0A=\n" + "> \tconst struct ina2xx_config *config;=0A=\n" + "> \tstruct mutex state_lock;=0A=\n" + "> \tunsigned int shunt_resistor_uohm;=0A=\n" + "> +\tunsigned int current_lsb_uA;=0A=\n" + "=0A=\n" + "I don't think you need current_lsb_uA, see below ...=0A=\n" + "=0A=\n" + "> \tint avg;=0A=\n" + "> \tint int_time_vbus; /* Bus voltage integration time uS */=0A=\n" + "> \tint int_time_vshunt; /* Shunt voltage integration time uS */=0A=\n" + "> @@ -149,20 +150,20 @@ struct ina2xx_chip_info {=0A=\n" + "> static const struct ina2xx_config ina2xx_config[] =3D {=0A=\n" + "> \t[ina219] =3D {=0A=\n" + "> \t\t.config_default =3D INA219_CONFIG_DEFAULT,=0A=\n" + "> -\t\t.calibration_factor =3D 40960000,=0A=\n" + "> +\t\t.calibration_value =3D 4096,=0A=\n" + "> \t\t.shunt_voltage_lsb =3D 10000,=0A=\n" + "> \t\t.bus_voltage_shift =3D INA219_BUS_VOLTAGE_SHIFT,=0A=\n" + "> \t\t.bus_voltage_lsb =3D 4000,=0A=\n" + "> -\t\t.power_lsb =3D 20000,=0A=\n" + "> +\t\t.power_lsb_factor =3D 20,=0A=\n" + "> \t\t.chip_id =3D ina219,=0A=\n" + "> \t},=0A=\n" + "> \t[ina226] =3D {=0A=\n" + "> \t\t.config_default =3D INA226_CONFIG_DEFAULT,=0A=\n" + "> -\t\t.calibration_factor =3D 5120000,=0A=\n" + "> +\t\t.calibration_value =3D 2048,=0A=\n" + "> \t\t.shunt_voltage_lsb =3D 2500,=0A=\n" + "> \t\t.bus_voltage_shift =3D 0,=0A=\n" + "> \t\t.bus_voltage_lsb =3D 1250,=0A=\n" + "> -\t\t.power_lsb =3D 25000,=0A=\n" + "> +\t\t.power_lsb_factor =3D 25,=0A=\n" + "> \t\t.chip_id =3D ina226,=0A=\n" + "> \t},=0A=\n" + "> };=0A=\n" + "> @@ -229,14 +230,16 @@ static int ina2xx_read_raw(struct iio_dev *indio_de=\n" + "v,=0A=\n" + "> =0A=\n" + "> \t\tcase INA2XX_POWER:=0A=\n" + "> \t\t\t/* processed (mW) =3D raw*lsb (uW) / 1000 */=0A=\n" + "> -\t\t\t*val =3D chip->config->power_lsb;=0A=\n" + "> +\t\t\t*val =3D chip->config->power_lsb_factor *=0A=\n" + "> +\t\t\t chip->current_lsb_uA;=0A=\n" + "> \t\t\t*val2 =3D 1000;=0A=\n" + "> \t\t\treturn IIO_VAL_FRACTIONAL;=0A=\n" + "> =0A=\n" + "> \t\tcase INA2XX_CURRENT:=0A=\n" + "> -\t\t\t/* processed (mA) =3D raw (mA) */=0A=\n" + "> -\t\t\t*val =3D 1;=0A=\n" + "> -\t\t\treturn IIO_VAL_INT;=0A=\n" + "> +\t\t\t/* processed (mA) =3D raw*lsb (uA) / 1000 */=0A=\n" + "> +\t\t\t*val =3D chip->current_lsb_uA;=0A=\n" + "> +\t\t\t*val2 =3D 1000;=0A=\n" + "=0A=\n" + "... this is the only place where current_lsb_uA is used ...=0A=\n" + "=0A=\n" + "> +\t\t\treturn IIO_VAL_FRACTIONAL;=0A=\n" + "> \t\t}=0A=\n" + "> =0A=\n" + "> \tcase IIO_CHAN_INFO_HARDWAREGAIN:=0A=\n" + "> @@ -541,28 +544,34 @@ static ssize_t ina2xx_allow_async_readout_store(str=\n" + "uct=0A=\n" + "> device *dev, }=0A=\n" + "> =0A=\n" + "> /*=0A=\n" + "> - * Set current LSB to 1mA, shunt is in uOhms=0A=\n" + "> - * (equation 13 in datasheet). We hardcode a Current_LSB=0A=\n" + "> - * of 1.0 x10-3. The only remaining parameter is RShunt.=0A=\n" + "> - * There is no need to expose the CALIBRATION register=0A=\n" + "> - * to the user for now. But we need to reset this register=0A=\n" + "> - * if the user updates RShunt after driver init, e.g upon=0A=\n" + "> - * reading an EEPROM/Probe-type value.=0A=\n" + "> + * Calibration register is set to the best value, which eliminates=0A=\n" + "> + * truncation errors on calculating current register in hardware.=0A=\n" + "> + * According to datasheet (INA 226: eq. 3, INA219: eq. 4) the best value=\n" + "s=0A=\n" + "> + * are 2048 for ina226 and 4096 for ina219. They are hardcoded as=0A=\n" + "> + * calibration_value.=0A=\n" + "> */=0A=\n" + "> static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)=0A=\n" + "> {=0A=\n" + "> -\tu16 regval =3D DIV_ROUND_CLOSEST(chip->config->calibration_factor,=0A=\n" + "> -\t\t\t\t chip->shunt_resistor_uohm);=0A=\n" + "> -=0A=\n" + "> -\treturn regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);=0A=\n" + "> +\treturn regmap_write(chip->regmap, INA2XX_CALIBRATION,=0A=\n" + "> +\t\t\t chip->config->calibration_value);=0A=\n" + "> }=0A=\n" + "> =0A=\n" + "> +/*=0A=\n" + "> + * In order to keep calibration register value fixed, the product=0A=\n" + "> + * of current_lsb and shunt_resistor should also be fixed and equal=0A=\n" + "> + * to shunt_voltage_lsb =3D 1 / shunt_div multiplied by 10^9 in order=0A=\n" + "> + * to keep the scale.=0A=\n" + "> + */=0A=\n" + "> static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned in=\n" + "t=0A=\n" + "> val) {=0A=\n" + "> -\tif (val <=3D 0 || val > chip->config->calibration_factor)=0A=\n" + "> +\tunsigned int dividend =3D DIV_ROUND_CLOSEST(1000000000,=0A=\n" + "> +\t\t\t\t\t\t chip->config->shunt_div);=0A=\n" + "> +=0A=\n" + "> +\tif (val <=3D 0 || val > dividend)=0A=\n" + "> \t\treturn -EINVAL;=0A=\n" + "> =0A=\n" + "> \tchip->shunt_resistor_uohm =3D val;=0A=\n" + "> +\tchip->current_lsb_uA =3D DIV_ROUND_CLOSEST(dividend, val);=0A=\n" + "=0A=\n" + "1st: shunt_div no longer exists, but shunt_voltage_lsb[nV], so this code do=\n" + "es =0A=\n" + "not even compile ...=0A=\n" + "=0A=\n" + "This function does two things - range check and calculation of the =0A=\n" + "curren_lsb_uA auxiliary variable.=0A=\n" + "=0A=\n" + "Lets start with the second part:=0A=\n" + "=0A=\n" + "read_raw returns the IIO_VAL_FRACTIONAL=0A=\n" + "val / val2 =3D current_lsb_uA / 1000 (1)=0A=\n" + "=0A=\n" + "=0A=\n" + "from set_shunt_resistor(...):=0A=\n" + "=0A=\n" + "dividend =3D 1e9 / shunt_div\t (2)=0A=\n" + "current_lsb_uA =3D dividend / shunt_resistor_uohm (3)=0A=\n" + "=0A=\n" + "shunt_div no longer exists, but can be substituted:=0A=\n" + "shunt_div =3D 1e6 / shunt_voltage_lsb (4)=0A=\n" + "=0A=\n" + "substituting (2), (3) and (4) in (1), we end up with=0A=\n" + "=0A=\n" + "val / val2 =3D 1e9 / (shunt_div * shunt_resistor_uohm * 1000)=0A=\n" + "=0A=\n" + "=3D> val / val2 =3D 1e6 / ((1e6 / shunt_voltage_lsb) * shunt_resistor_uohm=\n" + ")=0A=\n" + "=3D> val / val2 =3D shunt_voltage_lsb / shunt_resistor_uohm=0A=\n" + "=3D> [mA] =3D [nV] / [uOhm]=0A=\n" + "=0A=\n" + "The auxiliary variable is gone, as are the two DIV_ROUND_CLOSEST calls.=0A=\n" + "=0A=\n" + "=0A=\n" + "Part 2, range check:=0A=\n" + "=0A=\n" + "lower bound is obviously 1 uOhm. The upper bound is only limited by=0A=\n" + "typeof(val2) and typeof(shunt_resistor_uohm), both are ints.=0A=\n" + "=0A=\n" + "so the range check can be simplified to=0A=\n" + "=0A=\n" + "if (val =3D=3D 0 || val > INT_MAX)=0A=\n" + "\treturn -EINVAL=0A=\n" + "=0A=\n" + "Kind regards,=0A=\n" + "=0A=\n" + "Stefan=0A=\n" + =0A= -6d2638600e67424c6067eddaed675b45e4215341a9e4a7d6237cdb3e5ba4aa96 +c427f00ff3e218140b98140a414f87aac6480178337f73b162b4d8edf3c84a50
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.