[PATCH v2 0/5] hwmon: (ina3221) Implement PM runtime to save power

[PATCH v2 0/5] hwmon: (ina3221) Implement PM runtime to save power

[Nicolin Chen]

This series patches implement PM runtime feature in the ina3221 hwmon
driver (PATCH-5). However, PATCH-[1:4] are required to make sure that
the PM runtime feature would be functional and safe.

Changelog
v1->v2:
 * Added device pointer check (PATCH-1)
 * Returned 0 for alert flags (PATCH-2)
 * Moved CVRF polling to data read routine (PATCH-4)
 * Bypassed i2c_client->dev in suspend/resume() (PATCH-5)

Nicolin Chen (5):
  hwmon: (core) Inherit power properties to hdev
  hwmon: (ina3221) Check channel status for alarms attribute read
  hwmon: (ina3221) Serialize sysfs ABI accesses
  hwmon: (ina3221) Make sure data is ready before reading
  hwmon: (ina3221) Add PM runtime support

 drivers/hwmon/hwmon.c   |   7 +-
 drivers/hwmon/ina3221.c | 213 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 192 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] hwmon: (core) Inherit power properties to hdev

[Nicolin Chen]

The new hdev is a child device related to the original parent
hwmon driver and its device. However, it doesn't support the
power features, typically being defined in the parent driver.

So this patch inherits three necessary power properties from
the parent dev to hdev: power, pm_domain and driver pointers.

Note that the dev->driver pointer is the place that contains
a dev_pm_ops pointer defined in the parent device driver and
the pm runtime core also checks this pointer:
       if (!cb && dev->driver && dev->driver->pm)

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
Changelog
v1->v2:
 * Added device pointer check

 drivers/hwmon/hwmon.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 975c95169884..14cfab64649f 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -625,7 +625,12 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 	hwdev->name = name;
 	hdev->class = &hwmon_class;
 	hdev->parent = dev;
-	hdev->of_node = dev ? dev->of_node : NULL;
+	if (dev) {
+		hdev->driver = dev->driver;
+		hdev->power = dev->power;
+		hdev->pm_domain = dev->pm_domain;
+		hdev->of_node = dev->of_node;
+	}
 	hwdev->chip = chip;
 	dev_set_drvdata(hdev, drvdata);
 	dev_set_name(hdev, HWMON_ID_FORMAT, id);
-- 
2.17.1

[PATCH v2 2/5] hwmon: (ina3221) Check channel status for alarms attribute read

[Nicolin Chen]

There is nothing critically wrong to read these two attributes
without having a is_enabled() check at this point. But reading
the MASK_ENABLE register would clear the CVRF bit according to
the datasheet. So it'd be safer to fence for disabled channels
in order to add pm runtime feature.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
Changelog
v1->v2:
 * Returned 0 for alert flags instead of -ENODATA

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

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index d61688f04594..26cdf3342d80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -200,6 +200,12 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
 		return 0;
 	case hwmon_curr_crit_alarm:
 	case hwmon_curr_max_alarm:
+		/* No actual register read if channel is disabled */
+		if (!ina3221_is_enabled(ina, channel)) {
+			/* Return 0 for alert flags */
+			*val = 0;
+			return 0;
+		}
 		ret = regmap_field_read(ina->fields[reg], &regval);
 		if (ret)
 			return ret;
-- 
2.17.1

[PATCH v2 3/5] hwmon: (ina3221) Serialize sysfs ABI accesses

[Nicolin Chen]

This change adds a mutex to serialize accesses of sysfs attributes.

This is required when polling CVRF bit of the MASK/ENABLE register
because this bit is cleared on a read of this MASK/ENABLE register
or a write to CONFIG register, which means that this bit might be
accidentally cleared by reading other fields like alert flags.

So this patch adds a mutex lock to protect the write() and read()
callbacks. The read_string() callback won't need the lock since it
just returns the label without touching any hardware register.

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

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 26cdf3342d80..10e8347a3c80 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -18,6 +18,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
 
