RE: [PATCH 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem driver

[Vishal Sagar]

Hi Hyun,

Thanks for reviewing the code. 

> -----Original Message-----
> From: Hyun Kwon [mailto:hyun.kwon@xxxxxxxxxx]
> Sent: Friday, June 14, 2019 3:35 AM
> To: Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> Cc: Hyun Kwon <hyunk@xxxxxxxxxx>; Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx>; Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Dinesh Kumar
> <dineshk@xxxxxxxxxx>; Sandip Kothari <sandipk@xxxxxxxxxx>
> Subject: Re: [PATCH 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem
> driver
> 
> Hi Vishal,
> 
> Thanks for the patch.
> 
> On Tue, 2019-06-04 at 06:55:56 -0700, 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 only 10 bpc YUV 422 media bus format. 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 configuration.
> >
> > Signed-off-by: Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> > ---
> >  drivers/media/platform/xilinx/Kconfig          |   11 +
> >  drivers/media/platform/xilinx/Makefile         |    1 +
> >  drivers/media/platform/xilinx/xilinx-sdirxss.c | 1846
> ++++++++++++++++++++++++
> >  include/uapi/linux/xilinx-sdirxss.h            |   63 +
> >  include/uapi/linux/xilinx-v4l2-controls.h      |   30 +
> >  include/uapi/linux/xilinx-v4l2-events.h        |    9 +
> >  6 files changed, 1960 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 cd1a0fd..0c68caa 100644
> > --- a/drivers/media/platform/xilinx/Kconfig
> > +++ b/drivers/media/platform/xilinx/Kconfig
> > @@ -20,6 +20,17 @@ config VIDEO_XILINX_CSI2RXSS
> >  	  Bridge. The driver is used to set the number of active lanes and
> >  	  get short packet data.
> >
> > +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 of 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 6119a34..223f2ea 100644
> > --- a/drivers/media/platform/xilinx/Makefile
> > +++ b/drivers/media/platform/xilinx/Makefile
> > @@ -4,5 +4,6 @@ xilinx-video-objs += xilinx-dma.o xilinx-vip.o xilinx-vipp.o
> >
> >  obj-$(CONFIG_VIDEO_XILINX) += xilinx-video.o
> >  obj-$(CONFIG_VIDEO_XILINX_CSI2RXSS) += xilinx-csi2rxss.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 0000000..ba2d9d0
> > --- /dev/null
> > +++ b/drivers/media/platform/xilinx/xilinx-sdirxss.c
> > @@ -0,0 +1,1846 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Driver for Xilinx SDI Rx Subsystem
> > + *
> > + * Copyright (C) 2017 - 2019 Xilinx, Inc.
> > + *
> > + * Contacts: Vishal Sagar <vishal.sagar@xxxxxxxxxx>

<snip>

> > +/*
> > + * SDI Rx register map, bitmask and offsets
> > + */
> > +#define XSDIRX_RST_CTRL_REG		0x00
> > +#define XSDIRX_MDL_CTRL_REG		0x04
> > +#define XSDIRX_GLBL_IER_REG		0x0C
> > +#define XSDIRX_ISR_REG			0x10
> > +#define XSDIRX_IER_REG			0x14
> > +#define XSDIRX_ST352_VALID_REG		0x18
> > +#define XSDIRX_ST352_DS1_REG		0x1C
> > +#define XSDIRX_ST352_DS3_REG		0x20
> > +#define XSDIRX_ST352_DS5_REG		0x24
> > +#define XSDIRX_ST352_DS7_REG		0x28
> > +#define XSDIRX_ST352_DS9_REG		0x2C
> > +#define XSDIRX_ST352_DS11_REG		0x30
> > +#define XSDIRX_ST352_DS13_REG		0x34
> > +#define XSDIRX_ST352_DS15_REG		0x38
> > +#define XSDIRX_VERSION_REG		0x3C
> > +#define XSDIRX_SS_CONFIG_REG		0x40
> > +#define XSDIRX_MODE_DET_STAT_REG	0x44
> > +#define XSDIRX_TS_DET_STAT_REG		0x48
> > +#define XSDIRX_EDH_STAT_REG		0x4C
> > +#define XSDIRX_EDH_ERRCNT_EN_REG	0x50
> > +#define XSDIRX_EDH_ERRCNT_REG		0x54
> > +#define XSDIRX_CRC_ERRCNT_REG		0x58
> > +#define XSDIRX_VID_LOCK_WINDOW_REG	0x5C
> > +#define XSDIRX_SB_RX_STS_REG		0x60
> > +
> > +#define XSDIRX_RST_CTRL_SS_EN_MASK			BIT(0)
> > +#define XSDIRX_RST_CTRL_SRST_MASK			BIT(1)
> > +#define XSDIRX_RST_CTRL_RST_CRC_ERRCNT_MASK		BIT(2)
> > +#define XSDIRX_RST_CTRL_RST_EDH_ERRCNT_MASK		BIT(3)
> > +#define XSDIRX_RST_CTRL_SDIRX_BRIDGE_ENB_MASK		BIT(8)
> > +#define XSDIRX_RST_CTRL_VIDIN_AXI4S_MOD_ENB_MASK	BIT(9)
> 
> I'd rather put these under corresponding offsets. In that way, it'd be easier
> to follow. Up to you.
> 

I would like to keep this as is.

> > +
> > +#define XSDIRX_MDL_CTRL_FRM_EN_MASK		BIT(4)
> > +#define XSDIRX_MDL_CTRL_MODE_DET_EN_MASK	BIT(5)
> > +#define XSDIRX_MDL_CTRL_MODE_HD_EN_MASK		BIT(8)
> > +#define XSDIRX_MDL_CTRL_MODE_SD_EN_MASK		BIT(9)
> > +#define XSDIRX_MDL_CTRL_MODE_3G_EN_MASK		BIT(10)
> > +#define XSDIRX_MDL_CTRL_MODE_6G_EN_MASK		BIT(11)
> > +#define XSDIRX_MDL_CTRL_MODE_12GI_EN_MASK	BIT(12)
> > +#define XSDIRX_MDL_CTRL_MODE_12GF_EN_MASK	BIT(13)
> 
> These are not actually mask, so I'd remove _MASK. But it maybe just me.
> 

I would like to keep this as is.

> > +#define XSDIRX_MDL_CTRL_MODE_AUTO_DET_MASK	GENMASK(13, 8)
> > +

<snip>

> > +#define XSDIRX_MODE_12GI_MASK	0x5
> > +#define XSDIRX_MODE_12GF_MASK	0x6
> > +
> > +/*
> > + * Maximum number of events per file handle.
> > + */
> 
> Nit, This doesn't have to be multieline comment.
> 

I will make this a single line comment in v2.

> > +#define XSDIRX_MAX_EVENTS	(128)
> > +
> > +/* ST352 related macros */

<snip>

> > +struct xsdirxss_core {
> > +	struct device *dev;
> > +	void __iomem *iomem;
> > +	int irq;
> > +	bool include_edh;
> 
> I wonder if this is required. I guess it's not critical anyway, so up to you.
> 

I would keep this as it is as itâs a IP configuration information.

> > +	int mode;
> > +	struct clk *axi_clk;
> > +	struct clk *sdirx_clk;
> > +	struct clk *vidout_clk;
> > +};
> > +
> > +/**
> > + * struct xsdirxss_state - SDI Rx Subsystem device structure
> > + * @core: Core structure for MIPI SDI Rx Subsystem
> > + * @subdev: The v4l2 subdev structure
> > + * @ctrl_handler: control handler
> > + * @event: Holds the video unlock event
> > + * @formats: Active V4L2 formats on each pad
> > + * @default_format: default V4L2 media bus format
> > + * @frame_interval: Captures the frame rate
> > + * @vip_format: format information corresponding to the active format
> > + * @pads: media pads
> > + * @streaming: Flag for storing streaming state
> > + * @vidlocked: Flag indicating SDI Rx has locked onto video stream
> > + * @ts_is_interlaced: Flag indicating Transport Stream is interlaced.
> > + *
> > + * This structure contains the device driver related parameters
> > + */
> > +struct xsdirxss_state {
> > +	struct xsdirxss_core core;
> > +	struct v4l2_subdev subdev;
> > +	struct v4l2_ctrl_handler ctrl_handler;
> > +	struct v4l2_event event;
> > +	struct v4l2_mbus_framefmt formats[XSDIRX_MEDIA_PADS];
> 
> It's always single port, so this doesn't have to be an array.
> 

I will fix this in v2.

> > +	struct v4l2_mbus_framefmt default_format;
> > +	struct v4l2_fract frame_interval;
> > +	const struct xvip_video_format *vip_format;
> > +	struct media_pad pads[XSDIRX_MEDIA_PADS];
> 
> Ditto.
> 

I will fix this in v2.

> > +	bool streaming;
> > +	bool vidlocked;
> > +	bool ts_is_interlaced;
> > +};
> > +

<snip>

> > +
> > +	dev_dbg(core->dev, "mask = 0x%x\n", mask);
> > +
> > +	val = xsdirxss_read(core, XSDIRX_MDL_CTRL_REG);
> > +	val &= ~(XSDIRX_MDL_CTRL_MODE_DET_EN_MASK);
> > +	val &= ~(XSDIRX_MDL_CTRL_MODE_AUTO_DET_MASK);
> > +	val &= ~(XSDIRX_MDL_CTRL_FORCED_MODE_MASK);
> 
> () is not needed here. I'd avoid unneeded ().
> 

I will fix this in v2.

> > +
> > +	if (hweight16(mask) > 1) {

<snip>

> > +	} else {
> > +		/* Fixed Mode */
> > +		u32 forced_mode_mask = 0;
> 
> Setting this under default below would make it clearer. But up to you.
> 

I will fix this in v2.

> > +
> > +		dev_dbg(core->dev, "Detect fixed mode\n");
> > +

<snip>

> > +
> > +static void xsdirx_globalintr(struct xsdirxss_core *core, bool flag)
> > +{
> > +	if (flag)
> 
> The flag can be determined at build time, so this 'if' isn't needed. Then
> the disabling call can be added to xsdirxss_remove().
> 

I will try to take care of this in v2.

> > +		xsdirxss_set(core, XSDIRX_GLBL_IER_REG,
> > +			     XSDIRX_GLBL_INTR_EN_MASK);
> > +	else
> > +		xsdirxss_clr(core, XSDIRX_GLBL_IER_REG,
> > +			     XSDIRX_GLBL_INTR_EN_MASK);
> > +}
> > +
> > +static void xsdirx_clearintr(struct xsdirxss_core *core, u32 mask)
> > +{
> > +	xsdirxss_set(core, XSDIRX_ISR_REG, mask);
> > +}
> > +
> > +static void xsdirx_vid_bridge_control(struct xsdirxss_core *core, bool
> enable)
> > +{
> > +	if (enable)
> > +		xsdirxss_set(core, XSDIRX_RST_CTRL_REG,
> > +			     XSDIRX_RST_CTRL_SDIRX_BRIDGE_ENB_MASK);
> > +	else
> > +		xsdirxss_clr(core, XSDIRX_RST_CTRL_REG,
> > +			     XSDIRX_RST_CTRL_SDIRX_BRIDGE_ENB_MASK);
> 
> Even these can be inlined. I don't see need for having dynamic check.
> 
> > +}
> > +
> > +static void xsdirx_axis4_bridge_control(struct xsdirxss_core *core, bool
> enable)
> > +{
> > +	if (enable)
> > +		xsdirxss_set(core, XSDIRX_RST_CTRL_REG,
> > +			     XSDIRX_RST_CTRL_VIDIN_AXI4S_MOD_ENB_MASK);
> > +	else
> > +		xsdirxss_clr(core, XSDIRX_RST_CTRL_REG,
> > +			     XSDIRX_RST_CTRL_VIDIN_AXI4S_MOD_ENB_MASK);
> 
> Ditto.
> 
> > +}
> > +
> > +static void xsdirx_streamflow_control(struct xsdirxss_core *core, bool
> enable)
> > +{
> > +	/* The sdi to native bridge is followed by native to axis4 bridge */
> > +	if (enable) {
> > +		xsdirx_axis4_bridge_control(core, enable);
> > +		xsdirx_vid_bridge_control(core, enable);
> > +	} else {
> > +		xsdirx_vid_bridge_control(core, enable);
> > +		xsdirx_axis4_bridge_control(core, enable);
> > +	}
> > +}
> > +
> > +static void xsdirx_streamdowncb(struct xsdirxss_core *core)
> > +{
> > +	xsdirx_streamflow_control(core, false);
> > +}
> 
> These thin wrapers don't seem useful. Wouldn't it be better to inline most of
> these?
> 

I will inline these functions in v2.

> > +
> > +static void xsdirxss_get_framerate(struct v4l2_fract *frame_interval,
> > +				   u32 framerate)

<snip>

> > +	val = xsdirxss_read(core, XSDIRX_TS_DET_STAT_REG);
> > +	if (valid & XSDIRX_ST352_VALID_DS1_MASK) {
> > +		payload = xsdirxss_read(core, XSDIRX_ST352_DS1_REG);
> > +		byte1 = (payload >> XST352_PAYLOAD_BYTE1_SHIFT) &
> > +				XST352_PAYLOAD_BYTE_MASK;
> > +		active_luma = (payload &
> XST352_BYTE3_ACT_LUMA_COUNT_MASK) >>
> > +				XST352_BYTE3_ACT_LUMA_COUNT_OFFSET;
> > +		pic_type = (payload & XST352_BYTE2_PIC_TYPE_MASK) >>
> > +				XST352_BYTE2_PIC_TYPE_OFFSET;
> > +		framerate = (payload >> XST352_BYTE2_FPS_SHIFT) &
> > +				XST352_BYTE2_FPS_MASK;
> > +		tscan = (payload & XST352_BYTE2_TS_TYPE_MASK) >>
> > +				XST352_BYTE2_TS_TYPE_OFFSET;
> 
> Please align consistently throughout the patch. I believe the checkpatch
> --strict warns on these.
> 

Checkpatch didn't warn about these.

> > +		sampling = (payload & XST352_BYTE3_COLOR_FORMAT_MASK)
> >>
> > +			   XST352_BYTE3_COLOR_FORMAT_OFFSET;
> > +	} else {
> > +		dev_dbg(core->dev, "No ST352 payload available : Mode =
> %d\n",
> > +			mode);
> > +		framerate = (val & XSDIRX_TS_DET_STAT_RATE_MASK) >>
> > +				XSDIRX_TS_DET_STAT_RATE_OFFSET;
> > +		tscan = (val & XSDIRX_TS_DET_STAT_SCAN_MASK) >>
> > +				XSDIRX_TS_DET_STAT_SCAN_OFFSET;
> > +	}
> > +
> > +	family = (val & XSDIRX_TS_DET_STAT_FAMILY_MASK) >>
> > +		  XSDIRX_TS_DET_STAT_FAMILY_OFFSET;
> 
> Just nit. Alignment.

Ok I will fix this in v2.

> 
> > +	state->ts_is_interlaced = tscan ? false : true;
> > +
> > +	dev_dbg(core->dev, "ts_is_interlaced = %d, family = %d\n",
> > +		state->ts_is_interlaced, family);
> > +
> > +	switch (mode) {
> > +	case XSDIRX_MODE_HD_MASK:
> > +		if (!valid) {
> > +			/* No payload obtained */
> > +			dev_dbg(core->dev, "frame rate : %d, tscan = %d\n",
> > +				framerate, tscan);
> > +			/*
> > +			 * NOTE : A progressive segmented frame pSF will be
> > +			 * reported incorrectly as Interlaced as we rely on IP's
> > +			 * transport scan locked bit.
> > +			 */
> > +			dev_warn(core->dev, "pSF will be incorrectly reported as
> Interlaced\n");
> > +
> > +			switch (framerate) {
> > +			case XSDIRX_TS_DET_STAT_RATE_23_98HZ:
> > +			case XSDIRX_TS_DET_STAT_RATE_24HZ:
> > +			case XSDIRX_TS_DET_STAT_RATE_25HZ:
> > +			case XSDIRX_TS_DET_STAT_RATE_29_97HZ:
> > +			case XSDIRX_TS_DET_STAT_RATE_30HZ:
> > +				if (family == XSDIRX_SMPTE_ST_296) {
> > +					format->width = 1280;
> > +					format->height = 720;
> > +					format->field = V4L2_FIELD_NONE;
> > +				} else if (family == XSDIRX_SMPTE_ST_2048_2) {
> > +					format->width = 2048;
> > +					format->height = 1080;
> > +					if (tscan)
> > +						format->field =
> V4L2_FIELD_NONE;
> > +					else
> > +						format->field =
> > +							V4L2_FIELD_ALTERNATE;
> > +				} else {
> > +					format->width = 1920;
> > +					format->height = 1080;
> > +					if (tscan)
> > +						format->field =
> V4L2_FIELD_NONE;
> > +					else
> > +						format->field =
> > +							V4L2_FIELD_ALTERNATE;
> > +				}
> > +				break;
> > +			case XSDIRX_TS_DET_STAT_RATE_50HZ:
> > +			case XSDIRX_TS_DET_STAT_RATE_59_94HZ:
> > +			case XSDIRX_TS_DET_STAT_RATE_60HZ:
> > +				if (family == XSDIRX_SMPTE_ST_274) {
> > +					format->width = 1920;
> > +					format->height = 1080;
> > +				} else {
> > +					format->width = 1280;
> > +					format->height = 720;
> > +				}
> > +				format->field = V4L2_FIELD_NONE;
> > +				break;
> > +			default:
> > +				format->width = 1920;
> > +				format->height = 1080;
> > +				format->field = V4L2_FIELD_NONE;
> > +			}
> > +		} else {
> > +			dev_dbg(core->dev, "Got the payload\n");
> > +			switch (byte1) {
> > +			case XST352_BYTE1_ST292_1x720L_1_5G:
> > +				/* SMPTE ST 292-1 for 720 line payloads */
> > +				format->width = 1280;
> > +				format->height = 720;
> > +				break;
> > +			case XST352_BYTE1_ST292_1x1080L_1_5G:
> > +				/* SMPTE ST 292-1 for 1080 line payloads */
> > +				format->height = 1080;
> > +				if (active_luma)
> > +					format->width = 2048;
> > +				else
> > +					format->width = 1920;
> > +				break;
> > +			default:
> > +				dev_dbg(core->dev, "Unknown HD Mode SMPTE
> standard\n");
> > +				return -EINVAL;
> > +			}
> > +		}
> > +		break;
> > +	case XSDIRX_MODE_SD_MASK:
> > +		format->field = V4L2_FIELD_ALTERNATE;
> > +
> > +		switch (family) {
> > +		case XSDIRX_NTSC:
> > +			format->width = 720;
> > +			format->height = 486;
> > +			break;
> > +		case XSDIRX_PAL:
> > +			format->width = 720;
> > +			format->height = 576;
> > +			break;
> > +		default:
> > +			dev_dbg(core->dev, "Unknown SD Mode SMPTE
> standard\n");
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	case XSDIRX_MODE_3G_MASK:
> > +		switch (byte1) {
> > +		case XST352_BYTE1_ST425_2008_750L_3GB:
> > +			/* Sec 4.1.6.1 SMPTE 425-2008 */
> > +		case XST352_BYTE1_ST372_2x720L_3GB:
> > +			/* Table 13 SMPTE 425-2008 */
> > +			format->width = 1280;
> > +			format->height = 720;
> > +			break;
> > +		case XST352_BYTE1_ST425_2008_1125L_3GA:
> > +			/* ST352 Table SMPTE 425-1 */
> > +		case XST352_BYTE1_ST372_DL_3GB:
> > +			/* Table 13 SMPTE 425-2008 */
> > +		case XST352_BYTE1_ST372_2x1080L_3GB:
> > +			/* Table 13 SMPTE 425-2008 */
> > +			format->height = 1080;
> > +			if (active_luma)
> > +				format->width = 2048;
> > +			else
> > +				format->width = 1920;
> > +			break;
> > +		default:
> > +			dev_dbg(core->dev, "Unknown 3G Mode SMPTE
> standard\n");
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	case XSDIRX_MODE_6G_MASK:
> > +		switch (byte1) {
> > +		case XST352_BYTE1_ST2081_10_DL_2160L_6G:
> > +			/* Dual link 6G */
> > +		case XST352_BYTE1_ST2081_10_2160L_6G:
> > +			/* Table 3 SMPTE ST 2081-10 */
> > +			format->height = 2160;
> > +			if (active_luma)
> > +				format->width = 4096;
> > +			else
> > +				format->width = 3840;
> > +			break;
> > +		default:
> > +			dev_dbg(core->dev, "Unknown 6G Mode SMPTE
> standard\n");
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	case XSDIRX_MODE_12GI_MASK:
> > +	case XSDIRX_MODE_12GF_MASK:
> > +		switch (byte1) {
> > +		case XST352_BYTE1_ST2082_10_2160L_12G:
> > +			/* Section 4.3.1 SMPTE ST 2082-10 */
> > +			format->height = 2160;
> > +			if (active_luma)
> > +				format->width = 4096;
> > +			else
> > +				format->width = 3840;
> > +			break;
> > +		default:
> > +			dev_dbg(core->dev, "Unknown 12G Mode SMPTE
> standard\n");
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	default:
> > +		dev_err(core->dev, "Invalid Mode\n");
> > +		return -EINVAL;
> > +	}
> 
> Could you please take another look if this can be organized simpler? This is
> a little too many switch - case and if statements.
> 

I am unable to come up with a better way. :(
I would appreciate if anyone can help regarding this or if this needs more documentation
to understand easily.

> > +
> > +	if (valid) {
> > +		if (pic_type)
> > +			format->field = V4L2_FIELD_NONE;
> > +		else
> > +			format->field = V4L2_FIELD_ALTERNATE;
> > +	}
> > +

<snip>

> > +static int xsdirxss_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	int ret = 0;
> > +	struct xsdirxss_state *xsdirxss =
> > +		container_of(ctrl->handler,
> 
> This doesn't have to be a new line.
> 

Ok but keeping container_of on same line makes the below line go beyond 80 char limit.
So keeping it as it is.

> > +			     struct xsdirxss_state, ctrl_handler);
> > +	struct xsdirxss_core *core = &xsdirxss->core;
> > +
> > +	dev_dbg(core->dev, "set ctrl id = 0x%08x val = 0x%08x\n",
> > +		ctrl->id, ctrl->val);
> > +
> > +	if (xsdirxss->streaming) {
> > +		dev_err(core->dev, "Cannot set controls while streaming\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	xsdirx_core_disable(core);
> 
> Not sure why this is needed here. Can this be part of s_stream?
> 

The core is enabled so that the SDI Rx can at least decode the stream type into
Width,height, frame interval and progressive/interlace type. The subsystem starts
To send out data only when the SDI to Video and Video to AXI4 Stream bridges are enabled.

The bridges are enabled in s_stream().

This is getting disabled and enabled in the s_ctrl so that the core properties can
Be modified.

> > +	switch (ctrl->id) {
> > +	case V4L2_CID_XILINX_SDIRX_FRAMER:

<snip>

> > +static int xsdirxss_log_status(struct v4l2_subdev *sd)
> > +{
> > +	struct xsdirxss_state *xsdirxss = to_xsdirxssstate(sd);
> > +	struct xsdirxss_core *core = &xsdirxss->core;
> > +	u32 data, i;
> 
> 'data' can be declared inside the for loop below.
> 

Ok I will fix this in v2.

> > +
> > +	v4l2_info(sd, "***** SDI Rx subsystem reg dump start *****\n");
> > +	for (i = 0; i < 0x28; i++) {
> > +		data = xsdirxss_read(core, i * 4);
> > +		v4l2_info(sd, "offset 0x%08x data 0x%08x\n",
> > +			  i * 4, data);
> > +	}
> > +	v4l2_info(sd, "***** SDI Rx subsystem reg dump end *****\n");
> > +	return 0;
> > +}
> > +
> > +static void xsdirxss_start_stream(struct xsdirxss_state *xsdirxss)
> > +{
> > +	xsdirx_streamflow_control(&xsdirxss->core, true);
> > +}
> > +
> > +static void xsdirxss_stop_stream(struct xsdirxss_state *xsdirxss)
> > +{
> > +	xsdirx_streamflow_control(&xsdirxss->core, false);
> > +}
> 
> Ditto. Not sure if these single liner wrappers are useful.
> 

I had kept them for readability purpose. But ok I will remove these in v2.

> > +
> > +/**
> > + * xsdirxss_g_frame_interval - Get the frame interval

<snip>

> > + */
> > +static int xsdirxss_get_format(struct v4l2_subdev *sd,
> > +			       struct v4l2_subdev_pad_config *cfg,
> > +					struct v4l2_subdev_format *fmt)
> 
> Please check the alignment.
> 

I will fix this in v2.

> > +{
> > +	struct xsdirxss_state *xsdirxss = to_xsdirxssstate(sd);
> > +	struct xsdirxss_core *core = &xsdirxss->core;
> > +
> > +	if (!xsdirxss->vidlocked) {
> > +		dev_err(core->dev, "Video not locked!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	fmt->format = *__xsdirxss_get_pad_format(xsdirxss, cfg,
> > +						 fmt->pad, fmt->which);
> > +
> > +	dev_dbg(core->dev, "Stream width = %d height = %d Field = %d\n",
> > +		fmt->format.width, fmt->format.height, fmt->format.field);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xsdirxss_set_format - This is used to set the pad format
> > + * @sd: Pointer to V4L2 Sub device structure
> > + * @cfg: Pointer to sub device pad information structure
> > + * @fmt: Pointer to pad level media bus format
> > + *
> > + * This function is used to set the pad format.
> > + * Since the pad format is fixed in hardware, it can't be
> > + * modified on run time.
> > + *
> > + * Return: 0 on success
> > + */
> > +static int xsdirxss_set_format(struct v4l2_subdev *sd,
> > +			       struct v4l2_subdev_pad_config *cfg,
> > +				struct v4l2_subdev_format *fmt)
> 
> Please check the alignment.
> 

I will fix this in v2.

> > +{
> > +	struct v4l2_mbus_framefmt *__format;
> > +	struct xsdirxss_state *xsdirxss = to_xsdirxssstate(sd);
> > +
> > +	dev_dbg(xsdirxss->core.dev,
> > +		"set width %d height %d code %d field %d colorspace %d\n",
> > +		fmt->format.width, fmt->format.height,
> > +		fmt->format.code, fmt->format.field,
> > +		fmt->format.colorspace);
> > +
> > +	__format = __xsdirxss_get_pad_format(xsdirxss, cfg,

<snip>

> > +
> > +	ret = of_property_read_string(node, "xlnx,line-rate",
> > +				      &sdi_std);
> 
> Single line is enough.
> 

Ok I will get it to a single line in v2.

> > +	if (ret < 0) {
> > +		dev_err(core->dev, "xlnx,line-rate property not found\n");
> > +		return ret;
> > +	}
> > +
> > +	if (!strncmp(sdi_std, "12G_SDI_8DS", XSDIRX_MAX_STR_LENGTH)) {
> > +		core->mode = XSDIRXSS_SDI_STD_12G_8DS;
> > +	} else if (!strncmp(sdi_std, "6G_SDI", XSDIRX_MAX_STR_LENGTH)) {
> > +		core->mode = XSDIRXSS_SDI_STD_6G;
> > +	} else if (!strncmp(sdi_std, "3G_SDI", XSDIRX_MAX_STR_LENGTH)) {
> > +		core->mode = XSDIRXSS_SDI_STD_3G;
> > +	} else {
> > +		dev_err(core->dev, "Invalid Line Rate\n");
> > +		return -EINVAL;
> > +	}
> > +	dev_dbg(core->dev, "SDI Rx Line Rate = %s, mode = %d\n", sdi_std,
> > +		core->mode);
> > +
> > +	ports = of_get_child_by_name(node, "ports");
> > +	if (!ports)
> > +		ports = node;
> > +
> > +	for_each_child_of_node(ports, port) {
> > +		const struct xvip_video_format *format;
> > +		struct device_node *endpoint;
> > +
> > +		if (!port->name || of_node_cmp(port->name, "port"))
> > +			continue;
> > +
> > +		format = xvip_of_get_format(port);
> > +		if (IS_ERR(format)) {
> > +			dev_err(core->dev, "invalid format in DT");
> > +			return PTR_ERR(format);
> > +		}
> > +
> > +		dev_dbg(core->dev, "vf_code = %d bpc = %d bpp = %d\n",
> > +			format->vf_code, format->width, format->bpp);
> 
> The commit mentions this driver supports only 10bpc. Wouldn't it be better to
> check it here?
> 

Yes I will add a check for width to be 10 here. 

> > +
> > +		if (format->vf_code != XVIP_VF_YUV_422 &&
> > +		    format->vf_code != XVIP_VF_YUV_420) {
> > +			dev_err(core->dev, "Incorrect UG934 video format
> set.\n");
> 
> You can make this single line as below,
> 
> 	dev_err(core->dev,
> 		"Incorrect UG934 video format set.\n");
> 

Ok will fix this in v2.

> > +			return -EINVAL;
> > +		}
> > +		xsdirxss->vip_format = format;
> > +
> > +		endpoint = of_get_next_child(port, NULL);
> > +		if (!endpoint) {
> > +			dev_err(core->dev, "No port at\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* Count the number of ports. */
> > +		nports++;
> > +	}
> > +
> > +	if (nports != 1) {
> > +		dev_err(core->dev, "invalid number of ports %u\n", nports);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Register interrupt handler */
> > +	core->irq = irq_of_parse_and_map(node, 0);
> > +
> > +	ret = devm_request_irq(core->dev, core->irq, xsdirxss_irq_handler,
> > +			       IRQF_SHARED, "xilinx-sdirxss", xsdirxss);
> > +	if (ret) {
> > +		dev_err(core->dev, "Err = %d Interrupt handler reg failed!\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +

<snip>

> > +			}
> > +		}
> > +	} else {
> > +		dev_dbg(xsdirxss->core.dev, "Not registering the EDH controls
> as EDH is disabled in IP\n");
> 
> Is this worth being here? There's already an equivalent print for dt is parsed.
> 

Agree. I will remove this in v2.

> > +	}
> > +
> > +	if (xsdirxss->ctrl_handler.error) {
> > +		dev_err(&pdev->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(&pdev->dev, "failed to set controls\n");
> > +		goto error;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, xsdirxss);
> > +
> > +	ret = v4l2_async_register_subdev(subdev);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to register subdev\n");
> > +		goto error;
> > +	}
> > +
> > +	xsdirxss->streaming = false;
> > +
> > +	dev_info(xsdirxss->core.dev, "Xilinx SDI Rx Subsystem device found!\n");
> > +
> > +	xsdirx_core_enable(core);
> > +
> > +	return 0;
> > +error:
> > +	v4l2_ctrl_handler_free(&xsdirxss->ctrl_handler);
> > +	media_entity_cleanup(&subdev->entity);
> > +
> > +clk_err:
> > +	clk_disable_unprepare(core->vidout_clk);
> > +vidout_clk_err:
> > +	clk_disable_unprepare(core->sdirx_clk);
> > +rx_clk_err:
> > +	clk_disable_unprepare(core->axi_clk);
> > +	return ret;
> > +}
> > +
> > +static int xsdirxss_remove(struct platform_device *pdev)
> > +{
> > +	struct xsdirxss_state *xsdirxss = platform_get_drvdata(pdev);
> > +	struct v4l2_subdev *subdev = &xsdirxss->subdev;
> > +
> > +	v4l2_async_unregister_subdev(subdev);
> > +	v4l2_ctrl_handler_free(&xsdirxss->ctrl_handler);
> > +	media_entity_cleanup(&subdev->entity);
> > +	clk_disable_unprepare(xsdirxss->core.vidout_clk);
> > +	clk_disable_unprepare(xsdirxss->core.sdirx_clk);
> > +	clk_disable_unprepare(xsdirxss->core.axi_clk);
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id xsdirxss_of_id_table[] = {
> > +	{ .compatible = "xlnx,v-smpte-uhdsdi-rx-ss" },
> > +	{ }
> > +};
> > +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 <vsagar@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Xilinx SDI Rx Subsystem Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/uapi/linux/xilinx-sdirxss.h b/include/uapi/linux/xilinx-
> sdirxss.h
> > new file mode 100644
> > index 0000000..983a43b
> > --- /dev/null
> > +++ b/include/uapi/linux/xilinx-sdirxss.h
> > @@ -0,0 +1,63 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Xilinx SDI Rx Subsystem mode and flag definitions.
> > + *
> > + * Copyright (C) 2019 Xilinx, Inc.
> > + *
> > + * Contacts: Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> > + *
> > + */
> > +
> > +#ifndef __UAPI_XILINX_SDIRXSS_H__
> > +#define __UAPI_XILINX_SDIRXSS_H__
> > +
> > +#include <linux/types.h>
> > +#include <linux/videodev2.h>
> > +
> > +/*
> > + * 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_3G_OFFSET,
> > +	XSDIRX_MODE_6G_OFFSET,
> > +	XSDIRX_MODE_12GI_OFFSET,
> > +	XSDIRX_MODE_12GF_OFFSET,
> > +	XSDIRX_MODE_NUM_SUPPORTED,
> > +};
> > +
> > +#define XSDIRX_DETECT_ALL_MODES
> 	(BIT(XSDIRX_MODE_SD_OFFSET) | \
> > +					BIT(XSDIRX_MODE_HD_OFFSET) | \
> > +					BIT(XSDIRX_MODE_3G_OFFSET) | \
> > +					BIT(XSDIRX_MODE_6G_OFFSET) | \
> > +					BIT(XSDIRX_MODE_12GI_OFFSET) | \
> > +					BIT(XSDIRX_MODE_12GF_OFFSET))
> > +
> > +/*
> > + * EDH Error Types
> > + * ANC - Ancillary Data Packet Errors
> > + * FF - Full Field Errors
> > + * AP - Active Portion Errors
> > + */
> > +
> > +#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
> > +
> > +#endif /* __UAPI_XILINX_SDIRXSS_H__ */
> > diff --git a/include/uapi/linux/xilinx-v4l2-controls.h
> b/include/uapi/linux/xilinx-v4l2-controls.h
> > index f023623..4c68a10 100644
> > --- a/include/uapi/linux/xilinx-v4l2-controls.h
> > +++ b/include/uapi/linux/xilinx-v4l2-controls.h
> > @@ -83,4 +83,34 @@
> >  /* Reset all event counters */
> >  #define V4L2_CID_XILINX_MIPICSISS_RESET_COUNTERS
> (V4L2_CID_XILINX_MIPICSISS + 2)
> >
> > +/*
> > + * Xilinx SDI Rx Subsystem
> > + */
> > +
> > +/* Base ID */
> > +#define V4L2_CID_XILINX_SDIRX			(V4L2_CID_USER_BASE +
> 0xc100)
> > +
> > +/* Framer Control */
> > +#define V4L2_CID_XILINX_SDIRX_FRAMER		(V4L2_CID_XILINX_SDIRX
> + 1)
> > +/* Video Lock Window Control */
> > +#define V4L2_CID_XILINX_SDIRX_VIDLOCK_WINDOW
> 	(V4L2_CID_XILINX_SDIRX + 2)
> > +/* EDH Error Mask Control */
> > +#define V4L2_CID_XILINX_SDIRX_EDH_ERRCNT_ENABLE
> 	(V4L2_CID_XILINX_SDIRX + 3)
> > +/* Mode search Control */
> > +#define V4L2_CID_XILINX_SDIRX_SEARCH_MODES	(V4L2_CID_XILINX_SDIRX
> + 4)
> > +/* Get Detected Mode control */
> > +#define V4L2_CID_XILINX_SDIRX_MODE_DETECT	(V4L2_CID_XILINX_SDIRX
> + 5)
> > +/* Get CRC error status */
> > +#define V4L2_CID_XILINX_SDIRX_CRC		(V4L2_CID_XILINX_SDIRX
> + 6)
> > +/* Get EDH error count control */
> > +#define V4L2_CID_XILINX_SDIRX_EDH_ERRCNT	(V4L2_CID_XILINX_SDIRX
> + 7)
> > +/* Get EDH status control */
> > +#define V4L2_CID_XILINX_SDIRX_EDH_STATUS	(V4L2_CID_XILINX_SDIRX
> + 8)
> > +/* Get Transport Interlaced status */
> > +#define V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED
> 	(V4L2_CID_XILINX_SDIRX + 9)
> > +/* Get Active Streams count */
> > +#define V4L2_CID_XILINX_SDIRX_ACTIVE_STREAMS
> 	(V4L2_CID_XILINX_SDIRX + 10)
> > +/* Is Mode 3GB */
> > +#define V4L2_CID_XILINX_SDIRX_IS_3GB		(V4L2_CID_XILINX_SDIRX
> + 11)
> > +
> >  #endif /* __UAPI_XILINX_V4L2_CONTROLS_H__ */
> > diff --git a/include/uapi/linux/xilinx-v4l2-events.h b/include/uapi/linux/xilinx-
> v4l2-events.h
> > index 2aa357c..7b47595 100644
> > --- a/include/uapi/linux/xilinx-v4l2-events.h
> > +++ b/include/uapi/linux/xilinx-v4l2-events.h
> > @@ -18,4 +18,13 @@
> >  /* Stream Line Buffer full */
> >  #define V4L2_EVENT_XLNXCSIRX_SLBF	(V4L2_EVENT_XLNXCSIRX_CLASS
> | 0x1)
> >
> > +/* Xilinx SMPTE UHD-SDI RX Subsystem */
> > +#define V4L2_EVENT_XLNXSDIRX_CLASS	(V4L2_EVENT_PRIVATE_START |
> 0x200)
> > +/* Video Unlock event */
> > +#define V4L2_EVENT_XLNXSDIRX_VIDUNLOCK
> 	(V4L2_EVENT_XLNXSDIRX_CLASS | 0x1)
> > +/* Video in to AXI4 Stream core underflowed */
> > +#define V4L2_EVENT_XLNXSDIRX_UNDERFLOW
> 	(V4L2_EVENT_XLNXSDIRX_CLASS | 0x2)
> > +/* Video in to AXI4 Stream core overflowed */
> > +#define V4L2_EVENT_XLNXSDIRX_OVERFLOW
> 	(V4L2_EVENT_XLNXSDIRX_CLASS | 0x3)
> 
> Could you please double-check if all events / controls are hard-required? More
> of these will make generic v4l applications harder to run on this driver. It'd
> be nicer if some of these (ex, from sdi spec) can be shared for any sdi if
> possible?
> 

I will document these controls and events in detail in v2.
I think these are needed as they are specific to Xilinx implementation.

Regards
Vishal Sagar

> Thanks,
> -hyun
> 
> > +
> >  #endif /* __UAPI_XILINX_V4L2_EVENTS_H__ */
> > --
> > 1.8.3.1
> >
> >