[PATCH 00/11] adv7180 subdev fixes

[PATCH 00/11] adv7180 subdev fixes

[Steve Longerbeam]

Steve Longerbeam (11):
  media: adv7180: Fix broken interrupt register access
  Revert "[media] adv7180: fix broken standards handling"
  media: adv7180: add power pin control
  media: adv7180: implement g_parm
  media: adv7180: init chip with AD recommended register settings
  media: adv7180: add bt.656-4 OF property
  media: adv7180: change mbus format to UYVY
  adv7180: send V4L2_EVENT_SOURCE_CHANGE on std change
  v4l: Add signal lock status to source change events
  media: adv7180: enable lock/unlock interrupts
  media: adv7180: fix field type

 Documentation/DocBook/media/v4l/vidioc-dqevent.xml |  12 +-
 drivers/media/i2c/Kconfig                          |   2 +-
 drivers/media/i2c/adv7180.c                        | 409 +++++++++++++++------
 include/uapi/linux/videodev2.h                     |   1 +
 4 files changed, 308 insertions(+), 116 deletions(-)

-- 
1.9.1

[PATCH 01/11] media: adv7180: Fix broken interrupt register access

[Steve Longerbeam]

Access to the interrupt page registers has been broken since
at least 3999e5d01da74f1a22afbb0b61b3992fea301478. That commit
forgot to add the inerrupt page number to the register defines.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index b77b0a4..95cbc85 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -100,7 +100,7 @@
 #define ADV7180_REG_IDENT 0x0011
 #define ADV7180_ID_7180 0x18
 
-#define ADV7180_REG_ICONF1		0x0040
+#define ADV7180_REG_ICONF1		0x2040
 #define ADV7180_ICONF1_ACTIVE_LOW	0x01
 #define ADV7180_ICONF1_PSYNC_ONLY	0x10
 #define ADV7180_ICONF1_ACTIVE_TO_CLR	0xC0
@@ -113,15 +113,15 @@
 
 #define ADV7180_IRQ1_LOCK	0x01
 #define ADV7180_IRQ1_UNLOCK	0x02
-#define ADV7180_REG_ISR1	0x0042
-#define ADV7180_REG_ICR1	0x0043
-#define ADV7180_REG_IMR1	0x0044
-#define ADV7180_REG_IMR2	0x0048
+#define ADV7180_REG_ISR1	0x2042
+#define ADV7180_REG_ICR1	0x2043
+#define ADV7180_REG_IMR1	0x2044
+#define ADV7180_REG_IMR2	0x2048
 #define ADV7180_IRQ3_AD_CHANGE	0x08
-#define ADV7180_REG_ISR3	0x004A
-#define ADV7180_REG_ICR3	0x004B
-#define ADV7180_REG_IMR3	0x004C
-#define ADV7180_REG_IMR4	0x50
+#define ADV7180_REG_ISR3	0x204A
+#define ADV7180_REG_ICR3	0x204B
+#define ADV7180_REG_IMR3	0x204C
+#define ADV7180_REG_IMR4	0x2050
 
 #define ADV7180_REG_NTSC_V_BIT_END	0x00E6
 #define ADV7180_NTSC_V_BIT_END_MANUAL_NVEND	0x4F
-- 
1.9.1

Re: [PATCH 01/11] media: adv7180: Fix broken interrupt register access

[Tim Harvey]

On Wed, Jul 6, 2016 at 3:59 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> Access to the interrupt page registers has been broken since
> at least 3999e5d01da74f1a22afbb0b61b3992fea301478. That commit
> forgot to add the inerrupt page number to the register defines.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/media/i2c/adv7180.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index b77b0a4..95cbc85 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -100,7 +100,7 @@
>  #define ADV7180_REG_IDENT 0x0011
>  #define ADV7180_ID_7180 0x18
>
> -#define ADV7180_REG_ICONF1             0x0040
> +#define ADV7180_REG_ICONF1             0x2040
>  #define ADV7180_ICONF1_ACTIVE_LOW      0x01
>  #define ADV7180_ICONF1_PSYNC_ONLY      0x10
>  #define ADV7180_ICONF1_ACTIVE_TO_CLR   0xC0
> @@ -113,15 +113,15 @@
>
>  #define ADV7180_IRQ1_LOCK      0x01
>  #define ADV7180_IRQ1_UNLOCK    0x02
> -#define ADV7180_REG_ISR1       0x0042
> -#define ADV7180_REG_ICR1       0x0043
> -#define ADV7180_REG_IMR1       0x0044
> -#define ADV7180_REG_IMR2       0x0048
> +#define ADV7180_REG_ISR1       0x2042
> +#define ADV7180_REG_ICR1       0x2043
> +#define ADV7180_REG_IMR1       0x2044
> +#define ADV7180_REG_IMR2       0x2048
>  #define ADV7180_IRQ3_AD_CHANGE 0x08
> -#define ADV7180_REG_ISR3       0x004A
> -#define ADV7180_REG_ICR3       0x004B
> -#define ADV7180_REG_IMR3       0x004C
> -#define ADV7180_REG_IMR4       0x50
> +#define ADV7180_REG_ISR3       0x204A
> +#define ADV7180_REG_ICR3       0x204B
> +#define ADV7180_REG_IMR3       0x204C
> +#define ADV7180_REG_IMR4       0x2050
>
>  #define ADV7180_REG_NTSC_V_BIT_END     0x00E6
>  #define ADV7180_NTSC_V_BIT_END_MANUAL_NVEND    0x4F
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tested on an IMX6 Gateworks Ventana with IMX6 capture drivers from Steve [1]

Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>

Added to Cc list those who signed-off and acked
3999e5d01da74f1a22afbb0b61b3992fea301478

[1] - http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/102914

Re: [PATCH 01/11] media: adv7180: Fix broken interrupt register access

[Lars-Peter Clausen]

On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
> Access to the interrupt page registers has been broken since
> at least 3999e5d01da74f1a22afbb0b61b3992fea301478. That commit
> forgot to add the inerrupt page number to the register defines.

typo: interrupt

> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>

Looks good, thanks.

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

[PATCH 02/11] Revert "[media] adv7180: fix broken standards handling"

[Steve Longerbeam]

Autodetect was likely broken only because access to the
interrupt registers were broken, so there were no standard
change interrupts. After fixing that, and reverting this,
autodetect seems to work just fine on an i.mx6q SabreAuto.

This reverts commit 937feeed3f0ae8a0389d5732f6db63dd912acd99.
---
 drivers/media/i2c/adv7180.c | 118 ++++++++++++++------------------------------
 1 file changed, 38 insertions(+), 80 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 95cbc85..967303a 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -26,9 +26,8 @@
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/of.h>
-#include <linux/videodev2.h>
 #include <media/v4l2-ioctl.h>
-#include <media/v4l2-event.h>
+#include <linux/videodev2.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ctrls.h>
 #include <linux/mutex.h>
