[PATCH] apds9802als: add runtime PM support

[PATCH] apds9802als: add runtime PM support

[Alan Cox]

From: Hong Liu <hong.liu@intel.com>


    Update the driver for the needed runtime power features. Remove the old
    user controlled power functions.

    Signed-off-by: Hong Liu <hong.liu@intel.com>
    Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/misc/apds9802als.c |  158 ++++++++++++++++++++++++++------------------
 1 files changed, 94 insertions(+), 64 deletions(-)


diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
index fbe4960..15f9436 100644
--- a/drivers/misc/apds9802als.c
+++ b/drivers/misc/apds9802als.c
@@ -29,19 +29,16 @@
 #include <linux/delay.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
-#include <linux/hwmon-sysfs.h>
+#include <linux/pm_runtime.h>
 
 #define ALS_MIN_RANGE_VAL 1
 #define ALS_MAX_RANGE_VAL 2
 #define POWER_STA_ENABLE 1
 #define POWER_STA_DISABLE 0
-#define APDS9802ALS_I2C_ADDR 0x29
 
 #define DRIVER_NAME "apds9802als"
 
 struct als_data {
-	struct device *hwmon_dev;
-	bool needresume;
 	struct mutex mutex;
 };
 
@@ -60,29 +57,64 @@ static ssize_t als_sensing_range_show(struct device *dev,
 		return sprintf(buf, "65535\n");
 }
 
+static int als_wait_for_data_ready(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+	int retry = 10;
+
+	do {
+		msleep(30);
+		ret = i2c_smbus_read_byte_data(client, 0x86);
+	} while (!(ret & 0x80) && retry--);
+
+	if (!retry) {
+		dev_warn(dev, "timeout waiting for data ready\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
 static ssize_t als_lux0_input_data_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct als_data *data = i2c_get_clientdata(client);
-	unsigned int ret_val;
+	int ret_val;
 	int temp;
 
 	/* Protect against parallel reads */
+	pm_runtime_get_sync(dev);
 	mutex_lock(&data->mutex);
-	temp = i2c_smbus_read_byte_data(client, 0x8C);/*LSB data*/
+
+	/* clear EOC interrupt status */
+	i2c_smbus_write_byte(client, 0x40);
+	/* start measurement */
+	temp = i2c_smbus_read_byte_data(client, 0x81);
+	i2c_smbus_write_byte_data(client, 0x81, temp | 0x08);
+
+	ret_val = als_wait_for_data_ready(dev);
+	if (ret_val < 0)
+		goto failed;
+
+	temp = i2c_smbus_read_byte_data(client, 0x8C); /* LSB data */
 	if (temp < 0) {
 		ret_val = temp;
 		goto failed;
 	}
-	ret_val = i2c_smbus_read_byte_data(client, 0x8D);/*MSB data*/
+	ret_val = i2c_smbus_read_byte_data(client, 0x8D); /* MSB data */
 	if (ret_val < 0)
 		goto failed;
+
 	mutex_unlock(&data->mutex);
-	ret_val = (ret_val << 8) | temp;
-	return sprintf(buf, "%d\n", ret_val);
+	pm_runtime_put_sync(dev);
+
+	temp = (ret_val << 8) | temp;
+	return sprintf(buf, "%d\n", temp);
 failed:
 	mutex_unlock(&data->mutex);
+	pm_runtime_put_sync(dev);
 	return ret_val;
 }
 
@@ -104,9 +136,10 @@ static ssize_t als_sensing_range_store(struct device *dev,
 	else
 		return -ERANGE;
 
+	pm_runtime_get_sync(dev);
+
 	/* Make sure nobody else reads/modifies/writes 0x81 while we
 	   are active */
-
 	mutex_lock(&data->mutex);
 
 	ret_val = i2c_smbus_read_byte_data(client, 0x81);
@@ -116,34 +149,25 @@ static ssize_t als_sensing_range_store(struct device *dev,
 	/* Reset the bits before setting them */
 	ret_val = ret_val & 0xFA;
 
-	if (val == 1) /* Setting the continous measurement up to 4k LUX */
-		ret_val = (ret_val | 0x05);
-	else /* Setting the continous measurement up to 64k LUX*/
-		ret_val = (ret_val | 0x04);
+	if (val == 1) /* Setting detection range up to 4k LUX */
+		ret_val = (ret_val | 0x01);
+	else /* Setting detection range up to 64k LUX*/
+		ret_val = (ret_val | 0x00);
 
 	ret_val = i2c_smbus_write_byte_data(client, 0x81, ret_val);
+
 	if (ret_val >= 0) {
 		/* All OK */
 		mutex_unlock(&data->mutex);
+		pm_runtime_put_sync(dev);
 		return count;
 	}
 fail:
 	mutex_unlock(&data->mutex);
+	pm_runtime_put_sync(dev);
 	return ret_val;
 }
 
-static ssize_t als_power_status_show(struct device *dev,
-			struct device_attribute *attr, char *buf)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	int ret_val;
-	ret_val = i2c_smbus_read_byte_data(client, 0x80);
-	if (ret_val < 0)
-		return ret_val;
-	ret_val = ret_val & 0x01;
-	return sprintf(buf, "%d\n", ret_val);
-}
-
 static int als_set_power_state(struct i2c_client *client, bool on_off)
 {
 	int ret_val;
@@ -163,39 +187,13 @@ fail:
 	return ret_val;
 }
 
-static ssize_t als_power_status_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	struct als_data *data = i2c_get_clientdata(client);
-	unsigned long val;
-	int ret_val;
-
-	if (strict_strtoul(buf, 10, &val))
-		return -EINVAL;
-	if (val == POWER_STA_ENABLE) {
-		ret_val = als_set_power_state(client, true);
-		data->needresume = true;
-	} else if (val == POWER_STA_DISABLE) {
-		ret_val = als_set_power_state(client, false);
-		data->needresume = false;
-	} else
-		return -EINVAL;
-	if (ret_val < 0)
-		return ret_val;
-	return count;
-}
-
 static DEVICE_ATTR(lux0_sensor_range, S_IRUGO | S_IWUSR,
 	als_sensing_range_show, als_sensing_range_store);
 static DEVICE_ATTR(lux0_input, S_IRUGO, als_lux0_input_data_show, NULL);
-static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
-	als_power_status_show, als_power_status_store);
 
 static struct attribute *mid_att_als[] = {
 	&dev_attr_lux0_sensor_range.attr,
 	&dev_attr_lux0_input.attr,
-	&dev_attr_power_state.attr,
 	NULL
 };
 
