[v4,1/5] hwmon: (core) Add pm ops to hwmon class
diff mbox series

Message ID 20181025235122.3240-2-nicoleotsuka@gmail.com
State Superseded
Headers show
Series
  • hwmon: (ina3221) Implement PM runtime to save power
Related show

Commit Message

Nicolin Chen Oct. 25, 2018, 11:51 p.m. UTC
The hwmon core generates an extra child dev pointer for every
registered hwmon driver so as to link the new device to hwmon
class, and it exposes this new dev in /sys/class/hwmon/hwmon*/
directory including a power directory for pm runtime. However,
there is currently no way for hwmon drivers to link their own
pm related information to this power directory, so it's always
showing unsupported even if the driver implements the pm ops.

This is because pm_runtime core scans PM runtime callbacks in
the dev->driver pointer while this new child dev doesn't have
a driver pointer. It sounds easier to merely copy this driver
pointer from the parent dev to the child dev, however, this'd
create some problem like the same suspend() function might be
called twice during system suspend cycle and the second call
may fail since the device is already suspended, so the driver
would have to work around to bypass one of the two callbacks.

Actually, pm_runtime core also scans a class-level pm pointer:
         else if (dev->class && dev->class->pm)
                  ops = dev->class->pm;

This means that hwmon class in the hwmon core could actually
have its own generic pm callbacks so that a registered driver
would have the capability to link their own callbacks to the
hwmon core's.

So this patch adds a pm pointer to the hwmon class with some
generic pm callbacks of system suspend/resume and pm_runtime
suspend/resume, and also allows hwmon drivers to pass valid
pm pointers through _with_info API when registering devices.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
Changelog
v3->v4:
 * Dropped the risky pointer copies
 * Added generic pm runtime callbacks to the hwmon class
v2->v3:
 * N/A
v1->v2:
 * Added device pointers

 drivers/hwmon/hwmon.c | 24 ++++++++++++++++++++++++
 include/linux/hwmon.h |  2 ++
 2 files changed, 26 insertions(+)

Comments

Guenter Roeck Nov. 2, 2018, 5:54 p.m. UTC | #1
On Thu, Oct 25, 2018 at 04:51:18PM -0700, Nicolin Chen wrote:
> The hwmon core generates an extra child dev pointer for every
> registered hwmon driver so as to link the new device to hwmon
> class, and it exposes this new dev in /sys/class/hwmon/hwmon*/
> directory including a power directory for pm runtime. However,
> there is currently no way for hwmon drivers to link their own
> pm related information to this power directory, so it's always
> showing unsupported even if the driver implements the pm ops.
> 
> This is because pm_runtime core scans PM runtime callbacks in
> the dev->driver pointer while this new child dev doesn't have
> a driver pointer. It sounds easier to merely copy this driver
> pointer from the parent dev to the child dev, however, this'd
> create some problem like the same suspend() function might be
> called twice during system suspend cycle and the second call
> may fail since the device is already suspended, so the driver
> would have to work around to bypass one of the two callbacks.
> 
> Actually, pm_runtime core also scans a class-level pm pointer:
>          else if (dev->class && dev->class->pm)
>                   ops = dev->class->pm;
> 
> This means that hwmon class in the hwmon core could actually
> have its own generic pm callbacks so that a registered driver
> would have the capability to link their own callbacks to the
> hwmon core's.
> 
> So this patch adds a pm pointer to the hwmon class with some
> generic pm callbacks of system suspend/resume and pm_runtime
> suspend/resume, and also allows hwmon drivers to pass valid
> pm pointers through _with_info API when registering devices.
> 

Just to give an update: I am not happy with having to add hwmon
specific pm callbacks. That doesn't look right to me. I'll have
to spend time figuring out how other virtual devices handle this
situation. Unfortunately, time is always scarce, so this will likely
take a while.

Also, please note that I won't accept function macros. I understand
that are used a lot, especially in older hwmon drivers, but they just
make the code more difficult to read and often add unnecessary code.

