| Message ID | 20220312152505.145453-1-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] media: imx: imx-mipi-csis: Simplify mipi_csis_s_stream() | expand |
Hi Jacopo, Thank you for the patch. On Sat, Mar 12, 2022 at 04:25:04PM +0100, Jacopo Mondi wrote: > Simplify the mipi_csis_s_stream() function. > > This actually fixes a bug, as if calling the subdev's s_stream(1) fails, > mipi_csis_stop_stream() was not called. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/platform/imx/imx-mipi-csis.c | 58 ++++++++++++---------- > 1 file changed, 33 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c > index c4d1a6fe5007..fa00b36fc394 100644 > --- a/drivers/media/platform/imx/imx-mipi-csis.c > +++ b/drivers/media/platform/imx/imx-mipi-csis.c > @@ -910,43 +910,51 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev) > static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable) > { > struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd); > - int ret = 0; > + int ret; > > - if (enable) { > - ret = mipi_csis_calculate_params(csis); > - if (ret < 0) > - return ret; > + if (!enable) { > + mutex_lock(&csis->lock); > > - mipi_csis_clear_counters(csis); > + v4l2_subdev_call(csis->src_sd, video, s_stream, 0); > > - ret = pm_runtime_resume_and_get(csis->dev); > - if (ret < 0) > - return ret; > + mipi_csis_stop_stream(csis); > + if (csis->debug.enable) > + mipi_csis_log_counters(csis, true); > + > + pm_runtime_put(csis->dev); > + > + mutex_unlock(&csis->lock); You can move the mutex_unlock() call before pm_runtime_put(). > + > + return 0; > } > > - mutex_lock(&csis->lock); > + ret = mipi_csis_calculate_params(csis); > + if (ret < 0) > + return ret; > > - if (enable) { > - mipi_csis_start_stream(csis); > - ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1); > - if (ret < 0) > - goto unlock; > + mipi_csis_clear_counters(csis); > > - mipi_csis_log_counters(csis, true); > - } else { > - v4l2_subdev_call(csis->src_sd, video, s_stream, 0); > + ret = pm_runtime_resume_and_get(csis->dev); > + if (ret < 0) > + return ret; > > - mipi_csis_stop_stream(csis); > + mutex_lock(&csis->lock); > > - if (csis->debug.enable) > - mipi_csis_log_counters(csis, true); > - } > + mipi_csis_start_stream(csis); > + ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1); > + if (ret < 0) > + goto out; > + > + mipi_csis_log_counters(csis, true); > > -unlock: > mutex_unlock(&csis->lock); > > - if (!enable || ret < 0) > - pm_runtime_put(csis->dev); > + return 0; > + > +out: > + mipi_csis_stop_stream(csis); > + pm_runtime_put(csis->dev); > + mutex_unlock(&csis->lock); Here too. When there's a single error path I'm tempted to just inline error handling instead of using a goto, but I don't mind either way. > > return ret; > }
diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c index c4d1a6fe5007..fa00b36fc394 100644 --- a/drivers/media/platform/imx/imx-mipi-csis.c +++ b/drivers/media/platform/imx/imx-mipi-csis.c @@ -910,43 +910,51 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev) static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable) { struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd); - int ret = 0; + int ret; - if (enable) { - ret = mipi_csis_calculate_params(csis); - if (ret < 0) - return ret; + if (!enable) { + mutex_lock(&csis->lock); - mipi_csis_clear_counters(csis); + v4l2_subdev_call(csis->src_sd, video, s_stream, 0); - ret = pm_runtime_resume_and_get(csis->dev); - if (ret < 0) - return ret; + mipi_csis_stop_stream(csis); + if (csis->debug.enable) + mipi_csis_log_counters(csis, true); + + pm_runtime_put(csis->dev); + + mutex_unlock(&csis->lock); + + return 0; } - mutex_lock(&csis->lock); + ret = mipi_csis_calculate_params(csis); + if (ret < 0) + return ret; - if (enable) { - mipi_csis_start_stream(csis); - ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1); - if (ret < 0) - goto unlock; + mipi_csis_clear_counters(csis); - mipi_csis_log_counters(csis, true); - } else { - v4l2_subdev_call(csis->src_sd, video, s_stream, 0); + ret = pm_runtime_resume_and_get(csis->dev); + if (ret < 0) + return ret; - mipi_csis_stop_stream(csis); + mutex_lock(&csis->lock); - if (csis->debug.enable) - mipi_csis_log_counters(csis, true); - } + mipi_csis_start_stream(csis); + ret = v4l2_subdev_call(csis->src_sd, video, s_stream, 1); + if (ret < 0) + goto out; + + mipi_csis_log_counters(csis, true); -unlock: mutex_unlock(&csis->lock); - if (!enable || ret < 0) - pm_runtime_put(csis->dev); + return 0; + +out: + mipi_csis_stop_stream(csis); + pm_runtime_put(csis->dev); + mutex_unlock(&csis->lock); return ret; }
Simplify the mipi_csis_s_stream() function. This actually fixes a bug, as if calling the subdev's s_stream(1) fails, mipi_csis_stop_stream() was not called. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/platform/imx/imx-mipi-csis.c | 58 ++++++++++++---------- 1 file changed, 33 insertions(+), 25 deletions(-) -- 2.35.1