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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hyun,

Thanks for the review.
Please see my comments.

> -----Original Message-----
> From: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> Sent: Thursday, July 16, 2020 4:13 AM
> To: Vishal Sagar <vsagar@xxxxxxxxxx>
> Cc: laurent.pinchart@xxxxxxxxxxxxxxxx; hverkuil@xxxxxxxxx;
> mchehab@xxxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Michal
> Simek <michals@xxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; joe@xxxxxxxxxxx; Sandip Kothari
> <sandipk@xxxxxxxxxx>; Dinesh Kumar <dineshk@xxxxxxxxxx>; Vishal Sagar
> <vsagar@xxxxxxxxxx>
> Subject: Re: [PATCH v3 3/3] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem
> driver
> 
> Hi Vishal,
> 
> Thank you for the patch. Sorry for late response.
> 
> On Wed, Jun 17, 2020 at 10:33:04PM -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 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 <vishal.sagar@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> > + */
> > +
> > +#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>
> > +#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
> > + */
> > +#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_ST352_DS2_REG		0x70
> > +#define XSDIRX_ST352_DS4_REG		0x74
> > +#define XSDIRX_ST352_DS6_REG		0x78
> > +#define XSDIRX_ST352_DS8_REG		0x7c
> > +#define XSDIRX_ST352_DS10_REG		0x80
> > +#define XSDIRX_ST352_DS12_REG		0x84
> > +#define XSDIRX_ST352_DS14_REG		0x88
> > +#define XSDIRX_ST352_DS16_REG		0x8c
> > +
> > +#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)
> > +#define XSDIRX_RST_CTRL_BRIDGE_CH_FMT_OFFSET		10
> > +#define XSDIRX_RST_CTRL_BRIDGE_CH_FMT_MASK		GENMASK(12,
> 10)
> > +#define XSDIRX_RST_CTRL_BRIDGE_CH_FMT_YUV444		1
> > +
> > +#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)
> > +#define XSDIRX_MDL_CTRL_MODE_AUTO_DET_MASK	GENMASK(13,
> 8)
> > +
> > +#define XSDIRX_MDL_CTRL_FORCED_MODE_OFFSET	16
> > +#define XSDIRX_MDL_CTRL_FORCED_MODE_MASK	GENMASK(18, 16)
> > +
> > +#define XSDIRX_GLBL_INTR_EN_MASK	BIT(0)
> > +
> > +#define XSDIRX_INTR_VIDLOCK_MASK	BIT(0)
> > +#define XSDIRX_INTR_VIDUNLOCK_MASK	BIT(1)
> > +#define XSDIRX_INTR_OVERFLOW_MASK	BIT(9)
> > +#define XSDIRX_INTR_UNDERFLOW_MASK	BIT(10)
> > +
> > +#define XSDIRX_INTR_ALL_MASK	(XSDIRX_INTR_VIDLOCK_MASK |\
> > +				XSDIRX_INTR_VIDUNLOCK_MASK |\
> > +				XSDIRX_INTR_OVERFLOW_MASK |\
> > +				XSDIRX_INTR_UNDERFLOW_MASK)
> > +
> > +#define XSDIRX_ST352_VALID_DS1_MASK	BIT(0)
> > +#define XSDIRX_ST352_VALID_DS3_MASK	BIT(1)
> > +#define XSDIRX_ST352_VALID_DS5_MASK	BIT(2)
> > +#define XSDIRX_ST352_VALID_DS7_MASK	BIT(3)
> > +#define XSDIRX_ST352_VALID_DS9_MASK	BIT(4)
> > +#define XSDIRX_ST352_VALID_DS11_MASK	BIT(5)
> > +#define XSDIRX_ST352_VALID_DS13_MASK	BIT(6)
> > +#define XSDIRX_ST352_VALID_DS15_MASK	BIT(7)
> > +
> > +#define XSDIRX_MODE_DET_STAT_RX_MODE_MASK	GENMASK(2, 0)
> > +#define XSDIRX_MODE_DET_STAT_MODE_LOCK_MASK	BIT(3)
> > +#define XSDIRX_MODE_DET_STAT_ACT_STREAM_MASK	GENMASK(6,
> 4)
> > +#define XSDIRX_MODE_DET_STAT_ACT_STREAM_OFFSET	4
> > +#define XSDIRX_MODE_DET_STAT_LVLB_3G_MASK	BIT(7)
> > +
> > +#define XSDIRX_TS_DET_STAT_LOCKED_MASK		BIT(0)
> > +#define XSDIRX_TS_DET_STAT_SCAN_MASK		BIT(1)
> > +#define XSDIRX_TS_DET_STAT_FAMILY_MASK		GENMASK(7,
> 4)
> > +#define XSDIRX_TS_DET_STAT_RATE_MASK		GENMASK(11, 8)
> > +
> > +#define XSDIRX_TS_DET_STAT_RATE_NONE		0x0
> > +#define XSDIRX_TS_DET_STAT_RATE_23_98HZ		0x2
> > +#define XSDIRX_TS_DET_STAT_RATE_24HZ		0x3
> > +#define XSDIRX_TS_DET_STAT_RATE_47_95HZ		0x4
> > +#define XSDIRX_TS_DET_STAT_RATE_25HZ		0x5
> > +#define XSDIRX_TS_DET_STAT_RATE_29_97HZ		0x6
> > +#define XSDIRX_TS_DET_STAT_RATE_30HZ		0x7
> > +#define XSDIRX_TS_DET_STAT_RATE_48HZ		0x8
> > +#define XSDIRX_TS_DET_STAT_RATE_50HZ		0x9
> > +#define XSDIRX_TS_DET_STAT_RATE_59_94HZ		0xa
> > +#define XSDIRX_TS_DET_STAT_RATE_60HZ		0xb
> > +
> > +#define XSDIRX_EDH_STAT_EDH_AP_MASK	BIT(0)
> > +#define XSDIRX_EDH_STAT_EDH_FF_MASK	BIT(1)
> > +#define XSDIRX_EDH_STAT_EDH_ANC_MASK	BIT(2)
> > +#define XSDIRX_EDH_STAT_AP_FLAG_MASK	GENMASK(8, 4)
> > +#define XSDIRX_EDH_STAT_FF_FLAG_MASK	GENMASK(13, 9)
> > +#define XSDIRX_EDH_STAT_ANC_FLAG_MASK	GENMASK(18, 14)
> > +#define XSDIRX_EDH_STAT_PKT_FLAG_MASK	GENMASK(22, 19)
> > +
> > +#define XSDIRX_EDH_ERRCNT_COUNT_MASK	GENMASK(15, 0)
> > +
> > +#define XSDIRX_CRC_ERRCNT_COUNT_MASK	GENMASK(31, 16)
> > +#define XSDIRX_CRC_ERRCNT_DS_CRC_MASK	GENMASK(15, 0)
> > +
> > +#define XSDIRX_VERSION_REV_MASK		GENMASK(7, 0)
> > +#define XSDIRX_VERSION_PATCHID_MASK	GENMASK(11, 8)
> > +#define XSDIRX_VERSION_VER_REV_MASK	GENMASK(15, 12)
> > +#define XSDIRX_VERSION_VER_MIN_MASK	GENMASK(23, 16)
> > +#define XSDIRX_VERSION_VER_MAJ_MASK	GENMASK(31, 24)
> > +
> > +#define XSDIRX_SS_CONFIG_EDH_INCLUDED_MASK		BIT(1)
> > +
> > +#define XSDIRX_STAT_SB_RX_TDATA_CHANGE_DONE_MASK	BIT(0)
> > +#define XSDIRX_STAT_SB_RX_TDATA_CHANGE_FAIL_MASK	BIT(1)
> > +#define XSDIRX_STAT_SB_RX_TDATA_GT_RESETDONE_MASK	BIT(2)
> > +#define XSDIRX_STAT_SB_RX_TDATA_GT_BITRATE_MASK		BIT(3)
> > +
> > +#define XSDIRX_DEFAULT_WIDTH	1920
> > +#define XSDIRX_DEFAULT_HEIGHT	1080
> > +
> > +#define XSDIRX_MAX_STR_LENGTH	16
> 
> This is not used. Please remove.

Ok will be removed in next version.

> 
> > +
> > +#define XSDIRX_DEFAULT_VIDEO_LOCK_WINDOW	0x3000
> 
> Not sure if this macro is needed. The value can be used directly. Single
> user macro without additional description may be removed. Please check.
> 

Ok I will remove this macro and replace it with 0x3000 in the control.


> > +
> > +#define XSDIRX_MODE_HD_MASK	0x0
> > +#define XSDIRX_MODE_SD_MASK	0x1
> > +#define XSDIRX_MODE_3G_MASK	0x2
> > +#define XSDIRX_MODE_6G_MASK	0x4
> > +#define XSDIRX_MODE_12GI_MASK	0x5
> > +#define XSDIRX_MODE_12GF_MASK	0x6
> 
> These are not masks, so _MASK better be removed from names.
> 

Ok I will remove the suffix _MASK in next version.


> > +
> > +/* Maximum number of events per file handle. */
> > +#define XSDIRX_MAX_EVENTS	8
> > +
> > +/* ST352 related macros */
> > +#define XST352_PAYLOAD_BYTE1_MASK	GENMASK(7, 0)
> > +#define XST352_PAYLOAD_BYTE2_OFFSET	8
> > +#define XST352_PAYLOAD_BYTE3_OFFSET	16
> > +#define XST352_PAYLOAD_BYTE4_OFFSET	24
> 
> Last 3 are not used. Those may have become unused from using FIELD_GET().
> Please remove.
> 

