* [PATCH 1/5] hwmon: (core) Inherit power properties to hdev
2018-10-17 1:24 [PATCH 0/5] hwmon: (ina3221) Implement PM runtime to save power Nicolin Chen
@ 2018-10-17 1:24 ` Nicolin Chen
2018-10-17 19:44 ` Guenter Roeck
2018-10-18 11:37 ` Dan Carpenter
2018-10-17 1:24 ` [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes Nicolin Chen
` (3 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Nicolin Chen @ 2018-10-17 1:24 UTC (permalink / raw)
To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/5] hwmon: (core) Inherit power properties to hdev
2018-10-17 1:24 ` [PATCH 1/5] hwmon: (core) Inherit power properties to hdev Nicolin Chen
@ 2018-10-17 19:44 ` Guenter Roeck
2018-10-17 20:15 ` Nicolin Chen
2018-10-18 11:37 ` Dan Carpenter
1 sibling, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2018-10-17 19:44 UTC (permalink / raw)
To: Nicolin Chen; +Cc: jdelvare, linux-hwmon, linux-kernel
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/5] hwmon: (core) Inherit power properties to hdev
2018-10-17 19:44 ` Guenter Roeck
@ 2018-10-17 20:15 ` Nicolin Chen
0 siblings, 0 replies; 17+ messages in thread
From: Nicolin Chen @ 2018-10-17 20:15 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel
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
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/5] hwmon: (core) Inherit power properties to hdev
2018-10-17 1:24 ` [PATCH 1/5] hwmon: (core) Inherit power properties to hdev Nicolin Chen
2018-10-17 19:44 ` Guenter Roeck
@ 2018-10-18 11:37 ` Dan Carpenter
1 sibling, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2018-10-18 11:37 UTC (permalink / raw)
To: kbuild, Nicolin Chen
Cc: kbuild-all, jdelvare, linux, linux-hwmon, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes
2018-10-17 1:24 [PATCH 0/5] hwmon: (ina3221) Implement PM runtime to save power Nicolin Chen
2018-10-17 1:24 ` [PATCH 1/5] hwmon: (core) Inherit power properties to hdev Nicolin Chen
@ 2018-10-17 1:24 ` Nicolin Chen
2018-10-17 19:46 ` Guenter Roeck
2018-10-17 1:24 ` [PATCH 3/5] hwmon: (ina3221) Serialize sysfs ABI accesses Nicolin Chen
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Nicolin Chen @ 2018-10-17 1:24 UTC (permalink / raw)
To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel
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], ®val);
if (ret)
return ret;
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes
2018-10-17 1:24 ` [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes Nicolin Chen
@ 2018-10-17 19:46 ` Guenter Roeck
2018-10-17 20:39 ` Nicolin Chen
0 siblings, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2018-10-17 19:46 UTC (permalink / raw)
To: Nicolin Chen; +Cc: jdelvare, linux-hwmon, linux-kernel
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], ®val);
> if (ret)
> return ret;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes
2018-10-17 19:46 ` Guenter Roeck
@ 2018-10-17 20:39 ` Nicolin Chen
2018-10-17 21:14 ` Guenter Roeck
0 siblings, 1 reply; 17+ messages in thread
From: Nicolin Chen @ 2018-10-17 20:39 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes
2018-10-17 20:39 ` Nicolin Chen
@ 2018-10-17 21:14 ` Guenter Roeck
2018-10-18 1:03 ` Nicolin Chen
0 siblings, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2018-10-17 21:14 UTC (permalink / raw)
To: Nicolin Chen; +Cc: jdelvare, linux-hwmon, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes
2018-10-17 21:14 ` Guenter Roeck
@ 2018-10-18 1:03 ` Nicolin Chen
0 siblings, 0 replies; 17+ messages in thread
From: Nicolin Chen @ 2018-10-18 1:03 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/5] hwmon: (ina3221) Serialize sysfs ABI accesses
2018-10-17 1:24 [PATCH 0/5] hwmon: (ina3221) Implement PM runtime to save power Nicolin Chen
2018-10-17 1:24 ` [PATCH 1/5] hwmon: (core) Inherit power properties to hdev Nicolin Chen
2018-10-17 1:24 ` [PATCH 2/5] hwmon: (ina3221) Return -ENODATA for two alarms attributes Nicolin Chen
@ 2018-10-17 1:24 ` Nicolin Chen
2018-10-17 1:24 ` [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling Nicolin Chen
2018-10-17 1:24 ` [PATCH 5/5] hwmon: (ina3221) Add PM runtime support Nicolin Chen
4 siblings, 0 replies; 17+ messages in thread
From: Nicolin Chen @ 2018-10-17 1:24 UTC (permalink / raw)
To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling
2018-10-17 1:24 [PATCH 0/5] hwmon: (ina3221) Implement PM runtime to save power Nicolin Chen
` (2 preceding siblings ...)
2018-10-17 1:24 ` [PATCH 3/5] hwmon: (ina3221) Serialize sysfs ABI accesses Nicolin Chen
@ 2018-10-17 1:24 ` Nicolin Chen
2018-10-17 16:55 ` Guenter Roeck
2018-10-17 1:24 ` [PATCH 5/5] hwmon: (ina3221) Add PM runtime support Nicolin Chen
4 siblings, 1 reply; 17+ messages in thread
From: Nicolin Chen @ 2018-10-17 1:24 UTC (permalink / raw)
To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling
2018-10-17 1:24 ` [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling Nicolin Chen
@ 2018-10-17 16:55 ` Guenter Roeck
2018-10-17 20:53 ` Nicolin Chen
0 siblings, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2018-10-17 16:55 UTC (permalink / raw)
To: Nicolin Chen; +Cc: jdelvare, linux-hwmon, linux-kernel
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling
2018-10-17 16:55 ` Guenter Roeck
@ 2018-10-17 20:53 ` Nicolin Chen
2018-10-17 21:17 ` Guenter Roeck
0 siblings, 1 reply; 17+ messages in thread
From: Nicolin Chen @ 2018-10-17 20:53 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling
2018-10-17 20:53 ` Nicolin Chen
@ 2018-10-17 21:17 ` Guenter Roeck
2018-10-18 1:04 ` Nicolin Chen
0 siblings, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2018-10-17 21:17 UTC (permalink / raw)
To: Nicolin Chen; +Cc: jdelvare, linux-hwmon, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling
2018-10-17 21:17 ` Guenter Roeck
@ 2018-10-18 1:04 ` Nicolin Chen
0 siblings, 0 replies; 17+ messages in thread
From: Nicolin Chen @ 2018-10-18 1:04 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/5] hwmon: (ina3221) Add PM runtime support
2018-10-17 1:24 [PATCH 0/5] hwmon: (ina3221) Implement PM runtime to save power Nicolin Chen
` (3 preceding siblings ...)
2018-10-17 1:24 ` [PATCH 4/5] hwmon: (ina3221) Make sure data is ready after channel enabling Nicolin Chen
@ 2018-10-17 1:24 ` Nicolin Chen
4 siblings, 0 replies; 17+ messages in thread
From: Nicolin Chen @ 2018-10-17 1:24 UTC (permalink / raw)
To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread