Hello.
On 12-04-2013 12:05, Hans Verkuil wrote:
Thanks for the patch!
I've got some comments about this, though.
See below:
On Fri April 12 2013 00:08:09 Sergei Shtylyov wrote:
From: Vladimir Barinov <vladimir.barinov@xxxxxxxxxxxxxxxxxx>
Add subdev video ops for ADV7180 video decoder. This makes decoder usable on
the soc-camera drivers.
Signed-off-by: Vladimir Barinov <vladimir.barinov@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
---
drivers/media/i2c/adv7180.c | 105 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)
Index: linux/drivers/media/i2c/adv7180.c
===================================================================
--- linux.orig/drivers/media/i2c/adv7180.c
+++ linux/drivers/media/i2c/adv7180.c
[...]
@@ -397,10 +400,112 @@ static void adv7180_exit_controls(struct
v4l2_ctrl_handler_free(&state->ctrl_hdl);
}
+static int adv7180_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
+ enum v4l2_mbus_pixelcode *code)
+{
+ if (index > 0)
+ return -EINVAL;
+
+ *code = V4L2_MBUS_FMT_YUYV8_2X8;
+
+ return 0;
+}
+
+static int adv7180_try_mbus_fmt(struct v4l2_subdev *sd,
+ struct v4l2_mbus_framefmt *fmt)
+{
+ struct adv7180_state *state = to_state(sd);
+
+ adv7180_querystd(sd, &state->curr_norm);
No, you must use the currently set std here. What querystd returns is
effectively unpredictable, and that defeats the purpose of this try
function. It is always up to the application to call querystd and then
call s_std to set the standard explicitly. The same problem applies to
the other calls to querystd in this patch.
The current set/change of std is implemented in the initialization stage
or in the interrupt handler. So it is not able to catch the change of STD if
the h/w do not use the ADC7180 IRQ line. Implementation of polling scheme for
systems without IRQ line used will provide unnecessary overhead. Hence the
adv7180_querystd was added in order to update the state->curr_norm. Should
this be changed to:
+ if (!state->irq)
+ adv7180_querystd(sd, &state->curr_norm);
?
[...]
+static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
+{
+ struct adv7180_state *state = to_state(sd);
+
+ adv7180_querystd(sd, &state->curr_norm);
+
+ a->bounds.left = 0;
+ a->bounds.top = 0;
+ a->bounds.width = 720;
+ a->bounds.height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
+ a->defrect = a->bounds;
+ a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ a->pixelaspect.numerator = 1;
+ a->pixelaspect.denominator = 1;
+
+ return 0;
+}
+
+static int adv7180_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
+{
+ struct adv7180_state *state = to_state(sd);
+
+ adv7180_querystd(sd, &state->curr_norm);
+
+ a->c.left = 0;
+ a->c.top = 0;
+ a->c.width = 720;
+ a->c.height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
+ a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+
+ return 0;
+}
You are not actually implementing any cropping, so are these two ops really
necessary?
Thank you for the comment.
You are right, cropping and not implemented and g_crop is not a demand for
camera-soc
(the sample for minimal ops needed for camera-soc was taken from
drivers/media/platform/
soc_camera/soc_camera_platform.c that also does not implement cropping via
s_crop), but
cropcap currently is needed by some soc-camera drivers in order to determine
the default
rectangle.
[...]
Regards,
Hans
WBR, Sergei
--
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