Ok I will remove these unused macro.


> > +
> > +#define XST352_BYTE1_ST292_1x720L_1_5G		0x84
> > +#define XST352_BYTE1_ST292_1x1080L_1_5G		0x85
> > +#define XST352_BYTE1_ST425_2008_750L_3GB	0x88
> > +#define XST352_BYTE1_ST425_2008_1125L_3GA	0x89
> > +#define XST352_BYTE1_ST372_DL_3GB		0x8a
> > +#define XST352_BYTE1_ST372_2x720L_3GB		0x8b
> > +#define XST352_BYTE1_ST372_2x1080L_3GB		0x8c
> > +#define XST352_BYTE1_ST2081_10_2160L_6G		0xc0
> > +#define XST352_BYTE1_ST2081_10_2_1080L_6G	0xc1
> > +#define XST352_BYTE1_ST2081_10_DL_2160L_6G	0xc2
> > +#define XST352_BYTE1_ST2082_10_2160L_12G	0xce
> > +
> > +#define XST352_BYTE2_TS_TYPE_MASK		BIT(15)
> > +#define XST352_BYTE2_PIC_TYPE_MASK		BIT(14)
> > +#define XST352_BYTE2_TS_PIC_TYPE_INTERLACED	0
> > +#define XST352_BYTE2_TS_PIC_TYPE_PROGRESSIVE	1
> 
> Not used. :) Please check and remove unneeded definitions.
> 


Ok I will remove but keep this as comments as reference.