@@ -94,12 +95,14 @@ struct ina3221_input {
  * @regmap: Register map of the device
  * @fields: Register fields of the device
  * @inputs: Array of channel input source specific structures
+ * @lock: mutex lock to serialize sysfs attribute accesses
  * @reg_config: Register value of INA3221_CONFIG
  */
 struct ina3221_data {
 	struct regmap *regmap;
 	struct regmap_field *fields[F_MAX_FIELDS];
 	struct ina3221_input inputs[INA3221_NUM_CHANNELS];
+	struct mutex lock;
 	u32 reg_config;
 };
 
@@ -265,29 +268,53 @@ static int ina3221_write_enable(struct device *dev, int channel, bool enable)
 static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long *val)
 {
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	int ret;
+
+	mutex_lock(&ina->lock);
+
 	switch (type) {
 	case hwmon_in:
 		/* 0-align channel ID */
-		return ina3221_read_in(dev, attr, channel - 1, val);
+		ret = ina3221_read_in(dev, attr, channel - 1, val);
+		break;
 	case hwmon_curr:
-		return ina3221_read_curr(dev, attr, channel, val);
+		ret = ina3221_read_curr(dev, attr, channel, val);
+		break;
 	default:
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		break;
 	}
+
+	mutex_unlock(&ina->lock);
+
+	return ret;
 }
 
 static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
 			 u32 attr, int channel, long val)
 {
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	int ret;
+
+	mutex_lock(&ina->lock);
+
 	switch (type) {
 	case hwmon_in:
 		/* 0-align channel ID */
-		return ina3221_write_enable(dev, channel - 1, val);
+		ret = ina3221_write_enable(dev, channel - 1, val);
+		break;
 	case hwmon_curr:
-		return ina3221_write_curr(dev, attr, channel, val);
+		ret = ina3221_write_curr(dev, attr, channel, val);
+		break;
 	default:
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		break;
 	}
+
+	mutex_unlock(&ina->lock);
+
+	return ret;
 }
 
 static int ina3221_read_string(struct device *dev, enum hwmon_sensor_types type,
@@ -582,6 +609,7 @@ static int ina3221_probe(struct i2c_client *client,
 	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,
@@ -589,12 +617,22 @@ static int ina3221_probe(struct i2c_client *client,
 							 ina3221_groups);
 	if (IS_ERR(hwmon_dev)) {
 		dev_err(dev, "Unable to register hwmon device\n");
+		mutex_destroy(&ina->lock);
 		return PTR_ERR(hwmon_dev);
 	}
 
 	return 0;
 }
 
+static int ina3221_remove(struct i2c_client *client)
+{
+	struct ina3221_data *ina = dev_get_drvdata(&client->dev);
+
+	mutex_destroy(&ina->lock);
+
+	return 0;
+}
+
 static int __maybe_unused ina3221_suspend(struct device *dev)
 {
 	struct ina3221_data *ina = dev_get_drvdata(dev);
@@ -663,6 +701,7 @@ MODULE_DEVICE_TABLE(i2c, ina3221_ids);
 
 static struct i2c_driver ina3221_i2c_driver = {
 	.probe = ina3221_probe,
+	.remove = ina3221_remove,
 	.driver = {
 		.name = INA3221_DRIVER_NAME,
 		.of_match_table = ina3221_of_match_table,
-- 
2.17.1

[PATCH v2 4/5] hwmon: (ina3221) Make sure data is ready before reading

[Nicolin Chen]

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 <nicoleotsuka@gmail.com>
---
 * 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(+)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 10e8347a3c80..9bbac826e50b 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -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)
 {
@@ -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;
 
+		ret = ina3221_wait_for_data(ina);
+		if (ret) {
+			dev_err(dev, "Timed out at waiting for CVRF bit\n");
+			return ret;
+		}
+
 		ret = ina3221_read_value(ina, reg, &regval);
 		if (ret)
 			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

[PATCH v2 5/5] hwmon: (ina3221) Add PM runtime support

[Nicolin Chen]

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 <nicoleotsuka@gmail.com>
---
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(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 9bbac826e50b..2cdc37ab0cf3 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -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;
+
+	/* For disabling routine, decrease refcount or suspend() at last */
+	if (!enable)
+		pm_runtime_put_sync(ina->hdev);
 
 	return 0;
+
+fail:
+	if (enable) {
+		dev_err(dev, "Reverting channel%d enabling: %d\n",
+			channel, ret);
+		pm_runtime_put_sync(ina->hdev);
+	}
+
+	return ret;
 }
 
 static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
@@ -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;
+	}
+
+	mutex_unlock(&ina->lock);
+
 	return 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);
+fail_lock:
+	mutex_unlock(&ina->lock);
+	mutex_destroy(&ina->lock);
+
+	return ret;
 }
 
 static int ina3221_remove(struct i2c_client *client)
 {
 	struct ina3221_data *ina = dev_get_drvdata(&client->dev);
+	int i;
+
+	pm_runtime_disable(ina->hdev);
+	pm_runtime_set_suspended(ina->hdev);
+
+	/* Decrease the PM refcount */
+	for (i = 0; i < INA3221_NUM_CHANNELS; i++)
+		pm_runtime_put_noidle(ina->hdev);
 
 	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);
 	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);
 
 	/* 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

Re: [PATCH v2 4/5] hwmon: (ina3221) Make sure data is ready before reading

[linux]

Quoting Nicolin Chen <nicoleotsuka@gmail.com>:

> 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 <nicoleotsuka@gmail.com>
> ---
>  * 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(+)
>
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 10e8347a3c80..9bbac826e50b 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -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)
>  {
> @@ -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;
>
> +		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

> +
>  		ret = ina3221_read_value(ina, reg, &regval);
>  		if (ret)
>  			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

Re: [PATCH v2 5/5] hwmon: (ina3221) Add PM runtime support

[linux]

Quoting Nicolin Chen <nicoleotsuka@gmail.com>:

> 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 <nicoleotsuka@gmail.com>
> ---
> 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(-)
>
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 9bbac826e50b..2cdc37ab0cf3 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -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;
> +
> +	/* For disabling routine, decrease refcount or suspend() at last */
> +	if (!enable)
> +		pm_runtime_put_sync(ina->hdev);
>
>  	return 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.

