Re: [lm-sensors] [RFC PATCH 1/9] hwmon: lis3: pm_runtime support

[Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>]

On 10/03/10 06:03, Onkalo Samu wrote:
> 
> Thanks for comments.
> 
> On Sat, 2010-10-02 at 19:14 +0200, ext Jonathan Cameron wrote:
>> On 10/01/10 12:46, Samu Onkalo wrote:
>>> Add pm_runtime support to lis3 core driver.
>>> Add pm_runtime support to lis3 i2c driver.
>>>
>>> spi and hp_accel drivers are not yet supported. Old always
>>> on functionality remains for those.
>>>
>>> For sysfs there is 5 second delay before turning off the
>>> chip to avoid long ramp up delay.
>> I'm not overly familiar with the runtime stuff so looking at this based
>> on a quick read of their documentation.
>>
>> Note I'm also not that familiar with the driver :)
>>
>> One real query about the logic in lis3lv02d_i2c_resume. My reading is
>> that if we are runtime suspended when coming out of a system suspend
>> then we don't want to power up the chip?  However I'm not entirely
>> sure how the two types of power management interact so sorry if I
>> have completely misunderstood this!
>>
> 
> It took some time understand how this system works over the system level
> suspend. And depending on the use, lis3 chip can be kept active or
> powered down over the system standby :)
The answer below covers my query (and is a handy thing to know about in
general so thanks for that!)  I'll certainly be copying this code for
some of my drivers when I have the time to play with runtime pm.

Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> 
> 
>> Jonathan
>>>
>>> Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  drivers/hwmon/lis3lv02d.c     |   59 +++++++++++++++++++++++++++++++++++++++++
>>>  drivers/hwmon/lis3lv02d.h     |    1 +
>>>  drivers/hwmon/lis3lv02d_i2c.c |   48 +++++++++++++++++++++++++--------
>>>  3 files changed, 96 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
>>> index fc591ae..eaa5bf0 100644
>>> --- a/drivers/hwmon/lis3lv02d.c
>>> +++ b/drivers/hwmon/lis3lv02d.c
>>> @@ -34,6 +34,7 @@
>>>  #include <linux/freezer.h>
>>>  #include <linux/uaccess.h>
>>>  #include <linux/miscdevice.h>
>>> +#include <linux/pm_runtime.h>
>>>  #include <asm/atomic.h>
>>>  #include "lis3lv02d.h"
>>>  
>>> @@ -43,6 +44,9 @@
>>>  #define MDPS_POLL_INTERVAL 50
>>>  #define MDPS_POLL_MIN	   0
>>>  #define MDPS_POLL_MAX	   2000
>>> +
>>> +#define SYSFS_POWERDOWN_DELAY 5000 /* In milliseconds */
>> Name is rather generic...
>>> +
>>>  /*
>>>   * The sensor can also generate interrupts (DRDY) but it's pretty pointless
>>>   * because they are generated even if the data do not change. So it's better
>>> @@ -262,6 +266,18 @@ static void lis3lv02d_joystick_poll(struct input_polled_dev *pidev)
>>>  	mutex_unlock(&lis3_dev.mutex);
>>>  }
>>>  
>>> +static void lis3lv02d_joystick_open(struct input_polled_dev *pidev)
>>> +{
>>> +	if (lis3_dev.pm_dev)
>>> +		pm_runtime_get_sync(lis3_dev.pm_dev);
>>> +}
>>> +
>>> +static void lis3lv02d_joystick_close(struct input_polled_dev *pidev)
>>> +{
>>> +	if (lis3_dev.pm_dev)
>>> +		pm_runtime_put(lis3_dev.pm_dev);
>>> +}
>>> +
>>>  static irqreturn_t lis302dl_interrupt(int irq, void *dummy)
>>>  {
>>>  	if (!test_bit(0, &lis3_dev.misc_opened))
>>> @@ -356,6 +372,9 @@ static int lis3lv02d_misc_open(struct inode *inode, struct file *file)
>>>  	if (test_and_set_bit(0, &lis3_dev.misc_opened))
>>>  		return -EBUSY; /* already open */
>>>  
>>> +	if (lis3_dev.pm_dev)
>>> +		pm_runtime_get_sync(lis3_dev.pm_dev);
>>> +
>>>  	atomic_set(&lis3_dev.count, 0);
>>>  	return 0;
>>>  }
>>> @@ -364,6 +383,8 @@ static int lis3lv02d_misc_release(struct inode *inode, struct file *file)
>>>  {
>>>  	fasync_helper(-1, file, 0, &lis3_dev.async_queue);
>>>  	clear_bit(0, &lis3_dev.misc_opened); /* release the device */
>>> +	if (lis3_dev.pm_dev)
>>> +		pm_runtime_put(lis3_dev.pm_dev);
>>>  	return 0;
>>>  }
>>>  
>>> @@ -460,6 +481,8 @@ int lis3lv02d_joystick_enable(void)
>>>  		return -ENOMEM;
>>>  
>>>  	lis3_dev.idev->poll = lis3lv02d_joystick_poll;
>>> +	lis3_dev.idev->open = lis3lv02d_joystick_open;
>>> +	lis3_dev.idev->close = lis3lv02d_joystick_close;
>>>  	lis3_dev.idev->poll_interval = MDPS_POLL_INTERVAL;
>>>  	lis3_dev.idev->poll_interval_min = MDPS_POLL_MIN;
>>>  	lis3_dev.idev->poll_interval_max = MDPS_POLL_MAX;
>>> @@ -512,12 +535,30 @@ void lis3lv02d_joystick_disable(void)
>>>  EXPORT_SYMBOL_GPL(lis3lv02d_joystick_disable);
>>>  
>>>  /* Sysfs stuff */
>>> +static void lis3lv02d_sysfs_poweron(struct lis3lv02d *lis3)
>>> +{
>>> +	/*
>>> +	 * SYSFS functions are fast visitors so put-call
>>> +	 * immediately after the get-call. However, keep
>>> +	 * chip running for a while and schedule delayed
>>> +	 * suspend. This way periodic sysfs calls doesn't
>>> +	 * suffer from relatively long power up time.
>>> +	 */
>>> +
>>> +	if (lis3_dev.pm_dev) {
>>> +		pm_runtime_get_sync(lis3->pm_dev);
>>> +		pm_runtime_put_noidle(lis3->pm_dev);
>>> +		pm_schedule_suspend(lis3->pm_dev, SYSFS_POWERDOWN_DELAY);
>>> +	}
>>> +}
>>> +
>>>  static ssize_t lis3lv02d_selftest_show(struct device *dev,
>>>  				struct device_attribute *attr, char *buf)
>>>  {
>>>  	int result;
>>>  	s16 values[3];
>>>  
>>> +	lis3lv02d_sysfs_poweron(&lis3_dev);
>>>  	result = lis3lv02d_selftest(&lis3_dev, values);
>>>  	return sprintf(buf, "%s %d %d %d\n", result == 0 ? "OK" : "FAIL",
>>>  		values[0], values[1], values[2]);
>>> @@ -528,6 +569,7 @@ static ssize_t lis3lv02d_position_show(struct device *dev,
>>>  {
>>>  	int x, y, z;
>>>  
>>> +	lis3lv02d_sysfs_poweron(&lis3_dev);
>>>  	mutex_lock(&lis3_dev.mutex);
>>>  	lis3lv02d_get_xyz(&lis3_dev, &x, &y, &z);
>>>  	mutex_unlock(&lis3_dev.mutex);
>>> @@ -549,6 +591,7 @@ static ssize_t lis3lv02d_rate_set(struct device *dev,
>>>  	if (strict_strtoul(buf, 0, &rate))
>>>  		return -EINVAL;
>>>  
>>> +	lis3lv02d_sysfs_poweron(&lis3_dev);
>>>  	if (lis3lv02d_set_odr(rate))
>>>  		return -EINVAL;
>>>  
>>> @@ -585,6 +628,17 @@ int lis3lv02d_remove_fs(struct lis3lv02d *lis3)
>>>  {
>>>  	sysfs_remove_group(&lis3->pdev->dev.kobj, &lis3lv02d_attribute_group);
>>>  	platform_device_unregister(lis3->pdev);
>>> +	if (lis3_dev.pm_dev) {
>>> +		/* Barrier after the sysfs remove */
>>> +		pm_runtime_barrier(lis3->pm_dev);
>>> +
>>> +		/* SYSFS may have left chip running. Turn off if necessary */
>>> +		if (!pm_runtime_suspended(lis3->pm_dev))
>>> +			lis3lv02d_poweroff(&lis3_dev);
>>> +
>>> +		pm_runtime_disable(lis3->pm_dev);
>>> +		pm_runtime_set_suspended(lis3->pm_dev);
>>> +	}
>>>  	return 0;
>>>  }
>>>  EXPORT_SYMBOL_GPL(lis3lv02d_remove_fs);
>>> @@ -685,6 +739,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>>>  	lis3lv02d_add_fs(dev);
>>>  	lis3lv02d_poweron(dev);
>>>  
>>> +	if (lis3_dev.pm_dev) {
>>> +		pm_runtime_set_active(dev->pm_dev);
>>> +		pm_runtime_enable(dev->pm_dev);
>>> +	}
>>> +
>>>  	if (lis3lv02d_joystick_enable())
>>>  		printk(KERN_ERR DRIVER_NAME ": joystick initialization failed\n");
>>>  
>>> diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
>>> index 8540913..3e8a208 100644
>>> --- a/drivers/hwmon/lis3lv02d.h
>>> +++ b/drivers/hwmon/lis3lv02d.h
>>> @@ -214,6 +214,7 @@ struct axis_conversion {
>>>  
>>>  struct lis3lv02d {
>>>  	void			*bus_priv; /* used by the bus layer only */
>>> +	struct device		*pm_dev; /* for pm_runtime purposes */
>>>  	int (*init) (struct lis3lv02d *lis3);
>>>  	int (*write) (struct lis3lv02d *lis3, int reg, u8 val);
>>>  	int (*read) (struct lis3lv02d *lis3, int reg, u8 *ret);
>>> diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
>>> index 8e5933b..b9ed1fb 100644
>>> --- a/drivers/hwmon/lis3lv02d_i2c.c
>>> +++ b/drivers/hwmon/lis3lv02d_i2c.c
>>> @@ -29,6 +29,7 @@
>>>  #include <linux/init.h>
>>>  #include <linux/err.h>
>>>  #include <linux/i2c.h>
>>> +#include <linux/pm_runtime.h>
>>>  #include "lis3lv02d.h"
>>>  
>>>  #define DRV_NAME 	"lis3lv02d_i2c"
>>> @@ -95,6 +96,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
>>>  	lis3_dev.write	  = lis3_i2c_write;
>>>  	lis3_dev.irq	  = client->irq;
>>>  	lis3_dev.ac	  = lis3lv02d_axis_map;
>>> +	lis3_dev.pm_dev	  = &client->dev;
>>>  
>>>  	i2c_set_clientdata(client, &lis3_dev);
>>>  	ret = lis3lv02d_init_device(&lis3_dev);
>>> @@ -111,14 +113,14 @@ static int __devexit lis3lv02d_i2c_remove(struct i2c_client *client)
>>>  		pdata->release_resources();
>>>  
>>>  	lis3lv02d_joystick_disable();
>>> -	lis3lv02d_poweroff(lis3);
>>>  
>>>  	return lis3lv02d_remove_fs(&lis3_dev);
>>>  }
>>>  
>>>  #ifdef CONFIG_PM
>>> -static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
>>> +static int lis3lv02d_i2c_suspend(struct device *dev)
>>>  {
>>> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>>>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>>  
>>>  	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
>>> @@ -126,18 +128,16 @@ static int lis3lv02d_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
>>>  	return 0;
>>>  }
>>>  
>>> -static int lis3lv02d_i2c_resume(struct i2c_client *client)
>>> +static int lis3lv02d_i2c_resume(struct device *dev)
>>>  {
>>> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>>>  	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>>  
>>> -	if (!lis3->pdata || !lis3->pdata->wakeup_flags)
>>> +	if (!lis3->pdata || !lis3->pdata->wakeup_flags ||
>>> +		pm_runtime_suspended(dev))
>> I may be misunderstanding the docs on this, but I would have thought
>> we only want to power it up if it isn't in suspended state?
> 
> That was what I also though, but ...
> 
> runtime documentation says:
> 
> "During system resume, devices generally should be brought back to full power,
> even if they were suspended before the system sleep began.  There are several
> reasons for this, including:"
> 
> And there was quite good reasons in the doc to do that :)
> 
> I2C core code turns suspended child devices on. However, there are no users
> and pm_runtime notices that. Pm_runtime calls runtime_suspend-cb soon after the 
> system level resume. If the chip is not powered up when that happens, it is possible
> that regulators are off and all the accesses towards the chip fails.
I guess that makes sense in a kind of roundabout way!
> 
> 
> 
> 
> 
>>>  		lis3lv02d_poweron(lis3);
>>> -	return 0;
>>> -}
>>>  
>>> -static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
>>> -{
>>> -	lis3lv02d_i2c_suspend(client, PMSG_SUSPEND);
>>> +	return 0;
>>>  }
>>>  #else
>>>  #define lis3lv02d_i2c_suspend	NULL
>>> @@ -145,6 +145,24 @@ static void lis3lv02d_i2c_shutdown(struct i2c_client *client)
>>>  #define lis3lv02d_i2c_shutdown	NULL
>>>  #endif
>>>  
>>> +static int lis3_i2c_runtime_suspend(struct device *dev)
>>> +{
>>> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>>> +	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>> +
>>> +	lis3lv02d_poweroff(lis3);
>>> +	return 0;
>>> +}
>>> +
>>> +static int lis3_i2c_runtime_resume(struct device *dev)
>>> +{
>>> +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
>>> +	struct lis3lv02d *lis3 = i2c_get_clientdata(client);
>>> +
>>> +	lis3lv02d_poweron(lis3);
>>> +	return 0;
>>> +}
>>> +
>>>  static const struct i2c_device_id lis3lv02d_id[] = {
>>>  	{"lis3lv02d", 0 },
>>>  	{}
>>> @@ -152,14 +170,20 @@ static const struct i2c_device_id lis3lv02d_id[] = {
>>>  
>>>  MODULE_DEVICE_TABLE(i2c, lis3lv02d_id);
>>>  
>>> +static const struct dev_pm_ops lis3_pm_ops = {
>>> +	SET_SYSTEM_SLEEP_PM_OPS(lis3lv02d_i2c_suspend,
>>> +				lis3lv02d_i2c_resume)
>>> +	SET_RUNTIME_PM_OPS(lis3_i2c_runtime_suspend,
>>> +			   lis3_i2c_runtime_resume,
>>> +			   NULL)
>>> +};
>>> +
>>>  static struct i2c_driver lis3lv02d_i2c_driver = {
>>>  	.driver	 = {
>>>  		.name   = DRV_NAME,
>>>  		.owner  = THIS_MODULE,
>>> +		.pm     = &lis3_pm_ops,
>>>  	},
>>> -	.suspend = lis3lv02d_i2c_suspend,
>>> -	.shutdown = lis3lv02d_i2c_shutdown,
>>> -	.resume = lis3lv02d_i2c_resume,
>>>  	.probe	= lis3lv02d_i2c_probe,
>>>  	.remove	= __devexit_p(lis3lv02d_i2c_remove),
>>>  	.id_table = lis3lv02d_id,
>>
> 
> 
>