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

From: Vishal Sagar
Date: Wed Aug 19 2020 - 09:48:43 EST


Hi Laurent,

Thanks for the review! Please see my comments below.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Thursday, July 16, 2020 3:03 AM
> To: Vishal Sagar <vsagar@xxxxxxxxxx>
> Cc: Hans Verkuil <hverkuil@xxxxxxxxx>; Hyun Kwon <hyunk@xxxxxxxxxx>;
> 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>
> Subject: Re: [PATCH v3 3/3] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem
> driver
>
> Hi Vishal,
>
> Thank you for the patch.
>
> Hans, there are a few questions for you below.
>
> On Thu, Jun 25, 2020 at 11:43:01AM +0200, Hans Verkuil wrote:
> > On 18/06/2020 07:33, Vishal Sagar wrote:
> > > The Xilinx UHD-SDI Rx subsystem soft IP is used to capture native SDI
> > > streams from SDI sources like SDI broadcast equipment like cameras and
> > > mixers. This block outputs either native SDI, native video or
> > > AXI4-Stream compliant data stream for further processing. Please refer
> > > to PG290 for details.
> > >
> > > The driver is used to configure the IP to add framer, search for
> > > specific modes, get the detected mode, stream parameters, errors, etc.
> > > It also generates events for video lock/unlock, bridge over/under flow.
> > >
> > > The driver supports 10/12 bpc YUV 422 media bus format currently. It
> > > also decodes the stream parameters based on the ST352 packet embedded
> in the
> > > stream. In case the ST352 packet isn't present in the stream, the core's
> > > detected properties are used to set stream properties.
> > >
> > > The driver currently supports only the AXI4-Stream IP configuration.
> > >
> > > Signed-off-by: Vishal Sagar <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>
>
> That's an impressive trim down of headers, but I think it may have gone
> a bit too far :-S I've pointed out to headers that are not needed in v2,
> but you've removed all headers that are implicitly included through
> other headers. This makes the driver more fragile, as if headers are
> refactored, there's a large risk of compilation breakages.
>

Ok. I will add back the headers here excluding the ones as suggested in your v2 comment.

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

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

Correct. I will rename accordingly in next version.

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

<snip>

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

Noted. This will be moved to xsdirxss_dv_timings_cap().

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

I will make all these as static inline functions instead of macros.


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

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

Yes I had tested this path. Ok I will see how to take the spin lock.

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

Noted. I will update this in the next version.

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

<snip>