@@ -193,8 +192,8 @@ struct adv7180_state {
 	struct mutex		mutex; /* mutual excl. when accessing chip */
 	int			irq;
 	v4l2_std_id		curr_norm;
+	bool			autodetect;
 	bool			powered;
-	bool			streaming;
 	u8			input;
 
 	struct i2c_client	*client;
@@ -339,26 +338,12 @@ static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
 	if (err)
 		return err;
 
-	if (state->streaming) {
-		err = -EBUSY;
-		goto unlock;
-	}
-
-	err = adv7180_set_video_standard(state,
-			ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
-	if (err)
-		goto unlock;
-
-	msleep(100);
-	__adv7180_status(state, NULL, std);
-
-	err = v4l2_std_to_adv7180(state->curr_norm);
-	if (err < 0)
-		goto unlock;
-
-	err = adv7180_set_video_standard(state, err);
+	/* when we are interrupt driven we know the state */
+	if (!state->autodetect || state->irq > 0)
+		*std = state->curr_norm;
+	else
+		err = __adv7180_status(state, NULL, std);
 
-unlock:
 	mutex_unlock(&state->mutex);
 	return err;
 }
@@ -402,13 +387,23 @@ static int adv7180_program_std(struct adv7180_state *state)
 {
 	int ret;
 
-	ret = v4l2_std_to_adv7180(state->curr_norm);
-	if (ret < 0)
-		return ret;
+	if (state->autodetect) {
+		ret = adv7180_set_video_standard(state,
+			ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
+		if (ret < 0)
+			return ret;
+
+		__adv7180_status(state, NULL, &state->curr_norm);
+	} else {
+		ret = v4l2_std_to_adv7180(state->curr_norm);
+		if (ret < 0)
+			return ret;
+
+		ret = adv7180_set_video_standard(state, ret);
+		if (ret < 0)
+			return ret;
+	}
 
-	ret = adv7180_set_video_standard(state, ret);
-	if (ret < 0)
-		return ret;
 	return 0;
 }
 
@@ -420,12 +415,18 @@ static int adv7180_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 	if (ret)
 		return ret;
 
-	/* Make sure we can support this std */
-	ret = v4l2_std_to_adv7180(std);
-	if (ret < 0)
-		goto out;
+	/* all standards -> autodetect */
+	if (std == V4L2_STD_ALL) {
+		state->autodetect = true;
+	} else {
+		/* Make sure we can support this std */
+		ret = v4l2_std_to_adv7180(std);
+		if (ret < 0)
+			goto out;
 
-	state->curr_norm = std;
+		state->curr_norm = std;
+		state->autodetect = false;
+	}
 
 	ret = adv7180_program_std(state);
 out:
@@ -746,40 +747,6 @@ static int adv7180_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
 	return 0;
 }
 
-static int adv7180_s_stream(struct v4l2_subdev *sd, int enable)
-{
-	struct adv7180_state *state = to_state(sd);
-	int ret;
-
-	/* It's always safe to stop streaming, no need to take the lock */
-	if (!enable) {
-		state->streaming = enable;
-		return 0;
-	}
-
-	/* Must wait until querystd released the lock */
-	ret = mutex_lock_interruptible(&state->mutex);
-	if (ret)
-		return ret;
-	state->streaming = enable;
-	mutex_unlock(&state->mutex);
-	return 0;
-}
-
-static int adv7180_subscribe_event(struct v4l2_subdev *sd,
-				   struct v4l2_fh *fh,
-				   struct v4l2_event_subscription *sub)
-{
-	switch (sub->type) {
-	case V4L2_EVENT_SOURCE_CHANGE:
-		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
-	case V4L2_EVENT_CTRL:
-		return v4l2_ctrl_subdev_subscribe_event(sd, fh, sub);
-	default:
-		return -EINVAL;
-	}
-}
-
 static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.s_std = adv7180_s_std,
 	.g_std = adv7180_g_std,
@@ -789,13 +756,10 @@ static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.g_mbus_config = adv7180_g_mbus_config,
 	.cropcap = adv7180_cropcap,
 	.g_tvnorms = adv7180_g_tvnorms,
-	.s_stream = adv7180_s_stream,
 };
 
 static const struct v4l2_subdev_core_ops adv7180_core_ops = {
 	.s_power = adv7180_s_power,
-	.subscribe_event = adv7180_subscribe_event,
-	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 };
 
 static const struct v4l2_subdev_pad_ops adv7180_pad_ops = {
@@ -820,14 +784,8 @@ static irqreturn_t adv7180_irq(int irq, void *devid)
 	/* clear */
 	adv7180_write(state, ADV7180_REG_ICR3, isr3);
 
-	if (isr3 & ADV7180_IRQ3_AD_CHANGE) {
-		static const struct v4l2_event src_ch = {
-			.type = V4L2_EVENT_SOURCE_CHANGE,
-			.u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
-		};
-
-		v4l2_subdev_notify_event(&state->sd, &src_ch);
-	}
+	if (isr3 & ADV7180_IRQ3_AD_CHANGE && state->autodetect)
+		__adv7180_status(state, NULL, &state->curr_norm);
 	mutex_unlock(&state->mutex);
 
 	return IRQ_HANDLED;
@@ -1272,7 +1230,7 @@ static int adv7180_probe(struct i2c_client *client,
 
 	state->irq = client->irq;
 	mutex_init(&state->mutex);
-	state->curr_norm = V4L2_STD_NTSC;
+	state->autodetect = true;
 	if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
 		state->powered = true;
 	else
@@ -1280,7 +1238,7 @@ static int adv7180_probe(struct i2c_client *client,
 	state->input = 0;
 	sd = &state->sd;
 	v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
-	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	ret = adv7180_init_controls(state);
 	if (ret)
-- 
1.9.1

Re: [PATCH 02/11] Revert "[media] adv7180: fix broken standards handling"

[Tim Harvey]

On Wed, Jul 6, 2016 at 3:59 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> Autodetect was likely broken only because access to the
> interrupt registers were broken, so there were no standard
> change interrupts. After fixing that, and reverting this,
> autodetect seems to work just fine on an i.mx6q SabreAuto.
>
> This reverts commit 937feeed3f0ae8a0389d5732f6db63dd912acd99.
> ---
>  drivers/media/i2c/adv7180.c | 118 ++++++++++++++------------------------------
>  1 file changed, 38 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 95cbc85..967303a 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -26,9 +26,8 @@
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/of.h>
> -#include <linux/videodev2.h>
>  #include <media/v4l2-ioctl.h>
> -#include <media/v4l2-event.h>
> +#include <linux/videodev2.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ctrls.h>
>  #include <linux/mutex.h>
> @@ -193,8 +192,8 @@ struct adv7180_state {
>         struct mutex            mutex; /* mutual excl. when accessing chip */
>         int                     irq;
>         v4l2_std_id             curr_norm;
> +       bool                    autodetect;
>         bool                    powered;
> -       bool                    streaming;
>         u8                      input;
>
>         struct i2c_client       *client;
> @@ -339,26 +338,12 @@ static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
>         if (err)
>                 return err;
>
> -       if (state->streaming) {
> -               err = -EBUSY;
> -               goto unlock;
> -       }
> -
> -       err = adv7180_set_video_standard(state,
> -                       ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
> -       if (err)
> -               goto unlock;
> -
> -       msleep(100);
> -       __adv7180_status(state, NULL, std);
> -
> -       err = v4l2_std_to_adv7180(state->curr_norm);
> -       if (err < 0)
> -               goto unlock;
> -
> -       err = adv7180_set_video_standard(state, err);
> +       /* when we are interrupt driven we know the state */
> +       if (!state->autodetect || state->irq > 0)
> +               *std = state->curr_norm;
> +       else
> +               err = __adv7180_status(state, NULL, std);
>
> -unlock:
>         mutex_unlock(&state->mutex);
>         return err;
>  }
> @@ -402,13 +387,23 @@ static int adv7180_program_std(struct adv7180_state *state)
>  {
>         int ret;
>
> -       ret = v4l2_std_to_adv7180(state->curr_norm);
> -       if (ret < 0)
> -               return ret;
> +       if (state->autodetect) {
> +               ret = adv7180_set_video_standard(state,
> +                       ADV7180_STD_AD_PAL_BG_NTSC_J_SECAM);
> +               if (ret < 0)
> +                       return ret;
> +
> +               __adv7180_status(state, NULL, &state->curr_norm);
> +       } else {
> +               ret = v4l2_std_to_adv7180(state->curr_norm);
> +               if (ret < 0)
> +                       return ret;
> +
> +               ret = adv7180_set_video_standard(state, ret);
> +               if (ret < 0)
> +                       return ret;
> +       }
>
> -       ret = adv7180_set_video_standard(state, ret);
> -       if (ret < 0)
> -               return ret;
>         return 0;
>  }
>
> @@ -420,12 +415,18 @@ static int adv7180_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
>         if (ret)
>                 return ret;
>
> -       /* Make sure we can support this std */
> -       ret = v4l2_std_to_adv7180(std);
> -       if (ret < 0)
> -               goto out;
> +       /* all standards -> autodetect */
> +       if (std == V4L2_STD_ALL) {
> +               state->autodetect = true;
> +       } else {
> +               /* Make sure we can support this std */
> +               ret = v4l2_std_to_adv7180(std);
> +               if (ret < 0)
> +                       goto out;
>
> -       state->curr_norm = std;
> +               state->curr_norm = std;
> +               state->autodetect = false;
> +       }
>
>         ret = adv7180_program_std(state);
>  out:
> @@ -746,40 +747,6 @@ static int adv7180_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
>         return 0;
>  }
>
> -static int adv7180_s_stream(struct v4l2_subdev *sd, int enable)
> -{
> -       struct adv7180_state *state = to_state(sd);
> -       int ret;
> -
> -       /* It's always safe to stop streaming, no need to take the lock */
> -       if (!enable) {
> -               state->streaming = enable;
> -               return 0;
> -       }
> -
> -       /* Must wait until querystd released the lock */
> -       ret = mutex_lock_interruptible(&state->mutex);
> -       if (ret)
> -               return ret;
> -       state->streaming = enable;
> -       mutex_unlock(&state->mutex);
> -       return 0;
> -}
> -
> -static int adv7180_subscribe_event(struct v4l2_subdev *sd,
> -                                  struct v4l2_fh *fh,
> -                                  struct v4l2_event_subscription *sub)
> -{
> -       switch (sub->type) {
> -       case V4L2_EVENT_SOURCE_CHANGE:
> -               return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
> -       case V4L2_EVENT_CTRL:
> -               return v4l2_ctrl_subdev_subscribe_event(sd, fh, sub);
> -       default:
> -               return -EINVAL;
> -       }
> -}
> -
>  static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>         .s_std = adv7180_s_std,
>         .g_std = adv7180_g_std,
> @@ -789,13 +756,10 @@ static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>         .g_mbus_config = adv7180_g_mbus_config,
>         .cropcap = adv7180_cropcap,
>         .g_tvnorms = adv7180_g_tvnorms,
> -       .s_stream = adv7180_s_stream,
>  };
>
>  static const struct v4l2_subdev_core_ops adv7180_core_ops = {
>         .s_power = adv7180_s_power,
> -       .subscribe_event = adv7180_subscribe_event,
> -       .unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  };
>
>  static const struct v4l2_subdev_pad_ops adv7180_pad_ops = {
> @@ -820,14 +784,8 @@ static irqreturn_t adv7180_irq(int irq, void *devid)
>         /* clear */
>         adv7180_write(state, ADV7180_REG_ICR3, isr3);
>
> -       if (isr3 & ADV7180_IRQ3_AD_CHANGE) {
> -               static const struct v4l2_event src_ch = {
> -                       .type = V4L2_EVENT_SOURCE_CHANGE,
> -                       .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
> -               };
> -
> -               v4l2_subdev_notify_event(&state->sd, &src_ch);
> -       }
> +       if (isr3 & ADV7180_IRQ3_AD_CHANGE && state->autodetect)
> +               __adv7180_status(state, NULL, &state->curr_norm);
>         mutex_unlock(&state->mutex);
>
>         return IRQ_HANDLED;
> @@ -1272,7 +1230,7 @@ static int adv7180_probe(struct i2c_client *client,
>
>         state->irq = client->irq;
>         mutex_init(&state->mutex);
> -       state->curr_norm = V4L2_STD_NTSC;
> +       state->autodetect = true;
>         if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
>                 state->powered = true;
>         else
> @@ -1280,7 +1238,7 @@ static int adv7180_probe(struct i2c_client *client,
>         state->input = 0;
>         sd = &state->sd;
>         v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
> -       sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> +       sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
>
>         ret = adv7180_init_controls(state);
>         if (ret)
> --

Tested on an IMX6 Gateworks Ventana with IMX6 capture drivers from
Steve [1]. Verified that interrupts work via signal lock/unlock.

Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>

Added to Cc list those who signed-off and acked
937feeed3f0ae8a0389d5732f6db63dd912acd99:

Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Federico Vaga <federico.vaga@gmail.com>
Cc: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Regards,

Tim

[1] - http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/102914

Re: [PATCH 02/11] Revert "[media] adv7180: fix broken standards handling"

[Lars-Peter Clausen]

On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
> Autodetect was likely broken only because access to the
> interrupt registers were broken, so there were no standard
> change interrupts. After fixing that, and reverting this,
> autodetect seems to work just fine on an i.mx6q SabreAuto.
> 
> This reverts commit 937feeed3f0ae8a0389d5732f6db63dd912acd99.

The brokenness the commit refers to is conceptual not functional. The driver
simply implemented the API incorrect. A subdev driver is not allowed to
automatically switch the output format/resolution randomly. In the best case
this will confuse the receiver which is not prepared to receive the changed
resolution, in the worst case it will cause buffer overruns with hardware
that has no boundary checks. This is why this was removed from the driver.

The correct sequence is for the driver to generate a change notification and
then have userspace react to that notification by stopping the current
stream, query the new format/resolution, reconfigure the video pipeline for
the new format/resolution and re-start the stream.

Re: [PATCH 02/11] Revert "[media] adv7180: fix broken standards handling"

[Steve Longerbeam]

On 07/07/2016 08:45 AM, Lars-Peter Clausen wrote:
> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>> Autodetect was likely broken only because access to the
>> interrupt registers were broken, so there were no standard
>> change interrupts. After fixing that, and reverting this,
>> autodetect seems to work just fine on an i.mx6q SabreAuto.
>>
>> This reverts commit 937feeed3f0ae8a0389d5732f6db63dd912acd99.
> The brokenness the commit refers to is conceptual not functional. The driver
> simply implemented the API incorrect. A subdev driver is not allowed to
> automatically switch the output format/resolution randomly. In the best case
> this will confuse the receiver which is not prepared to receive the changed
> resolution, in the worst case it will cause buffer overruns with hardware
> that has no boundary checks. This is why this was removed from the driver.
>
> The correct sequence is for the driver to generate a change notification and
> then have userspace react to that notification by stopping the current
> stream, query the new format/resolution, reconfigure the video pipeline for
> the new format/resolution and re-start the stream.

Hi Lars, ok thanks for the clarification. Yes I agree that makes sense.
I will undo the revert in the next version and retest.

Steve

[PATCH 03/11] media: adv7180: add power pin control

[Steve Longerbeam]

Some targets control the ADV7180 power pin via a gpio, so add
support for "pwdn-gpio" pin control.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/Kconfig   |  2 +-
 drivers/media/i2c/adv7180.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 993dc50..80d39f6 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -187,7 +187,7 @@ comment "Video decoders"
 
 config VIDEO_ADV7180
 	tristate "Analog Devices ADV7180 decoder"
-	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
+	depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
 	---help---
 	  Support for the Analog Devices ADV7180 video decoder.
 
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 967303a..38e5161 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -26,6 +26,7 @@
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/gpio/consumer.h>
 #include <media/v4l2-ioctl.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-device.h>
@@ -191,6 +192,7 @@ struct adv7180_state {
 	struct media_pad	pad;
 	struct mutex		mutex; /* mutual excl. when accessing chip */
 	int			irq;
+	struct gpio_desc	*pwdn_gpio;
 	v4l2_std_id		curr_norm;
 	bool			autodetect;
 	bool			powered;
@@ -443,6 +445,19 @@ static int adv7180_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
 	return 0;
 }
 
+static void adv7180_set_power_pin(struct adv7180_state *state, bool on)
+{
+	if (!state->pwdn_gpio)
+		return;
+
+	if (on) {
+		gpiod_set_value_cansleep(state->pwdn_gpio, 0);
+		usleep_range(5000, 10000);
+	} else {
+		gpiod_set_value_cansleep(state->pwdn_gpio, 1);
+	}
+}
+
 static int adv7180_set_power(struct adv7180_state *state, bool on)
 {
 	u8 val;
@@ -1143,6 +1158,8 @@ static int init_device(struct adv7180_state *state)
 
 	mutex_lock(&state->mutex);
 
+	adv7180_set_power_pin(state, true);
+
 	adv7180_write(state, ADV7180_REG_PWR_MAN, ADV7180_PWR_MAN_RES);
 	usleep_range(5000, 10000);
 
@@ -1190,6 +1207,20 @@ out_unlock:
 	return ret;
 }
 
+static int adv7180_of_parse(struct adv7180_state *state)
+{
+	struct i2c_client *client = state->client;
+
+	state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
+						   GPIOD_OUT_HIGH);
+	if (IS_ERR(state->pwdn_gpio)) {
+		v4l_err(client, "request for power pin failed\n");
+		return PTR_ERR(state->pwdn_gpio);
+	}
+
+	return 0;
+}
+
 static int adv7180_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -1212,6 +1243,10 @@ static int adv7180_probe(struct i2c_client *client,
 	state->field = V4L2_FIELD_INTERLACED;
 	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
 
+	ret = adv7180_of_parse(state);
+	if (ret)
+		return ret;
+
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
 		state->csi_client = i2c_new_dummy(client->adapter,
 				ADV7180_DEFAULT_CSI_I2C_ADDR);
@@ -1303,6 +1338,8 @@ static int adv7180_remove(struct i2c_client *client)
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2)
 		i2c_unregister_device(state->csi_client);
 
+	adv7180_set_power_pin(state, false);
+
 	mutex_destroy(&state->mutex);
 
 	return 0;
-- 
1.9.1

Re: [PATCH 03/11] media: adv7180: add power pin control

[Tim Harvey]

On Wed, Jul 6, 2016 at 3:59 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> Some targets control the ADV7180 power pin via a gpio, so add
> support for "pwdn-gpio" pin control.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/media/i2c/Kconfig   |  2 +-
>  drivers/media/i2c/adv7180.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 993dc50..80d39f6 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -187,7 +187,7 @@ comment "Video decoders"
>
>  config VIDEO_ADV7180
>         tristate "Analog Devices ADV7180 decoder"
> -       depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> +       depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
>         ---help---
>           Support for the Analog Devices ADV7180 video decoder.
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 967303a..38e5161 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -26,6 +26,7 @@
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  #include <media/v4l2-ioctl.h>
>  #include <linux/videodev2.h>
>  #include <media/v4l2-device.h>
> @@ -191,6 +192,7 @@ struct adv7180_state {
>         struct media_pad        pad;
>         struct mutex            mutex; /* mutual excl. when accessing chip */
>         int                     irq;
> +       struct gpio_desc        *pwdn_gpio;
>         v4l2_std_id             curr_norm;
>         bool                    autodetect;
>         bool                    powered;
> @@ -443,6 +445,19 @@ static int adv7180_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
>         return 0;
>  }
>
> +static void adv7180_set_power_pin(struct adv7180_state *state, bool on)
> +{
> +       if (!state->pwdn_gpio)
> +               return;
> +
> +       if (on) {
> +               gpiod_set_value_cansleep(state->pwdn_gpio, 0);
> +               usleep_range(5000, 10000);
> +       } else {
> +               gpiod_set_value_cansleep(state->pwdn_gpio, 1);
> +       }
> +}
> +
>  static int adv7180_set_power(struct adv7180_state *state, bool on)
>  {
>         u8 val;
> @@ -1143,6 +1158,8 @@ static int init_device(struct adv7180_state *state)
>
>         mutex_lock(&state->mutex);
>
> +       adv7180_set_power_pin(state, true);
> +
>         adv7180_write(state, ADV7180_REG_PWR_MAN, ADV7180_PWR_MAN_RES);
>         usleep_range(5000, 10000);
>
> @@ -1190,6 +1207,20 @@ out_unlock:
>         return ret;
>  }
>
> +static int adv7180_of_parse(struct adv7180_state *state)
> +{
> +       struct i2c_client *client = state->client;
> +
> +       state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",
> +                                                  GPIOD_OUT_HIGH);
> +       if (IS_ERR(state->pwdn_gpio)) {
> +               v4l_err(client, "request for power pin failed\n");
> +               return PTR_ERR(state->pwdn_gpio);
> +       }
> +
> +       return 0;
> +}
> +
>  static int adv7180_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>  {
> @@ -1212,6 +1243,10 @@ static int adv7180_probe(struct i2c_client *client,
>         state->field = V4L2_FIELD_INTERLACED;
>         state->chip_info = (struct adv7180_chip_info *)id->driver_data;
>
> +       ret = adv7180_of_parse(state);
> +       if (ret)
> +               return ret;
> +
>         if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
>                 state->csi_client = i2c_new_dummy(client->adapter,
>                                 ADV7180_DEFAULT_CSI_I2C_ADDR);
> @@ -1303,6 +1338,8 @@ static int adv7180_remove(struct i2c_client *client)
>         if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2)
>                 i2c_unregister_device(state->csi_client);
>
> +       adv7180_set_power_pin(state, false);
> +
>         mutex_destroy(&state->mutex);
>
>         return 0;
> --

Steve,

For completeness, you also need to provide a patch to
Documentation/devicetree/bindings/media/i2c/adv7180.txt adding the
Optional property.

Tested on an IMX6 Gateworks Ventana with IMX6 capture drivers [1].
Verified that adv7180_set_power_pin() gets called properly.

Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>

Added to Cc:
Cc: Lars-Peter Clausen <lars@metafoo.de>

Regards,

Tim

[1] - http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/102914

Re: [PATCH 03/11] media: adv7180: add power pin control

[Lars-Peter Clausen]

> @@ -1190,6 +1207,20 @@ out_unlock:
>  	return ret;
>  }
>  
> +static int adv7180_of_parse(struct adv7180_state *state)

Since there is nothing of specific in here anymore the name should be
changed, or maybe just inline the code directly in probe.

> +{
> +	struct i2c_client *client = state->client;
> +
> +	state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "pwdn",

I'd use "powerdown", vowels don't cost extra ;):

> +						   GPIOD_OUT_HIGH);
> +	if (IS_ERR(state->pwdn_gpio)) {
> +		v4l_err(client, "request for power pin failed\n");

Include the error number in the message.

> +		return PTR_ERR(state->pwdn_gpio);
> +	}
> +
> +	return 0;
> +}

[PATCH 04/11] media: adv7180: implement g_parm

[Steve Longerbeam]

Implement g_parm to return the current standard's frame period.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 38e5161..42816d4 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -741,6 +741,27 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int adv7180_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
+{
+	struct adv7180_state *state = to_state(sd);
+	struct v4l2_captureparm *cparm = &a->parm.capture;
+
+	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	memset(a, 0, sizeof(*a));
+	a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	if (state->curr_norm & V4L2_STD_525_60) {
+		cparm->timeperframe.numerator = 1001;
+		cparm->timeperframe.denominator = 30000;
+	} else {
+		cparm->timeperframe.numerator = 1;
+		cparm->timeperframe.denominator = 25;
+	}
+
+	return 0;
+}
+
 static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *cropcap)
 {
 	struct adv7180_state *state = to_state(sd);
@@ -765,6 +786,7 @@ static int adv7180_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
 static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.s_std = adv7180_s_std,
 	.g_std = adv7180_g_std,
+	.g_parm = adv7180_g_parm,
 	.querystd = adv7180_querystd,
 	.g_input_status = adv7180_g_input_status,
 	.s_routing = adv7180_s_routing,
-- 
1.9.1

Re: [PATCH 04/11] media: adv7180: implement g_parm

[Tim Harvey]

On Wed, Jul 6, 2016 at 3:59 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> Implement g_parm to return the current standard's frame period.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/media/i2c/adv7180.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 38e5161..42816d4 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -741,6 +741,27 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
>         return 0;
>  }
>
> +static int adv7180_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
> +{
> +       struct adv7180_state *state = to_state(sd);
> +       struct v4l2_captureparm *cparm = &a->parm.capture;
> +
> +       if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +               return -EINVAL;
> +
> +       memset(a, 0, sizeof(*a));
> +       a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +       if (state->curr_norm & V4L2_STD_525_60) {
> +               cparm->timeperframe.numerator = 1001;
> +               cparm->timeperframe.denominator = 30000;
> +       } else {
> +               cparm->timeperframe.numerator = 1;
> +               cparm->timeperframe.denominator = 25;
> +       }
> +
> +       return 0;
> +}
> +
>  static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *cropcap)
>  {
>         struct adv7180_state *state = to_state(sd);
> @@ -765,6 +786,7 @@ static int adv7180_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
>  static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>         .s_std = adv7180_s_std,
>         .g_std = adv7180_g_std,
> +       .g_parm = adv7180_g_parm,
>         .querystd = adv7180_querystd,
>         .g_input_status = adv7180_g_input_status,
>         .s_routing = adv7180_s_routing,
> --

Steve,

Tested on an IMX6 Gateworks Ventana with IMX6 capture drivers [1].

Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>

Added to Cc:
Cc: Lars-Peter Clausen <lars@metafoo.de>

Regards,

Tim

[1] - http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/102914

[PATCH 05/11] media: adv7180: init chip with AD recommended register settings

[Steve Longerbeam]

Define and load register tables that conform to Analog Device's
recommended register settings. It loads the default single-ended
CVBS on Ain1 configuration for both ADV7180 and ADV7182 chips.

New register addresses have been defined for the tables. Those new
defines are also used in existing locations where hard-coded addresses
were used.

Note this patch also enables NEWAVMODE, which is also recommended by
Analog Devices. This will likely break any current backends using this
subdev that are expecting different or manually configured AV codes.

Note also that bt.656-4 support has been removed in this patch, but it
will be brought back in a subsequent patch.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 168 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 130 insertions(+), 38 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 42816d4..92e2f37 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -56,10 +56,11 @@
 
 #define ADV7182_REG_INPUT_VIDSEL			0x0002
 
+#define ADV7180_REG_OUTPUT_CONTROL			0x0003
 #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL		0x0004
 #define ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS		0xC5
 
-#define ADV7180_REG_AUTODETECT_ENABLE			0x07
+#define ADV7180_REG_AUTODETECT_ENABLE			0x0007
 #define ADV7180_AUTODETECT_DEFAULT			0x7f
 /* Contrast */
 #define ADV7180_REG_CON		0x0008	/*Unsigned */
@@ -100,6 +101,20 @@
 #define ADV7180_REG_IDENT 0x0011
 #define ADV7180_ID_7180 0x18
 
+#define ADV7180_REG_STATUS3		0x0013
+#define ADV7180_REG_ANALOG_CLAMP_CTL	0x0014
+#define ADV7180_REG_SHAP_FILTER_CTL_1	0x0017
+#define ADV7180_REG_CTRL_2		0x001d
+#define ADV7180_REG_VSYNC_FIELD_CTL_1	0x0031
+#define ADV7180_REG_MANUAL_WIN_CTL_1	0x003d
+#define ADV7180_REG_MANUAL_WIN_CTL_2	0x003e
+#define ADV7180_REG_MANUAL_WIN_CTL_3	0x003f
+#define ADV7180_REG_LOCK_CNT		0x0051
+#define ADV7180_REG_CVBS_TRIM		0x0052
+#define ADV7180_REG_CLAMP_ADJ		0x005a
+#define ADV7180_REG_RES_CIR		0x005f
+#define ADV7180_REG_DIFF_MODE		0x0060
+
 #define ADV7180_REG_ICONF1		0x2040
 #define ADV7180_ICONF1_ACTIVE_LOW	0x01
 #define ADV7180_ICONF1_PSYNC_ONLY	0x10
@@ -129,9 +144,15 @@
 #define ADV7180_REG_VPP_SLAVE_ADDR	0xFD
 #define ADV7180_REG_CSI_SLAVE_ADDR	0xFE
 
-#define ADV7180_REG_FLCONTROL 0x40e0
+#define ADV7180_REG_ACE_CTRL1		0x4080
+#define ADV7180_REG_ACE_CTRL5		0x4084
+#define ADV7180_REG_FLCONTROL		0x40e0
 #define ADV7180_FLCONTROL_FL_ENABLE 0x1
 
+#define ADV7180_REG_RST_CLAMP	0x809c
+#define ADV7180_REG_AGC_ADJ1	0x80b6
+#define ADV7180_REG_AGC_ADJ2	0x80c0
+
 #define ADV7180_CSI_REG_PWRDN	0x00
 #define ADV7180_CSI_PWRDN	0x80
 
@@ -209,6 +230,11 @@ struct adv7180_state {
 					    struct adv7180_state,	\
 					    ctrl_hdl)->sd)
 
