Re: [lm-sensors] [RFC PATCH 1/9] hwmon: lis3: pm_runtime support
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
>
>
>> Jonathan
>>>
>>> Signed-off-by: Samu Onkalo
>>> ---
>>> 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
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #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);
>>> + }
>>> +}
>>> +
>>> s
Re: [lm-sensors] [RFC PATCH 1/9] hwmon: lis3: pm_runtime support
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
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #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);
> > retur
Re: [lm-sensors] [RFC PATCH 1/9] hwmon: lis3: pm_runtime support
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!
Jonathan
>
> Signed-off-by: Samu Onkalo
> ---
> 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
> #include
> #include
> +#include
> #include
> #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(stru
