| Message ID | 20230913135638.26277-16-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: imx219: Miscellaneous cleanups and improvements | expand |
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) > > }; > >
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); 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) };
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(-)