+struct adv7180_reg_tbl_t {
+	unsigned int reg;
+	unsigned int val;
+};
+
 static int adv7180_select_page(struct adv7180_state *state, unsigned int page)
 {
 	if (state->register_page != page) {
@@ -235,6 +261,20 @@ static int adv7180_read(struct adv7180_state *state, unsigned int reg)
 	return i2c_smbus_read_byte_data(state->client, reg & 0xff);
 }
 
+static int adv7180_load_reg_tbl(struct adv7180_state *state,
+				const struct adv7180_reg_tbl_t *tbl, int n)
+{
+	int ret, i;
+
+	for (i = 0; i < n; i++) {
+		ret = adv7180_write(state, tbl[i].reg, tbl[i].val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int adv7180_csi_write(struct adv7180_state *state, unsigned int reg,
 	unsigned int value)
 {
@@ -828,19 +868,36 @@ static irqreturn_t adv7180_irq(int irq, void *devid)
 	return IRQ_HANDLED;
 }
 
+/*
+ * This register table conforms to Analog Device's Register Settings
+ * Recommendation for the ADV7180. It configures single-ended CVBS
+ * input on Ain1, and enables NEWAVMODE.
+ */
+static const struct adv7180_reg_tbl_t adv7180_single_ended_cvbs[] = {
+	/* Set analog mux for CVBS on Ain1 */
+	{ ADV7180_REG_INPUT_CONTROL, 0x00 },
+	/* ADI Required Write: Reset Clamp Circuitry */
+	{ ADV7180_REG_ANALOG_CLAMP_CTL, 0x30 },
+	/* Enable SFL Output */
+	{ ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57 },
+	/* Select SH1 Chroma Shaping Filter */
+	{ ADV7180_REG_SHAP_FILTER_CTL_1, 0x41 },
+	/* Enable NEWAVMODE */
+	{ ADV7180_REG_VSYNC_FIELD_CTL_1, 0x02 },
+	/* ADI Required Write: optimize windowing function Step 1,2,3 */
+	{ ADV7180_REG_MANUAL_WIN_CTL_1, 0xA2 },
+	{ ADV7180_REG_MANUAL_WIN_CTL_2, 0x6A },
+	{ ADV7180_REG_MANUAL_WIN_CTL_3, 0xA0 },
+	/* ADI Required Write: Enable ADC step 1,2,3 */
+	{ 0x8055, 0x81 }, /* undocumented register 0x55 */
+	/* Recommended AFE I BIAS Setting for CVBS mode */
+	{ ADV7180_REG_CVBS_TRIM, 0x0D },
+};
+
 static int adv7180_init(struct adv7180_state *state)
 {
-	int ret;
-
-	/* ITU-R BT.656-4 compatible */
-	ret = adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
-			ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS);
-	if (ret < 0)
-		return ret;
-
-	/* Manually set V bit end position in NTSC mode */
-	return adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
-					ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+	return adv7180_load_reg_tbl(state, adv7180_single_ended_cvbs,
+				    ARRAY_SIZE(adv7180_single_ended_cvbs));
 }
 
 static int adv7180_set_std(struct adv7180_state *state, unsigned int std)
@@ -862,8 +919,48 @@ static int adv7180_select_input(struct adv7180_state *state, unsigned int input)
 	return adv7180_write(state, ADV7180_REG_INPUT_CONTROL, ret);
 }
 
+/*
+ * This register table conforms to Analog Device's Register Settings
+ * Recommendation revision C for the ADV7182. It configures single-ended
+ * CVBS inputs on Ain1, and enables NEWAVMODE.
+ */
+static const struct adv7180_reg_tbl_t adv7182_single_ended_cvbs[] = {
+	/* Exit Power Down Mode */
+	{ ADV7180_REG_PWR_MAN, 0x00 },
+	/* Enable ADV7182 for 28.63636 MHz Crystal Clock Input */
+	{ ADV7180_REG_STATUS3, 0x00 },
+	/* Set optimized IBIAS for single-ended CVBS input */
+	{ ADV7180_REG_CVBS_TRIM, 0xCD },
+	/* Switch to single-ended CVBS on AIN1 */
+	{ ADV7180_REG_INPUT_CONTROL, 0x00 },
+	/* ADI Required Write: Reset Current Clamp Circuitry steps 1,2,3,4 */
+	{ ADV7180_REG_RST_CLAMP, 0x00 },
+	{ ADV7180_REG_RST_CLAMP, 0xFF },
+	/* Select SH1 Chroma Shaping Filter */
+	{ ADV7180_REG_SHAP_FILTER_CTL_1, 0x41 },
+	/* Enable Pixel & Sync output drivers */
+	{ ADV7180_REG_OUTPUT_CONTROL, 0x0C },
+	/* Power-up INTRQ, HS and VS/FIELD/SFL pad */
+	{ ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x07 },
+	/* Enable LLC Output Driver */
+	{ ADV7180_REG_CTRL_2, 0x40 },
+	/* Optimize ACE Performance */
+	{ ADV7180_REG_ACE_CTRL5, 0x00 },
+	/* Enable ACE Feature */
+	{ ADV7180_REG_ACE_CTRL1, 0x80 },
+	/* Enable NEWAVMODE */
+	{ ADV7180_REG_VSYNC_FIELD_CTL_1, 0x02 },
+};
+
 static int adv7182_init(struct adv7180_state *state)
 {
+	int ret;
+
+	ret = adv7180_load_reg_tbl(state, adv7182_single_ended_cvbs,
+				   ARRAY_SIZE(adv7182_single_ended_cvbs));
+	if (ret)
+		return ret;
+
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2)
 		adv7180_write(state, ADV7180_REG_CSI_SLAVE_ADDR,
 			ADV7180_DEFAULT_CSI_I2C_ADDR << 1);
@@ -881,20 +978,15 @@ static int adv7182_init(struct adv7180_state *state)
 
 	/* ADI required writes */
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
-		adv7180_write(state, 0x0003, 0x4e);
-		adv7180_write(state, 0x0004, 0x57);
-		adv7180_write(state, 0x001d, 0xc0);
+		adv7180_write(state, ADV7180_REG_OUTPUT_CONTROL, 0x4e);
+		adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57);
+		adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
 	} else {
 		if (state->chip_info->flags & ADV7180_FLAG_V2)
-			adv7180_write(state, 0x0004, 0x17);
-		else
-			adv7180_write(state, 0x0004, 0x07);
-		adv7180_write(state, 0x0003, 0x0c);
-		adv7180_write(state, 0x001d, 0x40);
+			adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
+				      0x17);
 	}
 
-	adv7180_write(state, 0x0013, 0x00);
-
 	return 0;
 }
 
@@ -967,8 +1059,8 @@ static int adv7182_select_input(struct adv7180_state *state, unsigned int input)
 		return ret;
 
 	/* Reset clamp circuitry - ADI recommended writes */
-	adv7180_write(state, 0x809c, 0x00);
-	adv7180_write(state, 0x809c, 0xff);
+	adv7180_write(state, ADV7180_REG_RST_CLAMP, 0x00);
+	adv7180_write(state, ADV7180_REG_RST_CLAMP, 0xff);
 
 	input_type = adv7182_get_input_type(input);
 
@@ -976,10 +1068,10 @@ static int adv7182_select_input(struct adv7180_state *state, unsigned int input)
 	case ADV7182_INPUT_TYPE_CVBS:
 	case ADV7182_INPUT_TYPE_DIFF_CVBS:
 		/* ADI recommends to use the SH1 filter */
-		adv7180_write(state, 0x0017, 0x41);
+		adv7180_write(state, ADV7180_REG_SHAP_FILTER_CTL_1, 0x41);
 		break;
 	default:
-		adv7180_write(state, 0x0017, 0x01);
+		adv7180_write(state, ADV7180_REG_SHAP_FILTER_CTL_1, 0x01);
 		break;
 	}
 
@@ -989,21 +1081,21 @@ static int adv7182_select_input(struct adv7180_state *state, unsigned int input)
 		lbias = adv7182_lbias_settings[input_type];
 
 	for (i = 0; i < ARRAY_SIZE(adv7182_lbias_settings[0]); i++)
-		adv7180_write(state, 0x0052 + i, lbias[i]);
+		adv7180_write(state, ADV7180_REG_CVBS_TRIM + i, lbias[i]);
 
 	if (input_type == ADV7182_INPUT_TYPE_DIFF_CVBS) {
 		/* ADI required writes to make differential CVBS work */
-		adv7180_write(state, 0x005f, 0xa8);
-		adv7180_write(state, 0x005a, 0x90);
-		adv7180_write(state, 0x0060, 0xb0);
-		adv7180_write(state, 0x80b6, 0x08);
-		adv7180_write(state, 0x80c0, 0xa0);
+		adv7180_write(state, ADV7180_REG_RES_CIR, 0xa8);
+		adv7180_write(state, ADV7180_REG_CLAMP_ADJ, 0x90);
+		adv7180_write(state, ADV7180_REG_DIFF_MODE, 0xb0);
+		adv7180_write(state, ADV7180_REG_AGC_ADJ1, 0x08);
+		adv7180_write(state, ADV7180_REG_AGC_ADJ2, 0xa0);
 	} else {
-		adv7180_write(state, 0x005f, 0xf0);
-		adv7180_write(state, 0x005a, 0xd0);
-		adv7180_write(state, 0x0060, 0x10);
-		adv7180_write(state, 0x80b6, 0x9c);
-		adv7180_write(state, 0x80c0, 0x00);
+		adv7180_write(state, ADV7180_REG_RES_CIR, 0xf0);
+		adv7180_write(state, ADV7180_REG_CLAMP_ADJ, 0xd0);
+		adv7180_write(state, ADV7180_REG_DIFF_MODE, 0x10);
+		adv7180_write(state, ADV7180_REG_AGC_ADJ1, 0x9c);
+		adv7180_write(state, ADV7180_REG_AGC_ADJ2, 0x00);
 	}
 
 	return 0;
-- 
1.9.1

Re: [PATCH 05/11] media: adv7180: init chip with AD recommended register settings

[Tim Harvey]

On Wed, Jul 6, 2016 at 3:59 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> Define and load register tables that conform to Analog Device's
> recommended register settings. It loads the default single-ended
> CVBS on Ain1 configuration for both ADV7180 and ADV7182 chips.
>
> New register addresses have been defined for the tables. Those new
> defines are also used in existing locations where hard-coded addresses
> were used.
>
> Note this patch also enables NEWAVMODE, which is also recommended by
> Analog Devices. This will likely break any current backends using this
> subdev that are expecting different or manually configured AV codes.
>
> Note also that bt.656-4 support has been removed in this patch, but it
> will be brought back in a subsequent patch.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/media/i2c/adv7180.c | 168 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 130 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 42816d4..92e2f37 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -56,10 +56,11 @@
>
>  #define ADV7182_REG_INPUT_VIDSEL                       0x0002
>
> +#define ADV7180_REG_OUTPUT_CONTROL                     0x0003
>  #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL            0x0004
>  #define ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS                0xC5
>
> -#define ADV7180_REG_AUTODETECT_ENABLE                  0x07
> +#define ADV7180_REG_AUTODETECT_ENABLE                  0x0007
>  #define ADV7180_AUTODETECT_DEFAULT                     0x7f
>  /* Contrast */
>  #define ADV7180_REG_CON                0x0008  /*Unsigned */
> @@ -100,6 +101,20 @@
>  #define ADV7180_REG_IDENT 0x0011
>  #define ADV7180_ID_7180 0x18
>
> +#define ADV7180_REG_STATUS3            0x0013
> +#define ADV7180_REG_ANALOG_CLAMP_CTL   0x0014
> +#define ADV7180_REG_SHAP_FILTER_CTL_1  0x0017
> +#define ADV7180_REG_CTRL_2             0x001d
> +#define ADV7180_REG_VSYNC_FIELD_CTL_1  0x0031
> +#define ADV7180_REG_MANUAL_WIN_CTL_1   0x003d
> +#define ADV7180_REG_MANUAL_WIN_CTL_2   0x003e
> +#define ADV7180_REG_MANUAL_WIN_CTL_3   0x003f
> +#define ADV7180_REG_LOCK_CNT           0x0051
> +#define ADV7180_REG_CVBS_TRIM          0x0052
> +#define ADV7180_REG_CLAMP_ADJ          0x005a
> +#define ADV7180_REG_RES_CIR            0x005f
> +#define ADV7180_REG_DIFF_MODE          0x0060
> +
>  #define ADV7180_REG_ICONF1             0x2040
>  #define ADV7180_ICONF1_ACTIVE_LOW      0x01
>  #define ADV7180_ICONF1_PSYNC_ONLY      0x10
> @@ -129,9 +144,15 @@
>  #define ADV7180_REG_VPP_SLAVE_ADDR     0xFD
>  #define ADV7180_REG_CSI_SLAVE_ADDR     0xFE
>
> -#define ADV7180_REG_FLCONTROL 0x40e0
> +#define ADV7180_REG_ACE_CTRL1          0x4080
> +#define ADV7180_REG_ACE_CTRL5          0x4084
> +#define ADV7180_REG_FLCONTROL          0x40e0
>  #define ADV7180_FLCONTROL_FL_ENABLE 0x1
>
> +#define ADV7180_REG_RST_CLAMP  0x809c
> +#define ADV7180_REG_AGC_ADJ1   0x80b6
> +#define ADV7180_REG_AGC_ADJ2   0x80c0
> +
>  #define ADV7180_CSI_REG_PWRDN  0x00
>  #define ADV7180_CSI_PWRDN      0x80
>
> @@ -209,6 +230,11 @@ struct adv7180_state {
>                                             struct adv7180_state,       \
>                                             ctrl_hdl)->sd)
>
> +struct adv7180_reg_tbl_t {
> +       unsigned int reg;
> +       unsigned int val;
> +};
> +
>  static int adv7180_select_page(struct adv7180_state *state, unsigned int page)
>  {
>         if (state->register_page != page) {
> @@ -235,6 +261,20 @@ static int adv7180_read(struct adv7180_state *state, unsigned int reg)
>         return i2c_smbus_read_byte_data(state->client, reg & 0xff);
>  }
>
> +static int adv7180_load_reg_tbl(struct adv7180_state *state,
> +                               const struct adv7180_reg_tbl_t *tbl, int n)
> +{
> +       int ret, i;
> +
> +       for (i = 0; i < n; i++) {
> +               ret = adv7180_write(state, tbl[i].reg, tbl[i].val);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  static int adv7180_csi_write(struct adv7180_state *state, unsigned int reg,
>         unsigned int value)
>  {
> @@ -828,19 +868,36 @@ static irqreturn_t adv7180_irq(int irq, void *devid)
>         return IRQ_HANDLED;
>  }
>
> +/*
> + * This register table conforms to Analog Device's Register Settings
> + * Recommendation for the ADV7180. It configures single-ended CVBS
> + * input on Ain1, and enables NEWAVMODE.
> + */
> +static const struct adv7180_reg_tbl_t adv7180_single_ended_cvbs[] = {
> +       /* Set analog mux for CVBS on Ain1 */
> +       { ADV7180_REG_INPUT_CONTROL, 0x00 },
> +       /* ADI Required Write: Reset Clamp Circuitry */
> +       { ADV7180_REG_ANALOG_CLAMP_CTL, 0x30 },
> +       /* Enable SFL Output */
> +       { ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57 },
> +       /* Select SH1 Chroma Shaping Filter */
> +       { ADV7180_REG_SHAP_FILTER_CTL_1, 0x41 },
> +       /* Enable NEWAVMODE */
> +       { ADV7180_REG_VSYNC_FIELD_CTL_1, 0x02 },
> +       /* ADI Required Write: optimize windowing function Step 1,2,3 */
> +       { ADV7180_REG_MANUAL_WIN_CTL_1, 0xA2 },
> +       { ADV7180_REG_MANUAL_WIN_CTL_2, 0x6A },
> +       { ADV7180_REG_MANUAL_WIN_CTL_3, 0xA0 },
> +       /* ADI Required Write: Enable ADC step 1,2,3 */
> +       { 0x8055, 0x81 }, /* undocumented register 0x55 */
> +       /* Recommended AFE I BIAS Setting for CVBS mode */
> +       { ADV7180_REG_CVBS_TRIM, 0x0D },
> +};
> +
>  static int adv7180_init(struct adv7180_state *state)
>  {
> -       int ret;
> -
> -       /* ITU-R BT.656-4 compatible */
> -       ret = adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> -                       ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS);
> -       if (ret < 0)
> -               return ret;
> -
> -       /* Manually set V bit end position in NTSC mode */
> -       return adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
> -                                       ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> +       return adv7180_load_reg_tbl(state, adv7180_single_ended_cvbs,
> +                                   ARRAY_SIZE(adv7180_single_ended_cvbs));
>  }
>
>  static int adv7180_set_std(struct adv7180_state *state, unsigned int std)
> @@ -862,8 +919,48 @@ static int adv7180_select_input(struct adv7180_state *state, unsigned int input)
>         return adv7180_write(state, ADV7180_REG_INPUT_CONTROL, ret);
>  }
>
> +/*
> + * This register table conforms to Analog Device's Register Settings
> + * Recommendation revision C for the ADV7182. It configures single-ended
> + * CVBS inputs on Ain1, and enables NEWAVMODE.
> + */
> +static const struct adv7180_reg_tbl_t adv7182_single_ended_cvbs[] = {
> +       /* Exit Power Down Mode */
> +       { ADV7180_REG_PWR_MAN, 0x00 },
> +       /* Enable ADV7182 for 28.63636 MHz Crystal Clock Input */
> +       { ADV7180_REG_STATUS3, 0x00 },
> +       /* Set optimized IBIAS for single-ended CVBS input */
> +       { ADV7180_REG_CVBS_TRIM, 0xCD },
> +       /* Switch to single-ended CVBS on AIN1 */
> +       { ADV7180_REG_INPUT_CONTROL, 0x00 },
> +       /* ADI Required Write: Reset Current Clamp Circuitry steps 1,2,3,4 */
> +       { ADV7180_REG_RST_CLAMP, 0x00 },
> +       { ADV7180_REG_RST_CLAMP, 0xFF },
> +       /* Select SH1 Chroma Shaping Filter */
> +       { ADV7180_REG_SHAP_FILTER_CTL_1, 0x41 },
> +       /* Enable Pixel & Sync output drivers */
> +       { ADV7180_REG_OUTPUT_CONTROL, 0x0C },
> +       /* Power-up INTRQ, HS and VS/FIELD/SFL pad */
> +       { ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x07 },
> +       /* Enable LLC Output Driver */
> +       { ADV7180_REG_CTRL_2, 0x40 },
> +       /* Optimize ACE Performance */
> +       { ADV7180_REG_ACE_CTRL5, 0x00 },
> +       /* Enable ACE Feature */
> +       { ADV7180_REG_ACE_CTRL1, 0x80 },
> +       /* Enable NEWAVMODE */
> +       { ADV7180_REG_VSYNC_FIELD_CTL_1, 0x02 },
> +};
> +
>  static int adv7182_init(struct adv7180_state *state)
>  {
> +       int ret;
> +
> +       ret = adv7180_load_reg_tbl(state, adv7182_single_ended_cvbs,
> +                                  ARRAY_SIZE(adv7182_single_ended_cvbs));
> +       if (ret)
> +               return ret;
> +
>         if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2)
>                 adv7180_write(state, ADV7180_REG_CSI_SLAVE_ADDR,
>                         ADV7180_DEFAULT_CSI_I2C_ADDR << 1);
> @@ -881,20 +978,15 @@ static int adv7182_init(struct adv7180_state *state)
>
>         /* ADI required writes */
>         if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
> -               adv7180_write(state, 0x0003, 0x4e);
> -               adv7180_write(state, 0x0004, 0x57);
> -               adv7180_write(state, 0x001d, 0xc0);
> +               adv7180_write(state, ADV7180_REG_OUTPUT_CONTROL, 0x4e);
> +               adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57);
> +               adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
>         } else {
>                 if (state->chip_info->flags & ADV7180_FLAG_V2)
> -                       adv7180_write(state, 0x0004, 0x17);
> -               else
> -                       adv7180_write(state, 0x0004, 0x07);
> -               adv7180_write(state, 0x0003, 0x0c);
> -               adv7180_write(state, 0x001d, 0x40);
> +                       adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> +                                     0x17);
>         }
>
> -       adv7180_write(state, 0x0013, 0x00);
> -
>         return 0;
>  }
>
> @@ -967,8 +1059,8 @@ static int adv7182_select_input(struct adv7180_state *state, unsigned int input)
>                 return ret;
>
>         /* Reset clamp circuitry - ADI recommended writes */
> -       adv7180_write(state, 0x809c, 0x00);
> -       adv7180_write(state, 0x809c, 0xff);
> +       adv7180_write(state, ADV7180_REG_RST_CLAMP, 0x00);
> +       adv7180_write(state, ADV7180_REG_RST_CLAMP, 0xff);
>
>         input_type = adv7182_get_input_type(input);
>
> @@ -976,10 +1068,10 @@ static int adv7182_select_input(struct adv7180_state *state, unsigned int input)
>         case ADV7182_INPUT_TYPE_CVBS:
>         case ADV7182_INPUT_TYPE_DIFF_CVBS:
>                 /* ADI recommends to use the SH1 filter */
> -               adv7180_write(state, 0x0017, 0x41);
> +               adv7180_write(state, ADV7180_REG_SHAP_FILTER_CTL_1, 0x41);
>                 break;
>         default:
> -               adv7180_write(state, 0x0017, 0x01);
> +               adv7180_write(state, ADV7180_REG_SHAP_FILTER_CTL_1, 0x01);
>                 break;
>         }
>
> @@ -989,21 +1081,21 @@ static int adv7182_select_input(struct adv7180_state *state, unsigned int input)
>                 lbias = adv7182_lbias_settings[input_type];
>
>         for (i = 0; i < ARRAY_SIZE(adv7182_lbias_settings[0]); i++)
> -               adv7180_write(state, 0x0052 + i, lbias[i]);
> +               adv7180_write(state, ADV7180_REG_CVBS_TRIM + i, lbias[i]);
>
>         if (input_type == ADV7182_INPUT_TYPE_DIFF_CVBS) {
>                 /* ADI required writes to make differential CVBS work */
> -               adv7180_write(state, 0x005f, 0xa8);
> -               adv7180_write(state, 0x005a, 0x90);
> -               adv7180_write(state, 0x0060, 0xb0);
> -               adv7180_write(state, 0x80b6, 0x08);
> -               adv7180_write(state, 0x80c0, 0xa0);
> +               adv7180_write(state, ADV7180_REG_RES_CIR, 0xa8);
> +               adv7180_write(state, ADV7180_REG_CLAMP_ADJ, 0x90);
> +               adv7180_write(state, ADV7180_REG_DIFF_MODE, 0xb0);
> +               adv7180_write(state, ADV7180_REG_AGC_ADJ1, 0x08);
> +               adv7180_write(state, ADV7180_REG_AGC_ADJ2, 0xa0);
>         } else {
> -               adv7180_write(state, 0x005f, 0xf0);
> -               adv7180_write(state, 0x005a, 0xd0);
> -               adv7180_write(state, 0x0060, 0x10);
> -               adv7180_write(state, 0x80b6, 0x9c);
> -               adv7180_write(state, 0x80c0, 0x00);
> +               adv7180_write(state, ADV7180_REG_RES_CIR, 0xf0);
> +               adv7180_write(state, ADV7180_REG_CLAMP_ADJ, 0xd0);
> +               adv7180_write(state, ADV7180_REG_DIFF_MODE, 0x10);
> +               adv7180_write(state, ADV7180_REG_AGC_ADJ1, 0x9c);
> +               adv7180_write(state, ADV7180_REG_AGC_ADJ2, 0x00);
>         }
>
>         return 0;
> --

