Hi Bastian,
On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
> 2011/8/31 Laurent Pinchart:
> > 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"
>
> I think that you think I should do the right thing. I removed out_sizes and
> repost v3 in a moment :)
Thanks :-)
> > 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.
>
> We talked about it in the ov5642 controls thread. I need to initialize
> the sensor at some point and it doesn't work to divide the calls
> between different locations.
Sure, but calling s_fmt isn't mandatory for hosts/bridges. What about moving
sensor initialization to s_stream() ?
> >> > - 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 ?
>
> No idea, but if you say so, I'll change it.
VIDIOC_G_FMT documentation states that
"When the requested buffer type is not supported drivers return an EINVAL
error code."
I thought VIDIOC_G_CROP documentation did as well, but it doesn't. However I
believe the above should apply to VIDIOC_G_CROP as well. There is no explicit
documentation about error codes for subdev operations, but I think it makes
sense to follow what the V4L2 ioctls do.
--
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