> > +
> > +#define XST352_BYTE2_FPS_MASK			GENMASK(11, 8)
> > +#define XST352_BYTE2_FPS_24F			0x2
> > +#define XST352_BYTE2_FPS_24			0x3
> > +#define XST352_BYTE2_FPS_48F			0x4
> > +#define XST352_BYTE2_FPS_25			0x5
> > +#define XST352_BYTE2_FPS_30F			0x6
> > +#define XST352_BYTE2_FPS_30			0x7
> > +#define XST352_BYTE2_FPS_48			0x8
> > +#define XST352_BYTE2_FPS_50			0x9
> > +#define XST352_BYTE2_FPS_60F			0xa
> > +#define XST352_BYTE2_FPS_60			0xb
> > +/* Table 4 ST 2081-10:2015 */
> > +#define XST352_BYTE2_FPS_96			0xc
> > +#define XST352_BYTE2_FPS_100			0xd
> > +#define XST352_BYTE2_FPS_120			0xe
> > +#define XST352_BYTE2_FPS_120F			0xf
> > +
> > +#define XST352_BYTE3_ACT_LUMA_COUNT_MASK	BIT(22)
> > +
> > +#define XST352_BYTE3_COLOR_FORMAT_MASK		GENMASK(19,
> 16)
> > +#define XST352_BYTE3_COLOR_FORMAT_422		0x0
> > +#define XST352_BYTE3_COLOR_FORMAT_YUV444	0x1
> > +#define XST352_BYTE3_COLOR_FORMAT_420		0x3
> > +#define XST352_BYTE3_COLOR_FORMAT_GBR		0x2
> > +
> > +#define XST352_BYTE3_COLORIMETRY_MASK		GENMASK(21,
> 20)
> > +#define XST352_BYTE3_COLORIMETRY_BT709		0
> > +#define XST352_BYTE3_COLORIMETRY_COLOR_VANC	1
> > +#define XST352_BYTE3_COLORIMETRY_UHDTV		2
> > +#define XST352_BYTE3_COLORIMETRY_UNKNOWN	3
> > +
> > +#define XST352_BYTE4_BIT_DEPTH_MASK		GENMASK(25, 24)
> > +#define XST352_BYTE4_BIT_DEPTH_10		0x1
> > +#define XST352_BYTE4_BIT_DEPTH_12		0x2
> > +
> > +/* GT input clock for sdi_rx_clk */
> > +#define CLK_INT		148500000UL
> > +
> > +#define XSDIRXSS_WIDTH_MIN                    720
> > +#define XSDIRXSS_WIDTH_MAX                    4096
> > +#define XSDIRXSS_HEIGHT_MIN                   243
> > +#define XSDIRXSS_HEIGHT_MAX                   2160
> > +#define XSDIRXSS_PIXELCLOCK_MIN               13500000
> > +#define XSDIRXSS_PIXELCLOCK_MAX               594000000
> > +
> > +/**
> > + * enum sdi_family_enc - SDI Transport Video Format Detected with Active
> Pixels
> > + * @XSDIRX_SMPTE_ST_274: SMPTE ST 274 detected with AP 1920x1080
> > + * @XSDIRX_SMPTE_ST_296: SMPTE ST 296 detected with AP 1280x720
> > + * @XSDIRX_SMPTE_ST_2048_2: SMPTE ST 2048-2 detected with AP
> 2048x1080
> > + * @XSDIRX_SMPTE_ST_295: SMPTE ST 295 detected with AP 1920x1080
> > + * @XSDIRX_NTSC: NTSC encoding detected with AP 720x486
> > + * @XSDIRX_PAL: PAL encoding detected with AP 720x576
> > + * @XSDIRX_TS_UNKNOWN: Unknown SMPTE Transport family type
> > + */
> > +enum sdi_family_enc {
> > +	XSDIRX_SMPTE_ST_274	= 0,
> > +	XSDIRX_SMPTE_ST_296	= 1,
> > +	XSDIRX_SMPTE_ST_2048_2	= 2,
> > +	XSDIRX_SMPTE_ST_295	= 3,
> > +	XSDIRX_NTSC		= 8,
> > +	XSDIRX_PAL		= 9,
> > +	XSDIRX_TS_UNKNOWN	= 15
> > +};
> > +
> > +/**
> > + * struct xsdirxss_state - SDI Rx Subsystem device structure
> > + * @subdev: The v4l2 subdev structure
> > + * @ctrl_handler: control handler
> > + * @default_format: default V4L2 media bus format
> > + * @pad: source media pad
> > + * @dev: Platform structure
> > + * @iomem: Base address of subsystem
> > + * @clks: array of clocks
> > + * @prev_is_frac: Previous clock is fractional or not flag
> > + * @bpc: Bits per component, can be 10 or 12
> > + * @mode: 3G/6G/12G mode
> > + * @num_clks: number of clocks
> > + * @include_edh: EDH processor presence
> > + * @slock: spinlock to protect below members
> > + * @format: Active V4L2 format detected
> > + * @src_format: Active V4L2 format on source pad
> > + * @frame_interval: Captures the frame rate
> > + * @current_timings: DV timings from application
> > + * @detected_timings_index: index of DV timings detected on incoming
> stream
> > + * @vidlockwin: Video lock window value set by control
> > + * @edhmask: EDH mask set by control
> > + * @searchmask: Search mask set by control
> > + * @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.
> > + * @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;
> > +	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,
> > +	 * streaming, vidlocked, ts_is_interlaced, framer_enable
> > +	 */
> > +	spinlock_t slock;
> > +	struct v4l2_mbus_framefmt format;
> > +	struct v4l2_mbus_framefmt src_format;
> > +	struct v4l2_fract frame_interval;
> > +	struct v4l2_dv_timings current_timings;
> > +	u32 detected_timings_index;
> > +	u32 vidlockwin;
> > +	u32 edhmask;
> > +	u16 searchmask;
> > +	bool streaming;
> > +	bool vidlocked;
> > +	bool ts_is_interlaced;
> 
> Use of bool in struct is not recommended. Not sure if it's doable with ones
> getting values from other functions, but I beleive above 3 can change using
> bit field or normal integer type.
> 

