Message-ID: <20190213194539.GA11819@smtp.xilinx.com>
Date:   Wed, 13 Feb 2019 11:45:42 -0800
From:   Hyun Kwon <hyun.kwon@...inx.com>
To:     Vishal Sagar <vishal.sagar@...inx.com>
CC:     <laurent.pinchart@...asonboard.com>, <mchehab@...nel.org>,
        <robh+dt@...nel.org>, <mark.rutland@....com>, <michals@...inx.com>,
        <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <sakari.ailus@...ux.intel.com>, <hans.verkuil@...co.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <dineshk@...inx.com>,
        <sandipk@...inx.com>
Subject: Re: [v3,2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem

Hi Vishal,

Thanks for the patch. Sorry for the delay.

On Fri, 2019-02-01 at 18:26:06 +0530, 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@...inx.com>
> ---
> v3
> - Fixed comments given by Hyun.
> - Removed DPHY 200 MHz clock. This will be controlled by DPHY driver
> - Minor code formatting
> - en_csi_v20 and vfb members removed from struct and made local to dt parsing

[snip]

> +};
> +
> +/*
> + * 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
> + * @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
> + * @lite_aclk: AXI4-Lite interface clock
> + * @video_aclk: Video clock
> + */
> +struct xcsi2rxss_core {
> +	struct device *dev;
> +	void __iomem *iomem;
> +	int irq;

This doesn't have to be stored.

> +	bool enable_active_lanes;
> +	u32 max_num_lanes;
> +	u32 datatype;
> +	u32 bayer;
> +	struct xcsi2rxss_event *events;
> +	struct xcsi2rxss_event *vcx_events;
> +	bool en_vcx;
> +	struct clk *lite_aclk;
> +	struct clk *video_aclk;
> +};
> +
> +/**
> + * struct xcsi2rxss_state - CSI2 Rx Subsystem device structure
> + * @core: Core structure for MIPI CSI2 Rx Subsystem
> + * @subdev: The v4l2 subdev structure

[snip]

> +
> +/* 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)

Does checkpatch warn if putting {} here, and

> +			dev_info(core->dev, "%s events: %d\n",
> +				 core->events[i].name,
> +				 core->events[i].counter);
> +	}
> +
> +	if (core->en_vcx)

here, and

> +		for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> +			if (core->vcx_events[i].counter > 0)

here? But up to you.

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

[snip]

> +
> +/**
> + * 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;
> +
> +	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) {
> +			u32 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);
> +			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:

-EINVAL?

> +		break;
> +	}
> +
> +	mutex_unlock(&xcsi2rxss->lock);
> +
> +	return ret;
> +}
> +

[snip]

> +
> +static void xcsi2rxss_clk_disable(struct xcsi2rxss_core *core)
> +{
> +	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;
> +	bool en_csi_v20, vfb;
> +	int ret;
> +
> +	en_csi_v20 = of_property_read_bool(node, "xlnx,en-csi-v2-0");
> +	if (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", en_csi_v20 ? "enabled" :
> +		"disabled");

Would it be better to have these options visible with log?
It can be printed as a part of final banner in probe. Up to you.

> +
> +	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;
> +	}
> +
> +	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 (!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);
> +
> +	vfb = of_property_read_bool(node, "xlnx,vfb");
> +	dev_dbg(core->dev, "Video Format Bridge property = %s\n",
> +		vfb ? "Present" : "Absent");

I'd collect all these print in one print or one location. But up to you.

> +
> +	if (!vfb) {
> +		dev_err(core->dev, "failed as VFB is disabled!\n");
> +		return -EINVAL;
> +	}

[snip]

> +	/* 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;

Not sure if I mentioned, please consider having these with
stream on / off later.

I only have minor comments, and it looks fine to me. Please take a look at
those comments, then

	Reviewed-by: Hyun Kwon <hyun.kwon@...inx.com>

Thanks,
-hyun

Powered by blists - more mailing lists