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@xxxxxxxxxxxxxxx
Attachment:
signature.asc
Description: OpenPGP digital signature
