[v3,15/20] media: i2c: imx219: Drop system suspend/resume operations
Commit Message
There is no need to stop streaming at system suspend time, or restart it
when the system is resumed, as the host-side driver is responsible for
stopping and restarting the pipeline in a controlled way by calling the
.s_stream() operation. Drop the system suspend and resume handlers, and
simplify the .s_stream() handler as a result.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/i2c/imx219.c | 56 ++------------------------------------
1 file changed, 2 insertions(+), 54 deletions(-)
Comments
Hi Laurent
On Wed, Sep 13, 2023 at 04:56:33PM +0300, Laurent Pinchart wrote:
> There is no need to stop streaming at system suspend time, or restart it
> when the system is resumed, as the host-side driver is responsible for
> stopping and restarting the pipeline in a controlled way by calling the
> .s_stream() operation. Drop the system suspend and resume handlers, and
> simplify the .s_stream() handler as a result.
>
I wish we could remove these from all drivers in media/i2c ..
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/i2c/imx219.c | 56 ++------------------------------------
> 1 file changed, 2 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 8c928202a23b..a12ebf288dd2 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -358,9 +358,6 @@ struct imx219 {
> struct v4l2_ctrl *vblank;
> struct v4l2_ctrl *hblank;
>
> - /* Streaming on/off */
> - bool streaming;
> -
> /* Two or Four lanes */
> u8 lanes;
> };
> @@ -763,24 +760,11 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
>
> state = v4l2_subdev_lock_and_get_active_state(sd);
>
> - if (imx219->streaming == enable)
> - goto unlock;
> -
> - if (enable) {
> - /*
> - * Apply default & customized values
> - * and then start streaming.
> - */
> + if (enable)
> ret = imx219_start_streaming(imx219, state);
> - if (ret)
> - goto unlock;
> - } else {
> + else
> imx219_stop_streaming(imx219);
> - }
>
> - imx219->streaming = enable;
> -
> -unlock:
> v4l2_subdev_unlock_state(state);
Maybe an empty line now that this is not an error path anymore
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Thanks
j
> return ret;
> }
> @@ -1014,41 +998,6 @@ static int imx219_power_off(struct device *dev)
> return 0;
> }
>
> -static int __maybe_unused imx219_suspend(struct device *dev)
> -{
> - struct v4l2_subdev *sd = dev_get_drvdata(dev);
> - struct imx219 *imx219 = to_imx219(sd);
> -
> - if (imx219->streaming)
> - imx219_stop_streaming(imx219);
> -
> - return 0;
> -}
> -
> -static int __maybe_unused imx219_resume(struct device *dev)
> -{
> - struct v4l2_subdev *sd = dev_get_drvdata(dev);
> - struct imx219 *imx219 = to_imx219(sd);
> - struct v4l2_subdev_state *state;
> - int ret;
> -
> - if (imx219->streaming) {
> - state = v4l2_subdev_lock_and_get_active_state(sd);
> - ret = imx219_start_streaming(imx219, state);
> - v4l2_subdev_unlock_state(state);
> - if (ret)
> - goto error;
> - }
> -
> - return 0;
> -
> -error:
> - imx219_stop_streaming(imx219);
> - imx219->streaming = false;
> -
> - return ret;
> -}
> -
> /* -----------------------------------------------------------------------------
> * Probe & remove
> */
> @@ -1294,7 +1243,6 @@ static const struct of_device_id imx219_dt_ids[] = {
> MODULE_DEVICE_TABLE(of, imx219_dt_ids);
>
> static const struct dev_pm_ops imx219_pm_ops = {
> - SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
> SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
> };
>
> --
> Regards,
>
> Laurent Pinchart
>
On Wed, Sep 13, 2023 at 04:44:15PM +0200, Jacopo Mondi wrote:
> Hi Laurent
>
> On Wed, Sep 13, 2023 at 04:56:33PM +0300, Laurent Pinchart wrote:
> > There is no need to stop streaming at system suspend time, or restart it
> > when the system is resumed, as the host-side driver is responsible for
> > stopping and restarting the pipeline in a controlled way by calling the
> > .s_stream() operation. Drop the system suspend and resume handlers, and
> > simplify the .s_stream() handler as a result.
>
> I wish we could remove these from all drivers in media/i2c ..
I got fed up by cargo cult, so I'm cooking a patch to do this.
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > drivers/media/i2c/imx219.c | 56 ++------------------------------------
> > 1 file changed, 2 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 8c928202a23b..a12ebf288dd2 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -358,9 +358,6 @@ struct imx219 {
> > struct v4l2_ctrl *vblank;
> > struct v4l2_ctrl *hblank;
> >
> > - /* Streaming on/off */
> > - bool streaming;
> > -
> > /* Two or Four lanes */
> > u8 lanes;
> > };
> > @@ -763,24 +760,11 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
> >
> > state = v4l2_subdev_lock_and_get_active_state(sd);
> >
> > - if (imx219->streaming == enable)
> > - goto unlock;
> > -
> > - if (enable) {
> > - /*
> > - * Apply default & customized values
> > - * and then start streaming.
> > - */
> > + if (enable)
> > ret = imx219_start_streaming(imx219, state);
> > - if (ret)
> > - goto unlock;
> > - } else {
> > + else
> > imx219_stop_streaming(imx219);
> > - }
> >
> > - imx219->streaming = enable;
> > -
> > -unlock:
> > v4l2_subdev_unlock_state(state);
>
> Maybe an empty line now that this is not an error path anymore
If that's all it takes to make you happy, sure :-)
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> > return ret;
> > }
> > @@ -1014,41 +998,6 @@ static int imx219_power_off(struct device *dev)
> > return 0;
> > }
> >
> > -static int __maybe_unused imx219_suspend(struct device *dev)
> > -{
> > - struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > - struct imx219 *imx219 = to_imx219(sd);
> > -
> > - if (imx219->streaming)
> > - imx219_stop_streaming(imx219);
> > -
> > - return 0;
> > -}
> > -
> > -static int __maybe_unused imx219_resume(struct device *dev)
> > -{
> > - struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > - struct imx219 *imx219 = to_imx219(sd);
> > - struct v4l2_subdev_state *state;
> > - int ret;
> > -
> > - if (imx219->streaming) {
> > - state = v4l2_subdev_lock_and_get_active_state(sd);
> > - ret = imx219_start_streaming(imx219, state);
> > - v4l2_subdev_unlock_state(state);
> > - if (ret)
> > - goto error;
> > - }
> > -
> > - return 0;
> > -
> > -error:
> > - imx219_stop_streaming(imx219);
> > - imx219->streaming = false;
> > -
> > - return ret;
> > -}
> > -
> > /* -----------------------------------------------------------------------------
> > * Probe & remove
> > */
> > @@ -1294,7 +1243,6 @@ static const struct of_device_id imx219_dt_ids[] = {
> > MODULE_DEVICE_TABLE(of, imx219_dt_ids);
> >
> > static const struct dev_pm_ops imx219_pm_ops = {
> > - SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
> > SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
> > };
> >
@@ -358,9 +358,6 @@ struct imx219 {
struct v4l2_ctrl *vblank;
struct v4l2_ctrl *hblank;
- /* Streaming on/off */
- bool streaming;
-
/* Two or Four lanes */
u8 lanes;
};
@@ -763,24 +760,11 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
state = v4l2_subdev_lock_and_get_active_state(sd);
- if (imx219->streaming == enable)
- goto unlock;
-
- if (enable) {
- /*
- * Apply default & customized values
- * and then start streaming.
- */
+ if (enable)
ret = imx219_start_streaming(imx219, state);
- if (ret)
- goto unlock;
- } else {
+ else
imx219_stop_streaming(imx219);
- }
- imx219->streaming = enable;
-
-unlock:
v4l2_subdev_unlock_state(state);
return ret;
}
@@ -1014,41 +998,6 @@ static int imx219_power_off(struct device *dev)
return 0;
}
-static int __maybe_unused imx219_suspend(struct device *dev)
-{
- struct v4l2_subdev *sd = dev_get_drvdata(dev);
- struct imx219 *imx219 = to_imx219(sd);
-
- if (imx219->streaming)
- imx219_stop_streaming(imx219);
-
- return 0;
-}
-
-static int __maybe_unused imx219_resume(struct device *dev)
-{
- struct v4l2_subdev *sd = dev_get_drvdata(dev);
- struct imx219 *imx219 = to_imx219(sd);
- struct v4l2_subdev_state *state;
- int ret;
-
- if (imx219->streaming) {
- state = v4l2_subdev_lock_and_get_active_state(sd);
- ret = imx219_start_streaming(imx219, state);
- v4l2_subdev_unlock_state(state);
- if (ret)
- goto error;
- }
-
- return 0;
-
-error:
- imx219_stop_streaming(imx219);
- imx219->streaming = false;
-
- return ret;
-}
-
/* -----------------------------------------------------------------------------
* Probe & remove
*/
@@ -1294,7 +1243,6 @@ static const struct of_device_id imx219_dt_ids[] = {
MODULE_DEVICE_TABLE(of, imx219_dt_ids);
static const struct dev_pm_ops imx219_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(imx219_suspend, imx219_resume)
SET_RUNTIME_PM_OPS(imx219_power_off, imx219_power_on, NULL)
};