Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1338355 > unrolled thread

Re: [Patch v6 3/3] media: ti-vpe: Add CAL v4l2 camera capture driver

Started byMauro Carvalho Chehab <mchehab@osg.samsung.com>
First post2016-02-19 18:00 +0100
Last post2016-02-19 20:30 +0100
Articles 2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Patch v6 3/3] media: ti-vpe: Add CAL v4l2 camera capture  driver Mauro Carvalho Chehab <mchehab@osg.samsung.com> - 2016-02-19 18:00 +0100
    Re: [Patch v6 3/3] media: ti-vpe: Add CAL v4l2 camera capture driver Benoit Parrot <bparrot@ti.com> - 2016-02-19 20:30 +0100

#1338355 — Re: [Patch v6 3/3] media: ti-vpe: Add CAL v4l2 camera capture driver

FromMauro Carvalho Chehab <mchehab@osg.samsung.com>
Date2016-02-19 18:00 +0100
SubjectRe: [Patch v6 3/3] media: ti-vpe: Add CAL v4l2 camera capture driver
Message-ID<r3Wzo-82f-9@gated-at.bofh.it>
Em Wed, 6 Jan 2016 17:37:26 -0600
Benoit Parrot <bparrot@ti.com> escreveu:

> The Camera Adaptation Layer (CAL) is a block which consists of a dual
> port CSI2/MIPI camera capture engine.
> Port #0 can handle CSI2 camera connected to up to 4 data lanes.
> Port #1 can handle CSI2 camera connected to up to 2 data lanes.
> The driver implements the required API/ioctls to be V4L2 compliant.
> Driver supports the following:
>     - V4L2 API using DMABUF/MMAP buffer access based on videobuf2 api
>     - Asynchronous sensor sub device registration
>     - DT support
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---

...

> +/* timeperframe is arbitrary and continuous */
> +static int cal_enum_frameintervals(struct file *file, void *priv,
> +				   struct v4l2_frmivalenum *fival)
> +{
> +	struct cal_ctx *ctx = video_drvdata(file);
> +	const struct cal_fmt *fmt;
> +	struct v4l2_subdev_frame_size_enum fse;
> +	int ret;
> +
> +	if (fival->index)
> +		return -EINVAL;
> +
> +	fmt = find_format_by_pix(ctx, fival->pixel_format);
> +	if (!fmt)
> +		return -EINVAL;
> +
> +	/* check for valid width/height */
> +	ret = 0;
> +	fse.pad = 0;
> +	fse.code = fmt->code;
> +	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	for (fse.index = 0; ; fse.index++) {
> +		ret = v4l2_subdev_call(ctx->sensor, pad, enum_frame_size,
> +				       NULL, &fse);
> +		if (ret)
> +			return -EINVAL;
> +
> +		if ((fival->width == fse.max_width) &&
> +		    (fival->height == fse.max_height))
> +			break;
> +		else if ((fival->width >= fse.min_width) &&
> +			 (fival->width <= fse.max_width) &&
> +			 (fival->height >= fse.min_height) &&
> +			 (fival->height <= fse.max_height))
> +			break;
> +
> +		return -EINVAL;
> +	}
> +
> +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> +	fival->discrete.numerator = 1;
> +	fival->discrete.denominator = 30;
> +
> +	return 0;
> +}

The above routine is too complex and sounds wrong. Why do you
need a loop there, if the loop will either return -EINVAL or
be aborted the first time it runs?

The way it is, it is just confusing and produces a smatch error:

	drivers/media/platform/ti-vpe/cal.c:1219 cal_enum_frameintervals() info: ignoring unreachable code.

If all you want here is to run the loop once, this patch would do the
same, with a clearer logic.

ti-vpe/cal: Simplify the logic to avoid confusing smatch

drivers/media/platform/ti-vpe/cal.c:1219 cal_enum_frameintervals() info: ignoring unreachable code.

