Hi Bastian,
Guennadi pointed out that "should" can sound a bit harsh, so please read my
reviews as if
#define "you should" "I think you should"
was prepended to all of them :-)
On Wednesday 31 August 2011 19:06:25 Laurent Pinchart wrote:
> On Wednesday 31 August 2011 17:05:52 Bastian Hecht wrote:
> > This patch adds the ability to get arbitrary resolutions with a width
> > up to 2592 and a height up to 720 pixels instead of the standard 1280x720
> > only.
> >
> > Signed-off-by: Bastian Hecht <hechtb@xxxxxxxxx>
> > ---
> > diff --git a/drivers/media/video/ov5642.c b/drivers/media/video/ov5642.c
> > index 6410bda..87b432e 100644
> > --- a/drivers/media/video/ov5642.c
> > +++ b/drivers/media/video/ov5642.c
>
> [snip]
>
> > @@ -684,107 +737,101 @@ static int ov5642_write_array(struct i2c_client
>
> [snip]
>
> > -static int ov5642_s_fmt(struct v4l2_subdev *sd,
> > - struct v4l2_mbus_framefmt *mf)
> > +static int ov5642_s_fmt(struct v4l2_subdev *sd, struct
> > v4l2_mbus_framefmt *mf) {
> >
> > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > struct ov5642 *priv = to_ov5642(client);
> >
> > -
> > - dev_dbg(sd->v4l2_dev->dev, "%s(%u)\n", __func__, mf->code);
> > + int ret;
> >
> > /* MIPI CSI could have changed the format, double-check */
> > if (!ov5642_find_datafmt(mf->code))
> >
> > return -EINVAL;
> >
> > ov5642_try_fmt(sd, mf);
> >
> > -
> >
> > priv->fmt = ov5642_find_datafmt(mf->code);
> >
> > - ov5642_write_array(client, ov5642_default_regs_init);
> > - ov5642_set_resolution(client);
> > - ov5642_write_array(client, ov5642_default_regs_finalise);
> > + ret = ov5642_write_array(client, ov5642_default_regs_init);
> > + if (!ret)
> > + ret = ov5642_set_resolution(sd);
> > + if (!ret)
> > + ret = ov5642_write_array(client, ov5642_default_regs_finalise);
>
> You shouldn't write anything to the sensor here. As only .s_crop can
> currently change the format, .s_fmt should just return the current format
> without performing any change or writing anything to the device.
>
> > - return 0;
> > + return ret;
> >
> > }
>
> [snip]
>
> > @@ -827,15 +874,42 @@ static int ov5642_g_chip_ident(struct v4l2_subdev
>
> [snip]
>
> > static int ov5642_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> > {
> >
> > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > + struct ov5642 *priv = to_ov5642(client);
> >
> > struct v4l2_rect *rect = &a->c;
> >
> > - a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > - rect->top = 0;
> > - rect->left = 0;
> > - rect->width = OV5642_WIDTH;
> > - rect->height = OV5642_HEIGHT;
> > + a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>
> Shouldn't you return an error instead when a->type is not
> V4L2_BUF_TYPE_VIDEO_CAPTURE ?
>
> > + *rect = priv->crop_rect;
> >
> > return 0;
> >
> > }
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
