[PATCH v2 0/2] hwmon: (ina3221) Two changes for INA3221

[PATCH v2 0/2] hwmon: (ina3221) Two changes for INA3221

[Nicolin Chen]

Adding two changes for INA3221 HWMON driver.

Changlog
v1->v2:
 * Added PATCH-1
 * Rebased PATCH-2 (see detail of changelog in it)

Nicolin Chen (2):
  hwmon: (ina3221) Do not read-back to cache reg_config
  hwmon: (ina3221) Add voltage conversion time settings

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

-- 
2.17.1

[PATCH v2 1/2] hwmon: (ina3221) Do not read-back to cache reg_config

[Nicolin Chen]

Reading back the CONFIG register increases an extra I2C
transaction. This's not necessary and could be replaced
with a local variable caching the register settings.

So this patch replaces two readback regmap_read() calls
with a tmp variable.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 drivers/hwmon/ina3221.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 74d39d212931..62040aac653c 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -309,21 +309,22 @@ static int ina3221_write_chip(struct device *dev, u32 attr, long val)
 {
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	int ret, idx;
+	u32 tmp;
 
 	switch (attr) {
 	case hwmon_chip_samples:
 		idx = find_closest(val, ina3221_avg_samples,
 				   ARRAY_SIZE(ina3221_avg_samples));
 
-		ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
-					 INA3221_CONFIG_AVG_MASK,
-					 idx << INA3221_CONFIG_AVG_SHIFT);
+		tmp = (ina->reg_config & ~INA3221_CONFIG_AVG_MASK) |
+		      (idx << INA3221_CONFIG_AVG_SHIFT);
+		ret = regmap_write(ina->regmap, INA3221_CONFIG, tmp);
 		if (ret)
 			return ret;
 
 		/* Update reg_config accordingly */
-		return regmap_read(ina->regmap, INA3221_CONFIG,
-				   &ina->reg_config);
+		ina->reg_config = tmp;
+		return 0;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -359,6 +360,7 @@ static int ina3221_write_enable(struct device *dev, int channel, bool enable)
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	u16 config, mask = INA3221_CONFIG_CHx_EN(channel);
 	u16 config_old = ina->reg_config & mask;
+	u32 tmp;
 	int ret;
 
 	config = enable ? mask : 0;
@@ -377,14 +379,13 @@ static int ina3221_write_enable(struct device *dev, int channel, bool enable)
 	}
 
 	/* Enable or disable the channel */
-	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
+	tmp = (ina->reg_config & ~mask) | (config & mask);
+	ret = regmap_write(ina->regmap, INA3221_CONFIG, tmp);
 	if (ret)
 		goto fail;
 
 	/* Cache the latest config register value */
-	ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
-	if (ret)
-		goto fail;
+	ina->reg_config = tmp;
 
 	/* For disabling routine, decrease refcount or suspend() at last */
 	if (!enable)
-- 
2.17.1

[PATCH v2 2/2] hwmon: (ina3221) Add voltage conversion time settings

[Nicolin Chen]

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@gmail.com>
---
Changelog
v1->v2:
 * Merged two adjacent calls of regmap_update_bits().
 * Replaced CONFIG register read-back with tmp variable

 Documentation/hwmon/ina3221 |  9 ++++++
 drivers/hwmon/ina3221.c     | 58 ++++++++++++++++++++++++++++++++-----
 2 files changed, 59 insertions(+), 8 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 62040aac653c..e0637fed9585 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(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(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 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;
 	}
@@ -322,6 +345,23 @@ static int ina3221_write_chip(struct device *dev, u32 attr, long val)
 		if (ret)
 			return ret;
 
+		/* Update reg_config accordingly */
+		ina->reg_config = tmp;
+		return 0;
+	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 and Shunt voltage conversion times */
+		tmp = INA3221_CONFIG_VBUS_CT_MASK | INA3221_CONFIG_VSH_CT_MASK;
+		tmp = (ina->reg_config & ~tmp) |
+		      (idx << INA3221_CONFIG_VBUS_CT_SHIFT) |
+		      (idx << INA3221_CONFIG_VSH_CT_SHIFT);
+		ret = regmap_write(ina->regmap, INA3221_CONFIG, tmp);
+		if (ret)
+			return ret;
+
 		/* Update reg_config accordingly */
 		ina->reg_config = tmp;
 		return 0;
@@ -483,6 +523,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;
@@ -528,7 +569,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,
-- 
2.17.1

Re: [PATCH v2 1/2] hwmon: (ina3221) Do not read-back to cache reg_config

[Guenter Roeck]

On Wed, Apr 17, 2019 at 04:12:09PM -0700, Nicolin Chen wrote:
> Reading back the CONFIG register increases an extra I2C
> transaction. This's not necessary and could be replaced
> with a local variable caching the register settings.
> 
> So this patch replaces two readback regmap_read() calls
> with a tmp variable.
> 
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>

Applied to hwmon-next.

Thanks,
Guenter

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

[Guenter Roeck]

On Wed, Apr 17, 2019 at 04:12:10PM -0700, 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@gmail.com>

Applied to -next. I had to update the documentation for the .rst formatting
changes. Hope I got it right.

Thanks,
Guenter

> ---
> Changelog
> v1->v2:
>  * Merged two adjacent calls of regmap_update_bits().
>  * Replaced CONFIG register read-back with tmp variable
> 
>  Documentation/hwmon/ina3221 |  9 ++++++
>  drivers/hwmon/ina3221.c     | 58 ++++++++++++++++++++++++++++++++-----
>  2 files changed, 59 insertions(+), 8 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 62040aac653c..e0637fed9585 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(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(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 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;
>  	}
> @@ -322,6 +345,23 @@ static int ina3221_write_chip(struct device *dev, u32 attr, long val)
>  		if (ret)
>  			return ret;
>  
> +		/* Update reg_config accordingly */
> +		ina->reg_config = tmp;
> +		return 0;
> +	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 and Shunt voltage conversion times */
> +		tmp = INA3221_CONFIG_VBUS_CT_MASK | INA3221_CONFIG_VSH_CT_MASK;
> +		tmp = (ina->reg_config & ~tmp) |
> +		      (idx << INA3221_CONFIG_VBUS_CT_SHIFT) |
> +		      (idx << INA3221_CONFIG_VSH_CT_SHIFT);
> +		ret = regmap_write(ina->regmap, INA3221_CONFIG, tmp);
> +		if (ret)
> +			return ret;
> +
>  		/* Update reg_config accordingly */
>  		ina->reg_config = tmp;
>  		return 0;
> @@ -483,6 +523,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;
> @@ -528,7 +569,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,