> > > +/**
> > > + * xsdirxss_irq_handler - Interrupt handler for SDI Rx
> > > + * @irq: IRQ number
> > > + * @dev_id: Pointer to device state
> > > + *
> > > + * The SDI Rx interrupts are cleared by writing 1 to corresponding bit.
> > > + *
> > > + * Return: IRQ_HANDLED after handling interrupts
> > > + */
> > > +static irqreturn_t xsdirxss_irq_handler(int irq, void *dev_id)
> > > +{
> > > + struct xsdirxss_state *state = (struct xsdirxss_state *)dev_id;
> > > + struct device *dev = state->dev;
> > > + u32 status;
> > > +
> > > + status = xsdirxss_read(state, XSDIRX_ISR_REG);
> > > + xsdirxss_write(state, XSDIRX_ISR_REG, status);
> > > + dev_dbg(dev, "interrupt status = 0x%08x\n", status);
> > > +
> > > + if (!status)
> > > + return IRQ_NONE;
> > > +
> > > + if (status & XSDIRX_INTR_VIDLOCK_MASK ||
> > > + status & XSDIRX_INTR_VIDUNLOCK_MASK) {
> > > + u32 val1, val2;
> > > + struct v4l2_event event = { 0 };
> > > + unsigned long flags;
> > > +
> > > + dev_dbg(dev, "video lock/unlock interrupt\n");
> > > +
> > > + spin_lock_irqsave(&state->slock, flags);
> > > + xsdirx_streamflow_control(state, false);
> > > +
> > > + val1 = xsdirxss_read(state, XSDIRX_MODE_DET_STAT_REG);
> > > + val2 = xsdirxss_read(state, XSDIRX_TS_DET_STAT_REG);
> > > +
> > > + if ((val1 & XSDIRX_MODE_DET_STAT_MODE_LOCK_MASK) &&
> > > + (val2 & XSDIRX_TS_DET_STAT_LOCKED_MASK)) {
> > > + u32 mask =
> XSDIRX_RST_CTRL_RST_CRC_ERRCNT_MASK |
> > > +
> XSDIRX_RST_CTRL_RST_EDH_ERRCNT_MASK;
> > > +
> > > + dev_dbg(dev, "video lock interrupt\n");
> > > +
> > > + xsdirxss_set(state, XSDIRX_RST_CTRL_REG, mask);
> > > + xsdirxss_clr(state, XSDIRX_RST_CTRL_REG, mask);
> > > +
> > > + val1 = xsdirxss_read(state, XSDIRX_ST352_VALID_REG);
> > > + val2 = xsdirxss_read(state, XSDIRX_ST352_DS1_REG);
> > > +
> > > + dev_dbg(dev, "valid st352 mask = 0x%08x\n", val1);
> > > + dev_dbg(dev, "st352 payload = 0x%08x\n", val2);
> > > +
> > > + if (!xsdirx_get_stream_properties(state)) {
> > > + state->vidlocked = true;
> > > + xsdirxss_set_gtclk(state);
> > > + } else {
> > > + dev_err(dev, "Unable to get stream
> properties!\n");
> > > + state->vidlocked = false;
> > > + }
> > > + } else {
> > > + dev_dbg(dev, "video unlock interrupt\n");
> > > + state->vidlocked = false;
> > > + }
> > > + spin_unlock_irqrestore(&state->slock, flags);
> > > +
> > > + event.type = V4L2_EVENT_SOURCE_CHANGE;
> > > + event.u.src_change.changes =
> V4L2_EVENT_SRC_CH_RESOLUTION;
> > > + v4l2_subdev_notify_event(&state->subdev, &event);
> > > + }
> > > +
> > > + if (status & (XSDIRX_INTR_UNDERFLOW_MASK |
> XSDIRX_INTR_OVERFLOW_MASK)) {
> > > + struct v4l2_event event = { 0 };
> > > +
> > > + dev_dbg(dev, "Video in to AXI4 Stream core under/overflow
> interrupt\n");
> > > +
> > > + event.type = V4L2_EVENT_XILINX_SDIRX_UND_OVR_FLOW;
> > > + if (status & XSDIRX_INTR_UNDERFLOW_MASK)
> > > + event.u.data[0] = XILINX_SDIRX_UNDERFLOW_EVENT;
>
> Should this be
>
> event.u.data[0] |= XILINX_SDIRX_UNDERFLOW_EVENT;
>
> > > + if (status & XSDIRX_INTR_OVERFLOW_MASK)
> > > + event.u.data[0] = XILINX_SDIRX_OVERFLOW_EVENT;
>
> And this
>
> event.u.data[0] |= XILINX_SDIRX_OVERFLOW_EVENT;
>
> in case both underflow and overflow need to be reported at the same time
> ? I'm not sure that can happen in practice though, but even if it can't,
> it also won't hurt to treat the field as a bitflag.
>

Agree. I will keep it as a bit flag in next version.


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

Noted. I will update this in next version.


> > > + }
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * xsdirxss_s_ctrl - This is used to set the Xilinx SDI Rx V4L2 controls
> > > + * @ctrl: V4L2 control to be set
> > > + *
> > > + * This function is used to set the V4L2 controls for the Xilinx SDI Rx
> > > + * Subsystem.
> > > + *
> > > + * Return: 0 on success, errors otherwise
> > > + */
> > > +static int xsdirxss_s_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > + int ret = 0;
> > > + struct xsdirxss_state *xsdirxss =
> > > + container_of(ctrl->handler, struct xsdirxss_state,
> > > + ctrl_handler);
> > > + struct device *dev = xsdirxss->dev;
> > > + unsigned long flags;
> > > +
> > > + dev_dbg(dev, "set ctrl id = 0x%08x val = 0x%08x\n",
> > > + ctrl->id, ctrl->val);
> > > +
> > > + spin_lock_irqsave(&xsdirxss->slock, flags);
> > > +
> > > + if (xsdirxss->streaming) {
> > > + spin_unlock_irqrestore(&xsdirxss->slock, flags);
> > > + dev_err(dev, "Cannot set controls while streaming\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + XSDIRX_CORE_DISABLE(xsdirxss);
> > > + switch (ctrl->id) {
> > > + case V4L2_CID_XILINX_SDIRX_FRAMER:
> > > + xsdirxss->framer_enable = ctrl->val;
> > > + if (xsdirxss->framer_enable) {
> > > + xsdirxss_set(xsdirxss, XSDIRX_MDL_CTRL_REG,
> > > + XSDIRX_MDL_CTRL_FRM_EN_MASK);
> > > + } else {
> > > + xsdirxss_clr(xsdirxss, XSDIRX_MDL_CTRL_REG,
> > > + XSDIRX_MDL_CTRL_FRM_EN_MASK);
> > > + }
> > > + break;
> > > + case V4L2_CID_XILINX_SDIRX_VIDLOCK_WINDOW:
> > > + /*
> > > + * The video lock window is the amount of time for which the
> > > + * the mode and transport stream should be locked to get the
> > > + * video lock interrupt.
> > > + */
> > > + xsdirxss->vidlockwin = ctrl->val;
> > > + xsdirxss_write(xsdirxss, XSDIRX_VID_LOCK_WINDOW_REG,
> > > + xsdirxss->vidlockwin);
> > > + break;
> > > + case V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES:
> > > + xsdirxss->edhmask = ctrl->val & XSDIRX_EDH_ALLERR_MASK;
> >
> > The '& XSDIRX_EDH_ALLERR_MASK' is not needed since the control's 'max'
> value
> > is set to XSDIRX_EDH_ALLERR_MASK, so ctrl->val can't contain any other bits.
> > The control framework ensures that.
> >
> > > + xsdirxss_write(xsdirxss, XSDIRX_EDH_ERRCNT_EN_REG,
> > > + xsdirxss->edhmask);
> > > + break;
> > > + case V4L2_CID_XILINX_SDIRX_SEARCH_MODES:
> > > + if (!ctrl->val) {
> >
> > This check should be done in a try_ctrl function instead of s_ctrl.
> >
> > Interesting, this is the first bitmask control where you don't want it to
> > be 0. If we get more of these in the future, then it would make sense if this
> > is supported in the control framework itself (e.g. if min is set to 1, then
> > that means that the value can't be 0). But for now just check this in the
> > try_ctrl() function.
> >
> > > + spin_unlock_irqrestore(&xsdirxss->slock, flags);
> > > + dev_err(dev, "Select at least one mode!\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (xsdirxss->mode == XSDI_STD_3G) {
> > > + dev_dbg(dev, "Upto 3G supported\n");
> > > + ctrl->val &= ~(BIT(XSDIRX_MODE_6G_OFFSET) |
> > > + BIT(XSDIRX_MODE_12GI_OFFSET) |
> > > + BIT(XSDIRX_MODE_12GF_OFFSET));
> > > + }
> > > +
> > > + if (xsdirxss->mode == XSDI_STD_6G) {
> > > + dev_dbg(dev, "Upto 6G supported\n");
> > > + ctrl->val &= ~(BIT(XSDIRX_MODE_12GI_OFFSET) |
> > > + BIT(XSDIRX_MODE_12GF_OFFSET));
> > > + }
> >
> > This shouldn't be done here. Instead the 'max' field of the control must be
> > set correctly based on the mode. You can call v4l2_ctrl_modify_range() in
> > xsdirxss_probe() to update the max value.
>
> Hans, I wrote this comment in v2:
>
> "The traditional approach to select timing standards is to use ioctls.
> Hans, do you think a custom control is fine here, or should the dv
> timings ioctls be extended (or new sdi timings ioctls created) ?
>
> If we go for a single control, I'm a bit concerned that this control
> will restrict the search when multiple bits are set, but force a
> specific mode when a single bit is set. I don't have enough experience
> with SDI to tell whether this is the right behaviour."
>
> What do you think ? It's of course hard to standardize an API with a
> single device as an example (although there's also a spec to help here),
> so I'm not opposed to using controls in this driver.
>
> > > +
> > > + ret = xsdirx_set_modedetect(xsdirxss, ctrl->val);
> > > + if (!ret)
> > > + xsdirxss->searchmask = ctrl->val;
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + break;
> > > + }
> > > + XSDIRX_CORE_ENABLE(xsdirxss);
> > > +
> > > + spin_unlock_irqrestore(&xsdirxss->slock, flags);
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * xsdirxss_g_volatile_ctrl - get the Xilinx SDI Rx controls
> > > + * @ctrl: Pointer to V4L2 control
> > > + *
> > > + * Return: 0 on success, errors otherwise
> > > + */
> > > +static int xsdirxss_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > + u32 val;
> > > + struct xsdirxss_state *xsdirxss =
> > > + container_of(ctrl->handler,
> > > + struct xsdirxss_state, ctrl_handler);
> > > + struct device *dev = xsdirxss->dev;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&xsdirxss->slock, flags);
> > > + if (!xsdirxss->vidlocked) {
> > > + spin_unlock_irqrestore(&xsdirxss->slock, flags);
> > > + dev_err(dev, "Can't get values when video not locked!\n");
> > > + return -EINVAL;
> > > + }
> > > + switch (ctrl->id) {
> > > + case V4L2_CID_XILINX_SDIRX_MODE_DETECT:
> > > + val = xsdirxss_read(xsdirxss, XSDIRX_MODE_DET_STAT_REG);
> > > + val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK;
> > > +
> > > + switch (val) {
> > > + case XSDIRX_MODE_SD_MASK:
> > > + ctrl->val = XSDIRX_MODE_SD_OFFSET;
> > > + break;
> > > + case XSDIRX_MODE_HD_MASK:
> > > + ctrl->val = XSDIRX_MODE_HD_OFFSET;
> > > + break;
> > > + case XSDIRX_MODE_3G_MASK:
> > > + val = xsdirxss_read(xsdirxss,
> XSDIRX_MODE_DET_STAT_REG);
> > > + val &= XSDIRX_MODE_DET_STAT_LVLB_3G_MASK;
> > > + ctrl->val = val ? XSDIRX_MODE_3GB_OFFSET :
> > > + XSDIRX_MODE_3GA_OFFSET;
> > > + break;
> > > + case XSDIRX_MODE_6G_MASK:
> > > + ctrl->val = XSDIRX_MODE_6G_OFFSET;
> > > + break;
> > > + case XSDIRX_MODE_12GI_MASK:
> > > + ctrl->val = XSDIRX_MODE_12GI_OFFSET;
> > > + break;
> > > + case XSDIRX_MODE_12GF_MASK:
> > > + ctrl->val = XSDIRX_MODE_12GF_OFFSET;
> > > + break;
> > > + }
> >
> > There is no interrupt that will tell you when the mode changes? It's much
> > nicer if updating this control is interrupt driven rather than requiring
> > userspace to poll.
> >
> > > + break;
> > > + case V4L2_CID_XILINX_SDIRX_CRC:
> > > + ctrl->val = xsdirxss_read(xsdirxss, XSDIRX_CRC_ERRCNT_REG);
> > > + xsdirxss_write(xsdirxss, XSDIRX_CRC_ERRCNT_REG, 0xffff);
> > > + break;
> > > + case V4L2_CID_XILINX_SDIRX_EDH_ERRCNT:
> > > + val = xsdirxss_read(xsdirxss, XSDIRX_MODE_DET_STAT_REG);
> > > + val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK;
> > > + if (val == XSDIRX_MODE_SD_MASK) {
> > > + ctrl->val = xsdirxss_read(xsdirxss,
> > > + XSDIRX_EDH_ERRCNT_REG);
> > > + } else {
> > > + spin_unlock_irqrestore(&xsdirxss->slock, flags);
> > > + dev_dbg(dev, "%d - not in SD mode\n", ctrl->id);
> > > + return -EINVAL;
> >
> > Getting a control value shouldn't fail. Just set ctrl->val to 0 and
> > return 0. You can leave the dev_dbg though, that can be useful.
> >
> > > + }
> > > + break;
> > > + case V4L2_CID_XILINX_SDIRX_EDH_STATUS:
> > > + val = xsdirxss_read(xsdirxss, XSDIRX_MODE_DET_STAT_REG);
> > > + val &= XSDIRX_MODE_DET_STAT_RX_MODE_MASK;
> > > + if (val == XSDIRX_MODE_SD_MASK) {
> > > + ctrl->val = xsdirxss_read(xsdirxss,
> > > + XSDIRX_EDH_STAT_REG);
> > > + } else {
> > > + spin_unlock_irqrestore(&xsdirxss->slock, flags);
> > > + dev_dbg(dev, "%d - not in SD mode\n", ctrl->id);
> > > + return -EINVAL;
> >
> > Ditto.
> >
> > > + }
> > > + break;
> > > + case V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED:
> > > + ctrl->val = xsdirxss->ts_is_interlaced;
> > > + break;
> >
> > I assume this control will disappear once you added support for
> > FIELD_ALTERNATE_PROG?
>
> I'm not sure FIELD_ALTERNATE_PROG is a good idea. The v4l2_field
> specifies today how frames are split into multiple buffers. There's an
> implicit assumption that a frame split into two buffers is captured with
> interlacing. In the SDI case, the two concepts get decoupled, a
> progressive frame can be transmitted (and captured) in two separate
> parts. If we add a *_PROG field, we'll need to duplicate most of the
> v4l2_field values with a _PROG suffix, as the progressive frame can be
> captured in alternate buffers on a video node, but also in separate odd
> and even buffers on two video nodes. Tt the hardware level, data is
> transmitted with odd lines on one link, and even lines on a second link.
> There are then two instances of this IP core, one for each link. One
> instance would receive and process the even lines, the other instance
> the odd lines. The output of the two instances can then be connected to
> two separate DMA engines, or combined in the FPGA fabric, depending on
> how the user designs the system.
>

My apologies to give incorrect info regarding this.
In the progressive segmented frame, a progressive captured frame is sent
across to receiver over an interlaced transport. The 2 fields received
are similar to how V4L2_FIELD_ALTERNATE is except that the fields weren't
captured at 2 different times.
So I will add the V4L2_FIELD_ALTERNATE_PROG in next patch version.


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

Agree. I will update this in next version.


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

<snip>

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

Noted. I will update this in the next patch.


> > > + }
> > > +
> > > + if (xsdirxss->include_edh) {
> > > + for (i = 0; i < ARRAY_SIZE(xsdirxss_edh_ctrls); i++) {
> > > + struct v4l2_ctrl *ctrl;
> > > +
> > > + dev_dbg(dev, "%d %s ctrl = 0x%x\n", i,
> > > + xsdirxss_edh_ctrls[i].name,
> > > + xsdirxss_edh_ctrls[i].id);
> > > +
> > > + ctrl = v4l2_ctrl_new_custom(&xsdirxss->ctrl_handler,
> > > + &xsdirxss_edh_ctrls[i],
> > > + NULL);
> > > + }
> > > + }
> > > +
> > > + if (xsdirxss->ctrl_handler.error) {
> > > + dev_err(dev, "failed to add controls\n");
> > > + ret = xsdirxss->ctrl_handler.error;
> > > + goto error;
> > > + }
> > > +
> > > + subdev->ctrl_handler = &xsdirxss->ctrl_handler;
> > > +
> > > + ret = v4l2_ctrl_handler_setup(&xsdirxss->ctrl_handler);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to set controls\n");
> > > + goto error;
> > > + }
> > > +
> > > + platform_set_drvdata(pdev, xsdirxss);
> > > +
> > > + ret = v4l2_async_register_subdev(subdev);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to register subdev\n");
> > > + goto error;
> > > + }
> > > +
> > > + xsdirxss->prev_is_frac = -1;
> > > +
> > > + XSDIRX_CORE_ENABLE(xsdirxss);
> > > +
> > > + return 0;
> > > +error:
> > > + v4l2_ctrl_handler_free(&xsdirxss->ctrl_handler);
> > > + media_entity_cleanup(&subdev->entity);
> > > + XSDIRX_GLOBAL_INTR_DISABLE(xsdirxss);
> > > + xsdirxss_clr(xsdirxss, XSDIRX_IER_REG, XSDIRX_INTR_ALL_MASK);
> > > +clk_err:
> > > + clk_bulk_disable_unprepare(xsdirxss->num_clks, xsdirxss->clks);
> > > + return ret;
> > > +}
> > > +
> > > +static int xsdirxss_remove(struct platform_device *pdev)
> > > +{
> > > + struct xsdirxss_state *xsdirxss = platform_get_drvdata(pdev);
> > > + struct v4l2_subdev *subdev = &xsdirxss->subdev;
> > > +
> > > + XSDIRX_CORE_DISABLE(xsdirxss);
> > > + XSDIRX_GLOBAL_INTR_DISABLE(xsdirxss);
> > > + xsdirxss_clr(xsdirxss, XSDIRX_IER_REG, XSDIRX_INTR_ALL_MASK);
> > > + xsdirx_streamflow_control(xsdirxss, false);
> > > +
> > > + v4l2_async_unregister_subdev(subdev);
> > > + v4l2_ctrl_handler_free(&xsdirxss->ctrl_handler);
> > > + media_entity_cleanup(&subdev->entity);
> > > +
> > > + clk_bulk_disable_unprepare(xsdirxss->num_clks, xsdirxss->clks);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id xsdirxss_of_id_table[] = {
> > > + { .compatible = "xlnx,v-smpte-uhdsdi-rx-ss-2.0" },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, xsdirxss_of_id_table);
> > > +
> > > +static struct platform_driver xsdirxss_driver = {
> > > + .driver = {
> > > + .name = "xilinx-sdirxss",
> > > + .of_match_table = xsdirxss_of_id_table,
> > > + },
> > > + .probe = xsdirxss_probe,
> > > + .remove = xsdirxss_remove,
> > > +};
> > > +
> > > +module_platform_driver(xsdirxss_driver);
> > > +
> > > +MODULE_AUTHOR("Vishal Sagar <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,
> > > +};
> >
> > These are all standard SDI modes, right?
> >
> > > +
> > > +#define XSDIRX_DETECT_ALL_MODES
> (BIT(XSDIRX_MODE_SD_OFFSET) | \
> > > + BIT(XSDIRX_MODE_HD_OFFSET) | \
> > > + BIT(XSDIRX_MODE_3GA_OFFSET) | \
> > > + BIT(XSDIRX_MODE_3GB_OFFSET) | \
> > > + BIT(XSDIRX_MODE_6G_OFFSET) | \
> > > + BIT(XSDIRX_MODE_12GI_OFFSET) | \
> > > + BIT(XSDIRX_MODE_12GF_OFFSET))
> > > +
> > > +/*
> > > + * EDH - Error Detection and Handling.
> > > + * In the SD-SDI mode, the UHD-SDI core fully supports RP 165.
> > > + * The bitmask is named as XSDIRX_EDH_ERRCNT_XX_YY_ERR except
> > > + * for packet checksum error.
> > > + *
> > > + * XX - EDH Error Types
> > > + * ANC - Ancillary Data Packet Errors
> > > + * FF - Full Field Errors
> > > + * AP - Active Portion Errors
> > > + *
> > > + * YY - Error Flags
> > > + * EDH - error detected here
> > > + * EDA - error Detected already
> > > + * IDH - internal error detected here
> > > + * IDA - internal error detected already
> > > + * UES - unknown error status
> > > + *
> > > + * Refer to Sec 4.3 Error Flags in RP 165-1994 for details
> > > + */
> > > +
> > > +#define XSDIRX_EDH_ERRCNT_ANC_EDH_ERR BIT(0)
> > > +#define XSDIRX_EDH_ERRCNT_ANC_EDA_ERR BIT(1)
> > > +#define XSDIRX_EDH_ERRCNT_ANC_IDH_ERR BIT(2)
> > > +#define XSDIRX_EDH_ERRCNT_ANC_IDA_ERR BIT(3)
> > > +#define XSDIRX_EDH_ERRCNT_ANC_UES_ERR BIT(4)
> > > +#define XSDIRX_EDH_ERRCNT_FF_EDH_ERR BIT(5)
> > > +#define XSDIRX_EDH_ERRCNT_FF_EDA_ERR BIT(6)
> > > +#define XSDIRX_EDH_ERRCNT_FF_IDH_ERR BIT(7)
> > > +#define XSDIRX_EDH_ERRCNT_FF_IDA_ERR BIT(8)
> > > +#define XSDIRX_EDH_ERRCNT_FF_UES_ERR BIT(9)
> > > +#define XSDIRX_EDH_ERRCNT_AP_EDH_ERR BIT(10)
> > > +#define XSDIRX_EDH_ERRCNT_AP_EDA_ERR BIT(11)
> > > +#define XSDIRX_EDH_ERRCNT_AP_IDH_ERR BIT(12)
> > > +#define XSDIRX_EDH_ERRCNT_AP_IDA_ERR BIT(13)
> > > +#define XSDIRX_EDH_ERRCNT_AP_UES_ERR BIT(14)
> > > +#define XSDIRX_EDH_ERRCNT_PKT_CHKSUM_ERR BIT(15)
> > > +
> > > +#define XSDIRX_EDH_ALLERR_MASK 0xFFFF
> >
> > Lowercase 0xffff.
> >
> > And these error conditions are also standardized?
> >
> > If so, then I think these defines/enums can be part of V4L2 itself rather
> > than Xilinx specific.
> >
> > > +
> > > +/*
> > > + * V4L2 Controls - We reserved 16 controls for this driver.
> >
> > I'd increase that to 32.
> >
> > > + *
> > > + * The V4L2_CID_XILINX_SDIRX_EDH_* controls are present only if
> > > + * EDH is enabled.
> > > + * The controls which can be set should only be set before enabling
> > > + * streaming. The controls which can be got should be called while
> > > + * streaming to get correct values.
> > > + * The V4L2_CID_XILINX_SDIRX_MODE_DETECT can be called when query
> dv timing
> >
> > query dv timing -> query_dv_timings
> >
> > > + * returns a valid timing.
> > > + */
> > > +
> > > +/*
> > > + * Framer Control to enable or disable the framer. When this is set, the
> framer
> > > + * automatically readjusts the output word alignment to match the
> alignment of
> > > + * each timing reference signal(TRS). Normally this should be set. But user
> may
> > > + * control this input to implement TRS filtering to prevent a signal
> misaligned
> > > + * TRS from causing erroneous alignment changes.
> > > + * Refer to PG205 rx_frame_en for more details.
> > > + */
> > > +#define V4L2_CID_XILINX_SDIRX_FRAMER
> (V4L2_CID_USER_XILINX_SDIRX_BASE + 1)
> > > +
> > > +/*
> > > + * Video Lock Window Control to set the video lock window value
> > > + * This is the amount of time the mode and transport stream need
> > > + * to be locked before a video lock interrupt occurs.
> > > + */
> > > +#define V4L2_CID_XILINX_SDIRX_VIDLOCK_WINDOW
> (V4L2_CID_USER_XILINX_SDIRX_BASE + 2)
> > > +
> > > +/*
> > > + * EDH Error Mask Control to enable EDH error count
> > > + * This control takes in the bitmask of XSDIRX_EDH_ERRCNT_*_ERR to
> enable counting
> > > + * such errors.
> > > + */
> > > +#define V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES
> (V4L2_CID_USER_XILINX_SDIRX_BASE + 3)
> >
> > If these EDH error sources are from the SDI standard, then this can become a
> standard
> > control as well.
> >
> > > +
> > > +/*
> > > + * Mode search Control to pass the bit mask of modes to detect.
> > > + * If only 1 bit is set, the driver programs IP to be in fixed mode else
> > > + * in multi detection mode.
> > > + *
> > > + * Set this when not streaming.
> > > + *
> > > + * bit 0 set to detect SD mode,
> > > + * bit 1 set to detect HD mode,
> > > + * bit 2 set to detect 3GA mode,
> > > + * bit 3 set to detect 3GB mode,
> > > + * bit 4 set to detect 6G mode,
> > > + * bit 5 set to detect 12G integer frame rate mode,
> > > + * bit 6 set to detect 12G fractional frame rate mode,
> > > + */
> > > +#define V4L2_CID_XILINX_SDIRX_SEARCH_MODES
> (V4L2_CID_USER_XILINX_SDIRX_BASE + 4)
> >
> > Same here if these modes are standardized.
> >
> > > +
> > > +/*
> > > + * Get Detected SDI Mode control (read only)
> > > + *
> > > + * Control Value - Mode detected
> > > + * 0 - SD
> > > + * 1 - HD
> > > + * 2 - 3GA
> > > + * 3 - 3GB
> > > + * 4 - 6G
> > > + * 5 - 12G integer frame rate
> > > + * 6 - 12G fractional frame rate
> > > + */
> > > +#define V4L2_CID_XILINX_SDIRX_MODE_DETECT
> (V4L2_CID_USER_XILINX_SDIRX_BASE + 5)
> >
> > Ditto.
> >
> > > +
> > > +/* Get number of CRC errors status control
> > > + *
> > > + * When a CRC is detected on a line, the CRC error signal of that data
> stream
> > > + * becomes asserted starting a few clock cycles after the last CRC word is
> > > + * output on the data stream ports following the EAV that ends the line
> > > + * containing the error. The CRC signal remains asserted for one line time.
> > > + *
> > > + * The LSB 16 bits of value returned by thsi control represent the error
> > > + * signal on each of 16 data streams. The MSB 16 bits contains the
> accumulated
> > > + * error count.
> > > + *
> > > + * Refer to PG205 rx_crc_err_dsX (X = 1 to 16) description for details.
> > > + */
> > > +#define V4L2_CID_XILINX_SDIRX_CRC
> (V4L2_CID_USER_XILINX_SDIRX_BASE + 6)
> >
> > As suggested earlier, I think this should be split into two controls.
> >
> > > +
> > > +/*
> > > + * Get EDH error count control
> > > + *
> > > + * Reading this control will give the number of EDH errors occurred based
> > > + * on the bitmask passed in
> V4L2_CID_XILINX_SDIRX_EDH_ERROR_SOURCES.
> > > + *
> > > + * It increments once per field when any of the error conditions enabled by
> > > + * the RX_EDH_ERRCNT_EN register bit(s) occur during that field.
> > > + *
> > > + * Refer to PG205 rx_edh_errcnt
> > > + */
> > > +#define V4L2_CID_XILINX_SDIRX_EDH_ERRCNT
> (V4L2_CID_USER_XILINX_SDIRX_BASE + 7)
> >
> > Even though the EDH errors appear to be standard, I'm not sure if this
> specific control
> > can be standardized. The precise behavior of a counter like this might differ
> between
> > HW implementations.
> >
> > > +
> > > +/*
> > > + * Get EDH status control
> > > + *
> > > + * This control returns the RX_EDH_STS register contents.
> > > + * Refer to PG290 register space section for more details.
> > > + */
> > > +#define V4L2_CID_XILINX_SDIRX_EDH_STATUS
> (V4L2_CID_USER_XILINX_SDIRX_BASE + 8)
> >
> > As mentioned above: this is a dubious control, reporting it in log_status seems
> > a more logical approach.
> >
> > > +
> > > +/* Get Transport Interlaced status whether it is interlaced or not */
> > > +#define V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED
> (V4L2_CID_USER_XILINX_SDIRX_BASE + 9)
> >
> > And as also mentioned above, this will be replaced by a new
> FIELD_ALTERNATE_PROG?
> >
> > > +
> > > +/*
> > > + * Xilinx DV timings
> > > + * TODO - Remove these once they are in v4l2-dv-timings.h
> > > + */
> > > +#define XLNX_V4L2_DV_BT_2048X1080P24 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 74250000, 510, 44, 148, 4, 5, 36, 0, 0, 0, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#define XLNX_V4L2_DV_BT_2048X1080P25 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 74250000, 400, 44, 148, 4, 5, 36, 0, 0, 0, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#define XLNX_V4L2_DV_BT_2048X1080P30 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 74250000, 66, 20, 66, 4, 5, 36, 0, 0, 0, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#define XLNX_V4L2_DV_BT_2048X1080I48 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 1, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 74250000, 329, 44, 329, 2, 5, 15, 3, 5, 15, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#define XLNX_V4L2_DV_BT_2048X1080I50 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 1, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 74250000, 274, 44, 274, 2, 5, 15, 3, 5, 15, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#define XLNX_V4L2_DV_BT_2048X1080I60 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 1, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 74250000, 66, 20, 66, 2, 5, 15, 3, 5, 15, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#define XLNX_V4L2_DV_BT_2048X1080P48 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 148500000, 510, 44, 148, 4, 5, 36, 0, 0, 0, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#define XLNX_V4L2_DV_BT_2048X1080P50 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 148500000, 400, 44, 148, 4, 5, 36, 0, 0, 0, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#define XLNX_V4L2_DV_BT_2048X1080P60 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(2048, 1080, 0, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 148500000, 88, 44, 20, 4, 5, 36, 0, 0, 0, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#define XLNX_V4L2_DV_BT_1920X1080I48 { \
> > > + .type = V4L2_DV_BT_656_1120, \
> > > + V4L2_INIT_BT_TIMINGS(1920, 1080, 1, \
> > > + V4L2_DV_HSYNC_POS_POL | V4L2_DV_VSYNC_POS_POL, \
> > > + 148500000, 371, 88, 371, 2, 5, 15, 3, 5, 15, \
> > > + V4L2_DV_BT_STD_SDI) \
> > > +}
> > > +
> > > +#endif /* __UAPI_XILINX_SDIRXSS_H__ */
>
> --
> Regards,
>
> Laurent Pinchart

Regards
Vishal Sagar