Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Received-SPF: Pass (protection.outlook.com: domain of xilinx.com designates
 149.199.60.100 as permitted sender) receiver=protection.outlook.com;
 client-ip=149.199.60.100; helo=xsj-pvapsmtpgw02;
Date:   Mon, 28 Jan 2019 10:34:30 -0800
From:   Hyun Kwon <hyun.kwon@xilinx.com>
To:     Vishal Sagar <vsagar@xilinx.com>
CC:     Hyun Kwon <hyunk@xilinx.com>,
        Vishal Sagar <vishal.sagar@xilinx.com>,
        "laurent.pinchart@ideasonboard.com" 
        <laurent.pinchart@ideasonboard.com>,
        "mchehab@kernel.org" <mchehab@kernel.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        Michal Simek <michals@xilinx.com>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "sakari.ailus@linux.intel.com" <sakari.ailus@linux.intel.com>,
        "hans.verkuil@cisco.com" <hans.verkuil@cisco.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Dinesh Kumar <dineshk@xilinx.com>,
        Sandip Kothari <sandipk@xilinx.com>
Subject: Re: [PATCH v2 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
 Subsystem driver
Message-ID: <20190128183428.GA23150@smtp.xilinx.com>
References: <1548438777-11203-1-git-send-email-vishal.sagar@xilinx.com>
 <1548438777-11203-3-git-send-email-vishal.sagar@xilinx.com>
 <20190126021457.GB2412@smtp.xilinx.com>
 <CY4PR02MB2709EE444A18D73A3025D200A7960@CY4PR02MB2709.namprd02.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CY4PR02MB2709EE444A18D73A3025D200A7960@CY4PR02MB2709.namprd02.prod.outlook.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jan 2019 18:35:03.5669
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: cb635717-b8d6-4651-e61c-08d6854f51cf
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.60.100];Helo=[xsj-pvapsmtpgw02]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR0201MB3518
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Vishal,

On Mon, 2019-01-28 at 03:16:49 -0800, Vishal Sagar wrote:
> Hi Hyun,
> 
> Thanks for the review.
> 
> > -----Original Message-----
> > From: Hyun Kwon [mailto:hyun.kwon@xilinx.com]
> > Sent: Saturday, January 26, 2019 7:45 AM
> > To: Vishal Sagar <vishal.sagar@xilinx.com>
> > Cc: Hyun Kwon <hyunk@xilinx.com>; laurent.pinchart@ideasonboard.com;
> > mchehab@kernel.org; robh+dt@kernel.org; mark.rutland@arm.com; Michal
> > Simek <michals@xilinx.com>; linux-media@vger.kernel.org;
> > devicetree@vger.kernel.org; sakari.ailus@linux.intel.com;
> > hans.verkuil@cisco.com; linux-arm-kernel@lists.infradead.org; linux-
> > kernel@vger.kernel.org; Dinesh Kumar <dineshk@xilinx.com>; Sandip Kothari
> > <sandipk@xilinx.com>
> > Subject: Re: [PATCH v2 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
> > Subsystem driver
> > 
> > Hi Vishal,
> > 
> > Thanks for the patch.
> > 
> > On Fri, 2019-01-25 at 09:52:57 -0800, Vishal Sagar wrote:
> > > The Xilinx MIPI CSI-2 Rx Subsystem soft IP is used to capture images
> > > from MIPI CSI-2 camera sensors and output AXI4-Stream video data ready
> > > for image processing. Please refer to PG232 for details.
> > >
> > > The driver is used to set the number of active lanes, if enabled
> > > in hardware. The CSI2 Rx controller filters out all packets except for
> > > the packets with data type fixed in hardware. RAW8 packets are always
> > > allowed to pass through.
> > >
> > > It is also used to setup and handle interrupts and enable the core. It
> > > logs all the events in respective counters between streaming on and off.
> > > The generic short packets received are notified to application via
> > > v4l2_events.
> > >
> > > The driver supports only the video format bridge enabled configuration.
> > > Some data types like YUV 422 10bpc, RAW16, RAW20 are supported when
> > the
> > > CSI v2.0 feature is enabled in design. When the VCX feature is enabled,
> > > the maximum number of virtual channels becomes 16 from 4.
> > >
> > > Signed-off-by: Vishal Sagar <vishal.sagar@xilinx.com>
> > > ---
> > > v2
> > > - Fixed comments given by Hyun and Sakari.
> > > - Made all bitmask using BIT() and GENMASK()
> > > - Removed unused definitions
> > > - Removed DPHY access. This will be done by separate DPHY PHY driver.
> > > - Added support for CSI v2.0 for YUV 422 10bpc, RAW16, RAW20 and extra
> > >   virtual channels
> > > - Fixed the ports as sink and source
> > > - Now use the v4l2fwnode API to get number of data-lanes
> > > - Added clock framework support
> > > - Removed the close() function
> > > - updated the set format function
> > > - support only VFB enabled configuration
> > >
> > >  drivers/media/platform/xilinx/Kconfig           |   10 +
> > >  drivers/media/platform/xilinx/Makefile          |    1 +
> > >  drivers/media/platform/xilinx/xilinx-csi2rxss.c | 1609
> > +++++++++++++++++++++++
> > >  include/uapi/linux/xilinx-v4l2-controls.h       |   14 +
> > >  include/uapi/linux/xilinx-v4l2-events.h         |   28 +
> > >  5 files changed, 1662 insertions(+)
> > >  create mode 100644 drivers/media/platform/xilinx/xilinx-csi2rxss.c
> > >  create mode 100644 include/uapi/linux/xilinx-v4l2-events.h
> > >
> > > diff --git a/drivers/media/platform/xilinx/Kconfig
> > b/drivers/media/platform/xilinx/Kconfig
> > > index 74ec8aa..30b4a25 100644
> > > --- a/drivers/media/platform/xilinx/Kconfig
> > > +++ b/drivers/media/platform/xilinx/Kconfig
> > > @@ -10,6 +10,16 @@ config VIDEO_XILINX
> > >
> > >  if VIDEO_XILINX
> > >
> > > +config VIDEO_XILINX_CSI2RXSS
> > > +	tristate "Xilinx CSI2 Rx Subsystem"
> > > +	help
> > > +	  Driver for Xilinx MIPI CSI2 Rx Subsystem. This is a V4L sub-device
> > > +	  based driver that takes input from CSI2 Tx source and converts
> > > +	  it into an AXI4-Stream. The subsystem comprises of a CSI2 Rx
> > > +	  controller, DPHY, an optional I2C controller and a Video Format
> > > +	  Bridge. The driver is used to set the number of active lanes and
> > > +	  get short packet data.
> > > +
> > >  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 4cdc0b1..6119a34 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_CSI2RXSS) += xilinx-csi2rxss.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-csi2rxss.c
> > b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> > > new file mode 100644
> > > index 0000000..5accf01
> > > --- /dev/null
> > > +++ b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> > > @@ -0,0 +1,1609 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Driver for Xilinx MIPI CSI2 Rx Subsystem
> > > + *
> > > + * Copyright (C) 2016 - 2019 Xilinx, Inc.
> > > + *
> > > + * Contacts: Vishal Sagar <vishal.sagar@xilinx.com>
> > > + *
> > > + */
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/v4l2-subdev.h>
> > > +#include <linux/xilinx-v4l2-controls.h>
> > > +#include <linux/xilinx-v4l2-events.h>
> > > +#include <media/media-entity.h>
> > > +#include <media/v4l2-common.h>
> > > +#include <media/v4l2-ctrls.h>
> > > +#include <media/v4l2-event.h>
> > > +#include <media/v4l2-fwnode.h>
> > > +#include <media/v4l2-subdev.h>
> > > +#include "xilinx-vip.h"
> > > +
> > > +/* Register register map */
> > > +#define XCSI_CCR_OFFSET		0x00
> > > +#define XCSI_CCR_SOFTRESET	BIT(1)
> > > +#define XCSI_CCR_ENABLE		BIT(0)
> > > +
> > > +#define XCSI_PCR_OFFSET		0x04
> > > +#define XCSI_PCR_MAXLANES_MASK	GENMASK(4, 3)
> > > +#define XCSI_PCR_ACTLANES_MASK	GENMASK(1, 0)
> > > +
> > > +#define XCSI_CSR_OFFSET		0x10
> > > +#define XCSI_CSR_PKTCNT		GENMASK(31, 16)
> > > +#define XCSI_CSR_SPFIFOFULL	BIT(3)
> > > +#define XCSI_CSR_SPFIFONE	BIT(2)
> > > +#define XCSI_CSR_SLBF		BIT(1)
> > > +#define XCSI_CSR_RIPCD		BIT(0)
> > > +
> > > +#define XCSI_GIER_OFFSET	0x20
> > > +#define XCSI_GIER_GIE		BIT(0)
> > > +
> > > +#define XCSI_ISR_OFFSET		0x24
> > > +#define XCSI_IER_OFFSET		0x28
> > > +
> > > +#define XCSI_ISR_FR		BIT(31)
> > > +#define XCSI_ISR_VCXFE		BIT(30)
> > > +#define XCSI_ISR_WCC		BIT(22)
> > > +#define XCSI_ISR_ILC		BIT(21)
> > > +#define XCSI_ISR_SPFIFOF	BIT(20)
> > > +#define XCSI_ISR_SPFIFONE	BIT(19)
> > > +#define XCSI_ISR_SLBF		BIT(18)
> > > +#define XCSI_ISR_STOP		BIT(17)
> > > +#define XCSI_ISR_SOTERR		BIT(13)
> > > +#define XCSI_ISR_SOTSYNCERR	BIT(12)
> > > +#define XCSI_ISR_ECC2BERR	BIT(11)
> > > +#define XCSI_ISR_ECC1BERR	BIT(10)
> > > +#define XCSI_ISR_CRCERR		BIT(9)
> > > +#define XCSI_ISR_DATAIDERR	BIT(8)
> > > +#define XCSI_ISR_VC3FSYNCERR	BIT(7)
> > > +#define XCSI_ISR_VC3FLVLERR	BIT(6)
> > > +#define XCSI_ISR_VC2FSYNCERR	BIT(5)
> > > +#define XCSI_ISR_VC2FLVLERR	BIT(4)
> > > +#define XCSI_ISR_VC1FSYNCERR	BIT(3)
> > > +#define XCSI_ISR_VC1FLVLERR	BIT(2)
> > > +#define XCSI_ISR_VC0FSYNCERR	BIT(1)
> > > +#define XCSI_ISR_VC0FLVLERR	BIT(0)
> > > +
> > > +#define XCSI_INTR_PROT_MASK	(XCSI_ISR_VC3FSYNCERR |
> > 	XCSI_ISR_VC3FLVLERR |\
> > > +				 XCSI_ISR_VC2FSYNCERR |
> > XCSI_ISR_VC2FLVLERR |\
> > > +				 XCSI_ISR_VC1FSYNCERR |
> > XCSI_ISR_VC1FLVLERR |\
> > > +				 XCSI_ISR_VC0FSYNCERR |
> > 	XCSI_ISR_VC0FLVLERR |\
> > > +				 XCSI_ISR_VCXFE)
> > > +
> > > +#define XCSI_INTR_PKTLVL_MASK	(XCSI_ISR_ECC2BERR |
> > XCSI_ISR_ECC1BERR |\
> > > +				 XCSI_ISR_CRCERR | XCSI_ISR_DATAIDERR)
> > > +
> > > +#define XCSI_INTR_DPHY_MASK	(XCSI_ISR_SOTERR |
> > XCSI_ISR_SOTSYNCERR)
> > > +
> > > +#define XCSI_INTR_SPKT_MASK	(XCSI_ISR_SPFIFOF | XCSI_ISR_SPFIFONE)
> > > +
> > > +#define XCSI_INTR_ERR_MASK	(XCSI_ISR_WCC | XCSI_ISR_ILC |
> > XCSI_ISR_SLBF |\
> > > +				 XCSI_ISR_STOP)
> > > +
> > > +#define XCSI_INTR_FRAMERCVD_MASK	(XCSI_ISR_FR)
> > > +
> > > +#define XCSI_ISR_ALLINTR_MASK	(XCSI_INTR_PROT_MASK |
> > XCSI_INTR_PKTLVL_MASK |\
> > > +				 XCSI_INTR_DPHY_MASK |
> > XCSI_INTR_SPKT_MASK |\
> > > +				 XCSI_INTR_ERR_MASK |
> > XCSI_INTR_FRAMERCVD_MASK)
> > > +
> > > +/*
> > > + * Removed VCXFE mask as it doesn't exist in IER
> > > + * Removed STOP state irq as this will keep driver in irq handler only
> > > + */
> > > +#define XCSI_IER_INTR_MASK	(XCSI_ISR_ALLINTR_MASK &\
> > > +				 ~(XCSI_ISR_STOP | XCSI_ISR_VCXFE))
> > > +
> > > +#define XCSI_SPKTR_OFFSET	0x30
> > > +#define XCSI_SPKTR_DATA		GENMASK(23, 8)
> > > +#define XCSI_SPKTR_VC		GENMASK(7, 6)
> > > +#define XCSI_SPKTR_DT		GENMASK(5, 0)
> > > +
> > > +#define XCSI_VCXR_OFFSET	0x34
> > > +#define XCSI_VCXR_VCERR		GENMASK(23, 0)
> > > +#define XCSI_VCXR_VCSTART	4
> > > +#define XCSI_VCXR_VCEND		15
> > > +#define XCSI_VCXR_FSYNCERR	BIT(1)
> > > +#define XCSI_VCXR_FLVLERR	BIT(0)
> > > +
> > > +#define XCSI_CLKINFR_OFFSET	0x3C
> > > +#define XCSI_CLKINFR_STOP	BIT(1)
> > > +
> > > +#define XCSI_DLXINFR_OFFSET	0x40
> > > +#define XCSI_DLXINFR_STOP	BIT(5)
> > > +#define XCSI_DLXINFR_SOTERR	BIT(1)
> > > +#define XCSI_DLXINFR_SOTSYNCERR	BIT(0)
> > > +#define XCSI_MAXDL_COUNT	0x4
> > > +
> > > +#define XCSI_VCXINF1R_OFFSET		0x60
> > > +#define XCSI_VCXINF1R_LINECOUNT		GENMASK(31, 16)
> > > +#define XCSI_VCXINF1R_LINECOUNT_SHIFT	16
> > > +#define XCSI_VCXINF1R_BYTECOUNT		GENMASK(15, 0)
> > > +
> > > +#define XCSI_VCXINF2R_OFFSET	0x64
> > > +#define XCSI_VCXINF2R_DT	GENMASK(5, 0)
> > > +#define XCSI_MAXVCX_COUNT	16
> > > +
> > > +/*
> > > + * The core takes less than 100 video clock cycles to reset.
> > > + * So choosing a timeout value larger than this.
> > > + */
> > > +#define XCSI_TIMEOUT_VAL	1000 /* us */
> > > +
> > > +/* Maximum short packet events */
> > > +#define XCSI_MAX_SPKT_EVENT	64
> > > +
> > > +/*
> > > + * Sink pad connected to sensor source pad.
> > > + * Source pad connected to next module like demosaic.
> > > + */
> > > +#define XCSI_MEDIA_PADS		2
> > > +#define XCSI_DEFAULT_WIDTH	1920
> > > +#define XCSI_DEFAULT_HEIGHT	1080
> > > +
> > > +/* Max string length for CSI Data type string */
> > > +#define XCSI_PXLFMT_STRLEN_MAX	16
> > > +
> > > +/* This range is the tolerance for DPHY clock of 200 MHz */
> > > +#define XCSI_DPHY_CLK_MIN	197000000000UL
> > > +#define XCSI_DPHY_CLK_MAX	203000000000UL
> > 
> > Could you share where this range comes from? Is it defined somewhere,
> > or from test?
> > 
> > > +#define XCSI_DPHY_CLK_REQ	200000000000UL
> > 
> > The changelog mentions there'll be a separate phy driver. Then wouldn't
> > the phy driver have to set own clock? If the phy driver becomes a child
> > driver of this subsystem driver, then it's fine to do it here.
> > 
> 
> This CSI2 Rx controller is tied to Xilinx DPHY controller. 
> It would be good to have a separate phy driver for Xilinx DPHY which controls this clock.
> But till then, the DPHY 200Mhz clock should be controlled by this driver.
> 
> Is this ok?
> 

