On Mon, 5 Sep 2011, Laurent Pinchart wrote:
> 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() ?
Throughout the development of this driver, I was opposing the "delayed
configuration" approach. I.e., the approach, in which all the ioctl()s,
like S_FMT, S_CROP, etc. only store user values internally, and the actual
hardware configuration is only performed at STREAMON time. There are
several reasons to this: the spec says "the driver may program the
hardware, allocate resources and generally prepare for data exchange"
(yes, "may" != "must"), most drivers seem to do the same, the possibility
to check and return any hardware errors, returned by this operation, I
probably have forgotten something. But if we ignore all these reasons as
insufficiently important, then yes, doing the actualy hardware
configuration in .s_stream() brings a couple of advantages with it,
especially for drivers / devices like this one.
So, if there are no strong objections, maybe indeed move this back to
.s_stream() would be the better solution here.
Thanks
Guennadi
>
> > >> > - 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
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
