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 > > > >
