hwmon: (ina3221) Implement PM runtime to save power

The data might need some time to get ready after channel enabling, although the data register is always readable. The CVRF bit is to indicate that data conversion is finished, so polling the CVRF bit before data reading could ensure the result being valid.

An alternative way could be to wait for expected time between the channel enabling and the data reading. And this could avoid extra I2C communications. However, INA3221 seemly takes longer time than what's stated in the datasheet. Test results show that sometimes it couldn't finish data conversion in time.

So this patch plays safe by adding a CVRF polling to make sure the data register is updated with the new data.

Signed-off-by Nicolin Chen <[email protected]>

---
* Moved CVRF polling to data read routine
* Added calculation of wait time based on conversion time setting

 drivers/hwmon/ina3221.c | 46 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

drivers/hwmon/ina3221.c +33 -0

index 10e8347a3c80..9bbac826e50b 100644
@@ -44,6 +44,13 @@
 #define INA3221_CONFIG_MODE_SHUNT	BIT(0)
 #define INA3221_CONFIG_MODE_BUS		BIT(1)
 #define INA3221_CONFIG_MODE_CONTINUOUS	BIT(2)
+#define INA3221_CONFIG_VSH_CT_SHIFT	3
+#define INA3221_CONFIG_VSH_CT_MASK	GENMASK(5, 3)
+#define INA3221_CONFIG_VSH_CT(x)	(((x) & GENMASK(5, 3)) >> 3)
+#define INA3221_CONFIG_VBUS_CT_SHIFT	6
+#define INA3221_CONFIG_VBUS_CT_MASK	GENMASK(8, 6)
+#define INA3221_CONFIG_VBUS_CT(x)	(((x) & GENMASK(8, 6)) >> 6)
+#define INA3221_CONFIG_CHs_EN_MASK	GENMASK(14, 12)
 #define INA3221_CONFIG_CHx_EN(x)	BIT(14 - (x))
 
 #define INA3221_RSHUNT_DEFAULT		10000

@@ -52,6 +59,9 @@ enum ina3221_fields {
 	/* Configuration */
 	F_RST,
 
+	/* Status Flags */
+	F_CVRF,
+
 	/* Alert Flags */
 	F_WF3, F_WF2, F_WF1,
 	F_CF3, F_CF2, F_CF1,

@@ -63,6 +73,7 @@ enum ina3221_fields {
 static const struct reg_field ina3221_reg_fields[] = {
 	[F_RST] = REG_FIELD(INA3221_CONFIG, 15, 15),
 
+	[F_CVRF] = REG_FIELD(INA3221_MASK_ENABLE, 0, 0),
 	[F_WF3] = REG_FIELD(INA3221_MASK_ENABLE, 3, 3),
 	[F_WF2] = REG_FIELD(INA3221_MASK_ENABLE, 4, 4),
 	[F_WF1] = REG_FIELD(INA3221_MASK_ENABLE, 5, 5),

@@ -111,6 +122,28 @@ static inline bool ina3221_is_enabled(struct  
 ina3221_data *ina, int channel)
 	return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
 }
 
+/* Lookup table for Bus and Shunt conversion times in usec */
+static const u16 ina3221_conv_time[] = {
+	140, 204, 332, 588, 1100, 2116, 4156, 8244,
+};
+
+static inline int ina3221_wait_for_data(struct ina3221_data *ina)
+{
+	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 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);
+
+	/* Polling the CVRF bit to make sure read data is ready */
+	return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
+					      cvrf, cvrf, wait, 100000);
+}
+
 static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
 			      int *val)

