On Tuesday 25 August 2009 21:17:45 Hans Verkuil wrote:
> On Tuesday 25 August 2009 21:00:19 Aguirre Rodriguez, Sergio Alberto wrote:
> > Guennadi,
> >
> > Some comments I came across embedded below:
> >
> > <snip>
> >
> > > +
> > > +/**
> > > + * struct v4l2_subdev_sensor_ops - v4l2-subdev sensor operations
> > > + * @enum_framesizes: enumerate supported framesizes
> > > + * @enum_frameintervals: enumerate supported frame format intervals
> > > + * @skip_top_lines: number of lines at the top of the image to be
> > > skipped. This
> > > + * is needed for some sensors, that corrupt several top
> > > lines.
> > > + */
> > > +struct v4l2_subdev_sensor_ops {
> > > int (*enum_framesizes)(struct v4l2_subdev *sd, struct
> > > v4l2_frmsizeenum *fsize);
> > > int (*enum_frameintervals)(struct v4l2_subdev *sd, struct
> > > v4l2_frmivalenum *fival);
> > > + int (*skip_top_lines)(struct v4l2_subdev *sd, u32 *lines);
> > > };
> >
> > 1. I honestly find a bit misleading the skip_top_lines name, since that
> > IMO could be misunderstood that the called function will DO skip lines in
> > the sensor, which is not the intended response...
> >
> > How about g_skip_top_lines, or get_skip_top_lines, or something that
> > clarifies it's a "get information" abstraction interface?
>
> Good point. g_skip_top_lines is a better choice.
>
> > 2. Why enumeration mechanisms are not longer needed for a video device?
> > (You're removing them from video_ops)
>
> Because these ops are closely related to sensor devices. They do not
> normally apply to generic video devices. The addition of this new operation
> is a good moment to move all sensor-specific ops to this new sensor_ops
> struct.
>
> > 3. Wouldn't it be better to report a valid region, instead of just the
> > top lines? I think that should be already covered by the driver reporting
> > the valid size regions on enumeration, no?
>
> This has nothing to do with a valid region. No matter what region you
> capture, the first X lines will always be corrupt for some sensors.
> Something that clearly needs to be clarified in the comments.
Could such sensors corrupt the bottom Y lines too, and maybe some columns on
the sides ? In that case a "non-corrupted" region would make sense (but would
be more difficult to handle).
--
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
