| Message ID | 20250121-potin-ina238-shunt-voltage-scaling-v1-1-36d5dfe027f5@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | hwmon: ina238: Add support for shunt voltage scaling | expand |
On 1/20/25 08:23, Potin Lai wrote: > The INA238 sensor reports shunt voltage with microvolt precision. > However, the hwmon driver currently exposes this value only in > millivolts via `in0_input`, which results in a loss of precision for > readings within the range of ±1 mV. > > This patch introduces an `in0_scale` attribute to provide the scaling > factor applied to the shunt voltage reading. By exposing this attribute, > users can accurately interpret the in0_input values in microvolts, > preserving the sensor's full precision. > > Signed-off-by: Potin Lai <potin.lai.pt@gmail.com> Sorry, this is an ABI violation and thus a no-go. in0_input is and will always be reported in mV. Guenter
Hi Potin, kernel test robot noticed the following build warnings: [auto build test WARNING on fc033cf25e612e840e545f8d5ad2edd6ba613ed5] url: https://github.com/intel-lab-lkp/linux/commits/Potin-Lai/hwmon-ina238-Add-support-for-shunt-voltage-scaling/20250121-002826 base: fc033cf25e612e840e545f8d5ad2edd6ba613ed5 patch link: https://lore.kernel.org/r/20250121-potin-ina238-shunt-voltage-scaling-v1-1-36d5dfe027f5%40gmail.com patch subject: [PATCH] hwmon: ina238: Add support for shunt voltage scaling config: i386-buildonly-randconfig-004-20250121 (https://download.01.org/0day-ci/archive/20250121/202501210454.YUnMEUaA-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250121/202501210454.YUnMEUaA-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202501210454.YUnMEUaA-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/hwmon/ina238.c: In function 'shunt_volt_scale_show': >> drivers/hwmon/ina238.c:120:31: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=] 120 | return sprintf(buf, "%d\n", data->shunt_volt_scale); | ~^ ~~~~~~~~~~~~~~~~~~~~~~ | | | | int long int | %ld vim +120 drivers/hwmon/ina238.c 114 115 static ssize_t shunt_volt_scale_show(struct device *dev, 116 struct device_attribute *attr, char *buf) 117 { 118 struct ina238_data *data = dev_get_drvdata(dev); 119 > 120 return sprintf(buf, "%d\n", data->shunt_volt_scale); 121 } 122
On Tue, Jan 21, 2025 at 12:38 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 1/20/25 08:23, Potin Lai wrote: > > The INA238 sensor reports shunt voltage with microvolt precision. > > However, the hwmon driver currently exposes this value only in > > millivolts via `in0_input`, which results in a loss of precision for > > readings within the range of ±1 mV. > > > > This patch introduces an `in0_scale` attribute to provide the scaling > > factor applied to the shunt voltage reading. By exposing this attribute, > > users can accurately interpret the in0_input values in microvolts, > > preserving the sensor's full precision. > > > > Signed-off-by: Potin Lai <potin.lai.pt@gmail.com> > > Sorry, this is an ABI violation and thus a no-go. in0_input is and will > always be reported in mV. > > Guenter > Hi Guenter, In our use case, the shunt voltage is less than 1 mv most of the time. I would like to get your advice on the correct way of getting microvolt reading from the driver? Thank you. Best regards, Potin
On 1/20/25 17:18, Potin Lai wrote: > On Tue, Jan 21, 2025 at 12:38 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 1/20/25 08:23, Potin Lai wrote: >>> The INA238 sensor reports shunt voltage with microvolt precision. >>> However, the hwmon driver currently exposes this value only in >>> millivolts via `in0_input`, which results in a loss of precision for >>> readings within the range of ±1 mV. >>> >>> This patch introduces an `in0_scale` attribute to provide the scaling >>> factor applied to the shunt voltage reading. By exposing this attribute, >>> users can accurately interpret the in0_input values in microvolts, >>> preserving the sensor's full precision. >>> >>> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com> >> >> Sorry, this is an ABI violation and thus a no-go. in0_input is and will >> always be reported in mV. >> >> Guenter >> > > Hi Guenter, > In our use case, the shunt voltage is less than 1 mv most of the time. > I would like to get your advice on the correct way of getting > microvolt reading from the driver? Thank you. > Given that V = I * R, assuming you know the shunt resistor value, it is an easy calculation which does not require any code changes. Possible alternatives: - Create a debugfs file to display the voltage in uV. See drivers/hwmon/isl28022.c for an example. - Implement a hwmon -> iio bridge and use IIO to report the voltage. Having said that, do you have an actual use case ? What is the point of knowing the voltage across the shunt resistor ? "Because the chip reports it" is not a use case. Literally _every_ current sensor chip ultimately reports the shunt resistor voltage, after all (typically as current; calculation see above). Commit 63fb21afc1f5 ("hwmon: (ina2xx) Use shunt voltage to calculate current") is a case in point. Guenter
Hi Potin, kernel test robot noticed the following build warnings: [auto build test WARNING on fc033cf25e612e840e545f8d5ad2edd6ba613ed5] url: https://github.com/intel-lab-lkp/linux/commits/Potin-Lai/hwmon-ina238-Add-support-for-shunt-voltage-scaling/20250121-002826 base: fc033cf25e612e840e545f8d5ad2edd6ba613ed5 patch link: https://lore.kernel.org/r/20250121-potin-ina238-shunt-voltage-scaling-v1-1-36d5dfe027f5%40gmail.com patch subject: [PATCH] hwmon: ina238: Add support for shunt voltage scaling config: x86_64-buildonly-randconfig-004-20250121 (https://download.01.org/0day-ci/archive/20250121/202501211015.8Szwv3Cd-lkp@intel.com/config) compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250121/202501211015.8Szwv3Cd-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202501211015.8Szwv3Cd-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hwmon/ina238.c:120:30: warning: format specifies type 'int' but the argument has type 'long' [-Wformat] 120 | return sprintf(buf, "%d\n", data->shunt_volt_scale); | ~~ ^~~~~~~~~~~~~~~~~~~~~~ | %ld 1 warning generated. vim +120 drivers/hwmon/ina238.c 114 115 static ssize_t shunt_volt_scale_show(struct device *dev, 116 struct device_attribute *attr, char *buf) 117 { 118 struct ina238_data *data = dev_get_drvdata(dev); 119 > 120 return sprintf(buf, "%d\n", data->shunt_volt_scale); 121 } 122
diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c index 2d9f12f68d50..58737a0703dc 100644 --- a/drivers/hwmon/ina238.c +++ b/drivers/hwmon/ina238.c @@ -8,6 +8,7 @@ #include <linux/err.h> #include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> #include <linux/i2c.h> #include <linux/init.h> #include <linux/kernel.h> @@ -108,8 +109,42 @@ struct ina238_data { struct regmap *regmap; u32 rshunt; int gain; + long shunt_volt_scale; }; +static ssize_t shunt_volt_scale_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct ina238_data *data = dev_get_drvdata(dev); + + return sprintf(buf, "%d\n", data->shunt_volt_scale); +} + +static ssize_t shunt_volt_scale_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ina238_data *data = dev_get_drvdata(dev); + long val; + int err; + + err = kstrtol(buf, 10, &val); + if (err) + return err; + + data->shunt_volt_scale = val; + return count; +} + +static SENSOR_DEVICE_ATTR_RW(in0_scale, shunt_volt_scale, 0); + +static struct attribute *ina238_attrs[] = { + &sensor_dev_attr_in0_scale.dev_attr.attr, + NULL, +}; + +ATTRIBUTE_GROUPS(ina238); + static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val) { u8 data[3]; @@ -197,8 +232,9 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel, regval = (s16)regval; if (channel == 0) /* gain of 1 -> LSB / 4 */ - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) / - (1000 * (4 - data->gain + 1)); + *val = (regval * INA238_SHUNT_VOLTAGE_LSB * + data->shunt_volt_scale) / + (1000 * (4 - data->gain + 1)); else *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000; break; @@ -603,9 +639,12 @@ static int ina238_probe(struct i2c_client *client) return -ENODEV; } + /* Setup default shunt voltage scale */ + data->shunt_volt_scale = 1; + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, &ina238_chip_info, - NULL); + ina238_groups); if (IS_ERR(hwmon_dev)) return PTR_ERR(hwmon_dev);
The INA238 sensor reports shunt voltage with microvolt precision. However, the hwmon driver currently exposes this value only in millivolts via `in0_input`, which results in a loss of precision for readings within the range of ±1 mV. This patch introduces an `in0_scale` attribute to provide the scaling factor applied to the shunt voltage reading. By exposing this attribute, users can accurately interpret the in0_input values in microvolts, preserving the sensor's full precision. Signed-off-by: Potin Lai <potin.lai.pt@gmail.com> --- drivers/hwmon/ina238.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) --- base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5 change-id: 20250121-potin-ina238-shunt-voltage-scaling-8a3f9dc3d2b8 Best regards,