2011/9/5 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>:
> Hi Guennadi,
>
> On Monday 05 September 2011 11:51:57 Guennadi Liakhovetski wrote:
>> On Mon, 5 Sep 2011, Laurent Pinchart wrote:
>> > On Monday 05 September 2011 11:10:48 Bastian Hecht wrote:
>> > > 2011/8/31 Laurent Pinchart:
>> > > > 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.
>
> I have no strong opinion here. Your points are certainly valid. I'm fine with
> performing direct hardware setup in .s_crop(), but doing it in .s_fmt() looks
> weird to me as .s_fmt() doesn't perform any operation now that the driver
> moved to using .s_crop(). Without delayed initialization I believe the device
> should be initialized when powered up, and have its crop rectangle altered in
> .s_crop().
Ok, it is moved to s_power and s_crop now. This approach sounds clean indeed.
best,
Bastian
> --
> 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