Thanks,
Guenter

> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
> Changelog
> v3->v4:
>  * Dropped the risky pointer copies
>  * Added generic pm runtime callbacks to the hwmon class
> v2->v3:
>  * N/A
> v1->v2:
>  * Added device pointers
> 
>  drivers/hwmon/hwmon.c | 24 ++++++++++++++++++++++++
>  include/linux/hwmon.h |  2 ++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 975c95169884..10bbd36be4a5 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -20,6 +20,7 @@
>  #include <linux/idr.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/thermal.h>
> @@ -103,11 +104,34 @@ static void hwmon_dev_release(struct device *dev)
>  	kfree(to_hwmon_device(dev));
>  }
>  
> +#define HWMON_PM_FUNCTION(name)					\
> +static int __maybe_unused hwmon_##name(struct device *dev)	\
> +{								\
> +	struct hwmon_device *hwdev = to_hwmon_device(dev);	\
> +	const struct hwmon_chip_info *chip = hwdev->chip;	\
> +								\
> +	if (chip && chip->pm && chip->pm->name)			\
> +		return chip->pm->name(dev);			\
> +								\
> +	return 0;						\
> +}
> +
> +HWMON_PM_FUNCTION(suspend)
> +HWMON_PM_FUNCTION(resume)
> +HWMON_PM_FUNCTION(runtime_suspend)
> +HWMON_PM_FUNCTION(runtime_resume)
> +
> +static const struct dev_pm_ops hwmon_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(hwmon_suspend, hwmon_resume)
> +	SET_RUNTIME_PM_OPS(hwmon_runtime_suspend, hwmon_runtime_resume, NULL)
> +};
> +
>  static struct class hwmon_class = {
>  	.name = "hwmon",
>  	.owner = THIS_MODULE,
>  	.dev_groups = hwmon_dev_attr_groups,
>  	.dev_release = hwmon_dev_release,
> +	.pm = &hwmon_pm,
>  };
>  
>  static DEFINE_IDA(hwmon_ida);
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 99e0c1b0b5fb..edbeddb489f8 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -369,10 +369,12 @@ struct hwmon_channel_info {
>   * Chip configuration
>   * @ops:	Pointer to hwmon operations.
>   * @info:	Null-terminated list of channel information.
> + * @pm:		Pointer to dev_pm_ops callbacks.
>   */
>  struct hwmon_chip_info {
>  	const struct hwmon_ops *ops;
>  	const struct hwmon_channel_info **info;
> +	const struct dev_pm_ops *pm;
>  };
>  
>  /* hwmon_device_register() is deprecated */
> -- 
> 2.17.1
>
Nicolin Chen Nov. 2, 2018, 7:48 p.m. UTC | #2
On Fri, Nov 02, 2018 at 10:54:35AM -0700, Guenter Roeck wrote:

> > Actually, pm_runtime core also scans a class-level pm pointer:
> >          else if (dev->class && dev->class->pm)
> >                   ops = dev->class->pm;
> > 
> > This means that hwmon class in the hwmon core could actually
> > have its own generic pm callbacks so that a registered driver
> > would have the capability to link their own callbacks to the
> > hwmon core's.
> > 
> > So this patch adds a pm pointer to the hwmon class with some
> > generic pm callbacks of system suspend/resume and pm_runtime
> > suspend/resume, and also allows hwmon drivers to pass valid
> > pm pointers through _with_info API when registering devices.
> > 
> 
> Just to give an update: I am not happy with having to add hwmon
> specific pm callbacks. That doesn't look right to me. I'll have
> to spend time figuring out how other virtual devices handle this
> situation. Unfortunately, time is always scarce, so this will likely
> take a while.

That's okay. Would it be possible then for me to add PM runtime
functions just in ina3221, without touching hwmon core? It will
belong to the I2C device, not hwmon device any more.

> Also, please note that I won't accept function macros. I understand
> that are used a lot, especially in older hwmon drivers, but they just
> make the code more difficult to read and often add unnecessary code.

Okay. Will be careful to use them in future hwmon patches.

Thanks
Nicolin
Guenter Roeck Nov. 3, 2018, 2:59 a.m. UTC | #3
On 11/2/18 12:48 PM, Nicolin Chen wrote:
> On Fri, Nov 02, 2018 at 10:54:35AM -0700, Guenter Roeck wrote:
> 
>>> Actually, pm_runtime core also scans a class-level pm pointer:
>>>           else if (dev->class && dev->class->pm)
>>>                    ops = dev->class->pm;
>>>
>>> This means that hwmon class in the hwmon core could actually
>>> have its own generic pm callbacks so that a registered driver
>>> would have the capability to link their own callbacks to the
>>> hwmon core's.
>>>
>>> So this patch adds a pm pointer to the hwmon class with some
>>> generic pm callbacks of system suspend/resume and pm_runtime
>>> suspend/resume, and also allows hwmon drivers to pass valid
>>> pm pointers through _with_info API when registering devices.
>>>
>>
>> Just to give an update: I am not happy with having to add hwmon
>> specific pm callbacks. That doesn't look right to me. I'll have
>> to spend time figuring out how other virtual devices handle this
>> situation. Unfortunately, time is always scarce, so this will likely
>> take a while.
> 
> That's okay. Would it be possible then for me to add PM runtime
> functions just in ina3221, without touching hwmon core? It will
> belong to the I2C device, not hwmon device any more.
> 

If that works, sure. We can always add generic support later.

Guenter

>> Also, please note that I won't accept function macros. I understand
>> that are used a lot, especially in older hwmon drivers, but they just
>> make the code more difficult to read and often add unnecessary code.
> 
> Okay. Will be careful to use them in future hwmon patches.
> 
> Thanks
> Nicolin
>

Patch
diff mbox series

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 975c95169884..10bbd36be4a5 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -20,6 +20,7 @@ 
 #include <linux/idr.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/thermal.h>
@@ -103,11 +104,34 @@  static void hwmon_dev_release(struct device *dev)
 	kfree(to_hwmon_device(dev));
 }
 
