hwmon: ina3221: Add suspend and resume functions
cover hwmon: ina3221: Add suspend and resume functions
This series of patches add system suspend and resume functiosn for ina3221 hwmon driver.
Nicolin Chen (2):
hwmon: ina3221: Add INA3221_CONFIG to volatile_table
hwmon: ina3221: Add suspend and resume functions
drivers/hwmon/ina3221.c | 56 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)
Signature · 1 line
2.17.1
PATCH 1/2 hwmon: ina3221: Add INA3221_CONFIG to volatile_table
The MSB (15th bit) of INA3221_CONFIG is a self-clear reset bit. So this register should be added to the volatile_table of the regmap_config. Otherwise, we will see this bit is sticky in the regcache which might accidentally reset the chip when an actual write happens to the register.
This might not be a severe bug for the current code line since there's no second place touching the INA3221_CONFIG except the reset routine in the probe().
--- drivers/hwmon/ina3221.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/hwmon/ina3221.c +1 -1
@@ -353,7 +353,7 @@ static struct attribute *ina3221_attrs[] = {ATTRIBUTE_GROUPS (ina3221);static const struct regmap_range ina3221_yes_ranges[] = { -regmap_reg_range (INA3221_SHUNT1 ,INA3221_BUS3 ), +regmap_reg_range (INA3221_CONFIG ,INA3221_BUS3 ),regmap_reg_range (INA3221_MASK_ENABLE ,INA3221_MASK_ENABLE ), };
Signature · 1 line
2.17.1
PATCH 2/2 hwmon: ina3221: Add suspend and resume functions
In reply to Nicolin ChenDepending on the hardware design, an INA3221 chip might lose its power during system suspend/resume. So this patch adds a pair of suspend and resume functions to cache the register values including config register value and limit settings.
--- drivers/hwmon/ina3221.c | 54 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)
drivers/hwmon/ina3221.c +54 -0
@@ -91,11 +91,13 @@ static const unsigned int register_channel[] = {* @regmap: Register map of the device * @fields: Register fields of the device * @shunt_resistors: Array of resistor values per channel +* @reg_config: Register value of INA3221_CONFIG */ struct ina3221_data {struct regmap *regmap;struct regmap_field *fields[F_MAX_FIELDS ];int shunt_resistors[INA3221_NUM_CHANNELS ]; +u32 reg_config; }; static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
@@ -415,8 +417,59 @@ static int ina3221_probe(struct i2c_client *client,return PTR_ERR (hwmon_dev); } +dev_set_drvdata (dev, ina); + +return 0 ; +} + +#ifdef CONFIG_PM +static int ina3221_suspend (struct device *dev) +{ +struct ina3221_data *ina =dev_get_drvdata (dev); +int ret; + +/* Save config register value and enable cache-only */ + ret =regmap_read (ina->regmap,INA3221_CONFIG , &ina->reg_config); +if (ret) +return ret; + +regcache_cache_only (ina->regmap, true); +regcache_mark_dirty (ina->regmap); + +return 0 ; +} + +static int ina3221_resume (struct device *dev) +{ +struct ina3221_data *ina =dev_get_drvdata (dev); +int ret; + +regcache_cache_only (ina->regmap, false); + +/* Software reset the chip */ + ret =regmap_field_write (ina->fields[F_RST ], true); +if (ret) { +dev_err (dev,"Unable to reset device\n" ); +return ret; + } + +/* Restore cached register values to hardware */ + ret =regcache_sync (ina->regmap); +if (ret) +return ret; + +/* Restore config register value to hardware */ + ret =regmap_write (ina->regmap,INA3221_CONFIG , ina->reg_config); +if (ret) +return ret; +return 0 ; } +#endif + +static const struct dev_pm_ops ina3221_pm = { +SET_SYSTEM_SLEEP_PM_OPS (ina3221_suspend, ina3221_resume) +};static const struct of_device_id ina3221_of_match_table[] = { { .compatible ="ti,ina3221" , },
@@ -435,6 +488,7 @@ static struct i2c_driver ina3221_i2c_driver = { .driver = { .name = INA3221_DRIVER_NAME, .of_match_table = ina3221_of_match_table, + .pm = &ina3221_pm, }, .id_table = ina3221_ids, };
Signature · 1 line
2.17.1
Guenter Roeck
maintainerQuoted 38 lines · Nicolin Chen
Depending on the hardware design, an INA3221 chip might lose its power during system suspend/resume. So this patch adds a pair of suspend and resume functions to cache the register values including config register value and limit settings.
Signed-off-by Nicolin Chen <[email protected]>--- drivers/hwmon/ina3221.c | 54 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)drivers/hwmon/ina3221.c +8
-0@@ -91,11 +91,13 @@ static const unsigned int register_channel[] = {* @regmap: Register map of the device * @fields: Register fields of the device * @shunt_resistors: Array of resistor values per channel +* @reg_config: Register value of INA3221_CONFIG */ struct ina3221_data {struct regmap *regmap;struct regmap_field *fields[F_MAX_FIELDS ];int shunt_resistors[INA3221_NUM_CHANNELS ]; +u32 reg_config; }; static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,@@ -415,8 +417,59 @@ static int ina3221_probe(struct i2c_client *client,return PTR_ERR (hwmon_dev); } +dev_set_drvdata (dev, ina); + +return 0 ; +} + +#ifdef CONFIG_PM
drivers/hwmon/ina3221.c static int ina3221_probe(struct i2c_client *client, · line 435
Quoted 10 lines
drivers/hwmon/ina3221.c +10
-0+static int ina3221_suspend (struct device *dev) +{ +struct ina3221_data *ina =dev_get_drvdata (dev); +int ret; + +/* Save config register value and enable cache-only */ + ret =regmap_read (ina->regmap,INA3221_CONFIG , &ina->reg_config); +if (ret) +return ret; +
Would it make sense to explicitly disable all channels here ?
Thanks,
Guenter
Quoted 51 lines
drivers/hwmon/ina3221.c +36
-0+regcache_cache_only (ina->regmap, true); +regcache_mark_dirty (ina->regmap); + +return 0 ; +} + +static int ina3221_resume (struct device *dev) +{ +struct ina3221_data *ina =dev_get_drvdata (dev); +int ret; + +regcache_cache_only (ina->regmap, false); + +/* Software reset the chip */ + ret =regmap_field_write (ina->fields[F_RST ], true); +if (ret) { +dev_err (dev,"Unable to reset device\n" ); +return ret; + } + +/* Restore cached register values to hardware */ + ret =regcache_sync (ina->regmap); +if (ret) +return ret; + +/* Restore config register value to hardware */ + ret =regmap_write (ina->regmap,INA3221_CONFIG , ina->reg_config); +if (ret) +return ret; +return 0 ; } +#endif + +static const struct dev_pm_ops ina3221_pm = { +SET_SYSTEM_SLEEP_PM_OPS (ina3221_suspend, ina3221_resume) +};static const struct of_device_id ina3221_of_match_table[] = { { .compatible ="ti,ina3221" , },@@ -435,6 +488,7 @@ static struct i2c_driver ina3221_i2c_driver = { .driver = { .name = INA3221_DRIVER_NAME, .of_match_table = ina3221_of_match_table, + .pm = &ina3221_pm, }, .id_table = ina3221_ids, };Signature · 1 line
2.17.1
Quoted 43 lines · Guenter Roeck
Quoted 42 lines · Nicolin Chen
Depending on the hardware design, an INA3221 chip might lose its power during system suspend/resume. So this patch adds a pair of suspend and resume functions to cache the register values including config register value and limit settings.
Signed-off-by Nicolin Chen <[email protected]>--- drivers/hwmon/ina3221.c | 54 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)drivers/hwmon/ina3221.c +12
-0@@ -91,11 +91,13 @@ static const unsigned int register_channel[] = {* @regmap: Register map of the device * @fields: Register fields of the device * @shunt_resistors: Array of resistor values per channel +* @reg_config: Register value of INA3221_CONFIG */ struct ina3221_data {struct regmap *regmap;struct regmap_field *fields[F_MAX_FIELDS ];int shunt_resistors[INA3221_NUM_CHANNELS ]; +u32 reg_config; }; static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,@@ -415,8 +417,59 @@ static int ina3221_probe(struct i2c_client *client,return PTR_ERR (hwmon_dev); } +dev_set_drvdata (dev, ina); + +return 0 ; +} + +#ifdef CONFIG_PM +static int ina3221_suspend (struct device *dev) +{ +struct ina3221_data *ina =dev_get_drvdata (dev); +int ret;
Quoted 8 lines
drivers/hwmon/ina3221.c static int ina3221_probe(struct i2c_client *client, · line 435
Quoted 6 lines
drivers/hwmon/ina3221.c +6
-0+ +/* Save config register value and enable cache-only */ + ret =regmap_read (ina->regmap,INA3221_CONFIG , &ina->reg_config); +if (ret) +return ret; +Would it make sense to explicitly disable all channels here ?
For power saving? I could add it to v2.
Thanks
Nicolin