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

[PATCH 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.

Nicolin Chen (5):
  hwmon: (core) Inherit power properties to hdev
  hwmon: (ina3221) Return -ENODATA for two alarms attributes
  hwmon: (ina3221) Serialize sysfs ABI accesses
  hwmon: (ina3221) Make sure data is ready after channel enabling
  hwmon: (ina3221) Add PM runtime support

 drivers/hwmon/hwmon.c   |   3 +
 drivers/hwmon/ina3221.c | 185 ++++++++++++++++++++++++++++++++++------
 2 files changed, 162 insertions(+), 26 deletions(-)

-- 
2.17.1

[PATCH 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>
---
 drivers/hwmon/hwmon.c | 3 +++
 1 file changed, 3 insertions(+)

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

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

[Guenter Roeck]

On Tue, Oct 16, 2018 at 06:24:22PM -0700, Nicolin Chen wrote:
> 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>
> ---
>  drivers/hwmon/hwmon.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 975c95169884..7c064e1218ba 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -625,6 +625,9 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>  	hwdev->name = name;
>  	hdev->class = &hwmon_class;
>  	hdev->parent = dev;
> +	hdev->driver = dev->driver;
> +	hdev->power = dev->power;
> +	hdev->pm_domain = dev->pm_domain;

dev can, unfortunately, be NULL

>  	hdev->of_node = dev ? dev->of_node : NULL;

... as you can see here.

Guenter

>  	hwdev->chip = chip;
>  	dev_set_drvdata(hdev, drvdata);
> -- 
> 2.17.1
> 

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

[Nicolin Chen]

On Wed, Oct 17, 2018 at 12:44:30PM -0700, Guenter Roeck wrote:

> > +	hdev->driver = dev->driver;
> > +	hdev->power = dev->power;
> > +	hdev->pm_domain = dev->pm_domain;
> 
> dev can, unfortunately, be NULL
> 
> >  	hdev->of_node = dev ? dev->of_node : NULL;
> 
> ... as you can see here.

Oops..will fix it.

Thanks!

> 
> Guenter
> 
> >  	hwdev->chip = chip;
> >  	dev_set_drvdata(hdev, drvdata);
> > -- 
> > 2.17.1
> > 

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

[Dan Carpenter]

Hi Nicolin,

I love your patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Nicolin-Chen/hwmon-ina3221-Implement-PM-runtime-to-save-power/20181018-010754
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next

smatch warnings:
drivers/hwmon/hwmon.c:631 __hwmon_device_register() warn: variable dereferenced before check 'dev' (see line 628)

# https://github.com/0day-ci/linux/commit/aa912316f2d30d4e699ac2f11b6197a4da011274
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout aa912316f2d30d4e699ac2f11b6197a4da011274
vim +/dev +631 drivers/hwmon/hwmon.c

d560168b Guenter Roeck    2015-08-26  562  
d560168b Guenter Roeck    2015-08-26  563  static struct device *
d560168b Guenter Roeck    2015-08-26  564  __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
d560168b Guenter Roeck    2015-08-26  565  			const struct hwmon_chip_info *chip,
bab2243c Guenter Roeck    2013-07-06  566  			const struct attribute_group **groups)
1236441f Mark M. Hoffman  2005-07-15  567  {
bab2243c Guenter Roeck    2013-07-06  568  	struct hwmon_device *hwdev;
d560168b Guenter Roeck    2015-08-26  569  	struct device *hdev;
d560168b Guenter Roeck    2015-08-26  570  	int i, j, err, id;
ded2b666 Mark M. Hoffman  2006-03-05  571  
74d3b641 Guenter Roeck    2017-01-27  572  	/* Complain about invalid characters in hwmon name attribute */
648cd48c Guenter Roeck    2014-02-28  573  	if (name && (!strlen(name) || strpbrk(name, "-* \t\n")))
74d3b641 Guenter Roeck    2017-01-27  574  		dev_warn(dev,
74d3b641 Guenter Roeck    2017-01-27  575  			 "hwmon: '%s' is not a valid name attribute, please fix\n",
74d3b641 Guenter Roeck    2017-01-27  576  			 name);
648cd48c Guenter Roeck    2014-02-28  577  
4ca5f468 Jonathan Cameron 2011-10-31  578  	id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL);
4ca5f468 Jonathan Cameron 2011-10-31  579  	if (id < 0)
4ca5f468 Jonathan Cameron 2011-10-31  580  		return ERR_PTR(id);
1236441f Mark M. Hoffman  2005-07-15  581  
bab2243c Guenter Roeck    2013-07-06  582  	hwdev = kzalloc(sizeof(*hwdev), GFP_KERNEL);
bab2243c Guenter Roeck    2013-07-06  583  	if (hwdev == NULL) {
bab2243c Guenter Roeck    2013-07-06  584  		err = -ENOMEM;
bab2243c Guenter Roeck    2013-07-06  585  		goto ida_remove;
bab2243c Guenter Roeck    2013-07-06  586  	}
1236441f Mark M. Hoffman  2005-07-15  587  
d560168b Guenter Roeck    2015-08-26  588  	hdev = &hwdev->dev;
d560168b Guenter Roeck    2015-08-26  589  
239552f4 Guenter Roeck    2016-10-16  590  	if (chip) {
d560168b Guenter Roeck    2015-08-26  591  		struct attribute **attrs;
b2a4cc3a Guenter Roeck    2016-10-16  592  		int ngroups = 2; /* terminating NULL plus &hwdev->groups */
d560168b Guenter Roeck    2015-08-26  593  
d560168b Guenter Roeck    2015-08-26  594  		if (groups)
d560168b Guenter Roeck    2015-08-26  595  			for (i = 0; groups[i]; i++)
d560168b Guenter Roeck    2015-08-26  596  				ngroups++;
d560168b Guenter Roeck    2015-08-26  597  
d560168b Guenter Roeck    2015-08-26  598  		hwdev->groups = devm_kcalloc(dev, ngroups, sizeof(*groups),
d560168b Guenter Roeck    2015-08-26  599  					     GFP_KERNEL);
38d8ed65 Colin Ian King   2016-10-23  600  		if (!hwdev->groups) {
38d8ed65 Colin Ian King   2016-10-23  601  			err = -ENOMEM;
38d8ed65 Colin Ian King   2016-10-23  602  			goto free_hwmon;
38d8ed65 Colin Ian King   2016-10-23  603  		}
d560168b Guenter Roeck    2015-08-26  604  
d560168b Guenter Roeck    2015-08-26  605  		attrs = __hwmon_create_attrs(dev, drvdata, chip);
d560168b Guenter Roeck    2015-08-26  606  		if (IS_ERR(attrs)) {
d560168b Guenter Roeck    2015-08-26  607  			err = PTR_ERR(attrs);
d560168b Guenter Roeck    2015-08-26  608  			goto free_hwmon;
d560168b Guenter Roeck    2015-08-26  609  		}
d560168b Guenter Roeck    2015-08-26  610  
d560168b Guenter Roeck    2015-08-26  611  		hwdev->group.attrs = attrs;
d560168b Guenter Roeck    2015-08-26  612  		ngroups = 0;
d560168b Guenter Roeck    2015-08-26  613  		hwdev->groups[ngroups++] = &hwdev->group;
d560168b Guenter Roeck    2015-08-26  614  
d560168b Guenter Roeck    2015-08-26  615  		if (groups) {
d560168b Guenter Roeck    2015-08-26  616  			for (i = 0; groups[i]; i++)
d560168b Guenter Roeck    2015-08-26  617  				hwdev->groups[ngroups++] = groups[i];
d560168b Guenter Roeck    2015-08-26  618  		}
d560168b Guenter Roeck    2015-08-26  619  
d560168b Guenter Roeck    2015-08-26  620  		hdev->groups = hwdev->groups;
d560168b Guenter Roeck    2015-08-26  621  	} else {
d560168b Guenter Roeck    2015-08-26  622  		hdev->groups = groups;
d560168b Guenter Roeck    2015-08-26  623  	}
d560168b Guenter Roeck    2015-08-26  624  
bab2243c Guenter Roeck    2013-07-06  625  	hwdev->name = name;
d560168b Guenter Roeck    2015-08-26  626  	hdev->class = &hwmon_class;
d560168b Guenter Roeck    2015-08-26  627  	hdev->parent = dev;
aa912316 Nicolin Chen     2018-10-16 @628  	hdev->driver = dev->driver;
                                                               ^^^^^^^^^^^
aa912316 Nicolin Chen     2018-10-16  629  	hdev->power = dev->power;
                                                              ^^^^^^^^^^
aa912316 Nicolin Chen     2018-10-16  630  	hdev->pm_domain = dev->pm_domain;
                                                                  ^^^^^^^^^^^^^^
The patch adds unchecked dereferences.

d560168b Guenter Roeck    2015-08-26 @631  	hdev->of_node = dev ? dev->of_node : NULL;
                                                                ^^^
The old code checks for NULL.

d560168b Guenter Roeck    2015-08-26  632  	hwdev->chip = chip;
d560168b Guenter Roeck    2015-08-26  633  	dev_set_drvdata(hdev, drvdata);
d560168b Guenter Roeck    2015-08-26  634  	dev_set_name(hdev, HWMON_ID_FORMAT, id);
d560168b Guenter Roeck    2015-08-26  635  	err = device_register(hdev);
bab2243c Guenter Roeck    2013-07-06  636  	if (err)
d560168b Guenter Roeck    2015-08-26  637  		goto free_hwmon;
d560168b Guenter Roeck    2015-08-26  638  
319fe159 Guenter Roeck    2017-01-31  639  	if (dev && chip && chip->ops->read &&
d560168b Guenter Roeck    2015-08-26  640  	    chip->info[0]->type == hwmon_chip &&
d560168b Guenter Roeck    2015-08-26  641  	    (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
d560168b Guenter Roeck    2015-08-26  642  		const struct hwmon_channel_info **info = chip->info;
d560168b Guenter Roeck    2015-08-26  643  
d560168b Guenter Roeck    2015-08-26  644  		for (i = 1; info[i]; i++) {
d560168b Guenter Roeck    2015-08-26  645  			if (info[i]->type != hwmon_temp)
d560168b Guenter Roeck    2015-08-26  646  				continue;
d560168b Guenter Roeck    2015-08-26  647  
d560168b Guenter Roeck    2015-08-26  648  			for (j = 0; info[i]->config[j]; j++) {
d560168b Guenter Roeck    2015-08-26  649  				if (!chip->ops->is_visible(drvdata, hwmon_temp,
d560168b Guenter Roeck    2015-08-26  650  							   hwmon_temp_input, j))
d560168b Guenter Roeck    2015-08-26  651  					continue;
47c332de Linus Walleij    2017-12-05  652  				if (info[i]->config[j] & HWMON_T_INPUT) {
47c332de Linus Walleij    2017-12-05  653  					err = hwmon_thermal_add_sensor(dev,
47c332de Linus Walleij    2017-12-05  654  								hwdev, j);
47c332de Linus Walleij    2017-12-05  655  					if (err)
47c332de Linus Walleij    2017-12-05  656  						goto free_device;
47c332de Linus Walleij    2017-12-05  657  				}
d560168b Guenter Roeck    2015-08-26  658  			}
d560168b Guenter Roeck    2015-08-26  659  		}
d560168b Guenter Roeck    2015-08-26  660  	}
bab2243c Guenter Roeck    2013-07-06  661  
d560168b Guenter Roeck    2015-08-26  662  	return hdev;
bab2243c Guenter Roeck    2013-07-06  663  
47c332de Linus Walleij    2017-12-05  664  free_device:
47c332de Linus Walleij    2017-12-05  665  	device_unregister(hdev);
d560168b Guenter Roeck    2015-08-26  666  free_hwmon:
bab2243c Guenter Roeck    2013-07-06  667  	kfree(hwdev);
bab2243c Guenter Roeck    2013-07-06  668  ida_remove:
4ca5f468 Jonathan Cameron 2011-10-31  669  	ida_simple_remove(&hwmon_ida, id);
bab2243c Guenter Roeck    2013-07-06  670  	return ERR_PTR(err);
bab2243c Guenter Roeck    2013-07-06  671  }
d560168b Guenter Roeck    2015-08-26  672  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes

[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>
---
 drivers/hwmon/ina3221.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index d61688f04594..3e98b59108ee 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -200,6 +200,8 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
 		return 0;
 	case hwmon_curr_crit_alarm:
 	case hwmon_curr_max_alarm:
+		if (!ina3221_is_enabled(ina, channel))
+			return -ENODATA;
 		ret = regmap_field_read(ina->fields[reg], &regval);
 		if (ret)
 			return ret;
-- 
2.17.1

Re: [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes

[Guenter Roeck]

On Tue, Oct 16, 2018 at 06:24:23PM -0700, Nicolin Chen wrote:
> 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>
> ---
>  drivers/hwmon/ina3221.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index d61688f04594..3e98b59108ee 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -200,6 +200,8 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
>  		return 0;
>  	case hwmon_curr_crit_alarm:
>  	case hwmon_curr_max_alarm:
> +		if (!ina3221_is_enabled(ina, channel))
> +			return -ENODATA;

Makes sense, but can you check what the sensors command does with this ?
If it bails out I'd rather have the code return 0 and no error (after all,
the sensor is disabled, so any alarm would be bogus).

Thanks,
Guenter

>  		ret = regmap_field_read(ina->fields[reg], &regval);
>  		if (ret)
>  			return ret;
> -- 
> 2.17.1
> 

Re: [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes

[Nicolin Chen]

On Wed, Oct 17, 2018 at 12:46:05PM -0700, Guenter Roeck wrote:
> On Tue, Oct 16, 2018 at 06:24:23PM -0700, Nicolin Chen wrote:
> > 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>
> > ---
> >  drivers/hwmon/ina3221.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> > index d61688f04594..3e98b59108ee 100644
> > --- a/drivers/hwmon/ina3221.c
> > +++ b/drivers/hwmon/ina3221.c
> > @@ -200,6 +200,8 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
> >  		return 0;
> >  	case hwmon_curr_crit_alarm:
> >  	case hwmon_curr_max_alarm:
> > +		if (!ina3221_is_enabled(ina, channel))
> > +			return -ENODATA;
> 
> Makes sense, but can you check what the sensors command does with this ?

Not quite understanding the question. Do you mean the user case
causing the race condition -- wiping out the CVRF bit?

> If it bails out I'd rather have the code return 0 and no error (after all,
> the sensor is disabled, so any alarm would be bogus).

That's true. Since they are alert flags, should return 0. I will
fix it.

Thanks

Re: [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes

[Guenter Roeck]

On Wed, Oct 17, 2018 at 01:39:17PM -0700, Nicolin Chen wrote:
> On Wed, Oct 17, 2018 at 12:46:05PM -0700, Guenter Roeck wrote:
> > On Tue, Oct 16, 2018 at 06:24:23PM -0700, Nicolin Chen wrote:
> > > 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>
> > > ---
> > >  drivers/hwmon/ina3221.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> > > index d61688f04594..3e98b59108ee 100644
> > > --- a/drivers/hwmon/ina3221.c
> > > +++ b/drivers/hwmon/ina3221.c
> > > @@ -200,6 +200,8 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
> > >  		return 0;
> > >  	case hwmon_curr_crit_alarm:
> > >  	case hwmon_curr_max_alarm:
> > > +		if (!ina3221_is_enabled(ina, channel))
> > > +			return -ENODATA;
> > 
> > Makes sense, but can you check what the sensors command does with this ?
> 
> Not quite understanding the question. Do you mean the user case
> causing the race condition -- wiping out the CVRF bit?
> 
No. Question is what the "sensors" command reports if reading the alarm
attribute returns -ENODATA. If it reports an error, we would have a regression.

Guenter

> > If it bails out I'd rather have the code return 0 and no error (after all,
> > the sensor is disabled, so any alarm would be bogus).
> 
> That's true. Since they are alert flags, should return 0. I will
> fix it.
> 
> Thanks

Re: [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes

[Nicolin Chen]

On Wed, Oct 17, 2018 at 02:14:26PM -0700, Guenter Roeck wrote:
> On Wed, Oct 17, 2018 at 01:39:17PM -0700, Nicolin Chen wrote:
> > On Wed, Oct 17, 2018 at 12:46:05PM -0700, Guenter Roeck wrote:
> > > On Tue, Oct 16, 2018 at 06:24:23PM -0700, Nicolin Chen wrote:
> > > > 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>
> > > > ---
> > > >  drivers/hwmon/ina3221.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> > > > index d61688f04594..3e98b59108ee 100644
> > > > --- a/drivers/hwmon/ina3221.c
> > > > +++ b/drivers/hwmon/ina3221.c
> > > > @@ -200,6 +200,8 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
> > > >  		return 0;
> > > >  	case hwmon_curr_crit_alarm:
> > > >  	case hwmon_curr_max_alarm:
> > > > +		if (!ina3221_is_enabled(ina, channel))
> > > > +			return -ENODATA;
> > > 
> > > Makes sense, but can you check what the sensors command does with this ?
> > 
> > Not quite understanding the question. Do you mean the user case
> > causing the race condition -- wiping out the CVRF bit?
> > 
> No. Question is what the "sensors" command reports if reading the alarm
> attribute returns -ENODATA. If it reports an error, we would have a regression.

I see. I will return 0 instead.

Thanks
Nicolin

[PATCH 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 3e98b59108ee..5fc375bf6635 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;
 };
 
@@ -261,29 +264,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,
@@ -578,6 +605,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,
@@ -585,12 +613,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);
@@ -659,6 +697,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 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling

[Nicolin Chen]

The data might need some time to get ready after channel enabling,
although the data register is readable without CVRF being set. So
this patch adds a CVRF polling to make sure that data register is
updated with the new data.

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

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 5fc375bf6635..160ddc404d73 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -45,6 +45,7 @@
 #define INA3221_CONFIG_MODE_BUS		BIT(1)
 #define INA3221_CONFIG_MODE_CONTINUOUS	BIT(2)
 #define INA3221_CONFIG_CHx_EN(x)	BIT(14 - (x))
+#define INA3221_CONFIG_CHs_EN_MASK	GENMASK(14, 12)
 
 #define INA3221_RSHUNT_DEFAULT		10000
 
@@ -52,6 +53,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 +67,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 +116,19 @@ static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
 	return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
 }
 
+static inline int ina3221_wait_for_data_if_active(struct ina3221_data *ina)
+{
+	u32 cvrf;
+
+	/* No need to wait if all channels are disabled */
+	if ((ina->reg_config & INA3221_CONFIG_CHs_EN_MASK) == 0)
+		return 0;
+
+	/* Polling the CVRF bit to make sure read data is ready */
+	return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
+					      cvrf, cvrf, 100, 100000);
+}
+
 static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
 			      int *val)
 {
@@ -258,6 +276,13 @@ static int ina3221_write_enable(struct device *dev, int channel, bool enable)
 	if (ret)
 		return ret;
 
+	/* Make sure data conversion is finished */
+	ret = ina3221_wait_for_data_if_active(ina);
+	if (ret) {
+		dev_err(dev, "Timed out at waiting for CVRF bit\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -676,6 +701,13 @@ static int __maybe_unused ina3221_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	/* Make sure data conversion is finished */
+	ret = ina3221_wait_for_data_if_active(ina);
+	if (ret) {
+		dev_err(dev, "Timed out at waiting for CVRF bit\n");
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling

[Guenter Roeck]

On Tue, Oct 16, 2018 at 06:24:25PM -0700, Nicolin Chen wrote:
> The data might need some time to get ready after channel enabling,
> although the data register is readable without CVRF being set. So
> this patch adds a CVRF polling to make sure that data register is
> updated with the new data.
> 
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>  drivers/hwmon/ina3221.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 5fc375bf6635..160ddc404d73 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -45,6 +45,7 @@
>  #define INA3221_CONFIG_MODE_BUS		BIT(1)
>  #define INA3221_CONFIG_MODE_CONTINUOUS	BIT(2)
>  #define INA3221_CONFIG_CHx_EN(x)	BIT(14 - (x))
> +#define INA3221_CONFIG_CHs_EN_MASK	GENMASK(14, 12)
>  
>  #define INA3221_RSHUNT_DEFAULT		10000
>  
> @@ -52,6 +53,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 +67,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 +116,19 @@ static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
>  	return ina->reg_config & INA3221_CONFIG_CHx_EN(channel);
>  }
>  
> +static inline int ina3221_wait_for_data_if_active(struct ina3221_data *ina)
> +{
> +	u32 cvrf;
> +
> +	/* No need to wait if all channels are disabled */
> +	if ((ina->reg_config & INA3221_CONFIG_CHs_EN_MASK) == 0)
> +		return 0;
> +
> +	/* Polling the CVRF bit to make sure read data is ready */
> +	return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
> +					      cvrf, cvrf, 100, 100000);
> +}
> +
>  static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
>  			      int *val)
>  {
> @@ -258,6 +276,13 @@ static int ina3221_write_enable(struct device *dev, int channel, bool enable)
>  	if (ret)
>  		return ret;
>  
> +	/* Make sure data conversion is finished */
> +	ret = ina3221_wait_for_data_if_active(ina);
> +	if (ret) {
> +		dev_err(dev, "Timed out at waiting for CVRF bit\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -676,6 +701,13 @@ static int __maybe_unused ina3221_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Make sure data conversion is finished */
> +	ret = ina3221_wait_for_data_if_active(ina);

This is quite expensive and would delay resume (and enable, for that matter).
A less expensive solution would be to save the enable time here and in
ina3221_write_enable(). ina3221_wait_for_data_if_active() can then be called
from read functions and would wait if not enough time has expired.

	if (time_before(...))
		usleep_range(missing wait time, missing_wait_time * 2);

or something like that. This could be done per channel or, to keep
things simple, just using a single time for all channels.

Thanks,
Guenter

> +	if (ret) {
> +		dev_err(dev, "Timed out at waiting for CVRF bit\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling

[Nicolin Chen]

Hello Guenter,

On Wed, Oct 17, 2018 at 09:55:43AM -0700, Guenter Roeck wrote:
> > @@ -676,6 +701,13 @@ static int __maybe_unused ina3221_resume(struct device *dev)
> >  	if (ret)
> >  		return ret;
> >  
> > +	/* Make sure data conversion is finished */
> > +	ret = ina3221_wait_for_data_if_active(ina);
> 
> This is quite expensive and would delay resume (and enable, for that matter).
> A less expensive solution would be to save the enable time here and in
> ina3221_write_enable(). ina3221_wait_for_data_if_active() can then be called
> from read functions and would wait if not enough time has expired.
> 
> 	if (time_before(...))
> 		usleep_range(missing wait time, missing_wait_time * 2);
> 
> or something like that. This could be done per channel or, to keep
> things simple, just using a single time for all channels.

I was thinking something that'd fit one-shot mode too so decided
to add a polling. But you are right. It does make sense to skip
polling until an actual read happens, though it also feels a bit
reasonable to me that putting a polling to the enabling routine.

The wait time would be sightly more complicated than the polling
actually, as it needs to involve total conversion time which may
vary depending on the number of enabled channels. I will see what
would be safer and easier and apply that in v2.

Thanks
Nicolin

Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling

[Guenter Roeck]

On Wed, Oct 17, 2018 at 01:53:48PM -0700, Nicolin Chen wrote:
> Hello Guenter,
> 
> On Wed, Oct 17, 2018 at 09:55:43AM -0700, Guenter Roeck wrote:
> > > @@ -676,6 +701,13 @@ static int __maybe_unused ina3221_resume(struct device *dev)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	/* Make sure data conversion is finished */
> > > +	ret = ina3221_wait_for_data_if_active(ina);
> > 
> > This is quite expensive and would delay resume (and enable, for that matter).
> > A less expensive solution would be to save the enable time here and in
> > ina3221_write_enable(). ina3221_wait_for_data_if_active() can then be called
> > from read functions and would wait if not enough time has expired.
> > 
> > 	if (time_before(...))
> > 		usleep_range(missing wait time, missing_wait_time * 2);
> > 
> > or something like that. This could be done per channel or, to keep
> > things simple, just using a single time for all channels.
> 
> I was thinking something that'd fit one-shot mode too so decided
> to add a polling. But you are right. It does make sense to skip
> polling until an actual read happens, though it also feels a bit
> reasonable to me that putting a polling to the enabling routine.
> 
> The wait time would be sightly more complicated than the polling
> actually, as it needs to involve total conversion time which may
> vary depending on the number of enabled channels. I will see what
> would be safer and easier and apply that in v2.
> 
It isn't that complex; we have done it in other drivers. It is less
costly and has less overhead than extra i2c read operation(s).

Thanks,
Guenter

Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling

[Nicolin Chen]

On Wed, Oct 17, 2018 at 02:17:14PM -0700, Guenter Roeck wrote:
> On Wed, Oct 17, 2018 at 01:53:48PM -0700, Nicolin Chen wrote:
> > Hello Guenter,
> > 
> > On Wed, Oct 17, 2018 at 09:55:43AM -0700, Guenter Roeck wrote:
> > > > @@ -676,6 +701,13 @@ static int __maybe_unused ina3221_resume(struct device *dev)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > +	/* Make sure data conversion is finished */
> > > > +	ret = ina3221_wait_for_data_if_active(ina);
> > > 
> > > This is quite expensive and would delay resume (and enable, for that matter).
> > > A less expensive solution would be to save the enable time here and in
> > > ina3221_write_enable(). ina3221_wait_for_data_if_active() can then be called
> > > from read functions and would wait if not enough time has expired.
> > > 
> > > 	if (time_before(...))
> > > 		usleep_range(missing wait time, missing_wait_time * 2);
> > > 
> > > or something like that. This could be done per channel or, to keep
> > > things simple, just using a single time for all channels.
> > 
> > I was thinking something that'd fit one-shot mode too so decided
> > to add a polling. But you are right. It does make sense to skip
> > polling until an actual read happens, though it also feels a bit
> > reasonable to me that putting a polling to the enabling routine.
> > 
> > The wait time would be sightly more complicated than the polling
> > actually, as it needs to involve total conversion time which may
> > vary depending on the number of enabled channels. I will see what
> > would be safer and easier and apply that in v2.
> > 
> It isn't that complex; we have done it in other drivers. It is less
> costly and has less overhead than extra i2c read operation(s).

OK. Will follow them in v2.

Thanks
Nicolin

[PATCH 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.

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

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 160ddc404d73..5ad3ab22b07a 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"
 
@@ -47,6 +48,7 @@
 #define INA3221_CONFIG_CHx_EN(x)	BIT(14 - (x))
 #define INA3221_CONFIG_CHs_EN_MASK	GENMASK(14, 12)
 
+#define INA3221_CONFIG_DEFAULT		0x7127
 #define INA3221_RSHUNT_DEFAULT		10000
 
 enum ina3221_fields {
@@ -97,6 +99,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
@@ -104,6 +107,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];
@@ -113,7 +117,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));
 }
 
 static inline int ina3221_wait_for_data_if_active(struct ina3221_data *ina)
@@ -262,19 +267,33 @@ 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");
+			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;
 
 	/* Make sure data conversion is finished */
 	ret = ina3221_wait_for_data_if_active(ina);
@@ -283,7 +302,20 @@ static int ina3221_write_enable(struct device *dev, int channel, bool enable)
 		return ret;
 	}
 
+	/* 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,
@@ -578,7 +610,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);
@@ -610,44 +641,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);
 
@@ -712,7 +770,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