Steve,

Tested on an IMX6 Gateworks Ventana with IMX6 capture drivers [1].

Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>

Added to Cc:
Cc: Lars-Peter Clausen <lars@metafoo.de>

Also adding Cc's to the people who are using the adv7180 on other
boards (renesas r8a779* boards) so we can get some feedback and/or
Tested-by from them:
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: Simon Horman <horms+renesas@verge.net.au>

I'm wondering if those boards need the bt656-4 which was previously
being enabled per your comment and is now moved to a dt-prop per a
subsequent patch.

Regards,

Tim

[1] - http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/102914

Re: [PATCH 05/11] media: adv7180: init chip with AD recommended register settings

[Lars-Peter Clausen]

On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
> Define and load register tables that conform to Analog Device's
> recommended register settings. It loads the default single-ended
> CVBS on Ain1 configuration for both ADV7180 and ADV7182 chips.

The driver should already setup the recommended configuration based on the
current mode. This patch seems to add a set of register writes that only
apply for a specific mode.

[...]
> Note this patch also enables NEWAVMODE, which is also recommended by
> Analog Devices. This will likely break any current backends using this
> subdev that are expecting different or manually configured AV codes.
> 
> Note also that bt.656-4 support has been removed in this patch, but it
> will be brought back in a subsequent patch.


I'm not sure if we can or should break backwards compatibility in this way.

[PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Steve Longerbeam]

Add a device tree boolean property "bt656-4" to allow setting
the ITU-R BT.656-4 compatible bit.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 92e2f37..fff887c 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -58,7 +58,7 @@
 
 #define ADV7180_REG_OUTPUT_CONTROL			0x0003
 #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL		0x0004
-#define ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS		0xC5
+#define ADV7180_EXTENDED_OUTPUT_CONTROL_BT656_4		0x80
 
 #define ADV7180_REG_AUTODETECT_ENABLE			0x0007
 #define ADV7180_AUTODETECT_DEFAULT			0x7f
@@ -216,6 +216,7 @@ struct adv7180_state {
 	struct gpio_desc	*pwdn_gpio;
 	v4l2_std_id		curr_norm;
 	bool			autodetect;
+	bool			bt656_4; /* use bt.656-4 standard for NTSC */
 	bool			powered;
 	u8			input;
 
@@ -1281,6 +1282,17 @@ static int init_device(struct adv7180_state *state)
 	if (ret)
 		goto out_unlock;
 
+	if (state->bt656_4) {
+		ret = adv7180_read(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL);
+		if (ret < 0)
+			goto out_unlock;
+		ret |= ADV7180_EXTENDED_OUTPUT_CONTROL_BT656_4;
+		ret = adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
+				    ret);
+		if (ret < 0)
+			goto out_unlock;
+	}
+
 	ret = adv7180_program_std(state);
 	if (ret)
 		goto out_unlock;
@@ -1332,6 +1344,10 @@ static int adv7180_of_parse(struct adv7180_state *state)
 		return PTR_ERR(state->pwdn_gpio);
 	}
 
+	/* select ITU-R BT.656-4 compatible? */
+	if (of_property_read_bool(client->dev.of_node, "bt656-4"))
+		state->bt656_4 = true;
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Lars-Peter Clausen]

On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
> Add a device tree boolean property "bt656-4" to allow setting
> the ITU-R BT.656-4 compatible bit.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>

Hi,

Thanks for the patch.

> ---
>  drivers/media/i2c/adv7180.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 92e2f37..fff887c 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -58,7 +58,7 @@
>  
>  #define ADV7180_REG_OUTPUT_CONTROL			0x0003
>  #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL		0x0004
> -#define ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS		0xC5
> +#define ADV7180_EXTENDED_OUTPUT_CONTROL_BT656_4		0x80
>  
>  #define ADV7180_REG_AUTODETECT_ENABLE			0x0007
>  #define ADV7180_AUTODETECT_DEFAULT			0x7f
> @@ -216,6 +216,7 @@ struct adv7180_state {
>  	struct gpio_desc	*pwdn_gpio;
>  	v4l2_std_id		curr_norm;
>  	bool			autodetect;
> +	bool			bt656_4; /* use bt.656-4 standard for NTSC */
>  	bool			powered;
>  	u8			input;
>  
> @@ -1281,6 +1282,17 @@ static int init_device(struct adv7180_state *state)
>  	if (ret)
>  		goto out_unlock;
>  
> +	if (state->bt656_4) {
> +		ret = adv7180_read(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL);
> +		if (ret < 0)
> +			goto out_unlock;
> +		ret |= ADV7180_EXTENDED_OUTPUT_CONTROL_BT656_4;
> +		ret = adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> +				    ret);
> +		if (ret < 0)
> +			goto out_unlock;
> +	}
> +
>  	ret = adv7180_program_std(state);
>  	if (ret)
>  		goto out_unlock;
> @@ -1332,6 +1344,10 @@ static int adv7180_of_parse(struct adv7180_state *state)
>  		return PTR_ERR(state->pwdn_gpio);
>  	}
>  
> +	/* select ITU-R BT.656-4 compatible? */
> +	if (of_property_read_bool(client->dev.of_node, "bt656-4"))
> +		state->bt656_4 = true;

This property needs to be documented. In my opinion it should also be a
property of the endpoint sub-node rather than the toplevel device node since
this is a configuration of the endpoint format.

> +
>  	return 0;
>  }
>  
> 

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Steve Longerbeam]

On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>> Add a device tree boolean property "bt656-4" to allow setting
>> the ITU-R BT.656-4 compatible bit.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>
> +	/* select ITU-R BT.656-4 compatible? */
> +	if (of_property_read_bool(client->dev.of_node, "bt656-4"))
> +		state->bt656_4 = true;
> This property needs to be documented. In my opinion it should also be a
> property of the endpoint sub-node rather than the toplevel device node since
> this is a configuration of the endpoint format.

Agreed, it's really a config of the backend capture endpoint. I'll move it
there and also document it in 
Documentation/devicetree/bindings/media/i2c/adv7180.txt.

Steve

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Steve Longerbeam]

On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>
>
> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>> Add a device tree boolean property "bt656-4" to allow setting
>>> the ITU-R BT.656-4 compatible bit.
>>>
>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>
>> +    /* select ITU-R BT.656-4 compatible? */
>> +    if (of_property_read_bool(client->dev.of_node, "bt656-4"))
>> +        state->bt656_4 = true;
>> This property needs to be documented. In my opinion it should also be a
>> property of the endpoint sub-node rather than the toplevel device 
>> node since
>> this is a configuration of the endpoint format.
>
> Agreed, it's really a config of the backend capture endpoint. I'll 
> move it
> there and also document it in 
> Documentation/devicetree/bindings/media/i2c/adv7180.txt.

er, scratch that. ITU-R BT.656 compatibility is really a property of the 
bt.656 bus. It
should be added to v4l2 endpoints and parsed by 
v4l2_of_parse_endpoint(). I've created
a patch to add a "bt656-mode" property to v4l2 endpoints and will copy 
all the maintainers.

Steve

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Steve Longerbeam]

On 07/09/2016 02:10 PM, Steve Longerbeam wrote:
>
>
> On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>>
>>
>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>>> Add a device tree boolean property "bt656-4" to allow setting
>>>> the ITU-R BT.656-4 compatible bit.
>>>>
>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>
>>> +    /* select ITU-R BT.656-4 compatible? */
>>> +    if (of_property_read_bool(client->dev.of_node, "bt656-4"))
>>> +        state->bt656_4 = true;
>>> This property needs to be documented. In my opinion it should also be a
>>> property of the endpoint sub-node rather than the toplevel device 
>>> node since
>>> this is a configuration of the endpoint format.
>>
>> Agreed, it's really a config of the backend capture endpoint. I'll 
>> move it
>> there and also document it in 
>> Documentation/devicetree/bindings/media/i2c/adv7180.txt.
>
> er, scratch that. ITU-R BT.656 compatibility is really a property of 
> the bt.656 bus. It
> should be added to v4l2 endpoints and parsed by 
> v4l2_of_parse_endpoint(). I've created
> a patch to add a "bt656-mode" property to v4l2 endpoints and will copy 
> all the maintainers.

But I'm going back and forth on whether this property should be added to 
the adv7180's
local endpoint, or to the remote endpoint. I'm leaning towards the 
remote endpoint, since
this is more about how the backend wants the bus configured.

Steve

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Lars-Peter Clausen]

On 07/09/2016 11:36 PM, Steve Longerbeam wrote:
> 
> 
> On 07/09/2016 02:10 PM, Steve Longerbeam wrote:
>>
>>
>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>>>
>>>
>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>>>> Add a device tree boolean property "bt656-4" to allow setting
>>>>> the ITU-R BT.656-4 compatible bit.
>>>>>
>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>
>>>> +    /* select ITU-R BT.656-4 compatible? */
>>>> +    if (of_property_read_bool(client->dev.of_node, "bt656-4"))
>>>> +        state->bt656_4 = true;
>>>> This property needs to be documented. In my opinion it should also be a
>>>> property of the endpoint sub-node rather than the toplevel device node
>>>> since
>>>> this is a configuration of the endpoint format.
>>>
>>> Agreed, it's really a config of the backend capture endpoint. I'll move it
>>> there and also document it in
>>> Documentation/devicetree/bindings/media/i2c/adv7180.txt.
>>
>> er, scratch that. ITU-R BT.656 compatibility is really a property of the
>> bt.656 bus. It
>> should be added to v4l2 endpoints and parsed by v4l2_of_parse_endpoint().
>> I've created
>> a patch to add a "bt656-mode" property to v4l2 endpoints and will copy all
>> the maintainers.
> 
> But I'm going back and forth on whether this property should be added to the
> adv7180's
> local endpoint, or to the remote endpoint. I'm leaning towards the remote
> endpoint, since
> this is more about how the backend wants the bus configured.

I think it should be set for both, as both endpoints need to agree on the
format.

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Hans Verkuil]

On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:
> On 07/09/2016 11:36 PM, Steve Longerbeam wrote:
>>
>>
>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote:
>>>
>>>
>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>>>>
>>>>
>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>>>>> Add a device tree boolean property "bt656-4" to allow setting
>>>>>> the ITU-R BT.656-4 compatible bit.
>>>>>>
>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>
>>>>> +    /* select ITU-R BT.656-4 compatible? */

Please don't use the '-4': BT.656 is sufficient. The -4 is just the version
number of the standard (and 5 is the latest anyway).

>>>>> +    if (of_property_read_bool(client->dev.of_node, "bt656-4"))
>>>>> +        state->bt656_4 = true;
>>>>> This property needs to be documented. In my opinion it should also be a
>>>>> property of the endpoint sub-node rather than the toplevel device node
>>>>> since
>>>>> this is a configuration of the endpoint format.
>>>>
>>>> Agreed, it's really a config of the backend capture endpoint. I'll move it
>>>> there and also document it in
>>>> Documentation/devicetree/bindings/media/i2c/adv7180.txt.
>>>
>>> er, scratch that. ITU-R BT.656 compatibility is really a property of the
>>> bt.656 bus. It
>>> should be added to v4l2 endpoints and parsed by v4l2_of_parse_endpoint().
>>> I've created
>>> a patch to add a "bt656-mode" property to v4l2 endpoints and will copy all
>>> the maintainers.
>>
>> But I'm going back and forth on whether this property should be added to the
>> adv7180's
>> local endpoint, or to the remote endpoint. I'm leaning towards the remote
>> endpoint, since
>> this is more about how the backend wants the bus configured.
> 
> I think it should be set for both, as both endpoints need to agree on the
> format.

Is it needed at all? Setting the polarity flags to H/VSYNC_ACTIVE_HIGH/LOW
will already signal BT.656 mode. See include/media/v4l2-mediabus.h and
v4l2-of.c.

I haven't looked in detail at adv7180, so I may have missed something, but this
looks strange.

Regards,

	Hans

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Ian Arkver]

On 10/07/16 13:55, Hans Verkuil wrote:
> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:
>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote:
>>>
>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote:
>>>>
>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>>>>>
>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>>>>>> Add a device tree boolean property "bt656-4" to allow setting
>>>>>>> the ITU-R BT.656-4 compatible bit.
>>>>>>>
>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>> +    /* select ITU-R BT.656-4 compatible? */
> Please don't use the '-4': BT.656 is sufficient. The -4 is just the version
> number of the standard (and 5 is the latest anyway).
 From the ADV7180 DS [1]:

BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7]

Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU 
has changed the toggling position for the V bit within the SAV EAV codes 
for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an 
output mode that is compliant with either the previous or new standard. 
For further information, visit the International Telecommunication Union 
website.

Note that the standard change only affects NTSC and has no bearing on PAL.

When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is 
used. The V bit goes low at EAV of Line 10 and Line 273.

When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The 
V bit goes low at EAV of Line 20 and Line 283.

[1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf 

>
>>>>>> +    if (of_property_read_bool(client->dev.of_node, "bt656-4"))
>>>>>> +        state->bt656_4 = true;
>>>>>> This property needs to be documented. In my opinion it should also be a
>>>>>> property of the endpoint sub-node rather than the toplevel device node
>>>>>> since
>>>>>> this is a configuration of the endpoint format.
>>>>> Agreed, it's really a config of the backend capture endpoint. I'll move it
>>>>> there and also document it in
>>>>> Documentation/devicetree/bindings/media/i2c/adv7180.txt.
>>>> er, scratch that. ITU-R BT.656 compatibility is really a property of the
>>>> bt.656 bus. It
>>>> should be added to v4l2 endpoints and parsed by v4l2_of_parse_endpoint().
>>>> I've created
>>>> a patch to add a "bt656-mode" property to v4l2 endpoints and will copy all
>>>> the maintainers.
>>> But I'm going back and forth on whether this property should be added to the
>>> adv7180's
>>> local endpoint, or to the remote endpoint. I'm leaning towards the remote
>>> endpoint, since
>>> this is more about how the backend wants the bus configured.
>> I think it should be set for both, as both endpoints need to agree on the
>> format.
> Is it needed at all? Setting the polarity flags to H/VSYNC_ACTIVE_HIGH/LOW
> will already signal BT.656 mode. See include/media/v4l2-mediabus.h and
> v4l2-of.c.
>
> I haven't looked in detail at adv7180, so I may have missed something, but this
> looks strange.
>
> Regards,
>
> 	Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Hans Verkuil]

On 07/10/2016 04:17 PM, Ian Arkver wrote:
> On 10/07/16 13:55, Hans Verkuil wrote:
>> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:
>>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote:
>>>>
>>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote:
>>>>>
>>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>>>>>>
>>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>>>>>>> Add a device tree boolean property "bt656-4" to allow setting
>>>>>>>> the ITU-R BT.656-4 compatible bit.
>>>>>>>>
>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>> +    /* select ITU-R BT.656-4 compatible? */
>> Please don't use the '-4': BT.656 is sufficient. The -4 is just the version
>> number of the standard (and 5 is the latest anyway).
>  From the ADV7180 DS [1]:
> 
> BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7]
> 
> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU 
> has changed the toggling position for the V bit within the SAV EAV codes 
> for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an 
> output mode that is compliant with either the previous or new standard. 
> For further information, visit the International Telecommunication Union 
> website.
> 
> Note that the standard change only affects NTSC and has no bearing on PAL.
> 
> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is 
> used. The V bit goes low at EAV of Line 10 and Line 273.
> 
> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The 
> V bit goes low at EAV of Line 20 and Line 283.
> 
> [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf 

Rev 4 came in in 1998. I would say that that is the default. We have had any
problems with this and I would need some proof that this is really needed.

Regards,

	Hans

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Steve Longerbeam]

On 07/10/2016 07:30 AM, Hans Verkuil wrote:
> On 07/10/2016 04:17 PM, Ian Arkver wrote:
>> On 10/07/16 13:55, Hans Verkuil wrote:
>>> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:
>>>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote:
>>>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote:
>>>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>>>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>>>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>>>>>>>> Add a device tree boolean property "bt656-4" to allow setting
>>>>>>>>> the ITU-R BT.656-4 compatible bit.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>>> +    /* select ITU-R BT.656-4 compatible? */
>>> Please don't use the '-4': BT.656 is sufficient. The -4 is just the version
>>> number of the standard (and 5 is the latest anyway).
>>   From the ADV7180 DS [1]:
>>
>> BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7]
>>
>> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU
>> has changed the toggling position for the V bit within the SAV EAV codes
>> for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an
>> output mode that is compliant with either the previous or new standard.
>> For further information, visit the International Telecommunication Union
>> website.
>>
>> Note that the standard change only affects NTSC and has no bearing on PAL.
>>
>> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is
>> used. The V bit goes low at EAV of Line 10 and Line 273.
>>
>> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The
>> V bit goes low at EAV of Line 20 and Line 283.
>>
>> [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf
> Rev 4 came in in 1998. I would say that that is the default. We have had any
> problems with this and I would need some proof that this is really needed.

Hi Hans, yeah I agree with you that new capture drivers should not
expect BT.656-3 compatible buss' any longer. The problem with i.MX6
however, is that the IPU CSI CCIR_CODE registers, which inform the IPU
about the AV code positions, are very poorly documented. In order for
i.MX6 to support BT.656-4 it would require very time-consuming reverse
engineering to find the right values to plug into those registers to conform
to the BT.656-4 AV code positions.

Steve

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Ian Arkver]