{

drivers/hwmon/ina3221.c +1 -0

+
 		ret = ina3221_read_value(ina, reg, &regval);
 		if (ret)

drivers/hwmon/ina3221.c +7 -0

 			return ret;

@@ -189,6 +228,13 @@ static int ina3221_read_curr(struct device  
 *dev, u32 attr,
 	case hwmon_curr_input:
 		if (!ina3221_is_enabled(ina, channel))
 			return -ENODATA;
+
+		ret = ina3221_wait_for_data(ina);
+		if (ret) {
+			dev_err(dev, "Timed out at waiting for CVRF bit\n");
+			return ret;
+		}
+
 		/* fall through */
 	case hwmon_curr_crit:

case hwmon_curr_max:
--
2.17.1

Quoted 5 lines

drivers/hwmon/ina3221.c +0 -0

@@ -150,6 +183,12 @@ static int ina3221_read_in(struct device *dev, u32
 attr, int channel, long *val)
 		if (!ina3221_is_enabled(ina, channel))
 			return -ENODATA;
 

drivers/hwmon/ina3221.c static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val) · line 190

Quoted 5 lines

drivers/hwmon/ina3221.c +5 -0

+		ret = ina3221_wait_for_data(ina);
+		if (ret) {
+			dev_err(dev, "Timed out at waiting for CVRF bit\n");
+			return ret;
+		}

Thanks for explaining why we can't just blindly wait. However, I am concerned about this log message: If something is wrong with the chip, this will spam the kernel log. Can you drop the message here and below ? After all, the error will be reported to userspace, and a kernel log message should not be necessary.

Thanks,
Guenter

Quoted 21 lines

drivers/hwmon/ina3221.c +1 -0

+
 		ret = ina3221_read_value(ina, reg, &regval);
 		if (ret)

drivers/hwmon/ina3221.c +7 -0

 			return ret;

@@ -189,6 +228,13 @@ static int ina3221_read_curr(struct device *dev,
 u32 attr,
 	case hwmon_curr_input:
 		if (!ina3221_is_enabled(ina, channel))
 			return -ENODATA;
+
+		ret = ina3221_wait_for_data(ina);
+		if (ret) {
+			dev_err(dev, "Timed out at waiting for CVRF bit\n");
+			return ret;
+		}
+
 		/* fall through */
 	case hwmon_curr_crit:

case hwmon_curr_max:
--
2.17.1

If all three channels are disabled via in[123]_enable ABI, the driver could suspend the chip for power saving purpose.

So this patch addsd the PM runtime support in order to gain more power control than system suspend and resume use case.

For PM runtime, there are a few related changes happening:
1) Added a new explicit hdev device pointer for all the pm runtime callbacks. This is because hwmon core registers a child device for each hwmon driver. So there might be a mismatch between two device pointers in the driver if mixing using them.
2) Added a check in ina3221_is_enabled() to make sure that the chip is resumed.
3) Bypassed the unchanged status in ina3221_write_enable() in order to keep the pm runtime refcount being matched.
4) Removed the reset routine in the probe() by calling the resume() via pm_runtime_get_sync(), as they're similar. It's also necessary to do so to match initial refcount with the number of enabled channels.
5) Bypassed the system suspend/resume callbacks from the i2c_client->dev, because both the i2c_client->dev and the ina->hdev coexist and share the same pair of system suspend/resume callback functions, which means there'd be two suspend() calls during the system suspend while the second one will fail.

Signed-off-by Nicolin Chen <[email protected]>

---
Changelog
v1->v2:
* Bypassed i2c_client->dev in suspend/resume()
* Added a missing '\n' in one dev_err()

 drivers/hwmon/ina3221.c | 112 ++++++++++++++++++++++++++++++++--------
 1 file changed, 90 insertions(+), 22 deletions(-)

drivers/hwmon/ina3221.c +23 -3

index 9bbac826e50b..2cdc37ab0cf3 100644
@@ -21,6 +21,7 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
+#include <linux/pm_runtime.h>
 
 #define INA3221_DRIVER_NAME		"ina3221"
 

@@ -53,6 +54,7 @@
 #define INA3221_CONFIG_CHs_EN_MASK	GENMASK(14, 12)
 #define INA3221_CONFIG_CHx_EN(x)	BIT(14 - (x))
 
