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 :)
> Jonathan
> >
> > Signed-off-by: Samu Onkalo <samu.p.onkalo@xxxxxxxxx>
> > ---
> > 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.
> > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
