drivers/media/i2c/ov5640.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
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);
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;
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. > 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; -- Kind regards, Sakari Ailus
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. -- Best regards, Andrey Skvortsov
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. > > 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; > > -- > Kind regards, > > Sakari Ailus
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; -- Kind regards, Sakari Ailus
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>
---
Changes since PATCHv1:
* Removed pm_runtime_mask_last_busy call from ov5640_write_reg and ov5640_read_reg
as suggested by Sakari Ailus
* Updated diff against next-20230113
drivers/media/i2c/ov5640.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e0f908af581b..24f4f395aad6 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -3316,6 +3316,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;
@@ -3391,6 +3392,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;
@@ -3710,8 +3712,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;
}
@@ -3912,6 +3916,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;
--
2.39.0
© 2016 - 2024 Red Hat, Inc.