> +		pm_runtime_put_sync(ina->hdev);
> +	}
> +
> +	return ret;
>  }
>
>  static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
> @@ -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;
> +	}
> +
> +	mutex_unlock(&ina->lock);
> +
>  	return 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.

> +fail_lock:
> +	mutex_unlock(&ina->lock);
> +	mutex_destroy(&ina->lock);
> +
> +	return ret;
>  }
>
>  static int ina3221_remove(struct i2c_client *client)
>  {
>  	struct ina3221_data *ina = dev_get_drvdata(&client->dev);
> +	int i;
> +
> +	pm_runtime_disable(ina->hdev);
> +	pm_runtime_set_suspended(ina->hdev);
> +
> +	/* 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).

>  	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);
>  	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);
>
>  	/* 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

Re: [PATCH v2 4/5] hwmon: (ina3221) Make sure data is ready before reading

[Nicolin Chen]

On Wed, Oct 24, 2018 at 09:10:35AM +0000, linux@roeck-us.net wrote:
> > @@ -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;
> > 
> > +		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.

Will do that.

Thanks
Nicolin

> 
> Thanks,
> Guenter
> 
> > +
> >  		ret = ina3221_read_value(ina, reg, &regval);
> >  		if (ret)
> >  			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
> 
> 
> 

Re: [PATCH v2 5/5] hwmon: (ina3221) Add PM runtime support

[Nicolin Chen]

On Wed, Oct 24, 2018 at 09:24:18AM +0000, linux@roeck-us.net wrote:
> > +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.

Will fix in v3.

> > +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.
> 
> > +	/* 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).

The API will stop decrease at 0. But I will see if I can explicitly
loop a matched number, or at lest will mention this in comments.

Thanks
Nicolin