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 <[email protected]> > > >> Jonathan >>> >>> Signed-off-by: Samu Onkalo <[email protected]> >>> --- >>> 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, >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