+#define HWMON_PM_FUNCTION(name)					\
+static int __maybe_unused hwmon_##name(struct device *dev)	\
+{								\
+	struct hwmon_device *hwdev = to_hwmon_device(dev);	\
+	const struct hwmon_chip_info *chip = hwdev->chip;	\
+								\
+	if (chip && chip->pm && chip->pm->name)			\
+		return chip->pm->name(dev);			\
+								\
+	return 0;						\
+}
+
+HWMON_PM_FUNCTION(suspend)
+HWMON_PM_FUNCTION(resume)
+HWMON_PM_FUNCTION(runtime_suspend)
+HWMON_PM_FUNCTION(runtime_resume)
+
+static const struct dev_pm_ops hwmon_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(hwmon_suspend, hwmon_resume)
+	SET_RUNTIME_PM_OPS(hwmon_runtime_suspend, hwmon_runtime_resume, NULL)
+};
+
 static struct class hwmon_class = {
 	.name = "hwmon",
 	.owner = THIS_MODULE,
 	.dev_groups = hwmon_dev_attr_groups,
 	.dev_release = hwmon_dev_release,
+	.pm = &hwmon_pm,
 };
 
 static DEFINE_IDA(hwmon_ida);
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 99e0c1b0b5fb..edbeddb489f8 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -369,10 +369,12 @@  struct hwmon_channel_info {
  * Chip configuration
  * @ops:	Pointer to hwmon operations.
  * @info:	Null-terminated list of channel information.
+ * @pm:		Pointer to dev_pm_ops callbacks.
  */
 struct hwmon_chip_info {
 	const struct hwmon_ops *ops;
 	const struct hwmon_channel_info **info;
+	const struct dev_pm_ops *pm;
 };
 
 /* hwmon_device_register() is deprecated */