Messages in this thread | | | Date | Mon, 26 Apr 2021 16:01:51 +0200 | From | Mauro Carvalho Chehab <> | Subject | Re: [PATCH 25/78] media: i2c: ccs-core: use pm_runtime_resume_and_get() |
| |
Em Sun, 25 Apr 2021 21:55:25 +0300 Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> Hi Mauro, > > Thanks for the patch. > > On Sat, Apr 24, 2021 at 08:44:35AM +0200, Mauro Carvalho Chehab wrote: > > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter") > > added pm_runtime_resume_and_get() in order to automatically handle > > dev->power.usage_count decrement on errors. > > > > Use the new API, in order to cleanup the error check logic. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > --- > > drivers/media/i2c/ccs/ccs-core.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > > index 9dc3f45da3dc..1441ddcc9b35 100644 > > --- a/drivers/media/i2c/ccs/ccs-core.c > > +++ b/drivers/media/i2c/ccs/ccs-core.c > > @@ -1880,12 +1880,11 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > > int rval; > > > > - rval = pm_runtime_get_sync(&client->dev); > > - if (rval < 0) { > > - pm_runtime_put_noidle(&client->dev); > > - > > + rval = pm_runtime_resume_and_get(&client->dev); > > + if (rval < 0) > > return rval; > > - } else if (!rval) { > > + > > + if (!rval) { > > rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> > > ctrl_handler); > > if (rval) > > @@ -3089,7 +3088,7 @@ static int __maybe_unused ccs_suspend(struct device *dev) > > bool streaming = sensor->streaming; > > int rval; > > > > - rval = pm_runtime_get_sync(dev); > > + rval = pm_runtime_resume_and_get(dev); > > if (rval < 0) { > > pm_runtime_put_noidle(dev); > > You'll need to drop pm_runtime_put_noidle() here.
OK!
---
On a non-related issue at the same code, after the change, the suspend function will be:
static int __maybe_unused ccs_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); struct v4l2_subdev *subdev = i2c_get_clientdata(client); struct ccs_sensor *sensor = to_ccs_sensor(subdev); bool streaming = sensor->streaming; int rval;
rval = pm_runtime_resume_and_get(dev); if (rval < 0) return -EAGAIN;
if (sensor->streaming) ccs_stop_streaming(sensor);
/* save state for resume */ sensor->streaming = streaming;
return 0; }
Not sure if "return -EAGAIN" is the right thing here. I mean, the PM runtime core has two error conditions that are independent on whatever the PM callback would be doing[1]:
if (dev->power.runtime_error) retval = -EINVAL; else if (dev->power.disable_depth > 0) retval = -EACCES;
It would be very unlikely that trying to suspend again would solve those conditions.
So, I guess that the right thing to do is to change the code to do, instead:
static int __maybe_unused ccs_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); struct v4l2_subdev *subdev = i2c_get_clientdata(client); struct ccs_sensor *sensor = to_ccs_sensor(subdev); bool streaming = sensor->streaming; int rval;
rval = pm_runtime_resume_and_get(dev); if (rval < 0) return rval; ... }
[1] see rpm_resume() code at drivers/base/power/runtime.c.
Thanks, Mauro
| |