hwmon: (ina3221) Implement PM runtime to save power
v3 cover hwmon: (ina3221) Implement PM runtime to save power
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.
Changelogv2->v3:
* Dropped timeout dev_err messages as it's indicated in errno (PATCH-4)
* Improved a dev_err message and comments (PATCH-5)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 | 210 ++++++++++++++++++++++++++++++++++------ 2 files changed, 189 insertions(+), 28 deletions(-)
Signature · 1 line
2.17.1
PATCH v3 1/5 hwmon: (core) Inherit power properties to hdev
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)
---
Changelogv2->v3:
* N/Av1->v2:
* Added device pointers
drivers/hwmon/hwmon.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/hwmon/hwmon.c +6 -1
@@ -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);
Signature · 1 line
2.17.1
[email protected]
maintainerRe: [PATCH v3 1/5] hwmon: (core) Inherit power properties to hdev
Quoting Nicolin Chen <[email protected]>:
Quoted 29 lines
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_domainand driver pointers.Note that the
dev->driverpointer is the place that contains adev_pm_opspointer 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 <[email protected]>---
Changelogv2->v3:
* N/Av1->v2:
* Added device pointersdrivers/hwmon/hwmon.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/hwmon/hwmon.c +0
-0@@ -625,7 +625,12 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
drivers/hwmon/hwmon.c __hwmon_device_register(struct device *dev, · line 634
Quoted 10 lines
drivers/hwmon/hwmon.c +6
-1hwdev->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; + }
We'l need to dig into this more; I suspect it may be inappropriate to do this. With this change, every hwmon driver supporting (runtime ?) suspend/resume will have the problem worked around in #5, and that just seems wrong.
Guenter
Quoted 5 lines
drivers/hwmon/hwmon.c +0
-0hwdev->chip = chip;dev_set_drvdata (hdev, drvdata);
dev_set_name(hdev, HWMON_ID_FORMAT, id);
--
2.17.1
Re: [PATCH v3 1/5] hwmon: (core) Inherit power properties to hdev
Quoted 10 lines · On Thu, Oct 25, 2018 at 12:13:01AM +0000, [email protected] wrote:
drivers/hwmon/hwmon.c __hwmon_device_register(struct device *dev, const char *name, void *drvdata, · line 633
Quoted 6 lines
drivers/hwmon/hwmon.c +6
-0+if (dev) { + hdev->driver = dev->driver; + hdev->power = dev->power; + hdev->pm_domain = dev->pm_domain; + hdev->of_node = dev->of_node; + }We'l need to dig into this more; I suspect it may be inappropriate to do this. With this change, every hwmon driver supporting (runtime ?) suspend/resume will have the problem worked around in #5, and that just seems wrong.
Hmm..that's true...thanks for catching it.
Actually I am not sure the reason of having a child device in the core, but could we use the parent dev pointer in the hwmon core as hwmon_dev upon confirming parent dev pointer != NULL?
The problem here is that the power directory under each hwmon directory is tied to the hwmon_dev pointer, not to the parent dev pointer, and the hwmon core creates all sysfs nodes based on the child node. So those nodes under power directory won't be valid unless we copy all pm information, especially PM ops.
There is an option of ignoring this problem though, while all hwmon drivers will need to be careful of mixing using the dev pointers. So it'd be a lot of easier if we could just use the original dev pointer in the core since we mainly just need to create sysfs nodes.
Another way of doing this might be to pass down the PM pointer via _info structure instead of linking it to the parent driver, which then will forbid all hwmon drivers having its own PM ops callbacks -- the very opposite way of this patch, and it does not sound fully reasonable and feasible to me...
What do you think about?
Thanks
Nicolin
Re: [PATCH v3 1/5] hwmon: (core) Inherit power properties to hdev
Quoted 13 lines · Nicolin Chen
Quoted 10 lines · On Thu, Oct 25, 2018 at 12:13:01AM +0000, [email protected] wrote:
drivers/hwmon/hwmon.c __hwmon_device_register(struct device *dev, const char *name, void *drvdata, · line 633
Quoted 6 lines
drivers/hwmon/hwmon.c +6
-0+if (dev) { + hdev->driver = dev->driver; + hdev->power = dev->power; + hdev->pm_domain = dev->pm_domain; + hdev->of_node = dev->of_node; + }We'l need to dig into this more; I suspect it may be inappropriate to do this. With this change, every hwmon driver supporting (runtime ?) suspend/resume will have the problem worked around in #5, and that just seems wrong.
Quoted 5 lines
Hmm..that's true...thanks for catching it.
Actually I am not sure the reason of having a child device in the core, but could we use the parent dev pointer in the hwmon core as
hwmon_devupon confirming parent dev pointer != NULL?
I just noticed that it is used to link to hwmon class. So this won't work then. I guess it'd be safer to ignore the problem of the power node, i.e. using parent dev pointer for pm runtime.
Thanks
Nicolin
Quoted 22 lines
The problem here is that the power directory under each hwmon directory is tied to the
hwmon_devpointer, not to the parent dev pointer, and the hwmon core creates all sysfs nodes based on the child node. So those nodes under power directory won't be valid unless we copy all pm information, especially PM ops.There is an option of ignoring this problem though, while all hwmon drivers will need to be careful of mixing using the dev pointers. So it'd be a lot of easier if we could just use the original dev pointer in the core since we mainly just need to create sysfs nodes.
Another way of doing this might be to pass down the PM pointer via
_infostructure instead of linking it to the parent driver, which then will forbid all hwmon drivers having its own PM ops callbacks -- the very opposite way of this patch, and it does not sound fully reasonable and feasible to me...What do you think about?
Thanks
Nicolin
[email protected]
maintainerRe: [PATCH v3 1/5] hwmon: (core) Inherit power properties to hdev
Quoting Nicolin Chen <[email protected]>:
Quoted 15 lines · Nicolin Chen
Quoted 14 lines · On Thu, Oct 25, 2018 at 12:13:01AM +0000, [email protected] wrote:
Quoted 9 lines
drivers/hwmon/hwmon.c __hwmon_device_register(struct device *dev, const char *name, void *drvdata, · line 633
Quoted 7 lines
drivers/hwmon/hwmon.c +6
-0+if (dev) { + hdev->driver = dev->driver; + hdev->power = dev->power; + hdev->pm_domain = dev->pm_domain; + hdev->of_node = dev->of_node; + }We'l need to dig into this more; I suspect it may be
inappropriate to do this.
Quoted 2 lines
With this change, every hwmon driver supporting (runtime ?) suspend/resume will have the problem worked around in #5, and that just seems wrong.
Quoted 7 lines
Quoted 5 lines
Hmm..that's true...thanks for catching it.
Actually I am not sure the reason of having a child device in the core, but could we use the parent dev pointer in the hwmon core as
hwmon_devupon confirming parent dev pointer != NULL?I just noticed that it is used to link to hwmon class. So this
Exactly.
Quoted 3 lines
won't work then. I guess it'd be safer to ignore the problem of the power node, i.e. using parent dev pointer for pm runtime.
It might be worthwhile looking up how other virtal devices handle this problem. Maybe the hwmon code could have its own suspend/resume callbacks. Not sure how to make that work, though, and what those callbacks would (have to) do.
Guenter
Quoted 25 lines
Thanks
NicolinQuoted 22 lines
The problem here is that the power directory under each hwmon directory is tied to the
hwmon_devpointer, not to the parent dev pointer, and the hwmon core creates all sysfs nodes based on the child node. So those nodes under power directory won't be valid unless we copy all pm information, especially PM ops.There is an option of ignoring this problem though, while all hwmon drivers will need to be careful of mixing using the dev pointers. So it'd be a lot of easier if we could just use the original dev pointer in the core since we mainly just need to create sysfs nodes.
Another way of doing this might be to pass down the PM pointer via
_infostructure instead of linking it to the parent driver, which then will forbid all hwmon drivers having its own PM ops callbacks -- the very opposite way of this patch, and it does not sound fully reasonable and feasible to me...What do you think about?
Thanks
Nicolin
Re: [PATCH v3 1/5] hwmon: (core) Inherit power properties to hdev
Quoted 7 lines · On Thu, Oct 25, 2018 at 06:55:31AM +0000, [email protected] wrote:
Quoted 3 lines
won't work then. I guess it'd be safer to ignore the problem of the power node, i.e. using parent dev pointer for pm runtime.
It might be worthwhile looking up how other virtal devices handle this problem. Maybe the hwmon code could have its own suspend/resume callbacks. Not sure how to make that work, though, and what those callbacks would (have to) do.
I am adding a core pm pointer to the class. A hwmon driver will also need to pass a private pm pointer in a chip/info structure. I will send it soon.
Thanks
Nicolin
PATCH v3 2/5 hwmon: (ina3221) Check channel status for alarms attribute read
In reply to Nicolin ChenThere 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.
---
Changelogv2->v3:
* N/Av1->v2:
* Returned 0 for alert flags instead of -ENODATA
drivers/hwmon/ina3221.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/hwmon/ina3221.c +6 -0
@@ -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], ®val);if (ret)return ret;
Signature · 1 line
2.17.1
PATCH v3 3/5 hwmon: (ina3221) Serialize sysfs ABI accesses
In reply to Nicolin ChenThis 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.
---
Changlogv1->v3:
* N/A
drivers/hwmon/ina3221.c | 51 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 6 deletions(-)
drivers/hwmon/ina3221.c +45 -6
@@ -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,
Signature · 1 line
2.17.1
PATCH v3 4/5 hwmon: (ina3221) Make sure data is ready before reading
In reply to Nicolin ChenThe 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.
---
Changelogv2->v3:
* Dropped timeout dev_err messages as it's indicated in the errnov1->v2:
* Moved CVRF polling to data read routine
* Added calculation of wait time based on conversion time setting
drivers/hwmon/ina3221.c | 42 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
drivers/hwmon/ina3221.c +42 -0
@@ -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,10 @@ 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) +return ret; + ret =ina3221_read_value (ina, reg, ®val);if (ret)return ret;
@@ -189,6 +226,11 @@ 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) +return ret; +/* fall through */ case hwmon_curr_crit:case hwmon_curr_max:
Signature · 1 line
2.17.1
PATCH v3 5/5 hwmon: (ina3221) Add PM runtime support
In reply to Nicolin ChenIf all three channels are disabled via in[123]_enable ABI, the driver could suspend the chip for power saving purpose.
So this patch adds 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() instead, as they're similar. It's also necessary to do so to match initial PM 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.
---
Changelogv2->v3:
* Improved a dev_err message
* Added comments at pm_runtime_put_noidle() callbacks
* Added pm_runtime header file in an alphabetical orderv1->v2:
* Bypassed i2c_client->dev in suspend/resume()
* Added a missing '\n' in one dev_err()
drivers/hwmon/ina3221.c | 113 ++++++++++++++++++++++++++++++++-------- 1 file changed, 91 insertions(+), 22 deletions(-)
drivers/hwmon/ina3221.c +91 -22
@@ -20,6 +20,7 @@#include <linux/module.h> #include <linux/mutex.h> #include <linux/of.h> +#include <linux/pm_runtime.h> #include <linux/regmap.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 10000enum 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 */
@@ -290,21 +295,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,"Failed to enable channel %d: error %d\n" , + channel, ret); +pm_runtime_put_sync (ina->hdev); + } + +return ret; } static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
@@ -599,7 +631,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 );
@@ -631,44 +662,72 @@ 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); +/* pm_runtime_put_noidle() will decrease the PM refcount until 0 */ +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); + +/* pm_runtime_put_noidle() will decrease the PM refcount until 0 */ +for (i =0 ; i <INA3221_NUM_CHANNELS ; i++) +pm_runtime_put_noidle (ina->hdev);mutex_destroy (&ina->lock);
@@ -680,6 +739,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)
@@ -703,6 +766,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 */
@@ -726,7 +793,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[] = {
Signature · 1 line
2.17.1