Re: [PATCH] hwmon: (ina3221) Add voltage conversion time settings

From: Guenter Roeck
Date: Wed Apr 17 2019 - 10:04:15 EST

On 4/17/19 6:46 AM, Guenter Roeck wrote:
On 4/16/19 4:55 PM, Nicolin Chen wrote:
The CONFIG register has two 3-bit fields for conversion time
settings of Bus-voltage and Shunt-voltage, respectively. The
conversion settings, along with averaging mode, allow users
to optimize available timing requirement.

This patch adds an 'update_interval' sysfs node through the
hwmon_chip_info of hwmon core. It reflects a total hardware
conversion time:
ÂÂÂÂ samples * channels * (Bus + Shunt conversion times)

Though INA3221 supports different conversion time setups for
Bus and Shunt voltages, this patch only adds the support of
a unified setting for both conversion times, by dividing the
conversion time into two equal values.

Signed-off-by: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
---

Hi Guenter,

I am not quite sure if this update_interval is the best way to
implement the conversion time settings but I can't find another
common approach. This implementation certainly has drawbacks:
 1) It can't configure Bus and Shunt conversion times separately
ÂÂÂÂ (Not crucial for me at this point as I set them equally)
 2) Users need to calculate for the settings of conversion time
 3) The ABI defines update_interval in msec while the hardware
ÂÂÂÂ and datasheet does in usec, and that generates rounding diff
 4) The update_interval value would be spontaneously modified
ÂÂÂÂ everytime number of samples or number of enabled channels
ÂÂÂÂ gets changed. This might confuses users who tries to have
ÂÂÂÂ a fixed update_interval other than really merely setting
ÂÂÂÂ conversion time.

I see IIO subsystem have something like IIO_CHAN_INFO_INT_TIME
for conversion time setting exclusively. Do we have something
similar under hwmon?


No. I think what you have should be good enough for now.
Please see comments below.

Thanks,
Guenter

Thanks

 Documentation/hwmon/ina3221 | 9 +++++
 drivers/hwmon/ina3221.c | 65 ++++++++++++++++++++++++++++++++-----
 2 files changed, 65 insertions(+), 9 deletions(-)

diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index ed3f22769d4b..3b05170112f0 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -38,3 +38,12 @@ in[456]_inputÂÂÂÂÂÂÂÂÂÂ Shunt voltage(uV) for channels 1, 2, and 3 respectively
 samples Number of samples using in the averaging mode.
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ Supports the list of number of samples:
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 1, 4, 16, 64, 128, 256, 512, 1024
+update_intervalÂÂÂÂÂÂÂÂ Data conversion time in millisecond, following:
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ update_interval = C x S x (BC + SC)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ C: number of enabled channels
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ S: number of samples
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ BC: bus-voltage conversion time in millisecond
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SC: shunt-voltage conversion time in millisecond
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ Affects both Bus- and Shunt-voltage conversion time.
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ Note that setting update_interval to 0ms sets both BC
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ and SC to 140 us (minimum conversion time).
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 74d39d212931..af4ab8ddddce 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -144,19 +144,37 @@ static const int ina3221_avg_samples[] = {
ÂÂÂÂÂ 1, 4, 16, 64, 128, 256, 512, 1024,
 };
