Re: [PATCH v1.1 4/5] smiapp: Use runtime PM

[Sakari Ailus <sakari.ailus@linux.intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1726 bytes --]

Hi Sebastian,

Thank you for the review.

On 09/20/16 01:51, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Sep 16, 2016 at 01:53:29AM +0300, Sakari Ailus wrote:
>> [...]
>>
>> diff --git a/drivers/media/i2c/smiapp/smiapp-regs.c b/drivers/media/i2c/smiapp/smiapp-regs.c
>> index 1e501c0..a9c7baf 100644
>> --- a/drivers/media/i2c/smiapp/smiapp-regs.c
>> +++ b/drivers/media/i2c/smiapp/smiapp-regs.c
>> @@ -18,6 +18,7 @@
>>  
>>  #include <linux/delay.h>
>>  #include <linux/i2c.h>
>> +#include <linux/pm_runtime.h>
>>  
>>  #include "smiapp.h"
>>  #include "smiapp-regs.h"
>> @@ -288,8 +289,12 @@ int smiapp_write_no_quirk(struct smiapp_sensor *sensor, u32 reg, u32 val)
>>   */
>>  int smiapp_write(struct smiapp_sensor *sensor, u32 reg, u32 val)
>>  {
>> +	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
>>  	int rval;
>>  
>> +	if (pm_runtime_suspended(&client->dev))
>> +		return 0;
>> +
> 
> This looks racy. What if idle countdown runs out immediately after
> this check? If you can't call get_sync in this function you can
> call pm_runtime_get() before the suspend check and pm_runtime_put
> before returning from the function, so that the device keeps being
> enabled.

Good point. It was probably late when I wrote the patch. X-)

I guess I need to put pm_runtime_get_noresume() before that, and then
put_autosuspend() it later on.

> 
> Also I would expect some error code instead of success for early
> return due to device being suspended?

That's by design.

If the sensor is off, there's no need to write anything there. The
configuration is re-applied to the sensor when it's powered on.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]