@@ -213,15 +211,21 @@ static int als_set_default_config(struct i2c_client *client)
 		dev_err(&client->dev, "failed default switch on write\n");
 		return ret_val;
 	}
-	/* Continous from 1Lux to 64k Lux */
-	ret_val = i2c_smbus_write_byte_data(client, 0x81, 0x04);
+	/* detection range: 1~64K Lux, maunal measurement */
+	ret_val = i2c_smbus_write_byte_data(client, 0x81, 0x08);
 	if (ret_val < 0)
 		dev_err(&client->dev, "failed default LUX on write\n");
+
+	/*  We always get 0 for the 1st measurement after system power on,
+	 *  so make sure it is finished before user asks for data.
+	 */
+	als_wait_for_data_ready(&client->dev);
+
 	return ret_val;
 }
 
-static int  apds9802als_probe(struct i2c_client *client,
-					const struct i2c_device_id *id)
+static int apds9802als_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
 {
 	int res;
 	struct als_data *data;
@@ -237,11 +241,14 @@ static int  apds9802als_probe(struct i2c_client *client,
 		dev_err(&client->dev, "device create file failed\n");
 		goto als_error1;
 	}
-	dev_info(&client->dev,
-		"%s apds9802als: ALS chip found\n", client->name);
+	dev_info(&client->dev, "ALS chip found\n");
 	als_set_default_config(client);
-	data->needresume = true;
 	mutex_init(&data->mutex);
+
+	pm_runtime_enable(&client->dev);
+	pm_runtime_get(&client->dev);
+	pm_runtime_put(&client->dev);
+
 	return res;
 als_error1:
 	i2c_set_clientdata(client, NULL);
@@ -252,6 +259,8 @@ als_error1:
 static int apds9802als_remove(struct i2c_client *client)
 {
 	struct als_data *data = i2c_get_clientdata(client);
+
+	als_set_power_state(client, false);
 	sysfs_remove_group(&client->dev.kobj, &m_als_gr);
 	kfree(data);
 	return 0;
@@ -265,13 +274,34 @@ static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
 
 static int apds9802als_resume(struct i2c_client *client)
 {
-	struct als_data *data = i2c_get_clientdata(client);
+	als_set_default_config(client);
+
+	pm_runtime_get(&client->dev);
+	pm_runtime_put(&client->dev);
+	return 0;
+}
+
+static int apds9802als_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	als_set_power_state(client, false);
+	return 0;
+}
+
+static int apds9802als_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
 
-	if (data->needresume == true)
-		als_set_power_state(client, true);
+	als_set_power_state(client, true);
 	return 0;
 }
 
