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@xxxxxxxxxxxxxxx> wrote on Fri [2016-Feb-19 14:54:23 -0200]:
> Em Wed, 6 Jan 2016 17:37:26 -0600
> Benoit Parrot <bparrot@xxxxxx> 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@xxxxxx>
> > Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> > ---
>
> ...
>
> > +/* 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@xxxxxxxxxxxxxxx>
>
> 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;
> }
>
> /*
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
