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

[Sebastian Reichel]

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.

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

>       rval = smiapp_call_quirk(sensor, reg_access, true, &reg, &val);
>       if (rval == -ENOIOCTLCMD)
>               return 0;
>
> [...]

-- Sebastian

signature.asc
Description: PGP signature