Ok I will change these to
u32 flag:1;


> > +	bool framer_enable;
> > +};
> > +
> > +/* List of clocks required by UHD-SDI Rx subsystem */
> > +static const char * const xsdirxss_clks[] = {
> > +	"s_axi_aclk", "sdi_rx_clk", "video_out_clk",
> > +};
> 
> This can move into probe() to reduce the scope.
> 

Ok I will move this to probe()


> > +
> > +/* TODO - Add YUV 444/420 and RBG 10/12 bpc mbus formats here */
> > +static const u32 xsdirxss_10bpc_mbus_fmts[] = {
> > +	MEDIA_BUS_FMT_UYVY10_1X20,
> > +};
> > +
> > +static const u32 xsdirxss_12bpc_mbus_fmts[] = {
> > +	MEDIA_BUS_FMT_UYVY12_1X24,
> > +};
> > +
> > +static const struct v4l2_dv_timings fmt_cap[] = {
> > +	V4L2_DV_BT_SDI_720X487I60,
> > +	V4L2_DV_BT_CEA_720X576I50,
> > +	V4L2_DV_BT_CEA_1280X720P24,
> > +	V4L2_DV_BT_CEA_1280X720P25,

<snip>


> > +
> > +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);
> > +}
> > +
> 
> In my opinion, from here to
> 
> > +#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)
> 
> here, inlining single liners or static inline would be better than above macros.
> 

Ok I will move to inlining.


> > +
> > +static int xsdirx_set_modedetect(struct xsdirxss_state *state, u16 mask)
> > +{
> > +	u32 val;
> > +	struct device *dev = state->dev;
> > +
> > +	mask &= XSDIRX_DETECT_ALL_MODES;
> > +	if (!mask) {
> > +		dev_err(dev, "Invalid bit mask = 0x%08x\n", mask);
> > +		return -EINVAL;
> > +	}
> > +

<snip>


> > +	case XSDIRX_TS_DET_STAT_RATE_50HZ:
> > +		frame_interval->numerator = 1000;
> > +		frame_interval->denominator = 50000;
> > +		break;
> > +	case XSDIRX_TS_DET_STAT_RATE_59_94HZ:
> > +		frame_interval->numerator = 1001;
> > +		frame_interval->denominator = 60000;
> > +		break;
> > +	case XSDIRX_TS_DET_STAT_RATE_60HZ:
> > +		frame_interval->numerator = 1000;
> > +		frame_interval->denominator = 60000;
> > +		break;
> > +	default:
> > +		frame_interval->numerator = 1;
> > +		frame_interval->denominator = 1;
> 
> This shouldn't happen, so would it make sense to have an error here?
> 

That's right. This will not happen. I will remove the default case.


> > +	}
> > +}
> > +
> > +static void xsdirxss_set_gtclk(struct xsdirxss_state *state)
> > +{
> > +	struct clk *gtclk;
> 
> This variable is not needed. Up to you.
> 

Right not needed. Will remove in next version.


> > +	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 */
> 
> I'd remove this comment,
> 
> > +	gtclk = state->clks[1].clk;
> > +
> > +	/* calculate clkrate */
> 
> and this. Up to you.
> 

Ok I will remove these comments in next version.


> > +	if (!is_frac)
> > +		clkrate = CLK_INT;
> > +	else
> > +		clkrate = (CLK_INT * 1000) / 1001;
> > +
> > +	ret = clk_set_rate(gtclk, clkrate);
> > +	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);
> 
> Shouldn't the returned rate be checked and warn if diff is too much? Above
> changes the rate by 0.1% which means the rate should be quite precise. So it
> better be checked.
> 

I will check and revert for this in next version.


> > +
> > +	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);
> > +	}

<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);
> 
> This can be done after check below, so IO is skipped when not needed.
> 

Agreed. I will move it after return IRQ_NONE in next version. 


> > +	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;
> > +		if (status & XSDIRX_INTR_OVERFLOW_MASK)
> > +			event.u.data[0] = XILINX_SDIRX_OVERFLOW_EVENT;
> 
> Should be ORed?
> 