On 10/07/16 23:34, Steve Longerbeam wrote:
>
>
> On 07/10/2016 07:30 AM, Hans Verkuil wrote:
>> On 07/10/2016 04:17 PM, Ian Arkver wrote:
>>> On 10/07/16 13:55, Hans Verkuil wrote:
>>>> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:
>>>>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote:
>>>>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote:
>>>>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>>>>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>>>>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>>>>>>>>> Add a device tree boolean property "bt656-4" to allow setting
>>>>>>>>>> the ITU-R BT.656-4 compatible bit.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>>>> +    /* select ITU-R BT.656-4 compatible? */
>>>> Please don't use the '-4': BT.656 is sufficient. The -4 is just the 
>>>> version
>>>> number of the standard (and 5 is the latest anyway).
>>>   From the ADV7180 DS [1]:
>>>
>>> BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7]
>>>
>>> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the 
>>> ITU
>>> has changed the toggling position for the V bit within the SAV EAV 
>>> codes
>>> for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an
>>> output mode that is compliant with either the previous or new standard.
>>> For further information, visit the International Telecommunication 
>>> Union
>>> website.
>>>
>>> Note that the standard change only affects NTSC and has no bearing 
>>> on PAL.
>>>
>>> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is
>>> used. The V bit goes low at EAV of Line 10 and Line 273.
>>>
>>> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The
>>> V bit goes low at EAV of Line 20 and Line 283.
>>>
>>> [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf 
>>>
>> Rev 4 came in in 1998. I would say that that is the default. We have 
>> had any
>> problems with this and I would need some proof that this is really 
>> needed.
>
> Hi Hans, yeah I agree with you that new capture drivers should not
> expect BT.656-3 compatible buss' any longer. The problem with i.MX6
> however, is that the IPU CSI CCIR_CODE registers, which inform the IPU
> about the AV code positions, are very poorly documented. In order for
> i.MX6 to support BT.656-4 it would require very time-consuming reverse
> engineering to find the right values to plug into those registers to 
> conform
> to the BT.656-4 AV code positions.
>
> Steve
>
Hi Steve,

Once you dsicover that the 3-bit fields in each CCIR_CODE register 
correspond to the H, V and F flags in the SAV/EAV code (in that order, 
MSbit->LSbit),  those registers make more sense. Pity that's not 
mentioned in the Ref Manual.

However, I don't think that's relevant here, since the spec revision 
didn't change the codes, but just moved the lines the V bit is sent on. 
I believe the spec change switched the NTSC timing from VSYNC to VBLANK, 
but the net effect was 10 fewer "active" video lines per field. Looking 
at a sample of one other video decoder (tvp5150), the default seems to 
be to change V on lines 20 and 283, as per the newer revision of the 
spec., though again bets may have been hedged with a programmable override.

In any case, I'm wondering if your extra ten lines per field are more 
related to this snippet from calc_default_crop in imx-camif.c, which 
seems like it would break other decoder front ends and adv7180 in 
bt656-4 mode...

     /*
      * FIXME: For NTSC standards, top must be set to an
      * offset of 13 lines to match fixed CCIR programming
      * in the IPU.
      */
     if (std != V4L2_STD_UNKNOWN && (std & V4L2_STD_525_60))
         rect->top = 13;

I believe tvp5150 at least sends 243 active lines per field for an NTSC 
source and the top 3 lines are generally dropped.

Regards,
Ian

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Steve Longerbeam]

On 07/11/2016 12:06 AM, Ian Arkver wrote:
> On 10/07/16 23:34, Steve Longerbeam wrote:
>>
>>
>> On 07/10/2016 07:30 AM, Hans Verkuil wrote:
>>> On 07/10/2016 04:17 PM, Ian Arkver wrote:
>>>> On 10/07/16 13:55, Hans Verkuil wrote:
>>>>> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:
>>>>>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote:
>>>>>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote:
>>>>>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>>>>>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>>>>>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>>>>>>>>>> Add a device tree boolean property "bt656-4" to allow setting
>>>>>>>>>>> the ITU-R BT.656-4 compatible bit.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>>>>> +    /* select ITU-R BT.656-4 compatible? */
>>>>> Please don't use the '-4': BT.656 is sufficient. The -4 is just the version
>>>>> number of the standard (and 5 is the latest anyway).
>>>>   From the ADV7180 DS [1]:
>>>>
>>>> BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7]
>>>>
>>>> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU
>>>> has changed the toggling position for the V bit within the SAV EAV codes
>>>> for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an
>>>> output mode that is compliant with either the previous or new standard.
>>>> For further information, visit the International Telecommunication Union
>>>> website.
>>>>
>>>> Note that the standard change only affects NTSC and has no bearing on PAL.
>>>>
>>>> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is
>>>> used. The V bit goes low at EAV of Line 10 and Line 273.
>>>>
>>>> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The
>>>> V bit goes low at EAV of Line 20 and Line 283.
>>>>
>>>> [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf
>>> Rev 4 came in in 1998. I would say that that is the default. We have had any
>>> problems with this and I would need some proof that this is really needed.
>>
>> Hi Hans, yeah I agree with you that new capture drivers should not
>> expect BT.656-3 compatible buss' any longer. The problem with i.MX6
>> however, is that the IPU CSI CCIR_CODE registers, which inform the IPU
>> about the AV code positions, are very poorly documented. In order for
>> i.MX6 to support BT.656-4 it would require very time-consuming reverse
>> engineering to find the right values to plug into those registers to conform
>> to the BT.656-4 AV code positions.
>>
>> Steve
>>
> Hi Steve,
>
> Once you dsicover that the 3-bit fields in each CCIR_CODE register correspond to the H, V and F flags in the SAV/EAV code (in that order, MSbit->LSbit),  those registers make more sense. Pity that's
> not mentioned in the Ref Manual.

Hi Ian, that's a plausible theory, but it doesn't work. I looked at the value
programmed to CCIR_CODE_1 register (according to imx6 ref manual is
values for first field), for NTSC : 0xD07DF.  Comparing with the definition of
the H/V/F bits in the AV codes in the bt.656 spec:

F = 1 during field 2, 0 during field 1
V = 1 during field blanking, 0 elsewhere
H = 1 in EAV, 0 in SAV

There's no correspondence, for example F bit should be zero everywhere in
CCIR_CODE_1. And wutz this "first/second blanking line command" stuff about?
None of it makes any sense to me.

> However, I don't think that's relevant here, since the spec revision didn't change the codes, but just moved the lines the V bit is sent on. I believe the spec change switched the NTSC timing from
> VSYNC to VBLANK, but the net effect was 10 fewer "active" video lines per field. Looking at a sample of one other video decoder (tvp5150), the default seems to be to change V on lines 20 and 283, as
> per the newer revision of the spec., though again bets may have been hedged with a programmable override.
>
> In any case, I'm wondering if your extra ten lines per field are more related to this snippet from calc_default_crop in imx-camif.c, which seems like it would break other decoder front ends and
> adv7180 in bt656-4 mode...
>
>     /*
>      * FIXME: For NTSC standards, top must be set to an
>      * offset of 13 lines to match fixed CCIR programming
>      * in the IPU.
>      */
>     if (std != V4L2_STD_UNKNOWN && (std & V4L2_STD_525_60))
>         rect->top = 13;

could be. I'll try removing that FIXME block and try with bt.656-4 mode.

Steve

>
> I believe tvp5150 at least sends 243 active lines per field for an NTSC source and the top 3 lines are generally dropped.
>
> Regards,
> Ian 

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Ian Arkver]

This conversation has drifted off topic, sorry.
It now relates to code in drivers/gpu/ipu-v3/ipu-csi.c, so adding 
Philipp Zabel.

On 11/07/16 23:03, Steve Longerbeam wrote:
> On 07/11/2016 12:06 AM, Ian Arkver wrote:
>> On 10/07/16 23:34, Steve Longerbeam wrote:
>>>
>>> On 07/10/2016 07:30 AM, Hans Verkuil wrote:
>>>> On 07/10/2016 04:17 PM, Ian Arkver wrote:
>>>>> On 10/07/16 13:55, Hans Verkuil wrote:
>>>>>> On 07/10/2016 02:10 PM, Lars-Peter Clausen wrote:
>>>>>>> On 07/09/2016 11:36 PM, Steve Longerbeam wrote:
>>>>>>>> On 07/09/2016 02:10 PM, Steve Longerbeam wrote:
>>>>>>>>> On 07/09/2016 11:59 AM, Steve Longerbeam wrote:
>>>>>>>>>> On 07/07/2016 07:52 AM, Lars-Peter Clausen wrote:
>>>>>>>>>>> On 07/07/2016 12:59 AM, Steve Longerbeam wrote:
>>>>>>>>>>>> Add a device tree boolean property "bt656-4" to allow setting
>>>>>>>>>>>> the ITU-R BT.656-4 compatible bit.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>>>>>> +    /* select ITU-R BT.656-4 compatible? */
>>>>>> Please don't use the '-4': BT.656 is sufficient. The -4 is just the version
>>>>>> number of the standard (and 5 is the latest anyway).
>>>>>    From the ADV7180 DS [1]:
>>>>>
>>>>> BT.656-4, ITU-R BT.656-4 Enable, Address 0x04[7]
>>>>>
>>>>> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, the ITU
>>>>> has changed the toggling position for the V bit within the SAV EAV codes
>>>>> for NTSC. The ITU-R BT.656-4 standard bit allows the user to select an
>>>>> output mode that is compliant with either the previous or new standard.
>>>>> For further information, visit the International Telecommunication Union
>>>>> website.
>>>>>
>>>>> Note that the standard change only affects NTSC and has no bearing on PAL.
>>>>>
>>>>> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 specification is
>>>>> used. The V bit goes low at EAV of Line 10 and Line 273.
>>>>>
>>>>> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is used. The
>>>>> V bit goes low at EAV of Line 20 and Line 283.
>>>>>
>>>>> [1]www.analog.com/media/en/technical-documentation/data-sheets/*ADV7180*.pdf
>>>> Rev 4 came in in 1998. I would say that that is the default. We have had any
>>>> problems with this and I would need some proof that this is really needed.
>>> Hi Hans, yeah I agree with you that new capture drivers should not
>>> expect BT.656-3 compatible buss' any longer. The problem with i.MX6
>>> however, is that the IPU CSI CCIR_CODE registers, which inform the IPU
>>> about the AV code positions, are very poorly documented. In order for
>>> i.MX6 to support BT.656-4 it would require very time-consuming reverse
>>> engineering to find the right values to plug into those registers to conform
>>> to the BT.656-4 AV code positions.
>>>
>>> Steve
>>>
>> Hi Steve,
>>
>> Once you dsicover that the 3-bit fields in each CCIR_CODE register correspond to the H, V and F flags in the SAV/EAV code (in that order, MSbit->LSbit),  those registers make more sense. Pity that's
>> not mentioned in the Ref Manual.
> Hi Ian, that's a plausible theory, but it doesn't work. I looked at the value
> programmed to CCIR_CODE_1 register (according to imx6 ref manual is
> values for first field), for NTSC : 0xD07DF.  Comparing with the definition of
> the H/V/F bits in the AV codes in the bt.656 spec:
>
> F = 1 during field 2, 0 during field 1
> V = 1 during field blanking, 0 elsewhere
> H = 1 in EAV, 0 in SAV
>
> There's no correspondence, for example F bit should be zero everywhere in
> CCIR_CODE_1. And wutz this "first/second blanking line command" stuff about?
> None of it makes any sense to me.
Hi,

d07df = 001 101 xxxx 011 111 011 111

                           H V F
CSI0_STRT_FLD0_ACTV       0 0 1
CSI0_END_FLD0_ACTV        1 0 1
CSI0_STRT_FLD0_BLNK_2ND   0 1 1
CSI0_END_FLD0_BLNK_2ND    1 1 1
CSI0_STRT_FLD0_BLNK_1ST   0 1 1
CSI0_END_FLD0_BLNK_1ST    1 1 1

40596 = 000 100 xxxx 010 110 010 110
405A6 = 000 100 xxxx 010 110 100 110

                           H V F
CSI0_STRT_FLD1_ACTV       0 0 0
CSI0_END_FLD1_ACTV        1 0 0
CSI0_STRT_FLD1_BLNK_2ND   0 1 0
CSI0_END_FLD1_BLNK_2ND    1 1 0
CSI0_STRT_FLD1_BLNK_1ST   0 1 0    or 1 0 0 ?
CSI0_END_FLD1_BLNK_1ST    1 1 0

For PAL:  IPU_CSI0_CCIR_CODE_1 = 0x40596, IPU_CSI0_CCIR_CODE_1 = 0xd07df
For NTSC: IPU_CSI0_CCIR_CODE_1 = 0xd07df, IPU_CSI0_CCIR_CODE_1 = 0x405A6

So, for the PAL case the field values make sense to me. The F bit is 
constant throughout each field.
I'm not sure why the NTSC case has 405A6 instead of 40596 again. This 
looks like a bug to me.
If you look back at the original Freescale capture driver[1], they use 
40596 for both PAL and NTSC.

The values are swapped between NTSC and PAL to account for the differing 
field order. I think using
the capture width and height to do this is poor as any "bt.656-like" 
source with non standard
dimensions will end up in the dev_err("Unsupported") case. Also, looking 
at BT.1120-8, for interlaced
video the same SAV and EAV codes as PAL are used, so 
IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR and
IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_DDR modes are currently broken.

I think the 1st and 2nd blanking values might be needed for some odd non 
656 streams maybe? Also for
progressive (originally) the value 40030 is used which is

40030 = 000 100 xxxx 000 000 110 000

This leaves the 2nd blank values empty. In the HW I'd guess both fire on 
SAV but there's a fixed
event priority in the state machine, or it ignores events where both 
STRT and END fire on the same
clock tick.

btw: The patch I sent you a while back[2] dumps the CCIR values in octal 
which makes them
a bit more readable. It also fixes up theCSI0_STRT_FLD0_BLNK_1ST value 
for progressive bt.656 to
be {0 1 0}, which matches what's seen in the stream. In theory the 
blanking end value should
be {1 1 0}, but it seems to work OK with the same code as unblanked SAV 
{0 0 0}. If it's useful
I can rebase and post that patch to the ML.

Maybe I should create an RFC patch for ipu-csi.c and stop hijacking this 
thread?

1: 
https://github.com/Freescale/linux-fslc/blob/3.14-1.1.x-imx/drivers/mxc/ipu3/ipu_capture.c#L168
2: 0003-ipu_csi-more-debug.-Fix-Blanking-SAV-for-progressive.patch


>
>> However, I don't think that's relevant here, since the spec revision didn't change the codes, but just moved the lines the V bit is sent on. I believe the spec change switched the NTSC timing from
>> VSYNC to VBLANK, but the net effect was 10 fewer "active" video lines per field. Looking at a sample of one other video decoder (tvp5150), the default seems to be to change V on lines 20 and 283, as
>> per the newer revision of the spec., though again bets may have been hedged with a programmable override.
>>
>> In any case, I'm wondering if your extra ten lines per field are more related to this snippet from calc_default_crop in imx-camif.c, which seems like it would break other decoder front ends and
>> adv7180 in bt656-4 mode...
>>
>>      /*
>>       * FIXME: For NTSC standards, top must be set to an
>>       * offset of 13 lines to match fixed CCIR programming
>>       * in the IPU.
>>       */
>>      if (std != V4L2_STD_UNKNOWN && (std & V4L2_STD_525_60))
>>          rect->top = 13;
> could be. I'll try removing that FIXME block and try with bt.656-4 mode.
>
> Steve
>
>> I believe tvp5150 at least sends 243 active lines per field for an NTSC source and the top 3 lines are generally dropped.
>>
>> Regards,
>> Ian

All the best,
Ian

Re: [PATCH 06/11] media: adv7180: add bt.656-4 OF property

[Steve Longerbeam]

Hi Ian,

On 07/12/2016 03:25 AM, Ian Arkver wrote:
> This conversation has drifted off topic, sorry.
> It now relates to code in drivers/gpu/ipu-v3/ipu-csi.c, so adding Philipp Zabel.
>
<snip>
> On 11/07/16 23:03, Steve Longerbeam wrote:
>> On 07/11/2016 12:06 AM, Ian Arkver wrote:
>>>
>>> Hi Steve,
>>>
>>> Once you dsicover that the 3-bit fields in each CCIR_CODE register correspond to the H, V and F flags in the SAV/EAV code (in that order, MSbit->LSbit),  those registers make more sense. Pity that's
>>> not mentioned in the Ref Manual.
>> Hi Ian, that's a plausible theory, but it doesn't work. I looked at the value
>> programmed to CCIR_CODE_1 register (according to imx6 ref manual is
>> values for first field), for NTSC : 0xD07DF.  Comparing with the definition of
>> the H/V/F bits in the AV codes in the bt.656 spec:
>>
>> F = 1 during field 2, 0 during field 1
>> V = 1 during field blanking, 0 elsewhere
>> H = 1 in EAV, 0 in SAV
>>
>> There's no correspondence, for example F bit should be zero everywhere in
>> CCIR_CODE_1. And wutz this "first/second blanking line command" stuff about?
>> None of it makes any sense to me.
> Hi,
>
> d07df = 001 101 xxxx 011 111 011 111

Oops, I was misreading the register fields in the ref manual.
I misread the reserved field starting at bit 12 as 3 bits long, but
it is 4 bits long. The H,V,F bits for those CCIR_CODE values make
sense now.

So this clears up a lot for me. Due to confusion surrounding these
registers, one of my theories as to what these registers were doing
was that they were telling the CSI actually where to look for the SAV/EAV
codes in the stream, as a line number or something. But I see now that
the registers are simply telling the IPU about the standard, so they are
more like "set it and forget it" registers, and would only be set to something
other than what the standard specifies in order to deal with non-standard
streams (like adv718x "NEWAVMODE", see below).

So the IPU must be detecting the AV codes using the AV code preamble,
which is a good thing.

Also, it clears up how to deal with bt.656-3 vs. bt.656-4 in the imx6 backend a bit
more. The CCIR code registers do not need to be touched when switching between
bt.656-3 and bt.656-4. It's more a matter of the number of active lines the decoder
sends (bt.656-3 has 10 extra active lines).

>
>                           H V F
> CSI0_STRT_FLD0_ACTV       0 0 1
> CSI0_END_FLD0_ACTV        1 0 1
> CSI0_STRT_FLD0_BLNK_2ND   0 1 1
> CSI0_END_FLD0_BLNK_2ND    1 1 1
> CSI0_STRT_FLD0_BLNK_1ST   0 1 1
> CSI0_END_FLD0_BLNK_1ST    1 1 1
>
> 40596 = 000 100 xxxx 010 110 010 110
> 405A6 = 000 100 xxxx 010 110 100 110
>
>                           H V F
> CSI0_STRT_FLD1_ACTV       0 0 0
> CSI0_END_FLD1_ACTV        1 0 0
> CSI0_STRT_FLD1_BLNK_2ND   0 1 0
> CSI0_END_FLD1_BLNK_2ND    1 1 0
> CSI0_STRT_FLD1_BLNK_1ST   0 1 0    or 1 0 0 ?
> CSI0_END_FLD1_BLNK_1ST    1 1 0
>
> For PAL:  IPU_CSI0_CCIR_CODE_1 = 0x40596, IPU_CSI0_CCIR_CODE_1 = 0xd07df
> For NTSC: IPU_CSI0_CCIR_CODE_1 = 0xd07df, IPU_CSI0_CCIR_CODE_1 = 0x405A6
>
> So, for the PAL case the field values make sense to me. The F bit is constant throughout each field.
> I'm not sure why the NTSC case has 405A6 instead of 40596 again. This looks like a bug to me.

