[PATCH v2] Add GS driver (SPI video serializer family)

[PATCH v2] Add GS driver (SPI video serializer family)

[Charles-Antoine Couret]

Hello,
Here my second patch version about GS serializer.
It should be easy to add support for GS1582 and GS1572.
I am not sure about V4L2 interfaces to set/get timings.
I tested with GS1662 component and v4l2-dbg and v4l2-ctl tools.

But this component family support CEA standards and other
(SMPTE XXXM in fact). V4L2 seems oriented to manage CEA or
VGA formats. So, I used timings structure with CEA values, but I
fill timings fields manually for other standards. I don't know if it
is the right method or if another interface should be more interesting.

And I used the reset method which seems deprecated. But for this
component, it should be the right way to reset in auto-detection mode
instead of "timings forced by user".

Thank you in advance for your comments.
Regards.
Charles-Antoine Couret

>From a1bc59b8b18dc75bbf3a70483f57d4ccd190b5f9 Mon Sep 17 00:00:00 2001
From: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
Date: Fri, 1 Apr 2016 17:19:26 +0200
Subject: [PATCH] Add GSXXXX driver (SPI video serializer family)

This patch was tested with GS1662:
http://www.c-dis.net/media/871/GS1662_Datasheet.pdf

It is a v4l2-subdev which serializes some CEA formats
and other. The driver could receive some custom timings and
other supported format.

Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
---
 drivers/media/Kconfig      |   1 +
 drivers/media/Makefile     |   2 +-
 drivers/media/spi/Kconfig  |   5 +
 drivers/media/spi/Makefile |   1 +
 drivers/media/spi/gsxxxx.c | 482 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 490 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/spi/Kconfig
 create mode 100644 drivers/media/spi/Makefile
 create mode 100644 drivers/media/spi/gsxxxx.c

diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index a8518fb..d2fa6e7 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -215,5 +215,6 @@ config MEDIA_ATTACH
 source "drivers/media/i2c/Kconfig"
 source "drivers/media/tuners/Kconfig"
 source "drivers/media/dvb-frontends/Kconfig"
+source "drivers/media/spi/Kconfig"
 
 endif # MEDIA_SUPPORT
diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index e608bbc..75bc82e 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -28,6 +28,6 @@ obj-y += rc/
 # Finally, merge the drivers that require the core
 #
 
-obj-y += common/ platform/ pci/ usb/ mmc/ firewire/
+obj-y += common/ platform/ pci/ usb/ mmc/ firewire/ spi/
 obj-$(CONFIG_VIDEO_DEV) += radio/
 
