| Message ID | 20230114112109.982005-1-andrej.skvortzov@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: ov5640: Update last busy timestamp to reset autosuspend timer | expand |
Hi Jacopo, On Sat, Jan 14, 2023 at 04:19:06PM +0100, Jacopo Mondi wrote: > Hi Sakari > > On Sat, Jan 14, 2023 at 02:29:14PM +0200, Sakari Ailus wrote: > > Hi Andrey, > > > > On Sat, Jan 14, 2023 at 02:21:09PM +0300, Andrey Skvortsov wrote: > > > Otherwise autosuspend delay doesn't work and power is cut off > > > immediately as device is freed. > > > > > > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com> > > > --- > > > drivers/media/i2c/ov5640.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > > index ac35350..f71c0f7 100644 > > > --- a/drivers/media/i2c/ov5640.c > > > +++ b/drivers/media/i2c/ov5640.c > > > @@ -1238,6 +1238,7 @@ static int ov5640_write_reg(struct ov5640_dev *sensor, u16 reg, u8 val) > > > return ret; > > > } > > > > > > + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); > > > return 0; > > > } > > > > > > @@ -1305,6 +1306,7 @@ static int ov5640_read_reg(struct ov5640_dev *sensor, u16 reg, u8 *val) > > > } > > > > > > *val = buf[0]; > > > + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); > > > > I wouldn't add these calls to register accesses. It's generally relevant > > with autosuspend_put(). > > > > The rest seems fine to me. > > > > Does it mean the same should be done for the all the sensor drivers that > use autosuspend ? I count 8 of them, not a huge number. pm_runtime_mark_last_busy() should be called before pm_runtime_put_autosuspend(). I was thinking of adding a helper that would simply call both of them. We don't have that yet though. > > > > return 0; > > > } > > > > > > @@ -3615,6 +3617,7 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > > > break; > > > } > > > > > > + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); > > > pm_runtime_put_autosuspend(&sensor->i2c_client->dev); > > > > > > return 0; > > > @@ -3702,6 +3705,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > > break; > > > } > > > > > > + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); > > > pm_runtime_put_autosuspend(&sensor->i2c_client->dev); > > > > > > return ret; > > > @@ -4034,8 +4038,10 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) > > > out: > > > mutex_unlock(&sensor->lock); > > > > > > - if (!enable || ret) > > > + if (!enable || ret) { > > > + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); > > > pm_runtime_put_autosuspend(&sensor->i2c_client->dev); > > > + } > > > > > > return ret; > > > } > > > @@ -4203,6 +4209,7 @@ static int ov5640_probe(struct i2c_client *client) > > > > > > pm_runtime_set_autosuspend_delay(dev, 1000); > > > pm_runtime_use_autosuspend(dev); > > > + pm_runtime_mark_last_busy(dev); > > > pm_runtime_put_autosuspend(dev); > > > > > > return 0;
On 23-01-14 14:29, Sakari Ailus wrote: > Hi Andrey, > > On Sat, Jan 14, 2023 at 02:21:09PM +0300, Andrey Skvortsov wrote: > > Otherwise autosuspend delay doesn't work and power is cut off > > immediately as device is freed. > > > > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com> > > --- > > drivers/media/i2c/ov5640.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index ac35350..f71c0f7 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -1238,6 +1238,7 @@ static int ov5640_write_reg(struct ov5640_dev *sensor, u16 reg, u8 val) > > return ret; > > } > > > > + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); > > return 0; > > } > > > > @@ -1305,6 +1306,7 @@ static int ov5640_read_reg(struct ov5640_dev *sensor, u16 reg, u8 *val) > > } > > > > *val = buf[0]; > > + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); > > I wouldn't add these calls to register accesses. It's generally relevant > with autosuspend_put(). > > The rest seems fine to me. > Thanks, Sakari, for the feedback. I wasn't sure about these calls either, I'll remove them in the v2.
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index ac35350..f71c0f7 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -1238,6 +1238,7 @@ static int ov5640_write_reg(struct ov5640_dev *sensor, u16 reg, u8 val) return ret; } + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); return 0; } @@ -1305,6 +1306,7 @@ static int ov5640_read_reg(struct ov5640_dev *sensor, u16 reg, u8 *val) } *val = buf[0]; + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); return 0; } @@ -3615,6 +3617,7 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) break; } + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); pm_runtime_put_autosuspend(&sensor->i2c_client->dev); return 0; @@ -3702,6 +3705,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) break; } + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); pm_runtime_put_autosuspend(&sensor->i2c_client->dev); return ret; @@ -4034,8 +4038,10 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) out: mutex_unlock(&sensor->lock); - if (!enable || ret) + if (!enable || ret) { + pm_runtime_mark_last_busy(&sensor->i2c_client->dev); pm_runtime_put_autosuspend(&sensor->i2c_client->dev); + } return ret; } @@ -4203,6 +4209,7 @@ static int ov5640_probe(struct i2c_client *client) pm_runtime_set_autosuspend_delay(dev, 1000); pm_runtime_use_autosuspend(dev); + pm_runtime_mark_last_busy(dev); pm_runtime_put_autosuspend(dev); return 0;
Otherwise autosuspend delay doesn't work and power is cut off immediately as device is freed. Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com> --- drivers/media/i2c/ov5640.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)