I will fix this 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);
> > +	}
> > +	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;
> 
> No need to initialize this.
> 

Agreed. I will change this in next version.


> > +	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);
> > +

<snip>


> > +/**
> > + * 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) {
> 
> Shouldn't this be accessed under spinlock?
> 

Thanks for catching this. I will update in next version.


> > +		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);
> 
> This too.
> 

Noted for update in next version.


> > +	return 0;
> > +}
> > +
> > +/**
> > + * xsdirxss_s_stream - It is used to start/stop the streaming.
> > + * @sd: V4L2 Sub device
> > + * @enable: Flag (True / False)
> > + *
> > + * This function controls the start or stop of streaming for the
> > + * Xilinx SDI Rx Subsystem.
> > + *
> > + * Return: 0 on success, errors otherwise
> > + */
> > +static int xsdirxss_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct xsdirxss_state *xsdirxss = to_xsdirxssstate(sd);
> > +	struct device *dev = xsdirxss->dev;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&xsdirxss->slock, flags);
> > +	if (enable == xsdirxss->streaming) {
> > +		spin_unlock_irqrestore(&xsdirxss->slock, flags);
> > +		dev_dbg(dev, "already in same streaming state as
> requested\n");
> > +		return 0;
> > +	}
> > +
> > +	if (enable) {
> > +		if (!xsdirxss->vidlocked) {
> > +			spin_unlock_irqrestore(&xsdirxss->slock, flags);
> > +			dev_err(dev, "Video is not locked\n");
> > +			return -EINVAL;
> > +		}
> > +		xsdirx_streamflow_control(xsdirxss, true);
> > +	} else {
> > +		xsdirx_streamflow_control(xsdirxss, false);
> 
> This can move out of this if statement, passing enable or !!enable directly.
> Up to you.
> 

Noted. I will update this in the next version.


> > +	}
> > +	spin_unlock_irqrestore(&xsdirxss->slock, flags);
> > +	dev_dbg(dev, "Streaming %s\n", enable ? "started" : "stopped");
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xsdirxss_g_input_status - It is used to determine if the video signal
> > + * is present / locked onto or not.
> > + *

<snip>


> > +
> > +static int xsdirxss_s_dv_timings(struct v4l2_subdev *sd,
> > +				 struct v4l2_dv_timings *timings)
> > +{
> > +	struct xsdirxss_state *state = to_xsdirxssstate(sd);
> > +	u32 i = state->detected_timings_index;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&state->slock, flags);
> > +	if (!state->vidlocked) {
> > +		spin_unlock_irqrestore(&state->slock, flags);
> > +		return -EINVAL;
> > +	}
> > +
> 
> Is the spinlock needed to be held for below?
> 

Yes. The state->format is updated in the irq handler.


> > +	/* input timing should match query dv_timing */
> > +	if (!v4l2_match_dv_timings(timings,
> > +				   &xsdirxss_dv_timings[i].timing,
> > +				   0, false)) {
> > +		spin_unlock_irqrestore(&state->slock, flags);
> > +		return -EINVAL;
> > +	}
> > +
> > +	state->current_timings = *timings;
> > +
> > +	/* Update the media bus format */
> > +	state->src_format = state->format;
> > +	spin_unlock_irqrestore(&state->slock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int xsdirxss_g_dv_timings(struct v4l2_subdev *sd,
> > +				 struct v4l2_dv_timings *timings)
> > +{
> > +	struct xsdirxss_state *state = to_xsdirxssstate(sd);
> > +
> > +	*timings = state->current_timings;
> 
> This needs the lock if above doesn't change, at least per lock description.
> 

Correct. I will fix this in the next version.


> > +	return 0;
> > +}
> > +
> > +static int xsdirxss_dv_timings_cap(struct v4l2_subdev *sd,
> > +				   struct v4l2_dv_timings_cap *cap)
> > +{
> > +	if (cap->pad != 0)
> > +		return -EINVAL;
> > +
> > +	*cap = xsdirxss_timings_cap;
> > +	return 0;
> > +}
> > +

<snip>


> > +		return ret;
> > +
> > +	ret = clk_bulk_prepare_enable(xsdirxss->num_clks, xsdirxss->clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Reset the core */
> > +	xsdirx_streamflow_control(xsdirxss, false);
> > +	XSDIRX_CORE_DISABLE(xsdirxss);
> > +	/* Clear all interrupts */
> > +	xsdirxss_set(xsdirxss, XSDIRX_ISR_REG, XSDIRX_INTR_ALL_MASK);
> > +	xsdirxss_clr(xsdirxss, XSDIRX_IER_REG, XSDIRX_INTR_ALL_MASK);
> > +	xsdirxss_set(xsdirxss, XSDIRX_IER_REG, XSDIRX_INTR_ALL_MASK);
> > +	XSDIRX_GLOBAL_INTR_ENABLE(xsdirxss);
> 
> Looking here, inlining the macro will be better.
> 
> 	xsdirxss_set(xsdirxss, XSDIRX_GLBL_IER_REG,
> XSDIRX_GLBL_INTR_EN_MASK)
> 

Ok I will make it inline function as suggested in next version.


> > +	xsdirxss_write(xsdirxss, XSDIRX_CRC_ERRCNT_REG, 0xffff);
> > +
> > +	/* Register interrupt handler */
> > +	irq = platform_get_irq(pdev, 0);
> > +	ret = devm_request_threaded_irq(dev, irq, NULL, xsdirxss_irq_handler,
> > +					IRQF_ONESHOT, dev_name(dev),
> xsdirxss);
> > +	if (ret) {
> > +		dev_err(dev, "Err = %d Interrupt handler reg failed!\n",
> > +			ret);
> > +		goto clk_err;
> > +	}
> > +
> > +	/* Initialize V4L2 subdevice and media entity */
> > +	xsdirxss->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +	/* Initialize the default format */
> > +	if (xsdirxss->bpc == 10)
> > +		xsdirxss->default_format.code =
> MEDIA_BUS_FMT_UYVY10_1X20;
> > +	else
> > +		xsdirxss->default_format.code =
> MEDIA_BUS_FMT_UYVY12_1X24;
> > +	xsdirxss->default_format.field = V4L2_FIELD_NONE;
> > +	xsdirxss->default_format.colorspace = V4L2_COLORSPACE_REC709;
> > +	xsdirxss->default_format.width = XSDIRX_DEFAULT_WIDTH;
> > +	xsdirxss->default_format.height = XSDIRX_DEFAULT_HEIGHT;
> > +	xsdirxss->default_format.xfer_func = V4L2_XFER_FUNC_709;
> > +	xsdirxss->default_format.quantization =
> V4L2_QUANTIZATION_LIM_RANGE;
> > +
> > +	xsdirxss->format = xsdirxss->default_format;
> > +
> > +	/* Initialize V4L2 subdevice and media entity */
> > +	subdev = &xsdirxss->subdev;
> > +	v4l2_subdev_init(subdev, &xsdirxss_ops);
> > +
> > +	subdev->dev = &pdev->dev;
> > +	strscpy(subdev->name, dev_name(dev), sizeof(subdev->name));
> > +
> > +	subdev->flags = V4L2_SUBDEV_FL_HAS_EVENTS |
> V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +	subdev->entity.ops = &xsdirxss_media_ops;
> > +
> > +	v4l2_set_subdevdata(subdev, xsdirxss);
> > +
> > +	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);
> > +	}
> > +
> > +	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;
> 
> Nit. Ane empty line.
> 

Noted. I will remove in next version.


> > +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 <vsagar@xxxxxxxxxx>");
> > +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@xxxxxxxxxx>
> > + */
> > +
> > +#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,
> > +};
> > +
> > +#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
> 
> Nit, lowercase for hex values.
> 

This will be removed in next version as I plan to remove EDH related controls.


> Thanks!
> -hyun
> 

Regards
Vishal Sagar


> > +
> > +/*
> > + * V4L2 Controls - We reserved 16 controls for this driver.
> > + *
> > + * 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
> > + * 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)
> > +
> > +/*
> > + * 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)
> > +
> > +/*
> > + * 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)
> > +
> > +/* 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)
> > +
> > +/*
> > + * 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)
> > +
> > +/*
> > + * 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)
> > +
> > +/* 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)
> > +
> > +/*
> > + * 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__ */
> > --
> > 2.21.0
> >
> >





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux