* [PATCH v2 2/2] adv7180: add more subdev video ops
@ 2013-04-11 22:08 Sergei Shtylyov
2013-04-12 8:05 ` Hans Verkuil
0 siblings, 1 reply; 3+ messages in thread
From: Sergei Shtylyov @ 2013-04-11 22:08 UTC (permalink / raw)
To: mchehab, linux-media; +Cc: vladimir.barinov
From: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Add subdev video ops for ADV7180 video decoder. This makes decoder usable on
the soc-camera drivers.
Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
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
@@ -1,6 +1,8 @@
/*
* adv7180.c Analog Devices ADV7180 video decoder driver
* Copyright (c) 2009 Intel Corporation
+ * Copyright (C) 2013 Cogent Embedded, Inc.
+ * Copyright (C) 2013 Renesas Solutions Corp.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -128,6 +130,7 @@ struct adv7180_state {
v4l2_std_id curr_norm;
bool autodetect;
u8 input;
+ struct v4l2_mbus_framefmt fmt;
};
#define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler, \
struct adv7180_state, \
@@ -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);
+
+ fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
+ fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
+ fmt->field = V4L2_FIELD_INTERLACED;
+ fmt->width = 720;
+ fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
+
+ return 0;
+}
+
+static int adv7180_g_mbus_fmt(struct v4l2_subdev *sd,
+ struct v4l2_mbus_framefmt *fmt)
+{
+ struct adv7180_state *state = to_state(sd);
+
+ *fmt = state->fmt;
+
+ return 0;
+}
+
+static int adv7180_s_mbus_fmt(struct v4l2_subdev *sd,
+ struct v4l2_mbus_framefmt *fmt)
+{
+ struct adv7180_state *state = to_state(sd);
+
+ adv7180_try_mbus_fmt(sd, fmt);
+ state->fmt = *fmt;
+
+ return 0;
+}
+
+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;
+}
+
+static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
+ struct v4l2_mbus_config *cfg)
+{
+ /*
+ * The ADV7180 sensor supports BT.601/656 output modes.
+ * The BT.656 is default and not yet configurable by s/w.
+ */
+ cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
+ V4L2_MBUS_DATA_ACTIVE_HIGH;
+ cfg->type = V4L2_MBUS_BT656;
+
+ return 0;
+}
+
static const struct v4l2_subdev_video_ops adv7180_video_ops = {
.querystd = adv7180_querystd,
.g_input_status = adv7180_g_input_status,
.s_routing = adv7180_s_routing,
+ .enum_mbus_fmt = adv7180_enum_mbus_fmt,
+ .try_mbus_fmt = adv7180_try_mbus_fmt,
+ .g_mbus_fmt = adv7180_g_mbus_fmt,
+ .s_mbus_fmt = adv7180_s_mbus_fmt,
+ .cropcap = adv7180_cropcap,
+ .g_crop = adv7180_g_crop,
+ .g_mbus_config = adv7180_g_mbus_config,
};
static const struct v4l2_subdev_core_ops adv7180_core_ops = {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 2/2] adv7180: add more subdev video ops
2013-04-11 22:08 [PATCH v2 2/2] adv7180: add more subdev video ops Sergei Shtylyov
@ 2013-04-12 8:05 ` Hans Verkuil
2013-04-12 11:35 ` Sergei Shtylyov
0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2013-04-12 8:05 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: mchehab, linux-media, vladimir.barinov
Hi Sergei,
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@cogentembedded.com>
>
> Add subdev video ops for ADV7180 video decoder. This makes decoder usable on
> the soc-camera drivers.
>
> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
> 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
> @@ -1,6 +1,8 @@
> /*
> * adv7180.c Analog Devices ADV7180 video decoder driver
> * Copyright (c) 2009 Intel Corporation
> + * Copyright (C) 2013 Cogent Embedded, Inc.
> + * Copyright (C) 2013 Renesas Solutions Corp.
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> @@ -128,6 +130,7 @@ struct adv7180_state {
> v4l2_std_id curr_norm;
> bool autodetect;
> u8 input;
> + struct v4l2_mbus_framefmt fmt;
> };
> #define to_adv7180_sd(_ctrl) (&container_of(_ctrl->handler, \
> struct adv7180_state, \
> @@ -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.
> +
> + fmt->code = V4L2_MBUS_FMT_YUYV8_2X8;
> + fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> + fmt->field = V4L2_FIELD_INTERLACED;
> + fmt->width = 720;
> + fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> +
> + return 0;
> +}
> +
> +static int adv7180_g_mbus_fmt(struct v4l2_subdev *sd,
> + struct v4l2_mbus_framefmt *fmt)
> +{
> + struct adv7180_state *state = to_state(sd);
> +
> + *fmt = state->fmt;
> +
> + return 0;
> +}
> +
> +static int adv7180_s_mbus_fmt(struct v4l2_subdev *sd,
> + struct v4l2_mbus_framefmt *fmt)
> +{
> + struct adv7180_state *state = to_state(sd);
> +
> + adv7180_try_mbus_fmt(sd, fmt);
> + state->fmt = *fmt;
> +
> + return 0;
> +}
> +
> +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?
> +
> +static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
> + struct v4l2_mbus_config *cfg)
> +{
> + /*
> + * The ADV7180 sensor supports BT.601/656 output modes.
> + * The BT.656 is default and not yet configurable by s/w.
> + */
> + cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> + V4L2_MBUS_DATA_ACTIVE_HIGH;
> + cfg->type = V4L2_MBUS_BT656;
> +
> + return 0;
> +}
> +
> static const struct v4l2_subdev_video_ops adv7180_video_ops = {
> .querystd = adv7180_querystd,
> .g_input_status = adv7180_g_input_status,
> .s_routing = adv7180_s_routing,
> + .enum_mbus_fmt = adv7180_enum_mbus_fmt,
> + .try_mbus_fmt = adv7180_try_mbus_fmt,
> + .g_mbus_fmt = adv7180_g_mbus_fmt,
> + .s_mbus_fmt = adv7180_s_mbus_fmt,
> + .cropcap = adv7180_cropcap,
> + .g_crop = adv7180_g_crop,
> + .g_mbus_config = adv7180_g_mbus_config,
> };
>
> static const struct v4l2_subdev_core_ops adv7180_core_ops = {
Regards,
Hans
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 2/2] adv7180: add more subdev video ops
2013-04-12 8:05 ` Hans Verkuil
@ 2013-04-12 11:35 ` Sergei Shtylyov
0 siblings, 0 replies; 3+ messages in thread
From: Sergei Shtylyov @ 2013-04-12 11:35 UTC (permalink / raw)
To: Hans Verkuil; +Cc: mchehab, linux-media, vladimir.barinov, kernel
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@cogentembedded.com>
>> Add subdev video ops for ADV7180 video decoder. This makes decoder usable on
>> the soc-camera drivers.
>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-04-12 11:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-11 22:08 [PATCH v2 2/2] adv7180: add more subdev video ops Sergei Shtylyov
2013-04-12 8:05 ` Hans Verkuil
2013-04-12 11:35 ` Sergei Shtylyov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.