[06/31] staging: bcm2835-camera: Return early on errors

Commit Message

Stefan Wahren June 27, 2019, 6:56 p.m. UTC

From: Dave Stevenson <[e-mail]>

Fix several instances where it is easier to return
early on error conditions than handle it as an else
clause. As requested by Mauro.

Signed-off-by: Dave Stevenson <[e-mail]>
---
 .../vc04_services/bcm2835-camera/bcm2835-camera.c  | 109 +++++++++++----------
 1 file changed, 58 insertions(+), 51 deletions(-)

--
2.7.4

Comments

Dan Carpenter June 28, 2019, 7:38 a.m. UTC | #1

On Thu, Jun 27, 2019 at 08:56:03PM +0200, Stefan Wahren wrote:
>  	v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev, "connecting %p to %p\n",
>  		 src, dst);
>  	ret = vchiq_mmal_port_connect_tunnel(dev->instance, src, dst);
>  	if (!ret)
>  		ret = vchiq_mmal_port_enable(dev->instance, src, NULL);
> -error:
> +
>  	return ret;

In future patches, you probably want to flip this one around as well.
Try to do error handling instead of success handling.  In other words,
keep the success patch indented one tab and the failure path indented
two tabs.  Don't make the last failure check in the function special.

	ret = vchiq_mmal_port_connect_tunnel(dev->instance, src, dst);
	if (ret)
		return ret;

	ret = vchiq_mmal_port_enable(dev->instance, src, NULL);
	if (ret)
		return ret;
	return 0;

Or you can make the last check a little special if you want...

	return vchiq_mmal_port_enable(dev->instance, src, NULL);

Either format is good.

regards,
dan carpenter

Patch

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index 456b686..b597475 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -334,7 +334,9 @@  static void buffer_cb(struct vchiq_mmal_instance *instance,
 			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
 		}
 		return;
-	} else if (length == 0) {
+	}
+
+	if (length == 0) {
 		/* stream ended */
 		if (buf) {
 			/* this should only ever happen if the port is
@@ -357,56 +359,59 @@  static void buffer_cb(struct vchiq_mmal_instance *instance,
 			/* signal frame completion */
 			complete(&dev->capture.frame_cmplt);
 		}
-	} else {
-		if (dev->capture.frame_count) {
-			if (dev->capture.vc_start_timestamp != -1) {
-				buf->vb.vb2_buf.timestamp = ktime_get_ns();
-			} else if (pts) {
-				ktime_t timestamp;
-				s64 runtime_us = pts -
-				    dev->capture.vc_start_timestamp;
-				timestamp = ktime_add_us(dev->capture.kernel_start_ts,
-							 runtime_us);
-				v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev,
-					 "Convert start time %llu and %llu with offset %llu to %llu\n",
-					 ktime_to_ns(dev->capture.kernel_start_ts),
-					 dev->capture.vc_start_timestamp, pts,
-					 ktime_to_ns(timestamp));
-				buf->vb.vb2_buf.timestamp = ktime_to_ns(timestamp);
-			} else {
-				if (dev->capture.last_timestamp) {
-					buf->vb.vb2_buf.timestamp =
-						dev->capture.last_timestamp;
-				} else {
-					buf->vb.vb2_buf.timestamp =
-						ktime_to_ns(dev->capture.kernel_start_ts);
-				}
-			}
-			dev->capture.last_timestamp = buf->vb.vb2_buf.timestamp;
+		return;
+	}

-			vb2_set_plane_payload(&buf->vb.vb2_buf, 0, length);
-			if (mmal_flags & MMAL_BUFFER_HEADER_FLAG_KEYFRAME)
-				buf->vb.flags |= V4L2_BUF_FLAG_KEYFRAME;
+	if (!dev->capture.frame_count) {
+		/* signal frame completion */
+		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
+		complete(&dev->capture.frame_cmplt);
+		return;
+	}

-			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);

-			if (mmal_flags & MMAL_BUFFER_HEADER_FLAG_EOS &&
-			    is_capturing(dev)) {
-				v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev,
-					 "Grab another frame as buffer has EOS");
-				vchiq_mmal_port_parameter_set(
-					instance,
-					dev->capture.camera_port,
-					MMAL_PARAMETER_CAPTURE,
-					&dev->capture.frame_count,
-					sizeof(dev->capture.frame_count));
-			}
+	if (dev->capture.vc_start_timestamp != -1) {
+		buf->vb.vb2_buf.timestamp = ktime_get_ns();
+	} else if (pts) {
+		ktime_t timestamp;
+		s64 runtime_us = pts -
+		    dev->capture.vc_start_timestamp;
+		timestamp = ktime_add_us(dev->capture.kernel_start_ts,
+					 runtime_us);
+		v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev,
+			 "Convert start time %llu and %llu with offset %llu to %llu\n",
+			 ktime_to_ns(dev->capture.kernel_start_ts),
+			 dev->capture.vc_start_timestamp, pts,
+			 ktime_to_ns(timestamp));
+		buf->vb.vb2_buf.timestamp = ktime_to_ns(timestamp);
+	} else {
+		if (dev->capture.last_timestamp) {
+			buf->vb.vb2_buf.timestamp =
+				dev->capture.last_timestamp;
 		} else {
-			/* signal frame completion */
-			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
-			complete(&dev->capture.frame_cmplt);
+			buf->vb.vb2_buf.timestamp =
+				ktime_to_ns(dev->capture.kernel_start_ts);
 		}
 	}
+	dev->capture.last_timestamp = buf->vb.vb2_buf.timestamp;
+
+	vb2_set_plane_payload(&buf->vb.vb2_buf, 0, length);
+	if (mmal_flags & MMAL_BUFFER_HEADER_FLAG_KEYFRAME)
+		buf->vb.flags |= V4L2_BUF_FLAG_KEYFRAME;
+
+	vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+
+	if (mmal_flags & MMAL_BUFFER_HEADER_FLAG_EOS &&
+	    is_capturing(dev)) {
+		v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev,
+			 "Grab another frame as buffer has EOS");
+		vchiq_mmal_port_parameter_set(
+			instance,
+			dev->capture.camera_port,
+			MMAL_PARAMETER_CAPTURE,
+			&dev->capture.frame_count,
+			sizeof(dev->capture.frame_count));
+	}
 }

 static int enable_camera(struct bm2835_mmal_dev *dev)
@@ -785,27 +790,29 @@  static int vidioc_overlay(struct file *file, void *f, unsigned int on)

 	ret = vchiq_mmal_port_set_format(dev->instance, src);
 	if (ret < 0)
-		goto error;
+		return ret;

 	ret = set_overlay_params(dev, dst);
 	if (ret < 0)
-		goto error;
+		return ret;

-	if (enable_camera(dev) < 0)
-		goto error;
+	if (enable_camera(dev) < 0) {
+		ret = -EINVAL;
+		return ret;
+	}

 	ret = vchiq_mmal_component_enable(
 			dev->instance,
 			dev->component[MMAL_COMPONENT_PREVIEW]);
 	if (ret < 0)
-		goto error;
+		return ret;

 	v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev, "connecting %p to %p\n",
 		 src, dst);
 	ret = vchiq_mmal_port_connect_tunnel(dev->instance, src, dst);
 	if (!ret)
 		ret = vchiq_mmal_port_enable(dev->instance, src, NULL);
-error:
+
 	return ret;
 }