Actually we made this change to deal with "NEWAVMODE" setting in the adv7180. But now
I see that NEWAVMODE breaks the bt.656 standard, and will create problems for other backends.

So I will remove the NEWAVMODE patch to adv7180, and revert the patch that sets the above
0x405A6 value in IPU_CSI0_CCIR_CODE_1.


> If you look back at the original Freescale capture driver[1], they use 40596 for both PAL and NTSC.
>
> The values are swapped between NTSC and PAL to account for the differing field order. I think using
> the capture width and height to do this is poor as any "bt.656-like" source with non standard
> dimensions will end up in the dev_err("Unsupported") case.

yes this code was inherited originally from Freescale. There's needs to be a better
API for that.

> Also, looking at BT.1120-8, for interlaced
> video the same SAV and EAV codes as PAL are used, so IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_SDR and
> IPU_CSI_CLK_MODE_CCIR1120_INTERLACED_DDR modes are currently broken.

right, I haven't even touched bt.1120 support. I don't have any h/w to test.

>
> I think the 1st and 2nd blanking values might be needed for some odd non 656 streams maybe?
> Also for progressive (originally) the value 40030 is used which is
>
> 40030 = 000 100 xxxx 000 000 110 000
>
> This leaves the 2nd blank values empty. In the HW I'd guess both fire on SAV but there's a fixed
> event priority in the state machine, or it ignores events where both STRT and END fire on the same
> clock tick.

Maybe the "1st" and "2nd" blanking fields just provide alternative matches
for the received AV codes.


>
> btw: The patch I sent you a while back[2] dumps the CCIR values in octal which makes them
> a bit more readable. It also fixes up theCSI0_STRT_FLD0_BLNK_1ST value for progressive bt.656 to
> be {0 1 0}, which matches what's seen in the stream. In theory the blanking end value should
> be {1 1 0}, but it seems to work OK with the same code as unblanked SAV {0 0 0}. If it's useful
> I can rebase and post that patch to the ML.
>
> Maybe I should create an RFC patch for ipu-csi.c and stop hijacking this thread?

Sure please do.

>
> 1: https://github.com/Freescale/linux-fslc/blob/3.14-1.1.x-imx/drivers/mxc/ipu3/ipu_capture.c#L168
> 2: 0003-ipu_csi-more-debug.-Fix-Blanking-SAV-for-progressive.patch
>
>
>>
>>> However, I don't think that's relevant here, since the spec revision didn't change the codes, but just moved the lines the V bit is sent on. I believe the spec change switched the NTSC timing from
>>> VSYNC to VBLANK, but the net effect was 10 fewer "active" video lines per field. Looking at a sample of one other video decoder (tvp5150), the default seems to be to change V on lines 20 and 283, as
>>> per the newer revision of the spec., though again bets may have been hedged with a programmable override.
>>>
>>> In any case, I'm wondering if your extra ten lines per field are more related to this snippet from calc_default_crop in imx-camif.c, which seems like it would break other decoder front ends and
>>> adv7180 in bt656-4 mode...
>>>
>>>      /*
>>>       * FIXME: For NTSC standards, top must be set to an
>>>       * offset of 13 lines to match fixed CCIR programming
>>>       * in the IPU.
>>>       */
>>>      if (std != V4L2_STD_UNKNOWN && (std & V4L2_STD_525_60))
>>>          rect->top = 13;
>> could be. I'll try removing that FIXME block and try with bt.656-4 mode.

Setting rect->top = 3 with bt.656-4 produces stable video. And I will fix
the FIXME comment, which reflects my confusion about the CCIR code registers.

Steve
 

[PATCH 07/11] media: adv7180: change mbus format to UYVY

[Steve Longerbeam]

Change the media bus format from YUYV8_2X8 to UYVY8_2X8. Colors
now look correct when capturing with the i.mx6 backend. The other
option is to set the SWPC bit in register 0x27 to swap the Cr and Cb
output samples.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index fff887c..427695d 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -654,7 +654,7 @@ static int adv7180_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index != 0)
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_YUYV8_2X8;
+	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
 
 	return 0;
 }