diff --git a/drivers/media/spi/Kconfig b/drivers/media/spi/Kconfig
new file mode 100644
index 0000000..19a257c
--- /dev/null
+++ b/drivers/media/spi/Kconfig
@@ -0,0 +1,5 @@
+config VIDEO_GSXXXX
+	tristate "Gennum Serializers video"
+	depends on SPI
+	---help---
+	  Enable the GSXXXX driver which serializes video streams.
diff --git a/drivers/media/spi/Makefile b/drivers/media/spi/Makefile
new file mode 100644
index 0000000..a67df8d
--- /dev/null
+++ b/drivers/media/spi/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_GSXXXX) += gsxxxx.o
diff --git a/drivers/media/spi/gsxxxx.c b/drivers/media/spi/gsxxxx.c
new file mode 100644
index 0000000..0745ef9
--- /dev/null
+++ b/drivers/media/spi/gsxxxx.c
@@ -0,0 +1,482 @@
+/*
+ * GSXXXX device registration. Tested with GS1662 device.
+ *
+ * Copyright (C) 2015-2016 Nexvision
+ * Author: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/platform_device.h>
+#include <linux/ctype.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-dv-timings.h>
+#include <linux/v4l2-dv-timings.h>
+
+
+#define REG_STATUS			0x04
+#define REG_FORCE_FMT			0x06
+#define REG_LINES_PER_FRAME		0x12
+#define REG_WORDS_PER_LINE		0x13
+#define REG_WORDS_PER_ACT_LINE		0x14
+#define REG_ACT_LINES_PER_FRAME	0x15
+
+#define MASK_H_LOCK		0x001
+#define MASK_V_LOCK		0x002
+#define MASK_STD_LOCK		0x004
+#define MASK_FORCE_STD		0x020
+#define MASK_STD_STATUS	0x3E0
+#define ADDRESS_MASK		0x0FFF
+
+#define GS_WIDTH_MIN		0
+#define GS_WIDTH_MAX		2048
+#define GS_HEIGHT_MIN		0
+#define GS_HEIGHT_MAX		1080
+#define GS_PIXELCLOCK_MIN	10519200
+#define GS_PIXELCLOCK_MAX	74250000
+
+#define READ_FLAG	0x8000
+#define WRITE_FLAG	0x0000
+#define BURST_FLAG	0x1000
+
+struct gsxxxx {
+	struct spi_device *pdev;
+	struct v4l2_subdev sd;
+	struct v4l2_dv_timings current_timings;
+};
+
+struct gsxxxx_reg_fmt {
+	u16 reg_value;
+	struct v4l2_dv_timings format;
+};
+
+struct gsxxxx_reg_fmt_custom {
+	u16 reg_value;
+	__u32 width;
+	__u32 height;
+	__u64 pixelclock;
+	__u32 interlaced;
+};
+
+static const struct spi_device_id gsxxxx_id[] = {
+	{ "gs1662", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(pdev, gsxxxx_id);
+
+static const struct gsxxxx_reg_fmt reg_fmt[] = {
+	{ 0x00, V4L2_DV_BT_CEA_1280X720P60 },
+	{ 0x01, V4L2_DV_BT_CEA_1280X720P60 },
+	{ 0x02, V4L2_DV_BT_CEA_1280X720P30 },
+	{ 0x03, V4L2_DV_BT_CEA_1280X720P30 },
+	{ 0x04, V4L2_DV_BT_CEA_1280X720P50 },
+	{ 0x05, V4L2_DV_BT_CEA_1280X720P50 },
+	{ 0x06, V4L2_DV_BT_CEA_1280X720P25 },
+	{ 0x07, V4L2_DV_BT_CEA_1280X720P25 },
+	{ 0x08, V4L2_DV_BT_CEA_1280X720P24 },
+	{ 0x09, V4L2_DV_BT_CEA_1280X720P24 },
+	{ 0x0A, V4L2_DV_BT_CEA_1920X1080I60 },
+	{ 0x0B, V4L2_DV_BT_CEA_1920X1080P30 },
+
+	/* Default value: keep this field before 0xC */
+	{ 0x14, V4L2_DV_BT_CEA_1920X1080I50 },
+	{ 0x0C, V4L2_DV_BT_CEA_1920X1080I50 },
+	{ 0x0D, V4L2_DV_BT_CEA_1920X1080P25 },
+	{ 0x0E, V4L2_DV_BT_CEA_1920X1080P25},
+	{ 0x10, V4L2_DV_BT_CEA_1920X1080P24 },
+	{ 0x12, V4L2_DV_BT_CEA_1920X1080P24 },
+	{ 0x18, V4L2_DV_BT_CEA_720X576P50 },
+	{ 0x1A, V4L2_DV_BT_CEA_720X576P50 },
+};
+
+/* Non CEA standard values supported by this component family.
+ * It does not care about *porch information.
+ */
+static const struct gsxxxx_reg_fmt_custom reg_fmt_custom[] = {
+	/* NULL value which means auto-detection mode or error */
+	{ 0xFF, 0, 0, 0 },
+	{ 0x0F, 1920, 1080, 25920000, 1 },
+	{ 0x11, 1920, 1080, 24883200, 1 },
+	{ 0x13, 1920, 1080, 24883200, 1 },
+	{ 0x15, 1920, 1035, 59616000, 1 },
+	{ 0x16, 720, 487, 10519200, 1 },
+	{ 0x17, 720, 507, 10951200, 1 },
+	{ 0x19, 720, 487, 10519200, 1 },
+	{ 0x1B, 720, 507, 10951200, 1 },
+	{ 0x1C, 2048, 1080, 55296000, 0 },
+};
+
+static const struct v4l2_dv_timings_cap gsxxxx_timings_cap = {
+	.type = V4L2_DV_BT_656_1120,
+	/* keep this initialization for compatibility with GCC < 4.4.6 */
+	.reserved = { 0 },
+	V4L2_INIT_BT_TIMINGS(GS_WIDTH_MIN, GS_WIDTH_MAX, GS_HEIGHT_MIN,
+			     GS_HEIGHT_MAX, GS_PIXELCLOCK_MIN,
+			     GS_PIXELCLOCK_MAX, V4L2_DV_BT_STD_CEA861,
+			     V4L2_DV_BT_CAP_PROGRESSIVE
+			     | V4L2_DV_BT_CAP_INTERLACED
+			     | V4L2_DV_BT_CAP_CUSTOM)
+};
+
+static int gsxxxx_read_register(struct spi_device *spi, u16 addr, u16 *value)
+{
+	int ret;
+	u16 buf_addr = (READ_FLAG | (ADDRESS_MASK & addr));
+	u16 buf_value = 0;
+	struct spi_message msg;
+	struct spi_transfer tx[] = {
+		{
+			.tx_buf = &buf_addr,
+			.len = 2,
+			.delay_usecs = 1,
+		}, {
+			.rx_buf = &buf_value,
+			.len = 2,
+			.delay_usecs = 1,
+		},
+	};
+
+	spi_message_init(&msg);
+	spi_message_add_tail(&tx[0], &msg);
+	spi_message_add_tail(&tx[1], &msg);
+	ret = spi_sync(spi, &msg);
+
+	*value = buf_value;
+
+	return ret;
+}
+
+static int gsxxxx_write_register(struct spi_device *spi, u16 addr, u16 value)
+{
+	int ret;
+	u16 buf_addr = (WRITE_FLAG | (ADDRESS_MASK & addr));
+	u16 buf_value = value;
+	struct spi_message msg;
+	struct spi_transfer tx[] = {
+		{
+			.tx_buf = &buf_addr,
+			.len = 2,
+			.delay_usecs = 1,
+		}, {
+			.tx_buf = &buf_value,
+			.len = 2,
+			.delay_usecs = 1,
+		},
+	};
+
+	spi_message_init(&msg);
+	spi_message_add_tail(&tx[0], &msg);
+	spi_message_add_tail(&tx[1], &msg);
+	ret = spi_sync(spi, &msg);
+
+	return ret;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int gsxxxx_g_register(struct v4l2_subdev *sd,
+			     struct v4l2_dbg_register *reg)
+{
+	struct spi_device *spi = v4l2_get_subdevdata(sd);
+	u16 val;
+	int ret;
+
+	ret = gsxxxx_read_register(spi, reg->reg & 0xFFFF, &val);
+	reg->val = val;
+	reg->size = 2;
+	return ret;
+}
+
+static int gsxxxx_s_register(struct v4l2_subdev *sd,
+			     struct v4l2_dbg_register *reg)
+{
+	struct spi_device *spi = v4l2_get_subdevdata(sd);
+
+	return gsxxxx_write_register(spi, reg->reg & 0xFFFF,
+				     reg->val & 0xFFFF);
+}
+#endif
+
+static void custom_to_timings(const struct gsxxxx_reg_fmt_custom *custom,
+			      struct v4l2_dv_timings *timings)
+{
+	timings->type = V4L2_DV_BT_656_1120;
+	timings->bt.width = custom->width;
+	timings->bt.height = custom->height;
+	timings->bt.pixelclock = custom->pixelclock;
+	timings->bt.interlaced = custom->interlaced;
+	timings->bt.polarities = 0;
+	timings->bt.hbackporch = 0;
+	timings->bt.hsync = 0;
+	timings->bt.hfrontporch = 0;
+	timings->bt.vbackporch = 0;
+	timings->bt.vsync = 0;
+	timings->bt.vfrontporch = 0;
+	timings->bt.il_vbackporch = 0;
+	timings->bt.il_vsync = 0;
+	timings->bt.il_vfrontporch = 0;
+	timings->bt.standards = 0;
+	timings->bt.flags = 0;
+}
+
+static int find_custom(struct v4l2_dv_timings *timings)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(reg_fmt_custom); i++) {
+		if (timings->bt.width == reg_fmt_custom[i].width
+		    && timings->bt.height == reg_fmt_custom[i].height
+		    && timings->bt.interlaced == reg_fmt_custom[i].interlaced
+		    && timings->bt.pixelclock == reg_fmt_custom[i].pixelclock)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int gsxxxx_status_format(u16 status, struct v4l2_dv_timings *timings)
+{
+	int std = (status & MASK_STD_STATUS) >> 5;
+	int i;
+
+	/* We parse CEA formats first */
+	for (i = 0; i < ARRAY_SIZE(reg_fmt); i++) {
+		if (reg_fmt[i].reg_value == std) {
+			*timings = reg_fmt[i].format;
+			return 0;
+		}
+	}
+
+	/* Then custom formats */
+	for (i = 0; i < ARRAY_SIZE(reg_fmt_custom); i++) {
+		if (reg_fmt_custom[i].reg_value == std) {
+			custom_to_timings(&reg_fmt_custom[i], timings);
+			return 0;
+		}
+	}
+
+	return -ERANGE;
+}
+
+static u16 get_register_timings(struct v4l2_dv_timings *timings)
+{
+	int i, ret;
+
+	/* We parse CEA formats first */
+	for (i = 0; i < ARRAY_SIZE(reg_fmt); i++) {
+		if (v4l2_match_dv_timings(timings, &reg_fmt[i].format,
+					  0, false))
+			return reg_fmt[i].reg_value | MASK_FORCE_STD;
+	}
+
+	/* Then custom formats */
+	ret = find_custom(timings);
+	if (ret >= 0)
+		return reg_fmt_custom[ret].reg_value | MASK_FORCE_STD;
+
+	return 0x0;
+}
+
+static inline struct gsxxxx *to_gsxxxx(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct gsxxxx, sd);
+}
+
+static int gsxxxx_reset(struct v4l2_subdev *sd, u32 val)
+{
+	struct gsxxxx *gs = to_gsxxxx(sd);
+
+	/* To renable auto-detection mode */
+	return gsxxxx_write_register(gs->pdev, REG_FORCE_FMT, 0x0);
+}
+
+static int gsxxxx_s_dv_timings(struct v4l2_subdev *sd,
+			       struct v4l2_dv_timings *timings)
+{
+	struct gsxxxx *gs = to_gsxxxx(sd);
+	int reg_value;
+
+	reg_value = get_register_timings(timings);
+	if (reg_value == 0x0)
+		return -EINVAL;
+
+	gs->current_timings = *timings;
+	return gsxxxx_write_register(gs->pdev, REG_FORCE_FMT, reg_value);
+}
+
+static int gsxxxx_g_dv_timings(struct v4l2_subdev *sd,
+			       struct v4l2_dv_timings *timings)
+{
+	struct gsxxxx *gs = to_gsxxxx(sd);
+
+	*timings = gs->current_timings;
+	return 0;
+}
+
+static int gsxxxx_query_dv_timings(struct v4l2_subdev *sd,
+				   struct v4l2_dv_timings *timings)
+{
+	struct gsxxxx *gs = to_gsxxxx(sd);
+	struct v4l2_dv_timings fmt;
+	u16 reg_value, i;
+	int ret;
+
+	/* If the device detects video (pixels, lines or fields) */
+	for (i = 0; i < 4; i++) {
+		gsxxxx_read_register(gs->pdev, REG_LINES_PER_FRAME + i,
+				     &reg_value);
+		if (reg_value)
+			break;
+	}
+
+	if (i >= 4)
+		return -ENOLINK;
+
+	/* Video locked */
+	gsxxxx_read_register(gs->pdev, REG_STATUS, &reg_value);
+	if (!(reg_value & MASK_H_LOCK) || !(reg_value & MASK_V_LOCK))
+		return -ENOLCK;
+	if (!(reg_value & MASK_STD_LOCK))
+		return -ERANGE;
+
+	/* Video format */
+	ret = gsxxxx_status_format(reg_value, &fmt);
+
+	if (ret < 0)
+		return ret;
+
+	*timings = fmt;
+	return 0;
+}
+
+static int gsxxxx_enum_dv_timings(struct v4l2_subdev *sd,
+				  struct v4l2_enum_dv_timings *timings)
+{
+	if (timings->index >= ARRAY_SIZE(reg_fmt))
+		return -EINVAL;
+
+	timings->timings = reg_fmt[timings->index].format;
+	return 0;
+}
+
+static int gsxxxx_dv_timings_cap(struct v4l2_subdev *sd,
+				 struct v4l2_dv_timings_cap *cap)
+{
+	if (cap->pad != 0)
+		return -EINVAL;
+
+	*cap = gsxxxx_timings_cap;
+	return 0;
+}
+
+/* V4L2 core operation handlers */
+static const struct v4l2_subdev_core_ops gsxxxx_core_ops = {
+	.reset = gsxxxx_reset,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register = gsxxxx_g_register,
+	.s_register = gsxxxx_s_register,
+#endif
+};
+
+static const struct v4l2_subdev_video_ops gsxxxx_video_ops = {
+	.s_dv_timings = gsxxxx_s_dv_timings,
+	.g_dv_timings = gsxxxx_g_dv_timings,
+	.query_dv_timings = gsxxxx_query_dv_timings,
+};
+
+static const struct v4l2_subdev_pad_ops gsxxxx_pad_ops = {
+	.enum_dv_timings = gsxxxx_enum_dv_timings,
+	.dv_timings_cap = gsxxxx_dv_timings_cap,
+};
+
+/* V4L2 top level operation handlers */
+static const struct v4l2_subdev_ops gsxxxx_ops = {
+	.core = &gsxxxx_core_ops,
+	.video = &gsxxxx_video_ops,
+	.pad = &gsxxxx_pad_ops,
+};
+
+static int gsxxxx_probe(struct spi_device *spi)
+{
+	int ret;
+	struct gsxxxx *gs;
+	struct v4l2_subdev *sd;
+	struct v4l2_dv_timings timings;
+
+	gs = devm_kzalloc(&spi->dev, sizeof(struct gsxxxx), GFP_KERNEL);
+	if (!gs)
+		return -ENOMEM;
+
+	gs->pdev = spi;
+	sd = &gs->sd;
+
+	spi->mode = SPI_MODE_0;
+	spi->irq = -1;
+	spi->max_speed_hz = 10000000;
+	spi->bits_per_word = 16;
+	ret = spi_setup(spi);
+
+	if (ret)
+		return ret;
+
+	v4l2_spi_subdev_init(sd, spi, &gsxxxx_ops);
+
+	/* Default timing is NULL -> Auto detection */
+	custom_to_timings(&reg_fmt_custom[0], &timings);
+	gs->current_timings = timings;
+
+	/* Set H_CONFIG to SMPTE timings */
+	gsxxxx_write_register(spi, 0x0, 0x100);
+
+	return 0;
+}
+
+static int gsxxxx_remove(struct spi_device *spi)
+{
+	struct v4l2_subdev *sd = spi_get_drvdata(spi);
+	struct gsxxxx *gs = to_gsxxxx(sd);
+
+	v4l2_device_unregister_subdev(sd);
+	kfree(gs);
+	return 0;
+}
+
+static struct spi_driver gsxxxx_driver = {
+	.driver = {
+		.name		= "gs1662",
+		.owner		= THIS_MODULE,
+	},
+
+	.probe		= gsxxxx_probe,
+	.remove		= gsxxxx_remove,
+	.id_table	= gsxxxx_id,
+};
+
+static int __init gsxxxx_init(void)
+{
+	spi_register_driver(&gsxxxx_driver);
+	return 0;
+}
+
+static void __exit gsxxxx_exit(void)
+{
+	spi_unregister_driver(&gsxxxx_driver);
+}
+
+module_init(gsxxxx_init);
+module_exit(gsxxxx_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>");
+MODULE_DESCRIPTION("GSXXXX SPI driver to read and write its registers");
-- 
2.5.5

Re: [PATCH v2] Add GS driver (SPI video serializer family)

[Hans Verkuil]

Hi Charles-Antoine,

On 04/28/2016 04:10 PM, Charles-Antoine Couret wrote:
> Hello,
> Here my second patch version about GS serializer.
> It should be easy to add support for GS1582 and GS1572.
> I am not sure about V4L2 interfaces to set/get timings.
> I tested with GS1662 component and v4l2-dbg and v4l2-ctl tools.
> 
> But this component family support CEA standards and other
> (SMPTE XXXM in fact). V4L2 seems oriented to manage CEA or
> VGA formats. So, I used timings structure with CEA values, but I
> fill timings fields manually for other standards. I don't know if it
> is the right method or if another interface should be more interesting.

As long as the timings are part of a standard, then just add them to
the v4l2-dv-timings.h header. Since these timings aren't part of the CEA-861
standard or the DMT VESA standard, just add a new SMPTE standard flag.

> 
> And I used the reset method which seems deprecated. But for this
> component, it should be the right way to reset in auto-detection mode
> instead of "timings forced by user".
> 
> Thank you in advance for your comments.
> Regards.
> Charles-Antoine Couret
> 
> From a1bc59b8b18dc75bbf3a70483f57d4ccd190b5f9 Mon Sep 17 00:00:00 2001
> From: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
> Date: Fri, 1 Apr 2016 17:19:26 +0200
> Subject: [PATCH] Add GSXXXX driver (SPI video serializer family)
> 
> This patch was tested with GS1662:
> http://www.c-dis.net/media/871/GS1662_Datasheet.pdf

A pointer to this datasheet should be in a comment in the source code.

> 
> It is a v4l2-subdev which serializes some CEA formats
> and other. The driver could receive some custom timings and
> other supported format.
> 
> Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
> ---
>  drivers/media/Kconfig      |   1 +
>  drivers/media/Makefile     |   2 +-
>  drivers/media/spi/Kconfig  |   5 +
>  drivers/media/spi/Makefile |   1 +
>  drivers/media/spi/gsxxxx.c | 482 +++++++++++++++++++++++++++++++++++++++++++++

I would just call it gs1662. That's all you've tested with, after all.

It is very common that drivers named after the first supported model also
support similar models.

>  5 files changed, 490 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/spi/Kconfig
>  create mode 100644 drivers/media/spi/Makefile
>  create mode 100644 drivers/media/spi/gsxxxx.c
> 
> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> index a8518fb..d2fa6e7 100644
> --- a/drivers/media/Kconfig
> +++ b/drivers/media/Kconfig
> @@ -215,5 +215,6 @@ config MEDIA_ATTACH
>  source "drivers/media/i2c/Kconfig"
>  source "drivers/media/tuners/Kconfig"
>  source "drivers/media/dvb-frontends/Kconfig"
> +source "drivers/media/spi/Kconfig"
>  
>  endif # MEDIA_SUPPORT
> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
> index e608bbc..75bc82e 100644
> --- a/drivers/media/Makefile
> +++ b/drivers/media/Makefile
> @@ -28,6 +28,6 @@ obj-y += rc/
>  # Finally, merge the drivers that require the core
>  #
>  
> -obj-y += common/ platform/ pci/ usb/ mmc/ firewire/
> +obj-y += common/ platform/ pci/ usb/ mmc/ firewire/ spi/
>  obj-$(CONFIG_VIDEO_DEV) += radio/
>  
> diff --git a/drivers/media/spi/Kconfig b/drivers/media/spi/Kconfig
> new file mode 100644
> index 0000000..19a257c
> --- /dev/null
> +++ b/drivers/media/spi/Kconfig
> @@ -0,0 +1,5 @@
> +config VIDEO_GSXXXX
> +	tristate "Gennum Serializers video"
> +	depends on SPI
> +	---help---
> +	  Enable the GSXXXX driver which serializes video streams.
> diff --git a/drivers/media/spi/Makefile b/drivers/media/spi/Makefile
> new file mode 100644
> index 0000000..a67df8d
> --- /dev/null
> +++ b/drivers/media/spi/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_GSXXXX) += gsxxxx.o
> diff --git a/drivers/media/spi/gsxxxx.c b/drivers/media/spi/gsxxxx.c
> new file mode 100644
> index 0000000..0745ef9
> --- /dev/null
> +++ b/drivers/media/spi/gsxxxx.c
> @@ -0,0 +1,482 @@
> +/*
> + * GSXXXX device registration. Tested with GS1662 device.
> + *
> + * Copyright (C) 2015-2016 Nexvision
> + * Author: Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/platform_device.h>
> +#include <linux/ctype.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-dv-timings.h>
> +#include <linux/v4l2-dv-timings.h>
> +
> +
> +#define REG_STATUS			0x04
> +#define REG_FORCE_FMT			0x06
> +#define REG_LINES_PER_FRAME		0x12
> +#define REG_WORDS_PER_LINE		0x13
> +#define REG_WORDS_PER_ACT_LINE		0x14
> +#define REG_ACT_LINES_PER_FRAME	0x15
> +
> +#define MASK_H_LOCK		0x001
> +#define MASK_V_LOCK		0x002
> +#define MASK_STD_LOCK		0x004
> +#define MASK_FORCE_STD		0x020
> +#define MASK_STD_STATUS	0x3E0
> +#define ADDRESS_MASK		0x0FFF
> +
> +#define GS_WIDTH_MIN		0
> +#define GS_WIDTH_MAX		2048
> +#define GS_HEIGHT_MIN		0
> +#define GS_HEIGHT_MAX		1080
> +#define GS_PIXELCLOCK_MIN	10519200
> +#define GS_PIXELCLOCK_MAX	74250000
> +
> +#define READ_FLAG	0x8000
> +#define WRITE_FLAG	0x0000
> +#define BURST_FLAG	0x1000
> +
> +struct gsxxxx {

The gsxxxx prefix is rather ugly. I'd just use gs_ instead.

> +	struct spi_device *pdev;
> +	struct v4l2_subdev sd;
> +	struct v4l2_dv_timings current_timings;
> +};
> +
> +struct gsxxxx_reg_fmt {
> +	u16 reg_value;
> +	struct v4l2_dv_timings format;
> +};
> +
> +struct gsxxxx_reg_fmt_custom {
> +	u16 reg_value;
> +	__u32 width;
> +	__u32 height;
> +	__u64 pixelclock;
> +	__u32 interlaced;
> +};
> +
> +static const struct spi_device_id gsxxxx_id[] = {
> +	{ "gs1662", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(pdev, gsxxxx_id);
> +
> +static const struct gsxxxx_reg_fmt reg_fmt[] = {
> +	{ 0x00, V4L2_DV_BT_CEA_1280X720P60 },
> +	{ 0x01, V4L2_DV_BT_CEA_1280X720P60 },
> +	{ 0x02, V4L2_DV_BT_CEA_1280X720P30 },
> +	{ 0x03, V4L2_DV_BT_CEA_1280X720P30 },
> +	{ 0x04, V4L2_DV_BT_CEA_1280X720P50 },
> +	{ 0x05, V4L2_DV_BT_CEA_1280X720P50 },
> +	{ 0x06, V4L2_DV_BT_CEA_1280X720P25 },
> +	{ 0x07, V4L2_DV_BT_CEA_1280X720P25 },
> +	{ 0x08, V4L2_DV_BT_CEA_1280X720P24 },
> +	{ 0x09, V4L2_DV_BT_CEA_1280X720P24 },
> +	{ 0x0A, V4L2_DV_BT_CEA_1920X1080I60 },
> +	{ 0x0B, V4L2_DV_BT_CEA_1920X1080P30 },
> +
> +	/* Default value: keep this field before 0xC */
> +	{ 0x14, V4L2_DV_BT_CEA_1920X1080I50 },
> +	{ 0x0C, V4L2_DV_BT_CEA_1920X1080I50 },
> +	{ 0x0D, V4L2_DV_BT_CEA_1920X1080P25 },
> +	{ 0x0E, V4L2_DV_BT_CEA_1920X1080P25},
> +	{ 0x10, V4L2_DV_BT_CEA_1920X1080P24 },
> +	{ 0x12, V4L2_DV_BT_CEA_1920X1080P24 },
> +	{ 0x18, V4L2_DV_BT_CEA_720X576P50 },
> +	{ 0x1A, V4L2_DV_BT_CEA_720X576P50 },
> +};
> +
> +/* Non CEA standard values supported by this component family.
> + * It does not care about *porch information.
> + */
> +static const struct gsxxxx_reg_fmt_custom reg_fmt_custom[] = {
> +	/* NULL value which means auto-detection mode or error */
> +	{ 0xFF, 0, 0, 0 },
> +	{ 0x0F, 1920, 1080, 25920000, 1 },
> +	{ 0x11, 1920, 1080, 24883200, 1 },
> +	{ 0x13, 1920, 1080, 24883200, 1 },
> +	{ 0x15, 1920, 1035, 59616000, 1 },
> +	{ 0x16, 720, 487, 10519200, 1 },
> +	{ 0x17, 720, 507, 10951200, 1 },
> +	{ 0x19, 720, 487, 10519200, 1 },
> +	{ 0x1B, 720, 507, 10951200, 1 },
> +	{ 0x1C, 2048, 1080, 55296000, 0 },
> +};
> +
> +static const struct v4l2_dv_timings_cap gsxxxx_timings_cap = {
> +	.type = V4L2_DV_BT_656_1120,
> +	/* keep this initialization for compatibility with GCC < 4.4.6 */
> +	.reserved = { 0 },
> +	V4L2_INIT_BT_TIMINGS(GS_WIDTH_MIN, GS_WIDTH_MAX, GS_HEIGHT_MIN,
> +			     GS_HEIGHT_MAX, GS_PIXELCLOCK_MIN,
> +			     GS_PIXELCLOCK_MAX, V4L2_DV_BT_STD_CEA861,
> +			     V4L2_DV_BT_CAP_PROGRESSIVE
> +			     | V4L2_DV_BT_CAP_INTERLACED
> +			     | V4L2_DV_BT_CAP_CUSTOM)
> +};
> +
> +static int gsxxxx_read_register(struct spi_device *spi, u16 addr, u16 *value)
> +{
> +	int ret;
> +	u16 buf_addr = (READ_FLAG | (ADDRESS_MASK & addr));
> +	u16 buf_value = 0;
> +	struct spi_message msg;
> +	struct spi_transfer tx[] = {
> +		{
> +			.tx_buf = &buf_addr,
> +			.len = 2,
> +			.delay_usecs = 1,
> +		}, {
> +			.rx_buf = &buf_value,
> +			.len = 2,
> +			.delay_usecs = 1,
> +		},
> +	};
> +
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&tx[0], &msg);
> +	spi_message_add_tail(&tx[1], &msg);
> +	ret = spi_sync(spi, &msg);
> +
> +	*value = buf_value;
> +
> +	return ret;
> +}
> +
> +static int gsxxxx_write_register(struct spi_device *spi, u16 addr, u16 value)
> +{
> +	int ret;
> +	u16 buf_addr = (WRITE_FLAG | (ADDRESS_MASK & addr));
> +	u16 buf_value = value;
> +	struct spi_message msg;
> +	struct spi_transfer tx[] = {
> +		{
> +			.tx_buf = &buf_addr,
> +			.len = 2,
> +			.delay_usecs = 1,
> +		}, {
> +			.tx_buf = &buf_value,
> +			.len = 2,
> +			.delay_usecs = 1,
> +		},
> +	};
> +
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&tx[0], &msg);
> +	spi_message_add_tail(&tx[1], &msg);
> +	ret = spi_sync(spi, &msg);
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int gsxxxx_g_register(struct v4l2_subdev *sd,
> +			     struct v4l2_dbg_register *reg)
> +{
> +	struct spi_device *spi = v4l2_get_subdevdata(sd);
> +	u16 val;
> +	int ret;
> +
> +	ret = gsxxxx_read_register(spi, reg->reg & 0xFFFF, &val);
> +	reg->val = val;
> +	reg->size = 2;
> +	return ret;
> +}
> +
> +static int gsxxxx_s_register(struct v4l2_subdev *sd,
> +			     struct v4l2_dbg_register *reg)
> +{
> +	struct spi_device *spi = v4l2_get_subdevdata(sd);
> +
> +	return gsxxxx_write_register(spi, reg->reg & 0xFFFF,
> +				     reg->val & 0xFFFF);
> +}
> +#endif
> +
> +static void custom_to_timings(const struct gsxxxx_reg_fmt_custom *custom,
> +			      struct v4l2_dv_timings *timings)
> +{
> +	timings->type = V4L2_DV_BT_656_1120;
> +	timings->bt.width = custom->width;
> +	timings->bt.height = custom->height;
> +	timings->bt.pixelclock = custom->pixelclock;
> +	timings->bt.interlaced = custom->interlaced;
> +	timings->bt.polarities = 0;
> +	timings->bt.hbackporch = 0;
> +	timings->bt.hsync = 0;
> +	timings->bt.hfrontporch = 0;
> +	timings->bt.vbackporch = 0;
> +	timings->bt.vsync = 0;
> +	timings->bt.vfrontporch = 0;
> +	timings->bt.il_vbackporch = 0;
> +	timings->bt.il_vsync = 0;
> +	timings->bt.il_vfrontporch = 0;

You still need to set the total blanking sizes, right?

For now assign that to the [hv]frontporch, leaving the sync and
backporch fields 0. I need to make some rules how this is handled when
the standard doesn't separate the blanking into back/frontporch and syncs.

> +	timings->bt.standards = 0;

So we need to define a proper standard for this.

> +	timings->bt.flags = 0;
> +}
> +
> +static int find_custom(struct v4l2_dv_timings *timings)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(reg_fmt_custom); i++) {
> +		if (timings->bt.width == reg_fmt_custom[i].width
> +		    && timings->bt.height == reg_fmt_custom[i].height
> +		    && timings->bt.interlaced == reg_fmt_custom[i].interlaced
> +		    && timings->bt.pixelclock == reg_fmt_custom[i].pixelclock)
> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int gsxxxx_status_format(u16 status, struct v4l2_dv_timings *timings)
> +{
> +	int std = (status & MASK_STD_STATUS) >> 5;
> +	int i;
> +
> +	/* We parse CEA formats first */
> +	for (i = 0; i < ARRAY_SIZE(reg_fmt); i++) {
> +		if (reg_fmt[i].reg_value == std) {
> +			*timings = reg_fmt[i].format;
> +			return 0;
> +		}
> +	}
> +
> +	/* Then custom formats */
> +	for (i = 0; i < ARRAY_SIZE(reg_fmt_custom); i++) {
> +		if (reg_fmt_custom[i].reg_value == std) {
> +			custom_to_timings(&reg_fmt_custom[i], timings);
> +			return 0;
> +		}
> +	}
> +
> +	return -ERANGE;
> +}
> +
> +static u16 get_register_timings(struct v4l2_dv_timings *timings)
> +{
> +	int i, ret;
> +
> +	/* We parse CEA formats first */
> +	for (i = 0; i < ARRAY_SIZE(reg_fmt); i++) {
> +		if (v4l2_match_dv_timings(timings, &reg_fmt[i].format,
> +					  0, false))
> +			return reg_fmt[i].reg_value | MASK_FORCE_STD;
> +	}
> +
> +	/* Then custom formats */
> +	ret = find_custom(timings);
> +	if (ret >= 0)
> +		return reg_fmt_custom[ret].reg_value | MASK_FORCE_STD;
> +
> +	return 0x0;
> +}
> +
> +static inline struct gsxxxx *to_gsxxxx(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct gsxxxx, sd);
> +}
> +
> +static int gsxxxx_reset(struct v4l2_subdev *sd, u32 val)
> +{
> +	struct gsxxxx *gs = to_gsxxxx(sd);
> +
> +	/* To renable auto-detection mode */
> +	return gsxxxx_write_register(gs->pdev, REG_FORCE_FMT, 0x0);
> +}
> +
> +static int gsxxxx_s_dv_timings(struct v4l2_subdev *sd,
> +			       struct v4l2_dv_timings *timings)
> +{
> +	struct gsxxxx *gs = to_gsxxxx(sd);
> +	int reg_value;
> +
> +	reg_value = get_register_timings(timings);
> +	if (reg_value == 0x0)
> +		return -EINVAL;
> +
> +	gs->current_timings = *timings;
> +	return gsxxxx_write_register(gs->pdev, REG_FORCE_FMT, reg_value);
> +}
> +
> +static int gsxxxx_g_dv_timings(struct v4l2_subdev *sd,
> +			       struct v4l2_dv_timings *timings)
> +{
> +	struct gsxxxx *gs = to_gsxxxx(sd);
> +
> +	*timings = gs->current_timings;
> +	return 0;
> +}
> +
> +static int gsxxxx_query_dv_timings(struct v4l2_subdev *sd,
> +				   struct v4l2_dv_timings *timings)
> +{
> +	struct gsxxxx *gs = to_gsxxxx(sd);
> +	struct v4l2_dv_timings fmt;
> +	u16 reg_value, i;
> +	int ret;
> +
> +	/* If the device detects video (pixels, lines or fields) */
> +	for (i = 0; i < 4; i++) {
> +		gsxxxx_read_register(gs->pdev, REG_LINES_PER_FRAME + i,
> +				     &reg_value);
> +		if (reg_value)
> +			break;
> +	}
> +
> +	if (i >= 4)
> +		return -ENOLINK;
> +
> +	/* Video locked */
> +	gsxxxx_read_register(gs->pdev, REG_STATUS, &reg_value);
> +	if (!(reg_value & MASK_H_LOCK) || !(reg_value & MASK_V_LOCK))
> +		return -ENOLCK;
> +	if (!(reg_value & MASK_STD_LOCK))
> +		return -ERANGE;
> +
> +	/* Video format */
> +	ret = gsxxxx_status_format(reg_value, &fmt);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*timings = fmt;
> +	return 0;
> +}

So, regarding the reset, s_dv_timings and query_dv_timings: it's not clear
what is happening here. The usual way things work is that the timings that
s/g_dv_timings set and get are indepedent of the timings that are detected
(query_dv_timings). The reason is that the explicitly set timings relate to
the buffers that the DMA engine needs to store the frames. Receivers that
spontaneously switch when new timings are detected can be very dangerous
depending on the details of the DMA engine (think buffer overruns when you
go from e.g. 720p to 1080p).

So typically when you set the timings the device is fixed to those timings,
even if it receives something different. If the device supports an interrupt,
then it is good practice to hook into that interrupt and, when it detects
that the timings changed, the device sends a V4L2_EVENT_SOURCE_CHANGE event.

Userspace will then typically stop streaming, query the new timings, setup
the new buffers and restart streaming.

Some devices cannot query the new timings unless they are in autodetect mode.
The correct implementation for that is that query_dv_timings returns EBUSY
while the device is streaming (you hook into the s_stream core op to know that),
otherwise it configures itself to autodetect mode and sees what is detected.

It is not really clear to me from the datasheet how this device behaves. But
having to use the reset op is almost certainly wrong.

> +
> +static int gsxxxx_enum_dv_timings(struct v4l2_subdev *sd,
> +				  struct v4l2_enum_dv_timings *timings)
> +{
> +	if (timings->index >= ARRAY_SIZE(reg_fmt))
> +		return -EINVAL;
> +
> +	timings->timings = reg_fmt[timings->index].format;

Hmm, there are duplicate format entries in the reg_fmt array. It would be
good if you can explain the differences between otherwise identical entries.
I would have to think about how those differences should be represented.

> +	return 0;
> +}
> +
> +static int gsxxxx_dv_timings_cap(struct v4l2_subdev *sd,
> +				 struct v4l2_dv_timings_cap *cap)
> +{
> +	if (cap->pad != 0)
> +		return -EINVAL;
> +
> +	*cap = gsxxxx_timings_cap;
> +	return 0;
> +}
> +
> +/* V4L2 core operation handlers */
> +static const struct v4l2_subdev_core_ops gsxxxx_core_ops = {
> +	.reset = gsxxxx_reset,
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	.g_register = gsxxxx_g_register,
> +	.s_register = gsxxxx_s_register,
> +#endif
> +};
> +
> +static const struct v4l2_subdev_video_ops gsxxxx_video_ops = {
> +	.s_dv_timings = gsxxxx_s_dv_timings,
> +	.g_dv_timings = gsxxxx_g_dv_timings,
> +	.query_dv_timings = gsxxxx_query_dv_timings,
> +};
> +
> +static const struct v4l2_subdev_pad_ops gsxxxx_pad_ops = {
> +	.enum_dv_timings = gsxxxx_enum_dv_timings,
> +	.dv_timings_cap = gsxxxx_dv_timings_cap,
> +};
> +
> +/* V4L2 top level operation handlers */
> +static const struct v4l2_subdev_ops gsxxxx_ops = {
> +	.core = &gsxxxx_core_ops,
> +	.video = &gsxxxx_video_ops,
> +	.pad = &gsxxxx_pad_ops,
> +};
> +
> +static int gsxxxx_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct gsxxxx *gs;
> +	struct v4l2_subdev *sd;
> +	struct v4l2_dv_timings timings;
> +
> +	gs = devm_kzalloc(&spi->dev, sizeof(struct gsxxxx), GFP_KERNEL);
> +	if (!gs)
> +		return -ENOMEM;
> +
> +	gs->pdev = spi;
> +	sd = &gs->sd;
> +
> +	spi->mode = SPI_MODE_0;
> +	spi->irq = -1;
> +	spi->max_speed_hz = 10000000;
> +	spi->bits_per_word = 16;
> +	ret = spi_setup(spi);
> +
> +	if (ret)
> +		return ret;
> +
> +	v4l2_spi_subdev_init(sd, spi, &gsxxxx_ops);
> +
> +	/* Default timing is NULL -> Auto detection */
> +	custom_to_timings(&reg_fmt_custom[0], &timings);
> +	gs->current_timings = timings;
> +
> +	/* Set H_CONFIG to SMPTE timings */
> +	gsxxxx_write_register(spi, 0x0, 0x100);
> +
> +	return 0;
> +}
> +
> +static int gsxxxx_remove(struct spi_device *spi)
> +{
> +	struct v4l2_subdev *sd = spi_get_drvdata(spi);
> +	struct gsxxxx *gs = to_gsxxxx(sd);
> +
> +	v4l2_device_unregister_subdev(sd);
> +	kfree(gs);
> +	return 0;
> +}
> +
> +static struct spi_driver gsxxxx_driver = {
> +	.driver = {
> +		.name		= "gs1662",
> +		.owner		= THIS_MODULE,
> +	},
> +
> +	.probe		= gsxxxx_probe,
> +	.remove		= gsxxxx_remove,
> +	.id_table	= gsxxxx_id,
> +};
> +
> +static int __init gsxxxx_init(void)
> +{
> +	spi_register_driver(&gsxxxx_driver);
> +	return 0;
> +}
> +
> +static void __exit gsxxxx_exit(void)
> +{
> +	spi_unregister_driver(&gsxxxx_driver);
> +}
> +
> +module_init(gsxxxx_init);
> +module_exit(gsxxxx_exit);

Use module_spi_driver here.

> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>");
> +MODULE_DESCRIPTION("GSXXXX SPI driver to read and write its registers");

That's rather vague. How about: "Gennum GS1662 HD/SD-SDI Serializer driver".

Regards,

	Hans

Re: [PATCH v2] Add GS driver (SPI video serializer family)

[Charles-Antoine Couret]

Le 04/05/2016 à 13:41, Hans Verkuil a écrit :
> Hi Charles-Antoine,

Hi,

> On 04/28/2016 04:10 PM, Charles-Antoine Couret wrote:
>> But this component family support CEA standards and other
>> (SMPTE XXXM in fact). V4L2 seems oriented to manage CEA or
>> VGA formats. So, I used timings structure with CEA values, but I
>> fill timings fields manually for other standards. I don't know if it
>> is the right method or if another interface should be more interesting.
> 
> As long as the timings are part of a standard, then just add them to
> the v4l2-dv-timings.h header. Since these timings aren't part of the CEA-861
> standard or the DMT VESA standard, just add a new SMPTE standard flag.

Ok, but I should have difficulties to define correctly these standards.
I worked on video stream in SMPTE-125M and I don't know if other SMPTE standards are based on the same characteristics.
In addition to this, those standards are not public.

For the SMPTE-125M, I have these information:
* Pixelclock : 27 MHz (I set 13.5MHz to have 60 FPS because its a interlaced signal, I don't know if it's correct)
* The organization of lines :
Line 1 to 9 : blanking
Line 10 to 19 : options (blanking in my case for example)
Line 20 to 264 : field 1
Line 266 to 272 : blanking
Line 273 to 282 : options
Line 283 to 525 : field 2