The dt binding shouldn't change, because otherwise it means the current
dt binding is incorrect.

> > > +
> > > +/* MIPI CSI2 Data Types from spec */
> > > +#define XCSI_DT_YUV4228B	0x1E
> > > +#define XCSI_DT_YUV42210B	0x1F
> > > +#define XCSI_DT_RGB444		0x20
> > > +#define XCSI_DT_RGB555		0x21
> > > +#define XCSI_DT_RGB565		0x22
> > > +#define XCSI_DT_RGB666		0x23
> > > +#define XCSI_DT_RGB888		0x24
> > > +#define XCSI_DT_RAW6		0x28
> > > +#define XCSI_DT_RAW7		0x29
> > > +#define XCSI_DT_RAW8		0x2A
> > > +#define XCSI_DT_RAW10		0x2B
> > > +#define XCSI_DT_RAW12		0x2C
> > > +#define XCSI_DT_RAW14		0x2D
> > > +#define XCSI_DT_RAW16		0x2E
> > > +#define XCSI_DT_RAW20		0x2F
> > > +
> > > +#define XCSI_VCX_START		4
> > > +#define XCSI_MAX_VC		4
> > > +#define XCSI_MAX_VCX		16
> > > +
> > > +#define XCSI_NEXTREG_OFFSET	4
> > > +
> > > +/* Bayer pattern for RAW data */
> > > +#define XCSI_BAYER_RGGB		0
> > > +#define XCSI_BAYER_BGGR		1
> > > +#define XCSI_BAYER_GBRG		2
> > > +#define XCSI_BAYER_GRBG		3
> > > +
> > > +/* There are 2 events frame sync and frame level error per VC */
> > > +#define XCSI_VCX_NUM_EVENTS	((XCSI_MAX_VCX - XCSI_MAX_VC) * 2)
> > > +
> > > +/*
> > > + * Macro to return "true" or "false" string if bit is set
> > > + */
> > 
> > Single line comment would be fine.
> 
> Agree. Will update it in next version.
> 
> > 
> > > +#define XCSI_GET_BITSET_STR(val, mask)	(val) & (mask) ? "true" : "false"
> > > +
> > > +static const struct of_device_id xcsi2rxss_of_id_table[] = {
> > > +	{ .compatible = "xlnx,mipi-csi2-rx-subsystem-4.0",},
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, xcsi2rxss_of_id_table);
> > > +
> > 
> > This is a little too far from where this is used. :-) I'd move it
> > to closer.
> 
> Ok will move it in next revision.
> 
> > 
> > > +/**
> > > + * struct xcsi2rxss_event - Event log structure
> > > + * @mask: Event mask
> > > + * @name: Name of the event
> > > + * @counter: Count number of events
> > > + */
> > > +struct xcsi2rxss_event {
> > > +	u32 mask;
> > > +	const char * const name;
> > 
> > Nit. extra space after '*'.
> 
> Will fix in next version.
> 
> > 
> > > +	unsigned int counter;
> > > +};
> > > +
> > > +/*
> > > + * struct xcsi2rxss_core - Core configuration CSI2 Rx Subsystem device
> > structure
> > > + * @dev: Platform structure
> > > + * @iomem: Base address of subsystem
> > > + * @irq: requested irq number
> > > + * @enable_active_lanes: If number of active lanes can be modified
> > > + * @max_num_lanes: Maximum number of lanes present
> > > + * @vfb: Video Format Bridge enabled or not
> > > + * @datatype: Data type filter
> > > + * @bayer: bayer pattern
> > > + * @events: Structure to maintain event logs
> > > + * @vcx_events: Structure to maintain VCX event logs
> > > + * @en_vcx: If more than 4 VC are enabled
> > > + * @en_csi_v20: If CSI v2.0 is enabled
> > > + * @lite_aclk: AXI4-Lite interface clock
> > > + * @video_aclk: Video clock
> > > + * @dphy_clk_200M: 200MHz DPHY clock
> > > + */
> > > +struct xcsi2rxss_core {
> > > +	struct device *dev;
> > > +	void __iomem *iomem;
> > > +	int irq;
> > > +	bool enable_active_lanes;
> > > +	u32 max_num_lanes;
> > > +	bool vfb;
> > > +	u32 datatype;
> > > +	u32 bayer;
> > > +	struct xcsi2rxss_event *events;
> > > +	struct xcsi2rxss_event *vcx_events;
> > > +	bool en_vcx;
> > > +	bool en_csi_v20;
> > > +	struct clk *lite_aclk;
> > > +	struct clk *video_aclk;
> > > +	struct clk *dphy_clk_200M;
> > > +};
> > > +
> > > +/**
> > > + * struct xcsi2rxss_state - CSI2 Rx Subsystem device structure
> > > + * @core: Core structure for MIPI CSI2 Rx Subsystem
> > > + * @subdev: The v4l2 subdev structure
> > > + * @ctrl_handler: control handler
> > > + * @format: Active V4L2 formats on each pad
> > > + * @event: Holds the short packet event
> > > + * @lock: mutex for serializing operations
> > 
> > It may be just me, but it's not clear what it wants to serialize.
> > The comment better describe what resource it protects.
> 
> Ok will mention that in the next revision.
> > 
> > > + * @pads: media pads
> > > + * @streaming: Flag for storing streaming state
> > > + *
> > > + * This structure contains the device driver related parameters
> > > + */
> > > +struct xcsi2rxss_state {
> > > +	struct xcsi2rxss_core core;
> > > +	struct v4l2_subdev subdev;
> > > +	struct v4l2_ctrl_handler ctrl_handler;
> > > +	struct v4l2_mbus_framefmt format;
> > > +	struct v4l2_event event;
> > > +	/* used to serialize v4l2 operations */
> > > +	struct mutex lock;
> > > +	struct media_pad pads[XCSI_MEDIA_PADS];
> > > +	bool streaming;
> > > +};
> > > +
> > > +static struct xcsi2rxss_event xcsi2rxss_events[] = {
> > > +	{ XCSI_ISR_FR, "Frame Received", 0 },
> > > +	{ XCSI_ISR_VCXFE, "VCX Frame Errors", 0 },
> > > +	{ XCSI_ISR_WCC, "Word Count Errors", 0 },
> > > +	{ XCSI_ISR_ILC, "Invalid Lane Count Error", 0 },
> > > +	{ XCSI_ISR_SPFIFOF, "Short Packet FIFO OverFlow Error", 0 },
> > > +	{ XCSI_ISR_SPFIFONE, "Short Packet FIFO Not Empty", 0 },
> > > +	{ XCSI_ISR_SLBF, "Streamline Buffer Full Error", 0 },
> > > +	{ XCSI_ISR_STOP, "Lane Stop State", 0 },
> > > +	{ XCSI_ISR_SOTERR, "SOT Error", 0 },
> > > +	{ XCSI_ISR_SOTSYNCERR, "SOT Sync Error", 0 },
> > > +	{ XCSI_ISR_ECC2BERR, "2 Bit ECC Unrecoverable Error", 0 },
> > > +	{ XCSI_ISR_ECC1BERR, "1 Bit ECC Recoverable Error", 0 },
> > > +	{ XCSI_ISR_CRCERR, "CRC Error", 0 },
> > > +	{ XCSI_ISR_DATAIDERR, "Data Id Error", 0 },
> > > +	{ XCSI_ISR_VC3FSYNCERR, "Virtual Channel 3 Frame Sync Error", 0 },
> > > +	{ XCSI_ISR_VC3FLVLERR, "Virtual Channel 3 Frame Level Error", 0 },
> > > +	{ XCSI_ISR_VC2FSYNCERR, "Virtual Channel 2 Frame Sync Error", 0 },
> > > +	{ XCSI_ISR_VC2FLVLERR, "Virtual Channel 2 Frame Level Error", 0 },
> > > +	{ XCSI_ISR_VC1FSYNCERR, "Virtual Channel 1 Frame Sync Error", 0 },
> > > +	{ XCSI_ISR_VC1FLVLERR, "Virtual Channel 1 Frame Level Error", 0 },
> > > +	{ XCSI_ISR_VC0FSYNCERR, "Virtual Channel 0 Frame Sync Error", 0 },
> > > +	{ XCSI_ISR_VC0FLVLERR, "Virtual Channel 0 Frame Level Error", 0 }
> > > +};
> > > +
> > > +#define XCSI_NUM_EVENTS		ARRAY_SIZE(xcsi2rxss_events)
> > > +
> > > +static inline struct xcsi2rxss_state *
> > > +to_xcsi2rxssstate(struct v4l2_subdev *subdev)
> > > +{
> > > +	return container_of(subdev, struct xcsi2rxss_state, subdev);
> > > +}
> > > +
> > > +/*
> > > + * Register related operations
> > > + */
> > 
> > The xilinx-vip has same IO functions, but up to you.
> > 
> > > +static inline u32 xcsi2rxss_read(struct xcsi2rxss_core *xcsi2rxss, u32 addr)
> > > +{
> > > +	return ioread32(xcsi2rxss->iomem + addr);
> > > +}
> > > +
> > > +static inline void xcsi2rxss_write(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
> > > +				   u32 value)
> > > +{
> > > +	iowrite32(value, xcsi2rxss->iomem + addr);
> > > +}
> > > +
> > > +static inline void xcsi2rxss_clr(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
> > > +				 u32 clr)
> > > +{
> > > +	xcsi2rxss_write(xcsi2rxss, addr,
> > > +			xcsi2rxss_read(xcsi2rxss, addr) & ~clr);
> > > +}
> > > +
> > > +static inline void xcsi2rxss_set(struct xcsi2rxss_core *xcsi2rxss, u32 addr,
> > > +				 u32 set)
> > > +{
> > > +	xcsi2rxss_write(xcsi2rxss, addr,
> > > +			xcsi2rxss_read(xcsi2rxss, addr) | set);
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_clr_and_set - Clear and set the register with a bitmask
> > > + * @xcsi2rxss: Xilinx MIPI CSI2 Rx Subsystem subdev core struct
> > > + * @addr: address of register
> > > + * @clr: bitmask to be cleared
> > > + * @set: bitmask to be set
> > > + *
> > > + * Clear a bit(s) of mask @clr in the register at address @addr, then set
> > > + * a bit(s) of mask @set in the register after.
> > > + */
> > > +static void xcsi2rxss_clr_and_set(struct xcsi2rxss_core *xcsi2rxss,
> > > +				  u32 addr, u32 clr, u32 set)
> > > +{
> > > +	u32 reg;
> > > +
> > > +	reg = xcsi2rxss_read(xcsi2rxss, addr);
> > > +	reg &= ~clr;
> > > +	reg |= set;
> > > +	xcsi2rxss_write(xcsi2rxss, addr, reg);
> > > +}
> > > +
> > > +static void xcsi2rxss_enable(struct xcsi2rxss_core *core)
> > > +{
> > > +	xcsi2rxss_set(core, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> > > +}
> > > +
> > > +static void xcsi2rxss_disable(struct xcsi2rxss_core *core)
> > > +{
> > > +	xcsi2rxss_clr(core, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> > > +}
> > > +
> > > +static void xcsi2rxss_intr_enable(struct xcsi2rxss_core *core)
> > > +{
> > > +	xcsi2rxss_clr(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> > > +	xcsi2rxss_write(core, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
> > > +	xcsi2rxss_set(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> > > +}
> > > +
> > > +static void xcsi2rxss_intr_disable(struct xcsi2rxss_core *core)
> > > +{
> > > +	xcsi2rxss_clr(core, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
> > > +	xcsi2rxss_clr(core, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_reset - Does a soft reset of the MIPI CSI2 Rx Subsystem
> > > + * @core: Core Xilinx CSI2 Rx Subsystem structure pointer
> > > + *
> > > + * Core takes less than 100 video clock cycles to reset.
> > > + * So a larger timeout value is chosen for margin.
> > > + *
> > > + * Return: 0 - on success OR -ETIME if reset times out
> > > + */
> > > +static int xcsi2rxss_reset(struct xcsi2rxss_core *core)
> > > +{
> > > +	u32 timeout = XCSI_TIMEOUT_VAL;
> > > +
> > > +	xcsi2rxss_set(core, XCSI_CCR_OFFSET, XCSI_CCR_SOFTRESET);
> > > +
> > > +	while (xcsi2rxss_read(core, XCSI_CSR_OFFSET) & XCSI_CSR_RIPCD) {
> > > +		if (timeout == 0) {
> > > +			dev_err(core->dev, "soft reset timed out!\n");
> > > +			return -ETIME;
> > > +		}
> > > +
> > > +		timeout--;
> > > +		udelay(1);
> > > +	}
> > > +
> > > +	xcsi2rxss_clr(core, XCSI_CCR_OFFSET, XCSI_CCR_SOFTRESET);
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_irq_handler - Interrupt handler for CSI-2
> > > + * @irq: IRQ number
> > > + * @dev_id: Pointer to device state
> > > + *
> > > + * In the interrupt handler, a list of event counters are updated for
> > > + * corresponding interrupts. This is useful to get status / debug.
> > > + * If the short packet FIFO not empty or overflow interrupt is received
> > > + * capture the short packet and notify of event occurrence
> > > + *
> > > + * Return: IRQ_HANDLED after handling interrupts
> > > + *         IRQ_NONE is no interrupts
> > > + */
> > > +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *dev_id)
> > > +{
> > > +	struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)dev_id;
> > > +	struct xcsi2rxss_core *core = &state->core;
> > > +	u32 status;
> > > +
> > > +	status = xcsi2rxss_read(core, XCSI_ISR_OFFSET) &
> > XCSI_ISR_ALLINTR_MASK;
> > > +	dev_dbg(core->dev, "interrupt status = 0x%08x\n", status);
> > 
> > Shouldn't this be rate-limited? I let you decide.
> 
> Right. I will add that to all messages in the IRQ handler.
> 
> > 
> > > +
> > > +	if (!status)
> > > +		return IRQ_NONE;
> > > +
> > > +	/* Received a short packet */
> > > +	if (status & XCSI_ISR_SPFIFONE) {
> > > +		memset(&state->event, 0, sizeof(state->event));
> > > +		state->event.type = V4L2_EVENT_XLNXCSIRX_SPKT;
> > > +		*((u32 *)(&state->event.u.data)) =
> > > +			xcsi2rxss_read(core, XCSI_SPKTR_OFFSET);
> > > +		v4l2_subdev_notify_event(&state->subdev, &state->event);
> > > +	}
> > > +
> > > +	/* Short packet FIFO overflow */
> > > +	if (status & XCSI_ISR_SPFIFOF) {
> > > +		dev_alert(core->dev, "Short packet FIFO overflowed\n");
> > > +		memset(&state->event, 0, sizeof(state->event));
> > > +		state->event.type = V4L2_EVENT_XLNXCSIRX_SPKT_OVF;
> > > +		v4l2_subdev_notify_event(&state->subdev, &state->event);
> > > +	}
> > > +
> > > +	/*
> > > +	 * Stream line buffer full
> > > +	 * This means there is a backpressure from downstream IP
> > > +	 */
> > > +	if (status & XCSI_ISR_SLBF) {
> > > +		dev_alert(core->dev, "Stream Line Buffer Full!\n");
> > > +		memset(&state->event, 0, sizeof(state->event));
> > > +		state->event.type = V4L2_EVENT_XLNXCSIRX_SLBF;
> > > +		v4l2_subdev_notify_event(&state->subdev, &state->event);
> > > +	}
> > > +
> > > +	/* Increment event counters */
> > > +	if (status & XCSI_ISR_ALLINTR_MASK) {
> > > +		unsigned int i;
> > > +
> > > +		for (i = 0; i < XCSI_NUM_EVENTS; i++) {
> > > +			if (!(status & core->events[i].mask))
> > > +				continue;
> > > +			core->events[i].counter++;
> > > +			dev_dbg(core->dev, "%s: %d\n", core->events[i].name,
> > > +				core->events[i].counter);
> > > +		}
> > > +
> > > +		if (status & XCSI_ISR_VCXFE && core->en_vcx) {
> > > +			u32 vcxstatus;
> > > +
> > > +			vcxstatus = xcsi2rxss_read(core, XCSI_VCXR_OFFSET);
> > > +			vcxstatus &= XCSI_VCXR_VCERR;
> > > +			for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> > > +				if (!(vcxstatus & core->vcx_events[i].mask))
> > > +					continue;
> > > +				core->vcx_events[i].counter++;
> > > +			}
> > > +			xcsi2rxss_write(core, XCSI_VCXR_OFFSET, vcxstatus);
> > > +		}
> > > +	}
> > > +
> > > +	xcsi2rxss_write(core, XCSI_ISR_OFFSET, status);
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static void xcsi2rxss_reset_event_counters(struct xcsi2rxss_state *state)
> > > +{
> > > +	int i;
> > 
> > Nit. unsigned.
> > 
> 
> Ok. 
> 
> > > +
> > > +	for (i = 0; i < XCSI_NUM_EVENTS; i++)
> > > +		state->core.events[i].counter = 0;
> > > +
> > > +	if (state->core.en_vcx)
> > 
> > I don't know what's recommened style, but for multiline statement,
> > putting braces seems better for readability. Up to you.
> > 
> 
> I didn't get any checkpatch warnings for this so kept it like this.
> Ok I will add the braces.
> 
> > > +		for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++)
> > > +			state->core.vcx_events[i].counter = 0;
> > > +}
> > > +
> > > +/* Print event counters */
> > > +static void xcsi2rxss_log_counters(struct xcsi2rxss_state *state)
> > > +{
> > > +	struct xcsi2rxss_core *core = &state->core;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < XCSI_NUM_EVENTS; i++) {
> > > +		if (core->events[i].counter > 0)
> > > +			dev_info(core->dev, "%s events: %d\n",
> > > +				 core->events[i].name,
> > > +				 core->events[i].counter);
> > > +	}
> > > +
> > > +	if (core->en_vcx)
> > > +		for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> > > +			if (core->vcx_events[i].counter > 0)
> > > +				dev_info(core->dev,
> > > +					 "VC %d Frame %s err vcx events: %d\n",
> > > +					 (i / 2) + XCSI_VCX_START,
> > > +					 i & 1 ? "Sync" : "Level",
> > > +					 core->vcx_events[i].counter);
> > > +		}
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_log_status - Logs the status of the CSI-2 Receiver
> > > + * @sd: Pointer to V4L2 subdevice structure
> > > + *
> > > + * This function prints the current status of Xilinx MIPI CSI-2
> > > + *
> > > + * Return: 0 on success
> > > + */
> > > +static int xcsi2rxss_log_status(struct v4l2_subdev *sd)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > > +	struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > > +	unsigned int reg, data, i, max_vc;
> > > +
> > > +	mutex_lock(&xcsi2rxss->lock);
> > 
> > This lock seems to protect access to xcsi2rxss. If so, could you please
> > clarify in the lock description?
> 
> Yes will update the name in the next version. 
> > 
> > > +
> > > +	xcsi2rxss_log_counters(xcsi2rxss);
> > > +
> > > +	dev_info(core->dev, "***** Core Status *****\n");
> > > +	data = xcsi2rxss_read(core, XCSI_CSR_OFFSET);
> > > +	dev_info(core->dev, "Short Packet FIFO Full = %s\n",
> > > +		 XCSI_GET_BITSET_STR(data, XCSI_CSR_SPFIFOFULL));
> > > +	dev_info(core->dev, "Short Packet FIFO Not Empty = %s\n",
> > > +		 XCSI_GET_BITSET_STR(data, XCSI_CSR_SPFIFONE));
> > > +	dev_info(core->dev, "Stream line buffer full = %s\n",
> > > +		 XCSI_GET_BITSET_STR(data, XCSI_CSR_SLBF));
> > > +	dev_info(core->dev, "Soft reset/Core disable in progress = %s\n",
> > > +		 XCSI_GET_BITSET_STR(data, XCSI_CSR_RIPCD));
> > > +
> > > +	/* Clk & Lane Info  */
> > > +	dev_info(core->dev, "******** Clock Lane Info *********\n");
> > > +	data = xcsi2rxss_read(core, XCSI_CLKINFR_OFFSET);
> > > +	dev_info(core->dev, "Clock Lane in Stop State = %s\n",
> > > +		 XCSI_GET_BITSET_STR(data, XCSI_CLKINFR_STOP));
> > > +
> > > +	dev_info(core->dev, "******** Data Lane Info *********\n");
> > > +	dev_info(core->dev, "Lane\tSoT Error\tSoT Sync Error\tStop State\n");
> > > +	reg = XCSI_DLXINFR_OFFSET;
> > > +	for (i = 0; i < XCSI_MAXDL_COUNT; i++) {
> > > +		data = xcsi2rxss_read(core, reg);
> > > +
> > > +		dev_info(core->dev, "%d\t%s\t\t%s\t\t%s\n", i,
> > > +			 XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_SOTERR),
> > > +			 XCSI_GET_BITSET_STR(data,
> > XCSI_DLXINFR_SOTSYNCERR),
> > > +			 XCSI_GET_BITSET_STR(data, XCSI_DLXINFR_STOP));
> > > +
> > > +		reg += XCSI_NEXTREG_OFFSET;
> > > +	}
> > > +
> > > +	/* Virtual Channel Image Information */
> > > +	dev_info(core->dev, "********** Virtual Channel Info
> > ************\n");
> > > +	dev_info(core->dev, "VC\tLine Count\tByte Count\tData Type\n");
> > > +	if (core->en_vcx)
> > > +		max_vc = XCSI_MAX_VCX;
> > > +	else
> > > +		max_vc = XCSI_MAX_VC;
> > > +
> > > +	reg = XCSI_VCXINF1R_OFFSET;
> > > +	for (i = 0; i < max_vc; i++) {
> > > +		u32 line_count, byte_count, data_type;
> > > +
> > > +		/* Get line and byte count from VCXINFR1 Register */
> > > +		data = xcsi2rxss_read(core, reg);
> > > +		byte_count = data & XCSI_VCXINF1R_BYTECOUNT;
> > > +		line_count = data & XCSI_VCXINF1R_LINECOUNT;
> > > +		line_count >>= XCSI_VCXINF1R_LINECOUNT_SHIFT;
> > > +
> > > +		/* Get data type from VCXINFR2 Register */
> > > +		reg += XCSI_NEXTREG_OFFSET;
> > > +		data = xcsi2rxss_read(core, reg);
> > > +		data_type = data & XCSI_VCXINF2R_DT;
> > > +
> > > +		dev_info(core->dev, "%d\t%d\t\t%d\t\t0x%x\n", i, line_count,
> > > +			 byte_count, data_type);
> > > +
> > > +		/* Move to next pair of VC Info registers */
> > > +		reg += XCSI_NEXTREG_OFFSET;
> > > +	}
> > > +
> > > +	mutex_unlock(&xcsi2rxss->lock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * xcsi2rxss_subscribe_event - Subscribe to the custom short packet
> > > + * receive event.
> > > + * @sd: V4L2 Sub device
> > > + * @fh: V4L2 File Handle
> > > + * @sub: Subcribe event structure
> > > + *
> > > + * There are two types of events to be subscribed.
> > > + *
> > > + * First is to register for receiving a generic short packet.
> > > + * The generic short packets received are queued up in a FIFO.
> > > + * On reception of a generic short packet, an event will be generated
> > > + * with the short packet contents copied to its data area.
> > > + * Application subscribed to this event will poll for POLLPRI.
> > > + * On getting the event, the app dequeues the event to get the short packet
> > > + * data.
> > > + *
> > > + * Second is to register for Short packet FIFO overflow
> > > + * In case the rate of receiving short packets is high and
> > > + * the short packet FIFO overflows, this event will be triggered.
> > > + *
> > > + * Return: 0 on success, errors otherwise
> > > + */
> > > +static int xcsi2rxss_subscribe_event(struct v4l2_subdev *sd,
> > > +				     struct v4l2_fh *fh,
> > > +				     struct v4l2_event_subscription *sub)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > > +	int ret;
> > > +
> > > +	mutex_lock(&xcsi2rxss->lock);
> > > +
> > > +	switch (sub->type) {
> > > +	case V4L2_EVENT_XLNXCSIRX_SPKT:
> > > +	case V4L2_EVENT_XLNXCSIRX_SPKT_OVF:
> > > +	case V4L2_EVENT_XLNXCSIRX_SLBF:
> > > +		ret = v4l2_event_subscribe(fh, sub, XCSI_MAX_SPKT_EVENT,
> > NULL);
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +	}
> > > +
> > > +	mutex_unlock(&xcsi2rxss->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_unsubscribe_event - Unsubscribe from all events registered
> > > + * @sd: V4L2 Sub device
> > > + * @fh: V4L2 file handle
> > > + * @sub: pointer to Event unsubscription structure
> > > + *
> > > + * Return: zero on success, else a negative error code.
> > > + */
> > > +static int xcsi2rxss_unsubscribe_event(struct v4l2_subdev *sd,
> > > +				       struct v4l2_fh *fh,
> > > +				       struct v4l2_event_subscription *sub)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > > +	int ret;
> > > +
> > > +	mutex_lock(&xcsi2rxss->lock);
> > > +	ret = v4l2_event_unsubscribe(fh, sub);
> > > +	mutex_unlock(&xcsi2rxss->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_s_ctrl - This is used to set the Xilinx MIPI CSI-2 V4L2 controls
> > > + * @ctrl: V4L2 control to be set
> > > + *
> > > + * This function is used to set the V4L2 controls for the Xilinx MIPI
> > > + * CSI-2 Rx Subsystem. It is used to set the active lanes in the system.
> > > + * The event counters can be reset.
> > > + *
> > > + * Return: 0 on success, errors otherwise
> > > + */
> > > +static int xcsi2rxss_s_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss =
> > > +		container_of(ctrl->handler, struct xcsi2rxss_state,
> > > +			     ctrl_handler);
> > > +	struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > > +	int ret = 0;
> > > +	u32 active_lanes = 1;
> > 
> > Nit. This can move into if statement below, without initializing it.
> > 
> Yes. Will do it in next version.
> 
> > > +
> > > +	mutex_lock(&xcsi2rxss->lock);
> > > +
> > > +	switch (ctrl->id) {
> > > +	case V4L2_CID_XILINX_MIPICSISS_ACT_LANES:
> > > +		/*
> > > +		 * This will be called only when "Enable Active Lanes" parameter
> > > +		 * is set in design
> > > +		 */
> > > +		if (core->enable_active_lanes) {
> > > +			xcsi2rxss_clr_and_set(core, XCSI_PCR_OFFSET,
> > > +					      XCSI_PCR_ACTLANES_MASK,
> > > +					      ctrl->val - 1);
> > > +			/*
> > > +			 * This delay is to allow the value to reflect as write
> > > +			 * and read paths are different.
> > > +			 */
> > > +			udelay(1);
> > 
> > Interesting this is required.
> 
> Yes I have mentioned the reason in the comments based on PG232.
> 
> > 
> > > +			active_lanes = xcsi2rxss_read(core, XCSI_PCR_OFFSET);
> > > +			active_lanes &= XCSI_PCR_ACTLANES_MASK;
> > > +			active_lanes++;
> > > +			if (active_lanes != ctrl->val)
> > > +				dev_info(core->dev, "RxByteClkHS absent\n");
> > > +			dev_dbg(core->dev, "active lanes = %d\n", ctrl->val);
> > > +		} else {
> > > +			ret = -EINVAL;
> > > +		}
> > > +		break;
> > > +	case V4L2_CID_XILINX_MIPICSISS_RESET_COUNTERS:
> > > +		xcsi2rxss_reset_event_counters(xcsi2rxss);
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	mutex_unlock(&xcsi2rxss->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_g_volatile_ctrl - get the Xilinx MIPI CSI-2 Rx controls
> > > + * @ctrl: Pointer to V4L2 control
> > > + *
> > > + * This is used to get the number of frames received by the Xilinx
> > > + * MIPI CSI-2 Rx.
> > > + *
> > > + * Return: 0 on success, errors otherwise
> > > + */
> > > +static int xcsi2rxss_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss =
> > > +		container_of(ctrl->handler, struct xcsi2rxss_state,
> > > +			     ctrl_handler);
> > > +	int ret = 0;
> > > +
> > > +	mutex_lock(&xcsi2rxss->lock);
> > > +
> > > +	switch (ctrl->id) {
> > > +	case V4L2_CID_XILINX_MIPICSISS_FRAME_COUNTER:
> > > +		ctrl->val = xcsi2rxss->core.events[0].counter;
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +	}
> > > +
> > > +	mutex_unlock(&xcsi2rxss->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state)
> > > +{
> > > +	struct xcsi2rxss_core *core = &state->core;
> > > +	int ret = 0;
> > > +
> > > +	xcsi2rxss_enable(core);
> > > +
> > > +	ret = xcsi2rxss_reset(core);
> > 
> > As the soft reset is done after enabling, I guess soft reset doesn't
> > reset those controls and registers which are set prior to it.
> > 
> 
> Correct. As mentioned in PG 232 page 21 soft reset bit description, 
> "All registers reset to their default value (except for this bit, Core Enable and Active lanes configuration).
> In addition to resetting registers when this bit is set to 1:
> • Shut down port is not asserted on the PPI lanes
> • Internal FIFOs (PPI, Packet, Generic Short Packet) are flushed
> • Control Finite State Machine (FSM) stops processing current packet. Any partially written packet to
> memory is marked as errored. This packet, when made available through the AXI4-Stream interface,
> reports the error on TUSER[1]."
> 

Thanks for sharing details. Wouldn't it be better to reset in disable then?

Thanks,
-hyun


> 
> > > +	if (ret < 0) {
> > > +		state->streaming = false;
> > > +		return ret;
> > > +	}
> > > +
> > > +	xcsi2rxss_intr_enable(core);
> > > +	state->streaming = true;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void xcsi2rxss_stop_stream(struct xcsi2rxss_state *state)
> > > +{
> > > +	struct xcsi2rxss_core *core = &state->core;
> > > +
> > > +	xcsi2rxss_intr_disable(core);
> > > +	xcsi2rxss_disable(core);
> > > +	state->streaming = false;
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_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 MIPI CSI-2 Rx Subsystem.
> > > + *
> > > + * Return: 0 on success, errors otherwise
> > > + */
> > > +static int xcsi2rxss_s_stream(struct v4l2_subdev *sd, int enable)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > > +	int ret = 0;
> > > +
> > > +	mutex_lock(&xcsi2rxss->lock);
> > > +
> > > +	if (enable) {
> > > +		if (!xcsi2rxss->streaming) {
> > > +			/* reset the event counters */
> > > +			xcsi2rxss_reset_event_counters(xcsi2rxss);
> > 
> > Is this required here? or can it be done explicitly by the application
> > separately?
> > 
> 
> Yes this can be done separately by the application using the reset counters v4l2 command.
> Reason to keep it here is to count the number of errors happening between enabling and disabling a stream.
> 
> > > +			ret = xcsi2rxss_start_stream(xcsi2rxss);
> > > +		}
> > > +	} else {
> > > +		if (xcsi2rxss->streaming)
> > > +			xcsi2rxss_stop_stream(xcsi2rxss);
> > > +	}
> > > +
> > > +	mutex_unlock(&xcsi2rxss->lock);
> > > +	return ret;
> > > +}
> > > +
> > > +static struct v4l2_mbus_framefmt *
> > > +__xcsi2rxss_get_pad_format(struct xcsi2rxss_state *xcsi2rxss,
> > > +			   struct v4l2_subdev_pad_config *cfg,
> > > +			   unsigned int pad, u32 which)
> > > +{
> > > +	switch (which) {
> > > +	case V4L2_SUBDEV_FORMAT_TRY:
> > > +		return v4l2_subdev_get_try_format(&xcsi2rxss->subdev, cfg,
> > > +						  pad);
> > > +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > > +		return &xcsi2rxss->format;
> > > +	default:
> > > +		return NULL;
> > > +	}
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_get_format - Get 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 get the pad format information.
> > > + *
> > > + * Return: 0 on success
> > > + */
> > > +static int xcsi2rxss_get_format(struct v4l2_subdev *sd,
> > > +				struct v4l2_subdev_pad_config *cfg,
> > > +				struct v4l2_subdev_format *fmt)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > > +
> > > +	mutex_lock(&xcsi2rxss->lock);
> > > +	fmt->format = *__xcsi2rxss_get_pad_format(xcsi2rxss, cfg, fmt->pad,
> > > +						  fmt->which);
> > > +	mutex_unlock(&xcsi2rxss->lock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_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. So when a format set is
> > > + * requested by application, all parameters except the format type is saved
> > > + * for the pad and the original pad format is sent back to the application.
> > > + *
> > > + * Return: 0 on success
> > > + */
> > > +static int xcsi2rxss_set_format(struct v4l2_subdev *sd,
> > > +				struct v4l2_subdev_pad_config *cfg,
> > > +				struct v4l2_subdev_format *fmt)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > > +	struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > > +	struct v4l2_mbus_framefmt *__format;
> > > +	u32 code;
> > > +
> > > +	/* only sink pad format can be updated */
> > > +	mutex_lock(&xcsi2rxss->lock);
> > > +
> > > +	/*
> > > +	 * Only the format->code parameter matters for CSI as the
> > > +	 * CSI format cannot be changed at runtime.
> > > +	 * Ensure that format to set is copied to over to CSI pad format
> > > +	 */
> > > +	__format = __xcsi2rxss_get_pad_format(xcsi2rxss, cfg,
> > > +					      fmt->pad, fmt->which);
> > > +
> > > +	/* Save the pad format code */
> > > +	code = __format->code;
> > > +
> > > +	/*
> > > +	 * RAW8 is supported in all datatypes. So if requested media bus format
> > > +	 * is of RAW8 type, then allow to be set. In case core is configured to
> > > +	 * other RAW, YUV422 8/10 or RGB888, set appropriate media bus
> > format.
> > > +	 */
> > > +	if ((fmt->format.code == MEDIA_BUS_FMT_SBGGR8_1X8 ||
> > > +	     fmt->format.code == MEDIA_BUS_FMT_SGBRG8_1X8 ||
> > > +	     fmt->format.code == MEDIA_BUS_FMT_SGRBG8_1X8 ||
> > > +	     fmt->format.code == MEDIA_BUS_FMT_SRGGB8_1X8) ||
> > > +	    (core->datatype == XCSI_DT_RAW10 &&
> > > +	     (fmt->format.code == MEDIA_BUS_FMT_SBGGR10_1X10 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SGBRG10_1X10 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SRGGB10_1X10)) ||
> > > +	    (core->datatype == XCSI_DT_RAW12 &&
> > > +	     (fmt->format.code == MEDIA_BUS_FMT_SBGGR12_1X12 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SGBRG12_1X12 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SGRBG12_1X12 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SRGGB12_1X12)) ||
> > > +	    (core->datatype == XCSI_DT_RAW14 &&
> > > +	     (fmt->format.code == MEDIA_BUS_FMT_SBGGR14_1X14 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SGBRG14_1X14 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SGRBG14_1X14 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SRGGB14_1X14)) ||
> > > +	    (core->datatype == XCSI_DT_RAW16 &&
> > > +	     (fmt->format.code == MEDIA_BUS_FMT_SBGGR16_1X16 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SGBRG16_1X16 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SGRBG16_1X16 ||
> > > +	      fmt->format.code == MEDIA_BUS_FMT_SRGGB16_1X16)) ||
> > > +	    (core->datatype == XCSI_DT_YUV4228B &&
> > > +	     fmt->format.code == MEDIA_BUS_FMT_UYVY8_1X16) ||
> > > +	    (core->datatype == XCSI_DT_YUV42210B &&
> > > +	     fmt->format.code == MEDIA_BUS_FMT_UYVY8_1X16) ||
> > 
> > Not sure if single bus format can be shared between 422 8B and 422 10B.
> 
> You are right. I missed it. I will remove it in next version.
> 
> > 
> > > +	    (core->datatype == XCSI_DT_RGB888 &&
> > > +	     fmt->format.code == MEDIA_BUS_FMT_RBG888_1X24)) {
> > > +		/* Copy over the format to be set */
> > > +		*__format = fmt->format;
> > > +	} else {
> > > +		/* Restore the original pad format code */
> > > +		fmt->format.code = code;
> > > +		__format->code = code;
> > > +		__format->width = fmt->format.width;
> > > +		__format->height = fmt->format.height;
> > > +	}
> > > +
> > > +	mutex_unlock(&xcsi2rxss->lock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * xcsi2rxss_open - Called on v4l2_open()
> > > + * @sd: Pointer to V4L2 sub device structure
> > > + * @fh: Pointer to V4L2 File handle
> > > + *
> > > + * This function is called on v4l2_open(). It sets the default format
> > > + * for both pads.
> > > + *
> > > + * Return: 0 on success
> > > + */
> > > +static int xcsi2rxss_open(struct v4l2_subdev *sd,
> > > +			  struct v4l2_subdev_fh *fh)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > > +	struct v4l2_mbus_framefmt *format;
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i < XCSI_MEDIA_PADS; i++) {
> > > +		format = v4l2_subdev_get_try_format(sd, fh->pad, i);
> > > +		*format = xcsi2rxss->format;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/* -----------------------------------------------------------------------------
> > > + * Media Operations
> > > + */
> > > +
> > > +static const struct media_entity_operations xcsi2rxss_media_ops = {
> > > +	.link_validate = v4l2_subdev_link_validate
> > > +};
> > > +
> > > +static const struct v4l2_ctrl_ops xcsi2rxss_ctrl_ops = {
> > > +	.g_volatile_ctrl = xcsi2rxss_g_volatile_ctrl,
> > > +	.s_ctrl	= xcsi2rxss_s_ctrl
> > > +};
> > > +
> > > +static struct v4l2_ctrl_config xcsi2rxss_ctrls[] = {
> > > +	{
> > > +		.ops	= &xcsi2rxss_ctrl_ops,
> > > +		.id	= V4L2_CID_XILINX_MIPICSISS_ACT_LANES,
> > > +		.name	= "Active Lanes",
> > > +		.type	= V4L2_CTRL_TYPE_INTEGER,
> > > +		.min	= 1,
> > > +		.max	= 4,
> > > +		.step	= 1,
> > > +		.def	= 1,
> > > +	}, {
> > > +		.ops	= &xcsi2rxss_ctrl_ops,
> > > +		.id	= V4L2_CID_XILINX_MIPICSISS_FRAME_COUNTER,
> > > +		.name	= "Frames Received Counter",
> > > +		.type	= V4L2_CTRL_TYPE_INTEGER,
> > > +		.min	= 0,
> > > +		.max	= 0xFFFFFFFF,
> > > +		.step	= 1,
> > > +		.def	= 0,
> > > +		.flags	= V4L2_CTRL_FLAG_VOLATILE |
> > V4L2_CTRL_FLAG_READ_ONLY,
> > > +	}, {
> > > +		.ops	= &xcsi2rxss_ctrl_ops,
> > > +		.id	= V4L2_CID_XILINX_MIPICSISS_RESET_COUNTERS,
> > > +		.name	= "Reset Counters",
> > > +		.type	= V4L2_CTRL_TYPE_BUTTON,
> > > +		.min	= 0,
> > > +		.max	= 1,
> > > +		.step	= 1,
> > > +		.def	= 0,
> > > +		.flags	= V4L2_CTRL_FLAG_WRITE_ONLY,
> > > +	}
> > > +};
> > > +
> > > +static const struct v4l2_subdev_core_ops xcsi2rxss_core_ops = {
> > > +	.log_status = xcsi2rxss_log_status,
> > > +	.subscribe_event = xcsi2rxss_subscribe_event,
> > > +	.unsubscribe_event = xcsi2rxss_unsubscribe_event
> > > +};
> > > +
> > > +static struct v4l2_subdev_video_ops xcsi2rxss_video_ops = {
> > > +	.s_stream = xcsi2rxss_s_stream
> > > +};
> > > +
> > > +static struct v4l2_subdev_pad_ops xcsi2rxss_pad_ops = {
> > > +	.get_fmt = xcsi2rxss_get_format,
> > > +	.set_fmt = xcsi2rxss_set_format,
> > > +};
> > > +
> > > +static struct v4l2_subdev_ops xcsi2rxss_ops = {
> > > +	.core = &xcsi2rxss_core_ops,
> > > +	.video = &xcsi2rxss_video_ops,
> > > +	.pad = &xcsi2rxss_pad_ops
> > > +};
> > > +
> > > +static const struct v4l2_subdev_internal_ops xcsi2rxss_internal_ops = {
> > > +	.open = xcsi2rxss_open,
> > > +};
> > > +
> > > +static void xcsi2rxss_set_default_format(struct xcsi2rxss_state *state)
> > > +{
> > > +	struct xcsi2rxss_core *core = &state->core;
> > > +
> > > +	memset(&state->format, 0, sizeof(state->format));
> > > +
> > > +	switch (core->datatype) {
> > > +	case XCSI_DT_YUV4228B:
> > > +	case XCSI_DT_YUV42210B:
> > 
> > As mentioned before, does this belong here? It looks like 10bit format,
> > but is classified as 8bit bus format.
> > 
> 
> I will remove this 10bit format. Xilinx YUV422 10bpc on AXI4 Stream isn't represented by any of the MEDIA_BUS_FMT_*. 
> 
> > > +		state->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
> > > +		break;
> > > +	case XCSI_DT_RGB888:
> > > +		state->format.code = MEDIA_BUS_FMT_RBG888_1X24;
> > > +		break;
> > > +	case XCSI_DT_RAW10:
> > > +		switch (core->bayer) {
> > > +		case XCSI_BAYER_BGGR:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SBGGR10_1X10;
> > > +			break;
> > > +		case XCSI_BAYER_GRBG:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SGRBG10_1X10;
> > > +			break;
> > > +		case XCSI_BAYER_GBRG:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SGBRG10_1X10;
> > > +			break;
> > > +		case XCSI_BAYER_RGGB:
> > > +		default:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SRGGB10_1X10;
> > > +		}
> > > +		break;
> > > +	case XCSI_DT_RAW12:
> > > +		switch (core->bayer) {
> > > +		case XCSI_BAYER_BGGR:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SBGGR12_1X12;
> > > +			break;
> > > +		case XCSI_BAYER_GRBG:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SGRBG12_1X12;
> > > +			break;
> > > +		case XCSI_BAYER_GBRG:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SGBRG12_1X12;
> > > +			break;
> > > +		case XCSI_BAYER_RGGB:
> > > +		default:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SRGGB12_1X12;
> > > +		}
> > > +		break;
> > > +	case XCSI_DT_RAW14:
> > > +		switch (core->bayer) {
> > > +		case XCSI_BAYER_BGGR:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SBGGR14_1X14;
> > > +			break;
> > > +		case XCSI_BAYER_GRBG:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SGRBG14_1X14;
> > > +			break;
> > > +		case XCSI_BAYER_GBRG:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SGBRG14_1X14;
> > > +			break;
> > > +		case XCSI_BAYER_RGGB:
> > > +		default:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SRGGB14_1X14;
> > > +		}
> > > +		break;
> > > +	case XCSI_DT_RAW16:
> > > +		switch (core->bayer) {
> > > +		case XCSI_BAYER_BGGR:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SBGGR16_1X16;
> > > +			break;
> > > +		case XCSI_BAYER_GRBG:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SGRBG16_1X16;
> > > +			break;
> > > +		case XCSI_BAYER_GBRG:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SGBRG16_1X16;
> > > +			break;
> > > +		case XCSI_BAYER_RGGB:
> > > +		default:
> > > +			state->format.code =
> > MEDIA_BUS_FMT_SRGGB16_1X16;
> > > +		}
> > > +		break;
> > > +	case XCSI_DT_RAW8:
> > > +	case XCSI_DT_RGB444:
> > > +	case XCSI_DT_RGB555:
> > > +	case XCSI_DT_RGB565:
> > > +	case XCSI_DT_RGB666:
> > > +		switch (core->bayer) {
> > > +		case XCSI_BAYER_BGGR:
> > > +			state->format.code = MEDIA_BUS_FMT_SBGGR8_1X8;
> > > +			break;
> > > +		case XCSI_BAYER_GRBG:
> > > +			state->format.code = MEDIA_BUS_FMT_SGRBG8_1X8;
> > > +			break;
> > > +		case XCSI_BAYER_GBRG:
> > > +			state->format.code = MEDIA_BUS_FMT_SGBRG8_1X8;
> > > +			break;
> > > +		case XCSI_BAYER_RGGB:
> > > +		default:
> > > +			state->format.code = MEDIA_BUS_FMT_SRGGB8_1X8;
> > > +		}
> > > +		break;
> > > +	}
> > > +
> > > +	state->format.field = V4L2_FIELD_NONE;
> > > +	state->format.colorspace = V4L2_COLORSPACE_SRGB;
> > > +	state->format.width = XCSI_DEFAULT_WIDTH;
> > > +	state->format.height = XCSI_DEFAULT_HEIGHT;
> > > +
> > > +	dev_dbg(core->dev, "default mediabus format = 0x%x",
> > > +		state->format.code);
> > > +}
> > > +
> > > +static int xcsi2rxss_clk_get(struct xcsi2rxss_core *core)
> > > +{
> > > +	int ret;
> > > +
> > > +	core->lite_aclk = devm_clk_get(core->dev, "lite_aclk");
> > > +	if (IS_ERR(core->lite_aclk)) {
> > > +		ret = PTR_ERR(core->lite_aclk);
> > > +		dev_err(core->dev, "failed to get lite_aclk (%d)\n",
> > > +			ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	core->video_aclk = devm_clk_get(core->dev, "video_aclk");
> > > +	if (IS_ERR(core->video_aclk)) {
> > > +		ret = PTR_ERR(core->video_aclk);
> > > +		dev_err(core->dev, "failed to get video_aclk (%d)\n",
> > > +			ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	core->dphy_clk_200M = devm_clk_get(core->dev, "dphy_clk_200M");
> > > +	if (IS_ERR(core->dphy_clk_200M)) {
> > > +		ret = PTR_ERR(core->dphy_clk_200M);
> > > +		dev_err(core->dev, "failed to get dphy_clk_200M (%d)\n",
> > > +			ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	dev_dbg(core->dev, "Got all clocks!\n");
> > 
> > This debug print doesn't add any, but it's fine if you want to keep.
> 
> I will remove in next version. It is more of trace print than debug.
> 
> > 
> > > +	return 0;
> > > +}
> > > +
> > > +static int xcsi2rxss_clk_enable(struct xcsi2rxss_core *core)
> > > +{
> > > +	unsigned long rate;
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(core->lite_aclk);
> > > +	if (ret) {
> > > +		dev_err(core->dev, "failed enabling lite_aclk (%d)\n",
> > > +			ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(core->video_aclk);
> > > +	if (ret) {
> > > +		dev_err(core->dev, "failed enabling video_aclk (%d)\n",
> > > +			ret);
> > > +		goto err_vidclk;
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(core->dphy_clk_200M);
> > > +	if (ret) {
> > > +		dev_err(core->dev, "failed enabling dphy clk (%d)\n",
> > > +			ret);
> > > +		goto err_dphyclk;
> > > +	}
> > > +
> > > +	ret = clk_set_rate(core->dphy_clk_200M, XCSI_DPHY_CLK_REQ);
> > > +	if (ret) {
> > > +		dev_err(core->dev, "failed to set dphy clk rate (%d)\n",
> > > +			ret);
> > > +		goto err_rate_set;
> > > +	}
> > > +
> > > +	rate = clk_get_rate(core->dphy_clk_200M);
> > > +	if (rate < XCSI_DPHY_CLK_MIN && rate > XCSI_DPHY_CLK_MAX) {
> > 
> > Please confirm on this range. From its name and so on, it looks like
> > it should be 200MHz.
> 
> This is range is confirmed from the RTL source code. The clock should be as close to 200MHz possible within given deviation.
> 
> > 
> > > +		dev_err(core->dev, "Err DPHY Clock = %lu\n",
> > > +			rate);
> > > +		ret = -EINVAL;
> > > +		goto err_rate_set;
> > > +	}
> > > +
> > > +	dev_dbg(core->dev, "all clocks enabled!\n");
> > > +
> > > +	return ret;
> > > +
> > > +err_rate_set:
> > > +	clk_disable_unprepare(core->dphy_clk_200M);
> > > +err_dphyclk:
> > > +	clk_disable_unprepare(core->video_aclk);
> > > +err_vidclk:
> > > +	clk_disable_unprepare(core->lite_aclk);
> > > +	return ret;
> > > +}
> > > +
> > > +static void xcsi2rxss_clk_disable(struct xcsi2rxss_core *core)
> > > +{
> > > +	clk_disable_unprepare(core->dphy_clk_200M);
> > > +	clk_disable_unprepare(core->video_aclk);
> > > +	clk_disable_unprepare(core->lite_aclk);
> > > +}
> > > +
> > > +static int xcsi2rxss_parse_of(struct xcsi2rxss_state *xcsi2rxss)
> > > +{
> > > +	struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > > +	struct device_node *node = xcsi2rxss->core.dev->of_node;
> > > +	struct device_node *ports = NULL;
> > > +	struct device_node *port = NULL;
> > > +	unsigned int nports;
> > > +	int ret;
> > > +
> > > +	core->en_csi_v20 = of_property_read_bool(node, "xlnx,en-csi-v2-0");
> > 
> > Can't this be a local variable?
> In its current usage, yes. I will modify this and resend in next version. 
> 
> > 
> > > +	if (core->en_csi_v20) {
> > > +		core->en_vcx = of_property_read_bool(node, "xlnx,en-vcx");
> > > +		dev_dbg(core->dev, "vcx %s", core->en_vcx ? "enabled" :
> > > +			"disabled");
> > > +	}
> > > +
> > > +	dev_dbg(core->dev, "en_csi_v20 %s", core->en_csi_v20 ? "enabled" :
> > > +		"disabled");
> > > +
> > > +	core->enable_active_lanes =
> > > +		of_property_read_bool(node, "xlnx,en-active-lanes");
> > > +	dev_dbg(core->dev, "Enable active lanes property = %s\n",
> > > +		core->enable_active_lanes ? "Present" : "Absent");
> > > +
> > > +	ret = of_property_read_u32(node, "xlnx,csi-pxl-format",
> > > +				   &core->datatype);
> > > +	if (ret < 0) {
> > > +		dev_err(core->dev, "missing xlnx,csi-pxl-format property\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = 0;
> > 
> > I think this line is not needed.
> 
> Ok. I will remove it in next version.
> 
> > 
> > > +	switch (core->datatype) {
> > > +	case XCSI_DT_YUV4228B:
> > > +	case XCSI_DT_RGB444:
> > > +	case XCSI_DT_RGB555:
> > > +	case XCSI_DT_RGB565:
> > > +	case XCSI_DT_RGB666:
> > > +	case XCSI_DT_RGB888:
> > > +	case XCSI_DT_RAW6:
> > > +	case XCSI_DT_RAW7:
> > > +	case XCSI_DT_RAW8:
> > > +	case XCSI_DT_RAW10:
> > > +	case XCSI_DT_RAW12:
> > > +	case XCSI_DT_RAW14:
> > > +		break;
> > > +	case XCSI_DT_YUV42210B:
> > > +	case XCSI_DT_RAW16:
> > > +	case XCSI_DT_RAW20:
> > > +		if (!core->en_csi_v20) {
> > > +			ret = -EINVAL;
> > > +			dev_dbg(core->dev, "enable csi v2 for this pixel
> > format");
> > > +		}
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +	}
> > > +
> > > +	if (ret < 0) {
> > > +		dev_err(core->dev, "invalid csi-pxl-format property!\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	dev_dbg(core->dev, "pixel format set as 0x%x\n", core->datatype);
> > > +
> > > +	core->vfb = of_property_read_bool(node, "xlnx,vfb");
> > 
> > Can't this be a local variable as well?
> 
> True. I will update it in next version.
> 
> > 
> > > +	dev_dbg(core->dev, "Video Format Bridge property = %s\n",
> > > +		core->vfb ? "Present" : "Absent");
> > > +
> > > +	if (!core->vfb) {
> > > +		dev_err(core->dev, "failed as VFB is disabled!\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ports = of_get_child_by_name(node, "ports");
> > > +	if (!ports)
> > > +		ports = node;
> > > +
> > > +	nports = 0;
> > > +	for_each_child_of_node(ports, port) {
> > > +		const char *pattern = "mono";
> > > +		int len = strlen(pattern);
> > > +		struct device_node *endpoint;
> > > +		struct v4l2_fwnode_endpoint v4lendpoint;
> > > +		int ret;
> > > +
> > > +		if (!port->name || of_node_cmp(port->name, "port"))
> > > +			continue;
> > > +
> > > +		endpoint = of_get_next_child(port, NULL);
> > > +		if (!endpoint) {
> > > +			dev_err(core->dev, "No port at\n");
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		/*
> > > +		 * since first port is sink port and it contains
> > > +		 * all info about data-lanes and cfa-pattern,
> > > +		 * don't parse second port but only check if exists
> > > +		 */
> > > +		if (nports == XVIP_PAD_SOURCE) {
> > > +			dev_dbg(core->dev, "no need to parse source port");
> > > +			nports++;
> > > +			of_node_put(endpoint);
> > > +			continue;
> > > +		}
> > > +
> > > +		core->bayer = XCSI_BAYER_RGGB;
> > > +		ret = of_property_read_string(port, "xlnx,cfa-pattern",
> > > +					      &pattern);
> > > +		if (ret == 0) {
> > 
> > Nit. !ret.
> > 
> 
> Will update in next version.
> 
> 
> > > +			if (!strncmp("grbg", pattern, len)) {
> > > +				core->bayer = XCSI_BAYER_GRBG;
> > > +			} else if (!strncmp("bggr", pattern, len)) {
> > > +				core->bayer = XCSI_BAYER_BGGR;
> > > +			} else if (!strncmp("rggb", pattern, len)) {
> > > +				core->bayer = XCSI_BAYER_RGGB;
> > > +			} else if (!strncmp("gbrg", pattern, len)) {
> > > +				core->bayer = XCSI_BAYER_GBRG;
> > > +			} else {
> > > +				dev_err(core->dev, "incorrect cfa string\n");
> > > +				return -EINVAL;
> > > +			}
> > > +		} else if (ret != -EINVAL) {
> > 
> > What about for other errors?
> 
> There are only 3 types of errors. -EINVAL means that property doesn't exist.
> This is ok xlnx,cfa-pattern is not a required property.
> In case it exists, then the value read should be one of those strings mentioned above.
> Otherwise ret val either means that there was NULL string value for the property or 
> The string was not a proper null terminated string both of which mean a default value will be set on core->bayer.
> 
> > 
> > > +			dev_err(core->dev, "invalid cfa pattern!\n");
> > > +			return ret;
> > > +		}
> > > +		dev_dbg(core->dev, "bayer pattern = %d\n", core->bayer);
> > > +
> > > +		ret =
> > v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint),
> > > +						 &v4lendpoint);
> > > +		of_node_put(endpoint);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		dev_dbg(core->dev, "%s : port %d bus type = %d\n",
> > > +			__func__, nports, v4lendpoint.bus_type);
> > > +
> > > +		if (v4lendpoint.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > > +			dev_dbg(core->dev, "%s : base.port = %d base.id =
> > %d\n",
> > > +				__func__, v4lendpoint.base.port,
> > > +				v4lendpoint.base.id);
> > > +
> > > +			dev_dbg(core->dev, "%s : mipi number lanes = %d\n",
> > > +				__func__,
> > > +				v4lendpoint.bus.mipi_csi2.num_data_lanes);
> > > +
> > > +			core->max_num_lanes =
> > > +				v4lendpoint.bus.mipi_csi2.num_data_lanes;
> > > +		} else {
> > > +			dev_dbg(core->dev, "%s : Not a CSI2 bus\n", __func__);
> > 
> > Not sure if this adds any info.
> 
> Not really required. I will convert this to a comment in next revision.
> 
> > 
> > > +		}
> > > +
> > > +		/* Count the number of ports. */
> > > +		nports++;
> > > +	}
> > > +
> > > +	dev_dbg(core->dev, "max lanes = %d", core->max_num_lanes);
> > > +
> > > +	if (nports != XCSI_MEDIA_PADS) {
> > > +		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, xcsi2rxss_irq_handler,
> > > +			       IRQF_SHARED, "xilinx-csi2rxss", xcsi2rxss);
> > > +	if (ret) {
> > > +		dev_err(core->dev, "Err = %d Interrupt handler reg failed!\n",
> > > +			ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int xcsi2rxss_probe(struct platform_device *pdev)
> > > +{
> > > +	struct v4l2_subdev *subdev;
> > > +	struct xcsi2rxss_state *xcsi2rxss;
> > > +	struct xcsi2rxss_core *core;
> > > +	struct resource *res;
> > > +	int ret, num_ctrls, i;
> > > +
> > > +	xcsi2rxss = devm_kzalloc(&pdev->dev, sizeof(*xcsi2rxss), GFP_KERNEL);
> > > +	if (!xcsi2rxss)
> > > +		return -ENOMEM;
> > > +
> > > +	core = &xcsi2rxss->core;
> > > +	core->dev = &pdev->dev;
> > > +
> > > +	mutex_init(&xcsi2rxss->lock);
> > > +
> > > +	ret = xcsi2rxss_parse_of(xcsi2rxss);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	core->iomem = devm_ioremap_resource(core->dev, res);
> > > +	if (IS_ERR(core->iomem))
> > > +		return PTR_ERR(core->iomem);
> > > +
> > > +	ret = xcsi2rxss_clk_get(core);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = xcsi2rxss_clk_enable(core);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	xcsi2rxss_reset(core);
> > > +
> > > +	core->events = (struct xcsi2rxss_event *)&xcsi2rxss_events;
> > > +
> > > +	if (core->en_vcx) {
> > > +		u32 alloc_size;
> > > +
> > > +		alloc_size = sizeof(struct xcsi2rxss_event) *
> > > +			     XCSI_VCX_NUM_EVENTS;
> > > +		core->vcx_events = devm_kzalloc(&pdev->dev, alloc_size,
> > > +						GFP_KERNEL);
> > > +		if (!core->vcx_events) {
> > > +			mutex_destroy(&xcsi2rxss->lock);
> > > +			ret = -ENOMEM;
> > > +			goto all_clk_err;
> > > +		}
> > > +
> > > +		for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++)
> > > +			core->vcx_events[i].mask = 1 << i;
> > > +	}
> > > +
> > > +	/* Initialize V4L2 subdevice and media entity */
> > > +	xcsi2rxss->pads[XVIP_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > > +	xcsi2rxss->pads[XVIP_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> > > +
> > > +	/* Initialize the default format */
> > > +	xcsi2rxss_set_default_format(xcsi2rxss);
> > > +
> > > +	/* Initialize V4L2 subdevice and media entity */
> > > +	subdev = &xcsi2rxss->subdev;
> > > +	v4l2_subdev_init(subdev, &xcsi2rxss_ops);
> > > +	subdev->dev = &pdev->dev;
> > > +	subdev->internal_ops = &xcsi2rxss_internal_ops;
> > > +	strlcpy(subdev->name, dev_name(&pdev->dev), sizeof(subdev->name));
> > > +	subdev->flags |= V4L2_SUBDEV_FL_HAS_EVENTS |
> > V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +	subdev->entity.ops = &xcsi2rxss_media_ops;
> > > +	v4l2_set_subdevdata(subdev, xcsi2rxss);
> > > +
> > > +	ret = media_entity_pads_init(&subdev->entity, XCSI_MEDIA_PADS,
> > > +				     xcsi2rxss->pads);
> > > +	if (ret < 0)
> > > +		goto error;
> > > +
> > > +	/*
> > > +	 * In case the Enable Active Lanes config parameter is not set,
> > > +	 * dynamic lane reconfiguration is not allowed.
> > > +	 * So V4L2_CID_XILINX_MIPICSISS_ACT_LANES ctrl will not be
> > registered.
> > > +	 * Accordingly allocate the number of controls
> > > +	 */
> > > +	num_ctrls = ARRAY_SIZE(xcsi2rxss_ctrls);
> > > +
> > > +	if (!core->enable_active_lanes)
> > > +		num_ctrls--;
> > > +
> > > +	dev_dbg(core->dev, "# of ctrls = %d\n", num_ctrls);
> > > +
> > > +	v4l2_ctrl_handler_init(&xcsi2rxss->ctrl_handler, num_ctrls);
> > > +	for (i = 0; i < ARRAY_SIZE(xcsi2rxss_ctrls); i++) {
> > > +		struct v4l2_ctrl *ctrl;
> > > +
> > > +		if (xcsi2rxss_ctrls[i].id ==
> > > +			V4L2_CID_XILINX_MIPICSISS_ACT_LANES) {
> > > +			if (!core->enable_active_lanes) {
> > > +				/* Don't register control */
> > > +				dev_dbg(core->dev,
> > > +					"Skip active lane control\n");
> > > +				continue;
> > 
> > This active lane control is not even registered, so the control handler
> > doesn't even have to check this flag. But I guess it doesn't hurt to
> > keep the check in the handler, so I let you decide.
> > 
> 
> It wasn't there in the first patch version. But based on suggestion added this, I think it would be good to keep the check.
> 
> > > +			}
> > > +			xcsi2rxss_ctrls[i].max = core->max_num_lanes;
> > > +			xcsi2rxss_ctrls[i].def = core->max_num_lanes;
> > > +		}
> > > +
> > > +		dev_dbg(core->dev, "%d ctrl = 0x%x\n", i,
> > > +			xcsi2rxss_ctrls[i].id);
> > > +		ctrl = v4l2_ctrl_new_custom(&xcsi2rxss->ctrl_handler,
> > > +					    &xcsi2rxss_ctrls[i], NULL);
> > > +		if (!ctrl) {
> > > +			dev_err(core->dev, "Failed for %s ctrl\n",
> > > +				xcsi2rxss_ctrls[i].name);
> > > +			goto error;
> > > +		}
> > > +	}
> > > +
> > > +	dev_dbg(core->dev, "# v4l2 ctrls registered = %d\n", i - 1);
> > > +
> > > +	if (xcsi2rxss->ctrl_handler.error) {
> > > +		dev_err(core->dev, "failed to add controls\n");
> > > +		ret = xcsi2rxss->ctrl_handler.error;
> > > +		goto error;
> > > +	}
> > > +
> > > +	subdev->ctrl_handler = &xcsi2rxss->ctrl_handler;
> > > +	ret = v4l2_ctrl_handler_setup(&xcsi2rxss->ctrl_handler);
> > > +	if (ret < 0) {
> > > +		dev_err(core->dev, "failed to set controls\n");
> > > +		goto error;
> > > +	}
> > > +
> > > +	platform_set_drvdata(pdev, xcsi2rxss);
> > > +
> > > +	ret = v4l2_async_register_subdev(subdev);
> > > +	if (ret < 0) {
> > > +		dev_err(core->dev, "failed to register subdev\n");
> > > +		goto error;
> > > +	}
> > > +
> > > +	dev_info(core->dev, "Xilinx CSI2 Rx Subsystem device found!\n");
> > > +	return 0;
> > 
> > Just a nit. I'd add en empty line.
> 
> Ok. Will add empty line here.
> 
> > 
> > > +error:
> > > +	v4l2_ctrl_handler_free(&xcsi2rxss->ctrl_handler);
> > > +	media_entity_cleanup(&subdev->entity);
> > > +	mutex_destroy(&xcsi2rxss->lock);
> > > +all_clk_err:
> > > +	xcsi2rxss_clk_disable(core);
> > > +	return ret;
> > > +}
> > > +
> > > +static int xcsi2rxss_remove(struct platform_device *pdev)
> > > +{
> > > +	struct xcsi2rxss_state *xcsi2rxss = platform_get_drvdata(pdev);
> > > +	struct xcsi2rxss_core *core = &xcsi2rxss->core;
> > > +	struct v4l2_subdev *subdev = &xcsi2rxss->subdev;
> > > +
> > > +	v4l2_async_unregister_subdev(subdev);
> > > +	v4l2_ctrl_handler_free(&xcsi2rxss->ctrl_handler);
> > > +	media_entity_cleanup(&subdev->entity);
> > > +	mutex_destroy(&xcsi2rxss->lock);
> > > +	xcsi2rxss_clk_disable(core);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver xcsi2rxss_driver = {
> > > +	.driver = {
> > > +		.name		= "xilinx-csi2rxss",
> > > +		.of_match_table	= xcsi2rxss_of_id_table,
> > > +	},
> > > +	.probe			= xcsi2rxss_probe,
> > > +	.remove			= xcsi2rxss_remove,
> > > +};
> > > +
> > > +module_platform_driver(xcsi2rxss_driver);
> > > +
> > > +MODULE_AUTHOR("Vishal Sagar <vsagar@xilinx.com>");
> > > +MODULE_DESCRIPTION("Xilinx MIPI CSI2 Rx Subsystem Driver");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/include/uapi/linux/xilinx-v4l2-controls.h
> > b/include/uapi/linux/xilinx-v4l2-controls.h
> > > index b6441fe..4ca3b44 100644
> > > --- a/include/uapi/linux/xilinx-v4l2-controls.h
> > > +++ b/include/uapi/linux/xilinx-v4l2-controls.h
> > > @@ -71,4 +71,18 @@
> > >  /* Noise level */
> > >  #define V4L2_CID_XILINX_TPG_NOISE_GAIN
> > 	(V4L2_CID_XILINX_TPG + 17)
> > >
> > > +/*
> > > + * Xilinx MIPI CSI2 Rx Subsystem
> > > + */
> > > +
> > > +/* Base ID */
> > > +#define V4L2_CID_XILINX_MIPICSISS		(V4L2_CID_USER_BASE +
> > 0xc080)
> > > +
> > > +/* Active Lanes */
> > > +#define V4L2_CID_XILINX_MIPICSISS_ACT_LANES
> > 	(V4L2_CID_XILINX_MIPICSISS + 1)
> > > +/* Frames received since streaming is set */
> > > +#define V4L2_CID_XILINX_MIPICSISS_FRAME_COUNTER
> > 	(V4L2_CID_XILINX_MIPICSISS + 2)
> > > +/* Reset all event counters */
> > > +#define V4L2_CID_XILINX_MIPICSISS_RESET_COUNTERS
> > (V4L2_CID_XILINX_MIPICSISS + 3)
> > > +
> > >  #endif /* __UAPI_XILINX_V4L2_CONTROLS_H__ */
> > > diff --git a/include/uapi/linux/xilinx-v4l2-events.h b/include/uapi/linux/xilinx-
> > v4l2-events.h
> > > new file mode 100644
> > > index 0000000..2fef0b5
> > > --- /dev/null
> > > +++ b/include/uapi/linux/xilinx-v4l2-events.h
> > > @@ -0,0 +1,28 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Xilinx V4L2 Events
> > > + *
> > > + * Copyright (C) 2019 Xilinx, Inc.
> > > + *
> > > + * Contacts: Vishal Sagar <vishal.sagar@xilinx.com>
> > > + *
> > > + */
> > > +
> > > +#ifndef __UAPI_XILINX_V4L2_EVENTS_H__
> > > +#define __UAPI_XILINX_V4L2_EVENTS_H__
> > > +
> > > +#include <linux/videodev2.h>
> > > +
> > > +/*
> > > + * Events
> > > + *
> > > + * V4L2_EVENT_XLNXCSIRX_SPKT: Short packet received
> > > + * V4L2_EVENT_XLNXCSIRX_SPKT_OVF: Short packet FIFO overflow
> > > + * V4L2_EVENT_XLNXCSIRX_SLBF: Stream line buffer full
> > > + */
> > 
> > Would be better to comment inline. But up to you.
> 
> Ok. Will do it in next version
> 
> Regards
> Vishal Sagar
> 
> > 
> > Thanks,
> > -hyun
> > 
> > > +#define V4L2_EVENT_XLNXCSIRX_CLASS	(V4L2_EVENT_PRIVATE_START |
> > 0x100)
> > > +#define V4L2_EVENT_XLNXCSIRX_SPKT	(V4L2_EVENT_XLNXCSIRX_CLASS
> > | 0x1)
> > > +#define V4L2_EVENT_XLNXCSIRX_SPKT_OVF
> > 	(V4L2_EVENT_XLNXCSIRX_CLASS | 0x2)
> > > +#define V4L2_EVENT_XLNXCSIRX_SLBF	(V4L2_EVENT_XLNXCSIRX_CLASS
> > | 0x3)
> > > +
> > > +#endif /* __UAPI_XILINX_V4L2_EVENTS_H__ */
> > > --
> > > 2.7.4
> > >
> > >
>