This is caused by a very confusing logic that looks like a loop, but
it runs only once.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 35fa1071c5b2..62721ee7b9bc 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1216,29 +1216,28 @@ static int cal_enum_frameintervals(struct file *file, void *priv,
 	fse.pad = 0;
 	fse.code = fmt->code;
 	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	for (fse.index = 0; ; fse.index++) {
-		ret = v4l2_subdev_call(ctx->sensor, pad, enum_frame_size,
-				       NULL, &fse);
-		if (ret)
-			return -EINVAL;
-
-		if ((fival->width == fse.max_width) &&
-		    (fival->height == fse.max_height))
-			break;
-		else if ((fival->width >= fse.min_width) &&
-			 (fival->width <= fse.max_width) &&
-			 (fival->height >= fse.min_height) &&
-			 (fival->height <= fse.max_height))
-			break;
+	fse.index = 0;
 
+	ret = v4l2_subdev_call(ctx->sensor, pad, enum_frame_size,
+			       NULL, &fse);
+	if (ret)
 		return -EINVAL;
-	}
 
 	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
 	fival->discrete.numerator = 1;
 	fival->discrete.denominator = 30;
 
-	return 0;
+	if ((fival->width == fse.max_width) &&
+	    (fival->height == fse.max_height))
+		return 0;
+
+	if ((fival->width >= fse.min_width) &&
+	    (fival->width <= fse.max_width) &&
+	    (fival->height >= fse.min_height) &&
+	    (fival->height <= fse.max_height))
+		return 0;
+
+	return -EINVAL;
 }
 
 /*

[toc] | [next] | [standalone]

#1338419 — Re: [Patch v6 3/3] media: ti-vpe: Add CAL v4l2 camera capture driver

FromBenoit Parrot <bparrot@ti.com>
Date2016-02-19 20:30 +0100
SubjectRe: [Patch v6 3/3] media: ti-vpe: Add CAL v4l2 camera capture driver
Message-ID<r3YUy-1Id-1@gated-at.bofh.it>
In reply to#1338355
Mauro,

Thanks for the proposed fix.
But I decided to fix it in a different way.
I'll send a patch shortly.

Regards,
Benoit

Mauro Carvalho Chehab <mchehab@osg.samsung.com> wrote on Fri [2016-Feb-19 14:54:23 -0200]:
> Em Wed, 6 Jan 2016 17:37:26 -0600
> Benoit Parrot <bparrot@ti.com> escreveu:
> 
> > The Camera Adaptation Layer (CAL) is a block which consists of a dual
> > port CSI2/MIPI camera capture engine.
> > Port #0 can handle CSI2 camera connected to up to 4 data lanes.
> > Port #1 can handle CSI2 camera connected to up to 2 data lanes.
> > The driver implements the required API/ioctls to be V4L2 compliant.
> > Driver supports the following:
> >     - V4L2 API using DMABUF/MMAP buffer access based on videobuf2 api
> >     - Asynchronous sensor sub device registration
> >     - DT support
> > 
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> 
> ...
> 
> > +/* timeperframe is arbitrary and continuous */
> > +static int cal_enum_frameintervals(struct file *file, void *priv,
> > +				   struct v4l2_frmivalenum *fival)
> > +{
> > +	struct cal_ctx *ctx = video_drvdata(file);
> > +	const struct cal_fmt *fmt;
> > +	struct v4l2_subdev_frame_size_enum fse;
> > +	int ret;
> > +
> > +	if (fival->index)
> > +		return -EINVAL;
> > +
> > +	fmt = find_format_by_pix(ctx, fival->pixel_format);
> > +	if (!fmt)
> > +		return -EINVAL;
> > +
> > +	/* check for valid width/height */
> > +	ret = 0;
> > +	fse.pad = 0;
> > +	fse.code = fmt->code;
> > +	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	for (fse.index = 0; ; fse.index++) {
> > +		ret = v4l2_subdev_call(ctx->sensor, pad, enum_frame_size,
> > +				       NULL, &fse);
> > +		if (ret)
> > +			return -EINVAL;
> > +
> > +		if ((fival->width == fse.max_width) &&
> > +		    (fival->height == fse.max_height))
> > +			break;
> > +		else if ((fival->width >= fse.min_width) &&
> > +			 (fival->width <= fse.max_width) &&
> > +			 (fival->height >= fse.min_height) &&
> > +			 (fival->height <= fse.max_height))
> > +			break;
> > +
> > +		return -EINVAL;
> > +	}
> > +
> > +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > +	fival->discrete.numerator = 1;
> > +	fival->discrete.denominator = 30;
> > +
> > +	return 0;
> > +}
> 
> The above routine is too complex and sounds wrong. Why do you
> need a loop there, if the loop will either return -EINVAL or
> be aborted the first time it runs?
> 
> The way it is, it is just confusing and produces a smatch error:
> 
> 	drivers/media/platform/ti-vpe/cal.c:1219 cal_enum_frameintervals() info: ignoring unreachable code.
> 
> If all you want here is to run the loop once, this patch would do the
> same, with a clearer logic.
> 
> ti-vpe/cal: Simplify the logic to avoid confusing smatch
> 
> drivers/media/platform/ti-vpe/cal.c:1219 cal_enum_frameintervals() info: ignoring unreachable code.
> 
> This is caused by a very confusing logic that looks like a loop, but
> it runs only once.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 35fa1071c5b2..62721ee7b9bc 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1216,29 +1216,28 @@ static int cal_enum_frameintervals(struct file *file, void *priv,
>  	fse.pad = 0;
>  	fse.code = fmt->code;
>  	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	for (fse.index = 0; ; fse.index++) {
> -		ret = v4l2_subdev_call(ctx->sensor, pad, enum_frame_size,
> -				       NULL, &fse);
> -		if (ret)
> -			return -EINVAL;
> -
> -		if ((fival->width == fse.max_width) &&
> -		    (fival->height == fse.max_height))
> -			break;
> -		else if ((fival->width >= fse.min_width) &&
> -			 (fival->width <= fse.max_width) &&
> -			 (fival->height >= fse.min_height) &&
> -			 (fival->height <= fse.max_height))
> -			break;
> +	fse.index = 0;
>  
> +	ret = v4l2_subdev_call(ctx->sensor, pad, enum_frame_size,
> +			       NULL, &fse);
> +	if (ret)
>  		return -EINVAL;
> -	}
>  
>  	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>  	fival->discrete.numerator = 1;
>  	fival->discrete.denominator = 30;
>  
> -	return 0;
> +	if ((fival->width == fse.max_width) &&
> +	    (fival->height == fse.max_height))
> +		return 0;
> +
> +	if ((fival->width >= fse.min_width) &&
> +	    (fival->width <= fse.max_width) &&
> +	    (fival->height >= fse.min_height) &&
> +	    (fival->height <= fse.max_height))
> +		return 0;
> +
> +	return -EINVAL;
>  }
>  
>  /*
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web