@@ -664,7 +664,7 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
 {
 	struct adv7180_state *state = to_state(sd);
 
-	fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
+	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
 	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
 	fmt->width = 720;
 	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
-- 
1.9.1

Re: [PATCH 07/11] media: adv7180: change mbus format to UYVY

[Lars-Peter Clausen]

On 07/07/2016 01:00 AM, Steve Longerbeam wrote:
> Change the media bus format from YUYV8_2X8 to UYVY8_2X8. Colors
> now look correct when capturing with the i.mx6 backend. The other
> option is to set the SWPC bit in register 0x27 to swap the Cr and Cb
> output samples.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>

The patch is certainly correct from the technical point of view. But we need
to be careful not to break any existing platforms which rely on this
setting. So the alternative solution of changing the default output order is
not an option.

Looking at things it seems like the Renesas vin driver, which is used in
combination with the adv7180 on some boards, uses the return value from
enum_mbus_code to setup the video pipeline. Adding Niklas to Cc, maybe he
can help to test this.

But otherwise

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
>  drivers/media/i2c/adv7180.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index fff887c..427695d 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -654,7 +654,7 @@ static int adv7180_enum_mbus_code(struct v4l2_subdev *sd,
>  	if (code->index != 0)
>  		return -EINVAL;
>  
> -	code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
>  
>  	return 0;
>  }
> @@ -664,7 +664,7 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
>  {
>  	struct adv7180_state *state = to_state(sd);
>  
> -	fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
>  	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>  	fmt->width = 720;
>  	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> 

Re: [PATCH 07/11] media: adv7180: change mbus format to UYVY

[Niklas Söderlund]

On 2016-07-07 17:18:25 +0200, Lars-Peter Clausen wrote:
> On 07/07/2016 01:00 AM, Steve Longerbeam wrote:
> > Change the media bus format from YUYV8_2X8 to UYVY8_2X8. Colors
> > now look correct when capturing with the i.mx6 backend. The other
> > option is to set the SWPC bit in register 0x27 to swap the Cr and Cb
> > output samples.
> > 
> > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> 
> The patch is certainly correct from the technical point of view. But we need
> to be careful not to break any existing platforms which rely on this
> setting. So the alternative solution of changing the default output order is
> not an option.
> 
> Looking at things it seems like the Renesas vin driver, which is used in
> combination with the adv7180 on some boards, uses the return value from
> enum_mbus_code to setup the video pipeline. Adding Niklas to Cc, maybe he
> can help to test this.

Yes this change will make the rcar-vin driver fail to probe since it do 
not recognise the MEDIA_BUS_FMT_UYVY8_2X8 format.

There is a error in the Renesas VIN driver where it looks for the wrong 
media format YUVU but expects UYVY data. This error have masked the bug 
in adv7180 fixed in this commit.

I have have sent a patch '[media] rcar-vin: add legacy mode for wrong 
media bus formats' which tries to remedy this. I would like to see that 
patched (or similar solution) merged before this one as to not break the 
Renesas R-Car2 Koelsch board which uses the adv7180.

If that can be arranged

Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> But otherwise
> 
> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> > ---
> >  drivers/media/i2c/adv7180.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > index fff887c..427695d 100644
> > --- a/drivers/media/i2c/adv7180.c
> > +++ b/drivers/media/i2c/adv7180.c
> > @@ -654,7 +654,7 @@ static int adv7180_enum_mbus_code(struct v4l2_subdev *sd,
> >  	if (code->index != 0)
> >  		return -EINVAL;
> >  
> > -	code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> > +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> >  
> >  	return 0;
> >  }
> > @@ -664,7 +664,7 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
> >  {
> >  	struct adv7180_state *state = to_state(sd);
> >  
> > -	fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
> > +	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
> >  	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> >  	fmt->width = 720;
> >  	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> > 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 07/11] media: adv7180: change mbus format to UYVY

[Tim Harvey]

On Wed, Jul 6, 2016 at 4:00 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> Change the media bus format from YUYV8_2X8 to UYVY8_2X8. Colors
> now look correct when capturing with the i.mx6 backend. The other
> option is to set the SWPC bit in register 0x27 to swap the Cr and Cb
> output samples.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/media/i2c/adv7180.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index fff887c..427695d 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -654,7 +654,7 @@ static int adv7180_enum_mbus_code(struct v4l2_subdev *sd,
>         if (code->index != 0)
>                 return -EINVAL;
>
> -       code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +       code->code = MEDIA_BUS_FMT_UYVY8_2X8;
>
>         return 0;
>  }
> @@ -664,7 +664,7 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
>  {
>         struct adv7180_state *state = to_state(sd);
>
> -       fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +       fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
>         fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>         fmt->width = 720;
>         fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> --

Steve,

Tested on an IMX6 Gateworks Ventana with IMX6 capture drivers [1].

Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>

Added to Cc:
Cc: Lars-Peter Clausen <lars@metafoo.de>

Also adding Cc's to the people who are using the adv7180 on other
boards (renesas r8a779* boards) so we can get some feedback and/or
Tested-by from them:
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: Simon Horman <horms+renesas@verge.net.au>

Regards,

Tim

[1] - http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/102914

[PATCH 08/11] adv7180: send V4L2_EVENT_SOURCE_CHANGE on std change

[Steve Longerbeam]

Subscribe to the V4L2_EVENT_SOURCE_CHANGE event and send
V4L2_EVENT_SRC_CH_RESOLUTION in the interrupt handler on a
detected standard change.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 427695d..f76a0e7 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -28,6 +28,7 @@
 #include <linux/of.h>
 #include <linux/gpio/consumer.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-event.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ctrls.h>
@@ -824,6 +825,20 @@ static int adv7180_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
 	return 0;
 }
 
+static int adv7180_subscribe_event(struct v4l2_subdev *sd,
+				   struct v4l2_fh *fh,
+				   struct v4l2_event_subscription *sub)
+{
+	switch (sub->type) {
+	case V4L2_EVENT_SOURCE_CHANGE:
+		return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
+	case V4L2_EVENT_CTRL:
+		return v4l2_ctrl_subdev_subscribe_event(sd, fh, sub);
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.s_std = adv7180_s_std,
 	.g_std = adv7180_g_std,
@@ -838,6 +853,8 @@ static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 
 static const struct v4l2_subdev_core_ops adv7180_core_ops = {
 	.s_power = adv7180_s_power,
+	.subscribe_event = adv7180_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 };
 
 static const struct v4l2_subdev_pad_ops adv7180_pad_ops = {
@@ -862,8 +879,18 @@ static irqreturn_t adv7180_irq(int irq, void *devid)
 	/* clear */
 	adv7180_write(state, ADV7180_REG_ICR3, isr3);
 
-	if (isr3 & ADV7180_IRQ3_AD_CHANGE && state->autodetect)
-		__adv7180_status(state, NULL, &state->curr_norm);
+	if (isr3 & ADV7180_IRQ3_AD_CHANGE) {
+		static const struct v4l2_event src_ch = {
+			.type = V4L2_EVENT_SOURCE_CHANGE,
+			.u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
+		};
+
+		v4l2_subdev_notify_event(&state->sd, &src_ch);
+
+		if (state->autodetect)
+			__adv7180_status(state, NULL, &state->curr_norm);
+	}
+
 	mutex_unlock(&state->mutex);
 
 	return IRQ_HANDLED;
@@ -1403,7 +1430,7 @@ static int adv7180_probe(struct i2c_client *client,
 	state->input = 0;
 	sd = &state->sd;
 	v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
-	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 
 	ret = adv7180_init_controls(state);
 	if (ret)
-- 
1.9.1

Re: [PATCH 08/11] adv7180: send V4L2_EVENT_SOURCE_CHANGE on std change

[Tim Harvey]

On Wed, Jul 6, 2016 at 4:00 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> Subscribe to the V4L2_EVENT_SOURCE_CHANGE event and send
> V4L2_EVENT_SRC_CH_RESOLUTION in the interrupt handler on a
> detected standard change.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/media/i2c/adv7180.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 427695d..f76a0e7 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -28,6 +28,7 @@
>  #include <linux/of.h>
>  #include <linux/gpio/consumer.h>
>  #include <media/v4l2-ioctl.h>
> +#include <media/v4l2-event.h>
>  #include <linux/videodev2.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ctrls.h>
> @@ -824,6 +825,20 @@ static int adv7180_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
>         return 0;
>  }
>
> +static int adv7180_subscribe_event(struct v4l2_subdev *sd,
> +                                  struct v4l2_fh *fh,
> +                                  struct v4l2_event_subscription *sub)
> +{
> +       switch (sub->type) {
> +       case V4L2_EVENT_SOURCE_CHANGE:
> +               return v4l2_src_change_event_subdev_subscribe(sd, fh, sub);
> +       case V4L2_EVENT_CTRL:
> +               return v4l2_ctrl_subdev_subscribe_event(sd, fh, sub);
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
>  static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>         .s_std = adv7180_s_std,
>         .g_std = adv7180_g_std,
> @@ -838,6 +853,8 @@ static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>
>  static const struct v4l2_subdev_core_ops adv7180_core_ops = {
>         .s_power = adv7180_s_power,
> +       .subscribe_event = adv7180_subscribe_event,
> +       .unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  };
>
>  static const struct v4l2_subdev_pad_ops adv7180_pad_ops = {
> @@ -862,8 +879,18 @@ static irqreturn_t adv7180_irq(int irq, void *devid)
>         /* clear */
>         adv7180_write(state, ADV7180_REG_ICR3, isr3);
>
> -       if (isr3 & ADV7180_IRQ3_AD_CHANGE && state->autodetect)
> -               __adv7180_status(state, NULL, &state->curr_norm);
> +       if (isr3 & ADV7180_IRQ3_AD_CHANGE) {
> +               static const struct v4l2_event src_ch = {
> +                       .type = V4L2_EVENT_SOURCE_CHANGE,
> +                       .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
> +               };
> +
> +               v4l2_subdev_notify_event(&state->sd, &src_ch);
> +
> +               if (state->autodetect)
> +                       __adv7180_status(state, NULL, &state->curr_norm);
> +       }
> +
>         mutex_unlock(&state->mutex);
>
>         return IRQ_HANDLED;
> @@ -1403,7 +1430,7 @@ static int adv7180_probe(struct i2c_client *client,
>         state->input = 0;
>         sd = &state->sd;
>         v4l2_i2c_subdev_init(sd, client, &adv7180_ops);
> -       sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +       sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>
>         ret = adv7180_init_controls(state);
>         if (ret)

Steve,

Tested on an IMX6 Gateworks Ventana with IMX6 capture drivers [1].

Tested-by: Tim Harvey <tharvey@gateworks.com>

Added to Cc:
Cc: Lars-Peter Clausen <lars@metafoo.de>

Regards,

Tim

[1] - http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/102914

[PATCH 09/11] v4l: Add signal lock status to source change events

[Steve Longerbeam]

Add a signal lock status change to the source changes bitmask.
This indicates there was a signal lock or unlock event detected
at the input of a video decoder.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 Documentation/DocBook/media/v4l/vidioc-dqevent.xml | 12 ++++++++++--
 include/uapi/linux/videodev2.h                     |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/vidioc-dqevent.xml b/Documentation/DocBook/media/v4l/vidioc-dqevent.xml
index c9c3c77..7758ad7 100644
--- a/Documentation/DocBook/media/v4l/vidioc-dqevent.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-dqevent.xml
@@ -233,8 +233,9 @@
 	    <entry>
 	      <para>This event is triggered when a source parameter change is
 	       detected during runtime by the video device. It can be a
-	       runtime resolution change triggered by a video decoder or the
-	       format change happening on an input connector.
+	       runtime resolution change or signal lock status change
+	       triggered by a video decoder, or the format change happening
+	       on an input connector.
 	       This event requires that the <structfield>id</structfield>
 	       matches the input index (when used with a video device node)
 	       or the pad index (when used with a subdevice node) from which
@@ -461,6 +462,13 @@
 	    from a video decoder.
 	    </entry>
 	  </row>
+	  <row>
+	    <entry><constant>V4L2_EVENT_SRC_CH_LOCK_STATUS</constant></entry>
+	    <entry>0x0002</entry>
+	    <entry>This event gets triggered when there is a signal lock or
+	    unlock detected at the input of a video decoder.
+	    </entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 724f43e..08a153f 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2078,6 +2078,7 @@ struct v4l2_event_frame_sync {
 };
 
 #define V4L2_EVENT_SRC_CH_RESOLUTION		(1 << 0)
+#define V4L2_EVENT_SRC_CH_LOCK_STATUS		(1 << 1)
 
 struct v4l2_event_src_change {
 	__u32 changes;
-- 
1.9.1

[PATCH 10/11] media: adv7180: enable lock/unlock interrupts

[Steve Longerbeam]

Enable the SD lock/unlock interrupts and send V4L2_EVENT_SRC_CH_LOCK_STATUS
in the interrupt handler on a detected lock/unlock. Keep track of current
input lock status with state->curr_status.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index f76a0e7..4c2623f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -216,6 +216,7 @@ struct adv7180_state {
 	int			irq;
 	struct gpio_desc	*pwdn_gpio;
 	v4l2_std_id		curr_norm;
+	u32			curr_status; /* lock status */
 	bool			autodetect;
 	bool			bt656_4; /* use bt.656-4 standard for NTSC */
 	bool			powered;
@@ -422,7 +423,12 @@ static int adv7180_g_input_status(struct v4l2_subdev *sd, u32 *status)
 	if (ret)
 		return ret;
 
-	ret = __adv7180_status(state, status, NULL);
+	/* when we are interrupt driven we know the input lock status */
+	if (!state->autodetect || state->irq > 0)
+		*status = state->curr_status;
+	else
+		ret = __adv7180_status(state, status, NULL);
+
 	mutex_unlock(&state->mutex);
 	return ret;
 }
@@ -437,7 +443,7 @@ static int adv7180_program_std(struct adv7180_state *state)
 		if (ret < 0)
 			return ret;
 
-		__adv7180_status(state, NULL, &state->curr_norm);
+		__adv7180_status(state, &state->curr_status, &state->curr_norm);
 	} else {
 		ret = v4l2_std_to_adv7180(state->curr_norm);
 		if (ret < 0)
@@ -872,23 +878,34 @@ static const struct v4l2_subdev_ops adv7180_ops = {
 static irqreturn_t adv7180_irq(int irq, void *devid)
 {
 	struct adv7180_state *state = devid;
-	u8 isr3;
+	u8 isr1, isr3;
 
 	mutex_lock(&state->mutex);
+	isr1 = adv7180_read(state, ADV7180_REG_ISR1);
 	isr3 = adv7180_read(state, ADV7180_REG_ISR3);
 	/* clear */
+	adv7180_write(state, ADV7180_REG_ICR1, isr1);
 	adv7180_write(state, ADV7180_REG_ICR3, isr3);
 
-	if (isr3 & ADV7180_IRQ3_AD_CHANGE) {
-		static const struct v4l2_event src_ch = {
+	if ((isr3 & ADV7180_IRQ3_AD_CHANGE) ||
+	    (isr1 & (ADV7180_IRQ1_LOCK | ADV7180_IRQ1_UNLOCK))) {
+		static struct v4l2_event src_ch = {
 			.type = V4L2_EVENT_SOURCE_CHANGE,
-			.u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
 		};
 
+		if (isr3 & ADV7180_IRQ3_AD_CHANGE)
+			src_ch.u.src_change.changes |=
+				V4L2_EVENT_SRC_CH_RESOLUTION;
+
+		if (isr1 & (ADV7180_IRQ1_LOCK | ADV7180_IRQ1_UNLOCK))
+			src_ch.u.src_change.changes |=
+				V4L2_EVENT_SRC_CH_LOCK_STATUS;
+
 		v4l2_subdev_notify_event(&state->sd, &src_ch);
 
 		if (state->autodetect)
-			__adv7180_status(state, NULL, &state->curr_norm);
+			__adv7180_status(state, &state->curr_status,
+					 &state->curr_norm);
 	}
 
 	mutex_unlock(&state->mutex);
@@ -1335,7 +1352,9 @@ static int init_device(struct adv7180_state *state)
 		if (ret < 0)
 			goto out_unlock;
 
-		ret = adv7180_write(state, ADV7180_REG_IMR1, 0);
+		/* enable lock/unlock interrupts */
+		ret = adv7180_write(state, ADV7180_REG_IMR1,
+				    ADV7180_IRQ1_LOCK | ADV7180_IRQ1_UNLOCK);
 		if (ret < 0)
 			goto out_unlock;
 
-- 
1.9.1

[PATCH 11/11] media: adv7180: fix field type

[Steve Longerbeam]

The ADV7180 and ADV7182 transmit whole fields, bottom field followed
by top (or vice-versa, depending on detected video standard). So
for chips that do not have support for explicitly setting the field
mode, set the field mode to SEQ_BT for PAL, and SEQ_TB for NTSC (there
seems to be conflicting info on field order of PAL vs NTSC, so follow
what is in adv7183.c).

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 4c2623f..c12fca7 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -743,10 +743,17 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 	switch (format->format.field) {
 	case V4L2_FIELD_NONE:
 		if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
-			format->format.field = V4L2_FIELD_INTERLACED;
+			format->format.field =
+				(state->curr_norm & V4L2_STD_525_60) ?
+				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
 		break;
 	default:
-		format->format.field = V4L2_FIELD_INTERLACED;
+		if (state->chip_info->flags & ADV7180_FLAG_I2P)
+			format->format.field = V4L2_FIELD_INTERLACED;
+		else
+			format->format.field =
+				(state->curr_norm & V4L2_STD_525_60) ?
+				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
 		break;
 	}
 
@@ -1416,9 +1423,14 @@ static int adv7180_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	state->client = client;
-	state->field = V4L2_FIELD_INTERLACED;
 	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
 
+	if (state->chip_info->flags & ADV7180_FLAG_I2P)
+		state->field = V4L2_FIELD_INTERLACED;
+	else
+		state->field = (state->curr_norm & V4L2_STD_525_60) ?
+			V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
+
 	ret = adv7180_of_parse(state);
 	if (ret)
 		return ret;
-- 
1.9.1

Re: [PATCH 00/11] adv7180 subdev fixes

[Tim Harvey]

On Wed, Jul 6, 2016 at 3:59 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> Steve Longerbeam (11):
>   media: adv7180: Fix broken interrupt register access
>   Revert "[media] adv7180: fix broken standards handling"
>   media: adv7180: add power pin control
>   media: adv7180: implement g_parm
>   media: adv7180: init chip with AD recommended register settings
>   media: adv7180: add bt.656-4 OF property
>   media: adv7180: change mbus format to UYVY
>   adv7180: send V4L2_EVENT_SOURCE_CHANGE on std change
>   v4l: Add signal lock status to source change events
>   media: adv7180: enable lock/unlock interrupts
>   media: adv7180: fix field type
>
>  Documentation/DocBook/media/v4l/vidioc-dqevent.xml |  12 +-
>  drivers/media/i2c/Kconfig                          |   2 +-
>  drivers/media/i2c/adv7180.c                        | 409 +++++++++++++++------
>  include/uapi/linux/videodev2.h                     |   1 +
>  4 files changed, 308 insertions(+), 116 deletions(-)
>

Steve,

I've tested this series and will provide my ack/tested-by in the
individual patches.

These are well grouped but per the MAINTAINERS file they should all be
sent to 'Lars-Peter Clausen <lars@metafoo.de>' with a cc of
'linux-media@vger.kernel.org' or they are liable to be missed by the
proper maintainer:

ANALOG DEVICES INC ADV7180 DRIVER
M:      Lars-Peter Clausen <lars@metafoo.de>
L:      linux-media@vger.kernel.org
W:      http://ez.analog.com/community/linux-device-drivers
S:      Supported
F:      drivers/media/i2c/adv7180.c

I've cc'd Lars-Peter Clausen so that he see's the patches. Also, the
patch that reverts a previous patch should cc the people that wrote,
tested, ack'd and signed off on that patch. I will add them when I add
my review/ack to that patch.

Regards,

Tim

[PATCH v2 00/10] adv7180 subdev fixes, v2

[Steve Longerbeam]

This version adds a new v4l2 endpoint property and BT.656 bus flag
for the "NEWAVMODE" setting of Analog Devices decoders. The i.MX6
capture backend is not able to sync on the bt.656 stream from the
ADV7180 when the latter is in manual video standard setting mode,
unless NEWWAVMODE is used in conjunction. The backend needs to be
aware of NEWAVMODE so that it can make adjustments to the AV code
detection.

That's the biggest addition in this version, besides the requested
feedback changes from last version.


Steve Longerbeam (10):
  v4l: of: add "newavmode" property for Analog Devices codecs
  media: adv7180: Fix broken interrupt register access
  media: adv7180: define more registers
  media: adv7180: add support for NEWAVMODE
  media: adv7180: add power pin control
  media: adv7180: implement g_parm
  media: adv7180: change mbus format to UYVY
  v4l: Add signal lock status to source change events
  media: adv7180: enable lock/unlock interrupts
  media: adv7180: fix field type

 Documentation/DocBook/media/v4l/vidioc-dqevent.xml |  12 +-
 .../devicetree/bindings/media/i2c/adv7180.txt      |   5 +
 .../devicetree/bindings/media/video-interfaces.txt |   2 +
 drivers/media/i2c/Kconfig                          |   2 +-
 drivers/media/i2c/adv7180.c                        | 233 ++++++++++++++++-----
 drivers/media/v4l2-core/v4l2-of.c                  |   4 +
 include/media/v4l2-mediabus.h                      |   5 +
 include/uapi/linux/videodev2.h                     |   1 +
 8 files changed, 214 insertions(+), 50 deletions(-)

-- 
1.9.1

[PATCH v2 01/10] v4l: of: add "newavmode" property for Analog Devices codecs

[Steve Longerbeam]

This patch adds a "newavmode" boolean property as part of the v4l2 endpoint
properties. This indicates an Analog Devices decoder is generating EAV/SAV
codes to suit Analog Devices encoders.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Cc: Javier Martinez Canillas <javier@osg.samsung.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
 drivers/media/v4l2-core/v4l2-of.c                            | 4 ++++
 include/media/v4l2-mediabus.h                                | 5 +++++
 3 files changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 9cd2a36..6f2df51 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -88,6 +88,8 @@ Optional endpoint properties
 - field-even-active: field signal level during the even field data transmission.
 - pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
   signal.
+- newavmode: a boolean property to indicate an Analog Devices decoder is
+  operating in NEWAVMODE. Valid for BT.656 busses only.
 - sync-on-green-active: active state of Sync-on-green (SoG) signal, 0/1 for
   LOW/HIGH respectively.
 - data-lanes: an array of physical data lane indexes. Position of an entry
diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index 93b3368..719a7d1 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -109,6 +109,10 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
 		flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH :
 			V4L2_MBUS_DATA_ACTIVE_LOW;
 
+	if (endpoint->bus_type == V4L2_MBUS_BT656 &&
+	    of_get_property(node, "newavmode", &v))
+		flags |= V4L2_MBUS_NEWAVMODE;
+
 	if (of_get_property(node, "slave-mode", &v))
 		flags |= V4L2_MBUS_SLAVE;
 	else
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 34cc99e..0bd5f0e 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -43,6 +43,11 @@
 /* Active state of Sync-on-green (SoG) signal, 0/1 for LOW/HIGH respectively. */
 #define V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH	(1 << 12)
 #define V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW		(1 << 13)
+/*
+ * BT.656 specific flags
+ */
+/* Analog Device's NEWAVMODE */
+#define V4L2_MBUS_NEWAVMODE			(1 << 14)
 
 /* Serial flags */
 /* How many lanes the client can use */
-- 
1.9.1

Re: [PATCH v2 01/10] v4l: of: add "newavmode" property for Analog Devices codecs

[Hans Verkuil]

On 07/20/2016 02:03 AM, Steve Longerbeam wrote:
> This patch adds a "newavmode" boolean property as part of the v4l2 endpoint
> properties. This indicates an Analog Devices decoder is generating EAV/SAV
> codes to suit Analog Devices encoders.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
>  drivers/media/v4l2-core/v4l2-of.c                            | 4 ++++
>  include/media/v4l2-mediabus.h                                | 5 +++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..6f2df51 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -88,6 +88,8 @@ Optional endpoint properties
>  - field-even-active: field signal level during the even field data transmission.
>  - pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
>    signal.
> +- newavmode: a boolean property to indicate an Analog Devices decoder is
> +  operating in NEWAVMODE. Valid for BT.656 busses only.

This property is adv7180 specific and does not belong here.

Add this to Documentation/devicetree/bindings/media/i2c/adv7180.txt instead.

Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

>  - sync-on-green-active: active state of Sync-on-green (SoG) signal, 0/1 for
>    LOW/HIGH respectively.
>  - data-lanes: an array of physical data lane indexes. Position of an entry
> diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
> index 93b3368..719a7d1 100644
> --- a/drivers/media/v4l2-core/v4l2-of.c
> +++ b/drivers/media/v4l2-core/v4l2-of.c
> @@ -109,6 +109,10 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
>  		flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH :
>  			V4L2_MBUS_DATA_ACTIVE_LOW;
>  
> +	if (endpoint->bus_type == V4L2_MBUS_BT656 &&
> +	    of_get_property(node, "newavmode", &v))
> +		flags |= V4L2_MBUS_NEWAVMODE;
> +
>  	if (of_get_property(node, "slave-mode", &v))
>  		flags |= V4L2_MBUS_SLAVE;
>  	else
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 34cc99e..0bd5f0e 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -43,6 +43,11 @@
>  /* Active state of Sync-on-green (SoG) signal, 0/1 for LOW/HIGH respectively. */
>  #define V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH	(1 << 12)
>  #define V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW		(1 << 13)
> +/*
> + * BT.656 specific flags
> + */
> +/* Analog Device's NEWAVMODE */
> +#define V4L2_MBUS_NEWAVMODE			(1 << 14)
>  
>  /* Serial flags */
>  /* How many lanes the client can use */
> 

Re: [PATCH v2 01/10] v4l: of: add "newavmode" property for Analog Devices codecs

[Steve Longerbeam]

On 07/20/2016 12:37 AM, Hans Verkuil wrote:
> On 07/20/2016 02:03 AM, Steve Longerbeam wrote:
>> This patch adds a "newavmode" boolean property as part of the v4l2 endpoint
>> properties. This indicates an Analog Devices decoder is generating EAV/SAV
>> codes to suit Analog Devices encoders.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
>>  drivers/media/v4l2-core/v4l2-of.c                            | 4 ++++
>>  include/media/v4l2-mediabus.h                                | 5 +++++
>>  3 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> index 9cd2a36..6f2df51 100644
>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> @@ -88,6 +88,8 @@ Optional endpoint properties
>>  - field-even-active: field signal level during the even field data transmission.
>>  - pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
>>    signal.
>> +- newavmode: a boolean property to indicate an Analog Devices decoder is
>> +  operating in NEWAVMODE. Valid for BT.656 busses only.
> This property is adv7180 specific and does not belong here.

Hi Hans, yes that was my initial reaction to this idea as well, but
you didn't respond to my first request for comment.

>
> Add this to Documentation/devicetree/bindings/media/i2c/adv7180.txt instead.

Yes, I'll move this property into adv7180.

Steve

[PATCH v2 02/10] media: adv7180: Fix broken interrupt register access

[Steve Longerbeam]

Access to the interrupt page registers has been broken since at least
commit 3999e5d01da7 ("[media] adv7180: Do implicit register paging").
That commit forgot to add the interrupt page number to the register
defines.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/media/i2c/adv7180.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index b77b0a4..95cbc85 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -100,7 +100,7 @@
 #define ADV7180_REG_IDENT 0x0011
 #define ADV7180_ID_7180 0x18
 
-#define ADV7180_REG_ICONF1		0x0040
+#define ADV7180_REG_ICONF1		0x2040
 #define ADV7180_ICONF1_ACTIVE_LOW	0x01
 #define ADV7180_ICONF1_PSYNC_ONLY	0x10
 #define ADV7180_ICONF1_ACTIVE_TO_CLR	0xC0
@@ -113,15 +113,15 @@
 
 #define ADV7180_IRQ1_LOCK	0x01
 #define ADV7180_IRQ1_UNLOCK	0x02
-#define ADV7180_REG_ISR1	0x0042
-#define ADV7180_REG_ICR1	0x0043
-#define ADV7180_REG_IMR1	0x0044
-#define ADV7180_REG_IMR2	0x0048
+#define ADV7180_REG_ISR1	0x2042
+#define ADV7180_REG_ICR1	0x2043
+#define ADV7180_REG_IMR1	0x2044
+#define ADV7180_REG_IMR2	0x2048
 #define ADV7180_IRQ3_AD_CHANGE	0x08
-#define ADV7180_REG_ISR3	0x004A
-#define ADV7180_REG_ICR3	0x004B
-#define ADV7180_REG_IMR3	0x004C
-#define ADV7180_REG_IMR4	0x50
+#define ADV7180_REG_ISR3	0x204A
+#define ADV7180_REG_ICR3	0x204B
+#define ADV7180_REG_IMR3	0x204C
+#define ADV7180_REG_IMR4	0x2050
 
 #define ADV7180_REG_NTSC_V_BIT_END	0x00E6
 #define ADV7180_NTSC_V_BIT_END_MANUAL_NVEND	0x4F
-- 
1.9.1

[PATCH v2 03/10] media: adv7180: define more registers

[Steve Longerbeam]

Replace hard-coded addresses with new register macro defines. No
functional changes.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 73 ++++++++++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 95cbc85..cb83ebb 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -56,10 +56,11 @@
 
 #define ADV7182_REG_INPUT_VIDSEL			0x0002
 
+#define ADV7180_REG_OUTPUT_CONTROL			0x0003
 #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL		0x0004
 #define ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS		0xC5
 
-#define ADV7180_REG_AUTODETECT_ENABLE			0x07
+#define ADV7180_REG_AUTODETECT_ENABLE			0x0007
 #define ADV7180_AUTODETECT_DEFAULT			0x7f
 /* Contrast */
 #define ADV7180_REG_CON		0x0008	/*Unsigned */
@@ -100,6 +101,20 @@
 #define ADV7180_REG_IDENT 0x0011
 #define ADV7180_ID_7180 0x18
 
+#define ADV7180_REG_STATUS3		0x0013
+#define ADV7180_REG_ANALOG_CLAMP_CTL	0x0014
+#define ADV7180_REG_SHAP_FILTER_CTL_1	0x0017
+#define ADV7180_REG_CTRL_2		0x001d
+#define ADV7180_REG_VSYNC_FIELD_CTL_1	0x0031
+#define ADV7180_REG_MANUAL_WIN_CTL_1	0x003d
+#define ADV7180_REG_MANUAL_WIN_CTL_2	0x003e
+#define ADV7180_REG_MANUAL_WIN_CTL_3	0x003f
+#define ADV7180_REG_LOCK_CNT		0x0051
+#define ADV7180_REG_CVBS_TRIM		0x0052
+#define ADV7180_REG_CLAMP_ADJ		0x005a
+#define ADV7180_REG_RES_CIR		0x005f
+#define ADV7180_REG_DIFF_MODE		0x0060
+
 #define ADV7180_REG_ICONF1		0x2040
 #define ADV7180_ICONF1_ACTIVE_LOW	0x01
 #define ADV7180_ICONF1_PSYNC_ONLY	0x10
@@ -129,9 +144,15 @@
 #define ADV7180_REG_VPP_SLAVE_ADDR	0xFD
 #define ADV7180_REG_CSI_SLAVE_ADDR	0xFE
 
-#define ADV7180_REG_FLCONTROL 0x40e0
+#define ADV7180_REG_ACE_CTRL1		0x4080
+#define ADV7180_REG_ACE_CTRL5		0x4084
+#define ADV7180_REG_FLCONTROL		0x40e0
 #define ADV7180_FLCONTROL_FL_ENABLE 0x1
 
+#define ADV7180_REG_RST_CLAMP	0x809c
+#define ADV7180_REG_AGC_ADJ1	0x80b6
+#define ADV7180_REG_AGC_ADJ2	0x80c0
+
 #define ADV7180_CSI_REG_PWRDN	0x00
 #define ADV7180_CSI_PWRDN	0x80
 
@@ -886,16 +907,20 @@ static int adv7182_init(struct adv7180_state *state)
 
 	/* ADI required writes */
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
-		adv7180_write(state, 0x0003, 0x4e);
-		adv7180_write(state, 0x0004, 0x57);
-		adv7180_write(state, 0x001d, 0xc0);
+		adv7180_write(state, ADV7180_REG_OUTPUT_CONTROL, 0x4e);
+		adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57);
+		adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
 	} else {
 		if (state->chip_info->flags & ADV7180_FLAG_V2)
-			adv7180_write(state, 0x0004, 0x17);
+			adv7180_write(state,
+				      ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
+				      0x17);
 		else
-			adv7180_write(state, 0x0004, 0x07);
-		adv7180_write(state, 0x0003, 0x0c);
-		adv7180_write(state, 0x001d, 0x40);
+			adv7180_write(state,
+				      ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
+				      0x07);
+		adv7180_write(state, ADV7180_REG_OUTPUT_CONTROL, 0x0c);
+		adv7180_write(state, ADV7180_REG_CTRL_2, 0x40);
 	}
 
 	adv7180_write(state, 0x0013, 0x00);
@@ -972,8 +997,8 @@ static int adv7182_select_input(struct adv7180_state *state, unsigned int input)
 		return ret;
 
 	/* Reset clamp circuitry - ADI recommended writes */
-	adv7180_write(state, 0x809c, 0x00);
-	adv7180_write(state, 0x809c, 0xff);
+	adv7180_write(state, ADV7180_REG_RST_CLAMP, 0x00);
+	adv7180_write(state, ADV7180_REG_RST_CLAMP, 0xff);
 
 	input_type = adv7182_get_input_type(input);
 
@@ -981,10 +1006,10 @@ static int adv7182_select_input(struct adv7180_state *state, unsigned int input)
 	case ADV7182_INPUT_TYPE_CVBS:
 	case ADV7182_INPUT_TYPE_DIFF_CVBS:
 		/* ADI recommends to use the SH1 filter */
-		adv7180_write(state, 0x0017, 0x41);
+		adv7180_write(state, ADV7180_REG_SHAP_FILTER_CTL_1, 0x41);
 		break;
 	default:
-		adv7180_write(state, 0x0017, 0x01);
+		adv7180_write(state, ADV7180_REG_SHAP_FILTER_CTL_1, 0x01);
 		break;
 	}
 
@@ -994,21 +1019,21 @@ static int adv7182_select_input(struct adv7180_state *state, unsigned int input)
 		lbias = adv7182_lbias_settings[input_type];
 
 	for (i = 0; i < ARRAY_SIZE(adv7182_lbias_settings[0]); i++)
-		adv7180_write(state, 0x0052 + i, lbias[i]);
+		adv7180_write(state, ADV7180_REG_CVBS_TRIM + i, lbias[i]);
 
 	if (input_type == ADV7182_INPUT_TYPE_DIFF_CVBS) {
 		/* ADI required writes to make differential CVBS work */
-		adv7180_write(state, 0x005f, 0xa8);
-		adv7180_write(state, 0x005a, 0x90);
-		adv7180_write(state, 0x0060, 0xb0);
-		adv7180_write(state, 0x80b6, 0x08);
-		adv7180_write(state, 0x80c0, 0xa0);
+		adv7180_write(state, ADV7180_REG_RES_CIR, 0xa8);
+		adv7180_write(state, ADV7180_REG_CLAMP_ADJ, 0x90);
+		adv7180_write(state, ADV7180_REG_DIFF_MODE, 0xb0);
+		adv7180_write(state, ADV7180_REG_AGC_ADJ1, 0x08);
+		adv7180_write(state, ADV7180_REG_AGC_ADJ2, 0xa0);
 	} else {
-		adv7180_write(state, 0x005f, 0xf0);
-		adv7180_write(state, 0x005a, 0xd0);
-		adv7180_write(state, 0x0060, 0x10);
-		adv7180_write(state, 0x80b6, 0x9c);
-		adv7180_write(state, 0x80c0, 0x00);
+		adv7180_write(state, ADV7180_REG_RES_CIR, 0xf0);
+		adv7180_write(state, ADV7180_REG_CLAMP_ADJ, 0xd0);
+		adv7180_write(state, ADV7180_REG_DIFF_MODE, 0x10);
+		adv7180_write(state, ADV7180_REG_AGC_ADJ1, 0x9c);
+		adv7180_write(state, ADV7180_REG_AGC_ADJ2, 0x00);
 	}
 
 	return 0;
-- 
1.9.1

Re: [PATCH v2 03/10] media: adv7180: define more registers

[Lars-Peter Clausen]

On 07/20/2016 02:03 AM, Steve Longerbeam wrote:
> Replace hard-coded addresses with new register macro defines. No
> functional changes.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

[PATCH v2 04/10] media: adv7180: add support for NEWAVMODE

[Steve Longerbeam]

Parse the optional v4l2 endpoint DT node. If the V4L2_MBUS_NEWAVMODE
parallel bus flag is set, configure the BT.656 bus in NEWAVMODE.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index cb83ebb..667931e9 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -31,6 +31,7 @@
 #include <media/v4l2-event.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>
 #include <linux/mutex.h>
 #include <linux/delay.h>
 
@@ -106,6 +107,7 @@
 #define ADV7180_REG_SHAP_FILTER_CTL_1	0x0017
 #define ADV7180_REG_CTRL_2		0x001d
 #define ADV7180_REG_VSYNC_FIELD_CTL_1	0x0031
+#define ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE 0x02
 #define ADV7180_REG_MANUAL_WIN_CTL_1	0x003d
 #define ADV7180_REG_MANUAL_WIN_CTL_2	0x003e
 #define ADV7180_REG_MANUAL_WIN_CTL_3	0x003f
@@ -214,6 +216,7 @@ struct adv7180_state {
 	struct mutex		mutex; /* mutual excl. when accessing chip */
 	int			irq;
 	v4l2_std_id		curr_norm;
+	bool			newavmode;
 	bool			powered;
 	bool			streaming;
 	u8			input;
@@ -740,6 +743,8 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
 		 */
 		cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
 				 V4L2_MBUS_DATA_ACTIVE_HIGH;
+		if (state->newavmode)
+			cfg->flags |= V4L2_MBUS_NEWAVMODE;
 		cfg->type = V4L2_MBUS_BT656;
 	}
 
@@ -864,9 +869,15 @@ static int adv7180_init(struct adv7180_state *state)
 	if (ret < 0)
 		return ret;
 
-	/* Manually set V bit end position in NTSC mode */
-	return adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
-					ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+	if (!state->newavmode) {
+		/* Manually set V bit end position in NTSC mode */
+		ret = adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
+				    ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int adv7180_set_std(struct adv7180_state *state, unsigned int std)
@@ -1217,6 +1228,13 @@ static int init_device(struct adv7180_state *state)
 	if (ret)
 		goto out_unlock;
 
+	if (state->newavmode) {
+		ret = adv7180_write(state, ADV7180_REG_VSYNC_FIELD_CTL_1,
+				    ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE);
+		if (ret < 0)
+			goto out_unlock;
+	}
+
 	ret = adv7180_program_std(state);
 	if (ret)
 		goto out_unlock;
@@ -1257,6 +1275,27 @@ out_unlock:
 	return ret;
 }
 
+static void adv7180_of_parse(struct adv7180_state *state)
+{
+	struct i2c_client *client = state->client;
+	struct device_node *np = client->dev.of_node;
+	struct device_node *endpoint;
+	struct v4l2_of_endpoint	ep;
+
+	endpoint = of_graph_get_next_endpoint(np, NULL);
+	if (!endpoint) {
+		v4l_warn(client, "endpoint node not found\n");
+		return;
+	}
+
+	v4l2_of_parse_endpoint(endpoint, &ep);
+	of_node_put(endpoint);
+
+	if (ep.bus_type == V4L2_MBUS_BT656 &&
+	    ep.bus.parallel.flags & V4L2_MBUS_NEWAVMODE)
+		state->newavmode = true;
+}
+
 static int adv7180_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -1279,6 +1318,8 @@ static int adv7180_probe(struct i2c_client *client,
 	state->field = V4L2_FIELD_INTERLACED;
 	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
 
+	adv7180_of_parse(state);
+
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
 		state->csi_client = i2c_new_dummy(client->adapter,
 				ADV7180_DEFAULT_CSI_I2C_ADDR);
-- 
1.9.1

[PATCH v2 05/10] media: adv7180: add power pin control

[Steve Longerbeam]

Some targets control the ADV7180 power pin via a gpio, so add
optional support for "powerdown" pin control.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>

---

v2:
- placed call to gpiod_get inline in adv7180_probe().
- rename gpio pin to "powerdown".
- document optional powerdown-gpios property in
  Documentation/devicetree/bindings/media/i2c/adv7180.txt.
- include error number in error message on gpiod_get failure.
---
 .../devicetree/bindings/media/i2c/adv7180.txt      |  5 ++++
 drivers/media/i2c/Kconfig                          |  2 +-
 drivers/media/i2c/adv7180.c                        | 27 ++++++++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7180.txt b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
index 0d50115..4da486f 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
@@ -15,6 +15,11 @@ Required Properties :
 		"adi,adv7282"
 		"adi,adv7282-m"
 
+Optional Properties :
+- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
+  if any.
+
+
 Example:
 
 	i2c0@1c22000 {
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index ce9006e..6769898 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -187,7 +187,7 @@ comment "Video decoders"
 
 config VIDEO_ADV7180
 	tristate "Analog Devices ADV7180 decoder"
-	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
+	depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
 	---help---
 	  Support for the Analog Devices ADV7180 video decoder.
 
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 667931e9..8612d21 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -26,6 +26,7 @@
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/gpio/consumer.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-event.h>
@@ -215,6 +216,7 @@ struct adv7180_state {
 	struct media_pad	pad;
 	struct mutex		mutex; /* mutual excl. when accessing chip */
 	int			irq;
+	struct gpio_desc	*pwdn_gpio;
 	v4l2_std_id		curr_norm;
 	bool			newavmode;
 	bool			powered;
@@ -466,6 +468,19 @@ static int adv7180_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm)
 	return 0;
 }
 
+static void adv7180_set_power_pin(struct adv7180_state *state, bool on)
+{
+	if (!state->pwdn_gpio)
+		return;
+
+	if (on) {
+		gpiod_set_value_cansleep(state->pwdn_gpio, 0);
+		usleep_range(5000, 10000);
+	} else {
+		gpiod_set_value_cansleep(state->pwdn_gpio, 1);
+	}
+}
+
 static int adv7180_set_power(struct adv7180_state *state, bool on)
 {
 	u8 val;
@@ -1221,6 +1236,8 @@ static int init_device(struct adv7180_state *state)
 
 	mutex_lock(&state->mutex);
 
+	adv7180_set_power_pin(state, true);
+
 	adv7180_write(state, ADV7180_REG_PWR_MAN, ADV7180_PWR_MAN_RES);
 	usleep_range(5000, 10000);
 
@@ -1320,6 +1337,14 @@ static int adv7180_probe(struct i2c_client *client,
 
 	adv7180_of_parse(state);
 
+	state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
+						   GPIOD_OUT_HIGH);
+	if (IS_ERR(state->pwdn_gpio)) {
+		ret = PTR_ERR(state->pwdn_gpio);
+		v4l_err(client, "request for power pin failed: %d\n", ret);
+		return ret;
+	}
+
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
 		state->csi_client = i2c_new_dummy(client->adapter,
 				ADV7180_DEFAULT_CSI_I2C_ADDR);
@@ -1411,6 +1436,8 @@ static int adv7180_remove(struct i2c_client *client)
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2)
 		i2c_unregister_device(state->csi_client);
 
+	adv7180_set_power_pin(state, false);
+
 	mutex_destroy(&state->mutex);
 
 	return 0;
-- 
1.9.1

Re: [PATCH v2 05/10] media: adv7180: add power pin control

[Lars-Peter Clausen]

On 07/20/2016 02:03 AM, Steve Longerbeam wrote:
> Some targets control the ADV7180 power pin via a gpio, so add
> optional support for "powerdown" pin control.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> Tested-by: Tim Harvey <tharvey@gateworks.com>
> Acked-by: Tim Harvey <tharvey@gateworks.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>

Looks good, thanks.

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

[PATCH v2 06/10] media: adv7180: implement g_parm

[Steve Longerbeam]

Implement g_parm to return the current standard's frame period.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/media/i2c/adv7180.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 8612d21..8259549 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -766,6 +766,27 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int adv7180_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
+{
+	struct adv7180_state *state = to_state(sd);
+	struct v4l2_captureparm *cparm = &a->parm.capture;
+
+	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	memset(a, 0, sizeof(*a));
+	a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	if (state->curr_norm & V4L2_STD_525_60) {
+		cparm->timeperframe.numerator = 1001;
+		cparm->timeperframe.denominator = 30000;
+	} else {
+		cparm->timeperframe.numerator = 1;
+		cparm->timeperframe.denominator = 25;
+	}
+
+	return 0;
+}
+
 static int adv7180_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *cropcap)
 {
 	struct adv7180_state *state = to_state(sd);
@@ -824,6 +845,7 @@ static int adv7180_subscribe_event(struct v4l2_subdev *sd,
 static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.s_std = adv7180_s_std,
 	.g_std = adv7180_g_std,
+	.g_parm = adv7180_g_parm,
 	.querystd = adv7180_querystd,
 	.g_input_status = adv7180_g_input_status,
 	.s_routing = adv7180_s_routing,
-- 
1.9.1

[PATCH v2 07/10] media: adv7180: change mbus format to UYVY

[Steve Longerbeam]

Change the media bus format from YUYV8_2X8 to UYVY8_2X8. Colors
now look correct when capturing with the i.mx6 backend.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 8259549..0f0568c 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -636,7 +636,7 @@ static int adv7180_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index != 0)
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_YUYV8_2X8;
+	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
 
 	return 0;
 }
@@ -646,7 +646,7 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
 {
 	struct adv7180_state *state = to_state(sd);
 
-	fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
+	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
 	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
 	fmt->width = 720;
 	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
-- 
1.9.1

[PATCH v2 08/10] v4l: Add signal lock status to source change events

[Steve Longerbeam]

Add a signal lock status change to the source changes bitmask.
This indicates there was a signal lock or unlock event detected
at the input of a video decoder.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 Documentation/DocBook/media/v4l/vidioc-dqevent.xml | 12 ++++++++++--
 include/uapi/linux/videodev2.h                     |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/vidioc-dqevent.xml b/Documentation/DocBook/media/v4l/vidioc-dqevent.xml
index c9c3c77..7758ad7 100644
--- a/Documentation/DocBook/media/v4l/vidioc-dqevent.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-dqevent.xml
@@ -233,8 +233,9 @@
 	    <entry>
 	      <para>This event is triggered when a source parameter change is
 	       detected during runtime by the video device. It can be a
-	       runtime resolution change triggered by a video decoder or the
-	       format change happening on an input connector.
+	       runtime resolution change or signal lock status change
+	       triggered by a video decoder, or the format change happening
+	       on an input connector.
 	       This event requires that the <structfield>id</structfield>
 	       matches the input index (when used with a video device node)
 	       or the pad index (when used with a subdevice node) from which
@@ -461,6 +462,13 @@
 	    from a video decoder.
 	    </entry>
 	  </row>
+	  <row>
+	    <entry><constant>V4L2_EVENT_SRC_CH_LOCK_STATUS</constant></entry>
+	    <entry>0x0002</entry>
+	    <entry>This event gets triggered when there is a signal lock or
+	    unlock detected at the input of a video decoder.
+	    </entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 724f43e..08a153f 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2078,6 +2078,7 @@ struct v4l2_event_frame_sync {
 };
 
 #define V4L2_EVENT_SRC_CH_RESOLUTION		(1 << 0)
+#define V4L2_EVENT_SRC_CH_LOCK_STATUS		(1 << 1)
 
 struct v4l2_event_src_change {
 	__u32 changes;
-- 
1.9.1

[PATCH v2 09/10] media: adv7180: enable lock/unlock interrupts

[Steve Longerbeam]

Enable the SD lock/unlock interrupts and send V4L2_EVENT_SRC_CH_LOCK_STATUS
in the interrupt handler on a detected lock/unlock.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>

---

v2:
- last version of this patch was based on the old reverted autodetect
  code. This version now only enables the interrupt and sends the
  event in the interrupt handler.
---
 drivers/media/i2c/adv7180.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 0f0568c..61f2140 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -876,19 +876,29 @@ static const struct v4l2_subdev_ops adv7180_ops = {
 static irqreturn_t adv7180_irq(int irq, void *devid)
 {
 	struct adv7180_state *state = devid;
-	u8 isr3;
+	u8 isr1, isr3;
 
 	mutex_lock(&state->mutex);
+	isr1 = adv7180_read(state, ADV7180_REG_ISR1);
 	isr3 = adv7180_read(state, ADV7180_REG_ISR3);
 	/* clear */
+	adv7180_write(state, ADV7180_REG_ICR1, isr1);
 	adv7180_write(state, ADV7180_REG_ICR3, isr3);
 
-	if (isr3 & ADV7180_IRQ3_AD_CHANGE) {
-		static const struct v4l2_event src_ch = {
+	if ((isr3 & ADV7180_IRQ3_AD_CHANGE) ||
+	    (isr1 & (ADV7180_IRQ1_LOCK | ADV7180_IRQ1_UNLOCK))) {
+		static struct v4l2_event src_ch = {
 			.type = V4L2_EVENT_SOURCE_CHANGE,
-			.u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
 		};
 
+		if (isr3 & ADV7180_IRQ3_AD_CHANGE)
+			src_ch.u.src_change.changes |=
+				V4L2_EVENT_SRC_CH_RESOLUTION;
+
+		if (isr1 & (ADV7180_IRQ1_LOCK | ADV7180_IRQ1_UNLOCK))
+			src_ch.u.src_change.changes |=
+				V4L2_EVENT_SRC_CH_LOCK_STATUS;
+
 		v4l2_subdev_notify_event(&state->sd, &src_ch);
 	}
 	mutex_unlock(&state->mutex);
@@ -1289,7 +1299,9 @@ static int init_device(struct adv7180_state *state)
 		if (ret < 0)
 			goto out_unlock;
 
-		ret = adv7180_write(state, ADV7180_REG_IMR1, 0);
+		/* enable lock/unlock interrupts */
+		ret = adv7180_write(state, ADV7180_REG_IMR1,
+				    ADV7180_IRQ1_LOCK | ADV7180_IRQ1_UNLOCK);
 		if (ret < 0)
 			goto out_unlock;
 
-- 
1.9.1

[PATCH v2 10/10] media: adv7180: fix field type

[Steve Longerbeam]

The ADV7180 and ADV7182 transmit whole fields, bottom field followed
by top (or vice-versa, depending on detected video standard). So
for chips that do not have support for explicitly setting the field
mode, set the field mode to SEQ_BT for PAL, and SEQ_TB for NTSC (there
seems to be conflicting info on field order of PAL vs NTSC, so follow
what is in adv7183.c).

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>

---

v2:
- the init of state->curr_norm in probe needs to be moved up, ahead
  of the init of state->field.
---
 drivers/media/i2c/adv7180.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 61f2140..58e1f53 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -718,10 +718,17 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 	switch (format->format.field) {
 	case V4L2_FIELD_NONE:
 		if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
-			format->format.field = V4L2_FIELD_INTERLACED;
+			format->format.field =
+				(state->curr_norm & V4L2_STD_525_60) ?
+				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
 		break;
 	default:
-		format->format.field = V4L2_FIELD_INTERLACED;
+		if (state->chip_info->flags & ADV7180_FLAG_I2P)
+			format->format.field = V4L2_FIELD_INTERLACED;
+		else
+			format->format.field =
+				(state->curr_norm & V4L2_STD_525_60) ?
+				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
 		break;
 	}
 
@@ -1366,8 +1373,14 @@ static int adv7180_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	state->client = client;
-	state->field = V4L2_FIELD_INTERLACED;
 	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
+	state->curr_norm = V4L2_STD_NTSC;
+
+	if (state->chip_info->flags & ADV7180_FLAG_I2P)
+		state->field = V4L2_FIELD_INTERLACED;
+	else
+		state->field = (state->curr_norm & V4L2_STD_525_60) ?
+			V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
 
 	adv7180_of_parse(state);
 
@@ -1397,7 +1410,6 @@ static int adv7180_probe(struct i2c_client *client,
 
 	state->irq = client->irq;
 	mutex_init(&state->mutex);
-	state->curr_norm = V4L2_STD_NTSC;
 	if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
 		state->powered = true;
 	else
-- 
1.9.1