On Tue, 25 Aug 2009, 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?
skip_top_lines was a suggestion of Hans, maybe you're right and
g_skip_top_lines better describes the purpose of this function, but since
Hans is the primary author and maintainer of this API, let's see what he
says.
> 2. Why enumeration mechanisms are not longer needed for a video device?
> (You're removing them from video_ops)
Also this was Hans' suggestion - he said, that they are only needed for
sensors.
> 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?
No, this is something different. This is not the same as what cropcap
delivers. Some sensors (e.g., mt9v022) corrupt some (1 with mt9v022) top
lines, regardless what cropping you configure. So, you have to configure
your bridge (or an SoC camera host) to skip that many lines at the top of
an image. I am not sure whether all bridges can do this, fortunately,
PXA270 and i.MX31 (not yet implemented), to which I can connect mt9v022,
can do this.
Thanks
Guennadi
---
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