+#define INA3221_CONFIG_DEFAULT		0x7127
 #define INA3221_RSHUNT_DEFAULT		10000
 
 enum ina3221_fields {

@@ -103,6 +105,7 @@ struct ina3221_input {
 
 /**
  * struct ina3221_data - device specific information
+ * @hdev: Device pointer of hwmon child device, used for pm runtime
  * @regmap: Register map of the device
  * @fields: Register fields of the device
  * @inputs: Array of channel input source specific structures

@@ -110,6 +113,7 @@ struct ina3221_input {
  * @reg_config: Register value of INA3221_CONFIG
  */
 struct ina3221_data {
+	struct device *hdev;
 	struct regmap *regmap;
 	struct regmap_field *fields[F_MAX_FIELDS];
 	struct ina3221_input inputs[INA3221_NUM_CHANNELS];

@@ -119,7 +123,8 @@ struct ina3221_data {
 
 static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
 {
-	return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
+	return pm_runtime_active(ina->hdev) &&
+	       (ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
 }
 
 /* Lookup table for Bus and Shunt conversion times in usec */

@@ -294,21 +299,48 @@ 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;
 	int ret;
 
 	config = enable ? mask : 0;
 
+	/* Bypass if enable status is not being changed */
+	if (config_old == config)
+		return 0;
+
+	/* For enabling routine, increase refcount and resume() at first */
+	if (enable) {
+		ret = pm_runtime_get_sync(ina->hdev);
+		if (ret < 0) {
+			dev_err(dev, "Failed to get PM runtime\n");
+			return ret;
+		}
+	}
+
 	/* Enable or disable the channel */
 	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
 	if (ret)
-		return ret;
+		goto fail;
 
 	/* Cache the latest config register value */
 	ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
 	if (ret)
-		return ret;
+		goto fail;
+

drivers/hwmon/ina3221.c +4 -0

+		pm_runtime_put_sync(ina->hdev);
+	}
+
+	return ret;
 }
 

static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,

drivers/hwmon/ina3221.c +22 -18

@@ -603,7 +635,6 @@ static int ina3221_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct ina3221_data *ina;
-	struct device *hwmon_dev;
 	int i, ret;
 
 	ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL);

@@ -635,44 +666,71 @@ static int ina3221_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = regmap_field_write(ina->fields[F_RST], true);
-	if (ret) {
-		dev_err(dev, "Unable to reset device\n");
-		return ret;
-	}
-
-	/* Sync config register after reset */
-	ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
-	if (ret)
-		return ret;
+	/* The driver will be reset, so use reset value */
+	ina->reg_config = INA3221_CONFIG_DEFAULT;
 
 	/* Disable channels if their inputs are disconnected */
 	for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
 		if (ina->inputs[i].disconnected)
 			ina->reg_config &= ~INA3221_CONFIG_CHx_EN(i);
 	}
-	ret = regmap_write(ina->regmap, INA3221_CONFIG, ina->reg_config);
-	if (ret)
-		return ret;
 
 	mutex_init(&ina->lock);
 	dev_set_drvdata(dev, ina);
 
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
+	/* Fence sysfs nodes till pm_runtime is resumed */
+	mutex_lock(&ina->lock);
+
+	/* Use the returned hdev for pm_runtime */
+	ina->hdev = devm_hwmon_device_register_with_info(dev, client->name, ina,
 							 &ina3221_chip_info,
 							 ina3221_groups);
-	if (IS_ERR(hwmon_dev)) {
+	if (IS_ERR(ina->hdev)) {
 		dev_err(dev, "Unable to register hwmon device\n");
-		mutex_destroy(&ina->lock);
-		return PTR_ERR(hwmon_dev);
+		ret = PTR_ERR(ina->hdev);
+		goto fail_lock;
 	}
 
+	/* Enable PM runtime -- status is suspended by default */
+	pm_runtime_enable(ina->hdev);
+
+	/* Initialize (resume) the device */
+	for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
+		if (ina->inputs[i].disconnected)
+			continue;
+		/* Match the refcount with number of enabled channels */
+		ret = pm_runtime_get_sync(ina->hdev);
+		if (ret < 0)
+			goto fail_pm;
+	}

drivers/hwmon/ina3221.c +5 -0

+fail_lock:
+	mutex_unlock(&ina->lock);
+	mutex_destroy(&ina->lock);
+
+	return ret;
 }
 
 static int ina3221_remove(struct i2c_client *client)
 {

drivers/hwmon/ina3221.c +4 -0

 	mutex_destroy(&ina->lock);
 

@@ -684,6 +742,10 @@ static int __maybe_unused  
 ina3221_suspend(struct device *dev)
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	int ret;
 
+	/* Let hdev control all PM runtime callbacks */
+	if (dev != ina->hdev)
+		return 0;
+
 	/* Save config register value and enable cache-only */
 	ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);

drivers/hwmon/ina3221.c +4 -0

 	if (ret)

@@ -707,6 +769,10 @@ static int __maybe_unused ina3221_resume(struct  
 device *dev)
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	int ret;
 
+	/* Let hdev control all PM runtime callbacks */
+	if (dev != ina->hdev)
+		return 0;
+
 	regcache_cache_only(ina->regmap, false);
 

drivers/hwmon/ina3221.c +3 -1

 	/* Software reset the chip */

@@ -730,7 +796,9 @@ static int __maybe_unused ina3221_resume(struct  
 device *dev)
 }
 
 static const struct dev_pm_ops ina3221_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(ina3221_suspend, ina3221_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(ina3221_suspend, ina3221_resume, NULL)
 };
 

static const struct of_device_id ina3221_of_match_table[] = {
--
2.17.1

drivers/hwmon/ina3221.c static int ina3221_write_enable(struct device *dev, int channel, bool enable) · line 339

Quoted 4 lines

drivers/hwmon/ina3221.c +4 -0

+fail:
+	if (enable) {
+		dev_err(dev, "Reverting channel%d enabling: %d\n",
+			channel, ret);

This message is confusing. Something like "Failed to enable channel %d:
error %d" would be much better.

drivers/hwmon/ina3221.c static int ina3221_probe(struct i2c_client *client, · line 715

Quoted 5 lines

drivers/hwmon/ina3221.c +5 -0

+fail_pm:
+	pm_runtime_disable(ina->hdev);
+	pm_runtime_set_suspended(ina->hdev);
+	for (i = 0; i < INA3221_NUM_CHANNELS; i++)
+		pm_runtime_put_noidle(ina->hdev);

The count here doesn't match the count above if some channels are disabled, or if the enable loop above aborted.

drivers/hwmon/ina3221.c static int ina3221_probe(struct i2c_client *client, · line 733

Quoted 4 lines

drivers/hwmon/ina3221.c +3 -0

+	/* Decrease the PM refcount */
+	for (i = 0; i < INA3221_NUM_CHANNELS; i++)
+		pm_runtime_put_noidle(ina->hdev);

As above, this doesn't take disabled channels into account. Maybe that doesn't matter; if so, there needs to be a comment indicating that negative use counts don't matter. If that is the case, make sure that this is acceptable use of the pm API (if it works but is not documented, the PM core may change and complain about it at a later time).