RE: [PATCH v3 3/3] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem driver | linux-arm-kernel

quoted

Hi Laurent,

Thanks for the review! Please see my comments below.

-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Thursday, July 16, 2020 3:03 AM
To: Vishal Sagar <redacted>
Cc: Hans Verkuil <redacted>; Hyun Kwon <redacted>;
mchehab@kernel.org; robh+dt@kernel.org; mark.rutland@arm.com; Michal
Simek [off-list ref]; linux-media@vger.kernel.org;
devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org; joe@perches.com; Sandip Kothari
[off-list ref]; Dinesh Kumar [off-list ref]
Subject: Re: [PATCH v3 3/3] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem
driver

Hi Vishal,

Thank you for the patch.

Hans, there are a few questions for you below.

On Thu, Jun 25, 2020 at 11:43:01AM +0200, Hans Verkuil wrote:
On 18/06/2020 07:33, Vishal Sagar wrote:
The Xilinx UHD-SDI Rx subsystem soft IP is used to capture native SDI
streams from SDI sources like SDI broadcast equipment like cameras and
mixers. This block outputs either native SDI, native video or
AXI4-Stream compliant data stream for further processing. Please refer
to PG290 for details.

The driver is used to configure the IP to add framer, search for
specific modes, get the detected mode, stream parameters, errors, etc.
It also generates events for video lock/unlock, bridge over/under flow.

The driver supports 10/12 bpc YUV 422 media bus format currently. It
also decodes the stream parameters based on the ST352 packet embedded
in the
stream. In case the ST352 packet isn't present in the stream, the core's
detected properties are used to set stream properties.

The driver currently supports only the AXI4-Stream IP configuration.

Signed-off-by: Vishal Sagar <redacted>
---
v3
- fixed KConfig with better description
- removed unnecessary header files
- converted uppercase to lowercase for all hex values
- merged core struct to state struct
- removed most one line functions and replaced with direct reg
  read/write or macros
