| Message ID | 20210624192200.22559-2-p.yadav@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CSI2RX support on J721E | expand |
Hi Pratyush, Thanks for the patch. On Fri, Jun 25, 2021 at 12:51:50AM +0530, Pratyush Yadav wrote: > Calling s_power subdev callback is discouraged. Instead, the subdevs > should use runtime PM to control its power. Use runtime PM callbacks to > control sensor power. The pm counter is incremented when the stream is > started and decremented when the stream is stopped. > > Refactor s_stream() a bit to make this new control flow easier. Add a > helper to choose whether mipi or dvp set_stream needs to be called. The > logic flow is also changed to make it a bit clearer. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > Changes in v3: > - Clean up the logic in ov5640_s_stream() a bit. > - Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(). > - Rename the label error_pm to disable_pm. > > Changes in v2: > - New in v2. > > drivers/media/i2c/Kconfig | 2 +- > drivers/media/i2c/ov5640.c | 127 +++++++++++++++++++++++-------------- > 2 files changed, 79 insertions(+), 50 deletions(-) > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 588f8eb95984..8f43a4d7bcc1 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -929,7 +929,7 @@ config VIDEO_OV2740 > > config VIDEO_OV5640 > tristate "OmniVision OV5640 sensor support" > - depends on OF > + depends on OF && PM Could you add support for runtime PM without requiring CONFIG_PM? Essentially you'll need to power on the device in probe and power it off in probe, and make sure the runtime PM nop variant functions return the value you'd expect. The ov5640_check_chip_id() function also calls ov5640_set_power() directly. That needs to be changed, too. > depends on GPIOLIB && VIDEO_V4L2 && I2C > select MEDIA_CONTROLLER > select VIDEO_V4L2_SUBDEV_API > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index f6e1e51e0375..2b7fd8631ad1 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -15,6 +15,7 @@ > #include <linux/init.h> > #include <linux/module.h> > #include <linux/of_device.h> > +#include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/types.h> > @@ -238,8 +239,6 @@ struct ov5640_dev { > /* lock to protect all members below */ > struct mutex lock; > > - int power_count; > - > struct v4l2_mbus_framefmt fmt; > bool pending_fmt_change; > > @@ -1277,6 +1276,14 @@ static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > on ? 0x00 : 0x0f); > } > > +static int ov5640_set_stream(struct ov5640_dev *sensor, bool on) > +{ > + if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) > + return ov5640_set_stream_mipi(sensor, on); > + else > + return ov5640_set_stream_dvp(sensor, on); > +} > + > static int ov5640_get_sysclk(struct ov5640_dev *sensor) > { > /* calculate sysclk */ > @@ -2155,37 +2162,6 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on) > > /* --------------- Subdev Operations --------------- */ > > -static int ov5640_s_power(struct v4l2_subdev *sd, int on) > -{ > - struct ov5640_dev *sensor = to_ov5640_dev(sd); > - int ret = 0; > - > - mutex_lock(&sensor->lock); > - > - /* > - * If the power count is modified from 0 to != 0 or from != 0 to 0, > - * update the power state. > - */ > - if (sensor->power_count == !on) { > - ret = ov5640_set_power(sensor, !!on); > - if (ret) > - goto out; > - } > - > - /* Update the power count. */ > - sensor->power_count += on ? 1 : -1; > - WARN_ON(sensor->power_count < 0); > -out: > - mutex_unlock(&sensor->lock); > - > - if (on && !ret && sensor->power_count == 1) { > - /* restore controls */ > - ret = v4l2_ctrl_handler_setup(&sensor->ctrls.handler); > - } > - > - return ret; > -} > - > static int ov5640_try_frame_interval(struct ov5640_dev *sensor, > struct v4l2_fract *fi, > u32 width, u32 height) > @@ -2681,6 +2657,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > { > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > struct ov5640_dev *sensor = to_ov5640_dev(sd); > + struct device *dev = &sensor->i2c_client->dev; > int ret; > > /* v4l2_ctrl_lock() locks our own mutex */ > @@ -2690,7 +2667,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > * not apply any controls to H/W at this time. Instead > * the controls will be restored right after power-up. > */ > - if (sensor->power_count == 0) > + if (pm_runtime_suspended(dev)) The problem with this is that it does not prevent powering the device off while you're here. Please use pm_runtime_get_if_active() instead (see other drivers for examples). > return 0; > > switch (ctrl->id) { > @@ -2939,39 +2916,57 @@ static int ov5640_enum_mbus_code(struct v4l2_subdev *sd, > static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) > { > struct ov5640_dev *sensor = to_ov5640_dev(sd); > + struct device *dev = &sensor->i2c_client->dev; > int ret = 0; > > mutex_lock(&sensor->lock); > > - if (sensor->streaming == !enable) { > - if (enable && sensor->pending_mode_change) { > + if (sensor->streaming == enable) { > + mutex_unlock(&sensor->lock); > + return 0; > + } > + > + if (enable) { > + ret = pm_runtime_resume_and_get(dev); > + if (ret < 0) > + goto err; > + > + if (sensor->pending_mode_change) { > ret = ov5640_set_mode(sensor); > if (ret) > - goto out; > + goto put_pm; > } > > - if (enable && sensor->pending_fmt_change) { > + if (sensor->pending_fmt_change) { > ret = ov5640_set_framefmt(sensor, &sensor->fmt); > if (ret) > - goto out; > + goto put_pm; > sensor->pending_fmt_change = false; > } > > - if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) > - ret = ov5640_set_stream_mipi(sensor, enable); > - else > - ret = ov5640_set_stream_dvp(sensor, enable); > + ret = ov5640_set_stream(sensor, true); > + if (ret) > + goto put_pm; > + } else { > + ret = ov5640_set_stream(sensor, false); > + if (ret) > + goto err; > > - if (!ret) > - sensor->streaming = enable; > + pm_runtime_put(dev); > } > -out: > + > + sensor->streaming = enable; > + mutex_unlock(&sensor->lock); > + return 0; > + > +put_pm: > + pm_runtime_put(dev); > +err: > mutex_unlock(&sensor->lock); > return ret; > } > > static const struct v4l2_subdev_core_ops ov5640_core_ops = { > - .s_power = ov5640_s_power, Nice! > .log_status = v4l2_ctrl_subdev_log_status, > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > @@ -3037,6 +3032,29 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor) > return ret; > } > > +static int ov5640_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > + struct ov5640_dev *sensor = to_ov5640_dev(subdev); > + > + return ov5640_set_power(sensor, false); > +} > + > +static int ov5640_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > + struct ov5640_dev *sensor = to_ov5640_dev(subdev); > + int ret = 0; > + > + ret = ov5640_set_power(sensor, true); > + if (ret) > + return ret; > + > + return __v4l2_ctrl_handler_setup(&sensor->ctrls.handler); This should be done by the ov5640_set_power() function --- there's no guarantee the sensor will be powered off after probe before someone tries to use it. In fact, it would be nicer to split ov5640_set_power() in two. There's little need for the convoluted calling of power management functions in this driver. (Almost all sensor drivers have one to power the sensor off and another to power it on.) > +} > + > static int ov5640_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > @@ -3162,13 +3180,17 @@ static int ov5640_probe(struct i2c_client *client) > if (ret) > goto entity_cleanup; > > + pm_runtime_enable(dev); > + pm_runtime_set_suspended(dev); You could also do this after registering the subdev below --- less error handling that way. See e.g. the imx355 driver for an example in what to do at the end of probe. The idea is runtime PM is used to turn the sensor off if it's enabled while the driver turns it on independently of runtime PM. > + > ret = v4l2_async_register_subdev_sensor(&sensor->sd); > if (ret) > - goto free_ctrls; > + goto pm_disable; > > return 0; > > -free_ctrls: > +pm_disable: > + pm_runtime_disable(dev); > v4l2_ctrl_handler_free(&sensor->ctrls.handler); > entity_cleanup: > media_entity_cleanup(&sensor->sd.entity); > @@ -3178,17 +3200,23 @@ static int ov5640_probe(struct i2c_client *client) > > static int ov5640_remove(struct i2c_client *client) > { > + struct device *dev = &client->dev; > struct v4l2_subdev *sd = i2c_get_clientdata(client); > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > v4l2_async_unregister_subdev(&sensor->sd); > media_entity_cleanup(&sensor->sd.entity); > + pm_runtime_disable(dev); > v4l2_ctrl_handler_free(&sensor->ctrls.handler); > mutex_destroy(&sensor->lock); > > return 0; > } > > +static const struct dev_pm_ops ov5640_pm_ops = { > + SET_RUNTIME_PM_OPS(ov5640_suspend, ov5640_resume, NULL) > +}; > + > static const struct i2c_device_id ov5640_id[] = { > {"ov5640", 0}, > {}, > @@ -3205,6 +3233,7 @@ static struct i2c_driver ov5640_i2c_driver = { > .driver = { > .name = "ov5640", > .of_match_table = ov5640_dt_ids, > + .pm = &ov5640_pm_ops, > }, > .id_table = ov5640_id, > .probe_new = ov5640_probe,
Hi Sakari, On Wed, Jul 07, 2021 at 11:37:18PM +0300, Sakari Ailus wrote: > On Fri, Jun 25, 2021 at 12:51:50AM +0530, Pratyush Yadav wrote: > > Calling s_power subdev callback is discouraged. Instead, the subdevs > > should use runtime PM to control its power. Use runtime PM callbacks to > > control sensor power. The pm counter is incremented when the stream is > > started and decremented when the stream is stopped. > > > > Refactor s_stream() a bit to make this new control flow easier. Add a > > helper to choose whether mipi or dvp set_stream needs to be called. The > > logic flow is also changed to make it a bit clearer. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > Changes in v3: > > - Clean up the logic in ov5640_s_stream() a bit. > > - Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(). > > - Rename the label error_pm to disable_pm. > > > > Changes in v2: > > - New in v2. > > > > drivers/media/i2c/Kconfig | 2 +- > > drivers/media/i2c/ov5640.c | 127 +++++++++++++++++++++++-------------- > > 2 files changed, 79 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index 588f8eb95984..8f43a4d7bcc1 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -929,7 +929,7 @@ config VIDEO_OV2740 > > > > config VIDEO_OV5640 > > tristate "OmniVision OV5640 sensor support" > > - depends on OF > > + depends on OF && PM > > Could you add support for runtime PM without requiring CONFIG_PM? > > Essentially you'll need to power on the device in probe and power it off in > probe, and make sure the runtime PM nop variant functions return the value > you'd expect. I've gone through that in several sensor drivers, and it really increases the complexity to get it right, to a point where I'm not comfortable asking someone to do the same (not to mention the very, very high chance that it won't be done correctly). What's the practical drawback in requiring CONFIG_PM ? > The ov5640_check_chip_id() function also calls ov5640_set_power() directly. > That needs to be changed, too. > > > depends on GPIOLIB && VIDEO_V4L2 && I2C > > select MEDIA_CONTROLLER > > select VIDEO_V4L2_SUBDEV_API > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index f6e1e51e0375..2b7fd8631ad1 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -15,6 +15,7 @@ > > #include <linux/init.h> > > #include <linux/module.h> > > #include <linux/of_device.h> > > +#include <linux/pm_runtime.h> > > #include <linux/regulator/consumer.h> > > #include <linux/slab.h> > > #include <linux/types.h> > > @@ -238,8 +239,6 @@ struct ov5640_dev { > > /* lock to protect all members below */ > > struct mutex lock; > > > > - int power_count; > > - > > struct v4l2_mbus_framefmt fmt; > > bool pending_fmt_change; > > > > @@ -1277,6 +1276,14 @@ static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > > on ? 0x00 : 0x0f); > > } > > > > +static int ov5640_set_stream(struct ov5640_dev *sensor, bool on) > > +{ > > + if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) > > + return ov5640_set_stream_mipi(sensor, on); > > + else > > + return ov5640_set_stream_dvp(sensor, on); > > +} > > + > > static int ov5640_get_sysclk(struct ov5640_dev *sensor) > > { > > /* calculate sysclk */ > > @@ -2155,37 +2162,6 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on) > > > > /* --------------- Subdev Operations --------------- */ > > > > -static int ov5640_s_power(struct v4l2_subdev *sd, int on) > > -{ > > - struct ov5640_dev *sensor = to_ov5640_dev(sd); > > - int ret = 0; > > - > > - mutex_lock(&sensor->lock); > > - > > - /* > > - * If the power count is modified from 0 to != 0 or from != 0 to 0, > > - * update the power state. > > - */ > > - if (sensor->power_count == !on) { > > - ret = ov5640_set_power(sensor, !!on); > > - if (ret) > > - goto out; > > - } > > - > > - /* Update the power count. */ > > - sensor->power_count += on ? 1 : -1; > > - WARN_ON(sensor->power_count < 0); > > -out: > > - mutex_unlock(&sensor->lock); > > - > > - if (on && !ret && sensor->power_count == 1) { > > - /* restore controls */ > > - ret = v4l2_ctrl_handler_setup(&sensor->ctrls.handler); > > - } > > - > > - return ret; > > -} > > - > > static int ov5640_try_frame_interval(struct ov5640_dev *sensor, > > struct v4l2_fract *fi, > > u32 width, u32 height) > > @@ -2681,6 +2657,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > { > > struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > + struct device *dev = &sensor->i2c_client->dev; > > int ret; > > > > /* v4l2_ctrl_lock() locks our own mutex */ > > @@ -2690,7 +2667,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) > > * not apply any controls to H/W at this time. Instead > > * the controls will be restored right after power-up. > > */ > > - if (sensor->power_count == 0) > > + if (pm_runtime_suspended(dev)) > > The problem with this is that it does not prevent powering the device off > while you're here. Please use pm_runtime_get_if_active() instead (see other > drivers for examples). > > > return 0; > > > > switch (ctrl->id) { > > @@ -2939,39 +2916,57 @@ static int ov5640_enum_mbus_code(struct v4l2_subdev *sd, > > static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) > > { > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > + struct device *dev = &sensor->i2c_client->dev; > > int ret = 0; > > > > mutex_lock(&sensor->lock); > > > > - if (sensor->streaming == !enable) { > > - if (enable && sensor->pending_mode_change) { > > + if (sensor->streaming == enable) { > > + mutex_unlock(&sensor->lock); > > + return 0; > > + } > > + > > + if (enable) { > > + ret = pm_runtime_resume_and_get(dev); > > + if (ret < 0) > > + goto err; > > + > > + if (sensor->pending_mode_change) { > > ret = ov5640_set_mode(sensor); > > if (ret) > > - goto out; > > + goto put_pm; > > } > > > > - if (enable && sensor->pending_fmt_change) { > > + if (sensor->pending_fmt_change) { > > ret = ov5640_set_framefmt(sensor, &sensor->fmt); > > if (ret) > > - goto out; > > + goto put_pm; > > sensor->pending_fmt_change = false; > > } > > > > - if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) > > - ret = ov5640_set_stream_mipi(sensor, enable); > > - else > > - ret = ov5640_set_stream_dvp(sensor, enable); > > + ret = ov5640_set_stream(sensor, true); > > + if (ret) > > + goto put_pm; > > + } else { > > + ret = ov5640_set_stream(sensor, false); > > + if (ret) > > + goto err; > > > > - if (!ret) > > - sensor->streaming = enable; > > + pm_runtime_put(dev); > > } > > -out: > > + > > + sensor->streaming = enable; > > + mutex_unlock(&sensor->lock); > > + return 0; > > + > > +put_pm: > > + pm_runtime_put(dev); > > +err: > > mutex_unlock(&sensor->lock); > > return ret; > > } > > > > static const struct v4l2_subdev_core_ops ov5640_core_ops = { > > - .s_power = ov5640_s_power, > > Nice! > > > .log_status = v4l2_ctrl_subdev_log_status, > > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > @@ -3037,6 +3032,29 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor) > > return ret; > > } > > > > +static int ov5640_suspend(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > > + struct ov5640_dev *sensor = to_ov5640_dev(subdev); > > + > > + return ov5640_set_power(sensor, false); > > +} > > + > > +static int ov5640_resume(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > > + struct ov5640_dev *sensor = to_ov5640_dev(subdev); > > + int ret = 0; > > + > > + ret = ov5640_set_power(sensor, true); > > + if (ret) > > + return ret; > > + > > + return __v4l2_ctrl_handler_setup(&sensor->ctrls.handler); > > This should be done by the ov5640_set_power() function --- there's no > guarantee the sensor will be powered off after probe before someone tries > to use it. > > In fact, it would be nicer to split ov5640_set_power() in two. There's > little need for the convoluted calling of power management functions in > this driver. (Almost all sensor drivers have one to power the sensor off > and another to power it on.) > > > +} > > + > > static int ov5640_probe(struct i2c_client *client) > > { > > struct device *dev = &client->dev; > > @@ -3162,13 +3180,17 @@ static int ov5640_probe(struct i2c_client *client) > > if (ret) > > goto entity_cleanup; > > > > + pm_runtime_enable(dev); > > + pm_runtime_set_suspended(dev); > > You could also do this after registering the subdev below --- less error > handling that way. > > See e.g. the imx355 driver for an example in what to do at the end of > probe. The idea is runtime PM is used to turn the sensor off if it's > enabled while the driver turns it on independently of runtime PM. > > > + > > ret = v4l2_async_register_subdev_sensor(&sensor->sd); > > if (ret) > > - goto free_ctrls; > > + goto pm_disable; > > > > return 0; > > > > -free_ctrls: > > +pm_disable: > > + pm_runtime_disable(dev); > > v4l2_ctrl_handler_free(&sensor->ctrls.handler); > > entity_cleanup: > > media_entity_cleanup(&sensor->sd.entity); > > @@ -3178,17 +3200,23 @@ static int ov5640_probe(struct i2c_client *client) > > > > static int ov5640_remove(struct i2c_client *client) > > { > > + struct device *dev = &client->dev; > > struct v4l2_subdev *sd = i2c_get_clientdata(client); > > struct ov5640_dev *sensor = to_ov5640_dev(sd); > > > > v4l2_async_unregister_subdev(&sensor->sd); > > media_entity_cleanup(&sensor->sd.entity); > > + pm_runtime_disable(dev); > > v4l2_ctrl_handler_free(&sensor->ctrls.handler); > > mutex_destroy(&sensor->lock); > > > > return 0; > > } > > > > +static const struct dev_pm_ops ov5640_pm_ops = { > > + SET_RUNTIME_PM_OPS(ov5640_suspend, ov5640_resume, NULL) > > +}; > > + > > static const struct i2c_device_id ov5640_id[] = { > > {"ov5640", 0}, > > {}, > > @@ -3205,6 +3233,7 @@ static struct i2c_driver ov5640_i2c_driver = { > > .driver = { > > .name = "ov5640", > > .of_match_table = ov5640_dt_ids, > > + .pm = &ov5640_pm_ops, > > }, > > .id_table = ov5640_id, > > .probe_new = ov5640_probe,
Hi Laurent, On Thu, Jul 08, 2021 at 12:00:57AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Jul 07, 2021 at 11:37:18PM +0300, Sakari Ailus wrote: > > On Fri, Jun 25, 2021 at 12:51:50AM +0530, Pratyush Yadav wrote: > > > Calling s_power subdev callback is discouraged. Instead, the subdevs > > > should use runtime PM to control its power. Use runtime PM callbacks to > > > control sensor power. The pm counter is incremented when the stream is > > > started and decremented when the stream is stopped. > > > > > > Refactor s_stream() a bit to make this new control flow easier. Add a > > > helper to choose whether mipi or dvp set_stream needs to be called. The > > > logic flow is also changed to make it a bit clearer. > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > > > --- > > > > > > Changes in v3: > > > - Clean up the logic in ov5640_s_stream() a bit. > > > - Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(). > > > - Rename the label error_pm to disable_pm. > > > > > > Changes in v2: > > > - New in v2. > > > > > > drivers/media/i2c/Kconfig | 2 +- > > > drivers/media/i2c/ov5640.c | 127 +++++++++++++++++++++++-------------- > > > 2 files changed, 79 insertions(+), 50 deletions(-) > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > > index 588f8eb95984..8f43a4d7bcc1 100644 > > > --- a/drivers/media/i2c/Kconfig > > > +++ b/drivers/media/i2c/Kconfig > > > @@ -929,7 +929,7 @@ config VIDEO_OV2740 > > > > > > config VIDEO_OV5640 > > > tristate "OmniVision OV5640 sensor support" > > > - depends on OF > > > + depends on OF && PM > > > > Could you add support for runtime PM without requiring CONFIG_PM? > > > > Essentially you'll need to power on the device in probe and power it off in > > probe, and make sure the runtime PM nop variant functions return the value > > you'd expect. > > I've gone through that in several sensor drivers, and it really > increases the complexity to get it right, to a point where I'm not > comfortable asking someone to do the same (not to mention the very, very I don't think it's very complicated, really. Looking at examples of other drivers (e.g. imx334) doing exactly the same helps as you don't need to check for individual functions. The complexity of the power management in this driver is mostly because of evolutionary development done over time, it's an old driver. > high chance that it won't be done correctly). What's the practical > drawback in requiring CONFIG_PM ? Good question. CONFIG_PM is something you can disable (for a reason I can't think of though). Why should a driver depend on it when it could perfectly work without it as well? Although this might not amount to a practical drawback. :-)
Hi Sakari, On Thu, Jul 08, 2021 at 12:44:15AM +0300, Sakari Ailus wrote: > On Thu, Jul 08, 2021 at 12:00:57AM +0300, Laurent Pinchart wrote: > > On Wed, Jul 07, 2021 at 11:37:18PM +0300, Sakari Ailus wrote: > > > On Fri, Jun 25, 2021 at 12:51:50AM +0530, Pratyush Yadav wrote: > > > > Calling s_power subdev callback is discouraged. Instead, the subdevs > > > > should use runtime PM to control its power. Use runtime PM callbacks to > > > > control sensor power. The pm counter is incremented when the stream is > > > > started and decremented when the stream is stopped. > > > > > > > > Refactor s_stream() a bit to make this new control flow easier. Add a > > > > helper to choose whether mipi or dvp set_stream needs to be called. The > > > > logic flow is also changed to make it a bit clearer. > > > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > > > > > --- > > > > > > > > Changes in v3: > > > > - Clean up the logic in ov5640_s_stream() a bit. > > > > - Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(). > > > > - Rename the label error_pm to disable_pm. > > > > > > > > Changes in v2: > > > > - New in v2. > > > > > > > > drivers/media/i2c/Kconfig | 2 +- > > > > drivers/media/i2c/ov5640.c | 127 +++++++++++++++++++++++-------------- > > > > 2 files changed, 79 insertions(+), 50 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > > > index 588f8eb95984..8f43a4d7bcc1 100644 > > > > --- a/drivers/media/i2c/Kconfig > > > > +++ b/drivers/media/i2c/Kconfig > > > > @@ -929,7 +929,7 @@ config VIDEO_OV2740 > > > > > > > > config VIDEO_OV5640 > > > > tristate "OmniVision OV5640 sensor support" > > > > - depends on OF > > > > + depends on OF && PM > > > > > > Could you add support for runtime PM without requiring CONFIG_PM? > > > > > > Essentially you'll need to power on the device in probe and power it off in > > > probe, and make sure the runtime PM nop variant functions return the value > > > you'd expect. > > > > I've gone through that in several sensor drivers, and it really > > increases the complexity to get it right, to a point where I'm not > > comfortable asking someone to do the same (not to mention the very, very > > I don't think it's very complicated, really. Looking at examples of other > drivers (e.g. imx334) doing exactly the same helps as you don't need to > check for individual functions. > > The complexity of the power management in this driver is mostly because of > evolutionary development done over time, it's an old driver. https://git.linuxtv.org/pinchartl/media.git/commit/?h=sensors/ar0330/driver&id=e72ca23c4c6b1ab6b06ac48280726e09d63cc818 Look at the changes to ar0330_probe(). As far as I understand, anything less than that would be incorrect, and it's way too easy to get it wrong. > > high chance that it won't be done correctly). What's the practical > > drawback in requiring CONFIG_PM ? > > Good question. CONFIG_PM is something you can disable (for a reason I can't > think of though). Why should a driver depend on it when it could perfectly > work without it as well? Because it requires additional complexity in the driver, times the number of sensor drivers we have in the kernel. Not even mentioning test coverage, I'm pretty sure very few people would test the sensor drivers without CONFIG_PM. > Although this might not amount to a practical drawback. :-)
Hi Laurent, On Thu, Jul 08, 2021 at 12:51:16AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Jul 08, 2021 at 12:44:15AM +0300, Sakari Ailus wrote: > > On Thu, Jul 08, 2021 at 12:00:57AM +0300, Laurent Pinchart wrote: > > > On Wed, Jul 07, 2021 at 11:37:18PM +0300, Sakari Ailus wrote: > > > > On Fri, Jun 25, 2021 at 12:51:50AM +0530, Pratyush Yadav wrote: > > > > > Calling s_power subdev callback is discouraged. Instead, the subdevs > > > > > should use runtime PM to control its power. Use runtime PM callbacks to > > > > > control sensor power. The pm counter is incremented when the stream is > > > > > started and decremented when the stream is stopped. > > > > > > > > > > Refactor s_stream() a bit to make this new control flow easier. Add a > > > > > helper to choose whether mipi or dvp set_stream needs to be called. The > > > > > logic flow is also changed to make it a bit clearer. > > > > > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > > > > > > > --- > > > > > > > > > > Changes in v3: > > > > > - Clean up the logic in ov5640_s_stream() a bit. > > > > > - Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(). > > > > > - Rename the label error_pm to disable_pm. > > > > > > > > > > Changes in v2: > > > > > - New in v2. > > > > > > > > > > drivers/media/i2c/Kconfig | 2 +- > > > > > drivers/media/i2c/ov5640.c | 127 +++++++++++++++++++++++-------------- > > > > > 2 files changed, 79 insertions(+), 50 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > > > > index 588f8eb95984..8f43a4d7bcc1 100644 > > > > > --- a/drivers/media/i2c/Kconfig > > > > > +++ b/drivers/media/i2c/Kconfig > > > > > @@ -929,7 +929,7 @@ config VIDEO_OV2740 > > > > > > > > > > config VIDEO_OV5640 > > > > > tristate "OmniVision OV5640 sensor support" > > > > > - depends on OF > > > > > + depends on OF && PM > > > > > > > > Could you add support for runtime PM without requiring CONFIG_PM? > > > > > > > > Essentially you'll need to power on the device in probe and power it off in > > > > probe, and make sure the runtime PM nop variant functions return the value > > > > you'd expect. > > > > > > I've gone through that in several sensor drivers, and it really > > > increases the complexity to get it right, to a point where I'm not > > > comfortable asking someone to do the same (not to mention the very, very > > > > I don't think it's very complicated, really. Looking at examples of other > > drivers (e.g. imx334) doing exactly the same helps as you don't need to > > check for individual functions. > > > > The complexity of the power management in this driver is mostly because of > > evolutionary development done over time, it's an old driver. > > https://git.linuxtv.org/pinchartl/media.git/commit/?h=sensors/ar0330/driver&id=e72ca23c4c6b1ab6b06ac48280726e09d63cc818 > > Look at the changes to ar0330_probe(). As far as I understand, anything > less than that would be incorrect, and it's way too easy to get it > wrong. The driver uses autosuspend that requires quite a few extra calls to runtime PM framework. The only other driver doing this may be the CCS driver. > > > > high chance that it won't be done correctly). What's the practical > > > drawback in requiring CONFIG_PM ? > > > > Good question. CONFIG_PM is something you can disable (for a reason I can't > > think of though). Why should a driver depend on it when it could perfectly > > work without it as well? > > Because it requires additional complexity in the driver, times the > number of sensor drivers we have in the kernel. Not even mentioning test How much of additional complexity? You need to call the function powering on the sensor in probe, and call another one to power the sensor off in remove, and make sure the use of runtime PM functions is correct. Good examples exist of this. Even if you fully relied on runtime PM and do not call the aforementioned functions directly, you need to power on the sensor in probe using runtime PM on DT based systems which is not the case on ACPI based systems. These differences need to be taken into account in that case. > coverage, I'm pretty sure very few people would test the sensor drivers > without CONFIG_PM. That is probably true.
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 588f8eb95984..8f43a4d7bcc1 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -929,7 +929,7 @@ config VIDEO_OV2740 config VIDEO_OV5640 tristate "OmniVision OV5640 sensor support" - depends on OF + depends on OF && PM depends on GPIOLIB && VIDEO_V4L2 && I2C select MEDIA_CONTROLLER select VIDEO_V4L2_SUBDEV_API diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index f6e1e51e0375..2b7fd8631ad1 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -15,6 +15,7 @@ #include <linux/init.h> #include <linux/module.h> #include <linux/of_device.h> +#include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> #include <linux/slab.h> #include <linux/types.h> @@ -238,8 +239,6 @@ struct ov5640_dev { /* lock to protect all members below */ struct mutex lock; - int power_count; - struct v4l2_mbus_framefmt fmt; bool pending_fmt_change; @@ -1277,6 +1276,14 @@ static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) on ? 0x00 : 0x0f); } +static int ov5640_set_stream(struct ov5640_dev *sensor, bool on) +{ + if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) + return ov5640_set_stream_mipi(sensor, on); + else + return ov5640_set_stream_dvp(sensor, on); +} + static int ov5640_get_sysclk(struct ov5640_dev *sensor) { /* calculate sysclk */ @@ -2155,37 +2162,6 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on) /* --------------- Subdev Operations --------------- */ -static int ov5640_s_power(struct v4l2_subdev *sd, int on) -{ - struct ov5640_dev *sensor = to_ov5640_dev(sd); - int ret = 0; - - mutex_lock(&sensor->lock); - - /* - * If the power count is modified from 0 to != 0 or from != 0 to 0, - * update the power state. - */ - if (sensor->power_count == !on) { - ret = ov5640_set_power(sensor, !!on); - if (ret) - goto out; - } - - /* Update the power count. */ - sensor->power_count += on ? 1 : -1; - WARN_ON(sensor->power_count < 0); -out: - mutex_unlock(&sensor->lock); - - if (on && !ret && sensor->power_count == 1) { - /* restore controls */ - ret = v4l2_ctrl_handler_setup(&sensor->ctrls.handler); - } - - return ret; -} - static int ov5640_try_frame_interval(struct ov5640_dev *sensor, struct v4l2_fract *fi, u32 width, u32 height) @@ -2681,6 +2657,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) { struct v4l2_subdev *sd = ctrl_to_sd(ctrl); struct ov5640_dev *sensor = to_ov5640_dev(sd); + struct device *dev = &sensor->i2c_client->dev; int ret; /* v4l2_ctrl_lock() locks our own mutex */ @@ -2690,7 +2667,7 @@ static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl) * not apply any controls to H/W at this time. Instead * the controls will be restored right after power-up. */ - if (sensor->power_count == 0) + if (pm_runtime_suspended(dev)) return 0; switch (ctrl->id) { @@ -2939,39 +2916,57 @@ static int ov5640_enum_mbus_code(struct v4l2_subdev *sd, static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) { struct ov5640_dev *sensor = to_ov5640_dev(sd); + struct device *dev = &sensor->i2c_client->dev; int ret = 0; mutex_lock(&sensor->lock); - if (sensor->streaming == !enable) { - if (enable && sensor->pending_mode_change) { + if (sensor->streaming == enable) { + mutex_unlock(&sensor->lock); + return 0; + } + + if (enable) { + ret = pm_runtime_resume_and_get(dev); + if (ret < 0) + goto err; + + if (sensor->pending_mode_change) { ret = ov5640_set_mode(sensor); if (ret) - goto out; + goto put_pm; } - if (enable && sensor->pending_fmt_change) { + if (sensor->pending_fmt_change) { ret = ov5640_set_framefmt(sensor, &sensor->fmt); if (ret) - goto out; + goto put_pm; sensor->pending_fmt_change = false; } - if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) - ret = ov5640_set_stream_mipi(sensor, enable); - else - ret = ov5640_set_stream_dvp(sensor, enable); + ret = ov5640_set_stream(sensor, true); + if (ret) + goto put_pm; + } else { + ret = ov5640_set_stream(sensor, false); + if (ret) + goto err; - if (!ret) - sensor->streaming = enable; + pm_runtime_put(dev); } -out: + + sensor->streaming = enable; + mutex_unlock(&sensor->lock); + return 0; + +put_pm: + pm_runtime_put(dev); +err: mutex_unlock(&sensor->lock); return ret; } static const struct v4l2_subdev_core_ops ov5640_core_ops = { - .s_power = ov5640_s_power, .log_status = v4l2_ctrl_subdev_log_status, .subscribe_event = v4l2_ctrl_subdev_subscribe_event, .unsubscribe_event = v4l2_event_subdev_unsubscribe, @@ -3037,6 +3032,29 @@ static int ov5640_check_chip_id(struct ov5640_dev *sensor) return ret; } +static int ov5640_suspend(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *subdev = i2c_get_clientdata(client); + struct ov5640_dev *sensor = to_ov5640_dev(subdev); + + return ov5640_set_power(sensor, false); +} + +static int ov5640_resume(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *subdev = i2c_get_clientdata(client); + struct ov5640_dev *sensor = to_ov5640_dev(subdev); + int ret = 0; + + ret = ov5640_set_power(sensor, true); + if (ret) + return ret; + + return __v4l2_ctrl_handler_setup(&sensor->ctrls.handler); +} + static int ov5640_probe(struct i2c_client *client) { struct device *dev = &client->dev; @@ -3162,13 +3180,17 @@ static int ov5640_probe(struct i2c_client *client) if (ret) goto entity_cleanup; + pm_runtime_enable(dev); + pm_runtime_set_suspended(dev); + ret = v4l2_async_register_subdev_sensor(&sensor->sd); if (ret) - goto free_ctrls; + goto pm_disable; return 0; -free_ctrls: +pm_disable: + pm_runtime_disable(dev); v4l2_ctrl_handler_free(&sensor->ctrls.handler); entity_cleanup: media_entity_cleanup(&sensor->sd.entity); @@ -3178,17 +3200,23 @@ static int ov5640_probe(struct i2c_client *client) static int ov5640_remove(struct i2c_client *client) { + struct device *dev = &client->dev; struct v4l2_subdev *sd = i2c_get_clientdata(client); struct ov5640_dev *sensor = to_ov5640_dev(sd); v4l2_async_unregister_subdev(&sensor->sd); media_entity_cleanup(&sensor->sd.entity); + pm_runtime_disable(dev); v4l2_ctrl_handler_free(&sensor->ctrls.handler); mutex_destroy(&sensor->lock); return 0; } +static const struct dev_pm_ops ov5640_pm_ops = { + SET_RUNTIME_PM_OPS(ov5640_suspend, ov5640_resume, NULL) +}; + static const struct i2c_device_id ov5640_id[] = { {"ov5640", 0}, {}, @@ -3205,6 +3233,7 @@ static struct i2c_driver ov5640_i2c_driver = { .driver = { .name = "ov5640", .of_match_table = ov5640_dt_ids, + .pm = &ov5640_pm_ops, }, .id_table = ov5640_id, .probe_new = ov5640_probe,
Calling s_power subdev callback is discouraged. Instead, the subdevs should use runtime PM to control its power. Use runtime PM callbacks to control sensor power. The pm counter is incremented when the stream is started and decremented when the stream is stopped. Refactor s_stream() a bit to make this new control flow easier. Add a helper to choose whether mipi or dvp set_stream needs to be called. The logic flow is also changed to make it a bit clearer. Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- Changes in v3: - Clean up the logic in ov5640_s_stream() a bit. - Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(). - Rename the label error_pm to disable_pm. Changes in v2: - New in v2. drivers/media/i2c/Kconfig | 2 +- drivers/media/i2c/ov5640.c | 127 +++++++++++++++++++++++-------------- 2 files changed, 79 insertions(+), 50 deletions(-)