+static const struct dev_pm_ops apds9802als_pm_ops = {
+	.runtime_suspend = apds9802als_runtime_suspend,
+	.runtime_resume = apds9802als_runtime_resume,
+};
+
 static struct i2c_device_id apds9802als_id[] = {
 	{ DRIVER_NAME, 0 },
 	{ }
@@ -281,8 +311,8 @@ MODULE_DEVICE_TABLE(i2c, apds9802als_id);
 
 static struct i2c_driver apds9802als_driver = {
 	.driver = {
-	.name = DRIVER_NAME,
-	.owner = THIS_MODULE,
+		.name = DRIVER_NAME,
+		.pm = &apds9802als_pm_ops,
 	},
 	.probe = apds9802als_probe,
 	.remove = apds9802als_remove,

Re: [PATCH] apds9802als: add runtime PM support

[Alan Stern]

On Fri, 15 Oct 2010, Alan Cox wrote:

> From: Hong Liu <hong.liu@intel.com>
> 
> 
>     Update the driver for the needed runtime power features. Remove the old
>     user controlled power functions.
> 
>     Signed-off-by: Hong Liu <hong.liu@intel.com>
>     Signed-off-by: Alan Cox <alan@linux.intel.com>

> diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
> index fbe4960..15f9436 100644
> --- a/drivers/misc/apds9802als.c
> +++ b/drivers/misc/apds9802als.c

> @@ -265,13 +274,34 @@ static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
>  
>  static int apds9802als_resume(struct i2c_client *client)
>  {
> -	struct als_data *data = i2c_get_clientdata(client);
> +	als_set_default_config(client);
> +
> +	pm_runtime_get(&client->dev);
> +	pm_runtime_put(&client->dev);
> +	return 0;
> +}

This almost certainly does not do what you think.

pm_runtime_get() will increment the device's usage count and queue an 
asynchronous resume request.  However, since the PM workqueue is frozen 
during system sleep transitions, the device will remain suspended.

The pm_runtime_put() will decrement the usage count again, but since
there is already an async resume on the queue it will not queue an
async suspend.  The final result will be that when tasks are unfrozen,
the device will finally be resumed -- long after it should have been.

It looks like what you want to do here is simply call
apds9802als_runtime_resume() directly.  And according to the advice in
Documentation/power/runtime_pm.txt section 6, you should also call

	pm_runtime_disable(dev);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

You probably also do not want the asynchronous calls to 
pm_runtime_get() and pm_runtime_put() in apds9802als_probe().  A more 
common sequence is:

	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

Alan Stern

Re: [PATCH] apds9802als: add runtime PM support

[Alan Stern]

On Fri, 15 Oct 2010, Alan Cox wrote:

> From: Hong Liu <hong.liu@intel.com>
> 
> 
>     Update the driver for the needed runtime power features. Remove the old
>     user controlled power functions.
> 
>     Signed-off-by: Hong Liu <hong.liu@intel.com>
>     Signed-off-by: Alan Cox <alan@linux.intel.com>

> diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
> index fbe4960..15f9436 100644
> --- a/drivers/misc/apds9802als.c
> +++ b/drivers/misc/apds9802als.c

> @@ -265,13 +274,34 @@ static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
>  
>  static int apds9802als_resume(struct i2c_client *client)
>  {
> -	struct als_data *data = i2c_get_clientdata(client);
> +	als_set_default_config(client);
> +
> +	pm_runtime_get(&client->dev);
> +	pm_runtime_put(&client->dev);
> +	return 0;
> +}

This almost certainly does not do what you think.

pm_runtime_get() will increment the device's usage count and queue an 
asynchronous resume request.  However, since the PM workqueue is frozen 
during system sleep transitions, the device will remain suspended.

The pm_runtime_put() will decrement the usage count again, but since
there is already an async resume on the queue it will not queue an
async suspend.  The final result will be that when tasks are unfrozen,
the device will finally be resumed -- long after it should have been.

It looks like what you want to do here is simply call
apds9802als_runtime_resume() directly.  And according to the advice in
Documentation/power/runtime_pm.txt section 6, you should also call

	pm_runtime_disable(dev);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

You probably also do not want the asynchronous calls to 
pm_runtime_get() and pm_runtime_put() in apds9802als_probe().  A more 
common sequence is:

	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

Alan Stern

Re: [PATCH] apds9802als: add runtime PM support

[Andrew Morton]

On Fri, 15 Oct 2010 14:47:25 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

>     Update the driver for the needed runtime power features. Remove the old
>     user controlled power functions.

How's this look?



put PM code under CONFIG_PM

Cc: Alan Cox <alan@linux.intel.com>
Cc: Hong Liu <hong.liu@intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/misc/apds9802als.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff -puN drivers/misc/apds9802als.c~drivers-misc-apds9802alsc-add-runtime-pm-support-fix drivers/misc/apds9802als.c
--- a/drivers/misc/apds9802als.c~drivers-misc-apds9802alsc-add-runtime-pm-support-fix
+++ a/drivers/misc/apds9802als.c
@@ -266,6 +266,7 @@ static int apds9802als_remove(struct i2c
 	return 0;
 }
 
+#ifdef CONFIG_PM
 static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
 {
 	als_set_power_state(client, false);
@@ -302,6 +303,14 @@ static const struct dev_pm_ops apds9802a
 	.runtime_resume = apds9802als_runtime_resume,
 };
 
+#define APDS9802ALS_PM_OPS (&apds9802als_pm_ops)
+
+#else	/* CONFIG_PM */
+#define apds9802als_suspend NULL
+#define apds9802als_resume NULL
+#define APDS9802ALS_PM_OPS NULL
+#endif	/* CONFIG_PM */
+
 static struct i2c_device_id apds9802als_id[] = {
 	{ DRIVER_NAME, 0 },
 	{ }
@@ -312,7 +321,7 @@ MODULE_DEVICE_TABLE(i2c, apds9802als_id)
 static struct i2c_driver apds9802als_driver = {
 	.driver = {
 		.name = DRIVER_NAME,
-		.pm = &apds9802als_pm_ops,
+		.pm = APDS9802ALS_PM_OPS,
 	},
 	.probe = apds9802als_probe,
 	.remove = apds9802als_remove,
_


And a question.  This driver puts the .suspend and .resume pointers
into the `struct i2c_driver'.  However drivers/misc/isl29020.c does not
do that.  What's up with that?

Re: [PATCH] apds9802als: add runtime PM support

[Hong Liu]

On Fri, 2010-10-22 at 02:34 +0800, Alan Stern wrote:
> On Fri, 15 Oct 2010, Alan Cox wrote:
> 
> > From: Hong Liu <hong.liu@intel.com>
> > 
> > 
> >     Update the driver for the needed runtime power features. Remove the old
> >     user controlled power functions.
> > 
> >     Signed-off-by: Hong Liu <hong.liu@intel.com>
> >     Signed-off-by: Alan Cox <alan@linux.intel.com>
> 
> > diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
> > index fbe4960..15f9436 100644
> > --- a/drivers/misc/apds9802als.c
> > +++ b/drivers/misc/apds9802als.c
> 
> > @@ -265,13 +274,34 @@ static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
> >  
> >  static int apds9802als_resume(struct i2c_client *client)
> >  {
> > -	struct als_data *data = i2c_get_clientdata(client);
> > +	als_set_default_config(client);
> > +
> > +	pm_runtime_get(&client->dev);
> > +	pm_runtime_put(&client->dev);
> > +	return 0;
> > +}
> 

Hi, Alan

Thanks for your review.

> This almost certainly does not do what you think.
> 
> pm_runtime_get() will increment the device's usage count and queue an 
> asynchronous resume request.  However, since the PM workqueue is frozen 
> during system sleep transitions, the device will remain suspended.
> 
> The pm_runtime_put() will decrement the usage count again, but since
> there is already an async resume on the queue it will not queue an
> async suspend.  The final result will be that when tasks are unfrozen,
> the device will finally be resumed -- long after it should have been.
> 
> It looks like what you want to do here is simply call
> apds9802als_runtime_resume() directly.  

You mean apds9802als_runtime_suspend()? I want to put the device into
runtime suspended state, can I just call pm_runtime_suspend() directly?

> And according to the advice in
> Documentation/power/runtime_pm.txt section 6, you should also call
> 
> 	pm_runtime_disable(dev);
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);
> 
> You probably also do not want the asynchronous calls to 
> pm_runtime_get() and pm_runtime_put() in apds9802als_probe().  A more 
> common sequence is:
> 
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);

I want to put the device into runtime suspend state after probe, so
pm_runtime_suspend() after these two calls is OK?

Thanks,
Hong

> 
> Alan Stern
> 

Re: [PATCH] apds9802als: add runtime PM support

[Hong Liu]

On Fri, 2010-10-22 at 02:34 +0800, Alan Stern wrote:
> On Fri, 15 Oct 2010, Alan Cox wrote:
> 
> > From: Hong Liu <hong.liu@intel.com>
> > 
> > 
> >     Update the driver for the needed runtime power features. Remove the old
> >     user controlled power functions.
> > 
> >     Signed-off-by: Hong Liu <hong.liu@intel.com>
> >     Signed-off-by: Alan Cox <alan@linux.intel.com>
> 
> > diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
> > index fbe4960..15f9436 100644
> > --- a/drivers/misc/apds9802als.c
> > +++ b/drivers/misc/apds9802als.c
> 
> > @@ -265,13 +274,34 @@ static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
> >  
> >  static int apds9802als_resume(struct i2c_client *client)
> >  {
> > -	struct als_data *data = i2c_get_clientdata(client);
> > +	als_set_default_config(client);
> > +
> > +	pm_runtime_get(&client->dev);
> > +	pm_runtime_put(&client->dev);
> > +	return 0;
> > +}
> 

Hi, Alan

Thanks for your review.

> This almost certainly does not do what you think.
> 
> pm_runtime_get() will increment the device's usage count and queue an 
> asynchronous resume request.  However, since the PM workqueue is frozen 
> during system sleep transitions, the device will remain suspended.
> 
> The pm_runtime_put() will decrement the usage count again, but since
> there is already an async resume on the queue it will not queue an
> async suspend.  The final result will be that when tasks are unfrozen,
> the device will finally be resumed -- long after it should have been.
> 
> It looks like what you want to do here is simply call
> apds9802als_runtime_resume() directly.  

You mean apds9802als_runtime_suspend()? I want to put the device into
runtime suspended state, can I just call pm_runtime_suspend() directly?

> And according to the advice in
> Documentation/power/runtime_pm.txt section 6, you should also call
> 
> 	pm_runtime_disable(dev);
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);
> 
> You probably also do not want the asynchronous calls to 
> pm_runtime_get() and pm_runtime_put() in apds9802als_probe().  A more 
> common sequence is:
> 
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);

I want to put the device into runtime suspend state after probe, so
pm_runtime_suspend() after these two calls is OK?

Thanks,
Hong

> 
> Alan Stern
> 

Re: [PATCH] apds9802als: add runtime PM support

[Alan Stern]

On Tue, 26 Oct 2010, Hong Liu wrote:

> > > diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
> > > index fbe4960..15f9436 100644
> > > --- a/drivers/misc/apds9802als.c
> > > +++ b/drivers/misc/apds9802als.c
> > 
> > > @@ -265,13 +274,34 @@ static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
> > >  
> > >  static int apds9802als_resume(struct i2c_client *client)
> > >  {
> > > -	struct als_data *data = i2c_get_clientdata(client);
> > > +	als_set_default_config(client);
> > > +
> > > +	pm_runtime_get(&client->dev);
> > > +	pm_runtime_put(&client->dev);
> > > +	return 0;
> > > +}
> > 
> 
> Hi, Alan
> 
> Thanks for your review.
> 
> > This almost certainly does not do what you think.
> > 
> > pm_runtime_get() will increment the device's usage count and queue an 
> > asynchronous resume request.  However, since the PM workqueue is frozen 
> > during system sleep transitions, the device will remain suspended.
> > 
> > The pm_runtime_put() will decrement the usage count again, but since
> > there is already an async resume on the queue it will not queue an
> > async suspend.  The final result will be that when tasks are unfrozen,
> > the device will finally be resumed -- long after it should have been.
> > 
> > It looks like what you want to do here is simply call
> > apds9802als_runtime_resume() directly.  
> 
> You mean apds9802als_runtime_suspend()?

This is about the function listed above: apds9802als_resume().  It
should call apds9802als_runtime_resume() directly.  I don't see any
reason why you would want it to call apds9802als_runtime_suspend() --
having a resume function that actually suspends the device doesn't make
any sense.

> I want to put the device into
> runtime suspended state, can I just call pm_runtime_suspend() directly?

Why do you want apds9802als_resume() to put the device into a suspended 
state?  Shouldn't it _resume_ the device?

> > And according to the advice in
> > Documentation/power/runtime_pm.txt section 6, you should also call

Have you read that document?

> > 	pm_runtime_disable(dev);
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_enable(dev);
> > 
> > You probably also do not want the asynchronous calls to 
> > pm_runtime_get() and pm_runtime_put() in apds9802als_probe().  A more 
> > common sequence is:
> > 
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_enable(dev);
> 
> I want to put the device into runtime suspend state after probe, so
> pm_runtime_suspend() after these two calls is OK?

No.  Try doing what I said.  You should find that the device _does_ get
runtime-suspended when the probe function returns.  Assuming you have
defined a proper apds9802als_runtime_idle() function.

Alan Stern

Re: [PATCH] apds9802als: add runtime PM support

[Alan Stern]

On Tue, 26 Oct 2010, Hong Liu wrote:

> > > diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
> > > index fbe4960..15f9436 100644
> > > --- a/drivers/misc/apds9802als.c
> > > +++ b/drivers/misc/apds9802als.c
> > 
> > > @@ -265,13 +274,34 @@ static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
> > >  
> > >  static int apds9802als_resume(struct i2c_client *client)
> > >  {
> > > -	struct als_data *data = i2c_get_clientdata(client);
> > > +	als_set_default_config(client);
> > > +
> > > +	pm_runtime_get(&client->dev);
> > > +	pm_runtime_put(&client->dev);
> > > +	return 0;
> > > +}
> > 
> 
> Hi, Alan
> 
> Thanks for your review.
> 
> > This almost certainly does not do what you think.
> > 
> > pm_runtime_get() will increment the device's usage count and queue an 
> > asynchronous resume request.  However, since the PM workqueue is frozen 
> > during system sleep transitions, the device will remain suspended.
> > 
> > The pm_runtime_put() will decrement the usage count again, but since
> > there is already an async resume on the queue it will not queue an
> > async suspend.  The final result will be that when tasks are unfrozen,
> > the device will finally be resumed -- long after it should have been.
> > 
> > It looks like what you want to do here is simply call
> > apds9802als_runtime_resume() directly.  
> 
> You mean apds9802als_runtime_suspend()?

This is about the function listed above: apds9802als_resume().  It
should call apds9802als_runtime_resume() directly.  I don't see any
reason why you would want it to call apds9802als_runtime_suspend() --
having a resume function that actually suspends the device doesn't make
any sense.

> I want to put the device into
> runtime suspended state, can I just call pm_runtime_suspend() directly?

Why do you want apds9802als_resume() to put the device into a suspended 
state?  Shouldn't it _resume_ the device?

> > And according to the advice in
> > Documentation/power/runtime_pm.txt section 6, you should also call

Have you read that document?

> > 	pm_runtime_disable(dev);
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_enable(dev);
> > 
> > You probably also do not want the asynchronous calls to 
> > pm_runtime_get() and pm_runtime_put() in apds9802als_probe().  A more 
> > common sequence is:
> > 
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_enable(dev);
> 
> I want to put the device into runtime suspend state after probe, so
> pm_runtime_suspend() after these two calls is OK?

No.  Try doing what I said.  You should find that the device _does_ get
runtime-suspended when the probe function returns.  Assuming you have
defined a proper apds9802als_runtime_idle() function.

Alan Stern

Re: [PATCH] apds9802als: add runtime PM support

[Hong Liu]

On Tue, 2010-10-26 at 21:51 +0800, Alan Stern wrote:
> On Tue, 26 Oct 2010, Hong Liu wrote:
> 
> > > > diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
> > > > index fbe4960..15f9436 100644
> > > > --- a/drivers/misc/apds9802als.c
> > > > +++ b/drivers/misc/apds9802als.c
> > > 
> > > > @@ -265,13 +274,34 @@ static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
> > > >  
> > > >  static int apds9802als_resume(struct i2c_client *client)
> > > >  {
> > > > -	struct als_data *data = i2c_get_clientdata(client);
> > > > +	als_set_default_config(client);
> > > > +
> > > > +	pm_runtime_get(&client->dev);
> > > > +	pm_runtime_put(&client->dev);
> > > > +	return 0;
> > > > +}
> > > 
> > 
> > Hi, Alan
> > 
> > Thanks for your review.
> > 
> > > This almost certainly does not do what you think.
> > > 
> > > pm_runtime_get() will increment the device's usage count and queue an 
> > > asynchronous resume request.  However, since the PM workqueue is frozen 
> > > during system sleep transitions, the device will remain suspended.
> > > 
> > > The pm_runtime_put() will decrement the usage count again, but since
> > > there is already an async resume on the queue it will not queue an
> > > async suspend.  The final result will be that when tasks are unfrozen,
> > > the device will finally be resumed -- long after it should have been.
> > > 
> > > It looks like what you want to do here is simply call
> > > apds9802als_runtime_resume() directly.  
> > 
> > You mean apds9802als_runtime_suspend()?
> 
> This is about the function listed above: apds9802als_resume().  It
> should call apds9802als_runtime_resume() directly.  I don't see any
> reason why you would want it to call apds9802als_runtime_suspend() --
> having a resume function that actually suspends the device doesn't make
> any sense.

In apds9802als_resume, I just want to power up the device, do some
configuration, and then runtime_suspend the device.

We don't have runtime_idle() implemented, the driver exported sysfs
entries to user space for data reading, and we do runtime_resume,
measaure data, runtime_suspend in the read/write funciton of those sysfs
entries. This is why I want to put the device into runtime suspend
manually whenever possible.


> 
> > I want to put the device into
> > runtime suspended state, can I just call pm_runtime_suspend() directly?
> 
> Why do you want apds9802als_resume() to put the device into a suspended 
> state?  Shouldn't it _resume_ the device?
> 
> > > And according to the advice in
> > > Documentation/power/runtime_pm.txt section 6, you should also call
> 
> Have you read that document?
> 
> > > 	pm_runtime_disable(dev);
> > > 	pm_runtime_set_active(dev);
> > > 	pm_runtime_enable(dev);
> > > 
> > > You probably also do not want the asynchronous calls to 
> > > pm_runtime_get() and pm_runtime_put() in apds9802als_probe().  A more 
> > > common sequence is:
> > > 
> > > 	pm_runtime_set_active(dev);
> > > 	pm_runtime_enable(dev);
> > 
> > I want to put the device into runtime suspend state after probe, so
> > pm_runtime_suspend() after these two calls is OK?
> 
> No.  Try doing what I said.  You should find that the device _does_ get
> runtime-suspended when the probe function returns.  Assuming you have
> defined a proper apds9802als_runtime_idle() function.

We don't have runtime_idle() implemented, so I have to manually put the
device into runtime suspend.

Thanks,
Hong

> 
> Alan Stern
> 

Re: [PATCH] apds9802als: add runtime PM support

[Hong Liu]

On Tue, 2010-10-26 at 21:51 +0800, Alan Stern wrote:
> On Tue, 26 Oct 2010, Hong Liu wrote:
> 
> > > > diff --git a/drivers/misc/apds9802als.c b/drivers/misc/apds9802als.c
> > > > index fbe4960..15f9436 100644
> > > > --- a/drivers/misc/apds9802als.c
> > > > +++ b/drivers/misc/apds9802als.c
> > > 
> > > > @@ -265,13 +274,34 @@ static int apds9802als_suspend(struct i2c_client *client, pm_message_t mesg)
> > > >  
> > > >  static int apds9802als_resume(struct i2c_client *client)
> > > >  {
> > > > -	struct als_data *data = i2c_get_clientdata(client);
> > > > +	als_set_default_config(client);
> > > > +
> > > > +	pm_runtime_get(&client->dev);
> > > > +	pm_runtime_put(&client->dev);
> > > > +	return 0;
> > > > +}
> > > 
> > 
> > Hi, Alan
> > 
> > Thanks for your review.
> > 
> > > This almost certainly does not do what you think.
> > > 
> > > pm_runtime_get() will increment the device's usage count and queue an 
> > > asynchronous resume request.  However, since the PM workqueue is frozen 
> > > during system sleep transitions, the device will remain suspended.
> > > 
> > > The pm_runtime_put() will decrement the usage count again, but since
> > > there is already an async resume on the queue it will not queue an
> > > async suspend.  The final result will be that when tasks are unfrozen,
> > > the device will finally be resumed -- long after it should have been.
> > > 
> > > It looks like what you want to do here is simply call
> > > apds9802als_runtime_resume() directly.  
> > 
> > You mean apds9802als_runtime_suspend()?
> 
> This is about the function listed above: apds9802als_resume().  It
> should call apds9802als_runtime_resume() directly.  I don't see any
> reason why you would want it to call apds9802als_runtime_suspend() --
> having a resume function that actually suspends the device doesn't make
> any sense.

In apds9802als_resume, I just want to power up the device, do some
configuration, and then runtime_suspend the device.

We don't have runtime_idle() implemented, the driver exported sysfs
entries to user space for data reading, and we do runtime_resume,
measaure data, runtime_suspend in the read/write funciton of those sysfs
entries. This is why I want to put the device into runtime suspend
manually whenever possible.


> 
> > I want to put the device into
> > runtime suspended state, can I just call pm_runtime_suspend() directly?
> 
> Why do you want apds9802als_resume() to put the device into a suspended 
> state?  Shouldn't it _resume_ the device?
> 
> > > And according to the advice in
> > > Documentation/power/runtime_pm.txt section 6, you should also call
> 
> Have you read that document?
> 
> > > 	pm_runtime_disable(dev);
> > > 	pm_runtime_set_active(dev);
> > > 	pm_runtime_enable(dev);
> > > 
> > > You probably also do not want the asynchronous calls to 
> > > pm_runtime_get() and pm_runtime_put() in apds9802als_probe().  A more 
> > > common sequence is:
> > > 
> > > 	pm_runtime_set_active(dev);
> > > 	pm_runtime_enable(dev);
> > 
> > I want to put the device into runtime suspend state after probe, so
> > pm_runtime_suspend() after these two calls is OK?
> 
> No.  Try doing what I said.  You should find that the device _does_ get
> runtime-suspended when the probe function returns.  Assuming you have
> defined a proper apds9802als_runtime_idle() function.

We don't have runtime_idle() implemented, so I have to manually put the
device into runtime suspend.

Thanks,
Hong

> 
> Alan Stern
> 

Re: [PATCH] apds9802als: add runtime PM support

[Alan Stern]

On Wed, 27 Oct 2010, Hong Liu wrote:

> In apds9802als_resume, I just want to power up the device, do some
> configuration, and then runtime_suspend the device.

Then you should change the function the way I suggested.  The PM core
will automatically do a runtime suspend after the system has fully
woken up.

> We don't have runtime_idle() implemented, the driver exported sysfs
> entries to user space for data reading, and we do runtime_resume,
> measaure data, runtime_suspend in the read/write funciton of those sysfs
> entries. This is why I want to put the device into runtime suspend
> manually whenever possible.

Your approach will not work.  You _must_ define runtime_idle() if you 
want your driver to suspend correctly.  It doesn't have to be 
complicated; just make it call pm_runtime_suspend().

...

> We don't have runtime_idle() implemented, so I have to manually put the
> device into runtime suspend.

The problem is that the device can be runtime-resumed by code other 
than your own, and that other code will not always do a runtime-suspend 
afterward -- it will do a runtime-idle.  That's why you need to define 
a runtime_idle() callback.

Alan Stern

Re: [PATCH] apds9802als: add runtime PM support

[Alan Stern]

On Wed, 27 Oct 2010, Hong Liu wrote:

> In apds9802als_resume, I just want to power up the device, do some
> configuration, and then runtime_suspend the device.

Then you should change the function the way I suggested.  The PM core
will automatically do a runtime suspend after the system has fully
woken up.

> We don't have runtime_idle() implemented, the driver exported sysfs
> entries to user space for data reading, and we do runtime_resume,
> measaure data, runtime_suspend in the read/write funciton of those sysfs
> entries. This is why I want to put the device into runtime suspend
> manually whenever possible.

Your approach will not work.  You _must_ define runtime_idle() if you 
want your driver to suspend correctly.  It doesn't have to be 
complicated; just make it call pm_runtime_suspend().

..

> We don't have runtime_idle() implemented, so I have to manually put the
> device into runtime suspend.

The problem is that the device can be runtime-resumed by code other 
than your own, and that other code will not always do a runtime-suspend 
afterward -- it will do a runtime-idle.  That's why you need to define 
a runtime_idle() callback.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Geschäftsführender Gesellschafter: Tjark Auerbach
Sitz der Gesellschaft: Tettnang
Handelsregister: Amtsgericht Ulm, HRB 630992
ALLGEMEINE GESCHÄFTSBEDINGUNGEN
Es gelten unsere Allgemeinen Geschäftsbedingungen
(AGB). Sie finden sie in der jeweils gültigen Fassung
im Internet unter http://www.avira.com/de/standard-terms-conditions-business-de
***************************************************
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: [PATCH] apds9802als: add runtime PM support

[Alan Stern]

On Wed, 27 Oct 2010, Hong Liu wrote:

> In apds9802als_resume, I just want to power up the device, do some
> configuration, and then runtime_suspend the device.

Then you should change the function the way I suggested.  The PM core
will automatically do a runtime suspend after the system has fully
woken up.

> We don't have runtime_idle() implemented, the driver exported sysfs
> entries to user space for data reading, and we do runtime_resume,
> measaure data, runtime_suspend in the read/write funciton of those sysfs
> entries. This is why I want to put the device into runtime suspend
> manually whenever possible.

Your approach will not work.  You _must_ define runtime_idle() if you 
want your driver to suspend correctly.  It doesn't have to be 
complicated; just make it call pm_runtime_suspend().

...

> We don't have runtime_idle() implemented, so I have to manually put the
> device into runtime suspend.

The problem is that the device can be runtime-resumed by code other 
than your own, and that other code will not always do a runtime-suspend 
afterward -- it will do a runtime-idle.  That's why you need to define 
a runtime_idle() callback.

Alan Stern

Re: [PATCH] apds9802als: add runtime PM support

[Alan Stern]

On Fri, 29 Oct 2010, Hong Liu wrote:

> Hi, Alan
> 
> Thanks for your help, I still have some questions about the runtime PM
> after reading the doc you mentioned.
> 
> 
> On Wed, 2010-10-27 at 22:13 +0800, Alan Stern wrote:
> > On Wed, 27 Oct 2010, Hong Liu wrote:
> > 
> > > In apds9802als_resume, I just want to power up the device, do some
> > > configuration, and then runtime_suspend the device.
> > 
> > Then you should change the function the way I suggested.  The PM core
> > will automatically do a runtime suspend after the system has fully
> > woken up.
> 
> Would you please point out which part of the code does this? I am trying
> to find out...

In drivers/base/power/main.c:dpm_complete() there is a call to
pm_runtime_put_sync().

> I am still not very clear about who will be in charge of putting the
> device into runtime suspend state after the probe(). I saw code in
> e1000e driver, before your patch (f3ec4f8), it will manually schedule a
> runtime suspend after 1 second. But with your new code, I can't see
> which part of the code will put e1000e to runtime suspend.
> 
> @@ -5721,11 +5721,8 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
>  
>         e1000_print_device_info(adapter);
>  
> -       if (pci_dev_run_wake(pdev)) {
> -               pm_runtime_set_active(&pdev->dev);
> -               pm_runtime_enable(&pdev->dev);
> -       }
> -       pm_schedule_suspend(&pdev->dev, MSEC_PER_SEC);
> +       if (pci_dev_run_wake(pdev))
> +               pm_runtime_put_noidle(&pdev->dev);
>  
>         return 0;

I'm not surprised you couldn't find the code; you were looking in the
wrong place.  After a probe, the runtime suspend is done by either
drivers/base/dd.c:driver_probe_device() or device_attach(), depending
on whether the driver was already loaded when the device was
registered.  Both functions call pm_runtime_put_sync().

> I think the runtime PM is working at bottom-up way, the child device
> will be in charge of itself, and once it decides to put itself into
> runtime suspend, it's parent will be notified, and if all children are
> runtime suspended, then the parent can runtime suspend itself. While for
> runtime resume, the child will runtime resume the parent first.

That's right.

> > > We don't have runtime_idle() implemented, so I have to manually put the
> > > device into runtime suspend.
> > 
> > The problem is that the device can be runtime-resumed by code other 
> > than your own, and that other code will not always do a runtime-suspend 
> > afterward -- it will do a runtime-idle.  That's why you need to define 
> > a runtime_idle() callback.
> 
> I saw subsystem level runtime_idle will call runtime_suspend if no
> runtime_idle is defined for the driver. So do we need to define a
> runtime_idle in the driver just to call runtime_suspend()?

I didn't realize the subsystem defined runtime_idle for you.  In that 
case no, you don't need to define your own version.  But you also don't 
have to manually put the device into runtime suspend (as you wrote 
above).

Alan Stern