- dt property bpp to bpc. default 10. not mandatory.
- fixed subscribe events, log_status, s_stream
- merged overflow/underflow to one event
- moved all controls to xilinx-sdirxss.h
- max events from 128 to 8
- used FIELD_GET() instead of custom macro
- updated the controls documentation
- added spinlock
- removed 3GB control and added mode to detect bitmask
- fixed format for (width, height, colorspace, xfer func, etc)
- added dv_timings_cap, s/g_dv_timings
- fixed set/get_format
- fix v4l control registrations
- fix order of registration / deregistration in probe() remove()
- fixed other comments from Hyun, Laurent and Hans
- things yet to close
  - adding source port for connector (Laurent's suggestion)
  - adding new FIELD type for Transport Stream
V4L2_FIELD_ALTERNATE_PROG (Han's suggestion)
  - Update / remove EDH or CRC related controls

v2
- Added DV timing support based on Hans Verkuilś feedback
- More documentation to custom v4l controls and events
- Fixed Hyunś comments
- Added macro for masking and shifting as per Joe Perches comments
- Updated to latest as per Xilinx github repo driver like
  adding new DV timings not in mainline yet uptill 03/21/20

 drivers/media/platform/xilinx/Kconfig         |   11 +
 drivers/media/platform/xilinx/Makefile        |    1 +
 .../media/platform/xilinx/xilinx-sdirxss.c    | 2121 +++++++++++++++++
 include/uapi/linux/v4l2-controls.h            |    6 +
 include/uapi/linux/xilinx-sdirxss.h           |  283 +++
 5 files changed, 2422 insertions(+)
 create mode 100644 drivers/media/platform/xilinx/xilinx-sdirxss.c
 create mode 100644 include/uapi/linux/xilinx-sdirxss.h

diff --git a/drivers/media/platform/xilinx/Kconfig
b/drivers/media/platform/xilinx/Kconfig
index 01c96fb66414..578cdcc1036e 100644
--- a/drivers/media/platform/xilinx/Kconfig
+++ b/drivers/media/platform/xilinx/Kconfig
@@ -12,6 +12,17 @@ config VIDEO_XILINX

 if VIDEO_XILINX

+config VIDEO_XILINX_SDIRXSS
+	tristate "Xilinx UHD SDI Rx Subsystem"
+	help
+	  Driver for Xilinx UHD-SDI Rx Subsystem. This is a V4L sub-device
+	  based driver that takes input from a SDI source like SDI camera and
+	  converts it into an AXI4-Stream. The subsystem comprises a SMPTE
+	  UHD-SDI Rx core, a SDI Rx to Native Video bridge and a Video In to
+	  AXI4-Stream bridge. The driver is used to set different stream
+	  detection modes and identify stream properties to properly configure
+	  downstream.
+
 config VIDEO_XILINX_TPG
 	tristate "Xilinx Video Test Pattern Generator"
 	depends on VIDEO_XILINX
diff --git a/drivers/media/platform/xilinx/Makefile
b/drivers/media/platform/xilinx/Makefile
index 4cdc0b1ec7a5..3beaf24d832c 100644
--- a/drivers/media/platform/xilinx/Makefile
+++ b/drivers/media/platform/xilinx/Makefile
@@ -3,5 +3,6 @@
 xilinx-video-objs += xilinx-dma.o xilinx-vip.o xilinx-vipp.o

 obj-$(CONFIG_VIDEO_XILINX) += xilinx-video.o
+obj-$(CONFIG_VIDEO_XILINX_SDIRXSS) += xilinx-sdirxss.o
 obj-$(CONFIG_VIDEO_XILINX_TPG) += xilinx-tpg.o
 obj-$(CONFIG_VIDEO_XILINX_VTC) += xilinx-vtc.o
diff --git a/drivers/media/platform/xilinx/xilinx-sdirxss.c
b/drivers/media/platform/xilinx/xilinx-sdirxss.c
new file mode 100644
index 000000000000..e39aab7c656a
--- /dev/null
+++ b/drivers/media/platform/xilinx/xilinx-sdirxss.c
@@ -0,0 +1,2121 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Xilinx SDI Rx Subsystem
+ *
+ * Copyright (C) 2017 - 2020 Xilinx, Inc.
+ *
+ * Contacts: Vishal Sagar <vishal.sagar@xilinx.com>
+ */
+
+#include <dt-bindings/media/xilinx-sdi.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/xilinx-sdirxss.h>
That's an impressive trim down of headers, but I think it may have gone
a bit too far :-S I've pointed out to headers that are not needed in v2,
but you've removed all headers that are implicitly included through
other headers. This makes the driver more fragile, as if headers are
refactored, there's a large risk of compilation breakages.
Ok. I will add back the headers here excluding the ones as suggested in your v2 comment.

+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-dv-timings.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-subdev.h>
+
+/*
+ * SDI Rx register map, bitmask and offsets
+ */
<snip>

+ * @framer_enable: Flag for framer enabled or not set by control
+ *
+ * This structure contains the device driver related parameters
+ */
+struct xsdirxss_state {
+	struct v4l2_subdev subdev;
+	struct v4l2_ctrl_handler ctrl_handler;
+	struct v4l2_mbus_framefmt default_format;
+	struct media_pad pad;
+	struct device *dev;
+	void __iomem *iomem;
+	struct clk_bulk_data *clks;
+	int prev_is_frac;
If I understand the code correctly, this field doesn't store the
previous value but the current value programmed to the hardware. It
could be named clk_is_frac or something similar.
Correct. I will rename accordingly in next version.

+	u32 bpc;
+	u32 mode;
+	unsigned int num_clks;
+	bool include_edh;
+
+	/*
+	 * This spinlock is used to protect the below members
+	 * format, src_format, frame_interval, current_timings,
+	 * detected_timings_index, vidlockwin, edhmask, searchmask,
<snip>

+
+static const struct v4l2_dv_timings_cap xsdirxss_timings_cap = {
+	.type = V4L2_DV_BT_656_1120,
+	.pad = 0,
+	.reserved = { 0 },
+	V4L2_INIT_BT_TIMINGS(XSDIRXSS_WIDTH_MIN,
XSDIRXSS_WIDTH_MAX,
+			     XSDIRXSS_HEIGHT_MIN, XSDIRXSS_HEIGHT_MAX,
+			     XSDIRXSS_PIXELCLOCK_MIN,
XSDIRXSS_PIXELCLOCK_MAX,
+			     V4L2_DV_BT_STD_CEA861 | V4L2_DV_BT_STD_SDI,
+			     V4L2_DV_BT_CAP_PROGRESSIVE
+			     | V4L2_DV_BT_CAP_INTERLACED)
+
+};
As this is only used in xsdirxss_dv_timings_cap(), I would just set the
fields there. Up to you.
Noted. This will be moved to xsdirxss_dv_timings_cap().

+
+struct regmap {
+	const char *name;
+	u32 offset;
+};
+
+static const struct regmap xsdirxss_regmap[] = {
+	{ .name = "Reset Control", .offset = XSDIRX_RST_CTRL_REG },
+	{ .name = "Module Control", .offset = XSDIRX_MDL_CTRL_REG },
+	{ .name = "Interrupt Enable", .offset = XSDIRX_IER_REG },
+	{ .name = "Global Interrupt Enable", .offset = XSDIRX_GLBL_IER_REG },
+	{ .name = "ST352 Valid", .offset = XSDIRX_ST352_VALID_REG },
+	{ .name = "ST352 DS1", .offset = XSDIRX_ST352_DS1_REG },
+	{ .name = "ST352 DS2", .offset = XSDIRX_ST352_DS2_REG },
+	{ .name = "ST352 DS3", .offset = XSDIRX_ST352_DS3_REG },
+	{ .name = "ST352 DS4", .offset = XSDIRX_ST352_DS4_REG },
+	{ .name = "ST352 DS5", .offset = XSDIRX_ST352_DS5_REG },
+	{ .name = "ST352 DS6", .offset = XSDIRX_ST352_DS6_REG },
+	{ .name = "ST352 DS7", .offset = XSDIRX_ST352_DS7_REG },
+	{ .name = "ST352 DS8", .offset = XSDIRX_ST352_DS8_REG },
+	{ .name = "ST352 DS9", .offset = XSDIRX_ST352_DS9_REG },
+	{ .name = "ST352 DS10", .offset = XSDIRX_ST352_DS10_REG },
+	{ .name = "ST352 DS11", .offset = XSDIRX_ST352_DS11_REG },
+	{ .name = "ST352 DS12", .offset = XSDIRX_ST352_DS12_REG },
+	{ .name = "ST352 DS13", .offset = XSDIRX_ST352_DS13_REG },
+	{ .name = "ST352 DS14", .offset = XSDIRX_ST352_DS14_REG },
+	{ .name = "ST352 DS15", .offset = XSDIRX_ST352_DS15_REG },
+	{ .name = "ST352 DS16", .offset = XSDIRX_ST352_DS16_REG },
+	{ .name = "Version", .offset = XSDIRX_VERSION_REG },
+	{ .name = "Subsystem Config ", .offset = XSDIRX_SS_CONFIG_REG },
+	{ .name = "Mode Detect", .offset = XSDIRX_MODE_DET_STAT_REG },
+	{ .name = "Transport Stream Detect", .offset =
XSDIRX_TS_DET_STAT_REG },
+	{ .name = "EDH Status", .offset = XSDIRX_EDH_STAT_REG },
+	{ .name = "EDH Error Count", .offset = XSDIRX_EDH_ERRCNT_EN_REG
},
+	{ .name = "CRC error indication", .offset = XSDIRX_CRC_ERRCNT_REG },
+	{ .name = "Video Lock Window", .offset =
XSDIRX_VID_LOCK_WINDOW_REG },
+};
+
+static inline struct xsdirxss_state *
+to_xsdirxssstate(struct v4l2_subdev *subdev)
+{
+	return container_of(subdev, struct xsdirxss_state, subdev);
+}
+
+/*
+ * Register related operations
+ */
+static inline u32 xsdirxss_read(struct xsdirxss_state *xsdirxss, u32 addr)
+{
+	return ioread32(xsdirxss->iomem + addr);
+}
+
+static inline void xsdirxss_write(struct xsdirxss_state *xsdirxss, u32 addr,
+				  u32 value)
+{
+	iowrite32(value, xsdirxss->iomem + addr);
+}
+
+static inline void xsdirxss_clr(struct xsdirxss_state *xsdirxss, u32 addr,
+				u32 clr)
+{
+	xsdirxss_write(xsdirxss, addr, xsdirxss_read(xsdirxss, addr) & ~clr);
+}
+
+static inline void xsdirxss_set(struct xsdirxss_state *xsdirxss, u32 addr,
+				u32 set)
+{
+	xsdirxss_write(xsdirxss, addr, xsdirxss_read(xsdirxss, addr) | set);
+}
+
+#define XSDIRX_CORE_DISABLE(state)  xsdirxss_clr((state),
XSDIRX_RST_CTRL_REG,\
+
XSDIRX_RST_CTRL_SS_EN_MASK)
+
+#define XSDIRX_CORE_ENABLE(state)  xsdirxss_set((state),
XSDIRX_RST_CTRL_REG,\
+
	XSDIRX_RST_CTRL_SS_EN_MASK)
+
+#define XSDIRX_GLOBAL_INTR_ENABLE(state)	\
+	xsdirxss_set((state), XSDIRX_GLBL_IER_REG,
XSDIRX_GLBL_INTR_EN_MASK)
+
+#define XSDIRX_GLOBAL_INTR_DISABLE(state)	\
+	xsdirxss_clr((state), XSDIRX_GLBL_IER_REG,
XSDIRX_GLBL_INTR_EN_MASK)

Any reason to make these macros and not inline functions ?
I will make all these as static inline functions instead of macros.


+
+static int xsdirx_set_modedetect(struct xsdirxss_state *state, u16 mask)
+{
+	u32 val;
+	struct device *dev = state->dev;
+
<snip>

+
+static void xsdirxss_set_gtclk(struct xsdirxss_state *state)
+{
+	struct clk *gtclk;
+	unsigned long clkrate;
+	int ret, is_frac;
+	u32 mode;
+
+	mode = xsdirxss_read(state, XSDIRX_MODE_DET_STAT_REG);
+	mode &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK;
+
+	/*
+	 * TODO: For now, don't change the clock rate for any mode except
12G.
+	 * In future, configure gt clock for all modes and enable clock only
+	 * when needed (stream on/off).
+	 */
+	if (mode != XSDIRX_MODE_12GI_MASK && mode !=
XSDIRX_MODE_12GF_MASK)
+		return;
+
+	/* When numerator is 1001 then frame rate is fractional else integer
*/
+	is_frac = state->frame_interval.numerator == 1001 ? 1 : 0;
+
+	if (state->prev_is_frac == is_frac)
+		return;
+
+	XSDIRX_GLOBAL_INTR_DISABLE(state);
+	xsdirxss_clr(state, XSDIRX_IER_REG, XSDIRX_INTR_ALL_MASK);
+	XSDIRX_CORE_DISABLE(state);
+
+	/* get sdi_rx_clk */
+	gtclk = state->clks[1].clk;
+
+	/* calculate clkrate */
+	if (!is_frac)
+		clkrate = CLK_INT;
+	else
+		clkrate = (CLK_INT * 1000) / 1001;
+
+	ret = clk_set_rate(gtclk, clkrate);
This function is called from the IRQ handler, and with a spinlock held.
clk_set_rate() can sleep as the clock source can be an external
I2C-controlled chip. Have you tested this code path ? I think taking the
spinlock at top level locations will need to be reconsidered.
Yes I had tested this path. Ok I will see how to take the spin lock.

+	if (ret)
+		dev_err(state->dev, "failed to set clk rate = %d\n", ret);
+
+	state->prev_is_frac = is_frac;
+	clkrate = clk_get_rate(gtclk);
+
+	dev_dbg(state->dev, "clkrate = %lu is_frac = %d\n",
+		clkrate, is_frac);
+
+	if (state->framer_enable) {
+		xsdirxss_set(state, XSDIRX_MDL_CTRL_REG,
+			     XSDIRX_MDL_CTRL_FRM_EN_MASK);
+	} else {
+		xsdirxss_clr(state, XSDIRX_MDL_CTRL_REG,
+			     XSDIRX_MDL_CTRL_FRM_EN_MASK);
+	}
No need for braces.
Noted. I will update this in the next version.


+	xsdirxss_write(state, XSDIRX_EDH_ERRCNT_EN_REG,
+		       state->edhmask & XSDIRX_EDH_ALLERR_MASK);
+	xsdirxss_write(state, XSDIRX_VID_LOCK_WINDOW_REG, state-
vidlocked);
+	xsdirx_set_modedetect(state, state->searchmask);
+	XSDIRX_CORE_ENABLE(state);
+	xsdirxss_set(state, XSDIRX_IER_REG, XSDIRX_INTR_ALL_MASK);
+	XSDIRX_GLOBAL_INTR_ENABLE(state);
+}
+
+/**
+ * xsdirx_get_stream_properties - Get SDI Rx stream properties
+ * @state: pointer to driver state
<snip>


+/**
+ * xsdirxss_irq_handler - Interrupt handler for SDI Rx
+ * @irq: IRQ number
+ * @dev_id: Pointer to device state
+ *
+ * The SDI Rx interrupts are cleared by writing 1 to corresponding bit.
+ *
+ * Return: IRQ_HANDLED after handling interrupts
+ */
+static irqreturn_t xsdirxss_irq_handler(int irq, void *dev_id)
+{
+	struct xsdirxss_state *state = (struct xsdirxss_state *)dev_id;
+	struct device *dev = state->dev;
+	u32 status;
+
+	status = xsdirxss_read(state, XSDIRX_ISR_REG);
+	xsdirxss_write(state, XSDIRX_ISR_REG, status);
+	dev_dbg(dev, "interrupt status = 0x%08x\n", status);
+
+	if (!status)
+		return IRQ_NONE;
+
+	if (status & XSDIRX_INTR_VIDLOCK_MASK ||
+	    status & XSDIRX_INTR_VIDUNLOCK_MASK) {
+		u32 val1, val2;
+		struct v4l2_event event = { 0 };
+		unsigned long flags;
+
+		dev_dbg(dev, "video lock/unlock interrupt\n");
+
+		spin_lock_irqsave(&state->slock, flags);
+		xsdirx_streamflow_control(state, false);
+
+		val1 = xsdirxss_read(state, XSDIRX_MODE_DET_STAT_REG);
+		val2 = xsdirxss_read(state, XSDIRX_TS_DET_STAT_REG);
+
+		if ((val1 & XSDIRX_MODE_DET_STAT_MODE_LOCK_MASK) &&
+		    (val2 & XSDIRX_TS_DET_STAT_LOCKED_MASK)) {
+			u32 mask =
XSDIRX_RST_CTRL_RST_CRC_ERRCNT_MASK |
+
XSDIRX_RST_CTRL_RST_EDH_ERRCNT_MASK;
+
+			dev_dbg(dev, "video lock interrupt\n");
+
+			xsdirxss_set(state, XSDIRX_RST_CTRL_REG, mask);
+			xsdirxss_clr(state, XSDIRX_RST_CTRL_REG, mask);
+
+			val1 = xsdirxss_read(state, XSDIRX_ST352_VALID_REG);
+			val2 = xsdirxss_read(state, XSDIRX_ST352_DS1_REG);
+
+			dev_dbg(dev, "valid st352 mask = 0x%08x\n", val1);
+			dev_dbg(dev, "st352 payload = 0x%08x\n", val2);
+
+			if (!xsdirx_get_stream_properties(state)) {
+				state->vidlocked = true;
+				xsdirxss_set_gtclk(state);
+			} else {
+				dev_err(dev, "Unable to get stream
properties!\n");
+				state->vidlocked = false;
+			}
+		} else {
+			dev_dbg(dev, "video unlock interrupt\n");
+			state->vidlocked = false;
+		}
+		spin_unlock_irqrestore(&state->slock, flags);
+
+		event.type = V4L2_EVENT_SOURCE_CHANGE;
+		event.u.src_change.changes =
V4L2_EVENT_SRC_CH_RESOLUTION;
+		v4l2_subdev_notify_event(&state->subdev, &event);
+	}
+
+	if (status & (XSDIRX_INTR_UNDERFLOW_MASK |
XSDIRX_INTR_OVERFLOW_MASK)) {
+		struct v4l2_event event = { 0 };
+
+		dev_dbg(dev, "Video in to AXI4 Stream core under/overflow
interrupt\n");
+
+		event.type = V4L2_EVENT_XILINX_SDIRX_UND_OVR_FLOW;
+		if (status & XSDIRX_INTR_UNDERFLOW_MASK)
+			event.u.data[0] = XILINX_SDIRX_UNDERFLOW_EVENT;
Should this be

			event.u.data[0] |= XILINX_SDIRX_UNDERFLOW_EVENT;

+		if (status & XSDIRX_INTR_OVERFLOW_MASK)
+			event.u.data[0] = XILINX_SDIRX_OVERFLOW_EVENT;
And this

			event.u.data[0] |= XILINX_SDIRX_OVERFLOW_EVENT;

in case both underflow and overflow need to be reported at the same time
? I'm not sure that can happen in practice though, but even if it can't,
it also won't hurt to treat the field as a bitflag.
Agree. I will keep it as a bit flag in next version.


+
+		v4l2_subdev_notify_event(&state->subdev, &event);
+	}
+	return IRQ_HANDLED;
+}
+
+/**
+ * xsdirxss_subscribe_event - Subscribe to video lock and unlock event
+ * @sd: V4L2 Sub device
+ * @fh: V4L2 File Handle
+ * @sub: Subcribe event structure
+ *
+ * Return: 0 on success, errors otherwise
+ */
+static int xsdirxss_subscribe_event(struct v4l2_subdev *sd,
+				    struct v4l2_fh *fh,
+				    struct v4l2_event_subscription *sub)
+{
+	int ret;
+	struct xsdirxss_state *xsdirxss = to_xsdirxssstate(sd);
+
+	dev_dbg(xsdirxss->dev, "Event subscribed : 0x%08x\n", sub->type);
+	switch (sub->type) {
+	case V4L2_EVENT_XILINX_SDIRX_UND_OVR_FLOW:
+		ret = v4l2_event_subscribe(fh, sub, XSDIRX_MAX_EVENTS,
NULL);
+		break;
+	case V4L2_EVENT_SOURCE_CHANGE:
+		ret = v4l2_src_change_event_subscribe(fh, sub);
+		break;
+	default:
+		ret = v4l2_ctrl_subscribe_event(fh, sub);
It's customary to have a break at the end of the last case.
Noted. I will update this in next version.


+	}
+	return ret;
+}
+
+/**
+ * xsdirxss_s_ctrl - This is used to set the Xilinx SDI Rx V4L2 controls
+ * @ctrl: V4L2 control to be set
+ *
+ * This function is used to set the V4L2 controls for the Xilinx SDI Rx
+ * Subsystem.
+ *
+ * Return: 0 on success, errors otherwise
+ */
+static int xsdirxss_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	int ret = 0;
+	struct xsdirxss_state *xsdirxss =
+		container_of(ctrl->handler, struct xsdirxss_state,
+			     ctrl_handler);
+	struct device *dev = xsdirxss->dev;
+	unsigned long flags;
+
+	dev_dbg(dev, "set ctrl id = 0x%08x val = 0x%08x\n",
+		ctrl->id, ctrl->val);
+
+	spin_lock_irqsave(&xsdirxss->slock, flags);
+
+	if (xsdirxss->streaming) {
+		spin_unlock_irqrestore(&xsdirxss->slock, flags);
+		dev_err(dev, "Cannot set controls while streaming\n");
+		return -EINVAL;
+	}
+
+	XSDIRX_CORE_DISABLE(xsdirxss);
+	switch (ctrl->id) {
+	case V4L2_CID_XILINX_SDIRX_FRAMER:
+		xsdirxss->framer_enable = ctrl->val;
+		if (xsdirxss->framer_enable) {
+			xsdirxss_set(xsdirxss, XSDIRX_MDL_CTRL_REG,
+				     XSDIRX_MDL_CTRL_FRM_EN_MASK);
+		} else {
+			xsdirxss_clr(xsdirxss, XSDIRX_MDL_CTRL_REG,
+				     XSDIRX_MDL_CTRL_FRM_EN_MASK);
+		}
+		break;
+	case V4L2_CID_XILINX_SDIRX_VIDLOCK_WINDOW:
+		/*
+		 * The video lock window is the amount of time for which the
+		 * the mode and transport stream should be locked to get the
+		 * video lock interrupt.
+		 */
+		xsdirxss->vidlockwin = ctrl->val;
+		xsdirxss_write(xsdirxss, XSDIRX_VID_LOCK_WINDOW_REG,
+			       xsdirxss->vidlockwin);
+		break;
+	case V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES:
+		xsdirxss->edhmask = ctrl->val & XSDIRX_EDH_ALLERR_MASK;
The '& XSDIRX_EDH_ALLERR_MASK' is not needed since the control's 'max'
value
is set to XSDIRX_EDH_ALLERR_MASK, so ctrl->val can't contain any other bits.
The control framework ensures that.

+		xsdirxss_write(xsdirxss, XSDIRX_EDH_ERRCNT_EN_REG,
+			       xsdirxss->edhmask);
+		break;
+	case V4L2_CID_XILINX_SDIRX_SEARCH_MODES:
+		if (!ctrl->val) {
This check should be done in a try_ctrl function instead of s_ctrl.

Interesting, this is the first bitmask control where you don't want it to
be 0. If we get more of these in the future, then it would make sense if this
is supported in the control framework itself (e.g. if min is set to 1, then
that means that the value can't be 0). But for now just check this in the
try_ctrl() function.

+			spin_unlock_irqrestore(&xsdirxss->slock, flags);
+			dev_err(dev, "Select at least one mode!\n");
+			return -EINVAL;
+		}
+
+		if (xsdirxss->mode == XSDI_STD_3G) {
+			dev_dbg(dev, "Upto 3G supported\n");
+			ctrl->val &= ~(BIT(XSDIRX_MODE_6G_OFFSET) |
+				       BIT(XSDIRX_MODE_12GI_OFFSET) |
+				       BIT(XSDIRX_MODE_12GF_OFFSET));
+		}
+
+		if (xsdirxss->mode == XSDI_STD_6G) {
+			dev_dbg(dev, "Upto 6G supported\n");
+			ctrl->val &= ~(BIT(XSDIRX_MODE_12GI_OFFSET) |
+				       BIT(XSDIRX_MODE_12GF_OFFSET));
+		}
This shouldn't be done here. Instead the 'max' field of the control must be
set correctly based on the mode. You can call v4l2_ctrl_modify_range() in
xsdirxss_probe() to update the max value.
Hans, I wrote this comment in v2:

"The traditional approach to select timing standards is to use ioctls.
Hans, do you think a custom control is fine here, or should the dv
timings ioctls be extended (or new sdi timings ioctls created) ?

If we go for a single control, I'm a bit concerned that this control
will restrict the search when multiple bits are set, but force a
specific mode when a single bit is set. I don't have enough experience
with SDI to tell whether this is the right behaviour."

What do you think ? It's of course hard to standardize an API with a
single device as an example (although there's also a spec to help here),
so I'm not opposed to using controls in this driver.

+
+		ret = xsdirx_set_modedetect(xsdirxss, ctrl->val);
+		if (!ret)
+			xsdirxss->searchmask = ctrl->val;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	XSDIRX_CORE_ENABLE(xsdirxss);
+
+	spin_unlock_irqrestore(&xsdirxss->slock, flags);
+	return ret;
+}
+
+/**
+ * xsdirxss_g_volatile_ctrl - get the Xilinx SDI Rx controls
+ * @ctrl: Pointer to V4L2 control
+ *
+ * Return: 0 on success, errors otherwise
+ */
+static int xsdirxss_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+	u32 val;
+	struct xsdirxss_state *xsdirxss =
+		container_of(ctrl->handler,
+			     struct xsdirxss_state, ctrl_handler);
+	struct device *dev = xsdirxss->dev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&xsdirxss->slock, flags);
+	if (!xsdirxss->vidlocked) {
+		spin_unlock_irqrestore(&xsdirxss->slock, flags);
+		dev_err(dev, "Can't get values when video not locked!\n");
+		return -EINVAL;
+	}
+	switch (ctrl->id) {
+	case V4L2_CID_XILINX_SDIRX_MODE_DETECT:
+		val = xsdirxss_read(xsdirxss, XSDIRX_MODE_DET_STAT_REG);
+		val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK;
+
+		switch (val) {
+		case XSDIRX_MODE_SD_MASK:
+			ctrl->val = XSDIRX_MODE_SD_OFFSET;
+			break;
+		case XSDIRX_MODE_HD_MASK:
+			ctrl->val = XSDIRX_MODE_HD_OFFSET;
+			break;
+		case XSDIRX_MODE_3G_MASK:
+			val = xsdirxss_read(xsdirxss,
XSDIRX_MODE_DET_STAT_REG);
+			val &= XSDIRX_MODE_DET_STAT_LVLB_3G_MASK;
+			ctrl->val = val ? XSDIRX_MODE_3GB_OFFSET :
+				XSDIRX_MODE_3GA_OFFSET;
+			break;
+		case XSDIRX_MODE_6G_MASK:
+			ctrl->val = XSDIRX_MODE_6G_OFFSET;
+			break;
+		case XSDIRX_MODE_12GI_MASK:
+			ctrl->val = XSDIRX_MODE_12GI_OFFSET;
+			break;
+		case XSDIRX_MODE_12GF_MASK:
+			ctrl->val = XSDIRX_MODE_12GF_OFFSET;
+			break;
+		}
There is no interrupt that will tell you when the mode changes? It's much
nicer if updating this control is interrupt driven rather than requiring
userspace to poll.

+		break;
+	case V4L2_CID_XILINX_SDIRX_CRC:
+		ctrl->val = xsdirxss_read(xsdirxss, XSDIRX_CRC_ERRCNT_REG);
+		xsdirxss_write(xsdirxss, XSDIRX_CRC_ERRCNT_REG, 0xffff);
+		break;
+	case V4L2_CID_XILINX_SDIRX_EDH_ERRCNT:
+		val = xsdirxss_read(xsdirxss, XSDIRX_MODE_DET_STAT_REG);
+		val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK;
+		if (val == XSDIRX_MODE_SD_MASK) {
+			ctrl->val = xsdirxss_read(xsdirxss,
+						  XSDIRX_EDH_ERRCNT_REG);
+		} else {
+			spin_unlock_irqrestore(&xsdirxss->slock, flags);
+			dev_dbg(dev, "%d - not in SD mode\n", ctrl->id);
+			return -EINVAL;
Getting a control value shouldn't fail. Just set ctrl->val to 0 and
return 0. You can leave the dev_dbg though, that can be useful.

+		}
+		break;
+	case V4L2_CID_XILINX_SDIRX_EDH_STATUS:
+		val = xsdirxss_read(xsdirxss, XSDIRX_MODE_DET_STAT_REG);
+		val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK;
+		if (val == XSDIRX_MODE_SD_MASK) {
+			ctrl->val = xsdirxss_read(xsdirxss,
+						  XSDIRX_EDH_STAT_REG);
+		} else {
+			spin_unlock_irqrestore(&xsdirxss->slock, flags);
+			dev_dbg(dev, "%d - not in SD mode\n", ctrl->id);
+			return -EINVAL;
Ditto.

+		}
+		break;
+	case V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED:
+		ctrl->val = xsdirxss->ts_is_interlaced;
+		break;
I assume this control will disappear once you added support for
FIELD_ALTERNATE_PROG?
I'm not sure FIELD_ALTERNATE_PROG is a good idea. The v4l2_field
specifies today how frames are split into multiple buffers. There's an
implicit assumption that a frame split into two buffers is captured with
interlacing. In the SDI case, the two concepts get decoupled, a
progressive frame can be transmitted (and captured) in two separate
parts. If we add a *_PROG field, we'll need to duplicate most of the
v4l2_field values with a _PROG suffix, as the progressive frame can be
captured in alternate buffers on a video node, but also in separate odd
and even buffers on two video nodes. Tt the hardware level, data is
transmitted with odd lines on one link, and even lines on a second link.
There are then two instances of this IP core, one for each link. One
instance would receive and process the even lines, the other instance
the odd lines. The output of the two instances can then be connected to
two separate DMA engines, or combined in the FPGA fabric, depending on
how the user designs the system.
My apologies to give incorrect info regarding this.
In the progressive segmented frame, a progressive captured frame is sent
across to receiver over an interlaced transport. The 2 fields received
are similar to how V4L2_FIELD_ALTERNATE is except that the fields weren't
captured at 2 different times.
So I will add the V4L2_FIELD_ALTERNATE_PROG in next patch version.


+	default:
+		spin_unlock_irqrestore(&xsdirxss->slock, flags);
+		dev_err(dev, "Get Invalid control id 0x%0x\n", ctrl->id);
+		return -EINVAL;
You can drop the default case altogether: this function will only be called
for volatile controls.

+	}
+
+	spin_unlock_irqrestore(&xsdirxss->slock, flags);
+	return 0;
+}
+
+/**
+ * xsdirxss_log_status - Logs the status of the SDI Rx Subsystem
+ * @sd: Pointer to V4L2 subdevice structure
+ *
+ * This function prints the current status of Xilinx SDI Rx Subsystem
+ *
+ * Return: 0 on success
+ */
+static int xsdirxss_log_status(struct v4l2_subdev *sd)
+{
+	struct xsdirxss_state *xsdirxss = to_xsdirxssstate(sd);
+	u32 i;
+
+	v4l2_info(sd, "***** SDI Rx subsystem reg dump start *****\n");
+	v4l2_info(sd, "No : Register Name : Value\n");
+	for (i = 0; i < ARRAY_SIZE(xsdirxss_regmap); i++) {
+		v4l2_info(sd, "%02d : %s register : 0x%08x\n", i,
+			  xsdirxss_regmap[i].name,
+			  xsdirxss_read(xsdirxss, xsdirxss_regmap[i].offset));
+	}
+	v4l2_info(sd, "***** SDI Rx subsystem reg dump end *****\n");
+
+	v4l2_ctrl_subdev_log_status(sd);
+
+	return 0;
+}
+
+/**
+ * xsdirxss_g_frame_interval - Get the frame interval
+ * @sd: V4L2 Sub device
+ * @fi: Pointer to V4l2 Sub device frame interval structure
+ *
+ * This function is used to get the frame interval.
+ * The frame rate can be integral or fractional.
+ * Integral frame rate e.g. numerator = 1000, denominator = 24000 => 24
fps
+ * Fractional frame rate e.g. numerator = 1001, denominator = 24000 =>
23.97 fps
+ *
+ * Return: 0 on success
+ */
+static int xsdirxss_g_frame_interval(struct v4l2_subdev *sd,
+				     struct v4l2_subdev_frame_interval *fi)
+{
+	struct xsdirxss_state *xsdirxss = to_xsdirxssstate(sd);
+	unsigned long flags;
+
+	if (!xsdirxss->vidlocked) {
+		dev_err(xsdirxss->dev, "Video not locked!\n");
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&xsdirxss->slock, flags);
+	fi->interval = xsdirxss->frame_interval;
+	spin_unlock_irqrestore(&xsdirxss->slock, flags);
+
+	dev_dbg(xsdirxss->dev, "frame rate numerator = %d denominator =
%d\n",
+		xsdirxss->frame_interval.numerator,
+		xsdirxss->frame_interval.denominator);
You should print fi->interval, not xsdirxss->frame_interval, as you're
outside of the spinlock.
Agree. I will update this in next version.


+	return 0;
+}
+
+/**
+ * xsdirxss_s_stream - It is used to start/stop the streaming.
+ * @sd: V4L2 Sub device
+ * @enable: Flag (True / False)
<snip>

+
+	ret = media_entity_pads_init(&subdev->entity, 1, &xsdirxss->pad);
+	if (ret < 0)
+		goto error;
+
+	/* Initialise and register the controls */
+	num_ctrls = ARRAY_SIZE(xsdirxss_ctrls);
+
+	if (xsdirxss->include_edh)
+		num_ctrls += ARRAY_SIZE(xsdirxss_edh_ctrls);
+
+	v4l2_ctrl_handler_init(&xsdirxss->ctrl_handler, num_ctrls);
+
+	for (i = 0; i < ARRAY_SIZE(xsdirxss_ctrls); i++) {
+		struct v4l2_ctrl *ctrl;
+
+		dev_dbg(dev, "%d %s ctrl = 0x%x\n", i, xsdirxss_ctrls[i].name,
+			xsdirxss_ctrls[i].id);
+
+		ctrl = v4l2_ctrl_new_custom(&xsdirxss->ctrl_handler,
+					    &xsdirxss_ctrls[i], NULL);
You can drop the ctrl variable, it's not used. Same below.
Noted. I will update this in the next patch.


+	}
+
+	if (xsdirxss->include_edh) {
+		for (i = 0; i < ARRAY_SIZE(xsdirxss_edh_ctrls); i++) {
+			struct v4l2_ctrl *ctrl;
+
+			dev_dbg(dev, "%d %s ctrl = 0x%x\n", i,
+				xsdirxss_edh_ctrls[i].name,
+				xsdirxss_edh_ctrls[i].id);
+
+			ctrl = v4l2_ctrl_new_custom(&xsdirxss->ctrl_handler,
+						    &xsdirxss_edh_ctrls[i],
+						    NULL);
+		}
+	}
+
+	if (xsdirxss->ctrl_handler.error) {
+		dev_err(dev, "failed to add controls\n");
+		ret = xsdirxss->ctrl_handler.error;
+		goto error;
+	}
+
+	subdev->ctrl_handler = &xsdirxss->ctrl_handler;
+
+	ret = v4l2_ctrl_handler_setup(&xsdirxss->ctrl_handler);
+	if (ret < 0) {
+		dev_err(dev, "failed to set controls\n");
+		goto error;
+	}
+
+	platform_set_drvdata(pdev, xsdirxss);
+
+	ret = v4l2_async_register_subdev(subdev);
+	if (ret < 0) {
+		dev_err(dev, "failed to register subdev\n");
+		goto error;
+	}
+
+	xsdirxss->prev_is_frac = -1;
+
+	XSDIRX_CORE_ENABLE(xsdirxss);
+
+	return 0;
+error:
+	v4l2_ctrl_handler_free(&xsdirxss->ctrl_handler);
+	media_entity_cleanup(&subdev->entity);
+	XSDIRX_GLOBAL_INTR_DISABLE(xsdirxss);
+	xsdirxss_clr(xsdirxss, XSDIRX_IER_REG, XSDIRX_INTR_ALL_MASK);
+clk_err:
+	clk_bulk_disable_unprepare(xsdirxss->num_clks, xsdirxss->clks);
+	return ret;
+}
+
+static int xsdirxss_remove(struct platform_device *pdev)
+{
+	struct xsdirxss_state *xsdirxss = platform_get_drvdata(pdev);
+	struct v4l2_subdev *subdev = &xsdirxss->subdev;
+
+	XSDIRX_CORE_DISABLE(xsdirxss);
+	XSDIRX_GLOBAL_INTR_DISABLE(xsdirxss);
+	xsdirxss_clr(xsdirxss, XSDIRX_IER_REG, XSDIRX_INTR_ALL_MASK);
+	xsdirx_streamflow_control(xsdirxss, false);
+
+	v4l2_async_unregister_subdev(subdev);
+	v4l2_ctrl_handler_free(&xsdirxss->ctrl_handler);
+	media_entity_cleanup(&subdev->entity);
+
+	clk_bulk_disable_unprepare(xsdirxss->num_clks, xsdirxss->clks);
+
+	return 0;
+}
+
+static const struct of_device_id xsdirxss_of_id_table[] = {
+	{ .compatible = "xlnx,v-smpte-uhdsdi-rx-ss-2.0" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, xsdirxss_of_id_table);
+
+static struct platform_driver xsdirxss_driver = {
+	.driver = {
+		.name		= "xilinx-sdirxss",
+		.of_match_table	= xsdirxss_of_id_table,
+	},
+	.probe			= xsdirxss_probe,
+	.remove			= xsdirxss_remove,
+};
+
+module_platform_driver(xsdirxss_driver);
+
+MODULE_AUTHOR("Vishal Sagar [off-list ref]");
+MODULE_DESCRIPTION("Xilinx SDI Rx Subsystem Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-
controls.h
index 62271418c1be..9526a6acc6f4 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -198,6 +198,12 @@ enum v4l2_colorfx {
  */
 #define V4L2_CID_USER_ATMEL_ISC_BASE
	(V4L2_CID_USER_BASE + 0x10c0)
+/*
+ * The base for the Xilinx SDI Rx driver controls.
+ * We reserve 16 controls for this driver.
+ */
+#define V4L2_CID_USER_XILINX_SDIRX_BASE
	(V4L2_CID_USER_BASE + 0x10e0)
+
 /* MPEG-class control IDs */
 /* The MPEG controls are applicable to all codec controls
  * and the 'MPEG' part of the define is historical */
diff --git a/include/uapi/linux/xilinx-sdirxss.h b/include/uapi/linux/xilinx-
sdirxss.h
new file mode 100644
index 000000000000..1bcbf5852b22
--- /dev/null
+++ b/include/uapi/linux/xilinx-sdirxss.h
@@ -0,0 +1,283 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Xilinx SDI Rx Subsystem mode, event, custom timings and
+ * flag definitions.
+ *
+ * Copyright (C) 2019 - 2020 Xilinx, Inc.
+ *
+ * Contacts: Vishal Sagar <vishal.sagar@xilinx.com>
+ */
+
+#ifndef __UAPI_XILINX_SDIRXSS_H__
+#define __UAPI_XILINX_SDIRXSS_H__
+
+#include <linux/types.h>
+#include <linux/v4l2-controls.h>
+#include <linux/v4l2-dv-timings.h>
+#include <linux/videodev2.h>
+
+/*
+ * Events
+ *
+ * V4L2_EVENT_XILINX_SDIRX_UND_OVR_FLOW: Video in to AXI4 Stream
core
+ * under/overflowed during a resolution or frame rate change.
+ */
+#define V4L2_EVENT_XILINX_SDIRX_CLASS
(V4L2_EVENT_PRIVATE_START | 0x200)
+#define V4L2_EVENT_XILINX_SDIRX_UND_OVR_FLOW	\
+					(V4L2_EVENT_XILINX_SDIRX_CLASS |
0x1)
+
+#define XILINX_SDIRX_UNDERFLOW_EVENT	BIT(1)
+#define XILINX_SDIRX_OVERFLOW_EVENT	BIT(2)
+/*
+ * This enum is used to prepare the bitmask of modes to be detected
+ */
+enum {
+	XSDIRX_MODE_SD_OFFSET = 0,
+	XSDIRX_MODE_HD_OFFSET,
+	XSDIRX_MODE_3GA_OFFSET,
+	XSDIRX_MODE_3GB_OFFSET,
+	XSDIRX_MODE_6G_OFFSET,
+	XSDIRX_MODE_12GI_OFFSET,
+	XSDIRX_MODE_12GF_OFFSET,
+	XSDIRX_MODE_NUM_SUPPORTED,
+};
These are all standard SDI modes, right?

+
+#define XSDIRX_DETECT_ALL_MODES
	(BIT(XSDIRX_MODE_SD_OFFSET) | \
+					BIT(XSDIRX_MODE_HD_OFFSET) | \
+					BIT(XSDIRX_MODE_3GA_OFFSET) | \
+					BIT(XSDIRX_MODE_3GB_OFFSET) | \
+					BIT(XSDIRX_MODE_6G_OFFSET) | \
+					BIT(XSDIRX_MODE_12GI_OFFSET) | \
+					BIT(XSDIRX_MODE_12GF_OFFSET))
+
+/*
+ * EDH - Error Detection and Handling.
+ * In the SD-SDI mode, the UHD-SDI core fully supports RP 165.
+ * The bitmask is named as XSDIRX_EDH_ERRCNT_XX_YY_ERR except
+ * for packet checksum error.
+ *
+ * XX - EDH Error Types
+ * ANC - Ancillary Data Packet Errors
+ * FF - Full Field Errors
+ * AP - Active Portion Errors
+ *
+ * YY - Error Flags
+ * EDH - error detected here
+ * EDA - error Detected already
+ * IDH - internal error detected here
+ * IDA - internal error detected already
+ * UES - unknown error status
+ *
+ * Refer to Sec 4.3 Error Flags in RP 165-1994 for details
+ */
+
+#define XSDIRX_EDH_ERRCNT_ANC_EDH_ERR		BIT(0)
+#define XSDIRX_EDH_ERRCNT_ANC_EDA_ERR		BIT(1)
+#define XSDIRX_EDH_ERRCNT_ANC_IDH_ERR		BIT(2)
+#define XSDIRX_EDH_ERRCNT_ANC_IDA_ERR		BIT(3)
+#define XSDIRX_EDH_ERRCNT_ANC_UES_ERR		BIT(4)
+#define XSDIRX_EDH_ERRCNT_FF_EDH_ERR		BIT(5)
+#define XSDIRX_EDH_ERRCNT_FF_EDA_ERR		BIT(6)
+#define XSDIRX_EDH_ERRCNT_FF_IDH_ERR		BIT(7)
+#define XSDIRX_EDH_ERRCNT_FF_IDA_ERR		BIT(8)
+#define XSDIRX_EDH_ERRCNT_FF_UES_ERR		BIT(9)
+#define XSDIRX_EDH_ERRCNT_AP_EDH_ERR		BIT(10)
+#define XSDIRX_EDH_ERRCNT_AP_EDA_ERR		BIT(11)
+#define XSDIRX_EDH_ERRCNT_AP_IDH_ERR		BIT(12)
+#define XSDIRX_EDH_ERRCNT_AP_IDA_ERR		BIT(13)
+#define XSDIRX_EDH_ERRCNT_AP_UES_ERR		BIT(14)
+#define XSDIRX_EDH_ERRCNT_PKT_CHKSUM_ERR	BIT(15)
+
+#define XSDIRX_EDH_ALLERR_MASK		0xFFFF
Lowercase 0xffff.

And these error conditions are also standardized?

If so, then I think these defines/enums can be part of V4L2 itself rather
than Xilinx specific.

+
+/*
+ * V4L2 Controls - We reserved 16 controls for this driver.
I'd increase that to 32.

+ *
+ * The V4L2_CID_XILINX_SDIRX_EDH_* controls are present only if
+ * EDH is enabled.
+ * The controls which can be set should only be set before enabling
+ * streaming. The controls which can be got should be called while
+ * streaming to get correct values.
+ * The V4L2_CID_XILINX_SDIRX_MODE_DETECT can be called when query
dv timing
query dv timing -> query_dv_timings

+ * returns a valid timing.
+ */
+
+/*
+ * Framer Control to enable or disable the framer. When this is set, the
framer
+ * automatically readjusts the output word alignment to match the
alignment of
+ * each timing reference signal(TRS). Normally this should be set. But user
may
+ * control this input to implement TRS filtering to prevent a signal
misaligned
+ * TRS from causing erroneous alignment changes.
+ * Refer to PG205 rx_frame_en for more details.
+ */
+#define V4L2_CID_XILINX_SDIRX_FRAMER
	(V4L2_CID_USER_XILINX_SDIRX_BASE + 1)
+
+/*
+ * Video Lock Window Control to set the video lock window value
+ * This is the amount of time the mode and transport stream need
+ * to be locked before a video lock interrupt occurs.
+ */
+#define V4L2_CID_XILINX_SDIRX_VIDLOCK_WINDOW
	(V4L2_CID_USER_XILINX_SDIRX_BASE + 2)
+
+/*
+ * EDH Error Mask Control to enable EDH error count
+ * This control takes in the bitmask of XSDIRX_EDH_ERRCNT_*_ERR to
enable counting
+ * such errors.
+ */
+#define V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES
	(V4L2_CID_USER_XILINX_SDIRX_BASE + 3)
If these EDH error sources are from the SDI standard, then this can become a
standard
control as well.

+
+/*
+ * Mode search Control to pass the bit mask of modes to detect.
+ * If only 1 bit is set, the driver programs IP to be in fixed mode else
+ * in multi detection mode.
+ *
+ * Set this when not streaming.
+ *
+ * bit 0 set to detect SD  mode,
+ * bit 1 set to detect HD  mode,
+ * bit 2 set to detect 3GA mode,
+ * bit 3 set to detect 3GB mode,
+ * bit 4 set to detect 6G  mode,
+ * bit 5 set to detect 12G integer frame rate mode,
+ * bit 6 set to detect 12G fractional frame rate mode,
+ */
+#define V4L2_CID_XILINX_SDIRX_SEARCH_MODES
	(V4L2_CID_USER_XILINX_SDIRX_BASE + 4)
Same here if these modes are standardized.

+
+/*
+ * Get Detected SDI Mode control (read only)
+ *
+ * Control Value - Mode detected
+ *        0      -     SD
+ *        1      -     HD
+ *        2      -     3GA
+ *        3      -     3GB
+ *        4      -     6G
+ *        5      -     12G integer frame rate
+ *        6      -     12G fractional frame rate
+ */
+#define V4L2_CID_XILINX_SDIRX_MODE_DETECT
	(V4L2_CID_USER_XILINX_SDIRX_BASE + 5)
Ditto.

+
+/* Get number of CRC errors status control
+ *
+ * When a CRC is detected on a line, the CRC error signal of that data
stream
+ * becomes asserted starting a few clock cycles after the last CRC word is
+ * output on the data stream ports following the EAV that ends the line
+ * containing the error. The CRC signal remains asserted for one line time.
+ *
+ * The LSB 16 bits of value returned by thsi control represent the error
+ * signal on each of 16 data streams. The MSB 16 bits contains the
accumulated
+ * error count.
+ *
+ * Refer to PG205 rx_crc_err_dsX (X = 1 to 16) description for details.
+ */
+#define V4L2_CID_XILINX_SDIRX_CRC
	(V4L2_CID_USER_XILINX_SDIRX_BASE + 6)
As suggested earlier, I think this should be split into two controls.

+
+/*
+ * Get EDH error count control
+ *
+ * Reading this control will give the number of EDH errors occurred based
+ * on the bitmask passed in
V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES.
+ *
+ * It increments once per field when any of the error conditions enabled by
+ * the RX_EDH_ERRCNT_EN register bit(s) occur during that field.
+ *
+ * Refer to PG205 rx_edh_errcnt
+ */
+#define V4L2_CID_XILINX_SDIRX_EDH_ERRCNT
	(V4L2_CID_USER_XILINX_SDIRX_BASE + 7)
Even though the EDH errors appear to be standard, I'm not sure if this
specific control
can be standardized. The precise behavior of a counter like this might differ
between
HW implementations.

+
+/*
+ * Get EDH status control
+ *
+ * This control returns the RX_EDH_STS register contents.
+ * Refer to PG290 register space section for more details.
+ */
+#define V4L2_CID_XILINX_SDIRX_EDH_STATUS
	(V4L2_CID_USER_XILINX_SDIRX_BASE + 8)
As mentioned above: this is a dubious control, reporting it in log_status seems
a more logical approach.

+
+/* Get Transport Interlaced status whether it is interlaced or not */
+#define V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED
	(V4L2_CID_USER_XILINX_SDIRX_BASE + 9)
And as also mentioned above, this will be replaced by a new
FIELD_ALTERNATE_PROG?

+
+/*
+ * Xilinx DV timings
+ * TODO - Remove these once they are in v4l2-dv-timings.h
+ */
+#define XLNX_V4L2_DV_BT_2048X1080P24 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		74250000, 510, 44, 148, 4, 5, 36, 0, 0, 0, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#define XLNX_V4L2_DV_BT_2048X1080P25 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		74250000, 400, 44, 148, 4, 5, 36, 0, 0, 0, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#define XLNX_V4L2_DV_BT_2048X1080P30 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		74250000, 66, 20, 66, 4, 5, 36, 0, 0, 0, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#define XLNX_V4L2_DV_BT_2048X1080I48 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(2048, 1080, 1, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		74250000, 329, 44, 329, 2, 5, 15, 3, 5, 15, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#define XLNX_V4L2_DV_BT_2048X1080I50 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(2048, 1080, 1, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		74250000, 274, 44, 274, 2, 5, 15, 3, 5, 15, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#define XLNX_V4L2_DV_BT_2048X1080I60 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(2048, 1080, 1, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		74250000, 66, 20, 66, 2, 5, 15, 3, 5, 15, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#define XLNX_V4L2_DV_BT_2048X1080P48 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		148500000, 510, 44, 148, 4, 5, 36, 0, 0, 0, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#define XLNX_V4L2_DV_BT_2048X1080P50 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		148500000, 400, 44, 148, 4, 5, 36, 0, 0, 0, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#define XLNX_V4L2_DV_BT_2048X1080P60 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		148500000, 88, 44, 20, 4, 5, 36, 0, 0, 0, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#define XLNX_V4L2_DV_BT_1920X1080I48 { \
+	.type = V4L2_DV_BT_656_1120, \
+	V4L2_INIT_BT_TIMINGS(1920, 1080, 1, \
+		V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
+		148500000, 371, 88, 371, 2, 5, 15, 3, 5, 15, \
+		V4L2_DV_BT_STD_SDI) \
+}
+
+#endif /* __UAPI_XILINX_SDIRXSS_H__ */
--
Regards,

Laurent Pinchart
Regards
Vishal Sagar
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help