-static inline int ina3221_wait_for_data(struct ina3221_data *ina)
+/* Converting update_interval in msec to conversion time in usec */
+static inline u32 ina3221_interval_ms_to_conv_time(u16 config, int interval)
 {
-ÂÂÂ u32 channels = hweight16(ina->reg_config & INA3221_CONFIG_CHs_EN_MASK);
-ÂÂÂ u32 vbus_ct_idx = INA3221_CONFIG_VBUS_CT(ina->reg_config);
-ÂÂÂ u32 vsh_ct_idx = INA3221_CONFIG_VSH_CT(ina->reg_config);
-ÂÂÂ u32 samples_idx = INA3221_CONFIG_AVG(ina->reg_config);
+ÂÂÂ u32 channels = hweight16(config & INA3221_CONFIG_CHs_EN_MASK);
+ÂÂÂ u32 samples_idx = INA3221_CONFIG_AVG(config);
+ÂÂÂ u32 samples = ina3221_avg_samples[samples_idx];
+
+ÂÂÂ /* Bisect the result to Bus and Shunt conversion times */
+ÂÂÂ return DIV_ROUND_CLOSEST(interval * 1000 / 2, channels * samples);
+}
+
+/* Converting CONFIG register value to update_interval in usec */
+static inline u32 ina3221_reg_to_interval_us(u16 config)
+{
+ÂÂÂ u32 channels = hweight16(config & INA3221_CONFIG_CHs_EN_MASK);
+ÂÂÂ u32 vbus_ct_idx = INA3221_CONFIG_VBUS_CT(config);
+ÂÂÂ u32 vsh_ct_idx = INA3221_CONFIG_VSH_CT(config);
+ÂÂÂ u32 samples_idx = INA3221_CONFIG_AVG(config);
ÂÂÂÂÂ u32 samples = ina3221_avg_samples[samples_idx];
ÂÂÂÂÂ u32 vbus_ct = ina3221_conv_time[vbus_ct_idx];
ÂÂÂÂÂ u32 vsh_ct = ina3221_conv_time[vsh_ct_idx];
-ÂÂÂ u32 wait, cvrf;
ÂÂÂÂÂ /* Calculate total conversion time */
-ÂÂÂ wait = channels * (vbus_ct + vsh_ct) * samples;
+ÂÂÂ return channels * (vbus_ct + vsh_ct) * samples;
+}
+
+static inline int ina3221_wait_for_data(struct ina3221_data *ina)
+{
+ÂÂÂ u32 wait, cvrf;
+
+ÂÂÂ wait = ina3221_reg_to_interval_us(ina->reg_config);
ÂÂÂÂÂ /* Polling the CVRF bit to make sure read data is ready */
ÂÂÂÂÂ return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
@@ -197,6 +215,11 @@ static int ina3221_read_chip(struct device *dev, u32 attr, long *val)
ÂÂÂÂÂÂÂÂÂ regval = INA3221_CONFIG_AVG(ina->reg_config);
ÂÂÂÂÂÂÂÂÂ *val = ina3221_avg_samples[regval];
ÂÂÂÂÂÂÂÂÂ return 0;
+ÂÂÂ case hwmon_chip_update_interval:
+ÂÂÂÂÂÂÂ /* Return in msec */
+ÂÂÂÂÂÂÂ *val = ina3221_reg_to_interval_us(ina->reg_config);
+ÂÂÂÂÂÂÂ *val = DIV_ROUND_CLOSEST(*val, 1000);
+ÂÂÂÂÂÂÂ return 0;
ÂÂÂÂÂ default:
ÂÂÂÂÂÂÂÂÂ return -EOPNOTSUPP;
ÂÂÂÂÂ }
@@ -308,7 +331,7 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
 static int ina3221_write_chip(struct device *dev, u32 attr, long val)
 {
ÂÂÂÂÂ struct ina3221_data *ina = dev_get_drvdata(dev);
-ÂÂÂ int ret, idx;
+ÂÂÂ int ret, idx, tmp;
ÂÂÂÂÂ switch (attr) {
ÂÂÂÂÂ case hwmon_chip_samples:
@@ -321,6 +344,28 @@ static int ina3221_write_chip(struct device *dev, u32 attr, long val)
ÂÂÂÂÂÂÂÂÂ if (ret)
ÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
+ÂÂÂÂÂÂÂ /* Update reg_config accordingly */
+ÂÂÂÂÂÂÂ return regmap_read(ina->regmap, INA3221_CONFIG,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &ina->reg_config);
+ÂÂÂ case hwmon_chip_update_interval:
+ÂÂÂÂÂÂÂ tmp = ina3221_interval_ms_to_conv_time(ina->reg_config, val);
+ÂÂÂÂÂÂÂ idx = find_closest(tmp, ina3221_conv_time,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ARRAY_SIZE(ina3221_conv_time));
+
+ÂÂÂÂÂÂÂ /* Update Bus-voltage conversion time */
+ÂÂÂÂÂÂÂ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ INA3221_CONFIG_VBUS_CT_MASK,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ idx << INA3221_CONFIG_VBUS_CT_SHIFT);
+ÂÂÂÂÂÂÂ if (ret)
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂÂÂÂÂ /* Update Shunt-voltage conversion time */
+ÂÂÂÂÂÂÂ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ INA3221_CONFIG_VSH_CT_MASK,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ idx << INA3221_CONFIG_VSH_CT_SHIFT);
+ÂÂÂÂÂÂÂ if (ret)
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;

It should be possible to update both conversion times with a single call,
since both calls touch only one register. Something like

ÂÂÂÂÂÂÂ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG
ÂÂÂÂÂÂÂÂÂÂÂ INA3221_CONFIG_VBUS_CT_MASK | INA3221_CONFIG_VSH_CT_MASK,
ÂÂÂÂÂÂÂÂÂÂÂ (idx << INA3221_CONFIG_VBUS_CT_SHIFT) | (idx << INA3221_CONFIG_VSH_CT_SHIFT));

Granted, that is a bit long, but it saves an extra i2c write operation.


Thinking about it ... does it even make sense to cache reg_config twice,
or would it be better to just update the local copy and use regmap_write()
to send it to the chip ?

Guenter

+
ÂÂÂÂÂÂÂÂÂ /* Update reg_config accordingly */
ÂÂÂÂÂÂÂÂÂ return regmap_read(ina->regmap, INA3221_CONFIG,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &ina->reg_config);
@@ -482,6 +527,7 @@ static umode_t ina3221_is_visible(const void *drvdata,
ÂÂÂÂÂ case hwmon_chip:
ÂÂÂÂÂÂÂÂÂ switch (attr) {
ÂÂÂÂÂÂÂÂÂ case hwmon_chip_samples:
+ÂÂÂÂÂÂÂ case hwmon_chip_update_interval:
ÂÂÂÂÂÂÂÂÂÂÂÂÂ return 0644;
ÂÂÂÂÂÂÂÂÂ default:
ÂÂÂÂÂÂÂÂÂÂÂÂÂ return 0;
@@ -527,7 +573,8 @@ static umode_t ina3221_is_visible(const void *drvdata,
 static const struct hwmon_channel_info *ina3221_info[] = {
ÂÂÂÂÂ HWMON_CHANNEL_INFO(chip,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ HWMON_C_SAMPLES),
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ HWMON_C_SAMPLES,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ HWMON_C_UPDATE_INTERVAL),
ÂÂÂÂÂ HWMON_CHANNEL_INFO(in,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* 0: dummy, skipped in is_visible */
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ HWMON_I_INPUT,