The time of blanking are not regular: 19 then 18 lines, how translate that in dv_timings?
The size of fields is different too.

The Field signal is changed in 266 line.
* Complete format size (with blanking) ; 858x525
* Image size : 720x487

Organization of horizontal sync :
Pixel 0 to 719 : active pixels
Pixel 720 to 857 : blanking (but the firsts 16 pixels are the front porch, but after, no info for sync or back porch timings)

Polarity: V-, H+

And, I don't have info for other standards and the GS1662 don't need that too.
I should create SMPTE format but only for the 125M? And the driver shouldn't use / consider other SMPTE formats without a right definition?

>> This patch was tested with GS1662:
>> http://www.c-dis.net/media/871/GS1662_Datasheet.pdf
> 
> A pointer to this datasheet should be in a comment in the source code.

Ok. The commit message should keep the link too?


>>  drivers/media/spi/gsxxxx.c | 482 +++++++++++++++++++++++++++++++++++++++++++++
> 
> I would just call it gs1662. That's all you've tested with, after all.
> 
> It is very common that drivers named after the first supported model also
> support similar models.

Ok, thanks.

>> +struct gsxxxx {
> 
> The gsxxxx prefix is rather ugly. I'd just use gs_ instead.

Yes, I agree with you.

>> +static void custom_to_timings(const struct gsxxxx_reg_fmt_custom *custom,
>> +			      struct v4l2_dv_timings *timings)
>> +{
>> +	timings->type = V4L2_DV_BT_656_1120;
>> +	timings->bt.width = custom->width;
>> +	timings->bt.height = custom->height;
>> +	timings->bt.pixelclock = custom->pixelclock;
>> +	timings->bt.interlaced = custom->interlaced;
>> +	timings->bt.polarities = 0;
>> +	timings->bt.hbackporch = 0;
>> +	timings->bt.hsync = 0;
>> +	timings->bt.hfrontporch = 0;
>> +	timings->bt.vbackporch = 0;
>> +	timings->bt.vsync = 0;
>> +	timings->bt.vfrontporch = 0;
>> +	timings->bt.il_vbackporch = 0;
>> +	timings->bt.il_vsync = 0;
>> +	timings->bt.il_vfrontporch = 0;
> 
> You still need to set the total blanking sizes, right?
> 
> For now assign that to the [hv]frontporch, leaving the sync and
> backporch fields 0. I need to make some rules how this is handled when
> the standard doesn't separate the blanking into back/frontporch and syncs.

Seeing my first comment.
I could precise some info (for 125M) but not for all of them.
And the GS1662 don't care about those information: we can't configure timings, only ask a specific format.

And, how manage the case of there are two different timings for vertical blanking for one image in the standard?

>> +	timings->bt.standards = 0;
> 
> So we need to define a proper standard for this.

Yes.
 
> So, regarding the reset, s_dv_timings and query_dv_timings: it's not clear
> what is happening here. The usual way things work is that the timings that
> s/g_dv_timings set and get are indepedent of the timings that are detected
> (query_dv_timings). The reason is that the explicitly set timings relate to
> the buffers that the DMA engine needs to store the frames. Receivers that
> spontaneously switch when new timings are detected can be very dangerous
> depending on the details of the DMA engine (think buffer overruns when you
> go from e.g. 720p to 1080p).

It's the case here.
s/g_dv_timings are independent of query_detect_timings which reads internal registers to
define the stream detected by the component.

The reset function are an error, I think.
By default the GS1662 is in auto-mode: it detects the input stream to create the serialized output stream.
The reset was to return in auto-mode selection, but this function should be to reset the component and not the mode.

I don't have idea to define properly the auto-mode, for userspace and the driver.
It's a useful information and I think, the userspace should force this mode. Define a specific timings for that?

> So typically when you set the timings the device is fixed to those timings,
> even if it receives something different. If the device supports an interrupt,
> then it is good practice to hook into that interrupt and, when it detects
> that the timings changed, the device sends a V4L2_EVENT_SOURCE_CHANGE event.
> 
> Userspace will then typically stop streaming, query the new timings, setup
> the new buffers and restart streaming.

GS1662 don't have interruption line to do that.

> Some devices cannot query the new timings unless they are in autodetect mode.
> The correct implementation for that is that query_dv_timings returns EBUSY
> while the device is streaming (you hook into the s_stream core op to know that),
> otherwise it configures itself to autodetect mode and sees what is detected.
> 
> It is not really clear to me from the datasheet how this device behaves. But
> having to use the reset op is almost certainly wrong.

I don't understand.
The GS1662 has a status to say the input format detected. Useful in auto-detect mode,
less in other cases. But, it needs a input, why send EBUSY error when the device streams?

>> +static int gsxxxx_enum_dv_timings(struct v4l2_subdev *sd,
>> +				  struct v4l2_enum_dv_timings *timings)
>> +{
>> +	if (timings->index >= ARRAY_SIZE(reg_fmt))
>> +		return -EINVAL;
>> +
>> +	timings->timings = reg_fmt[timings->index].format;
> 
> Hmm, there are duplicate format entries in the reg_fmt array. It would be
> good if you can explain the differences between otherwise identical entries.
> I would have to think about how those differences should be represented.

Yes, I didn't paid attention to that. Thanks.
The format difference:
* Sometimes it's generic/specific format (like in SMPTE-125M: 487 lines vs (858x525) 720x487)
without information about the "generic" meaning.
* In other case it's normal vs "EM" standards (difference between HANC and active pixels size per line).

But the "EM" version is not CEA-861 compliant for me, but close to.
Maybe this info should be important for others and I should add details.

>> +static void __exit gsxxxx_exit(void)
>> +{
>> +	spi_unregister_driver(&gsxxxx_driver);
>> +}
>> +
>> +module_init(gsxxxx_init);
>> +module_exit(gsxxxx_exit);
> 
> Use module_spi_driver here.

Yes, thanks!
 
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>");
>> +MODULE_DESCRIPTION("GSXXXX SPI driver to read and write its registers");
> 
> That's rather vague. How about: "Gennum GS1662 HD/SD-SDI Serializer driver".

Ok.

Thank you for all your comments. I will improve with a v3 patch but I need some answers to do that correctly.
I'm sorry for some mistakes, I use this driver in precise use case and I didn't take account all others use cases to design that correctly.
And, like I'm beginner, it's difficult to me to decide how implement common interfaces (for SMPTE timings for example) which could be used by other drivers.

Regards.
Charles-Antoine Couret

Re: [PATCH v2] Add GS driver (SPI video serializer family)

[Hans Verkuil]

Hi Charles-Antoine,

On 05/04/2016 03:27 PM, Charles-Antoine Couret wrote:
> Le 04/05/2016 à 13:41, Hans Verkuil a écrit :
>> Hi Charles-Antoine,
> 
> Hi,
> 
>> On 04/28/2016 04:10 PM, Charles-Antoine Couret wrote:
>>> But this component family support CEA standards and other
>>> (SMPTE XXXM in fact). V4L2 seems oriented to manage CEA or
>>> VGA formats. So, I used timings structure with CEA values, but I
>>> fill timings fields manually for other standards. I don't know if it
>>> is the right method or if another interface should be more interesting.
>>
>> As long as the timings are part of a standard, then just add them to
>> the v4l2-dv-timings.h header. Since these timings aren't part of the CEA-861
>> standard or the DMT VESA standard, just add a new SMPTE standard flag.
> 
> Ok, but I should have difficulties to define correctly these standards.
> I worked on video stream in SMPTE-125M and I don't know if other SMPTE standards are based on the same characteristics.
> In addition to this, those standards are not public.

I have access to the SMPTE standards, I'll take a look next week.

Regarding timings: I think this requires a separate discussion. I need to loop in 'nohous'
who is also working on SDI support, but unfortunately I don't have his email handy, otherwise
I'd have CC-ed him.

I'm no SDI expert myself, but I think I should set time aside to read up on this
and figure out together with you guys how this should be handled.

So I don't have a quick answer here, this requires more R&D.

> 
> For the SMPTE-125M, I have these information:
> * Pixelclock : 27 MHz (I set 13.5MHz to have 60 FPS because its a interlaced signal, I don't know if it's correct)
> * The organization of lines :
> Line 1 to 9 : blanking
> Line 10 to 19 : options (blanking in my case for example)
> Line 20 to 264 : field 1
> Line 266 to 272 : blanking
> Line 273 to 282 : options
> Line 283 to 525 : field 2
> 
> The time of blanking are not regular: 19 then 18 lines, how translate that in dv_timings?
> The size of fields is different too.
> 
> The Field signal is changed in 266 line.
> * Complete format size (with blanking) ; 858x525
> * Image size : 720x487
> 
> Organization of horizontal sync :
> Pixel 0 to 719 : active pixels
> Pixel 720 to 857 : blanking (but the firsts 16 pixels are the front porch, but after, no info for sync or back porch timings)
> 
> Polarity: V-, H+
> 
> And, I don't have info for other standards and the GS1662 don't need that too.
> I should create SMPTE format but only for the 125M? And the driver shouldn't use / consider other SMPTE formats without a right definition?
> 
>>> This patch was tested with GS1662:
>>> http://www.c-dis.net/media/871/GS1662_Datasheet.pdf
>>
>> A pointer to this datasheet should be in a comment in the source code.
> 
> Ok. The commit message should keep the link too?

It doesn't hurt.

> 
> 
>>>  drivers/media/spi/gsxxxx.c | 482 +++++++++++++++++++++++++++++++++++++++++++++
>>
>> I would just call it gs1662. That's all you've tested with, after all.
>>
>> It is very common that drivers named after the first supported model also
>> support similar models.
> 
> Ok, thanks.
> 
>>> +struct gsxxxx {
>>
>> The gsxxxx prefix is rather ugly. I'd just use gs_ instead.
> 
> Yes, I agree with you.
> 
>>> +static void custom_to_timings(const struct gsxxxx_reg_fmt_custom *custom,
>>> +			      struct v4l2_dv_timings *timings)
>>> +{
>>> +	timings->type = V4L2_DV_BT_656_1120;
>>> +	timings->bt.width = custom->width;
>>> +	timings->bt.height = custom->height;
>>> +	timings->bt.pixelclock = custom->pixelclock;
>>> +	timings->bt.interlaced = custom->interlaced;
>>> +	timings->bt.polarities = 0;
>>> +	timings->bt.hbackporch = 0;
>>> +	timings->bt.hsync = 0;
>>> +	timings->bt.hfrontporch = 0;
>>> +	timings->bt.vbackporch = 0;
>>> +	timings->bt.vsync = 0;
>>> +	timings->bt.vfrontporch = 0;
>>> +	timings->bt.il_vbackporch = 0;
>>> +	timings->bt.il_vsync = 0;
>>> +	timings->bt.il_vfrontporch = 0;
>>
>> You still need to set the total blanking sizes, right?
>>
>> For now assign that to the [hv]frontporch, leaving the sync and
>> backporch fields 0. I need to make some rules how this is handled when
>> the standard doesn't separate the blanking into back/frontporch and syncs.
> 
> Seeing my first comment.
> I could precise some info (for 125M) but not for all of them.
> And the GS1662 don't care about those information: we can't configure timings, only ask a specific format.
> 
> And, how manage the case of there are two different timings for vertical blanking for one image in the standard?
> 
>>> +	timings->bt.standards = 0;
>>
>> So we need to define a proper standard for this.
> 
> Yes.
>  
>> So, regarding the reset, s_dv_timings and query_dv_timings: it's not clear
>> what is happening here. The usual way things work is that the timings that
>> s/g_dv_timings set and get are indepedent of the timings that are detected
>> (query_dv_timings). The reason is that the explicitly set timings relate to
>> the buffers that the DMA engine needs to store the frames. Receivers that
>> spontaneously switch when new timings are detected can be very dangerous
>> depending on the details of the DMA engine (think buffer overruns when you
>> go from e.g. 720p to 1080p).
> 
> It's the case here.
> s/g_dv_timings are independent of query_detect_timings which reads internal registers to
> define the stream detected by the component.
> 
> The reset function are an error, I think.
> By default the GS1662 is in auto-mode: it detects the input stream to create the serialized output stream.
> The reset was to return in auto-mode selection, but this function should be to reset the component and not the mode.
> 
> I don't have idea to define properly the auto-mode, for userspace and the driver.
> It's a useful information and I think, the userspace should force this mode. Define a specific timings for that?

I think that you can use the s_stream op here: when you start streaming you force
the mode to whatever the timings set by s_dv_timings() requires. When you stop streaming
you go back to auto-mode.

> 
>> So typically when you set the timings the device is fixed to those timings,
>> even if it receives something different. If the device supports an interrupt,
>> then it is good practice to hook into that interrupt and, when it detects
>> that the timings changed, the device sends a V4L2_EVENT_SOURCE_CHANGE event.
>>
>> Userspace will then typically stop streaming, query the new timings, setup
>> the new buffers and restart streaming.
> 
> GS1662 don't have interruption line to do that.
> 
>> Some devices cannot query the new timings unless they are in autodetect mode.
>> The correct implementation for that is that query_dv_timings returns EBUSY
>> while the device is streaming (you hook into the s_stream core op to know that),
>> otherwise it configures itself to autodetect mode and sees what is detected.
>>
>> It is not really clear to me from the datasheet how this device behaves. But
>> having to use the reset op is almost certainly wrong.
> 
> I don't understand.
> The GS1662 has a status to say the input format detected. Useful in auto-detect mode,
> less in other cases. But, it needs a input, why send EBUSY error when the device streams?

Hmm, I don't understand either :-)

The question is: when the device is streaming video for a specific format (as set
by s_dv_timings), can it still detect the actual video format it receives? If so,
then there is no need for EBUSY since query_dv_timings will always work. If not,
then query_dv_timings should report that it is unable to query the detected timings
because it is in the wrong mode (EBUSY).

BTW, you also need to implement the g_input_status video op. I just realized that
that is missing. It is used to fill in the status field when calling VIDIOC_ENUMINPUTS.

> 
>>> +static int gsxxxx_enum_dv_timings(struct v4l2_subdev *sd,
>>> +				  struct v4l2_enum_dv_timings *timings)
>>> +{
>>> +	if (timings->index >= ARRAY_SIZE(reg_fmt))
>>> +		return -EINVAL;
>>> +
>>> +	timings->timings = reg_fmt[timings->index].format;
>>
>> Hmm, there are duplicate format entries in the reg_fmt array. It would be
>> good if you can explain the differences between otherwise identical entries.
>> I would have to think about how those differences should be represented.
> 
> Yes, I didn't paid attention to that. Thanks.
> The format difference:
> * Sometimes it's generic/specific format (like in SMPTE-125M: 487 lines vs (858x525) 720x487)
> without information about the "generic" meaning.
> * In other case it's normal vs "EM" standards (difference between HANC and active pixels size per line).
> 
> But the "EM" version is not CEA-861 compliant for me, but close to.
> Maybe this info should be important for others and I should add details.
> 
>>> +static void __exit gsxxxx_exit(void)
>>> +{
>>> +	spi_unregister_driver(&gsxxxx_driver);
>>> +}
>>> +
>>> +module_init(gsxxxx_init);
>>> +module_exit(gsxxxx_exit);
>>
>> Use module_spi_driver here.
> 
> Yes, thanks!
>  
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_AUTHOR("Charles-Antoine Couret <charles-antoine.couret@nexvision.fr>");
>>> +MODULE_DESCRIPTION("GSXXXX SPI driver to read and write its registers");
>>
>> That's rather vague. How about: "Gennum GS1662 HD/SD-SDI Serializer driver".
> 
> Ok.
> 
> Thank you for all your comments. I will improve with a v3 patch but I need some answers to do that correctly.
> I'm sorry for some mistakes, I use this driver in precise use case and I didn't take account all others use cases to design that correctly.
> And, like I'm beginner, it's difficult to me to decide how implement common interfaces (for SMPTE timings for example) which could be used by other drivers.

Remember that today there are no SDI drivers in the kernel. So you and nohous are the first
that work on this. So there will be some missing pieces that we need to add. It seems that
for SDI the timings are one such area.

It will be useful if you join the #v4l irc room (http://linuxtv.org/lists.php).

I think that's a good place to have a meeting about this topic together with nohous. I'm
traveling for a bit but will be back on Tuesday. Perhaps we can schedule something later
that week.

Regards,

	Hans

Re: [PATCH v2] Add GS driver (SPI video serializer family)

[Charles-Antoine Couret]

Le 04/05/2016 à 15:46, Hans Verkuil a écrit :
>> Ok, but I should have difficulties to define correctly these standards.
>> I worked on video stream in SMPTE-125M and I don't know if other SMPTE standards are based on the same characteristics.
>> In addition to this, those standards are not public.
> 
> I have access to the SMPTE standards, I'll take a look next week.

Oh, nice. Thanks. :)

> Regarding timings: I think this requires a separate discussion. I need to loop in 'nohous'
> who is also working on SDI support, but unfortunately I don't have his email handy, otherwise
> I'd have CC-ed him.
> 
> I'm no SDI expert myself, but I think I should set time aside to read up on this
> and figure out together with you guys how this should be handled.
> 
> So I don't have a quick answer here, this requires more R&D.

Ok. I could help a little bit I think.

>>> So, regarding the reset, s_dv_timings and query_dv_timings: it's not clear
>>> what is happening here. The usual way things work is that the timings that
>>> s/g_dv_timings set and get are indepedent of the timings that are detected
>>> (query_dv_timings). The reason is that the explicitly set timings relate to
>>> the buffers that the DMA engine needs to store the frames. Receivers that
>>> spontaneously switch when new timings are detected can be very dangerous
>>> depending on the details of the DMA engine (think buffer overruns when you
>>> go from e.g. 720p to 1080p).
>>
>> It's the case here.
>> s/g_dv_timings are independent of query_detect_timings which reads internal registers to
>> define the stream detected by the component.
>>
>> The reset function are an error, I think.
>> By default the GS1662 is in auto-mode: it detects the input stream to create the serialized output stream.
>> The reset was to return in auto-mode selection, but this function should be to reset the component and not the mode.
>>
>> I don't have idea to define properly the auto-mode, for userspace and the driver.
>> It's a useful information and I think, the userspace should force this mode. Define a specific timings for that?
> 
> I think that you can use the s_stream op here: when you start streaming you force
> the mode to whatever the timings set by s_dv_timings() requires. When you stop streaming
> you go back to auto-mode.

Hum. I agree with you.
But, if the stream was starting without previous timings settings, I should use a default value or keep the auto-mode?
I prefer the auto-mode solution in this case.

>>> So typically when you set the timings the device is fixed to those timings,
>>> even if it receives something different. If the device supports an interrupt,
>>> then it is good practice to hook into that interrupt and, when it detects
>>> that the timings changed, the device sends a V4L2_EVENT_SOURCE_CHANGE event.
>>>
>>> Userspace will then typically stop streaming, query the new timings, setup
>>> the new buffers and restart streaming.
>>
>> GS1662 don't have interruption line to do that.
>>
>>> Some devices cannot query the new timings unless they are in autodetect mode.
>>> The correct implementation for that is that query_dv_timings returns EBUSY
>>> while the device is streaming (you hook into the s_stream core op to know that),
>>> otherwise it configures itself to autodetect mode and sees what is detected.
>>>
>>> It is not really clear to me from the datasheet how this device behaves. But
>>> having to use the reset op is almost certainly wrong.
>>
>> I don't understand.
>> The GS1662 has a status to say the input format detected. Useful in auto-detect mode,
>> less in other cases. But, it needs a input, why send EBUSY error when the device streams?
> 
> Hmm, I don't understand either :-)
> 
> The question is: when the device is streaming video for a specific format (as set
> by s_dv_timings), can it still detect the actual video format it receives? If so,
> then there is no need for EBUSY since query_dv_timings will always work. If not,
> then query_dv_timings should report that it is unable to query the detected timings
> because it is in the wrong mode (EBUSY).

Oh, I see.

I didn't remember my results around that. I will configure like your suggestion.

> BTW, you also need to implement the g_input_status video op. I just realized that
> that is missing. It is used to fill in the status field when calling VIDIOC_ENUMINPUTS.

Ok, I will add that. Thanks.

> Remember that today there are no SDI drivers in the kernel. So you and nohous are the first
> that work on this. So there will be some missing pieces that we need to add. It seems that
> for SDI the timings are one such area.
> 
> It will be useful if you join the #v4l irc room (http://linuxtv.org/lists.php).
> 
> I think that's a good place to have a meeting about this topic together with nohous. I'm
> traveling for a bit but will be back on Tuesday. Perhaps we can schedule something later
> that week.

Ok, I could participate the next week.
Thank you for all.

Regards.
Charles-Antoine Couret